On 09/08/17 11:06, Viresh Kumar wrote:

On 09-08-17, 10:59, Sudeep Holla wrote:

quoted

On 09/08/17 05:18, Viresh Kumar wrote:

quoted

quoted

This stores the same handle pointer which is stored in the global variable
below. Right? Why keep a local variable here at all ?

Yes, you are right. Initially, started with just private pointers and
then added global. I was thinking of calling devm_scmi_handle_get per
policy to reflect the refcount correctly and drop global variable. Let
me know what you think.

A refcount of 1 should be fine as well, i.e. For the cpufreq driver. Why would
SCMI care if we manage multiple policies here ? Unless it makes something within
SCMI core better.

Not really, just we can get rid of global pointer which may be need in
system with multiple scmi instances, but that's long way to go.

quoted

quoted

This is something special which is used only when we are returning indexes and
I am not sure if this will have benefit here. I will rather return 0 here.
That's what other drivers are doing.

Indeed had 0 initially but changed as per Juri's suggestion.

Maybe he suggested doing that in the fast switch routine ? As that's the normal
protocol there. Though I have sent a patch today to propose using 0 there as
well (you cc'd).

Yes, saw that. I have changed both to 0 for now. I will watch that
thread and update if necessary before next posting.

quoted

But is 0
treated as failure and still running at current OPP ?

You have used that in the ->get() routine. So the OPP isn't changing, but we are
just trying to fetch it. cpufreq core doesn't do a lot with the value returned
from here, but at one place we break early if 0 is returned. And so all drivers
are returning that.

Agreed, I assumed _INVALID is new thing and changed at both target_indes
and fast_switch.

quoted

and not 0KHz I assume.

Yeah, 0 KHz is dead CPU really :)

:)

quoted

quoted

I suppose any CPU can change the frequency of any other CPU here, right? You
must set policy->dvfs_possible_from_any_cpu = true, from ->init() then.

OK, I missed to see something like that exists, will do.

Fairly recent stuff, present in pm/linux-next only.

Oh OK.

quoted

quoted

quoted

+	/*
+	 * But we need OPP table to function so if it is not there let's
+	 * give platform code chance to provide it for us.
+	 */

How are we getting the OPPs? DT or non DT ?

Non DT :), from the firmware.

I would improve the above comment in that case to clearly say that OPPs are
added by the platform, lets wait for it.

Done

-- 
Regards,
Sudeep