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

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Leonard Crestez
> Sent: 2019å2æ13æ 21:53
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: dl-linux-imx <linux-imx@xxxxxxx>; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; mturquette@xxxxxxxxxxxx;
> sboyd@xxxxxxxxxx; Aisheng Dong <aisheng.dong@xxxxxxx>; Daniel Baluta
> <daniel.baluta@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> clk@xxxxxxxxxxxxxxx; viresh.kumar@xxxxxxxxxx
> 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.