On Fri, Mar 21, 2014 at 03:01:29PM +0530, Viresh Kumar wrote:

On Thu, Mar 20, 2014 at 5:41 PM, Gautham R. Shenoy
[off-list ref] wrote:

quoted

From: "Gautham R. Shenoy" <redacted>

The current frequency of a cpu is reported through the sysfs file
cpuinfo_cur_freq. This requires the driver to implement a
"->get(unsigned int cpu)" method which will return the current
operating frequency.

Implement a function named powernv_cpufreq_get() which reads the local
pstate from the PMSR and returns the corresponding frequency.

Set the powernv_cpufreq_driver.get hook to powernv_cpufreq_get().

Signed-off-by: Gautham R. Shenoy <redacted>
---
 drivers/cpufreq/powernv-cpufreq.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Please merge these fixups with the first patch which is creating the driver.
I understand that a different guy has created this patch and so wanted
to have a patch on his name but its really difficult to review this

way.

Heh! Well, that wasn't the reason why this was sent out as a separate
patch, but never mind. Though I don't understand why it would be
difficult to review the patch though.

Better add your signed-off in the first patch instead. Sending such changes
once the driver is mainlined looks fine.

Sure, this makes sense.

quoted

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 46bee8a..ef6ed8c 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -206,6 +206,43 @@ static inline void set_pmspr(unsigned long sprn, unsigned long val)
        BUG();
 }

+/*
+ * Computes the current frequency on this cpu
+ * and stores the result in *ret_freq.
+ */
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+       unsigned long pmspr_val;
+       s8 local_pstate_id;
+       int *cur_freq, freq, pstate_id;
+
+       cur_freq = (int *)ret_freq;

You don't need cur_freq variable at all..

I don't like it either. But the compiler complains without this hack
:-(

quoted

+       pmspr_val = get_pmspr(SPRN_PMSR);
+
+       /* The local pstate id corresponds bits 48..55 in the PMSR.
+         * Note: Watch out for the sign! */
+       local_pstate_id = (pmspr_val >> 48) & 0xFF;
+       pstate_id = local_pstate_id;

similarly local_pstate_id

well, I am interested in the bits 48..55 of pmspr_val. That's the
pstate_id which can be negative. So I'll like to keep
local_pstate_id. 

quoted

+
+       freq = pstate_id_to_freq(pstate_id);
+       pr_debug("cpu %d pmsr %lx pstate_id %d frequency %d \n",
+               smp_processor_id(), pmspr_val, pstate_id, freq);
+       *cur_freq = freq;

Move above print here after a blank line. Also remove freq variable as
well and use *cur_freq directly.. And then you can rename it to freq

as well.

We can get rid of freq and use *cur_freq in its place.


Thanks for the detailed review.

--
Regards
gautham.