Hi, Stehpen Best Regards! Anson Huang > -----Original Message----- > From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx] > Sent: 2019年2月23日 3:08 > To: devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx; > kernel@xxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx; > mturquette@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > shawnguo@xxxxxxxxxx; Aisheng Dong <aisheng.dong@xxxxxxx>; Anson > Huang <anson.huang@xxxxxxx>; Daniel Baluta <daniel.baluta@xxxxxxx> > Cc: dl-linux-imx <linux-imx@xxxxxxx> > 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.Huang@xxxxxxx> > > 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; > > +} > > +
