diff mbox

[v4,01/12] dt-bindings: thermal: Describe Armada AP806 and CP110

Message ID 20171218143643.7714-2-miquel.raynal@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Miquel Raynal Dec. 18, 2017, 2:36 p.m. UTC
From: Baruch Siach <baruch@tkos.co.il>

Add compatible strings for AP806 and CP110 that are part of the Armada
8k/7k line of SoCs.

Add a note on the differences in the size of the control area in
different bindings. This is an existing difference between the Armada
375 binding and the other boards already supported. The new AP806 and
CP110 bindings are similar to the existing Armada 375 in this regard.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
[<miquel.raynal@free-electrons.com>: reword, additional details]
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 .../devicetree/bindings/thermal/armada-thermal.txt | 24 +++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Comments

Baruch Siach Dec. 18, 2017, 8:33 p.m. UTC | #1
Hi Miquèl,

On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:
> From: Baruch Siach <baruch@tkos.co.il>
> 
> Add compatible strings for AP806 and CP110 that are part of the Armada
> 8k/7k line of SoCs.
> 
> Add a note on the differences in the size of the control area in
> different bindings. This is an existing difference between the Armada
> 375 binding and the other boards already supported. The new AP806 and
> CP110 bindings are similar to the existing Armada 375 in this regard.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> [<miquel.raynal@free-electrons.com>: reword, additional details]
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  .../devicetree/bindings/thermal/armada-thermal.txt | 24 +++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> index 24aacf8948c5..9b7b2c03cc6f 100644
> --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -7,17 +7,31 @@ Required properties:
>  		marvell,armada375-thermal
>  		marvell,armada380-thermal
>  		marvell,armadaxp-thermal
> +		marvell,armada-ap806-thermal
> +		marvell,armada-cp110-thermal
>  
>  - reg:		Device's register space.
>  		Two entries are expected, see the examples below.
> -		The first one is required for the sensor register;
> -		the second one is required for the control register
> -		to be used for sensor initialization (a.k.a. calibration).
> +		The first one points to the status register (4B).
> +		The second one points to the control registers (8B).
> +		Note: with legacy bindings, the second entry pointed
> +		only to the so called "control MSB" ("control 1"), was
> +		4B wide and did not let the possibility to reach the
> +		"control LSB" ("control 0") register. This is only
> +		allowed for compatibility reasons in Armada
> +		370/375/38x/XP DT nodes.

"allowed" is not the right term, IMO. Legacy compatibles MUST point to the MSB 
control register to preserve compatibility with existing DTs.

The original patch had a list of legacy and non-legacy compatibles. I think we 
need to keep them.

baruch

> -Example:
> +Examples:
>  
> +	/* Legacy bindings */
>  	thermal@d0018300 {
>  		compatible = "marvell,armada370-thermal";
> -                reg = <0xd0018300 0x4
> +		reg = <0xd0018300 0x4
>  		       0xd0018304 0x4>;
>  	};
> +
> +	ap_thermal: thermal@6f8084 {
> +		compatible = "marvell,armada-ap806-thermal";
> +		reg = <0x6f808C 0x4>,
> +		      <0x6f8084 0x8>;
> +	};
Miquel Raynal Dec. 19, 2017, 12:43 a.m. UTC | #2
Hello Baruch,

On Mon, 18 Dec 2017 22:33:24 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Miquèl,
> 
> On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:
> > From: Baruch Siach <baruch@tkos.co.il>
> > 
> > Add compatible strings for AP806 and CP110 that are part of the
> > Armada 8k/7k line of SoCs.
> > 
> > Add a note on the differences in the size of the control area in
> > different bindings. This is an existing difference between the
> > Armada 375 binding and the other boards already supported. The new
> > AP806 and CP110 bindings are similar to the existing Armada 375 in
> > this regard.
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > [<miquel.raynal@free-electrons.com>: reword, additional details]
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  .../devicetree/bindings/thermal/armada-thermal.txt | 24
> > +++++++++++++++++----- 1 file changed, 19 insertions(+), 5
> > deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > index 24aacf8948c5..9b7b2c03cc6f 100644 ---
> > a/Documentation/devicetree/bindings/thermal/armada-thermal.txt +++
> > b/Documentation/devicetree/bindings/thermal/armada-thermal.txt @@
> > -7,17 +7,31 @@ Required properties: marvell,armada375-thermal
> > marvell,armada380-thermal marvell,armadaxp-thermal
> > +		marvell,armada-ap806-thermal
> > +		marvell,armada-cp110-thermal
> >  
> >  - reg:		Device's register space.
> >  		Two entries are expected, see the examples below.
> > -		The first one is required for the sensor register;
> > -		the second one is required for the control register
> > -		to be used for sensor initialization (a.k.a.
> > calibration).
> > +		The first one points to the status register (4B).
> > +		The second one points to the control registers
> > (8B).
> > +		Note: with legacy bindings, the second entry
> > pointed
> > +		only to the so called "control MSB" ("control 1"),
> > was
> > +		4B wide and did not let the possibility to reach
> > the
> > +		"control LSB" ("control 0") register. This is only
> > +		allowed for compatibility reasons in Armada
> > +		370/375/38x/XP DT nodes.  
> 
> "allowed" is not the right term, IMO. Legacy compatibles MUST point
> to the MSB control register to preserve compatibility with existing
> DTs.
> 
> The original patch had a list of legacy and non-legacy compatibles. I
> think we need to keep them.

Maybe I should reword this paragraph because we both agree on the
meaning:

"
Note: Legacy bindings are only supported with Armada 370/375/38x/XP
compatibles. The second memory resource entry only points to
"control MSB/control 1", is 4 bytes wide and is preventing any access
to "control LSB/control 0".
"

Does this sounds better to you?

Thank you,
Miquèl

> 
> baruch
> 
> > -Example:
> > +Examples:
> >  
> > +	/* Legacy bindings */
> >  	thermal@d0018300 {
> >  		compatible = "marvell,armada370-thermal";
> > -                reg = <0xd0018300 0x4
> > +		reg = <0xd0018300 0x4
> >  		       0xd0018304 0x4>;
> >  	};
> > +
> > +	ap_thermal: thermal@6f8084 {
> > +		compatible = "marvell,armada-ap806-thermal";
> > +		reg = <0x6f808C 0x4>,
> > +		      <0x6f8084 0x8>;
> > +	};  
>
Baruch Siach Dec. 19, 2017, 6:09 a.m. UTC | #3
Hi Miquèl,

On Tue, Dec 19, 2017 at 01:43:20AM +0100, Miquel RAYNAL wrote:
> On Mon, 18 Dec 2017 22:33:24 +0200
> Baruch Siach <baruch@tkos.co.il> wrote:
> > On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:
> > > From: Baruch Siach <baruch@tkos.co.il>
> > > 
> > > Add compatible strings for AP806 and CP110 that are part of the
> > > Armada 8k/7k line of SoCs.
> > > 
> > > Add a note on the differences in the size of the control area in
> > > different bindings. This is an existing difference between the
> > > Armada 375 binding and the other boards already supported. The new
> > > AP806 and CP110 bindings are similar to the existing Armada 375 in
> > > this regard.
> > > 
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > [<miquel.raynal@free-electrons.com>: reword, additional details]
> > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > > ---
> > >  .../devicetree/bindings/thermal/armada-thermal.txt | 24
> > > +++++++++++++++++----- 1 file changed, 19 insertions(+), 5
> > > deletions(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > index 24aacf8948c5..9b7b2c03cc6f 100644 ---
> > > a/Documentation/devicetree/bindings/thermal/armada-thermal.txt +++
> > > b/Documentation/devicetree/bindings/thermal/armada-thermal.txt @@
> > > -7,17 +7,31 @@ Required properties: marvell,armada375-thermal
> > > marvell,armada380-thermal marvell,armadaxp-thermal
> > > +		marvell,armada-ap806-thermal
> > > +		marvell,armada-cp110-thermal
> > >  
> > >  - reg:		Device's register space.
> > >  		Two entries are expected, see the examples below.
> > > -		The first one is required for the sensor register;
> > > -		the second one is required for the control register
> > > -		to be used for sensor initialization (a.k.a.
> > > calibration).
> > > +		The first one points to the status register (4B).
> > > +		The second one points to the control registers
> > > (8B).
> > > +		Note: with legacy bindings, the second entry
> > > pointed
> > > +		only to the so called "control MSB" ("control 1"),
> > > was
> > > +		4B wide and did not let the possibility to reach
> > > the
> > > +		"control LSB" ("control 0") register. This is only
> > > +		allowed for compatibility reasons in Armada
> > > +		370/375/38x/XP DT nodes.  
> > 
> > "allowed" is not the right term, IMO. Legacy compatibles MUST point
> > to the MSB control register to preserve compatibility with existing
> > DTs.
> > 
> > The original patch had a list of legacy and non-legacy compatibles. I
> > think we need to keep them.
> 
> Maybe I should reword this paragraph because we both agree on the
> meaning:
> 
> "
> Note: Legacy bindings are only supported with Armada 370/375/38x/XP
> compatibles. The second memory resource entry only points to
> "control MSB/control 1", is 4 bytes wide and is preventing any access
> to "control LSB/control 0".
> "
> 
> Does this sounds better to you?

I think we need to explicitly list the affected compatible strings. Something 
like:

  For backwards compatibility reasons, the compatibles 
  marvell,armada370-thermal, marvell,armada380-thermal, and 
  marvell,armadaxp-thermal must point to "control MSB/control 1", with size of 
  4. All other compatibles must point to "control LSB/control 0" with size of
  8.

But I think you are right that we can use the size of the control registers to  
tell whether e.g. marvell,armada380-thermal is of the old binding of the new 
one. So maybe the "allow" language is more correct. But let's make it explicit 
to avoid any doubt. How about:

  The compatibles marvell,armada370-thermal, marvell,armada380-thermal, and 
  marvell,armadaxp-thermal must point to "control MSB/control 1", with size of 
  4 (deprecated binding), or point to "control LSB/control 0" with size of 8 
  (current binding). All other compatibles must point to "control LSB/control 
  0" with size of 8.

baruch
Miquel Raynal Dec. 19, 2017, 7:44 a.m. UTC | #4
Hello Baruch,

On Tue, 19 Dec 2017 08:09:18 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Miquèl,
> 
> On Tue, Dec 19, 2017 at 01:43:20AM +0100, Miquel RAYNAL wrote:
> > On Mon, 18 Dec 2017 22:33:24 +0200
> > Baruch Siach <baruch@tkos.co.il> wrote:  
> > > On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:  
> > > > From: Baruch Siach <baruch@tkos.co.il>
> > > > 
> > > > Add compatible strings for AP806 and CP110 that are part of the
> > > > Armada 8k/7k line of SoCs.
> > > > 
> > > > Add a note on the differences in the size of the control area in
> > > > different bindings. This is an existing difference between the
> > > > Armada 375 binding and the other boards already supported. The
> > > > new AP806 and CP110 bindings are similar to the existing Armada
> > > > 375 in this regard.
> > > > 
> > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > > [<miquel.raynal@free-electrons.com>: reword, additional details]
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > > > ---
> > > >  .../devicetree/bindings/thermal/armada-thermal.txt | 24
> > > > +++++++++++++++++----- 1 file changed, 19 insertions(+), 5
> > > > deletions(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > index 24aacf8948c5..9b7b2c03cc6f 100644 ---
> > > > a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > +++
> > > > b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > @@ -7,17 +7,31 @@ Required properties:
> > > > marvell,armada375-thermal marvell,armada380-thermal
> > > > marvell,armadaxp-thermal
> > > > +		marvell,armada-ap806-thermal
> > > > +		marvell,armada-cp110-thermal
> > > >  
> > > >  - reg:		Device's register space.
> > > >  		Two entries are expected, see the examples
> > > > below.
> > > > -		The first one is required for the sensor
> > > > register;
> > > > -		the second one is required for the control
> > > > register
> > > > -		to be used for sensor initialization (a.k.a.
> > > > calibration).
> > > > +		The first one points to the status register
> > > > (4B).
> > > > +		The second one points to the control registers
> > > > (8B).
> > > > +		Note: with legacy bindings, the second entry
> > > > pointed
> > > > +		only to the so called "control MSB" ("control
> > > > 1"), was
> > > > +		4B wide and did not let the possibility to
> > > > reach the
> > > > +		"control LSB" ("control 0") register. This is
> > > > only
> > > > +		allowed for compatibility reasons in Armada
> > > > +		370/375/38x/XP DT nodes.    
> > > 
> > > "allowed" is not the right term, IMO. Legacy compatibles MUST
> > > point to the MSB control register to preserve compatibility with
> > > existing DTs.
> > > 
> > > The original patch had a list of legacy and non-legacy
> > > compatibles. I think we need to keep them.  
> > 
> > Maybe I should reword this paragraph because we both agree on the
> > meaning:
> > 
> > "
> > Note: Legacy bindings are only supported with Armada 370/375/38x/XP
> > compatibles. The second memory resource entry only points to
> > "control MSB/control 1", is 4 bytes wide and is preventing any
> > access to "control LSB/control 0".
> > "
> > 
> > Does this sounds better to you?  
> 
> I think we need to explicitly list the affected compatible strings.
> Something like:

I thought listing the SoCs directly would have been acceptable
but I am really not against listing them explicitly :)

> 
>   For backwards compatibility reasons, the compatibles 
>   marvell,armada370-thermal, marvell,armada380-thermal, and 
>   marvell,armadaxp-thermal must point to "control MSB/control 1",
> with size of 4. All other compatibles must point to "control
> LSB/control 0" with size of 8.
> 
> But I think you are right that we can use the size of the control
> registers to tell whether e.g. marvell,armada380-thermal is of the
> old binding of the new one. So maybe the "allow" language is more
> correct. But let's make it explicit to avoid any doubt. How about:
> 
>   The compatibles marvell,armada370-thermal,
> marvell,armada380-thermal, and marvell,armadaxp-thermal must point to
> "control MSB/control 1", with size of 4 (deprecated binding), or
> point to "control LSB/control 0" with size of 8 (current binding).
> All other compatibles must point to "control LSB/control 0" with size
> of 8.

This version looks good to me!

Thank you,
Miquèl

> 
> baruch
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
index 24aacf8948c5..9b7b2c03cc6f 100644
--- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
@@ -7,17 +7,31 @@  Required properties:
 		marvell,armada375-thermal
 		marvell,armada380-thermal
 		marvell,armadaxp-thermal
+		marvell,armada-ap806-thermal
+		marvell,armada-cp110-thermal
 
 - reg:		Device's register space.
 		Two entries are expected, see the examples below.
-		The first one is required for the sensor register;
-		the second one is required for the control register
-		to be used for sensor initialization (a.k.a. calibration).
+		The first one points to the status register (4B).
+		The second one points to the control registers (8B).
+		Note: with legacy bindings, the second entry pointed
+		only to the so called "control MSB" ("control 1"), was
+		4B wide and did not let the possibility to reach the
+		"control LSB" ("control 0") register. This is only
+		allowed for compatibility reasons in Armada
+		370/375/38x/XP DT nodes.
 
-Example:
+Examples:
 
+	/* Legacy bindings */
 	thermal@d0018300 {
 		compatible = "marvell,armada370-thermal";
-                reg = <0xd0018300 0x4
+		reg = <0xd0018300 0x4
 		       0xd0018304 0x4>;
 	};
+
+	ap_thermal: thermal@6f8084 {
+		compatible = "marvell,armada-ap806-thermal";
+		reg = <0x6f808C 0x4>,
+		      <0x6f8084 0x8>;
+	};