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

[Taniya Das <tdas@codeaurora.org>]

On 6/19/2018 2:24 PM, Viresh Kumar wrote:
> 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.
> 

Yes, Viresh, earlier code was updating the table frequency as I was 
marking the table frequency INVALID.
	if (core_count != c->max_cores)	
		c->table[i].frequency = CPUFREQ_ENTRY_INVALID;	

And thus I had to update the table frequency.

But now I have used the cur_freq instead and the table frequency is not 
touched.
	if (core_count != c->max_cores)
		cur_freq = CPUFREQ_ENTRY_INVALID;

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--