| Message ID | 20241212155237.848336-3-kevin_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/6] dt-bindings: interrupt-controller: Refine size/interrupt-cell usage. | expand |
On 12/12/2024 16:52, Kevin Chen wrote: > 1. Because size-cells is no need to use 2, modify to 1 for use. ??? > 2. Add minItems to 1 for interrupts for intc1. ??? > 3. Add 1 interrupt of intc1 example into yaml file. > 4. Add intc1 sub-module of uart12 as example using the intc0 and intc1. What is all this? BTW, there was no such patch in previous version and your changelog is silent about it. Subject: drop all full stops. Subject never ends with full stop. > --- > .../aspeed,ast2700-intc.yaml | 60 +++++++++++++++---- > 1 file changed, 47 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml > index 55636d06a674..eadfbc45326b 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml > +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml > @@ -31,6 +31,7 @@ properties: > type as defined in interrupt.txt in this directory. > > interrupts: > + minItems: 1 Nope, not explained, not constrained. Your schema is supposed to be constrained. > maxItems: 6 > description: | > Depend to which INTC0 or INTC1 used. > @@ -68,19 +69,52 @@ examples: > #include <dt-bindings/interrupt-controller/arm-gic.h> > > bus { > + #address-cells = <2>; > + #size-cells = <1>; > + > + intc0: interrupt-controller@12100000 { > + compatible = "simple-mfd"; > + reg = <0 0x12100000 0x4000>; > + ranges = <0x0 0x0 0x0 0x12100000 0x4000>; > #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>; I don't understand what is all this. > + #size-cells = <1>; > + > + intc0_11: interrupt-controller@1b00 { > + compatible = "aspeed,ast2700-intc-ic"; > + reg = <0 0x12101b00 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 = "simple-mfd"; > + reg = <0 0x14c18000 0x400>; > + ranges = <0x0 0x0 0x0 0x14c18000 0x400>; > + #address-cells = <2>; > + #size-cells = <1>; > + > + intc1_4: interrupt-controller@140 { > + compatible = "aspeed,ast2700-intc-ic"; > + reg = <0x0 0x140 0x10>; > + #interrupt-cells = <2>; > + interrupt-controller; > + interrupts-extended = <&intc0_11 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > + }; > + }; > + > + uart12: serial@14c33b00 { > + compatible = "ns16550a"; > + reg = <0x0 0x14c33b00 0x100>; > + interrupts-extended = <&intc1_4 18 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > + reg-shift = <2>; > + reg-io-width = <4>; > + no-loopback-test; > + }; And above is not related at all. Don't add entirely unrelated changes. Drop. Best regards, Krzysztof
On Thu, Dec 12, 2024 at 11:52:31PM +0800, Kevin Chen wrote: > 1. Because size-cells is no need to use 2, modify to 1 for use. > 2. Add minItems to 1 for interrupts for intc1. > 3. Add 1 interrupt of intc1 example into yaml file. > 4. Add intc1 sub-module of uart12 as example using the intc0 and intc1. > --- > .../aspeed,ast2700-intc.yaml | 60 +++++++++++++++---- > 1 file changed, 47 insertions(+), 13 deletions(-) > As with all your patches, repeating since v1 the same comment, so one more last time: Please run scripts/checkpatch.pl and fix reported warnings. Then please run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
Hi, > > 1. Because size-cells is no need to use 2, modify to 1 for use. > > 2. Add minItems to 1 for interrupts for intc1. > > 3. Add 1 interrupt of intc1 example into yaml file. > > 4. Add intc1 sub-module of uart12 as example using the intc0 and intc1. > > --- > > .../aspeed,ast2700-intc.yaml | 60 > +++++++++++++++---- > > 1 file changed, 47 insertions(+), 13 deletions(-) > > > > As with all your patches, repeating since v1 the same comment, so one more > last time: > > Please run scripts/checkpatch.pl and fix reported warnings. Then please run > 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code here > looks like it needs a fix. Feel free to get in touch if the warning is not clear. OK. I need to wait the Ryan's series of " "Add support for AST2700 clk driver"" for clk and reset drivers. I will wait for his commits to be merged with the warnings fixed. > > Best regards, > Krzysztof
Hi Krzk, > > 1. Because size-cells is no need to use 2, modify to 1 for use. > > ??? So, is it OK that I change the size-cells back to 2 include the aspeed,ast2700-intc.yaml examples and aspeed-g7.dtsi? > > > 2. Add minItems to 1 for interrupts for intc1. > > ??? For variable interrupt numbers, I need to fix the below warnings by minItems. DTC [C] arch/arm64/boot/dts/aspeed/ast2700-evb.dtb /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@100: interrupts-extended: [[3, 0, 3844]] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@110: interrupts-extended: [[3, 1, 3844]] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@120: interrupts-extended: [[3, 2, 3844]] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@130: interrupts-extended: [[3, 3, 3844]] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@140: interrupts-extended: [[3, 4, 3844]] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@150: interrupts-extended: [[3, 5, 3844]] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > > > 3. Add 1 interrupt of intc1 example into yaml file. > > > 4. Add intc1 sub-module of uart12 as example using the intc0 and intc1. > > What is all this? > > BTW, there was no such patch in previous version and your changelog is silent > about it. Agree, I will restore the previous version. > > Subject: drop all full stops. Subject never ends with full stop. > > > --- > > .../aspeed,ast2700-intc.yaml | 60 > +++++++++++++++---- > > 1 file changed, 47 insertions(+), 13 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270 > > 0-intc.yaml > > b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270 > > 0-intc.yaml index 55636d06a674..eadfbc45326b 100644 > > --- > > a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270 > > 0-intc.yaml > > +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,as > > +++ t2700-intc.yaml > > @@ -31,6 +31,7 @@ properties: > > type as defined in interrupt.txt in this directory. > > > > interrupts: > > + minItems: 1 > > Nope, not explained, not constrained. Your schema is supposed to be > constrained. > > > > maxItems: 6 > > description: | > > Depend to which INTC0 or INTC1 used. > > @@ -68,19 +69,52 @@ examples: > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > bus { > > + #address-cells = <2>; > > + #size-cells = <1>; > > + > > + intc0: interrupt-controller@12100000 { > > + compatible = "simple-mfd"; > > + reg = <0 0x12100000 0x4000>; > > + ranges = <0x0 0x0 0x0 0x12100000 0x4000>; > > #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>; > > > I don't understand what is all this. > > > + #size-cells = <1>; > > + > > + intc0_11: interrupt-controller@1b00 { > > + compatible = "aspeed,ast2700-intc-ic"; > > + reg = <0 0x12101b00 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 = "simple-mfd"; > > + reg = <0 0x14c18000 0x400>; > > + ranges = <0x0 0x0 0x0 0x14c18000 0x400>; > > + #address-cells = <2>; > > + #size-cells = <1>; > > + > > + intc1_4: interrupt-controller@140 { > > + compatible = "aspeed,ast2700-intc-ic"; > > + reg = <0x0 0x140 0x10>; > > + #interrupt-cells = <2>; > > + interrupt-controller; > > + interrupts-extended = <&intc0_11 4 > (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > > + }; > > + }; > > + > > + uart12: serial@14c33b00 { > > + compatible = "ns16550a"; > > + reg = <0x0 0x14c33b00 0x100>; > > + interrupts-extended = <&intc1_4 18 (GIC_CPU_MASK_SIMPLE(4) > | IRQ_TYPE_LEVEL_HIGH)>; > > + reg-shift = <2>; > > + reg-io-width = <4>; > > + no-loopback-test; > > + }; > > And above is not related at all. Don't add entirely unrelated changes. Drop. > > > Best regards, > Krzysztof Best Regards, Kevin. Chen
On 18/12/2024 04:04, Kevin Chen wrote: > Hi Krzk, > >>> 1. Because size-cells is no need to use 2, modify to 1 for use. >> >> ??? > So, is it OK that I change the size-cells back to 2 include the aspeed,ast2700-intc.yaml examples and aspeed-g7.dtsi? No, my total surprise is that I did not understand what it maens. Is this changelog? Commit msg? Why such change is made? > >> >>> 2. Add minItems to 1 for interrupts for intc1. >> >> ??? > For variable interrupt numbers, I need to fix the below warnings by minItems. > DTC [C] arch/arm64/boot/dts/aspeed/ast2700-evb.dtb > /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@100: interrupts-extended: [[3, 0, 3844]] is too short > from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@110: interrupts-extended: [[3, 1, 3844]] is too short > from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@120: interrupts-extended: [[3, 2, 3844]] is too short > from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@130: interrupts-extended: [[3, 3, 3844]] is too short > from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@140: interrupts-extended: [[3, 4, 3844]] is too short > from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > /home/kevin/linux-mainline/arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: interrupt-controller@150: interrupts-extended: [[3, 5, 3844]] is too short > from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml# > >> >>> 3. Add 1 interrupt of intc1 example into yaml file. >> >>> 4. Add intc1 sub-module of uart12 as example using the intc0 and intc1. >> >> What is all this? >> >> BTW, there was no such patch in previous version and your changelog is silent >> about it. > Agree, I will restore the previous version. > >> >> Subject: drop all full stops. Subject never ends with full stop. >> >>> --- >>> .../aspeed,ast2700-intc.yaml | 60 >> +++++++++++++++---- >>> 1 file changed, 47 insertions(+), 13 deletions(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270 >>> 0-intc.yaml >>> b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270 >>> 0-intc.yaml index 55636d06a674..eadfbc45326b 100644 >>> --- >>> a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270 >>> 0-intc.yaml >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,as >>> +++ t2700-intc.yaml >>> @@ -31,6 +31,7 @@ properties: >>> type as defined in interrupt.txt in this directory. >>> >>> interrupts: >>> + minItems: 1 >> >> Nope, not explained, not constrained. Your schema is supposed to be >> constrained. I still do not understand this commit at all. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml index 55636d06a674..eadfbc45326b 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml @@ -31,6 +31,7 @@ properties: type as defined in interrupt.txt in this directory. interrupts: + minItems: 1 maxItems: 6 description: | Depend to which INTC0 or INTC1 used. @@ -68,19 +69,52 @@ examples: #include <dt-bindings/interrupt-controller/arm-gic.h> bus { + #address-cells = <2>; + #size-cells = <1>; + + intc0: interrupt-controller@12100000 { + compatible = "simple-mfd"; + reg = <0 0x12100000 0x4000>; + ranges = <0x0 0x0 0x0 0x12100000 0x4000>; #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>; + #size-cells = <1>; + + intc0_11: interrupt-controller@1b00 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0 0x12101b00 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 = "simple-mfd"; + reg = <0 0x14c18000 0x400>; + ranges = <0x0 0x0 0x0 0x14c18000 0x400>; + #address-cells = <2>; + #size-cells = <1>; + + intc1_4: interrupt-controller@140 { + compatible = "aspeed,ast2700-intc-ic"; + reg = <0x0 0x140 0x10>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc0_11 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; + }; + }; + + uart12: serial@14c33b00 { + compatible = "ns16550a"; + reg = <0x0 0x14c33b00 0x100>; + interrupts-extended = <&intc1_4 18 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; + reg-shift = <2>; + reg-io-width = <4>; + no-loopback-test; + }; };