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

[Guenter Roeck <linux@roeck-us.net>]

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
>>