Re: [PATCH v2] dt-bindings: interrupt-controller: loongson,liointc: Fix warnings about liointc-2.0
From: Binbin Zhou
Date: Tue Aug 22 2023 - 04:13:33 EST
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
>