| Message ID | 1550802436-13007-2-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,V6,1/2] arm64: dts: freescale: imx8qxp: add cpu opp table | expand |
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@nxp.com> > --- Applied to clk-next
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@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? > + > + /* 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. > + > + return 0; > +} > +
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 <aisheng.dong@nxp.com>; Anson > Huang <anson.huang@nxp.com>; Daniel Baluta <daniel.baluta@nxp.com> > Cc: dl-linux-imx <linux-imx@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.Huang@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; > > +} > > +
Quoting Anson Huang (2019-02-22 18:32:28) > > > + 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. > Ok. So then please make the set_rate function fail if the rsrc_id doesn't match something expected. As the code is written right now, the compiler can't figure out that cluster_id will always be assigned, so it complains that it may be using uninitialized data.
Hi, Stephen Best Regards! Anson Huang > -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: 2019年2月26日 1:22 > 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 <aisheng.dong@nxp.com>; Anson > Huang <anson.huang@nxp.com>; Daniel Baluta <daniel.baluta@nxp.com> > Cc: dl-linux-imx <linux-imx@nxp.com> > Subject: RE: [PATCH RESEND V6 2/2] clk: imx: scu: add cpu frequency scaling > support > > Quoting Anson Huang (2019-02-22 18:32:28) > > > > + 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. > > > > Ok. So then please make the set_rate function fail if the rsrc_id doesn't > match something expected. As the code is written right now, the compiler > can't figure out that cluster_id will always be assigned, so it complains that it > may be using uninitialized data. Agreed, I have sent V7 patch series to add return fail if the resource ID is NOT expected. Thanks, Anson.
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c index f460526..34c7cdf4 100644 --- a/drivers/clk/imx/clk-scu.c +++ b/drivers/clk/imx/clk-scu.c @@ -4,12 +4,17 @@ * Dong Aisheng <aisheng.dong@nxp.com> */ +#include <dt-bindings/firmware/imx/rsrc.h> +#include <linux/arm-smccc.h> #include <linux/clk-provider.h> #include <linux/err.h> #include <linux/slab.h> #include "clk-scu.h" +#define IMX_SIP_CPUFREQ 0xC2000001 +#define IMX_SIP_SET_CPUFREQ 0x00 + static struct imx_sc_ipc *ccm_ipc_handle; /* @@ -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; + + /* 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); + + return 0; +} + /* * clk_scu_set_rate - Set rate for a SCU clock * @hw: clock to change rate for @@ -312,6 +334,14 @@ static const struct clk_ops clk_scu_ops = { .unprepare = clk_scu_unprepare, }; +static const struct clk_ops clk_scu_cpu_ops = { + .recalc_rate = clk_scu_recalc_rate, + .round_rate = clk_scu_round_rate, + .set_rate = clk_scu_atf_set_cpu_rate, + .prepare = clk_scu_prepare, + .unprepare = clk_scu_unprepare, +}; + struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents, int num_parents, u32 rsrc_id, u8 clk_type) { @@ -329,6 +359,10 @@ struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents, init.name = name; init.ops = &clk_scu_ops; + if (rsrc_id == IMX_SC_R_A35) + init.ops = &clk_scu_cpu_ops; + else + init.ops = &clk_scu_ops; init.parent_names = parents; init.num_parents = num_parents;
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@nxp.com> --- No changes compared to last version, just redo patch based on clk-next to fix patch conflicts. --- drivers/clk/imx/clk-scu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)