Re: [PATCH v4 2/2] dt: power: st: Provide bindings for ST's OPPs

From: Lee Jones
Date: Wed Sep 09 2015 - 09:39:54 EST

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@xxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/