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

[Leonard Crestez <leonard.crestez@nxp.com>]

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.

> +#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?