Re: [PATCH 3/5] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support

[Viresh Kumar]

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