Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From: Taniya Das
Date: Tue Jun 19 2018 - 06:25:29 EST
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.
--