Re: [PATCH 2/2] sbrmi: Add support for APML protocols

[Chatradhi, Naveen Krishna]

On 5/15/2024 4:02 PM, Lee Jones wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Tue, 14 May 2024, Guenter Roeck wrote:
>
> > On 5/14/24 12:15, Chatradhi, Naveen Krishna wrote:
> > > + @Misc and @MFD maintainers in CC
> > >
> > > Hi
> > >
> > > On 5/3/2024 3:56 AM, Guenter Roeck wrote:
> > > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > >
> > > >
> > > > On 5/2/24 15:05, Naveen Krishna Chatradhi wrote:
> > > > > From: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx>
> > > > >
> > > > > The present sbrmi module only support reporting power. However, AMD data
> > > > > center processors support various system management functionality
> > > > > Out-of-band over Advanced Platform Management Link APML.
> > > > >
> > > > > Register a miscdevice, which creates a device /dev/sbrmiX with an IOCTL
> > > > > interface for the user space to invoke the following protocols.
> > > > >     - Mailbox read/write (already defined in sbrmi_mailbox_xfer())
> > > > >     - CPUID read
> > > > >     - MCAMSR read
> > > > This is not hardware monitoring functionality and would have to reside
> > > > elsewhere, outside the hwmon subsystem.
> > > I thought as much, please provide your opinion on the following approach.
> > >
> > > Background: 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.
> > >
> > > As adding the core functionality of the APML interface in drivers/hwmon/sbrmi is not the correct approach.
> > >
> > > We would like do the following
> > >
> > > 1. Move the i2c client probe, misc device registration and rmi_mailbox_xfer() function to a drivers/misc.
> > >
> > > 2. Add an MFD device with a cell, which probes the hwmon/sbrmi and continues to report power using the symbols exported by the misc/sbrmi.
> > afaik mfd API function must not be used outside drivers/mfd. The mfd maintainer
> > is (or at least used to be) pretty strict on that. The core code of a
> > multi-function device might better be implemented in drivers/mfd, with
> > drivers in drivers/misc (for the misc device) and drivers/hwmon/ (for
> > hwmon functionality). The misc and hwmon drivers could then communicate
> > with the core using regmap.

Thanks Guenter, for the inputs.

> Yes, please only use the MFD API from drivers/mfd.
>
> There are 'offenders' that slipped by me, but in general if you need to
> create an MFD then it should be located in the MFD subsystem.
Thanks Lee, how about

1. Move the i2c client probe, misc device registration and rmi_mailbox_xfer() function from existing hwmon/sbrmi.c to a drivers/mfd (Instead of drivers/misc)

   a. Provide an ioctl interface through misc device node /dev/sbrmiX for the user space to invoke the following protocols.
     - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
     - CPUID access
     - MCAMSR access

2. Add an MFD device with a cell, which probes the hwmon/sbrmi and continues to report power using the symbols exported by the mfd/sbrmi.

> > drivers/mailbox/ supports mailbox style communication. I don't know if that
> > would apply to the mailbox functionality implemented here.
I've explored that path, and APML mailbox does not fit well into the 
design, we do not have a dedicated interrupt line as well.
> --
> Lee Jones [李琼斯]

regards,

Naveenk
>