Message-ID: <CAMpQs4JhfuB4=s9VFc+xmw_+8h5u2EwPdM_0x2vO_=SYabAAxw@mail.gmail.com>
Date:   Tue, 22 Aug 2023 16:13:16 +0800
From:   Binbin Zhou <zhoubb.aaron@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Binbin Zhou <zhoubinbin@...ngson.cn>,
        Huacai Chen <chenhuacai@...ngson.cn>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Huacai Chen <chenhuacai@...nel.org>,
        loongson-kernel@...ts.loongnix.cn, devicetree@...r.kernel.org,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        linux-mips@...r.kernel.org, diasyzhang@...cent.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] dt-bindings: interrupt-controller: loongson,liointc:
 Fix warnings about liointc-2.0

Hi Krzysztof:

Thanks for your detailed reply.

On Tue, Aug 22, 2023 at 1:44 PM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> 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@...11440: 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@...11440: 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@...ngson.cn>
> > ---
> > 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@loongson.cn/
> >
> >  .../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@loongson.cn/

Thanks.
Binbin
>
> > +      required:
> > +        - reg-names
> > +    else:
> > +      properties:
> > +        reg:
> > +          maxItems: 1
>
> so reg-names can be "pink-pony"?
>
> Best regards,
> Krzysztof
>

Powered by blists - more mailing lists