Hi Krzysztof: Thanks for your detailed reply. On Tue, Aug 22, 2023 at 1:44 PM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 21/08/2023 08:13, Binbin Zhou wrote: > > Since commit f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add > > Loongson-2K1000 LIOINTC"), the loongson liointc supports configuring > > routes for 64-bit interrupt sources. > > > > For liointc-2.0, we need to define two liointc nodes in dts, one for > > "0-31" interrupt sources and the other for "32-63" interrupt sources. > > This applies to mips Loongson-2K1000. > > > > Unfortunately, there are some warnings about "loongson,liointc-2.0": > > 1. "interrupt-names" should be "required", the driver gets the parent > > interrupts through it. > > No, why? Parent? This does not make sense. This was noted in the v1 patch discussion. The liointc driver now gets the parent interrupt via of_irq_get_byname(), so I think the "interrupt-names" should be "required". https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n345 static const char *const parent_names[] = {"int0", "int1", "int2", "int3"}; for (i = 0; i < LIOINTC_NUM_PARENT; i++) { parent_irq[i] = of_irq_get_byname(node, parent_names[i]); if (parent_irq[i] > 0) have_parent = TRUE; } if (!have_parent) return -ENODEV; > > > > > 2. Since not all CPUs are multicore, e.g. Loongson-2K0500 is a > > single-core CPU, there is no core1-related registers. So "reg" and > > "reg-names" should be set to "minItems 2". > > > > 3. Routing interrupts from "int0" is a common solution in practice, but > > theoretically there is no such requirement, as long as conflicts are > > avoided. So "interrupt-names" should be defined by "pattern". > > Why? What the pattern has to do with anything in routing or not routing > something? First of all, interrupt routing is configurable and each intx handles up to 32 interrupt sources. int0-int3 you can choose a single one or a combination of multiple ones, as long as the intx chosen matches the parent interrupt and is not duplicated: Parent interrupt --> intx 2-->int0 3-->int1 4-->int2 5-->int3 As: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts/loongson/loongson64g-package.dtsi?h=v6.5-rc6#n24 In addition, if there are 64 interrupt sources, such as the mips Loongson-2K1000, and we need two dts nodes to describe the interrupt routing, then there is bound to be a node without "int0". https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi?h=v6.5-rc6#n60 According to the current dt-binding rule, if the node does not have "int0", there will be a dts_check warning, which is not in line with our original intention. > > > > > This fixes dtbs_check warning: > > > > DTC_CHK arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb > > arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb: interrupt-controller@1fe11440: interrupt-names:0: 'int0' was expected > > From schema: Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml > > arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb: interrupt-controller@1fe11440: Unevaluated properties are not allowed ('interrupt-names' was unexpected) > > From schema: Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml > > > > Fixes: f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add Loongson-2K1000 LIOINTC") > > Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx> > > --- > > V2: > > 1. Update commit message; > > 2. "interruprt-names" should be "required", the driver gets the parent > > interrupts through it; > > 3. Add more descriptions to explain the rationale for multiple nodes; > > 4. Rewrite if-else statements. > > > > Link to V1: > > https://lore.kernel.org/all/20230815084713.1627520-1-zhoubinbin@xxxxxxxxxxx/ > > > > .../loongson,liointc.yaml | 74 +++++++++---------- > > 1 file changed, 37 insertions(+), 37 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml > > index 00b570c82903..f695d3a75ddf 100644 > > --- a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml > > +++ b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml > > @@ -11,11 +11,11 @@ maintainers: > > > > description: | > > This interrupt controller is found in the Loongson-3 family of chips and > > - Loongson-2K1000 chip, as the primary package interrupt controller which > > + Loongson-2K series chips, as the primary package interrupt controller which > > can route local I/O interrupt to interrupt lines of cores. > > - > > -allOf: > > - - $ref: /schemas/interrupt-controller.yaml# > > + In particular, the Loongson-2K1000/2K0500 has 64 interrupt sources that we > > + need to describe with two dts nodes. One for interrupt sources "0-31" and > > + the other for interrupt sources "32-63". > > > > properties: > > compatible: > > @@ -24,15 +24,9 @@ properties: > > - loongson,liointc-1.0a > > - loongson,liointc-2.0 > > > > - reg: > > - minItems: 1 > > - minItems: 3 > > + reg: true > > No. Constraints must be here. May I ask a question: Since different compatibles require different minItems/minItems for the attribute, this writeup of defining the attribute to be true first and then defining the specific value in an if-else statement is not recommended? > > > > > - reg-names: > > - items: > > - - const: main > > - - const: isr0 > > - - const: isr1 > > + reg-names: true > > No, keep at least min/maxItems here. > > > > > interrupt-controller: true > > > > @@ -45,11 +39,9 @@ properties: > > interrupt-names: > > description: List of names for the parent interrupts. > > items: > > - - const: int0 > > - - const: int1 > > - - const: int2 > > - - const: int3 > > + pattern: int[0-3] > > minItems: 1 > > + maxItems: 4 > > I don't see reason behind it. > > > > > '#interrupt-cells': > > const: 2 > > @@ -69,32 +61,41 @@ required: > > - compatible > > - reg > > - interrupts > > + - interrupt-names > > Why? You are doing multiple things at once, without proper explanation. Maybe this patch does too many things... There are actually 3 things here, as stated in the commit message, and since they are all about liointc-2.0 dts-check warnings, I put them in a patch. > > > - interrupt-controller > > - '#interrupt-cells' > > - loongson,parent_int_map > > > > - > > unevaluatedProperties: false > > > > -if: > > - properties: > > - compatible: > > - contains: > > - enum: > > - - loongson,liointc-2.0 > > - > > -then: > > - properties: > > - reg: > > - minItems: 3 > > - > > - required: > > - - reg-names > > - > > -else: > > - properties: > > - reg: > > - maxItems: 1 > > +allOf: > > + - $ref: /schemas/interrupt-controller.yaml# > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - loongson,liointc-2.0 > > + then: > > + properties: > > + reg: > > + minItems: 2 > > + items: > > + - description: Interrupt routing registers. > > + - description: Low/high 32-bit interrupt status routed to core0. > > + - description: Low/high 32-bit interrupt status routed to core1. > > + reg-names: > > + minItems: 2 > > + items: > > + - const: main > > + - const: isr0 > > + - const: isr1 > > Srsly, why this is moved here from the top? It does not make sense. In liointc-2.0, we need to deal with two dts nodes, and the setting and routing registers are not contiguous, so the driver needs "reg-names" to get the corresponding register mapping. So I put all this in the liointc-2.0 section. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n225 if (revision > 1) { for (i = 0; i < LIOINTC_NUM_CORES; i++) { int index = of_property_match_string(node, "reg-names", core_reg_names[i]); if (index < 0) continue; priv->core_isr[i] = of_iomap(node, index); } if (!priv->core_isr[0]) goto out_iounmap; } I referenced other dt-binding writeups and thought this would be clearer. Is this if-else style not recommended? Should I keep the v1 patch writeup? https://lore.kernel.org/all/20230815084713.1627520-1-zhoubinbin@xxxxxxxxxxx/ Thanks. Binbin > > > + required: > > + - reg-names > > + else: > > + properties: > > + reg: > > + maxItems: 1 > > so reg-names can be "pink-pony"? > > Best regards, > Krzysztof >