Message-ID: <CAMpQs4+dZjEzaBOrzknL2+5B5kkz5weZ2zWKu8GwHUoQZqPJ5A@mail.gmail.com>
Date: Fri, 23 May 2025 15:10:04 +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, Chong Qiao <qiaochong@...ngson.cn>
Subject: Re: [PATCH v2 1/3] mfd: ls2kbmc: Introduce Loongson-2K BMC MFD Core driver

Hi Lee:

Thanks for your detailed review.

On Thu, May 22, 2025 at 5:22 PM Lee Jones <lee@...nel.org> wrote:
>
> Just "core driver" in the subject line, rather than "MFD core driver".
>
> > 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-3C6000 CPUs. It supports multiple sub-devices like DRM.
> >
> > Co-developed-by: Chong Qiao <qiaochong@...ngson.cn>
> > Signed-off-by: Chong Qiao <qiaochong@...ngson.cn>
> > Signed-off-by: Binbin Zhou <zhoubinbin@...ngson.cn>
> > ---
> >  drivers/mfd/Kconfig       |  13 ++++
> >  drivers/mfd/Makefile      |   2 +
> >  drivers/mfd/ls2kbmc-mfd.c | 156 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 171 insertions(+)
> >  create mode 100644 drivers/mfd/ls2kbmc-mfd.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 22b936310039..04e40085441d 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2422,5 +2422,18 @@ config MFD_UPBOARD_FPGA
> >         To compile this driver as a module, choose M here: the module will be
> >         called upboard-fpga.
> >
> > +config MFD_LS2K_BMC
> > +     tristate "Loongson-2K Board Management Controller Support"
> > +     depends on LOONGARCH
> > +     default y if LOONGARCH
> > +     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 DRM.
> > +       This driver provides common support for accessing the devices;
> > +       additional drivers must be enabled in order to use the
> > +       functionality of the BMC device.
>
> This paragraph has some odd line breaks.  Please re-align.
>
> >  endmenu
> >  endif
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 948cbdf42a18..18960ea13b64 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -290,3 +290,5 @@ obj-$(CONFIG_MFD_RSMU_I2C)        += rsmu_i2c.o rsmu_core.o
> >  obj-$(CONFIG_MFD_RSMU_SPI)   += rsmu_spi.o rsmu_core.o
> >
> >  obj-$(CONFIG_MFD_UPBOARD_FPGA)       += upboard-fpga.o
> > +
> > +obj-$(CONFIG_MFD_LS2K_BMC)   += ls2kbmc-mfd.o
> > diff --git a/drivers/mfd/ls2kbmc-mfd.c b/drivers/mfd/ls2kbmc-mfd.c
> > new file mode 100644
> > index 000000000000..b309f6132c24
> > --- /dev/null
> > +++ b/drivers/mfd/ls2kbmc-mfd.c
> > @@ -0,0 +1,156 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Loongson-2K Board Management Controller (BMC) MFD Core Driver.
>
> Remove the MFD part.  It's not a thing - we made it up.
>
> > + * Copyright (C) 2024 Loongson Technology Corporation Limited.
>
> No changes since 2024?
>
> > + *
> > + * Originally written by Chong Qiao <qiaochong@...ngson.cn>
> > + * Rewritten for mainline by Binbin Zhou <zhoubinbin@...ngson.cn>
>
> "Mainline"
>
> Typically we just do:
>
> Authors:
>         Author One <one@...p.com>
>         Author Two <two@...p.com>
>
> > + */
> > +
> > +#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>
> > +
> > +#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)
>
> Line them _all_ up please.  One more tab should do it.
>
> > +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-2K0500 BMC hardware does not have an i2c interface to
>
> I2C
>
> > + * adapt to the resolution.
>
> Remove the line break here.
>
> > + * We set the resolution by presetting "video=1280x1024-16@2M" to the bmc memory.
>
> BMC
>
> > + */
> > +static int ls2k_bmc_get_video_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
> > +{
> > +     char *mode;
> > +     int depth, ret;
> > +
> > +     /* The pci mem bar last 16M is used to store the string. */
>
> PCI
>
> BAR's (maybe?)

/* 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;
> > +
> > +     /* env at last 16M's beginning, first env is "video=" */
>
> This doesn't make sense to me - please reword.

How about:

/* 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;
> > +}
>
> Surely there is a standard format / API for this already?
>
>
Now, simplefb_platform_data is mainly described in DTS and parsed by simpledrm.
And when it is used as platform_data, there doesn't seem to be a
unified API to populate simplefb_platform_data. e.g. in
sysfb_simplefb[1], it is parsed using sysfb_parse_mode().

[1]: https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/firmware/sysfb_simplefb.c#L27
>
> > +static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > +{
> > +     int ret = 0;
>
> There is no need to pre-initialise this.
>
> > +     resource_size_t base;
> > +     struct simplefb_platform_data pd;
>
> Reverse these please (reverse Christmas tree is preferred).
> > +
> > +     ret = pci_enable_device(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ls2k_bmc_get_video_mode(dev, &pd);
> > +     if (ret)
> > +             goto disable_pci;
> > +
> > +     ls2k_bmc_cells[0].platform_data = &pd;
> > +     ls2k_bmc_cells[0].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, "Remove firmware framebuffers failed: %d\n", ret);
>
> "Failed to removed firmware framebuffers"
>
> > +             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,
> > +};
> > +
>
> Remove this line.

All comments about code formatting will be changed in the next version.
>
> > +module_pci_driver(ls2k_bmc_driver);
> > +
> > +MODULE_DESCRIPTION("Loongson-2K BMC driver");
> > +MODULE_AUTHOR("Loongson Technology Corporation Limited");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.47.1
> >
>
> --
> Lee Jones [李琼斯]

-- 
Thanks.
Binbin

Powered by blists - more mailing lists