[PATCH 0/4] dt-bindings: memory: Describing LPDDR topology

[PATCH 0/4] dt-bindings: memory: Describing LPDDR topology

[Julius Werner]

This patch series implements a proposal previously discussed on the
mailing list under the topic `[RFC] Correct memory layout reporting for
"jedec,lpddr2" and related bindings`. It adds a new jedec,lpddr-channel
binding which should be used to group nodes of the existing jedec,lpddr
bindings to describe their relative topology on the system and the
amount of chips wired in parallel on each channel, as well as their
different ranks. This also adds bindings for LPDDR4 and LPDDR5 memory
types and deduplicates some common schema elements between different
LPDDR types.

Julius Werner (4):
  dt-bindings: memory: Factor out common properties of LPDDR bindings
  dt-bindings: memory: Add numeric LPDDR compatible string variant
  dt-bindings: memory: Add jedec,lpddr4 and jedec,lpddr5 bindings
  dt-bindings: memory: Add jedec,lpddrX-channel binding

 .../ddr/jedec,lpddr-channel.yaml              | 116 ++++++++++++++++++
 .../ddr/jedec,lpddr-props.yaml                |  82 +++++++++++++
 .../memory-controllers/ddr/jedec,lpddr2.yaml  |  48 ++------
 .../memory-controllers/ddr/jedec,lpddr3.yaml  |  51 ++------
 .../memory-controllers/ddr/jedec,lpddr4.yaml  |  36 ++++++
 .../memory-controllers/ddr/jedec,lpddr5.yaml  |  48 ++++++++
 6 files changed, 303 insertions(+), 78 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml

-- 
2.31.0

[PATCH 1/4] dt-bindings: memory: Factor out common properties of LPDDR bindings

[Julius Werner]

The bindings for different LPDDR versions mostly use the same kinds of
properties, so in order to reduce duplication when we're adding support
for more versions, this patch creates a new lpddr-props subschema that
can be referenced by the others to define these common parts. (This will
consider a few smaller I/O width and density numbers "legal" for LPDDR3
that are usually not used there, but this should be harmless.)

This also un-deprecates the manufacturer ID property for LPDDR3 (and
introduces it to LPDDR2), since it was found that having this
information available in a separate property can be useful in some
cases.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 .../ddr/jedec,lpddr-props.yaml                | 60 +++++++++++++++++++
 .../memory-controllers/ddr/jedec,lpddr2.yaml  | 40 ++-----------
 .../memory-controllers/ddr/jedec,lpddr3.yaml  | 39 ++----------
 3 files changed, 68 insertions(+), 71 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
new file mode 100644
index 00000000000000..8b31c60ea2435b
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr-props.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common properties for LPDDR types
+
+description:
+  Different LPDDR types generally use the same properties and only differ in the
+  range of legal values for each. This file defines the common parts that can be
+  reused for each type.
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+
+properties:
+  manufacturer-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Manufacturer ID read from Mode Register 5.
+    minimum: 0
+    maximum: 255
+
+  revision-id:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>).
+    minItems: 2
+    maxItems: 2
+    items:
+      minimum: 0
+      maximum: 255
+
+  density:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Density in megabits of SDRAM chip. Decoded from Mode Register 8.
+    enum:
+      - 64
+      - 128
+      - 256
+      - 512
+      - 1024
+      - 2048
+      - 4096
+      - 8192
+      - 16384
+      - 32768
+
+  io-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      IO bus width in bits of SDRAM chip. Decoded from Mode Register 8.
+    enum:
+      - 32
+      - 16
+      - 8
+
+additionalProperties: true
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
index 9d78f140609b6c..63c47235cb9896 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
@@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: LPDDR2 SDRAM compliant to JEDEC JESD209-2
 
+allOf:
+  - $ref: "jedec,lpddr-props.yaml#"
+
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
 
@@ -41,41 +44,6 @@ properties:
       Property is deprecated, use revision-id instead.
     deprecated: true
 
-  revision-id:
-    $ref: /schemas/types.yaml#/definitions/uint32-array
-    description: |
-      Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>).
-    minItems: 2
-    maxItems: 2
-    items:
-      minimum: 0
-      maximum: 255
-
-  density:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description: |
-      Density in megabits of SDRAM chip. Obtained from device datasheet.
-    enum:
-      - 64
-      - 128
-      - 256
-      - 512
-      - 1024
-      - 2048
-      - 4096
-      - 8192
-      - 16384
-      - 32768
-
-  io-width:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description: |
-      IO bus width in bits of SDRAM chip. Obtained from device datasheet.
-    enum:
-      - 32
-      - 16
-      - 8
-
   tRRD-min-tck:
     $ref: /schemas/types.yaml#/definitions/uint32
     maximum: 16
@@ -168,7 +136,7 @@ required:
   - density
   - io-width
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
index 48908a19473c3f..5969166cdc9e0f 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
@@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: LPDDR3 SDRAM compliant to JEDEC JESD209-3
 
+allOf:
+  - $ref: "jedec,lpddr-props.yaml#"
+
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
 
@@ -20,40 +23,6 @@ properties:
     const: 1
     deprecated: true
 
-  density:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description: |
-      Density in megabits of SDRAM chip.
-    enum:
-      - 4096
-      - 8192
-      - 16384
-      - 32768
-
-  io-width:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description: |
-      IO bus width in bits of SDRAM chip.
-    enum:
-      - 32
-      - 16
-
-  manufacturer-id:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description: |
-      Manufacturer ID value read from Mode Register 5.  The property is
-      deprecated, manufacturer should be derived from the compatible.
-    deprecated: true
-
-  revision-id:
-    $ref: /schemas/types.yaml#/definitions/uint32-array
-    minItems: 2
-    maxItems: 2
-    items:
-      maximum: 255
-    description: |
-      Revision value of SDRAM chip read from Mode Registers 6 and 7.
-
   '#size-cells':
     const: 0
     deprecated: true
@@ -206,7 +175,7 @@ required:
   - density
   - io-width
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
-- 
2.31.0

[PATCH 2/4] dt-bindings: memory: Add numeric LPDDR compatible string variant

[Julius Werner]

This patch allows a new kind of compatible string for LPDDR parts in the
device tree bindings, in addition to the existing hardcoded
<vendor>,<part-number> strings. The new format contains manufacturer and
part (revision) information in numerical form, such as lpddr3-ff,0201
for an LPDDR3 part with manufacturer ID ff and revision ID 0201. This
helps cases where LPDDR parts are probed at runtime by boot firmware and
cannot be matched to hardcoded part numbers.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 .../memory-controllers/ddr/jedec,lpddr-props.yaml    | 10 ++++++++++
 .../memory-controllers/ddr/jedec,lpddr2.yaml         |  8 +++++---
 .../memory-controllers/ddr/jedec,lpddr3.yaml         | 12 ++++++++----
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
index 8b31c60ea2435b..0c7d2feafd77c8 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
@@ -15,6 +15,16 @@ maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
 
 properties:
+  compatible:
+    description:
+      Compatible strings can be either explicit vendor names and part numbers
+      (e.g. elpida,ECB240ABACN), or generated strings of the form
+      lpddrX-YY,ZZZZ where X is the LPDDR version, YY is the manufacturer ID
+      (from MR5) and ZZZZ is the revision ID (from MR6 and MR7). Both IDs are
+      formatted in lower case hexadecimal representation with leading zeroes.
+      The latter form can be useful when LPDDR nodes are created at runtime by
+      boot firmware that doesn't have access to static part number information.
+
   manufacturer-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
index 63c47235cb9896..0eb7a66dfd08db 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
@@ -20,13 +20,15 @@ properties:
               - elpida,ECB240ABACN
               - elpida,B8132B2PB-6D-F
           - enum:
-              - jedec,lpddr2-s4
-      - items:
-          - enum:
+              - jedec,lpddr2-nvm
               - jedec,lpddr2-s2
+              - jedec,lpddr2-s4
       - items:
+          - pattern: "^lpddr2-[0-9a-f]{2},[0-9a-f]{4}$"
           - enum:
               - jedec,lpddr2-nvm
+              - jedec,lpddr2-s2
+              - jedec,lpddr2-s4
 
   revision-id1:
     $ref: /schemas/types.yaml#/definitions/uint32
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
index 5969166cdc9e0f..cd076a4fd91194 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
@@ -14,10 +14,14 @@ maintainers:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - samsung,K3QF2F20DB
-      - const: jedec,lpddr3
+    oneOf:
+      - items:
+          - enum:
+              - samsung,K3QF2F20DB
+          - const: jedec,lpddr3
+      - items:
+          - pattern: "^lpddr3-[0-9a-f]{2},[0-9a-f]{4}$"
+          - const: jedec,lpddr3
 
   '#address-cells':
     const: 1
-- 
2.31.0

[PATCH 3/4] dt-bindings: memory: Add jedec,lpddr4 and jedec,lpddr5 bindings

[Julius Werner]

This patch adds bindings for LPDDR4 and LPDDR5 memory analogous to the
existing bindings for LPDDR2 and LPDDR3. For now, the new types are only
needed for topology description, so other properties like timing
parameters are omitted. They can be added later if needed.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 .../ddr/jedec,lpddr-props.yaml                |  4 ++
 .../memory-controllers/ddr/jedec,lpddr4.yaml  | 36 ++++++++++++++
 .../memory-controllers/ddr/jedec,lpddr5.yaml  | 48 +++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
index 0c7d2feafd77c8..e1182e75ca1a3f 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
@@ -53,9 +53,13 @@ properties:
       - 512
       - 1024
       - 2048
+      - 3072
       - 4096
+      - 6144
       - 8192
+      - 12288
       - 16384
+      - 24576
       - 32768
 
   io-width:
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
new file mode 100644
index 00000000000000..860300cebee754
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr4.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LPDDR4 SDRAM compliant to JEDEC JESD209-4
+
+allOf:
+  - $ref: "jedec,lpddr-props.yaml#"
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+
+properties:
+  compatible:
+    items:
+      - pattern: "^lpddr4-[0-9a-f]{2},[0-9a-f]{4}$"
+      - const: jedec,lpddr4
+
+required:
+  - compatible
+  - density
+  - io-width
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    lpddr4 {
+        compatible = "lpddr4-ff,0100", "jedec,lpddr4";
+        density = <8192>;
+        io-width = <16>;
+        manufacturer-id = <255>;
+        revision-id = <1 0>;
+    };
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml
new file mode 100644
index 00000000000000..ae3894bb346d5e
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LPDDR5 SDRAM compliant to JEDEC JESD209-5
+
+allOf:
+  - $ref: "jedec,lpddr-props.yaml#"
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+
+properties:
+  compatible:
+    items:
+      - pattern: "^lpddr5-[0-9a-f]{2},[0-9a-f]{4}$"
+      - const: jedec,lpddr5
+
+  serial-id:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Serial IDs read from Mode Registers 47 through 54. One byte per uint32
+      cell (i.e. <MR47 MR48 MR49 MR50 MR51 MR52 MR53 MR54>).
+    minItems: 8
+    maxItems: 8
+    items:
+      minimum: 0
+      maximum: 255
+
+required:
+  - compatible
+  - density
+  - io-width
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    lpddr4 {
+        compatible = "lpddr5-01,0200", "jedec,lpddr5";
+        density = <8192>;
+        io-width = <8>;
+        manufacturer-id = <1>;
+        revision-id = <2 0>;
+        serial-id = <3 1 0 0 0 0 0 0>;
+    };
-- 
2.31.0

[PATCH 4/4] dt-bindings: memory: Add jedec,lpddrX-channel binding

[Julius Werner]

This patch adds a new device tree binding for an LPDDR channel to serve
as a top-level organizing node for LPDDR part nodes nested below it. An
LPDDR channel needs to have an "io-width" property to describe its width
(this is important because this width does not always match the io-width
of the part number, indicating that multiple parts are wired in parallel
on the same channel), as well as one or more nested "rank@X" nodes.
Those represent information about the individual ranks of each LPDDR
part connected on that channel and should match the existing
"jedec,lpddrX" bindings for individual LPDDR parts.

New platforms should be using this node -- the existing practice of
providing a raw, toplevel "jedec,lpddrX" node without indication of how
many identical parts are in the system should be considered deprecated.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 .../ddr/jedec,lpddr-channel.yaml              | 116 ++++++++++++++++++
 .../ddr/jedec,lpddr-props.yaml                |  10 +-
 2 files changed, 125 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml
new file mode 100644
index 00000000000000..517e770d8e7133
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr-channel.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LPDDR channel with chip/rank topology description
+
+description:
+  An LPDDR channel is a completely independent set of LPDDR pins (DQ, CA, CS,
+  CK, etc.) that connect one or more LPDDR chips to a host system. The main
+  purpose of this node is to overall LPDDR topology of the system, including the
+  amount of individual LPDDR chips and the ranks per chip.
+
+maintainers:
+  - Julius Werner <jwerner@chromium.org>
+
+properties:
+  compatible:
+    enum:
+      - jedec,lpddr2-channel
+      - jedec,lpddr3-channel
+      - jedec,lpddr4-channel
+      - jedec,lpddr5-channel
+
+  io-width:
+    description:
+      The number of DQ pins in the channel. If this number is different
+      from (a multiple of) the io-width of the LPDDR chip, that means that
+      multiple instances of that type of chip are wired in parallel on this
+      channel (with the channel's DQ pins split up between the different
+      chips, and the CA, CS, etc. pins of the different chips all shorted
+      together).  This means that the total physical memory controlled by a
+      channel is equal to the sum of the densities of each rank on the
+      connected LPDDR chip, times the io-width of the channel divided by
+      the io-width of the LPDDR chip.
+    enum:
+      - 8
+      - 16
+      - 32
+      - 64
+      - 128
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^rank@[0-9]+$":
+    type: object
+    description:
+      Each physical LPDDR chip may have one or more ranks. Ranks are
+      internal but fully independent sub-units of the chip. Each LPDDR bus
+      transaction on the channel targets exactly one rank, based on the
+      state of the CS pins. Different ranks may have different densities and
+      timing requirements.
+    oneOf:
+      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr2.yaml#
+      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr3.yaml#
+      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr4.yaml#
+      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr5.yaml#
+    required:
+      - reg
+
+required:
+  - compatible
+  - io-width
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    lpddr-channel0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "jedec,lpddr4-channel";
+      io-width = <32>;
+
+      rank@0 {
+        compatible = "lpddr4-ff,0100", "jedec,lpddr4";
+        reg = <0>;
+        density = <8192>;
+        io-width = <16>;
+        manufacturer-id = <255>;
+        revision-id = <1 0>;
+      };
+    };
+
+    lpddr-channel1 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "jedec,lpddr4-channel";
+      io-width = <32>;
+
+      rank@0 {
+        compatible = "lpddr4-05,0301", "jedec,lpddr4";
+        reg = <0>;
+        density = <4096>;
+        io-width = <32>;
+        manufacturer-id = <5>;
+        revision-id = <3 1>;
+      };
+
+      rank@1 {
+        compatible = "lpddr4-05,0301", "jedec,lpddr4";
+        reg = <1>;
+        density = <2048>;
+        io-width = <32>;
+        manufacturer-id = <5>;
+        revision-id = <3 1>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
index e1182e75ca1a3f..53a4836028cd25 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
@@ -9,7 +9,8 @@ title: Common properties for LPDDR types
 description:
   Different LPDDR types generally use the same properties and only differ in the
   range of legal values for each. This file defines the common parts that can be
-  reused for each type.
+  reused for each type. Nodes using this schema should generally be nested under
+  an LPDDR channel node.
 
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
@@ -71,4 +72,11 @@ properties:
       - 16
       - 8
 
+  reg:
+    description:
+      The rank number of this LPDDR rank when used as a subnode to an LPDDR
+      channel.
+    minimum: 0
+    maximum: 3
+
 additionalProperties: true
-- 
2.31.0

Re: [PATCH 1/4] dt-bindings: memory: Factor out common properties of LPDDR bindings

[Krzysztof Kozlowski]

On 31/08/2022 04:33, Julius Werner wrote:
> The bindings for different LPDDR versions mostly use the same kinds of
> properties, so in order to reduce duplication when we're adding support
> for more versions, this patch creates a new lpddr-props subschema that
> can be referenced by the others to define these common parts. (This will
> consider a few smaller I/O width and density numbers "legal" for LPDDR3
> that are usually not used there, but this should be harmless.)
> 
> This also un-deprecates the manufacturer ID property for LPDDR3 (and
> introduces it to LPDDR2), since it was found that having this
> information available in a separate property can be useful in some
> cases.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  .../ddr/jedec,lpddr-props.yaml                | 60 +++++++++++++++++++
>  .../memory-controllers/ddr/jedec,lpddr2.yaml  | 40 ++-----------
>  .../memory-controllers/ddr/jedec,lpddr3.yaml  | 39 ++----------
>  3 files changed, 68 insertions(+), 71 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> new file mode 100644
> index 00000000000000..8b31c60ea2435b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr-props.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common properties for LPDDR types
> +
> +description:
> +  Different LPDDR types generally use the same properties and only differ in the
> +  range of legal values for each. This file defines the common parts that can be
> +  reused for each type.
> +
> +maintainers:
> +  - Krzysztof Kozlowski <krzk@kernel.org>
> +
> +properties:
> +  manufacturer-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Manufacturer ID read from Mode Register 5.

Are you sure that register numbers (here 5, 6-7-8 later) are the same
between LPDDR 2-5? The description should match the broadest case and
specific schema can narrow or correct it.

> +    minimum: 0
> +    maximum: 255
> +
> +  revision-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>).
> +    minItems: 2

No need for minItems.

> +    maxItems: 2
> +    items:
> +      minimum: 0
> +      maximum: 255
> +
> +  density:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Density in megabits of SDRAM chip. Decoded from Mode Register 8.
> +    enum:
> +      - 64
> +      - 128
> +      - 256
> +      - 512
> +      - 1024
> +      - 2048
> +      - 4096
> +      - 8192
> +      - 16384
> +      - 32768
> +
> +  io-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      IO bus width in bits of SDRAM chip. Decoded from Mode Register 8.
> +    enum:
> +      - 32
> +      - 16
> +      - 8

While moving, order it from lowest to highest.

> +
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
> index 9d78f140609b6c..63c47235cb9896 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
> @@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: LPDDR2 SDRAM compliant to JEDEC JESD209-2
>  
> +allOf:
> +  - $ref: "jedec,lpddr-props.yaml#"
> +

This goes just before "properties:"

>  maintainers:
>    - Krzysztof Kozlowski <krzk@kernel.org>
>  
> @@ -41,41 +44,6 @@ properties:
>        Property is deprecated, use revision-id instead.
>      deprecated: true
>  
> -  revision-id:
> -    $ref: /schemas/types.yaml#/definitions/uint32-array
> -    description: |
> -      Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>).
> -    minItems: 2
> -    maxItems: 2
> -    items:
> -      minimum: 0
> -      maximum: 255
> -
> -  density:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> -    description: |
> -      Density in megabits of SDRAM chip. Obtained from device datasheet.
> -    enum:
> -      - 64
> -      - 128
> -      - 256
> -      - 512
> -      - 1024
> -      - 2048
> -      - 4096
> -      - 8192
> -      - 16384
> -      - 32768
> -
> -  io-width:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> -    description: |
> -      IO bus width in bits of SDRAM chip. Obtained from device datasheet.
> -    enum:
> -      - 32
> -      - 16
> -      - 8
> -
>    tRRD-min-tck:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      maximum: 16
> @@ -168,7 +136,7 @@ required:
>    - density
>    - io-width
>  
> -additionalProperties: false
> +unevaluatedProperties: false
>  
>  examples:
>    - |
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
> index 48908a19473c3f..5969166cdc9e0f 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml
> @@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: LPDDR3 SDRAM compliant to JEDEC JESD209-3
>  
> +allOf:
> +  - $ref: "jedec,lpddr-props.yaml#"
> +

Also move below (before properties:)

>  maintainers:
>    - Krzysztof Kozlowski <krzk@kernel.org>
>  
> @@ -20,40 +23,6 @@ properties:
>      const: 1
>      deprecated: true
>  
> -  density:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> -    description: |
> -      Density in megabits of SDRAM chip.
> -    enum:
> -      - 4096
> -      - 8192
> -      - 16384
> -      - 32768

This must stay (so density with enum, but no ref and no description).

> -
> -  io-width:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> -    description: |
> -      IO bus width in bits of SDRAM chip.
> -    enum:
> -      - 32
> -      - 16

The same.

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: memory: Add numeric LPDDR compatible string variant

[Krzysztof Kozlowski]

On 31/08/2022 04:33, Julius Werner wrote:
> This patch allows a new kind of compatible string for LPDDR parts in the
> device tree bindings, in addition to the existing hardcoded
> <vendor>,<part-number> strings. The new format contains manufacturer and
> part (revision) information in numerical form, such as lpddr3-ff,0201
> for an LPDDR3 part with manufacturer ID ff and revision ID 0201. This
> helps cases where LPDDR parts are probed at runtime by boot firmware and
> cannot be matched to hardcoded part numbers.

Please describe your case here as example.

Best regards,
Krzysztof

Re: [PATCH 3/4] dt-bindings: memory: Add jedec,lpddr4 and jedec,lpddr5 bindings

[Krzysztof Kozlowski]

On 31/08/2022 04:33, Julius Werner wrote:
> This patch adds bindings for LPDDR4 and LPDDR5 memory analogous to the
> existing bindings for LPDDR2 and LPDDR3. For now, the new types are only
> needed for topology description, so other properties like timing
> parameters are omitted. They can be added later if needed.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  .../ddr/jedec,lpddr-props.yaml                |  4 ++
>  .../memory-controllers/ddr/jedec,lpddr4.yaml  | 36 ++++++++++++++
>  .../memory-controllers/ddr/jedec,lpddr5.yaml  | 48 +++++++++++++++++++
>  3 files changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> index 0c7d2feafd77c8..e1182e75ca1a3f 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> @@ -53,9 +53,13 @@ properties:
>        - 512
>        - 1024
>        - 2048
> +      - 3072
>        - 4096
> +      - 6144
>        - 8192
> +      - 12288
>        - 16384
> +      - 24576
>        - 32768

Either you limit now LPDDR2 and LPDDR3 to old values or instead add this
bigger list to LPDDR4 and LPDDR5 (if it works that way).

>  
>    io-width:
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
> new file mode 100644
> index 00000000000000..860300cebee754
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr4.yaml
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr4.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LPDDR4 SDRAM compliant to JEDEC JESD209-4
> +
> +allOf:
> +  - $ref: "jedec,lpddr-props.yaml#"

allOf goes below maintainers.

> +
> +maintainers:
> +  - Krzysztof Kozlowski <krzk@kernel.org>
> +
> +properties:
> +  compatible:
> +    items:
> +      - pattern: "^lpddr4-[0-9a-f]{2},[0-9a-f]{4}$"
> +      - const: jedec,lpddr4
> +
> +required:
> +  - compatible
> +  - density
> +  - io-width
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    lpddr4 {

Let's just call it "lpddr" to be generic.

> +        compatible = "lpddr4-ff,0100", "jedec,lpddr4";
> +        density = <8192>;
> +        io-width = <16>;
> +        manufacturer-id = <255>;
> +        revision-id = <1 0>;
> +    };
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml
> new file mode 100644
> index 00000000000000..ae3894bb346d5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr5.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LPDDR5 SDRAM compliant to JEDEC JESD209-5
> +
> +allOf:
> +  - $ref: "jedec,lpddr-props.yaml#"
> +
> +maintainers:
> +  - Krzysztof Kozlowski <krzk@kernel.org>
> +
> +properties:
> +  compatible:
> +    items:
> +      - pattern: "^lpddr5-[0-9a-f]{2},[0-9a-f]{4}$"
> +      - const: jedec,lpddr5
> +
> +  serial-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      Serial IDs read from Mode Registers 47 through 54. One byte per uint32
> +      cell (i.e. <MR47 MR48 MR49 MR50 MR51 MR52 MR53 MR54>).
> +    minItems: 8

No need for minItems.

> +    maxItems: 8
> +    items:
> +      minimum: 0
> +      maximum: 255
> +
> +required:
> +  - compatible
> +  - density
> +  - io-width
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    lpddr4 {

Let's just call it "lpddr" to be generic (it's lpddr5 anyway).

> +        compatible = "lpddr5-01,0200", "jedec,lpddr5";
> +        density = <8192>;
> +        io-width = <8>;
> +        manufacturer-id = <1>;
> +        revision-id = <2 0>;
> +        serial-id = <3 1 0 0 0 0 0 0>;
> +    };


Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: memory: Factor out common properties of LPDDR bindings

[Krzysztof Kozlowski]

On 31/08/2022 04:33, Julius Werner wrote:
> The bindings for different LPDDR versions mostly use the same kinds of
> properties, so in order to reduce duplication when we're adding support
> for more versions, this patch creates a new lpddr-props subschema that
> can be referenced by the others to define these common parts. (This will
> consider a few smaller I/O width and density numbers "legal" for LPDDR3
> that are usually not used there, but this should be harmless.)
> 
> This also un-deprecates the manufacturer ID property for LPDDR3 (and
> introduces it to LPDDR2), since it was found that having this
> information available in a separate property can be useful in some
> cases.

Why do you need to un-deprecate them if you have this information in
compatible? This was not exactly the previous consensus. My statement
was ok for un-deprecating if you cannot derive them from compatible. Now
you can. This should be the same as USB device schema.

Best regards,
Krzysztof

Re: [PATCH 0/4] dt-bindings: memory: Describing LPDDR topology

[Krzysztof Kozlowski]

On 31/08/2022 04:33, Julius Werner wrote:
> This patch series implements a proposal previously discussed on the
> mailing list under the topic `[RFC] Correct memory layout reporting for
> "jedec,lpddr2" and related bindings`. It adds a new jedec,lpddr-channel
> binding which should be used to group nodes of the existing jedec,lpddr
> bindings to describe their relative topology on the system and the
> amount of chips wired in parallel on each channel, as well as their
> different ranks. This also adds bindings for LPDDR4 and LPDDR5 memory
> types and deduplicates some common schema elements between different
> LPDDR types.
> 
> Julius Werner (4):
>   dt-bindings: memory: Factor out common properties of LPDDR bindings
>   dt-bindings: memory: Add numeric LPDDR compatible string variant
>   dt-bindings: memory: Add jedec,lpddr4 and jedec,lpddr5 bindings
>   dt-bindings: memory: Add jedec,lpddrX-channel binding

Thanks for the patches. Where are the users of these bindings? Although
bindings do not have requirement of providing user (as kernel API has),
but this is quite a rework so I want to see that it is applicable. That
it matches real use case and need. I can do it only with real DTS in the
kernel.

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: memory: Add jedec,lpddrX-channel binding

[Krzysztof Kozlowski]

On 31/08/2022 04:33, Julius Werner wrote:
> This patch adds a new device tree binding for an LPDDR channel to serve
> as a top-level organizing node for LPDDR part nodes nested below it. An
> LPDDR channel needs to have an "io-width" property to describe its width
> (this is important because this width does not always match the io-width
> of the part number, indicating that multiple parts are wired in parallel
> on the same channel), as well as one or more nested "rank@X" nodes.
> Those represent information about the individual ranks of each LPDDR
> part connected on that channel and should match the existing
> "jedec,lpddrX" bindings for individual LPDDR parts.
> 
> New platforms should be using this node -- the existing practice of
> providing a raw, toplevel "jedec,lpddrX" node without indication of how
> many identical parts are in the system should be considered deprecated.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  .../ddr/jedec,lpddr-channel.yaml              | 116 ++++++++++++++++++
>  .../ddr/jedec,lpddr-props.yaml                |  10 +-
>  2 files changed, 125 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml
> new file mode 100644
> index 00000000000000..517e770d8e7133
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-channel.yaml
> @@ -0,0 +1,116 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr-channel.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LPDDR channel with chip/rank topology description
> +
> +description:
> +  An LPDDR channel is a completely independent set of LPDDR pins (DQ, CA, CS,
> +  CK, etc.) that connect one or more LPDDR chips to a host system. The main
> +  purpose of this node is to overall LPDDR topology of the system, including the
> +  amount of individual LPDDR chips and the ranks per chip.

"channel" in this context confuses me a bit, because usually everyone is
talking about DDR controller channels, not memory channels. I think this
actually maps to a DDR controller channel?

> +
> +maintainers:
> +  - Julius Werner <jwerner@chromium.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - jedec,lpddr2-channel
> +      - jedec,lpddr3-channel
> +      - jedec,lpddr4-channel
> +      - jedec,lpddr5-channel
> +
> +  io-width:
> +    description:
> +      The number of DQ pins in the channel. If this number is different
> +      from (a multiple of) the io-width of the LPDDR chip, that means that
> +      multiple instances of that type of chip are wired in parallel on this
> +      channel (with the channel's DQ pins split up between the different
> +      chips, and the CA, CS, etc. pins of the different chips all shorted
> +      together).  This means that the total physical memory controlled by a
> +      channel is equal to the sum of the densities of each rank on the
> +      connected LPDDR chip, times the io-width of the channel divided by
> +      the io-width of the LPDDR chip.
> +    enum:
> +      - 8
> +      - 16
> +      - 32
> +      - 64
> +      - 128
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^rank@[0-9]+$":
> +    type: object
> +    description:
> +      Each physical LPDDR chip may have one or more ranks. Ranks are
> +      internal but fully independent sub-units of the chip. Each LPDDR bus
> +      transaction on the channel targets exactly one rank, based on the
> +      state of the CS pins. Different ranks may have different densities and
> +      timing requirements.
> +    oneOf:
> +      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr2.yaml#
> +      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr3.yaml#
> +      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr4.yaml#
> +      - $ref: /schemas/memory-controllers/ddr/jedec,lpddr5.yaml#

This should be rather chosen depending on top-level compatible. IOW, add
allOf where ref is chosen. Something like:
https://lore.kernel.org/linux-devicetree/20220828084341.112146-15-krzysztof.kozlowski@linaro.org/

Unless LPDDR3 memory can be put in LPDDR5 channel? But I think it cannot...

> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - io-width
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    lpddr-channel0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      compatible = "jedec,lpddr4-channel";
> +      io-width = <32>;
> +
> +      rank@0 {
> +        compatible = "lpddr4-ff,0100", "jedec,lpddr4";
> +        reg = <0>;
> +        density = <8192>;
> +        io-width = <16>;
> +        manufacturer-id = <255>;
> +        revision-id = <1 0>;
> +      };
> +    };
> +
> +    lpddr-channel1 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      compatible = "jedec,lpddr4-channel";
> +      io-width = <32>;

I wonder now, how does it exactly work - channel is 32 bits, two ranks
each with 32 bit IO bus. Your description said that:

total_ram = (rank0 + rank1) * (channel_width / chip_width)
so for this case: (4+2)*(32/32) = 6 Mbit

If channel io-width = <64>, then memories are stacked in parallel and
according to your description total RAM would be: (4+2)*(64/32) = 12 Mbit
I wonder why stacking memories in parallel increases their size?

> +
> +      rank@0 {
> +        compatible = "lpddr4-05,0301", "jedec,lpddr4";
> +        reg = <0>;
> +        density = <4096>;
> +        io-width = <32>;
> +        manufacturer-id = <5>;
> +        revision-id = <3 1>;
> +      };
> +
> +      rank@1 {
> +        compatible = "lpddr4-05,0301", "jedec,lpddr4";
> +        reg = <1>;
> +        density = <2048>;
> +        io-width = <32>;
> +        manufacturer-id = <5>;
> +        revision-id = <3 1>;
> +      };
> +    };
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> index e1182e75ca1a3f..53a4836028cd25 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> @@ -9,7 +9,8 @@ title: Common properties for LPDDR types
>  description:
>    Different LPDDR types generally use the same properties and only differ in the
>    range of legal values for each. This file defines the common parts that can be
> -  reused for each type.
> +  reused for each type. Nodes using this schema should generally be nested under
> +  an LPDDR channel node.
>  
>  maintainers:
>    - Krzysztof Kozlowski <krzk@kernel.org>
> @@ -71,4 +72,11 @@ properties:
>        - 16
>        - 8
>  
> +  reg:
> +    description:
> +      The rank number of this LPDDR rank when used as a subnode to an LPDDR
> +      channel.
> +    minimum: 0
> +    maximum: 3

Put reg just after compatible.

> +
>  additionalProperties: true


Best regards,
Krzysztof

Re: [PATCH 0/4] dt-bindings: memory: Describing LPDDR topology

[Julius Werner]

> Thanks for the patches. Where are the users of these bindings? Although
> bindings do not have requirement of providing user (as kernel API has),
> but this is quite a rework so I want to see that it is applicable. That
> it matches real use case and need. I can do it only with real DTS in the
> kernel.

Well, the whole point of the new compatible string format is that it
can be generated by boot firmware at runtime, so I don't have a static
DTS with this that I can check into the kernel tree. The first user of
these bindings will be the
arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts board that's
already in the tree, but since these nodes get generated you won't see
them in that file. It's kinda like /chosen/kaslr-seed, that's also a
valid binding with a schema description that's actively being used but
doesn't show up in any DTS file checked into the tree.

Re: [PATCH 1/4] dt-bindings: memory: Factor out common properties of LPDDR bindings

[Julius Werner]

> > +properties:
> > +  manufacturer-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Manufacturer ID read from Mode Register 5.
>
> Are you sure that register numbers (here 5, 6-7-8 later) are the same
> between LPDDR 2-5? The description should match the broadest case and
> specific schema can narrow or correct it.

Yes, the new LPDDR versions only ever seem to add new mode registers,
but the meaning of 5, 6 and 7 have always stayed the same. (For 8, the
bit fields have remained the same but the mapping of bit patterns to
values has changed.)

> > This also un-deprecates the manufacturer ID property for LPDDR3 (and
> > introduces it to LPDDR2), since it was found that having this
> > information available in a separate property can be useful in some
> > cases.
>
> Why do you need to un-deprecate them if you have this information in
> compatible? This was not exactly the previous consensus. My statement
> was ok for un-deprecating if you cannot derive them from compatible. Now
> you can. This should be the same as USB device schema.

Okay. I think there is value in having these as separate properties,
because it makes them much easier to read and use. If we don't have
them we always need to do all this string parsing first, and if
systems allow both kinds of compatible strings (auto-generated and
static) they'll need to be able to distinguish and handle both of
those when parsing... I think it would be much less friction if each
datum of interest could directly be read out of a property. I think
having a bit of redundancy is fine and common in device tree bindings
(e.g. most properties for most devices are really implied by the
compatible string because that same type of device is always built in
the same way, but that doesn't mean it's not useful to list them
separately for ease-of-access). But I can remove them if you disagree.

Re: [PATCH 3/4] dt-bindings: memory: Add jedec,lpddr4 and jedec,lpddr5 bindings

[Julius Werner]

> > diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > index 0c7d2feafd77c8..e1182e75ca1a3f 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > @@ -53,9 +53,13 @@ properties:
> >        - 512
> >        - 1024
> >        - 2048
> > +      - 3072
> >        - 4096
> > +      - 6144
> >        - 8192
> > +      - 12288
> >        - 16384
> > +      - 24576
> >        - 32768
>
> Either you limit now LPDDR2 and LPDDR3 to old values or instead add this
> bigger list to LPDDR4 and LPDDR5 (if it works that way).

The problem is that each spec has its own set of valid values, e.g.
LPDDR3 only defines 4GB, 8GB, 16GB and 32GB, and then LPDDR4 inserted
the 6GB, 12GB and 24GB options in between there. I don't think there's
a way to exactly describe the valid values for each version without
having a whole separate enum list for each. Do you think checking for
that is important enough to be worth having all that extra duplication
between the schemas? I don't think it adds that much (e.g. a value for
an individual memory part can still be wrong even if it is one of the
valid values for that type, so how much use is this validation
anyway?), but I can split it out if you want to.

> > +  serial-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      Serial IDs read from Mode Registers 47 through 54. One byte per uint32
> > +      cell (i.e. <MR47 MR48 MR49 MR50 MR51 MR52 MR53 MR54>).
> > +    minItems: 8
>
> No need for minItems.

Can you explain why? I'm okay with taking these out, but it is a real
constraint so I'm not sure why we shouldn't be describing it here?
(The serial ID always has exactly 8 bytes, an ID with less than 8
would not be valid and probably a typo.)

Re: [PATCH 4/4] dt-bindings: memory: Add jedec,lpddrX-channel binding

[Julius Werner]

> > +description:
> > +  An LPDDR channel is a completely independent set of LPDDR pins (DQ, CA, CS,
> > +  CK, etc.) that connect one or more LPDDR chips to a host system. The main
> > +  purpose of this node is to overall LPDDR topology of the system, including the
> > +  amount of individual LPDDR chips and the ranks per chip.
>
> "channel" in this context confuses me a bit, because usually everyone is
> talking about DDR controller channels, not memory channels. I think this
> actually maps to a DDR controller channel?

I'm not really sure what you mean by "memory channel" here (that would
be different from the DDR controller channel)? According to my
understanding there's only one kind of "channel" in the context of
main memory, that's the DDR controller channel (i.e. each separate
complete set of DDR pins coming out of the controller, as I tried to
explain in the description).

> > +    lpddr-channel1 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      compatible = "jedec,lpddr4-channel";
> > +      io-width = <32>;
>
> I wonder now, how does it exactly work - channel is 32 bits, two ranks
> each with 32 bit IO bus. Your description said that:
>
> total_ram = (rank0 + rank1) * (channel_width / chip_width)
> so for this case: (4+2)*(32/32) = 6 Mbit
>
> If channel io-width = <64>, then memories are stacked in parallel and
> according to your description total RAM would be: (4+2)*(64/32) = 12 Mbit
> I wonder why stacking memories in parallel increases their size?

Well, stacking in parallel just means you have more of them? In the
original example, you have a single LPDDR chip with two ranks, one
4Gbit rank and one 2Gbit rank. That chip is directly hooked up to the
LPDDR controller and that's the only chip you have, so you have 4+2 =
6Gbit total memory in the system.

In your next example, the LPDDR controller has a 64 bit wide channel,
but you're still using that same 6Gbit LPDDR chip that only has 32 DQ
pins. The only way to fill out that 64 bit channel with this kind of
chip is to have two of them in parallel (one connected to DQ[0:31] and
one connected to DQ[32:63]). So we infer from the mismatch in io-width
that we have two chips. Each chip still has 6Gbit of memory, so the
total system would have 12Gbit.

Re: [PATCH 3/4] dt-bindings: memory: Add jedec,lpddr4 and jedec,lpddr5 bindings

[Rob Herring]

On Wed, Aug 31, 2022 at 06:10:51PM -0700, Julius Werner wrote:
> > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > > index 0c7d2feafd77c8..e1182e75ca1a3f 100644
> > > --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > > +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > > @@ -53,9 +53,13 @@ properties:
> > >        - 512
> > >        - 1024
> > >        - 2048
> > > +      - 3072
> > >        - 4096
> > > +      - 6144
> > >        - 8192
> > > +      - 12288
> > >        - 16384
> > > +      - 24576
> > >        - 32768
> >
> > Either you limit now LPDDR2 and LPDDR3 to old values or instead add this
> > bigger list to LPDDR4 and LPDDR5 (if it works that way).
> 
> The problem is that each spec has its own set of valid values, e.g.
> LPDDR3 only defines 4GB, 8GB, 16GB and 32GB, and then LPDDR4 inserted
> the 6GB, 12GB and 24GB options in between there. I don't think there's
> a way to exactly describe the valid values for each version without
> having a whole separate enum list for each. Do you think checking for
> that is important enough to be worth having all that extra duplication
> between the schemas? I don't think it adds that much (e.g. a value for
> an individual memory part can still be wrong even if it is one of the
> valid values for that type, so how much use is this validation
> anyway?), but I can split it out if you want to.

I tend to agree with you that it isn't worth the complexity.


> > > +  serial-id:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +    description:
> > > +      Serial IDs read from Mode Registers 47 through 54. One byte per uint32
> > > +      cell (i.e. <MR47 MR48 MR49 MR50 MR51 MR52 MR53 MR54>).
> > > +    minItems: 8
> >
> > No need for minItems.
> 
> Can you explain why? I'm okay with taking these out, but it is a real
> constraint so I'm not sure why we shouldn't be describing it here?
> (The serial ID always has exactly 8 bytes, an ID with less than 8
> would not be valid and probably a typo.)

Because if minItems is not specified, then it defaults to same as 
maxItems value. This is a departure from json-schema, but we almost 
always need a fixed number here and I didn't want to be specifying 
minItems/maxItems everywhere. We really need a 'numItems' or something.

Rob 

Re: [PATCH 1/4] dt-bindings: memory: Factor out common properties of LPDDR bindings

[Krzysztof Kozlowski]

On 01/09/2022 03:09, Julius Werner wrote:
>>> +properties:
>>> +  manufacturer-id:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Manufacturer ID read from Mode Register 5.
>>
>> Are you sure that register numbers (here 5, 6-7-8 later) are the same
>> between LPDDR 2-5? The description should match the broadest case and
>> specific schema can narrow or correct it.
> 
> Yes, the new LPDDR versions only ever seem to add new mode registers,
> but the meaning of 5, 6 and 7 have always stayed the same. (For 8, the
> bit fields have remained the same but the mapping of bit patterns to
> values has changed.)
> 
>>> This also un-deprecates the manufacturer ID property for LPDDR3 (and
>>> introduces it to LPDDR2), since it was found that having this
>>> information available in a separate property can be useful in some
>>> cases.
>>
>> Why do you need to un-deprecate them if you have this information in
>> compatible? This was not exactly the previous consensus. My statement
>> was ok for un-deprecating if you cannot derive them from compatible. Now
>> you can. This should be the same as USB device schema.
> 
> Okay. I think there is value in having these as separate properties,
> because it makes them much easier to read and use. 

Storing same value in multiple places is duplication and maintenance
effort. Does not make anything easier.


> If we don't have
> them we always need to do all this string parsing first, and if
> systems allow both kinds of compatible strings (auto-generated and
> static) they'll need to be able to distinguish and handle both of
> those when parsing... I think it would be much less friction if each
> datum of interest could directly be read out of a property. I think
> having a bit of redundancy is fine and common in device tree bindings

No, it's not common.

> (e.g. most properties for most devices are really implied by the
> compatible string because that same type of device is always built in
> the same way, but that doesn't mean it's not useful to list them
> separately for ease-of-access). But I can remove them if you disagree.

Just like we do not have them for USB, I don't really see the reason to
have them for memory.

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: memory: Add jedec,lpddrX-channel binding

[Krzysztof Kozlowski]

On 01/09/2022 03:11, Julius Werner wrote:
>>> +description:
>>> +  An LPDDR channel is a completely independent set of LPDDR pins (DQ, CA, CS,
>>> +  CK, etc.) that connect one or more LPDDR chips to a host system. The main
>>> +  purpose of this node is to overall LPDDR topology of the system, including the
>>> +  amount of individual LPDDR chips and the ranks per chip.
>>
>> "channel" in this context confuses me a bit, because usually everyone is
>> talking about DDR controller channels, not memory channels. I think this
>> actually maps to a DDR controller channel?
> 
> I'm not really sure what you mean by "memory channel" here (that would
> be different from the DDR controller channel)? According to my
> understanding there's only one kind of "channel" in the context of
> main memory, that's the DDR controller channel (i.e. each separate
> complete set of DDR pins coming out of the controller, as I tried to
> explain in the description).
> 
>>> +    lpddr-channel1 {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +      compatible = "jedec,lpddr4-channel";
>>> +      io-width = <32>;
>>
>> I wonder now, how does it exactly work - channel is 32 bits, two ranks
>> each with 32 bit IO bus. Your description said that:
>>
>> total_ram = (rank0 + rank1) * (channel_width / chip_width)
>> so for this case: (4+2)*(32/32) = 6 Mbit
>>
>> If channel io-width = <64>, then memories are stacked in parallel and
>> according to your description total RAM would be: (4+2)*(64/32) = 12 Mbit
>> I wonder why stacking memories in parallel increases their size?
> 
> Well, stacking in parallel just means you have more of them? In the
> original example, you have a single LPDDR chip with two ranks, one
> 4Gbit rank and one 2Gbit rank. That chip is directly hooked up to the
> LPDDR controller and that's the only chip you have, so you have 4+2 =
> 6Gbit total memory in the system.
> 
> In your next example, the LPDDR controller has a 64 bit wide channel,
> but you're still using that same 6Gbit LPDDR chip that only has 32 DQ
> pins. The only way to fill out that 64 bit channel with this kind of
> chip is to have two of them in parallel (one connected to DQ[0:31] and
> one connected to DQ[32:63]). So we infer from the mismatch in io-width
> that we have two chips. Each chip still has 6Gbit of memory, so the
> total system would have 12Gbit.

Two chips so more device nodes? Since there are no DTSes with it, please
provide an additional example in the bindings.

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: memory: Add jedec,lpddrX-channel binding

[Julius Werner]

> > Well, stacking in parallel just means you have more of them? In the
> > original example, you have a single LPDDR chip with two ranks, one
> > 4Gbit rank and one 2Gbit rank. That chip is directly hooked up to the
> > LPDDR controller and that's the only chip you have, so you have 4+2 =
> > 6Gbit total memory in the system.
> >
> > In your next example, the LPDDR controller has a 64 bit wide channel,
> > but you're still using that same 6Gbit LPDDR chip that only has 32 DQ
> > pins. The only way to fill out that 64 bit channel with this kind of
> > chip is to have two of them in parallel (one connected to DQ[0:31] and
> > one connected to DQ[32:63]). So we infer from the mismatch in io-width
> > that we have two chips. Each chip still has 6Gbit of memory, so the
> > total system would have 12Gbit.
>
> Two chips so more device nodes? Since there are no DTSes with it, please
> provide an additional example in the bindings.

No, there isn't a separate node for each chip in this case. There's
still only one node (per rank), but if the io-width of the rank node
is smaller than the io-width of the channel node, that implicitly
indicates that there are in fact multiple chips of the same type wired
in parallel on that channel. I tried to explain this in the
description for the channel's io-width property.

I chose to model it this way because having separate nodes for each
chip would be redundant since all their properties have to be equal
anyway, and because it more closely resembles the way this looks to
the firmware and the DDR controller. The DDR controller doesn't
actually "see" that there are multiple separate chips and cannot
enumerate them as individual entities, because only the DQ pins are
split among the different chips -- all other pins like chip select and
column address are shorted together between all the parallel chips,
and mode register values are only returned through the lowest DQ pins
(DQ[7:0]). So it's impossible for the DDR controller to read mode
register values from the other chips, it can only read them from the
first chip and it must trust that all the other chips are the exact
same part number, because that's the only valid way to wire this (and
when the controller writes timing configuration to the mode registers,
the same value is written out to all chips at once via the shorted
column address pins).

My example does contain this case already, in lpddr-channel0, rank@0:
there's only one rank node with density 8Gbits, but since that node
has io-width 16 and the channel has io-width 32, it is implied that
there are actually two single-rank chips wired in parallel on this
channel, and since each of them have 8Gbits of memory the channel has
16Gbits in total.