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

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

On 6/18/2024 5:57 PM, Lee Jones wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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

sbrmi is one physical i2c/i3c device with a set of registers and an IRQ

which provides multiple functions via different protocols.

         |------------|     -> apml_xfer

         |   sbrmi  |      -> cpuid_xfer

         |-----------|      -> msr_mca_xfer

                  L-----------> IRQ

We were thinking of adding each functionality as an mfd_cell + 1 
mfd_cell for hwmon interface.

I understand it now, it doesn't fit well. Can you suggest any other 
sub-system that can absorb this.

I can think of moving everything to a misc driver with a misc device 
node and

register a hwmon device for reporting power. please suggest.

>
> 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 [李琼斯]