Re: [PATCH v2] dt-bindings: interrupt-controller: loongson,liointc: Fix warnings about liointc-2.0

[Binbin Zhou]

Hi Krzysztof:

Thanks for your detailed reply.

On Tue, Aug 22, 2023 at 4:30 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 22/08/2023 10:13, Binbin Zhou wrote:
> > 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".
>
> of_irq_get_byname() does not give you parent interrupt, but the
> interrupt. Why do you need parent interrupt and what is it?
>
> >
> > 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;
>
> How requiring parents interrupt is related to other changes in this
> file? One logical change, one patch.

Yes, that was my mistake, whether or not the interrupt-names need to
be "required" is another issue. It does not cause a check warning.
I'll think about it some more.
>
> Anyway why did you do it and take it by names? Names here are basically
> useless if they match indices, so just get interrupt by indices.

There is a match between interrupts, interrupt names and interrupt maps:

interrupt->interrupt name->interrupt map
2->int0->int_map[0]
3->int1->int_map[1]
4->int2->int_map[2]
5->int3->int_map[3]

As part of the 2k1000 liointc1 node:

                liointc1: interrupt-controller@1fe11440 {
....
                        interrupt-parent = <&cpuintc>;
                        interrupts = <3>;
                        interrupt-names = "int1";

                        loongson,parent_int_map = <0x00000000>, /* int0 */
                                                <0xffffffff>, /* int1 */
                                                <0x00000000>, /* int2 */
                                                <0x00000000>; /* int3 */
                };

To ensure this mapping relationship, the interrupt name becomes the
intermediate bridge.

>
> >
> >>
> >>>
> >>> 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
>
> All of them start from 0, so why do you want to allow here starting from 3?

Actually now we are all starting at int0.
Since the 2K1000 has 64 interrupt sources, we need two nodes to route
the interrupts. Usually liointc0 (handle 0-31 interrupts sources )uses
int0 and liointc1 (handle 32-63 interrupts sources ) uses int1.
As follows:

                liointc0: interrupt-controller@1fe11400 {
.....
                        interrupt-parent = <&cpuintc>;
                        interrupts = <2>;
                        interrupt-names = "int0";

                        loongson,parent_int_map = <0xffffffff>, /* int0 */
                                                <0x00000000>, /* int1 */
                                                <0x00000000>, /* int2 */
                                                <0x00000000>; /* int3 */
                };

                liointc1: interrupt-controller@1fe11440 {
....
                        interrupt-parent = <&cpuintc>;
                        interrupts = <3>;
                        interrupt-names = "int1";

                        loongson,parent_int_map = <0x00000000>, /* int0 */
                                                <0xffffffff>, /* int1 */
                                                <0x00000000>, /* int2 */
                                                <0x00000000>; /* int3 */
                };

At this point, liointc1 will be warned that it is not starting from
int0, and that int0 is actually being used by liointc0.

>
> >
> > 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.
>
> Why DT node would not have int0? Provide proper upstreamed Linux kernel
> source proving this, not some imaginary code.
>
> >
> >>
> >>>
> >>> 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
>
> You don't have this case here. I don't see any device asking for 4 regs.
>
> > 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?
>
> The top-level defines widest constraints and if:else: narrows them per
> each variant.
>
> ...
>
> >>> +        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
>
> This is driver. You need to show the DTS, not driver.
>
> >
> >         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/
>
> if:else: is recommended, we do not discuss it. Your v1 was making
> everything totally loose, so incorrect. Explain - why the reg-names are
> not correct for the other variant? We expect just to have maxItems for
> the other variant... unless reg-names are not correct, then they can be
> made false - which you didn't.
>
This is mainly due to discontinuities in register definitions.

Interrupt routing configuration involves two aspects of registers (32 bits):
1. interrupt configuration registers: including interrupt enable,
interrupt status, etc;
2. the CORE routing register: indicating which CORE to route to.

First of all, for liointc-1.0, e.g. Loongson-3A, they are contiguous
and we only need a set of register definitions, so reg-names are not
needed.

                liointc: interrupt-controller@3ff01400 {
                        compatible = "loongson,liointc-1.0";
                        reg = <0 0x3ff01400 0x64>;
...........
                };

However, for liointc-2.0, e.g. Loongson-2K1000, they are not
contiguous and we can only define them separately (main/isr0/isr1).

                liointc0: interrupt-controller@1fe11400 {
                        compatible = "loongson,liointc-2.0";
                        reg = <0 0x1fe11400 0 0x40>,
                                <0 0x1fe11040 0 0x8>,
                                <0 0x1fe11140 0 0x8>;
                        reg-names = "main", "isr0", "isr1";
..........
                };


Unfortunately, the Loongson-2K0500 is again special in that it is a
single-core CPU. therefore the core1 routing register (isr1) does not
exist.

                liointc0: interrupt-controller@1fe11400 {
                        compatible = "loongson,liointc-2.0";
                        reg = <0x0 0x1fe11400 0x0 0x40>,
                              <0x0 0x1fe11040 0x0 0x8>;
                        reg-names = "main", "isr0";
......
                };

So I would like to set the minItems of reg-names to 2 (main/isr0).

Thanks.
Binbin

>
> Best regards,
> Krzysztof
>