On Mon, Jun 16, 2025 at 9:16 PM Corey Minyard <co...@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 <co...@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 <co...@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 <co...@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-...@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-...@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.zhoubin...@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.zhoubin...@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.zhoubin...@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
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
