Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

Sorry for being late..

On 07-06-18, 12:48, Taniya Das wrote:
> On 6/6/2018 11:31 AM, Viresh Kumar wrote:
> > On 04-06-18, 16:16, Taniya Das wrote:

> > > +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
> > > +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> > > +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> > > +	.verify		= cpufreq_generic_frequency_table_verify,
> > > +	.target_index	= qcom_cpufreq_fw_target_index,
> > > +	.get		= qcom_cpufreq_fw_get,
> > > +	.init		= qcom_cpufreq_fw_cpu_init,
> > 
> > What about CPU hotplug ? We can still do that, right ? So what will happen if
> > all CPUs of a freq-domain are removed (hence cpufreq policy is removed) and then
> > someone calls qcom_cpufreq_fw_get() ? You should really work on cpufreq_policy
> > there to get 'c'.
> > 
> 
> You want the _get to do something as below.
> Please correct me if my understanding is wrong.
> ....
> 
>  policy = cpufreq_cpu_get_raw(cpu);
>  if (!policy)
>     return 0;
> 
>  c = policy->driver_data;
> 
>  index = readl_relaxed(c->perf_base);
>  index = min(index, LUT_MAX_ENTRIES - 1);
> 
>  return c->table[index].frequency;
> 
> ....

Right.

> > > +static int qcom_read_lut(struct platform_device *pdev,
> > > +			 struct cpufreq_qcom *c)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> > > +
> > > +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> > > +				sizeof(*c->table), GFP_KERNEL);
> > > +	if (!c->table)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
> > > +		src = ((data & GENMASK(31, 30)) >> 30);
> > > +		lval = (data & GENMASK(7, 0));
> > > +		core_count = CORE_COUNT_VAL(data);
> > > +
> > > +		if (!src)
> > > +			c->table[i].frequency = INIT_RATE / 1000;
> > > +		else
> > > +			c->table[i].frequency = XO_RATE * lval / 1000;
> > > +
> > > +		cur_freq = c->table[i].frequency;
> > > +
> > > +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > +			i, c->table[i].frequency, core_count);
> > > +
> > > +		if (core_count != c->max_cores)
> > > +			cur_freq = CPUFREQ_ENTRY_INVALID;
> > > +
> > > +		/*
> > > +		 * Two of the same frequencies with the same core counts means
> > > +		 * end of table.
> > > +		 */
> > > +		if (i > 0 && c->table[i - 1].frequency ==
> > > +		   c->table[i].frequency && prev_cc == core_count) {
> > > +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
> > > +
> > > +			if (prev_freq == CPUFREQ_ENTRY_INVALID)
> > > +				prev->flags = CPUFREQ_BOOST_FREQ;
> > > +			break;
> > > +		}
> > > +		prev_cc = core_count;
> > > +		prev_freq = cur_freq;
> > > +	}
> > > +
> > > +	c->table[i].frequency = CPUFREQ_TABLE_END;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Looks like there are many problems here.
> > - You are assigning prev_freq with cur_freq (which may be uninitialized local
> >    variable here).
> > - In this version, you never write CPUFREQ_ENTRY_INVALID to table[i].frequency,
> >    which looks wrong as well.
> > 
> 
> - The code to detect boost, would only enter for i > 0 and the prev_freq
> would be initialized with the cur_freq.
> - In the case where the core_count != max_cores, the cur_freq is marked
> INVALID, and when both prev_freq == cur_freq && prev_cc && cur_cc match,
> that is the time the prev table flags need to be updated. Marking the
> table[i].frequency as INVALID is not required as cur_freq is already marked
> with the same. Please correct me if you think otherwise.

Yeah but the value of cur_freq isn't written to the table entries now. This
wasn't the case in the earlier version. Have a look at that one.

-- 
viresh