| Message ID | 20200505165212.76466-8-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dts/dt-bindings: Fix Arm Ltd. ARMv8 "boards" | expand |
On Tue, 05 May 2020 17:52:03 +0100, Andre Przywara <andre.przywara@arm.com> wrote: > > The GIC DT binding only allows a certain combination of DT compatible > strings, mostly just consisting of one name. > > Drop the combination of multiple names and go with the > "arm,cortex-a15-gic" name for GICv2, as this seems to be the most widely > accepted string. "arm,gic-400" would be more correct, but was introduced > much later into the kernel's GIC driver. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi | 2 +- > arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +- > arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi > index 15fe81738e94..61a1750fcdd6 100644 > --- a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi > +++ b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi > @@ -6,7 +6,7 @@ > > / { > gic: interrupt-controller@2c001000 { > - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; > + compatible = "arm,cortex-a15-gic"; > #interrupt-cells = <3>; > #address-cells = <2>; > interrupt-controller; > diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi > index 3feefd61eb76..62392ab1f880 100644 > --- a/arch/arm64/boot/dts/arm/juno-base.dtsi > +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi > @@ -69,7 +69,7 @@ > }; > > gic: interrupt-controller@2c010000 { > - compatible = "arm,gic-400", "arm,cortex-a15-gic"; > + compatible = "arm,cortex-a15-gic"; Why? GIC-400 is definitely the most correct compatible string. I'd rather see this compatible being generalised to the models rather than only referencing the A15 GIC. > reg = <0x0 0x2c010000 0 0x1000>, > <0x0 0x2c02f000 0 0x2000>, > <0x0 0x2c04f000 0 0x2000>, > diff --git a/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts b/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts > index c5d15cbd8cf6..f86f6451411f 100644 > --- a/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts > +++ b/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts > @@ -95,7 +95,7 @@ > }; > > gic: interrupt-controller@2c001000 { > - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; > + compatible = "arm,cortex-a15-gic"; > #interrupt-cells = <3>; > #address-cells = <0>; > interrupt-controller; > -- > 2.17.1 > > Thanks, M.
On 05/05/2020 19:25, Marc Zyngier wrote: > On Tue, 05 May 2020 17:52:03 +0100, > Andre Przywara <andre.przywara@arm.com> wrote: >> >> The GIC DT binding only allows a certain combination of DT compatible >> strings, mostly just consisting of one name. >> >> Drop the combination of multiple names and go with the >> "arm,cortex-a15-gic" name for GICv2, as this seems to be the most widely >> accepted string. "arm,gic-400" would be more correct, but was introduced >> much later into the kernel's GIC driver. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi | 2 +- >> arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +- >> arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >> index 15fe81738e94..61a1750fcdd6 100644 >> --- a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >> +++ b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >> @@ -6,7 +6,7 @@ >> >> / { >> gic: interrupt-controller@2c001000 { >> - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; >> + compatible = "arm,cortex-a15-gic"; >> #interrupt-cells = <3>; >> #address-cells = <2>; >> interrupt-controller; >> diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi >> index 3feefd61eb76..62392ab1f880 100644 >> --- a/arch/arm64/boot/dts/arm/juno-base.dtsi >> +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi >> @@ -69,7 +69,7 @@ >> }; >> >> gic: interrupt-controller@2c010000 { >> - compatible = "arm,gic-400", "arm,cortex-a15-gic"; >> + compatible = "arm,cortex-a15-gic"; > > Why? GIC-400 is definitely the most correct compatible string. I'd > rather see this compatible being generalised to the models rather than > only referencing the A15 GIC. I agree that gic-400 is the far better name, but it was only introduced in v3.16. So omitting arm,cortex-a15-gic would break any kernels before that, which I would like to avoid. It's actually a pity that we are so picky about the compatible listings, because the existing combination is actually quite nice: we get compatibility with older DT consumers, but still can say what it actually is. I wonder if I should introduce this combination to the GIC DT binding instead, it seems like there are other users in the tree as well. What do you think? Cheers, Andre > >> reg = <0x0 0x2c010000 0 0x1000>, >> <0x0 0x2c02f000 0 0x2000>, >> <0x0 0x2c04f000 0 0x2000>, >> diff --git a/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts b/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts >> index c5d15cbd8cf6..f86f6451411f 100644 >> --- a/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts >> +++ b/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts >> @@ -95,7 +95,7 @@ >> }; >> >> gic: interrupt-controller@2c001000 { >> - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; >> + compatible = "arm,cortex-a15-gic"; >> #interrupt-cells = <3>; >> #address-cells = <0>; >> interrupt-controller; >> -- >> 2.17.1 >> >> > > Thanks, > > M. >
On 2020-05-06 09:45, André Przywara wrote: > On 05/05/2020 19:25, Marc Zyngier wrote: >> On Tue, 05 May 2020 17:52:03 +0100, >> Andre Przywara <andre.przywara@arm.com> wrote: >>> >>> The GIC DT binding only allows a certain combination of DT compatible >>> strings, mostly just consisting of one name. >>> >>> Drop the combination of multiple names and go with the >>> "arm,cortex-a15-gic" name for GICv2, as this seems to be the most >>> widely >>> accepted string. "arm,gic-400" would be more correct, but was >>> introduced >>> much later into the kernel's GIC driver. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi | 2 +- >>> arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +- >>> arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts | 2 +- >>> 3 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>> b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>> index 15fe81738e94..61a1750fcdd6 100644 >>> --- a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>> +++ b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>> @@ -6,7 +6,7 @@ >>> >>> / { >>> gic: interrupt-controller@2c001000 { >>> - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; >>> + compatible = "arm,cortex-a15-gic"; >>> #interrupt-cells = <3>; >>> #address-cells = <2>; >>> interrupt-controller; >>> diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi >>> b/arch/arm64/boot/dts/arm/juno-base.dtsi >>> index 3feefd61eb76..62392ab1f880 100644 >>> --- a/arch/arm64/boot/dts/arm/juno-base.dtsi >>> +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi >>> @@ -69,7 +69,7 @@ >>> }; >>> >>> gic: interrupt-controller@2c010000 { >>> - compatible = "arm,gic-400", "arm,cortex-a15-gic"; >>> + compatible = "arm,cortex-a15-gic"; >> >> Why? GIC-400 is definitely the most correct compatible string. I'd >> rather see this compatible being generalised to the models rather than >> only referencing the A15 GIC. > > I agree that gic-400 is the far better name, but it was only introduced > in v3.16. So omitting arm,cortex-a15-gic would break any kernels before > that, which I would like to avoid. I am not talking about dropping the A15 GIC. I'm saying that both should stay. Is there anything in the DT binding that forbids multiple names in the compatible property? > It's actually a pity that we are so picky about the compatible > listings, > because the existing combination is actually quite nice: we get > compatibility with older DT consumers, but still can say what it > actually is. > I wonder if I should introduce this combination to the GIC DT binding > instead, it seems like there are other users in the tree as well. > > What do you think? I'd say that if the binding forbids multiple compatible strings, the binding is likely to be wrong. We should fix it, and not make the DTs worse as a result of a binding issue. Thanks, M.
On 06/05/2020 10:16, Marc Zyngier wrote: > On 2020-05-06 09:45, André Przywara wrote: >> On 05/05/2020 19:25, Marc Zyngier wrote: >>> On Tue, 05 May 2020 17:52:03 +0100, >>> Andre Przywara <andre.przywara@arm.com> wrote: >>>> >>>> The GIC DT binding only allows a certain combination of DT compatible >>>> strings, mostly just consisting of one name. >>>> >>>> Drop the combination of multiple names and go with the >>>> "arm,cortex-a15-gic" name for GICv2, as this seems to be the most >>>> widely >>>> accepted string. "arm,gic-400" would be more correct, but was >>>> introduced >>>> much later into the kernel's GIC driver. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi | 2 +- >>>> arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +- >>>> arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts | 2 +- >>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>> b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>> index 15fe81738e94..61a1750fcdd6 100644 >>>> --- a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>> +++ b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>> @@ -6,7 +6,7 @@ >>>> >>>> / { >>>> gic: interrupt-controller@2c001000 { >>>> - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; >>>> + compatible = "arm,cortex-a15-gic"; >>>> #interrupt-cells = <3>; >>>> #address-cells = <2>; >>>> interrupt-controller; >>>> diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi >>>> b/arch/arm64/boot/dts/arm/juno-base.dtsi >>>> index 3feefd61eb76..62392ab1f880 100644 >>>> --- a/arch/arm64/boot/dts/arm/juno-base.dtsi >>>> +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi >>>> @@ -69,7 +69,7 @@ >>>> }; >>>> >>>> gic: interrupt-controller@2c010000 { >>>> - compatible = "arm,gic-400", "arm,cortex-a15-gic"; >>>> + compatible = "arm,cortex-a15-gic"; >>> >>> Why? GIC-400 is definitely the most correct compatible string. I'd >>> rather see this compatible being generalised to the models rather than >>> only referencing the A15 GIC. >> >> I agree that gic-400 is the far better name, but it was only introduced >> in v3.16. So omitting arm,cortex-a15-gic would break any kernels before >> that, which I would like to avoid. > > I am not talking about dropping the A15 GIC. I'm saying that both should > stay. Is there anything in the DT binding that forbids multiple names in > the compatible property? Well, the current form of the YAML bindings require every combination of compatible strings to be listed, either explicitly, or using an list of allowed strings for each position. This combination here is not listed at the moment. >> It's actually a pity that we are so picky about the compatible listings, >> because the existing combination is actually quite nice: we get >> compatibility with older DT consumers, but still can say what it >> actually is. >> I wonder if I should introduce this combination to the GIC DT binding >> instead, it seems like there are other users in the tree as well. >> >> What do you think? > > I'd say that if the binding forbids multiple compatible strings, the > binding is likely to be wrong. We should fix it, and not make the DTs > worse as a result of a binding issue. OK, thanks for the confirmation, and I agree. I will ditch this patch and replace it with a respective bindings fix. Thanks, Andre.
On 2020-05-06 11:00, André Przywara wrote: > On 06/05/2020 10:16, Marc Zyngier wrote: >> On 2020-05-06 09:45, André Przywara wrote: >>> On 05/05/2020 19:25, Marc Zyngier wrote: >>>> On Tue, 05 May 2020 17:52:03 +0100, >>>> Andre Przywara <andre.przywara@arm.com> wrote: >>>>> >>>>> The GIC DT binding only allows a certain combination of DT >>>>> compatible >>>>> strings, mostly just consisting of one name. >>>>> >>>>> Drop the combination of multiple names and go with the >>>>> "arm,cortex-a15-gic" name for GICv2, as this seems to be the most >>>>> widely >>>>> accepted string. "arm,gic-400" would be more correct, but was >>>>> introduced >>>>> much later into the kernel's GIC driver. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>>> --- >>>>> arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi | 2 +- >>>>> arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +- >>>>> arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts | 2 +- >>>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>>> b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>>> index 15fe81738e94..61a1750fcdd6 100644 >>>>> --- a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>>> +++ b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi >>>>> @@ -6,7 +6,7 @@ >>>>> >>>>> / { >>>>> gic: interrupt-controller@2c001000 { >>>>> - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; >>>>> + compatible = "arm,cortex-a15-gic"; >>>>> #interrupt-cells = <3>; >>>>> #address-cells = <2>; >>>>> interrupt-controller; >>>>> diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi >>>>> b/arch/arm64/boot/dts/arm/juno-base.dtsi >>>>> index 3feefd61eb76..62392ab1f880 100644 >>>>> --- a/arch/arm64/boot/dts/arm/juno-base.dtsi >>>>> +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi >>>>> @@ -69,7 +69,7 @@ >>>>> }; >>>>> >>>>> gic: interrupt-controller@2c010000 { >>>>> - compatible = "arm,gic-400", "arm,cortex-a15-gic"; >>>>> + compatible = "arm,cortex-a15-gic"; >>>> >>>> Why? GIC-400 is definitely the most correct compatible string. I'd >>>> rather see this compatible being generalised to the models rather >>>> than >>>> only referencing the A15 GIC. >>> >>> I agree that gic-400 is the far better name, but it was only >>> introduced >>> in v3.16. So omitting arm,cortex-a15-gic would break any kernels >>> before >>> that, which I would like to avoid. >> >> I am not talking about dropping the A15 GIC. I'm saying that both >> should >> stay. Is there anything in the DT binding that forbids multiple names >> in >> the compatible property? > > Well, the current form of the YAML bindings require every combination > of > compatible strings to be listed, either explicitly, or using an list of > allowed strings for each position. This combination here is not listed > at the moment. I think this should be relaxed. What the tool should be warning against is a set of incompatible "compatible" strings (like a15 + a9, which is totally bonkers). >>> It's actually a pity that we are so picky about the compatible >>> listings, >>> because the existing combination is actually quite nice: we get >>> compatibility with older DT consumers, but still can say what it >>> actually is. >>> I wonder if I should introduce this combination to the GIC DT binding >>> instead, it seems like there are other users in the tree as well. >>> >>> What do you think? >> >> I'd say that if the binding forbids multiple compatible strings, the >> binding is likely to be wrong. We should fix it, and not make the DTs >> worse as a result of a binding issue. > > OK, thanks for the confirmation, and I agree. I will ditch this patch > and replace it with a respective bindings fix. Please keep removal of the A9 GIC reference though, because it doesn't make any sense as it is. Thanks, M.
diff --git a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi index 15fe81738e94..61a1750fcdd6 100644 --- a/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi +++ b/arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi @@ -6,7 +6,7 @@ / { gic: interrupt-controller@2c001000 { - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; + compatible = "arm,cortex-a15-gic"; #interrupt-cells = <3>; #address-cells = <2>; interrupt-controller; diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi index 3feefd61eb76..62392ab1f880 100644 --- a/arch/arm64/boot/dts/arm/juno-base.dtsi +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi @@ -69,7 +69,7 @@ }; gic: interrupt-controller@2c010000 { - compatible = "arm,gic-400", "arm,cortex-a15-gic"; + compatible = "arm,cortex-a15-gic"; reg = <0x0 0x2c010000 0 0x1000>, <0x0 0x2c02f000 0 0x2000>, <0x0 0x2c04f000 0 0x2000>, diff --git a/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts b/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts index c5d15cbd8cf6..f86f6451411f 100644 --- a/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts +++ b/arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts @@ -95,7 +95,7 @@ }; gic: interrupt-controller@2c001000 { - compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; + compatible = "arm,cortex-a15-gic"; #interrupt-cells = <3>; #address-cells = <0>; interrupt-controller;
The GIC DT binding only allows a certain combination of DT compatible strings, mostly just consisting of one name. Drop the combination of multiple names and go with the "arm,cortex-a15-gic" name for GICv2, as this seems to be the most widely accepted string. "arm,gic-400" would be more correct, but was introduced much later into the kernel's GIC driver. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arch/arm64/boot/dts/arm/foundation-v8-gicv2.dtsi | 2 +- arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +- arch/arm64/boot/dts/arm/rtsm_ve-aemv8a.dts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)