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 
> Cc: dl-linux-imx ; 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 ; Daniel Baluta
> ; 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 +, 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_CPUFREQ0xC201
> > +#define IMX_SIP_SET_CPUFREQ0x00
> > +
> >  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, );
> > +   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. 

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

[Leonard Crestez]

On Wed, 2019-02-13 at 13:32 +, 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  0xC201
> +#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.

> +#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, );
> + 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?