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

["Chatradhi, Naveen Krishna" <naveenkrishna.chatradhi@amd.com>]

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.

>
>> 4. drivers/mfd/sbrmi-core.c will contain the common functions which can be
>> used by i2c and i3c based drivers, such as
>>
>> core protocol definitions, creation of misc device and an ioctl for the user
> drivers/misc?

We will move the misc device registration and user space interface to 
drivers/misc and

use the ops provided by this mfd device.

Regards,

naveenk

>
>> interface.
>>
>> This patchset is an attempt toadd support for these core protocols in such a
>> way that Open-sourced and widely used
>> https://github.com/amd/esmi_oob_library will continue to provide user-space
>> programmable API. regards, Naveenk
>>
>>>>    drivers/mfd/sbrmi-core.h      |  37 +++
>>>>    drivers/mfd/sbrmi-i2c.c       | 165 ++++++++++++
>>>>    include/linux/mfd/amd-sb.h    |  55 ++++
>>>>    include/uapi/linux/amd-apml.h |  74 +++++
>>>>    9 files changed, 871 insertions(+), 246 deletions(-)
>>>>    create mode 100644 drivers/mfd/sbrmi-core.c
>>>>    create mode 100644 drivers/mfd/sbrmi-core.h
>>>>    create mode 100644 drivers/mfd/sbrmi-i2c.c
>>>>    create mode 100644 include/linux/mfd/amd-sb.h
>>>>    create mode 100644 include/uapi/linux/amd-apml.h
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>> --
>>> Lee Jones [李琼斯]
> --
> Lee Jones [李琼斯]