diff mbox series

[v5,3/5] dt-bindings: memory: Add bindings for imx8m ddr controller

Message ID 872fb6e3117955b679678280483f82b3d73dd376.1573595319.git.leonard.crestez@nxp.com (mailing list archive)
State Superseded
Headers show
Series PM / devfreq: Add dynamic scaling for imx8m ddr controller | expand

Commit Message

Leonard Crestez Nov. 12, 2019, 9:50 p.m. UTC 
Add devicetree bindings for the i.MX DDR Controller on imx8m series
chips. It supports dynamic frequency switching between multiple data
rates and this is exposed to Linux via the devfreq subsystem.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 .../memory-controllers/fsl/imx8m-ddrc.yaml    | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml

Comments

Chanwoo Choi Nov. 13, 2019, 2:38 a.m. UTC | #1 
On 11/13/19 6:50 AM, Leonard Crestez wrote:
> Add devicetree bindings for the i.MX DDR Controller on imx8m series
> chips. It supports dynamic frequency switching between multiple data
> rates and this is exposed to Linux via the devfreq subsystem.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  .../memory-controllers/fsl/imx8m-ddrc.yaml    | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
> new file mode 100644
> index 000000000000..7c98e3509f75
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/fsl/imx8m-ddrc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX8M DDR Controller
> +
> +maintainers:
> +  - Leonard Crestez <leonard.crestez@nxp.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - fsl,imx8mn-ddrc
> +        - fsl,imx8mm-ddrc
> +        - fsl,imx8mq-ddrc
> +      - const: fsl,imx8m-ddrc
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: pll
> +      - const: alt
> +      - const: apb
> +
> +  operating-points-v2: true
> +  opp-table: true
> +
> +required:
> +  - reg
> +  - compatible
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8mm-clock.h>
> +    ddrc: memory-controller@3d400000 {
> +        compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
> +        reg = <0x3d400000 0x400000>;

The probe() function doesn't get the IORESOURCE_MEM from dt?
Is it needed?

> +        clock-names = "core", "pll", "alt", "apb";
> +        clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
> +                 <&clk IMX8MM_DRAM_PLL>,
> +                 <&clk IMX8MM_CLK_DRAM_ALT>,
> +                 <&clk IMX8MM_CLK_DRAM_APB>;
> +        operating-points-v2 = <&ddrc_opp_table>;
> +    };
>
Leonard Crestez Nov. 13, 2019, 12:35 p.m. UTC | #2 
On 13.11.2019 04:32, Chanwoo Choi wrote:
> On 11/13/19 6:50 AM, Leonard Crestez wrote:
>> Add devicetree bindings for the i.MX DDR Controller on imx8m series
>> chips. It supports dynamic frequency switching between multiple data
>> rates and this is exposed to Linux via the devfreq subsystem.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   .../memory-controllers/fsl/imx8m-ddrc.yaml    | 57 +++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
>> new file mode 100644
>> index 000000000000..7c98e3509f75
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
>> @@ -0,0 +1,57 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmemory-controllers%2Ffsl%2Fimx8m-ddrc.yaml%23&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C23e819d42b664965975808d767e1c084%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637092091602846215&amp;sdata=frWd1MENZm%2FsPjQp%2FWbphMgkkCMtwsgV8hLQyIhC3%2BI%3D&amp;reserved=0
>> +$schema: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C23e819d42b664965975808d767e1c084%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637092091602846215&amp;sdata=4IweKQJO9ZsB%2B9QxixSQjfYOFm3%2FY7iMHFBSsquK1B0%3D&amp;reserved=0
>> +
>> +title: i.MX8M DDR Controller
>> +
>> +maintainers:
>> +  - Leonard Crestez <leonard.crestez@nxp.com>
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +        - fsl,imx8mn-ddrc
>> +        - fsl,imx8mm-ddrc
>> +        - fsl,imx8mq-ddrc
>> +      - const: fsl,imx8m-ddrc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    items:
>> +      - const: core
>> +      - const: pll
>> +      - const: alt
>> +      - const: apb
>> +
>> +  operating-points-v2: true
>> +  opp-table: true
>> +
>> +required:
>> +  - reg
>> +  - compatible
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/imx8mm-clock.h>
>> +    ddrc: memory-controller@3d400000 {
>> +        compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
>> +        reg = <0x3d400000 0x400000>;
> 
> The probe() function doesn't get the IORESOURCE_MEM from dt?
> Is it needed?

This area is not currently mapped by the driver. As far as I understand 
it's acceptable to "describe hardware" even if you don't use the full 
description in driver code.

If I were to remove the "reg" area wouldn't I also have to move the node 
outside of the bus to keep DT validation? It's better to keep the address.

Maybe it will be mapped in the future or maybe firmware will start to 
parse linux DT instead of hardcoding SOC-specific addresses (this 
already happens in some cases).

>> +        clock-names = "core", "pll", "alt", "apb";
>> +        clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
>> +                 <&clk IMX8MM_DRAM_PLL>,
>> +                 <&clk IMX8MM_CLK_DRAM_ALT>,
>> +                 <&clk IMX8MM_CLK_DRAM_APB>;
>> +        operating-points-v2 = <&ddrc_opp_table>;
>> +    };
Chanwoo Choi Nov. 14, 2019, 1:07 a.m. UTC | #3 
On 11/13/19 9:35 PM, Leonard Crestez wrote:
> On 13.11.2019 04:32, Chanwoo Choi wrote:
>> On 11/13/19 6:50 AM, Leonard Crestez wrote:
>>> Add devicetree bindings for the i.MX DDR Controller on imx8m series
>>> chips. It supports dynamic frequency switching between multiple data
>>> rates and this is exposed to Linux via the devfreq subsystem.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>   .../memory-controllers/fsl/imx8m-ddrc.yaml    | 57 +++++++++++++++++++
>>>   1 file changed, 57 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
>>> new file mode 100644
>>> index 000000000000..7c98e3509f75
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
>>> @@ -0,0 +1,57 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +%YAML 1.2
>>> +---
>>> +$id: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmemory-controllers%2Ffsl%2Fimx8m-ddrc.yaml%23&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C23e819d42b664965975808d767e1c084%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637092091602846215&amp;sdata=frWd1MENZm%2FsPjQp%2FWbphMgkkCMtwsgV8hLQyIhC3%2BI%3D&amp;reserved=0
>>> +$schema: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C23e819d42b664965975808d767e1c084%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637092091602846215&amp;sdata=4IweKQJO9ZsB%2B9QxixSQjfYOFm3%2FY7iMHFBSsquK1B0%3D&amp;reserved=0
>>> +
>>> +title: i.MX8M DDR Controller
>>> +
>>> +maintainers:
>>> +  - Leonard Crestez <leonard.crestez@nxp.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +        - fsl,imx8mn-ddrc
>>> +        - fsl,imx8mm-ddrc
>>> +        - fsl,imx8mq-ddrc
>>> +      - const: fsl,imx8m-ddrc
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 4
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: core
>>> +      - const: pll
>>> +      - const: alt
>>> +      - const: apb
>>> +
>>> +  operating-points-v2: true
>>> +  opp-table: true
>>> +
>>> +required:
>>> +  - reg
>>> +  - compatible
>>> +  - clocks
>>> +  - clock-names
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/imx8mm-clock.h>
>>> +    ddrc: memory-controller@3d400000 {
>>> +        compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
>>> +        reg = <0x3d400000 0x400000>;
>>
>> The probe() function doesn't get the IORESOURCE_MEM from dt?
>> Is it needed?
> 
> This area is not currently mapped by the driver. As far as I understand 
> it's acceptable to "describe hardware" even if you don't use the full 
> description in driver code.
> 
> If I were to remove the "reg" area wouldn't I also have to move the node 
> outside of the bus to keep DT validation? It's better to keep the address.

The dt bidning documentation and device driver have not any explanation
about this. It makes the confusion to user who don't know the history.
I'd like you to add the explanation on dt bidning documentation
in order to remove the confusion.

Actually, you get the confirm from DT maintainer. I'm OK.

> 
> Maybe it will be mapped in the future or maybe firmware will start to 
> parse linux DT instead of hardcoding SOC-specific addresses (this 
> already happens in some cases).

OK. Do you implement them in the future? If you have a plan,
you better to do this on this time.

> 
>>> +        clock-names = "core", "pll", "alt", "apb";
>>> +        clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
>>> +                 <&clk IMX8MM_DRAM_PLL>,
>>> +                 <&clk IMX8MM_CLK_DRAM_ALT>,
>>> +                 <&clk IMX8MM_CLK_DRAM_APB>;
>>> +        operating-points-v2 = <&ddrc_opp_table>;
>>> +    };
> 
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
new file mode 100644
index 000000000000..7c98e3509f75
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
@@ -0,0 +1,57 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/fsl/imx8m-ddrc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX8M DDR Controller
+
+maintainers:
+  - Leonard Crestez <leonard.crestez@nxp.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - fsl,imx8mn-ddrc
+        - fsl,imx8mm-ddrc
+        - fsl,imx8mq-ddrc
+      - const: fsl,imx8m-ddrc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 4
+
+  clock-names:
+    items:
+      - const: core
+      - const: pll
+      - const: alt
+      - const: apb
+
+  operating-points-v2: true
+  opp-table: true
+
+required:
+  - reg
+  - compatible
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    ddrc: memory-controller@3d400000 {
+        compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
+        reg = <0x3d400000 0x400000>;
+        clock-names = "core", "pll", "alt", "apb";
+        clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
+                 <&clk IMX8MM_DRAM_PLL>,
+                 <&clk IMX8MM_CLK_DRAM_ALT>,
+                 <&clk IMX8MM_CLK_DRAM_APB>;
+        operating-points-v2 = <&ddrc_opp_table>;
+    };