[PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support

Binbin Zhou posted 3 patches 3 months, 4 weeks ago
There is a newer version of this series
drivers/char/ipmi/Makefile       |   1 +
drivers/char/ipmi/ipmi_si.h      |   7 +
drivers/char/ipmi/ipmi_si_intf.c |   3 +
drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
drivers/mfd/Kconfig              |  12 +
drivers/mfd/Makefile             |   2 +
drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
7 files changed, 699 insertions(+)
create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
create mode 100644 drivers/mfd/ls2kbmc-mfd.c
[PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Binbin Zhou 3 months, 4 weeks ago
Hi all:

This patch set introduces the Loongson-2K BMC.

It is a PCIe device present on servers similar to the Loongson-3 CPUs.
And it is a multifunctional device (MFD), such as display as a sub-function
of it.

For IPMI, according to the existing design, we use software simulation to
implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.

Also since both host side and BMC side read and write kcs status, we use
fifo pointer to ensure data consistency.

For the display, based on simpledrm, the resolution is read from a fixed
position in the BMC since the hardware does not support auto-detection
of the resolution. Of course, we will try to support multiple
resolutions later, through a vbios-like approach.

Especially, for the BMC reset function, since the display will be
disconnected when BMC reset, we made a special treatment of re-push.

Based on this, I will present it in four patches:
patch-1: BMC device PCI resource allocation.
patch-2: BMC reset function support
patch-3: IPMI implementation

Thanks.

-------
V4:
- Add Reviewed-by tag;
- Change the order of the patches.
Patch (1/3):
  - Fix build warning by lkp: Kconfig tristate -> bool
    - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
 - Update commit message;
 - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
   Makefile.
Patch (2/3):
  - Remove unnecessary newlines;
  - Rename ls2k_bmc_check_pcie_connected() to
    ls2k_bmc_pcie_is_connected();
  - Update comment message.
Patch (3/3):
  - Remove unnecessary newlines.

Link to V3:
https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/

V3:
Patch (1/3):
 - Drop "MFD" in title and comment;
 - Fromatting code;
 - Add clearer comments.
Patch (2/3):
 - Rebase linux-ipmi/next tree;
 - Use readx()/writex() to read and write IPMI data instead of structure
   pointer references;
 - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
 - Drop unused output.
Patch (3/3):
 - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
 - Add clearer comments.
 - Use proper multi-line commentary as per the Coding Style documentation;
 - Define all magic numbers.

Link to V2:
https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/

V2:
- Drop ls2kdrm, use simpledrm instead.
Patch (1/3):
 - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
 - Add resolution fetching due to replacing the original display
   solution with simpledrm; 
 - Add aperture_remove_conflicting_devices() to avoid efifb
   conflict with simpledrm.
Patch (3/3):
 - This part of the function, moved from the original ls2kdrm to mfd;
 - Use set_console to implement the Re-push display function.

Link to V1:
https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/

Binbin Zhou (3):
  mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
  mfd: ls2kbmc: Add Loongson-2K BMC reset function support
  ipmi: Add Loongson-2K BMC support

 drivers/char/ipmi/Makefile       |   1 +
 drivers/char/ipmi/ipmi_si.h      |   7 +
 drivers/char/ipmi/ipmi_si_intf.c |   3 +
 drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
 drivers/mfd/Kconfig              |  12 +
 drivers/mfd/Makefile             |   2 +
 drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
 7 files changed, 699 insertions(+)
 create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
 create mode 100644 drivers/mfd/ls2kbmc-mfd.c


base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
-- 
2.47.1
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Corey Minyard 3 months, 4 weeks ago
On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> Hi all:
> 
> This patch set introduces the Loongson-2K BMC.
> 
> It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> And it is a multifunctional device (MFD), such as display as a sub-function
> of it.

I've asked this before, but I haven't gotten a answer, I don't think.

Is this really a multi-function device?  Is there (or will there be)
another driver that uses the MFD code?

If nothing else is going to use this, then it's really not a
multi-function device and all the code can go into the IPMI directory.
That simplifies maintenance.

If it is a multi-function device, then I want two separate Kconfig
items, one for the MFD and one for the IPMI portion.  That way it's
ready and you don't have to bother about the IPMI portion when
adding the other device.

All else looks good, I think.

-corey

> 
> For IPMI, according to the existing design, we use software simulation to
> implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> 
> Also since both host side and BMC side read and write kcs status, we use
> fifo pointer to ensure data consistency.
> 
> For the display, based on simpledrm, the resolution is read from a fixed
> position in the BMC since the hardware does not support auto-detection
> of the resolution. Of course, we will try to support multiple
> resolutions later, through a vbios-like approach.
> 
> Especially, for the BMC reset function, since the display will be
> disconnected when BMC reset, we made a special treatment of re-push.
> 
> Based on this, I will present it in four patches:
> patch-1: BMC device PCI resource allocation.
> patch-2: BMC reset function support
> patch-3: IPMI implementation
> 
> Thanks.
> 
> -------
> V4:
> - Add Reviewed-by tag;
> - Change the order of the patches.
> Patch (1/3):
>   - Fix build warning by lkp: Kconfig tristate -> bool
>     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
>  - Update commit message;
>  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
>    Makefile.
> Patch (2/3):
>   - Remove unnecessary newlines;
>   - Rename ls2k_bmc_check_pcie_connected() to
>     ls2k_bmc_pcie_is_connected();
>   - Update comment message.
> Patch (3/3):
>   - Remove unnecessary newlines.
> 
> Link to V3:
> https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> 
> V3:
> Patch (1/3):
>  - Drop "MFD" in title and comment;
>  - Fromatting code;
>  - Add clearer comments.
> Patch (2/3):
>  - Rebase linux-ipmi/next tree;
>  - Use readx()/writex() to read and write IPMI data instead of structure
>    pointer references;
>  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
>  - Drop unused output.
> Patch (3/3):
>  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
>  - Add clearer comments.
>  - Use proper multi-line commentary as per the Coding Style documentation;
>  - Define all magic numbers.
> 
> Link to V2:
> https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> 
> V2:
> - Drop ls2kdrm, use simpledrm instead.
> Patch (1/3):
>  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
>  - Add resolution fetching due to replacing the original display
>    solution with simpledrm; 
>  - Add aperture_remove_conflicting_devices() to avoid efifb
>    conflict with simpledrm.
> Patch (3/3):
>  - This part of the function, moved from the original ls2kdrm to mfd;
>  - Use set_console to implement the Re-push display function.
> 
> Link to V1:
> https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> 
> Binbin Zhou (3):
>   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
>   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
>   ipmi: Add Loongson-2K BMC support
> 
>  drivers/char/ipmi/Makefile       |   1 +
>  drivers/char/ipmi/ipmi_si.h      |   7 +
>  drivers/char/ipmi/ipmi_si_intf.c |   3 +
>  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
>  drivers/mfd/Kconfig              |  12 +
>  drivers/mfd/Makefile             |   2 +
>  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
>  7 files changed, 699 insertions(+)
>  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
>  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> 
> 
> base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> -- 
> 2.47.1
>
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Binbin Zhou 3 months, 4 weeks ago
Hi Corey:

On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
>
> On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > Hi all:
> >
> > This patch set introduces the Loongson-2K BMC.
> >
> > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > And it is a multifunctional device (MFD), such as display as a sub-function
> > of it.
>
> I've asked this before, but I haven't gotten a answer, I don't think.
>
> Is this really a multi-function device?  Is there (or will there be)
> another driver that uses the MFD code?

I am very sorry, I may have overlooked your previous question.

And I also may not have a thorough understanding of multifunction devices.

The Loongson-2K BMC device provides two functions: display and IPMI.
For display, we pass the simplefb_platform_data parameter and register
the simpledrm device, as shown in patch-1.

Therefore, I think this should be considered a multifunction device.

>
> If nothing else is going to use this, then it's really not a
> multi-function device and all the code can go into the IPMI directory.
> That simplifies maintenance.
>
> If it is a multi-function device, then I want two separate Kconfig
> items, one for the MFD and one for the IPMI portion.  That way it's
> ready and you don't have to bother about the IPMI portion when
> adding the other device.
>
> All else looks good, I think.
>
> -corey
>
> >
> > For IPMI, according to the existing design, we use software simulation to
> > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> >
> > Also since both host side and BMC side read and write kcs status, we use
> > fifo pointer to ensure data consistency.
> >
> > For the display, based on simpledrm, the resolution is read from a fixed
> > position in the BMC since the hardware does not support auto-detection
> > of the resolution. Of course, we will try to support multiple
> > resolutions later, through a vbios-like approach.
> >
> > Especially, for the BMC reset function, since the display will be
> > disconnected when BMC reset, we made a special treatment of re-push.
> >
> > Based on this, I will present it in four patches:
> > patch-1: BMC device PCI resource allocation.
> > patch-2: BMC reset function support
> > patch-3: IPMI implementation
> >
> > Thanks.
> >
> > -------
> > V4:
> > - Add Reviewed-by tag;
> > - Change the order of the patches.
> > Patch (1/3):
> >   - Fix build warning by lkp: Kconfig tristate -> bool
> >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> >  - Update commit message;
> >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> >    Makefile.
> > Patch (2/3):
> >   - Remove unnecessary newlines;
> >   - Rename ls2k_bmc_check_pcie_connected() to
> >     ls2k_bmc_pcie_is_connected();
> >   - Update comment message.
> > Patch (3/3):
> >   - Remove unnecessary newlines.
> >
> > Link to V3:
> > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> >
> > V3:
> > Patch (1/3):
> >  - Drop "MFD" in title and comment;
> >  - Fromatting code;
> >  - Add clearer comments.
> > Patch (2/3):
> >  - Rebase linux-ipmi/next tree;
> >  - Use readx()/writex() to read and write IPMI data instead of structure
> >    pointer references;
> >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> >  - Drop unused output.
> > Patch (3/3):
> >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> >  - Add clearer comments.
> >  - Use proper multi-line commentary as per the Coding Style documentation;
> >  - Define all magic numbers.
> >
> > Link to V2:
> > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> >
> > V2:
> > - Drop ls2kdrm, use simpledrm instead.
> > Patch (1/3):
> >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> >  - Add resolution fetching due to replacing the original display
> >    solution with simpledrm;
> >  - Add aperture_remove_conflicting_devices() to avoid efifb
> >    conflict with simpledrm.
> > Patch (3/3):
> >  - This part of the function, moved from the original ls2kdrm to mfd;
> >  - Use set_console to implement the Re-push display function.
> >
> > Link to V1:
> > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> >
> > Binbin Zhou (3):
> >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> >   ipmi: Add Loongson-2K BMC support
> >
> >  drivers/char/ipmi/Makefile       |   1 +
> >  drivers/char/ipmi/ipmi_si.h      |   7 +
> >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> >  drivers/mfd/Kconfig              |  12 +
> >  drivers/mfd/Makefile             |   2 +
> >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> >  7 files changed, 699 insertions(+)
> >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> >
> >
> > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > --
> > 2.47.1
> >


-- 
Thanks.
Binbin
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Corey Minyard 3 months, 4 weeks ago
On Sat, Jun 14, 2025 at 10:50:37AM +0800, Binbin Zhou wrote:
> Hi Corey:
> 
> On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
> >
> > On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > > Hi all:
> > >
> > > This patch set introduces the Loongson-2K BMC.
> > >
> > > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > > And it is a multifunctional device (MFD), such as display as a sub-function
> > > of it.
> >
> > I've asked this before, but I haven't gotten a answer, I don't think.
> >
> > Is this really a multi-function device?  Is there (or will there be)
> > another driver that uses the MFD code?
> 
> I am very sorry, I may have overlooked your previous question.
> 
> And I also may not have a thorough understanding of multifunction devices.
> 
> The Loongson-2K BMC device provides two functions: display and IPMI.
> For display, we pass the simplefb_platform_data parameter and register
> the simpledrm device, as shown in patch-1.
> 
> Therefore, I think this should be considered a multifunction device.

Ok, that's clear, thank you.

However, that's not really very clear from the patches you have
provided.  Particularly, the "bmc" in the name from patch 1 makes one
think that it's only a bmc.

The "bmc" name is also a little confusing; the devices with a "bmc" in
the name are all the BMC side, but you are doing a host side device.

If you look at most of the other MFDs, they have a "core" section then
various other parts that use the core.  And possibly parts in other
directories for individual functions.  I think you need to do the same
design.  Have a "core" section that both the display and bmc use, then a
separate display and bmc driver.

That way, you can use the display without the IPMI interface, or the
IPMI interface without the display.

I would like to see:

* A core mfd device named ls2k-core.c that has the core functions.
  It would have its own config item (MFD_LS2K) that would be
  selected if either the display or IPMI device is enabled.

* A separate display device in its own file with its own config item.
  This isn't my area, so I'm not sure where this should go.

* The IPMI device in the ipmi directory named ipmi_ls2k.c, again
  with it's own config item (IPMI_LS2K).

Does that make sense?  I don't want to make things too hard, but that's
the way pretty much everything else is done on MFDs.

Thanks,

-corey

> 
> >
> > If nothing else is going to use this, then it's really not a
> > multi-function device and all the code can go into the IPMI directory.
> > That simplifies maintenance.
> >
> > If it is a multi-function device, then I want two separate Kconfig
> > items, one for the MFD and one for the IPMI portion.  That way it's
> > ready and you don't have to bother about the IPMI portion when
> > adding the other device.
> >
> > All else looks good, I think.
> >
> > -corey
> >
> > >
> > > For IPMI, according to the existing design, we use software simulation to
> > > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> > >
> > > Also since both host side and BMC side read and write kcs status, we use
> > > fifo pointer to ensure data consistency.
> > >
> > > For the display, based on simpledrm, the resolution is read from a fixed
> > > position in the BMC since the hardware does not support auto-detection
> > > of the resolution. Of course, we will try to support multiple
> > > resolutions later, through a vbios-like approach.
> > >
> > > Especially, for the BMC reset function, since the display will be
> > > disconnected when BMC reset, we made a special treatment of re-push.
> > >
> > > Based on this, I will present it in four patches:
> > > patch-1: BMC device PCI resource allocation.
> > > patch-2: BMC reset function support
> > > patch-3: IPMI implementation
> > >
> > > Thanks.
> > >
> > > -------
> > > V4:
> > > - Add Reviewed-by tag;
> > > - Change the order of the patches.
> > > Patch (1/3):
> > >   - Fix build warning by lkp: Kconfig tristate -> bool
> > >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> > >  - Update commit message;
> > >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> > >    Makefile.
> > > Patch (2/3):
> > >   - Remove unnecessary newlines;
> > >   - Rename ls2k_bmc_check_pcie_connected() to
> > >     ls2k_bmc_pcie_is_connected();
> > >   - Update comment message.
> > > Patch (3/3):
> > >   - Remove unnecessary newlines.
> > >
> > > Link to V3:
> > > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> > >
> > > V3:
> > > Patch (1/3):
> > >  - Drop "MFD" in title and comment;
> > >  - Fromatting code;
> > >  - Add clearer comments.
> > > Patch (2/3):
> > >  - Rebase linux-ipmi/next tree;
> > >  - Use readx()/writex() to read and write IPMI data instead of structure
> > >    pointer references;
> > >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> > >  - Drop unused output.
> > > Patch (3/3):
> > >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> > >  - Add clearer comments.
> > >  - Use proper multi-line commentary as per the Coding Style documentation;
> > >  - Define all magic numbers.
> > >
> > > Link to V2:
> > > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> > >
> > > V2:
> > > - Drop ls2kdrm, use simpledrm instead.
> > > Patch (1/3):
> > >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> > >  - Add resolution fetching due to replacing the original display
> > >    solution with simpledrm;
> > >  - Add aperture_remove_conflicting_devices() to avoid efifb
> > >    conflict with simpledrm.
> > > Patch (3/3):
> > >  - This part of the function, moved from the original ls2kdrm to mfd;
> > >  - Use set_console to implement the Re-push display function.
> > >
> > > Link to V1:
> > > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> > >
> > > Binbin Zhou (3):
> > >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> > >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> > >   ipmi: Add Loongson-2K BMC support
> > >
> > >  drivers/char/ipmi/Makefile       |   1 +
> > >  drivers/char/ipmi/ipmi_si.h      |   7 +
> > >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> > >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> > >  drivers/mfd/Kconfig              |  12 +
> > >  drivers/mfd/Makefile             |   2 +
> > >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> > >  7 files changed, 699 insertions(+)
> > >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> > >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> > >
> > >
> > > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > > --
> > > 2.47.1
> > >
> 
> 
> -- 
> Thanks.
> Binbin
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Binbin Zhou 3 months, 4 weeks ago
Hi Corey:

Thanks for your detailed suggestions.

On Sat, Jun 14, 2025 at 11:20 AM Corey Minyard <corey@minyard.net> wrote:
>
> On Sat, Jun 14, 2025 at 10:50:37AM +0800, Binbin Zhou wrote:
> > Hi Corey:
> >
> > On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
> > >
> > > On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > > > Hi all:
> > > >
> > > > This patch set introduces the Loongson-2K BMC.
> > > >
> > > > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > > > And it is a multifunctional device (MFD), such as display as a sub-function
> > > > of it.
> > >
> > > I've asked this before, but I haven't gotten a answer, I don't think.
> > >
> > > Is this really a multi-function device?  Is there (or will there be)
> > > another driver that uses the MFD code?
> >
> > I am very sorry, I may have overlooked your previous question.
> >
> > And I also may not have a thorough understanding of multifunction devices.
> >
> > The Loongson-2K BMC device provides two functions: display and IPMI.
> > For display, we pass the simplefb_platform_data parameter and register
> > the simpledrm device, as shown in patch-1.
> >
> > Therefore, I think this should be considered a multifunction device.
>
> Ok, that's clear, thank you.
>
> However, that's not really very clear from the patches you have
> provided.  Particularly, the "bmc" in the name from patch 1 makes one
> think that it's only a bmc.
>
> The "bmc" name is also a little confusing; the devices with a "bmc" in
> the name are all the BMC side, but you are doing a host side device.
>
> If you look at most of the other MFDs, they have a "core" section then
> various other parts that use the core.  And possibly parts in other
> directories for individual functions.  I think you need to do the same
> design.  Have a "core" section that both the display and bmc use, then a
> separate display and bmc driver.

If it can be reconstructed in this way, it should be clearer.

However, there is a key point in my mind: if the display and IPMI are
separated into two parts, they should at least be able to be probed
separately, but in fact they belong to the same PCI-E device, just
allocated different resource intervals.

static struct pci_device_id ls2k_bmc_devices[] = {
       { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x1a05) },
       { }
};
MODULE_DEVICE_TABLE(pci, ls2k_bmc_devices);

I'm not sure if my understanding is correct?

>
> That way, you can use the display without the IPMI interface, or the
> IPMI interface without the display.
>
> I would like to see:
>
> * A core mfd device named ls2k-core.c that has the core functions.
>   It would have its own config item (MFD_LS2K) that would be
>   selected if either the display or IPMI device is enabled.
>
> * A separate display device in its own file with its own config item.
>   This isn't my area, so I'm not sure where this should go.
>
> * The IPMI device in the ipmi directory named ipmi_ls2k.c, again
>   with it's own config item (IPMI_LS2K).
>
> Does that make sense?  I don't want to make things too hard, but that's
> the way pretty much everything else is done on MFDs.
>
> Thanks,
>
> -corey
>
> >
> > >
> > > If nothing else is going to use this, then it's really not a
> > > multi-function device and all the code can go into the IPMI directory.
> > > That simplifies maintenance.
> > >
> > > If it is a multi-function device, then I want two separate Kconfig
> > > items, one for the MFD and one for the IPMI portion.  That way it's
> > > ready and you don't have to bother about the IPMI portion when
> > > adding the other device.
> > >
> > > All else looks good, I think.
> > >
> > > -corey
> > >
> > > >
> > > > For IPMI, according to the existing design, we use software simulation to
> > > > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> > > >
> > > > Also since both host side and BMC side read and write kcs status, we use
> > > > fifo pointer to ensure data consistency.
> > > >
> > > > For the display, based on simpledrm, the resolution is read from a fixed
> > > > position in the BMC since the hardware does not support auto-detection
> > > > of the resolution. Of course, we will try to support multiple
> > > > resolutions later, through a vbios-like approach.
> > > >
> > > > Especially, for the BMC reset function, since the display will be
> > > > disconnected when BMC reset, we made a special treatment of re-push.
> > > >
> > > > Based on this, I will present it in four patches:
> > > > patch-1: BMC device PCI resource allocation.
> > > > patch-2: BMC reset function support
> > > > patch-3: IPMI implementation
> > > >
> > > > Thanks.
> > > >
> > > > -------
> > > > V4:
> > > > - Add Reviewed-by tag;
> > > > - Change the order of the patches.
> > > > Patch (1/3):
> > > >   - Fix build warning by lkp: Kconfig tristate -> bool
> > > >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> > > >  - Update commit message;
> > > >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> > > >    Makefile.
> > > > Patch (2/3):
> > > >   - Remove unnecessary newlines;
> > > >   - Rename ls2k_bmc_check_pcie_connected() to
> > > >     ls2k_bmc_pcie_is_connected();
> > > >   - Update comment message.
> > > > Patch (3/3):
> > > >   - Remove unnecessary newlines.
> > > >
> > > > Link to V3:
> > > > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> > > >
> > > > V3:
> > > > Patch (1/3):
> > > >  - Drop "MFD" in title and comment;
> > > >  - Fromatting code;
> > > >  - Add clearer comments.
> > > > Patch (2/3):
> > > >  - Rebase linux-ipmi/next tree;
> > > >  - Use readx()/writex() to read and write IPMI data instead of structure
> > > >    pointer references;
> > > >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> > > >  - Drop unused output.
> > > > Patch (3/3):
> > > >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> > > >  - Add clearer comments.
> > > >  - Use proper multi-line commentary as per the Coding Style documentation;
> > > >  - Define all magic numbers.
> > > >
> > > > Link to V2:
> > > > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> > > >
> > > > V2:
> > > > - Drop ls2kdrm, use simpledrm instead.
> > > > Patch (1/3):
> > > >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> > > >  - Add resolution fetching due to replacing the original display
> > > >    solution with simpledrm;
> > > >  - Add aperture_remove_conflicting_devices() to avoid efifb
> > > >    conflict with simpledrm.
> > > > Patch (3/3):
> > > >  - This part of the function, moved from the original ls2kdrm to mfd;
> > > >  - Use set_console to implement the Re-push display function.
> > > >
> > > > Link to V1:
> > > > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> > > >
> > > > Binbin Zhou (3):
> > > >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> > > >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> > > >   ipmi: Add Loongson-2K BMC support
> > > >
> > > >  drivers/char/ipmi/Makefile       |   1 +
> > > >  drivers/char/ipmi/ipmi_si.h      |   7 +
> > > >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> > > >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> > > >  drivers/mfd/Kconfig              |  12 +
> > > >  drivers/mfd/Makefile             |   2 +
> > > >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> > > >  7 files changed, 699 insertions(+)
> > > >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> > > >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> > > >
> > > >
> > > > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > > > --
> > > > 2.47.1
> > > >
> >
> >
> > --
> > Thanks.
> > Binbin

-- 
Thanks.
Binbin
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Corey Minyard 3 months, 3 weeks ago
On Sat, Jun 14, 2025 at 01:25:17PM +0800, Binbin Zhou wrote:
> Hi Corey:
> 
> Thanks for your detailed suggestions.
> 
> On Sat, Jun 14, 2025 at 11:20 AM Corey Minyard <corey@minyard.net> wrote:
> >
> > On Sat, Jun 14, 2025 at 10:50:37AM +0800, Binbin Zhou wrote:
> > > Hi Corey:
> > >
> > > On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
> > > >
> > > > On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > > > > Hi all:
> > > > >
> > > > > This patch set introduces the Loongson-2K BMC.
> > > > >
> > > > > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > > > > And it is a multifunctional device (MFD), such as display as a sub-function
> > > > > of it.
> > > >
> > > > I've asked this before, but I haven't gotten a answer, I don't think.
> > > >
> > > > Is this really a multi-function device?  Is there (or will there be)
> > > > another driver that uses the MFD code?
> > >
> > > I am very sorry, I may have overlooked your previous question.
> > >
> > > And I also may not have a thorough understanding of multifunction devices.
> > >
> > > The Loongson-2K BMC device provides two functions: display and IPMI.
> > > For display, we pass the simplefb_platform_data parameter and register
> > > the simpledrm device, as shown in patch-1.
> > >
> > > Therefore, I think this should be considered a multifunction device.
> >
> > Ok, that's clear, thank you.
> >
> > However, that's not really very clear from the patches you have
> > provided.  Particularly, the "bmc" in the name from patch 1 makes one
> > think that it's only a bmc.
> >
> > The "bmc" name is also a little confusing; the devices with a "bmc" in
> > the name are all the BMC side, but you are doing a host side device.
> >
> > If you look at most of the other MFDs, they have a "core" section then
> > various other parts that use the core.  And possibly parts in other
> > directories for individual functions.  I think you need to do the same
> > design.  Have a "core" section that both the display and bmc use, then a
> > separate display and bmc driver.
> 
> If it can be reconstructed in this way, it should be clearer.
> 
> However, there is a key point in my mind: if the display and IPMI are
> separated into two parts, they should at least be able to be probed
> separately, but in fact they belong to the same PCI-E device, just
> allocated different resource intervals.
> 
> static struct pci_device_id ls2k_bmc_devices[] = {
>        { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x1a05) },
>        { }
> };
> MODULE_DEVICE_TABLE(pci, ls2k_bmc_devices);
> 
> I'm not sure if my understanding is correct?

You are already doing this, it appears.  I spent a little time to learn
how this works.  You are using the standard frame buffer driver, so no
special driver is required there (per earlier discussions).  And you are
registering it all as an MFD device, so the display buffer and IPMI
interface will pick it up there.

So from a design point of view this all looks good.

The IPMI interface needs to be separately selectable from the main mfd
device in the Kconfigs.  Add an IPMI_LS2K config in the IPMI section
that enables the IPMI interface and selects MFD_LS2K_BMC.  And both
configs need to be tristate, not bool, so they can be modules.

I don't know if you want to make the display part so it can be enabled
separately, I'm not sure how you would do that.  But that's not my
concern, really.

Thanks,

-corey

> 
> >
> > That way, you can use the display without the IPMI interface, or the
> > IPMI interface without the display.
> >
> > I would like to see:
> >
> > * A core mfd device named ls2k-core.c that has the core functions.
> >   It would have its own config item (MFD_LS2K) that would be
> >   selected if either the display or IPMI device is enabled.
> >
> > * A separate display device in its own file with its own config item.
> >   This isn't my area, so I'm not sure where this should go.
> >
> > * The IPMI device in the ipmi directory named ipmi_ls2k.c, again
> >   with it's own config item (IPMI_LS2K).
> >
> > Does that make sense?  I don't want to make things too hard, but that's
> > the way pretty much everything else is done on MFDs.
> >
> > Thanks,
> >
> > -corey
> >
> > >
> > > >
> > > > If nothing else is going to use this, then it's really not a
> > > > multi-function device and all the code can go into the IPMI directory.
> > > > That simplifies maintenance.
> > > >
> > > > If it is a multi-function device, then I want two separate Kconfig
> > > > items, one for the MFD and one for the IPMI portion.  That way it's
> > > > ready and you don't have to bother about the IPMI portion when
> > > > adding the other device.
> > > >
> > > > All else looks good, I think.
> > > >
> > > > -corey
> > > >
> > > > >
> > > > > For IPMI, according to the existing design, we use software simulation to
> > > > > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> > > > >
> > > > > Also since both host side and BMC side read and write kcs status, we use
> > > > > fifo pointer to ensure data consistency.
> > > > >
> > > > > For the display, based on simpledrm, the resolution is read from a fixed
> > > > > position in the BMC since the hardware does not support auto-detection
> > > > > of the resolution. Of course, we will try to support multiple
> > > > > resolutions later, through a vbios-like approach.
> > > > >
> > > > > Especially, for the BMC reset function, since the display will be
> > > > > disconnected when BMC reset, we made a special treatment of re-push.
> > > > >
> > > > > Based on this, I will present it in four patches:
> > > > > patch-1: BMC device PCI resource allocation.
> > > > > patch-2: BMC reset function support
> > > > > patch-3: IPMI implementation
> > > > >
> > > > > Thanks.
> > > > >
> > > > > -------
> > > > > V4:
> > > > > - Add Reviewed-by tag;
> > > > > - Change the order of the patches.
> > > > > Patch (1/3):
> > > > >   - Fix build warning by lkp: Kconfig tristate -> bool
> > > > >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> > > > >  - Update commit message;
> > > > >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> > > > >    Makefile.
> > > > > Patch (2/3):
> > > > >   - Remove unnecessary newlines;
> > > > >   - Rename ls2k_bmc_check_pcie_connected() to
> > > > >     ls2k_bmc_pcie_is_connected();
> > > > >   - Update comment message.
> > > > > Patch (3/3):
> > > > >   - Remove unnecessary newlines.
> > > > >
> > > > > Link to V3:
> > > > > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> > > > >
> > > > > V3:
> > > > > Patch (1/3):
> > > > >  - Drop "MFD" in title and comment;
> > > > >  - Fromatting code;
> > > > >  - Add clearer comments.
> > > > > Patch (2/3):
> > > > >  - Rebase linux-ipmi/next tree;
> > > > >  - Use readx()/writex() to read and write IPMI data instead of structure
> > > > >    pointer references;
> > > > >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> > > > >  - Drop unused output.
> > > > > Patch (3/3):
> > > > >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> > > > >  - Add clearer comments.
> > > > >  - Use proper multi-line commentary as per the Coding Style documentation;
> > > > >  - Define all magic numbers.
> > > > >
> > > > > Link to V2:
> > > > > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> > > > >
> > > > > V2:
> > > > > - Drop ls2kdrm, use simpledrm instead.
> > > > > Patch (1/3):
> > > > >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> > > > >  - Add resolution fetching due to replacing the original display
> > > > >    solution with simpledrm;
> > > > >  - Add aperture_remove_conflicting_devices() to avoid efifb
> > > > >    conflict with simpledrm.
> > > > > Patch (3/3):
> > > > >  - This part of the function, moved from the original ls2kdrm to mfd;
> > > > >  - Use set_console to implement the Re-push display function.
> > > > >
> > > > > Link to V1:
> > > > > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> > > > >
> > > > > Binbin Zhou (3):
> > > > >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> > > > >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> > > > >   ipmi: Add Loongson-2K BMC support
> > > > >
> > > > >  drivers/char/ipmi/Makefile       |   1 +
> > > > >  drivers/char/ipmi/ipmi_si.h      |   7 +
> > > > >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> > > > >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> > > > >  drivers/mfd/Kconfig              |  12 +
> > > > >  drivers/mfd/Makefile             |   2 +
> > > > >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> > > > >  7 files changed, 699 insertions(+)
> > > > >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> > > > >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> > > > >
> > > > >
> > > > > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > > > > --
> > > > > 2.47.1
> > > > >
> > >
> > >
> > > --
> > > Thanks.
> > > Binbin
> 
> -- 
> Thanks.
> Binbin
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Binbin Zhou 3 months, 3 weeks ago
Hi Corey:

Thanks  for your detailed review.

On Sat, Jun 14, 2025 at 9:59 PM Corey Minyard <corey@minyard.net> wrote:
>
> On Sat, Jun 14, 2025 at 01:25:17PM +0800, Binbin Zhou wrote:
> > Hi Corey:
> >
> > Thanks for your detailed suggestions.
> >
> > On Sat, Jun 14, 2025 at 11:20 AM Corey Minyard <corey@minyard.net> wrote:
> > >
> > > On Sat, Jun 14, 2025 at 10:50:37AM +0800, Binbin Zhou wrote:
> > > > Hi Corey:
> > > >
> > > > On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
> > > > >
> > > > > On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > > > > > Hi all:
> > > > > >
> > > > > > This patch set introduces the Loongson-2K BMC.
> > > > > >
> > > > > > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > > > > > And it is a multifunctional device (MFD), such as display as a sub-function
> > > > > > of it.
> > > > >
> > > > > I've asked this before, but I haven't gotten a answer, I don't think.
> > > > >
> > > > > Is this really a multi-function device?  Is there (or will there be)
> > > > > another driver that uses the MFD code?
> > > >
> > > > I am very sorry, I may have overlooked your previous question.
> > > >
> > > > And I also may not have a thorough understanding of multifunction devices.
> > > >
> > > > The Loongson-2K BMC device provides two functions: display and IPMI.
> > > > For display, we pass the simplefb_platform_data parameter and register
> > > > the simpledrm device, as shown in patch-1.
> > > >
> > > > Therefore, I think this should be considered a multifunction device.
> > >
> > > Ok, that's clear, thank you.
> > >
> > > However, that's not really very clear from the patches you have
> > > provided.  Particularly, the "bmc" in the name from patch 1 makes one
> > > think that it's only a bmc.
> > >
> > > The "bmc" name is also a little confusing; the devices with a "bmc" in
> > > the name are all the BMC side, but you are doing a host side device.
> > >
> > > If you look at most of the other MFDs, they have a "core" section then
> > > various other parts that use the core.  And possibly parts in other
> > > directories for individual functions.  I think you need to do the same
> > > design.  Have a "core" section that both the display and bmc use, then a
> > > separate display and bmc driver.
> >
> > If it can be reconstructed in this way, it should be clearer.
> >
> > However, there is a key point in my mind: if the display and IPMI are
> > separated into two parts, they should at least be able to be probed
> > separately, but in fact they belong to the same PCI-E device, just
> > allocated different resource intervals.
> >
> > static struct pci_device_id ls2k_bmc_devices[] = {
> >        { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x1a05) },
> >        { }
> > };
> > MODULE_DEVICE_TABLE(pci, ls2k_bmc_devices);
> >
> > I'm not sure if my understanding is correct?
>
> You are already doing this, it appears.  I spent a little time to learn
> how this works.  You are using the standard frame buffer driver, so no
> special driver is required there (per earlier discussions).  And you are
> registering it all as an MFD device, so the display buffer and IPMI
> interface will pick it up there.
>
> So from a design point of view this all looks good.
>
> The IPMI interface needs to be separately selectable from the main mfd
> device in the Kconfigs.  Add an IPMI_LS2K config in the IPMI section
> that enables the IPMI interface and selects MFD_LS2K_BMC.  And both
> configs need to be tristate, not bool, so they can be modules.

I tried rewriting Kconfig as follows:

IPMI Kconfig:

config IPMI_LS2K
       bool 'Loongson-2K IPMI interface'
       depends on LOONGARCH
       select MFD_LS2K_BMC_CORE
       help
         Provides a driver for Loongson-2K IPMI interfaces.

MFD Kconfig:

config MFD_LS2K_BMC_CORE
        bool "Loongson-2K Board Management Controller Support"
        select MFD_CORE


However, `tristate` does not seem to work.
On the IPMI side, in order to better reuse code, our driver is not
actually a completely independent driver; it is part of `ipmi_si`.

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7fe891783a37..c13d5132fffc 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2120,6 +2120,8 @@ static int __init init_ipmi_si(void)

        ipmi_si_pci_init();

+       ipmi_si_ls2k_init();
+
        ipmi_si_parisc_init();

        mutex_lock(&smi_infos_lock);
@@ -2334,6 +2335,8 @@ static void cleanup_ipmi_si(void)

        ipmi_si_pci_shutdown();

+       ipmi_si_ls2k_shutdown();
+
        ipmi_si_parisc_shutdown();

        ipmi_si_platform_shutdown();


Therefore, it seems that we can only use `bool` here, otherwise an
error will occur during compilation, as seen in the V3 patchset[1]:

[1]: https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/

>
> I don't know if you want to make the display part so it can be enabled
> separately, I'm not sure how you would do that.  But that's not my
> concern, really.
>
> Thanks,
>
> -corey
>
> >
> > >
> > > That way, you can use the display without the IPMI interface, or the
> > > IPMI interface without the display.
> > >
> > > I would like to see:
> > >
> > > * A core mfd device named ls2k-core.c that has the core functions.
> > >   It would have its own config item (MFD_LS2K) that would be
> > >   selected if either the display or IPMI device is enabled.
> > >
> > > * A separate display device in its own file with its own config item.
> > >   This isn't my area, so I'm not sure where this should go.
> > >
> > > * The IPMI device in the ipmi directory named ipmi_ls2k.c, again
> > >   with it's own config item (IPMI_LS2K).
> > >
> > > Does that make sense?  I don't want to make things too hard, but that's
> > > the way pretty much everything else is done on MFDs.
> > >
> > > Thanks,
> > >
> > > -corey
> > >
> > > >
> > > > >
> > > > > If nothing else is going to use this, then it's really not a
> > > > > multi-function device and all the code can go into the IPMI directory.
> > > > > That simplifies maintenance.
> > > > >
> > > > > If it is a multi-function device, then I want two separate Kconfig
> > > > > items, one for the MFD and one for the IPMI portion.  That way it's
> > > > > ready and you don't have to bother about the IPMI portion when
> > > > > adding the other device.
> > > > >
> > > > > All else looks good, I think.
> > > > >
> > > > > -corey
> > > > >
> > > > > >
> > > > > > For IPMI, according to the existing design, we use software simulation to
> > > > > > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> > > > > >
> > > > > > Also since both host side and BMC side read and write kcs status, we use
> > > > > > fifo pointer to ensure data consistency.
> > > > > >
> > > > > > For the display, based on simpledrm, the resolution is read from a fixed
> > > > > > position in the BMC since the hardware does not support auto-detection
> > > > > > of the resolution. Of course, we will try to support multiple
> > > > > > resolutions later, through a vbios-like approach.
> > > > > >
> > > > > > Especially, for the BMC reset function, since the display will be
> > > > > > disconnected when BMC reset, we made a special treatment of re-push.
> > > > > >
> > > > > > Based on this, I will present it in four patches:
> > > > > > patch-1: BMC device PCI resource allocation.
> > > > > > patch-2: BMC reset function support
> > > > > > patch-3: IPMI implementation
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > -------
> > > > > > V4:
> > > > > > - Add Reviewed-by tag;
> > > > > > - Change the order of the patches.
> > > > > > Patch (1/3):
> > > > > >   - Fix build warning by lkp: Kconfig tristate -> bool
> > > > > >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> > > > > >  - Update commit message;
> > > > > >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> > > > > >    Makefile.
> > > > > > Patch (2/3):
> > > > > >   - Remove unnecessary newlines;
> > > > > >   - Rename ls2k_bmc_check_pcie_connected() to
> > > > > >     ls2k_bmc_pcie_is_connected();
> > > > > >   - Update comment message.
> > > > > > Patch (3/3):
> > > > > >   - Remove unnecessary newlines.
> > > > > >
> > > > > > Link to V3:
> > > > > > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> > > > > >
> > > > > > V3:
> > > > > > Patch (1/3):
> > > > > >  - Drop "MFD" in title and comment;
> > > > > >  - Fromatting code;
> > > > > >  - Add clearer comments.
> > > > > > Patch (2/3):
> > > > > >  - Rebase linux-ipmi/next tree;
> > > > > >  - Use readx()/writex() to read and write IPMI data instead of structure
> > > > > >    pointer references;
> > > > > >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> > > > > >  - Drop unused output.
> > > > > > Patch (3/3):
> > > > > >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> > > > > >  - Add clearer comments.
> > > > > >  - Use proper multi-line commentary as per the Coding Style documentation;
> > > > > >  - Define all magic numbers.
> > > > > >
> > > > > > Link to V2:
> > > > > > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> > > > > >
> > > > > > V2:
> > > > > > - Drop ls2kdrm, use simpledrm instead.
> > > > > > Patch (1/3):
> > > > > >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> > > > > >  - Add resolution fetching due to replacing the original display
> > > > > >    solution with simpledrm;
> > > > > >  - Add aperture_remove_conflicting_devices() to avoid efifb
> > > > > >    conflict with simpledrm.
> > > > > > Patch (3/3):
> > > > > >  - This part of the function, moved from the original ls2kdrm to mfd;
> > > > > >  - Use set_console to implement the Re-push display function.
> > > > > >
> > > > > > Link to V1:
> > > > > > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> > > > > >
> > > > > > Binbin Zhou (3):
> > > > > >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> > > > > >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> > > > > >   ipmi: Add Loongson-2K BMC support
> > > > > >
> > > > > >  drivers/char/ipmi/Makefile       |   1 +
> > > > > >  drivers/char/ipmi/ipmi_si.h      |   7 +
> > > > > >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> > > > > >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> > > > > >  drivers/mfd/Kconfig              |  12 +
> > > > > >  drivers/mfd/Makefile             |   2 +
> > > > > >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> > > > > >  7 files changed, 699 insertions(+)
> > > > > >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> > > > > >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> > > > > >
> > > > > >
> > > > > > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > > > > > --
> > > > > > 2.47.1
> > > > > >
> > > >
> > > >
> > > > --
> > > > Thanks.
> > > > Binbin
> >
> > --
> > Thanks.
> > Binbin

-- 
Thanks.
Binbin
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Corey Minyard 3 months, 3 weeks ago
On Mon, Jun 16, 2025 at 03:35:56PM +0800, Binbin Zhou wrote:
> Hi Corey:
> 
> Thanks  for your detailed review.
> 
> On Sat, Jun 14, 2025 at 9:59 PM Corey Minyard <corey@minyard.net> wrote:
> >
> > On Sat, Jun 14, 2025 at 01:25:17PM +0800, Binbin Zhou wrote:
> > > Hi Corey:
> > >
> > > Thanks for your detailed suggestions.
> > >
> > > On Sat, Jun 14, 2025 at 11:20 AM Corey Minyard <corey@minyard.net> wrote:
> > > >
> > > > On Sat, Jun 14, 2025 at 10:50:37AM +0800, Binbin Zhou wrote:
> > > > > Hi Corey:
> > > > >
> > > > > On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
> > > > > >
> > > > > > On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > > > > > > Hi all:
> > > > > > >
> > > > > > > This patch set introduces the Loongson-2K BMC.
> > > > > > >
> > > > > > > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > > > > > > And it is a multifunctional device (MFD), such as display as a sub-function
> > > > > > > of it.
> > > > > >
> > > > > > I've asked this before, but I haven't gotten a answer, I don't think.
> > > > > >
> > > > > > Is this really a multi-function device?  Is there (or will there be)
> > > > > > another driver that uses the MFD code?
> > > > >
> > > > > I am very sorry, I may have overlooked your previous question.
> > > > >
> > > > > And I also may not have a thorough understanding of multifunction devices.
> > > > >
> > > > > The Loongson-2K BMC device provides two functions: display and IPMI.
> > > > > For display, we pass the simplefb_platform_data parameter and register
> > > > > the simpledrm device, as shown in patch-1.
> > > > >
> > > > > Therefore, I think this should be considered a multifunction device.
> > > >
> > > > Ok, that's clear, thank you.
> > > >
> > > > However, that's not really very clear from the patches you have
> > > > provided.  Particularly, the "bmc" in the name from patch 1 makes one
> > > > think that it's only a bmc.
> > > >
> > > > The "bmc" name is also a little confusing; the devices with a "bmc" in
> > > > the name are all the BMC side, but you are doing a host side device.
> > > >
> > > > If you look at most of the other MFDs, they have a "core" section then
> > > > various other parts that use the core.  And possibly parts in other
> > > > directories for individual functions.  I think you need to do the same
> > > > design.  Have a "core" section that both the display and bmc use, then a
> > > > separate display and bmc driver.
> > >
> > > If it can be reconstructed in this way, it should be clearer.
> > >
> > > However, there is a key point in my mind: if the display and IPMI are
> > > separated into two parts, they should at least be able to be probed
> > > separately, but in fact they belong to the same PCI-E device, just
> > > allocated different resource intervals.
> > >
> > > static struct pci_device_id ls2k_bmc_devices[] = {
> > >        { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x1a05) },
> > >        { }
> > > };
> > > MODULE_DEVICE_TABLE(pci, ls2k_bmc_devices);
> > >
> > > I'm not sure if my understanding is correct?
> >
> > You are already doing this, it appears.  I spent a little time to learn
> > how this works.  You are using the standard frame buffer driver, so no
> > special driver is required there (per earlier discussions).  And you are
> > registering it all as an MFD device, so the display buffer and IPMI
> > interface will pick it up there.
> >
> > So from a design point of view this all looks good.
> >
> > The IPMI interface needs to be separately selectable from the main mfd
> > device in the Kconfigs.  Add an IPMI_LS2K config in the IPMI section
> > that enables the IPMI interface and selects MFD_LS2K_BMC.  And both
> > configs need to be tristate, not bool, so they can be modules.
> 
> I tried rewriting Kconfig as follows:
> 
> IPMI Kconfig:
> 
> config IPMI_LS2K
>        bool 'Loongson-2K IPMI interface'
>        depends on LOONGARCH
>        select MFD_LS2K_BMC_CORE
>        help
>          Provides a driver for Loongson-2K IPMI interfaces.
> 
> MFD Kconfig:
> 
> config MFD_LS2K_BMC_CORE
>         bool "Loongson-2K Board Management Controller Support"
>         select MFD_CORE
> 
> 
> However, `tristate` does not seem to work.
> On the IPMI side, in order to better reuse code, our driver is not
> actually a completely independent driver; it is part of `ipmi_si`.

Ah, yes, that is true.  The trouble with the above is it will select
MFD_LS2K_BMC_CORE as "y" even if ipmi_si is a module.  And that will
force MFD_CORE to be "y" as well.  At least I think it works that way.
Anyway, that's not terrible, but it would be nice to have the core code
as a module if possible.

Another issue is you don't have help text on MFD_LS2K_BMC_CORE code.
You probably want to mention there that it enables the display on
the BMC.

-corey

> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7fe891783a37..c13d5132fffc 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2120,6 +2120,8 @@ static int __init init_ipmi_si(void)
> 
>         ipmi_si_pci_init();
> 
> +       ipmi_si_ls2k_init();
> +
>         ipmi_si_parisc_init();
> 
>         mutex_lock(&smi_infos_lock);
> @@ -2334,6 +2335,8 @@ static void cleanup_ipmi_si(void)
> 
>         ipmi_si_pci_shutdown();
> 
> +       ipmi_si_ls2k_shutdown();
> +
>         ipmi_si_parisc_shutdown();
> 
>         ipmi_si_platform_shutdown();
> 
> 
> Therefore, it seems that we can only use `bool` here, otherwise an
> error will occur during compilation, as seen in the V3 patchset[1]:
> 
> [1]: https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> 
> >
> > I don't know if you want to make the display part so it can be enabled
> > separately, I'm not sure how you would do that.  But that's not my
> > concern, really.
> >
> > Thanks,
> >
> > -corey
> >
> > >
> > > >
> > > > That way, you can use the display without the IPMI interface, or the
> > > > IPMI interface without the display.
> > > >
> > > > I would like to see:
> > > >
> > > > * A core mfd device named ls2k-core.c that has the core functions.
> > > >   It would have its own config item (MFD_LS2K) that would be
> > > >   selected if either the display or IPMI device is enabled.
> > > >
> > > > * A separate display device in its own file with its own config item.
> > > >   This isn't my area, so I'm not sure where this should go.
> > > >
> > > > * The IPMI device in the ipmi directory named ipmi_ls2k.c, again
> > > >   with it's own config item (IPMI_LS2K).
> > > >
> > > > Does that make sense?  I don't want to make things too hard, but that's
> > > > the way pretty much everything else is done on MFDs.
> > > >
> > > > Thanks,
> > > >
> > > > -corey
> > > >
> > > > >
> > > > > >
> > > > > > If nothing else is going to use this, then it's really not a
> > > > > > multi-function device and all the code can go into the IPMI directory.
> > > > > > That simplifies maintenance.
> > > > > >
> > > > > > If it is a multi-function device, then I want two separate Kconfig
> > > > > > items, one for the MFD and one for the IPMI portion.  That way it's
> > > > > > ready and you don't have to bother about the IPMI portion when
> > > > > > adding the other device.
> > > > > >
> > > > > > All else looks good, I think.
> > > > > >
> > > > > > -corey
> > > > > >
> > > > > > >
> > > > > > > For IPMI, according to the existing design, we use software simulation to
> > > > > > > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> > > > > > >
> > > > > > > Also since both host side and BMC side read and write kcs status, we use
> > > > > > > fifo pointer to ensure data consistency.
> > > > > > >
> > > > > > > For the display, based on simpledrm, the resolution is read from a fixed
> > > > > > > position in the BMC since the hardware does not support auto-detection
> > > > > > > of the resolution. Of course, we will try to support multiple
> > > > > > > resolutions later, through a vbios-like approach.
> > > > > > >
> > > > > > > Especially, for the BMC reset function, since the display will be
> > > > > > > disconnected when BMC reset, we made a special treatment of re-push.
> > > > > > >
> > > > > > > Based on this, I will present it in four patches:
> > > > > > > patch-1: BMC device PCI resource allocation.
> > > > > > > patch-2: BMC reset function support
> > > > > > > patch-3: IPMI implementation
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > > -------
> > > > > > > V4:
> > > > > > > - Add Reviewed-by tag;
> > > > > > > - Change the order of the patches.
> > > > > > > Patch (1/3):
> > > > > > >   - Fix build warning by lkp: Kconfig tristate -> bool
> > > > > > >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> > > > > > >  - Update commit message;
> > > > > > >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> > > > > > >    Makefile.
> > > > > > > Patch (2/3):
> > > > > > >   - Remove unnecessary newlines;
> > > > > > >   - Rename ls2k_bmc_check_pcie_connected() to
> > > > > > >     ls2k_bmc_pcie_is_connected();
> > > > > > >   - Update comment message.
> > > > > > > Patch (3/3):
> > > > > > >   - Remove unnecessary newlines.
> > > > > > >
> > > > > > > Link to V3:
> > > > > > > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> > > > > > >
> > > > > > > V3:
> > > > > > > Patch (1/3):
> > > > > > >  - Drop "MFD" in title and comment;
> > > > > > >  - Fromatting code;
> > > > > > >  - Add clearer comments.
> > > > > > > Patch (2/3):
> > > > > > >  - Rebase linux-ipmi/next tree;
> > > > > > >  - Use readx()/writex() to read and write IPMI data instead of structure
> > > > > > >    pointer references;
> > > > > > >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> > > > > > >  - Drop unused output.
> > > > > > > Patch (3/3):
> > > > > > >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> > > > > > >  - Add clearer comments.
> > > > > > >  - Use proper multi-line commentary as per the Coding Style documentation;
> > > > > > >  - Define all magic numbers.
> > > > > > >
> > > > > > > Link to V2:
> > > > > > > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> > > > > > >
> > > > > > > V2:
> > > > > > > - Drop ls2kdrm, use simpledrm instead.
> > > > > > > Patch (1/3):
> > > > > > >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> > > > > > >  - Add resolution fetching due to replacing the original display
> > > > > > >    solution with simpledrm;
> > > > > > >  - Add aperture_remove_conflicting_devices() to avoid efifb
> > > > > > >    conflict with simpledrm.
> > > > > > > Patch (3/3):
> > > > > > >  - This part of the function, moved from the original ls2kdrm to mfd;
> > > > > > >  - Use set_console to implement the Re-push display function.
> > > > > > >
> > > > > > > Link to V1:
> > > > > > > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> > > > > > >
> > > > > > > Binbin Zhou (3):
> > > > > > >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> > > > > > >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> > > > > > >   ipmi: Add Loongson-2K BMC support
> > > > > > >
> > > > > > >  drivers/char/ipmi/Makefile       |   1 +
> > > > > > >  drivers/char/ipmi/ipmi_si.h      |   7 +
> > > > > > >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> > > > > > >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> > > > > > >  drivers/mfd/Kconfig              |  12 +
> > > > > > >  drivers/mfd/Makefile             |   2 +
> > > > > > >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> > > > > > >  7 files changed, 699 insertions(+)
> > > > > > >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> > > > > > >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> > > > > > >
> > > > > > >
> > > > > > > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > > > > > > --
> > > > > > > 2.47.1
> > > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks.
> > > > > Binbin
> > >
> > > --
> > > Thanks.
> > > Binbin
> 
> -- 
> Thanks.
> Binbin
Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support
Posted by Binbin Zhou 3 months, 3 weeks ago
On Mon, Jun 16, 2025 at 9:16 PM Corey Minyard <corey@minyard.net> wrote:
>
> On Mon, Jun 16, 2025 at 03:35:56PM +0800, Binbin Zhou wrote:
> > Hi Corey:
> >
> > Thanks  for your detailed review.
> >
> > On Sat, Jun 14, 2025 at 9:59 PM Corey Minyard <corey@minyard.net> wrote:
> > >
> > > On Sat, Jun 14, 2025 at 01:25:17PM +0800, Binbin Zhou wrote:
> > > > Hi Corey:
> > > >
> > > > Thanks for your detailed suggestions.
> > > >
> > > > On Sat, Jun 14, 2025 at 11:20 AM Corey Minyard <corey@minyard.net> wrote:
> > > > >
> > > > > On Sat, Jun 14, 2025 at 10:50:37AM +0800, Binbin Zhou wrote:
> > > > > > Hi Corey:
> > > > > >
> > > > > > On Sat, Jun 14, 2025 at 8:41 AM Corey Minyard <corey@minyard.net> wrote:
> > > > > > >
> > > > > > > On Fri, Jun 13, 2025 at 02:43:38PM +0800, Binbin Zhou wrote:
> > > > > > > > Hi all:
> > > > > > > >
> > > > > > > > This patch set introduces the Loongson-2K BMC.
> > > > > > > >
> > > > > > > > It is a PCIe device present on servers similar to the Loongson-3 CPUs.
> > > > > > > > And it is a multifunctional device (MFD), such as display as a sub-function
> > > > > > > > of it.
> > > > > > >
> > > > > > > I've asked this before, but I haven't gotten a answer, I don't think.
> > > > > > >
> > > > > > > Is this really a multi-function device?  Is there (or will there be)
> > > > > > > another driver that uses the MFD code?
> > > > > >
> > > > > > I am very sorry, I may have overlooked your previous question.
> > > > > >
> > > > > > And I also may not have a thorough understanding of multifunction devices.
> > > > > >
> > > > > > The Loongson-2K BMC device provides two functions: display and IPMI.
> > > > > > For display, we pass the simplefb_platform_data parameter and register
> > > > > > the simpledrm device, as shown in patch-1.
> > > > > >
> > > > > > Therefore, I think this should be considered a multifunction device.
> > > > >
> > > > > Ok, that's clear, thank you.
> > > > >
> > > > > However, that's not really very clear from the patches you have
> > > > > provided.  Particularly, the "bmc" in the name from patch 1 makes one
> > > > > think that it's only a bmc.
> > > > >
> > > > > The "bmc" name is also a little confusing; the devices with a "bmc" in
> > > > > the name are all the BMC side, but you are doing a host side device.
> > > > >
> > > > > If you look at most of the other MFDs, they have a "core" section then
> > > > > various other parts that use the core.  And possibly parts in other
> > > > > directories for individual functions.  I think you need to do the same
> > > > > design.  Have a "core" section that both the display and bmc use, then a
> > > > > separate display and bmc driver.
> > > >
> > > > If it can be reconstructed in this way, it should be clearer.
> > > >
> > > > However, there is a key point in my mind: if the display and IPMI are
> > > > separated into two parts, they should at least be able to be probed
> > > > separately, but in fact they belong to the same PCI-E device, just
> > > > allocated different resource intervals.
> > > >
> > > > static struct pci_device_id ls2k_bmc_devices[] = {
> > > >        { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x1a05) },
> > > >        { }
> > > > };
> > > > MODULE_DEVICE_TABLE(pci, ls2k_bmc_devices);
> > > >
> > > > I'm not sure if my understanding is correct?
> > >
> > > You are already doing this, it appears.  I spent a little time to learn
> > > how this works.  You are using the standard frame buffer driver, so no
> > > special driver is required there (per earlier discussions).  And you are
> > > registering it all as an MFD device, so the display buffer and IPMI
> > > interface will pick it up there.
> > >
> > > So from a design point of view this all looks good.
> > >
> > > The IPMI interface needs to be separately selectable from the main mfd
> > > device in the Kconfigs.  Add an IPMI_LS2K config in the IPMI section
> > > that enables the IPMI interface and selects MFD_LS2K_BMC.  And both
> > > configs need to be tristate, not bool, so they can be modules.
> >
> > I tried rewriting Kconfig as follows:
> >
> > IPMI Kconfig:
> >
> > config IPMI_LS2K
> >        bool 'Loongson-2K IPMI interface'
> >        depends on LOONGARCH
> >        select MFD_LS2K_BMC_CORE
> >        help
> >          Provides a driver for Loongson-2K IPMI interfaces.
> >
> > MFD Kconfig:
> >
> > config MFD_LS2K_BMC_CORE
> >         bool "Loongson-2K Board Management Controller Support"
> >         select MFD_CORE
> >
> >
> > However, `tristate` does not seem to work.
> > On the IPMI side, in order to better reuse code, our driver is not
> > actually a completely independent driver; it is part of `ipmi_si`.
>
> Ah, yes, that is true.  The trouble with the above is it will select
> MFD_LS2K_BMC_CORE as "y" even if ipmi_si is a module.  And that will
> force MFD_CORE to be "y" as well.  At least I think it works that way.
> Anyway, that's not terrible, but it would be nice to have the core code
> as a module if possible.
>
> Another issue is you don't have help text on MFD_LS2K_BMC_CORE code.
> You probably want to mention there that it enables the display on
> the BMC.

Of course, I will add some information about BMC in the help text.

Finally, I would like to thanks you again for taking the time to
review my patch. I have benefited greatly from it.
>
> -corey
>
> >
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 7fe891783a37..c13d5132fffc 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -2120,6 +2120,8 @@ static int __init init_ipmi_si(void)
> >
> >         ipmi_si_pci_init();
> >
> > +       ipmi_si_ls2k_init();
> > +
> >         ipmi_si_parisc_init();
> >
> >         mutex_lock(&smi_infos_lock);
> > @@ -2334,6 +2335,8 @@ static void cleanup_ipmi_si(void)
> >
> >         ipmi_si_pci_shutdown();
> >
> > +       ipmi_si_ls2k_shutdown();
> > +
> >         ipmi_si_parisc_shutdown();
> >
> >         ipmi_si_platform_shutdown();
> >
> >
> > Therefore, it seems that we can only use `bool` here, otherwise an
> > error will occur during compilation, as seen in the V3 patchset[1]:
> >
> > [1]: https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> >
> > >
> > > I don't know if you want to make the display part so it can be enabled
> > > separately, I'm not sure how you would do that.  But that's not my
> > > concern, really.
> > >
> > > Thanks,
> > >
> > > -corey
> > >
> > > >
> > > > >
> > > > > That way, you can use the display without the IPMI interface, or the
> > > > > IPMI interface without the display.
> > > > >
> > > > > I would like to see:
> > > > >
> > > > > * A core mfd device named ls2k-core.c that has the core functions.
> > > > >   It would have its own config item (MFD_LS2K) that would be
> > > > >   selected if either the display or IPMI device is enabled.
> > > > >
> > > > > * A separate display device in its own file with its own config item.
> > > > >   This isn't my area, so I'm not sure where this should go.
> > > > >
> > > > > * The IPMI device in the ipmi directory named ipmi_ls2k.c, again
> > > > >   with it's own config item (IPMI_LS2K).
> > > > >
> > > > > Does that make sense?  I don't want to make things too hard, but that's
> > > > > the way pretty much everything else is done on MFDs.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > -corey
> > > > >
> > > > > >
> > > > > > >
> > > > > > > If nothing else is going to use this, then it's really not a
> > > > > > > multi-function device and all the code can go into the IPMI directory.
> > > > > > > That simplifies maintenance.
> > > > > > >
> > > > > > > If it is a multi-function device, then I want two separate Kconfig
> > > > > > > items, one for the MFD and one for the IPMI portion.  That way it's
> > > > > > > ready and you don't have to bother about the IPMI portion when
> > > > > > > adding the other device.
> > > > > > >
> > > > > > > All else looks good, I think.
> > > > > > >
> > > > > > > -corey
> > > > > > >
> > > > > > > >
> > > > > > > > For IPMI, according to the existing design, we use software simulation to
> > > > > > > > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
> > > > > > > >
> > > > > > > > Also since both host side and BMC side read and write kcs status, we use
> > > > > > > > fifo pointer to ensure data consistency.
> > > > > > > >
> > > > > > > > For the display, based on simpledrm, the resolution is read from a fixed
> > > > > > > > position in the BMC since the hardware does not support auto-detection
> > > > > > > > of the resolution. Of course, we will try to support multiple
> > > > > > > > resolutions later, through a vbios-like approach.
> > > > > > > >
> > > > > > > > Especially, for the BMC reset function, since the display will be
> > > > > > > > disconnected when BMC reset, we made a special treatment of re-push.
> > > > > > > >
> > > > > > > > Based on this, I will present it in four patches:
> > > > > > > > patch-1: BMC device PCI resource allocation.
> > > > > > > > patch-2: BMC reset function support
> > > > > > > > patch-3: IPMI implementation
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > > > -------
> > > > > > > > V4:
> > > > > > > > - Add Reviewed-by tag;
> > > > > > > > - Change the order of the patches.
> > > > > > > > Patch (1/3):
> > > > > > > >   - Fix build warning by lkp: Kconfig tristate -> bool
> > > > > > > >     - https://lore.kernel.org/all/202505312022.QmFmGE1F-lkp@intel.com/
> > > > > > > >  - Update commit message;
> > > > > > > >  - Move MFD_LS2K_BMC after MFD_INTEL_M10_BMC_PMCI in Kconfig and
> > > > > > > >    Makefile.
> > > > > > > > Patch (2/3):
> > > > > > > >   - Remove unnecessary newlines;
> > > > > > > >   - Rename ls2k_bmc_check_pcie_connected() to
> > > > > > > >     ls2k_bmc_pcie_is_connected();
> > > > > > > >   - Update comment message.
> > > > > > > > Patch (3/3):
> > > > > > > >   - Remove unnecessary newlines.
> > > > > > > >
> > > > > > > > Link to V3:
> > > > > > > > https://lore.kernel.org/all/cover.1748505446.git.zhoubinbin@loongson.cn/
> > > > > > > >
> > > > > > > > V3:
> > > > > > > > Patch (1/3):
> > > > > > > >  - Drop "MFD" in title and comment;
> > > > > > > >  - Fromatting code;
> > > > > > > >  - Add clearer comments.
> > > > > > > > Patch (2/3):
> > > > > > > >  - Rebase linux-ipmi/next tree;
> > > > > > > >  - Use readx()/writex() to read and write IPMI data instead of structure
> > > > > > > >    pointer references;
> > > > > > > >  - CONFIG_LOONGARCH -> MFD_LS2K_BMC;
> > > > > > > >  - Drop unused output.
> > > > > > > > Patch (3/3):
> > > > > > > >  - Inline the ls2k_bmc_gpio_reset_handler() function to ls2k_bmc_pdata_initial();
> > > > > > > >  - Add clearer comments.
> > > > > > > >  - Use proper multi-line commentary as per the Coding Style documentation;
> > > > > > > >  - Define all magic numbers.
> > > > > > > >
> > > > > > > > Link to V2:
> > > > > > > > https://lore.kernel.org/all/cover.1747276047.git.zhoubinbin@loongson.cn/
> > > > > > > >
> > > > > > > > V2:
> > > > > > > > - Drop ls2kdrm, use simpledrm instead.
> > > > > > > > Patch (1/3):
> > > > > > > >  - Use DEFINE_RES_MEM_NAMED/MFD_CELL_RES simplified code;
> > > > > > > >  - Add resolution fetching due to replacing the original display
> > > > > > > >    solution with simpledrm;
> > > > > > > >  - Add aperture_remove_conflicting_devices() to avoid efifb
> > > > > > > >    conflict with simpledrm.
> > > > > > > > Patch (3/3):
> > > > > > > >  - This part of the function, moved from the original ls2kdrm to mfd;
> > > > > > > >  - Use set_console to implement the Re-push display function.
> > > > > > > >
> > > > > > > > Link to V1:
> > > > > > > > https://lore.kernel.org/all/cover.1735550269.git.zhoubinbin@loongson.cn/
> > > > > > > >
> > > > > > > > Binbin Zhou (3):
> > > > > > > >   mfd: ls2kbmc: Introduce Loongson-2K BMC core driver
> > > > > > > >   mfd: ls2kbmc: Add Loongson-2K BMC reset function support
> > > > > > > >   ipmi: Add Loongson-2K BMC support
> > > > > > > >
> > > > > > > >  drivers/char/ipmi/Makefile       |   1 +
> > > > > > > >  drivers/char/ipmi/ipmi_si.h      |   7 +
> > > > > > > >  drivers/char/ipmi/ipmi_si_intf.c |   3 +
> > > > > > > >  drivers/char/ipmi/ipmi_si_ls2k.c | 189 ++++++++++++
> > > > > > > >  drivers/mfd/Kconfig              |  12 +
> > > > > > > >  drivers/mfd/Makefile             |   2 +
> > > > > > > >  drivers/mfd/ls2kbmc-mfd.c        | 485 +++++++++++++++++++++++++++++++
> > > > > > > >  7 files changed, 699 insertions(+)
> > > > > > > >  create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> > > > > > > >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> > > > > > > >
> > > > > > > >
> > > > > > > > base-commit: cd2e103d57e5615f9bb027d772f93b9efd567224
> > > > > > > > --
> > > > > > > > 2.47.1
> > > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks.
> > > > > > Binbin
> > > >
> > > > --
> > > > Thanks.
> > > > Binbin
> >
> > --
> > Thanks.
> > Binbin

-- 
Thanks.
Binbin