Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller

Arnd Bergmann
2023-09-22T10:24:30Z

On Wed, Sep 13, 2023, at 17:49, Brad Larson wrote:
> The Pensando SoC controller is a SPI connected companion device
> that is present in all Pensando SoC board designs. The essential
> board management registers are accessed on chip select 0 with
> board mgmt IO support accessed using additional chip selects.
>
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>
> v15 changes:
> - Drop custom ioctl and use existing miscdevice interface.
> - Delete unused definitions in amd-pensando-ctrl.h
> - Makefile change to compile for ARCH_PENSANDO

Hi Brad,

I'm sorry I've been out of the loop for so long, and I hope
we can find a way to manage your SoC support soon. My impression
is that the normal support patches (1, 3, 4, and 5) are largely
uncontroversial, while the SoC controller support seems like
we are still not converging onto something that is ready to
merge, so I would suggest you split the two parts and send
the basic support for inclusion in linux-6.7 while we continue
to discuss the soc controller driver.

Please remove any references to the soc controller from the
dts files and send that first series to:soc@kernel.org
cc:linux-arm-kernel (and the other interested parties) so
I can pick those up.

Regarding the soc controller driver, let me try to give
you my impression of where we are:

- you have gone through 16 revisions already, which is way
too much for a public review, we should have been able
to find a solution earlier than that, and this is partly
our fault on the reviewer side, and I'm sorry about that.

- Andy's latest comments and a lot of the earlier reviews
were focused on implementation details. While those comments
are helpful suggestions for improving the code, they miss
the larger point about the system design that I'm worried
about and probably don't help you actually get it merged.

- The main problem I still see is that this driver completely
bypasses our normal kernel abstractions and instead creates
a low-level passthrough interface for handling kernel
functionality in userspace. This creates a liability both
for the user ABI and the kernel implementation and prevents
any

- There is a chance that your design is in fact the
best way to handle this particular hardware, but it is
your job to write a convincing explanation of why this
platform is different from all the others in the patch
description. Your current one-paragraph text does not
explain this at all.

I would suggest you prioritize getting the other patches
included for the moment, but we can keep discussion the
API design for this driver either in this thread or on the
#armlinux IRC channel (irc.libera.chat) in parallel if you
like. In order to help you here, I would need either
the documentation of the SPI software interface, or the
source code for the userspace tool.

Arnd