[PATCH v1 0/2] soc: add NPCM LPC BPC driver support

[PATCH v1 0/2] soc: add NPCM LPC BPC driver support

[Tomer Maimon]

This patch set adds LPC BIOS Post code (BPC) support for the
Nuvoton NPCM Baseboard Management Controller (BMC).

Nuvoton BMC NPCM LPC BIOS Post Code (BPC) monitoring two
configurable I/O addresses written by the host on the
Low Pin Count (LPC) bus, the capture data stored in 128-word FIFO.

NPCM BPC can support capture double words.

The NPCM7xx BPC driver tested on NPCM750 evaluation board.

NPCM BPC driver upstream process start few months ago on misc folder
http://lkml.iu.edu/hypermail/linux/kernel/1904.2/00412.html

The NPCM LPC BPC is similar to Aspeed LPC snoop, last 
kernel 5.0.2 Aspeed LPC snoop driver moved from misc folder to 
soc folder, so it seems NPCM BPC dirver should upstream to soc 
as well.
https://lkml.org/lkml/2019/4/22/377

I have created common lpc-snoop documentation for both 
Nuvoton and Aspeed drivers as Andrew suggested.
Andrew Jeffery: https://patchwork.kernel.org/patch/10506269/ 

I add Andrew and Rob reviewed signature because they already reviewed 
and signed  the lpc-snoop documentation in the misc folder
https://lkml.org/lkml/2019/4/29/998 

Tomer Maimon (2):
  dt-binding: soc: Add common LPC snoop documentation
  soc: nuvoton: add NPCM LPC BPC driver

 .../devicetree/bindings/soc/lpc/lpc-snoop.txt |  27 ++
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/nuvoton/Kconfig                   |  16 +
 drivers/soc/nuvoton/Makefile                  |   2 +
 drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c      | 387 ++++++++++++++++++
 6 files changed, 434 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/lpc/lpc-snoop.txt
 create mode 100644 drivers/soc/nuvoton/Kconfig
 create mode 100644 drivers/soc/nuvoton/Makefile
 create mode 100644 drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c

-- 
2.18.0

[PATCH v1 1/2] dt-binding: soc: Add common LPC snoop documentation

[Tomer Maimon]

Added device tree binding documentation for Nuvoton BMC
NPCM BIOS Post Code (BPC) and Aspeed AST2500 LPC snoop.
The LPC snoop monitoring two configurable I/O addresses
written by the host on Low Pin Count (LPC) bus.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/soc/lpc/lpc-snoop.txt | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/lpc/lpc-snoop.txt

diff --git a/Documentation/devicetree/bindings/soc/lpc/lpc-snoop.txt b/Documentation/devicetree/bindings/soc/lpc/lpc-snoop.txt
new file mode 100644
index 000000000000..c21cb8df4ffb
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/lpc/lpc-snoop.txt
@@ -0,0 +1,27 @@
+LPC snoop interface
+
+The LPC snoop (BIOS Post Code) interface can monitor
+two configurable I/O addresses written by the host on
+the Low Pin Count (LPC) bus.
+
+Nuvoton NPCM7xx LPC snoop supports capture double words,
+when using capture double word only I/O address 1 is monitored.
+
+Required properties for lpc-snoop node
+- compatible   : "nuvoton,npcm750-lpc-bpc-snoop" for Poleg NPCM7XX
+                 "aspeed,ast2500-lpc-snoop" for Aspeed AST2500.
+- reg          : specifies physical base address and size of the registers.
+- interrupts   : contain the LPC snoop interrupt with flags for falling edge.
+- snoop-ports  : contain monitor I/O addresses, at least one monitor I/O
+                 address required
+
+Optional property for NPCM7xx lpc-snoop node
+- nuvoton,lpc-en-dwcapture : enable capture double words support.
+
+Example:
+	lpc-snoop: lpc_snoop@f0007040 {
+		compatible = "nuvoton,npcm750-lpc-bpc-snoop";
+		reg = <0xf0007040 0x14>;
+		snoop-ports = <0x80>;
+		interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+	};
-- 
2.18.0

[PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Tomer Maimon]

Add Nuvoton BMC NPCM BIOS post code (BPC) driver.

The NPCM BPC monitoring two I/O address written by
the host on the Low Pin Count (LPC) bus, the capure
data stored in 128-word FIFO.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/soc/Kconfig                      |   1 +
 drivers/soc/Makefile                     |   1 +
 drivers/soc/nuvoton/Kconfig              |  16 +
 drivers/soc/nuvoton/Makefile             |   2 +
 drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c | 387 +++++++++++++++++++++++
 5 files changed, 407 insertions(+)
 create mode 100644 drivers/soc/nuvoton/Kconfig
 create mode 100644 drivers/soc/nuvoton/Makefile
 create mode 100644 drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 833e04a7835c..9a04c7208f8f 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -10,6 +10,7 @@ source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/imx/Kconfig"
 source "drivers/soc/ixp4xx/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
+source "drivers/soc/nuvoton/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/renesas/Kconfig"
 source "drivers/soc/rockchip/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 524ecdc2a9bb..75018b656412 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_ARCH_MXC)		+= imx/
 obj-$(CONFIG_ARCH_IXP4XX)	+= ixp4xx/
 obj-$(CONFIG_SOC_XWAY)		+= lantiq/
 obj-y				+= mediatek/
+obj-$(CONFIG_SOC_NUVOTON)	+= nuvoton/
 obj-y				+= amlogic/
 obj-y				+= qcom/
 obj-y				+= renesas/
diff --git a/drivers/soc/nuvoton/Kconfig b/drivers/soc/nuvoton/Kconfig
new file mode 100644
index 000000000000..5490e2e5dade
--- /dev/null
+++ b/drivers/soc/nuvoton/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menu "Nuvoton NPCM BMC SoC drivers"
+
+config SOC_NUVOTON
+	def_bool y
+	depends on ARCH_NPCM || COMPILE_TEST
+
+config NPCM_LPC_BPC_SNOOP
+	tristate "NPCM LPC BIOS Post Code snoop support"
+	depends on (SOC_NUVOTON || COMPILE_TEST)
+	help
+	  Provides a NPCM BMC driver to control the LPC BIOS Post Code
+	  interface which allows the BMC to monitoring and save
+	  the data written by the host to an arbitrary LPC I/O port.
+
+endmenu
diff --git a/drivers/soc/nuvoton/Makefile b/drivers/soc/nuvoton/Makefile
new file mode 100644
index 000000000000..ffd0fb05d021
--- /dev/null
+++ b/drivers/soc/nuvoton/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_NPCM_LPC_BPC_SNOOP) += npcm-lpc-bpc-snoop.o
diff --git a/drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c b/drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c
new file mode 100644
index 000000000000..32c7fa254cc1
--- /dev/null
+++ b/drivers/soc/nuvoton/npcm-lpc-bpc-snoop.c
@@ -0,0 +1,387 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2014-2018 Nuvoton Technology corporation.
+
+#include <linux/fs.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/kfifo.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/miscdevice.h>
+#include <linux/poll.h>
+
+#define DEVICE_NAME	"npcm-lpc-bpc"
+
+/* BIOS POST Code FIFO Registers */
+#define NPCM_BPCFA2L_REG	0x2 //BIOS POST Code FIFO Address 2 LSB
+#define NPCM_BPCFA2M_REG	0x4 //BIOS POST Code FIFO Address 2 MSB
+#define NPCM_BPCFEN_REG		0x6 //BIOS POST Code FIFO Enable
+#define NPCM_BPCFSTAT_REG	0x8 //BIOS POST Code FIFO Status
+#define NPCM_BPCFDATA_REG	0xA //BIOS POST Code FIFO Data
+#define NPCM_BPCFMSTAT_REG	0xC //BIOS POST Code FIFO Miscellaneous Status
+#define NPCM_BPCFA1L_REG	0x10 //BIOS POST Code FIFO Address 1 LSB
+#define NPCM_BPCFA1M_REG	0x12 //BIOS POST Code FIFO Address 1 MSB
+
+/*BIOS regiser data*/
+#define FIFO_IOADDR1_ENABLE	0x80
+#define FIFO_IOADDR2_ENABLE	0x40
+
+/* BPC interface package and structure definition */
+#define BPC_KFIFO_SIZE		0x400
+
+/*BPC regiser data*/
+#define FIFO_DATA_VALID		0x80
+#define FIFO_OVERFLOW		0x20
+#define FIFO_READY_INT_ENABLE	0x8
+#define FIFO_DWCAPTURE		0x4
+#define FIFO_ADDR_DECODE	0x1
+
+/*Host Reset*/
+#define HOST_RESET_INT_ENABLE	0x10
+#define HOST_RESET_CHANGED	0x40
+
+#define NUM_BPC_CHANNELS	2
+#define DW_PAD_SIZE		3
+
+struct npcm_bpc_channel {
+	struct miscdevice	miscdev;
+	wait_queue_head_t	wq;
+	struct npcm_bpc		*data;
+	struct kfifo		fifo;
+	bool			host_reset;
+};
+
+struct npcm_bpc {
+	struct npcm_bpc_channel	ch[NUM_BPC_CHANNELS];
+	void __iomem		*base;
+	int			irq;
+	bool			en_dwcap;
+};
+
+static struct npcm_bpc_channel *npcm_file_to_ch(struct file *file)
+{
+	return container_of(file->private_data, struct npcm_bpc_channel,
+			    miscdev);
+}
+
+static ssize_t npcm_bpc_read(struct file *file, char __user *buffer,
+			     size_t count, loff_t *ppos)
+{
+	struct npcm_bpc_channel *chan = npcm_file_to_ch(file);
+	struct npcm_bpc *lpc_bpc = chan->data;
+	unsigned int copied;
+	int cond_size = 1;
+	int ret = 0;
+
+	if (lpc_bpc->en_dwcap)
+		cond_size = 3;
+
+	if (kfifo_len(&chan->fifo) < cond_size) {
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible
+			(chan->wq, kfifo_len(&chan->fifo) > cond_size);
+		if (ret == -ERESTARTSYS)
+			return -EINTR;
+	}
+
+	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
+
+	return ret ? ret : copied;
+}
+
+static __poll_t npcm_bpc_poll(struct file *file, struct poll_table_struct *pt)
+{
+	struct npcm_bpc_channel *chan = npcm_file_to_ch(file);
+	__poll_t mask = 0;
+
+	poll_wait(file, &chan->wq, pt);
+	if (!kfifo_is_empty(&chan->fifo))
+		mask |= POLLIN;
+
+	if (chan->host_reset) {
+		mask |= POLLHUP;
+		chan->host_reset = false;
+	}
+
+	return mask;
+}
+
+static const struct file_operations npcm_bpc_fops = {
+	.owner		= THIS_MODULE,
+	.read		= npcm_bpc_read,
+	.poll		= npcm_bpc_poll,
+	.llseek		= noop_llseek,
+};
+
+static irqreturn_t npcm_bpc_irq(int irq, void *arg)
+{
+	struct npcm_bpc *lpc_bpc = arg;
+	bool isr_flag = false;
+	u8 last_addr_bit = 0;
+	u8 padzero[3] = {0};
+	u8 addr_index = 0;
+	u8 fifo_st;
+	u8 host_st;
+	u8 Data;
+
+	fifo_st = ioread8(lpc_bpc->base + NPCM_BPCFSTAT_REG);
+	while (FIFO_DATA_VALID & fifo_st) {
+		 /* If dwcapture enabled only channel 0 (FIFO 0) used */
+		if (!lpc_bpc->en_dwcap)
+			addr_index = fifo_st & FIFO_ADDR_DECODE;
+		else
+			last_addr_bit = fifo_st & FIFO_ADDR_DECODE;
+
+		/* Read data from FIFO to clear interrupt */
+		Data = ioread8(lpc_bpc->base + NPCM_BPCFDATA_REG);
+		if (kfifo_is_full(&lpc_bpc->ch[addr_index].fifo))
+			kfifo_skip(&lpc_bpc->ch[addr_index].fifo);
+
+		kfifo_put(&lpc_bpc->ch[addr_index].fifo, Data);
+		if (fifo_st & FIFO_OVERFLOW)
+			dev_warn(lpc_bpc->ch->miscdev.this_device, "BIOS Post Codes FIFO Overflow\n");
+
+		fifo_st = ioread8(lpc_bpc->base + NPCM_BPCFSTAT_REG);
+		if (lpc_bpc->en_dwcap && last_addr_bit) {
+			if ((fifo_st & FIFO_ADDR_DECODE) ||
+			    ((FIFO_DATA_VALID & fifo_st) == 0)) {
+				while (kfifo_avail(&lpc_bpc->ch[addr_index].fifo) < DW_PAD_SIZE)
+					kfifo_skip(&lpc_bpc->ch[addr_index].fifo);
+				kfifo_in(&lpc_bpc->ch[addr_index].fifo,
+					 padzero, DW_PAD_SIZE);
+			}
+		}
+		isr_flag = true;
+	}
+
+	host_st = ioread8(lpc_bpc->base + NPCM_BPCFMSTAT_REG);
+	if (host_st & HOST_RESET_CHANGED) {
+		iowrite8(HOST_RESET_CHANGED,
+			 lpc_bpc->base + NPCM_BPCFMSTAT_REG);
+		lpc_bpc->ch[addr_index].host_reset = true;
+		isr_flag = true;
+	}
+
+	if (isr_flag) {
+		wake_up_interruptible(&lpc_bpc->ch[addr_index].wq);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int npcm_bpc_config_irq(struct npcm_bpc *lpc_bpc,
+			       struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	lpc_bpc->irq = platform_get_irq(pdev, 0);
+	if (lpc_bpc->irq < 0) {
+		dev_err(dev, "get IRQ failed\n");
+		return lpc_bpc->irq;
+	}
+
+	rc = devm_request_irq(dev, lpc_bpc->irq,
+			      npcm_bpc_irq, IRQF_SHARED,
+			      DEVICE_NAME, lpc_bpc);
+	if (rc < 0) {
+		dev_err(dev, "Unable to request IRQ %d\n", lpc_bpc->irq);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int npcm_enable_bpc(struct npcm_bpc *lpc_bpc, struct device *dev,
+			   int channel, u16 lpc_port)
+{
+	u8 addr_en;
+	u8 reg_en;
+	int rc;
+
+	init_waitqueue_head(&lpc_bpc->ch[channel].wq);
+
+	rc = kfifo_alloc(&lpc_bpc->ch[channel].fifo,
+			 BPC_KFIFO_SIZE, GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	lpc_bpc->ch[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
+	lpc_bpc->ch[channel].miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
+	lpc_bpc->ch[channel].miscdev.fops = &npcm_bpc_fops;
+	lpc_bpc->ch[channel].miscdev.parent = dev;
+	rc = misc_register(&lpc_bpc->ch[channel].miscdev);
+	if (rc)
+		return rc;
+
+	lpc_bpc->ch[channel].data = lpc_bpc;
+	lpc_bpc->ch[channel].host_reset = false;
+
+	/* Enable LPC snoop channel at requested port */
+	switch (channel) {
+	case 0:
+		addr_en = FIFO_IOADDR1_ENABLE;
+		iowrite8((u8)lpc_port & 0xFF,
+			 lpc_bpc->base + NPCM_BPCFA1L_REG);
+		iowrite8((u8)(lpc_port >> 8),
+			 lpc_bpc->base + NPCM_BPCFA1M_REG);
+		break;
+	case 1:
+		addr_en = FIFO_IOADDR2_ENABLE;
+		iowrite8((u8)lpc_port & 0xFF,
+			 lpc_bpc->base + NPCM_BPCFA2L_REG);
+		iowrite8((u8)(lpc_port >> 8),
+			 lpc_bpc->base + NPCM_BPCFA2M_REG);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (lpc_bpc->en_dwcap)
+		addr_en = FIFO_DWCAPTURE;
+
+	/*
+	 * Enable FIFO Ready Interrupt, FIFO Capture of I/O addr,
+	 * and Host Reset
+	 */
+	reg_en = ioread8(lpc_bpc->base + NPCM_BPCFEN_REG);
+	iowrite8(reg_en | addr_en | FIFO_READY_INT_ENABLE |
+		 HOST_RESET_INT_ENABLE, lpc_bpc->base + NPCM_BPCFEN_REG);
+
+	return 0;
+}
+
+static void npcm_disable_bpc(struct npcm_bpc *lpc_bpc, int channel)
+{
+	u8 reg_en;
+
+	switch (channel) {
+	case 0:
+		reg_en = ioread8(lpc_bpc->base + NPCM_BPCFEN_REG);
+		if (lpc_bpc->en_dwcap)
+			iowrite8(reg_en & ~FIFO_DWCAPTURE,
+				 lpc_bpc->base + NPCM_BPCFEN_REG);
+		else
+			iowrite8(reg_en & ~FIFO_IOADDR1_ENABLE,
+				 lpc_bpc->base + NPCM_BPCFEN_REG);
+		break;
+	case 1:
+		reg_en = ioread8(lpc_bpc->base + NPCM_BPCFEN_REG);
+		iowrite8(reg_en & ~FIFO_IOADDR2_ENABLE,
+			 lpc_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),
+			 lpc_bpc->base + NPCM_BPCFEN_REG);
+
+	kfifo_free(&lpc_bpc->ch[channel].fifo);
+	misc_deregister(&lpc_bpc->ch[channel].miscdev);
+}
+
+static int npcm_bpc_probe(struct platform_device *pdev)
+{
+	struct npcm_bpc *lpc_bpc;
+	struct device *dev;
+	u32 port;
+	int rc;
+
+	dev = &pdev->dev;
+
+	lpc_bpc = devm_kzalloc(dev, sizeof(*lpc_bpc), GFP_KERNEL);
+	if (!lpc_bpc)
+		return -ENOMEM;
+
+	lpc_bpc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lpc_bpc->base))
+		return PTR_ERR(lpc_bpc->base);
+
+	dev_set_drvdata(&pdev->dev, lpc_bpc);
+
+	rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0,
+					&port);
+	if (rc) {
+		dev_err(dev, "no snoop ports configured\n");
+		return -ENODEV;
+	}
+
+	lpc_bpc->en_dwcap =
+		of_property_read_bool(dev->of_node, "nuvoton,lpc-en-dwcapture");
+
+	rc = npcm_bpc_config_irq(lpc_bpc, pdev);
+	if (rc)
+		return rc;
+
+	rc = npcm_enable_bpc(lpc_bpc, dev, 0, port);
+	if (rc) {
+		dev_err(dev, "Enable BIOS post code I/O port 0 failed\n");
+		return rc;
+	}
+
+	/*
+	 * Configuration of second BPC channel port is optional
+	 * Double-Word Capture ignoring address 2
+	 */
+	if (!lpc_bpc->en_dwcap) {
+		if (of_property_read_u32_index(dev->of_node, "snoop-ports",
+					       1, &port) == 0) {
+			rc = npcm_enable_bpc(lpc_bpc, dev, 1, port);
+			if (rc) {
+				dev_err(dev, "Enable BIOS post code I/O port 1 failed, disable I/O port 0\n");
+				npcm_disable_bpc(lpc_bpc, 0);
+				return rc;
+			}
+		}
+	}
+
+	pr_info("NPCM BIOS post code probe\n");
+
+	return rc;
+}
+
+static int npcm_bpc_remove(struct platform_device *pdev)
+{
+	struct npcm_bpc *lpc_bpc = dev_get_drvdata(&pdev->dev);
+	u8 reg_en;
+
+	reg_en = ioread8(lpc_bpc->base + NPCM_BPCFEN_REG);
+
+	if (reg_en & FIFO_IOADDR1_ENABLE)
+		npcm_disable_bpc(lpc_bpc, 0);
+	if (reg_en & FIFO_IOADDR2_ENABLE)
+		npcm_disable_bpc(lpc_bpc, 1);
+
+	return 0;
+}
+
+static const struct of_device_id npcm_bpc_match[] = {
+	{ .compatible = "nuvoton,npcm750-lpc-bpc-snoop" },
+	{ },
+};
+
+static struct platform_driver npcm_bpc_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = npcm_bpc_match,
+	},
+	.probe = npcm_bpc_probe,
+	.remove = npcm_bpc_remove,
+};
+
+module_platform_driver(npcm_bpc_driver);
+
+MODULE_DEVICE_TABLE(of, npcm_bpc_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
+MODULE_DESCRIPTION("Linux driver to control NPCM LPC BIOS post code snooping");
-- 
2.18.0

Re: [PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Arnd Bergmann]

On Mon, Jun 10, 2019 at 4:19 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
>
> The NPCM BPC monitoring two I/O address written by
> the host on the Low Pin Count (LPC) bus, the capure
> data stored in 128-word FIFO.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

We've run into this situation before, but don't have a good solution yet:

The driver seems useful and well implemented, but I keep having a bad
feeling about adding a chardev driver into drivers/soc for something that
is clearly specific to a particular implementation on the hardware side
but generic on the user interface. The same user interface might be
used on an Aspeed BMC or any other one, so please coordinate at
least between Novoton and Aspeed developers on creating a common
user interface, and review each other's patches.

Maybe we can introduce a drivers/bmc/ (or even drivers/openbmc)
that collects all those user interfaces with a thin abstraction layer
and one or two hardware specific back-ends?

        Arnd

Re: [PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Arnd Bergmann]

On Mon, Jun 10, 2019 at 4:19 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
>
> The NPCM BPC monitoring two I/O address written by
> the host on the Low Pin Count (LPC) bus, the capure
> data stored in 128-word FIFO.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

We've run into this situation before, but don't have a good solution yet:

The driver seems useful and well implemented, but I keep having a bad
feeling about adding a chardev driver into drivers/soc for something that
is clearly specific to a particular implementation on the hardware side
but generic on the user interface. The same user interface might be
used on an Aspeed BMC or any other one, so please coordinate at
least between Novoton and Aspeed developers on creating a common
user interface, and review each other's patches.

Maybe we can introduce a drivers/bmc/ (or even drivers/openbmc)
that collects all those user interfaces with a thin abstraction layer
and one or two hardware specific back-ends?

        Arnd

Re: [PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Tomer Maimon]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

Adding Patrick and Joel to this discussion,

Thanks for your prompt reply!

On Tue, 11 Jun 2019 at 16:12, Arnd Bergmann <arnd@arndb.de> wrote:

> On Mon, Jun 10, 2019 at 4:19 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
> >
> > The NPCM BPC monitoring two I/O address written by
> > the host on the Low Pin Count (LPC) bus, the capure
> > data stored in 128-word FIFO.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> We've run into this situation before, but don't have a good solution yet:
>
> The driver seems useful and well implemented, but I keep having a bad
> feeling about adding a chardev driver into drivers/soc for something that
> is clearly specific to a particular implementation on the hardware side
> but generic on the user interface. The same user interface might be
> used on an Aspeed BMC or any other one, so please coordinate at
> least between Novoton and Aspeed developers on creating a common
> user interface, and review each other's patches.
>

Probably the only vendors that will use the snoop will be Nuvoton and
Aspeed.
is it worth to create new snoop common user interface for it,
if we will develop a new snoop user interface who will be the maintainer?


> Maybe we can introduce a drivers/bmc/ (or even drivers/openbmc)
>
that collects all those user interfaces with a thin abstraction layer
> and one or two hardware specific back-ends?
>

Sounds good, Maybe we can move the KCS BMC from driver/char/ipmi to the
drivers/bmc/?


>         Arnd
>

appreciate your opinions, Joel, Andrew and Patrick?

Thanks,

Tomer

[-- Attachment #2: Type: text/html, Size: 2856 bytes --]

Re: [PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Tomer Maimon]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

Adding Patrick and Joel to this discussion,

Thanks for your prompt reply!

On Tue, 11 Jun 2019 at 16:12, Arnd Bergmann <arnd@arndb.de> wrote:

> On Mon, Jun 10, 2019 at 4:19 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
> >
> > The NPCM BPC monitoring two I/O address written by
> > the host on the Low Pin Count (LPC) bus, the capure
> > data stored in 128-word FIFO.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> We've run into this situation before, but don't have a good solution yet:
>
> The driver seems useful and well implemented, but I keep having a bad
> feeling about adding a chardev driver into drivers/soc for something that
> is clearly specific to a particular implementation on the hardware side
> but generic on the user interface. The same user interface might be
> used on an Aspeed BMC or any other one, so please coordinate at
> least between Novoton and Aspeed developers on creating a common
> user interface, and review each other's patches.
>

Probably the only vendors that will use the snoop will be Nuvoton and
Aspeed.
is it worth to create new snoop common user interface for it,
if we will develop a new snoop user interface who will be the maintainer?


> Maybe we can introduce a drivers/bmc/ (or even drivers/openbmc)
>
that collects all those user interfaces with a thin abstraction layer
> and one or two hardware specific back-ends?
>

Sounds good, Maybe we can move the KCS BMC from driver/char/ipmi to the
drivers/bmc/?


>         Arnd
>

appreciate your opinions, Joel, Andrew and Patrick?

Thanks,

Tomer

[-- Attachment #2: Type: text/html, Size: 2856 bytes --]

Re: [PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Arnd Bergmann]

On Thu, Jun 13, 2019 at 11:18 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
>
> Probably the only vendors that will use the snoop will be Nuvoton and Aspeed.
> is it worth to create new snoop common user interface for it,
> if we will develop a new snoop user interface who will be the maintainer?

One or more of you will have to volunteer to maintain the new subsystem.

There are lots of ways this can be structured, and once you have
a maintainer (team), they can decide how to do it, but I'm available
to come up with ideas here.

Generally speaking, you don't need a ton of abstraction. The
drivers/watchdog subsystem could serve as a template there.
This has both models, the old way in which each HW specific
driver uses its own chardev, and the new model in which the
common code sits in a library module, and individual drivers
register to it.

I think the amount of code for two drivers is roughly the same
in either model, but the shared user interface implementation
makes it easier to ensure that the interfaces are in fact
compatible.

The other thing your own framework can do is to provide some
consistency between BMC specific drivers for different
functionality.

>> Maybe we can introduce a drivers/bmc/ (or even drivers/openbmc)
>>
>> that collects all those user interfaces with a thin abstraction layer
>> and one or two hardware specific back-ends?
>
> Sounds good, Maybe we can move the KCS BMC from driver/char/ipmi to the drivers/bmc/?

Good idea. Yes, please.

       Arnd

Re: [PATCH v1 2/2] soc: nuvoton: add NPCM LPC BPC driver

[Arnd Bergmann]

On Thu, Jun 13, 2019 at 11:18 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
>
> Probably the only vendors that will use the snoop will be Nuvoton and Aspeed.
> is it worth to create new snoop common user interface for it,
> if we will develop a new snoop user interface who will be the maintainer?

One or more of you will have to volunteer to maintain the new subsystem.

There are lots of ways this can be structured, and once you have
a maintainer (team), they can decide how to do it, but I'm available
to come up with ideas here.

Generally speaking, you don't need a ton of abstraction. The
drivers/watchdog subsystem could serve as a template there.
This has both models, the old way in which each HW specific
driver uses its own chardev, and the new model in which the
common code sits in a library module, and individual drivers
register to it.

I think the amount of code for two drivers is roughly the same
in either model, but the shared user interface implementation
makes it easier to ensure that the interfaces are in fact
compatible.

The other thing your own framework can do is to provide some
consistency between BMC specific drivers for different
functionality.

>> Maybe we can introduce a drivers/bmc/ (or even drivers/openbmc)
>>
>> that collects all those user interfaces with a thin abstraction layer
>> and one or two hardware specific back-ends?
>
> Sounds good, Maybe we can move the KCS BMC from driver/char/ipmi to the drivers/bmc/?

Good idea. Yes, please.

       Arnd

[PATCH v1 0/2] soc: add NPCM LPC BPC driver support

[Tomer Maimon]

This patch set adds LPC BIOS Post code (BPC) support for the
Nuvoton NPCM Baseboard Management Controller (BMC).

Nuvoton BMC NPCM LPC BIOS Post Code (BPC) monitoring two
configurable I/O addresses written by the host on the
Low Pin Count (LPC) bus, the capture data stored in 128-word FIFO.

NPCM BPC can support capture double words.

The NPCM LPC BPC driver tested on NPCM750 Olympus board and NPCM845 EVB.

Tomer Maimon (2):
  dt-binding: soc: nuvoton: Add NPCM BPC LPC documentation
  soc: nuvoton: add NPCM LPC BPC driver

 .../bindings/soc/nuvoton/npcm-lpc-bpc.yaml    | 112 +++++
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/nuvoton/Kconfig                   |  24 ++
 drivers/soc/nuvoton/Makefile                  |   3 +
 drivers/soc/nuvoton/npcm-lpc-bpc.c            | 396 ++++++++++++++++++
 6 files changed, 537 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
 create mode 100644 drivers/soc/nuvoton/Kconfig
 create mode 100644 drivers/soc/nuvoton/Makefile
 create mode 100644 drivers/soc/nuvoton/npcm-lpc-bpc.c

-- 
2.33.0

[PATCH v1 0/2] soc: add NPCM LPC BPC driver support

[Tomer Maimon]

This patch set adds LPC BIOS Post code (BPC) support for the
Nuvoton NPCM Baseboard Management Controller (BMC).

Nuvoton BMC NPCM LPC BIOS Post Code (BPC) monitoring two
configurable I/O addresses written by the host on the
Low Pin Count (LPC) bus, the capture data stored in 128-word FIFO.

NPCM BPC can support capture double words.

The NPCM LPC BPC driver tested on NPCM750 Olympus board and NPCM845 EVB.

Tomer Maimon (2):
  dt-binding: soc: nuvoton: Add NPCM BPC LPC documentation
  soc: nuvoton: add NPCM LPC BPC driver

 .../bindings/soc/nuvoton/npcm-lpc-bpc.yaml    | 112 +++++
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/nuvoton/Kconfig                   |  24 ++
 drivers/soc/nuvoton/Makefile                  |   3 +
 drivers/soc/nuvoton/npcm-lpc-bpc.c            | 396 ++++++++++++++++++
 6 files changed, 537 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
 create mode 100644 drivers/soc/nuvoton/Kconfig
 create mode 100644 drivers/soc/nuvoton/Makefile
 create mode 100644 drivers/soc/nuvoton/npcm-lpc-bpc.c

-- 
2.33.0