Hi Viresh ,
Thanks for reviewing in detail.
I will correct all comments related to coding standards in my next patch.
On 04/13/2016 10:33 AM, Viresh Kumar wrote:
Comments mostly on the coding standards which you have *not* followed.
Also, please run checkpatch --strict next time you send patches
upstream.
Thanks for pointing out the --strict option, was not aware of that. I will
run checkpatch --strict on the next versions.
On 12-04-16, 23:36, Akshay Adiga wrote:
+
+/*
+ * While resetting we don't want "timer" fields to be set to zero as we
+ * may lose track of timer and will not be able to cleanly remove it
+ */
+#define reset_gpstates(policy) memset(policy->driver_data, 0,\
+ sizeof(struct global_pstate_info)-\
+ sizeof(struct timer_list)-\
+ sizeof(spinlock_t))
That's super *ugly*. Why don't you create a simple routine which will
set the 5 integer variables to 0 in a straight forward way ?
Yeh, will create a routine.
@@ -348,14 +395,17 @@ static void set_pstate(void *freq_data)
unsigned long val;
unsigned long pstate_ul =
((struct powernv_smp_call_data *) freq_data)->pstate_id;
+ unsigned long gpstate_ul =
+ ((struct powernv_smp_call_data *) freq_data)->gpstate_id;
Remove these unnecessary casts and do:
struct powernv_smp_call_data *freq_data = data; //Name func arg as data
And then use freq_data->*.
Ok. Will do that.
+/*
+ * gpstate_timer_handler
+ *
+ * @data: pointer to cpufreq_policy on which timer was queued
+ *
+ * This handler brings down the global pstate closer to the local pstate
+ * according quadratic equation. Queues a new timer if it is still not equal
+ * to local pstate
+ */
+void gpstate_timer_handler(unsigned long data)
+{
+ struct cpufreq_policy *policy = (struct cpufreq_policy *) data;
no need to cast.
May be i need a cast here, because data is unsigned long ( unlike other places
where its void *).
On building without cast, it throws me a warning.
+ struct global_pstate_info *gpstates = (struct global_pstate_info *)
+ struct powernv_smp_call_data freq_data;
+ int ret;
+
+ ret = spin_trylock(&gpstates->gpstate_lock);
no need of 'ret' for just this, simply do: if (!spin_trylock())...
Sure will do that.
a
+ if (!ret)
+ return;
+
+ gpstates->last_sampled_time += time_diff;
+ gpstates->elapsed_time += time_diff;
+ freq_data.pstate_id = gpstates->last_lpstate;
+ if ((gpstates->last_gpstate == freq_data.pstate_id) ||
+ (gpstates->elapsed_time > MAX_RAMP_DOWN_TIME)) {
+ freq_data.gpstate_id = freq_data.pstate_id;
+ reset_gpstates(policy);
+ gpstates->highest_lpstate = freq_data.pstate_id;
+ } else {
+ freq_data.gpstate_id = calculate_global_pstate(
You can't break a line after ( of a function call :)
Let it go beyond 80 columns if it has to.
May be i will try to get it inside 80 columns with a temporary variable instead
of
freq_data.gpstate_id.
+ gpstates->elapsed_time, gpstates->highest_lpstate,
+ freq_data.pstate_id);
+ }
+
+ /* If local pstate is equal to global pstate, rampdown is over
Bad style again.
+ * So timer is not required to be queued.
+ */
+ if (freq_data.gpstate_id != freq_data.pstate_id)
+ ret = queue_gpstate_timer(gpstates);
ret not used.
Should i make it void instead of returning int?, as i cannot do much even if it
fails, except for notifying.
+gpstates_done:
+ gpstates->last_sampled_time = cur_msec;
+ gpstates->last_gpstate = freq_data.gpstate_id;
+ gpstates->last_lpstate = freq_data.pstate_id;
+
/*
* Use smp_call_function to send IPI and execute the
* mtspr on target CPU. We could do that without IPI
* if current CPU is within policy->cpus (core)
*/
smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
+ spin_unlock_irqrestore(&gpstates->gpstate_lock, flags);
+ return 0;
+}
+static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
Add this after the init() routine.
Ok will do it.
+ policy->driver_data = gpstates;
+
+ /* initialize timer */
+ init_timer_deferrable(&gpstates->timer);
+ gpstates->timer.data = (unsigned long) policy;
+ gpstates->timer.function = gpstate_timer_handler;
+ gpstates->timer.expires = jiffies +
+ msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
+
+ pr_info("Added global_pstate_info & timer for %d cpu\n", base);
return cpufreq_table_validate_and_show(policy, powernv_freqs);
Who will free gpstates if this fails ?
Thanks for pointing out. Will fix in v2.
Regards
Akshay Adiga
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
