[PATCH 0/7] PM / devfreq: Add initial imx support

[PATCH 0/7] PM / devfreq: Add initial imx support

[Leonard Crestez]

This adds devfreq support for imx8mm, covering dynamic scaling of
internal NOC and DDR Controller

Scaling for simple busses (NIC/NOC) is done through the clk framework
while DRAM scaling is performed in firmware with an "imx-ddrc" wrapper
for devfreq.

Changes since RFCv3:
 * Implement passive support and set NOC parent to DDRC
 * Drop scaling AHB/AXI for now (NOC/DDRC use most power anyway)
 * Split into two devreq drivers (and bindings) because the ddrc is
really a distinct piece of hardware.
 * Stop relying on clk_min_rate
 * Fetch info about dram clk parents from firmware instead of
hardcoding in driver. This can more easily support additional rates.
 * Perform DRAM frequency inside devfreq instead of clk, mostly due to
objections to earlier RFCs for "imx8m-dram-clk"
 * Link: https://patchwork.kernel.org/cover/11056779/
 * Link: https://patchwork.kernel.org/patch/11049429/

Stephen: It would be very helpful if you could comment on the dram
frequency switching code. I moved it outside of clk but now I have to
use provider APIs outside of drivers/clk for parent manipulation. Few
other drivers do that so maybe it's OK?

Scaling buses can cause problems for devices with realtime bandwith
requirements such as display, the intention is to use the interconnect
framework to make DEV_PM_QOS_MIN_FREQUENCY to devfreq. There are
separate patches for that:

 * https://patchwork.kernel.org/patch/11084279/
 * https://patchwork.kernel.org/cover/11078671/

Leonard Crestez (7):
  clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram_alt/apb
  dt-bindings: devfreq: Add bindings for generic imx buses
  PM / devfreq: Add generic imx bus driver
  dt-bindings: devfreq: Add bindings for imx ddr controller
  PM / devfreq: Add dynamic scaling for imx ddr controller
  PM / devfreq: imx-ddrc: Measure bandwidth with perf
  arm64: dts: imx8mm: Add devfreq nodes

 .../devicetree/bindings/devfreq/imx-ddrc.yaml |  53 ++
 .../devicetree/bindings/devfreq/imx.yaml      |  50 ++
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  51 +-
 drivers/clk/imx/clk-imx8mm.c                  |   6 +-
 drivers/clk/imx/clk-imx8mn.c                  |   6 +-
 drivers/clk/imx/clk-imx8mq.c                  |   7 +-
 drivers/devfreq/Kconfig                       |  12 +
 drivers/devfreq/Makefile                      |   1 +
 drivers/devfreq/imx-ddrc.c                    | 511 ++++++++++++++++++
 drivers/devfreq/imx-devfreq.c                 | 148 +++++
 10 files changed, 837 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
 create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml
 create mode 100644 drivers/devfreq/imx-ddrc.c
 create mode 100644 drivers/devfreq/imx-devfreq.c

-- 
2.17.1

[PATCH 1/7] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram_alt/apb

[Leonard Crestez]

Dram frequency changes required modifying these clocks outside the
control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
rates are always read back from registers.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/clk/imx/clk-imx8mm.c | 6 ++++--
 drivers/clk/imx/clk-imx8mn.c | 6 ++++--
 drivers/clk/imx/clk-imx8mq.c | 7 ++++---
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 1a944837f638..0417fa3c8e71 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -526,12 +526,14 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 	/* IPG */
 	clks[IMX8MM_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
 	clks[IMX8MM_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
 
 	/* IP */
-	clks[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
-	clks[IMX8MM_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
+	clks[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000,
+			CLK_GET_RATE_NOCACHE);
+	clks[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080,
+			CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
 	clks[IMX8MM_CLK_VPU_G1] = imx8m_clk_composite("vpu_g1", imx8mm_vpu_g1_sels, base + 0xa100);
 	clks[IMX8MM_CLK_VPU_G2] = imx8m_clk_composite("vpu_g2", imx8mm_vpu_g2_sels, base + 0xa180);
 	clks[IMX8MM_CLK_DISP_DTRC] = imx8m_clk_composite("disp_dtrc", imx8mm_disp_dtrc_sels, base + 0xa200);
 	clks[IMX8MM_CLK_DISP_DC8000] = imx8m_clk_composite("disp_dc8000", imx8mm_disp_dc8000_sels, base + 0xa280);
 	clks[IMX8MM_CLK_PCIE1_CTRL] = imx8m_clk_composite("pcie1_ctrl", imx8mm_pcie1_ctrl_sels, base + 0xa300);
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index ecd1062f6847..1dda3e6c3019 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -497,12 +497,14 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MN_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mn_ahb_sels, base + 0x9000);
 	clks[IMX8MN_CLK_AUDIO_AHB] = imx8m_clk_composite("audio_ahb", imx8mn_audio_ahb_sels, base + 0x9100);
 	clks[IMX8MN_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
 	clks[IMX8MN_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
 	clks[IMX8MN_CLK_DRAM_CORE] = imx_clk_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mn_dram_core_sels, ARRAY_SIZE(imx8mn_dram_core_sels), CLK_IS_CRITICAL);
-	clks[IMX8MN_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000);
-	clks[IMX8MN_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mn_dram_apb_sels, base + 0xa080);
+	clks[IMX8MN_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000,
+			CLK_GET_RATE_NOCACHE);
+	clks[IMX8MN_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mn_dram_apb_sels, base + 0xa080,
+			CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
 	clks[IMX8MN_CLK_DISP_PIXEL] = imx8m_clk_composite("disp_pixel", imx8mn_disp_pixel_sels, base + 0xa500);
 	clks[IMX8MN_CLK_SAI2] = imx8m_clk_composite("sai2", imx8mn_sai2_sels, base + 0xa600);
 	clks[IMX8MN_CLK_SAI3] = imx8m_clk_composite("sai3", imx8mn_sai3_sels, base + 0xa680);
 	clks[IMX8MN_CLK_SAI5] = imx8m_clk_composite("sai5", imx8mn_sai5_sels, base + 0xa780);
 	clks[IMX8MN_CLK_SAI6] = imx8m_clk_composite("sai6", imx8mn_sai6_sels, base + 0xa800);
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index e451e68eaa84..f18206d57020 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -436,13 +436,14 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MQ_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
 	clks[IMX8MQ_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
 
 	/* IP */
 	clks[IMX8MQ_CLK_DRAM_CORE] = imx_clk_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL);
-
-	clks[IMX8MQ_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000);
-	clks[IMX8MQ_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mq_dram_apb_sels, base + 0xa080);
+	clks[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000,
+			CLK_GET_RATE_NOCACHE);
+	clks[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080,
+			CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
 	clks[IMX8MQ_CLK_VPU_G1] = imx8m_clk_composite("vpu_g1", imx8mq_vpu_g1_sels, base + 0xa100);
 	clks[IMX8MQ_CLK_VPU_G2] = imx8m_clk_composite("vpu_g2", imx8mq_vpu_g2_sels, base + 0xa180);
 	clks[IMX8MQ_CLK_DISP_DTRC] = imx8m_clk_composite("disp_dtrc", imx8mq_disp_dtrc_sels, base + 0xa200);
 	clks[IMX8MQ_CLK_DISP_DC8000] = imx8m_clk_composite("disp_dc8000", imx8mq_disp_dc8000_sels, base + 0xa280);
 	clks[IMX8MQ_CLK_PCIE1_CTRL] = imx8m_clk_composite("pcie1_ctrl", imx8mq_pcie1_ctrl_sels, base + 0xa300);
-- 
2.17.1

[PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[Leonard Crestez]

Add initial dt bindings for the interconnects inside i.MX chips.
Multiple external IPs are involved but SOC integration means the
software controllable interfaces are very similar.

This is initially only for imx8mm but add an "fsl,imx8m-nic" fallback
similar to exynos-bus.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 .../devicetree/bindings/devfreq/imx.yaml      | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml

diff --git a/Documentation/devicetree/bindings/devfreq/imx.yaml b/Documentation/devicetree/bindings/devfreq/imx.yaml
new file mode 100644
index 000000000000..0e2ee3a5205e
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/imx.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/imx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <leonard.crestez@nxp.com>
+
+description: |
+  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
+  voltage) can be adjusted.
+
+  Some of those buses expose register areas mentioned in the memory maps as GPV
+  ("Global Programmers View") but not all. Access to this area might be denied for
+  normal world.
+
+  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
+  FlexNOC but DT bindings are specific to the integration of these bus
+  interconnect IPs into imx SOCs.
+ 
+properties:
+  compatible:
+    contains:
+      enum:
+       - fsl,imx8m-noc
+       - fsl,imx8m-nic
+
+  reg:
+    maxItems: 1
+    description: GPV area
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    noc: noc@32700000 {
+            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MM_CLK_NOC>;
+            operating-points-v2 = <&noc_opp_table>;
+    };
-- 
2.17.1

Re: [PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[Rob Herring]

On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> Add initial dt bindings for the interconnects inside i.MX chips.
> Multiple external IPs are involved but SOC integration means the
> software controllable interfaces are very similar.
>
> This is initially only for imx8mm but add an "fsl,imx8m-nic" fallback
> similar to exynos-bus.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  .../devicetree/bindings/devfreq/imx.yaml      | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml
>
> diff --git a/Documentation/devicetree/bindings/devfreq/imx.yaml b/Documentation/devicetree/bindings/devfreq/imx.yaml
> new file mode 100644
> index 000000000000..0e2ee3a5205e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/imx.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/imx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic i.MX bus frequency device
> +
> +maintainers:
> +  - Leonard Crestez <leonard.crestez@nxp.com>
> +
> +description: |
> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
> +  voltage) can be adjusted.
> +
> +  Some of those buses expose register areas mentioned in the memory maps as GPV
> +  ("Global Programmers View") but not all. Access to this area might be denied for
> +  normal world.
> +
> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
> +  FlexNOC but DT bindings are specific to the integration of these bus
> +  interconnect IPs into imx SOCs.

No need to use the interconnect binding?

> +
> +properties:
> +  compatible:
> +    contains:
> +      enum:
> +       - fsl,imx8m-noc
> +       - fsl,imx8m-nic

This means any combination of these 2 strings is valid. I suspect you
want a given node to have only one of them, so drop 'contains'

> +
> +  reg:
> +    maxItems: 1
> +    description: GPV area
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks

reg?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8mm-clock.h>
> +    noc: noc@32700000 {
> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";

Doesn't match the schema. (Well, it does with 'contains', but
fsl,imx8mm-noc is not documented.)

> +            reg = <0x32700000 0x100000>;
> +            clocks = <&clk IMX8MM_CLK_NOC>;
> +            operating-points-v2 = <&noc_opp_table>;

Not documented.

> +    };
> --
> 2.17.1
>

Re: [PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[Leonard Crestez]

On 8/12/2019 10:47 PM, Rob Herring wrote:
> On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:

>> Add initial dt bindings for the interconnects inside i.MX chips.
>> Multiple external IPs are involved but SOC integration means the
>> software controllable interfaces are very similar.
>>
>> +description: |
>> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
>> +  voltage) can be adjusted.
>> +
>> +  Some of those buses expose register areas mentioned in the memory maps as GPV
>> +  ("Global Programmers View") but not all. Access to this area might be denied for
>> +  normal world.
>> +
>> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
>> +  FlexNOC but DT bindings are specific to the integration of these bus
>> +  interconnect IPs into imx SOCs.
> 
> No need to use the interconnect binding?

Separate RFC: https://patchwork.kernel.org/patch/11078673/

The interconnect is represented by a separate "virtual" node which might 
not be OK. There was also a recent RFC from samsung which turns devfreq 
nodes into interconnect providers:
     https://patchwork.kernel.org/cover/11054417/

Is that preferable?

>> +required:
>> +  - compatible
>> +  - clocks
> 
> reg?

This is deliberately optional: for some NICs the GPV register area is 
not exposed in the memory map. This is unusual but an accurate 
description of the hardware.

The current driver doesn't even attempt to map registers, it only 
adjusts the clock.

>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/imx8mm-clock.h>
>> +    noc: noc@32700000 {
>> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
> 
> Doesn't match the schema. (Well, it does with 'contains', but
> fsl,imx8mm-noc is not documented.)

I'm confused about how per-SOC compatible strings works with validation. 
There is a rule that every SOC dtsi needs to add soc prefix to all 
device nodes but of_device_id in driver code doesn't need to be updated.

Without using "contains" on the "compatible" property then all 
SOC-specific compatible strings would need to be mentioned in every yaml 
files. Unless I'm missing something this means updating update every 
binding file for each new SOC?

I guess it can be useful because it also validates the compatible 
sequence itself.

For this current example something like this seems to work:

   compatible:
     oneOf:
       - items:
         - enum:
           - fsl,imx8mm-nic
           - fsl,imx8mq-nic
         - const: fsl,imx8m-nic
       - items:
         - enum:
           - fsl,imx8mm-noc
           - fsl,imx8mq-noc
         - const: fsl,imx8m-noc

--
Regards,
Leonard

Re: [PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[Rob Herring]

On Mon, Aug 12, 2019 at 7:32 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 8/12/2019 10:47 PM, Rob Herring wrote:
> > On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> >> Add initial dt bindings for the interconnects inside i.MX chips.
> >> Multiple external IPs are involved but SOC integration means the
> >> software controllable interfaces are very similar.
> >>
> >> +description: |
> >> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
> >> +  voltage) can be adjusted.
> >> +
> >> +  Some of those buses expose register areas mentioned in the memory maps as GPV
> >> +  ("Global Programmers View") but not all. Access to this area might be denied for
> >> +  normal world.
> >> +
> >> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
> >> +  FlexNOC but DT bindings are specific to the integration of these bus
> >> +  interconnect IPs into imx SOCs.
> >
> > No need to use the interconnect binding?
>
> Separate RFC: https://patchwork.kernel.org/patch/11078673/
>
> The interconnect is represented by a separate "virtual" node which might
> not be OK. There was also a recent RFC from samsung which turns devfreq
> nodes into interconnect providers:
>      https://patchwork.kernel.org/cover/11054417/
>
> Is that preferable?

Virtual nodes are not OK.

>
> >> +required:
> >> +  - compatible
> >> +  - clocks
> >
> > reg?
>
> This is deliberately optional: for some NICs the GPV register area is
> not exposed in the memory map. This is unusual but an accurate
> description of the hardware.

Different h/w blocks should have different compatibles. GPV is an Arm
thing and I'd expect FlexNOC to be different.

> The current driver doesn't even attempt to map registers, it only
> adjusts the clock.

Irrelevant to the binding...

>
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/clock/imx8mm-clock.h>
> >> +    noc: noc@32700000 {
> >> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
> >
> > Doesn't match the schema. (Well, it does with 'contains', but
> > fsl,imx8mm-noc is not documented.)
>
> I'm confused about how per-SOC compatible strings works with validation.
> There is a rule that every SOC dtsi needs to add soc prefix to all
> device nodes but of_device_id in driver code doesn't need to be updated.
>
> Without using "contains" on the "compatible" property then all
> SOC-specific compatible strings would need to be mentioned in every yaml
> files. Unless I'm missing something this means updating update every
> binding file for each new SOC?

Yes. The main exception is if various SoCs are just packaging,
binning, or fuse differences.

>
> I guess it can be useful because it also validates the compatible
> sequence itself.

Right. Order matters.

>
> For this current example something like this seems to work:
>
>    compatible:
>      oneOf:
>        - items:
>          - enum:
>            - fsl,imx8mm-nic
>            - fsl,imx8mq-nic
>          - const: fsl,imx8m-nic
>        - items:
>          - enum:
>            - fsl,imx8mm-noc
>            - fsl,imx8mq-noc
>          - const: fsl,imx8m-noc

Looks correct.

Rob

Re: [PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[Leonard Crestez]

On 13.08.2019 17:06, Rob Herring wrote:
> On Mon, Aug 12, 2019 at 7:32 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>> On 8/12/2019 10:47 PM, Rob Herring wrote:
>>> On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:

>>>> Add initial dt bindings for the interconnects inside i.MX chips.
>>>> Multiple external IPs are involved but SOC integration means the
>>>> software controllable interfaces are very similar.
>>>>
>>>> +description: |
>>>> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
>>>> +  voltage) can be adjusted.
>>>> +
>>>> +  Some of those buses expose register areas mentioned in the memory maps as GPV
>>>> +  ("Global Programmers View") but not all. Access to this area might be denied for
>>>> +  normal world.
>>>> +
>>>> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
>>>> +  FlexNOC but DT bindings are specific to the integration of these bus
>>>> +  interconnect IPs into imx SOCs.
>>>
>>> No need to use the interconnect binding?
>>
>> The interconnect is represented by a separate "virtual" node which might
>> not be OK. There was also a recent RFC from samsung which turns devfreq
>> nodes into interconnect providers:
>>
>> Is that preferable?
> 
> Virtual nodes are not OK.

Then I'll try to make the "interconnect" device probe from a soc driver 
and turn devfreq nodes into interconnect providers backed by this same 
singleton device.

Still separate from this series.

>>>> +required:
>>>> +  - compatible
>>>> +  - clocks
>>>
>>> reg?
>>
>> This is deliberately optional: for some NICs the GPV register area is
>> not exposed in the memory map. This is unusual but an accurate
>> description of the hardware.
> 
> Different h/w blocks should have different compatibles. GPV is an Arm
> thing and I'd expect FlexNOC to be different.

The imx reference manuals call them both "GPV" though layout is indeed 
quite different (and for FlexNoC it's not even documented).

The h/w blocks do have different compat strings (imx8m-nic and 
imx8m-noc). They have a single binding document because didn't want to 
create two nearly-identical bindings, I assume it would be fine to split 
later if needed.

--
Regards,
Leonard

[PATCH 3/7] PM / devfreq: Add generic imx bus driver

[Leonard Crestez]

Add initial support for dynamic frequency switching on pieces of the imx
interconnect fabric.

All this driver actually does is set a clk rate based on an opp table.

No attempt is made to map registers or anything clever.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/Kconfig       |  12 +++
 drivers/devfreq/Makefile      |   1 +
 drivers/devfreq/imx-devfreq.c | 148 ++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+)
 create mode 100644 drivers/devfreq/imx-devfreq.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index defe1d438710..9088a151bafe 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -90,10 +90,22 @@ config ARM_EXYNOS_BUS_DEVFREQ
 	  Each memory bus group could contain many memoby bus block. It reads
 	  PPMU counters of memory controllers by using DEVFREQ-event device
 	  and adjusts the operating frequencies and voltages with OPP support.
 	  This does not yet operate with optimal voltages.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_SIMPLE_ONDEMAND
+	select DEVFREQ_GOV_USERSPACE
+	select PM_OPP
+	help
+	  This adds the DEVFREQ driver for the i.MX family of SoCs.
+	  It allows adjusting frequencies for DDRC (DDR Controller) and various
+	  NICs and NOCs which form the SOC interconnect fabric
+
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
 	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
 		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
 		ARCH_TEGRA_210_SOC || \
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 338ae8440db6..c2463ed4c934 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE)	+= governor_powersave.o
 obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
 obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers
diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
new file mode 100644
index 000000000000..702a4368af7d
--- /dev/null
+++ b/drivers/devfreq/imx-devfreq.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev, unsigned long *freq, u32 flags)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+		struct devfreq_dev_status *stat)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	return dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;
+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "passive link to %s\n",
+				dev_name(priv->passive_data.parent->dev.parent));
+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get passive parent: %d\n", ret);
+		goto err;
+	}
+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+
+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
+MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH 4/7] dt-bindings: devfreq: Add bindings for imx ddr controller

[Leonard Crestez]

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>
---
 .../devicetree/bindings/devfreq/imx-ddrc.yaml | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml

diff --git a/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml b/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
new file mode 100644
index 000000000000..fa20280a682f
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/imx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX DDR Controller
+
+maintainers:
+  - Leonard Crestez <leonard.crestez@nxp.com>
+
+properties:
+  compatible:
+    contains:
+      enum:
+       - fsl,imx8m-ddrc
+
+  reg:
+    maxItems: 1
+    description: DDR Controller registers
+
+  clocks:
+    minItems: 5
+    maxItems: 5
+
+  clock-names:
+    items:
+      - const: dram_core
+      - const: dram_pll
+      - const: dram_alt_root
+      - const: dram_alt
+      - const: dram_apb
+
+required:
+  - reg
+  - compatible
+  - clocks
+  - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    ddrc: dram-controller@3d400000 {
+        compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
+        reg = <0x3d400000 0x400000>;
+        clock-names = "dram_core", "dram_pll", "dram_alt_root", "dram_alt", "dram_apb";
+        clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
+                 <&clk IMX8MM_DRAM_PLL>,
+                 <&clk IMX8MM_CLK_DRAM_ALT_ROOT>,
+                 <&clk IMX8MM_CLK_DRAM_ALT>,
+                 <&clk IMX8MM_CLK_DRAM_APB>;
+        operating-points-v2 = <&ddrc_opp_table>;
+    };
-- 
2.17.1

Re: [PATCH 4/7] dt-bindings: devfreq: Add bindings for imx ddr controller

[Rob Herring]

On Mon, Aug 12, 2019 at 12:50 PM Leonard Crestez
<leonard.crestez@nxp.com> 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>
> ---
>  .../devicetree/bindings/devfreq/imx-ddrc.yaml | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
>
> diff --git a/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml b/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
> new file mode 100644
> index 000000000000..fa20280a682f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/imx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX DDR Controller
> +
> +maintainers:
> +  - Leonard Crestez <leonard.crestez@nxp.com>
> +
> +properties:
> +  compatible:
> +    contains:

Don't use 'contains'.

> +      enum:
> +       - fsl,imx8m-ddrc
> +
> +  reg:
> +    maxItems: 1
> +    description: DDR Controller registers

Don't really need a description for a standard property with a single item.

> +
> +  clocks:
> +    minItems: 5

Just maxItems is sufficient unless there's a variable number of items.

> +    maxItems: 5
> +
> +  clock-names:
> +    items:
> +      - const: dram_core
> +      - const: dram_pll
> +      - const: dram_alt_root
> +      - const: dram_alt
> +      - const: dram_apb
> +
> +required:
> +  - reg
> +  - compatible
> +  - clocks
> +  - clock-names

You should add an 'additionalProperties: false' here.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8mm-clock.h>
> +    ddrc: dram-controller@3d400000 {
> +        compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
> +        reg = <0x3d400000 0x400000>;
> +        clock-names = "dram_core", "dram_pll", "dram_alt_root", "dram_alt", "dram_apb";
> +        clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
> +                 <&clk IMX8MM_DRAM_PLL>,
> +                 <&clk IMX8MM_CLK_DRAM_ALT_ROOT>,
> +                 <&clk IMX8MM_CLK_DRAM_ALT>,
> +                 <&clk IMX8MM_CLK_DRAM_APB>;
> +        operating-points-v2 = <&ddrc_opp_table>;

Not documented. You can assume a common property has a type definition
already (this one doesn't yet), so just this is enough:

operating-points-v2: true

[PATCH 5/7] PM / devfreq: Add dynamic scaling for imx ddr controller

[Leonard Crestez]

Add driver for dynamic scaling the DDR Controller on imx8m chips. Actual
frequency switching is implemented in TF-A, this driver just wraps the
SMC calls and updates the clk tree.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/Makefile   |   2 +-
 drivers/devfreq/imx-ddrc.c | 379 +++++++++++++++++++++++++++++++++++++
 2 files changed, 380 insertions(+), 1 deletion(-)
 create mode 100644 drivers/devfreq/imx-ddrc.c

diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index c2463ed4c934..eba138914c74 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -7,11 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE)	+= governor_powersave.o
 obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
 obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
-obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o imx-ddrc.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers
diff --git a/drivers/devfreq/imx-ddrc.c b/drivers/devfreq/imx-ddrc.c
new file mode 100644
index 000000000000..e46a1e711bc6
--- /dev/null
+++ b/drivers/devfreq/imx-ddrc.c
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/devfreq.h>
+#include <linux/pm_opp.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/arm-smccc.h>
+
+#define IMX_SIP_DDR_DVFS			0xc2000004
+
+/* Values starting from 0 switch to specific frequency */
+#define IMX_SIP_DDR_FREQ_SET_HIGH		0x00
+
+/* Deprecated after moving IRQ handling to ATF */
+#define IMX_SIP_DDR_DVFS_WAIT_CHANGE		0x0F
+
+/* Query available frequencies. */
+#define IMX_SIP_DDR_DVFS_GET_FREQ_COUNT		0x10
+#define IMX_SIP_DDR_DVFS_GET_FREQ_INFO		0x11
+
+/*
+ * This should be in a 1:1 mapping with devicetree OPPs but
+ * firmware provides additional info.
+ */
+struct imx_ddrc_freq {
+	unsigned long rate;
+	unsigned long smcarg;
+	int dram_core_parent_index;
+	int dram_alt_parent_index;
+	int dram_apb_parent_index;
+};
+
+/* Hardware limitation */
+#define IMX_DDRC_MAX_FREQ_COUNT 4
+
+/*
+ * imx DRAM controller
+ *
+ * imx DRAM controller clocks have the following structure (abridged):
+ *
+ * +----------+       |\            +------+
+ * | dram_pll |-------|M| dram_core |      |
+ * +----------+       |U|---------->| D    |
+ *                 /--|X|           |  D   |
+ *   dram_alt_root |  |/            |   R  |
+ *                 |                |    C |
+ *            +---------+           |      |
+ *            |FIX DIV/4|           |      |
+ *            +---------+           |      |
+ *  composite:     |                |      |
+ * +----------+    |                |      |
+ * | dram_alt |----/                |      |
+ * +----------+                     |      |
+ * | dram_apb |-------------------->|      |
+ * +----------+                     +------+
+ *
+ * The dram_pll is used for higher rates and dram_alt is used for lower rates.
+ *
+ * Frequency switching is implemented in TF-A (via SMC call) and can change the
+ * configuration of the clocks, including mux parents. The dram_alt and
+ * dram_apb clocks are "imx composite" and their parent can change too.
+ *
+ * We need to prepare/enable the new mux parents head of switching and update
+ * their information afterwards.
+ */
+struct imx_ddrc {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+
+	/* For frequency switching: */
+	struct clk *dram_core;
+	struct clk *dram_pll;
+	struct clk *dram_alt_root;
+	struct clk *dram_alt;
+	struct clk *dram_apb;
+
+	int freq_count;
+	struct imx_ddrc_freq freq_table[IMX_DDRC_MAX_FREQ_COUNT];
+};
+
+static struct imx_ddrc_freq* imx_ddrc_find_freq(struct imx_ddrc* priv,
+						unsigned long rate)
+{
+	int i;
+
+	for (i = 0; i < priv->freq_count; ++i)
+		if (priv->freq_table[i].rate == rate)
+			return &priv->freq_table[i];
+
+	return NULL;
+}
+
+static void imx_ddrc_smc_set_freq(int target_freq)
+{
+	struct arm_smccc_res res;
+	u32 online_cpus = 0;
+	int cpu;
+
+	local_irq_disable();
+
+	for_each_online_cpu(cpu)
+		online_cpus |= (1 << (cpu * 8));
+
+	/* change the ddr freqency */
+	arm_smccc_smc(IMX_SIP_DDR_DVFS, target_freq, online_cpus,
+			0, 0, 0, 0, 0, &res);
+
+	local_irq_enable();
+}
+
+struct clk *clk_get_parent_by_index(struct clk* clk, int index)
+{
+	struct clk_hw *hw;
+
+	hw = clk_hw_get_parent_by_index(__clk_get_hw(clk), index);
+
+	return hw ? hw->clk : NULL;
+}
+
+static int imx_ddrc_set_freq(struct device *dev, struct imx_ddrc_freq* freq)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+	struct clk* new_dram_core_parent;
+	struct clk* new_dram_alt_parent;
+	struct clk* new_dram_apb_parent;
+	int ret;
+
+	new_dram_core_parent = clk_get_parent_by_index(
+			priv->dram_core, freq->dram_core_parent_index - 1);
+	new_dram_alt_parent = clk_get_parent_by_index(
+			priv->dram_alt, freq->dram_alt_parent_index - 1);
+	new_dram_apb_parent = clk_get_parent_by_index(
+			priv->dram_apb, freq->dram_apb_parent_index - 1);
+
+	/* increase reference counts and ensure clks are ON before switch */
+	ret = clk_prepare_enable(new_dram_core_parent);
+	if (ret) {
+		dev_err(dev, "failed enable new dram_core parent: %d\n", ret);
+		goto out;
+	}
+	ret = clk_prepare_enable(new_dram_alt_parent);
+	if (ret) {
+		dev_err(dev, "failed enable new dram_alt parent: %d\n", ret);
+		goto out_dis_core;
+	}
+	ret = clk_prepare_enable(new_dram_apb_parent);
+	if (ret) {
+		dev_err(dev, "failed enable new dram_apb parent: %d\n", ret);
+		goto out_dis_alt;
+	}
+
+	imx_ddrc_smc_set_freq(freq->smcarg);
+
+	/* update parents in clk tree after switch. */
+	ret = clk_set_parent(priv->dram_core, new_dram_core_parent);
+	if (ret)
+		dev_err(dev, "failed clk_set_parent dram_core: %d\n", ret);
+	if (new_dram_alt_parent) {
+		ret = clk_set_parent(priv->dram_alt, new_dram_alt_parent);
+		if (ret)
+			dev_err(dev, "failed clk_set_parent dram_alt_parent: %d\n", ret);
+	}
+	if (new_dram_apb_parent) {
+		ret = clk_set_parent(priv->dram_apb, new_dram_apb_parent);
+		if (ret)
+			dev_err(dev, "failed clk_set_parent dram_apb_parent: %d\n", ret);
+	}
+	
+	/*
+	 * clk_set_parent transfer the reference count from old parent.
+	 * now we drop extra reference counts used during the switch
+	 */
+	clk_disable_unprepare(new_dram_apb_parent);
+out_dis_alt:
+	clk_disable_unprepare(new_dram_alt_parent);
+out_dis_core:
+	clk_disable_unprepare(new_dram_core_parent);
+out:
+	return ret;
+}
+
+static int imx_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+	struct imx_ddrc_freq *freq_info;
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq, cur_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+	cur_freq = clk_get_rate(priv->dram_core);
+
+	if (new_freq == cur_freq)
+		return 0;
+
+	freq_info = imx_ddrc_find_freq(priv, new_freq);
+	if (!freq_info)
+		return -EINVAL;
+	ret = imx_ddrc_set_freq(dev, freq_info);
+	if (ret)
+		dev_err(dev, "ddrc failed to change freq %lu to %lu\n",
+				cur_freq, new_freq);
+	else
+		dev_dbg(dev, "ddrc changed freq %lu to %lu\n",
+				cur_freq, new_freq);
+
+	return ret;
+}
+
+static int imx_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->dram_core);
+
+	return 0;
+}
+
+static int imx_ddrc_get_dev_status(struct device *dev,
+		struct devfreq_dev_status *stat)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->dram_core);
+
+	return 0;
+}
+
+static int imx_ddrc_init_freq_info(struct device *dev)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+	struct arm_smccc_res res;
+	int index;
+
+	/*
+	 * An error here means DDR DVFS API not supported by firmware
+	 */
+	arm_smccc_smc(IMX_SIP_DDR_DVFS, IMX_SIP_DDR_DVFS_GET_FREQ_COUNT,
+			0, 0, 0, 0, 0, 0, &res);
+	priv->freq_count = res.a0;
+	if (priv->freq_count <= 0 || priv->freq_count > IMX_DDRC_MAX_FREQ_COUNT)
+		return -ENOSYS;
+
+	for (index = 0; index < priv->freq_count; ++index) {
+		struct imx_ddrc_freq *freq = &priv->freq_table[index];
+
+		arm_smccc_smc(IMX_SIP_DDR_DVFS, IMX_SIP_DDR_DVFS_GET_FREQ_INFO,
+				index, 0, 0, 0, 0, 0, &res);
+		/* Result should be strictly positive */
+		if ((long)res.a0 <= 0)
+			return -ENOSYS;
+
+		freq->rate = res.a0 * 250000;
+		freq->smcarg = index;
+		freq->dram_core_parent_index = res.a1;
+		freq->dram_alt_parent_index = res.a2;
+		freq->dram_apb_parent_index = res.a3;
+
+		/* dram_core has 2 options: dram_pll or dram_alt_root */
+		if (freq->dram_core_parent_index != 1 &&
+				freq->dram_core_parent_index != 2)
+			return -ENOSYS;
+		/* dram_apb and dram_alt have exactly 8 possible parents */
+		if (freq->dram_alt_parent_index > 8 ||
+				freq->dram_apb_parent_index > 8)
+			return -ENOSYS;
+		/* dram_core from alt requires explicit dram_alt parent */
+		if (freq->dram_core_parent_index == 2 &&
+				freq->dram_alt_parent_index == 0)
+			return -ENOSYS;
+	}
+
+	return 0;
+}
+
+static void imx_ddrc_exit(struct device *dev)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+
+	return dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_ddrc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_ddrc *priv;
+	struct device_node *events_node;
+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = imx_ddrc_init_freq_info(dev);
+	if (ret) {
+		dev_err(dev, "failed to init firmware freq info: %d\n", ret);
+		return ret;
+	}
+
+	priv->dram_core = devm_clk_get(dev, "dram_core");
+	priv->dram_pll = devm_clk_get(dev, "dram_pll");
+	priv->dram_alt = devm_clk_get(dev, "dram_alt");
+	priv->dram_alt_root = devm_clk_get(dev, "dram_alt_root");
+	priv->dram_apb = devm_clk_get(dev, "dram_apb");
+	if (IS_ERR(priv->dram_core) ||
+		IS_ERR(priv->dram_pll) ||
+		IS_ERR(priv->dram_alt) ||
+		IS_ERR(priv->dram_alt_root) ||
+		IS_ERR(priv->dram_apb)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to fetch clocks: %d\n", ret);
+		return ret;
+	}
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_ddrc_target;
+	priv->profile.get_dev_status = imx_ddrc_get_dev_status;
+	priv->profile.exit = imx_ddrc_exit;
+	priv->profile.get_cur_freq = imx_ddrc_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->dram_core);
+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, NULL);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_ddrc_of_match[] = {
+	{ .compatible = "fsl,imx8m-ddrc", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_ddrc_of_match);
+
+static struct platform_driver imx_ddrc_platdrv = {
+	.probe		= imx_ddrc_probe,
+	.driver = {
+		.name	= "imx-ddrc-devfreq",
+		.of_match_table = of_match_ptr(imx_ddrc_of_match),
+	},
+};
+module_platform_driver(imx_ddrc_platdrv);
+
+MODULE_DESCRIPTION("i.MX DDR controller frequency driver");
+MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH 6/7] PM / devfreq: imx-ddrc: Measure bandwidth with perf

[Leonard Crestez]

The imx8m ddrc has a performance monitoring block attached which can
be used to measure bandwidth usage and automatically adjust frequency.

There is already a perf driver for that block so instead of implementing
a devfreq events driver use the in-kernel perf API to implement
get_dev_status directly.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/imx-ddrc.c | 136 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/imx-ddrc.c b/drivers/devfreq/imx-ddrc.c
index e46a1e711bc6..d7f27cdce697 100644
--- a/drivers/devfreq/imx-ddrc.c
+++ b/drivers/devfreq/imx-ddrc.c
@@ -11,10 +11,13 @@
 #include <linux/pm_opp.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/arm-smccc.h>
 
+#include <asm/perf_event.h>
+#include <linux/perf_event.h>
+
 #define IMX_SIP_DDR_DVFS			0xc2000004
 
 /* Values starting from 0 switch to specific frequency */
 #define IMX_SIP_DDR_FREQ_SET_HIGH		0x00
 
@@ -81,10 +84,22 @@ struct imx_ddrc {
 	struct clk *dram_alt;
 	struct clk *dram_apb;
 
 	int freq_count;
 	struct imx_ddrc_freq freq_table[IMX_DDRC_MAX_FREQ_COUNT];
+
+	/* For measuring load with perf events: */
+	struct platform_device* pmu_pdev;
+	struct pmu *pmu;
+
+	struct perf_event_attr rd_event_attr;
+	struct perf_event_attr wr_event_attr;
+	struct perf_event *rd_event;
+	struct perf_event *wr_event;
+
+	u64 last_rd_val, last_rd_ena, last_rd_run;
+	u64 last_wr_val, last_wr_ena, last_wr_run;
 };
 
 static struct imx_ddrc_freq* imx_ddrc_find_freq(struct imx_ddrc* priv,
 						unsigned long rate)
 {
@@ -233,17 +248,117 @@ static int imx_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
 static int imx_ddrc_get_dev_status(struct device *dev,
 		struct devfreq_dev_status *stat)
 {
 	struct imx_ddrc *priv = dev_get_drvdata(dev);
 
-	stat->busy_time = 0;
-	stat->total_time = 0;
 	stat->current_frequency = clk_get_rate(priv->dram_core);
 
+	if (priv->rd_event && priv->wr_event) {
+		u64 rd_delta, rd_val, rd_ena, rd_run;
+		u64 wr_delta, wr_val, wr_ena, wr_run;
+
+		rd_val = perf_event_read_value(priv->rd_event, &rd_ena, &rd_run);
+		wr_val = perf_event_read_value(priv->wr_event, &wr_ena, &wr_run);
+
+		rd_delta = (rd_val - priv->last_rd_val) * (rd_ena - priv->last_rd_ena) / (rd_run - priv->last_rd_run);
+		priv->last_rd_val = rd_val;
+		priv->last_rd_ena = rd_ena;
+		priv->last_rd_run = rd_run;
+		wr_delta = (wr_val - priv->last_wr_val) * (wr_ena - priv->last_wr_ena) / (wr_run - priv->last_wr_run);
+		priv->last_wr_val = wr_val;
+		priv->last_wr_ena = wr_ena;
+		priv->last_wr_run = wr_run;
+
+		/* magic numbers, possibly wrong */
+		stat->busy_time = 4 * (rd_delta + wr_delta);
+		stat->total_time = stat->current_frequency;
+	} else {
+		stat->busy_time = 0;
+		stat->total_time = 0;
+	}
+
+	return 0;
+}
+
+static int imx_ddrc_perf_disable(struct imx_ddrc *priv)
+{
+	/* release and set to NULL */
+	if (!IS_ERR_OR_NULL(priv->rd_event))
+		perf_event_release_kernel(priv->rd_event);
+	if (!IS_ERR_OR_NULL(priv->wr_event))
+		perf_event_release_kernel(priv->wr_event);
+	priv->rd_event = NULL;
+	priv->wr_event = NULL;
+
 	return 0;
 }
 
+static int imx_ddrc_perf_enable(struct imx_ddrc *priv)
+{
+	int ret;
+
+	priv->rd_event_attr.size = sizeof(priv->rd_event_attr);
+	priv->rd_event_attr.type = priv->pmu->type;
+	priv->rd_event_attr.config = 0x2a;
+
+	priv->rd_event = perf_event_create_kernel_counter(
+			&priv->rd_event_attr, 0, NULL, NULL, NULL);
+	if (IS_ERR(priv->rd_event)) {
+		ret = PTR_ERR(priv->rd_event);
+		goto err;
+	}
+
+	priv->wr_event_attr.size = sizeof(priv->wr_event_attr);
+	priv->wr_event_attr.type = priv->pmu->type;
+	priv->wr_event_attr.config = 0x2b;
+
+	priv->wr_event = perf_event_create_kernel_counter(
+			&priv->wr_event_attr, 0, NULL, NULL, NULL);
+	if (IS_ERR(priv->wr_event)) {
+		ret = PTR_ERR(priv->wr_event);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	imx_ddrc_perf_disable(priv);
+	return ret;
+}
+
+static int imx_ddrc_init_events(struct device *dev,
+				   struct device_node* events_node)
+{
+	struct imx_ddrc *priv = dev_get_drvdata(dev);
+	struct device_driver *driver;
+
+	/*
+	 * We need pmu->type for perf_event_attr but there is no API for
+	 * mapping device_node to pmu. Fetch private data for imx-ddr-pmu and
+	 * cast that to a struct pmu instead.
+	 */
+	priv->pmu_pdev = of_find_device_by_node(events_node);
+	if (!priv->pmu_pdev)
+		return -EPROBE_DEFER;
+	driver = priv->pmu_pdev->dev.driver;
+	if (!driver)
+		return -EPROBE_DEFER;
+	if (strcmp(driver->name, "imx-ddr-pmu")) {
+		dev_warn(dev, "devfreq-events node %pOF has unexpected driver %s\n",
+				events_node, driver->name);
+		return -ENODEV;
+	}
+
+	priv->pmu = platform_get_drvdata(priv->pmu_pdev);
+	if (!priv->pmu)
+		return -EINVAL;
+
+	dev_info(dev, "events from pmu %s\n", priv->pmu->name);
+
+	return imx_ddrc_perf_enable(priv);
+}
+
 static int imx_ddrc_init_freq_info(struct device *dev)
 {
 	struct imx_ddrc *priv = dev_get_drvdata(dev);
 	struct arm_smccc_res res;
 	int index;
@@ -291,10 +406,13 @@ static int imx_ddrc_init_freq_info(struct device *dev)
 
 static void imx_ddrc_exit(struct device *dev)
 {
 	struct imx_ddrc *priv = dev_get_drvdata(dev);
 
+	imx_ddrc_perf_disable(priv);
+	platform_device_put(priv->pmu_pdev);
+
 	return dev_pm_opp_of_remove_table(dev);
 }
 
 static int imx_ddrc_probe(struct platform_device *pdev)
 {
@@ -342,10 +460,22 @@ static int imx_ddrc_probe(struct platform_device *pdev)
 	priv->profile.get_dev_status = imx_ddrc_get_dev_status;
 	priv->profile.exit = imx_ddrc_exit;
 	priv->profile.get_cur_freq = imx_ddrc_get_cur_freq;
 	priv->profile.initial_freq = clk_get_rate(priv->dram_core);
 
+	/* Handle devfreq-events */
+	events_node = of_parse_phandle(dev->of_node, "devfreq-events", 0);
+	if (events_node) {
+		ret = imx_ddrc_init_events(dev, events_node);
+		of_node_put(events_node);
+		if (ret) {
+			dev_warn(dev, "failed to init perf events: %d\n", ret);
+			goto err;
+		}
+		gov = DEVFREQ_GOV_SIMPLE_ONDEMAND;
+	}
+
 	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
 						gov, NULL);
 	if (IS_ERR(priv->devfreq)) {
 		ret = PTR_ERR(priv->devfreq);
 		dev_err(dev, "failed to add devfreq device: %d\n", ret);
@@ -353,10 +483,12 @@ static int imx_ddrc_probe(struct platform_device *pdev)
 	}
 
 	return 0;
 
 err:
+	imx_ddrc_perf_disable(priv);
+	platform_device_put(priv->pmu_pdev);
 	dev_pm_opp_of_remove_table(dev);
 	return ret;
 }
 
 static const struct of_device_id imx_ddrc_of_match[] = {
-- 
2.17.1

[PATCH 7/7] arm64: dts: imx8mm: Add devfreq nodes

[Leonard Crestez]

Add initial support for bus scaling on imx8m, starting with noc and ddrc
because they're the biggest power hogs.

Add a devfreq-event link to the PMU in order to support on-demand
scaling of ddrc based on measured dram bandwith usage. Make ddrc a
parent of the NOC because all traffic to ddrc goes through

Support for proactive scaling via interconnect and support for scaling
additional NICs will come later. The high-performance bus masters which
need that (display, vpu, gpu) are not yet enabled in upstream anyway.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 51 ++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index e8560d15c130..a37d82133c06 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -122,10 +122,38 @@
 			clock-latency-ns = <150000>;
 			opp-suspend;
 		};
 	};
 
+	ddrc_opp_table: ddrc-opp-table {
+		compatible = "operating-points-v2";
+
+		opp-25M {
+			opp-hz = /bits/ 64 <25000000>;
+		};
+		opp-100M {
+			opp-hz = /bits/ 64 <100000000>;
+		};
+		opp-750M {
+			opp-hz = /bits/ 64 <750000000>;
+		};
+	};
+
+	noc_opp_table: noc-opp-table {
+		compatible = "operating-points-v2";
+
+		opp-150M {
+			opp-hz = /bits/ 64 <150000000>;
+		};
+		opp-375M {
+			opp-hz = /bits/ 64 <375000000>;
+		};
+		opp-750M {
+			opp-hz = /bits/ 64 <750000000>;
+		};
+	};
+
 	memory@40000000 {
 		device_type = "memory";
 		reg = <0x0 0x40000000 0 0x80000000>;
 	};
 
@@ -748,10 +776,18 @@
 				status = "disabled";
 			};
 
 		};
 
+		noc: noc@32700000 {
+			compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
+			reg = <0x32700000 0x100000>;
+			clocks = <&clk IMX8MM_CLK_NOC>;
+			devfreq = <&ddrc>;
+			operating-points-v2 = <&noc_opp_table>;
+		};
+
 		aips4: bus@32c00000 {
 			compatible = "fsl,aips-bus", "simple-bus";
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0x32c00000 0x32c00000 0x400000>;
@@ -832,11 +868,24 @@
 			#interrupt-cells = <3>;
 			interrupt-controller;
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-		ddr-pmu@3d800000 {
+		ddrc: dram-controller@3d400000 {
+			compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
+			reg = <0x3d400000 0x400000>;
+			clock-names = "dram_core", "dram_pll", "dram_alt_root", "dram_alt", "dram_apb";
+			clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
+				 <&clk IMX8MM_DRAM_PLL>,
+				 <&clk IMX8MM_CLK_DRAM_ALT_ROOT>,
+				 <&clk IMX8MM_CLK_DRAM_ALT>,
+				 <&clk IMX8MM_CLK_DRAM_APB>;
+			devfreq-events = <&ddr_pmu>;
+			operating-points-v2 = <&ddrc_opp_table>;
+		};
+
+		ddr_pmu: ddr-pmu@3d800000 {
 			compatible = "fsl,imx8mm-ddr-pmu", "fsl,imx8m-ddr-pmu";
 			reg = <0x3d800000 0x400000>;
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
 		};
-- 
2.17.1

RE: [PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[MyungJoo Ham]

>Add initial dt bindings for the interconnects inside i.MX chips.
>Multiple external IPs are involved but SOC integration means the
>software controllable interfaces are very similar.
>
>This is initially only for imx8mm but add an "fsl,imx8m-nic" fallback
>similar to exynos-bus.
>
>Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>---
> .../devicetree/bindings/devfreq/imx.yaml      | 50 +++++++++++++++++++
> 1 file changed, 50 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

RE: [PATCH 3/7] PM / devfreq: Add generic imx bus driver

[MyungJoo Ham]

>Add initial support for dynamic frequency switching on pieces of the imx
>interconnect fabric.
>
>All this driver actually does is set a clk rate based on an opp table.
>
>No attempt is made to map registers or anything clever.
>
>Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>---
> drivers/devfreq/Kconfig       |  12 +++
> drivers/devfreq/Makefile      |   1 +
> drivers/devfreq/imx-devfreq.c | 148 ++++++++++++++++++++++++++++++++++
> 3 files changed, 161 insertions(+)
> create mode 100644 drivers/devfreq/imx-devfreq.c
>
>diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>index defe1d438710..9088a151bafe 100644
>--- a/drivers/devfreq/Kconfig
>+++ b/drivers/devfreq/Kconfig
>@@ -90,10 +90,22 @@ config ARM_EXYNOS_BUS_DEVFREQ
> 	  Each memory bus group could contain many memoby bus block. It reads
> 	  PPMU counters of memory controllers by using DEVFREQ-event device
> 	  and adjusts the operating frequencies and voltages with OPP support.
> 	  This does not yet operate with optimal voltages.
> 
>+config ARM_IMX_DEVFREQ
>+	tristate "i.MX DEVFREQ Driver"
>+	depends on ARCH_MXC || COMPILE_TEST
>+	select DEVFREQ_GOV_PASSIVE
>+	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>+	select DEVFREQ_GOV_USERSPACE
>+	select PM_OPP

Hello,

I have a simple question:

Does it support ALL ARCH_MXC SoCs?

Cheers,
MyungJoo

Re: [PATCH 3/7] PM / devfreq: Add generic imx bus driver

[Leonard Crestez]

On 8/13/2019 5:33 AM, MyungJoo Ham wrote:

>> Add initial support for dynamic frequency switching on pieces of the imx
>> interconnect fabric.
>>
>> All this driver actually does is set a clk rate based on an opp table.
>>
>> +config ARM_IMX_DEVFREQ
>> +	tristate "i.MX DEVFREQ Driver"
>> +	depends on ARCH_MXC || COMPILE_TEST
>> +	select DEVFREQ_GOV_PASSIVE
>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +	select DEVFREQ_GOV_USERSPACE
>> +	select PM_OPP
> 
> Does it support ALL ARCH_MXC SoCs?

Only imx8m currently but out of tree we support bus+dram frequency 
switching for ~10 imx6/7 SOCs, all other than imx7ulp.

When imx8 was upstreamed as the first 64-bit imx chip the arm64 
maintainers told us to drop stuff like ARCH_FSL_IMX8MM so there is no 
per-soc kconfig more specific than "ARCH_MXC".

I guess we could make it depend on (ARCH_MXC && ARM64) but the ARM64 
would eventually be dropped anyway.

--
Regards,
Leonard