RE: [PATCH RESEND V6 2/2] clk: imx: scu: add cpu frequency scaling support

Hi, Stehpen

Best Regards!
Anson Huang

-----Original Message-----
From: Stephen Boyd [mailto:sboyd@kernel.org]
Sent: 2019年2月23日 3:08
To: devicetree@vger.kernel.org; festevam@gmail.com;
kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org; linux-
clk@vger.kernel.org; linux-kernel@vger.kernel.org; mark.rutland@arm.com;
mturquette@baylibre.com; robh+dt@kernel.org; s.hauer@pengutronix.de;
shawnguo@kernel.org; Aisheng Dong [off-list ref]; Anson
Huang [off-list ref]; Daniel Baluta [off-list ref]
Cc: dl-linux-imx <redacted>
Subject: Re: [PATCH RESEND V6 2/2] clk: imx: scu: add cpu frequency scaling
support

Quoting Anson Huang (2019-02-21 18:32:10)

quoted

On NXP's i.MX SoCs with system controller inside, CPU frequency
scaling can ONLY be done by system controller firmware, and it can
ONLY be requested from secure mode, so Linux kernel has to call ARM
SMC to trap to ARM-Trusted-Firmware to request system controller
firmware to do CPU frequency scaling.

This patch adds i.MX system controller CPU frequency scaling support,
it reuses cpufreq-dt driver and implement the CPU frequency scaling
inside SCU clock driver.

Signed-off-by: Anson Huang <redacted>

Ah I missed one thing, see below.

quoted

@@ -180,6 +185,23 @@ static long clk_scu_round_rate(struct clk_hw *hw,

unsigned long rate,

quoted

        return rate;
 }

+static int clk_scu_atf_set_cpu_rate(struct clk_hw *hw, unsigned long rate,
+                                   unsigned long parent_rate) {
+       struct clk_scu *clk = to_clk_scu(hw);
+       struct arm_smccc_res res;
+       unsigned long cluster_id;
+
+       if (clk->rsrc_id == IMX_SC_R_A35)
+               cluster_id = 0;

Do we still need to check this anymore? Why not just always use cluster_id 0?

The i.MX8QXP ONLY has 1 cluster named A35, while on i.MX8QM there will be
2 clusters, A53 and A72, so we need to use the resource ID to initialize the cluster_id.

quoted

+
+       /* CPU frequency scaling can ONLY be done by ARM-Trusted-Firmware

*/

quoted

+       arm_smccc_smc(IMX_SIP_CPUFREQ, IMX_SIP_SET_CPUFREQ,
+                     cluster_id, rate, 0, 0, 0, 0, &res);

Because not checking would make this work, vs. checking causes this code to
sometimes use an uninitialized value from the stack.

 89 +       if (rsrc_id == IMX_SC_R_A35)
 90 +               init.ops = &clk_scu_cpu_ops;
 91 +       else
 92 +               init.ops = &clk_scu_ops;

I think it should be good. Only when plan to support cpu-freq scaling, then the
CPU clock will be switched to use clk_scu_cpu_ops and the clutser_id initialization
will be done according to CPU resource. For example, when we plan to support i.MX8QM
cpu-freq scaling, we will add A53 and A72 check here and switch the clock ops to clk_scu_cpu_ops,
also we will add the cluster_id initialization in the SMC clock set rate.

Thanks,
Anson.

quoted

+
+       return 0;
+}
+

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