diff mbox

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

Message ID 1438010430-5802-2-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones July 27, 2015, 3:20 p.m. UTC
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

Comments

Viresh Kumar July 28, 2015, 2:29 a.m. UTC | #1
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@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@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 :)
Lee Jones July 28, 2015, 7:34 a.m. UTC | #2
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@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@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. ;)
Viresh Kumar July 28, 2015, 7:47 a.m. UTC | #3
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. :)
Lee Jones July 28, 2015, 8:30 a.m. UTC | #4
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 July 28, 2015, 2:39 p.m. UTC | #5
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@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 July 28, 2015, 3:43 p.m. UTC | #6
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@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 July 29, 2015, 8:14 a.m. UTC | #7
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@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@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 July 30, 2015, 8:46 a.m. UTC | #8
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.
diff mbox

Patch

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@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@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>;
+	};
+};