Re: [PATCH v1 04/10] ARM: tegra: Add reg property to Tegra20 EMC table device-tree nodes

[Dmitry Osipenko]

01.06.2021 14:25, Thierry Reding пишет:
> On Mon, May 31, 2021 at 11:45:19PM +0300, Dmitry Osipenko wrote:
>> 31.05.2021 12:14, Thierry Reding пишет:
>>> On Mon, May 10, 2021 at 11:25:54PM +0300, Dmitry Osipenko wrote:
>>>> The reg property is now specified for the emc-tables nodes in the Tegra20
>>>> device-tree binding. Add reg property to the EMC table device-tree nodes
>>>> of Tegra20 board device-trees in order to silence dt_binding_check warning
>>>> about the missing property.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>> ---
>>>>  arch/arm/boot/dts/tegra20-acer-a500-picasso.dts | 4 ++++
>>>>  arch/arm/boot/dts/tegra20-paz00.dts             | 1 +
>>>>  2 files changed, 5 insertions(+)
>>>
>>> In retrospect we should've just used "reg" in the first place rather
>>> than adding the custom "nvidia,ram-code". It's a bit redundant to have
>>> both of them with the same value. I wonder if we should deprecate the
>>> use of "nvidia,ram-code" and at least make the code look at the "reg"
>>> property first and only fall back to "nvidia,ram-code" if "reg" does
>>> not exist. We probably won't ever be able to get rid of the fallback
>>> for backwards-compatibility reasons, but at least that would make the
>>> intent a bit clearer.
>>
>> This may be not doable. We have Asus TF101 which doesn't use RAM code
>> for the memory identification, instead it uses LPDDR chip info [1]. I
>> will send the LPDDR patches later on.
>>
>> [1]
>> https://github.com/grate-driver/linux/blob/master/arch/arm/boot/dts/tegra20-asus-tf101.dts#L1115
> 
> That DTS defines both "jedec,lpddr-manufacturer-id" and "reg" with the
> same value, so we could simply use "reg" there. If you plan to support
> the JEDEC properties, we'll have to add code for that anyway, so there
> is no downside to first trying "reg". 

At least in my mind the reg property is associated with a hardware
register. Changing the purpose of the reg and removing the verbose
properties should create confusion for anyone who looks at device-tree
and not familiar with the binding.

The current main purpose of the reg is "Either an opaque enumerator to
tell different tables apart or the valid frequency for which the table
should be used (in kHz)", like the binding says. It should be better to
keep the reg optional and additional, especially if we will give it
another meaning, IMO.

> And we may not even need to add
> support for any of those JEDEC properties if we can just use the "reg"
> standard property in the first place.

We will need all those JEDEC properties for identifying memory chips,
including information about chip version and h/w geometry. This method
of matching is borrowed from the official downstream kernel of TF101,
apparently board designers decided not to use the RAM code.