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

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

[Binbin Zhou]

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.

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

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@loongson.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
-    maxItems: 3
+  reg: true
 
-  reg-names:
-    items:
-      - const: main
-      - const: isr0
-      - const: isr1
+  reg-names: true
 
   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
 
   '#interrupt-cells':
     const: 2
@@ -69,32 +61,41 @@ required:
   - compatible
   - reg
   - interrupts
+  - interrupt-names
   - 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
+      required:
+        - reg-names
+    else:
+      properties:
+        reg:
+          maxItems: 1
 
 examples:
   - |
@@ -113,7 +114,6 @@ examples:
                                 <0x0f000000>, /* int1 */
                                 <0x00000000>, /* int2 */
                                 <0x00000000>; /* int3 */
-
     };
 
 ...
-- 
2.39.3

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

[Huacai Chen]

Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>

On Mon, Aug 21, 2023 at 2:13 PM Binbin Zhou <zhoubinbin@loongson.cn> 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.
>
> 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".
>
> 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@loongson.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
> -    maxItems: 3
> +  reg: true
>
> -  reg-names:
> -    items:
> -      - const: main
> -      - const: isr0
> -      - const: isr1
> +  reg-names: true
>
>    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
>
>    '#interrupt-cells':
>      const: 2
> @@ -69,32 +61,41 @@ required:
>    - compatible
>    - reg
>    - interrupts
> +  - interrupt-names
>    - 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
> +      required:
> +        - reg-names
> +    else:
> +      properties:
> +        reg:
> +          maxItems: 1
>
>  examples:
>    - |
> @@ -113,7 +114,6 @@ examples:
>                                  <0x0f000000>, /* int1 */
>                                  <0x00000000>, /* int2 */
>                                  <0x00000000>; /* int3 */
> -
>      };
>
>  ...
> --
> 2.39.3
>

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

[Krzysztof Kozlowski]

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.

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

> 
> 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@loongson.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
> -    maxItems: 3
> +  reg: true

No. Constraints must be here.

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

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

> +      required:
> +        - reg-names
> +    else:
> +      properties:
> +        reg:
> +          maxItems: 1

so reg-names can be "pink-pony"?

Best regards,
Krzysztof

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 1:44 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.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@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@loongson.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
>

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

[Krzysztof Kozlowski]

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

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

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

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

[Krzysztof Kozlowski]

On 24/08/2023 13:32, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> Thanks for your detailed reply.
> 
> On Tue, Aug 22, 2023 at 4:30 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> 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@linaro.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.
> 
> 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 */


How did you sneak this property? The version - v2 - which was reviewed
by Rob:
https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/
did not have it.

Now v3 suddenly appears with Rob's review and this property:
https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/

Please help me understand this property appeared there and how did you
get it reviewed?

>                                                 <0xffffffff>, /* int1 */
>                                                 <0x00000000>, /* int2 */
>                                                 <0x00000000>; /* int3 */

So now you will keep bringing more hacks for a hacky property. No, this
cannot go on.

Best regards,
Krzysztof

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

[Jiaxun Yang]

在 2023/8/25 20:56, Krzysztof Kozlowski 写道:
[...]
> How did you sneak this property? The version - v2 - which was reviewed
> by Rob:
> https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/
> did not have it.
>
> Now v3 suddenly appears with Rob's review and this property:
> https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/
>
> Please help me understand this property appeared there and how did you
> get it reviewed?
Hi all,

It has been some years since this series was merged.
My vague memory tells me there was some off-list discussion made in IRC with
linux-arch folks and IRQ folks to come up with this binding design.

In this case I guess I forgot to drop Rob's R-b tag when updating this patch
between reversions. I  apologize for any inconvenience this may have caused.

>
>>                                                  <0xffffffff>, /* int1 */
>>                                                  <0x00000000>, /* int2 */
>>                                                  <0x00000000>; /* int3 */
> So now you will keep bringing more hacks for a hacky property. No, this
> cannot go on.

What's the best way, in your opinion, to overhaul this property? As we don't
really care backward compatibility of DTBs on those systems we can just 
redesign it.

A little bit background about this property, LIOINTC can route a 
interrupt to any of
4 upstream core interrupt pins. Downstream interrupt devicies should not 
care about
which pin the interrupt go but we want to leave a knob in devicetree for 
performance
tuning. So we designed such property that use masks corresponding to 
each upsteam
interrupt pins to tell where should a interrupt go.

Thnaks
- Jiaxun

>
> Best regards,
> Krzysztof
>

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

[Marc Zyngier]

On Wed, 30 Aug 2023 04:59:20 +0100,
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> 
> 
> 在 2023/8/25 20:56, Krzysztof Kozlowski 写道:
> [...]
> > How did you sneak this property? The version - v2 - which was reviewed
> > by Rob:
> > https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/
> > did not have it.
> > 
> > Now v3 suddenly appears with Rob's review and this property:
> > https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/
> > 
> > Please help me understand this property appeared there and how did you
> > get it reviewed?
> Hi all,
> 
> It has been some years since this series was merged.
> My vague memory tells me there was some off-list discussion made in IRC with
> linux-arch folks and IRQ folks to come up with this binding design.
> 
> In this case I guess I forgot to drop Rob's R-b tag when updating this patch
> between reversions. I  apologize for any inconvenience this may have caused.
> 
> > 
> >>                                                  <0xffffffff>, /* int1 */
> >>                                                  <0x00000000>, /* int2 */
> >>                                                  <0x00000000>; /* int3 */
> > So now you will keep bringing more hacks for a hacky property. No, this
> > cannot go on.
> 
> What's the best way, in your opinion, to overhaul this property? As we don't
> really care backward compatibility of DTBs on those systems we can
> just redesign it.

You may not care about backward compatibility, but I do. We don't
break existing systems, full stop.

As for the offending property, it has no place here either. DT is not
the place where you put "performance knobs".

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Krzysztof Kozlowski]

On 30/08/2023 05:59, Jiaxun Yang wrote:
> 
> 
> 在 2023/8/25 20:56, Krzysztof Kozlowski 写道:
> [...]
>> How did you sneak this property? The version - v2 - which was reviewed
>> by Rob:
>> https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/
>> did not have it.
>>
>> Now v3 suddenly appears with Rob's review and this property:
>> https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/
>>
>> Please help me understand this property appeared there and how did you
>> get it reviewed?
> Hi all,
> 
> It has been some years since this series was merged.
> My vague memory tells me there was some off-list discussion made in IRC with
> linux-arch folks and IRQ folks to come up with this binding design.

We would not suggest you property which in the name has underscores and
duplicates interrupt-map property.

> 
> In this case I guess I forgot to drop Rob's R-b tag when updating this patch
> between reversions. I  apologize for any inconvenience this may have caused.


> 
>>
>>>                                                  <0xffffffff>, /* int1 */
>>>                                                  <0x00000000>, /* int2 */
>>>                                                  <0x00000000>; /* int3 */
>> So now you will keep bringing more hacks for a hacky property. No, this
>> cannot go on.
> 
> What's the best way, in your opinion, to overhaul this property? As we don't
> really care backward compatibility of DTBs on those systems we can just 
> redesign it.

Deprecate the property in the bindings, allow driver to work with or
without it and finally drop it entirely from DTS.
> 
> A little bit background about this property, LIOINTC can route a 
> interrupt to any of
> 4 upstream core interrupt pins. Downstream interrupt devicies should not 
> care about
> which pin the interrupt go but we want to leave a knob in devicetree for 
> performance
> tuning. So we designed such property that use masks corresponding to 
> each upsteam
> interrupt pins to tell where should a interrupt go.


Best regards,
Krzysztof

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

[Jiaxun Yang]

在 2023/8/30 21:44, Marc Zyngier 写道:
[...]
>> What's the best way, in your opinion, to overhaul this property? As we don't
>> really care backward compatibility of DTBs on those systems we can
>> just redesign it.
> You may not care about backward compatibility, but I do. We don't
> break existing systems, full stop.
Ah it won't break any existing system. Sorry for not giving enough insight
into the platform in previous reply. As for Loongson64 all DTBs are built
into kernel binary. So as long as binding are changed together with all DTS
in tree we won't break any system.
> As for the offending property, it has no place here either. DT is not
> the place where you put "performance knobs".
Hmm, I can see various bindings with vendor prefix exposing device
configurations. If we seen this interrupt routing as a device configuration
I don't think it's against devicetree design philosophy.

Thanks
- Jiaxun
>
> 	M.
>

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

[Jiaxun Yang]

在 2023/8/30 22:35, Krzysztof Kozlowski 写道:
>> What's the best way, in your opinion, to overhaul this property? As we don't
>> really care backward compatibility of DTBs on those systems we can just
>> redesign it.
> Deprecate the property in the bindings, allow driver to work with or
> without it and finally drop it entirely from DTS.

I'd love to have such configuration flexibility so I'd be sad to see it go.
+ Huacai and Binbin, what's your opinion?

If dropping such functionality in kernel is a must go, we can hardcode
to route all downstream interrupt to the first pin that passed to DT.

Thanks
- Jiaxun
> Best regards,
> Krzysztof
>

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

[Binbin Zhou]

cc Jianmin Lv.

Hi all:

Jianmin knows Loongson interrupt controllers well, he may have other
suggestions.

Thanks.
Binbin

On Wed, Aug 30, 2023 at 11:31 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
>
>
> 在 2023/8/30 22:35, Krzysztof Kozlowski 写道:
> >> What's the best way, in your opinion, to overhaul this property? As we don't
> >> really care backward compatibility of DTBs on those systems we can just
> >> redesign it.
> > Deprecate the property in the bindings, allow driver to work with or
> > without it and finally drop it entirely from DTS.
>
> I'd love to have such configuration flexibility so I'd be sad to see it go.
> + Huacai and Binbin, what's your opinion?
>
> If dropping such functionality in kernel is a must go, we can hardcode
> to route all downstream interrupt to the first pin that passed to DT.
>
> Thanks
> - Jiaxun
> > Best regards,
> > Krzysztof
> >
>

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

[Jianmin Lv]

Each IRQ source of liointc may be routed to different IRQ source of 
cpuintc, for implementing this, bit mapping between liointc and cpuintc 
is required, and the mapping information is used for liointc IRQ routing 
register setting. It seems that interrupt-map can not pass the mapping 
to driver in current driver/of code,  so the mapping is used in a 
non-standard way(of cause, underscore style may be changed in dts and 
driver).

IMO, hardcode routing completely in driver is not flexible and not 
recommended, and if possible, keep current map unchanged please.

Jianmin

Thanks.

On 2023/8/31 上午9:47, Binbin Zhou wrote:
> cc Jianmin Lv.
>
> Hi all:
>
> Jianmin knows Loongson interrupt controllers well, he may have other
> suggestions.
>
> Thanks.
> Binbin
>
> On Wed, Aug 30, 2023 at 11:31 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>>
>>
>> 在 2023/8/30 22:35, Krzysztof Kozlowski 写道:
>>>> What's the best way, in your opinion, to overhaul this property? As we don't
>>>> really care backward compatibility of DTBs on those systems we can just
>>>> redesign it.
>>> Deprecate the property in the bindings, allow driver to work with or
>>> without it and finally drop it entirely from DTS.
>> I'd love to have such configuration flexibility so I'd be sad to see it go.
>> + Huacai and Binbin, what's your opinion?
>>
>> If dropping such functionality in kernel is a must go, we can hardcode
>> to route all downstream interrupt to the first pin that passed to DT.
>>
>> Thanks
>> - Jiaxun
>>> Best regards,
>>> Krzysztof
>>>

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

[Krzysztof Kozlowski]

On 02/09/2023 08:32, Jianmin Lv wrote:
> Each IRQ source of liointc may be routed to different IRQ source of 
> cpuintc, for implementing this, bit mapping between liointc and cpuintc 
> is required, and the mapping information is used for liointc IRQ routing 
> register setting. It seems that interrupt-map can not pass the mapping 
> to driver in current driver/of code,  so the mapping is used in a 
> non-standard way(of cause, underscore style may be changed in dts and 
> driver).
> 
> IMO, hardcode routing completely in driver is not flexible and not 

"not recommended" by whom?

> recommended, and if possible, keep current map unchanged please.
> 

You could have proposed that time, when review was happening. The
property was added AFTER review, so the review was invalidated.

Drop the property and use interrupt-map. Eventually change it to
something acceptable which will PASS review, not bypass it.

Best regards,
Krzysztof

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

[Marc Zyngier]

On Wed, 30 Aug 2023 16:25:48 +0100,
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> 
> 
> 在 2023/8/30 21:44, Marc Zyngier 写道:
> [...]
> >> What's the best way, in your opinion, to overhaul this property? As we don't
> >> really care backward compatibility of DTBs on those systems we can
> >> just redesign it.
> > You may not care about backward compatibility, but I do. We don't
> > break existing systems, full stop.
> Ah it won't break any existing system. Sorry for not giving enough insight
> into the platform in previous reply. As for Loongson64 all DTBs are built
> into kernel binary. So as long as binding are changed together with all DTS
> in tree we won't break any system.

This is factually wrong. QEMU produces a DT for Loongarch at runtime.
So no, you're not allowed to just drop bindings on the floor. They
stay forever.

> > As for the offending property, it has no place here either. DT is not
> > the place where you put "performance knobs".
> Hmm, I can see various bindings with vendor prefix exposing device
> configurations. If we seen this interrupt routing as a device configuration
> I don't think it's against devicetree design philosophy.

Just because we have tons of crap in the device trees doesn't give you
a license to be just as bad.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Binbin Zhou]

Hi all:

Sorry, it's been a while since the last discussion.

Previously, Krzysztof suggested using the standard "interrupt-map"
attribute instead of the "loongson,parent_int_map" attribute, which I
tried to implement, but the downside of this approach seems to be
obvious.

First of all, let me explain again the interrupt routing of the
loongson liointc.
For example, the Loongson-2K1000 has 64 interrupt sources, each with
the following 8-bit interrupt routing registers (main regs attribute
in dts):

+----+-------------------------------------------------------------------+
| bit  | description
            |
+----+-------------------------------------------------------------------+
| 3:0 | Processor core to route                                           |
| 7:4 | Routed processor core interrupt pins (INT0--INT3) |
+-----+------------------------------------------------------------------+

The "loongson,parent_int_map" attribute is to describe the routed
interrupt pins to cpuintc.

However, the "interrupt-map" attribute is not supposed to be used for
interrupt controller in the normal case. Though since commit
041284181226 ("of/irq: Allow matching of an interrupt-map local to an
interrupt controller"), the "interrupt-map" attribute can be used in
interrupt controller nodes. Some interrupt controllers were found not
to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
quirk for controllers with their own definition of interrupt-map"), a
quirk was added for these interrupt controllers. As we can see from
the commit message, this is a bad solution in itself.

Similarly, if we choose to use the "interrupt-map" attribute in the
interrupt controller, we have to use this unfriendly solution (quirk).
Because we hope of_irq_parse_raw() stops at the liointc level rather
than goto its parent level.

So, I don't think it's a good choice to use a bad solution as a replacement.

Do you have any other ideas?

Thanks.
Binbin

On Mon, Sep 4, 2023 at 2:54 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 30 Aug 2023 16:25:48 +0100,
> Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> >
> >
> >
> > 在 2023/8/30 21:44, Marc Zyngier 写道:
> > [...]
> > >> What's the best way, in your opinion, to overhaul this property? As we don't
> > >> really care backward compatibility of DTBs on those systems we can
> > >> just redesign it.
> > > You may not care about backward compatibility, but I do. We don't
> > > break existing systems, full stop.
> > Ah it won't break any existing system. Sorry for not giving enough insight
> > into the platform in previous reply. As for Loongson64 all DTBs are built
> > into kernel binary. So as long as binding are changed together with all DTS
> > in tree we won't break any system.
>
> This is factually wrong. QEMU produces a DT for Loongarch at runtime.
> So no, you're not allowed to just drop bindings on the floor. They
> stay forever.
>
> > > As for the offending property, it has no place here either. DT is not
> > > the place where you put "performance knobs".
> > Hmm, I can see various bindings with vendor prefix exposing device
> > configurations. If we seen this interrupt routing as a device configuration
> > I don't think it's against devicetree design philosophy.
>
> Just because we have tons of crap in the device trees doesn't give you
> a license to be just as bad.
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

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

[Jonas Gorski]

On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
>
> Hi all:
>
> Sorry, it's been a while since the last discussion.
>
> Previously, Krzysztof suggested using the standard "interrupt-map"
> attribute instead of the "loongson,parent_int_map" attribute, which I
> tried to implement, but the downside of this approach seems to be
> obvious.
>
> First of all, let me explain again the interrupt routing of the
> loongson liointc.
> For example, the Loongson-2K1000 has 64 interrupt sources, each with
> the following 8-bit interrupt routing registers (main regs attribute
> in dts):
>
> +----+-------------------------------------------------------------------+
> | bit  | description
>             |
> +----+-------------------------------------------------------------------+
> | 3:0 | Processor core to route                                           |
> | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> +-----+------------------------------------------------------------------+
>
> The "loongson,parent_int_map" attribute is to describe the routed
> interrupt pins to cpuintc.
>
> However, the "interrupt-map" attribute is not supposed to be used for
> interrupt controller in the normal case. Though since commit
> 041284181226 ("of/irq: Allow matching of an interrupt-map local to an
> interrupt controller"), the "interrupt-map" attribute can be used in
> interrupt controller nodes. Some interrupt controllers were found not
> to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
> quirk for controllers with their own definition of interrupt-map"), a
> quirk was added for these interrupt controllers. As we can see from
> the commit message, this is a bad solution in itself.
>
> Similarly, if we choose to use the "interrupt-map" attribute in the
> interrupt controller, we have to use this unfriendly solution (quirk).
> Because we hope of_irq_parse_raw() stops at the liointc level rather
> than goto its parent level.
>
> So, I don't think it's a good choice to use a bad solution as a replacement.
>
> Do you have any other ideas?

Assuming this is changeable at runtime, this sounds to me like this
mapping/routing could easily be exposed as irqchip cpu affinity. Then
userspace can apply all the performance optimizations it wants (and
can easily update them without fiddling with the kernel/dts).

And then there would be no need to hardcode/describe it in the dts(i) at all.

Best Regards,
Jonas

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

[Binbin Zhou]

On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
> On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> >
> > Hi all:
> >
> > Sorry, it's been a while since the last discussion.
> >
> > Previously, Krzysztof suggested using the standard "interrupt-map"
> > attribute instead of the "loongson,parent_int_map" attribute, which I
> > tried to implement, but the downside of this approach seems to be
> > obvious.
> >
> > First of all, let me explain again the interrupt routing of the
> > loongson liointc.
> > For example, the Loongson-2K1000 has 64 interrupt sources, each with
> > the following 8-bit interrupt routing registers (main regs attribute
> > in dts):
> >
> > +----+-------------------------------------------------------------------+
> > | bit  | description
> >             |
> > +----+-------------------------------------------------------------------+
> > | 3:0 | Processor core to route                                           |
> > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > +-----+------------------------------------------------------------------+
> >
> > The "loongson,parent_int_map" attribute is to describe the routed
> > interrupt pins to cpuintc.
> >
> > However, the "interrupt-map" attribute is not supposed to be used for
> > interrupt controller in the normal case. Though since commit
> > 041284181226 ("of/irq: Allow matching of an interrupt-map local to an
> > interrupt controller"), the "interrupt-map" attribute can be used in
> > interrupt controller nodes. Some interrupt controllers were found not
> > to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
> > quirk for controllers with their own definition of interrupt-map"), a
> > quirk was added for these interrupt controllers. As we can see from
> > the commit message, this is a bad solution in itself.
> >
> > Similarly, if we choose to use the "interrupt-map" attribute in the
> > interrupt controller, we have to use this unfriendly solution (quirk).
> > Because we hope of_irq_parse_raw() stops at the liointc level rather
> > than goto its parent level.
> >
> > So, I don't think it's a good choice to use a bad solution as a replacement.
> >
> > Do you have any other ideas?
>
> Assuming this is changeable at runtime, this sounds to me like this
> mapping/routing could easily be exposed as irqchip cpu affinity. Then
> userspace can apply all the performance optimizations it wants (and
> can easily update them without fiddling with the kernel/dts).
>
> And then there would be no need to hardcode/describe it in the dts(i) at all.

Hi Jonas:

Thanks for your reply.

It is possible that my non-detailed explanation caused your misunderstanding.
Allow me to explain again about the interrupt routing register above,
which we know is divided into two parts:

+----+-------------------------------------------------------------------+
| bit  | description |
+----+-------------------------------------------------------------------+
| 3:0 | Processor core to route                                           |
| 7:4 | Routed processor core interrupt pins (INT0--INT3) |
+-----+------------------------------------------------------------------+

The first part "processor core" will be set to "boot_cpu_id" in the
driver, which we assume is fixed and we don't need to care about it
here.
What we care about is the second part "mapping of device interrupts to
processor interrupt pins", which is what we want to describe in
dts(i).

Let's take the Loongson-2K1000 as an example again, it has 64
interrupt sources as inputs and 4 processor core interrupt pins as
outputs.
The sketch is shown below:

Device Interrupts           Interrupt Pins
                 +-------------+
         0---->|                |--> INT0
        ...       | Mapping |--> INT1
        ...       |                |--> INT2
        63--->|                |--> INT3
                 +-------------+

Therefore, this mapping relationship cannot be changed at runtime and
needs to be hardcoded/described in dts(i).

Thanks.
Binbin
>
> Best Regards,
> Jonas

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

[Jonas Gorski]

On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> >
> > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> > >
> > > Hi all:
> > >
> > > Sorry, it's been a while since the last discussion.
> > >
> > > Previously, Krzysztof suggested using the standard "interrupt-map"
> > > attribute instead of the "loongson,parent_int_map" attribute, which I
> > > tried to implement, but the downside of this approach seems to be
> > > obvious.
> > >
> > > First of all, let me explain again the interrupt routing of the
> > > loongson liointc.
> > > For example, the Loongson-2K1000 has 64 interrupt sources, each with
> > > the following 8-bit interrupt routing registers (main regs attribute
> > > in dts):
> > >
> > > +----+-------------------------------------------------------------------+
> > > | bit  | description
> > >             |
> > > +----+-------------------------------------------------------------------+
> > > | 3:0 | Processor core to route                                           |
> > > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > > +-----+------------------------------------------------------------------+
> > >
> > > The "loongson,parent_int_map" attribute is to describe the routed
> > > interrupt pins to cpuintc.
> > >
> > > However, the "interrupt-map" attribute is not supposed to be used for
> > > interrupt controller in the normal case. Though since commit
> > > 041284181226 ("of/irq: Allow matching of an interrupt-map local to an
> > > interrupt controller"), the "interrupt-map" attribute can be used in
> > > interrupt controller nodes. Some interrupt controllers were found not
> > > to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
> > > quirk for controllers with their own definition of interrupt-map"), a
> > > quirk was added for these interrupt controllers. As we can see from
> > > the commit message, this is a bad solution in itself.
> > >
> > > Similarly, if we choose to use the "interrupt-map" attribute in the
> > > interrupt controller, we have to use this unfriendly solution (quirk).
> > > Because we hope of_irq_parse_raw() stops at the liointc level rather
> > > than goto its parent level.
> > >
> > > So, I don't think it's a good choice to use a bad solution as a replacement.
> > >
> > > Do you have any other ideas?
> >
> > Assuming this is changeable at runtime, this sounds to me like this
> > mapping/routing could easily be exposed as irqchip cpu affinity. Then
> > userspace can apply all the performance optimizations it wants (and
> > can easily update them without fiddling with the kernel/dts).
> >
> > And then there would be no need to hardcode/describe it in the dts(i) at all.
>
> Hi Jonas:
>
> Thanks for your reply.
>
> It is possible that my non-detailed explanation caused your misunderstanding.
> Allow me to explain again about the interrupt routing register above,
> which we know is divided into two parts:
>
> +----+-------------------------------------------------------------------+
> | bit  | description |
> +----+-------------------------------------------------------------------+
> | 3:0 | Processor core to route                                           |
> | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> +-----+------------------------------------------------------------------+
>
> The first part "processor core" will be set to "boot_cpu_id" in the
> driver, which we assume is fixed and we don't need to care about it
> here.
> What we care about is the second part "mapping of device interrupts to
> processor interrupt pins", which is what we want to describe in
> dts(i).
>
> Let's take the Loongson-2K1000 as an example again, it has 64
> interrupt sources as inputs and 4 processor core interrupt pins as
> outputs.
> The sketch is shown below:
>
> Device Interrupts           Interrupt Pins
>                  +-------------+
>          0---->|                |--> INT0
>         ...       | Mapping |--> INT1
>         ...       |                |--> INT2
>         63--->|                |--> INT3
>                  +-------------+
>
> Therefore, this mapping relationship cannot be changed at runtime and
> needs to be hardcoded/described in dts(i).

But that's only a driver/description limitation, not an actual
physical limitation, right? In theory you could reroute them as much
as you want as long as you keep the kernel up-to-date about the
current routing (via whatever means).

Anyway, I guess you want to use the routed interrupt pin to identify
different irq controller blocks.

Can't the interrupt pin be inferred from the parent interrupt? If your
parent (hw) irq is two, route everything to INT0 etc? Or alternatively
use the name of the parent interrupt?

Best Regards,
Jonas

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

[Huacai Chen]

Hi, Jonas,

On Wed, Oct 18, 2023 at 5:38 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
> On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> >
> > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > >
> > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> > > >
> > > > Hi all:
> > > >
> > > > Sorry, it's been a while since the last discussion.
> > > >
> > > > Previously, Krzysztof suggested using the standard "interrupt-map"
> > > > attribute instead of the "loongson,parent_int_map" attribute, which I
> > > > tried to implement, but the downside of this approach seems to be
> > > > obvious.
> > > >
> > > > First of all, let me explain again the interrupt routing of the
> > > > loongson liointc.
> > > > For example, the Loongson-2K1000 has 64 interrupt sources, each with
> > > > the following 8-bit interrupt routing registers (main regs attribute
> > > > in dts):
> > > >
> > > > +----+-------------------------------------------------------------------+
> > > > | bit  | description
> > > >             |
> > > > +----+-------------------------------------------------------------------+
> > > > | 3:0 | Processor core to route                                           |
> > > > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > > > +-----+------------------------------------------------------------------+
> > > >
> > > > The "loongson,parent_int_map" attribute is to describe the routed
> > > > interrupt pins to cpuintc.
> > > >
> > > > However, the "interrupt-map" attribute is not supposed to be used for
> > > > interrupt controller in the normal case. Though since commit
> > > > 041284181226 ("of/irq: Allow matching of an interrupt-map local to an
> > > > interrupt controller"), the "interrupt-map" attribute can be used in
> > > > interrupt controller nodes. Some interrupt controllers were found not
> > > > to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
> > > > quirk for controllers with their own definition of interrupt-map"), a
> > > > quirk was added for these interrupt controllers. As we can see from
> > > > the commit message, this is a bad solution in itself.
> > > >
> > > > Similarly, if we choose to use the "interrupt-map" attribute in the
> > > > interrupt controller, we have to use this unfriendly solution (quirk).
> > > > Because we hope of_irq_parse_raw() stops at the liointc level rather
> > > > than goto its parent level.
> > > >
> > > > So, I don't think it's a good choice to use a bad solution as a replacement.
> > > >
> > > > Do you have any other ideas?
> > >
> > > Assuming this is changeable at runtime, this sounds to me like this
> > > mapping/routing could easily be exposed as irqchip cpu affinity. Then
> > > userspace can apply all the performance optimizations it wants (and
> > > can easily update them without fiddling with the kernel/dts).
> > >
> > > And then there would be no need to hardcode/describe it in the dts(i) at all.
> >
> > Hi Jonas:
> >
> > Thanks for your reply.
> >
> > It is possible that my non-detailed explanation caused your misunderstanding.
> > Allow me to explain again about the interrupt routing register above,
> > which we know is divided into two parts:
> >
> > +----+-------------------------------------------------------------------+
> > | bit  | description |
> > +----+-------------------------------------------------------------------+
> > | 3:0 | Processor core to route                                           |
> > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > +-----+------------------------------------------------------------------+
> >
> > The first part "processor core" will be set to "boot_cpu_id" in the
> > driver, which we assume is fixed and we don't need to care about it
> > here.
> > What we care about is the second part "mapping of device interrupts to
> > processor interrupt pins", which is what we want to describe in
> > dts(i).
> >
> > Let's take the Loongson-2K1000 as an example again, it has 64
> > interrupt sources as inputs and 4 processor core interrupt pins as
> > outputs.
> > The sketch is shown below:
> >
> > Device Interrupts           Interrupt Pins
> >                  +-------------+
> >          0---->|                |--> INT0
> >         ...       | Mapping |--> INT1
> >         ...       |                |--> INT2
> >         63--->|                |--> INT3
> >                  +-------------+
> >
> > Therefore, this mapping relationship cannot be changed at runtime and
> > needs to be hardcoded/described in dts(i).
>
> But that's only a driver/description limitation, not an actual
> physical limitation, right? In theory you could reroute them as much
> as you want as long as you keep the kernel up-to-date about the
> current routing (via whatever means).
>
> Anyway, I guess you want to use the routed interrupt pin to identify
> different irq controller blocks.
>
> Can't the interrupt pin be inferred from the parent interrupt? If your
> parent (hw) irq is two, route everything to INT0 etc? Or alternatively
> use the name of the parent interrupt?
Let me make things clear and ignore those irrelevant information.
1, Our liointc controller has 32 inputs from downstream interrupt
sources and 4 outputs to the parent irqchip, the "routing" here means
which input go to which output.
2, We use 'parent_int_map' to describe the boot-time routing in dts
previously, but Krzysztof suggests us to use 'interrupt-map' instead.
3, When we rework our driver to use 'interrupt-map', we found that
this property is not supposed to be used in a regular irqchip (it is
usually used in a pcie port which is also act as an irqchip).
4, If we still want to use 'interrupt-map' to describe the routing in
liointc, we should make of_irq_parse_raw() stop at the liointc level
rather than go to its parent level, because the hwirq is provided by
liointc, not its parent. To archive this goal, we should add liointc
to the quirk list.
5, So, for our liointc driver we have two choices: 1) still use the
'parent_int_map' property; 2) use 'interrupt-map' property and add
liointc to the quirk list. We prefer the first ourselves, but
Krzysztof may give us a best solution.

Huacai

>
> Best Regards,
> Jonas

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

[Binbin Zhou]

Hi Krzysztof & Marc:

Sorry for the interruption.
As said before, we tried to use the 'interrupt-map attribute' in our
Loongson liointc dts(i), but there are some unfriendly points.
Do you have any other different suggestions?

Thanks.
Binbin

On Wed, Oct 18, 2023 at 8:33 PM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Jonas,
>
> On Wed, Oct 18, 2023 at 5:38 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> >
> > On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > > >
> > > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> > > > >
> > > > > Hi all:
> > > > >
> > > > > Sorry, it's been a while since the last discussion.
> > > > >
> > > > > Previously, Krzysztof suggested using the standard "interrupt-map"
> > > > > attribute instead of the "loongson,parent_int_map" attribute, which I
> > > > > tried to implement, but the downside of this approach seems to be
> > > > > obvious.
> > > > >
> > > > > First of all, let me explain again the interrupt routing of the
> > > > > loongson liointc.
> > > > > For example, the Loongson-2K1000 has 64 interrupt sources, each with
> > > > > the following 8-bit interrupt routing registers (main regs attribute
> > > > > in dts):
> > > > >
> > > > > +----+-------------------------------------------------------------------+
> > > > > | bit  | description
> > > > >             |
> > > > > +----+-------------------------------------------------------------------+
> > > > > | 3:0 | Processor core to route                                           |
> > > > > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > > > > +-----+------------------------------------------------------------------+
> > > > >
> > > > > The "loongson,parent_int_map" attribute is to describe the routed
> > > > > interrupt pins to cpuintc.
> > > > >
> > > > > However, the "interrupt-map" attribute is not supposed to be used for
> > > > > interrupt controller in the normal case. Though since commit
> > > > > 041284181226 ("of/irq: Allow matching of an interrupt-map local to an
> > > > > interrupt controller"), the "interrupt-map" attribute can be used in
> > > > > interrupt controller nodes. Some interrupt controllers were found not
> > > > > to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
> > > > > quirk for controllers with their own definition of interrupt-map"), a
> > > > > quirk was added for these interrupt controllers. As we can see from
> > > > > the commit message, this is a bad solution in itself.
> > > > >
> > > > > Similarly, if we choose to use the "interrupt-map" attribute in the
> > > > > interrupt controller, we have to use this unfriendly solution (quirk).
> > > > > Because we hope of_irq_parse_raw() stops at the liointc level rather
> > > > > than goto its parent level.
> > > > >
> > > > > So, I don't think it's a good choice to use a bad solution as a replacement.
> > > > >
> > > > > Do you have any other ideas?
> > > >
> > > > Assuming this is changeable at runtime, this sounds to me like this
> > > > mapping/routing could easily be exposed as irqchip cpu affinity. Then
> > > > userspace can apply all the performance optimizations it wants (and
> > > > can easily update them without fiddling with the kernel/dts).
> > > >
> > > > And then there would be no need to hardcode/describe it in the dts(i) at all.
> > >
> > > Hi Jonas:
> > >
> > > Thanks for your reply.
> > >
> > > It is possible that my non-detailed explanation caused your misunderstanding.
> > > Allow me to explain again about the interrupt routing register above,
> > > which we know is divided into two parts:
> > >
> > > +----+-------------------------------------------------------------------+
> > > | bit  | description |
> > > +----+-------------------------------------------------------------------+
> > > | 3:0 | Processor core to route                                           |
> > > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > > +-----+------------------------------------------------------------------+
> > >
> > > The first part "processor core" will be set to "boot_cpu_id" in the
> > > driver, which we assume is fixed and we don't need to care about it
> > > here.
> > > What we care about is the second part "mapping of device interrupts to
> > > processor interrupt pins", which is what we want to describe in
> > > dts(i).
> > >
> > > Let's take the Loongson-2K1000 as an example again, it has 64
> > > interrupt sources as inputs and 4 processor core interrupt pins as
> > > outputs.
> > > The sketch is shown below:
> > >
> > > Device Interrupts           Interrupt Pins
> > >                  +-------------+
> > >          0---->|                |--> INT0
> > >         ...       | Mapping |--> INT1
> > >         ...       |                |--> INT2
> > >         63--->|                |--> INT3
> > >                  +-------------+
> > >
> > > Therefore, this mapping relationship cannot be changed at runtime and
> > > needs to be hardcoded/described in dts(i).
> >
> > But that's only a driver/description limitation, not an actual
> > physical limitation, right? In theory you could reroute them as much
> > as you want as long as you keep the kernel up-to-date about the
> > current routing (via whatever means).
> >
> > Anyway, I guess you want to use the routed interrupt pin to identify
> > different irq controller blocks.
> >
> > Can't the interrupt pin be inferred from the parent interrupt? If your
> > parent (hw) irq is two, route everything to INT0 etc? Or alternatively
> > use the name of the parent interrupt?
> Let me make things clear and ignore those irrelevant information.
> 1, Our liointc controller has 32 inputs from downstream interrupt
> sources and 4 outputs to the parent irqchip, the "routing" here means
> which input go to which output.
> 2, We use 'parent_int_map' to describe the boot-time routing in dts
> previously, but Krzysztof suggests us to use 'interrupt-map' instead.
> 3, When we rework our driver to use 'interrupt-map', we found that
> this property is not supposed to be used in a regular irqchip (it is
> usually used in a pcie port which is also act as an irqchip).
> 4, If we still want to use 'interrupt-map' to describe the routing in
> liointc, we should make of_irq_parse_raw() stop at the liointc level
> rather than go to its parent level, because the hwirq is provided by
> liointc, not its parent. To archive this goal, we should add liointc
> to the quirk list.
> 5, So, for our liointc driver we have two choices: 1) still use the
> 'parent_int_map' property; 2) use 'interrupt-map' property and add
> liointc to the quirk list. We prefer the first ourselves, but
> Krzysztof may give us a best solution.
>
> Huacai
>
> >
> > Best Regards,
> > Jonas

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

[Marc Zyngier]

On Fri, 20 Oct 2023 10:51:35 +0100,
Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> 
> Hi Krzysztof & Marc:
> 
> Sorry for the interruption.
> As said before, we tried to use the 'interrupt-map attribute' in our
> Loongson liointc dts(i), but there are some unfriendly points.
> Do you have any other different suggestions?

I don't have any suggestion, but if you are still thinking of adding
some extra crap to the of_irq_imap_abusers[] array, the answer is a
firm 'NO'.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Huacai Chen]

Hi, Krzysztof,

On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 20 Oct 2023 10:51:35 +0100,
> Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> >
> > Hi Krzysztof & Marc:
> >
> > Sorry for the interruption.
> > As said before, we tried to use the 'interrupt-map attribute' in our
> > Loongson liointc dts(i), but there are some unfriendly points.
> > Do you have any other different suggestions?
>
> I don't have any suggestion, but if you are still thinking of adding
> some extra crap to the of_irq_imap_abusers[] array, the answer is a
> firm 'NO'.
Excuse me, but as described before, 'interrupt-map' cannot be used for
liointc unless adding it to of_irq_imap_abusers[], can we still use
'parent_int_map' in this case? Or just change it to 'parent-int-map'
to satisfy the naming style?

Huacai

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

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

[Krzysztof Kozlowski]

On 25/10/2023 03:56, Huacai Chen wrote:
> Hi, Krzysztof,
> 
> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Fri, 20 Oct 2023 10:51:35 +0100,
>> Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
>>>
>>> Hi Krzysztof & Marc:
>>>
>>> Sorry for the interruption.
>>> As said before, we tried to use the 'interrupt-map attribute' in our
>>> Loongson liointc dts(i), but there are some unfriendly points.
>>> Do you have any other different suggestions?
>>
>> I don't have any suggestion, but if you are still thinking of adding
>> some extra crap to the of_irq_imap_abusers[] array, the answer is a
>> firm 'NO'.
> Excuse me, but as described before, 'interrupt-map' cannot be used for
> liointc unless adding it to of_irq_imap_abusers[], can we still use
> 'parent_int_map' in this case? Or just change it to 'parent-int-map'
> to satisfy the naming style?

Why do you respond to me? You received firm 'NO' about
of_irq_imap_abusers, so how adhering to naming style or violating naming
style has anything to do with it?

Best regards,
Krzysztof

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

[Huacai Chen]

Hi, Krzysztof

On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/10/2023 03:56, Huacai Chen wrote:
> > Hi, Krzysztof,
> >
> > On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On Fri, 20 Oct 2023 10:51:35 +0100,
> >> Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> >>>
> >>> Hi Krzysztof & Marc:
> >>>
> >>> Sorry for the interruption.
> >>> As said before, we tried to use the 'interrupt-map attribute' in our
> >>> Loongson liointc dts(i), but there are some unfriendly points.
> >>> Do you have any other different suggestions?
> >>
> >> I don't have any suggestion, but if you are still thinking of adding
> >> some extra crap to the of_irq_imap_abusers[] array, the answer is a
> >> firm 'NO'.
> > Excuse me, but as described before, 'interrupt-map' cannot be used for
> > liointc unless adding it to of_irq_imap_abusers[], can we still use
> > 'parent_int_map' in this case? Or just change it to 'parent-int-map'
> > to satisfy the naming style?
>
> Why do you respond to me? You received firm 'NO' about
> of_irq_imap_abusers, so how adhering to naming style or violating naming
> style has anything to do with it?
I'm sorry but of_irq_imap_abusers is to make 'interrupt-map' to work,
without of_irq_imap_abusers we can only use the existing
'parent_int_map'. We need your response because we want to know
whether you can accept the existing method since the other approach
has received 'NO'. And, changing 'parent_int_map' to 'parent-int-map'
can be a little better, at least it satisfies the naming style.

Huacai
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 26/10/2023 09:19, Huacai Chen wrote:
> Hi, Krzysztof
> 
> On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 25/10/2023 03:56, Huacai Chen wrote:
>>> Hi, Krzysztof,
>>>
>>> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>
>>>> On Fri, 20 Oct 2023 10:51:35 +0100,
>>>> Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
>>>>>
>>>>> Hi Krzysztof & Marc:
>>>>>
>>>>> Sorry for the interruption.
>>>>> As said before, we tried to use the 'interrupt-map attribute' in our
>>>>> Loongson liointc dts(i), but there are some unfriendly points.
>>>>> Do you have any other different suggestions?
>>>>
>>>> I don't have any suggestion, but if you are still thinking of adding
>>>> some extra crap to the of_irq_imap_abusers[] array, the answer is a
>>>> firm 'NO'.
>>> Excuse me, but as described before, 'interrupt-map' cannot be used for
>>> liointc unless adding it to of_irq_imap_abusers[], can we still use
>>> 'parent_int_map' in this case? Or just change it to 'parent-int-map'
>>> to satisfy the naming style?
>>
>> Why do you respond to me? You received firm 'NO' about
>> of_irq_imap_abusers, so how adhering to naming style or violating naming
>> style has anything to do with it?
> I'm sorry but of_irq_imap_abusers is to make 'interrupt-map' to work,
> without of_irq_imap_abusers we can only use the existing
> 'parent_int_map'. We need your response because we want to know
> whether you can accept the existing method since the other approach
> has received 'NO'. And, changing 'parent_int_map' to 'parent-int-map'
> can be a little better, at least it satisfies the naming style.

Indeed, interrupt-map might not fit here. I don't know whether your
custom property - purely for runtime performance purpose - will be
accepted. Initial description of this field suggested that it is OS
policy, not hardware choice. But sure, propose something with
justification, so we can review it. The proposal must not break ABI, so
you must support both parent_int_map and parent-int-map (or whatever we
call it) properties. The first we will probably deprecate.

The way this property was sneaked into kernel bypassing review is still
disappointing.

Best regards,
Krzysztof

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

[Binbin Zhou]

On Sun, Oct 29, 2023 at 1:42 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/10/2023 09:19, Huacai Chen wrote:
> > Hi, Krzysztof
> >
> > On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 25/10/2023 03:56, Huacai Chen wrote:
> >>> Hi, Krzysztof,
> >>>
> >>> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote:
> >>>>
> >>>> On Fri, 20 Oct 2023 10:51:35 +0100,
> >>>> Binbin Zhou <zhoubb.aaron@gmail.com> wrote:
> >>>>>
> >>>>> Hi Krzysztof & Marc:
> >>>>>
> >>>>> Sorry for the interruption.
> >>>>> As said before, we tried to use the 'interrupt-map attribute' in our
> >>>>> Loongson liointc dts(i), but there are some unfriendly points.
> >>>>> Do you have any other different suggestions?
> >>>>
> >>>> I don't have any suggestion, but if you are still thinking of adding
> >>>> some extra crap to the of_irq_imap_abusers[] array, the answer is a
> >>>> firm 'NO'.
> >>> Excuse me, but as described before, 'interrupt-map' cannot be used for
> >>> liointc unless adding it to of_irq_imap_abusers[], can we still use
> >>> 'parent_int_map' in this case? Or just change it to 'parent-int-map'
> >>> to satisfy the naming style?
> >>
> >> Why do you respond to me? You received firm 'NO' about
> >> of_irq_imap_abusers, so how adhering to naming style or violating naming
> >> style has anything to do with it?
> > I'm sorry but of_irq_imap_abusers is to make 'interrupt-map' to work,
> > without of_irq_imap_abusers we can only use the existing
> > 'parent_int_map'. We need your response because we want to know
> > whether you can accept the existing method since the other approach
> > has received 'NO'. And, changing 'parent_int_map' to 'parent-int-map'
> > can be a little better, at least it satisfies the naming style.
>
> Indeed, interrupt-map might not fit here. I don't know whether your
> custom property - purely for runtime performance purpose - will be
> accepted. Initial description of this field suggested that it is OS
> policy, not hardware choice. But sure, propose something with
> justification, so we can review it. The proposal must not break ABI, so
> you must support both parent_int_map and parent-int-map (or whatever we
> call it) properties. The first we will probably deprecate.
>
Hi Krzysztof:

Thanks a lot for your reply and suggestion!
I'll try to split the change points into separate patches in the next
version, it might be better understood.

Thanks.
Binbin

> The way this property was sneaked into kernel bypassing review is still
> disappointing.
>
> Best regards,
> Krzysztof
>