Re: [PATCH V8 3/3] OPP: Allow "opp-hz" and "opp-microvolt" to contain magic values

[Stephen Boyd]

On 01/08, Viresh Kumar wrote:
> On 05-01-18, 14:19, Stephen Boyd wrote:
> > On 12/29, Viresh Kumar wrote:
> 
> > Could you please point to Kevin's comments and also include the
> 
> https://lkml.kernel.org/r/m2r30i4w35.fsf@xxxxxxxxxxxx

Ok. That thread was months ago. Shocked I can't remember!

My read of Kevin's comments lead me to think he's saying that a
generic 'domain-performance-state' property is worse than putting
the numbers directly inside of the opp table with a comment above
it. Now that's all fine, but now that we have required-opps
binding we sort of have the domain-performance-state property
again, but it's a phandle instead of a raw state number.

So we have

	required-opps = <&perf_state>;

but what was proposed before was

	domain-performance-state = <1>;

or Kevin's 

	opp-table = <100000 1>;

>From what I can tell, the domain-performance-state proposal was
at the same abstraction level as the OPP itself, but now that's
moved to be inside of the power domain OPPs. So now we're talking
about describing some power domains performance levels with the
OPP binding, for a power domain, not describing a performance
state for some unknown power domain that's associated with a
device's OPP table.

The power domain is not running at any sort of frequency for us.
It's using some particular voltage, but we may or may not know
what that voltage is depending on the platform. This makes me see
it as 

	power_domain_opp {

	    low: point0 {
		opp-microvolt = <950000>
	    };

	    medium: point1 {
		opp-microvolt = <975000>
	    };

	    high: point2 {
		opp-microvolt = <1000000>
	    };
	};

for some sort of power domain that deals with voltages. This
makes perfect sense. The power domain needs to be at some voltage
and the binding says that. Honestly, the whole required-opps
thing works great here and it's all wonderful, and it looks like
can even be used for other OPP linking in the future, like
connecting frequencies between CPUs and caches.

Where it starts to break down is when the voltage isn't known to
the user, just some number that we've agreed means "low",
"medium", "high" or whatever. Again, the binding looks similar:


	power_domain_opp {
	    low: point0 {
	    	qcom,corner = <0>;
	    };

	    medium: point1 {
	    	qcom,corner = <10>;
	    };

	    high: point2 {
	    	qcom,corner = <250>;
	    };
	};

but now the number is only meaningful to the power domain driver.
What we really care about is associating some firmware specific
information via the phandle to some frequency that a device is
using. Behind the scenes of the firmware, that number is really
being translated into some voltage, like opp-microvolt =
<950000>, but we don't know what that is and it could vary at
runtime.

It all feels really close, and it totally works for the non-magic
value parts that we have to deal with, but I'm not convinced that
we should stick the firmware specific information into some
generic OPP property just so we don't have to review the binding
again. Hopefully there's some other reason why we shouldn't come
up with a firmware specific binding for this piece of
information.

> > >          * If we choose to use platform-specific bindings instead of opp-hz,
> > >          * then only this routine requires to change to read the DT and provide
> > >          * the value from platform specific binding.
> > 
> > If we wanted to change this function to do a platform specific
> > thing, will we somehow get a way to access the DT node of the opp
> > passed into this function?
> 
> Yes, we can do that. The OPP core already stores pointer to the node
> in the OPP structure, we will just need another API to expose that.

Great!

> 
> > Also, I don't see how the foo_get_performance function will use
> > the genpd pointer passed here. Maybe that's never used?
> 
> It depends actually and I think its better to pass it anyway. What if
> a single driver is handling multiple genpds and wants to do things
> differently for them? It should know which genpd it is called for,
> looks like  basic requirement to me.

Ok, fair enough.

> 
> > Finally, I would think that a "getter" like this function would
> > be informing the framework about what the current performance
> > state is, not translating an OPP into a performance state. So the
> > whole thing looks like a misnomer, and probably should be called
> > something like xlate_opp_performance.
> 
> Sure, I can name it anything.

Alright. Sounds like my read of the code is correct.

> 
> So the question still stands, are we all okay for using magic values
> or we want platform specific properties ?
> 

Kevin?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project