Re: [PATCH 06/12] riscv: dts: allwinner: Add the D1 SoC base devicetree

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

Hi Samuel,

On Fri, Sep 9, 2022 at 5:42 AM Samuel Holland <samuel@sholland.org> wrote:
> On 8/22/22 10:29 AM, Jessica Clarke wrote:
> > On 22 Aug 2022, at 14:56, conor.dooley@microchip.com wrote:
> >> On 22/08/2022 13:31, Geert Uytterhoeven wrote:
> >>>> Do you think this is worth doing? Or are you just providing an
> >>>> example of what could be done?
> >>>
> >>> Just some brainstorming...
> >>>
> >>>> Where would you envisage putting these macros? I forget the order
> >>>> of the CPP operations that are done, can they be put in the dts?
> >>>
> >>> The SOC_PERIPHERAL_IRQ() macro should be defined in the
> >>> ARM-based SoC.dtsi file and the RISC-V-based SoC.dtsi file.
> >>
> >> Right, one level up but ~the same result.
> >>
> >>>>> Nice! But it's gonna be a very large interrupt-map.
> >>>>
> >>>> I quite like the idea of not duplicating files across the archs
> >>>> if it can be helped, but not at the expense of making them hard to
> >>>> understand & I feel like unfortunately the large interrupt map is
> >>>> in that territory.
> >>>
> >>> I feel the same.
> >>> Even listing both interrupt numbers in SOC_PERIPHERAL_IRQ(na, nr)
> >>> is a risk for making mistakes.
> >>>
> >>> So personally, I'm in favor of teaching dtc arithmetic, so we can
> >>> handle the offset in SOC_PERIPHERAL_IRQ().
> >>
> >> Yup, in the same boat here. mayb I'll get bored enough to bite..
> >
> > Note that GPL’ed dtc isn’t the only implementation. FreeBSD uses a
> > BSD-licensed implementation[1] and so adding new features like this to
> > GPL dtc that actually get used would require us to reimplement it too.
> > I don’t know how much effort it would be but please keep this in mind.
>
> I plan to go with the "SOC_PERIPHERAL_IRQ(na, nr)" implementation for v2. I like
> that it only affects the DT source, and does not leak into the DTB. We still
> have the freedom to switch to using arithmetic later when all of the tools
> support it.

May I suggest an alternative solution, which would be more generic,
and can be extended to other/more CPU cores easily:

Specify both interrupts in the .dtsi, but wrapped inside e.g. ARM()
resp. RISCV() macros:

    ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
    RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)

The same construct can be used for e.g. interrupt-parent.
The ARM .dts would define:

    #define ARM(x...)    x
    #define RISCV(x....)

before including the .dtsi.
The RISC-V DTS would define instead:

    #define ARM(x...)
    #define RISCV(x...)    x

Cfr. the AR_CLASS(), M_CLASS(), ARM(), and THUMB() macros in
arch/arm/include/asm/unified.h.

Note that this is not exactly a new issue.  It's fairly common for
SoCs to have separate application and real-time cores, and on-SoC
devices may be wired to multiple interrupt controllers serving
the AP resp. RT cores. If both core sets are ARM, both interrupt
controllers may be GIC, and the problem may be less severe and just
need a different #interrupt-parent.
But in case of ARM vs. RISC-V, or different wirings from devices
to interrupt controllers, you do need completely different interrupts
properties.

> My other concern is that the big interrupt-map property would make DT overlays
> even more painful to deal with. I don't think overlays can append to a property,
> only replace it.

Indeed, overlays can only add/replace/delete properties.
Same for plain DTS files, BTW.
But I agree having /append-property/ would be useful in general.


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