[PATCH v1 0/3] Add AST2600 LPC PCC support

[PATCH v1 0/3] Add AST2600 LPC PCC support

[Kevin Chen]

The AST2600 has PCC controller in LPC, placed in LPC node. As a result,
add LPC PCC controller driver to support POST code capture.

--
v1:
  - Change pachset from 1.
  - Remove properties in pcc driver.

Kevin Chen (3):
  dt-binding: aspeed: Add LPC PCC controller
  ARM: dts: aspeed-g6: Add AST2600 LPC PCC support
  soc: aspeed: lpc-pcc: Add PCC controller support

 .../devicetree/bindings/mfd/aspeed-lpc.yaml   |  36 ++
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi       |   7 +
 drivers/soc/aspeed/Kconfig                    |  10 +
 drivers/soc/aspeed/Makefile                   |   1 +
 drivers/soc/aspeed/aspeed-lpc-pcc.c           | 439 ++++++++++++++++++
 5 files changed, 493 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-lpc-pcc.c

-- 
2.34.1

[PATCH v1 1/3] dt-binding: aspeed: Add LPC PCC controller

[Kevin Chen]

Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.

Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
---
 .../devicetree/bindings/mfd/aspeed-lpc.yaml   | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml b/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
index 5dfe77aca167..367847bd7e75 100644
--- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
+++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
@@ -149,6 +149,35 @@ patternProperties:
       - interrupts
       - snoop-ports
 
+  "^lpc-pcc@[0-9a-f]+$":
+    type: object
+    additionalProperties: false
+
+    description:
+      The LPC pcc interface allows the BMC to listen on and record the data
+      bytes written by the Host to the targeted LPC I/O pots.
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - aspeed,ast2600-lpc-pcc
+
+      reg:
+        maxItems: 1
+
+      interrupts:
+        maxItems: 1
+
+      pcc-ports:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        description: The LPC I/O ports to pcc
+
+    required:
+      - compatible
+      - interrupts
+      - pcc-ports
+
   "^uart-routing@[0-9a-f]+$":
     $ref: /schemas/soc/aspeed/uart-routing.yaml#
     description: The UART routing control under LPC register space
@@ -176,6 +205,13 @@ examples:
         #size-cells = <1>;
         ranges = <0x0 0x1e789000 0x1000>;
 
+        lpc_pcc: lpc-pcc@0 {
+            compatible = "aspeed,ast2600-lpc-pcc";
+            reg = <0x0 0x140>;
+            interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+            pcc-ports = <0x80>;
+        };
+
         lpc_ctrl: lpc-ctrl@80 {
             compatible = "aspeed,ast2600-lpc-ctrl";
             reg = <0x80 0x80>;
-- 
2.34.1

Re: [PATCH v1 1/3] dt-binding: aspeed: Add LPC PCC controller

[Krzysztof Kozlowski]

On 17/02/2025 12:48, Kevin Chen wrote:
> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
> 
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/mfd/aspeed-lpc.yaml   | 36 +++++++++++++++++++

Nothing improved. You will not get different review.

Also, provide detailed changelog explaining how you fixed here
previously reported issues.


<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

Best regards,
Krzysztof

[PATCH v1 2/3] ARM: dts: aspeed-g6: Add AST2600 LPC PCC support

[Kevin Chen]

The AST2600 has PCC controller in LPC, placed in LPC node.

Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 8ed715bd53aa..87dcacb78692 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -626,6 +626,13 @@ lpc_snoop: lpc-snoop@80 {
 					status = "disabled";
 				};
 
+				lpc_pcc: lpc-pcc@0 {
+					compatible = "aspeed,ast2600-lpc-pcc";
+					reg = <0x0 0x140>;
+					interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+					status = "disabled";
+				};
+
 				lhc: lhc@a0 {
 					compatible = "aspeed,ast2600-lhc";
 					reg = <0xa0 0x24 0xc8 0x8>;
-- 
2.34.1

[PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Kevin Chen]

Add LPC PCC controller driver to support POST code capture.

Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
---
 drivers/soc/aspeed/Kconfig          |  10 +
 drivers/soc/aspeed/Makefile         |   1 +
 drivers/soc/aspeed/aspeed-lpc-pcc.c | 439 ++++++++++++++++++++++++++++
 3 files changed, 450 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-lpc-pcc.c

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f579ee0b5afa..dff1a82f4201 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -52,6 +52,16 @@ config ASPEED_SOCINFO
 	help
 	  Say yes to support decoding of ASPEED BMC information.
 
+config ASPEED_LPC_PCC
+	tristate "Aspeed Post Code Capture support"
+	select REGMAP
+	select MFD_SYSCON
+	default ARCH_ASPEED
+	help
+	  Provides a driver to control the LPC PCC interface,
+	  allowing the BMC to capture post code written by the
+	  the host to an arbitrary LPC I/O port.
+
 endmenu
 
 endif
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..1692350b3512 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
 obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
 obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
+obj-$(CONFIG_ASPEED_LPC_PCC)		+= aspeed-lpc-pcc.o
diff --git a/drivers/soc/aspeed/aspeed-lpc-pcc.c b/drivers/soc/aspeed/aspeed-lpc-pcc.c
new file mode 100644
index 000000000000..a9733dc932a2
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-lpc-pcc.c
@@ -0,0 +1,439 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ */
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/interrupt.h>
+#include <linux/fs.h>
+#include <linux/kfifo.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/dma-mapping.h>
+#include <linux/sizes.h>
+
+#define DEVICE_NAME "aspeed-lpc-pcc"
+
+static DEFINE_IDA(aspeed_pcc_ida);
+
+#define HICR5	0x80
+#define HICR5_EN_SNP0W			BIT(0)
+#define HICR5_EN_SNP1W			BIT(2)
+#define HICR6	0x084
+#define   HICR6_EN2BMODE		BIT(19)
+#define SNPWADR	0x090
+#define PCCR6	0x0c4
+#define   PCCR6_DMA_CUR_ADDR		GENMASK(27, 0)
+#define PCCR4	0x0d0
+#define   PCCR4_DMA_ADDRL_MASK		GENMASK(31, 0)
+#define   PCCR4_DMA_ADDRL_SHIFT		0
+#define PCCR5	0x0d4
+#define   PCCR5_DMA_ADDRH_MASK		GENMASK(27, 24)
+#define   PCCR5_DMA_ADDRH_SHIFT		24
+#define   PCCR5_DMA_LEN_MASK		GENMASK(23, 0)
+#define   PCCR5_DMA_LEN_SHIFT		0
+#define HICRB	0x100
+#define   HICRB_ENSNP0D			BIT(14)
+#define   HICRB_ENSNP1D			BIT(15)
+#define PCCR0	0x130
+#define   PCCR0_EN_DMA_INT		BIT(31)
+#define   PCCR0_EN_DMA_MODE		BIT(14)
+#define   PCCR0_ADDR_SEL_MASK		GENMASK(13, 12)
+#define   PCCR0_ADDR_SEL_SHIFT		12
+#define   PCCR0_RX_TRIG_LVL_MASK	GENMASK(10, 8)
+#define   PCCR0_RX_TRIG_LVL_SHIFT	8
+#define   PCCR0_CLR_RX_FIFO		BIT(7)
+#define   PCCR0_MODE_SEL_MASK		GENMASK(5, 4)
+#define   PCCR0_MODE_SEL_SHIFT		4
+#define   PCCR0_EN_RX_TMOUT_INT		BIT(2)
+#define   PCCR0_EN_RX_AVAIL_INT		BIT(1)
+#define   PCCR0_EN			BIT(0)
+#define PCCR1	0x134
+#define   PCCR1_BASE_ADDR_MASK		GENMASK(15, 0)
+#define   PCCR1_BASE_ADDR_SHIFT		0
+#define   PCCR1_DONT_CARE_BITS_MASK	GENMASK(21, 16)
+#define   PCCR1_DONT_CARE_BITS_SHIFT	16
+#define PCCR2	0x138
+#define   PCCR2_INT_STATUS_PATTERN_B	BIT(16)
+#define   PCCR2_INT_STATUS_PATTERN_A	BIT(8)
+#define   PCCR2_INT_STATUS_DMA_DONE	BIT(4)
+#define   PCCR2_INT_STATUS_DATA_RDY	PCCR2_INT_STATUS_DMA_DONE
+#define   PCCR2_INT_STATUS_RX_OVER	BIT(3)
+#define   PCCR2_INT_STATUS_RX_TMOUT	BIT(2)
+#define   PCCR2_INT_STATUS_RX_AVAIL	BIT(1)
+#define PCCR3	0x13c
+#define   PCCR3_FIFO_DATA_MASK		GENMASK(7, 0)
+
+#define PCC_DMA_BUFSZ	(256 * SZ_1K)
+
+enum pcc_fifo_threshold {
+	PCC_FIFO_THR_1_BYTE,
+	PCC_FIFO_THR_1_EIGHTH,
+	PCC_FIFO_THR_2_EIGHTH,
+	PCC_FIFO_THR_3_EIGHTH,
+	PCC_FIFO_THR_4_EIGHTH,
+	PCC_FIFO_THR_5_EIGHTH,
+	PCC_FIFO_THR_6_EIGHTH,
+	PCC_FIFO_THR_7_EIGHTH,
+	PCC_FIFO_THR_8_EIGHTH,
+};
+
+enum pcc_record_mode {
+	PCC_REC_1B,
+	PCC_REC_2B,
+	PCC_REC_4B,
+	PCC_REC_FULL,
+};
+
+enum pcc_port_hbits_select {
+	PCC_PORT_HBITS_SEL_NONE,
+	PCC_PORT_HBITS_SEL_45,
+	PCC_PORT_HBITS_SEL_67,
+	PCC_PORT_HBITS_SEL_89,
+};
+
+struct aspeed_pcc_dma {
+	uint32_t rptr;
+	uint8_t *virt;
+	dma_addr_t addr;
+	uint32_t size;
+};
+
+struct aspeed_pcc_ctrl {
+	struct device *dev;
+	struct regmap *regmap;
+	int irq;
+	uint32_t port;
+	struct aspeed_pcc_dma dma;
+	struct kfifo fifo;
+	wait_queue_head_t wq;
+	struct miscdevice mdev;
+	int mdev_id;
+};
+
+static inline bool is_valid_rec_mode(uint32_t mode)
+{
+	return (mode > PCC_REC_FULL) ? false : true;
+}
+
+static inline bool is_valid_high_bits_select(uint32_t sel)
+{
+	return (sel > PCC_PORT_HBITS_SEL_89) ? false : true;
+}
+
+static ssize_t aspeed_pcc_file_read(struct file *file, char __user *buffer,
+				    size_t count, loff_t *ppos)
+{
+	int rc;
+	unsigned int copied;
+	struct aspeed_pcc_ctrl *pcc = container_of(file->private_data,
+					      struct aspeed_pcc_ctrl,
+					      mdev);
+
+	if (kfifo_is_empty(&pcc->fifo)) {
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		rc = wait_event_interruptible(pcc->wq,
+					      !kfifo_is_empty(&pcc->fifo));
+		if (rc == -ERESTARTSYS)
+			return -EINTR;
+	}
+
+	rc = kfifo_to_user(&pcc->fifo, buffer, count, &copied);
+
+	return rc ? rc : copied;
+}
+
+static __poll_t aspeed_pcc_file_poll(struct file *file,
+				     struct poll_table_struct *pt)
+{
+	struct aspeed_pcc_ctrl *pcc = container_of(file->private_data, struct aspeed_pcc_ctrl, mdev);
+
+	poll_wait(file, &pcc->wq, pt);
+
+	return !kfifo_is_empty(&pcc->fifo) ? POLLIN : 0;
+}
+
+static const struct file_operations pcc_fops = {
+	.owner = THIS_MODULE,
+	.read = aspeed_pcc_file_read,
+	.poll = aspeed_pcc_file_poll,
+};
+
+static irqreturn_t aspeed_pcc_dma_isr(int irq, void *arg)
+{
+	uint32_t reg, rptr, wptr;
+	struct aspeed_pcc_ctrl *pcc = (struct aspeed_pcc_ctrl *)arg;
+	struct kfifo *fifo = &pcc->fifo;
+
+	regmap_write_bits(pcc->regmap, PCCR2, PCCR2_INT_STATUS_DMA_DONE, PCCR2_INT_STATUS_DMA_DONE);
+
+	regmap_read(pcc->regmap, PCCR6, &reg);
+	wptr = (reg & PCCR6_DMA_CUR_ADDR) - (pcc->dma.addr & PCCR6_DMA_CUR_ADDR);
+	rptr = pcc->dma.rptr;
+
+	do {
+		if (kfifo_is_full(fifo))
+			kfifo_skip(fifo);
+
+		kfifo_put(fifo, pcc->dma.virt[rptr]);
+
+		rptr = (rptr + 1) % pcc->dma.size;
+	} while (rptr != wptr);
+
+	pcc->dma.rptr = rptr;
+
+	wake_up_interruptible(&pcc->wq);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t aspeed_pcc_isr(int irq, void *arg)
+{
+	uint32_t sts;
+	struct aspeed_pcc_ctrl *pcc = (struct aspeed_pcc_ctrl *)arg;
+
+	regmap_read(pcc->regmap, PCCR2, &sts);
+
+	if (!(sts & (PCCR2_INT_STATUS_RX_TMOUT |
+		     PCCR2_INT_STATUS_RX_AVAIL |
+		     PCCR2_INT_STATUS_DMA_DONE)))
+		return IRQ_NONE;
+
+	return aspeed_pcc_dma_isr(irq, arg);
+}
+
+/*
+ * A2600-15 AP note
+ *
+ * SW workaround to prevent generating Non-Fatal-Error (NFE)
+ * eSPI response when PCC is used for port I/O byte snooping
+ * over eSPI.
+ */
+static int aspeed_a2600_15(struct aspeed_pcc_ctrl *pcc, struct device *dev)
+{
+	u32 hicr5_en, hicrb_en;
+
+	/* abort if snoop is enabled */
+	regmap_read(pcc->regmap, HICR5, &hicr5_en);
+	if (hicr5_en & (HICR5_EN_SNP0W | HICR5_EN_SNP1W)) {
+		dev_err(dev, "A2600-15 should be applied with snoop disabled\n");
+		return -EPERM;
+	}
+
+	/* set SNPWADR of snoop device */
+	regmap_write(pcc->regmap, SNPWADR, pcc->port | ((pcc->port + 2) << 16));
+
+	/* set HICRB[15:14]=11b to enable ACCEPT response for SNPWADR */
+	hicrb_en = HICRB_ENSNP0D | HICRB_ENSNP1D;
+	regmap_update_bits(pcc->regmap, HICRB, hicrb_en, hicrb_en);
+
+	/* set HICR6[19] to extend SNPWADR to 2x range */
+	regmap_update_bits(pcc->regmap, HICR6, HICR6_EN2BMODE, HICR6_EN2BMODE);
+
+	return 0;
+}
+
+static int aspeed_pcc_enable(struct aspeed_pcc_ctrl *pcc, struct device *dev)
+{
+	int rc;
+
+	rc = aspeed_a2600_15(pcc, dev);
+	if (rc)
+		return rc;
+
+	/* record mode: Set 2-Byte mode. */
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_MODE_SEL_MASK,
+			   PCC_REC_2B << PCCR0_MODE_SEL_SHIFT);
+
+	/* port address */
+	regmap_update_bits(pcc->regmap, PCCR1,
+			   PCCR1_BASE_ADDR_MASK,
+			   pcc->port << PCCR1_BASE_ADDR_SHIFT);
+
+	/* Set address high bits selection to 0b01 for address bit[5:4] */
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_ADDR_SEL_MASK,
+			   PCC_PORT_HBITS_SEL_45 << PCCR0_ADDR_SEL_SHIFT);
+
+	/* Set LPC don't care address to 0x3 for port 80~83h */
+	regmap_update_bits(pcc->regmap, PCCR1,
+			   PCCR1_DONT_CARE_BITS_MASK,
+			   0x3 << PCCR1_DONT_CARE_BITS_SHIFT);
+
+	/* set DMA ring buffer size and enable interrupts */
+	regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
+#ifdef CONFIG_ARM64
+	regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_ADDRH_MASK,
+			   (pcc->dma.addr >> 32) << PCCR5_DMA_ADDRH_SHIFT);
+#endif
+	regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_LEN_MASK,
+			   (pcc->dma.size / 4) << PCCR5_DMA_LEN_SHIFT);
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE,
+			   PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE);
+
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
+
+	return 0;
+}
+
+static int aspeed_pcc_disable(struct aspeed_pcc_ctrl *pcc)
+{
+	/* Disable PCC and DMA Mode for safety */
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN |  PCCR0_EN_DMA_MODE, 0);
+
+	/* Clear Rx FIFO. */
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_CLR_RX_FIFO, 1);
+
+	/* Clear All interrupts status. */
+	regmap_write(pcc->regmap, PCCR2,
+		     PCCR2_INT_STATUS_RX_OVER | PCCR2_INT_STATUS_DMA_DONE |
+		     PCCR2_INT_STATUS_PATTERN_A | PCCR2_INT_STATUS_PATTERN_B);
+
+	return 0;
+}
+
+static int aspeed_pcc_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct aspeed_pcc_ctrl *pcc;
+	struct device *dev;
+	uint32_t fifo_size = PAGE_SIZE;
+
+	dev = &pdev->dev;
+
+	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
+	if (!pcc)
+		return -ENOMEM;
+
+	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(pcc->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		return -ENODEV;
+	}
+
+	rc = of_property_read_u32(dev->of_node, "pcc-ports", &pcc->port);
+	if (rc) {
+		dev_err(dev, "no pcc ports configured\n");
+		return -ENODEV;
+	}
+
+	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	if (rc) {
+		dev_err(dev, "cannot set 64-bits DMA mask\n");
+		return rc;
+	}
+
+	pcc->dma.size = PCC_DMA_BUFSZ;
+	pcc->dma.virt = dmam_alloc_coherent(dev,
+					    pcc->dma.size,
+					    &pcc->dma.addr,
+					    GFP_KERNEL);
+	if (!pcc->dma.virt) {
+		dev_err(dev, "cannot allocate DMA buffer\n");
+		return -ENOMEM;
+	}
+
+	fifo_size = roundup(pcc->dma.size, PAGE_SIZE);
+	rc = kfifo_alloc(&pcc->fifo, fifo_size, GFP_KERNEL);
+	if (rc) {
+		dev_err(dev, "cannot allocate kFIFO\n");
+		return -ENOMEM;
+	}
+
+	/* Disable PCC to clean up DMA buffer before request IRQ. */
+	rc = aspeed_pcc_disable(pcc);
+	if (rc) {
+		dev_err(dev, "Couldn't disable PCC\n");
+		goto err_free_kfifo;
+	}
+
+	pcc->irq = platform_get_irq(pdev, 0);
+	if (pcc->irq < 0) {
+		dev_err(dev, "Couldn't get IRQ\n");
+		rc = -ENODEV;
+		goto err_free_kfifo;
+	}
+
+	rc = devm_request_irq(dev, pcc->irq, aspeed_pcc_isr, 0, DEVICE_NAME, pcc);
+	if (rc < 0) {
+		dev_err(dev, "Couldn't request IRQ %d\n", pcc->irq);
+		goto err_free_kfifo;
+	}
+
+	init_waitqueue_head(&pcc->wq);
+
+	pcc->mdev_id = ida_alloc(&aspeed_pcc_ida, GFP_KERNEL);
+	if (pcc->mdev_id < 0) {
+		dev_err(dev, "Couldn't allocate ID\n");
+		return pcc->mdev_id;
+	}
+
+	pcc->mdev.parent = dev;
+	pcc->mdev.minor = MISC_DYNAMIC_MINOR;
+	pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
+					pcc->mdev_id);
+	pcc->mdev.fops = &pcc_fops;
+	rc = misc_register(&pcc->mdev);
+	if (rc) {
+		dev_err(dev, "Couldn't register misc device\n");
+		goto err_free_kfifo;
+	}
+
+	rc = aspeed_pcc_enable(pcc, dev);
+	if (rc) {
+		dev_err(dev, "Couldn't enable PCC\n");
+		goto err_dereg_mdev;
+	}
+
+	dev_set_drvdata(&pdev->dev, pcc);
+
+	return 0;
+
+err_dereg_mdev:
+	misc_deregister(&pcc->mdev);
+
+err_free_kfifo:
+	kfifo_free(&pcc->fifo);
+
+	return rc;
+}
+
+static void aspeed_pcc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct aspeed_pcc_ctrl *pcc = dev_get_drvdata(dev);
+
+	kfifo_free(&pcc->fifo);
+	misc_deregister(&pcc->mdev);
+}
+
+static const struct of_device_id aspeed_pcc_table[] = {
+	{ .compatible = "aspeed,ast2600-lpc-pcc" },
+	{ },
+};
+
+static struct platform_driver aspeed_pcc_driver = {
+	.driver = {
+		.name = "aspeed-pcc",
+		.of_match_table = aspeed_pcc_table,
+	},
+	.probe = aspeed_pcc_probe,
+	.remove = aspeed_pcc_remove,
+};
+
+module_platform_driver(aspeed_pcc_driver);
+
+MODULE_AUTHOR("Chia-Wei Wang <chiawei_wang@aspeedtech.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Driver for Aspeed Post Code Capture");
-- 
2.34.1

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Krzysztof Kozlowski]

On 17/02/2025 12:48, Kevin Chen wrote:
> +
> +	pcc->mdev.parent = dev;
> +	pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> +	pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
> +					pcc->mdev_id);
> +	pcc->mdev.fops = &pcc_fops;
> +	rc = misc_register(&pcc->mdev);
> +	if (rc) {
> +		dev_err(dev, "Couldn't register misc device\n");
> +		goto err_free_kfifo;
> +	}

You cannot expose user-space interfaces from SoC drivers. Use
appropriate subsystem for this with proper ABI documentation.

See:
https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e909@app.fastmail.com/
and more discussions on LKML

Best regards,
Krzysztof

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Andrew Jeffery]

On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> On 17/02/2025 12:48, Kevin Chen wrote:
> > +
> > +       pcc->mdev.parent = dev;
> > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d",
> > DEVICE_NAME,
> > +                                       pcc->mdev_id);
> > +       pcc->mdev.fops = &pcc_fops;
> > +       rc = misc_register(&pcc->mdev);
> > +       if (rc) {
> > +               dev_err(dev, "Couldn't register misc device\n");
> > +               goto err_free_kfifo;
> > +       }
> 
> You cannot expose user-space interfaces from SoC drivers. Use
> appropriate subsystem for this with proper ABI documentation.
> 
> See:
> https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e909@app.fastmail.com/
> and more discussions on LKML

Further, drivers/misc/aspeed-lpc-snoop.c already exists:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1

Kevin: Did you consider reworking it?

Nuvoton have a similar capability in their NPCM BMC SoC(s) with the
"BPC" ("BIOS POST Code" controller). There should be some consensus on
the binding and userspace interface.

Andrew

RE: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Kevin Chen]

> On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > On 17/02/2025 12:48, Kevin Chen wrote:
> > > +
> > > +       pcc->mdev.parent = dev;
> > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d",
> > > DEVICE_NAME,
> > >
> +                                       pcc->mdev_id);
> > > +       pcc->mdev.fops = &pcc_fops;
> > > +       rc = misc_register(&pcc->mdev);
> > > +       if (rc) {
> > > +               dev_err(dev, "Couldn't register misc device\n");
> > > +               goto err_free_kfifo;
> > > +       }
> >
> > You cannot expose user-space interfaces from SoC drivers. Use
> > appropriate subsystem for this with proper ABI documentation.
> >
> > See:
> > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e909@app.f
> > astmail.com/
> > and more discussions on LKML
> 
> Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=
> 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> 
> Kevin: Did you consider reworking it?
Andrew: No, I do not rework it but add the post code capture driver using the SNOOP registers. As a result, I add some code in aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe abort if snoop is enabled. PCC is used for port I/O byte snooping over eSPI.


> 
> Nuvoton have a similar capability in their NPCM BMC SoC(s) with the "BPC"
> ("BIOS POST Code" controller). There should be some consensus on the binding
> and userspace interface.
> 
> Andrew

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Andrew Jeffery]

On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > +
> > > > +       pcc->mdev.parent = dev;
> > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > "%s%d",
> > > > DEVICE_NAME,
> > > > 
> > +                                       pcc->mdev_id);
> > > > +       pcc->mdev.fops = &pcc_fops;
> > > > +       rc = misc_register(&pcc->mdev);
> > > > +       if (rc) {
> > > > +               dev_err(dev, "Couldn't register misc
> > > > device\n");
> > > > +               goto err_free_kfifo;
> > > > +       }
> > > 
> > > You cannot expose user-space interfaces from SoC drivers. Use
> > > appropriate subsystem for this with proper ABI documentation.
> > > 
> > > See:
> > > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e909@app.f
> > > astmail.com/
> > > and more discussions on LKML
> > 
> > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=
> > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > 
> > Kevin: Did you consider reworking it?
> Andrew: No, I do not rework it but add the post code capture driver
> using the SNOOP registers. As a result, I add some code in
> aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe abort
> if snoop is enabled.

Hmm, I think OpenBMC's history regarding POST code support caused some
confusion on my part. For whatever reason, the snoop device was used as
a source of POST codes despite the existence of the dedicated POST code
hardware since at least the AST2400, but...

> PCC is used for port I/O byte snooping over eSPI.

... it seems that they're largely interchangeable, just with different
hardware features (PCC has DMA)? My impression is that the snoop device
could also be used over eSPI?

> 
> 
> > 
> > Nuvoton have a similar capability in their NPCM BMC SoC(s) with the
> > "BPC"
> > ("BIOS POST Code" controller). There should be some consensus on
> > the binding
> > and userspace interface.

This is still the case.

Andrew

RE: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Kevin Chen]

> On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > > +
> > > > > +       pcc->mdev.parent = dev;
> > > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > "%s%d",
> > > > > DEVICE_NAME,
> > > > >
> > >
> +                                       pcc->mdev_id);
> > > > > +       pcc->mdev.fops = &pcc_fops;
> > > > > +       rc = misc_register(&pcc->mdev);
> > > > > +       if (rc) {
> > > > > +               dev_err(dev, "Couldn't register misc
> > > > > device\n");
> > > > > +               goto err_free_kfifo;
> > > > > +       }
> > > >
> > > > You cannot expose user-space interfaces from SoC drivers. Use
> > > > appropriate subsystem for this with proper ABI documentation.
> > > >
> > > > See:
> > > > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e909@a
> > > > pp.f
> > > > astmail.com/
> > > > and more discussions on LKML
> > >
> > > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/c
> > > ommit/?id=
> > > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > >
> > > Kevin: Did you consider reworking it?
> > Andrew: No, I do not rework it but add the post code capture driver
> > using the SNOOP registers. As a result, I add some code in
> > aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe abort
> > if snoop is enabled.
> 
> Hmm, I think OpenBMC's history regarding POST code support caused some
> confusion on my part. For whatever reason, the snoop device was used as a
> source of POST codes despite the existence of the dedicated POST code
> hardware since at least the AST2400, but...
What I know about the dedicated POST code hardware in ASPEED should be the same one in LPC module.

> 
> > PCC is used for port I/O byte snooping over eSPI.
> 
> ... it seems that they're largely interchangeable, just with different hardware
> features (PCC has DMA)? My impression is that the snoop device could also be
> used over eSPI?
Yes, PCC has DMA to capture the POST code.
And snoop device also can be used over eSPI.

These two devices of PCC and snoop use the same port I/O of 80h and 81h.
But, in current usage of PCC, it can support a continuous, 4-bytes maximum region from port I/O 80h to 83h.
What I know about PCC or snoop usage, depends on INTEL platform or AMD platform.

For ASPEED, we want to upstream the PCC driver for the PCC usage.


> 
> >
> >
> > >
> > > Nuvoton have a similar capability in their NPCM BMC SoC(s) with the
> > > "BPC"
> > > ("BIOS POST Code" controller). There should be some consensus on the
> > > binding and userspace interface.
> 
> This is still the case.
> 
> Andrew

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Andrew Jeffery]

On Wed, 2025-02-19 at 11:59 +0000, Kevin Chen wrote:
> > On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > > > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > > > +
> > > > > > +       pcc->mdev.parent = dev;
> > > > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > "%s%d",
> > > > > > DEVICE_NAME,
> > > > > > 
> > > > 
> > +                                       pcc->mdev_id);
> > > > > > +       pcc->mdev.fops = &pcc_fops;
> > > > > > +       rc = misc_register(&pcc->mdev);
> > > > > > +       if (rc) {
> > > > > > +               dev_err(dev, "Couldn't register misc
> > > > > > device\n");
> > > > > > +               goto err_free_kfifo;
> > > > > > +       }
> > > > > 
> > > > > You cannot expose user-space interfaces from SoC drivers. Use
> > > > > appropriate subsystem for this with proper ABI documentation.
> > > > > 
> > > > > See:
> > > > > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e909@a
> > > > > pp.f
> > > > > astmail.com/
> > > > > and more discussions on LKML
> > > > 
> > > > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/c
> > > > ommit/?id=
> > > > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > > > 
> > > > Kevin: Did you consider reworking it?
> > > Andrew: No, I do not rework it but add the post code capture
> > > driver
> > > using the SNOOP registers. As a result, I add some code in
> > > aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe
> > > abort
> > > if snoop is enabled.
> > 
> > Hmm, I think OpenBMC's history regarding POST code support caused
> > some
> > confusion on my part. For whatever reason, the snoop device was
> > used as a
> > source of POST codes despite the existence of the dedicated POST
> > code
> > hardware since at least the AST2400, but...
> What I know about the dedicated POST code hardware in ASPEED should
> be the same one in LPC module.
> 
> > 
> > > PCC is used for port I/O byte snooping over eSPI.
> > 
> > ... it seems that they're largely interchangeable, just with
> > different hardware
> > features (PCC has DMA)? My impression is that the snoop device
> > could also be
> > used over eSPI?
> Yes, PCC has DMA to capture the POST code.
> And snoop device also can be used over eSPI.
> 
> These two devices of PCC and snoop use the same port I/O of 80h and
> 81h.
> But, in current usage of PCC, it can support a continuous, 4-bytes
> maximum region from port I/O 80h to 83h.
> What I know about PCC or snoop usage, depends on INTEL platform or
> AMD platform.
> 
> For ASPEED, we want to upstream the PCC driver for the PCC usage.

Yeah, that's fine, but I think some work needs to be done to provide
coherence in the devicetree binding and userspace APIs across both the
ASPEED snoop and PCC bits, as well as the Nuvoton BPC. Bespoke designs
create pain.

The PCC driver above reads the data out of the DMA ring-buffer straight
into the kfifo hooked up the the miscdev read callback. The datasheet
notes: "the data structure of the FIFO is mode dependent" in the
description of PCCR3, but no in-band or out-of-band mechanism (sysfs,
ioctl) is provided for userspace to query whether it's 1B, 2B, 4B or
"full" mode.

The situation with the snoop driver is similar (1 or 2 1B channels
multiplexed into the one data stream). It also looks a bit quirky with
multiple channels enabled, as what userspace reads will depend on the
host access patterns, but no metadata is provided to userspace about
what it's reading.

This should be fixed so userspace can have some certainty and a useful
API to work against (one that provides a direct description of what's
being read out).

I expect we could similarly consolidate the devicetree bindings,
covering what IO port range to attach to.

Andrew

RE: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Kevin Chen]

> On Wed, 2025-02-19 at 11:59 +0000, Kevin Chen wrote:
> > > On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > > > > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > > > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > > > > +
> > > > > > > +       pcc->mdev.parent = dev;
> > > > > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > > "%s%d",
> > > > > > > DEVICE_NAME,
> > > > > > >
> > > > >
> > >
> +                                       pcc->mdev_id);
> > > > > > > +       pcc->mdev.fops = &pcc_fops;
> > > > > > > +       rc = misc_register(&pcc->mdev);
> > > > > > > +       if (rc) {
> > > > > > > +               dev_err(dev, "Couldn't register misc
> > > > > > > device\n");
> > > > > > > +               goto err_free_kfifo;
> > > > > > > +       }
> > > > > >
> > > > > > You cannot expose user-space interfaces from SoC drivers. Use
> > > > > > appropriate subsystem for this with proper ABI documentation.
> > > > > >
> > > > > > See:
> > > > > > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e9
> > > > > > 09@a
> > > > > > pp.f
> > > > > > astmail.com/
> > > > > > and more discussions on LKML
> > > > >
> > > > > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.g
> > > > > it/c
> > > > > ommit/?id=
> > > > > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > > > >
> > > > > Kevin: Did you consider reworking it?
> > > > Andrew: No, I do not rework it but add the post code capture
> > > > driver using the SNOOP registers. As a result, I add some code in
> > > > aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe
> > > > abort if snoop is enabled.
> > >
> > > Hmm, I think OpenBMC's history regarding POST code support caused
> > > some confusion on my part. For whatever reason, the snoop device was
> > > used as a source of POST codes despite the existence of the
> > > dedicated POST code hardware since at least the AST2400, but...
> > What I know about the dedicated POST code hardware in ASPEED should be
> > the same one in LPC module.
> >
> > >
> > > > PCC is used for port I/O byte snooping over eSPI.
> > >
> > > ... it seems that they're largely interchangeable, just with
> > > different hardware features (PCC has DMA)? My impression is that the
> > > snoop device could also be used over eSPI?
> > Yes, PCC has DMA to capture the POST code.
> > And snoop device also can be used over eSPI.
> >
> > These two devices of PCC and snoop use the same port I/O of 80h and
> > 81h.
> > But, in current usage of PCC, it can support a continuous, 4-bytes
> > maximum region from port I/O 80h to 83h.
> > What I know about PCC or snoop usage, depends on INTEL platform or AMD
> > platform.
> >
> > For ASPEED, we want to upstream the PCC driver for the PCC usage.
> 
> Yeah, that's fine, but I think some work needs to be done to provide coherence
> in the devicetree binding and userspace APIs across both the ASPEED snoop
> and PCC bits, as well as the Nuvoton BPC. Bespoke designs create pain.
https://lore.kernel.org/linux-kernel//7661de74-f68c-6617-6a4e-3b0eb76a2a2e@linaro.org/T/
Andrew, I find the "NPCM BPC driver" to get the link. Are these patches match what you mentioned?

> 
> The PCC driver above reads the data out of the DMA ring-buffer straight into
> the kfifo hooked up the the miscdev read callback. The datasheet
> notes: "the data structure of the FIFO is mode dependent" in the description of
> PCCR3, but no in-band or out-of-band mechanism (sysfs,
> ioctl) is provided for userspace to query whether it's 1B, 2B, 4B or "full" mode.
For the data structure in PCCR3, I checked with designer. We only need 2B mode to get the information about data and related addresses.
For example, from espi master send the port 80h~83h with first data 0x11223344 and second data 0x55667788. The PCC kfifo would be written in the following output from hexdump.
# hexdump  /dev/aspeed-lpc-pcc0
0000000 4044 4133 4222 4311 4088 4177 4266 4355
> 
> The situation with the snoop driver is similar (1 or 2 1B channels multiplexed
> into the one data stream). It also looks a bit quirky with multiple channels
> enabled, as what userspace reads will depend on the host access patterns, but
> no metadata is provided to userspace about what it's reading.
Yes, for the snoop driver and PCC driver, some mechanism is the same. But snoop only supports 2 bytes data from the 2 1B channels multiplexed.
So, we need to add PCC driver to upstream for customer's 4 Bytes POST code capture usage even the PCC driver needs to check the snoop enabled or not.
Or, could you please give us come comments about how I can upstream the PCC driver.

> 
> This should be fixed so userspace can have some certainty and a useful API to
> work against (one that provides a direct description of what's being read out).
> 
> I expect we could similarly consolidate the devicetree bindings, covering what
> IO port range to attach to.
> 
> Andrew

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Andrew Jeffery]

On Fri, 2025-02-21 at 00:51 +0000, Kevin Chen wrote:
> > On Wed, 2025-02-19 at 11:59 +0000, Kevin Chen wrote:
> > > > On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > > > > > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > > > > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > > > > > +
> > > > > > > > +       pcc->mdev.parent = dev;
> > > > > > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > > > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > > > "%s%d",
> > > > > > > > DEVICE_NAME,
> > > > > > > > 
> > > > > > 
> > > > 
> > +                                       pcc->mdev_id);
> > > > > > > > +       pcc->mdev.fops = &pcc_fops;
> > > > > > > > +       rc = misc_register(&pcc->mdev);
> > > > > > > > +       if (rc) {
> > > > > > > > +               dev_err(dev, "Couldn't register misc
> > > > > > > > device\n");
> > > > > > > > +               goto err_free_kfifo;
> > > > > > > > +       }
> > > > > > > 
> > > > > > > You cannot expose user-space interfaces from SoC drivers. Use
> > > > > > > appropriate subsystem for this with proper ABI documentation.
> > > > > > > 
> > > > > > > See:
> > > > > > > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e9
> > > > > > > 09@a
> > > > > > > pp.f
> > > > > > > astmail.com/
> > > > > > > and more discussions on LKML
> > > > > > 
> > > > > > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.g
> > > > > > it/c
> > > > > > ommit/?id=
> > > > > > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > > > > > 
> > > > > > Kevin: Did you consider reworking it?
> > > > > Andrew: No, I do not rework it but add the post code capture
> > > > > driver using the SNOOP registers. As a result, I add some code in
> > > > > aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe
> > > > > abort if snoop is enabled.
> > > > 
> > > > Hmm, I think OpenBMC's history regarding POST code support caused
> > > > some confusion on my part. For whatever reason, the snoop device was
> > > > used as a source of POST codes despite the existence of the
> > > > dedicated POST code hardware since at least the AST2400, but...
> > > What I know about the dedicated POST code hardware in ASPEED should be
> > > the same one in LPC module.
> > > 
> > > > 
> > > > > PCC is used for port I/O byte snooping over eSPI.
> > > > 
> > > > ... it seems that they're largely interchangeable, just with
> > > > different hardware features (PCC has DMA)? My impression is that the
> > > > snoop device could also be used over eSPI?
> > > Yes, PCC has DMA to capture the POST code.
> > > And snoop device also can be used over eSPI.
> > > 
> > > These two devices of PCC and snoop use the same port I/O of 80h and
> > > 81h.
> > > But, in current usage of PCC, it can support a continuous, 4-bytes
> > > maximum region from port I/O 80h to 83h.
> > > What I know about PCC or snoop usage, depends on INTEL platform or AMD
> > > platform.
> > > 
> > > For ASPEED, we want to upstream the PCC driver for the PCC usage.
> > 
> > Yeah, that's fine, but I think some work needs to be done to provide coherence
> > in the devicetree binding and userspace APIs across both the ASPEED snoop
> > and PCC bits, as well as the Nuvoton BPC. Bespoke designs create pain.
> https://lore.kernel.org/linux-kernel//7661de74-f68c-6617-6a4e-3b0eb76a2a2e@linaro.org/T/
> Andrew, I find the "NPCM BPC driver" to get the link. Are these patches match what you mentioned?

That looks about right.

Note Rob's response there about disparate bindings:

https://lore.kernel.org/linux-kernel//20221130193014.GA2645083-robh@kernel.org/

> 
> > 
> > The PCC driver above reads the data out of the DMA ring-buffer straight into
> > the kfifo hooked up the the miscdev read callback. The datasheet
> > notes: "the data structure of the FIFO is mode dependent" in the description of
> > PCCR3, but no in-band or out-of-band mechanism (sysfs,
> > ioctl) is provided for userspace to query whether it's 1B, 2B, 4B or "full" mode.
> For the data structure in PCCR3, I checked with designer. We only need 2B mode to get the information about data and related addresses.
> For example, from espi master send the port 80h~83h with first data 0x11223344 and second data 0x55667788. The PCC kfifo would be written in the following output from hexdump.
> # hexdump  /dev/aspeed-lpc-pcc0
> 0000000 4044 4133 4222 4311 4088 4177 4266 4355

Right, but that's different to how the Aspeed snoop chardev behaves.
Both the snoop and PCC features can be used to present the same data
(POST codes) to userspace. There should be a unified chardev behaviour
for POST codes so we don't end up with a mess in userspace to deal with
the differences in hardware capabilities.

If we've got a chardev it would feel idiomatic to me to have ioctls to
configure the hardware to the required capture mode for the platform.

> > 
> > The situation with the snoop driver is similar (1 or 2 1B channels multiplexed
> > into the one data stream). It also looks a bit quirky with multiple channels
> > enabled, as what userspace reads will depend on the host access patterns, but
> > no metadata is provided to userspace about what it's reading.
> Yes, for the snoop driver and PCC driver, some mechanism is the same. But snoop only supports 2 bytes data from the 2 1B channels multiplexed.
> So, we need to add PCC driver to upstream for customer's 4 Bytes POST code capture usage even the PCC driver needs to check the snoop enabled or not.

I understand.

> Or, could you please give us come comments about how I can upstream the PCC driver.

Essentially I don't think it's okay that each driver implement a
bespoke chardev, or that the devices don't share a common devicetree
binding. I think what's needed is a small abstraction that provides
common chardev semantics to userspace (e.g. /dev/postcodeN) that can be
connected to the different backends provided by the hardware (Aspeed
snoop and PCC, Nuvoton BPC) and act on the needs of userspace (1, 2, 4-
byte POST codes, configured using ioctls).

Andrew

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Mo Elbadry]

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

Hi Andrew,

I agree that a small layer of abstraction is needed to provide common
chardev semantics to userspace. I think that effort can come where both
Nuvoton and Aspeed unify their design and agree on a common abstraction
layer.

I think such efforts may take some time for both to unify, is it possible
to get this upstreamed (after addressing all other comments) while both
parties work on an agreed unified abstraction layer?

Thank you,
Mo

On Mon, Feb 24, 2025 at 8:27 PM Andrew Jeffery <andrew@codeconstruct.com.au>
wrote:

> On Fri, 2025-02-21 at 00:51 +0000, Kevin Chen wrote:
> > > On Wed, 2025-02-19 at 11:59 +0000, Kevin Chen wrote:
> > > > > On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > > > > > > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > > > > > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > > > > > > +
> > > > > > > > > +       pcc->mdev.parent = dev;
> > > > > > > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > > > > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > > > > "%s%d",
> > > > > > > > > DEVICE_NAME,
> > > > > > > > >
> > > > > > >
> > > > >
> > > +                                       pcc->mdev_id);
> > > > > > > > > +       pcc->mdev.fops = &pcc_fops;
> > > > > > > > > +       rc = misc_register(&pcc->mdev);
> > > > > > > > > +       if (rc) {
> > > > > > > > > +               dev_err(dev, "Couldn't register misc
> > > > > > > > > device\n");
> > > > > > > > > +               goto err_free_kfifo;
> > > > > > > > > +       }
> > > > > > > >
> > > > > > > > You cannot expose user-space interfaces from SoC drivers. Use
> > > > > > > > appropriate subsystem for this with proper ABI documentation.
> > > > > > > >
> > > > > > > > See:
> > > > > > > >
> https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e9
> > > > > > > > 09@a
> > > > > > > > pp.f
> > > > > > > > astmail.com/
> > > > > > > > and more discussions on LKML
> > > > > > >
> > > > > > > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > > > > > >
> > > > > > >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.g
> > > > > > > it/c
> > > > > > > ommit/?id=
> > > > > > > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > > > > > >
> > > > > > > Kevin: Did you consider reworking it?
> > > > > > Andrew: No, I do not rework it but add the post code capture
> > > > > > driver using the SNOOP registers. As a result, I add some code in
> > > > > > aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe
> > > > > > abort if snoop is enabled.
> > > > >
> > > > > Hmm, I think OpenBMC's history regarding POST code support caused
> > > > > some confusion on my part. For whatever reason, the snoop device
> was
> > > > > used as a source of POST codes despite the existence of the
> > > > > dedicated POST code hardware since at least the AST2400, but...
> > > > What I know about the dedicated POST code hardware in ASPEED should
> be
> > > > the same one in LPC module.
> > > >
> > > > >
> > > > > > PCC is used for port I/O byte snooping over eSPI.
> > > > >
> > > > > ... it seems that they're largely interchangeable, just with
> > > > > different hardware features (PCC has DMA)? My impression is that
> the
> > > > > snoop device could also be used over eSPI?
> > > > Yes, PCC has DMA to capture the POST code.
> > > > And snoop device also can be used over eSPI.
> > > >
> > > > These two devices of PCC and snoop use the same port I/O of 80h and
> > > > 81h.
> > > > But, in current usage of PCC, it can support a continuous, 4-bytes
> > > > maximum region from port I/O 80h to 83h.
> > > > What I know about PCC or snoop usage, depends on INTEL platform or
> AMD
> > > > platform.
> > > >
> > > > For ASPEED, we want to upstream the PCC driver for the PCC usage.
> > >
> > > Yeah, that's fine, but I think some work needs to be done to provide
> coherence
> > > in the devicetree binding and userspace APIs across both the ASPEED
> snoop
> > > and PCC bits, as well as the Nuvoton BPC. Bespoke designs create pain.
> >
> https://lore.kernel.org/linux-kernel//7661de74-f68c-6617-6a4e-3b0eb76a2a2e@linaro.org/T/
> > Andrew, I find the "NPCM BPC driver" to get the link. Are these patches
> match what you mentioned?
>
> That looks about right.
>
> Note Rob's response there about disparate bindings:
>
>
> https://lore.kernel.org/linux-kernel//20221130193014.GA2645083-robh@kernel.org/
>
> >
> > >
> > > The PCC driver above reads the data out of the DMA ring-buffer
> straight into
> > > the kfifo hooked up the the miscdev read callback. The datasheet
> > > notes: "the data structure of the FIFO is mode dependent" in the
> description of
> > > PCCR3, but no in-band or out-of-band mechanism (sysfs,
> > > ioctl) is provided for userspace to query whether it's 1B, 2B, 4B or
> "full" mode.
> > For the data structure in PCCR3, I checked with designer. We only need
> 2B mode to get the information about data and related addresses.
> > For example, from espi master send the port 80h~83h with first data
> 0x11223344 and second data 0x55667788. The PCC kfifo would be written in
> the following output from hexdump.
> > # hexdump  /dev/aspeed-lpc-pcc0
> > 0000000 4044 4133 4222 4311 4088 4177 4266 4355
>
> Right, but that's different to how the Aspeed snoop chardev behaves.
> Both the snoop and PCC features can be used to present the same data
> (POST codes) to userspace. There should be a unified chardev behaviour
> for POST codes so we don't end up with a mess in userspace to deal with
> the differences in hardware capabilities.
>
> If we've got a chardev it would feel idiomatic to me to have ioctls to
> configure the hardware to the required capture mode for the platform.
>
> > >
> > > The situation with the snoop driver is similar (1 or 2 1B channels
> multiplexed
> > > into the one data stream). It also looks a bit quirky with multiple
> channels
> > > enabled, as what userspace reads will depend on the host access
> patterns, but
> > > no metadata is provided to userspace about what it's reading.
> > Yes, for the snoop driver and PCC driver, some mechanism is the same.
> But snoop only supports 2 bytes data from the 2 1B channels multiplexed.
> > So, we need to add PCC driver to upstream for customer's 4 Bytes POST
> code capture usage even the PCC driver needs to check the snoop enabled or
> not.
>
> I understand.
>
> > Or, could you please give us come comments about how I can upstream the
> PCC driver.
>
> Essentially I don't think it's okay that each driver implement a
> bespoke chardev, or that the devices don't share a common devicetree
> binding. I think what's needed is a small abstraction that provides
> common chardev semantics to userspace (e.g. /dev/postcodeN) that can be
> connected to the different backends provided by the hardware (Aspeed
> snoop and PCC, Nuvoton BPC) and act on the needs of userspace (1, 2, 4-
> byte POST codes, configured using ioctls).
>
> Andrew
>

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

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Andrew Jeffery]

Hi Mo,

On Mon, 2025-02-24 at 20:34 -0800, Mo Elbadry wrote:
> Hi Andrew,
> 
> I agree that a small layer of abstraction is needed to provide common
> chardev semantics to userspace. I think that effort can come where both
> Nuvoton and Aspeed unify their design and agree on a common abstraction
> layer.
> 
> I think such efforts may take some time for both to unify, is it possible
> to get this upstreamed (after addressing all other comments) while both
> parties work on an agreed unified abstraction layer?
> 

Given Arnd doesn't want bespoke userspace interfaces in the SoC drivers
this will need to go elsewhere, perhaps drivers/char or drivers/misc.
Greg and Arnd maintain both, so the patch needs to make a convincing
argument to them. For my part, my comments are just opinions based on
my understanding of the use-cases and the SoCs involved, and the desire
for reasonable devicetree and userspace interfaces.

I don't think it's right to try to rush things as devicetree and
userspace interfaces can be tricky to change or remove. Rushing tends
to be painful for all involved in the long run.

Cheers,

Andrew

Re: [PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Mo Elbadry]

Hi Andrew,

Understood, your comments do make sense and are the right way to go.

I agree on not rushing, we want to see things moving forward but
properly as well.

Thank you for your help,
Mo



On Tue, Feb 25, 2025 at 3:28 PM Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> Hi Mo,
>
> On Mon, 2025-02-24 at 20:34 -0800, Mo Elbadry wrote:
> > Hi Andrew,
> >
> > I agree that a small layer of abstraction is needed to provide common
> > chardev semantics to userspace. I think that effort can come where both
> > Nuvoton and Aspeed unify their design and agree on a common abstraction
> > layer.
> >
> > I think such efforts may take some time for both to unify, is it possible
> > to get this upstreamed (after addressing all other comments) while both
> > parties work on an agreed unified abstraction layer?
> >
>
> Given Arnd doesn't want bespoke userspace interfaces in the SoC drivers
> this will need to go elsewhere, perhaps drivers/char or drivers/misc.
> Greg and Arnd maintain both, so the patch needs to make a convincing
> argument to them. For my part, my comments are just opinions based on
> my understanding of the use-cases and the SoCs involved, and the desire
> for reasonable devicetree and userspace interfaces.
>
> I don't think it's right to try to rush things as devicetree and
> userspace interfaces can be tricky to change or remove. Rushing tends
> to be painful for all involved in the long run.
>
> Cheers,
>
> Andrew