From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Sam Protsenko <semen.protsenko@linaro.org>
Cc: Chanho Park <chanho61.park@samsung.com>,
Jaewon Kim <jaewon02.kim@samsung.com>,
Wolfram Sang <wsa@kernel.org>, Rob Herring <robh+dt@kernel.org>,
linux-samsung-soc@vger.kernel.org, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC
Date: Fri, 19 Nov 2021 09:54:34 +0100 [thread overview]
Message-ID: <773110c9-fc74-6cab-68c0-1c771a3be104@canonical.com> (raw)
In-Reply-To: <CAPLW+4kS-pzROC5oyAjW1aJp5cb1e3XK+40HsKwgPdCziSp1ZQ@mail.gmail.com>
On 18/11/2021 20:59, Sam Protsenko wrote:
> On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 16/11/2021 02:12, Chanho Park wrote:
>>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
>>>> from my side (just a food for thought): do we want to configure USI
>>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would
>>>> be better design to implement some platform driver for that, so we can
>>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this
>>>> series is good to be merged as is, but we should probably consider all
>>>> upsides and downsides of each option, for the future work.
>>>
>>> I'm also considering how to support this USI configuration gracefully.
>>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
>>> But, there is a possibility that the USI hw version can be bumped for future SoCs.
>>>
>>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
>>>
>>> Option1) Make a USI driver under soc/samsung/ like [1].
>>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
>>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
>>>
>>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c
>>
>> I don't have user manuals, so all my knowledge here is based on
>> Exynos9825 vendor source code, therefore it is quite limited. In
>> devicetree the USI devices have their own nodes - but does it mean it's
>> separate SFR range dedicated to USI? Looks like that, especially that
>> address space is just for one register (4 bytes).
>>
>> In such case having separate dedicated driver makes sense and you would
>> only have to care about driver ordering (e.g. via device links or phandles).
>>
>> Option 2 looks interesting - reusing reset framework to set proper USI
>> mode, however this looks more like a hack. As you said Chanho, if there
>> is a USI version 3, this reset framework might not be sufficient.
>>
>> In option 3 each driver (UART/I2C/SPI) would need to receive second IO
>> range and toggle some registers, which could be done via shared
>> function. If USI v3 is coming, all such drivers could get more complicated.
>>
>> I think option 1 is the cleanest and extendable in future. It's easy to
>> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
>> also nicely encapsulates USI-related stuff in separate driver. Probe
>> ordering should not be a problem now.
>>
>> But as I said, I don't have even the big picture here, so I rely on your
>> opinions more.
>>
>
> Hi Krzysztof,
>
> Can you please let me know if you're going to apply this series as is,
> or if you want me to submit USIv2 driver first, and then rework this
> patch on top of it? I'm working on some HSI2C related patches right
> now, and thus it'd nice to know about your decision on this series
> beforehand, as some of my patches (like bindings doc patches) might
> depend on it. Basically I'd like to base my patches on the proper
> baseline, so we don't have to rebase those later.
This set won't go via my tree anyway, but I am against it. David pointed
out that his USIv1 is a little bit different and embedding in each of
I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 looks too
big. The solution with a dedicated driver looks to me more flexible and
encapsulated/cleaner.
Therefore after the discussions I am against this solution, so a
soft-NAK from my side.
Best regards,
Krzysztof
next prev parent reply other threads:[~2021-11-19 8:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20211112010603epcas2p331fe717eabfd9fc0280792921b25c535@epcas2p3.samsung.com>
2021-11-12 1:01 ` [PATCH v3 0/2] i2c: exynos5: add support for ExynosAutov9 SoC Jaewon Kim
[not found] ` <CGME20211112010603epcas2p26c076e65e0cb286cb53f06053165ef60@epcas2p2.samsung.com>
2021-11-12 1:01 ` [PATCH v3 1/2] dt-bindings: i2c: exynos5: add exynosautov9-hsi2c compatible Jaewon Kim
2021-11-15 18:56 ` Sam Protsenko
2021-11-19 8:57 ` Krzysztof Kozlowski
[not found] ` <CGME20211112010603epcas2p339d1a6ef3df7cdbe61c87c8afa541fd0@epcas2p3.samsung.com>
2021-11-12 1:01 ` [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC Jaewon Kim
2021-11-12 8:09 ` Krzysztof Kozlowski
2021-11-15 18:55 ` Sam Protsenko
2021-11-16 1:12 ` Chanho Park
2021-11-16 9:31 ` Krzysztof Kozlowski
2021-11-16 15:31 ` Sam Protsenko
2021-11-19 8:51 ` Krzysztof Kozlowski
2021-11-18 19:59 ` Sam Protsenko
2021-11-19 8:54 ` Krzysztof Kozlowski [this message]
2021-11-19 14:12 ` Sam Protsenko
2021-11-22 2:51 ` Jaewon Kim
2021-11-17 22:17 ` David Virag
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=773110c9-fc74-6cab-68c0-1c771a3be104@canonical.com \
--to=krzysztof.kozlowski@canonical.com \
--cc=chanho61.park@samsung.com \
--cc=devicetree@vger.kernel.org \
--cc=jaewon02.kim@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=semen.protsenko@linaro.org \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).