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
