[8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

Message ID 1434987837-24212-9-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones June 22, 2015, 3:43 p.m.
Cc: devicetree@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt

Comments

Viresh Kumar June 23, 2015, 2:34 a.m. | #1
[Adding Rob]

On 22-06-15, 16:43, Lee Jones wrote:

At least some description was required here on why you need additional
bindings are what are they.

Over that, this patch should have been present before any other
patches using these bindings.

> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 48 ++++++++++++++++++++++
>  1 file changed, 48 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..cfa8952
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> @@ -0,0 +1,48 @@
> +Binding for ST's CPUFreq driver
> +===============================

OPP-v2 bindings are out now and you can probably use them to make life
simple, they are part of Rafael's recent pull request:
https://lkml.org/lkml/2015/6/22/606

> +Required properties:
> +-------------------
> +- compatible		: Supported values are:
> +				"st,stih407-cpufreq"

Nodes for virtual devices aren't allowed in DT.

> +Required properties [for working voltage scaling]:
> +-------------------------------------------------
> +
> +Located in CPUFreq's node:
> +
> +- 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
> +
> +Located in CPU's node:
> +
> +- st,opp-list		: Bootloader provided node containing one or more 'opp@X' sub-nodes

I can see that this will be passed in from the bootloader. But at
least an example on how exactly things would actually look would have
been good. In logs if not in this file.
Lee Jones June 23, 2015, 7:06 a.m. | #2
> [Adding Rob]

Rob is not the only DT Maintainer, there are many of them.  The DT
list was CC'ed, which they are all part of.  Adding them all
separately is not required IMO.

> On 22-06-15, 16:43, Lee Jones wrote:
> 
> At least some description was required here on why you need additional
> bindings are what are they.

Sure, I can do that.

> Over that, this patch should have been present before any other
> patches using these bindings.

I've never heard that one before, but it's easy to re-order the set.

> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 48 ++++++++++++++++++++++
> >  1 file changed, 48 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..cfa8952
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> > @@ -0,0 +1,48 @@
> > +Binding for ST's CPUFreq driver
> > +===============================
> 
> OPP-v2 bindings are out now and you can probably use them to make life
> simple, they are part of Rafael's recent pull request:
> https://lkml.org/lkml/2015/6/22/606
> 
> > +Required properties:
> > +-------------------
> > +- compatible		: Supported values are:
> > +				"st,stih407-cpufreq"
> 
> Nodes for virtual devices aren't allowed in DT.

Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?

One rule for one ... ?

> > +Required properties [for working voltage scaling]:
> > +-------------------------------------------------
> > +
> > +Located in CPUFreq's node:
> > +
> > +- 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
> > +
> > +Located in CPU's node:
> > +
> > +- st,opp-list		: Bootloader provided node containing one or more 'opp@X' sub-nodes
> 
> I can see that this will be passed in from the bootloader. But at
> least an example on how exactly things would actually look would have
> been good. In logs if not in this file.

Sure.
Viresh Kumar June 23, 2015, 7:55 a.m. | #3
On 23-06-15, 08:06, Lee Jones wrote:
> > [Adding Rob]
> 
> Rob is not the only DT Maintainer, there are many of them.  The DT
> list was CC'ed, which they are all part of.  Adding them all
> separately is not required IMO.

I didn't Cc him because you missed him, but because we have been
discussing opp-v2 bindings recently and this was somehow related to
that. :)

> > On 22-06-15, 16:43, Lee Jones wrote:
> > 
> > At least some description was required here on why you need additional
> > bindings are what are they.
> 
> Sure, I can do that.
> 
> > Over that, this patch should have been present before any other
> > patches using these bindings.
> 
> I've never heard that one before, but it's easy to re-order the set.

I don't know, but it seems obvious to me: Bindings first and then the
code.

> > > +Required properties:
> > > +-------------------
> > > +- compatible		: Supported values are:
> > > +				"st,stih407-cpufreq"
> > 
> > Nodes for virtual devices aren't allowed in DT.
> 
> Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?
> 
> One rule for one ... ?

Not really, but I got a bit confused now with your reply.

So, what I meant when I wrote: "Nodes for virtual devices aren't
allowed in DT", was that we aren't supposed to do something like:

cpufreq {
 ...
}

in DT as cpufreq isn't a device here. A CPU is a device and that can
contain whatever property we feel is reasonable.

What SPEAr and Exyons did was putting something in the cpu-node. Not a
node for cpufreq device itself. Couldn't find HREF and snowball's
bindings though..
Lee Jones June 23, 2015, 8:38 a.m. | #4
On Tue, 23 Jun 2015, Viresh Kumar wrote:
> On 23-06-15, 08:06, Lee Jones wrote:
> > > [Adding Rob]
> > 
> > Rob is not the only DT Maintainer, there are many of them.  The DT
> > list was CC'ed, which they are all part of.  Adding them all
> > separately is not required IMO.
> 
> I didn't Cc him because you missed him, but because we have been
> discussing opp-v2 bindings recently and this was somehow related to
> that. :)

Okay, fair point.

> > > On 22-06-15, 16:43, Lee Jones wrote:
> > > 
> > > At least some description was required here on why you need additional
> > > bindings are what are they.
> > 
> > Sure, I can do that.
> > 
> > > Over that, this patch should have been present before any other
> > > patches using these bindings.
> > 
> > I've never heard that one before, but it's easy to re-order the set.
> 
> I don't know, but it seems obvious to me: Bindings first and then the
> code.

Do you always write your documentation before implementing a
feature?

Surely it goes;
  Requirements Gathering
  Plan and Prepare
  Implement
  Test
  Document
  Deliver

;)

... but as I say, I can re-order if required.  It's really not a problem.

> > > > +Required properties:
> > > > +-------------------
> > > > +- compatible		: Supported values are:
> > > > +				"st,stih407-cpufreq"
> > > 
> > > Nodes for virtual devices aren't allowed in DT.
> > 
> > Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?
> > 
> > One rule for one ... ?
> 
> Not really, but I got a bit confused now with your reply.
> 
> So, what I meant when I wrote: "Nodes for virtual devices aren't
> allowed in DT", was that we aren't supposed to do something like:
> 
> cpufreq {
>  ...
> }
> 
> in DT as cpufreq isn't a device here. A CPU is a device and that can
> contain whatever property we feel is reasonable.
> 
> What SPEAr and Exyons did was putting something in the cpu-node. Not a
> node for cpufreq device itself. Couldn't find HREF and snowball's
> bindings though..

That's not what it looks like to me:

git grep -C20 "compatible.*cpufreq" -- arch
Viresh Kumar June 23, 2015, 9 a.m. | #5
On 23-06-15, 09:38, Lee Jones wrote:
> Do you always write your documentation before implementing a
> feature?
> 
> Surely it goes;
>   Requirements Gathering
>   Plan and Prepare
>   Implement
>   Test
>   Document
>   Deliver
> 
> ;)

DT bindings aren't just simple documentation of how things are
working. I do things in the way you wrote earlier though :)

> That's not what it looks like to me:
> 
> git grep -C20 "compatible.*cpufreq" -- arch

Yeah, that's wrong. They got in much earlier and the bindings weren't
well thought through.

But they are discouraged today ..

Patch

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
new file mode 100644
index 0000000..cfa8952
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
@@ -0,0 +1,48 @@ 
+Binding for ST's CPUFreq driver
+===============================
+
+Required properties:
+-------------------
+- compatible		: Supported values are:
+				"st,stih407-cpufreq"
+
+Required properties [for working voltage scaling]:
+-------------------------------------------------
+
+Located in CPUFreq's node:
+
+- 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
+
+Located in CPU's node:
+
+- st,opp-list		: Bootloader provided node containing one or more 'opp@X' sub-nodes
+ - opp@{1..X} 		: Each 'opp@X' subnode will contain the following properties:
+  - st,avs		: List of available voltages [uV] indexed by process code
+  - st,freq		: CPU frequency [Hz] for this OPP
+  - st,cuts		: Cut version this OPP is suitable for [0xFF means ALL]
+  - st,substrate	: Substrate version this OPP is suitable for [0xFF means ALL]
+
+WARNING: The st,opp-list will be provided by the bootloader.  Do not attempt to
+	 artificially synthesise the st,opp-list node or any of its descendants.
+	 They are very platform specific and may damage the hardware if created
+	 incorrectly.
+
+Required properties [if the voltage scaling properties are missing]:
+-------------------------------------------------------------------
+
+Located in CPU's node:
+
+- operating-points	: [See: ../power/opp.txt]
+
+Example:
+-------
+
+cpufreq {
+	compatible	= "st,stih407-cpufreq";
+	st,syscfg	= <&syscfg [major_offset]>;
+	st,syscfg-eng	= <&syscfg_eng [pcode_offset] [minor_offset]>;
+};