Re: [PATCH v3 3/3] soc: nuvoton: add NPCM BPC driver

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 13/12/2023 20:05, Tomer Maimon wrote:
> Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
> 
> The NPCM BPC monitoring two configurable I/O address written by the host
> on the bus.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
> ---
>  drivers/soc/nuvoton/Kconfig    |   9 +
>  drivers/soc/nuvoton/Makefile   |   1 +
>  drivers/soc/nuvoton/npcm-bpc.c | 387 +++++++++++++++++++++++++++++++++
>  3 files changed, 397 insertions(+)
>  create mode 100644 drivers/soc/nuvoton/npcm-bpc.c
> 
> diff --git a/drivers/soc/nuvoton/Kconfig b/drivers/soc/nuvoton/Kconfig
> index d5102f5f0c28..ebd162633942 100644
> --- a/drivers/soc/nuvoton/Kconfig
> +++ b/drivers/soc/nuvoton/Kconfig
> @@ -2,6 +2,15 @@
>  
>  menu "NUVOTON SoC drivers"

...

> +
> +static int npcm_bpc_config_irq(struct npcm_bpc *bpc,
> +			       struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int rc;
> +
> +	bpc->irq = platform_get_irq(pdev, 0);
> +	if (bpc->irq < 0) {
> +		dev_err(dev, "get IRQ failed\n");
> +		return bpc->irq;

return dev_err_probe

> +	}
> +
> +	rc = devm_request_irq(dev, bpc->irq,
> +			      npcm_bpc_irq, IRQF_SHARED,
> +			      DEVICE_NAME, bpc);
> +	if (rc < 0) {
> +		dev_warn(dev, "Unable to request IRQ %d\n", bpc->irq);

return dev_err_probe

> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static int npcm_enable_bpc(struct npcm_bpc *bpc, struct device *dev,
> +			   int channel, u16 host_port)
> +{
> +	u8 addr_en, reg_en;
> +	int err;
> +
> +	init_waitqueue_head(&bpc->ch[channel].wq);
> +
> +	err = kfifo_alloc(&bpc->ch[channel].fifo, BPC_KFIFO_SIZE,
> +			  GFP_KERNEL);
> +	if (err)
> +		return err;
> +
> +	bpc->ch[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
> +	bpc->ch[channel].miscdev.name =
> +		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
> +	bpc->ch[channel].miscdev.fops = &npcm_bpc_fops;
> +	bpc->ch[channel].miscdev.parent = dev;
> +	err = misc_register(&bpc->ch[channel].miscdev);
> +	if (err)
> +		return err;
> +
> +	bpc->ch[channel].data = bpc;
> +	bpc->ch[channel].host_reset = false;
> +
> +	/* Enable host snoop channel at requested port */
> +	switch (channel) {
> +	case 0:
> +		addr_en = FIFO_IOADDR1_ENABLE;
> +		iowrite8((u8)host_port & 0xFF,
> +			 bpc->base + NPCM_BPCFA1L_REG);
> +		iowrite8((u8)(host_port >> 8),
> +			 bpc->base + NPCM_BPCFA1M_REG);
> +		break;
> +	case 1:
> +		addr_en = FIFO_IOADDR2_ENABLE;
> +		iowrite8((u8)host_port & 0xFF,
> +			 bpc->base + NPCM_BPCFA2L_REG);
> +		iowrite8((u8)(host_port >> 8),
> +			 bpc->base + NPCM_BPCFA2M_REG);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (bpc->en_dwcap)
> +		addr_en = FIFO_DWCAPTURE;
> +
> +	/*
> +	 * Enable FIFO Ready Interrupt, FIFO Capture of I/O addr,
> +	 * and Host Reset
> +	 */
> +	reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG);
> +	iowrite8(reg_en | addr_en | FIFO_READY_INT_ENABLE |
> +		 HOST_RESET_INT_ENABLE, bpc->base + NPCM_BPCFEN_REG);
> +
> +	return 0;
> +}
> +
> +static void npcm_disable_bpc(struct npcm_bpc *bpc, int channel)
> +{
> +	u8 reg_en;
> +
> +	switch (channel) {
> +	case 0:
> +		reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG);
> +		if (bpc->en_dwcap)
> +			iowrite8(reg_en & ~FIFO_DWCAPTURE,
> +				 bpc->base + NPCM_BPCFEN_REG);
> +		else
> +			iowrite8(reg_en & ~FIFO_IOADDR1_ENABLE,
> +				 bpc->base + NPCM_BPCFEN_REG);
> +		break;
> +	case 1:
> +		reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG);
> +		iowrite8(reg_en & ~FIFO_IOADDR2_ENABLE,
> +			 bpc->base + NPCM_BPCFEN_REG);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (!(reg_en & (FIFO_IOADDR1_ENABLE | FIFO_IOADDR2_ENABLE)))
> +		iowrite8(reg_en &
> +			 ~(FIFO_READY_INT_ENABLE | HOST_RESET_INT_ENABLE),
> +			 bpc->base + NPCM_BPCFEN_REG);
> +
> +	kfifo_free(&bpc->ch[channel].fifo);
> +	misc_deregister(&bpc->ch[channel].miscdev);
> +}
> +
> +static int npcm_bpc_probe(struct platform_device *pdev)
> +{
> +	struct npcm_bpc *bpc;
> +	struct device *dev;
> +	u32 port;
> +	int err;
> +
> +	dev = &pdev->dev;
> +
> +	bpc = devm_kzalloc(dev, sizeof(*bpc), GFP_KERNEL);
> +	if (!bpc)
> +		return -ENOMEM;
> +
> +	bpc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(bpc->base))
> +		return PTR_ERR(bpc->base);
> +
> +	dev_set_drvdata(&pdev->dev, bpc);

Why do you use pdev again? Look earlier, you have local variable for this.

> +
> +	err = of_property_read_u32_index(dev->of_node, "nuvoton,monitor-ports",
> +					 0, &port);
> +	if (err) {
> +		dev_err(dev, "no monitor ports configured\n");
> +		return -ENODEV;
> +	}
> +
> +	bpc->en_dwcap =
> +		of_property_read_bool(dev->of_node, "nuvoton,bpc-en-dwcapture");
> +
> +	bpc->dev = dev;
> +
> +	err = npcm_bpc_config_irq(bpc, pdev);
> +	if (err)
> +		return err;
> +
> +	err = npcm_enable_bpc(bpc, dev, 0, port);
> +	if (err) {
> +		dev_err(dev, "Enable BIOS post code I/O port 0 failed\n");
> +		return err;
> +	}
> +
> +	/*
> +	 * Configuration of second BPC channel port is optional
> +	 * Double-Word Capture ignoring address 2
> +	 */
> +	if (!bpc->en_dwcap) {
> +		if (of_property_read_u32_index(dev->of_node,
> +					       "nuvoton,monitor-ports", 1,
> +					       &port) == 0) {
> +			err = npcm_enable_bpc(bpc, dev, 1, port);
> +			if (err) {
> +				dev_err(dev, "Enable BIOS post code I/O port 1 failed, disable I/O port 0\n");
> +				npcm_disable_bpc(bpc, 0);
> +				return err;
> +			}
> +		}
> +	}
> +
> +	pr_info("NPCM BIOS Post Code probe\n");

Drop

> +
> +	return err;
> +}
> +
> +static int npcm_bpc_remove(struct platform_device *pdev)
> +{
> +	struct npcm_bpc *bpc = dev_get_drvdata(&pdev->dev);
> +	u8 reg_en;
> +
> +	reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG);
> +
> +	if (reg_en & FIFO_IOADDR1_ENABLE)
> +		npcm_disable_bpc(bpc, 0);
> +	if (reg_en & FIFO_IOADDR2_ENABLE)
> +		npcm_disable_bpc(bpc, 1);
> +

Best regards,
Krzysztof