Re: [PATCH 0/5] mfd: add amd side-band functionality

[Lee Jones <lee@kernel.org>]

On Tue, 18 Jun 2024, Chatradhi, Naveen Krishna wrote:

> 
> On 6/14/2024 8:19 PM, Lee Jones wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Fri, 14 Jun 2024, Chatradhi, Naveen Krishna wrote:
> > 
> > > On 6/13/2024 10:35 PM, Lee Jones wrote:
> > > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > > 
> > > > 
> > > > On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
> > > > 
> > > > > From: Akshay Gupta <akshay.gupta@amd.com>
> > > > > 
> > > > > At present, sbrmi under hwmon subsystem is probed as an i2c
> > > > > driver and reports power.
> > > > > 
> > > > > However, APML interface defines few other protocols to support
> > > > > OOB system management functionality.
> > > > > 
> > > > > This patchset the following
> > > > > 1. Based on hwmon maintainers feedback, move the i2c client
> > > > >      probe and sbrmi core functionality to drivers/mfd/
> > > > > 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
> > > > >      report power using the symbol exported by the mfd/sbrmi-core.
> > > > > 3. Convert i2c to regmap which provides multiple benefits
> > > > >      over direct smbus APIs.
> > > > > 4. Register a misc device which provides
> > > > >       a. An ioctl interface through node /dev/sbrmiX
> > > > >       b. Open-sourced and widely used https://github.com/amd/esmi_oob_library
> > > > >          will continue to provide user-space programmable API for the following
> > > > >         - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
> > > > >         - CPUID access
> > > > >         - MCAMSR access
> > > > > 
> > > > > Akshay Gupta (5):
> > > > >     hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
> > > > >     mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
> > > > >     mfd/hwmon sbrmi: Use regmap subsystem
> > > > >     mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
> > > > >     mfd/hwmon: sbrmi: Add support for APML protocols
> > > > > 
> > > > >    drivers/hwmon/Kconfig         |   1 +
> > > > >    drivers/hwmon/sbrmi.c         | 284 +++-----------------
> > > > >    drivers/mfd/Kconfig           |   9 +-
> > > > >    drivers/mfd/Makefile          |   2 +
> > > > >    drivers/mfd/sbrmi-core.c      | 490 ++++++++++++++++++++++++++++++++++
> > > > It's not clear to me what any of these 500 lines do, but they do not
> > > > look like a good fit for MFD.  Perhaps I'm missing something.  Can you
> > > > provide some more information about the device and why you think MFD is
> > > > a suitable location for it?
> > > Hi lee,
> > > 
> > > Data center processors from AMD provide a side-band (often called
> > > out-of-band) path for manageability
> > > 
> > > called Advanced Platform Management Link (APML), which consists of i2c/i3c
> > > client devices called
> > > 
> > > Side-band Remote Management Interface (SB-RMI) and Side-band Temperature
> > > Sensor Interface (SB-TSI).
> > > 
> > > 
> > > We have i2c client drivers upstreamed under drivers/hwmon sbrmi.c and
> > > sbtsi_temp.c reporting power and
> > > 
> > > temperature via hwmon interfaces. However, sbrmi device can also provide
> > > performance, telemetry and RAS
> > MFD knows nothing of these characteristics.
> 
> Yes, we will modify the implementation to define ops structure with
> functionality that
> 
> can be called by platforms drivers in hwmon and other subsystems.
> 
> > 
> > > monitoring, management using AMD defined protocols. One of them
> > > sbrmi_mailbox_xfer()is defined in
> > I large portion of this should be moved out to drivers/mailbox.
> 
> we have explored the mailbox subsystem, APML xfer is not a ful-fledge
> mailbox interface as such,
> 
> it is a custom protocol, which accepts inputs and provides outputs over
> i2c/i3c. It does not support
> 
> multichannel (tx/rx) or have IRQs or a memory mapped IO and it is one of the
> protocols supported
> 
> by the RMI device.
> 
> > 
> > > drivers/hwmon/sbrmi.c.
> > > 
> > > Patchset would do the following
> > > 
> > > 1. Move core functionality from hwmon/sbrmi.c to drivers/mfd/sbrmi-i2c.c as
> > > an i2c_driver.
> > > 
> > > 2. Convert the hwmon/sbrmi.c to a platform driver.
> > > 
> > > 3. Use mfd_add_devices() API to add cells which will probe the platform
> > > driver under hwmon/sbrmi.c
> > How many devices will mfd_add_devices() be registering?
> 
> This patch is adding one hwmon device.
> 
> We can add additional cell which probes a platform driver in drivers/misc
> which handles
> 
> the user space interface part.

It sounds like MFD is (once more) being used as a dumping ground for
random devices which do not fit anywhere else.  A Multi-Function Device
driver's role is to create shared resources (memory, IRQs, Clocks,
Regulators, etc) for and register more than one real device that uses
those shared resources, that's all.  Even if you were to move the Misc
part out, using that as a second device to prove it MFD-worthy is not
going to fly.  Take a look at what these devices usually consist of:

  git grep -W "struct mfd_cell.*{" -- drivers/mfd

This submission results in a 650-line driver that registers a single
cell.  One that is attributed only to the device it's being removed
from.

I see that Guenter already said:

  "This is not hardware monitoring functionality and would have to
  reside elsewhere, outside the hwmon subsystem."

Well it's not MFD functionality either.  Sorry.

-- 
Lee Jones [李琼斯]