Re: [PATCH 1/3] Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

[Rob Herring <robh+dt@kernel.org>]

On Thu, Apr 26, 2018 at 10:01 AM, Dinh Nguyen <dinguyen@kernel.org> wrote:
>
>
> On 04/26/2018 09:58 AM, Thor Thayer wrote:
>> On 04/26/2018 09:43 AM, Thor Thayer wrote:
>>> On 04/25/2018 09:16 PM, Dinh Nguyen wrote:
>>>>
>>>>
>>>> On 04/24/2018 01:35 PM, thor.thayer@linux.intel.com wrote:
>>>>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>>>>
>>>>> Add the device tree bindings needed to support the Stratix10
>>>>> ECC Manager and SDRAM ECC to the existing bindings.
>>>>>
>>>>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>>>>> ---
>>>>>   .../bindings/arm/altera/socfpga-eccmgr.txt         | 47
>>>>> ++++++++++++++++++++++
>>>>>   1 file changed, 47 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>>>>> b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>>>>> index 4a1714f96bab..fe48ad293a24 100644
>>>>> --- a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>>>>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>>>>> @@ -231,3 +231,50 @@ Example:
>>>>>                        <48 IRQ_TYPE_LEVEL_HIGH>;
>>>>>           };
>>>>>       };
>>>>> +
>>>>> +Stratix10 SoCFPGA ECC Manager
>>>>> +The Stratix10 SoC ECC Manager handles the IRQs for each peripheral
>>>>> +in a shared register similar to the Arria10. However, ECC requires
>>>>> +access to registers that can only be read in EL3 with SMC calls.
>>>>> +Therefore the device tree is slightly different.
>>>>> +
>>>>> +Required Properties:
>>>>> +- compatible : Should be "altr,socfpga-s10-ecc-manager"
>>>>
>>>> Altera technically doesn't exist anymore, should this be
>>>> "intel,stratix10-ecc-manager"?
>>>>
>>> I was trying to be consistent with the older names but I agree with
>>> your argument. I will change this. Thanks
>>>
>> After looking at the Stratix10 device tree, there are only "altr," in
>> there. For instance the top node is:
>>
>> compatible = "altr,socfpga-stratix10";
>>
>> and even the directory that the device tree is in is named "altera"
>>
>> so I'll stick with the existing "altr," designation to be consistent.
>>
>
> That's fine and I don't have any strong inclination for 1 way or
> another. But it's not quite true that there are only "altr" in the dts file.
>
> I did recently switch the clock manager to an intel designation. I
> upstreamed the original stratix10 DTS file back in 2015, before Altera
> was purchased by Intel.
>
> My only argument is that if you're adding new support for devices, it
> should be an intel binding.

I think I would make the switch when you change it at the SoC level
(for a new SoC). IOW, if the top level compatible is using 'intel',
then use that for the device bindings otherwise stick with 'altr' for
that SoC.

Rob