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

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

[Chatradhi, Naveen Krishna]

+ @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@amd.com>
>>
>> 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.

3. Define an ioctl for user-space to access other system management 
functionality.

    a. The open-sourced https://github.com/amd/esmi_oob_library will 
continue to provide user space programmable API

Regards,

naveenk

>
> Guenter
>

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

[Guenter Roeck]

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@amd.com>
>>>
>>> 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.

drivers/mailbox/ supports mailbox style communication. I don't know if that
would apply to the mailbox functionality implemented here.

Guenter

> 3. Define an ioctl for user-space to access other system management functionality.
> 
>     a. The open-sourced https://github.com/amd/esmi_oob_library will continue to provide user space programmable API
> 
> Regards,
> 
> naveenk
> 
>>
>> Guenter
>>

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

[Lee Jones]

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@amd.com>
> > > > 
> > > > 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.

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.

> drivers/mailbox/ supports mailbox style communication. I don't know if that
> would apply to the mailbox functionality implemented here.

-- 
Lee Jones [李琼斯]

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@amd.com>
>>>>>
>>>>> 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
>