On 05/30/2014 03:45 PM, Rob Herring wrote: > On Fri, May 30, 2014 at 3:33 PM, Nishanth Menon <nm@xxxxxx> wrote: >> On 05/30/2014 03:19 PM, Tomasz Figa wrote: >>> On 30.05.2014 22:13, Nishanth Menon wrote: >>>> On 05/30/2014 03:02 PM, Tomasz Figa wrote: >>>>> On 30.05.2014 21:50, Nishanth Menon wrote: >>>>>> On 05/30/2014 01:55 PM, Mark Rutland wrote: >>>>>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote: >>>>>>>> Hi Mark, >>>>>>>> >>>>>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> Apologies for being somewhat late w.r.t. review on this. >>>>>>>>> [...] >>>>>>>>> Why are these in both operating-points and boost-frequencies? It'll be >>>>>>>>> really easy to accidentally forget to mark something as a >>>>>>>>> boost-frequency this way. Why not have a boost-points instead? >>>>>>>> >>>>>>>> Does boost-points mean a set of clock speeds which are not listed as >>>>>>>> part of operating-points property? If yes, that also is a possible >>>>>>>> implementation (it was implemented in one of the earlier version of >>>>>>>> this series). Could you confirm that you want the boost mode speeds to >>>>>>>> be exclusive of the speeds listed in operating-points? >>>>>>> >>>>>>> If these boost mode operating points are not generally advisable for use >>>>>>> as the other operating-points are, then they should IMO been in an >>>>>>> entirely separate property exclusive of (but in the same format as) the >>>>>>> operating-points property, e.g. >>>>>>> >>>>>>> operating points = <A B>, <C D>; >>>>>>> boost-points = <E F>; >>>>>> >>>>>> you are asking boost frequencies to remain for ever tied down to OPP >>>>>> style description. >>>>>> >>>>>> What we are trying to describe? "What are my SoC's overclocked >>>>>> frequencies"? That description can be used even in a system that does >>>>>> not use OPP style table (say ACPI based OPP tables or whatever >>>>>> acronymned table). >>>>>> >>>>>> Tying it down to operating points just because we have it today as a >>>>>> convenient description, is limiting. >>>>>> >>>>>> Further, if we decide to educate boost-frequencies to also indicate >>>>>> how long is it safe? That does indeed belong to boost-frequency >>>>>> description and not OPP description. Or if we decide to change >>>>>> operating-points description[1] in the future has an impact on >>>>>> "boost-points" description, when it should not have. >>>>>> >>>>>>> >>>>>>> Otherwise, without boost-mode support we have to parse the boost mode >>>>>>> table to figure out which points to avoid. Or if someone typos a value >>>>>> That is OS usage of h/w description - yeah - in anycase, if OS has no >>>>>> ability to deal with boost-frequencies, it should skip it for sure. >>>>>> >>>>>>> in either table we might go into a boost mode when we didn't want to! >>>>>>> >>>>>> There are other ways to screw up device with dts typo. you could give >>>>>> a wrong voltage(extra 0?) and ensure you never use the chip ever >>>>>> again.. typos are dt bugs, we can do the best to write robust code to >>>>>> report them. >>>>>> >>>>> >>>>> Typos are not the primary thing to worry about here. Adding boost >>>>> frequencies to the list of primary operating points is flawed, because >>>>> an OS that has no idea of boost mode will use them as normal operating >>>>> points and this is not desired. >>>> >>>> That means we have an implementation bug in OS since it does not >>>> consider the complete hardware description that device tree is >>>> providing. An analogy will be a regulator compatible match being used >>>> but regulator-min-microvolt and regulator-max-microvolt being ignored >>>> by OS. >>> >>> No. The operating points bindings were defined far before this >>> boost-frequency and so there is no requirement to support the latter. >> >> So, we dont add any new bindings ever again? /me blinks. *IF* we add a >> new property in the future, do we expect the new description to be >> supported in older kernel(which could not have known about it)? How >> far are we taking this ABI thing? >> Documentation/devicetree/bindings/ABI.txt states: >> 3) Bindings can be augmented, but the driver shouldn't break when given >> the old binding. ie. add additional properties, but don't change the >> meaning of an existing property. For drivers, default to the original >> behaviour when a newly added property is missing. >> >> we are not changing the meaning of existing property, we are >> augumenting it. > > You are changing the meaning of entries in that they can have > additional data which changes their properties. > > If we accept the DT changes (as DT maintainers) and reject the kernel > changes (as kernel maintainers), you would be left with a broken > system. That is true and I agree. > >> In my opinion, *IF* we are concerned about polluting operating-points >> description, why dont we enforce that the boost operating points >> should NOT be defined in the current "operating-points" description - >> and - just follow what Rob suggested and iMx already does - add such >> operating points from platform code. > > I believe I also said on this and other attempts to bandage up the > existing OPP binding, that we should not change/append it, but define > a new binding for OPPs that addresses this and other issues. > Otherwise, I'm going to just NAK every incomplete OPP binding bandaid. Are we open to creating a completely mutually-exclusive binding and maintain the legacy one as "legacy support without any modification" while we debate the new binding? I dont deny that we have more bandage discussions that we'd like on these bindings, So, would like views if we want to define a better one from scratch rather than continue to figure out how to live with existing one? Here are the options I can think of. 1. New binding: Lets say each OPP as phandles? -> that sounds like the closest I have heard as something that remotely makes sense to scale properties? 2. Remove OPP bindings entirely - only way to add/modify OPP is from platform code. Anyone has better suggestions what we can do? -- Regards, Nishanth Menon -- 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
