Re: [PATCH 4/4] cpufreq: qcom-hw: provide online/offline operations

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 07 Mar 07:30 PST 2022, Dmitry Baryshkov wrote:

> Provide lightweight online and offline operations. This saves us from
> parsing and tearing down the OPP tables each time the CPU is put online
> or offline.

Isn't that a slight understatement? Doesn't it also save us from e.g.
ioremapping the memory, traversing DT to discover the policy's
related_cpus and requesting the dcvs interrupt?

I like the idea of getting these things out of the init/exit path. I do
however think that we could move most of this to probe time, and thereby
be able to rely on devm operations for many of these things.

That said, I still like your idea of having a fast path for this...

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 580520215ee7..12b67f16b78f 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -424,10 +424,26 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>  	return 0;
>  }
>  
> -static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
> +static int qcom_cpufreq_hw_cpu_online(struct cpufreq_policy *policy)
>  {
> +	struct qcom_cpufreq_data *data = policy->driver_data;
> +	struct platform_device *pdev = cpufreq_get_driver_data();
> +	int ret;
> +

For backwards compatibility reasons it's valid to not have
data->throttle_irq. This will however cause irq_set_affinity_hint() to
return -EINVAL and we'll get a print.

So you should handle that gracefully.

> +	ret = irq_set_affinity_hint(data->throttle_irq, policy->cpus);
> +	if (ret)
> +		dev_err(&pdev->dev, "Failed to set CPU affinity of %s[%d]\n",
> +			data->irq_name, data->throttle_irq);
> +
> +	return ret;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_offline(struct cpufreq_policy *policy)
> +{
> +	struct qcom_cpufreq_data *data = policy->driver_data;
> +
>  	if (data->throttle_irq <= 0)
> -		return;
> +		return 0;
>  
>  	mutex_lock(&data->throttle_lock);
>  	data->cancel_throttle = true;

This will mark the throttle as cancelled, you need to clear this as
you're bringing the policy online again.

> @@ -435,6 +451,12 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
>  
>  	cancel_delayed_work_sync(&data->throttle_work);
>  	irq_set_affinity_hint(data->throttle_irq, NULL);

You don't disable_irq(data->throttle_irq) here. I think
qcom_lmh_dcvs_notify() will be unhappy if we get thermal pressure from a
policy with no cpus?

Note though that you can't enable it in online(), as it will be enabled
in ready()...

> +
> +	return 0;
> +}
> +
> +static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
> +{
>  	free_irq(data->throttle_irq, data);

As above, you should treat throttle_irq <= 0 gracefully.

Regards,
Bjorn

>  }
>  
> @@ -588,6 +610,8 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>  	.get		= qcom_cpufreq_hw_get,
>  	.init		= qcom_cpufreq_hw_cpu_init,
>  	.exit		= qcom_cpufreq_hw_cpu_exit,
> +	.online		= qcom_cpufreq_hw_cpu_online,
> +	.offline	= qcom_cpufreq_hw_cpu_offline,
>  	.register_em	= cpufreq_register_em_with_opp,
>  	.fast_switch    = qcom_cpufreq_hw_fast_switch,
>  	.name		= "qcom-cpufreq-hw",
> -- 
> 2.34.1
>