| Message ID | 20150909062722.GA5266@linux |
|---|---|
| State | New |
| Headers | show |
On Wed, 09 Sep 2015, Viresh Kumar wrote: > On 02-09-15, 13:58, Rob Herring wrote: > > What do you expect here? It is your job to close it. Ultimately, this > > will be your problem to deal with. If you have 10 different vendors > > doing selection of OPPs in 10 different ways you will not be able to > > change that easily later. Maybe if you can't come up with something > > common, then this should just not go into DT. You can always look at > > how to do this in a common way and move from the kernel to DT later. > > After getting ranted a bit, here is an real attempt to solve the > problem in a generic way. I hope this can take care of the complexity > of both Qualcomm and ST Microelectronics SoCs. > > @Lee and Stephen: Lemme know if this still wouldn't work :( Thanks for doing this Viresh. I appreciate your efforts. > -------------------------8<------------------------- > From: Viresh Kumar <viresh.kumar@linaro.org> > Date: Wed, 9 Sep 2015 11:47:37 +0530 > Subject: [PATCH] PM / OPP: Add "opp-cuts" and "opp-supply-version" bindings > > For many platforms it is unknown until runtime, about which OPPs should > be used or if used what should be voltage range for the supplies for a > particular frequency. And this mostly depends on the version of the > device or hardware, for which the OPPs are getting used. > > This patch adds two new OPP bindings to get this solved. > > 1. "opp-cuts": The purpose of this binding is to allow the platform to > identify the valid OPPs based on the different levels of versions > with which the hardware is identified. "cuts" is a very specific name for such a generic property. You are using the format I suggested weeks ago, which I like. So how about: opp-hw-version: User defined array containing a hierarchy of version numbers. E.g: Taking kernel version v2.6.19 for instance: <2, 6, 19>; E.g: Representing Major 2 Minor 0, Cuts ALL and Substrate 5: <2, 0, 0xffffffff, 5>; > 2. "opp-supply-name": The purpose of this binding is to allow the > platform to select the voltage range of the power supplies, for a > valid OPP. Did you mean 'opp-supply-version', like in the example below? I suggest '-name' would be better than '-version'. I guess this is a Qcom specific feature. I'll let Stephen comment. > Both of these can be used together, as well as independently. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Documentation/devicetree/bindings/opp/opp.txt | 90 +++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt > index c56a6b1a44ef..def75f7a3614 100644 > --- a/Documentation/devicetree/bindings/opp/opp.txt > +++ b/Documentation/devicetree/bindings/opp/opp.txt > @@ -98,6 +98,13 @@ This describes the OPPs belonging to a device. This node can have following > information. But for multiple power-supplies, this must be set to pass > triplet style values. > > +- opp-supply-version: One or more strings describing version of the supplies on What kind of supplies? Supplies to me means regulator supplies, which I fear would be too easily confused with the regulator '*-supply' property. > + the platform. This is useful for the cases, where the platform wants to select > + the voltage range for supplies at runtime, based on the specific version of > + the hardware. There should be one entry in opp-microvolt array, for each > + string present here. OPPs for the device must be configured, only for a single > + version of the supplies. > + > - status: Marks the OPP table enabled/disabled. > > > @@ -144,6 +151,16 @@ properties. > - opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in > the table should have this. > > +- opp-cuts: Variable length field that contains versions/cuts/substrate of the I'd remove any mention of cuts and substrate versions here. > + hardware for which the OPP is supported. Should contain entry per level of > + version type. For example: a platform with three levels of versions (cuts > + substrate pcode), this field should be like <X Y Z>, where X corresponds to > + different cuts, Y corresponds to different substrates and Z corresponds to > + different pcodes the OPP supports. Each bit of the value corresponds to a s/bit/cell/ > + particular version of the level, and so we can have maximum 32 different > + values of any level. A value of 0xFFFFFFFF means that all the versions of the > + level are supported. See my comments above. > - status: Marks the node enabled/disabled. > > Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together. > @@ -491,3 +508,76 @@ Example 5: Multiple OPP tables > }; > }; > }; > + > +Example 6: OPP selection based on hardware version > +(example: three levels of versions: cuts, substrate and pcode) > + > +/ { > + cpus { > + cpu@0 { > + compatible = "arm,cortex-a7"; > + ... > + > + cpu-supply = <&cpu_supply> > + operating-points-v2 = <&cpu0_opp_table_slow>; > + }; > + }; > + > + opp_table { > + compatible = "operating-points-v2"; > + status = "okay"; > + opp-shared; > + > + opp00 { > + /* > + * Supports all substrate and pcode versions for 0xf > + * cuts, i.e. only first four cuts. > + */ > + opp-cuts = <0xf 0xffffffff 0xffffffff> This does not avoid our issue, as it insists we have an OPP node per permutation. That's (pcodes * cuts * substrate) nodes, which if we support 16 pcodes, 4 cuts and 5 substrates is </me takes shoes and socks off to count> 320 nodes. This IMHO is too many to write/ maintain and is the nucleus of our issue. If we could index into opp-microvolt however (please see below), this would reduce down to (cuts * substrates) nodes, which if taking the example above would only result in a more manageable 20 nodes. > + opp-hz = /bits/ 64 <600000000>; > + ... It's still worth putting the opp-microvolt property in these nodes. > + }; > + > + opp01 { > + /* > + * Supports all substrate and pcode versions for 0x20 > + * cuts, i.e. only the 6th cut. > + */ Not sure what you mean by 6th cut? > + opp-cuts = <0x20 0xffffffff 0xffffffff> > + opp-hz = /bits/ 64 <800000000>; > + ... > + }; > + }; > +}; > + > +Example 7: Multiple voltage-ranges (opp-supply-version) per supply > +(example: device with 2 supplies: vcc0 and vcc1, with two possible ranges of > +voltages: slow and fast) > + > +/ { > + cpus { > + cpu@0 { > + compatible = "arm,cortex-a7"; > + ... > + > + vcc0-supply = <&cpu_supply0>; > + vcc1-supply = <&cpu_supply1>; > + operating-points-v2 = <&cpu0_opp_table>; > + }; > + }; > + > + cpu0_opp_table: opp_table0 { > + compatible = "operating-points-v2"; > + supply-names = "vcc0", "vcc1"; > + opp-supply-version = "slow", "fast"; You've broken your own convention. In the explanation above you say, "There should be one entry in opp-microvolt array, for each string present here." However, you seem to have 4 arrays of values in opp-microvolt below. I guess (supply-names * opp-supply-version) gives you the 4 in your example, but the docs bear no mention of this. Then each of those 4 entries are actually arrays? What are they? Are they user defined? If so, then that's great. In our driver we can choose to index by 'pcode', then our node count gets reduced dramatically and our problems are solved. \o/ > + opp-shared; > + > + opp00 { > + opp-hz = /bits/ 64 <1000000000>; > + opp-microvolt = <900000 915000 925000>, /* Supply vcc0: slow */ > + <910000 925000 935000>, /* Supply vcc1: slow */ > + <970000 975000 985000>, /* Supply vcc0: fast */ > + <960000 965000 975000>; /* Supply vcc1: fast */ > + }; > + }; > +};
On 09-09-15, 08:59, Lee Jones wrote: > Thanks for doing this Viresh. I appreciate your efforts. I wanted to get this sorted out, before we meet face to face :) > > -------------------------8<------------------------- > > From: Viresh Kumar <viresh.kumar@linaro.org> > > Date: Wed, 9 Sep 2015 11:47:37 +0530 > > Subject: [PATCH] PM / OPP: Add "opp-cuts" and "opp-supply-version" bindings > > > > For many platforms it is unknown until runtime, about which OPPs should > > be used or if used what should be voltage range for the supplies for a > > particular frequency. And this mostly depends on the version of the > > device or hardware, for which the OPPs are getting used. > > > > This patch adds two new OPP bindings to get this solved. > > > > 1. "opp-cuts": The purpose of this binding is to allow the platform to > > identify the valid OPPs based on the different levels of versions > > with which the hardware is identified. > > "cuts" is a very specific name for such a generic property. I agree... I wasn't concerned much about naming on the first try and so just wrote it quickly enough to get an overall idea .. > You are using the format I suggested weeks ago, which I like. I must have missed that :( > > So how about: > > opp-hw-version: > User defined array containing a hierarchy of version numbers. > > E.g: Taking kernel version v2.6.19 for instance: > <2, 6, 19>; > > E.g: Representing Major 2 Minor 0, Cuts ALL and Substrate 5: > <2, 0, 0xffffffff, 5>; At least what I suggested in my attempt here is a bit different than what you might be thinking. For example, consider a case with single level of hierarchy, say cuts. And that the SoC has got 10 different cuts, and we name them 0-9. So, the values I was looking to fill to the opp-hw-version field is like: (1 << version_num). So, if an OPP supports version 2, 5 and 7, the value will be 0x000000A4. i.e. with the respective bit positions set. And by this way only 0xffffffff can mean all versions .. > > 2. "opp-supply-name": The purpose of this binding is to allow the > > platform to select the voltage range of the power supplies, for a > > valid OPP. > > Did you mean 'opp-supply-version', like in the example below? Urg. Yeah. > I suggest '-name' would be better than '-version'. So, its not name of the supply really, but a virtual name given to the voltage-range which we need to pick based on the hardware version. > I guess this is a Qcom specific feature. I'll let Stephen comment. No. So, my plan was to use this instead of the st,avs & pcode thing you have got in your bindings. So, instead of 'slow' and 'fast' from my example, it will have the corresponding strings for pcode numbers. And at runtime the platform will pass a string to the OPP library, to activate/add OPPs only for a specific opp-supply-version. Maybe we can name it 'opp-supply-range-name'.. > > Both of these can be used together, as well as independently. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > Documentation/devicetree/bindings/opp/opp.txt | 90 +++++++++++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt > > index c56a6b1a44ef..def75f7a3614 100644 > > --- a/Documentation/devicetree/bindings/opp/opp.txt > > +++ b/Documentation/devicetree/bindings/opp/opp.txt > > @@ -98,6 +98,13 @@ This describes the OPPs belonging to a device. This node can have following > > information. But for multiple power-supplies, this must be set to pass > > triplet style values. > > > > +- opp-supply-version: One or more strings describing version of the supplies on > > What kind of supplies? Supplies to me means regulator supplies, which > I fear would be too easily confused with the regulator '*-supply' > property. Yeah, its more like opp-supply-range-names .. > > + the platform. This is useful for the cases, where the platform wants to select > > + the voltage range for supplies at runtime, based on the specific version of > > + the hardware. There should be one entry in opp-microvolt array, for each > > + string present here. OPPs for the device must be configured, only for a single > > + version of the supplies. > > + > > - status: Marks the OPP table enabled/disabled. > > > > > > @@ -144,6 +151,16 @@ properties. > > - opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in > > the table should have this. > > > > +- opp-cuts: Variable length field that contains versions/cuts/substrate of the > > I'd remove any mention of cuts and substrate versions here. I agree, but probably keep the same in example code to make it simple to understand. > > + hardware for which the OPP is supported. Should contain entry per level of > > + version type. For example: a platform with three levels of versions (cuts > > + substrate pcode), this field should be like <X Y Z>, where X corresponds to > > + different cuts, Y corresponds to different substrates and Z corresponds to > > + different pcodes the OPP supports. Each bit of the value corresponds to a > > s/bit/cell/ No. Its like each bit of the 32 bit cell corresponds ... > > + particular version of the level, and so we can have maximum 32 different > > + values of any level. A value of 0xFFFFFFFF means that all the versions of the > > + level are supported. > > + opp_table { > > + compatible = "operating-points-v2"; > > + status = "okay"; > > + opp-shared; > > + > > + opp00 { > > + /* > > + * Supports all substrate and pcode versions for 0xf > > + * cuts, i.e. only first four cuts. > > + */ > > + opp-cuts = <0xf 0xffffffff 0xffffffff> > > This does not avoid our issue, as it insists we have an OPP node per > permutation. That's (pcodes * cuts * substrate) nodes, which if we > support 16 pcodes, 4 cuts and 5 substrates is </me takes shoes and > socks off to count> 320 nodes. This IMHO is too many to write/ > maintain and is the nucleus of our issue. Not with the bitwise logic I just tried to explain. Obviously we are doing all this to avoid writing so many nodes. > If we could index into opp-microvolt however (please see below), this > would reduce down to (cuts * substrates) nodes, which if taking the > example above would only result in a more manageable 20 nodes. I don't want 20 nodes but only ONE. And in your case, you may only want to use pcode in the opp-supply-range-name property. But its fine if you want to enable/disable some OPPs based on that as well :) > > + opp-hz = /bits/ 64 <600000000>; > > + ... > > It's still worth putting the opp-microvolt property in these nodes. Okay, but I didn't wanted to confuse in the sense that opp-cuts doesn't have anything to do with opp-microvolt.. > > + }; > > + > > + opp01 { > > + /* > > + * Supports all substrate and pcode versions for 0x20 > > + * cuts, i.e. only the 6th cut. > > + */ > > Not sure what you mean by 6th cut? Bit number 6th, i.e. 0x20. > > + opp-cuts = <0x20 0xffffffff 0xffffffff> > > + opp-hz = /bits/ 64 <800000000>; > > + ... > > + }; > > + }; > > +}; > > + > > +Example 7: Multiple voltage-ranges (opp-supply-version) per supply > > +(example: device with 2 supplies: vcc0 and vcc1, with two possible ranges of > > +voltages: slow and fast) > > + > > +/ { > > + cpus { > > + cpu@0 { > > + compatible = "arm,cortex-a7"; > > + ... > > + > > + vcc0-supply = <&cpu_supply0>; > > + vcc1-supply = <&cpu_supply1>; > > + operating-points-v2 = <&cpu0_opp_table>; > > + }; > > + }; > > + > > + cpu0_opp_table: opp_table0 { > > + compatible = "operating-points-v2"; > > + supply-names = "vcc0", "vcc1"; > > + opp-supply-version = "slow", "fast"; > > You've broken your own convention. In the explanation above you say, > "There should be one entry in opp-microvolt array, for each string > present here." However, you seem to have 4 arrays of values in > opp-microvolt below. I guess (supply-names * opp-supply-version) > gives you the 4 in your example, but the docs bear no mention of > this. > > Then each of those 4 entries are actually arrays? What are they? Are > they user defined? If so, then that's great. In our driver we can > choose to index by 'pcode', then our node count gets reduced > dramatically and our problems are solved. \o/ Not really. So this is the logic (I perhaps need to write the paragraph in the bindings in a better way): 1. A regulator's voltage can be supplied as <target> or <target min max> now. 2. For each regulator we need to have an array of size mentioned above. Now this is what I call as ONE entry. For each opp-supply-range-name string, we need a copy of this.. > > + opp-shared; > > + > > + opp00 { > > + opp-hz = /bits/ 64 <1000000000>; > > + opp-microvolt = <900000 915000 925000>, /* Supply vcc0: slow */ > > + <910000 925000 935000>, /* Supply vcc1: slow */ So this is one entry for two regulators in the form <target min max>, and it belongs to the opp-supply-range-name 'slow'. > > + <970000 975000 985000>, /* Supply vcc0: fast */ > > + <960000 965000 975000>; /* Supply vcc1: fast */ And this one is for 'fast'.
On Wed, 09 Sep 2015, Viresh Kumar wrote: > On 09-09-15, 08:59, Lee Jones wrote: > > Thanks for doing this Viresh. I appreciate your efforts. > > I wanted to get this sorted out, before we meet face to face :) > > > > -------------------------8<------------------------- > > > From: Viresh Kumar <viresh.kumar@linaro.org> > > > Date: Wed, 9 Sep 2015 11:47:37 +0530 > > > Subject: [PATCH] PM / OPP: Add "opp-cuts" and "opp-supply-version" bindings > > > > > > For many platforms it is unknown until runtime, about which OPPs should > > > be used or if used what should be voltage range for the supplies for a > > > particular frequency. And this mostly depends on the version of the > > > device or hardware, for which the OPPs are getting used. > > > > > > This patch adds two new OPP bindings to get this solved. > > > > > > 1. "opp-cuts": The purpose of this binding is to allow the platform to > > > identify the valid OPPs based on the different levels of versions > > > with which the hardware is identified. > > > > "cuts" is a very specific name for such a generic property. > > I agree... I wasn't concerned much about naming on the first try and > so just wrote it quickly enough to get an overall idea .. > > > You are using the format I suggested weeks ago, which I like. > > I must have missed that :( > > > So how about: > > > > opp-hw-version: > > User defined array containing a hierarchy of version numbers. > > > > E.g: Taking kernel version v2.6.19 for instance: > > <2, 6, 19>; > > > > E.g: Representing Major 2 Minor 0, Cuts ALL and Substrate 5: > > <2, 0, 0xffffffff, 5>; > > At least what I suggested in my attempt here is a bit different than > what you might be thinking. For example, consider a case with single > level of hierarchy, say cuts. And that the SoC has got 10 different > cuts, and we name them 0-9. So, the values I was looking to fill to > the opp-hw-version field is like: (1 << version_num). So, if an OPP > supports version 2, 5 and 7, the value will be 0x000000A4. i.e. with > the respective bit positions set. And by this way only 0xffffffff can > mean all versions .. Okay, I see what you mean. Sound fine, although only allows up to 31 versions. Not an issue for us I don't think, but could be for other vendors. Taking a recent example, the kernel recently went up to v2.6.39 and some of the stable releases have gone up to v3.4.108. Depends what you wish to support. > > > 2. "opp-supply-name": The purpose of this binding is to allow the > > > platform to select the voltage range of the power supplies, for a > > > valid OPP. > > > > Did you mean 'opp-supply-version', like in the example below? > > Urg. Yeah. > > > I suggest '-name' would be better than '-version'. > > So, its not name of the supply really, but a virtual name given to the > voltage-range which we need to pick based on the hardware version. We usually call these 'names'; reg-names, clock-names, pinctrl-names, phy-names, dma-names, etc. > > I guess this is a Qcom specific feature. I'll let Stephen comment. > > No. So, my plan was to use this instead of the st,avs & pcode thing > you have got in your bindings. So, instead of 'slow' and 'fast' from > my example, it will have the corresponding strings for pcode numbers. > And at runtime the platform will pass a string to the OPP library, to > activate/add OPPs only for a specific opp-supply-version. So you're using it to index into a 2 dimensional array of opp-hz's. Eek! > Maybe we can name it 'opp-supply-range-name'.. > > > > Both of these can be used together, as well as independently. > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > Documentation/devicetree/bindings/opp/opp.txt | 90 +++++++++++++++++++++++++++ > > > 1 file changed, 90 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt > > > index c56a6b1a44ef..def75f7a3614 100644 > > > --- a/Documentation/devicetree/bindings/opp/opp.txt > > > +++ b/Documentation/devicetree/bindings/opp/opp.txt > > > @@ -98,6 +98,13 @@ This describes the OPPs belonging to a device. This node can have following > > > information. But for multiple power-supplies, this must be set to pass > > > triplet style values. > > > > > > +- opp-supply-version: One or more strings describing version of the supplies on > > > > What kind of supplies? Supplies to me means regulator supplies, which > > I fear would be too easily confused with the regulator '*-supply' > > property. > > Yeah, its more like opp-supply-range-names .. > > > > + the platform. This is useful for the cases, where the platform wants to select > > > + the voltage range for supplies at runtime, based on the specific version of > > > + the hardware. There should be one entry in opp-microvolt array, for each > > > + string present here. OPPs for the device must be configured, only for a single > > > + version of the supplies. > > > + > > > - status: Marks the OPP table enabled/disabled. > > > > > > > > > @@ -144,6 +151,16 @@ properties. > > > - opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in > > > the table should have this. > > > > > > +- opp-cuts: Variable length field that contains versions/cuts/substrate of the > > > > I'd remove any mention of cuts and substrate versions here. > > I agree, but probably keep the same in example code to make it simple > to understand. > > > > + hardware for which the OPP is supported. Should contain entry per level of > > > + version type. For example: a platform with three levels of versions (cuts > > > + substrate pcode), this field should be like <X Y Z>, where X corresponds to > > > + different cuts, Y corresponds to different substrates and Z corresponds to > > > + different pcodes the OPP supports. Each bit of the value corresponds to a > > > > s/bit/cell/ > > No. Its like each bit of the 32 bit cell corresponds ... > > > > + particular version of the level, and so we can have maximum 32 different > > > + values of any level. A value of 0xFFFFFFFF means that all the versions of the > > > + level are supported. > > > > + opp_table { > > > + compatible = "operating-points-v2"; > > > + status = "okay"; > > > + opp-shared; > > > + > > > + opp00 { > > > + /* > > > + * Supports all substrate and pcode versions for 0xf > > > + * cuts, i.e. only first four cuts. > > > + */ > > > + opp-cuts = <0xf 0xffffffff 0xffffffff> > > > > This does not avoid our issue, as it insists we have an OPP node per > > permutation. That's (pcodes * cuts * substrate) nodes, which if we > > support 16 pcodes, 4 cuts and 5 substrates is </me takes shoes and > > socks off to count> 320 nodes. This IMHO is too many to write/ > > maintain and is the nucleus of our issue. > > Not with the bitwise logic I just tried to explain. Obviously we are > doing all this to avoid writing so many nodes. > > > If we could index into opp-microvolt however (please see below), this > > would reduce down to (cuts * substrates) nodes, which if taking the > > example above would only result in a more manageable 20 nodes. > > I don't want 20 nodes but only ONE. And in your case, you may only > want to use pcode in the opp-supply-range-name property. But its fine > if you want to enable/disable some OPPs based on that as well :) I'm not seeing how this can be achieved with 1 node. I guess you mean one node per frequency? > > > + opp-hz = /bits/ 64 <600000000>; > > > + ... > > > > It's still worth putting the opp-microvolt property in these nodes. > > Okay, but I didn't wanted to confuse in the sense that opp-cuts > doesn't have anything to do with opp-microvolt.. > > > > + }; > > > + > > > + opp01 { > > > + /* > > > + * Supports all substrate and pcode versions for 0x20 > > > + * cuts, i.e. only the 6th cut. > > > + */ > > > > Not sure what you mean by 6th cut? > > Bit number 6th, i.e. 0x20. Yes, okay. I think we can make the description and examples easier to understand, but I get the premise. > > > + opp-cuts = <0x20 0xffffffff 0xffffffff> > > > + opp-hz = /bits/ 64 <800000000>; > > > + ... > > > + }; > > > + }; > > > +}; > > > + > > > +Example 7: Multiple voltage-ranges (opp-supply-version) per supply > > > +(example: device with 2 supplies: vcc0 and vcc1, with two possible ranges of > > > +voltages: slow and fast) > > > + > > > +/ { > > > + cpus { > > > + cpu@0 { > > > + compatible = "arm,cortex-a7"; > > > + ... > > > + > > > + vcc0-supply = <&cpu_supply0>; > > > + vcc1-supply = <&cpu_supply1>; > > > + operating-points-v2 = <&cpu0_opp_table>; > > > + }; > > > + }; > > > + > > > + cpu0_opp_table: opp_table0 { > > > + compatible = "operating-points-v2"; > > > + supply-names = "vcc0", "vcc1"; > > > + opp-supply-version = "slow", "fast"; > > > + > > > > You've broken your own convention. In the explanation above you say, > > "There should be one entry in opp-microvolt array, for each string > > present here." However, you seem to have 4 arrays of values in > > opp-microvolt below. I guess (supply-names * opp-supply-version) > > gives you the 4 in your example, but the docs bear no mention of > > this. > > > > Then each of those 4 entries are actually arrays? What are they? Are > > they user defined? If so, then that's great. In our driver we can > > choose to index by 'pcode', then our node count gets reduced > > dramatically and our problems are solved. \o/ > > Not really. So this is the logic (I perhaps need to write the > paragraph in the bindings in a better way): > 1. A regulator's voltage can be supplied as <target> or <target min max> now. Understood. I don't think we'll be using that, but if it's useful to others, then fine. > 2. For each regulator we need to have an array of size mentioned above. Using 2 properties to index in a 2D array like this look scarily like it'll be prone to all sorts of fumbling errors. The complexity of all this will reduce massively by having a separate node per <regulator>-supply. Using the same nodes for this is squeezing too much into a single node. I was put off as soon as I saw you using 2D arrays in DT. > Now this is what I call as ONE entry. > > For each opp-supply-range-name string, we need a copy of this.. Fortunately for us we'll only have single celled opp-microvolt properties. > > > + opp-shared; > > > + > > > + opp00 { > > > + opp-hz = /bits/ 64 <1000000000>; > > > + opp-microvolt = <900000 915000 925000>, /* Supply vcc0: slow */ > > > + <910000 925000 935000>, /* Supply vcc1: slow */ > > So this is one entry for two regulators in the form <target min max>, and it > belongs to the opp-supply-range-name 'slow'. > > > > + <970000 975000 985000>, /* Supply vcc0: fast */ > > > + <960000 965000 975000>; /* Supply vcc1: fast */ > > And this one is for 'fast'. So I think our offering would look like this: cpus { cpu@0 { compatible = "arm,cortex-a7"; vcc-supply = <&cpu_supply0>; operating-points-v2 = <&cpu0_opp_table>; }; }; cpu0_opp_table: opp_table0 { compatible = "operating-points-v2"; opp-microvolt-names = "1", "2", "3", "4", "5", "6", "7", "8" "9", "10", "11", "12", "13", "14", "15", "16"; opp0 { /* Major Minor Substrate */ /* all all all */ opp-supported-hw = <0xffffffff 0xffffffff 0xffffffff> opp-hz = <1000000000>; opp-microvolt = <900000>, <915000>, <915000>, <925000>, <925000>, <925000>, <935000>, <945000>, <945000>, <945000>, <945000>, <955000>, <956000>, <975000>, <975000>, <975000>, }; opp1 { /* Major Minor Substrate */ /* 2 1 & 2 all */ opp-supported-hw = <0x2 0x3 0xffffffff> opp-hz = <1100000000>; opp-microvolt = <900000>, <915000>, <915000>, <925000>, <925000>, <925000>, <935000>, <945000>, <945000>, <945000>, <945000>, <955000>, <956000>, <975000>, <975000>, <975000>, }; opp2 { /* Major Minor Substrate */ /* 2 5 4 & 5 & 6 */ opp-supported-hw = <0x2 0x10 0x38> opp-hz = <1200000000>; opp-microvolt = <900000>, <915000>, <915000>, <925000>, <925000>, <925000>, <935000>, <945000>, <945000>, <945000>, <945000>, <955000>, <956000>, <975000>, <975000>, <975000>, }; }; Or have I got the wrong end of the stick? NB: Note the suggested new property names.
On 09-09-15, 14:39, Lee Jones wrote: > Okay, I see what you mean. Sound fine, although only allows up to 31 > versions. Not an issue for us I don't think, but could be for other > vendors. Taking a recent example, the kernel recently went up to > v2.6.39 and some of the stable releases have gone up to v3.4.108. > Depends what you wish to support. Oh, that is always expandable. No one is stopping a platform to have hw-versions as: cuts_part1 cuts_part2 cuts_part3, and that will give us 96 bits :) > > So, its not name of the supply really, but a virtual name given to the > > voltage-range which we need to pick based on the hardware version. > > We usually call these 'names'; reg-names, clock-names, pinctrl-names, > phy-names, dma-names, etc. Probably (opp-)supply-range-name suits well. > > > I guess this is a Qcom specific feature. I'll let Stephen comment. > > > > No. So, my plan was to use this instead of the st,avs & pcode thing > > you have got in your bindings. So, instead of 'slow' and 'fast' from > > my example, it will have the corresponding strings for pcode numbers. > > And at runtime the platform will pass a string to the OPP library, to > > activate/add OPPs only for a specific opp-supply-version. > > So you're using it to index into a 2 dimensional array of opp-hz's. s/opp-hz's/opp-microvolt > > Eek! :) > > I don't want 20 nodes but only ONE. And in your case, you may only > > want to use pcode in the opp-supply-range-name property. But its fine > > if you want to enable/disable some OPPs based on that as well :) > > I'm not seeing how this can be achieved with 1 node. > > I guess you mean one node per frequency? Within an OPP table, OPP-nodes must have unique frequencies. i.e. two nodes can't have same frequency. In simple terms (mapping to your case) it is going to be like: opp-table will have this: opp-supply-range-names = "avs1", "avs2", ... , "avsn"; Each OPP node will have opp-microvolt = <AVS1-volt>, <AVS2-volt>, <AVS3-volt> ... <AVSN-volt>; The platform with tell opp core to use avsX and OPP core will take care of rest. > > Not really. So this is the logic (I perhaps need to write the > > paragraph in the bindings in a better way): > > 1. A regulator's voltage can be supplied as <target> or <target min max> now. > > Understood. I don't think we'll be using that, but if it's useful to > others, then fine. Bindings for this are already present in kernel, so this wasn't new. > > 2. For each regulator we need to have an array of size mentioned above. > > Using 2 properties to index in a 2D array like this look scarily like > it'll be prone to all sorts of fumbling errors. > > The complexity of all this will reduce massively by having a separate > node per <regulator>-supply. Using the same nodes for this is > squeezing too much into a single node. I was put off as soon as I saw > you using 2D arrays in DT. So for the pcode thing, probably a separate entry like: opp-microvolt-pcode0 can be added to make things easy. opp-microvolts bindings are already added to support multiple regulators, and I don't really want to touch that again :) > > Now this is what I call as ONE entry. > > > > For each opp-supply-range-name string, we need a copy of this.. > > Fortunately for us we'll only have single celled opp-microvolt > properties. Haha. FWIW, you can't use voltage-tolerance with OPP-v2 bindings as the triplets have replaced it. Just in case you were planning to use that :) > So I think our offering would look like this: > > cpus { > cpu@0 { > compatible = "arm,cortex-a7"; > vcc-supply = <&cpu_supply0>; > operating-points-v2 = <&cpu0_opp_table>; > }; > }; > > cpu0_opp_table: opp_table0 { > compatible = "operating-points-v2"; > opp-microvolt-names = "1", "2", "3", "4", "5", "6", "7", "8" > "9", "10", "11", "12", "13", "14", "15", "16"; > > opp0 { > /* Major Minor Substrate */ > /* all all all */ > opp-supported-hw = <0xffffffff 0xffffffff 0xffffffff> > opp-hz = <1000000000>; > opp-microvolt = <900000>, <915000>, <915000>, <925000>, > <925000>, <925000>, <935000>, <945000>, > <945000>, <945000>, <945000>, <955000>, > <956000>, <975000>, <975000>, <975000>, > }; So this is based of the solution I proposed. But if we were to choose the one you gave, it will be: opp-microvolt-1 = <900000>; opp-microvolt-2 = <915000>; opp-microvolt-3 = <915000>; opp-microvolt-4 = <925000>; opp-microvolt-5 = <925000>; opp-microvolt-6 = <925000>; opp-microvolt-7 = <935000>; opp-microvolt-8 = <945000>; opp-microvolt-9 = <945000>; opp-microvolt-10 = <945000>; opp-microvolt-11 = <945000>; opp-microvolt-12 = <955000>; opp-microvolt-13 = <956000>; opp-microvolt-14 = <975000>; opp-microvolt-15 = <975000>; opp-microvolt-16 = <975000>; > opp1 { > /* Major Minor Substrate */ > /* 2 1 & 2 all */ > opp-supported-hw = <0x2 0x3 0xffffffff> > opp-hz = <1100000000>; > opp-microvolt = <900000>, <915000>, <915000>, <925000>, > <925000>, <925000>, <935000>, <945000>, > <945000>, <945000>, <945000>, <955000>, > <956000>, <975000>, <975000>, <975000>, > }; > > opp2 { > /* Major Minor Substrate */ > /* 2 5 4 & 5 & 6 */ > opp-supported-hw = <0x2 0x10 0x38> > opp-hz = <1200000000>; > opp-microvolt = <900000>, <915000>, <915000>, <925000>, > <925000>, <925000>, <935000>, <945000>, > <945000>, <945000>, <945000>, <955000>, > <956000>, <975000>, <975000>, <975000>, > }; > }; > > Or have I got the wrong end of the stick? > > NB: Note the suggested new property names. Yeah, all looks fine to me.
> > Or have I got the wrong end of the stick? > > > > NB: Note the suggested new property names. > > Yeah, all looks fine to me. I think these names are better: opp-supply-range-name => opp-microvolt-names opp-cuts => opp-supported-hw Apart from that, the binding is starting to come together. Let's see what Rob and Stephen have to say about it.
On 09/09/2015 11:36 AM, Lee Jones wrote: >>> Or have I got the wrong end of the stick? >>> >>> NB: Note the suggested new property names. >> >> Yeah, all looks fine to me. > > I think these names are better: > > opp-supply-range-name => opp-microvolt-names > opp-cuts => opp-supported-hw > > Apart from that, the binding is starting to come together. > > Let's see what Rob and Stephen have to say about it. Seems reasonable to me. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09, Rob Herring wrote: > On 09/09/2015 11:36 AM, Lee Jones wrote: > >>> Or have I got the wrong end of the stick? > >>> > >>> NB: Note the suggested new property names. > >> > >> Yeah, all looks fine to me. > > > > I think these names are better: > > > > opp-supply-range-name => opp-microvolt-names > > opp-cuts => opp-supported-hw > > > > Apart from that, the binding is starting to come together. > > > > Let's see what Rob and Stephen have to say about it. > > Seems reasonable to me. I think it will work for qcom use cases. We can collapse the tables down to one node and have speed bin and version as the opp-supported-hw property. The opp-microvolt-names property would be where we put the different voltage bins. What about the other properties like opp-microamp or opp-suspend? Will all of those also get *-names properties to index into them based on some string? I don't actually need those for my devices, but I'm just pointing it out in case someone else wants to compress tables but they have different microamps or clock latencies, etc. Finally, does this mean we will get rid of operating-points-names?
On 09-09-15, 17:57, Stephen Boyd wrote: > I think it will work for qcom use cases. Thanks for the Rant Rob, it finally got me moving :) > We can collapse the > tables down to one node and have speed bin and version as the > opp-supported-hw property. The opp-microvolt-names property would I am probably going to remove opp-microvolt-names property as well, if we are going to use separate entries for all voltage ranges in OPP node. i.e. two voltage ranges, slow and fast, like this: regulator A regulator B opp-microvolt-slow = <tarA minA maxA>, <tarB minB maxB>; opp-microvolt-fast = <tarA minA maxA>, <tarB minB maxB>; > be where we put the different voltage bins. What about the other > properties like opp-microamp or opp-suspend? Will all of those Lets keep them as is for now, unless we have a real user. > also get *-names properties to index into them based on some > string? I don't actually need those for my devices, but I'm just > pointing it out in case someone else wants to compress tables but > they have different microamps or clock latencies, etc. > > Finally, does this mean we will get rid of operating-points-names? That's the next thing I wanted to ask from Rob. We are surely not going to use them and there are no users or kernel code to support them today. Can we get rid of them from the DT ?
On Thu, 10 Sep 2015, Viresh Kumar wrote: > On 09-09-15, 17:57, Stephen Boyd wrote: > > I think it will work for qcom use cases. > > Thanks for the Rant Rob, it finally got me moving :) > > > We can collapse the > > tables down to one node and have speed bin and version as the > > opp-supported-hw property. The opp-microvolt-names property would > > I am probably going to remove opp-microvolt-names property as well, if > we are going to use separate entries for all voltage ranges in OPP > node. i.e. two voltage ranges, slow and fast, like this: > > regulator A regulator B > opp-microvolt-slow = <tarA minA maxA>, <tarB minB maxB>; > opp-microvolt-fast = <tarA minA maxA>, <tarB minB maxB>; > > > be where we put the different voltage bins. What about the other > > properties like opp-microamp or opp-suspend? Will all of those > > Lets keep them as is for now, unless we have a real user. > > > also get *-names properties to index into them based on some > > string? I don't actually need those for my devices, but I'm just > > pointing it out in case someone else wants to compress tables but > > they have different microamps or clock latencies, etc. > > > > Finally, does this mean we will get rid of operating-points-names? > > That's the next thing I wanted to ask from Rob. We are surely not > going to use them and there are no users or kernel code to support > them today. Can we get rid of them from the DT ? I think you answered your own question. No users == !ABI == Strip it out.
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index c56a6b1a44ef..def75f7a3614 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -98,6 +98,13 @@ This describes the OPPs belonging to a device. This node can have following information. But for multiple power-supplies, this must be set to pass triplet style values. +- opp-supply-version: One or more strings describing version of the supplies on + the platform. This is useful for the cases, where the platform wants to select + the voltage range for supplies at runtime, based on the specific version of + the hardware. There should be one entry in opp-microvolt array, for each + string present here. OPPs for the device must be configured, only for a single + version of the supplies. + - status: Marks the OPP table enabled/disabled. @@ -144,6 +151,16 @@ properties. - opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in the table should have this. +- opp-cuts: Variable length field that contains versions/cuts/substrate of the + hardware for which the OPP is supported. Should contain entry per level of + version type. For example: a platform with three levels of versions (cuts + substrate pcode), this field should be like <X Y Z>, where X corresponds to + different cuts, Y corresponds to different substrates and Z corresponds to + different pcodes the OPP supports. Each bit of the value corresponds to a + particular version of the level, and so we can have maximum 32 different + values of any level. A value of 0xFFFFFFFF means that all the versions of the + level are supported. + - status: Marks the node enabled/disabled. Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together. @@ -491,3 +508,76 @@ Example 5: Multiple OPP tables }; }; }; + +Example 6: OPP selection based on hardware version +(example: three levels of versions: cuts, substrate and pcode) + +/ { + cpus { + cpu@0 { + compatible = "arm,cortex-a7"; + ... + + cpu-supply = <&cpu_supply> + operating-points-v2 = <&cpu0_opp_table_slow>; + }; + }; + + opp_table { + compatible = "operating-points-v2"; + status = "okay"; + opp-shared; + + opp00 { + /* + * Supports all substrate and pcode versions for 0xf + * cuts, i.e. only first four cuts. + */ + opp-cuts = <0xf 0xffffffff 0xffffffff> + opp-hz = /bits/ 64 <600000000>; + ... + }; + + opp01 { + /* + * Supports all substrate and pcode versions for 0x20 + * cuts, i.e. only the 6th cut. + */ + opp-cuts = <0x20 0xffffffff 0xffffffff> + opp-hz = /bits/ 64 <800000000>; + ... + }; + }; +}; + +Example 7: Multiple voltage-ranges (opp-supply-version) per supply +(example: device with 2 supplies: vcc0 and vcc1, with two possible ranges of +voltages: slow and fast) + +/ { + cpus { + cpu@0 { + compatible = "arm,cortex-a7"; + ... + + vcc0-supply = <&cpu_supply0>; + vcc1-supply = <&cpu_supply1>; + operating-points-v2 = <&cpu0_opp_table>; + }; + }; + + cpu0_opp_table: opp_table0 { + compatible = "operating-points-v2"; + supply-names = "vcc0", "vcc1"; + opp-supply-version = "slow", "fast"; + opp-shared; + + opp00 { + opp-hz = /bits/ 64 <1000000000>; + opp-microvolt = <900000 915000 925000>, /* Supply vcc0: slow */ + <910000 925000 935000>, /* Supply vcc1: slow */ + <970000 975000 985000>, /* Supply vcc0: fast */ + <960000 965000 975000>; /* Supply vcc1: fast */ + }; + }; +};