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

From: "Arnd Bergmann" <arnd@arndb.de>
To: "Brad Larson" <blarson@amd.com>, linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org,
	"linux-mmc @ vger . kernel . org" <linux-mmc@vger.kernel.org>,
	linux-spi@vger.kernel.org,
	"Adrian Hunter" <adrian.hunter@intel.com>,
	alcooperx@gmail.com,
	"Andy Shevchenko" <andy.shevchenko@gmail.com>,
	brendan.higgins@linux.dev,
	"Brian Norris" <briannorris@chromium.org>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"David Gow" <davidgow@google.com>,
	gsomlo@gmail.com, "Greg Ungerer" <gerg@linux-m68k.org>,
	"Hal Feng" <hal.feng@starfivetech.com>,
	"Hitomi Hasegawa" <hasegawa-hitomi@fujitsu.com>,
	"Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
	"Joel Stanley" <joel@jms.id.au>,
	"Emil Renner Berthing" <kernel@esmil.dk>,
	"Krzysztof Kozlowski" <krzk@kernel.org>,
	krzysztof.kozlowski+dt@linaro.org, "Lee Jones" <lee@kernel.org>,
	"Lee Jones" <lee.jones@linaro.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Randy Dunlap" <rdunlap@infradead.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Samuel Holland" <samuel@sholland.org>,
	"Serge Semin" <fancer.lancer@gmail.com>,
	"Shuah Khan" <skhan@linuxfoundation.org>,
	suravee.suthikulpanit@amd.com,
	"Tom Lendacky" <thomas.lendacky@amd.com>,
	"Tony Huang" <tonyhuang.sunplus@gmail.com>,
	"Ulf Hansson" <ulf.hansson@linaro.org>,
	vaishnav.a@ti.com, "Walker Chen" <walker.chen@starfivetech.com>,
	"Will Deacon" <will@kernel.org>,
	"Yinbo Zhu" <zhuyinbo@loongson.cn>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller
Date: Fri, 22 Sep 2023 06:24:00 -0400	[thread overview]
Message-ID: <68ca306c-5ea2-437e-b7f1-68bad88557a0@app.fastmail.com> (raw)
In-Reply-To: <20230913214944.59804-7-blarson@amd.com>

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 