Re: [PATCH] dt-bindings: interrupt-controller: loongson,liointc: Fix warnings about reg and interrupt description

From: Binbin Zhou
Date: Wed Aug 16 2023 - 22:57:40 EST

Hi Conor:

Thanks for your reply.

On Tue, Aug 15, 2023 at 10:20 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> Hey,
>
> On Tue, Aug 15, 2023 at 04:47:13PM +0800, Binbin Zhou wrote:
> > As we know, some Loongson-2K CPUs are single-core, e.g. Loongson-2K0500,
> > and the "isr1" means routing interrupts to core1, which should be
> > optional. So add maxItems/minItems limits to reg/reg-names.
> > Also, The interrupt-names attribute represents a list of parent
> > interrupt names that should change with interrupts.
>
> This should have been with the other series that introduces the users
> probably so that things make more sense to the reader.

I was under the impression that the mips Loongson-2K1000 was also
required for this patch, so I committed it separately.
Maybe my commit should still be described in more detail.

>
> > Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > ---
> > .../interrupt-controller/loongson,liointc.yaml | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> > index 00b570c82903..adb428211a72 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> > @@ -11,7 +11,7 @@ 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:
> > @@ -33,6 +33,7 @@ properties:
> > - const: main
> > - const: isr0
> > - const: isr1
> > + minItems: 2
> >
> > interrupt-controller: true
> >
> > @@ -45,11 +46,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]
>
> From a quick look at the new devicetrees, I don't understand the
> ordering relaxation. Do you actually have a system that only has, for
> example, int3?

For a better understanding, allow me to first explain the composition
of the interrupt routing register:
It is an 8 bit register that is divided into two parts:
0-3 : The processor core vector number of the route, this part is
handled in the code.
4-7 : The processor core interrupt pin vector number for routing, i.e.
int0-int3.
Each intx can handle 32 interrupt sources.

For example, in Loongson-2K1000/Loongson-2K0500, there are a total of
64 interrupt sources, and we need to route them to two intx.

We don't mandate which interrupt vector number must be used, in our
practice the tendency is to start with int0.
It is worth noting that we must follow the following correspondence:
interrupt->interrupt-names
2->int0
3->int1
4->int2
5->int3

>
> Also, as the interrupt-names are not required, changing the ordering
> here is not ABI compatible AFAICT. Does that have any fallout?

Oh, this should be another point that needs to be modified, the
interrupt-names should be required. because in the driver code the
parent interrupts are fetched through of_irq_get_byname().

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n345

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;

Also the way liointc-2.0 is written in the dts does not match the dt-binding.
The dts using loongson,liointc-2.0 are:
arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi (mips Loongson-2K1000)
arch/loongarch/boot/dts/loongson-2k0500.dtsi
arch/loongarch/boot/dts/loongson-2k1000.dtsi
arch/loongarch/boot/dts/loongson-2k2000.dtsi

liointc0: interrupt-controller@1fe01400 {
......
interrupts = <2>;
interrupt-names = "int0";
loongson,parent_int_map = <0xffffffff>, /* int0 */
<0x00000000>, /* int1 */
<0x00000000>, /* int2 */
<0x00000000>; /* int3 */
};

liointc1: interrupt-controller@1fe01440 {
....
interrupts = <3>;
interrupt-names = "int1";
loongson,parent_int_map = <0x00000000>, /* int0 */
<0xffffffff>, /* int1 */
<0x00000000>, /* int2 */
<0x00000000>; /* int3 */
};

We split the two intx into two nodes because of register definitions
etc. There is the following WARNING at liointc1:

arch/loongarch/boot/dts/loongson-2k1000-ref.dtb:
interrupt-controller@1fe01440: interrupt-names:0: 'int0' was expected
From schema:
Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
arch/loongarch/boot/dts/loongson-2k1000-ref.dtb:
interrupt-controller@1fe01440: Unevaluated properties are not allowed
('interrupt-names' was unexpected)
From schema:
Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml

But actually, in liointc1, we only need int1.

Thanks.
Binbin

>
> Thanks,
> Conor.
>
> > minItems: 1
> > + maxItems: 4
> >
> > '#interrupt-cells':
> > const: 2
> > @@ -73,7 +72,6 @@ required:
> > - '#interrupt-cells'
> > - loongson,parent_int_map
> >
> > -
> > unevaluatedProperties: false
> >
> > if:
> > @@ -86,7 +84,8 @@ if:
> > then:
> > properties:
> > reg:
> > - minItems: 3
> > + minItems: 2
> > + maxItems: 3
> >
> > required:
> > - reg-names
> > @@ -113,7 +112,6 @@ examples:
> > <0x0f000000>, /* int1 */
> > <0x00000000>, /* int2 */
> > <0x00000000>; /* int3 */
> > -
> > };
> >
> > ...
> > --
> > 2.39.3
> >