Re: [PATCH V3 11/15] cpufreq: mediatek: Link CCI device to CPU

[AngeloGioacchino Del Regno]

Il 15/04/22 07:59, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx>
>
> In some MediaTek SoCs, like MT8183, CPU and CCI share the same power
> supplies. Cpufreq needs to check if CCI devfreq exists and wait until
> CCI devfreq ready before scaling frequency.
>
> Before CCI devfreq is ready, we record the voltage when booting to
> kernel and use the max(cpu target voltage, booting voltage) to
> prevent cpufreq adjust to the lower voltage which will cause the CCI
> crash because of high frequency and low voltage.
>
> - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start
>    DVFS when CCI is ready.
> - Add platform data for MT8183.
>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx>

I am enthusiast to see that the solution that I've proposed was welcome!

I only have one nit on this patch, check below:

> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 80 +++++++++++++++++++++++++++++-
>   1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index d4c00237e862..dd3f739fede1 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c

..snip..

> @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>   	vproc = dev_pm_opp_get_voltage(opp);
>   	dev_pm_opp_put(opp);
>   
> +	/*
> +	 * If MediaTek cci is supported but is not ready, we will use the value
> +	 * of max(target cpu voltage, booting voltage) to prevent high freqeuncy
> +	 * low voltage crash.
> +	 */
> +	if (info->soc_data->ccifreq_supported && !is_ccifreq_ready(info))
> +		vproc = max(vproc, info->vproc_on_boot);
> +
>   	/*
>   	 * If the new voltage or the intermediate voltage is higher than the
>   	 * current voltage, scale up voltage first.

..snip..

> @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   	if (ret)
>   		goto out_disable_mux_clock;
>   
> +	info->vproc_on_boot = regulator_get_voltage(info->proc_reg);

This result is used only if we use ccifreq, so this should be enclosed in an if
condition: this will spare us some (yes, small) time on devices that don't use it.

	if (info->soc_data->ccifreq_supported) {
		info->vproc_on_boot = regulator_get_voltage(info->proc_reg);
		if (info->vproc_on_boot < 0) {
			dev_err(....
			goto ..
		}
	}

P.S.: While at it, since the maximum width is 100 columns, the dev_err() call fits,
so don't break that line!

After the requested change:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>