[v2,1/7] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram_alt/apb
diff mbox series

Message ID 90bfeebcb76e76d286ed7f022ea9e0d9a569ebe2.1566315740.git.leonard.crestez@nxp.com
State New
Headers show
Series
  • PM / devfreq: Add initial imx support
Related show

Commit Message

Leonard Crestez Aug. 20, 2019, 3:45 p.m. UTC
Dram frequency changes required modifying these clocks outside the
control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
rates are always read back from registers.

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

Comments

Stephen Boyd Sept. 16, 2019, 8:33 p.m. UTC | #1
Quoting Leonard Crestez (2019-08-20 08:45:06)
> Dram frequency changes required modifying these clocks outside the
> control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
> rates are always read back from registers.

Why can't we control the clks from the clk framework? Please add that
information in the commit text here.

> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/clk/imx/clk-imx8mm.c | 6 ++++--
>  drivers/clk/imx/clk-imx8mn.c | 6 ++++--
>  drivers/clk/imx/clk-imx8mq.c | 7 ++++---
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index 4ead3ea2713c..6cac80550f43 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -526,12 +526,14 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>         /* IPG */
>         clks[IMX8MM_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
>         clks[IMX8MM_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
>  
>         /* IP */
> -       clks[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
> -       clks[IMX8MM_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
> +       clks[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000,
> +                       CLK_GET_RATE_NOCACHE);
> +       clks[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080,
> +                       CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);

Also, add a comment to this effect about why it can't be done from the
clk framework wherever the CLK_GET_RATE_NOCACHE flag is set. Basically
this flag is a hack and is an example of something that we need to fix.

>         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);
Leonard Crestez Sept. 16, 2019, 11:03 p.m. UTC | #2
On 2019-09-16 11:33 PM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-08-20 08:45:06)
>> Dram frequency changes required modifying these clocks outside the
>> control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
>> rates are always read back from registers.
> 
> Why can't we control the clks from the clk framework? Please add that
> information in the commit text here.

OK, I will update commit message and comments

These clocks are only modified for DRAM frequency switches during which 
DRAM 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, muxes are handled explicitly.

Linux code performing the SMC call is also part of this series:

     https://patchwork.kernel.org/patch/11104145/

>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/clk/imx/clk-imx8mm.c | 6 ++++--
>>   drivers/clk/imx/clk-imx8mn.c | 6 ++++--
>>   drivers/clk/imx/clk-imx8mq.c | 7 ++++---
>>   3 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
>> index 4ead3ea2713c..6cac80550f43 100644
>> --- a/drivers/clk/imx/clk-imx8mm.c
>> +++ b/drivers/clk/imx/clk-imx8mm.c
>> @@ -526,12 +526,14 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>>          /* IPG */
>>          clks[IMX8MM_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
>>          clks[IMX8MM_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
>>   
>>          /* IP */
>> -       clks[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
>> -       clks[IMX8MM_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
>> +       clks[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000,
>> +                       CLK_GET_RATE_NOCACHE);
>> +       clks[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080,
>> +                       CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> 
> Also, add a comment to this effect about why it can't be done from the
> clk framework wherever the CLK_GET_RATE_NOCACHE flag is set. Basically
> this flag is a hack and is an example of something that we need to fix.

DRAM freq switch requires multiple clk changes to be performed 
atomically while DRAM itself is not accessible so it's not something to 
"fix".

--
Regards,
Leonard
Stephen Boyd Sept. 17, 2019, 4:32 p.m. UTC | #3
Quoting Leonard Crestez (2019-09-16 16:03:53)
> On 2019-09-16 11:33 PM, Stephen Boyd wrote:
> > Quoting Leonard Crestez (2019-08-20 08:45:06)
> >> Dram frequency changes required modifying these clocks outside the
> >> control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
> >> rates are always read back from registers.
> > 
> > Why can't we control the clks from the clk framework? Please add that
> > information in the commit text here.
> 
> OK, I will update commit message and comments
> 
> These clocks are only modified for DRAM frequency switches during which 
> DRAM 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, muxes are handled explicitly.

Is there any reason to expose or control these clks from Linux then? It
might be easier to just make any children clks of the DRAM frequency clk
"root" clks and then ignore any frequency that they might have.
Similarly, because the SMC call is used to change the frequency, it may
be simpler to handle that completely outside of the clk framework (it
may already be this way in this patch series but I haven't read
everything here).

> 
> Linux code performing the SMC call is also part of this series:
> 
>      https://patchwork.kernel.org/patch/11104145/
> 
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   drivers/clk/imx/clk-imx8mm.c | 6 ++++--
> >>   drivers/clk/imx/clk-imx8mn.c | 6 ++++--
> >>   drivers/clk/imx/clk-imx8mq.c | 7 ++++---
> >>   3 files changed, 12 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> >> index 4ead3ea2713c..6cac80550f43 100644
> >> --- a/drivers/clk/imx/clk-imx8mm.c
> >> +++ b/drivers/clk/imx/clk-imx8mm.c
> >> @@ -526,12 +526,14 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
> >>          /* IPG */
> >>          clks[IMX8MM_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
> >>          clks[IMX8MM_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
> >>   
> >>          /* IP */
> >> -       clks[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
> >> -       clks[IMX8MM_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
> >> +       clks[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000,
> >> +                       CLK_GET_RATE_NOCACHE);
> >> +       clks[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080,
> >> +                       CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> > 
> > Also, add a comment to this effect about why it can't be done from the
> > clk framework wherever the CLK_GET_RATE_NOCACHE flag is set. Basically
> > this flag is a hack and is an example of something that we need to fix.
> 
> DRAM freq switch requires multiple clk changes to be performed 
> atomically while DRAM itself is not accessible so it's not something to 
> "fix".

Ok. Fix may be the wrong word.
Leonard Crestez Sept. 17, 2019, 4:59 p.m. UTC | #4
On 2019-09-17 7:32 PM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-09-16 16:03:53)
>> On 2019-09-16 11:33 PM, Stephen Boyd wrote:
>>> Quoting Leonard Crestez (2019-08-20 08:45:06)
>>>> Dram frequency changes required modifying these clocks outside the
>>>> control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
>>>> rates are always read back from registers.
>>>
>>> Why can't we control the clks from the clk framework? Please add that
>>> information in the commit text here.
>>
>> OK, I will update commit message and comments
>>
>> These clocks are only modified for DRAM frequency switches during which
>> DRAM 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, muxes are handled explicitly.
> 
> Is there any reason to expose or control these clks from Linux then? It
> might be easier to just make any children clks of the DRAM frequency clk
> "root" clks and then ignore any frequency that they might have.
> Similarly, because the SMC call is used to change the frequency, it may
> be simpler to handle that completely outside of the clk framework (it
> may already be this way in this patch series but I haven't read
> everything here).

The dram alt/apb clocks are real imx8m composite clocks with the same HW 
implementation as used for peripherals. They also have mux parents which 
are under the control of the clock framework so the freq switching code 
takes care to properly enable the new parents before calling SMC.

See imx_ddrc_set_freq: https://patchwork.kernel.org/patch/11104145/

Removing dram alt/apb clocks from the tree would still require keeping 
possible parents enabled somehow. It wouldn't be simpler but a lot uglier.

--
Regards,
Leonard

Patch
diff mbox series

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