Hi Corey:
Thanks so much for the detailed review.
On Fri, May 16, 2025 at 8:59 AM Corey Minyard <co...@minyard.net> wrote:
>
> On Thu, May 15, 2025 at 10:32:25AM +0800, Binbin Zhou wrote:
> > This patch adds Loongson-2K BMC IPMI support.
> >
> > According to the existing design, we use software simulation to
> > implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
>
> This is a strange way to do this. My preference would be to have a
> separate driver for this and not put it under the ipmi_si driver.
> But it's annoyingly close and it would duplicate a lot of ipmi_si_intf.c
> Anyway, I think I'm ok with this basic design. But there are problems.
>
> >
> > Also since both host side and BMC side read and write kcs status, I use
> > fifo pointer to ensure data consistency.
>
> I assume this fifo pointer is part of the interface hardware or the
> implementation on the other side of the interface.
>
> >
> > Therefore I made the whole IPMI driver independent.
>
> What do you mean by this statement?
>
> More comments inline.
>
> >
> > Co-developed-by: Chong Qiao <qiaoch...@loongson.cn>
> > Signed-off-by: Chong Qiao <qiaoch...@loongson.cn>
> > Signed-off-by: Binbin Zhou <zhoubin...@loongson.cn>
> > ---
> > 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 | 250 +++++++++++++++++++++++++++++++
> > 4 files changed, 261 insertions(+)
> > create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
> >
> > diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> > index e0944547c9d0..5eb3494f5f39 100644
> > --- a/drivers/char/ipmi/Makefile
> > +++ b/drivers/char/ipmi/Makefile
> > @@ -8,6 +8,7 @@ ipmi_si-y := ipmi_si_intf.o ipmi_kcs_sm.o ipmi_smic_sm.o
> > ipmi_bt_sm.o \
> > ipmi_si_mem_io.o
> > ipmi_si-$(CONFIG_HAS_IOPORT) += ipmi_si_port_io.o
> > ipmi_si-$(CONFIG_PCI) += ipmi_si_pci.o
> > +ipmi_si-$(CONFIG_LOONGARCH) += ipmi_si_ls2k.o
>
> Shouldn't this be dependent on MFD_LS2K_BMC? It appears you can disable
> that and still have CONFIG_LOONGARCH enabled.
Indeed, it should rely on MFD_LS2K_BMC.
>
> And this MFD can have multiple things hanging off of it, wouldn't you
> want to make the individual drivers their own CONFIG items?
>
> > ipmi_si-$(CONFIG_PARISC) += ipmi_si_parisc.o
> >
> > obj-$(CONFIG_IPMI_HANDLER) += ipmi_msghandler.o
> > diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
> > index a7ead2a4c753..71f1d4e1272c 100644
> > --- a/drivers/char/ipmi/ipmi_si.h
> > +++ b/drivers/char/ipmi/ipmi_si.h
> > @@ -93,6 +93,13 @@ void ipmi_si_pci_shutdown(void);
> > static inline void ipmi_si_pci_init(void) { }
> > static inline void ipmi_si_pci_shutdown(void) { }
> > #endif
> > +#ifdef CONFIG_LOONGARCH
> > +void ipmi_si_ls2k_init(void);
> > +void ipmi_si_ls2k_shutdown(void);
> > +#else
> > +static inline void ipmi_si_ls2k_init(void) { }
> > +static inline void ipmi_si_ls2k_shutdown(void) { }
> > +#endif
>
> I'm not excited about this, but there is history, I guess.
>
> Same comment as the Makefile on CONFIG_LOONGARCH.
It should be MFD_LS2K_BMC.
>
> > #ifdef CONFIG_PARISC
> > void ipmi_si_parisc_init(void);
> > void ipmi_si_parisc_shutdown(void);
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c
> > b/drivers/char/ipmi/ipmi_si_intf.c
> > index 12b0b77eb1cc..323da77698ea 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -2107,6 +2107,7 @@ static int __init init_ipmi_si(void)
> >
> > ipmi_si_pci_init();
> >
> > + ipmi_si_ls2k_init();
> > ipmi_si_parisc_init();
> >
> > /* We prefer devices with interrupts, but in the case of a machine
> > @@ -2288,6 +2289,8 @@ static void cleanup_ipmi_si(void)
> >
> > ipmi_si_pci_shutdown();
> >
> > + ipmi_si_ls2k_shutdown();
> > +
> > ipmi_si_parisc_shutdown();
> >
> > ipmi_si_platform_shutdown();
> > diff --git a/drivers/char/ipmi/ipmi_si_ls2k.c
> > b/drivers/char/ipmi/ipmi_si_ls2k.c
> > new file mode 100644
> > index 000000000000..cb31bb989fca
> > --- /dev/null
> > +++ b/drivers/char/ipmi/ipmi_si_ls2k.c
> > @@ -0,0 +1,250 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Loongson-2K BMC IPMI
> > + *
> > + * Copyright (C) 2024 Loongson Technology Corporation Limited.
> > + *
> > + * Originally written by Chong Qiao <qiaoch...@loongson.cn>
> > + * Rewritten for mainline by Binbin Zhou <zhoubin...@loongson.cn>
> > + */
> > +
> > +#include <linux/ioport.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +
> > +#include "ipmi_si.h"
> > +
> > +#define LS2K_KCS_STS_OBF BIT(0)
> > +#define LS2K_KCS_STS_IBF BIT(1)
> > +#define LS2K_KCS_STS_SMS_ATN BIT(2)
> > +#define LS2K_KCS_STS_CMD BIT(3)
> > +
> > +#define LS2K_KCS_DATA_MASK (LS2K_KCS_STS_OBF | LS2K_KCS_STS_IBF |
> > LS2K_KCS_STS_CMD)
> > +
> > +/* Read and write fifo pointers for data consistency. */
> > +struct ls2k_fifo_flag {
> > + u8 ibfh;
> > + u8 ibft;
> > + u8 obfh;
> > + u8 obft;
> > +};
> > +
> > +struct ls2k_kcs_reg {
> > + u8 status;
> > + u8 data_out;
> > + s16 data_in;
> > + s16 cmd;
> > +};
> > +
> > +struct ls2k_kcs_data {
> > + struct ls2k_fifo_flag fifo;
> > + struct ls2k_kcs_reg reg;
> > + u8 cmd_data;
> > + u8 version;
> > + u32 write_req;
> > + u32 write_ack;
> > + u32 reserved[2];
> > +};
>
> The above appears to be a memory overlay for registers. But you aren't
> using readb/writeb and associated functions to read/write it. That is
> not the right way to do things. Please read
> Documentation/driver-api/device-io.rst
Ok, I'll refactor this part of the code to redefine each struct item
into the appropriate macro and access it via readx()/writex().
>
> > +
> > +static void ls2k_set_obf(struct ls2k_kcs_data *ik, u8 sts)
> > +{
> > + ik->reg.status = (ik->reg.status & ~LS2K_KCS_STS_OBF) | (sts &
> > BIT(0));
> > +}
> > +
> > +static void ls2k_set_ibf(struct ls2k_kcs_data *ik, u8 sts)
> > +{
> > + ik->reg.status = (ik->reg.status & ~LS2K_KCS_STS_IBF) | ((sts &
> > BIT(0)) << 1);
> > +}
> > +
> > +static u8 ls2k_get_ibf(struct ls2k_kcs_data *ik)
> > +{
> > + return (ik->reg.status >> 1) & BIT(0);
> > +}
> > +
> > +static unsigned char intf_sim_inb_v0(struct ls2k_kcs_data *ik,
> > + unsigned int offset)
> > +{
> > + u32 inb = 0;
> > +
> > + switch (offset & BIT(0)) {
> > + case 0:
> > + inb = ik->reg.data_out;
> > + ls2k_set_obf(ik, 0);
> > + break;
> > + case 1:
> > + inb = ik->reg.status;
> > + break;
> > + }
> > +
> > + return inb;
> > +}
> > +
> > +static unsigned char intf_sim_inb_v1(struct ls2k_kcs_data *ik,
> > + unsigned int offset)
> > +{
> > + u32 inb = 0;
> > + int cmd;
> > + bool obf, ibf;
> > +
> > + obf = ik->fifo.obfh != ik->fifo.obft;
> > + ibf = ik->fifo.ibfh != ik->fifo.ibft;
> > + cmd = ik->cmd_data;
> > +
> > + switch (offset & BIT(0)) {
> > + case 0:
> > + inb = ik->reg.data_out;
> > + ik->fifo.obft = ik->fifo.obfh;
> > + break;
> > + case 1:
> > + inb = ik->reg.status & ~LS2K_KCS_DATA_MASK;
> > + inb |= obf | (ibf << 1) | (cmd << 3);
> > + break;
> > + }
> > +
> > + return inb;
> > +}
> > +
> > +static unsigned char ls2k_mem_inb(const struct si_sm_io *io,
> > + unsigned int offset)
> > +{
> > + struct ls2k_kcs_data *ik = io->addr;
> > + int inb = 0;
> > +
> > + if (ik->version == 0)
> > + inb = intf_sim_inb_v0(ik, offset);
> > + else if (ik->version == 1)
> > + inb = intf_sim_inb_v1(ik, offset);
> > +
> > + return inb;
> > +}
> > +
> > +static void intf_sim_outb_v0(struct ls2k_kcs_data *ik, unsigned int offset,
> > + unsigned char val)
> > +{
> > + if (ls2k_get_ibf(ik))
> > + return;
> > +
> > + switch (offset & BIT(0)) {
> > + case 0:
> > + ik->reg.data_in = val;
> > + ik->reg.status &= ~LS2K_KCS_STS_CMD;
> > + break;
> > +
> > + case 1:
> > + ik->reg.cmd = val;
> > + ik->reg.status |= LS2K_KCS_STS_CMD;
> > + break;
> > + }
> > +
> > + ls2k_set_ibf(ik, 1);
> > + ik->write_req++;
> > +}
> > +
> > +static void intf_sim_outb_v1(struct ls2k_kcs_data *ik, unsigned int offset,
> > + unsigned char val)
> > +{
> > + if (ik->fifo.ibfh != ik->fifo.ibft)
> > + return;
> > +
> > + switch (offset & BIT(0)) {
> > + case 0:
> > + ik->reg.data_in = val;
> > + ik->cmd_data = 0;
> > + break;
> > +
> > + case 1:
> > + ik->reg.cmd = val;
> > + ik->cmd_data = 1;
> > + break;
> > + }
> > +
> > + ik->fifo.ibfh = !ik->fifo.ibft;
> > + ik->write_req++;
> > +}
> > +
> > +static void ls2k_mem_outb(const struct si_sm_io *io, unsigned int offset,
> > + unsigned char val)
> > +{
> > + struct ls2k_kcs_data *ik = io->addr;
> > +
> > + if (ik->version == 0)
> > + intf_sim_outb_v0(ik, offset, val);
> > + else if (ik->version == 1)
> > + intf_sim_outb_v1(ik, offset, val);
> > +}
> > +
> > +static void ls2k_mem_cleanup(struct si_sm_io *io)
> > +{
> > + if (io->addr)
> > + iounmap(io->addr);
> > +}
> > +
> > +static int ipmi_ls2k_sim_setup(struct si_sm_io *io)
> > +{
> > + io->addr = ioremap(io->addr_data, io->regspacing);
> > + if (!io->addr)
> > + return -EIO;
> > +
> > + io->inputb = ls2k_mem_inb;
> > + io->outputb = ls2k_mem_outb;
> > + io->io_cleanup = ls2k_mem_cleanup;
> > +
> > + return 0;
> > +}
> > +
> > +static int ipmi_ls2k_probe(struct platform_device *pdev)
> > +{
> > + struct si_sm_io io;
> > +
> > + dev_info(&pdev->dev, "probing via ls2k platform");
> > + memset(&io, 0, sizeof(io));
> > +
> > + io.addr_source = SI_PLATFORM;
> > + io.si_type = SI_KCS;
>
> si_type has been reworked recently, the linux next tree has the changes.
> I'll need this modified to work with the linux next changes.
OK, I will rebase my driver.
>
> > + io.addr_space = IPMI_MEM_ADDR_SPACE;
> > + io.io_setup = ipmi_ls2k_sim_setup;
> > + io.addr_data = pdev->resource[0].start;
> > + io.regspacing = pdev->resource[0].end - pdev->resource[0].start + 1;
> > + io.regsize = DEFAULT_REGSIZE;
> > + io.regshift = 0;
>
> The above items, except for io_setup, don't have much meaning for your
> device; there's not much need to set them, and there's no need to
> initialize things to zero. They are for ipmi_si_port and ipmi_si_mem.
The addr_data and regspacing seem to be needed in
ipmi_ls2k_sim_setup(), in any case, I'll reorganize it based on the
latest code.
>
> > + io.dev = &pdev->dev;
> > + io.irq = 0;
> > + if (io.irq)
> > + io.irq_setup = ipmi_std_irq_setup;
>
> Just remove the irq thing, don't set it to zero and then check it.
OK..
>
> > +
> > + dev_info(&pdev->dev, "%pR regsize %d spacing %d irq %d\n",
> > + &pdev->resource[0], io.regsize, io.regspacing, io.irq);
> > +
> > + return ipmi_si_add_smi(&io);
> > +}
> > +
> > +static void ipmi_ls2k_remove(struct platform_device *pdev)
> > +{
> > + ipmi_si_remove_by_dev(&pdev->dev);
> > +}
> > +
> > +struct platform_driver ipmi_ls2k_platform_driver = {
> > + .driver = {
> > + .name = "ls2k-ipmi-si",
> > + },
> > + .probe = ipmi_ls2k_probe,
> > + .remove = ipmi_ls2k_remove,
> > +};
> > +
> > +static bool platform_registered;
> > +void ipmi_si_ls2k_init(void)
> > +{
> > + int rv;
> > +
> > + rv = platform_driver_register(&ipmi_ls2k_platform_driver);
> > + if (rv)
> > + pr_err("Unable to register driver: %d\n", rv);
>
> That's far to vague to be useful.
OK, let's just drop it.
>
> > + else
> > + platform_registered = true;
> > +}
> > +
> > +void ipmi_si_ls2k_shutdown(void)
> > +{
> > + if (platform_registered)
> > + platform_driver_unregister(&ipmi_ls2k_platform_driver);
> > +}
> > --
> > 2.47.1
> >
--
Thanks.
Binbin
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
