diff mbox series

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

Message ID 20211003013235.2357-2-digetx@gmail.com
State Changes Requested, archived
Headers show
Series tegra20-emc: Identify memory chip by LPDDR configuration | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check success

Commit Message

Dmitry Osipenko Oct. 3, 2021, 1:32 a.m. UTC 
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 ++++++
 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

Comments

Krzysztof Kozlowski Oct. 4, 2021, 7:42 a.m. UTC | #1 
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

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)?

>  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.

> +
> +  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.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/memory/lpddr2.h>
> +
> +    lpddr2 {
> +        jedec,lpddr2-manufacturer-id = <LPDDR2_MANID_ELPIDA>;
> +        jedec,lpddr2-revision-id1 = <1>;
> +        jedec,lpddr2-density-mbits = <2048>;
> +        jedec,lpddr2-io-width-bits = <16>;
> +        jedec,lpddr2-type = <LPDDR2_TYPE_S4>;
> +    };
> diff --git a/include/dt-bindings/memory/lpddr2.h b/include/dt-bindings/memory/lpddr2.h
> new file mode 100644
> index 000000000000..e837b0d8a11e
> --- /dev/null
> +++ b/include/dt-bindings/memory/lpddr2.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
> +#ifndef _DT_BINDINGS_LPDDR2_H
> +#define _DT_BINDINGS_LPDDR2_H
> +
> +#define LPDDR2_MANID_SAMSUNG		1
> +#define LPDDR2_MANID_QIMONDA		2
> +#define LPDDR2_MANID_ELPIDA		3
> +#define LPDDR2_MANID_ETRON		4
> +#define LPDDR2_MANID_NANYA		5
> +#define LPDDR2_MANID_HYNIX		6
> +#define LPDDR2_MANID_MOSEL		7
> +#define LPDDR2_MANID_WINBOND		8
> +#define LPDDR2_MANID_ESMT		9
> +#define LPDDR2_MANID_SPANSION		11
> +#define LPDDR2_MANID_SST		12
> +#define LPDDR2_MANID_ZMOS		13
> +#define LPDDR2_MANID_INTEL		14
> +#define LPDDR2_MANID_NUMONYX		254
> +#define LPDDR2_MANID_MICRON		255
> +
> +#define LPDDR2_TYPE_S4			0
> +#define LPDDR2_TYPE_S2			1
> +#define LPDDR2_TYPE_NVM			2
> +
> +#endif /*_DT_BINDINGS_LPDDR2_H */
> 


Best regards,
Krzysztof
Dmitry Osipenko Oct. 4, 2021, 4:53 p.m. UTC | #2 
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
diff mbox series

Patch

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:
+  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.
+
+  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)
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/memory/lpddr2.h>
+
+    lpddr2 {
+        jedec,lpddr2-manufacturer-id = <LPDDR2_MANID_ELPIDA>;
+        jedec,lpddr2-revision-id1 = <1>;
+        jedec,lpddr2-density-mbits = <2048>;
+        jedec,lpddr2-io-width-bits = <16>;
+        jedec,lpddr2-type = <LPDDR2_TYPE_S4>;
+    };
diff --git a/include/dt-bindings/memory/lpddr2.h b/include/dt-bindings/memory/lpddr2.h
new file mode 100644
index 000000000000..e837b0d8a11e
--- /dev/null
+++ b/include/dt-bindings/memory/lpddr2.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
+#ifndef _DT_BINDINGS_LPDDR2_H
+#define _DT_BINDINGS_LPDDR2_H
+
+#define LPDDR2_MANID_SAMSUNG		1
+#define LPDDR2_MANID_QIMONDA		2
+#define LPDDR2_MANID_ELPIDA		3
+#define LPDDR2_MANID_ETRON		4
+#define LPDDR2_MANID_NANYA		5
+#define LPDDR2_MANID_HYNIX		6
+#define LPDDR2_MANID_MOSEL		7
+#define LPDDR2_MANID_WINBOND		8
+#define LPDDR2_MANID_ESMT		9
+#define LPDDR2_MANID_SPANSION		11
+#define LPDDR2_MANID_SST		12
+#define LPDDR2_MANID_ZMOS		13
+#define LPDDR2_MANID_INTEL		14
+#define LPDDR2_MANID_NUMONYX		254
+#define LPDDR2_MANID_MICRON		255
+
+#define LPDDR2_TYPE_S4			0
+#define LPDDR2_TYPE_S2			1
+#define LPDDR2_TYPE_NVM			2
+
+#endif /*_DT_BINDINGS_LPDDR2_H */