Hi Rob, Krzysztof,
This is a topic that came up at the RISC-V BoF at Plumbers, and it was
suggested to bring it up with you.
The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
To avoid duplication, we would like to have:
- <riscv-soc>.dtsi includes <base-soc>.dtsi,
- <arm-soc>.dtsi includes <base-soc>.dtsi.
Unfortunately RISC-V and ARM typically use different types of interrupt
controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
possibly using different interrupt numbers. Hence the interrupt-parent
and interrupts{-extended} properties should be different, too.
Possible solutions[1]:
1. interrupt-map
2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
<base-soc>.dtsi, with
- #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
- #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
Note that the cpp/dtc combo does not support arithmetic, so even
the simple case where nr = 32 + na cannot be simplified.
3. Wrap inside RISCV() and ARM() macros, e.g.:
RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)
ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
Cfr. ARM() and THUMB() in arch/arm/include/asm/unified.h, as used
to express the same operation using plain ARM or ARM Thumb
instructions.
Personally, I'm leaning towards the third solution, as it is the most
flexible, and allows us to extend to more than 2 interrupt controllers.
Note that this is actually not a new issue. For years, ARM SoCs have
existed with multiple types of cores on the same die, using Cortex-A
cores for the application, and Cortex-R/SuperH/V850/... cores for
real-time and/or baseband operation. So far this wasn't an issue, as
only the Cortex-A cores ran Linux, and we ignored the other cores (and
the related interrupt controllers and hierarchy) in DT.
What do you think?
Thanks for your comments!
[1] https://lore.kernel.org/lkml/[email protected]
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 21/09/2022 09:46, Geert Uytterhoeven wrote:
> Hi Rob, Krzysztof,
>
> This is a topic that came up at the RISC-V BoF at Plumbers, and it was
> suggested to bring it up with you.
I guess you also need SoC maintainers as well (+Cc Arnd and Olof). :)
>
> The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
> cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
> To avoid duplication, we would like to have:
> - <riscv-soc>.dtsi includes <base-soc>.dtsi,
> - <arm-soc>.dtsi includes <base-soc>.dtsi.
>
> Unfortunately RISC-V and ARM typically use different types of interrupt
> controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
> possibly using different interrupt numbers. Hence the interrupt-parent
> and interrupts{-extended} properties should be different, too.
>
> Possible solutions[1]:
> 1. interrupt-map
>
> 2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
> <base-soc>.dtsi, with
> - #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
> - #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
> Note that the cpp/dtc combo does not support arithmetic, so even
> the simple case where nr = 32 + na cannot be simplified.
What do you mean? Macros support string concatenation and simple
arithmetic like adding numbers. I just tested it.
>
> 3. Wrap inside RISCV() and ARM() macros, e.g.:
>
> RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)
> ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
>
> Cfr. ARM() and THUMB() in arch/arm/include/asm/unified.h, as used
> to express the same operation using plain ARM or ARM Thumb
> instructions.
>
> Personally, I'm leaning towards the third solution, as it is the most
> flexible, and allows us to extend to more than 2 interrupt controllers.
>
> Note that this is actually not a new issue. For years, ARM SoCs have
> existed with multiple types of cores on the same die, using Cortex-A
> cores for the application, and Cortex-R/SuperH/V850/... cores for
> real-time and/or baseband operation. So far this wasn't an issue, as
> only the Cortex-A cores ran Linux, and we ignored the other cores (and
> the related interrupt controllers and hierarchy) in DT.
>
> What do you think?
> Thanks for your comments!
If it is doable with a macro (option 2), I would vote for this. Assuming
of course that the interrupts differ only by GIC_SPI/PPI and base
number. I guess this should be the case if this is the same SoC?
Best regards,
Krzysztof
On 2022-09-21 08:46, Geert Uytterhoeven wrote:
> Hi Rob, Krzysztof,
>
> This is a topic that came up at the RISC-V BoF at Plumbers, and it was
> suggested to bring it up with you.
>
> The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
> cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
> To avoid duplication, we would like to have:
> - <riscv-soc>.dtsi includes <base-soc>.dtsi,
> - <arm-soc>.dtsi includes <base-soc>.dtsi.
>
> Unfortunately RISC-V and ARM typically use different types of interrupt
> controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
> possibly using different interrupt numbers. Hence the interrupt-parent
> and interrupts{-extended} properties should be different, too.
>
> Possible solutions[1]:
> 1. interrupt-map
>
> 2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
> <base-soc>.dtsi, with
> - #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
> - #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
> Note that the cpp/dtc combo does not support arithmetic, so even
> the simple case where nr = 32 + na cannot be simplified.
>
> 3. Wrap inside RISCV() and ARM() macros, e.g.:
>
> RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)
> ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
>
> Cfr. ARM() and THUMB() in arch/arm/include/asm/unified.h, as used
> to express the same operation using plain ARM or ARM Thumb
> instructions.
4. Put all the "interrupts" properties in the SoC-specific DTSI at the
same level as the interrupt controller to which they correspond. Works
out of the box with no horrible mystery macros, and is really no more or
less error-prone than any other approach. Yes, it means replicating a
bit of structure and/or having labels for everything (many of which may
be wanted anyway), but that's not necessarily a bad thing for
readability anyway. Hierarchical definitions are standard FDT practice
and should be well understood, so this is arguably the simplest and
least surprising approach :)
Cheers,
Robin.
> Personally, I'm leaning towards the third solution, as it is the most
> flexible, and allows us to extend to more than 2 interrupt controllers.
>
> Note that this is actually not a new issue. For years, ARM SoCs have
> existed with multiple types of cores on the same die, using Cortex-A
> cores for the application, and Cortex-R/SuperH/V850/... cores for
> real-time and/or baseband operation. So far this wasn't an issue, as
> only the Cortex-A cores ran Linux, and we ignored the other cores (and
> the related interrupt controllers and hierarchy) in DT.
>
> What do you think?
> Thanks for your comments!
>
> [1] https://lore.kernel.org/lkml/[email protected]
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 21/09/2022 11:20, Lad, Prabhakar wrote:
>>
>> What do you mean? Macros support string concatenation and simple
>> arithmetic like adding numbers. I just tested it.
>>
> I did try the below:
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> index 689aa4ba416b..0f923c276cd3 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> @@ -8,6 +8,8 @@
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/clock/r9a07g043-cpg.h>
>
> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr na
> +
> / {
> compatible = "renesas,r9a07g043";
> #address-cells = <2>;
> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> compatible = "renesas,r9a07g043-ssi",
> "renesas,rz-ssi";
> reg = <0 0x1004a000 0 0x400>;
> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
> <GIC_SPI 331 IRQ_TYPE_EDGE_RISING>,
> <GIC_SPI 332 IRQ_TYPE_EDGE_RISING>,
> <GIC_SPI 333 IRQ_TYPE_EDGE_RISING>;
>
> This worked as expected, but couldn't get the arithmetic operation
> working. Could you please provide an example?
diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
index ff6aab388eb7..0ecca775fa3f 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
@@ -8,6 +8,8 @@
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/clock/r9a07g043-cpg.h>
+#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
+#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
/ {
compatible = "renesas,r9a07g043";
#address-cells = <2>;
@@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
compatible = "renesas,r9a07g043-ssi",
"renesas,rz-ssi";
reg = <0 0x1004a000 0 0x400>;
- interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
Or any other method like that....
Best regards,
Krzysztof
Hi Krzysztof,
On Wed, Sep 21, 2022 at 9:53 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 21/09/2022 09:46, Geert Uytterhoeven wrote:
> > Hi Rob, Krzysztof,
> >
> > This is a topic that came up at the RISC-V BoF at Plumbers, and it was
> > suggested to bring it up with you.
>
> I guess you also need SoC maintainers as well (+Cc Arnd and Olof). :)
>
> >
> > The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
> > cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
> > To avoid duplication, we would like to have:
> > - <riscv-soc>.dtsi includes <base-soc>.dtsi,
> > - <arm-soc>.dtsi includes <base-soc>.dtsi.
> >
> > Unfortunately RISC-V and ARM typically use different types of interrupt
> > controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
> > possibly using different interrupt numbers. Hence the interrupt-parent
> > and interrupts{-extended} properties should be different, too.
> >
> > Possible solutions[1]:
> > 1. interrupt-map
> >
> > 2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
> > <base-soc>.dtsi, with
> > - #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
> > - #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
> > Note that the cpp/dtc combo does not support arithmetic, so even
> > the simple case where nr = 32 + na cannot be simplified.
>
> What do you mean? Macros support string concatenation and simple
> arithmetic like adding numbers. I just tested it.
>
I did try the below:
diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
index 689aa4ba416b..0f923c276cd3 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
@@ -8,6 +8,8 @@
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/clock/r9a07g043-cpg.h>
+#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr na
+
/ {
compatible = "renesas,r9a07g043";
#address-cells = <2>;
@@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
compatible = "renesas,r9a07g043-ssi",
"renesas,rz-ssi";
reg = <0 0x1004a000 0 0x400>;
- interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
<GIC_SPI 331 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 332 IRQ_TYPE_EDGE_RISING>,
<GIC_SPI 333 IRQ_TYPE_EDGE_RISING>;
This worked as expected, but couldn't get the arithmetic operation
working. Could you please provide an example?
Cheers,
Prabhakar
On Wed, Sep 21, 2022 at 10:26 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 21/09/2022 11:20, Lad, Prabhakar wrote:
> >>
> >> What do you mean? Macros support string concatenation and simple
> >> arithmetic like adding numbers. I just tested it.
> >>
> > I did try the below:
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > index 689aa4ba416b..0f923c276cd3 100644
> > --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > @@ -8,6 +8,8 @@
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/clock/r9a07g043-cpg.h>
> >
> > +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr na
> > +
> > / {
> > compatible = "renesas,r9a07g043";
> > #address-cells = <2>;
> > @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> > compatible = "renesas,r9a07g043-ssi",
> > "renesas,rz-ssi";
> > reg = <0 0x1004a000 0 0x400>;
> > - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> > + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
> > <GIC_SPI 331 IRQ_TYPE_EDGE_RISING>,
> > <GIC_SPI 332 IRQ_TYPE_EDGE_RISING>,
> > <GIC_SPI 333 IRQ_TYPE_EDGE_RISING>;
> >
> > This worked as expected, but couldn't get the arithmetic operation
> > working. Could you please provide an example?
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> index ff6aab388eb7..0ecca775fa3f 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> @@ -8,6 +8,8 @@
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/clock/r9a07g043-cpg.h>
>
> +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
> / {
> compatible = "renesas,r9a07g043";
> #address-cells = <2>;
> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> compatible = "renesas,r9a07g043-ssi",
> "renesas,rz-ssi";
> reg = <0 0x1004a000 0 0x400>;
> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
>
>
>
> Or any other method like that....
>
Thanks for the pointer! (Ive tested the above and it works)
Cheers,
Prabhakar
Hi Robin,
On Wed, Sep 21, 2022 at 11:20 AM Robin Murphy <[email protected]> wrote:
> On 2022-09-21 08:46, Geert Uytterhoeven wrote:
> > This is a topic that came up at the RISC-V BoF at Plumbers, and it was
> > suggested to bring it up with you.
> >
> > The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
> > cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
> > To avoid duplication, we would like to have:
> > - <riscv-soc>.dtsi includes <base-soc>.dtsi,
> > - <arm-soc>.dtsi includes <base-soc>.dtsi.
> >
> > Unfortunately RISC-V and ARM typically use different types of interrupt
> > controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
> > possibly using different interrupt numbers. Hence the interrupt-parent
> > and interrupts{-extended} properties should be different, too.
> >
> > Possible solutions[1]:
> > 1. interrupt-map
> >
> > 2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
> > <base-soc>.dtsi, with
> > - #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
> > - #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
> > Note that the cpp/dtc combo does not support arithmetic, so even
> > the simple case where nr = 32 + na cannot be simplified.
> >
> > 3. Wrap inside RISCV() and ARM() macros, e.g.:
> >
> > RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)
> > ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
> >
> > Cfr. ARM() and THUMB() in arch/arm/include/asm/unified.h, as used
> > to express the same operation using plain ARM or ARM Thumb
> > instructions.
>
> 4. Put all the "interrupts" properties in the SoC-specific DTSI at the
> same level as the interrupt controller to which they correspond. Works
> out of the box with no horrible mystery macros, and is really no more or
> less error-prone than any other approach. Yes, it means replicating a
> bit of structure and/or having labels for everything (many of which may
> be wanted anyway), but that's not necessarily a bad thing for
> readability anyway. Hierarchical definitions are standard FDT practice
> and should be well understood, so this is arguably the simplest and
> least surprising approach :)
Thanks for the suggestion!
It does mean we have to update 3 .dtsi files when adding support
for a new device. As long as all DT changes go through the same (soc)
tree, we can easily manage the dependencies.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 21/09/2022 12:08, Geert Uytterhoeven wrote:
>> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> index ff6aab388eb7..0ecca775fa3f 100644
>> --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> @@ -8,6 +8,8 @@
>> #include <dt-bindings/interrupt-controller/arm-gic.h>
>> #include <dt-bindings/clock/r9a07g043-cpg.h>
>>
>> +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
>> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
>
> #define SOC_PERIPHERAL_IRQ(nr, flags) GIC_SPI
> SOC_PERIPHERAL_IRQ_NUMBER(nr) flags
Right. Let's consider my code just proof-of-concept :)
Best regards,
Krzysztof
On 2022-09-21 11:17, Krzysztof Kozlowski wrote:
> On 21/09/2022 12:14, Robin Murphy wrote:
>>> +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
>>> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
>>> / {
>>> compatible = "renesas,r9a07g043";
>>> #address-cells = <2>;
>>> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
>>> compatible = "renesas,r9a07g043-ssi",
>>> "renesas,rz-ssi";
>>> reg = <0 0x1004a000 0 0x400>;
>>> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
>>> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
>>>
>>>
>>>
>>> Or any other method like that....
>>
>> Which will generate the text:
>>
>> "interrupts = <GIC_SPI 330 (IRQ_TYPE_LEVEL_HIGH + 32)>,"
>>
>> (give or take some whitespace)
>>
>> CPP supports constant expressions in #if and #elif directives, but
>> macros are purely literal text replacement. It might technically be
>> achievable with some insane CPP metaprogramming, but for all practical
>> purposes this is a non-starter unless dtc itself grows the ability to
>> process arithmetic expressions.
>
> Except I put it into flags, not to IRQ number, it works, so I am not
> sure why do you call it non-starter?
Oh, it seems dtc *does* understand arithmetic already, that's what I was
missing.
$ echo "/dts-v1/;/{foo = <(2 + 3)>;};" | dtc -Odts
/dts-v1/;
/ {
foo = <0x05>;
};
Thanks for teaching me something new!
Robin.
Hi Krzysztof,
On Wed, Sep 21, 2022 at 10:49 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 21/09/2022 09:46, Geert Uytterhoeven wrote:
> > This is a topic that came up at the RISC-V BoF at Plumbers, and it was
> > suggested to bring it up with you.
>
> I guess you also need SoC maintainers as well (+Cc Arnd and Olof). :)
Indeed, I had intended to include them, but forgot in the end.
Thanks for adding!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 21/09/2022 12:14, Robin Murphy wrote:
>> +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
>> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
>> / {
>> compatible = "renesas,r9a07g043";
>> #address-cells = <2>;
>> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
>> compatible = "renesas,r9a07g043-ssi",
>> "renesas,rz-ssi";
>> reg = <0 0x1004a000 0 0x400>;
>> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
>> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
>>
>>
>>
>> Or any other method like that....
>
> Which will generate the text:
>
> "interrupts = <GIC_SPI 330 (IRQ_TYPE_LEVEL_HIGH + 32)>,"
>
> (give or take some whitespace)
>
> CPP supports constant expressions in #if and #elif directives, but
> macros are purely literal text replacement. It might technically be
> achievable with some insane CPP metaprogramming, but for all practical
> purposes this is a non-starter unless dtc itself grows the ability to
> process arithmetic expressions.
Except I put it into flags, not to IRQ number, it works, so I am not
sure why do you call it non-starter?
Best regards,
Krzysztof
On 2022-09-21 10:26, Krzysztof Kozlowski wrote:
> On 21/09/2022 11:20, Lad, Prabhakar wrote:
>>>
>>> What do you mean? Macros support string concatenation and simple
>>> arithmetic like adding numbers. I just tested it.
>>>
>> I did try the below:
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> index 689aa4ba416b..0f923c276cd3 100644
>> --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
>> @@ -8,6 +8,8 @@
>> #include <dt-bindings/interrupt-controller/arm-gic.h>
>> #include <dt-bindings/clock/r9a07g043-cpg.h>
>>
>> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr na
>> +
>> / {
>> compatible = "renesas,r9a07g043";
>> #address-cells = <2>;
>> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
>> compatible = "renesas,r9a07g043-ssi",
>> "renesas,rz-ssi";
>> reg = <0 0x1004a000 0 0x400>;
>> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
>> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
>> <GIC_SPI 331 IRQ_TYPE_EDGE_RISING>,
>> <GIC_SPI 332 IRQ_TYPE_EDGE_RISING>,
>> <GIC_SPI 333 IRQ_TYPE_EDGE_RISING>;
>>
>> This worked as expected, but couldn't get the arithmetic operation
>> working. Could you please provide an example?
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> index ff6aab388eb7..0ecca775fa3f 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> @@ -8,6 +8,8 @@
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/clock/r9a07g043-cpg.h>
>
> +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
> / {
> compatible = "renesas,r9a07g043";
> #address-cells = <2>;
> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> compatible = "renesas,r9a07g043-ssi",
> "renesas,rz-ssi";
> reg = <0 0x1004a000 0 0x400>;
> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
>
>
>
> Or any other method like that....
Which will generate the text:
"interrupts = <GIC_SPI 330 (IRQ_TYPE_LEVEL_HIGH + 32)>,"
(give or take some whitespace)
CPP supports constant expressions in #if and #elif directives, but
macros are purely literal text replacement. It might technically be
achievable with some insane CPP metaprogramming, but for all practical
purposes this is a non-starter unless dtc itself grows the ability to
process arithmetic expressions.
Thanks,
Robin.
On 21/09/2022 12:13, Geert Uytterhoeven wrote:
>> 4. Put all the "interrupts" properties in the SoC-specific DTSI at the
>> same level as the interrupt controller to which they correspond. Works
>> out of the box with no horrible mystery macros, and is really no more or
>> less error-prone than any other approach. Yes, it means replicating a
>> bit of structure and/or having labels for everything (many of which may
>> be wanted anyway), but that's not necessarily a bad thing for
>> readability anyway. Hierarchical definitions are standard FDT practice
>> and should be well understood, so this is arguably the simplest and
>> least surprising approach :)
>
> Thanks for the suggestion!
>
> It does mean we have to update 3 .dtsi files when adding support
> for a new device. As long as all DT changes go through the same (soc)
> tree, we can easily manage the dependencies.
If the new nodes are disabled (in main shared DTSI), then it would not
need immediate update in other arch. However enabling it in other arch
would require cross-tree pull (AFAIR, RISC-V changes do not go to SoC tree).
Best regards,
Krzysztof
Hi Krzysztof,
On Wed, Sep 21, 2022 at 11:26 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 21/09/2022 11:20, Lad, Prabhakar wrote:
> >> What do you mean? Macros support string concatenation and simple
> >> arithmetic like adding numbers. I just tested it.
> >>
> > I did try the below:
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > index 689aa4ba416b..0f923c276cd3 100644
> > --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > @@ -8,6 +8,8 @@
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/clock/r9a07g043-cpg.h>
> >
> > +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr na
> > +
> > / {
> > compatible = "renesas,r9a07g043";
> > #address-cells = <2>;
> > @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> > compatible = "renesas,r9a07g043-ssi",
> > "renesas,rz-ssi";
> > reg = <0 0x1004a000 0 0x400>;
> > - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> > + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
> > <GIC_SPI 331 IRQ_TYPE_EDGE_RISING>,
> > <GIC_SPI 332 IRQ_TYPE_EDGE_RISING>,
> > <GIC_SPI 333 IRQ_TYPE_EDGE_RISING>;
> >
> > This worked as expected, but couldn't get the arithmetic operation
> > working. Could you please provide an example?
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> index ff6aab388eb7..0ecca775fa3f 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> @@ -8,6 +8,8 @@
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/clock/r9a07g043-cpg.h>
>
> +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
> +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
#define SOC_PERIPHERAL_IRQ(nr, flags) GIC_SPI
SOC_PERIPHERAL_IRQ_NUMBER(nr) flags
> / {
> compatible = "renesas,r9a07g043";
> #address-cells = <2>;
> @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> compatible = "renesas,r9a07g043-ssi",
> "renesas,rz-ssi";
> reg = <0 0x1004a000 0 0x400>;
> - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
>
>
>
> Or any other method like that....
Oh cool, seems like arithmetic is supported.
No idea what I did wrong last time I tried...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, Sep 21, 2022 at 12:08:11PM +0200, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> > > This worked as expected, but couldn't get the arithmetic operation
> > > working. Could you please provide an example?
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > index ff6aab388eb7..0ecca775fa3f 100644
> > --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
> > @@ -8,6 +8,8 @@
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/clock/r9a07g043-cpg.h>
> >
> > +#define SOC_PERIPHERAL_IRQ_NUMBER(na) (na + 32)
> > +#define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI nr SOC_PERIPHERAL_IRQ_NUMBER(na)
>
> #define SOC_PERIPHERAL_IRQ(nr, flags) GIC_SPI
> SOC_PERIPHERAL_IRQ_NUMBER(nr) flags
>
> > / {
> > compatible = "renesas,r9a07g043";
> > #address-cells = <2>;
> > @@ -128,7 +130,7 @@ ssi1: ssi@1004a000 {
> > compatible = "renesas,r9a07g043-ssi",
> > "renesas,rz-ssi";
> > reg = <0 0x1004a000 0 0x400>;
> > - interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> > + interrupts = <SOC_PERIPHERAL_IRQ(330, IRQ_TYPE_LEVEL_HIGH)>,
> >
> >
> >
> > Or any other method like that....
>
> Oh cool, seems like arithmetic is supported.
> No idea what I did wrong last time I tried...
Oh sick, it does actually work??? So, am I missing something or is this
sort of approach now a no-brainer?
On Wed, Sep 21, 2022, at 11:20 AM, Robin Murphy wrote:
> On 2022-09-21 08:46, Geert Uytterhoeven wrote:
>> Hi Rob, Krzysztof,
>>
>> This is a topic that came up at the RISC-V BoF at Plumbers, and it was
>> suggested to bring it up with you.
>>
>> The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
>> cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
>> To avoid duplication, we would like to have:
>> - <riscv-soc>.dtsi includes <base-soc>.dtsi,
>> - <arm-soc>.dtsi includes <base-soc>.dtsi.
>>
>> Unfortunately RISC-V and ARM typically use different types of interrupt
>> controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
>> possibly using different interrupt numbers. Hence the interrupt-parent
>> and interrupts{-extended} properties should be different, too.
>>
>> Possible solutions[1]:
>> 1. interrupt-map
>>
>> 2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
>> <base-soc>.dtsi, with
>> - #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
>> - #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
>> Note that the cpp/dtc combo does not support arithmetic, so even
>> the simple case where nr = 32 + na cannot be simplified.
>>
>> 3. Wrap inside RISCV() and ARM() macros, e.g.:
>>
>> RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)
>> ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
>>
>> Cfr. ARM() and THUMB() in arch/arm/include/asm/unified.h, as used
>> to express the same operation using plain ARM or ARM Thumb
>> instructions.
>
> 4. Put all the "interrupts" properties in the SoC-specific DTSI at the
> same level as the interrupt controller to which they correspond. Works
> out of the box with no horrible mystery macros, and is really no more or
> less error-prone than any other approach. Yes, it means replicating a
> bit of structure and/or having labels for everything (many of which may
> be wanted anyway), but that's not necessarily a bad thing for
> readability anyway. Hierarchical definitions are standard FDT practice
> and should be well understood, so this is arguably the simplest and
> least surprising approach :)
FWIW, approaches 1, 2 and 4 all seem reasonable to me, but I don't
like number 3 if this is only about the IRQ definitions.
It sounds like we're already converging on #2, so just one more
idea from me: we could fold the IRQ type into the macro, and
make it just take a single argument for extra flexibility:
#define SOC_PERIPHERAL_IRQ_LEVEL_HIGH(nr) \
GIC_SPI (nr + offset) IRQ_TYPE_LEVEL_HIGH
If all the irqs on the chip have the same type, the name
can be shorter of course.
Either way, some variation of the macro sounds like a good enough
approach to me.
Arnd
Hi Arnd,
On Thu, Sep 22, 2022 at 8:30 AM Arnd Bergmann <[email protected]> wrote:
> On Wed, Sep 21, 2022, at 11:20 AM, Robin Murphy wrote:
> > On 2022-09-21 08:46, Geert Uytterhoeven wrote:
> >> This is a topic that came up at the RISC-V BoF at Plumbers, and it was
> >> suggested to bring it up with you.
> >>
> >> The same SoC may be available with either RISC-V or other (e.g. ARM) CPU
> >> cores (an example of this are the Renesas RZ/Five and RZ/G2UL SoCs).
> >> To avoid duplication, we would like to have:
> >> - <riscv-soc>.dtsi includes <base-soc>.dtsi,
> >> - <arm-soc>.dtsi includes <base-soc>.dtsi.
> >>
> >> Unfortunately RISC-V and ARM typically use different types of interrupt
> >> controllers, using different bindings (e.g. 2-cell vs. 3-cell), and
> >> possibly using different interrupt numbers. Hence the interrupt-parent
> >> and interrupts{-extended} properties should be different, too.
> >>
> >> Possible solutions[1]:
> >> 1. interrupt-map
> >>
> >> 2. Use a SOC_PERIPHERAL_IRQ() macro in interrupts properties in
> >> <base-soc>.dtsi, with
> >> - #define SOC_PERIPHERAL_IRQ(nr, na) nr // RISC-V
> >> - #define SOC_PERIPHERAL_IRQ(nr, na) GIC_SPI na // ARM
> >> Note that the cpp/dtc combo does not support arithmetic, so even
> >> the simple case where nr = 32 + na cannot be simplified.
> >>
> >> 3. Wrap inside RISCV() and ARM() macros, e.g.:
> >>
> >> RISCV(interrupts = <412 IRQ_TYPE_LEVEL_HIGH>;)
> >> ARM(interrupts = <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>;)
> >>
> >> Cfr. ARM() and THUMB() in arch/arm/include/asm/unified.h, as used
> >> to express the same operation using plain ARM or ARM Thumb
> >> instructions.
> >
> > 4. Put all the "interrupts" properties in the SoC-specific DTSI at the
> > same level as the interrupt controller to which they correspond. Works
> > out of the box with no horrible mystery macros, and is really no more or
> > less error-prone than any other approach. Yes, it means replicating a
> > bit of structure and/or having labels for everything (many of which may
> > be wanted anyway), but that's not necessarily a bad thing for
> > readability anyway. Hierarchical definitions are standard FDT practice
> > and should be well understood, so this is arguably the simplest and
> > least surprising approach :)
>
> FWIW, approaches 1, 2 and 4 all seem reasonable to me, but I don't
> like number 3 if this is only about the IRQ definitions.
We also have to handle interrupt-parent at the /soc level.
And of course you never know what pops up next ;-)
> It sounds like we're already converging on #2, so just one more
> idea from me: we could fold the IRQ type into the macro, and
> make it just take a single argument for extra flexibility:
>
> #define SOC_PERIPHERAL_IRQ_LEVEL_HIGH(nr) \
> GIC_SPI (nr + offset) IRQ_TYPE_LEVEL_HIGH
>
> If all the irqs on the chip have the same type, the name
> can be shorter of course.
This is usually the case, but not always.
And the numbering may be the same (modulo the offset), but
not it really depends on the on-SoC wiring.
> Either way, some variation of the macro sounds like a good enough
> approach to me.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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