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

[Conor Dooley]

On Thu, Aug 17, 2023 at 10:56:37AM +0800, Binbin Zhou wrote:
> 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.

Ah, I just assumed, given the timing, that it was for the loongson
stuff only.

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

Yeah, that should probably be made required so.

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

Did you split it in two because of register definitions, or because
there are physically two controllers on the SoC? Your comments earlier
sound like there are physically two interrupt controllers, which would
be a valid reason to split the nodes.

Thanks,
Conor.

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

Attachment: signature.asc
Description: PGP signature