Message-ID: <6ba31912-6738-6156-d5f4-3c8d3a3ca7bc@linaro.org>
Date:   Tue, 22 Aug 2023 10:30:26 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Binbin Zhou <zhoubb.aaron@...il.com>
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

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@...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".

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.

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.

> 
>>
>>>
>>> 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?

> 
> 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@...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

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@loongson.cn/

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.


Best regards,
Krzysztof

Powered by blists - more mailing lists