Message-ID: <5687bb27-973e-b774-b876-46c8dffc1176@canonical.com>
Date:   Mon, 29 Nov 2021 18:35:51 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Sam Protsenko <semen.protsenko@...aro.org>,
        David Virag <virag.david003@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>, Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jaewon Kim <jaewon02.kim@...sung.com>,
        Chanho Park <chanho61.park@...sung.com>,
        Youngmin Nam <youngmin.nam@...sung.com>,
        devicetree@...r.kernel.org, linux-spi@...r.kernel.org,
        linux-serial@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
        linux-i2c@...r.kernel.org
Subject: Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

On 29/11/2021 14:56, Sam Protsenko wrote:
> On Sun, 28 Nov 2021 at 05:15, David Virag <virag.david003@...il.com> wrote:
>>
>> Also this way is pretty USIv2 centric. Adding USIv1 support to this
>> driver is difficult this way because of the the lack of USI_CON and
>> USI_OPTION registers as a whole (so having nowhere to actually set the
>> reg of the USI node to, as the only thing USIv1 has is the SW_CONF
>> register). In my opinion being able to use the same driver and same
>> device tree layout for USIv1 and USIv2 is a definite plus
>>
> 
> Well, it's USIv2 driver after all. I never expected it can be extended
> for USIv1 support. If you think it can be reused for USIv1, it's fine
> by me. But we need to consider next things:
>   - rename the driver to just "usi.c" (and also its configuration symbol)
>   - provide different compatible for USIv1 (and maybe corresponding driver data)
>   - rework bindings (header and doc); make sure existing bindings are
> intact (we shouldn't change already introduced interfaces)
>   - in case of USIv1 compatible; don't try to tinker with USIv2 registers
>   - samsung,clkreq-on won't be available in case of USIv1 compatible

I expect this driver to be in future extended for USIv1 and I do not see
any problems in doing that for current Sam's approach. Most of our
drivers support several devices, sometimes with differences, and we
already have patterns solving it, e.g. ops structure or quirks bitmap.
Driver for new USIv1 compatible would skip setting USI_CON (or any other
unrelated register). Modification of SW_CONF could be shared or could be
also split, depending on complexity.

> 
> Because I don't have USIv1 SoC TRM (and neither do I possess some
> USIv1 board which I can use for test), I don't think it's my place to
> add USIv1 support. But I think it's possible to do so, using my input
> above.
> 
> I can see how it might be frustrating having to do some extra work
> (comparing to just using the code existing in downstream). But I guess
> that's the difference: vendor is mostly concerned about competitive
> advantage and getting to market fast, while upstream is more concerned
> about quality, considering all use cases, and having proper design.
> Anyway, we can work together to make it right, and to have both
> IP-cores support. In the worst case, if those are too different, we
> can have two separate drivers for those.
> 
>> The only real drawback of that way is having to add code for USIv2
>> inside the UART, HSI2C, and SPI drivers but in my opinion the benefits
>> overweigh the drawbacks greatly. We could even make the uart/spi/hsi2c
>> drivers call a helper function in the USI driver to set their USI_CON
>> and USI_OPTION registers up so that code would be shared and not
>> duplicated. Wether this patch gets applied like this is not my choice
>> though, I'll let the people responsible decide
>> :-)
>>
> 
> I'd argue that there are a lot of real drawbacks of using downstream
> driver as is. That's why I completely re-designed and re-implemented
> it. Downstream driver can't be built and function as a module, it
> doesn't respect System Register sharing between consumers, it leads to
> USI reset code duplication scattered across protocol drivers (that
> arguably shouldn't even be aware of that), it doesn't reflect HW
> structure clearly, it's not holding clocks needed for registers access
> (btw, sysreg clock can be provided in syscon node, exactly for that
> reason). As Krzysztof said, it also can't handle correct probe order
> and deferred probes. Downstream driver might work fine for some
> particular use-cases the vendor has, but in upstream it's better to
> cover more cases we can expect, as upstream kernel is used on more
> platforms, with more user space variants, etc.

Implementing USI in each of I2C/SPI/UART drivers is a big minus. Current
approach nicely encapsulates USI in dedicated driver without polluting
the other drivers with unrelated bus/protocol stuff.

Best regards,
Krzysztof

Powered by blists - more mailing lists