Re: [PATCH v2 3/3] mfd: ls2kbmc: Add Loongson-2K BMC reset function support

[Lee Jones <lee@kernel.org>]

On Thu, 15 May 2025, Binbin Zhou wrote:

> Since the display is a sub-function of the Loongson-2K BMC, when the
> BMC reset, the entire BMC PCIe is disconnected, including the display
> which is interrupted.
> 
> Not only do you need to save/restore the relevant PCIe registers
> throughout the reset process, but you also need to re-push the display
> to the monitor at the end.
> 
> 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/mfd/ls2kbmc-mfd.c | 242 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 242 insertions(+)
> 
> diff --git a/drivers/mfd/ls2kbmc-mfd.c b/drivers/mfd/ls2kbmc-mfd.c
> index b309f6132c24..4d35a13b3da5 100644
> --- a/drivers/mfd/ls2kbmc-mfd.c
> +++ b/drivers/mfd/ls2kbmc-mfd.c
> @@ -9,8 +9,11 @@
>   */
>  
>  #include <linux/aperture.h>
> +#include <linux/delay.h>
>  #include <linux/errno.h>
>  #include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/kbd_kern.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
> @@ -18,6 +21,8 @@
>  #include <linux/pci_ids.h>
>  #include <linux/platform_data/simplefb.h>
>  #include <linux/platform_device.h>
> +#include <linux/stop_machine.h>
> +#include <linux/vt_kern.h>
>  
>  #define LS2K_DISPLAY_RES_START (SZ_16M + SZ_2M)
>  #define LS2K_IPMI_RES_SIZE	0x1c
> @@ -27,6 +32,9 @@
>  #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)
>  
> +#define LS2K_BMC_RESET_DELAY	(60 * HZ)
> +#define LS2K_BMC_RESET_WAIT	(10 * HZ)
> +
>  static struct resource ls2k_display_resources[] = {
>  	DEFINE_RES_MEM_NAMED(LS2K_DISPLAY_RES_START, SZ_4M, "simpledrm-res"),
>  };
> @@ -60,6 +68,227 @@ static struct mfd_cell ls2k_bmc_cells[] = {
>  	MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi4_resources),
>  };
>  
> +static const u32 index[] = { 0x4, 0x10, 0x14, 0x18, 0x1c, 0x20, 0x24,
> +			     0x30, 0x3c, 0x54, 0x58, 0x78, 0x7c, 0x80 };
> +static const u32 cindex[] = { 0x4, 0x10, 0x3c };

What are these?  You need to define them or at least provide a comment
that describes what they are and how they're used.

> +struct ls2k_bmc_pci_data {
> +	u32 d80c;
> +	u32 d71c;
> +	u32 data[14];
> +	u32 cdata[3];

To the uninitiated, this is all gibberish.

Either change the nomenclature or provide commentary.

> +};
> +
> +struct ls2k_bmc_pdata {
> +	struct device *dev;
> +	struct work_struct reset_work;
> +	unsigned long reset_time;

This always appears to be jiffies - is it used elsewhere?

> +	struct ls2k_bmc_pci_data pci_data;
> +};
> +
> +static bool ls2k_bmc_bar0_addr_is_set(struct pci_dev *ppdev)
> +{
> +	u32 addr;
> +
> +	pci_read_config_dword(ppdev, PCI_BASE_ADDRESS_0, &addr);
> +	addr &= PCI_BASE_ADDRESS_MEM_MASK;
> +
> +	return addr ? true : false;
> +}
> +
> +static bool ls2k_bmc_check_pcie_connected(struct pci_dev *parent,
> +					  struct ls2k_bmc_pdata *priv)
> +{
> +	void __iomem *mmio;
> +	int sts, ret;
> +
> +	mmio = pci_iomap(parent, 0, 0x100);

Why 0x100?  This should be defined.

> +	if (!mmio)
> +		return false;
> +
> +	writel(readl(mmio) | 0x8, mmio);
> +
> +	ret = readl_poll_timeout_atomic(mmio + 0xc, sts, (sts & 0x11) == 0x11,
> +					1000, 1000000);
> +	if (ret) {

All of the magic numbers in this function should be defined.

> +		pci_iounmap(parent, mmio);
> +		dev_err(priv->dev, "PCIE train failed status=0x%x\n", sts);
> +		return false;
> +	}
> +
> +	pci_iounmap(parent, mmio);
> +	return true;
> +}
> +
> +static int ls2k_bmc_recover_pci_data(void *data)
> +{
> +	struct ls2k_bmc_pdata *priv = data;
> +	struct pci_dev *pdev = to_pci_dev(priv->dev);
> +	struct pci_dev *parent = pdev->bus->self;
> +	bool ready, dirty;
> +	u32 i;
> +
> +	pci_write_config_dword(parent, PCI_BASE_ADDRESS_2, 0);
> +	pci_write_config_dword(parent, PCI_BASE_ADDRESS_3, 0);
> +	pci_write_config_dword(parent, PCI_BASE_ADDRESS_4, 0);

You need to provide commentary throughout this driver!

> +
> +	for (i = 2000; i > 0 ; i--) {

Why 2000?  Please define and explain it.

> +		dirty = ls2k_bmc_bar0_addr_is_set(parent);

Since these are bools, you can put the function in an if() statement.

> +		if (!dirty)
> +			break;
> +		mdelay(1);

Why?  What are we waiting for?

> +	};
> +
> +	if (i == 0)
> +		dev_warn(priv->dev, "The PCI Bar is not cleared.\n");

But it's okay to continue?  Why?

> +	for (i = 0; i < ARRAY_SIZE(index); i++)
> +		pci_write_config_dword(parent, index[i], priv->pci_data.data[i]);
> +
> +	pci_write_config_dword(parent, 0x80c, priv->pci_data.d80c);
> +	pci_write_config_dword(parent, 0x71c, priv->pci_data.d71c);
> +
> +	/* Check if the pcie is connected */

PCI-E

> +	ready = ls2k_bmc_check_pcie_connected(parent, priv);

As above.

> +	if (!ready)
> +		return ready;
> +
> +	dev_dbg(priv->dev, "The PCIE recovered done.\n");

How useful is this once development is complete?

> +	/* Waiting for u-boot ddr config ready */

Will it always be U-Boot?

Should be "U-Boot" and "DDR".

> +	mdelay(jiffies_to_msecs(LS2K_BMC_RESET_WAIT));

Why not define the value in ms already?

> +	ready = ls2k_bmc_bar0_addr_is_set(parent);

As above.

> +	if (!ready)
> +		return ready;
> +
> +	for (i = 0; i < ARRAY_SIZE(cindex); i++)
> +		pci_write_config_dword(pdev, cindex[i], priv->pci_data.cdata[i]);
> +
> +	return 0;
> +}
> +
> +static void ls2k_bmc_events_fn(struct work_struct *work)
> +{
> +	struct ls2k_bmc_pdata *priv = container_of(work, struct ls2k_bmc_pdata, reset_work);
> +
> +	/*
> +	 * The pcie is lost when the BMC resets,

Strange line-breaks.  Pick a limit (80-100, etc) and stick to it.

> +	 * at which point access to the pcie from other CPUs
> +	 * is suspended to prevent a crash.
> +	 */
> +	stop_machine(ls2k_bmc_recover_pci_data, priv, NULL);
> +
> +#ifdef CONFIG_VT

No avoidable #ifery in C-files please.

> +	/* Re-push the display due to previous pcie loss. */
> +	set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
> +#endif
> +
> +	dev_info(priv->dev, "Loongson-2K BMC recovered done.\n");

Remove this line - we want clean bootlogs.

> +}
> +
> +static irqreturn_t ls2k_bmc_interrupt(int irq, void *arg)
> +{
> +	struct ls2k_bmc_pdata *priv = arg;
> +
> +	if (system_state != SYSTEM_RUNNING)
> +		return IRQ_HANDLED;
> +
> +	/* Skip interrupt in LS2K_BMC_RESET_DELAY */
> +	if (time_after(jiffies, priv->reset_time + LS2K_BMC_RESET_DELAY))
> +		schedule_work(&priv->reset_work);
> +
> +	priv->reset_time = jiffies;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +#define BMC_RESET_GPIO			14
> +#define LOONGSON_GPIO_REG_BASE		0x1fe00500
> +#define LOONGSON_GPIO_REG_SIZE		0x18
> +#define LOONGSON_GPIO_OEN		0x0
> +#define LOONGSON_GPIO_FUNC		0x4
> +#define LOONGSON_GPIO_INTPOL		0x10
> +#define LOONGSON_GPIO_INTEN		0x14

Put these up with the reset of the defines.

> +/* The gpio interrupt is a watchdog interrupt that is triggered when the BMC resets. */
> +static int ls2k_bmc_gpio_reset_handler(struct ls2k_bmc_pdata *priv)

Why are we now doing GPIO things in here?  Why not a GPIO driver?

> +{
> +	int gsi = 16 + (BMC_RESET_GPIO & 7);
> +	void __iomem *gpio_base;
> +	int irq, ret = 0;
> +
> +	/* Since Loongson-3A hardware does not support GPIO interrupt cascade,
> +	 * chip->gpio_to_irq() cannot be implemented,
> +	 * here acpi_register_gsi() is used to get gpio irq.
> +	 */

Use proper multi-line commentary as per the Coding Style documentation.

> +	irq = acpi_register_gsi(NULL, gsi, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW);
> +	if (irq < 0)
> +		return irq;
> +
> +	gpio_base = ioremap(LOONGSON_GPIO_REG_BASE, LOONGSON_GPIO_REG_SIZE);
> +	if (!gpio_base) {
> +		ret = PTR_ERR(gpio_base);
> +		goto acpi_failed;
> +	}
> +
> +	writel(readl(gpio_base + LOONGSON_GPIO_OEN) | BIT(BMC_RESET_GPIO),
> +	       gpio_base + LOONGSON_GPIO_OEN);
> +	writel(readl(gpio_base + LOONGSON_GPIO_FUNC) & ~BIT(BMC_RESET_GPIO),
> +	       gpio_base + LOONGSON_GPIO_FUNC);
> +	writel(readl(gpio_base + LOONGSON_GPIO_INTPOL) & ~BIT(BMC_RESET_GPIO),
> +	       gpio_base + LOONGSON_GPIO_INTPOL);
> +	writel(readl(gpio_base + LOONGSON_GPIO_INTEN) | BIT(BMC_RESET_GPIO),
> +	       gpio_base + LOONGSON_GPIO_INTEN);
> +
> +	ret = devm_request_irq(priv->dev, irq, ls2k_bmc_interrupt,
> +			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc gpio", priv);
> +	if (ret)
> +		dev_err(priv->dev, "ls2kbmc gpio request_irq(%d) failed\n", irq);

"LS2KBMC" and "GPIO", etc.

> +
> +	iounmap(gpio_base);
> +
> +acpi_failed:
> +	acpi_unregister_gsi(gsi);
> +
> +	return ret;
> +}
> +
> +static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_pdata *priv)
> +{
> +	struct pci_dev *parent = pdev->bus->self;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(index); i++)
> +		pci_read_config_dword(parent, index[i], &priv->pci_data.data[i]);
> +
> +	for (i = 0; i < ARRAY_SIZE(cindex); i++)
> +		pci_read_config_dword(pdev, cindex[i], &priv->pci_data.cdata[i]);
> +
> +	pci_read_config_dword(parent, 0x80c, &priv->pci_data.d80c);
> +	priv->pci_data.d80c = (priv->pci_data.d80c & ~(3 << 17)) | BIT(17);
> +
> +	pci_read_config_dword(parent, 0x71c, &priv->pci_data.d71c);
> +	priv->pci_data.d71c |= BIT(26);

No magic numbers please.  Define them all.

> +}
> +
> +static int ls2k_bmc_pdata_initial(struct pci_dev *pdev, struct ls2k_bmc_pdata *priv)
> +{
> +	int ret;
> +
> +	ls2k_bmc_save_pci_data(pdev, priv);
> +
> +	INIT_WORK(&priv->reset_work, ls2k_bmc_events_fn);
> +
> +	ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
> +			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", priv);
> +	if (ret) {
> +		dev_err(priv->dev, "ls2kbmc pcie request_irq(%d) failed\n", pdev->irq);
> +		return ret;
> +	}
> +
> +	return ls2k_bmc_gpio_reset_handler(priv);
> +}
> +
>  /*
>   * Currently the Loongson-2K0500 BMC hardware does not have an i2c interface to
>   * adapt to the resolution.
> @@ -101,12 +330,25 @@ static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	int ret = 0;
>  	resource_size_t base;
> +	struct ls2k_bmc_pdata *priv;
>  	struct simplefb_platform_data pd;
>  
>  	ret = pci_enable_device(dev);
>  	if (ret)
>  		return ret;
>  
> +	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (IS_ERR(priv)) {
> +		ret = -ENOMEM;
> +		goto disable_pci;
> +	}
> +
> +	priv->dev = &dev->dev;
> +
> +	ret = ls2k_bmc_pdata_initial(dev, priv);
> +	if (ret)
> +		goto disable_pci;
> +
>  	ret = ls2k_bmc_get_video_mode(dev, &pd);
>  	if (ret)
>  		goto disable_pci;
> -- 
> 2.47.1
> 

-- 
Lee Jones [李琼斯]