Best Regards! Anson Huang
> -----Original Message----- > From: Leonard Crestez > Sent: 2019年2月13日 21:53 > To: Anson Huang <anson.hu...@nxp.com> > Cc: dl-linux-imx <linux-...@nxp.com>; robh...@kernel.org; > mark.rutl...@arm.com; shawn...@kernel.org; s.ha...@pengutronix.de; > ker...@pengutronix.de; feste...@gmail.com; mturque...@baylibre.com; > sb...@kernel.org; Aisheng Dong <aisheng.d...@nxp.com>; Daniel Baluta > <daniel.bal...@nxp.com>; devicet...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > c...@vger.kernel.org; viresh.ku...@linaro.org > Subject: Re: [PATCH V2 2/2] clk: imx: scu: add cpu frequency scaling support > > On Wed, 2019-02-13 at 13:32 +0000, Anson Huang wrote: > > 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. > > > +#ifdef CONFIG_CPUFREQ_DT > > +#define IMX_SIP_CPUFREQ 0xC2000001 > > +#define IMX_SIP_SET_CPUFREQ 0x00 > > + > > static struct imx_sc_ipc *ccm_ipc_handle; > > Without CONFIG_CPUFREQ_DT the ccm_ipc_handle won't be defined and > build will break. > > But is there a good reason for ifdef? In general clock function are compiled > even if no driver is calling them. Oops, this is my mistake to put the ccm_ipc_handle inside the ifdef. Adding Ifdef is just to save some code if CPUFREQ_DT is NOT defined, but I can remove it in next version. > > > +#ifdef CONFIG_CPUFREQ_DT > > + /* CPU clock can ONLY be done by TF-A */ > > + if (clk->clk_type == IMX_SC_PM_CLK_CPU) { > > + struct arm_smccc_res res; > > + unsigned int cluster_id; > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(imx_sc_cpufreq_data); i++) { > > + if (!strcmp(clk_hw_get_name(hw), > > + imx_sc_cpufreq_data[i].clk_name)) { > > + cluster_id = > imx_sc_cpufreq_data[i].cluster_id; > > + break; > > + } > > + } > > + > > + /* > > + * As some other clock types have same value as > > + * IMX_SC_PM_CLK_CPU, so we need to double check > > + * the clock being scaled is indeed CPU clock which > > + * matches the table we define. > > + */ > > + if (i < ARRAY_SIZE(imx_sc_cpufreq_data)) { > > + arm_smccc_smc(IMX_SIP_CPUFREQ, > IMX_SIP_SET_CPUFREQ, > > + cluster_id, rate, 0, 0, 0, 0, &res); > > + return 0; > > + } > > + } > > +#endif > > The code inside the ifdef would look better in a separate > imx_clk_atf_set_rate function. Maybe even separate clk_ops? I can add a separate function for CPU clock scaling, adding new clk_ops changes too much, using a separate function should be good for now? Thanks. Anson.