Hi, Stehpen Best Regards! Anson Huang
> -----Original Message----- > From: Stephen Boyd [mailto:sb...@kernel.org] > Sent: 2019年2月23日 3:08 > To: devicet...@vger.kernel.org; feste...@gmail.com; > ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; linux- > c...@vger.kernel.org; linux-kernel@vger.kernel.org; mark.rutl...@arm.com; > mturque...@baylibre.com; robh...@kernel.org; s.ha...@pengutronix.de; > shawn...@kernel.org; Aisheng Dong <aisheng.d...@nxp.com>; Anson > Huang <anson.hu...@nxp.com>; Daniel Baluta <daniel.bal...@nxp.com> > Cc: dl-linux-imx <linux-...@nxp.com> > Subject: Re: [PATCH RESEND V6 2/2] clk: imx: scu: add cpu frequency scaling > support > > Quoting Anson Huang (2019-02-21 18:32:10) > > 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 <anson.hu...@nxp.com> > > Ah I missed one thing, see below. > > > @@ -180,6 +185,23 @@ static long clk_scu_round_rate(struct clk_hw *hw, > unsigned long rate, > > 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. > > > + > > + /* CPU frequency scaling can ONLY be done by ARM-Trusted-Firmware > */ > > + 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. > > > + > > + return 0; > > +} > > +
