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

[PATCH v0 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.

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

 .../bindings/soc/aspeed/lpc-pcc.yaml          |  51 ++
 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           | 505 ++++++++++++++++++
 5 files changed, 574 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/aspeed/lpc-pcc.yaml
 create mode 100644 drivers/soc/aspeed/aspeed-lpc-pcc.c

-- 
2.34.1

[PATCH v0 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>
---
 .../bindings/soc/aspeed/lpc-pcc.yaml          | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/aspeed/lpc-pcc.yaml

diff --git a/Documentation/devicetree/bindings/soc/aspeed/lpc-pcc.yaml b/Documentation/devicetree/bindings/soc/aspeed/lpc-pcc.yaml
new file mode 100644
index 000000000000..6c2134497de2
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/aspeed/lpc-pcc.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# # Copyright (c) 2024 Aspeed Technology Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Post Cost Capture Controller
+
+maintainers:
+  - Kevin Chen <kevin_chen@aspeedtech.com>
+  - Chia-Wei Wang <chiawei_wang@aspeedtech.com>
+
+description:
+  The Aspeed PCC control allow to capture POST code from Host.
+
+  This driver is for the BMC side. POST code data can be access by
+  /dev/aspeed-lpc-pcc0. For example, POST code data is below.
+  ~# hexdump /dev/aspeed-lpc-pcc0
+  0000000 40b3 40a0 40a3 40a2 40a2 40b4 40a5 40ee
+  0000010 40ee 40ee 40ee 40ee 40ee 40ee 40ee 40ee
+  0000020 40ee 40ee 40ee 40ee 40ee 40ee 40ee 40a6
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - aspeed,ast2600-lpc-pcc
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    lpc: lpc@1e789000 {
+        compatible = "aspeed,ast2600-lpc-v2", "simple-mfd", "syscon";
+        reg = <0x1e789000 0x1000>;
+
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges = <0x0 0x1e789000 0x1000>;
+
+        lpc_pcc: lpc-pcc@0 {
+          compatible = "aspeed,ast2600-lpc-pcc";
+          reg = <0x0 0x140>;
+        };
+    };
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 17/01/2025 10:52, Kevin Chen wrote:
> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
> 
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> ---

Patchsets start from 1, not 0.

This wasn't tested as it has obvious errors, so no review. Test your
patches before you send them.


<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

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

[Kevin Chen]

> On 17/01/2025 10:52, Kevin Chen wrote:
> > Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
> >
> > Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> > ---
> 
> Patchsets start from 1, not 0.
Agree, I will resend the patchsets of v1.

> 
> This wasn't tested as it has obvious errors, so no review. Test your patches
> before you send them.
Agree, I will run "make dtbs_check W=1" and " make dt_binding_check " to check result is clean before the next commits.

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

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

[Krzysztof Kozlowski]

On 13/02/2025 09:46, Kevin Chen wrote:
>> On 17/01/2025 10:52, Kevin Chen wrote:
>>> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
>>>
>>> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
>>> ---
>>
>> Patchsets start from 1, not 0.
> Agree, I will resend the patchsets of v1.
> 
>>
>> This wasn't tested as it has obvious errors, so no review. Test your patches
>> before you send them.
> Agree, I will run "make dtbs_check W=1" and " make dt_binding_check " to check result is clean before the next commits.
> 
>>
>>
>> <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>

How did you implement this feedback?

Best regards,
Krzysztof

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

[Kevin Chen]

> On 13/02/2025 09:46, Kevin Chen wrote:
> >> On 17/01/2025 10:52, Kevin Chen wrote:
> >>> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
> >>>
> >>> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> >>> ---
> >>
> >> Patchsets start from 1, not 0.
> > Agree, I will resend the patchsets of v1.
> >
> >>
> >> This wasn't tested as it has obvious errors, so no review. Test your
> >> patches before you send them.
> > Agree, I will run "make dtbs_check W=1" and " make dt_binding_check " to
> check result is clean before the next commits.
> >
> >>
> >>
> >> <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>
> 
> How did you implement this feedback?

make dtbs_check W=1
# Check the lpc-pcc or pcc message from the log

make dt_binding_check 
# No Error

make coccicheck MODE=report
# Stop at the following log but no related log.
#   OCaml scripting is unsupported.
#   coccicheck failed


> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 18/02/2025 12:05, Kevin Chen wrote:
>> On 13/02/2025 09:46, Kevin Chen wrote:
>>>> On 17/01/2025 10:52, Kevin Chen wrote:
>>>>> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
>>>>>
>>>>> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
>>>>> ---
>>>>
>>>> Patchsets start from 1, not 0.
>>> Agree, I will resend the patchsets of v1.
>>>
>>>>
>>>> This wasn't tested as it has obvious errors, so no review. Test your
>>>> patches before you send them.
>>> Agree, I will run "make dtbs_check W=1" and " make dt_binding_check " to
>> check result is clean before the next commits.
>>>
>>>>
>>>>
>>>> <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>
>>
>> How did you implement this feedback?
> 
> make dtbs_check W=1
> # Check the lpc-pcc or pcc message from the log
> 
> make dt_binding_check 
> # No Error
> 
> make coccicheck MODE=report
> # Stop at the following log but no related log.
> #   OCaml scripting is unsupported.
> #   coccicheck failed

You answered probably to earlier part of email, not to the one I asked
about.

Best regards,
Krzysztof

[PATCH v0 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 v0 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 | 505 ++++++++++++++++++++++++++++
 3 files changed, 516 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 snoop data bytes 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..156707642224
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-lpc-pcc.c
@@ -0,0 +1,505 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ */
+#include <linux/bitops.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 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 {
+	struct device *dev;
+	struct regmap *regmap;
+	int irq;
+	uint32_t rec_mode;
+	uint32_t port;
+	uint32_t port_xbits;
+	uint32_t port_hbits_select;
+	uint32_t dma_mode;
+	struct aspeed_pcc_dma dma;
+	struct kfifo fifo;
+	wait_queue_head_t wq;
+	struct miscdevice mdev;
+	int mdev_id;
+	bool a2600_15;
+};
+
+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 *pcc = container_of(file->private_data,
+					      struct aspeed_pcc,
+					      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 *pcc = container_of(file->private_data, struct aspeed_pcc, 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 *pcc = (struct aspeed_pcc *)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, reg;
+	struct aspeed_pcc *pcc = (struct aspeed_pcc *)arg;
+	struct kfifo *fifo = &pcc->fifo;
+
+	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;
+
+	if (pcc->dma_mode)
+		return aspeed_pcc_dma_isr(irq, arg);
+
+	while (sts & PCCR2_INT_STATUS_DATA_RDY) {
+		regmap_read(pcc->regmap, PCCR3, &reg);
+
+		if (kfifo_is_full(fifo))
+			kfifo_skip(fifo);
+
+		kfifo_put(fifo, reg & PCCR3_FIFO_DATA_MASK);
+
+		regmap_read(pcc->regmap, PCCR2, &sts);
+	}
+
+	wake_up_interruptible(&pcc->wq);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * 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 *pcc, struct device *dev)
+{
+	struct device_node *np;
+	u32 hicrb_en;
+
+	/* abort if snoop is enabled */
+	np = of_find_compatible_node(dev->parent->of_node, NULL, "aspeed,ast2600-lpc-snoop");
+	if (np) {
+		if (of_device_is_available(np)) {
+			dev_err(dev, "A2600-15 should be applied with snoop disabled\n");
+			return -EPERM;
+		}
+	}
+
+	/* abort if port is not 4-bytes continuous range */
+	if (pcc->port_xbits != 0x3) {
+		dev_err(dev, "A2600-15 should be applied on 4-bytes continuous I/O address range\n");
+		return -EINVAL;
+	}
+
+	/* 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 *pcc, struct device *dev)
+{
+	int rc;
+
+	if (pcc->a2600_15) {
+		rc = aspeed_a2600_15(pcc, dev);
+		if (rc)
+			return rc;
+	}
+
+	/* record mode */
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_MODE_SEL_MASK,
+			   pcc->rec_mode << PCCR0_MODE_SEL_SHIFT);
+
+	/* port address */
+	regmap_update_bits(pcc->regmap, PCCR1,
+			   PCCR1_BASE_ADDR_MASK,
+			   pcc->port << PCCR1_BASE_ADDR_SHIFT);
+
+	/* port address high bits selection or parser control */
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_ADDR_SEL_MASK,
+			   pcc->port_hbits_select << PCCR0_ADDR_SEL_SHIFT);
+
+	/* port address dont care bits */
+	regmap_update_bits(pcc->regmap, PCCR1,
+			   PCCR1_DONT_CARE_BITS_MASK,
+			   pcc->port_xbits << PCCR1_DONT_CARE_BITS_SHIFT);
+
+	/* set DMA ring buffer size and enable interrupts */
+	if (pcc->dma_mode) {
+		regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
+		regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_ADDRH_MASK,
+				   (pcc->dma.addr >> 32) << PCCR5_DMA_ADDRH_SHIFT);
+		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);
+	} else {
+		regmap_update_bits(pcc->regmap, PCCR0, PCCR0_RX_TRIG_LVL_MASK,
+				   PCC_FIFO_THR_4_EIGHTH << PCCR0_RX_TRIG_LVL_SHIFT);
+		regmap_update_bits(pcc->regmap, PCCR0,
+				   PCCR0_EN_RX_TMOUT_INT | PCCR0_EN_RX_AVAIL_INT,
+				   PCCR0_EN_RX_TMOUT_INT | PCCR0_EN_RX_AVAIL_INT);
+	}
+
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
+
+	return 0;
+}
+
+static int aspeed_pcc_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct aspeed_pcc *pcc;
+	struct device *dev = &pdev->dev;
+	uint32_t fifo_size = PAGE_SIZE;
+
+	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
+	if (!pcc)
+		return -ENOMEM;
+
+	pcc->dev = dev;
+	rc = of_property_read_u32(dev->of_node, "port-addr", &pcc->port);
+	if (rc) {
+		dev_err(dev, "cannot get port address\n");
+		return -ENODEV;
+	}
+
+	/* optional, by default: 0 -> 1-Byte mode */
+	of_property_read_u32(dev->of_node, "rec-mode", &pcc->rec_mode);
+	if (!is_valid_rec_mode(pcc->rec_mode)) {
+		dev_err(dev, "invalid record mode: %u\n",
+			pcc->rec_mode);
+		return -EINVAL;
+	}
+
+	/* optional, by default: 0 -> no don't care bits */
+	of_property_read_u32(dev->of_node, "port-addr-xbits", &pcc->port_xbits);
+
+	/*
+	 * optional, by default: 0 -> no high address bits
+	 *
+	 * Note that when record mode is set to 1-Byte, this
+	 * property is ignored and the corresponding HW bits
+	 * behave as read/write cycle parser control with the
+	 * value set to 0b11
+	 */
+	if (pcc->rec_mode) {
+		of_property_read_u32(dev->of_node, "port-addr-hbits-select",
+				     &pcc->port_hbits_select);
+		if (!is_valid_high_bits_select(pcc->port_hbits_select)) {
+			dev_err(dev, "invalid high address bits selection: %u\n",
+				pcc->port_hbits_select);
+			return -EINVAL;
+		}
+	} else {
+		pcc->port_hbits_select = 0x3;
+	}
+
+	/* AP note A2600-15 */
+	pcc->a2600_15 = of_property_read_bool(dev->of_node, "A2600-15");
+	if (pcc->a2600_15)
+		dev_info(dev, "A2600-15 AP note patch is selected\n");
+
+	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_mode = of_property_read_bool(dev->of_node, "dma-mode");
+	if (pcc->dma_mode) {
+		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;
+	}
+
+	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(pcc->regmap)) {
+		dev_err(dev, "cannot map register\n");
+		return -ENODEV;
+	}
+
+	/* 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);
+
+	pcc->irq = platform_get_irq(pdev, 0);
+	if (pcc->irq < 0) {
+		dev_err(dev, "cannot 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, "cannot request IRQ handler\n");
+		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, "cannot 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, "cannot register misc device\n");
+		goto err_free_kfifo;
+	}
+
+	rc = aspeed_pcc_enable(pcc, dev);
+	if (rc) {
+		dev_err(dev, "cannot enable PCC\n");
+		goto err_dereg_mdev;
+	}
+
+	dev_set_drvdata(&pdev->dev, pcc);
+
+	dev_info(dev, "module loaded\n");
+
+	return 0;
+
+err_dereg_mdev:
+	misc_deregister(&pcc->mdev);
+
+err_free_kfifo:
+	kfifo_free(&pcc->fifo);
+
+	return rc;
+}
+
+static int aspeed_pcc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct aspeed_pcc *pcc = dev_get_drvdata(dev);
+
+	kfifo_free(&pcc->fifo);
+	misc_deregister(&pcc->mdev);
+
+	return 0;
+}
+
+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 v0 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Krzysztof Kozlowski]

On 17/01/2025 10:52, Kevin Chen wrote:
> Add LPC PCC controller driver to support POST code capture.
> 
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> ---


This patch was never tested with your DTS and bindings. Please do not
send untested code.

> +
> +/*
> + * 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 *pcc, struct device *dev)
> +{
> +	struct device_node *np;
> +	u32 hicrb_en;
> +
> +	/* abort if snoop is enabled */
> +	np = of_find_compatible_node(dev->parent->of_node, NULL, "aspeed,ast2600-lpc-snoop");

No, don't sprinkle compatible to epxress relationship. Use proper
phandles or parent-child relationship.

> +	if (np) {
> +		if (of_device_is_available(np)) {
> +			dev_err(dev, "A2600-15 should be applied with snoop disabled\n");
> +			return -EPERM;
> +		}
> +	}
> +
> +	/* abort if port is not 4-bytes continuous range */
> +	if (pcc->port_xbits != 0x3) {
> +		dev_err(dev, "A2600-15 should be applied on 4-bytes continuous I/O address range\n");
> +		return -EINVAL;
> +	}
> +
> +	/* 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 *pcc, struct device *dev)
> +{
> +	int rc;
> +
> +	if (pcc->a2600_15) {
> +		rc = aspeed_a2600_15(pcc, dev);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* record mode */
> +	regmap_update_bits(pcc->regmap, PCCR0,
> +			   PCCR0_MODE_SEL_MASK,
> +			   pcc->rec_mode << PCCR0_MODE_SEL_SHIFT);
> +
> +	/* port address */
> +	regmap_update_bits(pcc->regmap, PCCR1,
> +			   PCCR1_BASE_ADDR_MASK,
> +			   pcc->port << PCCR1_BASE_ADDR_SHIFT);
> +
> +	/* port address high bits selection or parser control */
> +	regmap_update_bits(pcc->regmap, PCCR0,
> +			   PCCR0_ADDR_SEL_MASK,
> +			   pcc->port_hbits_select << PCCR0_ADDR_SEL_SHIFT);
> +
> +	/* port address dont care bits */
> +	regmap_update_bits(pcc->regmap, PCCR1,
> +			   PCCR1_DONT_CARE_BITS_MASK,
> +			   pcc->port_xbits << PCCR1_DONT_CARE_BITS_SHIFT);
> +
> +	/* set DMA ring buffer size and enable interrupts */
> +	if (pcc->dma_mode) {
> +		regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
> +		regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_ADDRH_MASK,
> +				   (pcc->dma.addr >> 32) << PCCR5_DMA_ADDRH_SHIFT);
> +		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);
> +	} else {
> +		regmap_update_bits(pcc->regmap, PCCR0, PCCR0_RX_TRIG_LVL_MASK,
> +				   PCC_FIFO_THR_4_EIGHTH << PCCR0_RX_TRIG_LVL_SHIFT);
> +		regmap_update_bits(pcc->regmap, PCCR0,
> +				   PCCR0_EN_RX_TMOUT_INT | PCCR0_EN_RX_AVAIL_INT,
> +				   PCCR0_EN_RX_TMOUT_INT | PCCR0_EN_RX_AVAIL_INT);
> +	}
> +
> +	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
> +
> +	return 0;
> +}
> +
> +static int aspeed_pcc_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	struct aspeed_pcc *pcc;
> +	struct device *dev = &pdev->dev;
> +	uint32_t fifo_size = PAGE_SIZE;
> +
> +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);

Why not dev?

> +	if (!pcc)
> +		return -ENOMEM;
> +
> +	pcc->dev = dev;
> +	rc = of_property_read_u32(dev->of_node, "port-addr", &pcc->port);

Nope

> +	if (rc) {
> +		dev_err(dev, "cannot get port address\n");
> +		return -ENODEV;
> +	}
> +
> +	/* optional, by default: 0 -> 1-Byte mode */
> +	of_property_read_u32(dev->of_node, "rec-mode", &pcc->rec_mode);

Nope

> +	if (!is_valid_rec_mode(pcc->rec_mode)) {
> +		dev_err(dev, "invalid record mode: %u\n",
> +			pcc->rec_mode);
> +		return -EINVAL;
> +	}
> +
> +	/* optional, by default: 0 -> no don't care bits */
> +	of_property_read_u32(dev->of_node, "port-addr-xbits", &pcc->port_xbits);

Nope

> +
> +	/*
> +	 * optional, by default: 0 -> no high address bits
> +	 *
> +	 * Note that when record mode is set to 1-Byte, this
> +	 * property is ignored and the corresponding HW bits
> +	 * behave as read/write cycle parser control with the
> +	 * value set to 0b11
> +	 */
> +	if (pcc->rec_mode) {
> +		of_property_read_u32(dev->of_node, "port-addr-hbits-select",

Nope, none of these properties exist.

> +				     &pcc->port_hbits_select);
> +		if (!is_valid_high_bits_select(pcc->port_hbits_select)) {
> +			dev_err(dev, "invalid high address bits selection: %u\n",
> +				pcc->port_hbits_select);
> +			return -EINVAL;
> +		}
> +	} else {
> +		pcc->port_hbits_select = 0x3;
> +	}
> +
> +	/* AP note A2600-15 */
> +	pcc->a2600_15 = of_property_read_bool(dev->of_node, "A2600-15");

NAK even more.

> +	if (pcc->a2600_15)
> +		dev_info(dev, "A2600-15 AP note patch is selected\n");
> +> +	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_mode = of_property_read_bool(dev->of_node, "dma-mode");

NAK, drop

> +	if (pcc->dma_mode) {
> +		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");

Run coccinelle on your code first.

> +		return -ENOMEM;
> +	}
> +
> +	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(pcc->regmap)) {
> +		dev_err(dev, "cannot map register\n");
> +		return -ENODEV;
> +	}
> +
> +	/* 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);
> +
> +	pcc->irq = platform_get_irq(pdev, 0);
> +	if (pcc->irq < 0) {
> +		dev_err(dev, "cannot 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, "cannot request IRQ handler\n");
> +		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, "cannot 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, "cannot register misc device\n");
> +		goto err_free_kfifo;
> +	}
> +
> +	rc = aspeed_pcc_enable(pcc, dev);
> +	if (rc) {
> +		dev_err(dev, "cannot enable PCC\n");
> +		goto err_dereg_mdev;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, pcc);
> +
> +	dev_info(dev, "module loaded\n");

Drop such function success messages, everywhere.

Best regards,
Krzysztof

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

[Kevin Chen]

> On 17/01/2025 10:52, Kevin Chen wrote:
> > Add LPC PCC controller driver to support POST code capture.
> >
> > Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> > ---
> 
> 
> This patch was never tested with your DTS and bindings. Please do not send
> untested code.
For the dts, I asked Google to upstream their dts using lpc_pcc node.

> 
> > +
> > +/*
> > + * 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 *pcc, struct device
> > +*dev) {
> > +	struct device_node *np;
> > +	u32 hicrb_en;
> > +
> > +	/* abort if snoop is enabled */
> > +	np = of_find_compatible_node(dev->parent->of_node, NULL,
> > +"aspeed,ast2600-lpc-snoop");
> 
> No, don't sprinkle compatible to epxress relationship. Use proper phandles or
> parent-child relationship.
Agree, I will remove it.

> 
> > +	if (np) {
> > +		if (of_device_is_available(np)) {
> > +			dev_err(dev, "A2600-15 should be applied with snoop
> disabled\n");
> > +			return -EPERM;
> > +		}
> > +	}
> > +
> > +	/* abort if port is not 4-bytes continuous range */
> > +	if (pcc->port_xbits != 0x3) {
> > +		dev_err(dev, "A2600-15 should be applied on 4-bytes continuous I/O
> address range\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* 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 *pcc, struct device
> > +*dev) {
> > +	int rc;
> > +
> > +	if (pcc->a2600_15) {
> > +		rc = aspeed_a2600_15(pcc, dev);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> > +	/* record mode */
> > +	regmap_update_bits(pcc->regmap, PCCR0,
> > +			   PCCR0_MODE_SEL_MASK,
> > +			   pcc->rec_mode << PCCR0_MODE_SEL_SHIFT);
> > +
> > +	/* port address */
> > +	regmap_update_bits(pcc->regmap, PCCR1,
> > +			   PCCR1_BASE_ADDR_MASK,
> > +			   pcc->port << PCCR1_BASE_ADDR_SHIFT);
> > +
> > +	/* port address high bits selection or parser control */
> > +	regmap_update_bits(pcc->regmap, PCCR0,
> > +			   PCCR0_ADDR_SEL_MASK,
> > +			   pcc->port_hbits_select << PCCR0_ADDR_SEL_SHIFT);
> > +
> > +	/* port address dont care bits */
> > +	regmap_update_bits(pcc->regmap, PCCR1,
> > +			   PCCR1_DONT_CARE_BITS_MASK,
> > +			   pcc->port_xbits << PCCR1_DONT_CARE_BITS_SHIFT);
> > +
> > +	/* set DMA ring buffer size and enable interrupts */
> > +	if (pcc->dma_mode) {
> > +		regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
> > +		regmap_update_bits(pcc->regmap, PCCR5,
> PCCR5_DMA_ADDRH_MASK,
> > +				   (pcc->dma.addr >> 32) <<
> PCCR5_DMA_ADDRH_SHIFT);
> > +		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);
> > +	} else {
> > +		regmap_update_bits(pcc->regmap, PCCR0,
> PCCR0_RX_TRIG_LVL_MASK,
> > +				   PCC_FIFO_THR_4_EIGHTH <<
> PCCR0_RX_TRIG_LVL_SHIFT);
> > +		regmap_update_bits(pcc->regmap, PCCR0,
> > +				   PCCR0_EN_RX_TMOUT_INT |
> PCCR0_EN_RX_AVAIL_INT,
> > +				   PCCR0_EN_RX_TMOUT_INT |
> PCCR0_EN_RX_AVAIL_INT);
> > +	}
> > +
> > +	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
> > +
> > +	return 0;
> > +}
> > +
> > +static int aspeed_pcc_probe(struct platform_device *pdev) {
> > +	int rc;
> > +	struct aspeed_pcc *pcc;
> > +	struct device *dev = &pdev->dev;
> > +	uint32_t fifo_size = PAGE_SIZE;
> > +
> > +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
> 
> Why not dev?
Agree, I will change pcc to dev.

> 
> > +	if (!pcc)
> > +		return -ENOMEM;
> > +
> > +	pcc->dev = dev;
> > +	rc = of_property_read_u32(dev->of_node, "port-addr", &pcc->port);
> 
> Nope
For lpc_snoop node, there is snoop-ports in lpc_snoop to specify which IO port address for use.
In lpc_pcc, the usage is the same. Could I add the pcc-ports property?

> 
> > +	if (rc) {
> > +		dev_err(dev, "cannot get port address\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* optional, by default: 0 -> 1-Byte mode */
> > +	of_property_read_u32(dev->of_node, "rec-mode", &pcc->rec_mode);
> 
> Nope
OK, I will remove the property.

> 
> > +	if (!is_valid_rec_mode(pcc->rec_mode)) {
> > +		dev_err(dev, "invalid record mode: %u\n",
> > +			pcc->rec_mode);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* optional, by default: 0 -> no don't care bits */
> > +	of_property_read_u32(dev->of_node, "port-addr-xbits",
> > +&pcc->port_xbits);
> 
> Nope
OK, I will remove the property.

> 
> > +
> > +	/*
> > +	 * optional, by default: 0 -> no high address bits
> > +	 *
> > +	 * Note that when record mode is set to 1-Byte, this
> > +	 * property is ignored and the corresponding HW bits
> > +	 * behave as read/write cycle parser control with the
> > +	 * value set to 0b11
> > +	 */
> > +	if (pcc->rec_mode) {
> > +		of_property_read_u32(dev->of_node, "port-addr-hbits-select",
> 
> Nope, none of these properties exist.
OK, I will remove the property.

> 
> > +				     &pcc->port_hbits_select);
> > +		if (!is_valid_high_bits_select(pcc->port_hbits_select)) {
> > +			dev_err(dev, "invalid high address bits selection: %u\n",
> > +				pcc->port_hbits_select);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		pcc->port_hbits_select = 0x3;
> > +	}
> > +
> > +	/* AP note A2600-15 */
> > +	pcc->a2600_15 = of_property_read_bool(dev->of_node, "A2600-15");
> 
> NAK even more.
OK, I will remove the property.

> 
> > +	if (pcc->a2600_15)
> > +		dev_info(dev, "A2600-15 AP note patch is selected\n");
> > +> +	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_mode = of_property_read_bool(dev->of_node, "dma-mode");
> 
> NAK, drop
OK, I will remove the property.

> 
> > +	if (pcc->dma_mode) {
> > +		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");
> 
> Run coccinelle on your code first.
OK, I will try to run conccinelle by the following link.
https://docs.kernel.org/dev-tools/coccinelle.html

> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > +	if (IS_ERR(pcc->regmap)) {
> > +		dev_err(dev, "cannot map register\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* 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);
> > +
> > +	pcc->irq = platform_get_irq(pdev, 0);
> > +	if (pcc->irq < 0) {
> > +		dev_err(dev, "cannot 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, "cannot request IRQ handler\n");
> > +		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, "cannot 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, "cannot register misc device\n");
> > +		goto err_free_kfifo;
> > +	}
> > +
> > +	rc = aspeed_pcc_enable(pcc, dev);
> > +	if (rc) {
> > +		dev_err(dev, "cannot enable PCC\n");
> > +		goto err_dereg_mdev;
> > +	}
> > +
> > +	dev_set_drvdata(&pdev->dev, pcc);
> > +
> > +	dev_info(dev, "module loaded\n");
> 
> Drop such function success messages, everywhere.
OK, I will remove the messages.

> 
> Best regards,
> Krzysztof

Re: [PATCH v0 0/3] Add AST2600 LPC PCC support

[Rob Herring (Arm)]

On Fri, 17 Jan 2025 17:52:14 +0800, Kevin Chen wrote:
> The AST2600 has PCC controller in LPC, placed in LPC node. As a result,
> add LPC PCC controller driver to support POST code capture.
> 
> 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
> 
>  .../bindings/soc/aspeed/lpc-pcc.yaml          |  51 ++
>  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           | 505 ++++++++++++++++++
>  5 files changed, 574 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/aspeed/lpc-pcc.yaml
>  create mode 100644 drivers/soc/aspeed/aspeed-lpc-pcc.c
> 
> --
> 2.34.1
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y for arch/arm/boot/dts/aspeed/' for 20250117095217.661070-1-kevin_chen@aspeedtech.com:

arch/arm/boot/dts/aspeed/aspeed-bmc-quanta-s6q.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-sbp1.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-fuji.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge-4u.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-ast2600-evb-a1.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-transformers.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-starscream.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-4u.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-catalina.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-asus-x4tf.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-1s4u.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-qcom-dc-scm-v1.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ufispace-ncplite.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-minerva.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-greatlakes.dtb: lpc-pcc@0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/soc/aspeed/lpc-pcc.yaml#