[PATCH v4 0/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[PATCH v4 0/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Leonard Crestez]

This adds support for dynamic scaling of the DDR Controller (ddrc) present in
imx8m series. Actual frequency switching is implemented inside TF-A, this
driver wraps the SMC calls and synchronizes the clk tree.

DRAM frequency switching requires clock manipulation but during this operation
DRAM itself is briefly inaccessible so this operation is performed a SMC call
to by TF-A which runs from a SRAM area. Upon returning to linux the clock tree
is updated to correspond to hardware configuration.

This is handled via CLK_GET_RATE_NO_CACHE for dividers but muxes are handled
manually: the driver will prepare/enable the new parents ahead of switching (so
that the expected roots are enabled) and afterwards it will call clk_set_parent
to ensure the parents in clock framework are up-to-date.

This series is atomically useful and roughly similar to devfreq drivers for
tegra and rockchip.

Running at lower dram rates saves power but can affect the functionality of
other blocks in the chip (display, vpu etc). Support for in-kernel constraints
will some separately.

Angus/Martin: You previously attempted to test on purism boards, this updated
version should work without hacks and has no dependencies.

Changes since v3:
* Rename to imx8m-ddrc. Similar blocks are present on imx7d and imx8qxp/imx8qm
but soc integration is different.
* Move dt bindings to /memory-controllers/fsl/
* Fix dt validation issues
* Fix imx8mm.dtsi ddrc referencing ddrc_opp_table which is only defined in evk
* Move opps to child of ddrc device node
* Only add imx_ddrc_get_dev_status in perf patch.
* Adjust print messages
Link to v3: https://patchwork.kernel.org/cover/11221935/

Leonard Crestez (6):
  clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram clocks
  clk: imx: Mark dram pll on 8mm and 8mn with CLK_GET_RATE_NOCACHE
  dt-bindings: memory: Add bindings for imx8m ddr controller
  PM / devfreq: Add dynamic scaling for imx8m ddr controller
  PM / devfreq: imx8m-ddrc: Measure bandwidth with perf
  arm64: dts: imx8m: Add ddr controller nodes

 .../memory-controllers/fsl/imx8m-ddrc.yaml    |  61 ++
 arch/arm64/boot/dts/freescale/imx8mm-evk.dts  |  18 +
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  13 +-
 .../boot/dts/freescale/imx8mn-ddr4-evk.dts    |  18 +
 arch/arm64/boot/dts/freescale/imx8mn.dtsi     |  13 +-
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts  |  24 +
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  13 +-
 drivers/clk/imx/clk-imx8mm.c                  |  13 +-
 drivers/clk/imx/clk-imx8mn.c                  |  14 +-
 drivers/clk/imx/clk-imx8mq.c                  |  15 +-
 drivers/clk/imx/clk-pll14xx.c                 |   7 +
 drivers/clk/imx/clk.h                         |   1 +
 drivers/devfreq/Kconfig                       |  10 +
 drivers/devfreq/Makefile                      |   1 +
 drivers/devfreq/imx8m-ddrc.c                  | 569 ++++++++++++++++++
 15 files changed, 777 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
 create mode 100644 drivers/devfreq/imx8m-ddrc.c

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/6] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram clocks

[Leonard Crestez]

These clocks are only modified as part of DRAM frequency switches during
which DRAM itself is briefly inaccessible. The switch is performed with
a SMC call to by TF-A which runs from a SRAM area; upon returning to
linux several clocks bits are modified and we need to update them.

For rate bits an easy solution is to just mark with
CLK_GET_RATE_NOCACHE so that new rates are always read back from
registers.

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

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 030b15d7c0ce..c58f988191a5 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -440,13 +440,20 @@ 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);
 
+	/*
+	 * DRAM clocks are manipulated from TF-A outside clock framework.
+	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
+	 */
+	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);
+
 	/* 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_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 9f5a5a56b45e..ca78cb1249a7 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -428,12 +428,20 @@ 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);
+
+	/*
+	 * DRAM clocks are manipulated from TF-A outside clock framework.
+	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
+	 */
+	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 5f10a606d836..3e2ccc17dc66 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -341,11 +341,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MQ_VIDEO_PLL1_OUT] = imx_clk_gate("video_pll1_out", "video_pll1_bypass", base + 0x10, 21);
 
 	clks[IMX8MQ_SYS1_PLL_OUT] = imx_clk_fixed("sys1_pll_out", 800000000);
 	clks[IMX8MQ_SYS2_PLL_OUT] = imx_clk_fixed("sys2_pll_out", 1000000000);
 	clks[IMX8MQ_SYS3_PLL_OUT] = imx_clk_sccg_pll("sys3_pll_out", sys3_pll_out_sels, ARRAY_SIZE(sys3_pll_out_sels), 0, 0, 0, base + 0x48, CLK_IS_CRITICAL);
-	clks[IMX8MQ_DRAM_PLL_OUT] = imx_clk_sccg_pll("dram_pll_out", dram_pll_out_sels, ARRAY_SIZE(dram_pll_out_sels), 0, 0, 0, base + 0x60, CLK_IS_CRITICAL);
+	clks[IMX8MQ_DRAM_PLL_OUT] = imx_clk_sccg_pll("dram_pll_out", dram_pll_out_sels, ARRAY_SIZE(dram_pll_out_sels), 0, 0, 0, base + 0x60,
+			CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
 	clks[IMX8MQ_VIDEO2_PLL_OUT] = imx_clk_sccg_pll("video2_pll_out", video2_pll_out_sels, ARRAY_SIZE(video2_pll_out_sels), 0, 0, 0, base + 0x54, 0);
 
 	/* SYS PLL1 fixed output */
 	clks[IMX8MQ_SYS1_PLL_40M_CG] = imx_clk_gate("sys1_pll_40m_cg", "sys1_pll_out", base + 0x30, 9);
 	clks[IMX8MQ_SYS1_PLL_80M_CG] = imx_clk_gate("sys1_pll_80m_cg", "sys1_pll_out", base + 0x30, 11);
@@ -433,15 +434,21 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
 
 	/* IPG */
 	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 */
+	/*
+	 * DRAM clocks are manipulated from TF-A outside clock framework.
+	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
+	 */
 	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,
+			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_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);
+	/* IP */
 	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/6] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram clocks

[Abel Vesa]

On 19-11-09 00:39:51, Leonard Crestez wrote:
> These clocks are only modified as part of DRAM frequency switches during
> which DRAM itself is briefly inaccessible. The switch is performed with
> a SMC call to by TF-A which runs from a SRAM area; upon returning to
> linux several clocks bits are modified and we need to update them.
> 
> For rate bits an easy solution is to just mark with
> CLK_GET_RATE_NOCACHE so that new rates are always read back from
> registers.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/clk/imx/clk-imx8mm.c | 11 +++++++++--
>  drivers/clk/imx/clk-imx8mn.c | 12 ++++++++++--
>  drivers/clk/imx/clk-imx8mq.c | 15 +++++++++++----
>  3 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index 030b15d7c0ce..c58f988191a5 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -440,13 +440,20 @@ 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);
>  
> +	/*
> +	 * DRAM clocks are manipulated from TF-A outside clock framework.
> +	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 */
> +	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);
> +
>  	/* 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_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 9f5a5a56b45e..ca78cb1249a7 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -428,12 +428,20 @@ 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);
> +
> +	/*
> +	 * DRAM clocks are manipulated from TF-A outside clock framework.
> +	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 */
> +	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);

nitpick: I think it looks better if we stick to one line each clock. 
I know it's against the 80 chars rule, but at least is consistent.

> +
>  	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 5f10a606d836..3e2ccc17dc66 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -341,11 +341,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>  	clks[IMX8MQ_VIDEO_PLL1_OUT] = imx_clk_gate("video_pll1_out", "video_pll1_bypass", base + 0x10, 21);
>  
>  	clks[IMX8MQ_SYS1_PLL_OUT] = imx_clk_fixed("sys1_pll_out", 800000000);
>  	clks[IMX8MQ_SYS2_PLL_OUT] = imx_clk_fixed("sys2_pll_out", 1000000000);
>  	clks[IMX8MQ_SYS3_PLL_OUT] = imx_clk_sccg_pll("sys3_pll_out", sys3_pll_out_sels, ARRAY_SIZE(sys3_pll_out_sels), 0, 0, 0, base + 0x48, CLK_IS_CRITICAL);
> -	clks[IMX8MQ_DRAM_PLL_OUT] = imx_clk_sccg_pll("dram_pll_out", dram_pll_out_sels, ARRAY_SIZE(dram_pll_out_sels), 0, 0, 0, base + 0x60, CLK_IS_CRITICAL);
> +	clks[IMX8MQ_DRAM_PLL_OUT] = imx_clk_sccg_pll("dram_pll_out", dram_pll_out_sels, ARRAY_SIZE(dram_pll_out_sels), 0, 0, 0, base + 0x60,
> +			CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
>  	clks[IMX8MQ_VIDEO2_PLL_OUT] = imx_clk_sccg_pll("video2_pll_out", video2_pll_out_sels, ARRAY_SIZE(video2_pll_out_sels), 0, 0, 0, base + 0x54, 0);
>  
>  	/* SYS PLL1 fixed output */
>  	clks[IMX8MQ_SYS1_PLL_40M_CG] = imx_clk_gate("sys1_pll_40m_cg", "sys1_pll_out", base + 0x30, 9);
>  	clks[IMX8MQ_SYS1_PLL_80M_CG] = imx_clk_gate("sys1_pll_80m_cg", "sys1_pll_out", base + 0x30, 11);
> @@ -433,15 +434,21 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>  
>  	/* IPG */
>  	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 */
> +	/*
> +	 * DRAM clocks are manipulated from TF-A outside clock framework.
> +	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 */
>  	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,
> +			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_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);
> +	/* IP */
>  	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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/6] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram clocks

[Leonard Crestez]

On 12.11.2019 13:18, Abel Vesa wrote:
> On 19-11-09 00:39:51, Leonard Crestez wrote:
>> These clocks are only modified as part of DRAM frequency switches during
>> which DRAM itself is briefly inaccessible. The switch is performed with
>> a SMC call to by TF-A which runs from a SRAM area; upon returning to
>> linux several clocks bits are modified and we need to update them.
>>
>> For rate bits an easy solution is to just mark with
>> CLK_GET_RATE_NOCACHE so that new rates are always read back from
>> registers.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/clk/imx/clk-imx8mm.c | 11 +++++++++--
>>   drivers/clk/imx/clk-imx8mn.c | 12 ++++++++++--
>>   drivers/clk/imx/clk-imx8mq.c | 15 +++++++++++----
>>   3 files changed, 30 insertions(+), 8 deletions(-)

>> --- a/drivers/clk/imx/clk-imx8mn.c
>> +++ b/drivers/clk/imx/clk-imx8mn.c
>> @@ -428,12 +428,20 @@ 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);
>> +
>> +	/*
>> +	 * DRAM clocks are manipulated from TF-A outside clock framework.
>> +	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
>> +	 */
>> +	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);
> 
> nitpick: I think it looks better if we stick to one line each clock.
> I know it's against the 80 chars rule, but at least is consistent.

Yes, there are longer lines in the imx8m* files anyway.

If I fix this (in all instances) can I also add a "reviewed-by"?

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/6] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram clocks

[Abel Vesa]

On 19-11-12 13:43:35, Leonard Crestez wrote:
> On 12.11.2019 13:18, Abel Vesa wrote:
> > On 19-11-09 00:39:51, Leonard Crestez wrote:
> >> These clocks are only modified as part of DRAM frequency switches during
> >> which DRAM itself is briefly inaccessible. The switch is performed with
> >> a SMC call to by TF-A which runs from a SRAM area; upon returning to
> >> linux several clocks bits are modified and we need to update them.
> >>
> >> For rate bits an easy solution is to just mark with
> >> CLK_GET_RATE_NOCACHE so that new rates are always read back from
> >> registers.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   drivers/clk/imx/clk-imx8mm.c | 11 +++++++++--
> >>   drivers/clk/imx/clk-imx8mn.c | 12 ++++++++++--
> >>   drivers/clk/imx/clk-imx8mq.c | 15 +++++++++++----
> >>   3 files changed, 30 insertions(+), 8 deletions(-)
> 
> >> --- a/drivers/clk/imx/clk-imx8mn.c
> >> +++ b/drivers/clk/imx/clk-imx8mn.c
> >> @@ -428,12 +428,20 @@ 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);
> >> +
> >> +	/*
> >> +	 * DRAM clocks are manipulated from TF-A outside clock framework.
> >> +	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> >> +	 */
> >> +	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);
> > 
> > nitpick: I think it looks better if we stick to one line each clock.
> > I know it's against the 80 chars rule, but at least is consistent.
> 
> Yes, there are longer lines in the imx8m* files anyway.
> 
> If I fix this (in all instances) can I also add a "reviewed-by"?
> 

Sorry, I forgot to add the line.

For all the clock related changes:

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> --
> Regards,
> Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/6] clk: imx: Mark dram pll on 8mm and 8mn with CLK_GET_RATE_NOCACHE

[Leonard Crestez]

DRAM frequency switches are executed in firmware and can change the
configuration of the DRAM PLL outside linux. Mark these CLKs with
CLK_GET_RATE_NOCACHE so we always read back the PLL config registers and
recalculate rates.

In current DRAM frequency tables on 8mm/8mn only the maximum frequency
uses the PLL so it's always configured in the same way. However reading
back the PLL configuration is the correct behavior and allows additional
setpoints in the future.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/clk/imx/clk-imx8mm.c  | 2 +-
 drivers/clk/imx/clk-imx8mn.c  | 2 +-
 drivers/clk/imx/clk-pll14xx.c | 7 +++++++
 drivers/clk/imx/clk.h         | 1 +
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index c58f988191a5..d500bac3afa1 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -326,11 +326,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MM_SYS_PLL3_REF_SEL] = imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
 
 	clks[IMX8MM_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx_1443x_pll);
 	clks[IMX8MM_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx_1443x_pll);
 	clks[IMX8MM_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx_1443x_pll);
-	clks[IMX8MM_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_pll);
+	clks[IMX8MM_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_dram_pll);
 	clks[IMX8MM_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx_1416x_pll);
 	clks[IMX8MM_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx_1416x_pll);
 	clks[IMX8MM_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx_1416x_pll);
 	clks[IMX8MM_SYS_PLL1] = imx_clk_fixed("sys_pll1", 800000000);
 	clks[IMX8MM_SYS_PLL2] = imx_clk_fixed("sys_pll2", 1000000000);
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index ca78cb1249a7..9c605ca1b631 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -323,11 +323,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MN_SYS_PLL3_REF_SEL] = imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
 
 	clks[IMX8MN_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx_1443x_pll);
 	clks[IMX8MN_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx_1443x_pll);
 	clks[IMX8MN_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx_1443x_pll);
-	clks[IMX8MN_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_pll);
+	clks[IMX8MN_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_dram_pll);
 	clks[IMX8MN_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx_1416x_pll);
 	clks[IMX8MN_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx_1416x_pll);
 	clks[IMX8MN_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx_1416x_pll);
 	clks[IMX8MN_SYS_PLL1] = imx_clk_fixed("sys_pll1", 800000000);
 	clks[IMX8MN_SYS_PLL2] = imx_clk_fixed("sys_pll2", 1000000000);
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 5c458199060a..a6d31a7262ef 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -65,10 +65,17 @@ struct imx_pll14xx_clk imx_1443x_pll = {
 	.type = PLL_1443X,
 	.rate_table = imx_pll1443x_tbl,
 	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
 };
 
+struct imx_pll14xx_clk imx_1443x_dram_pll = {
+	.type = PLL_1443X,
+	.rate_table = imx_pll1443x_tbl,
+	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
+	.flags = CLK_GET_RATE_NOCACHE,
+};
+
 struct imx_pll14xx_clk imx_1416x_pll = {
 	.type = PLL_1416X,
 	.rate_table = imx_pll1416x_tbl,
 	.rate_count = ARRAY_SIZE(imx_pll1416x_tbl),
 };
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index bc5bb6ac8636..81122c9ab842 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -50,10 +50,11 @@ struct imx_pll14xx_clk {
 	int flags;
 };
 
 extern struct imx_pll14xx_clk imx_1416x_pll;
 extern struct imx_pll14xx_clk imx_1443x_pll;
+extern struct imx_pll14xx_clk imx_1443x_dram_pll;
 
 #define imx_clk_cpu(name, parent_name, div, mux, pll, step) \
 	imx_clk_hw_cpu(name, parent_name, div, mux, pll, step)->clk
 
 #define clk_register_gate2(dev, name, parent_name, flags, reg, bit_idx, \
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] clk: imx: Mark dram pll on 8mm and 8mn with CLK_GET_RATE_NOCACHE

[Abel Vesa]

On 19-11-09 00:39:52, Leonard Crestez wrote:
> DRAM frequency switches are executed in firmware and can change the
> configuration of the DRAM PLL outside linux. Mark these CLKs with
> CLK_GET_RATE_NOCACHE so we always read back the PLL config registers and
> recalculate rates.
> 
> In current DRAM frequency tables on 8mm/8mn only the maximum frequency
> uses the PLL so it's always configured in the same way. However reading
> back the PLL configuration is the correct behavior and allows additional
> setpoints in the future.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

This one looks fine.

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> ---
>  drivers/clk/imx/clk-imx8mm.c  | 2 +-
>  drivers/clk/imx/clk-imx8mn.c  | 2 +-
>  drivers/clk/imx/clk-pll14xx.c | 7 +++++++
>  drivers/clk/imx/clk.h         | 1 +
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index c58f988191a5..d500bac3afa1 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -326,11 +326,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  	clks[IMX8MM_SYS_PLL3_REF_SEL] = imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
>  
>  	clks[IMX8MM_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx_1443x_pll);
>  	clks[IMX8MM_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx_1443x_pll);
>  	clks[IMX8MM_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx_1443x_pll);
> -	clks[IMX8MM_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_pll);
> +	clks[IMX8MM_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_dram_pll);
>  	clks[IMX8MM_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx_1416x_pll);
>  	clks[IMX8MM_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx_1416x_pll);
>  	clks[IMX8MM_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx_1416x_pll);
>  	clks[IMX8MM_SYS_PLL1] = imx_clk_fixed("sys_pll1", 800000000);
>  	clks[IMX8MM_SYS_PLL2] = imx_clk_fixed("sys_pll2", 1000000000);
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index ca78cb1249a7..9c605ca1b631 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -323,11 +323,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
>  	clks[IMX8MN_SYS_PLL3_REF_SEL] = imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
>  
>  	clks[IMX8MN_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx_1443x_pll);
>  	clks[IMX8MN_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx_1443x_pll);
>  	clks[IMX8MN_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx_1443x_pll);
> -	clks[IMX8MN_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_pll);
> +	clks[IMX8MN_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_dram_pll);
>  	clks[IMX8MN_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx_1416x_pll);
>  	clks[IMX8MN_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx_1416x_pll);
>  	clks[IMX8MN_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx_1416x_pll);
>  	clks[IMX8MN_SYS_PLL1] = imx_clk_fixed("sys_pll1", 800000000);
>  	clks[IMX8MN_SYS_PLL2] = imx_clk_fixed("sys_pll2", 1000000000);
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 5c458199060a..a6d31a7262ef 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -65,10 +65,17 @@ struct imx_pll14xx_clk imx_1443x_pll = {
>  	.type = PLL_1443X,
>  	.rate_table = imx_pll1443x_tbl,
>  	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
>  };
>  
> +struct imx_pll14xx_clk imx_1443x_dram_pll = {
> +	.type = PLL_1443X,
> +	.rate_table = imx_pll1443x_tbl,
> +	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
> +	.flags = CLK_GET_RATE_NOCACHE,
> +};
> +
>  struct imx_pll14xx_clk imx_1416x_pll = {
>  	.type = PLL_1416X,
>  	.rate_table = imx_pll1416x_tbl,
>  	.rate_count = ARRAY_SIZE(imx_pll1416x_tbl),
>  };
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index bc5bb6ac8636..81122c9ab842 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -50,10 +50,11 @@ struct imx_pll14xx_clk {
>  	int flags;
>  };
>  
>  extern struct imx_pll14xx_clk imx_1416x_pll;
>  extern struct imx_pll14xx_clk imx_1443x_pll;
> +extern struct imx_pll14xx_clk imx_1443x_dram_pll;
>  
>  #define imx_clk_cpu(name, parent_name, div, mux, pll, step) \
>  	imx_clk_hw_cpu(name, parent_name, div, mux, pll, step)->clk
>  
>  #define clk_register_gate2(dev, name, parent_name, flags, reg, bit_idx, \
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/6] dt-bindings: memory: Add bindings for imx8m 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>
---
 .../memory-controllers/fsl/imx8m-ddrc.yaml    | 61 +++++++++++++++++++
 1 file changed, 61 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..ac06d7595143
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/imx8m-ddrc.yaml
@@ -0,0 +1,61 @@
+# 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
+
+  devfreq-events:
+    description: Phandle of PMU node
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+
+required:
+  - reg
+  - compatible
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    ddrc: dram-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>;
+    };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Leonard Crestez]

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

The DRAM clocks on imx8m 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. The dram_alt and dram_apb clocks are "imx composite" and their
parent can also be modified.

This driver will prepare/enable the new parents ahead of switching (so
that the expected roots are enabled) and afterwards it will call
clk_set_parent to ensure the parents in clock framework are up-to-date.

The driver relies on dram_pll dram_alt and dram_apb being marked with
CLK_GET_RATE_NOCACHE for rate updates.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/Kconfig      |  10 +
 drivers/devfreq/Makefile     |   1 +
 drivers/devfreq/imx8m-ddrc.c | 416 +++++++++++++++++++++++++++++++++++
 3 files changed, 427 insertions(+)
 create mode 100644 drivers/devfreq/imx8m-ddrc.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index defe1d438710..c519fd27808f 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -90,10 +90,20 @@ 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_IMX8M_DDRC_DEVFREQ
+	tristate "i.MX8M DDRC DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_SIMPLE_ONDEMAND
+	select DEVFREQ_GOV_USERSPACE
+	select PM_OPP
+	help
+	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
+	  adjusting DRAM frequency.
+
 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..3eb4d5e6635c 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-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/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
new file mode 100644
index 000000000000..51903fee21a7
--- /dev/null
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -0,0 +1,416 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 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 imx8m_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 IMX8M_DDRC_MAX_FREQ_COUNT 4
+
+/*
+ * i.MX8M 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 imx8m_ddrc {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+
+	/* For frequency switching: */
+	struct clk *dram_core;
+	struct clk *dram_pll;
+	struct clk *dram_alt;
+	struct clk *dram_apb;
+
+	int freq_count;
+	struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT];
+};
+
+static struct imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
+						    unsigned long rate)
+{
+	struct imx8m_ddrc_freq *freq;
+	int i;
+
+	/*
+	 * Firmware reports values in MT/s, so we round-down from Hz
+	 * Rounding is extra generous to ensure a match.
+	 */
+	rate = DIV_ROUND_CLOSEST(rate, 250000);
+	for (i = 0; i < priv->freq_count; ++i) {
+		freq = &priv->freq_table[i];
+		if (freq->rate == rate ||
+				freq->rate + 1 == rate ||
+				freq->rate - 1 == rate)
+			return freq;
+	}
+
+	return NULL;
+}
+
+static void imx8m_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 imx8m_ddrc_set_freq(struct device *dev, struct imx8m_ddrc_freq *freq)
+{
+	struct imx8m_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;
+	}
+
+	imx8m_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 set dram_core parent: %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 set 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 set dram_apb parent: %d\n", ret);
+	}
+
+	/*
+	 * Explicitly refresh dram PLL rate.
+	 *
+	 * Even if it's marked with CLK_GET_RATE_NOCACHE the rate will not be
+	 * automatically refreshed when clk_get_rate is called on children.
+	 */
+	clk_get_rate(priv->dram_pll);
+
+	/*
+	 * 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 imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
+{
+	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
+	struct imx8m_ddrc_freq *freq_info;
+	struct dev_pm_opp *new_opp;
+	unsigned long old_freq, 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;
+	}
+	dev_pm_opp_put(new_opp);
+
+	old_freq = clk_get_rate(priv->dram_core);
+	if (*freq == old_freq)
+		return 0;
+
+	freq_info = imx8m_ddrc_find_freq(priv, *freq);
+	if (!freq_info)
+		return -EINVAL;
+	ret = imx8m_ddrc_set_freq(dev, freq_info);
+
+	/* Also read back the clk rate to verify switch was correct */
+	new_freq = clk_get_rate(priv->dram_core);
+	if (ret || *freq != new_freq)
+		dev_err(dev, "ddrc failed freq set to %lu from %lu, now at %lu\n",
+			old_freq, *freq, new_freq);
+	else
+		dev_dbg(dev, "ddrc freq set to %lu (was %lu)\n",
+			*freq, old_freq);
+
+	return ret;
+}
+
+static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->dram_core);
+
+	return 0;
+}
+
+static int imx8m_ddrc_init_freq_info(struct device *dev)
+{
+	struct imx8m_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 > IMX8M_DDRC_MAX_FREQ_COUNT)
+		return -ENODEV;
+
+	for (index = 0; index < priv->freq_count; ++index) {
+		struct imx8m_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 -ENODEV;
+
+		freq->rate = res.a0;
+		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 -ENODEV;
+		/* dram_apb and dram_alt have exactly 8 possible parents */
+		if (freq->dram_alt_parent_index > 8 ||
+				freq->dram_apb_parent_index > 8)
+			return -ENODEV;
+		/* dram_core from alt requires explicit dram_alt parent */
+		if (freq->dram_core_parent_index == 2 &&
+				freq->dram_alt_parent_index == 0)
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
+/* imx8m_ddrc_check_opps() - disable OPPs not supported by firmware */
+static int imx8m_ddrc_check_opps(struct device *dev)
+{
+	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
+	struct imx8m_ddrc_freq *freq_info;
+	struct dev_pm_opp *opp;
+	unsigned long freq;
+
+	freq = ULONG_MAX;
+	while (true) {
+		opp = dev_pm_opp_find_freq_floor(dev, &freq);
+		if (opp == ERR_PTR(-ERANGE))
+			break;
+		if (IS_ERR(opp)) {
+			dev_err(dev, "Failed enumerating OPPs: %ld\n",
+				PTR_ERR(opp));
+			return PTR_ERR(opp);
+		}
+		dev_pm_opp_put(opp);
+
+		freq_info = imx8m_ddrc_find_freq(priv, freq);
+		if (!freq_info) {
+			dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",
+					freq, DIV_ROUND_CLOSEST(freq, 250000));
+			dev_pm_opp_disable(dev, freq);
+		}
+
+		freq--;
+	}
+
+	return 0;
+}
+
+static void imx8m_ddrc_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx8m_ddrc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx8m_ddrc *priv;
+	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 = imx8m_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, "core");
+	priv->dram_pll = devm_clk_get(dev, "pll");
+	priv->dram_alt = devm_clk_get(dev, "alt");
+	priv->dram_apb = devm_clk_get(dev, "apb");
+	if (IS_ERR(priv->dram_core) ||
+		IS_ERR(priv->dram_pll) ||
+		IS_ERR(priv->dram_alt) ||
+		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;
+	}
+
+	ret = imx8m_ddrc_check_opps(dev);
+	if (ret < 0)
+		goto err;
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx8m_ddrc_target;
+	priv->profile.exit = imx8m_ddrc_exit;
+	priv->profile.get_cur_freq = imx8m_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 imx8m_ddrc_of_match[] = {
+	{ .compatible = "fsl,imx8m-ddrc", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match);
+
+static struct platform_driver imx8m_ddrc_platdrv = {
+	.probe		= imx8m_ddrc_probe,
+	.driver = {
+		.name	= "imx8m-ddrc-devfreq",
+		.of_match_table = of_match_ptr(imx8m_ddrc_of_match),
+	},
+};
+module_platform_driver(imx8m_ddrc_platdrv);
+
+MODULE_DESCRIPTION("i.MX8M DDR Controller frequency driver");
+MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Chanwoo Choi]

Hi Leonard,

On 11/9/19 7:39 AM, Leonard Crestez wrote:
> Add driver for dynamic scaling the DDR Controller on imx8m chips. Actual
> frequency switching is implemented inside TF-A, this driver wraps the
> SMC calls and synchronizes the clk tree.
> 
> The DRAM clocks on imx8m 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. The dram_alt and dram_apb clocks are "imx composite" and their
> parent can also be modified.
> 
> This driver will prepare/enable the new parents ahead of switching (so
> that the expected roots are enabled) and afterwards it will call
> clk_set_parent to ensure the parents in clock framework are up-to-date.
> 
> The driver relies on dram_pll dram_alt and dram_apb being marked with
> CLK_GET_RATE_NOCACHE for rate updates.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/Kconfig      |  10 +
>  drivers/devfreq/Makefile     |   1 +
>  drivers/devfreq/imx8m-ddrc.c | 416 +++++++++++++++++++++++++++++++++++
>  3 files changed, 427 insertions(+)
>  create mode 100644 drivers/devfreq/imx8m-ddrc.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index defe1d438710..c519fd27808f 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -90,10 +90,20 @@ 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_IMX8M_DDRC_DEVFREQ
> +	tristate "i.MX8M DDRC DEVFREQ Driver"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
> +	select DEVFREQ_GOV_USERSPACE
> +	select PM_OPP

It doesn't need to add. CONFIG_DEVFREQ add 'select PM_OPP'
because 'PM_OPP' is mandatory for devfreq.

> +	help
> +	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
> +	  adjusting DRAM frequency.
> +
>  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..3eb4d5e6635c 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-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/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
> new file mode 100644
> index 000000000000..51903fee21a7
> --- /dev/null
> +++ b/drivers/devfreq/imx8m-ddrc.c
> @@ -0,0 +1,416 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 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 imx8m_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 IMX8M_DDRC_MAX_FREQ_COUNT 4
> +
> +/*
> + * i.MX8M 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 imx8m_ddrc {
> +	struct devfreq_dev_profile profile;
> +	struct devfreq *devfreq;
> +
> +	/* For frequency switching: */
> +	struct clk *dram_core;
> +	struct clk *dram_pll;
> +	struct clk *dram_alt;
> +	struct clk *dram_apb;
> +
> +	int freq_count;
> +	struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT];
> +};
> +
> +static struct imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
> +						    unsigned long rate)
> +{
> +	struct imx8m_ddrc_freq *freq;
> +	int i;
> +
> +	/*
> +	 * Firmware reports values in MT/s, so we round-down from Hz
> +	 * Rounding is extra generous to ensure a match.
> +	 */
> +	rate = DIV_ROUND_CLOSEST(rate, 250000);
> +	for (i = 0; i < priv->freq_count; ++i) {
> +		freq = &priv->freq_table[i];
> +		if (freq->rate == rate ||
> +				freq->rate + 1 == rate ||
> +				freq->rate - 1 == rate)
> +			return freq;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void imx8m_ddrc_smc_set_freq(int target_freq)
> +{
> +	struct arm_smccc_res res;
> +	u32 online_cpus = 0;
> +	int cpu;
> +
> +	local_irq_disable();

local_irq_disable is more proper than local_irq_save()?

> +
> +	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();

ditto. local_irq_restore() instead of 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 imx8m_ddrc_set_freq(struct device *dev, struct imx8m_ddrc_freq *freq)
> +{
> +	struct imx8m_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(

You can use 'clk_hw_get_parent_by_index' directly.
And, you need to check whether the return value is NULL or not.

> +			priv->dram_core, freq->dram_core_parent_index - 1);
> +	new_dram_alt_parent = clk_get_parent_by_index(

ditto.

> +			priv->dram_alt, freq->dram_alt_parent_index - 1);
> +	new_dram_apb_parent = clk_get_parent_by_index(

ditto.

> +			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;

I think that 'dis' is not general expression for 'disable'.
Just, I think that 'out_disable_core' is better than '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;

ditto.

> +	}
> +
> +	imx8m_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 set dram_core parent: %d\n", ret);

if you don't return directly, you better to use 'dev_warn' instead of 'dev_err'.

> +	if (new_dram_alt_parent) {
> +		ret = clk_set_parent(priv->dram_alt, new_dram_alt_parent);
> +		if (ret)
> +			dev_err(dev, "failed set dram_alt parent: %d\n", ret);

ditto. Use dev_warn.
 
> +	}
> +	if (new_dram_apb_parent) {
> +		ret = clk_set_parent(priv->dram_apb, new_dram_apb_parent);
> +		if (ret)
> +			dev_err(dev, "failed set dram_apb parent: %d\n", ret);

ditto. Use dev_warn.

> +	}
> +
> +	/*
> +	 * Explicitly refresh dram PLL rate.
> +	 *
> +	 * Even if it's marked with CLK_GET_RATE_NOCACHE the rate will not be

nitpick:
What is more proper description either or 'Even if' or 'if' ?

> +	 * automatically refreshed when clk_get_rate is called on children.
> +	 */
> +	clk_get_rate(priv->dram_pll);
> +
> +	/*
> +	 * 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 imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
> +{
> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> +	struct imx8m_ddrc_freq *freq_info;
> +	struct dev_pm_opp *new_opp;
> +	unsigned long old_freq, 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;
> +	}
> +	dev_pm_opp_put(new_opp);
> +
> +	old_freq = clk_get_rate(priv->dram_core);
> +	if (*freq == old_freq)
> +		return 0;
> +
> +	freq_info = imx8m_ddrc_find_freq(priv, *freq);
> +	if (!freq_info)
> +		return -EINVAL;
> +	ret = imx8m_ddrc_set_freq(dev, freq_info);

Need to check whether 'ret' is valid or not.

> +
> +	/* Also read back the clk rate to verify switch was correct */
> +	new_freq = clk_get_rate(priv->dram_core);
> +	if (ret || *freq != new_freq)

You should check 'ret' right after imx8m_ddrc_set_freq()
instead of this position.

> +		dev_err(dev, "ddrc failed freq set to %lu from %lu, now at %lu\n",
> +			old_freq, *freq, new_freq);
> +	else
> +		dev_dbg(dev, "ddrc freq set to %lu (was %lu)\n",
> +			*freq, old_freq);
> +
> +	return ret;
> +}
> +
> +static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(priv->dram_core);
> +
> +	return 0;
> +}
> +
> +static int imx8m_ddrc_init_freq_info(struct device *dev)
> +{
> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> +	struct arm_smccc_res res;
> +	int index;
> +
> +	/*
> +	 * An error here means DDR DVFS API not supported by firmware
> +	 */

Don't need to add multiple line comments. It is possible to be change as following:

	/* 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 > IMX8M_DDRC_MAX_FREQ_COUNT)
> +		return -ENODEV;
> +
> +	for (index = 0; index < priv->freq_count; ++index) {
> +		struct imx8m_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 -ENODEV;
> +
> +		freq->rate = res.a0;
> +		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 -ENODEV;
> +		/* dram_apb and dram_alt have exactly 8 possible parents */
> +		if (freq->dram_alt_parent_index > 8 ||
> +				freq->dram_apb_parent_index > 8)
> +			return -ENODEV;
> +		/* dram_core from alt requires explicit dram_alt parent */
> +		if (freq->dram_core_parent_index == 2 &&
> +				freq->dram_alt_parent_index == 0)
> +			return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +/* imx8m_ddrc_check_opps() - disable OPPs not supported by firmware */

nitpick:
On this driver, there are no some description for internal function.
In order to keep the coding style, you better to move this comment
into function before 'freq = ULONG_MAX'.

> +static int imx8m_ddrc_check_opps(struct device *dev)
> +{
> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> +	struct imx8m_ddrc_freq *freq_info;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq;
> +
> +	freq = ULONG_MAX;
> +	while (true) {
> +		opp = dev_pm_opp_find_freq_floor(dev, &freq);
> +		if (opp == ERR_PTR(-ERANGE))
> +			break;
> +		if (IS_ERR(opp)) {
> +			dev_err(dev, "Failed enumerating OPPs: %ld\n",
> +				PTR_ERR(opp));
> +			return PTR_ERR(opp);
> +		}
> +		dev_pm_opp_put(opp);
> +
> +		freq_info = imx8m_ddrc_find_freq(priv, freq);
> +		if (!freq_info) {
> +			dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",
> +					freq, DIV_ROUND_CLOSEST(freq, 250000));
> +			dev_pm_opp_disable(dev, freq);
> +		}
> +
> +		freq--;
> +	}
> +
> +	return 0;
> +}
> +
> +static void imx8m_ddrc_exit(struct device *dev)
> +{
> +	dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int imx8m_ddrc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx8m_ddrc *priv;
> +	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 = imx8m_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, "core");
> +	priv->dram_pll = devm_clk_get(dev, "pll");
> +	priv->dram_alt = devm_clk_get(dev, "alt");
> +	priv->dram_apb = devm_clk_get(dev, "apb");
> +	if (IS_ERR(priv->dram_core) ||
> +		IS_ERR(priv->dram_pll) ||
> +		IS_ERR(priv->dram_alt) ||
> +		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;
> +	}
> +
> +	ret = imx8m_ddrc_check_opps(dev);
> +	if (ret < 0)
> +		goto err;
> +
> +	priv->profile.polling_ms = 1000;
> +	priv->profile.target = imx8m_ddrc_target;
> +	priv->profile.exit = imx8m_ddrc_exit;
> +	priv->profile.get_cur_freq = imx8m_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 imx8m_ddrc_of_match[] = {
> +	{ .compatible = "fsl,imx8m-ddrc", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match);
> +
> +static struct platform_driver imx8m_ddrc_platdrv = {
> +	.probe		= imx8m_ddrc_probe,
> +	.driver = {
> +		.name	= "imx8m-ddrc-devfreq",
> +		.of_match_table = of_match_ptr(imx8m_ddrc_of_match),
> +	},
> +};
> +module_platform_driver(imx8m_ddrc_platdrv);
> +
> +MODULE_DESCRIPTION("i.MX8M DDR Controller frequency driver");
> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
> +MODULE_LICENSE("GPL v2");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Leonard Crestez]

On 11.11.2019 05:18, Chanwoo Choi wrote:
> Hi Leonard,
> 
> On 11/9/19 7:39 AM, Leonard Crestez wrote:
>> Add driver for dynamic scaling the DDR Controller on imx8m chips. Actual
>> frequency switching is implemented inside TF-A, this driver wraps the
>> SMC calls and synchronizes the clk tree.
>>
>> The DRAM clocks on imx8m 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. The dram_alt and dram_apb clocks are "imx composite" and their
>> parent can also be modified.
>>
>> This driver will prepare/enable the new parents ahead of switching (so
>> that the expected roots are enabled) and afterwards it will call
>> clk_set_parent to ensure the parents in clock framework are up-to-date.
>>
>> The driver relies on dram_pll dram_alt and dram_apb being marked with
>> CLK_GET_RATE_NOCACHE for rate updates.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/Kconfig      |  10 +
>>   drivers/devfreq/Makefile     |   1 +
>>   drivers/devfreq/imx8m-ddrc.c | 416 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 427 insertions(+)
>>   create mode 100644 drivers/devfreq/imx8m-ddrc.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index defe1d438710..c519fd27808f 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -90,10 +90,20 @@ 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_IMX8M_DDRC_DEVFREQ
>> +	tristate "i.MX8M DDRC DEVFREQ Driver"
>> +	depends on ARCH_MXC || COMPILE_TEST
>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +	select DEVFREQ_GOV_USERSPACE
>> +	select PM_OPP
> 
> It doesn't need to add. CONFIG_DEVFREQ add 'select PM_OPP'
> because 'PM_OPP' is mandatory for devfreq.

OK.

All other drivers select PM_OPP, this was just copied

>> +	help
>> +	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
>> +	  adjusting DRAM frequency.
>> +
>>   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..3eb4d5e6635c 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-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/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
>> new file mode 100644
>> index 000000000000..51903fee21a7
>> --- /dev/null
>> +++ b/drivers/devfreq/imx8m-ddrc.c
>> @@ -0,0 +1,416 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 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 imx8m_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 IMX8M_DDRC_MAX_FREQ_COUNT 4
>> +
>> +/*
>> + * i.MX8M 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 imx8m_ddrc {
>> +	struct devfreq_dev_profile profile;
>> +	struct devfreq *devfreq;
>> +
>> +	/* For frequency switching: */
>> +	struct clk *dram_core;
>> +	struct clk *dram_pll;
>> +	struct clk *dram_alt;
>> +	struct clk *dram_apb;
>> +
>> +	int freq_count;
>> +	struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT];
>> +};
>> +
>> +static struct imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>> +						    unsigned long rate)
>> +{
>> +	struct imx8m_ddrc_freq *freq;
>> +	int i;
>> +
>> +	/*
>> +	 * Firmware reports values in MT/s, so we round-down from Hz
>> +	 * Rounding is extra generous to ensure a match.
>> +	 */
>> +	rate = DIV_ROUND_CLOSEST(rate, 250000);
>> +	for (i = 0; i < priv->freq_count; ++i) {
>> +		freq = &priv->freq_table[i];
>> +		if (freq->rate == rate ||
>> +				freq->rate + 1 == rate ||
>> +				freq->rate - 1 == rate)
>> +			return freq;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static void imx8m_ddrc_smc_set_freq(int target_freq)
>> +{
>> +	struct arm_smccc_res res;
>> +	u32 online_cpus = 0;
>> +	int cpu;
>> +
>> +	local_irq_disable();
> 
> local_irq_disable is more proper than local_irq_save()?

There's no need to use save/restore because we know that irqs are 
enabled when entering the function. This is only called from devfreq 
set_target which runs in process context.

> 
>> +
>> +	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();
> 
> ditto. local_irq_restore() instead of 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 imx8m_ddrc_set_freq(struct device *dev, struct imx8m_ddrc_freq *freq)
>> +{
>> +	struct imx8m_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(
>  > You can use 'clk_hw_get_parent_by_index' directly.

The helper above avoids duplicating ?:

> And, you need to check whether the return value is NULL or not.
Yes for dram_core_parent but others are deliberately allowed to be NULL. 
For some setpoints some parents might be indifferent or require no 
changes. For example when dram_core has pll as parent the expectation is 
that dram_all will be OFF so parent is irrelevant.

Driver relies on the fact that clock API ignores prepare/enable for NULL 
clocks. I can add a comment explaining that.

>> +			priv->dram_core, freq->dram_core_parent_index - 1);
>> +	new_dram_alt_parent = clk_get_parent_by_index(
> 
> ditto.
> 
>> +			priv->dram_alt, freq->dram_alt_parent_index - 1);
>> +	new_dram_apb_parent = clk_get_parent_by_index(
> 
> ditto.

>> +			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;
> 
> I think that 'dis' is not general expression for 'disable'.
> Just, I think that 'out_disable_core' is better than 'out_dis_core'.

OK

>> +	}
>> +	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;
> 
> ditto.
> 
>> +	}
>> +
>> +	imx8m_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 set dram_core parent: %d\n", ret);
> 
> if you don't return directly, you better to use 'dev_warn' instead of 'dev_err'.

OK.

>> +	if (new_dram_alt_parent) {
>> +		ret = clk_set_parent(priv->dram_alt, new_dram_alt_parent);
>> +		if (ret)
>> +			dev_err(dev, "failed set dram_alt parent: %d\n", ret);
> 
> ditto. Use dev_warn.
>   
>> +	}
>> +	if (new_dram_apb_parent) {
>> +		ret = clk_set_parent(priv->dram_apb, new_dram_apb_parent);
>> +		if (ret)
>> +			dev_err(dev, "failed set dram_apb parent: %d\n", ret);
> 
> ditto. Use dev_warn.
> 
>> +	}
>> +
>> +	/*
>> +	 * Explicitly refresh dram PLL rate.
>> +	 *
>> +	 * Even if it's marked with CLK_GET_RATE_NOCACHE the rate will not be
> 
> nitpick:
> What is more proper description either or 'Even if' or 'if' ?

Rate updates work for dram_alt/apb but not for dram_pll because 
additional clocks might be present between the PLL and dram_core mux. 
This happens *even if* pll is marked with CLK_GET_RATE_NOCACHE.

>> +	 * automatically refreshed when clk_get_rate is called on children.
>> +	 */
>> +	clk_get_rate(priv->dram_pll);
>> +
>> +	/*
>> +	 * 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 imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
>> +{
>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>> +	struct imx8m_ddrc_freq *freq_info;
>> +	struct dev_pm_opp *new_opp;
>> +	unsigned long old_freq, 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;
>> +	}
>> +	dev_pm_opp_put(new_opp);
>> +
>> +	old_freq = clk_get_rate(priv->dram_core);
>> +	if (*freq == old_freq)
>> +		return 0;
>> +
>> +	freq_info = imx8m_ddrc_find_freq(priv, *freq);
>> +	if (!freq_info)
>> +		return -EINVAL;
>> +	ret = imx8m_ddrc_set_freq(dev, freq_info);
> 
> Need to check whether 'ret' is valid or not.
> 
>> +
>> +	/* Also read back the clk rate to verify switch was correct */
>> +	new_freq = clk_get_rate(priv->dram_core);
>> +	if (ret || *freq != new_freq)
> 
> You should check 'ret' right after imx8m_ddrc_set_freq()
> instead of this position.

OK, I can add two error paths.

>> +		dev_err(dev, "ddrc failed freq set to %lu from %lu, now at %lu\n",
>> +			old_freq, *freq, new_freq);
>> +	else
>> +		dev_dbg(dev, "ddrc freq set to %lu (was %lu)\n",
>> +			*freq, old_freq);
>> +
>> +	return ret;
>> +}
>> +
>> +static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>> +{
>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>> +
>> +	*freq = clk_get_rate(priv->dram_core);
>> +
>> +	return 0;
>> +}
>> +
>> +static int imx8m_ddrc_init_freq_info(struct device *dev)
>> +{
>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>> +	struct arm_smccc_res res;
>> +	int index;
>> +
>> +	/*
>> +	 * An error here means DDR DVFS API not supported by firmware
>> +	 */
> 
> Don't need to add multiple line comments. It is possible to be change as following:
> 
> 	/* An error here means DDR DVFS API not supported by firmware */

OK.
>> +	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 > IMX8M_DDRC_MAX_FREQ_COUNT)
>> +		return -ENODEV;
>> +
>> +	for (index = 0; index < priv->freq_count; ++index) {
>> +		struct imx8m_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 -ENODEV;
>> +
>> +		freq->rate = res.a0;
>> +		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 -ENODEV;
>> +		/* dram_apb and dram_alt have exactly 8 possible parents */
>> +		if (freq->dram_alt_parent_index > 8 ||
>> +				freq->dram_apb_parent_index > 8)
>> +			return -ENODEV;
>> +		/* dram_core from alt requires explicit dram_alt parent */
>> +		if (freq->dram_core_parent_index == 2 &&
>> +				freq->dram_alt_parent_index == 0)
>> +			return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/* imx8m_ddrc_check_opps() - disable OPPs not supported by firmware */
> 
> nitpick:
> On this driver, there are no some description for internal function.
> In order to keep the coding style, you better to move this comment
> into function before 'freq = ULONG_MAX'.

Is it a problem if only some internal functions have documentation? I 
can add docs to more functions.

BTW: how can I generate devfreq documentation from these comments?

>> +static int imx8m_ddrc_check_opps(struct device *dev)
>> +{
>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>> +	struct imx8m_ddrc_freq *freq_info;
>> +	struct dev_pm_opp *opp;
>> +	unsigned long freq;
>> +
>> +	freq = ULONG_MAX;
>> +	while (true) {
>> +		opp = dev_pm_opp_find_freq_floor(dev, &freq);
>> +		if (opp == ERR_PTR(-ERANGE))
>> +			break;
>> +		if (IS_ERR(opp)) {
>> +			dev_err(dev, "Failed enumerating OPPs: %ld\n",
>> +				PTR_ERR(opp));
>> +			return PTR_ERR(opp);
>> +		}
>> +		dev_pm_opp_put(opp);
>> +
>> +		freq_info = imx8m_ddrc_find_freq(priv, freq);
>> +		if (!freq_info) {
>> +			dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",
>> +					freq, DIV_ROUND_CLOSEST(freq, 250000));
>> +			dev_pm_opp_disable(dev, freq);
>> +		}
>> +
>> +		freq--;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void imx8m_ddrc_exit(struct device *dev)
>> +{
>> +	dev_pm_opp_of_remove_table(dev);
>> +}
>> +
>> +static int imx8m_ddrc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct imx8m_ddrc *priv;
>> +	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 = imx8m_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, "core");
>> +	priv->dram_pll = devm_clk_get(dev, "pll");
>> +	priv->dram_alt = devm_clk_get(dev, "alt");
>> +	priv->dram_apb = devm_clk_get(dev, "apb");
>> +	if (IS_ERR(priv->dram_core) ||
>> +		IS_ERR(priv->dram_pll) ||
>> +		IS_ERR(priv->dram_alt) ||
>> +		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;
>> +	}
>> +
>> +	ret = imx8m_ddrc_check_opps(dev);
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	priv->profile.polling_ms = 1000;
>> +	priv->profile.target = imx8m_ddrc_target;
>> +	priv->profile.exit = imx8m_ddrc_exit;
>> +	priv->profile.get_cur_freq = imx8m_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 imx8m_ddrc_of_match[] = {
>> +	{ .compatible = "fsl,imx8m-ddrc", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match);
>> +
>> +static struct platform_driver imx8m_ddrc_platdrv = {
>> +	.probe		= imx8m_ddrc_probe,
>> +	.driver = {
>> +		.name	= "imx8m-ddrc-devfreq",
>> +		.of_match_table = of_match_ptr(imx8m_ddrc_of_match),
>> +	},
>> +};
>> +module_platform_driver(imx8m_ddrc_platdrv);
>> +
>> +MODULE_DESCRIPTION("i.MX8M DDR Controller frequency driver");
>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
>> +MODULE_LICENSE("GPL v2");


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Chanwoo Choi]

On 11/12/19 2:23 AM, Leonard Crestez wrote:
> On 11.11.2019 05:18, Chanwoo Choi wrote:
>> Hi Leonard,
>>
>> On 11/9/19 7:39 AM, Leonard Crestez wrote:
>>> Add driver for dynamic scaling the DDR Controller on imx8m chips. Actual
>>> frequency switching is implemented inside TF-A, this driver wraps the
>>> SMC calls and synchronizes the clk tree.
>>>
>>> The DRAM clocks on imx8m 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. The dram_alt and dram_apb clocks are "imx composite" and their
>>> parent can also be modified.
>>>
>>> This driver will prepare/enable the new parents ahead of switching (so
>>> that the expected roots are enabled) and afterwards it will call
>>> clk_set_parent to ensure the parents in clock framework are up-to-date.
>>>
>>> The driver relies on dram_pll dram_alt and dram_apb being marked with
>>> CLK_GET_RATE_NOCACHE for rate updates.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>   drivers/devfreq/Kconfig      |  10 +
>>>   drivers/devfreq/Makefile     |   1 +
>>>   drivers/devfreq/imx8m-ddrc.c | 416 +++++++++++++++++++++++++++++++++++
>>>   3 files changed, 427 insertions(+)
>>>   create mode 100644 drivers/devfreq/imx8m-ddrc.c
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index defe1d438710..c519fd27808f 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -90,10 +90,20 @@ 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_IMX8M_DDRC_DEVFREQ
>>> +	tristate "i.MX8M DDRC DEVFREQ Driver"
>>> +	depends on ARCH_MXC || COMPILE_TEST
>>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>>> +	select DEVFREQ_GOV_USERSPACE
>>> +	select PM_OPP
>>
>> It doesn't need to add. CONFIG_DEVFREQ add 'select PM_OPP'
>> because 'PM_OPP' is mandatory for devfreq.
> 
> OK.
> 
> All other drivers select PM_OPP, this was just copied
> 
>>> +	help
>>> +	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
>>> +	  adjusting DRAM frequency.
>>> +
>>>   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..3eb4d5e6635c 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-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/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
>>> new file mode 100644
>>> index 000000000000..51903fee21a7
>>> --- /dev/null
>>> +++ b/drivers/devfreq/imx8m-ddrc.c
>>> @@ -0,0 +1,416 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright 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 imx8m_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 IMX8M_DDRC_MAX_FREQ_COUNT 4
>>> +
>>> +/*
>>> + * i.MX8M 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 imx8m_ddrc {
>>> +	struct devfreq_dev_profile profile;
>>> +	struct devfreq *devfreq;
>>> +
>>> +	/* For frequency switching: */
>>> +	struct clk *dram_core;
>>> +	struct clk *dram_pll;
>>> +	struct clk *dram_alt;
>>> +	struct clk *dram_apb;
>>> +
>>> +	int freq_count;
>>> +	struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT];
>>> +};
>>> +
>>> +static struct imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>>> +						    unsigned long rate)
>>> +{
>>> +	struct imx8m_ddrc_freq *freq;
>>> +	int i;
>>> +
>>> +	/*
>>> +	 * Firmware reports values in MT/s, so we round-down from Hz
>>> +	 * Rounding is extra generous to ensure a match.
>>> +	 */
>>> +	rate = DIV_ROUND_CLOSEST(rate, 250000);
>>> +	for (i = 0; i < priv->freq_count; ++i) {
>>> +		freq = &priv->freq_table[i];
>>> +		if (freq->rate == rate ||
>>> +				freq->rate + 1 == rate ||
>>> +				freq->rate - 1 == rate)
>>> +			return freq;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static void imx8m_ddrc_smc_set_freq(int target_freq)
>>> +{
>>> +	struct arm_smccc_res res;
>>> +	u32 online_cpus = 0;
>>> +	int cpu;
>>> +
>>> +	local_irq_disable();
>>
>> local_irq_disable is more proper than local_irq_save()?
> 
> There's no need to use save/restore because we know that irqs are 
> enabled when entering the function. This is only called from devfreq 
> set_target which runs in process context.

local_irq might affect the whole architecture and other running device driver
which requires the some CPU irq at the same time. Don't need to save the irqflag?

> 
>>
>>> +
>>> +	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();
>>
>> ditto. local_irq_restore() instead of 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 imx8m_ddrc_set_freq(struct device *dev, struct imx8m_ddrc_freq *freq)
>>> +{
>>> +	struct imx8m_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(
>>  > You can use 'clk_hw_get_parent_by_index' directly.
> 
> The helper above avoids duplicating ?:

I think that it is not duplicate. You use 'clk_hw_get_parent_by_index' function
and check the return value. it is essential behavior.

> 
>> And, you need to check whether the return value is NULL or not.
> Yes for dram_core_parent but others are deliberately allowed to be NULL. 
> For some setpoints some parents might be indifferent or require no 
> changes. For example when dram_core has pll as parent the expectation is 
> that dram_all will be OFF so parent is irrelevant.

If all case is not same as you commented, you just add the proper dev_warn
or dev_info message according to the h/w characteristic. Because user 
cannot know the detailed reason why don't check the return value.


> 
> Driver relies on the fact that clock API ignores prepare/enable for NULL 
> clocks. I can add a comment explaining that.
> 
>>> +			priv->dram_core, freq->dram_core_parent_index - 1);
>>> +	new_dram_alt_parent = clk_get_parent_by_index(
>>
>> ditto.
>>
>>> +			priv->dram_alt, freq->dram_alt_parent_index - 1);
>>> +	new_dram_apb_parent = clk_get_parent_by_index(
>>
>> ditto.
> 
>>> +			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;
>>
>> I think that 'dis' is not general expression for 'disable'.
>> Just, I think that 'out_disable_core' is better than 'out_dis_core'.
> 
> OK
> 
>>> +	}
>>> +	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;
>>
>> ditto.
>>
>>> +	}
>>> +
>>> +	imx8m_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 set dram_core parent: %d\n", ret);
>>
>> if you don't return directly, you better to use 'dev_warn' instead of 'dev_err'.
> 
> OK.
> 
>>> +	if (new_dram_alt_parent) {
>>> +		ret = clk_set_parent(priv->dram_alt, new_dram_alt_parent);
>>> +		if (ret)
>>> +			dev_err(dev, "failed set dram_alt parent: %d\n", ret);
>>
>> ditto. Use dev_warn.
>>   
>>> +	}
>>> +	if (new_dram_apb_parent) {
>>> +		ret = clk_set_parent(priv->dram_apb, new_dram_apb_parent);
>>> +		if (ret)
>>> +			dev_err(dev, "failed set dram_apb parent: %d\n", ret);
>>
>> ditto. Use dev_warn.
>>
>>> +	}
>>> +
>>> +	/*
>>> +	 * Explicitly refresh dram PLL rate.
>>> +	 *
>>> +	 * Even if it's marked with CLK_GET_RATE_NOCACHE the rate will not be
>>
>> nitpick:
>> What is more proper description either or 'Even if' or 'if' ?
> 
> Rate updates work for dram_alt/apb but not for dram_pll because 
> additional clocks might be present between the PLL and dram_core mux. 
> This happens *even if* pll is marked with CLK_GET_RATE_NOCACHE.

ok.

> 
>>> +	 * automatically refreshed when clk_get_rate is called on children.
>>> +	 */
>>> +	clk_get_rate(priv->dram_pll);
>>> +
>>> +	/*
>>> +	 * 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 imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
>>> +{
>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>> +	struct imx8m_ddrc_freq *freq_info;
>>> +	struct dev_pm_opp *new_opp;
>>> +	unsigned long old_freq, 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;
>>> +	}
>>> +	dev_pm_opp_put(new_opp);
>>> +
>>> +	old_freq = clk_get_rate(priv->dram_core);
>>> +	if (*freq == old_freq)
>>> +		return 0;
>>> +
>>> +	freq_info = imx8m_ddrc_find_freq(priv, *freq);
>>> +	if (!freq_info)
>>> +		return -EINVAL;
>>> +	ret = imx8m_ddrc_set_freq(dev, freq_info);
>>
>> Need to check whether 'ret' is valid or not.
>>
>>> +
>>> +	/* Also read back the clk rate to verify switch was correct */
>>> +	new_freq = clk_get_rate(priv->dram_core);
>>> +	if (ret || *freq != new_freq)
>>
>> You should check 'ret' right after imx8m_ddrc_set_freq()
>> instead of this position.
> 
> OK, I can add two error paths.
> 
>>> +		dev_err(dev, "ddrc failed freq set to %lu from %lu, now at %lu\n",
>>> +			old_freq, *freq, new_freq);
>>> +	else
>>> +		dev_dbg(dev, "ddrc freq set to %lu (was %lu)\n",
>>> +			*freq, old_freq);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>>> +{
>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>> +
>>> +	*freq = clk_get_rate(priv->dram_core);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int imx8m_ddrc_init_freq_info(struct device *dev)
>>> +{
>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>> +	struct arm_smccc_res res;
>>> +	int index;
>>> +
>>> +	/*
>>> +	 * An error here means DDR DVFS API not supported by firmware
>>> +	 */
>>
>> Don't need to add multiple line comments. It is possible to be change as following:
>>
>> 	/* An error here means DDR DVFS API not supported by firmware */
> 
> OK.
>>> +	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 > IMX8M_DDRC_MAX_FREQ_COUNT)
>>> +		return -ENODEV;
>>> +
>>> +	for (index = 0; index < priv->freq_count; ++index) {
>>> +		struct imx8m_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 -ENODEV;
>>> +
>>> +		freq->rate = res.a0;
>>> +		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 -ENODEV;
>>> +		/* dram_apb and dram_alt have exactly 8 possible parents */
>>> +		if (freq->dram_alt_parent_index > 8 ||
>>> +				freq->dram_apb_parent_index > 8)
>>> +			return -ENODEV;
>>> +		/* dram_core from alt requires explicit dram_alt parent */
>>> +		if (freq->dram_core_parent_index == 2 &&
>>> +				freq->dram_alt_parent_index == 0)
>>> +			return -ENODEV;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/* imx8m_ddrc_check_opps() - disable OPPs not supported by firmware */
>>
>> nitpick:
>> On this driver, there are no some description for internal function.
>> In order to keep the coding style, you better to move this comment
>> into function before 'freq = ULONG_MAX'.
> 
> Is it a problem if only some internal functions have documentation? I 
> can add docs to more functions.

It is not problem. It is just preferable way to add the comment.
Usually, if the function is public and exported, I added the detailed
function comment as following example of devfreq_add_device().

/**                                                                             
 656  * devfreq_add_device() - Add devfreq feature to the device                     
 657  * @dev:        the device to add devfreq feature.                              
 658  * @profile:    device-specific profile to run devfreq.                         
 659  * @governor_name:      name of the policy to choose frequency.                 
 660  * @data:       private data for the governor. The devfreq framework does not   
 661  *              touch this value.                                               
 662  */                                                                             
 663 struct devfreq *devfreq_add_device(struct device *dev,                          
 664                                    struct devfreq_dev_profile *profile,         
 665                                    const char *governor_name,                   
 666                                    void *data)              


Otherwise, if function is used in the only local with static keyword,
usually I don't add the above detailed description. Instead,
add the detailed comment into the function.

It is not the standard way. Just preferable way for the function description
if possible.



> 
> BTW: how can I generate devfreq documentation from these comments?
> 
>>> +static int imx8m_ddrc_check_opps(struct device *dev)
>>> +{
>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>> +	struct imx8m_ddrc_freq *freq_info;
>>> +	struct dev_pm_opp *opp;
>>> +	unsigned long freq;
>>> +
>>> +	freq = ULONG_MAX;
>>> +	while (true) {
>>> +		opp = dev_pm_opp_find_freq_floor(dev, &freq);
>>> +		if (opp == ERR_PTR(-ERANGE))
>>> +			break;
>>> +		if (IS_ERR(opp)) {
>>> +			dev_err(dev, "Failed enumerating OPPs: %ld\n",
>>> +				PTR_ERR(opp));
>>> +			return PTR_ERR(opp);
>>> +		}
>>> +		dev_pm_opp_put(opp);
>>> +
>>> +		freq_info = imx8m_ddrc_find_freq(priv, freq);
>>> +		if (!freq_info) {
>>> +			dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",
>>> +					freq, DIV_ROUND_CLOSEST(freq, 250000));
>>> +			dev_pm_opp_disable(dev, freq);
>>> +		}
>>> +
>>> +		freq--;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void imx8m_ddrc_exit(struct device *dev)
>>> +{
>>> +	dev_pm_opp_of_remove_table(dev);
>>> +}
>>> +
>>> +static int imx8m_ddrc_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct imx8m_ddrc *priv;
>>> +	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 = imx8m_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, "core");
>>> +	priv->dram_pll = devm_clk_get(dev, "pll");
>>> +	priv->dram_alt = devm_clk_get(dev, "alt");
>>> +	priv->dram_apb = devm_clk_get(dev, "apb");
>>> +	if (IS_ERR(priv->dram_core) ||
>>> +		IS_ERR(priv->dram_pll) ||
>>> +		IS_ERR(priv->dram_alt) ||
>>> +		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;
>>> +	}
>>> +
>>> +	ret = imx8m_ddrc_check_opps(dev);
>>> +	if (ret < 0)
>>> +		goto err;
>>> +
>>> +	priv->profile.polling_ms = 1000;
>>> +	priv->profile.target = imx8m_ddrc_target;
>>> +	priv->profile.exit = imx8m_ddrc_exit;
>>> +	priv->profile.get_cur_freq = imx8m_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 imx8m_ddrc_of_match[] = {
>>> +	{ .compatible = "fsl,imx8m-ddrc", },
>>> +	{ /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match);
>>> +
>>> +static struct platform_driver imx8m_ddrc_platdrv = {
>>> +	.probe		= imx8m_ddrc_probe,
>>> +	.driver = {
>>> +		.name	= "imx8m-ddrc-devfreq",
>>> +		.of_match_table = of_match_ptr(imx8m_ddrc_of_match),
>>> +	},
>>> +};
>>> +module_platform_driver(imx8m_ddrc_platdrv);
>>> +
>>> +MODULE_DESCRIPTION("i.MX8M DDR Controller frequency driver");
>>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
>>> +MODULE_LICENSE("GPL v2");
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Leonard Crestez]

On 12.11.2019 02:54, Chanwoo Choi wrote:
> On 11/12/19 2:23 AM, Leonard Crestez wrote:
>> On 11.11.2019 05:18, Chanwoo Choi wrote:
>>> Hi Leonard,
>>>
>>> On 11/9/19 7:39 AM, Leonard Crestez wrote:
>>>> Add driver for dynamic scaling the DDR Controller on imx8m chips. Actual
>>>> frequency switching is implemented inside TF-A, this driver wraps the
>>>> SMC calls and synchronizes the clk tree.
>>>>
>>>> The DRAM clocks on imx8m 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. The dram_alt and dram_apb clocks are "imx composite" and their
>>>> parent can also be modified.
>>>>
>>>> This driver will prepare/enable the new parents ahead of switching (so
>>>> that the expected roots are enabled) and afterwards it will call
>>>> clk_set_parent to ensure the parents in clock framework are up-to-date.
>>>>
>>>> The driver relies on dram_pll dram_alt and dram_apb being marked with
>>>> CLK_GET_RATE_NOCACHE for rate updates.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    drivers/devfreq/Kconfig      |  10 +
>>>>    drivers/devfreq/Makefile     |   1 +
>>>>    drivers/devfreq/imx8m-ddrc.c | 416 +++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 427 insertions(+)
>>>>    create mode 100644 drivers/devfreq/imx8m-ddrc.c
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index defe1d438710..c519fd27808f 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -90,10 +90,20 @@ 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_IMX8M_DDRC_DEVFREQ
>>>> +	tristate "i.MX8M DDRC DEVFREQ Driver"
>>>> +	depends on ARCH_MXC || COMPILE_TEST
>>>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>>>> +	select DEVFREQ_GOV_USERSPACE
>>>> +	select PM_OPP
>>>
>>> It doesn't need to add. CONFIG_DEVFREQ add 'select PM_OPP'
>>> because 'PM_OPP' is mandatory for devfreq.
>>
>> OK.
>>
>> All other drivers select PM_OPP, this was just copied
>>
>>>> +	help
>>>> +	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
>>>> +	  adjusting DRAM frequency.
>>>> +
>>>>    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..3eb4d5e6635c 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-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/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
>>>> new file mode 100644
>>>> index 000000000000..51903fee21a7
>>>> --- /dev/null
>>>> +++ b/drivers/devfreq/imx8m-ddrc.c
>>>> @@ -0,0 +1,416 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright 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 imx8m_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 IMX8M_DDRC_MAX_FREQ_COUNT 4
>>>> +
>>>> +/*
>>>> + * i.MX8M 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 imx8m_ddrc {
>>>> +	struct devfreq_dev_profile profile;
>>>> +	struct devfreq *devfreq;
>>>> +
>>>> +	/* For frequency switching: */
>>>> +	struct clk *dram_core;
>>>> +	struct clk *dram_pll;
>>>> +	struct clk *dram_alt;
>>>> +	struct clk *dram_apb;
>>>> +
>>>> +	int freq_count;
>>>> +	struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT];
>>>> +};
>>>> +
>>>> +static struct imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>>>> +						    unsigned long rate)
>>>> +{
>>>> +	struct imx8m_ddrc_freq *freq;
>>>> +	int i;
>>>> +
>>>> +	/*
>>>> +	 * Firmware reports values in MT/s, so we round-down from Hz
>>>> +	 * Rounding is extra generous to ensure a match.
>>>> +	 */
>>>> +	rate = DIV_ROUND_CLOSEST(rate, 250000);
>>>> +	for (i = 0; i < priv->freq_count; ++i) {
>>>> +		freq = &priv->freq_table[i];
>>>> +		if (freq->rate == rate ||
>>>> +				freq->rate + 1 == rate ||
>>>> +				freq->rate - 1 == rate)
>>>> +			return freq;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static void imx8m_ddrc_smc_set_freq(int target_freq)
>>>> +{
>>>> +	struct arm_smccc_res res;
>>>> +	u32 online_cpus = 0;
>>>> +	int cpu;
>>>> +
>>>> +	local_irq_disable();
>>>
>>> local_irq_disable is more proper than local_irq_save()?
>>
>> There's no need to use save/restore because we know that irqs are
>> enabled when entering the function. This is only called from devfreq
>> set_target which runs in process context.
> 
> local_irq might affect the whole architecture and other running device driver
> which requires the some CPU irq at the same time. Don't need to save the irqflag?

As far as I know IRQ disabling implicitly disables all forms of 
preemption, no other code can run until irqs are restored.

The difference between disable/enable and save/restore is that the 
latter also works when function is called with irqs already disabled but 
that is not required here. We know that devfreq set_target is called in 
process context with irqs enabled, otherwise it would be impossible to 
use the OPP framework (it uses mutexes internally) or touch regulators 
(they frequently sleep waiting on i2c transactions).

>>>> +	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();
>>>
>>> ditto. local_irq_restore() instead of 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 imx8m_ddrc_set_freq(struct device *dev, struct imx8m_ddrc_freq *freq)
>>>> +{
>>>> +	struct imx8m_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(
>>>   > You can use 'clk_hw_get_parent_by_index' directly.
>>
>> The helper above avoids duplicating ?:
> 
> I think that it is not duplicate. You use 'clk_hw_get_parent_by_index' function
> and check the return value. it is essential behavior.

The clk_hw_get_parent_by_index function returns a clk_hw and I'm using 
the inner hw->clk. Without clk_get_parent_by_index I'd have to write:

     hw = clk_hw_get_parent_by_index
     if (!hw || !hw->clk) {
         dev_err(...);
         return
     }
     clk = hw->clk;

This seems pointlessly verbose.

>>> And, you need to check whether the return value is NULL or not.

>> Yes for dram_core_parent but others are deliberately allowed to be NULL.
>> For some setpoints some parents might be indifferent or require no
>> changes. For example when dram_core has pll as parent the expectation is
>> that dram_all will be OFF so parent is irrelevant.
> 
> If all case is not same as you commented, you just add the proper dev_warn
> or dev_info message according to the h/w characteristic. Because user
> cannot know the detailed reason why don't check the return value.

Really it's only freq->dram_alt/apb_parent_index which can be zero, if 
that has a valid value then clk fetching shouldn't fail for any other 
reason. Current code could indeed hide some errors.
>> Driver relies on the fact that clock API ignores prepare/enable for NULL
>> clocks. I can add a comment explaining that.
>>
>>>> +			priv->dram_core, freq->dram_core_parent_index - 1);
>>>> +	new_dram_alt_parent = clk_get_parent_by_index(
>>>
>>> ditto.
>>>
>>>> +			priv->dram_alt, freq->dram_alt_parent_index - 1);
>>>> +	new_dram_apb_parent = clk_get_parent_by_index(
>>>
>>> ditto.
>>
>>>> +			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;
>>>
>>> I think that 'dis' is not general expression for 'disable'.
>>> Just, I think that 'out_disable_core' is better than 'out_dis_core'.
>>
>> OK
>>
>>>> +	}
>>>> +	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;
>>>
>>> ditto.
>>>
>>>> +	}
>>>> +
>>>> +	imx8m_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 set dram_core parent: %d\n", ret);
>>>
>>> if you don't return directly, you better to use 'dev_warn' instead of 'dev_err'.
>>
>> OK.
>>
>>>> +	if (new_dram_alt_parent) {
>>>> +		ret = clk_set_parent(priv->dram_alt, new_dram_alt_parent);
>>>> +		if (ret)
>>>> +			dev_err(dev, "failed set dram_alt parent: %d\n", ret);
>>>
>>> ditto. Use dev_warn.
>>>    
>>>> +	}
>>>> +	if (new_dram_apb_parent) {
>>>> +		ret = clk_set_parent(priv->dram_apb, new_dram_apb_parent);
>>>> +		if (ret)
>>>> +			dev_err(dev, "failed set dram_apb parent: %d\n", ret);
>>>
>>> ditto. Use dev_warn.
>>>
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Explicitly refresh dram PLL rate.
>>>> +	 *
>>>> +	 * Even if it's marked with CLK_GET_RATE_NOCACHE the rate will not be
>>>
>>> nitpick:
>>> What is more proper description either or 'Even if' or 'if' ?
>>
>> Rate updates work for dram_alt/apb but not for dram_pll because
>> additional clocks might be present between the PLL and dram_core mux.
>> This happens *even if* pll is marked with CLK_GET_RATE_NOCACHE.
> 
> ok.
> 
>>
>>>> +	 * automatically refreshed when clk_get_rate is called on children.
>>>> +	 */
>>>> +	clk_get_rate(priv->dram_pll);
>>>> +
>>>> +	/*
>>>> +	 * 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 imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
>>>> +{
>>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> +	struct imx8m_ddrc_freq *freq_info;
>>>> +	struct dev_pm_opp *new_opp;
>>>> +	unsigned long old_freq, 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;
>>>> +	}
>>>> +	dev_pm_opp_put(new_opp);
>>>> +
>>>> +	old_freq = clk_get_rate(priv->dram_core);
>>>> +	if (*freq == old_freq)
>>>> +		return 0;
>>>> +
>>>> +	freq_info = imx8m_ddrc_find_freq(priv, *freq);
>>>> +	if (!freq_info)
>>>> +		return -EINVAL;
>>>> +	ret = imx8m_ddrc_set_freq(dev, freq_info);
>>>
>>> Need to check whether 'ret' is valid or not.
>>>
>>>> +
>>>> +	/* Also read back the clk rate to verify switch was correct */
>>>> +	new_freq = clk_get_rate(priv->dram_core);
>>>> +	if (ret || *freq != new_freq)
>>>
>>> You should check 'ret' right after imx8m_ddrc_set_freq()
>>> instead of this position.
>>
>> OK, I can add two error paths.
>>
>>>> +		dev_err(dev, "ddrc failed freq set to %lu from %lu, now at %lu\n",
>>>> +			old_freq, *freq, new_freq);
>>>> +	else
>>>> +		dev_dbg(dev, "ddrc freq set to %lu (was %lu)\n",
>>>> +			*freq, old_freq);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>>>> +{
>>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> +
>>>> +	*freq = clk_get_rate(priv->dram_core);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int imx8m_ddrc_init_freq_info(struct device *dev)
>>>> +{
>>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> +	struct arm_smccc_res res;
>>>> +	int index;
>>>> +
>>>> +	/*
>>>> +	 * An error here means DDR DVFS API not supported by firmware
>>>> +	 */
>>>
>>> Don't need to add multiple line comments. It is possible to be change as following:
>>>
>>> 	/* An error here means DDR DVFS API not supported by firmware */
>>
>> OK.
>>>> +	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 > IMX8M_DDRC_MAX_FREQ_COUNT)
>>>> +		return -ENODEV;
>>>> +
>>>> +	for (index = 0; index < priv->freq_count; ++index) {
>>>> +		struct imx8m_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 -ENODEV;
>>>> +
>>>> +		freq->rate = res.a0;
>>>> +		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 -ENODEV;
>>>> +		/* dram_apb and dram_alt have exactly 8 possible parents */
>>>> +		if (freq->dram_alt_parent_index > 8 ||
>>>> +				freq->dram_apb_parent_index > 8)
>>>> +			return -ENODEV;
>>>> +		/* dram_core from alt requires explicit dram_alt parent */
>>>> +		if (freq->dram_core_parent_index == 2 &&
>>>> +				freq->dram_alt_parent_index == 0)
>>>> +			return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/* imx8m_ddrc_check_opps() - disable OPPs not supported by firmware */
>>>
>>> nitpick:
>>> On this driver, there are no some description for internal function.
>>> In order to keep the coding style, you better to move this comment
>>> into function before 'freq = ULONG_MAX'.
>>
>> Is it a problem if only some internal functions have documentation? I
>> can add docs to more functions.
> 
> It is not problem. It is just preferable way to add the comment.
> Usually, if the function is public and exported, I added the detailed
> function comment as following example of devfreq_add_device().
> 
> /**
>   656  * devfreq_add_device() - Add devfreq feature to the device
>   657  * @dev:        the device to add devfreq feature.
>   658  * @profile:    device-specific profile to run devfreq.
>   659  * @governor_name:      name of the policy to choose frequency.
>   660  * @data:       private data for the governor. The devfreq framework does not
>   661  *              touch this value.
>   662  */
>   663 struct devfreq *devfreq_add_device(struct device *dev,
>   664                                    struct devfreq_dev_profile *profile,
>   665                                    const char *governor_name,
>   666                                    void *data)
> 
> 
> Otherwise, if function is used in the only local with static keyword,
> usually I don't add the above detailed description. Instead,
> add the detailed comment into the function.
> 
> It is not the standard way. Just preferable way for the function description
> if possible.
> 
> 
> 
>>
>> BTW: how can I generate devfreq documentation from these comments?
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/6] PM / devfreq: imx8m-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/imx8m-ddrc.c | 153 +++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index 51903fee21a7..6372191f72d7 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-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
 
@@ -78,10 +81,22 @@ struct imx8m_ddrc {
 	struct clk *dram_alt;
 	struct clk *dram_apb;
 
 	int freq_count;
 	struct imx8m_ddrc_freq freq_table[IMX8M_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 imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
 						    unsigned long rate)
 {
@@ -245,10 +260,131 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
 	*freq = clk_get_rate(priv->dram_core);
 
 	return 0;
 }
 
+static int imx8m_ddrc_get_dev_status(struct device *dev,
+				     struct devfreq_dev_status *stat)
+{
+	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
+
+	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);
+		do_div(rd_delta, 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);
+		do_div(wr_delta, 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 imx8m_ddrc_perf_disable(struct imx8m_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 imx8m_ddrc_perf_enable(struct imx8m_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:
+	imx8m_ddrc_perf_disable(priv);
+	return ret;
+}
+
+static int imx8m_ddrc_init_events(struct device *dev,
+				  struct device_node *events_node)
+{
+	struct imx8m_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) {
+		dev_err(dev, "devfreq-events device missing private data\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "events from pmu %s\n", priv->pmu->name);
+
+	return imx8m_ddrc_perf_enable(priv);
+}
+
 static int imx8m_ddrc_init_freq_info(struct device *dev)
 {
 	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
 	struct arm_smccc_res res;
 	int index;
@@ -328,17 +464,23 @@ static int imx8m_ddrc_check_opps(struct device *dev)
 	return 0;
 }
 
 static void imx8m_ddrc_exit(struct device *dev)
 {
+	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
+
+	imx8m_ddrc_perf_disable(priv);
+	platform_device_put(priv->pmu_pdev);
+
 	dev_pm_opp_of_remove_table(dev);
 }
 
 static int imx8m_ddrc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct imx8m_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)
@@ -350,10 +492,19 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "failed to init firmware freq info: %d\n", ret);
 		return ret;
 	}
 
+	events_node = of_parse_phandle(dev->of_node, "devfreq-events", 0);
+	if (events_node) {
+		ret = imx8m_ddrc_init_events(dev, events_node);
+		of_node_put(events_node);
+		if (ret)
+			goto err;
+		gov = DEVFREQ_GOV_SIMPLE_ONDEMAND;
+	}
+
 	priv->dram_core = devm_clk_get(dev, "core");
 	priv->dram_pll = devm_clk_get(dev, "pll");
 	priv->dram_alt = devm_clk_get(dev, "alt");
 	priv->dram_apb = devm_clk_get(dev, "apb");
 	if (IS_ERR(priv->dram_core) ||
@@ -390,10 +541,12 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
 	}
 
 	return 0;
 
 err:
+	imx8m_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 imx8m_ddrc_of_match[] = {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/6] PM / devfreq: imx8m-ddrc: Measure bandwidth with perf

[Chanwoo Choi]

Hi Leonard,

On 11/9/19 7:39 AM, Leonard Crestez wrote:
> 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/imx8m-ddrc.c | 153 +++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
> index 51903fee21a7..6372191f72d7 100644
> --- a/drivers/devfreq/imx8m-ddrc.c
> +++ b/drivers/devfreq/imx8m-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
>  
> @@ -78,10 +81,22 @@ struct imx8m_ddrc {
>  	struct clk *dram_alt;
>  	struct clk *dram_apb;
>  
>  	int freq_count;
>  	struct imx8m_ddrc_freq freq_table[IMX8M_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 imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>  						    unsigned long rate)
>  {
> @@ -245,10 +260,131 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>  	*freq = clk_get_rate(priv->dram_core);
>  
>  	return 0;
>  }
>  
> +static int imx8m_ddrc_get_dev_status(struct device *dev,
> +				     struct devfreq_dev_status *stat)
> +{
> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> +
> +	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);
> +		do_div(rd_delta, 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);
> +		do_div(wr_delta, 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 imx8m_ddrc_perf_disable(struct imx8m_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 imx8m_ddrc_perf_enable(struct imx8m_ddrc *priv)

Actually, this function have to call the just function for enabling
the bandwidth monitoring instead of writing the detailed something.
It looks like that you move the part of the different device driver into here.

> +{
> +	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:
> +	imx8m_ddrc_perf_disable(priv);
> +	return ret;
> +}
> +
> +static int imx8m_ddrc_init_events(struct device *dev,
> +				  struct device_node *events_node)

ditto.

> +{
> +	struct imx8m_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);

It seems that you access the different device driver without
any standard interface from some linux kernel subsystem.

For the communication or control between different device drivers,
we have to use the standard interface instead of direct access or call.
I think that it make it too tightly coupled driver between them.

So, I developed the devfreq-event subsystem for this reason
in order to remove the non-standard direct access to other device driver.

Unfortunately, I can't agree this approach. I don't prefer to use
the direct access of other device driver(imx-ddr-pmu). You need to
use standard interface provided from subsystem. or You need to make
the new device driver with devfreq-event subsystem.



> +	if (!priv->pmu) {
> +		dev_err(dev, "devfreq-events device missing private data\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "events from pmu %s\n", priv->pmu->name);
> +
> +	return imx8m_ddrc_perf_enable(priv);
> +}
> +
>  static int imx8m_ddrc_init_freq_info(struct device *dev)
>  {
>  	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>  	struct arm_smccc_res res;
>  	int index;
> @@ -328,17 +464,23 @@ static int imx8m_ddrc_check_opps(struct device *dev)
>  	return 0;
>  }
>  
>  static void imx8m_ddrc_exit(struct device *dev)
>  {
> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> +
> +	imx8m_ddrc_perf_disable(priv);
> +	platform_device_put(priv->pmu_pdev);
> +
>  	dev_pm_opp_of_remove_table(dev);
>  }
>  
>  static int imx8m_ddrc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct imx8m_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)
> @@ -350,10 +492,19 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(dev, "failed to init firmware freq info: %d\n", ret);
>  		return ret;
>  	}
>  
> +	events_node = of_parse_phandle(dev->of_node, "devfreq-events", 0);
> +	if (events_node) {
> +		ret = imx8m_ddrc_init_events(dev, events_node);
> +		of_node_put(events_node);
> +		if (ret)
> +			goto err;
> +		gov = DEVFREQ_GOV_SIMPLE_ONDEMAND;
> +	}
> +
>  	priv->dram_core = devm_clk_get(dev, "core");
>  	priv->dram_pll = devm_clk_get(dev, "pll");
>  	priv->dram_alt = devm_clk_get(dev, "alt");
>  	priv->dram_apb = devm_clk_get(dev, "apb");
>  	if (IS_ERR(priv->dram_core) ||
> @@ -390,10 +541,12 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>  	}
>  
>  	return 0;
>  
>  err:
> +	imx8m_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 imx8m_ddrc_of_match[] = {
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/6] PM / devfreq: imx8m-ddrc: Measure bandwidth with perf

[Leonard Crestez]

On 11.11.2019 07:13, Chanwoo Choi wrote:
> Hi Leonard,
> 
> On 11/9/19 7:39 AM, Leonard Crestez wrote:
>> 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/imx8m-ddrc.c | 153 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 153 insertions(+)
>>
>> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
>> index 51903fee21a7..6372191f72d7 100644
>> --- a/drivers/devfreq/imx8m-ddrc.c
>> +++ b/drivers/devfreq/imx8m-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
>>   
>> @@ -78,10 +81,22 @@ struct imx8m_ddrc {
>>   	struct clk *dram_alt;
>>   	struct clk *dram_apb;
>>   
>>   	int freq_count;
>>   	struct imx8m_ddrc_freq freq_table[IMX8M_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 imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>>   						    unsigned long rate)
>>   {
>> @@ -245,10 +260,131 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>>   	*freq = clk_get_rate(priv->dram_core);
>>   
>>   	return 0;
>>   }
>>   
>> +static int imx8m_ddrc_get_dev_status(struct device *dev,
>> +				     struct devfreq_dev_status *stat)
>> +{
>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>> +
>> +	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);
>> +		do_div(rd_delta, 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);
>> +		do_div(wr_delta, 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 imx8m_ddrc_perf_disable(struct imx8m_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 imx8m_ddrc_perf_enable(struct imx8m_ddrc *priv)
> 
> Actually, this function have to call the just function for enabling
> the bandwidth monitoring instead of writing the detailed something.
> It looks like that you move the part of the different device driver into here.

This is the code for enabling bandwith monitoring: it creates perf 
counters using in-kernel API. The perf api doesn't seem to expose 
anything else to enable/disable the counter after creation and honestly 
just create/destroy seems fine.

As far as I can tell bandwidth monitoring in devfreq is always enabled 
on probe anyway, no matter which governor is in use. It would be nice if 
devfreq drivers could receive a callback and dynamically acquire/release 
monitoring resources only when the ondemand governor is in used.

Until then this driver will permanently allocate 2 (out of 3) counters 
in ddr pmu hardware.

>> +{
>> +	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:
>> +	imx8m_ddrc_perf_disable(priv);
>> +	return ret;
>> +}
>> +
>> +static int imx8m_ddrc_init_events(struct device *dev,
>> +				  struct device_node *events_node)
> 
> ditto.
> 
>> +{
>> +	struct imx8m_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);
> 
> It seems that you access the different device driver without
> any standard interface from some linux kernel subsystem.
> 
> For the communication or control between different device drivers,
> we have to use the standard interface instead of direct access or call.
> I think that it make it too tightly coupled driver between them.
> 
> So, I developed the devfreq-event subsystem for this reason
> in order to remove the non-standard direct access to other device driver.
> 
> Unfortunately, I can't agree this approach. I don't prefer to use
> the direct access of other device driver(imx-ddr-pmu). You need to
> use standard interface provided from subsystem. or You need to make
> the new device driver with devfreq-event subsystem.

This could be cleaned-up by adding a new API to "fetch struct pmu* by 
struct device_node*" as available for many other subsystems. The perf 
api is otherwise standard/generic and has a few other in-kernel users.

The perf driver for DDR PMU is already functional and useful for 
profiling and reusing it seem very worthwhile. If you're suggesting I 
implemented an unrelated "devfreq-event" driver then how would it get 
probed? There's only one PMU node in DT.

I wouldn't mind to delay the monitoring part into a second series.

>> +	if (!priv->pmu) {
>> +		dev_err(dev, "devfreq-events device missing private data\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_dbg(dev, "events from pmu %s\n", priv->pmu->name);
>> +
>> +	return imx8m_ddrc_perf_enable(priv);
>> +}
>> +
>>   static int imx8m_ddrc_init_freq_info(struct device *dev)
>>   {
>>   	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>   	struct arm_smccc_res res;
>>   	int index;
>> @@ -328,17 +464,23 @@ static int imx8m_ddrc_check_opps(struct device *dev)
>>   	return 0;
>>   }
>>   
>>   static void imx8m_ddrc_exit(struct device *dev)
>>   {
>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>> +
>> +	imx8m_ddrc_perf_disable(priv);
>> +	platform_device_put(priv->pmu_pdev);
>> +
>>   	dev_pm_opp_of_remove_table(dev);
>>   }
>>   
>>   static int imx8m_ddrc_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>>   	struct imx8m_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)
>> @@ -350,10 +492,19 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>>   	if (ret) {
>>   		dev_err(dev, "failed to init firmware freq info: %d\n", ret);
>>   		return ret;
>>   	}
>>   
>> +	events_node = of_parse_phandle(dev->of_node, "devfreq-events", 0);
>> +	if (events_node) {
>> +		ret = imx8m_ddrc_init_events(dev, events_node);
>> +		of_node_put(events_node);
>> +		if (ret)
>> +			goto err;
>> +		gov = DEVFREQ_GOV_SIMPLE_ONDEMAND;
>> +	}
>> +
>>   	priv->dram_core = devm_clk_get(dev, "core");
>>   	priv->dram_pll = devm_clk_get(dev, "pll");
>>   	priv->dram_alt = devm_clk_get(dev, "alt");
>>   	priv->dram_apb = devm_clk_get(dev, "apb");
>>   	if (IS_ERR(priv->dram_core) ||
>> @@ -390,10 +541,12 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>>   	}
>>   
>>   	return 0;
>>   
>>   err:
>> +	imx8m_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 imx8m_ddrc_of_match[] = {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/6] PM / devfreq: imx8m-ddrc: Measure bandwidth with perf

[Chanwoo Choi]

Hi Leonard,

On 11/12/19 10:17 PM, Leonard Crestez wrote:
> On 11.11.2019 07:13, Chanwoo Choi wrote:
>> Hi Leonard,
>>
>> On 11/9/19 7:39 AM, Leonard Crestez wrote:
>>> 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/imx8m-ddrc.c | 153 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 153 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
>>> index 51903fee21a7..6372191f72d7 100644
>>> --- a/drivers/devfreq/imx8m-ddrc.c
>>> +++ b/drivers/devfreq/imx8m-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
>>>   
>>> @@ -78,10 +81,22 @@ struct imx8m_ddrc {
>>>   	struct clk *dram_alt;
>>>   	struct clk *dram_apb;
>>>   
>>>   	int freq_count;
>>>   	struct imx8m_ddrc_freq freq_table[IMX8M_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 imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>>>   						    unsigned long rate)
>>>   {
>>> @@ -245,10 +260,131 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>>>   	*freq = clk_get_rate(priv->dram_core);
>>>   
>>>   	return 0;
>>>   }
>>>   
>>> +static int imx8m_ddrc_get_dev_status(struct device *dev,
>>> +				     struct devfreq_dev_status *stat)
>>> +{
>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>> +
>>> +	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);
>>> +		do_div(rd_delta, 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);
>>> +		do_div(wr_delta, 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 imx8m_ddrc_perf_disable(struct imx8m_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 imx8m_ddrc_perf_enable(struct imx8m_ddrc *priv)
>>
>> Actually, this function have to call the just function for enabling
>> the bandwidth monitoring instead of writing the detailed something.
>> It looks like that you move the part of the different device driver into here.
> 
> This is the code for enabling bandwith monitoring: it creates perf 
> counters using in-kernel API. The perf api doesn't seem to expose 
> anything else to enable/disable the counter after creation and honestly 
> just create/destroy seems fine.
> 
> As far as I can tell bandwidth monitoring in devfreq is always enabled 
> on probe anyway, no matter which governor is in use. It would be nice if 
> devfreq drivers could receive a callback and dynamically acquire/release 
> monitoring resources only when the ondemand governor is in used.

As you commented, it doesn't matter to keep the enabling state of perf.
Just, I want to say that I think that it is not proper to access
the different device driver directly without any standard subsystem interface.

> 
> Until then this driver will permanently allocate 2 (out of 3) counters 
> in ddr pmu hardware.
> 
>>> +{
>>> +	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:
>>> +	imx8m_ddrc_perf_disable(priv);
>>> +	return ret;
>>> +}
>>> +
>>> +static int imx8m_ddrc_init_events(struct device *dev,
>>> +				  struct device_node *events_node)
>>
>> ditto.
>>
>>> +{
>>> +	struct imx8m_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);
>>
>> It seems that you access the different device driver without
>> any standard interface from some linux kernel subsystem.
>>
>> For the communication or control between different device drivers,
>> we have to use the standard interface instead of direct access or call.
>> I think that it make it too tightly coupled driver between them.
>>
>> So, I developed the devfreq-event subsystem for this reason
>> in order to remove the non-standard direct access to other device driver.
>>
>> Unfortunately, I can't agree this approach. I don't prefer to use
>> the direct access of other device driver(imx-ddr-pmu). You need to
>> use standard interface provided from subsystem. or You need to make
>> the new device driver with devfreq-event subsystem.
> 
> This could be cleaned-up by adding a new API to "fetch struct pmu* by 
> struct device_node*" as available for many other subsystems.

OK. I'll expect the next version with keeping the rule without
any direct access between two device drivers.

 The perf 
> api is otherwise standard/generic and has a few other in-kernel users.

> 
> The perf driver for DDR PMU is already functional and useful for 
> profiling and reusing it seem very worthwhile. If you're suggesting I 
> implemented an unrelated "devfreq-event" driver then how would it get 
> probed? There's only one PMU node in DT.

No, I don't force to develop the new devfreq-event instead of exiting perf driver.
As I said already,I think that it is not proper to access the different device driver
directly without any standard subsystem interface.

The following style access is not safe. Any device driver might change
the platform data of the other device driver. 
	priv->pmu = platform_get_drvdata(priv->pmu_pdev);

If you support this patch with standard subsystem interface, I'm OK.
Just, I don't want to access the data of different device driver.

> 
> I wouldn't mind to delay the monitoring part into a second series.
> 
>>> +	if (!priv->pmu) {
>>> +		dev_err(dev, "devfreq-events device missing private data\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	dev_dbg(dev, "events from pmu %s\n", priv->pmu->name);
>>> +
>>> +	return imx8m_ddrc_perf_enable(priv);
>>> +}
>>> +
>>>   static int imx8m_ddrc_init_freq_info(struct device *dev)
>>>   {
>>>   	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>   	struct arm_smccc_res res;
>>>   	int index;
>>> @@ -328,17 +464,23 @@ static int imx8m_ddrc_check_opps(struct device *dev)
>>>   	return 0;
>>>   }
>>>   
>>>   static void imx8m_ddrc_exit(struct device *dev)
>>>   {
>>> +	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>> +
>>> +	imx8m_ddrc_perf_disable(priv);
>>> +	platform_device_put(priv->pmu_pdev);
>>> +
>>>   	dev_pm_opp_of_remove_table(dev);
>>>   }
>>>   
>>>   static int imx8m_ddrc_probe(struct platform_device *pdev)
>>>   {
>>>   	struct device *dev = &pdev->dev;
>>>   	struct imx8m_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)
>>> @@ -350,10 +492,19 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>>>   	if (ret) {
>>>   		dev_err(dev, "failed to init firmware freq info: %d\n", ret);
>>>   		return ret;
>>>   	}
>>>   
>>> +	events_node = of_parse_phandle(dev->of_node, "devfreq-events", 0);
>>> +	if (events_node) {
>>> +		ret = imx8m_ddrc_init_events(dev, events_node);
>>> +		of_node_put(events_node);
>>> +		if (ret)
>>> +			goto err;
>>> +		gov = DEVFREQ_GOV_SIMPLE_ONDEMAND;
>>> +	}
>>> +
>>>   	priv->dram_core = devm_clk_get(dev, "core");
>>>   	priv->dram_pll = devm_clk_get(dev, "pll");
>>>   	priv->dram_alt = devm_clk_get(dev, "alt");
>>>   	priv->dram_apb = devm_clk_get(dev, "apb");
>>>   	if (IS_ERR(priv->dram_core) ||
>>> @@ -390,10 +541,12 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>>>   	}
>>>   
>>>   	return 0;
>>>   
>>>   err:
>>> +	imx8m_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 imx8m_ddrc_of_match[] = {
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 6/6] arm64: dts: imx8m: Add ddr controller nodes

[Leonard Crestez]

This is used by the imx-ddrc devfreq driver to implement dynamic
frequency scaling of DRAM.

Add a devfreq-event link to the dram PMU in order to support on-demand
scaling of ddrc based on measured dram bandwidth usage.

Support for proactive scaling via interconnect 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-evk.dts  | 18 ++++++++++++++
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     | 13 +++++++++-
 .../boot/dts/freescale/imx8mn-ddr4-evk.dts    | 18 ++++++++++++++
 arch/arm64/boot/dts/freescale/imx8mn.dtsi     | 13 +++++++++-
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts  | 24 +++++++++++++++++++
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 13 +++++++++-
 6 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
index 28ab17a277bb..ecf0d385c164 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
@@ -75,10 +75,28 @@
 
 &A53_0 {
 	cpu-supply = <&buck2_reg>;
 };
 
+&ddrc {
+	operating-points-v2 = <&ddrc_opp_table>;
+
+	ddrc_opp_table: 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>;
+		};
+	};
+};
+
 &fec1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_fec1>;
 	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy0>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 6edbdfe2d0d7..0fffc6362c43 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -856,11 +856,22 @@
 			#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 = "core", "pll", "alt", "apb";
+			clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
+				 <&clk IMX8MM_DRAM_PLL>,
+				 <&clk IMX8MM_CLK_DRAM_ALT>,
+				 <&clk IMX8MM_CLK_DRAM_APB>;
+			devfreq-events = <&ddr_pmu>;
+		};
+
+		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>;
 		};
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
index 071949412caf..b051c927c11e 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
@@ -15,10 +15,28 @@
 
 &A53_0 {
 	cpu-supply = <&buck2_reg>;
 };
 
+&ddrc {
+	operating-points-v2 = <&ddrc_opp_table>;
+
+	ddrc_opp_table: opp-table {
+		compatible = "operating-points-v2";
+
+		opp-25M {
+			opp-hz = /bits/ 64 <25000000>;
+		};
+		opp-100M {
+			opp-hz = /bits/ 64 <100000000>;
+		};
+		opp-600M {
+			opp-hz = /bits/ 64 <600000000>;
+		};
+	};
+};
+
 &i2c1 {
 	pmic@4b {
 		compatible = "rohm,bd71847";
 		reg = <0x4b>;
 		pinctrl-0 = <&pinctrl_pmic>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index e91625063f8e..c952bfb906a7 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -757,11 +757,22 @@
 			#interrupt-cells = <3>;
 			interrupt-controller;
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-		ddr-pmu@3d800000 {
+		ddrc: dram-controller@3d400000 {
+			compatible = "fsl,imx8mn-ddrc", "fsl,imx8m-ddrc";
+			reg = <0x3d400000 0x400000>;
+			clock-names = "core", "pll", "alt", "apb";
+			clocks = <&clk IMX8MN_CLK_DRAM_CORE>,
+				 <&clk IMX8MN_DRAM_PLL>,
+				 <&clk IMX8MN_CLK_DRAM_ALT>,
+				 <&clk IMX8MN_CLK_DRAM_APB>;
+			devfreq-events = <&ddr_pmu>;
+		};
+
+		ddr_pmu: ddr-pmu@3d800000 {
 			compatible = "fsl,imx8mn-ddr-pmu", "fsl,imx8m-ddr-pmu";
 			reg = <0x3d800000 0x400000>;
 			interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
index c36685916683..ee6dc5f07622 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
@@ -103,10 +103,34 @@
 
 &A53_3 {
 	cpu-supply = <&buck2_reg>;
 };
 
+&ddrc {
+	operating-points-v2 = <&ddrc_opp_table>;
+
+	ddrc_opp_table: opp-table {
+		compatible = "operating-points-v2";
+
+		opp-25M {
+			opp-hz = /bits/ 64 <25000000>;
+		};
+		opp-100M {
+			opp-hz = /bits/ 64 <100000000>;
+		};
+		/*
+		 * On imx8mq B0 PLL can't be bypassed so low bus is 166M
+		 */
+		opp-166M {
+			opp-hz = /bits/ 64 <166935483>;
+		};
+		opp-800M {
+			opp-hz = /bits/ 64 <800000000>;
+		};
+	};
+};
+
 &fec1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_fec1>;
 	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy0>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 7f9319452b58..d2270e99098e 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -1111,11 +1111,22 @@
 			interrupt-controller;
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-parent = <&gic>;
 		};
 
-		ddr-pmu@3d800000 {
+		ddrc: dram-controller@3d400000 {
+			compatible = "fsl,imx8mq-ddrc", "fsl,imx8m-ddrc";
+			reg = <0x3d400000 0x400000>;
+			clock-names = "core", "pll", "alt", "apb";
+			clocks = <&clk IMX8MQ_CLK_DRAM_CORE>,
+				 <&clk IMX8MQ_DRAM_PLL_OUT>,
+				 <&clk IMX8MQ_CLK_DRAM_ALT>,
+				 <&clk IMX8MQ_CLK_DRAM_APB>;
+			devfreq-events = <&ddr_pmu>;
+		};
+
+		ddr_pmu: ddr-pmu@3d800000 {
 			compatible = "fsl,imx8mq-ddr-pmu", "fsl,imx8m-ddr-pmu";
 			reg = <0x3d800000 0x400000>;
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
 		};
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Leonard Crestez]

On 6/22/20 4:58 PM, Martin Kepplinger wrote:
> hi Leondard,
> 
> before using this patchset I'd like to ask: Do you have plans to create
> an update and push this forward? It is useful.

Hello.

I am no longer with NXP and don't have access to imx hardware right now.

However the series that you replied to is very old and was accepted many 
months ago. You shouldn't have to apply out-of-tree kernel patches.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Martin Kepplinger]

hi Leonard,

On 24.06.20 08:08, Leonard Crestez wrote:
> On 6/22/20 4:58 PM, Martin Kepplinger wrote:
>> hi Leondard,
>>
>> before using this patchset I'd like to ask: Do you have plans to create
>> an update and push this forward? It is useful.
> 
> Hello.
> 
> I am no longer with NXP and don't have access to imx hardware right now.

I guess it'll get even harder to get the ATF part for devfreq
implemented now :) Thanks for the update and all the best for your new
stuff.

> 
> However the series that you replied to is very old and was accepted many
> months ago. You shouldn't have to apply out-of-tree kernel patches.
> 

that particular series doesn't seem to be in mainline, see
https://elixir.bootlin.com/linux/latest/source/drivers/devfreq/imx8m-ddrc.c#L283
or do I miss something?

do you know who at nxp would be likely actively working on devfreq?

thanks,
                           martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Abel Vesa]

On 20-06-25 08:57:52, Martin Kepplinger wrote:
> hi Leonard,
> 
> On 24.06.20 08:08, Leonard Crestez wrote:
> > On 6/22/20 4:58 PM, Martin Kepplinger wrote:
> >> hi Leondard,
> >>
> >> before using this patchset I'd like to ask: Do you have plans to create
> >> an update and push this forward? It is useful.
> > 
> > Hello.
> > 
> > I am no longer with NXP and don't have access to imx hardware right now.
> 
> I guess it'll get even harder to get the ATF part for devfreq
> implemented now :) Thanks for the update and all the best for your new
> stuff.
> 
> > 
> > However the series that you replied to is very old and was accepted many
> > months ago. You shouldn't have to apply out-of-tree kernel patches.
> > 
> 
> that particular series doesn't seem to be in mainline, see
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fdevfreq%2Fimx8m-ddrc.c%23L283&data=02%7C01%7Cabel.vesa%40nxp.com%7Cb00f437e756d4850238f08d818d51b59%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637286650857331523&sdata=S7%2BN3%2BiTFkUW5YnmVzl36wEBlr%2BkTatGoDDrvY9XfTk%3D&reserved=0
> or do I miss something?
> 
> do you know who at nxp would be likely actively working on devfreq?

Hi Martin,

I will be working on this in the following weeks.

> 
> thanks,
>                            martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/6] PM / devfreq: Add dynamic scaling for imx8m ddr controller

[Martin Kepplinger]

On 25.06.20 16:47, Abel Vesa wrote:
> On 20-06-25 08:57:52, Martin Kepplinger wrote:
>> hi Leonard,
>>
>> On 24.06.20 08:08, Leonard Crestez wrote:
>>> On 6/22/20 4:58 PM, Martin Kepplinger wrote:
>>>> hi Leondard,
>>>>
>>>> before using this patchset I'd like to ask: Do you have plans to create
>>>> an update and push this forward? It is useful.
>>>
>>> Hello.
>>>
>>> I am no longer with NXP and don't have access to imx hardware right now.
>>
>> I guess it'll get even harder to get the ATF part for devfreq
>> implemented now :) Thanks for the update and all the best for your new
>> stuff.
>>
>>>
>>> However the series that you replied to is very old and was accepted many
>>> months ago. You shouldn't have to apply out-of-tree kernel patches.
>>>
>>
>> that particular series doesn't seem to be in mainline, see
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fdevfreq%2Fimx8m-ddrc.c%23L283&data=02%7C01%7Cabel.vesa%40nxp.com%7Cb00f437e756d4850238f08d818d51b59%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637286650857331523&sdata=S7%2BN3%2BiTFkUW5YnmVzl36wEBlr%2BkTatGoDDrvY9XfTk%3D&reserved=0
>> or do I miss something?
>>
>> do you know who at nxp would be likely actively working on devfreq?
> 
> Hi Martin,
> 
> I will be working on this in the following weeks.
> 

hi Abel,

that's good to hear. I'm basically always happy to test changes to
devfreq (the ondemand governor or an atf implementation).

thanks,
                        martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel