Message-ID: <CAMpQs4JccEmMAguB92jQriwD65Ra+hQKMZnjAsWhNOKhN_Om7A@mail.gmail.com>
Date: Fri, 11 Jul 2025 15:17:29 +0800
From: Binbin Zhou <zhoubb.aaron@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Binbin Zhou <zhoubinbin@...ngson.cn>, Huacai Chen <chenhuacai@...ngson.cn>, 
	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, Chong Qiao <qiaochong@...ngson.cn>, 
	Corey Minyard <corey@...yard.net>
Subject: Re: [PATCH v7 1/3] mfd: ls2kbmc: Introduce Loongson-2K BMC core driver

Hi Lee:

Thanks for your review.

On Thu, Jul 10, 2025 at 5:56 PM Lee Jones <lee@...nel.org> wrote:
>
> On Fri, 04 Jul 2025, Binbin Zhou wrote:
>
> > The Loongson-2K Board Management Controller provides an PCIe interface
> > to the host to access the feature implemented in the BMC.
> >
> > The BMC is assembled on a server similar to the server machine with
> > Loongson-3 CPU. It supports multiple sub-devices like DRM and IPMI.
> >
> > Co-developed-by: Chong Qiao <qiaochong@...ngson.cn>
> > Signed-off-by: Chong Qiao <qiaochong@...ngson.cn>
> > Reviewed-by: Huacai Chen <chenhuacai@...ngson.cn>
> > Acked-by: Corey Minyard <corey@...yard.net>
> > Signed-off-by: Binbin Zhou <zhoubinbin@...ngson.cn>
> > ---
> >  MAINTAINERS                 |   6 ++
> >  drivers/mfd/Kconfig         |  13 +++
> >  drivers/mfd/Makefile        |   2 +
> >  drivers/mfd/ls2k-bmc-core.c | 156 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 177 insertions(+)
> >  create mode 100644 drivers/mfd/ls2k-bmc-core.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0d053c45f7f9..4eb0f7b69d35 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14199,6 +14199,12 @@ S:   Maintained
> >  F:   Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> >  F:   drivers/thermal/loongson2_thermal.c
> >
> > +LOONGSON-2K Board Management Controller (BMC) DRIVER
> > +M:   Binbin Zhou <zhoubinbin@...ngson.cn>
> > +M:   Chong Qiao <qiaochong@...ngson.cn>
> > +S:   Maintained
> > +F:   drivers/mfd/ls2k-bmc-core.c
> > +
> >  LOONGSON EDAC DRIVER
> >  M:   Zhao Qunqin <zhaoqunqin@...ngson.cn>
> >  L:   linux-edac@...r.kernel.org
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index c635790afa75..47cc8ea9d2ef 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2400,6 +2400,19 @@ config MFD_INTEL_M10_BMC_PMCI
> >         additional drivers must be enabled in order to use the functionality
> >         of the device.
> >
> > +config MFD_LS2K_BMC_CORE
> > +     bool "Loongson-2K Board Management Controller Support"
> > +     depends on PCI && ACPI_GENERIC_GSI
> > +     select MFD_CORE
> > +     help
> > +       Say yes here to add support for the Loongson-2K BMC which is a Board
> > +       Management Controller connected to the PCIe bus. The device supports
> > +       multiple sub-devices like display and IPMI. This driver provides common
> > +       support for accessing the devices.
> > +
> > +       The display is enabled by default in the driver, while the IPMI interface
> > +       is enabled independently through the IPMI_LS2K option in the IPMI section.
> > +
> >  config MFD_QNAP_MCU
> >       tristate "QNAP microcontroller unit core driver"
> >       depends on SERIAL_DEV_BUS
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index ca351cb0ddcc..675b4ec6ef4c 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -284,6 +284,8 @@ obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc-core.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)    += intel-m10-bmc-spi.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI)   += intel-m10-bmc-pmci.o
> >
> > +obj-$(CONFIG_MFD_LS2K_BMC_CORE)              += ls2k-bmc-core.o
> > +
> >  obj-$(CONFIG_MFD_ATC260X)    += atc260x-core.o
> >  obj-$(CONFIG_MFD_ATC260X_I2C)        += atc260x-i2c.o
> >
> > diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> > new file mode 100644
> > index 000000000000..50d560a4611c
> > --- /dev/null
> > +++ b/drivers/mfd/ls2k-bmc-core.c
> > @@ -0,0 +1,156 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Loongson-2K Board Management Controller (BMC) Core Driver.
> > + *
> > + * Copyright (C) 2024-2025 Loongson Technology Corporation Limited.
> > + *
> > + * Authors:
> > + *   Chong Qiao <qiaochong@...ngson.cn>
> > + *   Binbin Zhou <zhoubinbin@...ngson.cn>
> > + */
> > +
> > +#include <linux/aperture.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/pci_ids.h>
> > +#include <linux/platform_data/simplefb.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* LS2K BMC resources */
> > +#define LS2K_DISPLAY_RES_START               (SZ_16M + SZ_2M)
> > +#define LS2K_IPMI_RES_SIZE           0x1C
> > +#define LS2K_IPMI0_RES_START         (SZ_16M + 0xF00000)
> > +#define LS2K_IPMI1_RES_START         (LS2K_IPMI0_RES_START + LS2K_IPMI_RES_SIZE)
> > +#define LS2K_IPMI2_RES_START         (LS2K_IPMI1_RES_START + LS2K_IPMI_RES_SIZE)
> > +#define LS2K_IPMI3_RES_START         (LS2K_IPMI2_RES_START + LS2K_IPMI_RES_SIZE)
> > +#define LS2K_IPMI4_RES_START         (LS2K_IPMI3_RES_START + LS2K_IPMI_RES_SIZE)
> > +
> > +static struct resource ls2k_display_resources[] = {
> > +     DEFINE_RES_MEM_NAMED(LS2K_DISPLAY_RES_START, SZ_4M, "simpledrm-res"),
> > +};
> > +
> > +static struct resource ls2k_ipmi0_resources[] = {
> > +     DEFINE_RES_MEM_NAMED(LS2K_IPMI0_RES_START, LS2K_IPMI_RES_SIZE, "ipmi0-res"),
> > +};
> > +
> > +static struct resource ls2k_ipmi1_resources[] = {
> > +     DEFINE_RES_MEM_NAMED(LS2K_IPMI1_RES_START, LS2K_IPMI_RES_SIZE, "ipmi1-res"),
> > +};
> > +
> > +static struct resource ls2k_ipmi2_resources[] = {
> > +     DEFINE_RES_MEM_NAMED(LS2K_IPMI2_RES_START, LS2K_IPMI_RES_SIZE, "ipmi2-res"),
> > +};
> > +
> > +static struct resource ls2k_ipmi3_resources[] = {
> > +     DEFINE_RES_MEM_NAMED(LS2K_IPMI3_RES_START, LS2K_IPMI_RES_SIZE, "ipmi3-res"),
> > +};
> > +
> > +static struct resource ls2k_ipmi4_resources[] = {
> > +     DEFINE_RES_MEM_NAMED(LS2K_IPMI4_RES_START, LS2K_IPMI_RES_SIZE, "ipmi4-res"),
> > +};
> > +
> > +static struct mfd_cell ls2k_bmc_cells[] = {
> > +     MFD_CELL_RES("simple-framebuffer", ls2k_display_resources),
> > +     MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi0_resources),
> > +     MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi1_resources),
> > +     MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi2_resources),
> > +     MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi3_resources),
> > +     MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi4_resources),
> > +};
> > +
> > +/*
> > + * Currently the Loongson-2K BMC hardware does not have an I2C interface to adapt to the
> > + * resolution. We set the resolution by presetting "video=1280x1024-16@2M" to the BMC memory.
> > + */
> > +static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
> > +{
> > +     char *mode;
> > +     int depth, ret;
> > +
> > +     /* The last 16M of PCI BAR0 is used to store the resolution string. */
> > +     mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> > +     if (!mode)
> > +             return -ENOMEM;
> > +
> > +     /* The resolution field starts with the flag "video=". */
> > +     if (!strncmp(mode, "video=", 6))
> > +             mode = mode + 6;
> > +
> > +     ret = kstrtoint(strsep(&mode, "x"), 10, &pd->width);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = kstrtoint(strsep(&mode, "-"), 10, &pd->height);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
> > +     if (ret)
> > +             return ret;
> > +
> > +     pd->stride = pd->width * depth / 8;
> > +     pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";
> > +
> > +     return 0;
> > +}
> > +
> > +static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > +{
> > +     struct simplefb_platform_data pd;
> > +     resource_size_t base;
> > +     int ret;
> > +
> > +     ret = pci_enable_device(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ls2k_bmc_parse_mode(dev, &pd);
> > +     if (ret)
> > +             goto disable_pci;
> > +
> > +     ls2k_bmc_cells[0].platform_data = &pd;
> > +     ls2k_bmc_cells[0].pdata_size = sizeof(pd);
>
> This is fragile.
>
> Please identify the elements in ls2k_bmc_cells and use it to index here.
>
> See: `static struct mfd_cell as3711_subdevs`

How about this:

enum {
        LS2K_BMC_DISPLAY,
        LS2k_BMC_IPMI0,
        LS2k_BMC_IPMI1,
        LS2k_BMC_IPMI2,
        LS2k_BMC_IPMI3,
        LS2k_BMC_IPMI4,
};

static struct mfd_cell ls2k_bmc_cells[] = {
        [LS2K_BMC_DISPLAY] = {
                .name = "simple-framebuffer",
                .num_resources = ARRAY_SIZE(ls2k_display_resources),
                .resources = ls2k_display_resources
        },
        [LS2k_BMC_IPMI0] = {
                .name = "ls2k-ipmi-si",
                .num_resources = ARRAY_SIZE(ls2k_ipmi0_resources),
                .resources = ls2k_ipmi0_resources
        },
        [LS2k_BMC_IPMI1] = {
                .name = "ls2k-ipmi-si",
                .num_resources = ARRAY_SIZE(ls2k_ipmi1_resources),
                .resources = ls2k_ipmi1_resources
        },
        [LS2k_BMC_IPMI2] = {
                .name = "ls2k-ipmi-si",
                .num_resources = ARRAY_SIZE(ls2k_ipmi2_resources),
                .resources = ls2k_ipmi2_resources
        },
        [LS2k_BMC_IPMI3] = {
                .name = "ls2k-ipmi-si",
                .num_resources = ARRAY_SIZE(ls2k_ipmi3_resources),
                .resources = ls2k_ipmi3_resources
        },
        [LS2k_BMC_IPMI4] = {
                .name = "ls2k-ipmi-si",
                .num_resources = ARRAY_SIZE(ls2k_ipmi4_resources),
                .resources = ls2k_ipmi4_resources
        },
};

and

        ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
        ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);

>
> > +     base = dev->resource[0].start + LS2K_DISPLAY_RES_START;
> > +
> > +     /* Remove conflicting efifb device */
> > +     ret = aperture_remove_conflicting_devices(base, SZ_4M, "simple-framebuffer");
> > +     if (ret) {
> > +             dev_err(&dev->dev, "Failed to removed firmware framebuffers: %d\n", ret);
> > +             goto disable_pci;
> > +     }
> > +
> > +     return devm_mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > +                                 ls2k_bmc_cells, ARRAY_SIZE(ls2k_bmc_cells),
> > +                                 &dev->resource[0], 0, NULL);
> > +
> > +disable_pci:
> > +     pci_disable_device(dev);
> > +     return ret;
> > +}
> > +
> > +static void ls2k_bmc_remove(struct pci_dev *dev)
> > +{
> > +     pci_disable_device(dev);
> > +}
> > +
> > +static struct pci_device_id ls2k_bmc_devices[] = {
> > +     { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x1a05) },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(pci, ls2k_bmc_devices);
> > +
> > +static struct pci_driver ls2k_bmc_driver = {
> > +     .name = "ls2k-bmc",
> > +     .id_table = ls2k_bmc_devices,
> > +     .probe = ls2k_bmc_probe,
> > +     .remove = ls2k_bmc_remove,
> > +};
> > +module_pci_driver(ls2k_bmc_driver);
> > +
> > +MODULE_DESCRIPTION("Loongson-2K Board Management Controller (BMC) Core driver");
> > +MODULE_AUTHOR("Loongson Technology Corporation Limited");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.47.1
> >
>
> --
> Lee Jones [李琼斯]


--
Thanks.
Binbin

Powered by blists - more mailing lists