Add AST2600 LPC PCC support

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.

--
v2:
  -- Change driver path to drivers/misc

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/misc/Kconfig                          |  10 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/aspeed-lpc-pcc.c                 | 441 ++++++++++++++++++
 5 files changed, 495 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-pcc.c

-- 
2.34.1

[PATCH v2 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 v2 1/3] dt-binding: aspeed: Add LPC PCC controller

[Krzysztof Kozlowski]

On 04/03/2025 11:44, Kevin Chen wrote:
> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Missing 's'.

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

Missing vendor prefix

> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description: The LPC I/O ports to pcc

Description is too vague. Why would we encode I/O ports as some numbers
instead of GPIOs for example? If these are ports, why this is not a graph?

Missing constraints - min/maxItems, defaults, minimum/maximum etc.

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

So what 0x80 stands for?


Best regards,
Krzysztof

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

[Kevin Chen]

> On 04/03/2025 11:44, Kevin Chen wrote:
> > Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your
> patch is touching. For bindings, the preferred subjects are explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patch
> es.html#i-for-patch-submitters
> 
> Missing 's'.
> 
> >
> > 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:
> 
> Missing vendor prefix
OK, so I need to add the vendor prefix such as "aspeed,pcc-ports". Right?
But I want to check the snoop-ports usage. In the HW controller, the usage should be the same.
Not only aspeed but nuvoton, there exist a HW module for the snoop-ports or pcc-ports usage.

> 
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > +        description: The LPC I/O ports to pcc
> 
> Description is too vague. Why would we encode I/O ports as some numbers
> instead of GPIOs for example? If these are ports, why this is not a graph?
For the port-mmaped I/O in x80 architecture, BMC need to handle specific port I/O in the relative HW module.
So, I need to add the pcc-ports property as the snoop-ports property in Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml

> 
> Missing constraints - min/maxItems, defaults, minimum/maximum etc.
The port-mmaped I/O is defined from host, BMC as the device would capture the port I/O from the pcc-ports property defined in dts.

> 
> > +
> > +    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>;
> 
> So what 0x80 stands for?
Host as x86 architecture would access the 0x80 port, which is mapped to the BMC PCC HW module.
As a result, x86 can keep the port-mmaped I/O usage and access the BMC device, which is needed to know which port using in the PCC module in BMC.

> 
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 10/03/2025 02:50, Kevin Chen wrote:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +        description: The LPC I/O ports to pcc
>>
>> Description is too vague. Why would we encode I/O ports as some numbers
>> instead of GPIOs for example? If these are ports, why this is not a graph?
> For the port-mmaped I/O in x80 architecture, BMC need to handle specific port I/O in the relative HW module.
> So, I need to add the pcc-ports property as the snoop-ports property in Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
> 
>>
>> Missing constraints - min/maxItems, defaults, minimum/maximum etc.
> The port-mmaped I/O is defined from host, BMC as the device would capture the port I/O from the pcc-ports property defined in dts.

Put this information in the description, instead of copying property name.

> 
>>
>>> +
>>> +    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>;
>>
>> So what 0x80 stands for?
> Host as x86 architecture would access the 0x80 port, which is mapped to the BMC PCC HW module.
> As a result, x86 can keep the port-mmaped I/O usage and access the BMC device, which is needed to know which port using in the PCC module in BMC.

And on different boards this is not 0x80?

Best regards,
Krzysztof

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

[Kevin Chen]

> On 10/03/2025 02:50, Kevin Chen wrote:
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> +        description: The LPC I/O ports to pcc
> >>
> >> Description is too vague. Why would we encode I/O ports as some
> >> numbers instead of GPIOs for example? If these are ports, why this is not a
> graph?
> > For the port-mmaped I/O in x86 architecture, BMC need to handle specific
> port I/O in the relative HW module.
> > So, I need to add the pcc-ports property as the snoop-ports property
> > in Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
> >
> >>
> >> Missing constraints - min/maxItems, defaults, minimum/maximum etc.
> > The port-mmaped I/O is defined from host, BMC as the device would capture
> the port I/O from the pcc-ports property defined in dts.
> 
> Put this information in the description, instead of copying property name.
OK. I will put this information in the description.

> 
> >
> >>
> >>> +
> >>> +    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>;
> >>
> >> So what 0x80 stands for?
> > Host as x86 architecture would access the 0x80 port, which is mapped to the
> BMC PCC HW module.
> > As a result, x86 can keep the port-mmaped I/O usage and access the BMC
> device, which is needed to know which port using in the PCC module in BMC.
> 
> And on different boards this is not 0x80?
The port-mmaped I/O defined in the intel legacy document as the example usage.
For example, the common usage agreement of port-mmaped I/O are the following. But this setting can be modified due to the host usage. We provide the flexibility to modify the I/O port settings.
KCS : 0xCA2(CMD)/CA3(Data)
BT : 0xE4/E5/E6
SNOOP/PCC : 0x80/81/82/83
Mailbox : 0xCC0
SuperIO : 0x2E/2F or 0x4E/0x4F
System UART : 0x3F8/2F8/3E8/2E8

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 10/03/2025 10:49, Kevin Chen wrote:
>>>>> +            compatible = "aspeed,ast2600-lpc-pcc";
>>>>> +            reg = <0x0 0x140>;
>>>>> +            interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +            pcc-ports = <0x80>;
>>>>
>>>> So what 0x80 stands for?
>>> Host as x86 architecture would access the 0x80 port, which is mapped to the
>> BMC PCC HW module.
>>> As a result, x86 can keep the port-mmaped I/O usage and access the BMC
>> device, which is needed to know which port using in the PCC module in BMC.
>>
>> And on different boards this is not 0x80?
> The port-mmaped I/O defined in the intel legacy document as the example usage.
> For example, the common usage agreement of port-mmaped I/O are the following. But this setting can be modified due to the host usage. We provide the flexibility to modify the I/O port settings.
> KCS : 0xCA2(CMD)/CA3(Data)
> BT : 0xE4/E5/E6
> SNOOP/PCC : 0x80/81/82/83
> Mailbox : 0xCC0
> SuperIO : 0x2E/2F or 0x4E/0x4F
> System UART : 0x3F8/2F8/3E8/2E8
So which boards have it modified?

Best regards,
Krzysztof

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

[Kevin Chen]

> On 10/03/2025 10:49, Kevin Chen wrote:
> >>>>> +            compatible = "aspeed,ast2600-lpc-pcc";
> >>>>> +            reg = <0x0 0x140>;
> >>>>> +            interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
> >>>>> +            pcc-ports = <0x80>;
> >>>>
> >>>> So what 0x80 stands for?
> >>> Host as x86 architecture would access the 0x80 port, which is mapped to
> the
> >> BMC PCC HW module.
> >>> As a result, x86 can keep the port-mmaped I/O usage and access the BMC
> >> device, which is needed to know which port using in the PCC module in
> BMC.
> >>
> >> And on different boards this is not 0x80?
> > The port-mmaped I/O defined in the intel legacy document as the example
> usage.
> > For example, the common usage agreement of port-mmaped I/O are the
> following. But this setting can be modified due to the host usage. We provide
> the flexibility to modify the I/O port settings.
> > KCS : 0xCA2(CMD)/CA3(Data)
> > BT : 0xE4/E5/E6
> > SNOOP/PCC : 0x80/81/82/83
> > Mailbox : 0xCC0
> > SuperIO : 0x2E/2F or 0x4E/0x4F
> > System UART : 0x3F8/2F8/3E8/2E8
> So which boards have it modified?
This setting should be as same as the snoop-ports. You can check the dts in arch/arm/boot/dts/aspeed folder.
aspeed-bmc-amd-daytonax.dts
212:    snoop-ports = <0x80>, <0x81>;

aspeed-bmc-facebook-tiogapass.dts
77:     snoop-ports = <0x80>;


> 
> Best regards,
> Krzysztof

[PATCH v2 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

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

[Krzysztof Kozlowski]

On 04/03/2025 11:44, Kevin Chen wrote:
> 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";

Incomplete. Your driver clearly bails on missing ports...

Best regards,
Krzysztof

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

[Kevin Chen]

> On 04/03/2025 11:44, Kevin Chen wrote:
> > 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";
> 
> Incomplete. Your driver clearly bails on missing ports...
Agree.

> 
> Best regards,
> Krzysztof

[PATCH v2 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/misc/Kconfig          |  10 +
 drivers/misc/Makefile         |   1 +
 drivers/misc/aspeed-lpc-pcc.c | 441 ++++++++++++++++++++++++++++++++++
 3 files changed, 452 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-pcc.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 56bc72c7ce4a..d8e2d6c1fe40 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -50,6 +50,16 @@ config AD525X_DPOT_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad525x_dpot-spi.
 
+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.
+
 config DUMMY_IRQ
 	tristate "Dummy IRQ handler"
 	help
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 545aad06d088..4762da7804bf 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_IBMVMC)		+= ibmvmc.o
 obj-$(CONFIG_AD525X_DPOT)	+= ad525x_dpot.o
 obj-$(CONFIG_AD525X_DPOT_I2C)	+= ad525x_dpot-i2c.o
 obj-$(CONFIG_AD525X_DPOT_SPI)	+= ad525x_dpot-spi.o
+obj-$(CONFIG_ASPEED_LPC_PCC)	+= aspeed-lpc-pcc.o
 obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
 obj-$(CONFIG_DUMMY_IRQ)		+= dummy-irq.o
 obj-$(CONFIG_ICS932S401)	+= ics932s401.o
diff --git a/drivers/misc/aspeed-lpc-pcc.c b/drivers/misc/aspeed-lpc-pcc.c
new file mode 100644
index 000000000000..f3cbf375d61f
--- /dev/null
+++ b/drivers/misc/aspeed-lpc-pcc.c
@@ -0,0 +1,441 @@
+// 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 v2 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Krzysztof Kozlowski]

On 04/03/2025 11:44, Kevin Chen wrote:
> +
> +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;

This goes to declaration.

> +
> +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);

Maybe my previous comment was not clear, but you agreed with it. Anyway
nothing improved here.

If you have 'dev' variable, use it.

> +	if (!pcc)
> +		return -ENOMEM;
> +
> +	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);

same here and everywhere else.

> +	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");

Drop

> +		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");

Drop, core already prints this. Do not duplicate messages.

> +		rc = -ENODEV;

Why not using pcc->irq as rc?

> +		goto err_free_kfifo;
> +	}
> +
Best regards,
Krzysztof

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

[Kevin Chen]

> On 04/03/2025 11:44, Kevin Chen wrote:
> > +
> > +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;
> 
> This goes to declaration.
OK. I will move it to declaration.

> 
> > +
> > +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
> 
> Maybe my previous comment was not clear, but you agreed with it. Anyway
> nothing improved here.
Could I reserve the pcc variable using for the pcc_ctrl data structure?
Pcc_ctrl data include the regmap/irq/io_port/dma/kfifo.
If I change the name to dev, it does not make sense for these data to be.

> 
> If you have 'dev' variable, use it.
Do you mean just use the pdev->dev not local dev variable?

> 
> > +	if (!pcc)
> > +		return -ENOMEM;
> > +
> > +	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
> 
> same here and everywhere else.
> 
> > +	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");
> 
> Drop
Agree.

> 
> > +		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");
> 
> Drop, core already prints this. Do not duplicate messages.
Agree.

> 
> > +		rc = -ENODEV;
> 
> Why not using pcc->irq as rc?
Agree.

> 
> > +		goto err_free_kfifo;
> > +	}
> > +
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 10/03/2025 11:05, Kevin Chen wrote:
>> On 04/03/2025 11:44, Kevin Chen wrote:
>>> +
>>> +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;
>>
>> This goes to declaration.
> OK. I will move it to declaration.
> 
>>
>>> +
>>> +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
>>
>> Maybe my previous comment was not clear, but you agreed with it. Anyway
>> nothing improved here.
> Could I reserve the pcc variable using for the pcc_ctrl data structure?
> Pcc_ctrl data include the regmap/irq/io_port/dma/kfifo.
> If I change the name to dev, it does not make sense for these data to be.
> 
>>
>> If you have 'dev' variable, use it.
> Do you mean just use the pdev->dev not local dev variable?
What is the point of this:
dev = &pdev->dev;

if you do not use 'dev'? If you come with a reason, sure. If you do not
have such, then everything should use 'dev', not pdev->dev.

Best regards,
Krzysztof

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

[Kevin Chen]

> >> On 04/03/2025 11:44, Kevin Chen wrote:
> >>> +
> >>> +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;
> >>
> >> This goes to declaration.
> > OK. I will move it to declaration.
> >
> >>
> >>> +
> >>> +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
> >>
> >> Maybe my previous comment was not clear, but you agreed with it.
> >> Anyway nothing improved here.
> > Could I reserve the pcc variable using for the pcc_ctrl data structure?
> > Pcc_ctrl data include the regmap/irq/io_port/dma/kfifo.
> > If I change the name to dev, it does not make sense for these data to be.
> >
> >>
> >> If you have 'dev' variable, use it.
> > Do you mean just use the pdev->dev not local dev variable?
> What is the point of this:
> dev = &pdev->dev;
> 
> if you do not use 'dev'? If you come with a reason, sure. If you do not have such,
> then everything should use 'dev', not pdev->dev.
Agree, I fixed it in my v3 patch, and I already submitted.
Thanks for your detailed explanation.
Sorry for my misunderstanding.

> 
> Best regards,
> Krzysztof

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

[Christophe JAILLET]

Le 04/03/2025 à 11:44, Kevin Chen a écrit :
> Add LPC PCC controller driver to support POST code capture.
> 
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>

Hi,

> +	init_waitqueue_head(&pcc->wq);
> +
> +	pcc->mdev_id = ida_alloc(&aspeed_pcc_ida, GFP_KERNEL);

Missing ida_free() in therror handling path and in the rmove function?

> +	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" },
> +	{ },

Unneeded trailing comma after a terminator.

> +};
> +
> +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,
> +};

...

CJ

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

[Kevin Chen]

> Le 04/03/2025 à 11:44, Kevin Chen a écrit :
> > Add LPC PCC controller driver to support POST code capture.
> >
> > Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> 
> Hi,
> 
> > +	init_waitqueue_head(&pcc->wq);
> > +
> > +	pcc->mdev_id = ida_alloc(&aspeed_pcc_ida, GFP_KERNEL);
> 
> Missing ida_free() in therror handling path and in the rmove function?
OK. I will add the ida_free function in remove function. Thanks a lot.

> 
> > +	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" },
> > +	{ },
> 
> Unneeded trailing comma after a terminator.
OK, I will remove it.

> 
> > +};
> > +
> > +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,
> > +};
> 
> ...
> 
> CJ

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

[kernel test robot]

Hi Kevin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus lee-leds/for-leds-next lee-mfd/for-mfd-next robh/for-next lee-mfd/for-mfd-fixes linus/master v6.14-rc5 next-20250305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kevin-Chen/ARM-dts-aspeed-g6-Add-AST2600-LPC-PCC-support/20250304-194530
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20250304104434.481429-4-kevin_chen%40aspeedtech.com
patch subject: [PATCH v2 3/3] soc: aspeed: lpc-pcc: Add PCC controller support
config: s390-kismet-CONFIG_MFD_SYSCON-CONFIG_ASPEED_LPC_PCC-0-0 (https://download.01.org/0day-ci/archive/20250306/202503060750.pkwFWR24-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250306/202503060750.pkwFWR24-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503060750.pkwFWR24-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for MFD_SYSCON when selected by ASPEED_LPC_PCC
   WARNING: unmet direct dependencies detected for MFD_SYSCON
     Depends on [n]: HAS_IOMEM [=n]
     Selected by [y]:
     - ASPEED_LPC_PCC [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Kevin,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus robh/for-next linus/master v6.14-rc6 next-20250307]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kevin-Chen/ARM-dts-aspeed-g6-Add-AST2600-LPC-PCC-support/20250304-194530
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20250304104434.481429-4-kevin_chen%40aspeedtech.com
patch subject: [PATCH v2 3/3] soc: aspeed: lpc-pcc: Add PCC controller support
config: s390-randconfig-r112-20250310 (https://download.01.org/0day-ci/archive/20250310/202503101519.AIZNCLz1-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250310/202503101519.AIZNCLz1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503101519.AIZNCLz1-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/io.h:14,
                    from include/linux/of_address.h:7,
                    from drivers/mfd/syscon.c:18:
   drivers/mfd/syscon.c: In function 'of_syscon_register':
>> arch/s390/include/asm/io.h:31:17: error: implicit declaration of function 'iounmap' [-Wimplicit-function-declaration]
      31 | #define iounmap iounmap
         |                 ^~~~~~~
   drivers/mfd/syscon.c:157:9: note: in expansion of macro 'iounmap'
     157 |         iounmap(base);
         |         ^~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MFD_SYSCON
   Depends on [n]: HAS_IOMEM [=n]
   Selected by [y]:
   - ASPEED_LPC_PCC [=y]


vim +/iounmap +31 arch/s390/include/asm/io.h

cd24834130ac65 Jan Glauber 2012-11-29  26  
b43b3fff042d08 Baoquan He  2023-07-06  27  /*
b43b3fff042d08 Baoquan He  2023-07-06  28   * I/O memory mapping functions.
b43b3fff042d08 Baoquan He  2023-07-06  29   */
b43b3fff042d08 Baoquan He  2023-07-06  30  #define ioremap_prot ioremap_prot
b43b3fff042d08 Baoquan He  2023-07-06 @31  #define iounmap iounmap
b43b3fff042d08 Baoquan He  2023-07-06  32  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki