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.

Also since both host side and BMC side read and write kcs status, I use
fifo pointer to ensure data consistency.

Therefore I made the whole IPMI driver independent.

Co-developed-by: Chong Qiao <qiaochong@loongson.cn>
Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
Signed-off-by: Binbin Zhou <zhoubinbin@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
 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
 #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 <qiaochong@loongson.cn>
+ * Rewritten for mainline by Binbin Zhou <zhoubinbin@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];
+};
+
+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;
+	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;
+	io.dev		= &pdev->dev;
+	io.irq		= 0;
+	if (io.irq)
+		io.irq_setup = ipmi_std_irq_setup;
+
+	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);
+	else
+		platform_registered = true;
+}
+
+void ipmi_si_ls2k_shutdown(void)
+{
+	if (platform_registered)
+		platform_driver_unregister(&ipmi_ls2k_platform_driver);
+}
-- 
2.47.1

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 <qiaochong@loongson.cn>
> Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@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.

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.

>  #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 <qiaochong@loongson.cn>
> + * Rewritten for mainline by Binbin Zhou <zhoubinbin@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

> +
> +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.

> +	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.

> +	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.

> +
> +	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.

> +	else
> +		platform_registered = true;
> +}
> +
> +void ipmi_si_ls2k_shutdown(void)
> +{
> +	if (platform_registered)
> +		platform_driver_unregister(&ipmi_ls2k_platform_driver);
> +}
> -- 
> 2.47.1
>

Hi Corey:

Thanks so much for the detailed review.

On Fri, May 16, 2025 at 8:59 AM Corey Minyard <corey@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 <qiaochong@loongson.cn>
> > Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
> > Signed-off-by: Binbin Zhou <zhoubinbin@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 <qiaochong@loongson.cn>
> > + * Rewritten for mainline by Binbin Zhou <zhoubinbin@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