Message-ID: <000001d7df4b$cc619f10$6524dd30$@samsung.com>
Date:   Mon, 22 Nov 2021 11:51:07 +0900
From:   "Jaewon Kim" <jaewon02.kim@...sung.com>
To:     "'Sam Protsenko'" <semen.protsenko@...aro.org>
Cc:     "'Chanho Park'" <chanho61.park@...sung.com>,
        "'Wolfram Sang'" <wsa@...nel.org>,
        "'Rob Herring'" <robh+dt@...nel.org>,
        <linux-samsung-soc@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        "'Krzysztof Kozlowski'" <krzysztof.kozlowski@...onical.com>
Subject: RE: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

Hi Protsenko,

> On Fri, 19 Nov 2021 at 10:54, Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com> wrote:
> >
> > On 18/11/2021 20:59, Sam Protsenko wrote:
> > > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@...onical.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://protect2.fireeye.com/v1/url?k=b290a67b-ed0b9f6a-b2912d34-0
> > >>> cc47a31cdbc-ceadd8e62313162a&q=1&e=317825c0-3fac-46ad-9b4e-f93de42
> > >>> ad5ba&u=https%3A%2F%2Fgithub.com%2Fianmacd%2Fd2s%2Fblob%2Fmaster%2
> > >>> Fdrivers%2Fsoc%2Fsamsung%2Fusi_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.
> >
> 
> Hi Jaewon,
> 
> I'm going to submit USI driver soon, and also some more HSI2C patches.
> Do you mind if I rework your patches to rely on USI drver (instead of modifying System Register in
> HSI2C driver), and include those in my patch series? Of course, I'll preserve your authorship. Just
> think that would be easier and faster this way.
> 
> Thanks!
> 

I'm glad you're working on a USI driver.
You can use my patchset.

> >
> > Best regards,
> > Krzysztof

Thanks
Jaewon Kim

Powered by blists - more mailing lists