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

[Leonard Crestez <leonard.crestez@xxxxxxx>]

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@xxxxxxx>
>> ---
>>   .../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@xxxxxxx>
>> +
>> +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>;
>> +    };