Re: [PATCH V7 2/3] OPP: Allow multiple OPP tables to be passed via DT

[Stephen Boyd <sboyd@codeaurora.org>]

On 06/17/2015 06:47 AM, Rob Herring wrote:
> On Wed, Jun 17, 2015 at 8:33 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 17-06-15, 08:23, Rob Herring wrote:
>>>> +                       operating-points-v2 = <&cpu0_opp_table_slow>, <&cpu0_opp_table_fast>;
>>> You've made a fundamental change here in that this can now be a list
>>> of phandles. There should be some description on what a list means
>>> (merge the tables?, select one?).
>> Did you miss the description I wrote few lines earlier or are you
>> asking for something else? This is what I wrote earlier:
>>
>>>> +Devices may want to choose OPP tables at runtime and so can provide a list of
>>>> +phandles here. But only *one* of them should be chosen at runtime.
>> So, clearly only ONE of the tables should be used.
> Yes, never mind.
>
>>> I think this needs to have a defined order and the platform should
>>> know what that is. For example, if you read the efuses and decide you
>>> need the "slow" table, you know to pick the first entry. Then you
>>> don't need opp-name. Does that work for QCom?
>> Why forcing on the order here? For example, consider a case where the
>> platform can have four tables, A B C D. Now DT is free to pass all
>> four or just a subset of those. Like, for some boards table B doesn't
>> stand valid. And so it may wanna pass just A C D. And so keeping these
>> tables in order is going to break for sure. Flexibility is probably
>> better in this case.
> Defined order is a key part of DT bindings. We solve the variable
> length problem with name lists associated with variable length
> property like:
>
> operating-point-names = "slow", "fast";
>
> I'm not a fan of doing this if we can avoid it, but we should at least
> follow the same pattern. Don't send me a patch with that yet, I want
> to hear from Stephen.
>
> You can also use "status" to disable specific tables rather than
> removing from the list.
>

An operating-point(s?)-names property seems ok... but doesn't that mean
that every CPU that uses the OPP has to have the same list of
operating-point-names? It would make sense to me if the operating points
were called something different depending on *which* CPU is using them,
but in this case the only name for the operating point is "slow" or
"fast", etc.

In reality we've assigned them names like speedX-binY-vZ so that we know
which speed bin, voltage bin, and version they're part of. Maybe OPP
node properties like qcom,speed-bin = <u32>, qcom,pvs-bin = <u32>, etc.
would be better? It would make the parsing code slightly more
complicated because it needs to look for 2 or 3 properties instead of a
name property though. Or would having different node names be
acceptable? That would avoid this list of strings.

At the least, operating-points-names will be required on qcom platforms.
A fixed ordering known to the platform would mean that we know exactly
how many voltage bins and speed bins and how many voltage bins per speed
bin are used for a particular SoC, which we've avoided knowing so far.

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