Re: [PATCH 07/16] arm64: dts: arm: Fix GIC compatible names

[Marc Zyngier <maz@kernel.org>]

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.
-- 
Jazz is not dead. It just smells funny...