On Tue, 8 Mar 2022 at 01:38, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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.
Ack.
>
> > + 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.
ack.
>
> > @@ -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()...
And we can't just disable it here, as it might be enabled or might be
disabled. I think the simplest would be to call free_irq() here and
request_irq() to online().
>
> > +
> > + 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
> >
--
With best wishes
Dmitry
