Re: [PATCH v3 1/4] dt-bindings: memory: Add LPDDR2 binding

[Dmitry Osipenko <digetx@gmail.com>]

04.10.2021 10:42, Krzysztof Kozlowski пишет:
> On 03/10/2021 03:32, Dmitry Osipenko wrote:
>> Add binding for standard LPDDR2 memory chip properties.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  .../memory-controllers/jedec,lpddr2.yaml      | 80 +++++++++++++++++++
>>  include/dt-bindings/memory/lpddr2.h           | 25 ++++++
> 
> Hi Dmitry,
> 
> Thanks for doing this. I think I should be slightly more descriptive in
> my previous comment. What I meant, is to use existing DDR bindings
> (which might include or require converting them to YAML):
> Documentation/devicetree/bindings/ddr/
> 
> The bindings are already used:
> arch/arm/boot/dts/elpida_ecb240abacn.dtsi
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> drivers/memory/samsung/exynos5422-dmc.c

Thanks! I missed that there is ddr/ subdir.

> You can remove the Documentation/devicetree/bindings/ddr/lpddr2.txt
> after full conversion, so also including AC timings and AC timing
> parameters. The timing parameters could be a separate YAML, if you want
> to convert everything. You can also skip it, because it is not necessary
> for your work.
> 
> 
> Rob,
> Any advice from your side where to put LPDDR2 dtschema bindings? The
> existing location was bindings/ddr/ but maybe this should be part of
> memory-controllers (although it is not actually a controller but rather
> used by the controller)?

+1 for having it inside of memory-controllers/

>>  2 files changed, 105 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/jedec,lpddr2.yaml
>>  create mode 100644 include/dt-bindings/memory/lpddr2.h
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/jedec,lpddr2.yaml
>> new file mode 100644
>> index 000000000000..ef227eba1e4a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/jedec,lpddr2.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/memory-controllers/jedec,lpddr2.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: JEDEC LPDDR2 SDRAM
>> +
>> +maintainers:
>> +  - Krzysztof Kozlowski <krzk@kernel.org>
>> +
>> +properties:
> 
> You need compatible (see lpddr2.txt)
> 
>> +  jedec,lpddr2-manufacturer-id:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maximum: 255
>> +    description: |
>> +      Unique manufacturer ID of SDRAM chip. See MR5 description in JESD209-2.
> 
> Plus:
> "See include/dt-bindings/memory/lpddr2.h for known manufactured IDs."
> 
> However I wonder whether we need it. It should be taken from the vendor
> part of compatible.

It shouldn't be needed if compatible is used.

>> +
>> +  jedec,lpddr2-revision-id1:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maximum: 255
>> +    description: |
>> +      Revision 1 value of SDRAM chip.
>> +      See MR6 description in chip vendor specification.
>> +
>> +  jedec,lpddr2-revision-id2:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maximum: 255
>> +    description: |
>> +      Revision 2 value of SDRAM chip.
>> +      See MR7 description in chip vendor specification.
>> +
>> +  jedec,lpddr2-density-mbits:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      Density in megabits of SDRAM chip. See MR8 description in JESD209-2.
>> +    enum:
>> +      - 64
>> +      - 128
>> +      - 256
>> +      - 512
>> +      - 1024
>> +      - 2048
>> +      - 4096
>> +      - 8192
>> +      - 16384
>> +      - 32768
>> +
>> +  jedec,lpddr2-io-width-bits:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      IO bus width in bits of SDRAM chip. See MR8 description in JESD209-2.
>> +    enum:
>> +      - 32
>> +      - 16
>> +      - 8
>> +
>> +  jedec,lpddr2-type:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      LPDDR type which corresponds to a number of words SDRAM pre-fetches
>> +      per column request. See MR8 description in JESD209-2.
>> +    enum:
>> +      - 0 # S4 (4 words prefetch architecture)
>> +      - 1 # S2 (2 words prefetch architecture)
>> +      - 2 # NVM (Non-volatile memory)
> 
> Type should not be needed but instead taken from compatible. Unless Rob
> has here preference and maybe change the DDR bindings?
> 
> requiredProperties for compatible, density, io-width.

Alright