Extending OPP bindings

[Rob Herring]

On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
<lorenzo.pieralisi at arm.com> wrote:
> Hi Nishanth,
>
> On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
>> Hi Sudeep,
>>
>> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
>>
>> >
>> > I am looking into a couple shortcomings in the current OPP bindings and
>> > how to address them. Feel free to add to the list if you think of any more
>> > issues that needs to be addressed or if and how any problem mentioned below
>> > can be handled with the existing bindings.
>> >
>> > 1. indexing: currently there are no indices in the operating-points.
>> indexing is based on frequency which is why the accessors use
>> frequency to pull out the OPP data.
>>
>> indexing is a horrible idea - on platforms where OPP may be disabled
>> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
>> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
>> dts is just a myth that may not exist once the nodes are loaded and
>> operated upon depending on SoC variations (example efuse describing
>> which OPPs can be used and which not).
>
> I do not understand why. As long as a mapping from DT to data structures
> in the kernel is done at boot, I can see how by indexing device nodes
> can refer to specific OPPs. If they are disabled, amen, as long as we
> can point at specific operating points that should be ok.
>
> Indexing does not mean that the index in the DT is the same as the one
> allocated by the OS. Indexing is there to point at specific OPPs from
> different DT nodes, a good example are clock bindings and that's exactly
> how they work.

That is not a good comparison. With clocks, you are describing which
physical signal coming out of a hardware block much like interrupts.
Granted the h/w is not typically documented that way for clocks
although the numbering could correspond to register bit locations as
interrupt numbers do. With OPP indexes, they are a totally made up
software construct.

Perhaps OPPs should be nodes so new fields can be added easily and
then you have a phandle for each OPP.

>
> Can you provide me with an example where the indexing would go wrong
> please ?

Putting s/w indexes into DT. Using cell-index for uarts to define
their tty number was one example.

> Certainly relying on implicit ordering is not great either, actually I
> would say that it is broken.
>
>> That said, the original OPP[1][2] series discussion started by trying
>> to kill indexing precisely for the same reason.. once you have it - it
>> becomes just crazy to deal with.
>
> I could not find any "index killing" :) discussion in there, but I will
> keep reading.
>
>> >     It's assumed that the list is either in ascending or descending
>> >     order of frequency but not explicit in the binding document.
>> >     There are arch/arm/boot/dts/* files with opps in both styles.
>> it should not matter -> opp library should do an insertion sort and
>> organize it in ascending order once all the data is inserted. (line
>> 449ish in opp.c)
>
> That's OS details and they must not be mandated by the bindings.
> We cannot rely on word of mouth for things to work, I do not like
> implicit rules, so the bindings should speficy the ordering or better
> a way to reference OPPs.
>
>> if you see issues with the insertion sort not functioning, it is a bug
>> and should be easy to track down and fix.
>
> Problem is not the insertion sort, problem is that DT bindings do not
> define a way to refer to a specific OPP, unless we do that implicitly
> and again, I rest my case, that is broken.
>
>> >     Few other bindings like thermal defines bindings like
>> >     cooling-{min,max}-state assuming some order which is broken IMO.
>> Now that you bring it up, I missed it :(.. yeah, I might have
>> preferred it to be min frequency and max_frequency - I agree it is
>> probably broken. I'd let Eduardo comment more about it.
>>
>> >
>> >     One such use-case that came up recently[0] is the c-state latencies
>> >     which could be different for each OPP. It would be good if the
>> >     latencies are specified with the indices to OPP table to avoid
>> >     inconsistency between the bindings.
>>
>> You can define C states based on frequencies as well - which really
>> makes sense - since that sounds really like our constraint (say
>> valid-at-frequency "xyz"
>
> I do not think that's generic enough, I do not like the idea of looking
> up frequencies (eg a C-state target_residency does not depend on the
> frequency only - ie voltage and other factors play a role, it really
> depends on an operating point- looking it up by frequency is misleading IMO).

It seems we are trying to fit a square peg into a round hole here.

Rob