[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4ad137c0-1199-56bc-f77e-0ea8dcc4ba9c@gmail.com>
Date: Mon, 4 Oct 2021 19:53:46 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Rob Herring <robh+dt@...nel.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-tegra@...r.kernel.org
Subject: Re: [PATCH v3 1/4] dt-bindings: memory: Add LPDDR2 binding
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@...il.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@...nel.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
Powered by blists - more mailing lists