Re: [PATCH v14 8/8] soc: amd: Add support for AMD Pensando SoC Controller

[Arnd Bergmann]

On Wed, May 24, 2023, at 00:11, Brad Larson wrote:
>> On Mon, May 15, 2023, at 20:16, Brad Larson wrote:
>
>> Also, can you explain why this needs a low-lever user interface
>> in the first place, rather than something that can be expressed
>> using high-level abstractions as you already do with the reset
>> control?
>>
>> All of the above should be part of the changelog text to get a
>> driver like this merged. I don't think we can get a quick
>> solution here though, so maybe you can start by removing the
>> ioctl side and having the rest of the driver in drivers/reset?
>
> In the original patchset I added a pensando compatible to spidev and that
> was nacked in review and reusing some random compatible that made it into 
> spidev was just wrong.  Further it was recommended this should be a system 
> specific driver and don't rely on a debug driver like spidev.  I changed 
> over to a platform specific driver and at that time I also needed to include 
> a reset controller (emmc reset only).  I put these in drivers/mfd and 
> drivers/reset.  Review of the device tree for this approach went back and 
> forth to _not_ have four child nodes on the spi device each with the same 
> compatible. Decision was to squash the child nodes into the parent and put 
> the reset-controller there also.  One driver and since its pensando
> specific its currently in drivers/soc/amd.
>
> There are five different user processes and some utilities that access the 
> functionality in the cpld/fpga.  You're correct, its passing messages that 
> are specific to the IP accessed via chip-select.  No Elba system will boot 
> without this driver providing ioctl access.

Thank you for the detailed summary. Moving away from spidev and
from mfd seems all reasonable here. I'm still a bit confused by
why you have multiple chipselects here that are for different
subdevices but ended with a single user interface for all of them,
but that's not a big deal.

The main bit that sticks out about this high-level design is how
it relies on user space utilities at all to understand the message
format. From what I understand about the actual functionality of
this device, it most closely resembles an embedded controller that
you might find in a laptop or server machine, and those usually
have kernel drivers in drivers/platform/ to interact with the
device.

Has anyone tried to do it like that? Maybe it would help
to see what the full protocol and the user space side looks
like, in order to move some or all of it into the kernel.

     Arnd