lkml.org 
[lkml]   [2018]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From
Date


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.

--

\
 
 \ /
  Last update: 2018-06-19 12:26    [W:0.116 / U:0.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site