Message-ID: <aE2AR8rj9OEM8TcB@mail.minyard.net>
Date: Sat, 14 Jun 2025 08:59:35 -0500
From: Corey Minyard <corey@...yard.net>
To: Binbin Zhou <zhoubb.aaron@...il.com>
Cc: Binbin Zhou <zhoubinbin@...ngson.cn>,
	Huacai Chen <chenhuacai@...ngson.cn>, Lee Jones <lee@...nel.org>,
	Corey Minyard <minyard@....org>,
	Huacai Chen <chenhuacai@...nel.org>,
	Xuerui Wang <kernel@...0n.name>, loongarch@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	openipmi-developer@...ts.sourceforge.net, jeffbai@...c.io,
	kexybiscuit@...c.io, wangyao@...ote.com
Subject: Re: [PATCH v4 0/3] LoongArch: Add Loongson-2K BMC support

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@...yard.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@...yard.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

Powered by blists - more mailing lists