Re: [PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/09/2022 14:20, Jesper Nilsson wrote:
> Hi,
> 
> 
> Thanks for your comments Krzysztof and sorry for being late in replying, I'll
> try to fill in the blanks for some of the issues below.
> 
> 
> The ARTPEC-8 is derived from a Samsung design, but built to order for Axis, so
> long term responsibility will fall to Axis (me and Lars primarily).

Sure, yet still the code looked quite generic, not Artpec specific. At
least as far as I remember, because it was long time ago since I
commented on this. This looked like generic driver, so not really tied
to SoC platform, so it deserves even merging with existing similar drivers.

It is very likely that Samsung will re-use this bloc for several other
customers wanting FPGA, so it should not be made specific to Artpec.
Naming "samsung xxx" is okay (if it cannot be merged to existing FPGA
manager driver), but then make it a separate maintainer entry, add you
(Axis/Artpec folks), me and maybe someone from Samsung as maintainers.

> The ARTPEC-6 and ARTPEC-7 were built by an other vendor and are quite different
> not to mention that they are 32-bit ARMs, compared to ARM 64-bit for the ARTPEC-8.
> 
> 
> The driver in this patchset is derived from the drivers/mfd/altera-sysmgr.c and
> solves the same problem, in where the SoC system controller IP is a collection
> of registers that controls quite a lot of different peripheral functions (from
> PCIe and Ethernet to PWM) and is reachable only through SMC calls. I think the
> naming of the software was set as samsung-sysmgr since it is not Axis design at
> all, but (to my knowledge) only existing in ARTPEC-8 as yet. I can't recall why
> the mfd driver route was selected, but it could be that the earlier
> implementation was more complex and used both smc and direct mmio writes.
> 
> The users of this system manager would initially be the ARTPEC-8 DWC EQoS and
> ARTPEC-8 DWC PCIe drivers sent in other patch-sets.

The patchset did not contain or reference (if I remember correctly) any
 users of exposed public API, so it cannot be accepted. One cannot add
API and not use it. With users and description of the problem the driver
is solving, it's different case... but the patchset lacked both.

> I believe a possible alternative to solve the system manager problem is to open
> code the SMC calls directly from the drivers in question, quite a lot of
> drivers seem to do this, notably a specific altera driver
> (drivers/edac/altera_edac.c) even though it also has a reference to the above
> mentioned altera-sysmgr regmap... :-)
> 
> Does that seem reasonable?

Exposing SMC firmware pieces with dedicated firmware driver is already
practiced, although I don't know whether it is recommended or not. The
driver however did not expose it and instead rather was creating unused,
undocumented loophole (at least that was my impression... but again -
long time ago).

Several drivers are using ARM SMC calls, so using this would be probably
OK. If ARM SMC calls work in this case - use it and most problems are gone.

Recently Apple M1 SMC driver is being upstreamed and I did not see any
objections to its concept.

> 
> Thanks for your patience and excuses for the top-posting.
> 
> /Jesper

Best regards,
Krzysztof