On 14-06-21, 21:58, Thara Gopinath wrote:
> On 6/14/21 6:31 AM, Viresh Kumar wrote:
> > On 08-06-21, 18:29, Thara Gopinath wrote:
> > > +static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
> > > +{
> > > + struct cpufreq_policy policy;
> > > + struct dev_pm_opp *opp;
> > > + struct device *dev;
> > > + unsigned long max_capacity, capacity, freq_hz;
> > > + unsigned int val, freq;
> > > +
> > > + val = readl_relaxed(data->base + data->soc_data->reg_current_vote);
> > > + freq = qcom_lmh_vote_to_freq(val);
> > > + freq_hz = freq * HZ_PER_KHZ;
> > > +
> > > + /* Do I need to calculate ceil and floor ? */
> >
> > You don't know ?
>
> stray comment! Will remove it.
>
> >
> > > + dev = get_cpu_device(cpumask_first(data->cpus));
> > > + opp = dev_pm_opp_find_freq_floor(dev, &freq_hz);
> > > + if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE)
> > > + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> > > +
> > > + data->throttled_freq = freq_hz / HZ_PER_KHZ;
> > > +
> >
> > What exactly are we trying to do here ? A comment would be good as
> > well.
>
> You want me to put a comment saying converting frequency in hz to khz ?
Not that, but for the entire routine. What exactly are we looking to
do here and why?
> >
> > > + cpufreq_get_policy(&policy, cpumask_first(data->cpus));
> > > +
> > > + /* Update thermal pressure */
> > > + max_capacity = arch_scale_cpu_capacity(cpumask_first(data->cpus));
> >
> > Set capacity of a single CPU from a policy ?
>
> Get maximum capacity of a cpu.
Ahh, I thought you are setting it :(
> >
> > > + capacity = data->throttled_freq * max_capacity;
> > > + capacity /= policy.cpuinfo.max_freq;
> > > + /* Don't pass boost capacity to scheduler */
> > > + if (capacity > max_capacity)
> > > + capacity = max_capacity;
> > > + arch_set_thermal_pressure(data->cpus, max_capacity - capacity);
> >
> > You should really be using policy->cpus instead of allocating
> > data->cpus..
>
> Yes I should be. But I still need data->cpus to get the policy.
>From the comment which comes later on, you shall get the policy here
anyway.
> > > +}
> > > +
> > > +static void qcom_lmh_dcvs_poll(struct work_struct *work)
> > > +{
> > > + struct qcom_cpufreq_data *data;
> > > +
> > > + data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work);
> > > +
> > > + qcom_lmh_dcvs_notify(data);
> >
> > You should really move the below stuff the disable_irq_nosync(), it
> > will make your life easier.
Damn, s/disable_irq_nosync/qcom_lmh_dcvs_notify/
> I don't understand your comment here. I want to disable irq. call notify.
> Start polling. And in polling I want to call notify and if the thermal event
> has passed stop polling else continue polling.
Yeah, I messed up in the comment. I was asking to move the enable-irq
and mod_delayed_work to qcom_lmh_dcvs_notify() itself.
> > > + /**
> > > + * If h/w throttled frequency is higher than what cpufreq has requested for, stop
> > > + * polling and switch back to interrupt mechanism
> > > + */
> > > + if (data->throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(data->cpus)))
> > > + /* Clear the existing interrupts and enable it back */
> > > + enable_irq(data->lmh_dcvs_irq);
> > > + else
> > > + mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work,
> > > + msecs_to_jiffies(10));
> > > +}
> > > +
> > > +static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
> > > +{
> > > + struct qcom_cpufreq_data *c_data = data;
> > > +
> > > + /* Disable interrupt and enable polling */
> > > + disable_irq_nosync(c_data->lmh_dcvs_irq);
> > > + qcom_lmh_dcvs_notify(c_data);
> > > + mod_delayed_work(system_highpri_wq, &c_data->lmh_dcvs_poll_work, msecs_to_jiffies(10));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static const struct qcom_cpufreq_soc_data qcom_soc_data = {
> > > .reg_enable = 0x0,
> > > .reg_freq_lut = 0x110,
> > > .reg_volt_lut = 0x114,
> > > + .reg_current_vote = 0x704,
> >
> > Should this be a different patch ?
>
> Why ? This is the register to read the throttled frequency.
Okay, it looked this is separate. Leave it.
> > > + ret = devm_request_irq(dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq,
> > > + 0, "dcvsh-irq", data);
> >
> > I would rather pass policy as data here.
>
> So policy for a cpu can change runtime, right ?
No, it is allocated just once.
--
viresh
