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

[Binbin Zhou]

On Thu, Aug 17, 2023 at 4:51 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> 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.

Hi, Conor:

Whoops, it looks like my description is still not clear enough.

First of all, a chip has only one physical liointc.

Unlike other chips, Loongson-2K1000/2K0500 has 64 interrupt sources,
which requires two sets of registers to set the routing of all the
interrupts, and correspondingly two nodes to describe them in dts.
Concretely, in Loongson-2K1000/2K0500:
     liointc0: indicates the routing of interrupt sources 0-31;
     liointc1: indicates the routing of interrupt sources 32-63.

Others, such as Loongson-3A, have only 32 interrupt sources and need
only one liointc node.

For better understanding, I'll add a more detailed description, which
should especially point out the 2K1000/2K0500 differences.
For example, for reg-names description:
main: interrupt routing registers
isr0: low/high 32-bit interrupt status routed to CORE0.
isr1: low/high 32-bit interrupt status routed to CORE1.

Thanks.
Binbin

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