| Message ID | 20240530112307.3089696-1-naveenkrishna.chatradhi@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mfd: add amd side-band functionality | expand |
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? > 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 >
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 monitoring, management using AMD defined protocols. One of them sbrmi_mailbox_xfer()is defined in 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 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 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 [李琼斯]
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. > 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. > 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? > 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? > 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 [李琼斯]
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 [李琼斯]
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 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.
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 [李琼斯]
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 ++++++++++++++++++++++++++++++++++ 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