[v1,1/2] dt-bindings: memory: tegra20: emc: Document optional LPDDR properties

Some Tegra20 boards don't use RAM code for the memory chip identification
and the identity information should read out from LPDDR chip in this case.
Document new optional generic LPDDR properties that will be used for the
memory chip identification if RAM code isn't provided.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 .../nvidia,tegra20-emc.yaml                   | 43 +++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

On 29/09/2021 22:03, Dmitry Osipenko wrote:
> Some Tegra20 boards don't use RAM code for the memory chip identification
> and the identity information should read out from LPDDR chip in this case.
> Document new optional generic LPDDR properties that will be used for the
> memory chip identification if RAM code isn't provided.

Please mention how they are going to be used. Naively I would assume
that these new properties describe the RAM you have. However it seems
you do not use them to configure the device but to compare with the
device. Why do you need them?

> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../nvidia,tegra20-emc.yaml                   | 43 +++++++++++++++++--
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.yaml
> index cac6842dc8f1..6d01b1bf6304 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.yaml
> @@ -158,6 +158,46 @@ patternProperties:
>          description:
>            Value of RAM_CODE this timing set is used for.
>  
> +      jedec,lpddr-manufacturer-id:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Unique manufacturer ID of SDRAM chip this timing set is used for.
> +          See MR5 description in JEDEC LPDDR2 specification (JESD209-2).
> +
> +      jedec,lpddr-revision-id1:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Revision 1 value of SDRAM chip this timing set is used for.
> +          See MR6 description in chip vendor specification.
> +
> +      jedec,lpddr-revision-id2:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Revision 2 value of SDRAM chip this timing set is used for.
> +          See MR7 description in chip vendor specification.
> +
> +      jedec,lpddr-density-mbits:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Density in megabits of SDRAM chip this timing set is used for.
> +          See MR8 description in JEDEC LPDDR2 specification.
> +
> +      jedec,lpddr-io-width-bits:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          IO bus width in bits of SDRAM chip this timing set is used for.
> +          See MR8 description in JEDEC LPDDR2 specification.
> +
> +      jedec,lpddr-type:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          LPDDR type which corresponds to a number of words SDRAM pre-fetches
> +          per column request that this timing set is used for.
> +          See MR8 description in JEDEC LPDDR2 specification.
> +        enum:
> +          - 4 # S4 (4 words prefetch architecture)
> +          - 2 # S2 (2 words prefetch architecture)

I think instead you should use generic lpddr{2,3} bindings - have a
separate node and reference it via a phandle.

Best regards,
Krzysztof

30.09.2021 09:54, Krzysztof Kozlowski пишет:
> On 29/09/2021 22:03, Dmitry Osipenko wrote:
>> Some Tegra20 boards don't use RAM code for the memory chip identification
>> and the identity information should read out from LPDDR chip in this case.
>> Document new optional generic LPDDR properties that will be used for the
>> memory chip identification if RAM code isn't provided.
> 
> Please mention how they are going to be used. Naively I would assume
> that these new properties describe the RAM you have. However it seems
> you do not use them to configure the device but to compare with the
> device. Why do you need them?

Yes, the properties describe hardware configuration of external DRAM
chip. This information is read-only and it's actually used for
configuring SoC memory controller. This MC configuration is already
pre-configured by bootloader and partially it shouldn't be ever touched
by software. Kernel driver needs to reconfigure only a part of hardware
on memory freq changes. The memory timing data is tuned for a specific
DRAM chip and board, it doesn't include info which identifies the chip.
So we need to read out DRAM config from hardware and find the matching
timing in a device-tree by comparing the chip-unique properties. Note
that only LPDDR chips have that chip-identity info. Regular DDR chips
require SPD or other means, like NVMEM in case of Tegra.

I'll extend the commit message.

...
>> +          - 4 # S4 (4 words prefetch architecture)
>> +          - 2 # S2 (2 words prefetch architecture)
> 
> I think instead you should use generic lpddr{2,3} bindings - have a
> separate node and reference it via a phandle.

It indeed shouldn't be a problem to create lpddr binding and move these
props there.

Extra phandle shouldn't be needed, should be fine to keep these new DRAM
properties within the chip-descriptor nodes that we already have in
tegra device-trees. We'll only need to $ref the lpddr binding for the
descriptor node in the binding. I.e. to make it similar to regulator
bindings where there is generic regulator.yaml + hw-specific properties.

I'll try to implement this in v2, thanks!