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

[Viresh Kumar <viresh.kumar@linaro.org>]

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.

-- 
viresh