On 29/11/2021 14:56, Sam Protsenko wrote:

On Sun, 28 Nov 2021 at 05:15, David Virag [off-list ref] wrote:

quoted

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.

quoted

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