[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

Cc: devicetree at vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---

Changelog:
 - Using OPP-v2
 - Moved (and linked) a bunch of documentation to ../power/

 .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
new file mode 100644
index 0000000..79add9d
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
@@ -0,0 +1,40 @@
+Binding for ST's CPUFreq driver
+===============================
+
+Frequency Scaling only
+----------------------
+
+Located in CPU's node:
+
+- operating-points		: [See: ../power/opp.txt]
+
+Example [safe]
+--------------
+
+cpus {
+	cpu at 0 {
+				 /* kHz     uV   */
+		operating-points = <1500000 0
+				    1200000 0
+				    800000  0
+				    500000  0>;
+	};
+};
+
+Dynamic Voltage and Frequency Scaling (DVFS)
+--------------------------------------------
+
+Located in CPU's node:
+
+- operating-points-v2-sti	: [See ../power/opp-st.txt]
+
+Example [unsafe]
+----------------
+
+cpus {
+	cpu at 0 {
+		operating-points-v2	= <[OPP list phandle]>;
+	};
+};
+
+For an example of an OPP list, see ../power/opp-st.txt.
-- 
1.9.1

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

[Lee Jones]

These OPPs are used in ST's CPUFreq implementation.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---

Changelog:
 - None, new patch

 Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt

diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
new file mode 100644
index 0000000..6eb2a91
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/opp-st.txt
@@ -0,0 +1,76 @@
+STMicroelectronics OPP (Operating Performance Points) Bindings
+--------------------------------------------------------------
+
+Frequency Scaling only
+----------------------
+
+Located in CPU's node:
+
+- operating-points	: [See: ./opp.txt]
+
+Example [safe]
+--------------
+
+cpus {
+	cpu at 0 {
+				 /* kHz     uV   */
+		operating-points = <1500000 0
+				    1200000 0
+				    800000  0
+				    500000  0>;
+	};
+};
+
+Dynamic Voltage and Frequency Scaling (DVFS)
+--------------------------------------------
+
+Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
+
+- compatible		: Should be "operating-points-v2-sti"
+- opp{1..N} 		: Each 'oppX' subnode will contain the following properties:
+ - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
+ - st,avs		: List of available voltages [uV] indexed by process code
+ - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
+ - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
+- st,syscfg		: Phandle to Major number register
+				First cell: offset to major number
+- st,syscfg-eng		: Phandle to Minor number and Pcode registers
+				First cell: offset to process code
+				Second cell: offset to minor number
+
+WARNING: The opp{1..N} nodes will be provided by the bootloader.  Do not attempt to
+	 artificially synthesise the opp{1..N} nodes or any of their descendants.
+	 They are very platform specific and may damage the hardware if created
+	 incorrectly.
+
+Example [unsafe]
+----------------
+
+cpus {
+	cpu at 0 {
+		operating-points-v2	= <&cpu0_opp_list>;
+	};
+};
+
+/* ############################################################ */
+/* # WARNING: Do not attempt to copy/replicate this node,     # */
+/* #          it is only to be supplied by the bootloader !!! # */
+/* ############################################################ */
+cpu0-opp-list {
+	compatible	= "operating-points-v2-sti";
+	st,syscfg	= <&syscfg [major_offset]>;
+	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
+
+	opp0 {
+		opp-hz		= <1200000000>;
+		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
+		st,substrate	= <0xff>;
+		st,cuts		= <0xff>;
+	};
+	opp1 {
+		opp-hz		= <1500000000>;
+		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
+		st,substrate	= <0xff>;
+		st,cuts		= <0x2>;
+	};
+};
-- 
1.9.1

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

[Viresh Kumar]

Cc'ing few people (whom I cc'd last time as well :)).

On 27-07-15, 16:20, Lee Jones wrote:
> These OPPs are used in ST's CPUFreq implementation.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> 
> Changelog:
>  - None, new patch
> 
>  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> new file mode 100644
> index 0000000..6eb2a91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> @@ -0,0 +1,76 @@
> +STMicroelectronics OPP (Operating Performance Points) Bindings
> +--------------------------------------------------------------
> +
> +Frequency Scaling only
> +----------------------
> +
> +Located in CPU's node:
> +
> +- operating-points	: [See: ./opp.txt]
> +
> +Example [safe]
> +--------------
> +
> +cpus {
> +	cpu at 0 {
> +				 /* kHz     uV   */
> +		operating-points = <1500000 0
> +				    1200000 0
> +				    800000  0
> +				    500000  0>;
> +	};
> +};
> +
> +Dynamic Voltage and Frequency Scaling (DVFS)
> +--------------------------------------------
> +
> +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> +
> +- compatible		: Should be "operating-points-v2-sti"
> +- opp{1..N} 		: Each 'oppX' subnode will contain the following properties:

Or should we mention:
- opp{1..N} 		: Each 'oppX' subnode shall contain below properties,
                          over what ./opp.txt defines:

?


> + - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
> + - st,avs		: List of available voltages [uV] indexed by process code
> + - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
> + - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
> +- st,syscfg		: Phandle to Major number register
> +				First cell: offset to major number
> +- st,syscfg-eng		: Phandle to Minor number and Pcode registers
> +				First cell: offset to process code
> +				Second cell: offset to minor number
> +
> +WARNING: The opp{1..N} nodes will be provided by the bootloader.  Do not attempt to
> +	 artificially synthesise the opp{1..N} nodes or any of their descendants.
> +	 They are very platform specific and may damage the hardware if created
> +	 incorrectly.
> +
> +Example [unsafe]
> +----------------
> +
> +cpus {
> +	cpu at 0 {
> +		operating-points-v2	= <&cpu0_opp_list>;
> +	};
> +};
> +
> +/* ############################################################ */
> +/* # WARNING: Do not attempt to copy/replicate this node,     # */
> +/* #          it is only to be supplied by the bootloader !!! # */
> +/* ############################################################ */
> +cpu0-opp-list {
> +	compatible	= "operating-points-v2-sti";
> +	st,syscfg	= <&syscfg [major_offset]>;
> +	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
> +
> +	opp0 {
> +		opp-hz		= <1200000000>;
> +		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
> +		st,substrate	= <0xff>;
> +		st,cuts		= <0xff>;
> +	};
> +	opp1 {
> +		opp-hz		= <1500000000>;
> +		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
> +		st,substrate	= <0xff>;
> +		st,cuts		= <0x2>;
> +	};
> +};

I don't see more problems here, unless we can move some of this to the
generic bindings.

@Rob/Stephen: Please respond before it is late :)

-- 
viresh

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

[Lee Jones]

On Tue, 28 Jul 2015, Viresh Kumar wrote:

> Cc'ing few people (whom I cc'd last time as well :)).
> 
> On 27-07-15, 16:20, Lee Jones wrote:
> > These OPPs are used in ST's CPUFreq implementation.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > 
> > Changelog:
> >  - None, new patch
> > 
> >  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> > new file mode 100644
> > index 0000000..6eb2a91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> > @@ -0,0 +1,76 @@
> > +STMicroelectronics OPP (Operating Performance Points) Bindings
> > +--------------------------------------------------------------
> > +
> > +Frequency Scaling only
> > +----------------------
> > +
> > +Located in CPU's node:
> > +
> > +- operating-points	: [See: ./opp.txt]
> > +
> > +Example [safe]
> > +--------------
> > +
> > +cpus {
> > +	cpu at 0 {
> > +				 /* kHz     uV   */
> > +		operating-points = <1500000 0
> > +				    1200000 0
> > +				    800000  0
> > +				    500000  0>;
> > +	};
> > +};
> > +
> > +Dynamic Voltage and Frequency Scaling (DVFS)
> > +--------------------------------------------
> > +
> > +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> > +
> > +- compatible		: Should be "operating-points-v2-sti"
> > +- opp{1..N} 		: Each 'oppX' subnode will contain the following properties:
> 
> Or should we mention:
> - opp{1..N} 		: Each 'oppX' subnode shall contain below properties,
>                           over what ./opp.txt defines:
> 
> ?

I disagree.  For one, only 'opp-hz' is defined in ./opp.tx.  Secondly
it would be annoying to have to have to keep jumping between documents
to obtain the whole picture.  Finally, generic bindings are repeated
in platform/device specific documentation all the time.  Grep for
'clocks' or 'regulator-* or 'interrupts' or 'reg' or 'clock-frequency'
(which IMHO I think you should have used instead of 'opp-hz', but
that's by the by), or any number of other generic properties.

> > + - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
> > + - st,avs		: List of available voltages [uV] indexed by process code
> > + - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
> > + - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
> > +- st,syscfg		: Phandle to Major number register
> > +				First cell: offset to major number
> > +- st,syscfg-eng		: Phandle to Minor number and Pcode registers
> > +				First cell: offset to process code
> > +				Second cell: offset to minor number
> > +
> > +WARNING: The opp{1..N} nodes will be provided by the bootloader.  Do not attempt to
> > +	 artificially synthesise the opp{1..N} nodes or any of their descendants.
> > +	 They are very platform specific and may damage the hardware if created
> > +	 incorrectly.
> > +
> > +Example [unsafe]
> > +----------------
> > +
> > +cpus {
> > +	cpu at 0 {
> > +		operating-points-v2	= <&cpu0_opp_list>;
> > +	};
> > +};
> > +
> > +/* ############################################################ */
> > +/* # WARNING: Do not attempt to copy/replicate this node,     # */
> > +/* #          it is only to be supplied by the bootloader !!! # */
> > +/* ############################################################ */
> > +cpu0-opp-list {
> > +	compatible	= "operating-points-v2-sti";
> > +	st,syscfg	= <&syscfg [major_offset]>;
> > +	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
> > +
> > +	opp0 {
> > +		opp-hz		= <1200000000>;
> > +		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
> > +		st,substrate	= <0xff>;
> > +		st,cuts		= <0xff>;
> > +	};
> > +	opp1 {
> > +		opp-hz		= <1500000000>;
> > +		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
> > +		st,substrate	= <0xff>;
> > +		st,cuts		= <0x2>;
> > +	};
> > +};
> 
> I don't see more problems here, unless we can move some of this to the
> generic bindings.
> 
> @Rob/Stephen: Please respond before it is late :)

No one knows this stuff better than you.  If you can't think of an
already existing binding that could suit to portray our 'cuts' and
'substrate' information (with a similar way to support our "all cuts"
and "all substrates" options, then there probably isn't one. ;)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 28-07-15, 08:34, Lee Jones wrote:
> I disagree.  For one, only 'opp-hz' is defined in ./opp.tx.  Secondly

There are other properties in op.txt like turbo, opp-suspend, latency,
etc.. which can be useful for your platform to. Its not used for now
is a different thing.

> it would be annoying to have to have to keep jumping between documents
> to obtain the whole picture.  Finally, generic bindings are repeated
> in platform/device specific documentation all the time.  Grep for
> 'clocks' or 'regulator-* or 'interrupts' or 'reg' or 'clock-frequency'

Yeah, I agree. I am not against that. What I meant to say was that its
an extension to opp-v2. So whatever is present in opp-v2 can be used
by ST's driver, without mentioning that here as well.

> (which IMHO I think you should have used instead of 'opp-hz', but
> that's by the by), or any number of other generic properties.

Hmm, probably yes. See I don't know everything :)

> > @Rob/Stephen: Please respond before it is late :)
> 
> No one knows this stuff better than you.  If you can't think of an
> already existing binding that could suit to portray our 'cuts' and
> 'substrate' information (with a similar way to support our "all cuts"
> and "all substrates" options, then there probably isn't one. ;)

Oh, I wasn't saying that there is an existing binding for supporting
your case. But if we want to move the cuts property to the generic
bindings so that others can use it. :)

-- 
viresh

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

[Lee Jones]

On Tue, 28 Jul 2015, Viresh Kumar wrote:

> On 28-07-15, 08:34, Lee Jones wrote:
> > I disagree.  For one, only 'opp-hz' is defined in ./opp.tx.  Secondly
> 
> There are other properties in op.txt like turbo, opp-suspend, latency,
> etc.. which can be useful for your platform to. Its not used for now
> is a different thing.
> 
> > it would be annoying to have to have to keep jumping between documents
> > to obtain the whole picture.  Finally, generic bindings are repeated
> > in platform/device specific documentation all the time.  Grep for
> > 'clocks' or 'regulator-* or 'interrupts' or 'reg' or 'clock-frequency'
> 
> Yeah, I agree. I am not against that. What I meant to say was that its
> an extension to opp-v2. So whatever is present in opp-v2 can be used
> by ST's driver, without mentioning that here as well.
> 
> > (which IMHO I think you should have used instead of 'opp-hz', but
> > that's by the by), or any number of other generic properties.
> 
> Hmm, probably yes. See I don't know everything :)
> 
> > > @Rob/Stephen: Please respond before it is late :)
> > 
> > No one knows this stuff better than you.  If you can't think of an
> > already existing binding that could suit to portray our 'cuts' and
> > 'substrate' information (with a similar way to support our "all cuts"
> > and "all substrates" options, then there probably isn't one. ;)
> 
> Oh, I wasn't saying that there is an existing binding for supporting
> your case. But if we want to move the cuts property to the generic
> bindings so that others can use it. :)

Okay, so what are we waiting for before you'd be prepared to Ack the
binding?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Stephen Boyd]

On 07/28, Viresh Kumar wrote:
> Cc'ing few people (whom I cc'd last time as well :)).
> 
> On 27-07-15, 16:20, Lee Jones wrote:
> > These OPPs are used in ST's CPUFreq implementation.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > 
> > Changelog:
> >  - None, new patch
> > 
> >  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> > new file mode 100644
> > index 0000000..6eb2a91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> > @@ -0,0 +1,76 @@
> > +STMicroelectronics OPP (Operating Performance Points) Bindings
> > +--------------------------------------------------------------
> > +
> > +Frequency Scaling only
> > +----------------------
> > +
> > +Located in CPU's node:
> > +
> > +- operating-points	: [See: ./opp.txt]
> > +
> > +Example [safe]
> > +--------------
> > +
> > +cpus {
> > +	cpu at 0 {
> > +				 /* kHz     uV   */
> > +		operating-points = <1500000 0
> > +				    1200000 0
> > +				    800000  0
> > +				    500000  0>;
> > +	};
> > +};
> > +
> > +Dynamic Voltage and Frequency Scaling (DVFS)
> > +--------------------------------------------
> > +
> > +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> > +
> > +- compatible		: Should be "operating-points-v2-sti"
> > +- opp{1..N} 		: Each 'oppX' subnode will contain the following properties:
> 
> Or should we mention:
> - opp{1..N} 		: Each 'oppX' subnode shall contain below properties,
>                           over what ./opp.txt defines:
> 
> ?
> 
> 
> > + - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
> > + - st,avs		: List of available voltages [uV] indexed by process code
> > + - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
> > + - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
> > +- st,syscfg		: Phandle to Major number register
> > +				First cell: offset to major number
> > +- st,syscfg-eng		: Phandle to Minor number and Pcode registers
> > +				First cell: offset to process code
> > +				Second cell: offset to minor number
> > +
> > +WARNING: The opp{1..N} nodes will be provided by the bootloader.  Do not attempt to
> > +	 artificially synthesise the opp{1..N} nodes or any of their descendants.
> > +	 They are very platform specific and may damage the hardware if created
> > +	 incorrectly.
> > +
> > +Example [unsafe]
> > +----------------
> > +
> > +cpus {
> > +	cpu at 0 {
> > +		operating-points-v2	= <&cpu0_opp_list>;
> > +	};
> > +};
> > +
> > +/* ############################################################ */
> > +/* # WARNING: Do not attempt to copy/replicate this node,     # */
> > +/* #          it is only to be supplied by the bootloader !!! # */
> > +/* ############################################################ */
> > +cpu0-opp-list {
> > +	compatible	= "operating-points-v2-sti";
> > +	st,syscfg	= <&syscfg [major_offset]>;
> > +	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
> > +
> > +	opp0 {
> > +		opp-hz		= <1200000000>;
> > +		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
> > +		st,substrate	= <0xff>;
> > +		st,cuts		= <0xff>;
> > +	};
> > +	opp1 {
> > +		opp-hz		= <1500000000>;
> > +		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
> > +		st,substrate	= <0xff>;
> > +		st,cuts		= <0x2>;
> > +	};
> > +};
> 
> I don't see more problems here, unless we can move some of this to the
> generic bindings.
> 
> @Rob/Stephen: Please respond before it is late :)
> 

It's interesting to have vendor specific properties like avs,
cuts, and substrate. That could replace our planned usage of the
opp-names property where we encode similar information (speed
bin, revision, etc.) into a string that we look for.

So I wonder why the avs/cut/substrate information can't be
encoded into the opp name? That would make these properties
obsolete, given that all they're used for is to pick out the
correct OPP?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lee Jones]

On Tue, 28 Jul 2015, Stephen Boyd wrote:

> On 07/28, Viresh Kumar wrote:
> > Cc'ing few people (whom I cc'd last time as well :)).
> > 
> > On 27-07-15, 16:20, Lee Jones wrote:
> > > These OPPs are used in ST's CPUFreq implementation.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > > 
> > > Changelog:
> > >  - None, new patch
> > > 
> > >  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
> > >  1 file changed, 76 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> > > new file mode 100644
> > > index 0000000..6eb2a91
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> > > @@ -0,0 +1,76 @@
> > > +STMicroelectronics OPP (Operating Performance Points) Bindings
> > > +--------------------------------------------------------------
> > > +
> > > +Frequency Scaling only
> > > +----------------------
> > > +
> > > +Located in CPU's node:
> > > +
> > > +- operating-points	: [See: ./opp.txt]
> > > +
> > > +Example [safe]
> > > +--------------
> > > +
> > > +cpus {
> > > +	cpu at 0 {
> > > +				 /* kHz     uV   */
> > > +		operating-points = <1500000 0
> > > +				    1200000 0
> > > +				    800000  0
> > > +				    500000  0>;
> > > +	};
> > > +};
> > > +
> > > +Dynamic Voltage and Frequency Scaling (DVFS)
> > > +--------------------------------------------
> > > +
> > > +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> > > +
> > > +- compatible		: Should be "operating-points-v2-sti"
> > > +- opp{1..N} 		: Each 'oppX' subnode will contain the following properties:
> > 
> > Or should we mention:
> > - opp{1..N} 		: Each 'oppX' subnode shall contain below properties,
> >                           over what ./opp.txt defines:
> > 
> > ?
> > 
> > 
> > > + - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
> > > + - st,avs		: List of available voltages [uV] indexed by process code
> > > + - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
> > > + - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
> > > +- st,syscfg		: Phandle to Major number register
> > > +				First cell: offset to major number
> > > +- st,syscfg-eng		: Phandle to Minor number and Pcode registers
> > > +				First cell: offset to process code
> > > +				Second cell: offset to minor number
> > > +
> > > +WARNING: The opp{1..N} nodes will be provided by the bootloader.  Do not attempt to
> > > +	 artificially synthesise the opp{1..N} nodes or any of their descendants.
> > > +	 They are very platform specific and may damage the hardware if created
> > > +	 incorrectly.
> > > +
> > > +Example [unsafe]
> > > +----------------
> > > +
> > > +cpus {
> > > +	cpu at 0 {
> > > +		operating-points-v2	= <&cpu0_opp_list>;
> > > +	};
> > > +};
> > > +
> > > +/* ############################################################ */
> > > +/* # WARNING: Do not attempt to copy/replicate this node,     # */
> > > +/* #          it is only to be supplied by the bootloader !!! # */
> > > +/* ############################################################ */
> > > +cpu0-opp-list {
> > > +	compatible	= "operating-points-v2-sti";
> > > +	st,syscfg	= <&syscfg [major_offset]>;
> > > +	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
> > > +
> > > +	opp0 {
> > > +		opp-hz		= <1200000000>;
> > > +		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
> > > +		st,substrate	= <0xff>;
> > > +		st,cuts		= <0xff>;
> > > +	};
> > > +	opp1 {
> > > +		opp-hz		= <1500000000>;
> > > +		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
> > > +		st,substrate	= <0xff>;
> > > +		st,cuts		= <0x2>;
> > > +	};
> > > +};
> > 
> > I don't see more problems here, unless we can move some of this to the
> > generic bindings.
> > 
> > @Rob/Stephen: Please respond before it is late :)
> 
> It's interesting to have vendor specific properties like avs,
> cuts, and substrate. That could replace our planned usage of the
> opp-names property where we encode similar information (speed
> bin, revision, etc.) into a string that we look for.
> 
> So I wonder why the avs/cut/substrate information can't be
> encoded into the opp name? That would make these properties
> obsolete, given that all they're used for is to pick out the
> correct OPP?

You could hack the substrate and cut version into a string, but that's
exactly what it would be, a hack.  I'm struggling how you would do the
same for 'st,avs', which is an array of u32s.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Stephen Boyd]

On 07/29, Lee Jones wrote:
> On Tue, 28 Jul 2015, Stephen Boyd wrote:
> 
> > On 07/28, Viresh Kumar wrote:
> > > Cc'ing few people (whom I cc'd last time as well :)).
> > > 
> > > On 27-07-15, 16:20, Lee Jones wrote:
> > > > + - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
> > > > + - st,avs		: List of available voltages [uV] indexed by process code
> > > > + - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
> > > > + - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
[...]
> > > > +cpu0-opp-list {
> > > > +	compatible	= "operating-points-v2-sti";
> > > > +	st,syscfg	= <&syscfg [major_offset]>;
> > > > +	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
> > > > +
> > > > +	opp0 {
> > > > +		opp-hz		= <1200000000>;
> > > > +		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
> > > > +		st,substrate	= <0xff>;
> > > > +		st,cuts		= <0xff>;
> > > > +	};
> > > > +	opp1 {
> > > > +		opp-hz		= <1500000000>;
> > > > +		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
> > > > +		st,substrate	= <0xff>;
> > > > +		st,cuts		= <0x2>;
> > > > +	};
> > > > +};
> > > 
> > > I don't see more problems here, unless we can move some of this to the
> > > generic bindings.
> > > 
> > > @Rob/Stephen: Please respond before it is late :)
> > 
> > It's interesting to have vendor specific properties like avs,
> > cuts, and substrate. That could replace our planned usage of the
> > opp-names property where we encode similar information (speed
> > bin, revision, etc.) into a string that we look for.
> > 
> > So I wonder why the avs/cut/substrate information can't be
> > encoded into the opp name? That would make these properties
> > obsolete, given that all they're used for is to pick out the
> > correct OPP?
> 
> You could hack the substrate and cut version into a string, but that's
> exactly what it would be, a hack.  I'm struggling how you would do the
> same for 'st,avs', which is an array of u32s.
> 


(I don't understand the st,avs property to begin with, so feel
free to ignore the rest of this mail.)

For qcom platforms we have the pvs bin and speed bin, and
sometimes a revision number. Each one of these properties
corresponds to a different set of OPPs (opp table). So we might
have speed1-pvs2-v0 for speed bin 1, pvs bin 2 and version 0. We
fill out an opp table for this and then point the opps property
at the table and have a corresponding opp-name "speed1-pvs2-v0"
in the "consumer" node.

	operating-points-v2 = <&speed1_pvs2_v0>;
	operating-points-names = "speed1-pvs2-v0";

We have quite a few of these tables because the values are
always different. If the values were the same then we could use
the same table with different names I suppose, but we're not
doing that.

>From a quick read of the st properties (that I admit I don't
understand), it looks like we're trying to compress the OPP
tables by listing all the voltages that could be used for a
particular frequency depending on which avs is present on the
device? And then limiting the frequency voltage pairs depending
on which cut and substrate is present?

So we'd probably have to expand out the tables to be unique per
avs/cut/substrate parameter. Something like:

avs0-cut2 {
	compatible = "operating-points-v2";

	opp0 {
		opp-hz = <1200000000>;
		opp-microvolt = <1100>;
	};

	opp1 {
		opp-hz = <1500000000>;
		opp-microvolt = <1200>;
	};
};

avs1-cut2 {
	compatible = "operating-points-v2";

	opp0 {
		opp-hz = <1200000000>;
		opp-microvolt = <1150>;
	};

	opp1 {
		opp-hz = <1500000000>;
		opp-microvolt = <1200>;
	};
};

And then another copy of these for the devices without cuts != 2
where the top frequency is gone?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lee Jones]

On Wed, 29 Jul 2015, Stephen Boyd wrote:

> On 07/29, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Stephen Boyd wrote:
> > 
> > > On 07/28, Viresh Kumar wrote:
> > > > Cc'ing few people (whom I cc'd last time as well :)).
> > > > 
> > > > On 27-07-15, 16:20, Lee Jones wrote:
> > > > > + - opp-hz		: CPU frequency [Hz] for this OPP [See: ./opp.txt]
> > > > > + - st,avs		: List of available voltages [uV] indexed by process code
> > > > > + - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
> > > > > + - st,substrate		: Substrate version this OPP is suitable for [0xFF means ALL]
> [...]
> > > > > +cpu0-opp-list {
> > > > > +	compatible	= "operating-points-v2-sti";
> > > > > +	st,syscfg	= <&syscfg [major_offset]>;
> > > > > +	st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
> > > > > +
> > > > > +	opp0 {
> > > > > +		opp-hz		= <1200000000>;
> > > > > +		st,avs		= <1110 1150 1100 1080 1040 1020 980 930>;
> > > > > +		st,substrate	= <0xff>;
> > > > > +		st,cuts		= <0xff>;
> > > > > +	};
> > > > > +	opp1 {
> > > > > +		opp-hz		= <1500000000>;
> > > > > +		st,avs		= <1200 1200 1200 1200 1170 1140 1100 1070>;
> > > > > +		st,substrate	= <0xff>;
> > > > > +		st,cuts		= <0x2>;
> > > > > +	};
> > > > > +};
> > > > 
> > > > I don't see more problems here, unless we can move some of this to the
> > > > generic bindings.
> > > > 
> > > > @Rob/Stephen: Please respond before it is late :)
> > > 
> > > It's interesting to have vendor specific properties like avs,
> > > cuts, and substrate. That could replace our planned usage of the
> > > opp-names property where we encode similar information (speed
> > > bin, revision, etc.) into a string that we look for.
> > > 
> > > So I wonder why the avs/cut/substrate information can't be
> > > encoded into the opp name? That would make these properties
> > > obsolete, given that all they're used for is to pick out the
> > > correct OPP?
> > 
> > You could hack the substrate and cut version into a string, but that's
> > exactly what it would be, a hack.  I'm struggling how you would do the
> > same for 'st,avs', which is an array of u32s.
> > 
> 
> 
> (I don't understand the st,avs property to begin with, so feel
> free to ignore the rest of this mail.)
> 
> For qcom platforms we have the pvs bin and speed bin, and
> sometimes a revision number. Each one of these properties
> corresponds to a different set of OPPs (opp table). So we might
> have speed1-pvs2-v0 for speed bin 1, pvs bin 2 and version 0. We
> fill out an opp table for this and then point the opps property
> at the table and have a corresponding opp-name "speed1-pvs2-v0"
> in the "consumer" node.
> 
> 	operating-points-v2 = <&speed1_pvs2_v0>;
> 	operating-points-names = "speed1-pvs2-v0";
> 
> We have quite a few of these tables because the values are
> always different. If the values were the same then we could use
> the same table with different names I suppose, but we're not
> doing that.
> 
> From a quick read of the st properties (that I admit I don't
> understand), it looks like we're trying to compress the OPP
> tables by listing all the voltages that could be used for a
> particular frequency depending on which avs is present on the
> device? And then limiting the frequency voltage pairs depending
> on which cut and substrate is present?
> 
> So we'd probably have to expand out the tables to be unique per
> avs/cut/substrate parameter. Something like:
> 
> avs0-cut2 {
> 	compatible = "operating-points-v2";
> 
> 	opp0 {
> 		opp-hz = <1200000000>;
> 		opp-microvolt = <1100>;
> 	};
> 
> 	opp1 {
> 		opp-hz = <1500000000>;
> 		opp-microvolt = <1200>;
> 	};
> };
> 
> avs1-cut2 {
> 	compatible = "operating-points-v2";
> 
> 	opp0 {
> 		opp-hz = <1200000000>;
> 		opp-microvolt = <1150>;
> 	};
> 
> 	opp1 {
> 		opp-hz = <1500000000>;
> 		opp-microvolt = <1200>;
> 	};
> };
> 
> And then another copy of these for the devices without cuts != 2
> where the top frequency is gone?

There is nothing stopping us from representing the data in this way.
On the plus side, it would mean that we wouldn't need any vendor
specific properties.  However, far outweighing the positives are the
fact that, even in our very simple example provided, where we only
have 2 frequencies, differ between only 1 cut and support all
substrates, we would still need 16 OPP tables.  When any one of those
variables increase (and they will), we would then have a large number
of permutations and subsequently and unruly amount of OPP tables.

(... and we haven't even provided the body-biasing information yet.)

The way we've chosen to represent our circumstance is the least
intrusive and the most succinct way we can think of.  Which IMHO
outweighs the fact that we have to introduce a couple of vendor
properties by a country mile.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Rob Herring]

On Thu, Jul 30, 2015 at 3:46 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Wed, 29 Jul 2015, Stephen Boyd wrote:
>
>> On 07/29, Lee Jones wrote:
>> > On Tue, 28 Jul 2015, Stephen Boyd wrote:
>> >
>> > > On 07/28, Viresh Kumar wrote:
>> > > > Cc'ing few people (whom I cc'd last time as well :)).
>> > > >
>> > > > On 27-07-15, 16:20, Lee Jones wrote:
>> > > > > + - opp-hz            : CPU frequency [Hz] for this OPP [See: ./opp.txt]
>> > > > > + - st,avs            : List of available voltages [uV] indexed by process code
>> > > > > + - st,cuts           : Cut version this OPP is suitable for [0xFF means ALL]
>> > > > > + - st,substrate              : Substrate version this OPP is suitable for [0xFF means ALL]
>> [...]
>> > > > > +cpu0-opp-list {
>> > > > > +     compatible      = "operating-points-v2-sti";
>> > > > > +     st,syscfg       = <&syscfg [major_offset]>;
>> > > > > +     st,syscfg-eng   = <&syscfg_eng [pcode_offset] [minor_offset]>;
>> > > > > +
>> > > > > +     opp0 {
>> > > > > +             opp-hz          = <1200000000>;
>> > > > > +             st,avs          = <1110 1150 1100 1080 1040 1020 980 930>;
>> > > > > +             st,substrate    = <0xff>;
>> > > > > +             st,cuts         = <0xff>;
>> > > > > +     };
>> > > > > +     opp1 {
>> > > > > +             opp-hz          = <1500000000>;
>> > > > > +             st,avs          = <1200 1200 1200 1200 1170 1140 1100 1070>;
>> > > > > +             st,substrate    = <0xff>;
>> > > > > +             st,cuts         = <0x2>;
>> > > > > +     };
>> > > > > +};
>> > > >
>> > > > I don't see more problems here, unless we can move some of this to the
>> > > > generic bindings.
>> > > >
>> > > > @Rob/Stephen: Please respond before it is late :)
>> > >
>> > > It's interesting to have vendor specific properties like avs,
>> > > cuts, and substrate. That could replace our planned usage of the
>> > > opp-names property where we encode similar information (speed
>> > > bin, revision, etc.) into a string that we look for.
>> > >
>> > > So I wonder why the avs/cut/substrate information can't be
>> > > encoded into the opp name? That would make these properties
>> > > obsolete, given that all they're used for is to pick out the
>> > > correct OPP?
>> >
>> > You could hack the substrate and cut version into a string, but that's
>> > exactly what it would be, a hack.  I'm struggling how you would do the
>> > same for 'st,avs', which is an array of u32s.
>> >
>>
>>
>> (I don't understand the st,avs property to begin with, so feel
>> free to ignore the rest of this mail.)
>>
>> For qcom platforms we have the pvs bin and speed bin, and
>> sometimes a revision number. Each one of these properties
>> corresponds to a different set of OPPs (opp table). So we might
>> have speed1-pvs2-v0 for speed bin 1, pvs bin 2 and version 0. We
>> fill out an opp table for this and then point the opps property
>> at the table and have a corresponding opp-name "speed1-pvs2-v0"
>> in the "consumer" node.
>>
>>       operating-points-v2 = <&speed1_pvs2_v0>;
>>       operating-points-names = "speed1-pvs2-v0";
>>
>> We have quite a few of these tables because the values are
>> always different. If the values were the same then we could use
>> the same table with different names I suppose, but we're not
>> doing that.
>>
>> From a quick read of the st properties (that I admit I don't
>> understand), it looks like we're trying to compress the OPP
>> tables by listing all the voltages that could be used for a
>> particular frequency depending on which avs is present on the
>> device? And then limiting the frequency voltage pairs depending
>> on which cut and substrate is present?
>>
>> So we'd probably have to expand out the tables to be unique per
>> avs/cut/substrate parameter. Something like:
>>
>> avs0-cut2 {
>>       compatible = "operating-points-v2";
>>
>>       opp0 {
>>               opp-hz = <1200000000>;
>>               opp-microvolt = <1100>;
>>       };
>>
>>       opp1 {
>>               opp-hz = <1500000000>;
>>               opp-microvolt = <1200>;
>>       };
>> };
>>
>> avs1-cut2 {
>>       compatible = "operating-points-v2";
>>
>>       opp0 {
>>               opp-hz = <1200000000>;
>>               opp-microvolt = <1150>;
>>       };
>>
>>       opp1 {
>>               opp-hz = <1500000000>;
>>               opp-microvolt = <1200>;
>>       };
>> };
>>
>> And then another copy of these for the devices without cuts != 2
>> where the top frequency is gone?
>
> There is nothing stopping us from representing the data in this way.
> On the plus side, it would mean that we wouldn't need any vendor
> specific properties.  However, far outweighing the positives are the
> fact that, even in our very simple example provided, where we only
> have 2 frequencies, differ between only 1 cut and support all
> substrates, we would still need 16 OPP tables.  When any one of those
> variables increase (and they will), we would then have a large number
> of permutations and subsequently and unruly amount of OPP tables.
>
> (... and we haven't even provided the body-biasing information yet.)
>
> The way we've chosen to represent our circumstance is the least
> intrusive and the most succinct way we can think of.  Which IMHO
> outweighs the fact that we have to introduce a couple of vendor
> properties by a country mile.

Regardless of which is better or worse, you are both doing essentially
the same thing. You are selecting OPPs based on some Si parameters. We
should not really be doing this 2 different ways. I'd be fine with 2
ways if it was 2 for all SOCs, but right now it is 2 for 2 SOCs.
Really, I'd like to see most if not all the selection or fixup of OPPs
be done in the bootloader and the kernel just deals with the correct
OPP table.

Where are you storing the data that gets selected to fill into these
properties? Is it just look-up tables or is there some kind of
algorithm to generate the values? If the former, then DT is as good a
data struct as any to store the tables. There is a lot of duplication
though with only a single property varying in each set. If you both
have that problem, then perhaps we can come up with a generic way to
list all possible values more concisely.

Rob

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

[Stephen Boyd]

On 07/30, Rob Herring wrote:
> On Thu, Jul 30, 2015 at 3:46 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > There is nothing stopping us from representing the data in this way.
> > On the plus side, it would mean that we wouldn't need any vendor
> > specific properties.  However, far outweighing the positives are the
> > fact that, even in our very simple example provided, where we only
> > have 2 frequencies, differ between only 1 cut and support all
> > substrates, we would still need 16 OPP tables.  When any one of those
> > variables increase (and they will), we would then have a large number
> > of permutations and subsequently and unruly amount of OPP tables.
> >
> > (... and we haven't even provided the body-biasing information yet.)
> >
> > The way we've chosen to represent our circumstance is the least
> > intrusive and the most succinct way we can think of.  Which IMHO
> > outweighs the fact that we have to introduce a couple of vendor
> > properties by a country mile.
> 
> Regardless of which is better or worse, you are both doing essentially
> the same thing. You are selecting OPPs based on some Si parameters. We
> should not really be doing this 2 different ways. I'd be fine with 2
> ways if it was 2 for all SOCs, but right now it is 2 for 2 SOCs.
> Really, I'd like to see most if not all the selection or fixup of OPPs
> be done in the bootloader and the kernel just deals with the correct
> OPP table.
> 
> Where are you storing the data that gets selected to fill into these
> properties? Is it just look-up tables or is there some kind of
> algorithm to generate the values? If the former, then DT is as good a
> data struct as any to store the tables. There is a lot of duplication
> though with only a single property varying in each set. If you both
> have that problem, then perhaps we can come up with a generic way to
> list all possible values more concisely.

For qcom platforms, the frequency is almost always constant.
There may be some tables where we have a couple higher
frequencies than others because the speed bin is different.
Otherwise the voltage/current is changing based on the silicon
characteristics. So the biggest duplication is the frequency
property.

As far as I know there isn't any algorithm to generate the
voltage values. It's all hand tuned tables based on the silicon
characterization, so we're left to store these tables in DT and
pick the right one at runtime. With regards to the table
explosion, on qcom platforms we haven't worried that we have ~40
tables, but I'm not opposed to expressing it in a smaller set of
nodes, tables, etc. if that's what's desired.

Do we need vendor specific properties for that though? Or do we
need some sort of extended frequency/voltage properties that are
arrays of values that we can index into based on some silicon
characteristics? I like the name based approach because it's
simple. Use this OPP table because it's called
x-y-z-characteristics and be done. Cramming the tables into less
lines may save us some typing and dtb space, but I'm not sure
what else it does.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Viresh Kumar]

On 31-07-15, 09:37, Stephen Boyd wrote:
> Do we need vendor specific properties for that though?

Sorry Lee :), but this is exactly why I wanted this thread to exist. We must and
should do this in a generic enough way.

-- 
viresh

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

[Viresh Kumar]

On 31-07-15, 09:37, Stephen Boyd wrote:
> For qcom platforms, the frequency is almost always constant.
> There may be some tables where we have a couple higher
> frequencies than others because the speed bin is different.
> Otherwise the voltage/current is changing based on the silicon
> characteristics. So the biggest duplication is the frequency
> property.
> 
> As far as I know there isn't any algorithm to generate the
> voltage values. It's all hand tuned tables based on the silicon
> characterization, so we're left to store these tables in DT and
> pick the right one at runtime. With regards to the table
> explosion, on qcom platforms we haven't worried that we have ~40
> tables, but I'm not opposed to expressing it in a smaller set of
> nodes, tables, etc. if that's what's desired.
> 
> Do we need vendor specific properties for that though? Or do we
> need some sort of extended frequency/voltage properties that are
> arrays of values that we can index into based on some silicon
> characteristics? I like the name based approach because it's
> simple. Use this OPP table because it's called
> x-y-z-characteristics and be done. Cramming the tables into less
> lines may save us some typing and dtb space, but I'm not sure
> what else it does.

What about something like this:

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 0cb44dc21f97..bad7a8299b9c 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -74,6 +74,8 @@ This describes the OPPs belonging to a device. This node can have following
   reference an OPP.
 
 Optional properties:
+- opp-cuts: One or more strings, describing the versions of hardware the OPPs
+  can support.
 - opp-shared: Indicates that device nodes using this OPP Table Node's phandle
   switch their DVFS state together, i.e. they share clock/voltage/current lines.
   Missing property means devices have independent clock/voltage/current lines,
@@ -100,6 +102,9 @@ properties.
   Entries for multiple regulators must be present in the same order as
   regulators are specified in device's DT node.
 
+  If used with 'opp-cuts', then the number of entries present here must match
+  the number of strings present in 'opp-cuts'.
+
 - opp-microamp: The maximum current drawn by the device in microamperes
   considering system specific parameters (such as transients, process, aging,
   maximum operating temperature range etc.) as necessary. This may be used to

-- 
viresh

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

[Lee Jones]

On Mon, 03 Aug 2015, Viresh Kumar wrote:

> On 31-07-15, 09:37, Stephen Boyd wrote:
> > For qcom platforms, the frequency is almost always constant.
> > There may be some tables where we have a couple higher
> > frequencies than others because the speed bin is different.
> > Otherwise the voltage/current is changing based on the silicon
> > characteristics. So the biggest duplication is the frequency
> > property.
> > 
> > As far as I know there isn't any algorithm to generate the
> > voltage values. It's all hand tuned tables based on the silicon
> > characterization, so we're left to store these tables in DT and
> > pick the right one at runtime. With regards to the table
> > explosion, on qcom platforms we haven't worried that we have ~40
> > tables, but I'm not opposed to expressing it in a smaller set of
> > nodes, tables, etc. if that's what's desired.
> > 
> > Do we need vendor specific properties for that though? Or do we
> > need some sort of extended frequency/voltage properties that are
> > arrays of values that we can index into based on some silicon
> > characteristics? I like the name based approach because it's
> > simple. Use this OPP table because it's called
> > x-y-z-characteristics and be done. Cramming the tables into less
> > lines may save us some typing and dtb space, but I'm not sure
> > what else it does.
> 
> What about something like this:
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 0cb44dc21f97..bad7a8299b9c 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -74,6 +74,8 @@ This describes the OPPs belonging to a device. This node can have following
>    reference an OPP.
>  
>  Optional properties:
> +- opp-cuts: One or more strings, describing the versions of hardware the OPPs
> +  can support.

This isn't very generic.

I'm guessing some vendors my have quite a few ways to differentiate
between board versions/revisions/cuts etc.

How about another array where a vendor can choose to identify a piece
of hardware however they see fit.

Example 1 (simple version):

/* Version 1 */
opp-version = <1>;

Example 2 (using the kernel's versioning):

/* 2.6.32-rc1 */
opp-version = <2 6 32 1>;

Example 3 (using ST's versioning):

/* Major 2, Minor 0, Cut 2, All substrates */
opp-version = <2 0 2 0xff>;

Qcom (or anyone else wanting to use names to identify their revisions)
can continue to use their node name method, as it doesn't break any
convention.

>  - opp-shared: Indicates that device nodes using this OPP Table Node's phandle
>    switch their DVFS state together, i.e. they share clock/voltage/current lines.
>    Missing property means devices have independent clock/voltage/current lines,
> @@ -100,6 +102,9 @@ properties.
>    Entries for multiple regulators must be present in the same order as
>    regulators are specified in device's DT node.
>  
> +  If used with 'opp-cuts', then the number of entries present here must match
> +  the number of strings present in 'opp-cuts'.
> +
>  - opp-microamp: The maximum current drawn by the device in microamperes
>    considering system specific parameters (such as transients, process, aging,
>    maximum operating temperature range etc.) as necessary. This may be used to
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 10-08-15, 14:22, Lee Jones wrote:
> >  Optional properties:
> > +- opp-cuts: One or more strings, describing the versions of hardware the OPPs
> > +  can support.
> 
> This isn't very generic.
> 
> I'm guessing some vendors my have quite a few ways to differentiate
> between board versions/revisions/cuts etc.
> 
> How about another array where a vendor can choose to identify a piece
> of hardware however they see fit.
> 
> Example 1 (simple version):
> 
> /* Version 1 */
> opp-version = <1>;
> 
> Example 2 (using the kernel's versioning):
> 
> /* 2.6.32-rc1 */
> opp-version = <2 6 32 1>;
> 
> Example 3 (using ST's versioning):
> 
> /* Major 2, Minor 0, Cut 2, All substrates */
> opp-version = <2 0 2 0xff>;

But how will we parse this with generic code ?

-- 
viresh

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

[Lee Jones]

On Tue, 11 Aug 2015, Viresh Kumar wrote:

> On 10-08-15, 14:22, Lee Jones wrote:
> > >  Optional properties:
> > > +- opp-cuts: One or more strings, describing the versions of hardware the OPPs
> > > +  can support.
> > 
> > This isn't very generic.
> > 
> > I'm guessing some vendors my have quite a few ways to differentiate
> > between board versions/revisions/cuts etc.
> > 
> > How about another array where a vendor can choose to identify a piece
> > of hardware however they see fit.
> > 
> > Example 1 (simple version):
> > 
> > /* Version 1 */
> > opp-version = <1>;
> > 
> > Example 2 (using the kernel's versioning):
> > 
> > /* 2.6.32-rc1 */
> > opp-version = <2 6 32 1>;
> > 
> > Example 3 (using ST's versioning):
> > 
> > /* Major 2, Minor 0, Cut 2, All substrates */
> > opp-version = <2 0 2 0xff>;
> 
> But how will we parse this with generic code ?

Why would you want to?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 11-08-15, 10:30, Lee Jones wrote:
> On Tue, 11 Aug 2015, Viresh Kumar wrote:
> 
> > On 10-08-15, 14:22, Lee Jones wrote:
> > > >  Optional properties:
> > > > +- opp-cuts: One or more strings, describing the versions of hardware the OPPs
> > > > +  can support.
> > > 
> > > This isn't very generic.
> > > 
> > > I'm guessing some vendors my have quite a few ways to differentiate
> > > between board versions/revisions/cuts etc.
> > > 
> > > How about another array where a vendor can choose to identify a piece
> > > of hardware however they see fit.
> > > 
> > > Example 1 (simple version):
> > > 
> > > /* Version 1 */
> > > opp-version = <1>;
> > > 
> > > Example 2 (using the kernel's versioning):
> > > 
> > > /* 2.6.32-rc1 */
> > > opp-version = <2 6 32 1>;
> > > 
> > > Example 3 (using ST's versioning):
> > > 
> > > /* Major 2, Minor 0, Cut 2, All substrates */
> > > opp-version = <2 0 2 0xff>;
> > 
> > But how will we parse this with generic code ?
> 
> Why would you want to?

So that individual platforms don't need to reinvent the wheel ?

-- 
viresh

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

[Lee Jones]

On Tue, 11 Aug 2015, Viresh Kumar wrote:
> On 11-08-15, 10:30, Lee Jones wrote:
> > On Tue, 11 Aug 2015, Viresh Kumar wrote:
> > 
> > > On 10-08-15, 14:22, Lee Jones wrote:
> > > > >  Optional properties:
> > > > > +- opp-cuts: One or more strings, describing the versions of hardware the OPPs
> > > > > +  can support.
> > > > 
> > > > This isn't very generic.
> > > > 
> > > > I'm guessing some vendors my have quite a few ways to differentiate
> > > > between board versions/revisions/cuts etc.
> > > > 
> > > > How about another array where a vendor can choose to identify a piece
> > > > of hardware however they see fit.
> > > > 
> > > > Example 1 (simple version):
> > > > 
> > > > /* Version 1 */
> > > > opp-version = <1>;
> > > > 
> > > > Example 2 (using the kernel's versioning):
> > > > 
> > > > /* 2.6.32-rc1 */
> > > > opp-version = <2 6 32 1>;
> > > > 
> > > > Example 3 (using ST's versioning):
> > > > 
> > > > /* Major 2, Minor 0, Cut 2, All substrates */
> > > > opp-version = <2 0 2 0xff>;
> > > 
> > > But how will we parse this with generic code ?
> > 
> > Why would you want to?
> 
> So that individual platforms don't need to reinvent the wheel ?

The framework does not need to parse this information.  It is used
solely by the platform driver, whose job it is to decide which OPPs
are appropriate for the running platform.

Back to my question; how would you like arbitrary version information,
which means different things to different vendors to be used in
shared/generic/framework code?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 11-08-15, 12:54, Lee Jones wrote:
> The framework does not need to parse this information.  It is used
> solely by the platform driver, whose job it is to decide which OPPs
> are appropriate for the running platform.

The OPP layer needs to parse OPP nodes in DT. But for doing that it
needs to know which OPPs to pick from the table as, in cases like
yours or qcom, not all OPPs might be available.

One of the ways to do that is:
- the platform reads its efuses (or whatever) and encodes the
  information into a string.
- This string should match with the strings present (somewhere) in the
  OPP table. That location can be like what I proposed few mails back.
- Then the *generic* OPP code can parse only those OPP nodes which
  match with that string.

This way, we can avoid pushing the platform code to parse OPP tables.

> Back to my question; how would you like arbitrary version information,
> which means different things to different vendors to be used in
> shared/generic/framework code?

Exactly like I wrote above. The platform just needs to give a string
that should match with the OPPs ..

-- 
viresh

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

[Lee Jones]

On Tue, 11 Aug 2015, Viresh Kumar wrote:
> On 11-08-15, 12:54, Lee Jones wrote:
> > The framework does not need to parse this information.  It is used
> > solely by the platform driver, whose job it is to decide which OPPs
> > are appropriate for the running platform.
> 
> The OPP layer needs to parse OPP nodes in DT. But for doing that it
> needs to know which OPPs to pick from the table as, in cases like
> yours or qcom, not all OPPs might be available.
> 
> One of the ways to do that is:
> - the platform reads its efuses (or whatever) and encodes the
>   information into a string.
> - This string should match with the strings present (somewhere) in the
>   OPP table. That location can be like what I proposed few mails back.
> - Then the *generic* OPP code can parse only those OPP nodes which
>   match with that string.
> 
> This way, we can avoid pushing the platform code to parse OPP tables.

Okay, so what you're saying is that you've already made the decision
to create a separate node for every OPP permutation, despite the fact
that I've told you this could lead to more nodes than anyone would
care to successfully write or maintain?

Perhaps an example might help explain the issue.

Using the current driver, we need to place the following in DT and the
driver does the rest:

opp-list {
	opp1 {
		opp-hz = <1500000000>;
		st,avs = <1200 1200 1200 1200 1170 1140 1100 1070>;
		st,substrate = <0xff>;
		st,cuts = <0xff>;
	};
	opp0 {
		opp-hz = <1200000000>;
		st,avs = <1110 1150 1100 1080 1040 1020 980 930>;
		st,substrate = <0xff>;
		st,cuts = <0x2>;
	};
};

However, what you're suggesting, even for this very simple example
(imagine what this would look like with 5 or more frequencies where
two or more of them were only appropriate to run on particular
variants) requires this to be broken out to:

opp-list {
	pcode0-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <1110000>;
		};
	};

	pcode0-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1200000>;
		};
	};

	pcode1-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <1150000>;
		};
	};

	pcode1-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1200000>;
		};
	};

	pcode2-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <1100000>;
		};
	};

	pcode2-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1200000>;
		};
	};

	pcode3-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <1080000>;
		};
	};

	pcode3-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1200000>;
		};
	};

	pcode4-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <1040000>;
		};

	pcode4-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1170000>;
		};
	};

	pcode5-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <1020000>;
		};
	};

	pcode5-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1140000>;
		};
	};

	pcode6-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <980000>;
		};
	};

	pcode6-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1100000>;
		};
	};

	pcode7-cut2-allsubstrates {
		opp0 {
			opp-hz = <1200000000>;
			opp-microvolt = <930000>;
		};
	};

	pcode7-allcuts-allsubstrates {
		opp0 {
			opp-hz = <1500000000>;
			opp-microvolt = <1070000>;
		};
	};
};


These will soon multiply once you start providing more complex
examples.  And how do you plan on handling this in the framework?  Can
the driver submit more than one variations of the string?  In the
current example the driver would need to submit four strings to
provide all acceptable variations; "pcodeX-cutY-substrateZ",
"pcodeX-allcuts-substrateZ", "pcodeX-cutY-allsubstrates" and
"pcodeX-allcuts-allsubstrates"

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 11-08-15, 14:27, Lee Jones wrote:
> Okay, so what you're saying is that you've already made the decision
> to create a separate node for every OPP permutation,

Absolutely not.

> despite the fact
> that I've told you this could lead to more nodes than anyone would
> care to successfully write or maintain?

I have enough fear of yours and then I have to see you in another
month as well. I wouldn't dare to disobey your command SIR :)

> Perhaps an example might help explain the issue.
> 
> Using the current driver, we need to place the following in DT and the
> driver does the rest:
> 
> opp-list {
> 	opp1 {
> 		opp-hz = <1500000000>;
> 		st,avs = <1200 1200 1200 1200 1170 1140 1100 1070>;
> 		st,substrate = <0xff>;
> 		st,cuts = <0xff>;
> 	};
> 	opp0 {
> 		opp-hz = <1200000000>;
> 		st,avs = <1110 1150 1100 1080 1040 1020 980 930>;
> 		st,substrate = <0xff>;
> 		st,cuts = <0x2>;
> 	};
> };

Nothing is fixed as of now but this is what I am thinking of:

	cpu0_opp_table: opp_table0 {
		compatible = "operating-points-v2";
                opp-cuts = "10", "3c", "f0";
		supply-names = "vcc0", "vcc1", "vcc2";
		opp-shared;

		opp00 {
			opp-hz = /bits/ 64 <1000000000>;
			clock-latency-ns = <300000>;
			opp-microvolt-10 = <970000>;
			opp-microvolt-3c = <950000>;
			opp-microvolt-f0 = <930000>;
		};

		/* OR */

		opp00 {
			opp-hz = /bits/ 64 <1000000000>;
			clock-latency-ns = <300000>;
                        opp-microvolt = <970000>, <950000>, <930000>;
		};
        };

And then the platform code needs to tell OPP layer:
"Use OPPs for cut f0 for device X", and OPP layer will store that
somewhere.

And then it will only initialize OPPs after matching this string with
the values.

Out of the earlier two options, I may prefer the first one. As we will
be soon adding support for multiple regulators, and a single regulator
can have min/max/target values.. So, a single list will become too
long.

But, something like this should be generic enough to capture most of
the cases.

@Stephen/Rob ??

-- 
viresh

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

[Lee Jones]

On Tue, 11 Aug 2015, Viresh Kumar wrote:
> On 11-08-15, 14:27, Lee Jones wrote:
> > Okay, so what you're saying is that you've already made the decision
> > to create a separate node for every OPP permutation,
> 
> Absolutely not.
> 
> > despite the fact
> > that I've told you this could lead to more nodes than anyone would
> > care to successfully write or maintain?
> 
> I have enough fear of yours and then I have to see you in another
> month as well. I wouldn't dare to disobey your command SIR :)

Funny guy! ;)

> > Perhaps an example might help explain the issue.
> > 
> > Using the current driver, we need to place the following in DT and the
> > driver does the rest:
> > 
> > opp-list {
> > 	opp1 {
> > 		opp-hz = <1500000000>;
> > 		st,avs = <1200 1200 1200 1200 1170 1140 1100 1070>;
> > 		st,substrate = <0xff>;
> > 		st,cuts = <0xff>;
> > 	};
> > 	opp0 {
> > 		opp-hz = <1200000000>;
> > 		st,avs = <1110 1150 1100 1080 1040 1020 980 930>;
> > 		st,substrate = <0xff>;
> > 		st,cuts = <0x2>;
> > 	};
> > };
> 
> Nothing is fixed as of now but this is what I am thinking of:
> 
> 	cpu0_opp_table: opp_table0 {
> 		compatible = "operating-points-v2";
>                 opp-cuts = "10", "3c", "f0";
> 		supply-names = "vcc0", "vcc1", "vcc2";
> 		opp-shared;
> 
> 		opp00 {
> 			opp-hz = /bits/ 64 <1000000000>;
> 			clock-latency-ns = <300000>;
> 			opp-microvolt-10 = <970000>;
> 			opp-microvolt-3c = <950000>;
> 			opp-microvolt-f0 = <930000>;
> 		};
> 
> 		/* OR */
> 
> 		opp00 {
> 			opp-hz = /bits/ 64 <1000000000>;
> 			clock-latency-ns = <300000>;
>                         opp-microvolt = <970000>, <950000>, <930000>;
> 		};
>         };
> 
> And then the platform code needs to tell OPP layer:
> "Use OPPs for cut f0 for device X", and OPP layer will store that
> somewhere.
> 
> And then it will only initialize OPPs after matching this string with
> the values.
> 
> Out of the earlier two options, I may prefer the first one. As we will
> be soon adding support for multiple regulators, and a single regulator
> can have min/max/target values.. So, a single list will become too
> long.

This would work if we only had a single variable to contend with, but
what I showed you in my previous example is that we have 3 variables
to consider; cut (version), pcode and substrate.

Using the two (simple) examples I provided, how would your suggestion
look in our case?

> But, something like this should be generic enough to capture most of
> the cases.
> 
> @Stephen/Rob ??
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 11-08-15, 16:17, Lee Jones wrote:
> This would work if we only had a single variable to contend with, but
> what I showed you in my previous example is that we have 3 variables
> to consider; cut (version), pcode and substrate.
> 
> Using the two (simple) examples I provided, how would your suggestion
> look in our case?

So the solution I gave is for picking the microvolt based on pcode.
The other two (cut, substrate) aren't about picking microvolt, but if
the OPP is available or not. Right?

If these terms are generic enough, then we can add something similar
to what you have added..

@Stephen ?

-- 
viresh

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

[Lee Jones]

On Wed, 12 Aug 2015, Viresh Kumar wrote:

> On 11-08-15, 16:17, Lee Jones wrote:
> > This would work if we only had a single variable to contend with, but
> > what I showed you in my previous example is that we have 3 variables
> > to consider; cut (version), pcode and substrate.
> > 
> > Using the two (simple) examples I provided, how would your suggestion
> > look in our case?
> 
> So the solution I gave is for picking the microvolt based on pcode.
> The other two (cut, substrate) aren't about picking microvolt, but if
> the OPP is available or not. Right?

'pcode', 'cut' and 'substrate' all determine whether a given set of
OPPs an be used on the running platform.  I do not believe that you
can differentiate between them. 

> If these terms are generic enough, then we can add something similar
> to what you have added..

If it makes it easier, you can treat them as version numbers 2.2.1
<pcode.cut.substrate>, but I don't see how this can help.  Obviously
this becomes more difficult when you add wild cards to the OPPs, where
a particular OPP would be suitable for all cuts for example.

If you still think you can come up with a generic method to lay out
CPUFreq OPP nodes that will satisfy all vendors and not be a mass of
10's of separate nodes, then great.  Again, I'm struggling to see how
that might be possible.

What I believe we shouldn't do, is have this blocked forever for the
sake of adding a couple of vendor properties however.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 26-08-15, 13:06, Lee Jones wrote:
> On Wed, 12 Aug 2015, Viresh Kumar wrote:
> 
> > On 11-08-15, 16:17, Lee Jones wrote:
> > > This would work if we only had a single variable to contend with, but
> > > what I showed you in my previous example is that we have 3 variables
> > > to consider; cut (version), pcode and substrate.
> > > 
> > > Using the two (simple) examples I provided, how would your suggestion
> > > look in our case?
> > 
> > So the solution I gave is for picking the microvolt based on pcode.
> > The other two (cut, substrate) aren't about picking microvolt, but if
> > the OPP is available or not. Right?
> 
> 'pcode', 'cut' and 'substrate' all determine whether a given set of
> OPPs an be used on the running platform.  I do not believe that you
> can differentiate between them. 
> 
> > If these terms are generic enough, then we can add something similar
> > to what you have added..
> 
> If it makes it easier, you can treat them as version numbers 2.2.1
> <pcode.cut.substrate>, but I don't see how this can help.  Obviously
> this becomes more difficult when you add wild cards to the OPPs, where
> a particular OPP would be suitable for all cuts for example.
> 
> If you still think you can come up with a generic method to lay out
> CPUFreq OPP nodes that will satisfy all vendors and not be a mass of
> 10's of separate nodes, then great.  Again, I'm struggling to see how
> that might be possible.
> 
> What I believe we shouldn't do, is have this blocked forever for the
> sake of adding a couple of vendor properties however.

I agree and can understand the pain you are feeling..

@Rob/Stephen: Please close this thread soon and let Lee get his work
done :)

-- 
viresh

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

[Rob Herring]

On Wed, Sep 2, 2015 at 3:06 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-08-15, 13:06, Lee Jones wrote:
>> On Wed, 12 Aug 2015, Viresh Kumar wrote:
>>
>> > On 11-08-15, 16:17, Lee Jones wrote:
>> > > This would work if we only had a single variable to contend with, but
>> > > what I showed you in my previous example is that we have 3 variables
>> > > to consider; cut (version), pcode and substrate.
>> > >
>> > > Using the two (simple) examples I provided, how would your suggestion
>> > > look in our case?
>> >
>> > So the solution I gave is for picking the microvolt based on pcode.
>> > The other two (cut, substrate) aren't about picking microvolt, but if
>> > the OPP is available or not. Right?
>>
>> 'pcode', 'cut' and 'substrate' all determine whether a given set of
>> OPPs an be used on the running platform.  I do not believe that you
>> can differentiate between them.
>>
>> > If these terms are generic enough, then we can add something similar
>> > to what you have added..
>>
>> If it makes it easier, you can treat them as version numbers 2.2.1
>> <pcode.cut.substrate>, but I don't see how this can help.  Obviously
>> this becomes more difficult when you add wild cards to the OPPs, where
>> a particular OPP would be suitable for all cuts for example.
>>
>> If you still think you can come up with a generic method to lay out
>> CPUFreq OPP nodes that will satisfy all vendors and not be a mass of
>> 10's of separate nodes, then great.  Again, I'm struggling to see how
>> that might be possible.
>>
>> What I believe we shouldn't do, is have this blocked forever for the
>> sake of adding a couple of vendor properties however.
>
> I agree and can understand the pain you are feeling..
>
> @Rob/Stephen: Please close this thread soon and let Lee get his work
> done :)

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.

Rob

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

[Viresh Kumar]

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 :(

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

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.

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
+  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 at 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 at 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 */
+		};
+	};
+};

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

[Lee Jones]

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 at 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 at 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 */
> +		};
> +	};
> +};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

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 at 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'.

-- 
viresh

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

[Lee Jones]

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 at 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 at 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

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

[Viresh Kumar]

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 at 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

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

[Lee Jones]

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Rob Herring]

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

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

[Stephen Boyd]

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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Viresh Kumar]

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 ?

-- 
viresh

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

[Lee Jones]

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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Viresh Kumar]

On 10-09-15, 09:31, Lee Jones wrote:
> I think you answered your own question.
> 
> No users == !ABI == Strip it out.

Okay, as I have delayed things enough for you, didn't wanted to do
that anymore. And so worked on it despite very tight schedule :)

Below is the refreshed binding changes (I have split that into 3
patches, but kept the diff here for simplicity).

Other than that, all code changes you need to test your driver are
pushed here:

https://git.linaro.org/people/viresh.kumar/linux.git opp/supported-hw-prop-name-v1

I am not gonna post the patches to the lists, until the time existing
patches get reviewed.

-- 
viresh

-------------------------8<-------------------------
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 719603b87353..b652d0403e93 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -45,21 +45,10 @@ Devices supporting OPPs must set their "operating-points-v2" property with
 phandle to a OPP table in their DT node. The OPP core will use this phandle to
 find the operating points for the device.
 
-Devices may want to choose OPP tables at runtime and so can provide a list of
-phandles here. But only *one* of them should be chosen@runtime. This must be
-accompanied by a corresponding "operating-points-names" property, to uniquely
-identify the OPP tables.
-
 If required, this can be extended for SoC vendor specfic bindings. Such bindings
 should be documented as Documentation/devicetree/bindings/power/<vendor>-opp.txt
 and should have a compatible description like: "operating-points-v2-<vendor>".
 
-Optional properties:
-- operating-points-names: Names of OPP tables (required if multiple OPP
-  tables are present), to uniquely identify them. The same list must be present
-  for all the CPUs which are sharing clock/voltage rails and hence the OPP
-  tables.
-
 * OPP Table Node
 
 This describes the OPPs belonging to a device. This node can have following
@@ -117,6 +106,14 @@ properties.
   Entries for multiple regulators must be present in the same order as their
   names are present in 'supply-names' property of the opp-table.
 
+- opp-microvolt-<name>: Named opp-microvolt property. This is exactly similar to
+  the above opp-microvolt property, but allows multiple voltage ranges to be
+  provided for the same OPP. At runtime, the platform can pick a <name> and
+  matching opp-microvolt-<name> property will be enabled for all OPPs. If the
+  platform doesn't pick a specific <name> or the <name> doesn't match with any
+  opp-microvolt-<name> properties, then opp-microvolt property shall be used, if
+  present.
+
 - opp-microamp: The maximum current drawn by the device in microamperes
   considering system specific parameters (such as transients, process, aging,
   maximum operating temperature range etc.) as necessary. This may be used to
@@ -131,6 +128,9 @@ properties.
   as zero for them. If it isn't required for any regulator, then this property
   need not be present.
 
+- opp-microamp-<name>: Named opp-microamp property. Similar to
+  opp-microvolt-<name> property, but for microamp instead.
+
 - clock-latency-ns: Specifies the maximum possible transition latency (in
   nanoseconds) for switching to this OPP from any other OPP.
 
@@ -139,9 +139,27 @@ properties.
   frequency for a short duration of time limited by the device's power, current
   and thermal limits.
 
+- turbo-mode-<name>: Named turbo-mode property. Similar to opp-microvolt-<name>
+  property, but for turbo mode instead.
+
 - opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in
   the table should have this.
 
+- opp-suspend-<name>: Named opp-suspend property. Similar to
+  opp-microvolt-<name> property, but for suspend opp instead.
+
+- opp-supported-hw: User defined array containing a hierarchy of hardware
+  version numbers, supported by the OPP. For example: a platform with hierarchy
+  of three levels of versions (A, B and C), this field should be like <X Y Z>,
+  where X corresponds to Version hierarchy A, Y corresponds to version hierarchy
+  B and Z corresponds to version hierarchy C.
+
+  Each level of hierarchy is represented by a 32 bit value, and so there can be
+  only 32 different supported version per hierarchy. i.e. 1 bit per version. A
+  value of 0xFFFFFFFF will enable the OPP for all versions for that hierarchy
+  level. And a value of 0x00000000 will disable the OPP completely, and so we
+  never want that to happen.
+
 - status: Marks the node enabled/disabled.
 
 Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
@@ -443,7 +461,8 @@ Example 4: Handling multiple regulators
 	};
 };
 
-Example 5: Multiple OPP tables
+Example 5: opp-supported-hw
+(example: three level hierarchy of versions: cuts, substrate and process)
 
 / {
 	cpus {
@@ -452,40 +471,84 @@ Example 5: Multiple OPP tables
 			...
 
 			cpu-supply = <&cpu_supply>
-			operating-points-v2 = <&cpu0_opp_table_slow>, <&cpu0_opp_table_fast>;
-			operating-points-names = "slow", "fast";
+			operating-points-v2 = <&cpu0_opp_table_slow>;
 		};
 	};
 
-	cpu0_opp_table_slow: opp_table_slow {
+	opp_table {
 		compatible = "operating-points-v2";
 		status = "okay";
 		opp-shared;
 
 		opp00 {
+			/*
+			 * Supports all substrate and process versions for 0xF
+			 * cuts, i.e. only first four cuts.
+			 */
+			opp-supported-hw = <0xF 0xFFFFFFFF 0xFFFFFFFF>
 			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <900000 915000 925000>;
 			...
 		};
 
 		opp01 {
+			/*
+			 * Supports:
+			 * - cuts: only one, 6th cut (represented by 6th bit).
+			 * - substrate: supports 16 different substrate versions
+			 * - process: supports 9 different process versions
+			 */
+			opp-supported-hw = <0x20 0xff0000ff 0x0000f4f0>
 			opp-hz = /bits/ 64 <800000000>;
+			opp-microvolt = <900000 915000 925000>;
 			...
 		};
 	};
+};
+
+Example 6: opp-microvolt-<name>, opp-microamp-<name>, turbo-mode-<name>,
+opp-suspend-<name>:
+(example: device with 2 supplies: vcc0 and vcc1, with two possible ranges: slow
+and fast)
+
+/ {
+	cpus {
+		cpu at 0 {
+			compatible = "arm,cortex-a7";
+			...
 
-	cpu0_opp_table_fast: opp_table_fast {
+			vcc0-supply = <&cpu_supply0>;
+			vcc1-supply = <&cpu_supply1>;
+			operating-points-v2 = <&cpu0_opp_table>;
+		};
+	};
+
+	cpu0_opp_table: opp_table0 {
 		compatible = "operating-points-v2";
-		status = "okay";
+		supply-names = "vcc0", "vcc1";
 		opp-shared;
 
-		opp10 {
+		opp00 {
 			opp-hz = /bits/ 64 <1000000000>;
-			...
+			opp-microvolt-slow = <900000 915000 925000>, /* Supply vcc0 */
+					      <910000 925000 935000>; /* Supply vcc1 */
+			opp-microvolt-fast = <970000 975000 985000>, /* Supply vcc0 */
+					     <960000 965000 975000>; /* Supply vcc1 */
+			opp-microamp-slow =  <70000>;
+			opp-microamp-fast =  <71000>;
+			turbo-mode-slow; /* Will marked as turbo only if 'slow' is chosen */
+			opp-suspend-slow; /* Will marked as suspend-opp only if 'slow' is chosen */
 		};
 
-		opp11 {
-			opp-hz = /bits/ 64 <1100000000>;
-			...
+		opp01 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt-slow = <900000 915000 925000>, /* Supply vcc0 */
+					      <910000 925000 935000>; /* Supply vcc1 */
+			opp-microvolt-fast = <970000 975000 985000>, /* Supply vcc0 */
+					     <960000 965000 975000>; /* Supply vcc1 */
+			opp-microamp =  <70000>; /* Will be used for both slow/fast */
+			turbo-mode; /* Always marked as turbo */
+			opp-suspend-fast; /* Will marked as suspend opp only if 'fast' is chosen */
 		};
 	};
 };

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

[Lee Jones]

On Wed, 16 Sep 2015, Viresh Kumar wrote:

> On 10-09-15, 09:31, Lee Jones wrote:
> > I think you answered your own question.
> > 
> > No users == !ABI == Strip it out.
> 
> Okay, as I have delayed things enough for you, didn't wanted to do
> that anymore. And so worked on it despite very tight schedule :)
> 
> Below is the refreshed binding changes (I have split that into 3
> patches, but kept the diff here for simplicity).
> 
> Other than that, all code changes you need to test your driver are
> pushed here:
> 
> https://git.linaro.org/people/viresh.kumar/linux.git opp/supported-hw-prop-name-v1
> 
> I am not gonna post the patches to the lists, until the time existing
> patches get reviewed.

Thanks dude.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Rob Herring]

On Mon, Jul 27, 2015 at 10:20 AM, Lee Jones <lee.jones@linaro.org> wrote:
> These OPPs are used in ST's CPUFreq implementation.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>
> Changelog:
>  - None, new patch
>
>  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
>
> diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> new file mode 100644
> index 0000000..6eb2a91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> @@ -0,0 +1,76 @@
> +STMicroelectronics OPP (Operating Performance Points) Bindings
> +--------------------------------------------------------------
> +
> +Frequency Scaling only
> +----------------------
> +
> +Located in CPU's node:
> +
> +- operating-points     : [See: ./opp.txt]
> +
> +Example [safe]
> +--------------
> +
> +cpus {
> +       cpu at 0 {
> +                                /* kHz     uV   */
> +               operating-points = <1500000 0
> +                                   1200000 0
> +                                   800000  0
> +                                   500000  0>;
> +       };
> +};
> +
> +Dynamic Voltage and Frequency Scaling (DVFS)
> +--------------------------------------------
> +
> +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> +
> +- compatible           : Should be "operating-points-v2-sti"
> +- opp{1..N}            : Each 'oppX' subnode will contain the following properties:
> + - opp-hz              : CPU frequency [Hz] for this OPP [See: ./opp.txt]
> + - st,avs              : List of available voltages [uV] indexed by process code

Add a unit suffix (-microvolt).

> + - st,cuts             : Cut version this OPP is suitable for [0xFF means ALL]
> + - st,substrate                : Substrate version this OPP is suitable for [0xFF means ALL]

How about not present means all?

> +- st,syscfg            : Phandle to Major number register
> +                               First cell: offset to major number
> +- st,syscfg-eng                : Phandle to Minor number and Pcode registers
> +                               First cell: offset to process code
> +                               Second cell: offset to minor number

Would the proposed nvmem binding work for this?

Rob

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

[Lee Jones]

On Tue, 28 Jul 2015, Rob Herring wrote:

> On Mon, Jul 27, 2015 at 10:20 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > These OPPs are used in ST's CPUFreq implementation.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >
> > Changelog:
> >  - None, new patch
> >
> >  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
> >
> > diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> > new file mode 100644
> > index 0000000..6eb2a91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> > @@ -0,0 +1,76 @@
> > +STMicroelectronics OPP (Operating Performance Points) Bindings
> > +--------------------------------------------------------------
> > +
> > +Frequency Scaling only
> > +----------------------
> > +
> > +Located in CPU's node:
> > +
> > +- operating-points     : [See: ./opp.txt]
> > +
> > +Example [safe]
> > +--------------
> > +
> > +cpus {
> > +       cpu at 0 {
> > +                                /* kHz     uV   */
> > +               operating-points = <1500000 0
> > +                                   1200000 0
> > +                                   800000  0
> > +                                   500000  0>;
> > +       };
> > +};
> > +
> > +Dynamic Voltage and Frequency Scaling (DVFS)
> > +--------------------------------------------
> > +
> > +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> > +
> > +- compatible           : Should be "operating-points-v2-sti"
> > +- opp{1..N}            : Each 'oppX' subnode will contain the following properties:
> > + - opp-hz              : CPU frequency [Hz] for this OPP [See: ./opp.txt]
> > + - st,avs              : List of available voltages [uV] indexed by process code
> 
> Add a unit suffix (-microvolt).

Sure.

> > + - st,cuts             : Cut version this OPP is suitable for [0xFF means ALL]
> > + - st,substrate                : Substrate version this OPP is suitable for [0xFF means ALL]
> 
> How about not present means all?

I think that would be an unsafe assumption.  If it's forgotten, then
we may try to run an invalid/dangerous voltage/frequency combination.

I'd really like 'all' to be defined an deliberate.

> > +- st,syscfg            : Phandle to Major number register
> > +                               First cell: offset to major number
> > +- st,syscfg-eng                : Phandle to Minor number and Pcode registers
> > +                               First cell: offset to process code
> > +                               Second cell: offset to minor number
> 
> Would the proposed nvmem binding work for this?

We already use sysconf for all of this type of stuff, as this
information is contained in ST's Sysconf banks.  Moving over to a new
API would be a huge move and would require lots of planning
discussions with ST.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Rob Herring]

On Tue, Jul 28, 2015 at 9:39 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 28 Jul 2015, Rob Herring wrote:
>
>> On Mon, Jul 27, 2015 at 10:20 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > These OPPs are used in ST's CPUFreq implementation.
>> >
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> > ---
>> >
>> > Changelog:
>> >  - None, new patch
>> >
>> >  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
>> >  1 file changed, 76 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
>> > new file mode 100644
>> > index 0000000..6eb2a91
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/power/opp-st.txt
>> > @@ -0,0 +1,76 @@
>> > +STMicroelectronics OPP (Operating Performance Points) Bindings
>> > +--------------------------------------------------------------
>> > +
>> > +Frequency Scaling only
>> > +----------------------
>> > +
>> > +Located in CPU's node:
>> > +
>> > +- operating-points     : [See: ./opp.txt]
>> > +
>> > +Example [safe]
>> > +--------------
>> > +
>> > +cpus {
>> > +       cpu at 0 {
>> > +                                /* kHz     uV   */
>> > +               operating-points = <1500000 0
>> > +                                   1200000 0
>> > +                                   800000  0
>> > +                                   500000  0>;
>> > +       };
>> > +};
>> > +
>> > +Dynamic Voltage and Frequency Scaling (DVFS)
>> > +--------------------------------------------
>> > +
>> > +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
>> > +
>> > +- compatible           : Should be "operating-points-v2-sti"
>> > +- opp{1..N}            : Each 'oppX' subnode will contain the following properties:
>> > + - opp-hz              : CPU frequency [Hz] for this OPP [See: ./opp.txt]
>> > + - st,avs              : List of available voltages [uV] indexed by process code
>>
>> Add a unit suffix (-microvolt).
>
> Sure.
>
>> > + - st,cuts             : Cut version this OPP is suitable for [0xFF means ALL]
>> > + - st,substrate                : Substrate version this OPP is suitable for [0xFF means ALL]
>>
>> How about not present means all?
>
> I think that would be an unsafe assumption.  If it's forgotten, then
> we may try to run an invalid/dangerous voltage/frequency combination.
>
> I'd really like 'all' to be defined an deliberate.

Okay.

>> > +- st,syscfg            : Phandle to Major number register
>> > +                               First cell: offset to major number
>> > +- st,syscfg-eng                : Phandle to Minor number and Pcode registers
>> > +                               First cell: offset to process code
>> > +                               Second cell: offset to minor number
>>
>> Would the proposed nvmem binding work for this?
>
> We already use sysconf for all of this type of stuff, as this
> information is contained in ST's Sysconf banks.  Moving over to a new
> API would be a huge move and would require lots of planning
> discussions with ST.

Okay.

Rob

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

[Lee Jones]

On Tue, 28 Jul 2015, Rob Herring wrote:

> On Tue, Jul 28, 2015 at 9:39 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 28 Jul 2015, Rob Herring wrote:
> >
> >> On Mon, Jul 27, 2015 at 10:20 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > These OPPs are used in ST's CPUFreq implementation.
> >> >
> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> > ---
> >> >
> >> > Changelog:
> >> >  - None, new patch
> >> >
> >> >  Documentation/devicetree/bindings/power/opp-st.txt | 76 ++++++++++++++++++++++
> >> >  1 file changed, 76 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/power/opp-st.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/power/opp-st.txt b/Documentation/devicetree/bindings/power/opp-st.txt
> >> > new file mode 100644
> >> > index 0000000..6eb2a91
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/power/opp-st.txt
> >> > @@ -0,0 +1,76 @@
> >> > +STMicroelectronics OPP (Operating Performance Points) Bindings
> >> > +--------------------------------------------------------------
> >> > +
> >> > +Frequency Scaling only
> >> > +----------------------
> >> > +
> >> > +Located in CPU's node:
> >> > +
> >> > +- operating-points     : [See: ./opp.txt]
> >> > +
> >> > +Example [safe]
> >> > +--------------
> >> > +
> >> > +cpus {
> >> > +       cpu at 0 {
> >> > +                                /* kHz     uV   */
> >> > +               operating-points = <1500000 0
> >> > +                                   1200000 0
> >> > +                                   800000  0
> >> > +                                   500000  0>;
> >> > +       };
> >> > +};
> >> > +
> >> > +Dynamic Voltage and Frequency Scaling (DVFS)
> >> > +--------------------------------------------
> >> > +
> >> > +Located in 'cpu0-opp-list' node [to be provided ONLY by the bootloader]:
> >> > +
> >> > +- compatible           : Should be "operating-points-v2-sti"
> >> > +- opp{1..N}            : Each 'oppX' subnode will contain the following properties:
> >> > + - opp-hz              : CPU frequency [Hz] for this OPP [See: ./opp.txt]
> >> > + - st,avs              : List of available voltages [uV] indexed by process code
> >>
> >> Add a unit suffix (-microvolt).
> >
> > Sure.
> >
> >> > + - st,cuts             : Cut version this OPP is suitable for [0xFF means ALL]
> >> > + - st,substrate                : Substrate version this OPP is suitable for [0xFF means ALL]
> >>
> >> How about not present means all?
> >
> > I think that would be an unsafe assumption.  If it's forgotten, then
> > we may try to run an invalid/dangerous voltage/frequency combination.
> >
> > I'd really like 'all' to be defined an deliberate.
> 
> Okay.
> 
> >> > +- st,syscfg            : Phandle to Major number register
> >> > +                               First cell: offset to major number
> >> > +- st,syscfg-eng                : Phandle to Minor number and Pcode registers
> >> > +                               First cell: offset to process code
> >> > +                               Second cell: offset to minor number
> >>
> >> Would the proposed nvmem binding work for this?
> >
> > We already use sysconf for all of this type of stuff, as this
> > information is contained in ST's Sysconf banks.  Moving over to a new
> > API would be a huge move and would require lots of planning
> > discussions with ST.
> 
> Okay.

Thanks.

I'm going to fix up your suggestions above and add your Ack to make
Viresh happy. :)

Thanks Rob.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Viresh Kumar]

On 27-07-15, 16:20, Lee Jones wrote:
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> 
> Changelog:
>  - Using OPP-v2
>  - Moved (and linked) a bunch of documentation to ../power/
> 
>  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> new file mode 100644
> index 0000000..79add9d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> @@ -0,0 +1,40 @@
> +Binding for ST's CPUFreq driver
> +===============================
> +
> +Frequency Scaling only
> +----------------------
> +
> +Located in CPU's node:
> +
> +- operating-points		: [See: ../power/opp.txt]
> +
> +Example [safe]
> +--------------
> +
> +cpus {
> +	cpu at 0 {
> +				 /* kHz     uV   */
> +		operating-points = <1500000 0
> +				    1200000 0
> +				    800000  0
> +				    500000  0>;
> +	};
> +};
> +
> +Dynamic Voltage and Frequency Scaling (DVFS)
> +--------------------------------------------
> +
> +Located in CPU's node:
> +
> +- operating-points-v2-sti	: [See ../power/opp-st.txt]
> +
> +Example [unsafe]
> +----------------
> +
> +cpus {
> +	cpu at 0 {
> +		operating-points-v2	= <[OPP list phandle]>;
> +	};
> +};
> +
> +For an example of an OPP list, see ../power/opp-st.txt.

I don't think you need this file/patch at all.. It is almost blank and
doesn't define a binding. And so there is no need to specify this at
all. Just an OPP binding file is enough.

-- 
viresh

[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

On Tue, 28 Jul 2015, Viresh Kumar wrote:

> On 27-07-15, 16:20, Lee Jones wrote:
> > Cc: devicetree at vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > 
> > Changelog:
> >  - Using OPP-v2
> >  - Moved (and linked) a bunch of documentation to ../power/
> > 
> >  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 40 ++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> > new file mode 100644
> > index 0000000..79add9d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> > @@ -0,0 +1,40 @@
> > +Binding for ST's CPUFreq driver
> > +===============================
> > +
> > +Frequency Scaling only
> > +----------------------
> > +
> > +Located in CPU's node:
> > +
> > +- operating-points		: [See: ../power/opp.txt]
> > +
> > +Example [safe]
> > +--------------
> > +
> > +cpus {
> > +	cpu at 0 {
> > +				 /* kHz     uV   */
> > +		operating-points = <1500000 0
> > +				    1200000 0
> > +				    800000  0
> > +				    500000  0>;
> > +	};
> > +};
> > +
> > +Dynamic Voltage and Frequency Scaling (DVFS)
> > +--------------------------------------------
> > +
> > +Located in CPU's node:
> > +
> > +- operating-points-v2-sti	: [See ../power/opp-st.txt]
> > +
> > +Example [unsafe]
> > +----------------
> > +
> > +cpus {
> > +	cpu at 0 {
> > +		operating-points-v2	= <[OPP list phandle]>;
> > +	};
> > +};
> > +
> > +For an example of an OPP list, see ../power/opp-st.txt.
> 
> I don't think you need this file/patch at all.. It is almost blank and
> doesn't define a binding. And so there is no need to specify this at
> all. Just an OPP binding file is enough.

I have two issues with that.  Firstly, although the driver uses the
OPP API (it also uses the Regulator and Clock API too), it is
fundamentally a CPUFreq driver, so I think it should have a CPUFreq
DT entry.  Secondly, if someone doesn't know the history of the
ST CPUFreq set, they will look here for an accompanying document.  I
personally wouldn't think to look in power/*opp* for a CPUFreq
binding.

Perhaps, as all of the CPUFreq drivers use the OPP API, everything
should be moved to drivers/base/power or drivers/power?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Viresh Kumar]

Cc'ing Rob as well..

On 28-07-15, 08:41, Lee Jones wrote:
> I have two issues with that.  Firstly, although the driver uses the
> OPP API (it also uses the Regulator and Clock API too), it is
> fundamentally a CPUFreq driver, so I think it should have a CPUFreq
> DT entry.  Secondly, if someone doesn't know the history of the
> ST CPUFreq set, they will look here for an accompanying document.  I
> personally wouldn't think to look in power/*opp* for a CPUFreq
> binding.
> 
> Perhaps, as all of the CPUFreq drivers use the OPP API, everything
> should be moved to drivers/base/power or drivers/power?

Okay, looks fine :)

-- 
viresh

[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Viresh Kumar]

On 27-07-15, 16:20, Lee Jones wrote:
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> 
> Changelog:
>  - Using OPP-v2
>  - Moved (and linked) a bunch of documentation to ../power/
> 
>  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt

For both patches:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

But I would like Rob to have a look as well :)
-- 
viresh

[PATCH v4 1/2] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

On Tue, 28 Jul 2015, Viresh Kumar wrote:

> On 27-07-15, 16:20, Lee Jones wrote:
> > Cc: devicetree at vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > 
> > Changelog:
> >  - Using OPP-v2
> >  - Moved (and linked) a bunch of documentation to ../power/
> > 
> >  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 40 ++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> 
> For both patches:
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

W00t!  I'll start fixing up the driver.

> But I would like Rob to have a look as well :)

No problem.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog