irqchip: aspeed: Add AST2700 INTC debugfs support and yaml update
Subsystems (3)
Ryan Chen
reviewerv3 cover irqchip: aspeed: Add AST2700 INTC debugfs support and yaml update
This patch series adds device tree bindings and driver support for the AST2700 SoC��s two interrupt controllers (INTC0 and INTC1), along with debugfs entries for runtime inspection of routing and register protection status, and bindings example refine.
v3:
- aspeed,ast2700-intc.yaml
- improve commit message description.
- irq-aspeed-intc.c
- add platform driver for "aspeed,ast2700-intc0/1" compatible nodes.
v2:
- fix dt bindingcheck
Ryan Chen (2):
dt-bindings: interrupt-controller: aspeed: Add parent node compatibles
and refine documentation
irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1
routing/protection display
.../aspeed,ast2700-intc.yaml | 158 ++++++++---- drivers/irqchip/irq-aspeed-intc.c | 238 ++++++++++++++++++ 2 files changed, 353 insertions(+), 43 deletions(-)
Signature · 1 line
2.34.1
Ryan Chen
reviewerPATCH v3 1/2 dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
The AST2700 SoC contains two independent top-level interrupt controllers (INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parent nodes of INTC0 and INTC1.
- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC in the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.
This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
--- .../aspeed,ast2700-intc.yaml | 158 +++++++++++++----- 1 file changed, 115 insertions(+), 43 deletions(-)
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +115 -43
@@ -10,6 +10,33 @@ description: This interrupt controller hardware is second level interrupt controller that is hooked to a parent interrupt controller. It's useful to combine multiple interrupt sources into 1 interrupt to parent interrupt controller. + Depend to which INTC0 or INTC1 used. + INTC0 and INTC1 are two kinds of interrupt controller with enable and raw + status registers for use. + INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 | + | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31 maintainers: - Kevin Chen <[email protected]>
@@ -17,49 +44,70 @@ maintainers: properties: compatible: enum: - - aspeed,ast2700-intc-ic + - aspeed,ast2700-intc0 + - aspeed,ast2700-intc1 reg: maxItems: 1 - interrupt-controller: true + '#address-cells': + const: 2 - '#interrupt-cells': + '#size-cells': const: 2 - description: - The first cell is the IRQ number, the second cell is the trigger - type as defined in interrupt.txt in this directory. - - interrupts: - maxItems: 6 - description: | - Depend to which INTC0 or INTC1 used. - INTC0 and INTC1 are two kinds of interrupt controller with enable and raw - status registers for use. - INTC0 is used to assert GIC if interrupt in INTC1 asserted. - INTC1 is used to assert INTC0 if interrupt of modules asserted. - +-----+ +-------+ +---------+---module0 - | GIC |---| INTC0 |--+--| INTC1_0 |---module2 - | | | | | | |---... - +-----+ +-------+ | +---------+---module31 - | - | +---------+---module0 - +---| INTC1_1 |---module2 - | | |---... - | +---------+---module31 - ... - | +---------+---module0 - +---| INTC1_5 |---module2 - | |---... - +---------+---module31 + ranges: true + +patternProperties: + "^interrupt-controller@": + type: object + description: Interrupt group child nodes + additionalProperties: false + + properties: + compatible: + enum: + - aspeed,ast2700-intc-ic + + reg: + maxItems: 1 + + interrupt-controller: true + + '#interrupt-cells': + const: 2 + description: | + The first cell is the IRQ number, the second cell is the trigger + type as defined in interrupt.txt in this directory. + + interrupts: + minItems: 1 + maxItems: 6 + description: | + The interrupts provided by this interrupt controller. + + interrupts-extended: + minItems: 1 + maxItems: 6 + description: | + This property is required when defining a cascaded interrupt controller + that is connected under another interrupt controller. It specifies the + parent interrupt(s) in the upstream controller to which this controller + is connected. + + oneOf: + - required: [interrupts] + - required: [interrupts-extended] + + required: + - compatible + - reg + - interrupt-controller + - '#interrupt-cells' required: - compatible - reg - - interrupt-controller - - '#interrupt-cells' - - interrupts additionalProperties: false
@@ -68,19 +116,43 @@ examples: #include <dt-bindings/interrupt-controller/arm-gic.h> bus { + #address-cells = <2>; + #size-cells = <2>; + + intc0: interrupt-controller@12100000 { + compatible = "aspeed,ast2700-intc0"; + reg = <0 0x12100000 0 0x4000>; + ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>; + #address-cells = <2>; + #size-cells = <2>; + + intc0_11: interrupt-controller@1b00 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0 0x12101b00 0 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; + }; + }; + + intc1: interrupt-controller@14c18000 { + compatible = "aspeed,ast2700-intc1"; + reg = <0 0x14c18000 0 0x400>; + ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>; #address-cells = <2>; #size-cells = <2>; - interrupt-controller@12101b00 { - compatible = "aspeed,ast2700-intc-ic"; - reg = <0 0x12101b00 0 0x10>; - #interrupt-cells = <2>; - interrupt-controller; - 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>; + intc1_0: interrupt-controller@100 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0x0 0x100 0x0 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc0_11 0 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; }; + }; };
Signature · 1 line
2.34.1
Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 4 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +0
-4- interrupt-controller@12101b00 { - compatible = "aspeed,ast2700-intc-ic"; - reg = <0 0x12101b00 0 0x10>; - #interrupt-cells = <2>;
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml examples: · line 152
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +3
-7- interrupt-controller; - 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>; + intc1_0: interrupt-controller@100 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0x0 0x100 0x0 0x10>;
I doubt that the controller base address is at 0x100 ...
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 19 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 4 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +0
-4- interrupt-controller@12101b00 { - compatible = "aspeed,ast2700-intc-ic"; - reg = <0 0x12101b00 0 0x10>; - #interrupt-cells = <2>;Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml examples: · line 152
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +3
-7- interrupt-controller; - 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>; + intc1_0: interrupt-controller@100 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0x0 0x100 0x0 0x10>;
Quoted 1 line
I doubt that the controller base address is at
0x100...
Sorry, besides the interrupt cascade, our interrupt architecture is most like this one.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/marvell%2Ccp110-icu.yaml#L74-L98
and also others
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/mmc/aspeed%2Csdhci.yaml#L83-L107
I don't understand you doubt it, and also we have proven in our internal Linux release.
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L1676-L1730
Could you point out more information what you doubt? And I can provide more information.
Rob Herring
maintainerRe: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
In reply to Ryan ChenQuoted 17 lines · Ryan Chen
The AST2700 SoC contains two independent top-level interrupt controllers (INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parent nodes of INTC0 and INTC1.
- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC in the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.
Quoted 3 lines
This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
That's nice, but is an ABI break.
Quoted 100 lines
Signed-off-by Ryan Chen <[email protected]>--- .../aspeed,ast2700-intc.yaml | 158 +++++++++++++----- 1 file changed, 115 insertions(+), 43 deletions(-)Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +43
-29@@ -10,6 +10,33 @@ description: This interrupt controller hardware is second level interrupt controller that is hooked to a parent interrupt controller. It's useful to combine multiple interrupt sources into 1 interrupt to parent interrupt controller. + Depend to which INTC0 or INTC1 used. + INTC0 and INTC1 are two kinds of interrupt controller with enable and raw + status registers for use. + INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 | + | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31 maintainers: - Kevin Chen <[email protected]>@@ -17,49 +44,70 @@ maintainers: properties: compatible: enum: - - aspeed,ast2700-intc-ic + - aspeed,ast2700-intc0 + - aspeed,ast2700-intc1 reg: maxItems: 1 - interrupt-controller: true + '#address-cells': + const: 2 - '#interrupt-cells': + '#size-cells': const: 2 - description: - The first cell is the IRQ number, the second cell is the trigger - type as defined in interrupt.txt in this directory. - - interrupts: - maxItems: 6 - description: | - Depend to which INTC0 or INTC1 used. - INTC0 and INTC1 are two kinds of interrupt controller with enable and raw - status registers for use. - INTC0 is used to assert GIC if interrupt in INTC1 asserted. - INTC1 is used to assert INTC0 if interrupt of modules asserted. - +-----+ +-------+ +---------+---module0 - | GIC |---| INTC0 |--+--| INTC1_0 |---module2 - | | | | | | |---... - +-----+ +-------+ | +---------+---module31 - | - | +---------+---module0 - +---| INTC1_1 |---module2 - | | |---... - | +---------+---module31 - ... - | +---------+---module0 - +---| INTC1_5 |---module2 - | |---... - +---------+---module31 + ranges: true + +patternProperties: + "^interrupt-controller@": + type: object + description: Interrupt group child nodes + additionalProperties: false + + properties: + compatible: + enum:
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers: · line 79
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ - aspeed,ast2700-intc-ic + + reg: + maxItems: 1 + + interrupt-controller: true + + '#interrupt-cells': + const: 2 + description: |
Don't need '|'.
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers: · line 81
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +2
-0+ The first cell is the IRQ number, the second cell is the trigger + type as defined in interrupt.txt in this directory.
Don't add links to legacy documents.
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers: · line 89
Quoted 8 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +8
-0+ + interrupts: + minItems: 1 + maxItems: 6 + description: | + The interrupts provided by this interrupt controller. + + interrupts-extended:
Why do you have both interrupts and interrupts-extended? They are mutually exclusive and both are auto-magically supported. The schemas only have to document 'interrupts'.
Quoted 28 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +17
-3+ minItems: 1 + maxItems: 6 + description: | + This property is required when defining a cascaded interrupt controller + that is connected under another interrupt controller. It specifies the + parent interrupt(s) in the upstream controller to which this controller + is connected. + + oneOf: + - required: [interrupts] + - required: [interrupts-extended] + + required: + - compatible + - reg + - interrupt-controller + - '#interrupt-cells' required: - compatible - reg - - interrupt-controller - - '#interrupt-cells' - - interrupts additionalProperties: false
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml examples: · line 122
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +4
-0@@ -68,19 +116,43 @@ examples: #include <dt-bindings/interrupt-controller/arm-gic.h> bus { + #address-cells = <2>; + #size-cells = <2>; + + intc0: interrupt-controller@12100000 {
This node isn't an interrupt-controller.
Quoted 50 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +31
-11+ compatible = "aspeed,ast2700-intc0"; + reg = <0 0x12100000 0 0x4000>; + ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>; + #address-cells = <2>; + #size-cells = <2>; + + intc0_11: interrupt-controller@1b00 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0 0x12101b00 0 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>, + <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; + }; + }; + + intc1: interrupt-controller@14c18000 { + compatible = "aspeed,ast2700-intc1"; + reg = <0 0x14c18000 0 0x400>; + ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>; #address-cells = <2>; #size-cells = <2>; - interrupt-controller@12101b00 { - compatible = "aspeed,ast2700-intc-ic"; - reg = <0 0x12101b00 0 0x10>; - #interrupt-cells = <2>; - interrupt-controller; - 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>; + intc1_0: interrupt-controller@100 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0x0 0x100 0x0 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc0_11 0 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; }; + }; };Signature · 1 line
2.34.1
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 22 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 18 lines · Ryan Chen
The AST2700 SoC contains two independent top-level interrupt controllers
(INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parent nodes of INTC0 and INTC1.- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC in
the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.
Quoted 5 lines
Quoted 3 lines
This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
That's nice, but is an ABI break.
Sorry, Is it an ABI break?
I keep the compatible aspeed,ast2700-intc-ic, not change it.
Just move it to be child, and make aspeed,ast2700-intc0/1 to be parent for aspeed,ast2700-intc-ic.
bus {
#address-cells = <2>;
#size-cells = <2>;
interrupt-controller@12101b00 {
compatible = "aspeed,ast2700-intc-ic";
reg = <0 0x12101b00 0 0x10>;
#interrupt-cells = <2>;
interrupt-controller;
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>;
};
}; New parent and child
bus {
#address-cells = <2>;
#size-cells = <2>;
intc0: interrupt-controller@12100000 {
compatible = "aspeed,ast2700-intc0";
reg = <0 0x12100000 0 0x4000>;
ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;
#address-cells = <2>;
#size-cells = <2>;
intc0_11: interrupt-controller@1b00 {
compatible = "aspeed,ast2700-intc-ic";
reg = <0 0x12101b00 0 0x10>;
#interrupt-cells = <2>;
interrupt-controller;
interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
};
};
intc1: interrupt-controller@14c18000 {
compatible = "aspeed,ast2700-intc1";
reg = <0 0x14c18000 0 0x400>;
ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
#address-cells = <2>;
#size-cells = <2>;
intc1_0: interrupt-controller@100 {
compatible = "aspeed,ast2700-intc-ic";
reg = <0x0 0x100 0x0 0x10>;
#interrupt-cells = <2>;
interrupt-controller;
interrupts-extended = <&intc0_11 0 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
};
};
};Quoted 240 lines
Quoted 5 lines
Signed-off-by Ryan Chen <[email protected]>--- .../aspeed,ast2700-intc.yaml | 158+++++++++++++-----
Quoted 13 lines
1 file changed, 115 insertions(+), 43 deletions(-)diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270
0-intc.yamlb/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yamlindex55636d06a674..bdc4d8835843100644
---
a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,as +++t2700-intc.yamlReview description: · line 10
Quoted 1 line
(quoted diff) +0
-0@@ -10,6 +10,33 @@ description: This interrupt controller hardware is second level interrupt controllerthat
Quoted 49 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +27
-0is hooked to a parent interrupt controller. It's useful to combine multiple interrupt sources into 1 interrupt to parent interrupt controller. + Depend to which INTC0 or INTC1 used. + INTC0 and INTC1 are two kinds of interrupt controller with enable + and raw status registers for use. + INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 | + | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31 maintainers: - Kevin Chen <[email protected]> @@ -17,49 +44,70 @@maintainers:
properties:
compatible:
enum:Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +5
-3- - aspeed,ast2700-intc-ic + - aspeed,ast2700-intc0 + - aspeed,ast2700-intc1 reg: maxItems: 1 - interrupt-controller: true + '#address-cells': + const: 2 - '#interrupt-cells': + '#size-cells':Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers:
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +0
-9const: 2 - description: - The first cell is the IRQ number, the second cell is the trigger - type as defined in interrupt.txt in this directory. - - interrupts: - maxItems: 6 - description: | - Depend to which INTC0 or INTC1 used. - INTC0 and INTC1 are two kinds of interrupt controller with enableand raw
Quoted 29 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +11
-17- status registers for use. - INTC0 is used to assert GIC if interrupt in INTC1 asserted. - INTC1 is used to assert INTC0 if interrupt of modules asserted. - +-----+ +-------+ +---------+---module0 - | GIC |---| INTC0 |--+--| INTC1_0 |---module2 - | | | | | | |---... - +-----+ +-------+ | +---------+---module31 - | - | +---------+---module0 - +---| INTC1_1 |---module2 - | | |---... - | +---------+---module31 - ... - | +---------+---module0 - +---| INTC1_5 |---module2 - | |---... - +---------+---module31 + ranges: true + +patternProperties: + "^interrupt-controller@": + type: object + description: Interrupt group child nodes + additionalProperties: false + + properties: + compatible: + enum:Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers: · line 79
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ - aspeed,ast2700-intc-ic + + reg: + maxItems: 1 + + interrupt-controller: true + + '#interrupt-cells': + const: 2 + description: |Don't need '|'.
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers: · line 81
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +2
-0+ The first cell is the IRQ number, the second cell is the trigger + type as defined in interrupt.txt in this directory.Don't add links to legacy documents.
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers: · line 89
Quoted 8 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +8
-0+ + interrupts: + minItems: 1 + maxItems: 6 + description: | + The interrupts provided by this interrupt controller. + + interrupts-extended:Why do you have both interrupts and interrupts-extended? They are mutually exclusive and both are auto-magically supported. The schemas only have to document 'interrupts'.
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml maintainers:
Quoted 4 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +4
-0+ minItems: 1 + maxItems: 6 + description: | + This property is required when defining a cascaded interruptcontroller
Quoted 1 line
+ that is connected under another interrupt controller. It specifies
the
Quoted 1 line
+ parent interrupt(s) in the upstream controller to which this
controller
Quoted 22 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +11
-3+ is connected. + + oneOf: + - required: [interrupts] + - required: [interrupts-extended] + + required: + - compatible + - reg + - interrupt-controller + - '#interrupt-cells' required: - compatible - reg - - interrupt-controller - - '#interrupt-cells' - - interrupts additionalProperties: falseDocumentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml examples: · line 122
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +4
-0@@ -68,19 +116,43 @@ examples: #include <dt-bindings/interrupt-controller/arm-gic.h> bus { + #address-cells = <2>; + #size-cells = <2>; + + intc0: interrupt-controller@12100000 {This node isn't an interrupt-controller.
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml examples:
Quoted 12 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +12
-0+ compatible = "aspeed,ast2700-intc0"; + reg = <0 0x12100000 0 0x4000>; + ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>; + #address-cells = <2>; + #size-cells = <2>; + + intc0_11: interrupt-controller@1b00 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0 0x12101b00 0 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_HIGH)>,Review examples: · line 135
Quoted 1 line
(quoted diff) +1
-0+ <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_HIGH)>,Review examples: · line 136
Quoted 1 line
(quoted diff) +1
-0+ <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_HIGH)>,Review examples: · line 137
Quoted 1 line
(quoted diff) +1
-0+ <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_HIGH)>,Review examples: · line 138
Quoted 1 line
(quoted diff) +1
-0+ <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_HIGH)>,Review examples: · line 139
Quoted 1 line
(quoted diff) +1
-0+ <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_HIGH)>;Quoted 17 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +7
-7+ }; + }; + + intc1: interrupt-controller@14c18000 { + compatible = "aspeed,ast2700-intc1"; + reg = <0 0x14c18000 0 0x400>; + ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>; #address-cells = <2>; #size-cells = <2>; - interrupt-controller@12101b00 { - compatible = "aspeed,ast2700-intc-ic"; - reg = <0 0x12101b00 0 0x10>; - #interrupt-cells = <2>; - interrupt-controller; - interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml examples:
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +6
-4- <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>; + intc1_0: interrupt-controller@100 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0x0 0x100 0x0 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc0_11 0(
GIC_CPU_MASK_SIMPLE(4)Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +2
-1+ | IRQ_TYPE_LEVEL_HIGH)>; }; + }; }; --2.34.1
Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
In reply to Ryan ChenQuoted 10 lines · Ryan Chen
The AST2700 SoC contains two independent top-level interrupt controllers (INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parent nodes of INTC0 and INTC1.
- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
I don't understand how this solves your problem at all. Look at old diagram - is it correct? If not, what makes you think that new diagram is correct?
What is the meaning of existing binding and existing intc-ic compatible?
Quoted 10 lines
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC in the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
And how it was not possible before? That's poor argument especially that DT does not have to ever distinguish that.
Best regards,
Krzysztof
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 12 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 8 lines · Ryan Chen
The AST2700 SoC contains two independent top-level interrupt controllers
(INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parent nodes of INTC0 and INTC1.
Quoted 9 lines
Quoted 3 lines
- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
I don't understand how this solves your problem at all. Look at old diagram - is it correct? If not, what makes you think that new diagram is correct?
What is the meaning of existing binding and existing intc-ic compatible?
The new parent nodes (aspeed,ast2700-intc0/intc1) make the device tree layout match the actual hardware separation shown in the SoC datasheet.
This allows us to register the full resource region, allocate platform resources properly, and cleanly extend/debug in the future.
The previous "aspeed,ast2700-intc-ic" compatible only describes the interrupt controller instance, not the full register block. In practice, with only a single child node, there is no way to:
map and manage the entire address space for each INTC block (0x12100000 and 0x14c18000), or cleanly expose debug features that must access routing/protection registers outside the intc-ic range.
The old diagram was incomplete, since it implied that the interrupt controller block had only the intc-ic instance, but in hardware each INTC region contains multiple functions and register ranges.
This binding change is mainly for clarity and correctness, aligning DT and driver with the real SoC register map and future-proofing for debug/maintenance.
Quoted 18 lines
Quoted 11 lines
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC in
the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
And how it was not possible before? That's poor argument especially that DT does not have to ever distinguish that.
Best regards,
Krzysztof
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 39 lines
Subject: RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 11 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed:
Add parent node compatibles and refine documentationQuoted 7 lines · Ryan Chen
The AST2700 SoC contains two independent top-level interrupt controllers
(INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parentnodes of INTC0 and INTC1.
Quoted 7 lines
Quoted 4 lines
- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
I don't understand how this solves your problem at all. Look at old diagram - is it correct? If not, what makes you think that new diagram is
correct?
Quoted 3 lines
What is the meaning of existing binding and existing intc-ic compatible?
The new parent nodes (aspeed,ast2700-intc0/intc1) make the device tree layout match the actual hardware separation shown in the SoC datasheet. This allows us to register the full resource region, allocate platform resources properly, and cleanly extend/debug in the future.
The previous "aspeed,ast2700-intc-ic" compatible only describes the interrupt controller instance, not the full register block. In practice, with only a single child node, there is no way to:
map and manage the entire address space for each INTC block (0x12100000and0x14c18000), or cleanly expose debug features that must access routing/protection registers outside the intc-ic range.
Quoted 21 lines
The old diagram was incomplete, since it implied that the interrupt controller block had only the intc-ic instance, but in hardware each INTC region contains multiple functions and register ranges.
This binding change is mainly for clarity and correctness, aligning DT and driver with the real SoC register map and future-proofing for debug/maintenance.
Quoted 15 lines
Quoted 11 lines
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
in the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
And how it was not possible before? That's poor argument especially that DT does not have to ever distinguish that.
Hi Krzysztof,
I wanted to follow up on my previous explanation about separating parent and child nodes for AST2700 INTC in the device tree.
There is other SoCs, such as Marvell’s CP110 ICU, also use a similar approach to separate parent controller and functional child nodes in the device tree, as shown here:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/marvell%2Ccp110-icu.yaml#L74-L98
Do you need me to provide further details or additional about our SOC design information?
Or is there anything specific you’d like clarified regarding the motivation or the binding structure?
Thanks for your feedback and guidance.
Best regards,
Ryan
Quoted 3 lines
Quoted 3 lines
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 61 lines · Ryan Chen
Quoted 39 lines
Subject: RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 11 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed:
Add parent node compatibles and refine documentationQuoted 7 lines · Ryan Chen
The AST2700 SoC contains two independent top-level interrupt controllers
(INTC0 and INTC1), each responsible for handling different peripheral groups and occupying separate register spaces. Above them, PSP(CA35) GIC controller acts as the root interrupt aggregator. Accurately describing this hierarchical hardware structure in the device tree requires distinct compatible strings for the parentnodes of INTC0 and INTC1.
Quoted 7 lines
Quoted 4 lines
- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible strings for parent interrupt controller nodes. (in addition to the existing 'aspeed,ast2700-intc-ic' for child nodes)
I don't understand how this solves your problem at all. Look at old diagram - is it correct? If not, what makes you think that new diagram is
correct?
Quoted 3 lines
What is the meaning of existing binding and existing intc-ic compatible?
The new parent nodes (aspeed,ast2700-intc0/intc1) make the device tree layout match the actual hardware separation shown in the SoC datasheet. This allows us to register the full resource region, allocate platform resources properly, and cleanly extend/debug in the future.
The previous "aspeed,ast2700-intc-ic" compatible only describes the interrupt controller instance, not the full register block. In practice, with only a single child node, there is no way to:
map and manage the entire address space for each INTC block (0x12100000and0x14c18000), or cleanly expose debug features that must access routing/protection registers outside the intc-ic range.Quoted 21 lines
The old diagram was incomplete, since it implied that the interrupt controller block had only the intc-ic instance, but in hardware each INTC region contains multiple functions and register ranges.
This binding change is mainly for clarity and correctness, aligning DT and driver with the real SoC register map and future-proofing for debug/maintenance.
Quoted 15 lines
Quoted 11 lines
- Clarifies the relationship and function of INTC0 parent (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
in the documentation.
- Updates block diagrams and device tree examples to illustrate the hierarchy and compatible usage.
- Refines documentation and example formatting.This change allows the device tree and driver to distinguish between parent (top-level) and child (group) interrupt controller nodes, enabling more precise driver matching SOC register space allocation.
And how it was not possible before? That's poor argument especially that DT does not have to ever distinguish that.
Quoted 7 lines
Hi Krzysztof,
I wanted to follow up on my previous explanation about separating parent and child nodes for AST2700 INTC in the device tree.
There is other SoCs, such as Marvell’s CP110 ICU, also use a similar approach to separate parent controller and functional child nodes in the device tree, as shown here:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/marvell%2Ccp110-icu.yaml#L74-L98
Do you need me to provide further details or additional about our SOC design information?
Or is there anything specific you’d like clarified regarding the motivation or the binding structure?
Start properly wrapping your email responses. All of them are misformatted, all the time!
You got replies from two DT binding maintainers. Work with that.
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
In reply to Ryan ChenQuoted 14 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +14
-0+ INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 |
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml description: · line 39
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31
You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
This diagram and new binding do not match at all.
Best regards,
Krzysztof
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 29 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 14 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +14
-0+ INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 |Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml description: · line 39
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31
Quoted 3 lines
You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
This diagram and new binding do not match at all.
The corresponded compatible is following.
+-----+ +---------+
| GIC |---| INTC0 | -> (parent : aspeed,ast2700-intc0)
+-----+ +---------+
+---------+
| |---module0
| INTC0_0 |---module1
(child : aspeed,ast2700-intc-ic)
| |---...
+---------+---module31
|---.... |
+---------+
| | +---------+
| INTC0_11 | +---------------------------- | INTC1 | -> -> (parent : aspeed,ast2700-intc1)
(child : aspeed,ast2700-intc-ic)
| | +---------+
+---------+ +-------- -+---module0
| INTC1_0 | --> (child : aspeed,ast2700-intc-ic)
| |---...
+--------- +---module31
...
+--------- +---module0
| INTC1_5 | --> ((child : aspeed,ast2700-intc-ic))
| |---...
+--------- +---module31Quoted 4 lines
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 35 lines · Ryan Chen
Quoted 29 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 14 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +14
-0+ INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 |Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml description: · line 39
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31Quoted 3 lines
You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
This diagram and new binding do not match at all.
The corresponded compatible is following.
Quoted 13 lines
+-----+ +---------+
| GIC |---| INTC0 | -> (parent : aspeed,ast2700-intc0)
+-----+ +---------+
+---------+
| |---module0
| INTC0_0 |---module1
(child : aspeed,ast2700-intc-ic)
| |---...
+---------+---module31
|---.... |
+---------+
| | +---------+
| INTC0_11 | +---------------------------- | INTC1 | -> -> (parent : aspeed,ast2700-intc1)AGAIN (second time): that's not what your binding said.
Your binding is explicit here, which is what we want in general. It says that inct1 is one of the parents of intc-ic.
Let me be clear, because you will be dragging this talk with irrelevant arguments forever - changing this binding is close to no. If you come with correct arguments, maybe would work. But the main point is that you probably do not have to even change the binding to achieve proper hardware description. Work on that.
Best regards,
Krzysztof
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 56 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 35 lines · Ryan Chen
Quoted 29 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
aspeed: Add parent node compatibles and refine documentationQuoted 14 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +14
-0+ INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 |Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml description: · line 39
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31Quoted 3 lines
You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
This diagram and new binding do not match at all.
The corresponded compatible is following.
Quoted 13 lines
+-----+ +---------+ | GIC |---| INTC0 | -> (parent : aspeed,ast2700-intc0) +-----+ +---------+ +---------+ | |---module0 | INTC0_0 |---module1 (child : aspeed,ast2700-intc-ic) | |---... +---------+---module31 |---.... | +---------+ | | +---------+ | INTC0_11 | +---------------------------- | INTC1 | -> ->(parent : aspeed,ast2700-intc1)
AGAIN (second time): that's not what your binding said.
Quoted 8 lines
Your binding is explicit here, which is what we want in general. It says that inct1 is one of the parents of intc-ic.
Let me be clear, because you will be dragging this talk with irrelevant arguments forever - changing this binding is close to no. If you come with correct arguments, maybe would work. But the main point is that you probably do not have to even change the binding to achieve proper hardware description. Work on that.
If I do not change the binding, I think the yaml and dts can still fit the interrupt nesting architecture by using both interrupts and interrupts-extended.
For first-level controllers, use the standard interrupts property (e.g. with the GIC as the parent).
For second-level INTC-IC instances, use interrupts-extended to refer to the first-level INTC-IC, following common Linux practice for stacked interrupt controllers. For example:
dts
// First level
intc0_11: interrupt-controller@12101b00 {
compatible = "aspeed,ast2700-intc-ic";
reg = <...>;
interrupt-controller;
#interrupt-cells = <2>;
interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, ...;
};// Second level, cascaded
intc1_0: interrupt-controller@14c18100 {
compatible = "aspeed,ast2700-intc-ic";
reg = <...>;
interrupt-controller;
#interrupt-cells = <2>;
interrupts-extended = <&intc0_11 0 IRQ_TYPE_LEVEL_HIGH>;
};In yaml, I can use:
oneOf:
- required: [interrupts]
- required: [interrupts-extended]
This allows both cases to be valid.
Please let me know if this is the recommended approach, or if further changes are needed.
Quoted 2 lines
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 52 lines · Ryan Chen
Quoted 52 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 48 lines · Ryan Chen
Quoted 29 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
aspeed: Add parent node compatibles and refine documentationQuoted 14 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +14
-0+ INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 |Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml description: · line 39
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31Quoted 3 lines
You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
This diagram and new binding do not match at all.
The corresponded compatible is following.
+-----+ +---------+ | GIC |---| INTC0 | -> (parent : aspeed,ast2700-intc0) +-----+ +---------+ +---------+ | |---module0 | INTC0_0 |---module1 (child : aspeed,ast2700-intc-ic) | |---... +---------+---module31 |---.... | +---------+ | | +---------+ | INTC0_11 | +---------------------------- | INTC1 | -> ->
Quoted 6 lines
Quoted 6 lines
(parent : aspeed,ast2700-intc1)
AGAIN (second time): that's not what your binding said.
Your binding is explicit here, which is what we want in general. It says that inct1 is one of the parents of intc-ic.
... and you never addressed that. :/
Quoted 26 lines
Quoted 6 lines
Let me be clear, because you will be dragging this talk with irrelevant arguments forever - changing this binding is close to no. If you come with correct arguments, maybe would work. But the main point is that you probably do not have to even change the binding to achieve proper hardware description. Work on that.
If I do not change the binding, I think the yaml and dts can still fit the interrupt nesting architecture by using both interrupts and interrupts-extended.
For first-level controllers, use the standard interrupts property (e.g. with the GIC as the parent).
For second-level INTC-IC instances, use interrupts-extended to refer to the first-level INTC-IC, following common Linux practice for stacked interrupt controllers. For example:
dts
// First levelintc0_11: interrupt-controller@12101b00 { compatible = "aspeed,ast2700-intc-ic"; reg = <...>; interrupt-controller; #interrupt-cells = <2>; interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, ...; };
Quoted 7 lines
// Second level, cascaded
intc1_0: interrupt-controller@14c18100 { compatible = "aspeed,ast2700-intc-ic"; reg = <...>; interrupt-controller; #interrupt-cells = <2>; interrupts-extended = <&intc0_11 0 IRQ_TYPE_LEVEL_HIGH>;
This looks like changing the meaning of the interrupt. What was the interrupt here before? What interrupt is here now?
Quoted 6 lines
};
In yaml, I can use:
oneOf:
- required: [interrupts]
- required: [interrupts-extended]
This allows both cases to be valid.
Hm? Since when you need both cases?
Best regards,
Krzysztof
Ryan Chen
reviewerRE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 56 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation
Quoted 52 lines · Ryan Chen
Quoted 52 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
aspeed: Add parent node compatibles and refine documentationQuoted 48 lines · Ryan Chen
Quoted 29 lines
Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
aspeed: Add parent node compatibles and refine documentationQuoted 14 lines · Ryan Chen
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +14
-0+ INTC0 is used to assert GIC if interrupt in INTC1 asserted. + INTC1 is used to assert INTC0 if interrupt of modules asserted. + +-----+ +---------+ + | GIC |---| INTC0 | + +-----+ +---------+ + +---------+ + | |---module0 + | INTC0_0 |---module1 + | |---... + +---------+---module31 + |---.... | + +---------+ + | | +---------+ + | INTC0_11| +---| INTC1 |Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml description: · line 39
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +10
-0+ | | +---------+ + +---------+ +---------+---module0 + | INTC1_0 |---module1 + | |---... + +---------+---module31 + ... + +---------+---module0 + | INTC1_5 |---module1 + | |---... + +---------+---module31Quoted 3 lines
You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
This diagram and new binding do not match at all.
The corresponded compatible is following.
+-----+ +---------+ | GIC |---| INTC0 | -> (parent : aspeed,ast2700-intc0) +-----+ +---------+ +---------+ | |---module0 | INTC0_0 |---module1 (child : aspeed,ast2700-intc-ic) | |---... +---------+---module31 |---.... | +---------+ | | +---------+ | INTC0_11 | +---------------------------- | INTC1 | -> ->
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
(parent : aspeed,ast2700-intc1)
AGAIN (second time): that's not what your binding said.
Your binding is explicit here, which is what we want in general. It says that inct1 is one of the parents of intc-ic.
... and you never addressed that. :/
The following is datasheet description.
AST2700 Interrupt Controller Hierarchy (from datasheet):
INTC0 and INTC1 are AMBA slave devices on the AHB bus,
each with their own register space. 480 interrupt sources: INTn (n=0~479) INT0~127 can be routed directly to PSP, SSP, or TSP. INT128~319 are handled by INTC1, which have multiple instances (INTC1_0, INTC1_1, ...)
INTC1 outputs are routed into INTC0; INTC0 outputs go to the GIC.
This structure means:
INTC0 receives INT0~127 and also all outputs from INTC1. INTC1 handles a subset of interrupt sources, and its output is routed as an input to INTC0.
Block Diagram / Interrupt Chain:
GIC
|
v
INTC0 (parent, aspeed,ast2700-intc0)
|
+-- INTC0_0 (aspeed,ast2700-intc-ic) --> [module0, module1, ...]
|
+-- ...
|
+-- INTC0_11 (aspeed,ast2700-intc-ic)
|
v
INTC1 (parent, aspeed,ast2700-intc1)
|
+-- INTC1_0 (aspeed,ast2700-intc-ic) --> [moduleA, moduleB, ...]
|
+-- ...
|
+-- INTC1_5 (aspeed,ast2700-intc-ic) --> [moduleY, moduleZ, ...]| Device Tree Node | Hardware Block | Output Routed To | | ------- | --------------- | ---------------------- | | intc0 | INTC0 @12100000 | GIC | | intc1 | INTC1 @14c18000 | INTC0 input (cascaded)|
intc0 uses interrupts to connect to the GIC (top-level parent) intc1 uses interrupts-extended to connect to an input on INTC0 (second-level, cascaded)
This approach ensures the software and device tree reflect the actual hardware interrupt paths.
It allows the kernel to correctly map register space and handle interrupt delivery, and makes future debug/maintenance straightforward.
If there are any details you'd like clarified or if you recommend a different device tree structure, please let me know!
Quoted 37 lines
Quoted 6 lines
Quoted 6 lines
Let me be clear, because you will be dragging this talk with irrelevant arguments forever - changing this binding is close to no. If you come with correct arguments, maybe would work. But the main point is that you probably do not have to even change the binding to achieve
proper hardware description. Work on that.
Quoted 4 lines
Quoted 1 line
If I do not change the binding, I think the yaml and dts can still fit the interrupt nesting architecture by using both interrupts and
interrupts-extended.
Quoted 6 lines
For first-level controllers, use the standard interrupts property (e.g. with the GIC as the parent).
For second-level INTC-IC instances, use interrupts-extended to refer to the first-level INTC-IC, following common Linux practice for stacked interrupt
controllers.
Quoted 10 lines
For example:
dts
// First levelintc0_11: interrupt-controller@12101b00 { compatible = "aspeed,ast2700-intc-ic"; reg = <...>; interrupt-controller; #interrupt-cells = <2>; interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, ...; };Quoted 7 lines
// Second level, cascaded
intc1_0: interrupt-controller@14c18100 { compatible = "aspeed,ast2700-intc-ic"; reg = <...>; interrupt-controller; #interrupt-cells = <2>; interrupts-extended = <&intc0_11 0 IRQ_TYPE_LEVEL_HIGH>;
Quoted 3 lines
This looks like changing the meaning of the interrupt. What was the interrupt here before? What interrupt is here now?
The change from interrupts to interrupts-extended does not change the source or meaning of the interrupt itself.
For first-level INTC-IC nodes, the parent is the GIC, so we use interrupts.
For second-level (cascaded) INTC-IC nodes, the parent is an input on INTC0, so interrupts-extended is required to correctly reflect the hardware chain as described in the datasheet.
This ensures the DT matches the hardware hierarchy—the actual interrupt source and routing path are not changed, only described more precisely.
Quoted 9 lines
Quoted 6 lines
};
In yaml, I can use:
oneOf:
- required: [interrupts]
- required: [interrupts-extended]
This allows both cases to be valid.Hm? Since when you need both cases?
The oneOf schema allows the binding to support both scenarios, matching the hardware and software requirements.
first-level INTC-IC nodes required [interrupts] second-level (cascaded) required [interrupts-extended]
Quoted 4 lines
Best regards,
Krzysztof
Ryan Chen
reviewerPATCH v3 2/2 irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
In reply to Ryan ChenAST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only include the interrupt controller child node ("aspeed,ast2700-intc-ic"), but also provide interrupt routing and register protection features. This patch adds debugfs entries for interrupt routing and protection status for AST2700 INTC0/INTC1.
- Register platform driver for "aspeed,ast2700-intc0" and "aspeed,ast2700-intc1" compatible nodes.
- Add show_routing/show_prot callbacks for both intc0 and intc1, displaying current interrupt routing and protection register status.
- Expose routing/protection information via debugfs for debugging and validation.
--- drivers/irqchip/irq-aspeed-intc.c | 238 ++++++++++++++++++++++++++++++ 1 file changed, 238 insertions(+)
drivers/irqchip/irq-aspeed-intc.c +238 -0
@@ -6,6 +6,7 @@*/ #include <linux/bitops.h> +#include <linux/debugfs.h> #include <linux/irq.h> #include <linux/irqchip.h> #include <linux/irqchip/chained_irq.h>
@@ -15,6 +16,13 @@#include <linux/io.h> #include <linux/spinlock.h> +#define INTC0_ROUTING0_SEL0 0x200 +#define INTC0_ROUTING0_SEL1 0x300 +#define INTC0_ROUTING0_SEL2 0x400 +#define INTC1_ROUTING0_SEL0 0x80 +#define INTC1_ROUTING0_SEL1 0xa0 +#define INTC1_ROUTING0_SEL2 0xc0 +#define INTC_INT_ENABLE_REG 0x00#define INTC_INT_STATUS_REG 0x04#define INTC_IRQS_PER_WORD 32
@@ -137,3 +145,233 @@ static int __init aspeed_intc_ic_of_init(struct device_node *node, }IRQCHIP_DECLARE (ast2700_intc_ic,"aspeed,ast2700-intc-ic" , aspeed_intc_ic_of_init); + +struct aspeed_intc { +void __iomem *base; +struct device *dev; +struct dentry *dbg_root; +int (*show_routing)(struct seq_file *s,void *unused); +int (*show_prot)(struct seq_file *s,void *unused); +}; + +/* +* 000: Route interrupt INTn to PSP GICINT0-31 +* 001: Route interrupt INTn to SSPINT0-31 +* 010: Route interrupt INTn to TSPINT0-31 +*/ +static int aspeed_intc0_show_routing (struct seq_file *s,void *unused) +{ +struct aspeed_intc *intc = s->private; +int group, bit; + +seq_puts (s,"int | PSP | SSP | TSP\n" ); +seq_puts (s,"----+-----+-----+----\n" ); + +for (group =0 ; group <4 ; group++) { +u32 reg0 =readl (intc->base +INTC0_ROUTING0_SEL0 + group *4 ); +u32 reg1 =readl (intc->base +INTC0_ROUTING0_SEL1 + group *4 ); +u32 reg2 =readl (intc->base +INTC0_ROUTING0_SEL2 + group *4 ); + +for (bit =0 ; bit <32 ; bit++) { +int idx = group *32 + bit; +u8 routing = (((reg2 >> bit) &0x1 ) <<2 ) | + (((reg1 >> bit) &0x1 ) <<1 ) | + (((reg0 >> bit) &0x1 ) <<0 ); + +const char *ca35 = (routing ==0 ) ?" O " :" - " ; +const char *ssp = (routing ==1 ) ?" O " :" - " ; +const char *tsp = (routing ==2 ) ?" O " :" - " ; + +seq_printf (s,"%-4d| %s | %s | %s\n" , idx, ca35, ssp, tsp); + } + } +return 0 ; +} + +static int aspeed_intc0_show_prot (struct seq_file *s,void *unused) +{ +struct aspeed_intc *intc = s->private; +u32 prot =readl (intc->base +0x40 ); + +seq_printf (s,"INTC040 : 0x%08x\n" , prot); + +static const char *const prot_bits[] = { +"hprot_ca35: Protect INTC010~018,1xxx accessed by PSP only" , +"hprot_ssp: Protect INTC020~028,2xxx accessed by SSP only" , +"hprot_tsp: Protect INTC030~038,3xxx accessed by TSP only" , +"hprot_sirqs: Protect INTC0C0~0D4 to be read only" , +"hprot_sirqs_1700: Protect INTC0D8~0DC to be read only" , +"hprot_sirqs_ext: Protect INTC0E0 to be read only" , +"hprot_reg_prot: Protect INTC044,2xx~3xx to be read only" , +"hprot_rd1_prot: Read protect for INTC044,200-438" , +"hprot_rd2_prot: Read protect for INTC0C0~164" , +"hprot_rd3_prot: Read protect for INTC02x,1xxx to be read by PSP only" , +"hprot_rd4_prot: Read protect for INTC03x,2xxx to be read by SSP only" , +"hprot_rd5_prot: Read protect for INTC04x,3xxx to be read by TSP only" , +"hprot_mcu0: Protect INTC050~054,028 accessed by MCU0 only" , +"hprot_ca35p: Protect INTC010~018 accessed by PSP secure only" + }; + +for (int i =0 ; i <14 ; i++) +seq_printf (s," [%2d] %s: %s\n" , i, prot_bits[i], + (prot &BIT (i)) ?"Enable" :"Disable" ); +return 0 ; +} + +/* +* 000: Route interrupt INTi to PSP(default) +* 001: Route interrupt INTi to INTC controller +* 010: Route interrupt INTi to SSP +* 011: Route interrupt INTi to TSP +* 100: Route interrupt INTi to PSP S1 +* 101: Route interrupt INTi to PSP S2 +* 110: Route interrupt INTi to MCU0 +*/ +static int aspeed_intc1_show_routing (struct seq_file *s,void *unused) +{ +struct aspeed_intc *intc = s->private; +int group, bit; + +seq_puts (s,"index | PSP | INTC| SSP | TSP | S1 | S2 | MCU0\n" ); +seq_puts (s,"-----------+-----+-----+-----+-----+-----+-----+-----\n" ); + +for (group =0 ; group <6 ; group++) { +u32 reg0 =readl (intc->base +INTC1_ROUTING0_SEL0 + group *4 ); +u32 reg1 =readl (intc->base +INTC1_ROUTING0_SEL1 + group *4 ); +u32 reg2 =readl (intc->base +INTC1_ROUTING0_SEL2 + group *4 ); + +for (bit =0 ; bit <32 ; bit++) { +u8 routing = (((reg2 >> bit) &0x1 ) <<2 ) | + (((reg1 >> bit) &0x1 ) <<1 ) | + (((reg0 >> bit) &0x1 ) <<0 ); + +const char *psp = (routing ==0 ) ?" O " :" - " ; +const char *intc = (routing ==1 ) ?" O " :" - " ; +const char *ssp = (routing ==2 ) ?" O " :" - " ; +const char *tsp = (routing ==3 ) ?" O " :" - " ; +const char *s1 = (routing ==4 ) ?" O " :" - " ; +const char *s2 = (routing ==5 ) ?" O " :" - " ; +const char *mcu0 = (routing ==6 ) ?" O " :" - " ; + +seq_printf (s,"intc1_%d_%02d | %s | %s | %s | %s | %s | %s | %s\n" , + group, bit, psp, intc, ssp, tsp, s1, s2, mcu0); + } + } +return 0 ; +} + +static int aspeed_intc1_show_prot (struct seq_file *s,void *unused) +{ +struct aspeed_intc *intc = s->private; +u32 prot =readl (intc->base); + +seq_printf (s,"INTC1: 0x%08x\n" , prot); + +static const char *const prot_bits[] = { +"pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSP only" , +"pprot_ssp: Protect INTC180~1D0 write by SSP only" , +"pprot_tsp: Protect INTC200~250 write by TSP only" , +"pprot_reg_prot: Protect INTC080~0D4 to be read only" , +"pprot_regrd: Protect INTC080~0D4 to be read protected" , +"pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSP only" , +"pprot_regrd3: Protect INTC180~1D0 read by SSP only" , +"pprot_regrd4: Protect INTC200~250 read by TSP only" , +"pprot_mcu0: Protect INTC010,014 write by MCU0 only" , +"pprot_regrd5: Protect INTC010,014 read by MCU0 only" , +"pprot_treg: Protect INTC040~054 to be read protected" + }; + +for (int i =0 ; i <11 ; i++) +seq_printf (s," [%2d] %s: %s\n" , i, prot_bits[i], + (prot &BIT (i)) ?"Enable" :"Disable" ); +return 0 ; +} + +static int aspeed_intc_open_routing (struct inode *inode,struct file *file) +{ +struct aspeed_intc *intc = inode->i_private; + +if (!intc->show_routing) +return -ENODEV ; +return single_open (file, intc->show_routing, intc); +} + +static int aspeed_intc_open_prot (struct inode *inode,struct file *file) +{ +struct aspeed_intc *intc = inode->i_private; + +if (!intc->show_prot) +return -ENODEV ; +return single_open (file, intc->show_prot, intc); +} + +static const struct file_operations aspeed_intc_routing_fops = { + .owner =THIS_MODULE , + .open = aspeed_intc_open_routing, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static const struct file_operations aspeed_intc_prot_fops = { + .owner =THIS_MODULE , + .open = aspeed_intc_open_prot, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static int aspeed_intc_probe (struct platform_device *pdev) +{ +struct aspeed_intc *intc; +struct resource *res; + + intc =devm_kzalloc (&pdev->dev,sizeof (*intc),GFP_KERNEL ); +if (!intc) +return -ENOMEM ; + intc->dev = &pdev->dev; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + intc->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (intc->base)) +return PTR_ERR (intc->base); + +if (of_device_is_compatible (pdev->dev.of_node,"aspeed,ast2700-intc0" )) { + intc->show_routing = aspeed_intc0_show_routing; + intc->show_prot = aspeed_intc0_show_prot; + }else if (of_device_is_compatible (pdev->dev.of_node,"aspeed,ast2700-intc1" )) { + intc->show_routing = aspeed_intc1_show_routing; + intc->show_prot = aspeed_intc1_show_prot; + }else { + intc->show_routing =NULL ; + intc->show_prot =NULL ; + } + +platform_set_drvdata (pdev, intc); + + intc->dbg_root =debugfs_create_dir (dev_name (&pdev->dev),NULL ); +if (intc->dbg_root) { +debugfs_create_file ("routing" ,0400 , intc->dbg_root, intc, + &aspeed_intc_routing_fops); +debugfs_create_file ("protection" ,0400 , intc->dbg_root, intc, + &aspeed_intc_prot_fops); + } + +return 0 ; +} + +static const struct of_device_id aspeed_intc_of_match[] = { + { .compatible ="aspeed,ast2700-intc0" , }, + { .compatible ="aspeed,ast2700-intc1" , }, + {}, +}; + +static struct platform_driver aspeed_intc_driver = { + .probe = aspeed_intc_probe, + .driver = { + .name ="ast2700-intc" , + .of_match_table = aspeed_intc_of_match, + }, +}; +builtin_platform_driver (aspeed_intc_driver); +
Signature · 1 line
2.34.1
Re: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:
The subsystem prefix is made up. See:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes
Quoted 5 lines
AST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only include the interrupt controller child node ("aspeed,ast2700-intc-ic"), but also provide interrupt routing and register protection features.
This patch adds debugfs entries for interrupt routing and protection
# git grep 'This patch' Documentation/process
Quoted 8 lines
status for AST2700 INTC0/INTC1.
- Register platform driver for "aspeed,ast2700-intc0" and "aspeed,ast2700-intc1" compatible nodes.
- Add show_routing/show_prot callbacks for both intc0 and intc1, displaying current interrupt routing and protection register status.
- Expose routing/protection information via debugfs for debugging and validation.
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 155
Quoted 8 lines
drivers/irqchip/irq-aspeed-intc.c +8
-0+ +struct aspeed_intc { +void __iomem *base; +struct device *dev; +struct dentry *dbg_root; +int (*show_routing)(struct seq_file *s,void *unused); +int (*show_prot)(struct seq_file *s,void *unused); +};
See the chapter about struct declarations and initializers in the documentation I linked to above.
Quoted 16 lines
drivers/irqchip/irq-aspeed-intc.c +16
-0+static int aspeed_intc1_show_prot (struct seq_file *s,void *unused) +{ +struct aspeed_intc *intc = s->private; +u32 prot =readl (intc->base); + +seq_printf (s,"INTC1: 0x%08x\n" , prot); + +static const char *const prot_bits[] = { +"pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSP only" , +"pprot_ssp: Protect INTC180~1D0 write by SSP only" , +"pprot_tsp: Protect INTC200~250 write by TSP only" , +"pprot_reg_prot: Protect INTC080~0D4 to be read only" , +"pprot_regrd: Protect INTC080~0D4 to be read protected" , +"pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSP only" , +"pprot_regrd3: Protect INTC180~1D0 read by SSP only" , +"pprot_regrd4: Protect INTC200~250 read by TSP only" ,
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 288
Quoted 10 lines
drivers/irqchip/irq-aspeed-intc.c +10
-0+"pprot_mcu0: Protect INTC010,014 write by MCU0 only" , +"pprot_regrd5: Protect INTC010,014 read by MCU0 only" , + "pprot_treg: Protect INTC040~054 to be read protected" + }; + +for (int i =0 ; i <11 ; i++) +seq_printf (s," [%2d] %s: %s\n" , i, prot_bits[i], + (prot &BIT (i)) ?"Enable" :"Disable" ); +return 0 ; +}
I really have to ask, what the value of this metric ton of string constants and decoding is. This is a debug interface, which is intended for developers and experts. As these are hardware bits, which are immutable, it's completely sufficient to print out the raw values here and let the engineer decode it, no?
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 332
Quoted 9 lines
drivers/irqchip/irq-aspeed-intc.c +9
-0+static int aspeed_intc_probe (struct platform_device *pdev) +{ +struct aspeed_intc *intc; +struct resource *res; + + intc =devm_kzalloc (&pdev->dev,sizeof (*intc),GFP_KERNEL ); +if (!intc) +return -ENOMEM ; + intc->dev = &pdev->dev;
intc->dev is not used anywhere.
Quoted 4 lines
drivers/irqchip/irq-aspeed-intc.c +4
-0+ res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + intc->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (intc->base)) +return PTR_ERR (intc->base);
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 347
Quoted 10 lines
drivers/irqchip/irq-aspeed-intc.c +10
-0+ +if (of_device_is_compatible (pdev->dev.of_node,"aspeed,ast2700-intc0" )) { + intc->show_routing = aspeed_intc0_show_routing; + intc->show_prot = aspeed_intc0_show_prot; + }else if (of_device_is_compatible (pdev->dev.of_node,"aspeed,ast2700-intc1" )) { + intc->show_routing = aspeed_intc1_show_routing; + intc->show_prot = aspeed_intc1_show_prot; + }else { + intc->show_routing =NULL ; + intc->show_prot =NULL ;
What's the point of creating the debugfs entry instead of bailing out?
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 352
Quoted 5 lines
drivers/irqchip/irq-aspeed-intc.c +5
-0+ } + +platform_set_drvdata (pdev, intc); + + intc->dbg_root =debugfs_create_dir (dev_name (&pdev->dev),NULL );
Why storing this? It's just used for setting up the debugfs entry, no?
Quoted 14 lines
drivers/irqchip/irq-aspeed-intc.c +14
-0+if (intc->dbg_root) { +debugfs_create_file ("routing" ,0400 , intc->dbg_root, intc, + &aspeed_intc_routing_fops); +debugfs_create_file ("protection" ,0400 , intc->dbg_root, intc, + &aspeed_intc_prot_fops); + } + +return 0 ; +} + +static const struct of_device_id aspeed_intc_of_match[] = { + { .compatible ="aspeed,ast2700-intc0" , }, + { .compatible ="aspeed,ast2700-intc1" , }, + {},
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 376
Quoted 10 lines
drivers/irqchip/irq-aspeed-intc.c +10
-0+}; + +static struct platform_driver aspeed_intc_driver = { + .probe = aspeed_intc_probe, + .driver = { + .name ="ast2700-intc" , + .of_match_table = aspeed_intc_of_match, + }, +}; +builtin_platform_driver (aspeed_intc_driver);
Why has this to be builtin and not a module? It has zero dependencies on the existing code in this file, right?
Just stick it into a seperate source file and make it modular with a seperate Kconfig switch. No point in carrying this code as built-in in multi-platform builds.
This whole platform driver muck is just there to expose the routing and protection registers in debugfs even if debugfs is disabled. Seriously?
It's completely disconnected from the interrupt delivery chain as far as the kernel is concerned, i.e. it does not provide a interrupt domain/chip. So that interface dumps just some register values with a lot of effort and leaves it to the user to decode which actual linux interrupt in the real intc-ic interrupt domains is affected, right?
I'm still failing to see the value of all of this. As the kernel does not even modify these registers, you are basically implementing a dump facility for decoding what the firmware put into those registers, right?
I don't have a strong opinion about it, but if it has a value, then all of this can be done with way smaller code by just dumping the raw register values all in one go. No need for two files and string encoding. That's what user space is for.
Something like the completely uncompiled below, which I cobbled together quickly for illustration. You get the idea.
Thanks,
tglx
---
#define INTC_TYPE_C0 0
#define INTC_TYPE_C1 1
struct aspeed_intc {
void __iomem *base;
unsigned int type;
};const struct aspeed_intc_data {
char *name;
unsigned int groups;
unsigned int prot_offs;
unsigned int rout_offs;
unsigned int rout_gap;
} aspeed_intc_data[2] = {
{
.name = "INTC0",
.groups = 4,
.prot_offs = 0x40,
.rout_offs = 0x200,
.rout_gap = 0x100,
},
{
.name = "INTC1",
.groups = 6,
.prot_offs = 0x0,
.rout_offs = 0x80,
.rout_gap = 0x20,
},
};static int aspeed_intc_show(struct seq_file *m, void *unused)
{
struct aspeed_intc *intc = m->private;
const struct aspeed_intc_data *d = &aspeed_intc_data[intc->type];
void __iomem *base = intc->base;
seq_printf(m, "%s\n", d->name)
seq_printf(m, "P: 0x%08x\n", readl(base + d->prot_offs));
base += d->rout_offs;
for (unsigned int i = 0; i < d->groups; i++, base += 4) {
seq_printf(m, "R%d: 0x%08x 0x%08x 0x%08x\n", i, readl(base),
readl(base + d->rout_gap), readl(base + 2 * d->rout_gap));
}
return 0;
}DEFINE_SHOW_ATTRIBUTE(aspeed_intc);
static int aspeed_intc_probe(struct platform_device *pdev)
{
struct aspeed_intc *intc;
struct resource *res;
struct dentry *dir;
intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
if (!intc)
return -ENOMEM;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
intc->base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(intc->base))
return PTR_ERR(intc->base);
if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0"))
intc->type = INTC_TYPE_C0;
else if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc1"))
intc->type = INTC_TYPE_C1;
else
return -ENOTSUPP;
platform_set_drvdata(pdev, intc);
dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
debugfs_create_file("intc_regs", 0400, dir, intc, &aspeed_intc_fops);
return 0;
}static const struct of_device_id aspeed_intc_of_match[] = {
{ .compatible = "aspeed,ast2700-intc0", },
{ .compatible = "aspeed,ast2700-intc1", },
{ },
};MODULE_DEVICE_TABLE(of, aspeed_intc_of_match);
static struct platform_driver aspeed_intc_driver = {
.probe = aspeed_intc_probe,
.driver = {
.name = "ast2700-intc",
.of_match_table = aspeed_intc_of_match,
},
};module_platform_driver(aspeed_intc_driver);
Ryan Chen
reviewerRE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
Quoted 9 lines
Subject: Re: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:
The subsystem prefix is made up. See:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-su
bmission-notes
Will prefix update to
Irqchip/aspeed-intc: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
Quoted 8 lines
Quoted 6 lines
AST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only include the interrupt controller child node ("aspeed,ast2700-intc-ic"), but also provide interrupt routing and register protection features.
This patch adds debugfs entries for interrupt routing and protection
# git grep 'This patch' Documentation/process
Modify to
Adds debugfs entries for interrupt routing and protection status for AST2700 INTC0/INTC1.
Quoted 19 lines
Quoted 9 lines
status for AST2700 INTC0/INTC1.
- Register platform driver for "aspeed,ast2700-intc0" and "aspeed,ast2700-intc1" compatible nodes.
- Add show_routing/show_prot callbacks for both intc0 and intc1, displaying current interrupt routing and protection register status.
- Expose routing/protection information via debugfs for debugging and validation.drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 7 lines
drivers/irqchip/irq-aspeed-intc.c +7
-0+ +struct aspeed_intc { +void __iomem *base; +struct device *dev; +struct dentry *dbg_root; +int (*show_routing)(struct seq_file *s,void *unused); +int (*show_prot)(struct seq_file *s,void *unused); };See the chapter about struct declarations and initializers in the documentation I linked to above.
Sorry, I don't see the struct "> > + int (*show_prot)(struct seq_file *s, void *unused); };"
My original submit is following, it should ok. Am I right?
+struct aspeed_intc {
+ void __iomem *base;
+ struct device *dev;
+ struct dentry *dbg_root;
+ int (*show_routing)(struct seq_file *s, void *unused);
+ int (*show_prot)(struct seq_file *s, void *unused);
+};
https://www.spinics.net/lists/kernel/msg5776957.html
Quoted 25 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 9 lines
drivers/irqchip/irq-aspeed-intc.c +8
-0+static int aspeed_intc1_show_prot(struct seq_file *s, void *unused) { + struct aspeed_intc *intc = s->private; + u32 prot = readl(intc->base); + + seq_printf(s, "INTC1: 0x%08x\n", prot); + + static const char * const prot_bits[] = { + "pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSPonly",
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 5 lines
drivers/irqchip/irq-aspeed-intc.c +5
-0+"pprot_ssp: Protect INTC180~1D0 write by SSP only" , +"pprot_tsp: Protect INTC200~250 write by TSP only" , +"pprot_reg_prot: Protect INTC080~0D4 to be read only" , +"pprot_regrd: Protect INTC080~0D4 to be read protected" , + "pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSPonly",
Quoted 9 lines
drivers/irqchip/irq-aspeed-intc.c +9
-0+"pprot_regrd3: Protect INTC180~1D0 read by SSP only" , +"pprot_regrd4: Protect INTC200~250 read by TSP only" , +"pprot_mcu0: Protect INTC010,014 write by MCU0 only" , +"pprot_regrd5: Protect INTC010,014 read by MCU0 only" , +"pprot_treg: Protect INTC040~054 to be read protected" + }; + +for (int i =0 ; i <11 ; i++) + seq_printf(s, " [%2d] %s: %s\n", i, prot_bits[i],
Quoted 8 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 219
Quoted 3 lines
drivers/irqchip/irq-aspeed-intc.c +3
-0+ (prot & BIT(i)) ? "Enable" : "Disable"); +return 0 ; +}I really have to ask, what the value of this metric ton of string constants and decoding is. This is a debug interface, which is intended for developers and experts. As these are hardware bits, which are immutable, it's completely sufficient to print out the raw values here and let the engineer decode it, no?
The reason for the string decoding was to make debug output more friendly for quick diagnosis during bring-up, If it is not accepted, I will revise the patch to remove the extra string decoding and only output the raw values.
Quoted 11 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 332
Quoted 9 lines
drivers/irqchip/irq-aspeed-intc.c +8
-0+static int aspeed_intc_probe (struct platform_device *pdev) { +struct aspeed_intc *intc; +struct resource *res; + + intc =devm_kzalloc (&pdev->dev,sizeof (*intc),GFP_KERNEL ); +if (!intc) +return -ENOMEM ; + intc->dev = &pdev->dev;
intc->devis not used anywhere.
Will remove it.
Quoted 12 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 7 lines
drivers/irqchip/irq-aspeed-intc.c +6
-0+ res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + intc->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (intc->base)) +return PTR_ERR (intc->base); + +if (of_device_is_compatible (pdev->dev.of_node,"aspeed,ast2700-intc0" )){
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 3 lines
drivers/irqchip/irq-aspeed-intc.c +3
-0+ intc->show_routing = aspeed_intc0_show_routing; + intc->show_prot = aspeed_intc0_show_prot; + } else if (of_device_is_compatible(pdev->dev.of_node,"aspeed,ast2700-intc1")) {
Quoted 7 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 347
Quoted 5 lines
drivers/irqchip/irq-aspeed-intc.c +5
-0+ intc->show_routing = aspeed_intc1_show_routing; + intc->show_prot = aspeed_intc1_show_prot; + }else { + intc->show_routing =NULL ; + intc->show_prot =NULL ;What's the point of creating the debugfs entry instead of bailing out?
Yes, will update to
return -ENODEV;
Quoted 8 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 352
Quoted 6 lines
drivers/irqchip/irq-aspeed-intc.c +5
-0+ } + +platform_set_drvdata (pdev, intc); + + intc->dbg_root =debugfs_create_dir (dev_name (&pdev->dev),NULL );Why storing this? It's just used for setting up the debugfs entry, no?
I'll update the code to use a local variable, by following.
struct dentry *dbg_root;dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);
if (dbg_root) {
debugfs_create_file("routing", 0400, dbg_root, intc, &aspeed_intc_routing_fops);
debugfs_create_file("protection", 0400, dbg_root, intc, &aspeed_intc_prot_fops);
}Quoted 32 lines
Quoted 15 lines
drivers/irqchip/irq-aspeed-intc.c +14
-0+if (intc->dbg_root) { +debugfs_create_file ("routing" ,0400 , intc->dbg_root, intc, + &aspeed_intc_routing_fops); +debugfs_create_file ("protection" ,0400 , intc->dbg_root, intc, + &aspeed_intc_prot_fops); + } + +return 0 ; +} + +static const struct of_device_id aspeed_intc_of_match[] = { + { .compatible ="aspeed,ast2700-intc0" , }, + { .compatible ="aspeed,ast2700-intc1" , }, + {},drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node, · line 376
Quoted 10 lines
drivers/irqchip/irq-aspeed-intc.c +10
-0+}; + +static struct platform_driver aspeed_intc_driver = { + .probe = aspeed_intc_probe, + .driver = { + .name ="ast2700-intc" , + .of_match_table = aspeed_intc_of_match, + }, +}; +builtin_platform_driver (aspeed_intc_driver);Why has this to be builtin and not a module? It has zero dependencies on the existing code in this file, right?
Just stick it into a seperate source file and make it modular with a seperateKconfigswitch. No point in carrying this code as built-in in multi-platform builds.
Quoted 3 lines
This whole platform driver muck is just there to expose the routing and protection registers in debugfs even if debugfs is disabled. Seriously?
OK. I will do it in separate source c file and make it modular with Kconfig. And also depends on DEBUG_FS
Thanks for guidance.
Quoted 6 lines
It's completely disconnected from the interrupt delivery chain as far as the kernel is concerned, i.e. it does not provide a interrupt domain/chip. So that interface dumps just some register values with a lot of effort and leaves it to the user to decode which actual linux interrupt in the real intc-ic interrupt domains is affected, right?
Quoted 9 lines
I'm still failing to see the value of all of this. As the kernel does not even modify these registers, you are basically implementing a dump facility for decoding what the firmware put into those registers, right?
I don't have a strong opinion about it, but if it has a value, then all of this can be done with way smaller code by just dumping the raw register values all in one go. No need for two files and string encoding. That's what user space is for.
Thanks, I will send a new version with a modular driver and a simplified debugfs interface as you suggested.
Quoted 100 lines
Something like the completely uncompiled below, which I cobbled together quickly for illustration. You get the idea.
Thanks,
tglx
---#define
INTC_TYPE_C00
#defineINTC_TYPE_C11struct aspeed_intc { void __iomem *base; unsigned int type; };const struct aspeed_intc_data { char *name; unsigned int groups; unsigned int prot_offs; unsigned int rout_offs; unsigned int rout_gap; } aspeed_intc_data[2] = { { .name = "INTC0", .groups = 4, .prot_offs = 0x40, .rout_offs = 0x200, .rout_gap = 0x100, }, { .name = "INTC1", .groups = 6, .prot_offs = 0x0, .rout_offs = 0x80, .rout_gap = 0x20, }, };static int aspeed_intc_show(struct seq_file *m, void *unused) { struct aspeed_intc *intc = m->private; const struct aspeed_intc_data *d = &aspeed_intc_data[intc->type]; void __iomem *base = intc->base; seq_printf(m, "%s\n", d->name) seq_printf(m, "P: 0x%08x\n", readl(base + d->prot_offs)); base += d->rout_offs; for (unsigned int i = 0; i < d->groups; i++, base += 4) { seq_printf(m, "R%d: 0x%08x 0x%08x 0x%08x\n", i, readl(base), readl(base + d->rout_gap), readl(base + 2 * d->rout_gap)); } return 0; }
DEFINE_SHOW_ATTRIBUTE(aspeed_intc);static int aspeed_intc_probe(struct platform_device *pdev) { struct aspeed_intc *intc; struct resource *res; struct dentry *dir; intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL); if (!intc) return -ENOMEM; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); intc->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(intc->base)) return PTR_ERR(intc->base); if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0")) intc->type = INTC_TYPE_C0; else if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc1")) intc->type = INTC_TYPE_C1; else return -ENOTSUPP; platform_set_drvdata(pdev, intc); dir = debugfs_create_dir(dev_name(&pdev->dev), NULL); debugfs_create_file("intc_regs", 0400, dir, intc, &aspeed_intc_fops); return 0; }static const struct of_device_id aspeed_intc_of_match[] = { { .compatible = "aspeed,ast2700-intc0", }, { .compatible = "aspeed,ast2700-intc1", }, { }, };
MODULE_DEVICE_TABLE(of, aspeed_intc_of_match);static struct platform_driver aspeed_intc_driver = { .probe = aspeed_intc_probe, .driver = { .name = "ast2700-intc", .of_match_table = aspeed_intc_of_match, }, };
module_platform_driver(aspeed_intc_driver);
RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
Quoted 11 lines · Ryan Chen
Quoted 9 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 6 lines
drivers/irqchip/irq-aspeed-intc.c +6
-0+struct aspeed_intc { +void __iomem *base; +struct device *dev; +struct dentry *dbg_root; +int (*show_routing)(struct seq_file *s,void *unused); +int (*show_prot)(struct seq_file *s,void *unused); };See the chapter about struct declarations and initializers in the documentation I linked to above.
Sorry, I don't see the struct "> > + int (*show_prot)(struct
seq_file*s, void *unused); };"
I fatfingered that, but that's not the problem.
Quoted 1 line
My original submit is following, it should ok. Am I right?
No. Read the chapter I pointed you to.
Quoted 1 line
I have replied to this very mail. No need to paste me this and the pointer to some random mail archive
Ryan Chen
reviewerRE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
Quoted 14 lines
Subject: RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display
Quoted 10 lines · Ryan Chen
Quoted 9 lines
drivers/irqchip/irq-aspeed-intc.c static int __init aspeed_intc_ic_of_init(struct device_node *node,
Quoted 6 lines
drivers/irqchip/irq-aspeed-intc.c +6
-0+struct aspeed_intc { +void __iomem *base; +struct device *dev; +struct dentry *dbg_root; +int (*show_routing)(struct seq_file *s,void *unused); +int (*show_prot)(struct seq_file *s,void *unused); };See the chapter about struct declarations and initializers in the documentation I linked to above.
Quoted 10 lines
Quoted 1 line
Sorry, I don't see the struct "> > + int (*show_prot)(struct
seq_file*s, void*unused); };"
I fatfingered that, but that's not the problem.
Quoted 1 line
My original submit is following, it should ok. Am I right?
No. Read the chapter I pointed you to.
Quoted 1 line
Thanks, I think your point is align the struct member names. Will update.
struct aspeed_intc {
void __iomem *base;
struct device *dev;
struct dentry *dbg_root;
int (*show_routing)(struct seq_file *s, void *unused);
int (*show_prot)(struct seq_file *s, void *unused);
};Quoted 4 lines
I have replied to this very mail. No need to paste me this and the pointer to some random mail archive