[PATCH v1 0/2] Add support for AST2700 INTC driver

[PATCH v1 0/2] Add support for AST2700 INTC driver

[Kevin Chen]

Introduce to the AST27XX INTC modules, which contain two conponents in
CPU die(12nm) and IO die(40mm) comunicating by SLI or LTPI protocol.

There are lots of device in IO die, which need to be serviced in
requested interrupt handler. As two die ICs, combine 32 interrupt source
in IO die into 1 interrupt in CPU die.

soc0_intc11 represent CPU die INTC, which each bit mapping to soc1_intcX.
soc1_intcX represent IO die INTC, which combines 32 interrupt sources.

Kevin Chen (2):
  dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC
  irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX
    platforms

 .../aspeed,ast2700-intc.yaml                  | 120 +++++++++++
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-aspeed-intc.c             | 198 ++++++++++++++++++
 3 files changed, 319 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

-- 
2.34.1

[PATCH v1 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Kevin Chen]

The ASPEED AST27XX interrupt controller(INTC) combines 32 interrupt
sources into 1 interrupt into GIC from CPU die to CPU die.
The INTC design contains soc0_intc and soc1_intc module doing hand shake
between CPU die and IO die INTC.

In soc0_intc11, each bit represent 1 GIC_SPI interrupt from soc1_intcX.
In soc1_intcX, each bit represent 1 device interrupt in IO die.

By soc1_intcX in IO die, AST27XX INTC combines 32 interrupt sources to
1 interrupt source in soc0_intc11 in CPU die, which achieve the
interrupt passing between the different die in AST27XX.
---
 .../aspeed,ast2700-intc.yaml                  | 120 ++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
new file mode 100644
index 000000000000..93d7141bf9f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Interrupt Controller driver
+
+description:
+  These bindings are for the Aspeed interrupt controller. The AST2700
+  SoC families include a legacy register layout before a re-designed
+  layout, but the bindings do not prescribe the use of one or the other.
+
+maintainers:
+  - Kevin Chen <kevin_chen@aspeedtech.com>
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+        - enum:
+          - aspeed,ast2700-intc-ic
+          - aspeed,ast2700-intc-icv2
+        description: |
+          Use "aspeed,ast2700-intc-ic" for soc1 INTC in IO die
+          Use "aspeed,ast2700-intc-icv2" for soc0 INTC in CPU die
+
+  interrupt-controller: true
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 3
+    description:
+      Specifies which contexts are connected to the INTC, with "-1" specifying
+      that a context is not present. Each node pointed to should be a
+      aspeed,ast2700-intc-ic or aspeed,ast2700-intc-icv2 nodes which are pointed
+      to gic node.
+
+  "#address-cells":
+    const: 2
+  "#size-cells":
+    const: 2
+
+  '#interrupt-cells':
+    const: 2
+    description: |
+      The first cell cell is the interrupt source IRQ number, and the second cell
+      is the trigger type as defined in interrupt.txt in this directory.
+
+  reg:
+    minItems: 1
+    maxItems: 2
+    description: |
+      The first cell cell is the interrupt enable register, and the second cell
+      is the status register.
+
+  ranges: true
+
+  interrupts:
+    minItems: 1
+    maxItems: 10
+    description: |
+      Interrupt source of the CPU interrupts. In soc0_intc in CPU die INTC each bit
+      represent soc1_intc interrupt source. soc0_intc take care 10 interrupt source
+      from soc1_intc0~5 and ltpi0/1_soc1_intc0/1.
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - '#interrupt-cells'
+
+additionalProperties: false
+
+example:
+  - |
+    soc0_intc: interrupt-controller@12100000 {
+      compatible = "simple-mfd";
+      reg = <0 0x12100000 0 0x4000>;
+      #address-cells = <2>;
+      #size-cells = <2>;
+      ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;
+
+      soc0_intc11: interrupt-controller@1b00 {
+        compatible = "aspeed,ast2700-intc-icv2";
+        interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
+        #interrupt-cells = <2>;
+        interrupt-controller;
+        reg = <0x0 0x1b00 0x0 0x10>;
+      };
+    };
+
+  - |
+    soc1_intc: interrupt-controller@14c18000 {
+      compatible = "simple-mfd";
+      reg = <0 0x14c18000 0 0x400>;
+      #address-cells = <2>;
+      #size-cells = <2>;
+      ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
+
+      soc1_intc0: interrupt-controller@100 {
+       compatible = "aspeed,ast2700-intc-ic";
+        interrupts-extended = <&soc0_intc11 0 IRQ_TYPE_LEVEL_HIGH>;
+        #interrupt-cells = <2>;
+        interrupt-controller;
+        reg = <0x0 0x100 0x0 0x10>;
+      };
+    };
-- 
2.34.1

Re: [PATCH v1 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Krzysztof Kozlowski]

On 13/08/2024 09:43, Kevin Chen wrote:
> The ASPEED AST27XX interrupt controller(INTC) combines 32 interrupt
> sources into 1 interrupt into GIC from CPU die to CPU die.
> The INTC design contains soc0_intc and soc1_intc module doing hand shake
> between CPU die and IO die INTC.
> 
> In soc0_intc11, each bit represent 1 GIC_SPI interrupt from soc1_intcX.
> In soc1_intcX, each bit represent 1 device interrupt in IO die.
> 
> By soc1_intcX in IO die, AST27XX INTC combines 32 interrupt sources to
> 1 interrupt source in soc0_intc11 in CPU die, which achieve the
> interrupt passing between the different die in AST27XX.
> ---

This was never tested. Please do not send untested code.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Limited review follows due to lack of basic testing.

>  .../aspeed,ast2700-intc.yaml                  | 120 ++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> new file mode 100644
> index 000000000000..93d7141bf9f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Interrupt Controller driver

Drop driver, describe hardware. Aspeed or some specific SoC?

> +
> +description:
> +  These bindings are for the Aspeed interrupt controller. The AST2700

Drop first sentence. Pointless.

> +  SoC families include a legacy register layout before a re-designed
> +  layout, but the bindings do not prescribe the use of one or the other.

Entire description is pointless - you do not say anything valuable here.
Describe this hardware instead.


> +
> +maintainers:
> +  - Kevin Chen <kevin_chen@aspeedtech.com>
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop

> +      - items:

Drop

> +        - enum:
> +          - aspeed,ast2700-intc-ic
> +          - aspeed,ast2700-intc-icv2
> +        description: |
> +          Use "aspeed,ast2700-intc-ic" for soc1 INTC in IO die
> +          Use "aspeed,ast2700-intc-icv2" for soc0 INTC in CPU die

Use consistent naming. Isn't your other block called 0 and 1? Why using
different namings?

> +
> +  interrupt-controller: true
> +
> +  interrupts-extended:

interrupts instead.

> +    minItems: 1
> +    maxItems: 3
> +    description:
> +      Specifies which contexts are connected to the INTC, with "-1" specifying
> +      that a context is not present. Each node pointed to should be a
> +      aspeed,ast2700-intc-ic or aspeed,ast2700-intc-icv2 nodes which are pointed
> +      to gic node.

Don't repeat constraints in free form text. Describe items instead.


> +
> +  "#address-cells":
> +    const: 2

Blank line

> +  "#size-cells":
> +    const: 2
> +
> +  '#interrupt-cells':
> +    const: 2
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      The first cell cell is the interrupt source IRQ number, and the second cell
> +      is the trigger type as defined in interrupt.txt in this directory.
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +    description: |
> +      The first cell cell is the interrupt enable register, and the second cell
> +      is the status register.

List and describe the items instead.

> +
> +  ranges: true
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 10
> +    description: |
> +      Interrupt source of the CPU interrupts. In soc0_intc in CPU die INTC each bit
> +      represent soc1_intc interrupt source. soc0_intc take care 10 interrupt source
> +      from soc1_intc0~5 and ltpi0/1_soc1_intc0/1.

No, you cannot have both. That's total mess. Anyway, standard comment
applies - list and describe items.


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +
> +additionalProperties: false
> +
> +example:
> +  - |
> +    soc0_intc: interrupt-controller@12100000 {

Drop label

> +      compatible = "simple-mfd";

No, it's not. Drop. Look how other bindings do it.

> +      reg = <0 0x12100000 0 0x4000>;
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;

Read DTS coding style.

> +
> +      soc0_intc11: interrupt-controller@1b00 {

Drop label

> +        compatible = "aspeed,ast2700-intc-icv2";
> +        interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
> +        #interrupt-cells = <2>;
> +        interrupt-controller;
> +        reg = <0x0 0x1b00 0x0 0x10>;

DTS coding style

> +      };
> +    };
> +
> +  - |
> +    soc1_intc: interrupt-controller@14c18000 {
> +      compatible = "simple-mfd";
> +      reg = <0 0x14c18000 0 0x400>;
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
> +
> +      soc1_intc0: interrupt-controller@100 {
> +       compatible = "aspeed,ast2700-intc-ic";

Drop this example, almost no differences.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Rob Herring (Arm)]

On Tue, 13 Aug 2024 15:43:37 +0800, Kevin Chen wrote:
> The ASPEED AST27XX interrupt controller(INTC) combines 32 interrupt
> sources into 1 interrupt into GIC from CPU die to CPU die.
> The INTC design contains soc0_intc and soc1_intc module doing hand shake
> between CPU die and IO die INTC.
> 
> In soc0_intc11, each bit represent 1 GIC_SPI interrupt from soc1_intcX.
> In soc1_intcX, each bit represent 1 device interrupt in IO die.
> 
> By soc1_intcX in IO die, AST27XX INTC combines 32 interrupt sources to
> 1 interrupt source in soc0_intc11 in CPU die, which achieve the
> interrupt passing between the different die in AST27XX.
> ---
>  .../aspeed,ast2700-intc.yaml                  | 120 ++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml:25:11: [warning] wrong indentation: expected 12 but found 10 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml: 'example' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240813074338.969883-2-kevin_chen@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

RE: [PATCH v1 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Kevin Chen]

> > The ASPEED AST27XX interrupt controller(INTC) combines 32 interrupts
> > sources into 1 interrupt into GIC from CPU die to CPU die.
> > The INTC design contains soc0_intc and soc1_intc module doing hand
> > shake between CPU die and IO die INTC.
> >
> > In soc0_intc11, each bit represent 1 GIC_SPI interrupt from soc1_intcX.
> > In soc1_intcX, each bit represent 1 device interrupt in IO die.
> >
> > By soc1_intcX in IO die, AST27XX INTC combines 32 interrupt sources to
> > 1 interrupt source in soc0_intc11 in CPU die, which achieve the
> > interrupt passing between the different die in AST27XX.
> > ---
> >  .../aspeed,ast2700-intc.yaml                  | 120
> ++++++++++++++++++
> >  1 file changed, 120 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-
> > intc.yaml
> >
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc
> .yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
> ./Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc
> .yaml:25:11: [warning] wrong indentation: expected 12 but found 10
> (indentation)
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/inte
> rrupt-controller/aspeed,ast2700-intc.yaml: 'example' is not one of ['$id',
> '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf',
> 'definitions', '$defs', 'additionalProperties', 'dependencies',
> 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties',
> 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers',
> 'select', '$ref']
> 	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/202408130743
> 38.969883-2-kevin_chen@aspeedtech.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
Agree.

> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
Agree.

I will fix all the warnings in ' make dt_binding_check '.

Thanks a lot.

[PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms

[Kevin Chen]

There are 10 interrupt source of soc0_intc in CPU die INTC.
  1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
  2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
  3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
Request GIC interrupt to check each bit in status register to do next
level INTC handler.

In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
would check 32 bit of status register to do the requested device
handler.
---
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
 2 files changed, 199 insertions(+)
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15635812b2d6..d2fe686ae018 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
 obj-$(CONFIG_LS_EXTIRQ)			+= irq-ls-extirq.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o irq-aspeed-scu-ic.o
+obj-$(CONFIG_MACH_ASPEED_G7)		+= irq-aspeed-intc.o
 obj-$(CONFIG_STM32MP_EXTI)		+= irq-stm32mp-exti.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
new file mode 100644
index 000000000000..71407475fb27
--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-intc.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Aspeed Interrupt Controller.
+ *
+ *  Copyright (C) 2023 ASPEED Technology Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+
+#define INTC_INT_ENABLE_REG	0x00
+#define INTC_INT_STATUS_REG	0x04
+
+struct aspeed_intc_ic {
+	void __iomem		*base;
+	raw_spinlock_t		gic_lock;
+	raw_spinlock_t		intc_lock;
+	struct irq_domain	*irq_domain;
+};
+
+static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
+{
+	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long bit, status, flags;
+
+	chained_irq_enter(chip, desc);
+
+	raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
+	status = readl(intc_ic->base + INTC_INT_STATUS_REG);
+	for_each_set_bit(bit, &status, 32) {
+		generic_handle_domain_irq(intc_ic->irq_domain, bit);
+		writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
+	}
+	raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
+
+	chained_irq_exit(chip, desc);
+}
+
+static void aspeed_intc_irq_mask(struct irq_data *data)
+{
+	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
+	unsigned int mask = readl(intc_ic->base + INTC_INT_ENABLE_REG) & ~BIT(data->hwirq);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
+	writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
+	raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
+}
+
+static void aspeed_intc_irq_unmask(struct irq_data *data)
+{
+	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
+	unsigned int unmask = readl(intc_ic->base + INTC_INT_ENABLE_REG) | BIT(data->hwirq);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
+	writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
+	raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
+}
+
+static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
+					bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip aspeed_intc_chip = {
+	.name			= "ASPEED INTC",
+	.irq_mask		= aspeed_intc_irq_mask,
+	.irq_unmask		= aspeed_intc_irq_unmask,
+	.irq_set_affinity	= aspeed_intc_irq_set_affinity,
+};
+
+static int aspeed_intc_ic_map_irq_domain(struct irq_domain *domain, unsigned int irq,
+					 irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &aspeed_intc_chip, handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_intc_ic_irq_domain_ops = {
+	.map = aspeed_intc_ic_map_irq_domain,
+};
+
+static int __init aspeed_intc_ic_of_init(struct device_node *node, struct device_node *parent)
+{
+	struct aspeed_intc_ic *intc_ic;
+	int ret = 0;
+	int irq;
+
+	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
+	if (!intc_ic)
+		return -ENOMEM;
+
+	intc_ic->base = of_iomap(node, 0);
+	if (!intc_ic->base) {
+		pr_err("Failed to iomap intc_ic base\n");
+		ret = -ENOMEM;
+		goto err_free_ic;
+	}
+	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
+	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq) {
+		pr_err("Failed to get irq number\n");
+		ret = -EINVAL;
+		goto err_iounmap;
+	}
+
+	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
+						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
+	if (!intc_ic->irq_domain) {
+		ret = -ENOMEM;
+		goto err_iounmap;
+	}
+
+	raw_spin_lock_init(&intc_ic->gic_lock);
+	raw_spin_lock_init(&intc_ic->intc_lock);
+
+	intc_ic->irq_domain->name = "aspeed-intc-domain";
+
+	irq_set_chained_handler_and_data(irq,
+					 aspeed_intc_ic_irq_handler, intc_ic);
+
+	return 0;
+
+err_iounmap:
+	iounmap(intc_ic->base);
+err_free_ic:
+	kfree(intc_ic);
+	return ret;
+}
+
+static int __init aspeed_intc_ic_of_init_v2(struct device_node *node,
+					    struct device_node *parent)
+{
+	struct aspeed_intc_ic *intc_ic;
+	int ret = 0;
+	int irq, i;
+
+	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
+	if (!intc_ic)
+		return -ENOMEM;
+
+	intc_ic->base = of_iomap(node, 0);
+	if (!intc_ic->base) {
+		pr_err("Failed to iomap intc_ic base\n");
+		ret = -ENOMEM;
+		goto err_free_ic;
+	}
+	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
+	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
+
+	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
+						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
+	if (!intc_ic->irq_domain) {
+		ret = -ENOMEM;
+		goto err_iounmap;
+	}
+
+	raw_spin_lock_init(&intc_ic->gic_lock);
+	raw_spin_lock_init(&intc_ic->intc_lock);
+
+	intc_ic->irq_domain->name = "aspeed-intc-domain";
+
+	for (i = 0; i < of_irq_count(node); i++) {
+		irq = irq_of_parse_and_map(node, i);
+		if (!irq) {
+			pr_err("Failed to get irq number\n");
+			ret = -EINVAL;
+			goto err_iounmap;
+		} else {
+			irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);
+		}
+	}
+
+	return 0;
+
+err_iounmap:
+	iounmap(intc_ic->base);
+err_free_ic:
+	kfree(intc_ic);
+	return ret;
+}
+
+IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-1.0", aspeed_intc_ic_of_init);
+IRQCHIP_DECLARE(ast2700_intc_icv2, "aspeed,ast2700-intc-2.0", aspeed_intc_ic_of_init_v2);
-- 
2.34.1

Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms

[Krzysztof Kozlowski]

On 13/08/2024 09:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
>   1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
>   2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
>   3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
> 
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
> ---
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
>  2 files changed, 199 insertions(+)
>  create mode 100644 drivers/irqchip/irq-aspeed-intc.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15635812b2d6..d2fe686ae018 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
>  obj-$(CONFIG_LS_EXTIRQ)			+= irq-ls-extirq.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o irq-aspeed-scu-ic.o
> +obj-$(CONFIG_MACH_ASPEED_G7)		+= irq-aspeed-intc.o

There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.

You already received feedback on this, so why do you keep pushing your
solution? You did not respond to any feedback given, just send the same
and the same till we agree?

NAK.

>  obj-$(CONFIG_STM32MP_EXTI)		+= irq-stm32mp-exti.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
> new file mode 100644
> index 000000000000..71407475fb27

...

> +static int __init aspeed_intc_ic_of_init_v2(struct device_node *node,
> +					    struct device_node *parent)
> +{
> +	struct aspeed_intc_ic *intc_ic;
> +	int ret = 0;
> +	int irq, i;
> +
> +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> +	if (!intc_ic)
> +		return -ENOMEM;
> +
> +	intc_ic->base = of_iomap(node, 0);
> +	if (!intc_ic->base) {
> +		pr_err("Failed to iomap intc_ic base\n");
> +		ret = -ENOMEM;
> +		goto err_free_ic;
> +	}
> +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> +
> +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> +	if (!intc_ic->irq_domain) {
> +		ret = -ENOMEM;
> +		goto err_iounmap;
> +	}
> +
> +	raw_spin_lock_init(&intc_ic->gic_lock);
> +	raw_spin_lock_init(&intc_ic->intc_lock);
> +
> +	intc_ic->irq_domain->name = "aspeed-intc-domain";
> +
> +	for (i = 0; i < of_irq_count(node); i++) {
> +		irq = irq_of_parse_and_map(node, i);
> +		if (!irq) {
> +			pr_err("Failed to get irq number\n");
> +			ret = -EINVAL;
> +			goto err_iounmap;
> +		} else {
> +			irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);
> +		}
> +	}
> +
> +	return 0;
> +
> +err_iounmap:
> +	iounmap(intc_ic->base);
> +err_free_ic:
> +	kfree(intc_ic);
> +	return ret;
> +}

Don't duplicate code. These are almost the same, so define one function
which is then called by aspeed_intc_ic_of_init and
aspeed_intc_ic_of_init_v2.

> +
> +IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-1.0", aspeed_intc_ic_of_init);
> +IRQCHIP_DECLARE(ast2700_intc_icv2, "aspeed,ast2700-intc-2.0", aspeed_intc_ic_of_init_v2);

Best regards,
Krzysztof

Re: 回覆: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms

[Krzysztof Kozlowski]

On 13/08/2024 11:44, Kevin Chen wrote:
> Hi Krzk,
> 
> In ASPEED, ast2400/2500/2600 use arm architecture with KCONFIG_ARCH_ASPEED which slect MACH_ASPEED_G4/G5/G6 in arch/arm/mach-aspeed/Kconfig.
> In the fureture, there would be ast2800/2900/... using arm64. We need to clarify the IC generation between 7th/8th/9th/....
> 
> Maybe change ARCH_ASPEED/MACH_ASPEEDG7 to ARCH_ASPEED first.
> Or, do you have better Kconfig usage?

Fix your quotes and do not top-post.

Please respond inline, instead of top-posting, because it makes your
emails hard to follow.
https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L340

> 
> 
>> +config ARCH_ASPEED
>> +     bool "Aspeed SoC family"
>> +     select MACH_ASPEED_G7
>> +     help
>> +       Say yes if you intend to run on an Aspeed ast2700 or similar
>> +       seventh generation Aspeed BMCs.
>> +
>> +config MACH_ASPEED_G7
>> +     bool "Aspeed SoC AST2700"
> 
> There are no MACHines for arm64. Look at this code. Do you see MACH
> anywhere else? No. Then why Aspeed must be different?

What is this?

> 
> --
> Best Regards,
> Kevin. Chen
> 
> ________________________________
> 寄件者: Krzysztof Kozlowski <krzk@kernel.org>
> 寄件日期: 2024年8月13日 下午 04:50
> 收件者: Kevin Chen <kevin_chen@aspeedtech.com>; tglx@linutronix.de <tglx@linutronix.de>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>; joel@jms.id.au <joel@jms.id.au>; andrew@codeconstruct.com.au <andrew@codeconstruct.com.au>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>
> 主旨: Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
> 

...

>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15635812b2d6..d2fe686ae018 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -84,6 +84,7 @@ obj-$(CONFIG_MVEBU_SEI)                     += irq-mvebu-sei.o
>>  obj-$(CONFIG_LS_EXTIRQ)                      += irq-ls-extirq.o
>>  obj-$(CONFIG_LS_SCFG_MSI)            += irq-ls-scfg-msi.o
>>  obj-$(CONFIG_ARCH_ASPEED)            += irq-aspeed-vic.o irq-aspeed-i2c-ic.o irq-aspeed-scu-ic.o
>> +obj-$(CONFIG_MACH_ASPEED_G7)         += irq-aspeed-intc.o
> 
> There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
> 
> You already received feedback on this, so why do you keep pushing your
> solution? You did not respond to any feedback given, just send the same
> and the same till we agree?
> 
> NAK.

And this?

> 
>>  obj-$(CONFIG_STM32MP_EXTI)           += irq-stm32mp-exti.o
>>  obj-$(CONFIG_STM32_EXTI)              += irq-stm32-exti.o
>>  obj-$(CONFIG_QCOM_IRQ_COMBINER)              += qcom-irq-combiner.o
>> diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
>> new file mode 100644
>> index 000000000000..71407475fb27
> 

...

> 
> ************* Email Confidentiality Notice ********************
> 免責聲明:
> 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
> 
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

Maybe I am the intended recipient of your message, maybe not. I don't
want to have any legal questions regarding upstream, public
collaboration, thus probably I should just remove your messages.

Please talk with your IT that such disclaimers in open-source are not
desired (and maybe even harmful).
If you do not understand why, please also see:
https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835

If you need to go around company SMTP server, then consider using b4
web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html

Please be informed that by responding to this email you agree that all
communications from you and/or your company is made public. In other
words, all messages originating from you and/or your company will be
made public.

You already received exactly this feedback. Around three times. If you
keep ignoring feedback, I will keep NAKing your patches.

Best regards,
Krzysztof

Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms

[Thomas Gleixner]

On Tue, Aug 13 2024 at 15:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
>   1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
>   2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
>   3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.

I can't figure out what this word salad is trying to tell me. Nothing in
the code does any combining. The handler reads the very same
INTC_INT_STATUS_REG.

>

This lacks a Signed-off-by: tag. See Documentation/process/

> ---
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> +
> +#define INTC_INT_ENABLE_REG	0x00
> +#define INTC_INT_STATUS_REG	0x04
> +
> +struct aspeed_intc_ic {
> +	void __iomem		*base;
> +	raw_spinlock_t		gic_lock;
> +	raw_spinlock_t		intc_lock;
> +	struct irq_domain	*irq_domain;
> +};
> +
> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
> +{
> +	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long bit, status, flags;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);

There is no point for irqsave(). This code is invoked with interrupts
disabled and please convert to:

        scoped_guard(raw_spinlock, &intc_ic->gic_lock) {

> +	status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> +	for_each_set_bit(bit, &status, 32) {

Please use a define and not a hardcoded number.

> +		generic_handle_domain_irq(intc_ic->irq_domain, bit);
> +		writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> +	}

        }

> +	raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void aspeed_intc_irq_mask(struct irq_data *data)
> +{
> +	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
> +	unsigned int mask = readl(intc_ic->base + INTC_INT_ENABLE_REG) & ~BIT(data->hwirq);
> +	unsigned long flags;

Invoked with interrupts disabled too.

> +	raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> +	writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> +	raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);

        guard(raw_spinlock)(&intc_ic->intc_lock);
	writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);

> +}
> +
> +static void aspeed_intc_irq_unmask(struct irq_data *data)
> +{
> +	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
> +	unsigned int unmask = readl(intc_ic->base + INTC_INT_ENABLE_REG) | BIT(data->hwirq);
> +	unsigned long flags;

Ditto.

> +	raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> +	writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> +	raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
> +}
> +
> +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> +					bool force)
> +{
> +	return -EINVAL;
> +}

No point for this stub, just leave irq_set_affinity uninitialized. The
core code checks that pointer for NULL. Aside of that this stub and the
assignment would need a #ifdef CONFIG_SMP guard.

> +static struct irq_chip aspeed_intc_chip = {
> +	.name			= "ASPEED INTC",
> +	.irq_mask		= aspeed_intc_irq_mask,
> +	.irq_unmask		= aspeed_intc_irq_unmask,
> +	.irq_set_affinity	= aspeed_intc_irq_set_affinity,
> +};
> +
> +static int aspeed_intc_ic_map_irq_domain(struct irq_domain *domain, unsigned int irq,
> +					 irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &aspeed_intc_chip, handle_level_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops aspeed_intc_ic_irq_domain_ops = {
> +	.map = aspeed_intc_ic_map_irq_domain,

	.map	= aspeed_intc_ic_map_irq_domain,

> +};
> +
> +static int __init aspeed_intc_ic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct aspeed_intc_ic *intc_ic;
> +	int ret = 0;
> +	int irq;

        int irq, ret;

No point in initializing ret.

> +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> +	if (!intc_ic)
> +		return -ENOMEM;
> +
> +	intc_ic->base = of_iomap(node, 0);
> +	if (!intc_ic->base) {
> +		pr_err("Failed to iomap intc_ic base\n");
> +		ret = -ENOMEM;
> +		goto err_free_ic;
> +	}
> +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (!irq) {
> +		pr_err("Failed to get irq number\n");
> +		ret = -EINVAL;
> +		goto err_iounmap;
> +	}
> +
> +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> +	if (!intc_ic->irq_domain) {
> +		ret = -ENOMEM;
> +		goto err_iounmap;
> +	}
> +
> +	raw_spin_lock_init(&intc_ic->gic_lock);
> +	raw_spin_lock_init(&intc_ic->intc_lock);
> +
> +	intc_ic->irq_domain->name = "aspeed-intc-domain";

See above.

> +	irq_set_chained_handler_and_data(irq,
> +					 aspeed_intc_ic_irq_handler, intc_ic);
> +
> +	return 0;
> +
> +err_iounmap:
> +	iounmap(intc_ic->base);
> +err_free_ic:
> +	kfree(intc_ic);
> +	return ret;
> +}
> +
> +static int __init aspeed_intc_ic_of_init_v2(struct device_node *node,
> +					    struct device_node *parent)
> +{
> +	struct aspeed_intc_ic *intc_ic;
> +	int ret = 0;
> +	int irq, i;
> +
> +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> +	if (!intc_ic)
> +		return -ENOMEM;
> +
> +	intc_ic->base = of_iomap(node, 0);
> +	if (!intc_ic->base) {
> +		pr_err("Failed to iomap intc_ic base\n");
> +		ret = -ENOMEM;
> +		goto err_free_ic;
> +	}
> +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> +
> +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> +	if (!intc_ic->irq_domain) {
> +		ret = -ENOMEM;
> +		goto err_iounmap;
> +	}
> +
> +	raw_spin_lock_init(&intc_ic->gic_lock);
> +	raw_spin_lock_init(&intc_ic->intc_lock);
> +
> +	intc_ic->irq_domain->name = "aspeed-intc-domain";

So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not
rocket science to make that work.

> +	for (i = 0; i < of_irq_count(node); i++) {
> +		irq = irq_of_parse_and_map(node, i);
> +		if (!irq) {
> +			pr_err("Failed to get irq number\n");
> +			ret = -EINVAL;
> +			goto err_iounmap;

Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and
the irqdomain around, but then unmaps the base and frees the data which
the handler and the domain code needs. Seriously?

> +		} else {

Pointless else as the if clause terminates with a goto.

> +			irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);

So if I understand the code correctly then the hardware coalesces the
pending bits into a single status register, but depending on which part
of the SoC raised the interrupt one of the demultiplex interrupts is
raised in the GIC.

Any of those demultiplex interrupt handles _all_ pending bits and
therefore you need gic_lock to serialize them, right?

Thanks,

        tglx

RE: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms

[Kevin Chen]

> > There are 10 interrupt sources of soc0_intc in CPU die INTC.
> >   1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> >   2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> >   3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> > Request GIC interrupt to check each bit in status register to do next
> > level INTC handler.
> >
> > In next level INTC handler of IO die or LTPI INTC using soc1_intcX
> > combining
> > 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX,
> > handler would check 32 bit of status register to do the requested
> > device handler.
> 
> I can't figure out what this word salad is trying to tell me. Nothing in the code
> does any combining. The handler reads the very same INTC_INT_STATUS_REG.
According to AST2700 datasheet, there are two kinds of interrupt controller with enable and raw status registers for use.
  1. INTC0 is used to assert GIC(#192~#197) if interrupt in INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in one INTC0.
  2. INTC1 is used to assert INTC0 if interrupt of modules asserted. There are 32 module interrupts used in one INTC1.
+------+   +---------+      +-----------+ ---module0
| GIC | -----|INTC0 | ---+----| INTC1_0|---module2...
+------+   +---------+  |   +-----------+---module31
                  |
                  |   +-----------+---module0
                  +-----| INTC1_0|---module2...
                  |   +-----------+---module31
                  ...
                  |   +-----------+---module0
                  +-----| INTC1_5|---module2...
                  |   +-----------+---module31
> 
> >
> 
> This lacks a Signed-off-by: tag. See Documentation/process/
> 
> > ---
> >  drivers/irqchip/Makefile          |   1 +
> >  drivers/irqchip/irq-aspeed-intc.c | 198
> > ++++++++++++++++++++++++++++++
> > +
> > +#define INTC_INT_ENABLE_REG	0x00
> > +#define INTC_INT_STATUS_REG	0x04
> > +
> > +struct aspeed_intc_ic {
> > +	void __iomem		*base;
> > +	raw_spinlock_t		gic_lock;
> > +	raw_spinlock_t		intc_lock;
> > +	struct irq_domain	*irq_domain;
> > +};
> > +
> > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) {
> > +	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	unsigned long bit, status, flags;
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
> 
> There is no point for irqsave(). This code is invoked with interrupts disabled and
> please convert to:
> 
>         scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
Agree.

> 
> > +	status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> > +	for_each_set_bit(bit, &status, 32) {
> 
> Please use a define and not a hardcoded number.
Agree.

> 
> > +		generic_handle_domain_irq(intc_ic->irq_domain, bit);
> > +		writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> > +	}
> 
>         }
> 
> > +	raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void aspeed_intc_irq_mask(struct irq_data *data) {
> > +	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
> > +	unsigned int mask = readl(intc_ic->base + INTC_INT_ENABLE_REG) &
> ~BIT(data->hwirq);
> > +	unsigned long flags;
> 
> Invoked with interrupts disabled too.
Agree.

> 
> > +	raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> > +	writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> > +	raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
> 
>         guard(raw_spinlock)(&intc_ic->intc_lock);
Agree.

> 	writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> 
> > +}
> > +
> > +static void aspeed_intc_irq_unmask(struct irq_data *data) {
> > +	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
> > +	unsigned int unmask = readl(intc_ic->base + INTC_INT_ENABLE_REG) |
> BIT(data->hwirq);
> > +	unsigned long flags;
> 
> Ditto.
Agree.

> 
> > +	raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> > +	writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> > +	raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags); }
> > +
> > +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct
> cpumask *dest,
> > +					bool force)
> > +{
> > +	return -EINVAL;
> > +}
> 
> No point for this stub, just leave irq_set_affinity uninitialized. The core code
> checks that pointer for NULL. Aside of that this stub and the assignment would
> need a #ifdef CONFIG_SMP guard.
Agree.

> 
> > +static struct irq_chip aspeed_intc_chip = {
> > +	.name			= "ASPEED INTC",
> > +	.irq_mask		= aspeed_intc_irq_mask,
> > +	.irq_unmask		= aspeed_intc_irq_unmask,
> > +	.irq_set_affinity	= aspeed_intc_irq_set_affinity,
> > +};
> > +
> > +static int aspeed_intc_ic_map_irq_domain(struct irq_domain *domain,
> unsigned int irq,
> > +					 irq_hw_number_t hwirq)
> > +{
> > +	irq_set_chip_and_handler(irq, &aspeed_intc_chip, handle_level_irq);
> > +	irq_set_chip_data(irq, domain->host_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops aspeed_intc_ic_irq_domain_ops = {
> > +	.map = aspeed_intc_ic_map_irq_domain,
> 
> 	.map	= aspeed_intc_ic_map_irq_domain,
Agree.

> 
> > +};
> > +
> > +static int __init aspeed_intc_ic_of_init(struct device_node *node,
> > +struct device_node *parent) {
> > +	struct aspeed_intc_ic *intc_ic;
> > +	int ret = 0;
> > +	int irq;
> 
>         int irq, ret;
Agree.

> 
> No point in initializing ret.
Agree.

> 
> > +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> > +	if (!intc_ic)
> > +		return -ENOMEM;
> > +
> > +	intc_ic->base = of_iomap(node, 0);
> > +	if (!intc_ic->base) {
> > +		pr_err("Failed to iomap intc_ic base\n");
> > +		ret = -ENOMEM;
> > +		goto err_free_ic;
> > +	}
> > +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> > +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> > +
> > +	irq = irq_of_parse_and_map(node, 0);
> > +	if (!irq) {
> > +		pr_err("Failed to get irq number\n");
> > +		ret = -EINVAL;
> > +		goto err_iounmap;
> > +	}
> > +
> > +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> > +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> > +	if (!intc_ic->irq_domain) {
> > +		ret = -ENOMEM;
> > +		goto err_iounmap;
> > +	}
> > +
> > +	raw_spin_lock_init(&intc_ic->gic_lock);
> > +	raw_spin_lock_init(&intc_ic->intc_lock);
> > +
> > +	intc_ic->irq_domain->name = "aspeed-intc-domain";
> 
> See above.
Do you mean the name of "ASPEED INTC" would be covered by "aspeed-intc-doman"?

> 
> > +	irq_set_chained_handler_and_data(irq,
> > +					 aspeed_intc_ic_irq_handler, intc_ic);
> > +
> > +	return 0;
> > +
> > +err_iounmap:
> > +	iounmap(intc_ic->base);
> > +err_free_ic:
> > +	kfree(intc_ic);
> > +	return ret;
> > +}
> > +
> > +static int __init aspeed_intc_ic_of_init_v2(struct device_node *node,
> > +					    struct device_node *parent)
> > +{
> > +	struct aspeed_intc_ic *intc_ic;
> > +	int ret = 0;
> > +	int irq, i;
> > +
> > +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> > +	if (!intc_ic)
> > +		return -ENOMEM;
> > +
> > +	intc_ic->base = of_iomap(node, 0);
> > +	if (!intc_ic->base) {
> > +		pr_err("Failed to iomap intc_ic base\n");
> > +		ret = -ENOMEM;
> > +		goto err_free_ic;
> > +	}
> > +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> > +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> > +
> > +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> > +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> > +	if (!intc_ic->irq_domain) {
> > +		ret = -ENOMEM;
> > +		goto err_iounmap;
> > +	}
> > +
> > +	raw_spin_lock_init(&intc_ic->gic_lock);
> > +	raw_spin_lock_init(&intc_ic->intc_lock);
> > +
> > +	intc_ic->irq_domain->name = "aspeed-intc-domain";
> 
> So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
> aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not rocket
> science to make that work.
Agree.

> 
> > +	for (i = 0; i < of_irq_count(node); i++) {
> > +		irq = irq_of_parse_and_map(node, i);
> > +		if (!irq) {
> > +			pr_err("Failed to get irq number\n");
> > +			ret = -EINVAL;
> > +			goto err_iounmap;
> 
> Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and the
> irqdomain around, but then unmaps the base and frees the data which the
> handler and the domain code needs. Seriously?
So, do you recommend moving check irq out of for loop?
And, irq_set_chained_hanlder_and_data in another for loop?


> 
> > +		} else {
> 
> Pointless else as the if clause terminates with a goto.
Agree. I will remove the else

> 
> > +			irq_set_chained_handler_and_data(irq,
> aspeed_intc_ic_irq_handler,
> > +intc_ic);
> 
> So if I understand the code correctly then the hardware coalesces the pending
> bits into a single status register, but depending on which part of the SoC raised
> the interrupt one of the demultiplex interrupts is raised in the GIC.
Yes. 

> 
> Any of those demultiplex interrupt handles _all_ pending bits and therefore
> you need gic_lock to serialize them, right?
Yes.

> 
> Thanks,
> 
>         tglx
Thanks a lot for your review.