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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

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