On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html