diff mbox series

[v4,2/7] dt-bindings: memory: lpddr2: Convert to schema

Message ID 20211005230009.3635-3-digetx@gmail.com
State Superseded, archived
Headers show
Series tegra20-emc: Identify memory chip by LPDDR configuration | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 1 warnings, 195 lines checked

Commit Message

Dmitry Osipenko Oct. 5, 2021, 11 p.m. UTC 
Convert LPDDR2 binding to schema. I removed obsolete ti,jedec-lpddr2-*
compatibles since they were never used by device-trees and by the code.

Suggested-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 .../memory-controllers/ddr/jedec,lpddr2.yaml  | 195 ++++++++++++++++++
 .../memory-controllers/ddr/lpddr2.txt         | 102 ---------
 2 files changed, 195 insertions(+), 102 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
 delete mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/lpddr2.txt

Comments

Krzysztof Kozlowski Oct. 6, 2021, 10:57 a.m. UTC | #1 
On 06/10/2021 01:00, Dmitry Osipenko wrote:
> Convert LPDDR2 binding to schema. I removed obsolete ti,jedec-lpddr2-*
> compatibles since they were never used by device-trees and by the code.
> 
> Suggested-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../memory-controllers/ddr/jedec,lpddr2.yaml  | 195 ++++++++++++++++++
>  .../memory-controllers/ddr/lpddr2.txt         | 102 ---------
>  2 files changed, 195 insertions(+), 102 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
>  delete mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/lpddr2.txt
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
> new file mode 100644
> index 000000000000..d99ccad54938
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
> @@ -0,0 +1,195 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LPDDR2 SDRAM compliant to JEDEC JESD209-2
> +
> +maintainers:
> +  - Krzysztof Kozlowski <krzk@kernel.org>

My Canonical address please, so:
Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - Elpida,ECB240ABACN
> +          - enum:
> +              - jedec,lpddr2-s4
> +      - items:
> +          - enum:
> +              - jedec,lpddr2-s2
> +      - items:
> +          - enum:
> +              - jedec,lpddr2-nvm
> +
> +  density:
> +    description: |
> +      Density in megabits of SDRAM chip. Obtained from device datasheet.

You need here a type/ref, so uint32.

> +    enum:
> +      - 64
> +      - 128
> +      - 256
> +      - 512
> +      - 1024
> +      - 2048
> +      - 4096
> +      - 8192
> +      - 16384
> +      - 32768
> +
> +  io-width:
> +    description: |
> +      IO bus width in bits of SDRAM chip. Obtained from device datasheet.

You need here a type/ref, so uint32.

> +    enum:
> +      - 32
> +      - 16
> +      - 8
> +
> +  tRRD-min-tck:
> +    maximum: 16

Here and further type is needed.


> +    description: |
> +      Active bank a to active bank b in terms of number of clock cycles.
> +      Obtained from device datasheet.
> +
> +  tWTR-min-tck:
> +    maximum: 16
> +    description: |
> +      Internal WRITE-to-READ command delay in terms of number of clock cycles.
> +      Obtained from device datasheet.
> +

Best regards,
Krzysztof
Dmitry Osipenko Oct. 6, 2021, 3:41 p.m. UTC | #2 
06.10.2021 13:57, Krzysztof Kozlowski пишет:
>> +  density:
>> +    description: |
>> +      Density in megabits of SDRAM chip. Obtained from device datasheet.
> You need here a type/ref, so uint32.
> 

The type is uint32 by default. I can add it, but it's not really necessary.
Dmitry Osipenko Oct. 6, 2021, 3:44 p.m. UTC | #3 
06.10.2021 18:41, Dmitry Osipenko пишет:
> 06.10.2021 13:57, Krzysztof Kozlowski пишет:
>>> +  density:
>>> +    description: |
>>> +      Density in megabits of SDRAM chip. Obtained from device datasheet.
>> You need here a type/ref, so uint32.
>>
> 
> The type is uint32 by default. I can add it, but it's not really necessary.
> 

You may grep bindings for 'enum: [' to see that nobody is specifying the
type.
Krzysztof Kozlowski Oct. 6, 2021, 5:21 p.m. UTC | #4 
On 06/10/2021 17:41, Dmitry Osipenko wrote:
> 06.10.2021 13:57, Krzysztof Kozlowski пишет:
>>> +  density:
>>> +    description: |
>>> +      Density in megabits of SDRAM chip. Obtained from device datasheet.
>> You need here a type/ref, so uint32.
>>
> 
> The type is uint32 by default. I can add it, but it's not really necessary.

Hmmm, maybe I missed some background here. Who sets uint32 as default?
The schema does not define "density" so this is more like a vendor-type
property which required therefore a type.

Also Rob's pointed this to my patch here around "op_mode":
https://lore.kernel.org/linux-samsung-soc/cdcd4eda-a7a9-2aa2-1316-e7184ff30bf3@canonical.com/T/#m17adea693a9cbcca75b0ba6f9d878f5fd1bf5d14

Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 6, 2021, 5:23 p.m. UTC | #5 
On 06/10/2021 17:44, Dmitry Osipenko wrote:
> 06.10.2021 18:41, Dmitry Osipenko пишет:
>> 06.10.2021 13:57, Krzysztof Kozlowski пишет:
>>>> +  density:
>>>> +    description: |
>>>> +      Density in megabits of SDRAM chip. Obtained from device datasheet.
>>> You need here a type/ref, so uint32.
>>>
>>
>> The type is uint32 by default. I can add it, but it's not really necessary.
>>
> 
> You may grep bindings for 'enum: [' to see that nobody is specifying the
> type.
> 

Just because everyone makes a mistake, is not a proof it should be done
like that. Please see example schema and vendor,int-property.

AFAIR, only properties defined by schema (directly or by unit suffix,
e.g. microvolt) do not need types.

Best regards,
Krzysztof
Dmitry Osipenko Oct. 6, 2021, 5:31 p.m. UTC | #6 
06.10.2021 20:23, Krzysztof Kozlowski пишет:
> On 06/10/2021 17:44, Dmitry Osipenko wrote:
>> 06.10.2021 18:41, Dmitry Osipenko пишет:
>>> 06.10.2021 13:57, Krzysztof Kozlowski пишет:
>>>>> +  density:
>>>>> +    description: |
>>>>> +      Density in megabits of SDRAM chip. Obtained from device datasheet.
>>>> You need here a type/ref, so uint32.
>>>>
>>>
>>> The type is uint32 by default. I can add it, but it's not really necessary.
>>>
>>
>> You may grep bindings for 'enum: [' to see that nobody is specifying the
>> type.
>>
> 
> Just because everyone makes a mistake, is not a proof it should be done
> like that. Please see example schema and vendor,int-property.
> 
> AFAIR, only properties defined by schema (directly or by unit suffix,
> e.g. microvolt) do not need types.

I'm not DT expert, but dt_binding_check says that binding is valid, even
with W=1. Meanwhile negative values are wrong. All integers are u32 by
default for DTB.

Alright, I'll just add the type since it won't hurt anyways.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
new file mode 100644
index 000000000000..d99ccad54938
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
@@ -0,0 +1,195 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LPDDR2 SDRAM compliant to JEDEC JESD209-2
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - Elpida,ECB240ABACN
+          - enum:
+              - jedec,lpddr2-s4
+      - items:
+          - enum:
+              - jedec,lpddr2-s2
+      - items:
+          - enum:
+              - jedec,lpddr2-nvm
+
+  density:
+    description: |
+      Density in megabits of SDRAM chip. Obtained from device datasheet.
+    enum:
+      - 64
+      - 128
+      - 256
+      - 512
+      - 1024
+      - 2048
+      - 4096
+      - 8192
+      - 16384
+      - 32768
+
+  io-width:
+    description: |
+      IO bus width in bits of SDRAM chip. Obtained from device datasheet.
+    enum:
+      - 32
+      - 16
+      - 8
+
+  tRRD-min-tck:
+    maximum: 16
+    description: |
+      Active bank a to active bank b in terms of number of clock cycles.
+      Obtained from device datasheet.
+
+  tWTR-min-tck:
+    maximum: 16
+    description: |
+      Internal WRITE-to-READ command delay in terms of number of clock cycles.
+      Obtained from device datasheet.
+
+  tXP-min-tck:
+    maximum: 16
+    description: |
+      Exit power-down to next valid command delay in terms of number of clock
+      cycles. Obtained from device datasheet.
+
+  tRTP-min-tck:
+    maximum: 16
+    description: |
+      Internal READ to PRECHARGE command delay in terms of number of clock
+      cycles. Obtained from device datasheet.
+
+  tCKE-min-tck:
+    maximum: 16
+    description: |
+      CKE minimum pulse width (HIGH and LOW pulse width) in terms of number
+      of clock cycles. Obtained from device datasheet.
+
+  tRPab-min-tck:
+    maximum: 16
+    description: |
+      Row precharge time (all banks) in terms of number of clock cycles.
+      Obtained from device datasheet.
+
+  tRCD-min-tck:
+    maximum: 16
+    description: |
+      RAS-to-CAS delay in terms of number of clock cycles. Obtained from
+      device datasheet.
+
+  tWR-min-tck:
+    maximum: 16
+    description: |
+      WRITE recovery time in terms of number of clock cycles. Obtained from
+      device datasheet.
+
+  tRASmin-min-tck:
+    maximum: 16
+    description: |
+      Row active time in terms of number of clock cycles. Obtained from device
+      datasheet.
+
+  tCKESR-min-tck:
+    maximum: 16
+    description: |
+      CKE minimum pulse width during SELF REFRESH (low pulse width during
+      SELF REFRESH) in terms of number of clock cycles. Obtained from device
+      datasheet.
+
+  tFAW-min-tck:
+    maximum: 16
+    description: |
+      Four-bank activate window in terms of number of clock cycles. Obtained
+      from device datasheet.
+
+patternProperties:
+  "^lpddr2-timings":
+    type: object
+    description: |
+      The lpddr2 node may have one or more child nodes of type "lpddr2-timings".
+      "lpddr2-timings" provides AC timing parameters of the device for
+      a given speed-bin. The user may provide the timings for as many
+      speed-bins as is required. Please see Documentation/devicetree/
+      bindings/memory-controllers/ddr/lpddr2-timings.txt for more information
+      on "lpddr2-timings".
+
+required:
+  - compatible
+  - density
+  - io-width
+
+additionalProperties: false
+
+examples:
+  - |
+    elpida_ECB240ABACN: lpddr2 {
+        compatible = "Elpida,ECB240ABACN", "jedec,lpddr2-s4";
+        density = <2048>;
+        io-width = <32>;
+
+        tRPab-min-tck = <3>;
+        tRCD-min-tck = <3>;
+        tWR-min-tck = <3>;
+        tRASmin-min-tck = <3>;
+        tRRD-min-tck = <2>;
+        tWTR-min-tck = <2>;
+        tXP-min-tck = <2>;
+        tRTP-min-tck = <2>;
+        tCKE-min-tck = <3>;
+        tCKESR-min-tck = <3>;
+        tFAW-min-tck = <8>;
+
+        timings_elpida_ECB240ABACN_400mhz: lpddr2-timings0 {
+            compatible = "jedec,lpddr2-timings";
+            min-freq = <10000000>;
+            max-freq = <400000000>;
+            tRPab = <21000>;
+            tRCD = <18000>;
+            tWR = <15000>;
+            tRAS-min = <42000>;
+            tRRD = <10000>;
+            tWTR = <7500>;
+            tXP = <7500>;
+            tRTP = <7500>;
+            tCKESR = <15000>;
+            tDQSCK-max = <5500>;
+            tFAW = <50000>;
+            tZQCS = <90000>;
+            tZQCL = <360000>;
+            tZQinit = <1000000>;
+            tRAS-max-ns = <70000>;
+        };
+
+        timings_elpida_ECB240ABACN_200mhz: lpddr2-timings1 {
+            compatible = "jedec,lpddr2-timings";
+            min-freq = <10000000>;
+            max-freq = <200000000>;
+            tRPab = <21000>;
+            tRCD = <18000>;
+            tWR = <15000>;
+            tRAS-min = <42000>;
+            tRRD = <10000>;
+            tWTR = <10000>;
+            tXP = <7500>;
+            tRTP = <7500>;
+            tCKESR = <15000>;
+            tDQSCK-max = <5500>;
+            tFAW = <50000>;
+            tZQCS = <90000>;
+            tZQCL = <360000>;
+            tZQinit = <1000000>;
+            tRAS-max-ns = <70000>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/lpddr2.txt b/Documentation/devicetree/bindings/memory-controllers/ddr/lpddr2.txt
deleted file mode 100644
index ddd40121e6f6..000000000000
--- a/Documentation/devicetree/bindings/memory-controllers/ddr/lpddr2.txt
+++ /dev/null
@@ -1,102 +0,0 @@ 
-* LPDDR2 SDRAM memories compliant to JEDEC JESD209-2
-
-Required properties:
-- compatible : Should be one of - "jedec,lpddr2-nvm", "jedec,lpddr2-s2",
-  "jedec,lpddr2-s4"
-
-  "ti,jedec-lpddr2-s2" should be listed if the memory part is LPDDR2-S2 type
-
-  "ti,jedec-lpddr2-s4" should be listed if the memory part is LPDDR2-S4 type
-
-  "ti,jedec-lpddr2-nvm" should be listed if the memory part is LPDDR2-NVM type
-
-- density  : <u32> representing density in Mb (Mega bits)
-
-- io-width : <u32> representing bus width. Possible values are 8, 16, and 32
-
-Optional properties:
-
-The following optional properties represent the minimum value of some AC
-timing parameters of the DDR device in terms of number of clock cycles.
-These values shall be obtained from the device data-sheet.
-- tRRD-min-tck
-- tWTR-min-tck
-- tXP-min-tck
-- tRTP-min-tck
-- tCKE-min-tck
-- tRPab-min-tck
-- tRCD-min-tck
-- tWR-min-tck
-- tRASmin-min-tck
-- tCKESR-min-tck
-- tFAW-min-tck
-
-Child nodes:
-- The lpddr2 node may have one or more child nodes of type "lpddr2-timings".
-  "lpddr2-timings" provides AC timing parameters of the device for
-  a given speed-bin. The user may provide the timings for as many
-  speed-bins as is required. Please see Documentation/devicetree/
-  bindings/ddr/lpddr2-timings.txt for more information on "lpddr2-timings"
-
-Example:
-
-elpida_ECB240ABACN : lpddr2 {
-	compatible	= "Elpida,ECB240ABACN","jedec,lpddr2-s4";
-	density		= <2048>;
-	io-width	= <32>;
-
-	tRPab-min-tck	= <3>;
-	tRCD-min-tck	= <3>;
-	tWR-min-tck	= <3>;
-	tRASmin-min-tck	= <3>;
-	tRRD-min-tck	= <2>;
-	tWTR-min-tck	= <2>;
-	tXP-min-tck	= <2>;
-	tRTP-min-tck	= <2>;
-	tCKE-min-tck	= <3>;
-	tCKESR-min-tck	= <3>;
-	tFAW-min-tck	= <8>;
-
-	timings_elpida_ECB240ABACN_400mhz: lpddr2-timings@0 {
-		compatible	= "jedec,lpddr2-timings";
-		min-freq	= <10000000>;
-		max-freq	= <400000000>;
-		tRPab		= <21000>;
-		tRCD		= <18000>;
-		tWR		= <15000>;
-		tRAS-min	= <42000>;
-		tRRD		= <10000>;
-		tWTR		= <7500>;
-		tXP		= <7500>;
-		tRTP		= <7500>;
-		tCKESR		= <15000>;
-		tDQSCK-max	= <5500>;
-		tFAW		= <50000>;
-		tZQCS		= <90000>;
-		tZQCL		= <360000>;
-		tZQinit		= <1000000>;
-		tRAS-max-ns	= <70000>;
-	};
-
-	timings_elpida_ECB240ABACN_200mhz: lpddr2-timings@1 {
-		compatible	= "jedec,lpddr2-timings";
-		min-freq	= <10000000>;
-		max-freq	= <200000000>;
-		tRPab		= <21000>;
-		tRCD		= <18000>;
-		tWR		= <15000>;
-		tRAS-min	= <42000>;
-		tRRD		= <10000>;
-		tWTR		= <10000>;
-		tXP		= <7500>;
-		tRTP		= <7500>;
-		tCKESR		= <15000>;
-		tDQSCK-max	= <5500>;
-		tFAW		= <50000>;
-		tZQCS		= <90000>;
-		tZQCL		= <360000>;
-		tZQinit		= <1000000>;
-		tRAS-max-ns	= <70000>;
-	};
-
-}