Re: [PATCH dt-schema] Fix interrupt controllers with interrupt-map

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Rob,

Cc Rasmus

On Fri, May 29, 2020 at 11:44 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, May 29, 2020 at 10:02 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Fri, May 29, 2020 at 5:54 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > On Thu, May 28, 2020 at 7:23 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > > > When an interrupt controller has an "interrupt-map" property, an "is
> > > > valid under each of" error is triggered.
> > > >
> > > > Fix this by allowing "interrupt-controller" and "interrupt-map" to
> > > > coexist, in both the interrrupts meta-schema and the
> > > > interrupt-controller schema.
> > >
> > > But both should not be present. If 'interrupt-controller' is present,
> >
> > Why not?
>
> Well, maybe I'm wrong. If you have more than just transparent
> remapping (i.e. mask/unmask/clear), then perhaps both are appropriate
> because you want get the irq domain for the first irq parent.

Indeed, that's the case for rza1-irqc.

> > > the Linux irq parsing code will ignore 'interrupt-map'. Seems like
> > > that's backwards, but this parsing code is older than dirt and we'd
> > > probably break some 1990s machine changing it.
> >
> > That's fine.  rza1_irqc_parse_map() parses the interrupt-map itself,
> > to map from downstream to upstream interrupts.
>
> You shouldn't really be parsing interrupt-map yourself. The code there

Of course I had tried that before[1];

|    I also considered extracting the parsing code in of_irq_parse_raw() in a
|    new public helper function:
|
|        int of_irq_parse_map(struct device_node **ipar, u32 *addrsize,
|                             u32 *intsize, const __be32 **match_array,
|                             struct of_phandle_args *out_irq);
|
|    However, that API is a bit ugly due to many output parameters
|    (of_irq_parse_raw() needs to iterate the interrupt hierarchy).
|    In addition, it's less efficient for irq-renesas-rza1.c, as the
|    interrupt-map must be parsed 8 times.

Or is there a better way?

In the mean time, drivers/irqchip/irq-ls-extirq.c has landed, which was
mimiced after irq-renesas-rza1.c.

> doesn't account for #address-cells which can be a factor for
> interrupt-map. dtc is gaining some checks for 'interrupt-map', so
> let's hope you have it right.

The driver code indeed doesn't account for #address-cells, but the
binding says it must be zero anyway.

[1] "[PATCH v4 2/2] irqchip: Add Renesas RZ/A1 Interrupt Controller driver"
    http://lore.kernel.org/r/20190527121711.5138-3-geert+renesas@glider.be

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds