On 16/11/2021 16:31, Sam Protsenko wrote: > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxxxxx> 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. >> > > To provide more context, USI registers are split across two different > register maps: > > - USI protocol configuration (where we choose which protocol to > use: HSI2C, SPI or UART) is done via *_SW_CONF registers, from System > Register SFR range. To access those SW_CONF registers we need to > either: > (Option 3) pass System Register registers to corresponding > driver (HSI2C/SPI/UART) via syscon > (Option 1) or implement separate USI driver, so we can choose > desired protocol in device tree; in that case I guess System Register > registers should be passed via syscon to USI driver > - USI registers (like USI_CON register, which contains USI_RESET > bit) are contained in the same SFR range as corresponding > HSI2C/SPI/UART IP-core. Or rather HSI2C/SPI/UART IP-cores are > encapsulated within USI block(s). So to access registers like USI_CON > we only need to use memory already passed to corresponding > HSI2C/SPI/UART driver via "reg" property. > > So I'd say ideally we should do next: > - modify USI registers (like USI_CON) in corresponding > HSI2C/SPI/UART drivers; but because all HSI2C/SPI/UART drivers share > the same USI registers, we might want to pull USI register offsets and > bits to some common header file, for example (to not duplicate that > code in drivers) > - implement separate USI driver to control SW_CONF registers from > System Register module (Option 1), so we can choose desired protocol > in device tree (in some USI node, not in HSI2C node) > - also, it probably makes sense to group all USI related nodes in > some separate *-usi.dtsi file; that would reduce confusion, given that > we have even more encapsulation in Exynos850 thanks to CMGP (Common > GPIO) block > > My suggestion is to take this patch as is, and then we can work on USI > driver implementation/upstreaming. No, you cannot later rework it. It becomes a binding which you need to support. > Right now we don't have much of > device tree files that use USI HSI2C driver, so it'll be fairly easy > to fix those dts's once we implemented USI driver. Once sysreg solution is accepted, it's removal would be ABI break. Please do not rush with incomplete solutions. > That will also > unblock me with submitting dev board support (dts files) I'm working > on right now. Krzysztof, please let me know if I'm wrong and if we > shouldn't change dts API too much, so it's mandatory to implement USI > driver before accepting this patch. David's point of USIv1 points me to the separate driver solution. Best regards, Krzysztof
