[PATCH v4 00/12] Armada thermal: improvements and A7K/A8K SoCs support

[PATCH v4 00/12] Armada thermal: improvements and A7K/A8K SoCs support

[Miquel Raynal]

Hi,

This series takes over Baruch's series by integrating his patches about
supporting thermal on Armada 7K and 8K SoCs within a larger series with
several improvements on the armada_thermal.c driver.

For now, Armada 380 and CP110 compatibles share the same initialization
routine but this will probably change in the near future when adding
support for overheat interrupts.

DT bindings documentation is updated to match existing code.

Armada AP806 and CP110 DT are also updated with thermal nodes.

Thank you,
Miquèl


Changes since v3:
  - Added Gregory's Reviewed-by tags
  - Detailed what I have changed over Baruch's series in the commit logs
  - Removed the list of every supported SoC, used "Marvell EBU Armada
  SoCs" instead as suggested by Thomas (unless for the Kconfig
  description, where having the list is useful).
  - Changed the comment about the Armada 380 reset section in the
  armada380_init() callback.
  - Removed the freshly introduced marvell,thermal-zone-name property in
  favor of the use of dev_name(dev) to name the thermal zone.
  - Introduced the needs_control0 capability and removed checks in the
  init routines (probe will fail if the bindings used are not
  appropriate).
  - Changed coefficients type to s64 to handle signed values, as well as
  some local variables around in the get_temp() callback
  - Used a do_div() instead of the traditionnal "/" to handle 64-bit
  values.
  - Split the patch renaiming the registers to do the "status" renaiming
  aside.


Baruch Siach (4):
  dt-bindings: thermal: Describe Armada AP806 and CP110
  thermal: armada: Use msleep for long delays
  thermal: armada: Add support for Armada AP806
  thermal: armada: Add support for Armada CP110

Miquel Raynal (8):
  thermal: armada: Simplify the check of the validity bit
  thermal: armada: Clarify control registers accesses
  thermal: armada: Use real status register name
  thermal: armada: Update Kconfig and module description
  thermal: armada: Change sensors trim default value
  thermal: armada: Wait sensors validity before exiting the init
    callback
  thermal: armada: Give meaningful names to the thermal zones
  ARM64: dts: marvell: Add thermal support for A7K/A8K

 .../devicetree/bindings/thermal/armada-thermal.txt |  24 +-
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |   6 +
 .../boot/dts/marvell/armada-cp110-master.dtsi      |   6 +
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi |   6 +
 drivers/thermal/Kconfig                            |   4 +-
 drivers/thermal/armada_thermal.c                   | 252 +++++++++++++++------
 6 files changed, 226 insertions(+), 72 deletions(-)

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 00/12] Armada thermal: improvements and A7K/A8K SoCs support

[Miquel Raynal]

Hi,

This series takes over Baruch's series by integrating his patches about
supporting thermal on Armada 7K and 8K SoCs within a larger series with
several improvements on the armada_thermal.c driver.

For now, Armada 380 and CP110 compatibles share the same initialization
routine but this will probably change in the near future when adding
support for overheat interrupts.

DT bindings documentation is updated to match existing code.

Armada AP806 and CP110 DT are also updated with thermal nodes.

Thank you,
Miqu?l


Changes since v3:
  - Added Gregory's Reviewed-by tags
  - Detailed what I have changed over Baruch's series in the commit logs
  - Removed the list of every supported SoC, used "Marvell EBU Armada
  SoCs" instead as suggested by Thomas (unless for the Kconfig
  description, where having the list is useful).
  - Changed the comment about the Armada 380 reset section in the
  armada380_init() callback.
  - Removed the freshly introduced marvell,thermal-zone-name property in
  favor of the use of dev_name(dev) to name the thermal zone.
  - Introduced the needs_control0 capability and removed checks in the
  init routines (probe will fail if the bindings used are not
  appropriate).
  - Changed coefficients type to s64 to handle signed values, as well as
  some local variables around in the get_temp() callback
  - Used a do_div() instead of the traditionnal "/" to handle 64-bit
  values.
  - Split the patch renaiming the registers to do the "status" renaiming
  aside.


Baruch Siach (4):
  dt-bindings: thermal: Describe Armada AP806 and CP110
  thermal: armada: Use msleep for long delays
  thermal: armada: Add support for Armada AP806
  thermal: armada: Add support for Armada CP110

Miquel Raynal (8):
  thermal: armada: Simplify the check of the validity bit
  thermal: armada: Clarify control registers accesses
  thermal: armada: Use real status register name
  thermal: armada: Update Kconfig and module description
  thermal: armada: Change sensors trim default value
  thermal: armada: Wait sensors validity before exiting the init
    callback
  thermal: armada: Give meaningful names to the thermal zones
  ARM64: dts: marvell: Add thermal support for A7K/A8K

 .../devicetree/bindings/thermal/armada-thermal.txt |  24 +-
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |   6 +
 .../boot/dts/marvell/armada-cp110-master.dtsi      |   6 +
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi |   6 +
 drivers/thermal/Kconfig                            |   4 +-
 drivers/thermal/armada_thermal.c                   | 252 +++++++++++++++------
 6 files changed, 226 insertions(+), 72 deletions(-)

-- 
2.11.0

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

[Miquel Raynal]

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.
 
-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>;
+	};
-- 
2.11.0

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

[Miquel Raynal]

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.
 
-Example:
+Examples:
 
+	/* Legacy bindings */
 	thermal at d0018300 {
 		compatible = "marvell,armada370-thermal";
-                reg = <0xd0018300 0x4
+		reg = <0xd0018300 0x4
 		       0xd0018304 0x4>;
 	};
+
+	ap_thermal: thermal at 6f8084 {
+		compatible = "marvell,armada-ap806-thermal";
+		reg = <0x6f808C 0x4>,
+		      <0x6f8084 0x8>;
+	};
-- 
2.11.0

[PATCH v4 02/12] thermal: armada: Use msleep for long delays

[Miquel Raynal]

From: Baruch Siach <baruch@tkos.co.il>

Use msleep for long (> 10ms) delays, instead of the busy waiting mdelay.
All delays are called from the probe routine, where scheduling is
allowed.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 706d74798cbe..6c4af2622d4f 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -113,7 +113,7 @@ static void armada370_init_sensor(struct platform_device *pdev,
 	reg &= ~PMU_TDC0_START_CAL_MASK;
 	writel(reg, priv->control);
 
-	mdelay(10);
+	msleep(10);
 }
 
 static void armada375_init_sensor(struct platform_device *pdev,
@@ -127,11 +127,11 @@ static void armada375_init_sensor(struct platform_device *pdev,
 	reg &= ~A375_HW_RESETn;
 
 	writel(reg, priv->control + 4);
-	mdelay(20);
+	msleep(20);
 
 	reg |= A375_HW_RESETn;
 	writel(reg, priv->control + 4);
-	mdelay(50);
+	msleep(50);
 }
 
 static void armada380_init_sensor(struct platform_device *pdev,
@@ -143,7 +143,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
 		writel(reg, priv->control);
-		mdelay(10);
+		msleep(10);
 	}
 }
 
-- 
2.11.0

[PATCH v4 02/12] thermal: armada: Use msleep for long delays

[Miquel Raynal]

From: Baruch Siach <baruch@tkos.co.il>

Use msleep for long (> 10ms) delays, instead of the busy waiting mdelay.
All delays are called from the probe routine, where scheduling is
allowed.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 706d74798cbe..6c4af2622d4f 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -113,7 +113,7 @@ static void armada370_init_sensor(struct platform_device *pdev,
 	reg &= ~PMU_TDC0_START_CAL_MASK;
 	writel(reg, priv->control);
 
-	mdelay(10);
+	msleep(10);
 }
 
 static void armada375_init_sensor(struct platform_device *pdev,
@@ -127,11 +127,11 @@ static void armada375_init_sensor(struct platform_device *pdev,
 	reg &= ~A375_HW_RESETn;
 
 	writel(reg, priv->control + 4);
-	mdelay(20);
+	msleep(20);
 
 	reg |= A375_HW_RESETn;
 	writel(reg, priv->control + 4);
-	mdelay(50);
+	msleep(50);
 }
 
 static void armada380_init_sensor(struct platform_device *pdev,
@@ -143,7 +143,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
 		writel(reg, priv->control);
-		mdelay(10);
+		msleep(10);
 	}
 }
 
-- 
2.11.0

[PATCH v4 03/12] thermal: armada: Simplify the check of the validity bit

[Miquel Raynal]

All Armada SoCs use one bit to declare if the sensor values are valid.
This bit moves across the versions of the IP.

The method until then was to do both a shift and compare with an useless
flag of "0x1". It is clearer and quicker to directly save the value that
must be ANDed instead of the bit position and do a single bitwise AND
operation.

Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/armada_thermal.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 6c4af2622d4f..f350d7efd35a 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -24,8 +24,6 @@
 #include <linux/of_device.h>
 #include <linux/thermal.h>
 
-#define THERMAL_VALID_MASK		0x1
-
 /* Thermal Manager Control and Status Register */
 #define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
 #define PMU_TM_DISABLE_OFFS		0
@@ -67,7 +65,7 @@ struct armada_thermal_data {
 	/* Register shift and mask to access the sensor temperature */
 	unsigned int temp_shift;
 	unsigned int temp_mask;
-	unsigned int is_valid_shift;
+	u32 is_valid_bit;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,
@@ -149,9 +147,9 @@ static void armada380_init_sensor(struct platform_device *pdev,
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->sensor);
 
-	return (reg >> priv->data->is_valid_shift) & THERMAL_VALID_MASK;
+	return reg & priv->data->is_valid_bit;
 }
 
 static int armada_get_temp(struct thermal_zone_device *thermal,
@@ -199,7 +197,7 @@ static const struct armada_thermal_data armadaxp_data = {
 static const struct armada_thermal_data armada370_data = {
 	.is_valid = armada_is_valid,
 	.init_sensor = armada370_init_sensor,
-	.is_valid_shift = 9,
+	.is_valid_bit = BIT(9),
 	.temp_shift = 10,
 	.temp_mask = 0x1ff,
 	.coef_b = 3153000000UL,
@@ -210,7 +208,7 @@ static const struct armada_thermal_data armada370_data = {
 static const struct armada_thermal_data armada375_data = {
 	.is_valid = armada_is_valid,
 	.init_sensor = armada375_init_sensor,
-	.is_valid_shift = 10,
+	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x1ff,
 	.coef_b = 3171900000UL,
@@ -221,7 +219,7 @@ static const struct armada_thermal_data armada375_data = {
 static const struct armada_thermal_data armada380_data = {
 	.is_valid = armada_is_valid,
 	.init_sensor = armada380_init_sensor,
-	.is_valid_shift = 10,
+	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x3ff,
 	.coef_b = 1172499100UL,
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 03/12] thermal: armada: Simplify the check of the validity bit

[Miquel Raynal]

All Armada SoCs use one bit to declare if the sensor values are valid.
This bit moves across the versions of the IP.

The method until then was to do both a shift and compare with an useless
flag of "0x1". It is clearer and quicker to directly save the value that
must be ANDed instead of the bit position and do a single bitwise AND
operation.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 6c4af2622d4f..f350d7efd35a 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -24,8 +24,6 @@
 #include <linux/of_device.h>
 #include <linux/thermal.h>
 
-#define THERMAL_VALID_MASK		0x1
-
 /* Thermal Manager Control and Status Register */
 #define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
 #define PMU_TM_DISABLE_OFFS		0
@@ -67,7 +65,7 @@ struct armada_thermal_data {
 	/* Register shift and mask to access the sensor temperature */
 	unsigned int temp_shift;
 	unsigned int temp_mask;
-	unsigned int is_valid_shift;
+	u32 is_valid_bit;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,
@@ -149,9 +147,9 @@ static void armada380_init_sensor(struct platform_device *pdev,
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->sensor);
 
-	return (reg >> priv->data->is_valid_shift) & THERMAL_VALID_MASK;
+	return reg & priv->data->is_valid_bit;
 }
 
 static int armada_get_temp(struct thermal_zone_device *thermal,
@@ -199,7 +197,7 @@ static const struct armada_thermal_data armadaxp_data = {
 static const struct armada_thermal_data armada370_data = {
 	.is_valid = armada_is_valid,
 	.init_sensor = armada370_init_sensor,
-	.is_valid_shift = 9,
+	.is_valid_bit = BIT(9),
 	.temp_shift = 10,
 	.temp_mask = 0x1ff,
 	.coef_b = 3153000000UL,
@@ -210,7 +208,7 @@ static const struct armada_thermal_data armada370_data = {
 static const struct armada_thermal_data armada375_data = {
 	.is_valid = armada_is_valid,
 	.init_sensor = armada375_init_sensor,
-	.is_valid_shift = 10,
+	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x1ff,
 	.coef_b = 3171900000UL,
@@ -221,7 +219,7 @@ static const struct armada_thermal_data armada375_data = {
 static const struct armada_thermal_data armada380_data = {
 	.is_valid = armada_is_valid,
 	.init_sensor = armada380_init_sensor,
-	.is_valid_shift = 10,
+	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x3ff,
 	.coef_b = 1172499100UL,
-- 
2.11.0

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel Raynal]

Bindings were incomplete for a long time by only exposing one of the two
available control registers. To ease the migration to the full bindings
(already in use for the Armada 375 SoC), rename the pointers for
clarification. This way, it will only be needed to add another pointer
to access the other control register when the time comes.

This avoids dangerous situations where the offset 0 of the control
area can be either one register or the other depending on the bindings
used. After this change, device trees of other SoCs could be migrated to
the "full" bindings if they may benefit from features from the
unaccessible register, without any change in the driver.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 69 +++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index f350d7efd35a..f422563e617c 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -39,12 +39,21 @@
 #define A375_HW_RESETn			BIT(8)
 #define A380_HW_RESET			BIT(8)
 
+/* Legacy bindings */
+#define LEGACY_CONTROL_MEM_LEN		0x4
+
+/* Current bindings with the 2 control registers under the same memory area */
+#define LEGACY_CONTROL1_OFFSET		0x0
+#define CONTROL0_OFFSET			0x0
+#define CONTROL1_OFFSET			0x4
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
 struct armada_thermal_priv {
 	void __iomem *sensor;
-	void __iomem *control;
+	void __iomem *control0;
+	void __iomem *control1;
 	struct armada_thermal_data *data;
 };
 
@@ -71,22 +80,22 @@ struct armada_thermal_data {
 static void armadaxp_init_sensor(struct platform_device *pdev,
 				 struct armada_thermal_priv *priv)
 {
-	unsigned long reg;
+	u32 reg;
 
-	reg = readl_relaxed(priv->control);
+	reg = readl_relaxed(priv->control1);
 	reg |= PMU_TDC0_OTF_CAL_MASK;
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Reference calibration value */
 	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
 	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Reset the sensor */
-	reg = readl_relaxed(priv->control);
-	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
+	reg = readl_relaxed(priv->control1);
+	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
 
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Enable the sensor */
 	reg = readl_relaxed(priv->sensor);
@@ -97,19 +106,19 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
 static void armada370_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
-	unsigned long reg;
+	u32 reg;
 
-	reg = readl_relaxed(priv->control);
+	reg = readl_relaxed(priv->control1);
 	reg |= PMU_TDC0_OTF_CAL_MASK;
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Reference calibration value */
 	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
 	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	reg &= ~PMU_TDC0_START_CAL_MASK;
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	msleep(10);
 }
@@ -117,30 +126,30 @@ static void armada370_init_sensor(struct platform_device *pdev,
 static void armada375_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
-	unsigned long reg;
+	u32 reg;
 
-	reg = readl(priv->control + 4);
+	reg = readl(priv->control1);
 	reg &= ~(A375_UNIT_CONTROL_MASK << A375_UNIT_CONTROL_SHIFT);
 	reg &= ~A375_READOUT_INVERT;
 	reg &= ~A375_HW_RESETn;
 
-	writel(reg, priv->control + 4);
+	writel(reg, priv->control1);
 	msleep(20);
 
 	reg |= A375_HW_RESETn;
-	writel(reg, priv->control + 4);
+	writel(reg, priv->control1);
 	msleep(50);
 }
 
 static void armada380_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->control);
+	u32 reg = readl_relaxed(priv->control1);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, priv->control1);
 		msleep(10);
 	}
 }
@@ -253,6 +262,7 @@ MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
 
 static int armada_thermal_probe(struct platform_device *pdev)
 {
+	void __iomem *control = NULL;
 	struct thermal_zone_device *thermal;
 	const struct of_device_id *match;
 	struct armada_thermal_priv *priv;
@@ -272,11 +282,26 @@ static int armada_thermal_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->sensor);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	priv->control = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(priv->control))
-		return PTR_ERR(priv->control);
+	control = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(control))
+		return PTR_ERR(control);
 
 	priv->data = (struct armada_thermal_data *)match->data;
+
+	/*
+	 * Legacy DT bindings only described "control1" register (also referred
+	 * as "control MSB" on old documentation). New bindings cover
+	 * "control0/control LSB" and "control1/control MSB" registers within
+	 * the same resource, which is then of size 8 instead of 4.
+	 */
+	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
+		/* ->control0 unavailable in this configuration */
+		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
+	} else {
+		priv->control0 = control + CONTROL0_OFFSET;
+		priv->control1 = control + CONTROL1_OFFSET;
+	}
+
 	priv->data->init_sensor(pdev, priv);
 
 	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
-- 
2.11.0

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel Raynal]

Bindings were incomplete for a long time by only exposing one of the two
available control registers. To ease the migration to the full bindings
(already in use for the Armada 375 SoC), rename the pointers for
clarification. This way, it will only be needed to add another pointer
to access the other control register when the time comes.

This avoids dangerous situations where the offset 0 of the control
area can be either one register or the other depending on the bindings
used. After this change, device trees of other SoCs could be migrated to
the "full" bindings if they may benefit from features from the
unaccessible register, without any change in the driver.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 69 +++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index f350d7efd35a..f422563e617c 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -39,12 +39,21 @@
 #define A375_HW_RESETn			BIT(8)
 #define A380_HW_RESET			BIT(8)
 
+/* Legacy bindings */
+#define LEGACY_CONTROL_MEM_LEN		0x4
+
+/* Current bindings with the 2 control registers under the same memory area */
+#define LEGACY_CONTROL1_OFFSET		0x0
+#define CONTROL0_OFFSET			0x0
+#define CONTROL1_OFFSET			0x4
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
 struct armada_thermal_priv {
 	void __iomem *sensor;
-	void __iomem *control;
+	void __iomem *control0;
+	void __iomem *control1;
 	struct armada_thermal_data *data;
 };
 
@@ -71,22 +80,22 @@ struct armada_thermal_data {
 static void armadaxp_init_sensor(struct platform_device *pdev,
 				 struct armada_thermal_priv *priv)
 {
-	unsigned long reg;
+	u32 reg;
 
-	reg = readl_relaxed(priv->control);
+	reg = readl_relaxed(priv->control1);
 	reg |= PMU_TDC0_OTF_CAL_MASK;
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Reference calibration value */
 	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
 	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Reset the sensor */
-	reg = readl_relaxed(priv->control);
-	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
+	reg = readl_relaxed(priv->control1);
+	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
 
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Enable the sensor */
 	reg = readl_relaxed(priv->sensor);
@@ -97,19 +106,19 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
 static void armada370_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
-	unsigned long reg;
+	u32 reg;
 
-	reg = readl_relaxed(priv->control);
+	reg = readl_relaxed(priv->control1);
 	reg |= PMU_TDC0_OTF_CAL_MASK;
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	/* Reference calibration value */
 	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
 	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	reg &= ~PMU_TDC0_START_CAL_MASK;
-	writel(reg, priv->control);
+	writel(reg, priv->control1);
 
 	msleep(10);
 }
@@ -117,30 +126,30 @@ static void armada370_init_sensor(struct platform_device *pdev,
 static void armada375_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
-	unsigned long reg;
+	u32 reg;
 
-	reg = readl(priv->control + 4);
+	reg = readl(priv->control1);
 	reg &= ~(A375_UNIT_CONTROL_MASK << A375_UNIT_CONTROL_SHIFT);
 	reg &= ~A375_READOUT_INVERT;
 	reg &= ~A375_HW_RESETn;
 
-	writel(reg, priv->control + 4);
+	writel(reg, priv->control1);
 	msleep(20);
 
 	reg |= A375_HW_RESETn;
-	writel(reg, priv->control + 4);
+	writel(reg, priv->control1);
 	msleep(50);
 }
 
 static void armada380_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->control);
+	u32 reg = readl_relaxed(priv->control1);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, priv->control1);
 		msleep(10);
 	}
 }
@@ -253,6 +262,7 @@ MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
 
 static int armada_thermal_probe(struct platform_device *pdev)
 {
+	void __iomem *control = NULL;
 	struct thermal_zone_device *thermal;
 	const struct of_device_id *match;
 	struct armada_thermal_priv *priv;
@@ -272,11 +282,26 @@ static int armada_thermal_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->sensor);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	priv->control = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(priv->control))
-		return PTR_ERR(priv->control);
+	control = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(control))
+		return PTR_ERR(control);
 
 	priv->data = (struct armada_thermal_data *)match->data;
+
+	/*
+	 * Legacy DT bindings only described "control1" register (also referred
+	 * as "control MSB" on old documentation). New bindings cover
+	 * "control0/control LSB" and "control1/control MSB" registers within
+	 * the same resource, which is then of size 8 instead of 4.
+	 */
+	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
+		/* ->control0 unavailable in this configuration */
+		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
+	} else {
+		priv->control0 = control + CONTROL0_OFFSET;
+		priv->control1 = control + CONTROL1_OFFSET;
+	}
+
 	priv->data->init_sensor(pdev, priv);
 
 	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
-- 
2.11.0

[PATCH v4 05/12] thermal: armada: Use real status register name

[Miquel Raynal]

Three 32-bit registers are used to drive the thermal IP: control0,
control1 and status. The two control registers share the same name both
in the documentation and in the code, while the latter is referred as
"sensor" in the code. Rename this pointer to be called "status" in order
to be aligned with the documentation.

Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/armada_thermal.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index f422563e617c..198485fa77f2 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -51,7 +51,7 @@ struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
 struct armada_thermal_priv {
-	void __iomem *sensor;
+	void __iomem *status;
 	void __iomem *control0;
 	void __iomem *control1;
 	struct armada_thermal_data *data;
@@ -98,9 +98,9 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
 	writel(reg, priv->control1);
 
 	/* Enable the sensor */
-	reg = readl_relaxed(priv->sensor);
+	reg = readl_relaxed(priv->status);
 	reg &= ~PMU_TM_DISABLE_MASK;
-	writel(reg, priv->sensor);
+	writel(reg, priv->status);
 }
 
 static void armada370_init_sensor(struct platform_device *pdev,
@@ -156,7 +156,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	u32 reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->status);
 
 	return reg & priv->data->is_valid_bit;
 }
@@ -175,7 +175,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 		return -EIO;
 	}
 
-	reg = readl_relaxed(priv->sensor);
+	reg = readl_relaxed(priv->status);
 	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
 
 	/* Get formula coeficients */
@@ -277,9 +277,9 @@ static int armada_thermal_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(priv->sensor))
-		return PTR_ERR(priv->sensor);
+	priv->status = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->status))
+		return PTR_ERR(priv->status);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	control = devm_ioremap_resource(&pdev->dev, res);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 05/12] thermal: armada: Use real status register name

[Miquel Raynal]

Three 32-bit registers are used to drive the thermal IP: control0,
control1 and status. The two control registers share the same name both
in the documentation and in the code, while the latter is referred as
"sensor" in the code. Rename this pointer to be called "status" in order
to be aligned with the documentation.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index f422563e617c..198485fa77f2 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -51,7 +51,7 @@ struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
 struct armada_thermal_priv {
-	void __iomem *sensor;
+	void __iomem *status;
 	void __iomem *control0;
 	void __iomem *control1;
 	struct armada_thermal_data *data;
@@ -98,9 +98,9 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
 	writel(reg, priv->control1);
 
 	/* Enable the sensor */
-	reg = readl_relaxed(priv->sensor);
+	reg = readl_relaxed(priv->status);
 	reg &= ~PMU_TM_DISABLE_MASK;
-	writel(reg, priv->sensor);
+	writel(reg, priv->status);
 }
 
 static void armada370_init_sensor(struct platform_device *pdev,
@@ -156,7 +156,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	u32 reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->status);
 
 	return reg & priv->data->is_valid_bit;
 }
@@ -175,7 +175,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 		return -EIO;
 	}
 
-	reg = readl_relaxed(priv->sensor);
+	reg = readl_relaxed(priv->status);
 	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
 
 	/* Get formula coeficients */
@@ -277,9 +277,9 @@ static int armada_thermal_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(priv->sensor))
-		return PTR_ERR(priv->sensor);
+	priv->status = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->status))
+		return PTR_ERR(priv->status);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	control = devm_ioremap_resource(&pdev->dev, res);
-- 
2.11.0

[PATCH v4 06/12] thermal: armada: Add support for Armada AP806

[Miquel Raynal]

From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

The AP806 component is integrated in the Armada 8K and 7K lines of
processors.

The thermal sensor sample field on the status register is a signed
value. Extend armada_get_temp() and the driver structure to handle
signed values.

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
[<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: Changes when applying over the
previous patches, including the register names changes, also switched
the coefficients values to s64 instead of unsigned long to deal with
negative values and used do_div instead of the traditionnal '/']
Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/armada_thermal.c | 80 ++++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 198485fa77f2..ec29ea76b818 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -47,6 +47,11 @@
 #define CONTROL0_OFFSET			0x0
 #define CONTROL1_OFFSET			0x4
 
+/* TSEN refers to the temperature sensors within the AP */
+#define CONTROL0_TSEN_START		BIT(0)
+#define CONTROL0_TSEN_RESET		BIT(1)
+#define CONTROL0_TSEN_ENABLE		BIT(2)
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -66,15 +71,17 @@ struct armada_thermal_data {
 	bool (*is_valid)(struct armada_thermal_priv *);
 
 	/* Formula coeficients: temp = (b - m * reg) / div */
-	unsigned long coef_b;
-	unsigned long coef_m;
-	unsigned long coef_div;
+	s64 coef_b;
+	s64 coef_m;
+	u32 coef_div;
 	bool inverted;
+	bool signed_sample;
 
 	/* Register shift and mask to access the sensor temperature */
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	u32 is_valid_bit;
+	bool needs_control0;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,
@@ -154,6 +161,18 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	}
 }
 
+static void armada_ap806_init_sensor(struct platform_device *pdev,
+				     struct armada_thermal_priv *priv)
+{
+	u32 reg;
+
+	reg = readl_relaxed(priv->control0);
+	reg &= ~CONTROL0_TSEN_RESET;
+	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
+	writel(reg, priv->control0);
+	msleep(10);
+}
+
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
 	u32 reg = readl_relaxed(priv->status);
@@ -165,8 +184,8 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 			  int *temp)
 {
 	struct armada_thermal_priv *priv = thermal->devdata;
-	unsigned long reg;
-	unsigned long m, b, div;
+	u32 reg, div;
+	s64 sample, b, m;
 
 	/* Valid check */
 	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
@@ -177,6 +196,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 
 	reg = readl_relaxed(priv->status);
 	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
+	if (priv->data->signed_sample)
+		/* The most significant bit is the sign bit */
+		sample = sign_extend32(reg, fls(priv->data->temp_mask) - 1);
+	else
+		sample = reg;
 
 	/* Get formula coeficients */
 	b = priv->data->coef_b;
@@ -184,9 +208,12 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 	div = priv->data->coef_div;
 
 	if (priv->data->inverted)
-		*temp = ((m * reg) - b) / div;
+		*temp = (m * sample) - b;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = b - (m * sample);
+
+	do_div(*temp, div);
+
 	return 0;
 }
 
@@ -198,8 +225,8 @@ static const struct armada_thermal_data armadaxp_data = {
 	.init_sensor = armadaxp_init_sensor,
 	.temp_shift = 10,
 	.temp_mask = 0x1ff,
-	.coef_b = 3153000000UL,
-	.coef_m = 10000000UL,
+	.coef_b = 3153000000ULL,
+	.coef_m = 10000000ULL,
 	.coef_div = 13825,
 };
 
@@ -209,8 +236,8 @@ static const struct armada_thermal_data armada370_data = {
 	.is_valid_bit = BIT(9),
 	.temp_shift = 10,
 	.temp_mask = 0x1ff,
-	.coef_b = 3153000000UL,
-	.coef_m = 10000000UL,
+	.coef_b = 3153000000ULL,
+	.coef_m = 10000000ULL,
 	.coef_div = 13825,
 };
 
@@ -220,8 +247,8 @@ static const struct armada_thermal_data armada375_data = {
 	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x1ff,
-	.coef_b = 3171900000UL,
-	.coef_m = 10000000UL,
+	.coef_b = 3171900000ULL,
+	.coef_m = 10000000ULL,
 	.coef_div = 13616,
 };
 
@@ -231,12 +258,26 @@ static const struct armada_thermal_data armada380_data = {
 	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x3ff,
-	.coef_b = 1172499100UL,
-	.coef_m = 2000096UL,
+	.coef_b = 1172499100ULL,
+	.coef_m = 2000096ULL,
 	.coef_div = 4201,
 	.inverted = true,
 };
 
+static const struct armada_thermal_data armada_ap806_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada_ap806_init_sensor,
+	.is_valid_bit = BIT(16),
+	.temp_shift = 0,
+	.temp_mask = 0x3ff,
+	.coef_b = -150000LL,
+	.coef_m = 423ULL,
+	.coef_div = 1,
+	.inverted = true,
+	.signed_sample = true,
+	.needs_control0 = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",
@@ -255,6 +296,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.data       = &armada380_data,
 	},
 	{
+		.compatible = "marvell,armada-ap806-thermal",
+		.data       = &armada_ap806_data,
+	},
+	{
 		/* sentinel */
 	},
 };
@@ -296,6 +341,11 @@ static int armada_thermal_probe(struct platform_device *pdev)
 	 */
 	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
 		/* ->control0 unavailable in this configuration */
+		if (priv->data->needs_control0) {
+			dev_err(&pdev->dev, "No access to control0 register\n");
+			return -EINVAL;
+		}
+
 		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
 	} else {
 		priv->control0 = control + CONTROL0_OFFSET;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 06/12] thermal: armada: Add support for Armada AP806

[Miquel Raynal]

From: Baruch Siach <baruch@tkos.co.il>

The AP806 component is integrated in the Armada 8K and 7K lines of
processors.

The thermal sensor sample field on the status register is a signed
value. Extend armada_get_temp() and the driver structure to handle
signed values.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
[<miquel.raynal@free-electrons.com>: Changes when applying over the
previous patches, including the register names changes, also switched
the coefficients values to s64 instead of unsigned long to deal with
negative values and used do_div instead of the traditionnal '/']
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 80 ++++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 198485fa77f2..ec29ea76b818 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -47,6 +47,11 @@
 #define CONTROL0_OFFSET			0x0
 #define CONTROL1_OFFSET			0x4
 
+/* TSEN refers to the temperature sensors within the AP */
+#define CONTROL0_TSEN_START		BIT(0)
+#define CONTROL0_TSEN_RESET		BIT(1)
+#define CONTROL0_TSEN_ENABLE		BIT(2)
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -66,15 +71,17 @@ struct armada_thermal_data {
 	bool (*is_valid)(struct armada_thermal_priv *);
 
 	/* Formula coeficients: temp = (b - m * reg) / div */
-	unsigned long coef_b;
-	unsigned long coef_m;
-	unsigned long coef_div;
+	s64 coef_b;
+	s64 coef_m;
+	u32 coef_div;
 	bool inverted;
+	bool signed_sample;
 
 	/* Register shift and mask to access the sensor temperature */
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	u32 is_valid_bit;
+	bool needs_control0;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,
@@ -154,6 +161,18 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	}
 }
 
+static void armada_ap806_init_sensor(struct platform_device *pdev,
+				     struct armada_thermal_priv *priv)
+{
+	u32 reg;
+
+	reg = readl_relaxed(priv->control0);
+	reg &= ~CONTROL0_TSEN_RESET;
+	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
+	writel(reg, priv->control0);
+	msleep(10);
+}
+
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
 	u32 reg = readl_relaxed(priv->status);
@@ -165,8 +184,8 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 			  int *temp)
 {
 	struct armada_thermal_priv *priv = thermal->devdata;
-	unsigned long reg;
-	unsigned long m, b, div;
+	u32 reg, div;
+	s64 sample, b, m;
 
 	/* Valid check */
 	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
@@ -177,6 +196,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 
 	reg = readl_relaxed(priv->status);
 	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
+	if (priv->data->signed_sample)
+		/* The most significant bit is the sign bit */
+		sample = sign_extend32(reg, fls(priv->data->temp_mask) - 1);
+	else
+		sample = reg;
 
 	/* Get formula coeficients */
 	b = priv->data->coef_b;
@@ -184,9 +208,12 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 	div = priv->data->coef_div;
 
 	if (priv->data->inverted)
-		*temp = ((m * reg) - b) / div;
+		*temp = (m * sample) - b;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = b - (m * sample);
+
+	do_div(*temp, div);
+
 	return 0;
 }
 
@@ -198,8 +225,8 @@ static const struct armada_thermal_data armadaxp_data = {
 	.init_sensor = armadaxp_init_sensor,
 	.temp_shift = 10,
 	.temp_mask = 0x1ff,
-	.coef_b = 3153000000UL,
-	.coef_m = 10000000UL,
+	.coef_b = 3153000000ULL,
+	.coef_m = 10000000ULL,
 	.coef_div = 13825,
 };
 
@@ -209,8 +236,8 @@ static const struct armada_thermal_data armada370_data = {
 	.is_valid_bit = BIT(9),
 	.temp_shift = 10,
 	.temp_mask = 0x1ff,
-	.coef_b = 3153000000UL,
-	.coef_m = 10000000UL,
+	.coef_b = 3153000000ULL,
+	.coef_m = 10000000ULL,
 	.coef_div = 13825,
 };
 
@@ -220,8 +247,8 @@ static const struct armada_thermal_data armada375_data = {
 	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x1ff,
-	.coef_b = 3171900000UL,
-	.coef_m = 10000000UL,
+	.coef_b = 3171900000ULL,
+	.coef_m = 10000000ULL,
 	.coef_div = 13616,
 };
 
@@ -231,12 +258,26 @@ static const struct armada_thermal_data armada380_data = {
 	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x3ff,
-	.coef_b = 1172499100UL,
-	.coef_m = 2000096UL,
+	.coef_b = 1172499100ULL,
+	.coef_m = 2000096ULL,
 	.coef_div = 4201,
 	.inverted = true,
 };
 
+static const struct armada_thermal_data armada_ap806_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada_ap806_init_sensor,
+	.is_valid_bit = BIT(16),
+	.temp_shift = 0,
+	.temp_mask = 0x3ff,
+	.coef_b = -150000LL,
+	.coef_m = 423ULL,
+	.coef_div = 1,
+	.inverted = true,
+	.signed_sample = true,
+	.needs_control0 = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",
@@ -255,6 +296,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.data       = &armada380_data,
 	},
 	{
+		.compatible = "marvell,armada-ap806-thermal",
+		.data       = &armada_ap806_data,
+	},
+	{
 		/* sentinel */
 	},
 };
@@ -296,6 +341,11 @@ static int armada_thermal_probe(struct platform_device *pdev)
 	 */
 	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
 		/* ->control0 unavailable in this configuration */
+		if (priv->data->needs_control0) {
+			dev_err(&pdev->dev, "No access to control0 register\n");
+			return -EINVAL;
+		}
+
 		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
 	} else {
 		priv->control0 = control + CONTROL0_OFFSET;
-- 
2.11.0

[PATCH v4 07/12] thermal: armada: Add support for Armada CP110

[Miquel Raynal]

From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

The CP110 component is integrated in the Armada 8k and 7k lines of
processors.

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
[<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: renamed the register pointers as
well as some definitions related to the new register names and
simplified the init sequence for Armada 380]
Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/armada_thermal.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index ec29ea76b818..11a94ad66c35 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -37,7 +37,6 @@
 #define A375_UNIT_CONTROL_MASK		0x7
 #define A375_READOUT_INVERT		BIT(15)
 #define A375_HW_RESETn			BIT(8)
-#define A380_HW_RESET			BIT(8)
 
 /* Legacy bindings */
 #define LEGACY_CONTROL_MEM_LEN		0x4
@@ -52,6 +51,10 @@
 #define CONTROL0_TSEN_RESET		BIT(1)
 #define CONTROL0_TSEN_ENABLE		BIT(2)
 
+/* EXT_TSEN refers to the external temperature sensors, out of the AP */
+#define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
+#define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -153,12 +156,11 @@ static void armada380_init_sensor(struct platform_device *pdev,
 {
 	u32 reg = readl_relaxed(priv->control1);
 
-	/* Reset hardware once */
-	if (!(reg & A380_HW_RESET)) {
-		reg |= A380_HW_RESET;
-		writel(reg, priv->control1);
-		msleep(10);
-	}
+	/* Disable the HW/SW reset */
+	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
+	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
+	writel(reg, priv->control1);
+	msleep(10);
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,
@@ -278,6 +280,19 @@ static const struct armada_thermal_data armada_ap806_data = {
 	.needs_control0 = true,
 };
 
+static const struct armada_thermal_data armada_cp110_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada380_init_sensor,
+	.is_valid_bit = BIT(10),
+	.temp_shift = 0,
+	.temp_mask = 0x3ff,
+	.coef_b = 1172499100ULL,
+	.coef_m = 2000096ULL,
+	.coef_div = 4201,
+	.inverted = true,
+	.needs_control0 = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",
@@ -300,6 +315,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.data       = &armada_ap806_data,
 	},
 	{
+		.compatible = "marvell,armada-cp110-thermal",
+		.data       = &armada_cp110_data,
+	},
+	{
 		/* sentinel */
 	},
 };
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 07/12] thermal: armada: Add support for Armada CP110

[Miquel Raynal]

From: Baruch Siach <baruch@tkos.co.il>

The CP110 component is integrated in the Armada 8k and 7k lines of
processors.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
[<miquel.raynal@free-electrons.com>: renamed the register pointers as
well as some definitions related to the new register names and
simplified the init sequence for Armada 380]
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index ec29ea76b818..11a94ad66c35 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -37,7 +37,6 @@
 #define A375_UNIT_CONTROL_MASK		0x7
 #define A375_READOUT_INVERT		BIT(15)
 #define A375_HW_RESETn			BIT(8)
-#define A380_HW_RESET			BIT(8)
 
 /* Legacy bindings */
 #define LEGACY_CONTROL_MEM_LEN		0x4
@@ -52,6 +51,10 @@
 #define CONTROL0_TSEN_RESET		BIT(1)
 #define CONTROL0_TSEN_ENABLE		BIT(2)
 
+/* EXT_TSEN refers to the external temperature sensors, out of the AP */
+#define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
+#define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -153,12 +156,11 @@ static void armada380_init_sensor(struct platform_device *pdev,
 {
 	u32 reg = readl_relaxed(priv->control1);
 
-	/* Reset hardware once */
-	if (!(reg & A380_HW_RESET)) {
-		reg |= A380_HW_RESET;
-		writel(reg, priv->control1);
-		msleep(10);
-	}
+	/* Disable the HW/SW reset */
+	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
+	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
+	writel(reg, priv->control1);
+	msleep(10);
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,
@@ -278,6 +280,19 @@ static const struct armada_thermal_data armada_ap806_data = {
 	.needs_control0 = true,
 };
 
+static const struct armada_thermal_data armada_cp110_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada380_init_sensor,
+	.is_valid_bit = BIT(10),
+	.temp_shift = 0,
+	.temp_mask = 0x3ff,
+	.coef_b = 1172499100ULL,
+	.coef_m = 2000096ULL,
+	.coef_div = 4201,
+	.inverted = true,
+	.needs_control0 = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",
@@ -300,6 +315,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.data       = &armada_ap806_data,
 	},
 	{
+		.compatible = "marvell,armada-cp110-thermal",
+		.data       = &armada_cp110_data,
+	},
+	{
 		/* sentinel */
 	},
 };
-- 
2.11.0

[PATCH v4 08/12] thermal: armada: Update Kconfig and module description

[Miquel Raynal]

Update Armada thermal driver Kconfig entry as well as the driver's
MODULE_DESCRIPTION content, now that 64-bit SoCs are also supported,
eg. Armada 7K and Armada 8K.

Use the generic term "Marvell EBU Armada SoCs" instead of listing all
the supported SoCs everywhere (excepted in the Kconfig description,
where it is useful to have a list).

Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/Kconfig          | 4 ++--
 drivers/thermal/armada_thermal.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 315ae2926e20..b6adc54b96f1 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -301,13 +301,13 @@ config DB8500_THERMAL
 	  thermal zone if trip points reached.
 
 config ARMADA_THERMAL
-	tristate "Armada 370/XP thermal management"
+	tristate "Marvell EBU Armada SoCs thermal management"
 	depends on ARCH_MVEBU || COMPILE_TEST
 	depends on HAS_IOMEM
 	depends on OF
 	help
 	  Enable this option if you want to have support for thermal management
-	  controller present in Armada 370 and Armada XP SoC.
+	  controller present in Marvell EBU Armada SoCs (370,375,XP,38x,7K,8K).
 
 config DA9062_THERMAL
 	tristate "DA9062/DA9061 Dialog Semiconductor thermal driver"
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 11a94ad66c35..cef5c65c8f32 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -1,5 +1,5 @@
 /*
- * Marvell Armada 370/XP thermal sensor driver
+ * Marvell EBU Armada SoCs thermal sensor driver
  *
  * Copyright (C) 2013 Marvell
  *
@@ -408,5 +408,5 @@ static struct platform_driver armada_thermal_driver = {
 module_platform_driver(armada_thermal_driver);
 
 MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
-MODULE_DESCRIPTION("Armada 370/XP thermal driver");
+MODULE_DESCRIPTION("Marvell EBU Armada SoCs thermal driver");
 MODULE_LICENSE("GPL v2");
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 08/12] thermal: armada: Update Kconfig and module description

[Miquel Raynal]

Update Armada thermal driver Kconfig entry as well as the driver's
MODULE_DESCRIPTION content, now that 64-bit SoCs are also supported,
eg. Armada 7K and Armada 8K.

Use the generic term "Marvell EBU Armada SoCs" instead of listing all
the supported SoCs everywhere (excepted in the Kconfig description,
where it is useful to have a list).

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/Kconfig          | 4 ++--
 drivers/thermal/armada_thermal.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 315ae2926e20..b6adc54b96f1 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -301,13 +301,13 @@ config DB8500_THERMAL
 	  thermal zone if trip points reached.
 
 config ARMADA_THERMAL
-	tristate "Armada 370/XP thermal management"
+	tristate "Marvell EBU Armada SoCs thermal management"
 	depends on ARCH_MVEBU || COMPILE_TEST
 	depends on HAS_IOMEM
 	depends on OF
 	help
 	  Enable this option if you want to have support for thermal management
-	  controller present in Armada 370 and Armada XP SoC.
+	  controller present in Marvell EBU Armada SoCs (370,375,XP,38x,7K,8K).
 
 config DA9062_THERMAL
 	tristate "DA9062/DA9061 Dialog Semiconductor thermal driver"
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 11a94ad66c35..cef5c65c8f32 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -1,5 +1,5 @@
 /*
- * Marvell Armada 370/XP thermal sensor driver
+ * Marvell EBU Armada SoCs thermal sensor driver
  *
  * Copyright (C) 2013 Marvell
  *
@@ -408,5 +408,5 @@ static struct platform_driver armada_thermal_driver = {
 module_platform_driver(armada_thermal_driver);
 
 MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-electrons.com>");
-MODULE_DESCRIPTION("Armada 370/XP thermal driver");
+MODULE_DESCRIPTION("Marvell EBU Armada SoCs thermal driver");
 MODULE_LICENSE("GPL v2");
-- 
2.11.0

[PATCH v4 09/12] thermal: armada: Change sensors trim default value

[Miquel Raynal]

Errata #132698 highlights an error in the default value of Tc trim.
Set this parameter to b'011.

Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index cef5c65c8f32..2eadd662591d 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -46,6 +46,10 @@
 #define CONTROL0_OFFSET			0x0
 #define CONTROL1_OFFSET			0x4
 
+/* Errata fields */
+#define CONTROL0_TSEN_TC_TRIM_MASK	0x7
+#define CONTROL0_TSEN_TC_TRIM_VAL	0x3
+
 /* TSEN refers to the temperature sensors within the AP */
 #define CONTROL0_TSEN_START		BIT(0)
 #define CONTROL0_TSEN_RESET		BIT(1)
@@ -161,6 +165,15 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
 	writel(reg, priv->control1);
 	msleep(10);
+
+	/* Set Tsen Tc Trim to correct default value (errata #132698) */
+	if (priv->control0) {
+		reg = readl_relaxed(priv->control0);
+		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
+		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
+		writel(reg, priv->control0);
+		msleep(10);
+	}
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,
-- 
2.11.0

[PATCH v4 09/12] thermal: armada: Change sensors trim default value

[Miquel Raynal]

Errata #132698 highlights an error in the default value of Tc trim.
Set this parameter to b'011.

Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index cef5c65c8f32..2eadd662591d 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -46,6 +46,10 @@
 #define CONTROL0_OFFSET			0x0
 #define CONTROL1_OFFSET			0x4
 
+/* Errata fields */
+#define CONTROL0_TSEN_TC_TRIM_MASK	0x7
+#define CONTROL0_TSEN_TC_TRIM_VAL	0x3
+
 /* TSEN refers to the temperature sensors within the AP */
 #define CONTROL0_TSEN_START		BIT(0)
 #define CONTROL0_TSEN_RESET		BIT(1)
@@ -161,6 +165,15 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
 	writel(reg, priv->control1);
 	msleep(10);
+
+	/* Set Tsen Tc Trim to correct default value (errata #132698) */
+	if (priv->control0) {
+		reg = readl_relaxed(priv->control0);
+		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
+		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
+		writel(reg, priv->control0);
+		msleep(10);
+	}
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,
-- 
2.11.0

[PATCH v4 10/12] thermal: armada: Wait sensors validity before exiting the init callback

[Miquel Raynal]

The thermal core will check for sensors validity right after the
initialization callback has returned. As the initialization routine make
a reset, the sensors are not ready immediately and the core spawns an
error in the dmesg. Avoid this annoying situation by polling on the
validity bit before exiting from these routines. This also avoid the use
of blind sleeps.

Suggested-by: David Sniatkiwicz <davidsn-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/armada_thermal.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 2eadd662591d..4a5164ddffe7 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_device.h>
 #include <linux/thermal.h>
+#include <linux/iopoll.h>
 
 /* Thermal Manager Control and Status Register */
 #define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
@@ -59,6 +60,9 @@
 #define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
 #define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
 
+#define STATUS_POLL_PERIOD_US		1000
+#define STATUS_POLL_TIMEOUT_US		100000
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -155,6 +159,16 @@ static void armada375_init_sensor(struct platform_device *pdev,
 	msleep(50);
 }
 
+static void armada_wait_sensor_validity(struct armada_thermal_priv *priv)
+{
+	u32 reg;
+
+	readl_relaxed_poll_timeout(priv->status, reg,
+				   reg & priv->data->is_valid_bit,
+				   STATUS_POLL_PERIOD_US,
+				   STATUS_POLL_TIMEOUT_US);
+}
+
 static void armada380_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
@@ -164,7 +178,6 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
 	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
 	writel(reg, priv->control1);
-	msleep(10);
 
 	/* Set Tsen Tc Trim to correct default value (errata #132698) */
 	if (priv->control0) {
@@ -172,8 +185,10 @@ static void armada380_init_sensor(struct platform_device *pdev,
 		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
 		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
 		writel(reg, priv->control0);
-		msleep(10);
 	}
+
+	/* Wait the sensors to be valid or the core will warn the user */
+	armada_wait_sensor_validity(priv);
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,
@@ -185,7 +200,9 @@ static void armada_ap806_init_sensor(struct platform_device *pdev,
 	reg &= ~CONTROL0_TSEN_RESET;
 	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
 	writel(reg, priv->control0);
-	msleep(10);
+
+	/* Wait the sensors to be valid or the core will warn the user */
+	armada_wait_sensor_validity(priv);
 }
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 10/12] thermal: armada: Wait sensors validity before exiting the init callback

[Miquel Raynal]

The thermal core will check for sensors validity right after the
initialization callback has returned. As the initialization routine make
a reset, the sensors are not ready immediately and the core spawns an
error in the dmesg. Avoid this annoying situation by polling on the
validity bit before exiting from these routines. This also avoid the use
of blind sleeps.

Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 2eadd662591d..4a5164ddffe7 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_device.h>
 #include <linux/thermal.h>
+#include <linux/iopoll.h>
 
 /* Thermal Manager Control and Status Register */
 #define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
@@ -59,6 +60,9 @@
 #define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
 #define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
 
+#define STATUS_POLL_PERIOD_US		1000
+#define STATUS_POLL_TIMEOUT_US		100000
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -155,6 +159,16 @@ static void armada375_init_sensor(struct platform_device *pdev,
 	msleep(50);
 }
 
+static void armada_wait_sensor_validity(struct armada_thermal_priv *priv)
+{
+	u32 reg;
+
+	readl_relaxed_poll_timeout(priv->status, reg,
+				   reg & priv->data->is_valid_bit,
+				   STATUS_POLL_PERIOD_US,
+				   STATUS_POLL_TIMEOUT_US);
+}
+
 static void armada380_init_sensor(struct platform_device *pdev,
 				  struct armada_thermal_priv *priv)
 {
@@ -164,7 +178,6 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
 	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
 	writel(reg, priv->control1);
-	msleep(10);
 
 	/* Set Tsen Tc Trim to correct default value (errata #132698) */
 	if (priv->control0) {
@@ -172,8 +185,10 @@ static void armada380_init_sensor(struct platform_device *pdev,
 		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
 		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
 		writel(reg, priv->control0);
-		msleep(10);
 	}
+
+	/* Wait the sensors to be valid or the core will warn the user */
+	armada_wait_sensor_validity(priv);
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,
@@ -185,7 +200,9 @@ static void armada_ap806_init_sensor(struct platform_device *pdev,
 	reg &= ~CONTROL0_TSEN_RESET;
 	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
 	writel(reg, priv->control0);
-	msleep(10);
+
+	/* Wait the sensors to be valid or the core will warn the user */
+	armada_wait_sensor_validity(priv);
 }
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
-- 
2.11.0

[PATCH v4 11/12] thermal: armada: Give meaningful names to the thermal zones

[Miquel Raynal]

After registration to the thermal core, sysfs will make one entry
per instance of the driver in /sys/class/thermal_zoneX and
/sys/class/hwmon/hwmonX, X being the index of the instance, all of them
having the type/name "armada_thermal".

Until now there was only one thermal zone per SoC but SoCs like Armada
A7K and Armada A8K have respectively two and three thermal zones (one
per AP and one per CP) and this number is subject to grow in the future.

Use dev_name() instead of the "armada_thermal" string to get a
meaningful name and be able to identify the thermal zones from
userspace.

Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/thermal/armada_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 4a5164ddffe7..42ef80b3b5f8 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -403,8 +403,8 @@ static int armada_thermal_probe(struct platform_device *pdev)
 
 	priv->data->init_sensor(pdev, priv);
 
-	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
-					       priv, &ops, NULL, 0, 0);
+	thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv,
+					       &ops, NULL, 0, 0);
 	if (IS_ERR(thermal)) {
 		dev_err(&pdev->dev,
 			"Failed to register thermal zone device\n");
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 11/12] thermal: armada: Give meaningful names to the thermal zones

[Miquel Raynal]

After registration to the thermal core, sysfs will make one entry
per instance of the driver in /sys/class/thermal_zoneX and
/sys/class/hwmon/hwmonX, X being the index of the instance, all of them
having the type/name "armada_thermal".

Until now there was only one thermal zone per SoC but SoCs like Armada
A7K and Armada A8K have respectively two and three thermal zones (one
per AP and one per CP) and this number is subject to grow in the future.

Use dev_name() instead of the "armada_thermal" string to get a
meaningful name and be able to identify the thermal zones from
userspace.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/thermal/armada_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 4a5164ddffe7..42ef80b3b5f8 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -403,8 +403,8 @@ static int armada_thermal_probe(struct platform_device *pdev)
 
 	priv->data->init_sensor(pdev, priv);
 
-	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
-					       priv, &ops, NULL, 0, 0);
+	thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv,
+					       &ops, NULL, 0, 0);
 	if (IS_ERR(thermal)) {
 		dev_err(&pdev->dev,
 			"Failed to register thermal zone device\n");
-- 
2.11.0

[PATCH v4 12/12] ARM64: dts: marvell: Add thermal support for A7K/A8K

[Miquel Raynal]

Add thermal DT nodes in AP806 and CP110 master/slave DTSI files.

Suggested-by: David Sniatkiwicz <davidsn-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 6 ++++++
 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 ++++++
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
index 1c4dd8ab9ad5..bbc5a4d3acac 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
@@ -285,6 +285,12 @@
 					gpio-ranges = <&ap_pinctrl 0 0 20>;
 				};
 			};
+
+			ap_thermal: thermal@6f808C {
+				compatible = "marvell,armada-ap806-thermal";
+				reg = <0x6f808C 0x4>,
+				      <0x6f8084 0x8>;
+			};
 		};
 	};
 };
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index e3b64d03fbd8..ecbc76d26dff 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -182,6 +182,12 @@
 				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			cpm_thermal: thermal@400078 {
+				compatible = "marvell,armada-cp110-thermal";
+				reg = <0x400078 0x4>,
+				      <0x400070 0x8>;
+			};
+
 			cpm_syscon0: system-controller@440000 {
 				compatible = "syscon", "simple-mfd";
 				reg = <0x440000 0x2000>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index 0d51096c69f8..29ba32c68870 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -182,6 +182,12 @@
 				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			cps_thermal: thermal@400078 {
+				compatible = "marvell,armada-cp110-thermal";
+				reg = <0x400078 0x4>,
+				      <0x400070 0x8>;
+			};
+
 			cps_syscon0: system-controller@440000 {
 				compatible = "syscon", "simple-mfd";
 				reg = <0x440000 0x2000>;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 12/12] ARM64: dts: marvell: Add thermal support for A7K/A8K

[Miquel Raynal]

Add thermal DT nodes in AP806 and CP110 master/slave DTSI files.

Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 6 ++++++
 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 ++++++
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
index 1c4dd8ab9ad5..bbc5a4d3acac 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
@@ -285,6 +285,12 @@
 					gpio-ranges = <&ap_pinctrl 0 0 20>;
 				};
 			};
+
+			ap_thermal: thermal at 6f808C {
+				compatible = "marvell,armada-ap806-thermal";
+				reg = <0x6f808C 0x4>,
+				      <0x6f8084 0x8>;
+			};
 		};
 	};
 };
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index e3b64d03fbd8..ecbc76d26dff 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -182,6 +182,12 @@
 				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			cpm_thermal: thermal at 400078 {
+				compatible = "marvell,armada-cp110-thermal";
+				reg = <0x400078 0x4>,
+				      <0x400070 0x8>;
+			};
+
 			cpm_syscon0: system-controller at 440000 {
 				compatible = "syscon", "simple-mfd";
 				reg = <0x440000 0x2000>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index 0d51096c69f8..29ba32c68870 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -182,6 +182,12 @@
 				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			cps_thermal: thermal at 400078 {
+				compatible = "marvell,armada-cp110-thermal";
+				reg = <0x400078 0x4>,
+				      <0x400070 0x8>;
+			};
+
 			cps_syscon0: system-controller at 440000 {
 				compatible = "syscon", "simple-mfd";
 				reg = <0x440000 0x2000>;
-- 
2.11.0

Re: [PATCH v4 05/12] thermal: armada: Use real status register name

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Three 32-bit registers are used to drive the thermal IP: control0,
> control1 and status. The two control registers share the same name both
> in the documentation and in the code, while the latter is referred as
> "sensor" in the code. Rename this pointer to be called "status" in order
> to be aligned with the documentation.
>
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Gregory

> ---
>  drivers/thermal/armada_thermal.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index f422563e617c..198485fa77f2 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -51,7 +51,7 @@ struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
>  struct armada_thermal_priv {
> -	void __iomem *sensor;
> +	void __iomem *status;
>  	void __iomem *control0;
>  	void __iomem *control1;
>  	struct armada_thermal_data *data;
> @@ -98,9 +98,9 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
>  	writel(reg, priv->control1);
>  
>  	/* Enable the sensor */
> -	reg = readl_relaxed(priv->sensor);
> +	reg = readl_relaxed(priv->status);
>  	reg &= ~PMU_TM_DISABLE_MASK;
> -	writel(reg, priv->sensor);
> +	writel(reg, priv->status);
>  }
>  
>  static void armada370_init_sensor(struct platform_device *pdev,
> @@ -156,7 +156,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  
>  static bool armada_is_valid(struct armada_thermal_priv *priv)
>  {
> -	u32 reg = readl_relaxed(priv->sensor);
> +	u32 reg = readl_relaxed(priv->status);
>  
>  	return reg & priv->data->is_valid_bit;
>  }
> @@ -175,7 +175,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  		return -EIO;
>  	}
>  
> -	reg = readl_relaxed(priv->sensor);
> +	reg = readl_relaxed(priv->status);
>  	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
>  
>  	/* Get formula coeficients */
> @@ -277,9 +277,9 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(priv->sensor))
> -		return PTR_ERR(priv->sensor);
> +	priv->status = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->status))
> +		return PTR_ERR(priv->status);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>  	control = devm_ioremap_resource(&pdev->dev, res);
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 05/12] thermal: armada: Use real status register name

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Three 32-bit registers are used to drive the thermal IP: control0,
> control1 and status. The two control registers share the same name both
> in the documentation and in the code, while the latter is referred as
> "sensor" in the code. Rename this pointer to be called "status" in order
> to be aligned with the documentation.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory

> ---
>  drivers/thermal/armada_thermal.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index f422563e617c..198485fa77f2 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -51,7 +51,7 @@ struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
>  struct armada_thermal_priv {
> -	void __iomem *sensor;
> +	void __iomem *status;
>  	void __iomem *control0;
>  	void __iomem *control1;
>  	struct armada_thermal_data *data;
> @@ -98,9 +98,9 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
>  	writel(reg, priv->control1);
>  
>  	/* Enable the sensor */
> -	reg = readl_relaxed(priv->sensor);
> +	reg = readl_relaxed(priv->status);
>  	reg &= ~PMU_TM_DISABLE_MASK;
> -	writel(reg, priv->sensor);
> +	writel(reg, priv->status);
>  }
>  
>  static void armada370_init_sensor(struct platform_device *pdev,
> @@ -156,7 +156,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  
>  static bool armada_is_valid(struct armada_thermal_priv *priv)
>  {
> -	u32 reg = readl_relaxed(priv->sensor);
> +	u32 reg = readl_relaxed(priv->status);
>  
>  	return reg & priv->data->is_valid_bit;
>  }
> @@ -175,7 +175,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  		return -EIO;
>  	}
>  
> -	reg = readl_relaxed(priv->sensor);
> +	reg = readl_relaxed(priv->status);
>  	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
>  
>  	/* Get formula coeficients */
> @@ -277,9 +277,9 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(priv->sensor))
> -		return PTR_ERR(priv->sensor);
> +	priv->status = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->status))
> +		return PTR_ERR(priv->status);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>  	control = devm_ioremap_resource(&pdev->dev, res);
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 06/12] thermal: armada: Add support for Armada AP806

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
>
> The AP806 component is integrated in the Armada 8K and 7K lines of
> processors.
>
> The thermal sensor sample field on the status register is a signed
> value. Extend armada_get_temp() and the driver structure to handle
> signed values.
>
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: Changes when applying over the
> previous patches, including the register names changes, also switched
> the coefficients values to s64 instead of unsigned long to deal with
> negative values and used do_div instead of the traditionnal '/']
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

I am just a little concerned by the fac that the value pass though
get_temp() is an int and now the intermediate calculation are in
s64. But maybe I'm too picky for this,

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Gregory


> ---
>  drivers/thermal/armada_thermal.c | 80 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 65 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 198485fa77f2..ec29ea76b818 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -47,6 +47,11 @@
>  #define CONTROL0_OFFSET			0x0
>  #define CONTROL1_OFFSET			0x4
>  
> +/* TSEN refers to the temperature sensors within the AP */
> +#define CONTROL0_TSEN_START		BIT(0)
> +#define CONTROL0_TSEN_RESET		BIT(1)
> +#define CONTROL0_TSEN_ENABLE		BIT(2)
> +
>  struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
> @@ -66,15 +71,17 @@ struct armada_thermal_data {
>  	bool (*is_valid)(struct armada_thermal_priv *);
>  
>  	/* Formula coeficients: temp = (b - m * reg) / div */
> -	unsigned long coef_b;
> -	unsigned long coef_m;
> -	unsigned long coef_div;
> +	s64 coef_b;
> +	s64 coef_m;
> +	u32 coef_div;
>  	bool inverted;
> +	bool signed_sample;
>  
>  	/* Register shift and mask to access the sensor temperature */
>  	unsigned int temp_shift;
>  	unsigned int temp_mask;
>  	u32 is_valid_bit;
> +	bool needs_control0;
>  };
>  
>  static void armadaxp_init_sensor(struct platform_device *pdev,
> @@ -154,6 +161,18 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  	}
>  }
>  
> +static void armada_ap806_init_sensor(struct platform_device *pdev,
> +				     struct armada_thermal_priv *priv)
> +{
> +	u32 reg;
> +
> +	reg = readl_relaxed(priv->control0);
> +	reg &= ~CONTROL0_TSEN_RESET;
> +	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
> +	writel(reg, priv->control0);
> +	msleep(10);
> +}
> +
>  static bool armada_is_valid(struct armada_thermal_priv *priv)
>  {
>  	u32 reg = readl_relaxed(priv->status);
> @@ -165,8 +184,8 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  			  int *temp)
>  {
>  	struct armada_thermal_priv *priv = thermal->devdata;
> -	unsigned long reg;
> -	unsigned long m, b, div;
> +	u32 reg, div;
> +	s64 sample, b, m;
>  
>  	/* Valid check */
>  	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
> @@ -177,6 +196,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  
>  	reg = readl_relaxed(priv->status);
>  	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
> +	if (priv->data->signed_sample)
> +		/* The most significant bit is the sign bit */
> +		sample = sign_extend32(reg, fls(priv->data->temp_mask) - 1);
> +	else
> +		sample = reg;
>  
>  	/* Get formula coeficients */
>  	b = priv->data->coef_b;
> @@ -184,9 +208,12 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  	div = priv->data->coef_div;
>  
>  	if (priv->data->inverted)
> -		*temp = ((m * reg) - b) / div;
> +		*temp = (m * sample) - b;
>  	else
> -		*temp = (b - (m * reg)) / div;
> +		*temp = b - (m * sample);
> +
> +	do_div(*temp, div);
> +
>  	return 0;
>  }
>  
> @@ -198,8 +225,8 @@ static const struct armada_thermal_data armadaxp_data = {
>  	.init_sensor = armadaxp_init_sensor,
>  	.temp_shift = 10,
>  	.temp_mask = 0x1ff,
> -	.coef_b = 3153000000UL,
> -	.coef_m = 10000000UL,
> +	.coef_b = 3153000000ULL,
> +	.coef_m = 10000000ULL,
>  	.coef_div = 13825,
>  };
>  
> @@ -209,8 +236,8 @@ static const struct armada_thermal_data armada370_data = {
>  	.is_valid_bit = BIT(9),
>  	.temp_shift = 10,
>  	.temp_mask = 0x1ff,
> -	.coef_b = 3153000000UL,
> -	.coef_m = 10000000UL,
> +	.coef_b = 3153000000ULL,
> +	.coef_m = 10000000ULL,
>  	.coef_div = 13825,
>  };
>  
> @@ -220,8 +247,8 @@ static const struct armada_thermal_data armada375_data = {
>  	.is_valid_bit = BIT(10),
>  	.temp_shift = 0,
>  	.temp_mask = 0x1ff,
> -	.coef_b = 3171900000UL,
> -	.coef_m = 10000000UL,
> +	.coef_b = 3171900000ULL,
> +	.coef_m = 10000000ULL,
>  	.coef_div = 13616,
>  };
>  
> @@ -231,12 +258,26 @@ static const struct armada_thermal_data armada380_data = {
>  	.is_valid_bit = BIT(10),
>  	.temp_shift = 0,
>  	.temp_mask = 0x3ff,
> -	.coef_b = 1172499100UL,
> -	.coef_m = 2000096UL,
> +	.coef_b = 1172499100ULL,
> +	.coef_m = 2000096ULL,
>  	.coef_div = 4201,
>  	.inverted = true,
>  };
>  
> +static const struct armada_thermal_data armada_ap806_data = {
> +	.is_valid = armada_is_valid,
> +	.init_sensor = armada_ap806_init_sensor,
> +	.is_valid_bit = BIT(16),
> +	.temp_shift = 0,
> +	.temp_mask = 0x3ff,
> +	.coef_b = -150000LL,
> +	.coef_m = 423ULL,
> +	.coef_div = 1,
> +	.inverted = true,
> +	.signed_sample = true,
> +	.needs_control0 = true,
> +};
> +
>  static const struct of_device_id armada_thermal_id_table[] = {
>  	{
>  		.compatible = "marvell,armadaxp-thermal",
> @@ -255,6 +296,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
>  		.data       = &armada380_data,
>  	},
>  	{
> +		.compatible = "marvell,armada-ap806-thermal",
> +		.data       = &armada_ap806_data,
> +	},
> +	{
>  		/* sentinel */
>  	},
>  };
> @@ -296,6 +341,11 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  	 */
>  	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
>  		/* ->control0 unavailable in this configuration */
> +		if (priv->data->needs_control0) {
> +			dev_err(&pdev->dev, "No access to control0 register\n");
> +			return -EINVAL;
> +		}
> +
>  		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
>  	} else {
>  		priv->control0 = control + CONTROL0_OFFSET;
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 06/12] thermal: armada: Add support for Armada AP806

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> From: Baruch Siach <baruch@tkos.co.il>
>
> The AP806 component is integrated in the Armada 8K and 7K lines of
> processors.
>
> The thermal sensor sample field on the status register is a signed
> value. Extend armada_get_temp() and the driver structure to handle
> signed values.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> [<miquel.raynal@free-electrons.com>: Changes when applying over the
> previous patches, including the register names changes, also switched
> the coefficients values to s64 instead of unsigned long to deal with
> negative values and used do_div instead of the traditionnal '/']
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

I am just a little concerned by the fac that the value pass though
get_temp() is an int and now the intermediate calculation are in
s64. But maybe I'm too picky for this,

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory


> ---
>  drivers/thermal/armada_thermal.c | 80 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 65 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 198485fa77f2..ec29ea76b818 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -47,6 +47,11 @@
>  #define CONTROL0_OFFSET			0x0
>  #define CONTROL1_OFFSET			0x4
>  
> +/* TSEN refers to the temperature sensors within the AP */
> +#define CONTROL0_TSEN_START		BIT(0)
> +#define CONTROL0_TSEN_RESET		BIT(1)
> +#define CONTROL0_TSEN_ENABLE		BIT(2)
> +
>  struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
> @@ -66,15 +71,17 @@ struct armada_thermal_data {
>  	bool (*is_valid)(struct armada_thermal_priv *);
>  
>  	/* Formula coeficients: temp = (b - m * reg) / div */
> -	unsigned long coef_b;
> -	unsigned long coef_m;
> -	unsigned long coef_div;
> +	s64 coef_b;
> +	s64 coef_m;
> +	u32 coef_div;
>  	bool inverted;
> +	bool signed_sample;
>  
>  	/* Register shift and mask to access the sensor temperature */
>  	unsigned int temp_shift;
>  	unsigned int temp_mask;
>  	u32 is_valid_bit;
> +	bool needs_control0;
>  };
>  
>  static void armadaxp_init_sensor(struct platform_device *pdev,
> @@ -154,6 +161,18 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  	}
>  }
>  
> +static void armada_ap806_init_sensor(struct platform_device *pdev,
> +				     struct armada_thermal_priv *priv)
> +{
> +	u32 reg;
> +
> +	reg = readl_relaxed(priv->control0);
> +	reg &= ~CONTROL0_TSEN_RESET;
> +	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
> +	writel(reg, priv->control0);
> +	msleep(10);
> +}
> +
>  static bool armada_is_valid(struct armada_thermal_priv *priv)
>  {
>  	u32 reg = readl_relaxed(priv->status);
> @@ -165,8 +184,8 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  			  int *temp)
>  {
>  	struct armada_thermal_priv *priv = thermal->devdata;
> -	unsigned long reg;
> -	unsigned long m, b, div;
> +	u32 reg, div;
> +	s64 sample, b, m;
>  
>  	/* Valid check */
>  	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
> @@ -177,6 +196,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  
>  	reg = readl_relaxed(priv->status);
>  	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
> +	if (priv->data->signed_sample)
> +		/* The most significant bit is the sign bit */
> +		sample = sign_extend32(reg, fls(priv->data->temp_mask) - 1);
> +	else
> +		sample = reg;
>  
>  	/* Get formula coeficients */
>  	b = priv->data->coef_b;
> @@ -184,9 +208,12 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  	div = priv->data->coef_div;
>  
>  	if (priv->data->inverted)
> -		*temp = ((m * reg) - b) / div;
> +		*temp = (m * sample) - b;
>  	else
> -		*temp = (b - (m * reg)) / div;
> +		*temp = b - (m * sample);
> +
> +	do_div(*temp, div);
> +
>  	return 0;
>  }
>  
> @@ -198,8 +225,8 @@ static const struct armada_thermal_data armadaxp_data = {
>  	.init_sensor = armadaxp_init_sensor,
>  	.temp_shift = 10,
>  	.temp_mask = 0x1ff,
> -	.coef_b = 3153000000UL,
> -	.coef_m = 10000000UL,
> +	.coef_b = 3153000000ULL,
> +	.coef_m = 10000000ULL,
>  	.coef_div = 13825,
>  };
>  
> @@ -209,8 +236,8 @@ static const struct armada_thermal_data armada370_data = {
>  	.is_valid_bit = BIT(9),
>  	.temp_shift = 10,
>  	.temp_mask = 0x1ff,
> -	.coef_b = 3153000000UL,
> -	.coef_m = 10000000UL,
> +	.coef_b = 3153000000ULL,
> +	.coef_m = 10000000ULL,
>  	.coef_div = 13825,
>  };
>  
> @@ -220,8 +247,8 @@ static const struct armada_thermal_data armada375_data = {
>  	.is_valid_bit = BIT(10),
>  	.temp_shift = 0,
>  	.temp_mask = 0x1ff,
> -	.coef_b = 3171900000UL,
> -	.coef_m = 10000000UL,
> +	.coef_b = 3171900000ULL,
> +	.coef_m = 10000000ULL,
>  	.coef_div = 13616,
>  };
>  
> @@ -231,12 +258,26 @@ static const struct armada_thermal_data armada380_data = {
>  	.is_valid_bit = BIT(10),
>  	.temp_shift = 0,
>  	.temp_mask = 0x3ff,
> -	.coef_b = 1172499100UL,
> -	.coef_m = 2000096UL,
> +	.coef_b = 1172499100ULL,
> +	.coef_m = 2000096ULL,
>  	.coef_div = 4201,
>  	.inverted = true,
>  };
>  
> +static const struct armada_thermal_data armada_ap806_data = {
> +	.is_valid = armada_is_valid,
> +	.init_sensor = armada_ap806_init_sensor,
> +	.is_valid_bit = BIT(16),
> +	.temp_shift = 0,
> +	.temp_mask = 0x3ff,
> +	.coef_b = -150000LL,
> +	.coef_m = 423ULL,
> +	.coef_div = 1,
> +	.inverted = true,
> +	.signed_sample = true,
> +	.needs_control0 = true,
> +};
> +
>  static const struct of_device_id armada_thermal_id_table[] = {
>  	{
>  		.compatible = "marvell,armadaxp-thermal",
> @@ -255,6 +296,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
>  		.data       = &armada380_data,
>  	},
>  	{
> +		.compatible = "marvell,armada-ap806-thermal",
> +		.data       = &armada_ap806_data,
> +	},
> +	{
>  		/* sentinel */
>  	},
>  };
> @@ -296,6 +341,11 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  	 */
>  	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
>  		/* ->control0 unavailable in this configuration */
> +		if (priv->data->needs_control0) {
> +			dev_err(&pdev->dev, "No access to control0 register\n");
> +			return -EINVAL;
> +		}
> +
>  		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
>  	} else {
>  		priv->control0 = control + CONTROL0_OFFSET;
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 07/12] thermal: armada: Add support for Armada CP110

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
>
> The CP110 component is integrated in the Armada 8k and 7k lines of
> processors.
>
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: renamed the register pointers as
> well as some definitions related to the new register names and
> simplified the init sequence for Armada 380]
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Gregory

> ---
>  drivers/thermal/armada_thermal.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index ec29ea76b818..11a94ad66c35 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -37,7 +37,6 @@
>  #define A375_UNIT_CONTROL_MASK		0x7
>  #define A375_READOUT_INVERT		BIT(15)
>  #define A375_HW_RESETn			BIT(8)
> -#define A380_HW_RESET			BIT(8)
>  
>  /* Legacy bindings */
>  #define LEGACY_CONTROL_MEM_LEN		0x4
> @@ -52,6 +51,10 @@
>  #define CONTROL0_TSEN_RESET		BIT(1)
>  #define CONTROL0_TSEN_ENABLE		BIT(2)
>  
> +/* EXT_TSEN refers to the external temperature sensors, out of the AP */
> +#define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
> +#define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
> +
>  struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
> @@ -153,12 +156,11 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  {
>  	u32 reg = readl_relaxed(priv->control1);
>  
> -	/* Reset hardware once */
> -	if (!(reg & A380_HW_RESET)) {
> -		reg |= A380_HW_RESET;
> -		writel(reg, priv->control1);
> -		msleep(10);
> -	}
> +	/* Disable the HW/SW reset */
> +	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
> +	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
> +	writel(reg, priv->control1);
> +	msleep(10);
>  }
>  
>  static void armada_ap806_init_sensor(struct platform_device *pdev,
> @@ -278,6 +280,19 @@ static const struct armada_thermal_data armada_ap806_data = {
>  	.needs_control0 = true,
>  };
>  
> +static const struct armada_thermal_data armada_cp110_data = {
> +	.is_valid = armada_is_valid,
> +	.init_sensor = armada380_init_sensor,
> +	.is_valid_bit = BIT(10),
> +	.temp_shift = 0,
> +	.temp_mask = 0x3ff,
> +	.coef_b = 1172499100ULL,
> +	.coef_m = 2000096ULL,
> +	.coef_div = 4201,
> +	.inverted = true,
> +	.needs_control0 = true,
> +};
> +
>  static const struct of_device_id armada_thermal_id_table[] = {
>  	{
>  		.compatible = "marvell,armadaxp-thermal",
> @@ -300,6 +315,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
>  		.data       = &armada_ap806_data,
>  	},
>  	{
> +		.compatible = "marvell,armada-cp110-thermal",
> +		.data       = &armada_cp110_data,
> +	},
> +	{
>  		/* sentinel */
>  	},
>  };
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 07/12] thermal: armada: Add support for Armada CP110

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> From: Baruch Siach <baruch@tkos.co.il>
>
> The CP110 component is integrated in the Armada 8k and 7k lines of
> processors.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> [<miquel.raynal@free-electrons.com>: renamed the register pointers as
> well as some definitions related to the new register names and
> simplified the init sequence for Armada 380]
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory

> ---
>  drivers/thermal/armada_thermal.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index ec29ea76b818..11a94ad66c35 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -37,7 +37,6 @@
>  #define A375_UNIT_CONTROL_MASK		0x7
>  #define A375_READOUT_INVERT		BIT(15)
>  #define A375_HW_RESETn			BIT(8)
> -#define A380_HW_RESET			BIT(8)
>  
>  /* Legacy bindings */
>  #define LEGACY_CONTROL_MEM_LEN		0x4
> @@ -52,6 +51,10 @@
>  #define CONTROL0_TSEN_RESET		BIT(1)
>  #define CONTROL0_TSEN_ENABLE		BIT(2)
>  
> +/* EXT_TSEN refers to the external temperature sensors, out of the AP */
> +#define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
> +#define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
> +
>  struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
> @@ -153,12 +156,11 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  {
>  	u32 reg = readl_relaxed(priv->control1);
>  
> -	/* Reset hardware once */
> -	if (!(reg & A380_HW_RESET)) {
> -		reg |= A380_HW_RESET;
> -		writel(reg, priv->control1);
> -		msleep(10);
> -	}
> +	/* Disable the HW/SW reset */
> +	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
> +	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
> +	writel(reg, priv->control1);
> +	msleep(10);
>  }
>  
>  static void armada_ap806_init_sensor(struct platform_device *pdev,
> @@ -278,6 +280,19 @@ static const struct armada_thermal_data armada_ap806_data = {
>  	.needs_control0 = true,
>  };
>  
> +static const struct armada_thermal_data armada_cp110_data = {
> +	.is_valid = armada_is_valid,
> +	.init_sensor = armada380_init_sensor,
> +	.is_valid_bit = BIT(10),
> +	.temp_shift = 0,
> +	.temp_mask = 0x3ff,
> +	.coef_b = 1172499100ULL,
> +	.coef_m = 2000096ULL,
> +	.coef_div = 4201,
> +	.inverted = true,
> +	.needs_control0 = true,
> +};
> +
>  static const struct of_device_id armada_thermal_id_table[] = {
>  	{
>  		.compatible = "marvell,armadaxp-thermal",
> @@ -300,6 +315,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
>  		.data       = &armada_ap806_data,
>  	},
>  	{
> +		.compatible = "marvell,armada-cp110-thermal",
> +		.data       = &armada_cp110_data,
> +	},
> +	{
>  		/* sentinel */
>  	},
>  };
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 08/12] thermal: armada: Update Kconfig and module description

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Update Armada thermal driver Kconfig entry as well as the driver's
> MODULE_DESCRIPTION content, now that 64-bit SoCs are also supported,
> eg. Armada 7K and Armada 8K.
>
> Use the generic term "Marvell EBU Armada SoCs" instead of listing all
> the supported SoCs everywhere (excepted in the Kconfig description,
> where it is useful to have a list).
>
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Gregory


> ---
>  drivers/thermal/Kconfig          | 4 ++--
>  drivers/thermal/armada_thermal.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 315ae2926e20..b6adc54b96f1 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -301,13 +301,13 @@ config DB8500_THERMAL
>  	  thermal zone if trip points reached.
>  
>  config ARMADA_THERMAL
> -	tristate "Armada 370/XP thermal management"
> +	tristate "Marvell EBU Armada SoCs thermal management"
>  	depends on ARCH_MVEBU || COMPILE_TEST
>  	depends on HAS_IOMEM
>  	depends on OF
>  	help
>  	  Enable this option if you want to have support for thermal management
> -	  controller present in Armada 370 and Armada XP SoC.
> +	  controller present in Marvell EBU Armada SoCs (370,375,XP,38x,7K,8K).
>  
>  config DA9062_THERMAL
>  	tristate "DA9062/DA9061 Dialog Semiconductor thermal driver"
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 11a94ad66c35..cef5c65c8f32 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -1,5 +1,5 @@
>  /*
> - * Marvell Armada 370/XP thermal sensor driver
> + * Marvell EBU Armada SoCs thermal sensor driver
>   *
>   * Copyright (C) 2013 Marvell
>   *
> @@ -408,5 +408,5 @@ static struct platform_driver armada_thermal_driver = {
>  module_platform_driver(armada_thermal_driver);
>  
>  MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
> -MODULE_DESCRIPTION("Armada 370/XP thermal driver");
> +MODULE_DESCRIPTION("Marvell EBU Armada SoCs thermal driver");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 08/12] thermal: armada: Update Kconfig and module description

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Update Armada thermal driver Kconfig entry as well as the driver's
> MODULE_DESCRIPTION content, now that 64-bit SoCs are also supported,
> eg. Armada 7K and Armada 8K.
>
> Use the generic term "Marvell EBU Armada SoCs" instead of listing all
> the supported SoCs everywhere (excepted in the Kconfig description,
> where it is useful to have a list).
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory


> ---
>  drivers/thermal/Kconfig          | 4 ++--
>  drivers/thermal/armada_thermal.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 315ae2926e20..b6adc54b96f1 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -301,13 +301,13 @@ config DB8500_THERMAL
>  	  thermal zone if trip points reached.
>  
>  config ARMADA_THERMAL
> -	tristate "Armada 370/XP thermal management"
> +	tristate "Marvell EBU Armada SoCs thermal management"
>  	depends on ARCH_MVEBU || COMPILE_TEST
>  	depends on HAS_IOMEM
>  	depends on OF
>  	help
>  	  Enable this option if you want to have support for thermal management
> -	  controller present in Armada 370 and Armada XP SoC.
> +	  controller present in Marvell EBU Armada SoCs (370,375,XP,38x,7K,8K).
>  
>  config DA9062_THERMAL
>  	tristate "DA9062/DA9061 Dialog Semiconductor thermal driver"
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 11a94ad66c35..cef5c65c8f32 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -1,5 +1,5 @@
>  /*
> - * Marvell Armada 370/XP thermal sensor driver
> + * Marvell EBU Armada SoCs thermal sensor driver
>   *
>   * Copyright (C) 2013 Marvell
>   *
> @@ -408,5 +408,5 @@ static struct platform_driver armada_thermal_driver = {
>  module_platform_driver(armada_thermal_driver);
>  
>  MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-electrons.com>");
> -MODULE_DESCRIPTION("Armada 370/XP thermal driver");
> +MODULE_DESCRIPTION("Marvell EBU Armada SoCs thermal driver");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 09/12] thermal: armada: Change sensors trim default value

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Errata #132698 highlights an error in the default value of Tc trim.
> Set this parameter to b'011.
>
> Suggested-by: David Sniatkiwicz <davidsn-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Gregory

> ---
>  drivers/thermal/armada_thermal.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index cef5c65c8f32..2eadd662591d 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -46,6 +46,10 @@
>  #define CONTROL0_OFFSET			0x0
>  #define CONTROL1_OFFSET			0x4
>  
> +/* Errata fields */
> +#define CONTROL0_TSEN_TC_TRIM_MASK	0x7
> +#define CONTROL0_TSEN_TC_TRIM_VAL	0x3
> +
>  /* TSEN refers to the temperature sensors within the AP */
>  #define CONTROL0_TSEN_START		BIT(0)
>  #define CONTROL0_TSEN_RESET		BIT(1)
> @@ -161,6 +165,15 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
>  	writel(reg, priv->control1);
>  	msleep(10);
> +
> +	/* Set Tsen Tc Trim to correct default value (errata #132698) */
> +	if (priv->control0) {
> +		reg = readl_relaxed(priv->control0);
> +		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
> +		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
> +		writel(reg, priv->control0);
> +		msleep(10);
> +	}
>  }
>  
>  static void armada_ap806_init_sensor(struct platform_device *pdev,
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 09/12] thermal: armada: Change sensors trim default value

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Errata #132698 highlights an error in the default value of Tc trim.
> Set this parameter to b'011.
>
> Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory

> ---
>  drivers/thermal/armada_thermal.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index cef5c65c8f32..2eadd662591d 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -46,6 +46,10 @@
>  #define CONTROL0_OFFSET			0x0
>  #define CONTROL1_OFFSET			0x4
>  
> +/* Errata fields */
> +#define CONTROL0_TSEN_TC_TRIM_MASK	0x7
> +#define CONTROL0_TSEN_TC_TRIM_VAL	0x3
> +
>  /* TSEN refers to the temperature sensors within the AP */
>  #define CONTROL0_TSEN_START		BIT(0)
>  #define CONTROL0_TSEN_RESET		BIT(1)
> @@ -161,6 +165,15 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
>  	writel(reg, priv->control1);
>  	msleep(10);
> +
> +	/* Set Tsen Tc Trim to correct default value (errata #132698) */
> +	if (priv->control0) {
> +		reg = readl_relaxed(priv->control0);
> +		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
> +		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
> +		writel(reg, priv->control0);
> +		msleep(10);
> +	}
>  }
>  
>  static void armada_ap806_init_sensor(struct platform_device *pdev,
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 10/12] thermal: armada: Wait sensors validity before exiting the init callback

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> The thermal core will check for sensors validity right after the
> initialization callback has returned. As the initialization routine make
> a reset, the sensors are not ready immediately and the core spawns an
> error in the dmesg. Avoid this annoying situation by polling on the
> validity bit before exiting from these routines. This also avoid the use
> of blind sleeps.
>
> Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

> ---
>  drivers/thermal/armada_thermal.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 2eadd662591d..4a5164ddffe7 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -23,6 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
>  #include <linux/thermal.h>
> +#include <linux/iopoll.h>
>  
>  /* Thermal Manager Control and Status Register */
>  #define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
> @@ -59,6 +60,9 @@
>  #define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
>  #define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
>  
> +#define STATUS_POLL_PERIOD_US		1000
> +#define STATUS_POLL_TIMEOUT_US		100000
> +
>  struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
> @@ -155,6 +159,16 @@ static void armada375_init_sensor(struct platform_device *pdev,
>  	msleep(50);
>  }
>  
> +static void armada_wait_sensor_validity(struct armada_thermal_priv *priv)
> +{
> +	u32 reg;
> +
> +	readl_relaxed_poll_timeout(priv->status, reg,
> +				   reg & priv->data->is_valid_bit,
> +				   STATUS_POLL_PERIOD_US,
> +				   STATUS_POLL_TIMEOUT_US);
> +}
> +
>  static void armada380_init_sensor(struct platform_device *pdev,
>  				  struct armada_thermal_priv *priv)
>  {
> @@ -164,7 +178,6 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
>  	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
>  	writel(reg, priv->control1);
> -	msleep(10);
>  
>  	/* Set Tsen Tc Trim to correct default value (errata #132698) */
>  	if (priv->control0) {
> @@ -172,8 +185,10 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
>  		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
>  		writel(reg, priv->control0);
> -		msleep(10);
>  	}
> +
> +	/* Wait the sensors to be valid or the core will warn the user */
> +	armada_wait_sensor_validity(priv);
>  }
>  
>  static void armada_ap806_init_sensor(struct platform_device *pdev,
> @@ -185,7 +200,9 @@ static void armada_ap806_init_sensor(struct platform_device *pdev,
>  	reg &= ~CONTROL0_TSEN_RESET;
>  	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
>  	writel(reg, priv->control0);
> -	msleep(10);
> +
> +	/* Wait the sensors to be valid or the core will warn the user

Just out of curiosity but how the core will warn the user?

Gregory

> */
> +	armada_wait_sensor_validity(priv);
>  }
>  
>  static bool armada_is_valid(struct armada_thermal_priv *priv)
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 10/12] thermal: armada: Wait sensors validity before exiting the init callback

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> The thermal core will check for sensors validity right after the
> initialization callback has returned. As the initialization routine make
> a reset, the sensors are not ready immediately and the core spawns an
> error in the dmesg. Avoid this annoying situation by polling on the
> validity bit before exiting from these routines. This also avoid the use
> of blind sleeps.
>
> Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

> ---
>  drivers/thermal/armada_thermal.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 2eadd662591d..4a5164ddffe7 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -23,6 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
>  #include <linux/thermal.h>
> +#include <linux/iopoll.h>
>  
>  /* Thermal Manager Control and Status Register */
>  #define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
> @@ -59,6 +60,9 @@
>  #define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
>  #define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)
>  
> +#define STATUS_POLL_PERIOD_US		1000
> +#define STATUS_POLL_TIMEOUT_US		100000
> +
>  struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
> @@ -155,6 +159,16 @@ static void armada375_init_sensor(struct platform_device *pdev,
>  	msleep(50);
>  }
>  
> +static void armada_wait_sensor_validity(struct armada_thermal_priv *priv)
> +{
> +	u32 reg;
> +
> +	readl_relaxed_poll_timeout(priv->status, reg,
> +				   reg & priv->data->is_valid_bit,
> +				   STATUS_POLL_PERIOD_US,
> +				   STATUS_POLL_TIMEOUT_US);
> +}
> +
>  static void armada380_init_sensor(struct platform_device *pdev,
>  				  struct armada_thermal_priv *priv)
>  {
> @@ -164,7 +178,6 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
>  	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
>  	writel(reg, priv->control1);
> -	msleep(10);
>  
>  	/* Set Tsen Tc Trim to correct default value (errata #132698) */
>  	if (priv->control0) {
> @@ -172,8 +185,10 @@ static void armada380_init_sensor(struct platform_device *pdev,
>  		reg &= ~CONTROL0_TSEN_TC_TRIM_MASK;
>  		reg |= CONTROL0_TSEN_TC_TRIM_VAL;
>  		writel(reg, priv->control0);
> -		msleep(10);
>  	}
> +
> +	/* Wait the sensors to be valid or the core will warn the user */
> +	armada_wait_sensor_validity(priv);
>  }
>  
>  static void armada_ap806_init_sensor(struct platform_device *pdev,
> @@ -185,7 +200,9 @@ static void armada_ap806_init_sensor(struct platform_device *pdev,
>  	reg &= ~CONTROL0_TSEN_RESET;
>  	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
>  	writel(reg, priv->control0);
> -	msleep(10);
> +
> +	/* Wait the sensors to be valid or the core will warn the user

Just out of curiosity but how the core will warn the user?

Gregory

> */
> +	armada_wait_sensor_validity(priv);
>  }
>  
>  static bool armada_is_valid(struct armada_thermal_priv *priv)
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 11/12] thermal: armada: Give meaningful names to the thermal zones

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> After registration to the thermal core, sysfs will make one entry
> per instance of the driver in /sys/class/thermal_zoneX and
> /sys/class/hwmon/hwmonX, X being the index of the instance, all of them
> having the type/name "armada_thermal".
>
> Until now there was only one thermal zone per SoC but SoCs like Armada
> A7K and Armada A8K have respectively two and three thermal zones (one
> per AP and one per CP) and this number is subject to grow in the future.
>
> Use dev_name() instead of the "armada_thermal" string to get a
> meaningful name and be able to identify the thermal zones from
> userspace.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory


> ---
>  drivers/thermal/armada_thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 4a5164ddffe7..42ef80b3b5f8 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -403,8 +403,8 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  
>  	priv->data->init_sensor(pdev, priv);
>  
> -	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
> -					       priv, &ops, NULL, 0, 0);
> +	thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv,
> +					       &ops, NULL, 0, 0);
>  	if (IS_ERR(thermal)) {
>  		dev_err(&pdev->dev,
>  			"Failed to register thermal zone device\n");
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 11/12] thermal: armada: Give meaningful names to the thermal zones

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> After registration to the thermal core, sysfs will make one entry
> per instance of the driver in /sys/class/thermal_zoneX and
> /sys/class/hwmon/hwmonX, X being the index of the instance, all of them
> having the type/name "armada_thermal".
>
> Until now there was only one thermal zone per SoC but SoCs like Armada
> A7K and Armada A8K have respectively two and three thermal zones (one
> per AP and one per CP) and this number is subject to grow in the future.
>
> Use dev_name() instead of the "armada_thermal" string to get a
> meaningful name and be able to identify the thermal zones from
> userspace.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Gregory


> ---
>  drivers/thermal/armada_thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 4a5164ddffe7..42ef80b3b5f8 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -403,8 +403,8 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  
>  	priv->data->init_sensor(pdev, priv);
>  
> -	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
> -					       priv, &ops, NULL, 0, 0);
> +	thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv,
> +					       &ops, NULL, 0, 0);
>  	if (IS_ERR(thermal)) {
>  		dev_err(&pdev->dev,
>  			"Failed to register thermal zone device\n");
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 12/12] ARM64: dts: marvell: Add thermal support for A7K/A8K

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., déc. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Add thermal DT nodes in AP806 and CP110 master/slave DTSI files.
>
> Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Applied on mvebu/dt64 (but not yet in linux-next)

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 6 ++++++
>  arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 ++++++
>  arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 6 ++++++
>  3 files changed, 18 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> index 1c4dd8ab9ad5..bbc5a4d3acac 100644
> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> @@ -285,6 +285,12 @@
>  					gpio-ranges = <&ap_pinctrl 0 0 20>;
>  				};
>  			};
> +
> +			ap_thermal: thermal@6f808C {
> +				compatible = "marvell,armada-ap806-thermal";
> +				reg = <0x6f808C 0x4>,
> +				      <0x6f8084 0x8>;
> +			};
>  		};
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> index e3b64d03fbd8..ecbc76d26dff 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> @@ -182,6 +182,12 @@
>  				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
>  			};
>  
> +			cpm_thermal: thermal@400078 {
> +				compatible = "marvell,armada-cp110-thermal";
> +				reg = <0x400078 0x4>,
> +				      <0x400070 0x8>;
> +			};
> +
>  			cpm_syscon0: system-controller@440000 {
>  				compatible = "syscon", "simple-mfd";
>  				reg = <0x440000 0x2000>;
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
> index 0d51096c69f8..29ba32c68870 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
> @@ -182,6 +182,12 @@
>  				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
>  			};
>  
> +			cps_thermal: thermal@400078 {
> +				compatible = "marvell,armada-cp110-thermal";
> +				reg = <0x400078 0x4>,
> +				      <0x400070 0x8>;
> +			};
> +
>  			cps_syscon0: system-controller@440000 {
>  				compatible = "syscon", "simple-mfd";
>  				reg = <0x440000 0x2000>;
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 12/12] ARM64: dts: marvell: Add thermal support for A7K/A8K

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., d?c. 18 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Add thermal DT nodes in AP806 and CP110 master/slave DTSI files.
>
> Suggested-by: David Sniatkiwicz <davidsn@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Applied on mvebu/dt64 (but not yet in linux-next)

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 6 ++++++
>  arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 ++++++
>  arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 6 ++++++
>  3 files changed, 18 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> index 1c4dd8ab9ad5..bbc5a4d3acac 100644
> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> @@ -285,6 +285,12 @@
>  					gpio-ranges = <&ap_pinctrl 0 0 20>;
>  				};
>  			};
> +
> +			ap_thermal: thermal at 6f808C {
> +				compatible = "marvell,armada-ap806-thermal";
> +				reg = <0x6f808C 0x4>,
> +				      <0x6f8084 0x8>;
> +			};
>  		};
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> index e3b64d03fbd8..ecbc76d26dff 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> @@ -182,6 +182,12 @@
>  				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
>  			};
>  
> +			cpm_thermal: thermal at 400078 {
> +				compatible = "marvell,armada-cp110-thermal";
> +				reg = <0x400078 0x4>,
> +				      <0x400070 0x8>;
> +			};
> +
>  			cpm_syscon0: system-controller at 440000 {
>  				compatible = "syscon", "simple-mfd";
>  				reg = <0x440000 0x2000>;
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
> index 0d51096c69f8..29ba32c68870 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
> @@ -182,6 +182,12 @@
>  				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
>  			};
>  
> +			cps_thermal: thermal at 400078 {
> +				compatible = "marvell,armada-cp110-thermal";
> +				reg = <0x400078 0x4>,
> +				      <0x400070 0x8>;
> +			};
> +
>  			cps_syscon0: system-controller at 440000 {
>  				compatible = "syscon", "simple-mfd";
>  				reg = <0x440000 0x2000>;
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Baruch Siach]

Hi Miquèl,

On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:
> From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> 
> 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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: reword, additional details]
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../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>;
> +	};

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.52.368.4656, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Baruch Siach]

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 at d0018300 {
>  		compatible = "marvell,armada370-thermal";
> -                reg = <0xd0018300 0x4
> +		reg = <0xd0018300 0x4
>  		       0xd0018304 0x4>;
>  	};
> +
> +	ap_thermal: thermal at 6f8084 {
> +		compatible = "marvell,armada-ap806-thermal";
> +		reg = <0x6f808C 0x4>,
> +		      <0x6f8084 0x8>;
> +	};

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Baruch Siach]

Hi Miquèl,

On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:
> Bindings were incomplete for a long time by only exposing one of the two
> available control registers. To ease the migration to the full bindings
> (already in use for the Armada 375 SoC), rename the pointers for
> clarification. This way, it will only be needed to add another pointer
> to access the other control register when the time comes.
> 
> This avoids dangerous situations where the offset 0 of the control
> area can be either one register or the other depending on the bindings
> used. After this change, device trees of other SoCs could be migrated to
> the "full" bindings if they may benefit from features from the
> unaccessible register, without any change in the driver.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---

[...]

> +	/*
> +	 * Legacy DT bindings only described "control1" register (also referred
> +	 * as "control MSB" on old documentation). New bindings cover
> +	 * "control0/control LSB" and "control1/control MSB" registers within
> +	 * the same resource, which is then of size 8 instead of 4.
> +	 */
> +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> +		/* ->control0 unavailable in this configuration */
> +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> +	} else {
> +		priv->control0 = control + CONTROL0_OFFSET;
> +		priv->control1 = control + CONTROL1_OFFSET;
> +	}

The needs_control0 field that you mentioned in the cover page is missing here.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.52.368.4656, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Baruch Siach]

Hi Miqu?l,

On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:
> Bindings were incomplete for a long time by only exposing one of the two
> available control registers. To ease the migration to the full bindings
> (already in use for the Armada 375 SoC), rename the pointers for
> clarification. This way, it will only be needed to add another pointer
> to access the other control register when the time comes.
> 
> This avoids dangerous situations where the offset 0 of the control
> area can be either one register or the other depending on the bindings
> used. After this change, device trees of other SoCs could be migrated to
> the "full" bindings if they may benefit from features from the
> unaccessible register, without any change in the driver.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---

[...]

> +	/*
> +	 * Legacy DT bindings only described "control1" register (also referred
> +	 * as "control MSB" on old documentation). New bindings cover
> +	 * "control0/control LSB" and "control1/control MSB" registers within
> +	 * the same resource, which is then of size 8 instead of 4.
> +	 */
> +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> +		/* ->control0 unavailable in this configuration */
> +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> +	} else {
> +		priv->control0 = control + CONTROL0_OFFSET;
> +		priv->control1 = control + CONTROL1_OFFSET;
> +	}

The needs_control0 field that you mentioned in the cover page is missing here.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH v4 06/12] thermal: armada: Add support for Armada AP806

[Miquel RAYNAL]

Hello Gregory,

Thank you for reviewing the series.

On Mon, 18 Dec 2017 17:05:07 +0100
Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Hi Miquel,
>  
>  On lun., déc. 18 2017, Miquel Raynal
> <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> > From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> >
> > The AP806 component is integrated in the Armada 8K and 7K lines of
> > processors.
> >
> > The thermal sensor sample field on the status register is a signed
> > value. Extend armada_get_temp() and the driver structure to handle
> > signed values.
> >
> > Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: Changes when applying over the
> > previous patches, including the register names changes, also
> > switched the coefficients values to s64 instead of unsigned long to
> > deal with negative values and used do_div instead of the
> > traditionnal '/'] Signed-off-by: Miquel Raynal
> > <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>  
> 
> I am just a little concerned by the fac that the value pass though
> get_temp() is an int and now the intermediate calculation are in
> s64. But maybe I'm too picky for this,

I know, but I thought having a temporary s64 variable and casting it
when setting *temp would have been exactly the same so I decided to let
it that way, but if you have something in mind (maybe a check that a
cast into a smaller type is valid? or limiting the temperature to
some boundaries?) I can update.

Thanks,
Miquèl

> 
> Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> Gregory
> 
> 
> > ---
> >  drivers/thermal/armada_thermal.c | 80
> > ++++++++++++++++++++++++++++++++-------- 1 file changed, 65
> > insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 198485fa77f2..ec29ea76b818
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -47,6 +47,11 @@
> >  #define CONTROL0_OFFSET			0x0
> >  #define CONTROL1_OFFSET			0x4
> >  
> > +/* TSEN refers to the temperature sensors within the AP */
> > +#define CONTROL0_TSEN_START		BIT(0)
> > +#define CONTROL0_TSEN_RESET		BIT(1)
> > +#define CONTROL0_TSEN_ENABLE		BIT(2)
> > +
> >  struct armada_thermal_data;
> >  
> >  /* Marvell EBU Thermal Sensor Dev Structure */
> > @@ -66,15 +71,17 @@ struct armada_thermal_data {
> >  	bool (*is_valid)(struct armada_thermal_priv *);
> >  
> >  	/* Formula coeficients: temp = (b - m * reg) / div */
> > -	unsigned long coef_b;
> > -	unsigned long coef_m;
> > -	unsigned long coef_div;
> > +	s64 coef_b;
> > +	s64 coef_m;
> > +	u32 coef_div;
> >  	bool inverted;
> > +	bool signed_sample;
> >  
> >  	/* Register shift and mask to access the sensor
> > temperature */ unsigned int temp_shift;
> >  	unsigned int temp_mask;
> >  	u32 is_valid_bit;
> > +	bool needs_control0;
> >  };
> >  
> >  static void armadaxp_init_sensor(struct platform_device *pdev,
> > @@ -154,6 +161,18 @@ static void armada380_init_sensor(struct
> > platform_device *pdev, }
> >  }
> >  
> > +static void armada_ap806_init_sensor(struct platform_device *pdev,
> > +				     struct armada_thermal_priv
> > *priv) +{
> > +	u32 reg;
> > +
> > +	reg = readl_relaxed(priv->control0);
> > +	reg &= ~CONTROL0_TSEN_RESET;
> > +	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
> > +	writel(reg, priv->control0);
> > +	msleep(10);
> > +}
> > +
> >  static bool armada_is_valid(struct armada_thermal_priv *priv)
> >  {
> >  	u32 reg = readl_relaxed(priv->status);
> > @@ -165,8 +184,8 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, int *temp)
> >  {
> >  	struct armada_thermal_priv *priv = thermal->devdata;
> > -	unsigned long reg;
> > -	unsigned long m, b, div;
> > +	u32 reg, div;
> > +	s64 sample, b, m;
> >  
> >  	/* Valid check */
> >  	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
> > @@ -177,6 +196,11 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, 
> >  	reg = readl_relaxed(priv->status);
> >  	reg = (reg >> priv->data->temp_shift) &
> > priv->data->temp_mask;
> > +	if (priv->data->signed_sample)
> > +		/* The most significant bit is the sign bit */
> > +		sample = sign_extend32(reg,
> > fls(priv->data->temp_mask) - 1);
> > +	else
> > +		sample = reg;
> >  
> >  	/* Get formula coeficients */
> >  	b = priv->data->coef_b;
> > @@ -184,9 +208,12 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, div = priv->data->coef_div;
> >  
> >  	if (priv->data->inverted)
> > -		*temp = ((m * reg) - b) / div;
> > +		*temp = (m * sample) - b;
> >  	else
> > -		*temp = (b - (m * reg)) / div;
> > +		*temp = b - (m * sample);
> > +
> > +	do_div(*temp, div);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -198,8 +225,8 @@ static const struct armada_thermal_data
> > armadaxp_data = { .init_sensor = armadaxp_init_sensor,
> >  	.temp_shift = 10,
> >  	.temp_mask = 0x1ff,
> > -	.coef_b = 3153000000UL,
> > -	.coef_m = 10000000UL,
> > +	.coef_b = 3153000000ULL,
> > +	.coef_m = 10000000ULL,
> >  	.coef_div = 13825,
> >  };
> >  
> > @@ -209,8 +236,8 @@ static const struct armada_thermal_data
> > armada370_data = { .is_valid_bit = BIT(9),
> >  	.temp_shift = 10,
> >  	.temp_mask = 0x1ff,
> > -	.coef_b = 3153000000UL,
> > -	.coef_m = 10000000UL,
> > +	.coef_b = 3153000000ULL,
> > +	.coef_m = 10000000ULL,
> >  	.coef_div = 13825,
> >  };
> >  
> > @@ -220,8 +247,8 @@ static const struct armada_thermal_data
> > armada375_data = { .is_valid_bit = BIT(10),
> >  	.temp_shift = 0,
> >  	.temp_mask = 0x1ff,
> > -	.coef_b = 3171900000UL,
> > -	.coef_m = 10000000UL,
> > +	.coef_b = 3171900000ULL,
> > +	.coef_m = 10000000ULL,
> >  	.coef_div = 13616,
> >  };
> >  
> > @@ -231,12 +258,26 @@ static const struct armada_thermal_data
> > armada380_data = { .is_valid_bit = BIT(10),
> >  	.temp_shift = 0,
> >  	.temp_mask = 0x3ff,
> > -	.coef_b = 1172499100UL,
> > -	.coef_m = 2000096UL,
> > +	.coef_b = 1172499100ULL,
> > +	.coef_m = 2000096ULL,
> >  	.coef_div = 4201,
> >  	.inverted = true,
> >  };
> >  
> > +static const struct armada_thermal_data armada_ap806_data = {
> > +	.is_valid = armada_is_valid,
> > +	.init_sensor = armada_ap806_init_sensor,
> > +	.is_valid_bit = BIT(16),
> > +	.temp_shift = 0,
> > +	.temp_mask = 0x3ff,
> > +	.coef_b = -150000LL,
> > +	.coef_m = 423ULL,
> > +	.coef_div = 1,
> > +	.inverted = true,
> > +	.signed_sample = true,
> > +	.needs_control0 = true,
> > +};
> > +
> >  static const struct of_device_id armada_thermal_id_table[] = {
> >  	{
> >  		.compatible = "marvell,armadaxp-thermal",
> > @@ -255,6 +296,10 @@ static const struct of_device_id
> > armada_thermal_id_table[] = { .data       = &armada380_data,
> >  	},
> >  	{
> > +		.compatible = "marvell,armada-ap806-thermal",
> > +		.data       = &armada_ap806_data,
> > +	},
> > +	{
> >  		/* sentinel */
> >  	},
> >  };
> > @@ -296,6 +341,11 @@ static int armada_thermal_probe(struct
> > platform_device *pdev) */
> >  	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> >  		/* ->control0 unavailable in this configuration */
> > +		if (priv->data->needs_control0) {
> > +			dev_err(&pdev->dev, "No access to control0
> > register\n");
> > +			return -EINVAL;
> > +		}
> > +
> >  		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> >  	} else {
> >  		priv->control0 = control + CONTROL0_OFFSET;
> > -- 
> > 2.11.0
> >  
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 06/12] thermal: armada: Add support for Armada AP806

[Miquel RAYNAL]

Hello Gregory,

Thank you for reviewing the series.

On Mon, 18 Dec 2017 17:05:07 +0100
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Miquel,
>  
>  On lun., d?c. 18 2017, Miquel Raynal
> <miquel.raynal@free-electrons.com> wrote:
> 
> > From: Baruch Siach <baruch@tkos.co.il>
> >
> > The AP806 component is integrated in the Armada 8K and 7K lines of
> > processors.
> >
> > The thermal sensor sample field on the status register is a signed
> > value. Extend armada_get_temp() and the driver structure to handle
> > signed values.
> >
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > [<miquel.raynal@free-electrons.com>: Changes when applying over the
> > previous patches, including the register names changes, also
> > switched the coefficients values to s64 instead of unsigned long to
> > deal with negative values and used do_div instead of the
> > traditionnal '/'] Signed-off-by: Miquel Raynal
> > <miquel.raynal@free-electrons.com>  
> 
> I am just a little concerned by the fac that the value pass though
> get_temp() is an int and now the intermediate calculation are in
> s64. But maybe I'm too picky for this,

I know, but I thought having a temporary s64 variable and casting it
when setting *temp would have been exactly the same so I decided to let
it that way, but if you have something in mind (maybe a check that a
cast into a smaller type is valid? or limiting the temperature to
some boundaries?) I can update.

Thanks,
Miqu?l

> 
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> Gregory
> 
> 
> > ---
> >  drivers/thermal/armada_thermal.c | 80
> > ++++++++++++++++++++++++++++++++-------- 1 file changed, 65
> > insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 198485fa77f2..ec29ea76b818
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -47,6 +47,11 @@
> >  #define CONTROL0_OFFSET			0x0
> >  #define CONTROL1_OFFSET			0x4
> >  
> > +/* TSEN refers to the temperature sensors within the AP */
> > +#define CONTROL0_TSEN_START		BIT(0)
> > +#define CONTROL0_TSEN_RESET		BIT(1)
> > +#define CONTROL0_TSEN_ENABLE		BIT(2)
> > +
> >  struct armada_thermal_data;
> >  
> >  /* Marvell EBU Thermal Sensor Dev Structure */
> > @@ -66,15 +71,17 @@ struct armada_thermal_data {
> >  	bool (*is_valid)(struct armada_thermal_priv *);
> >  
> >  	/* Formula coeficients: temp = (b - m * reg) / div */
> > -	unsigned long coef_b;
> > -	unsigned long coef_m;
> > -	unsigned long coef_div;
> > +	s64 coef_b;
> > +	s64 coef_m;
> > +	u32 coef_div;
> >  	bool inverted;
> > +	bool signed_sample;
> >  
> >  	/* Register shift and mask to access the sensor
> > temperature */ unsigned int temp_shift;
> >  	unsigned int temp_mask;
> >  	u32 is_valid_bit;
> > +	bool needs_control0;
> >  };
> >  
> >  static void armadaxp_init_sensor(struct platform_device *pdev,
> > @@ -154,6 +161,18 @@ static void armada380_init_sensor(struct
> > platform_device *pdev, }
> >  }
> >  
> > +static void armada_ap806_init_sensor(struct platform_device *pdev,
> > +				     struct armada_thermal_priv
> > *priv) +{
> > +	u32 reg;
> > +
> > +	reg = readl_relaxed(priv->control0);
> > +	reg &= ~CONTROL0_TSEN_RESET;
> > +	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
> > +	writel(reg, priv->control0);
> > +	msleep(10);
> > +}
> > +
> >  static bool armada_is_valid(struct armada_thermal_priv *priv)
> >  {
> >  	u32 reg = readl_relaxed(priv->status);
> > @@ -165,8 +184,8 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, int *temp)
> >  {
> >  	struct armada_thermal_priv *priv = thermal->devdata;
> > -	unsigned long reg;
> > -	unsigned long m, b, div;
> > +	u32 reg, div;
> > +	s64 sample, b, m;
> >  
> >  	/* Valid check */
> >  	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
> > @@ -177,6 +196,11 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, 
> >  	reg = readl_relaxed(priv->status);
> >  	reg = (reg >> priv->data->temp_shift) &
> > priv->data->temp_mask;
> > +	if (priv->data->signed_sample)
> > +		/* The most significant bit is the sign bit */
> > +		sample = sign_extend32(reg,
> > fls(priv->data->temp_mask) - 1);
> > +	else
> > +		sample = reg;
> >  
> >  	/* Get formula coeficients */
> >  	b = priv->data->coef_b;
> > @@ -184,9 +208,12 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, div = priv->data->coef_div;
> >  
> >  	if (priv->data->inverted)
> > -		*temp = ((m * reg) - b) / div;
> > +		*temp = (m * sample) - b;
> >  	else
> > -		*temp = (b - (m * reg)) / div;
> > +		*temp = b - (m * sample);
> > +
> > +	do_div(*temp, div);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -198,8 +225,8 @@ static const struct armada_thermal_data
> > armadaxp_data = { .init_sensor = armadaxp_init_sensor,
> >  	.temp_shift = 10,
> >  	.temp_mask = 0x1ff,
> > -	.coef_b = 3153000000UL,
> > -	.coef_m = 10000000UL,
> > +	.coef_b = 3153000000ULL,
> > +	.coef_m = 10000000ULL,
> >  	.coef_div = 13825,
> >  };
> >  
> > @@ -209,8 +236,8 @@ static const struct armada_thermal_data
> > armada370_data = { .is_valid_bit = BIT(9),
> >  	.temp_shift = 10,
> >  	.temp_mask = 0x1ff,
> > -	.coef_b = 3153000000UL,
> > -	.coef_m = 10000000UL,
> > +	.coef_b = 3153000000ULL,
> > +	.coef_m = 10000000ULL,
> >  	.coef_div = 13825,
> >  };
> >  
> > @@ -220,8 +247,8 @@ static const struct armada_thermal_data
> > armada375_data = { .is_valid_bit = BIT(10),
> >  	.temp_shift = 0,
> >  	.temp_mask = 0x1ff,
> > -	.coef_b = 3171900000UL,
> > -	.coef_m = 10000000UL,
> > +	.coef_b = 3171900000ULL,
> > +	.coef_m = 10000000ULL,
> >  	.coef_div = 13616,
> >  };
> >  
> > @@ -231,12 +258,26 @@ static const struct armada_thermal_data
> > armada380_data = { .is_valid_bit = BIT(10),
> >  	.temp_shift = 0,
> >  	.temp_mask = 0x3ff,
> > -	.coef_b = 1172499100UL,
> > -	.coef_m = 2000096UL,
> > +	.coef_b = 1172499100ULL,
> > +	.coef_m = 2000096ULL,
> >  	.coef_div = 4201,
> >  	.inverted = true,
> >  };
> >  
> > +static const struct armada_thermal_data armada_ap806_data = {
> > +	.is_valid = armada_is_valid,
> > +	.init_sensor = armada_ap806_init_sensor,
> > +	.is_valid_bit = BIT(16),
> > +	.temp_shift = 0,
> > +	.temp_mask = 0x3ff,
> > +	.coef_b = -150000LL,
> > +	.coef_m = 423ULL,
> > +	.coef_div = 1,
> > +	.inverted = true,
> > +	.signed_sample = true,
> > +	.needs_control0 = true,
> > +};
> > +
> >  static const struct of_device_id armada_thermal_id_table[] = {
> >  	{
> >  		.compatible = "marvell,armadaxp-thermal",
> > @@ -255,6 +296,10 @@ static const struct of_device_id
> > armada_thermal_id_table[] = { .data       = &armada380_data,
> >  	},
> >  	{
> > +		.compatible = "marvell,armada-ap806-thermal",
> > +		.data       = &armada_ap806_data,
> > +	},
> > +	{
> >  		/* sentinel */
> >  	},
> >  };
> > @@ -296,6 +341,11 @@ static int armada_thermal_probe(struct
> > platform_device *pdev) */
> >  	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> >  		/* ->control0 unavailable in this configuration */
> > +		if (priv->data->needs_control0) {
> > +			dev_err(&pdev->dev, "No access to control0
> > register\n");
> > +			return -EINVAL;
> > +		}
> > +
> >  		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> >  	} else {
> >  		priv->control0 = control + CONTROL0_OFFSET;
> > -- 
> > 2.11.0
> >  
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel RAYNAL]

Hello Baruch,

On Mon, 18 Dec 2017 22:35:42 +0200
Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

> Hi Miquèl,
> 
> On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:
> > Bindings were incomplete for a long time by only exposing one of
> > the two available control registers. To ease the migration to the
> > full bindings (already in use for the Armada 375 SoC), rename the
> > pointers for clarification. This way, it will only be needed to add
> > another pointer to access the other control register when the time
> > comes.
> > 
> > This avoids dangerous situations where the offset 0 of the control
> > area can be either one register or the other depending on the
> > bindings used. After this change, device trees of other SoCs could
> > be migrated to the "full" bindings if they may benefit from
> > features from the unaccessible register, without any change in the
> > driver.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---  
> 
> [...]
> 
> > +	/*
> > +	 * Legacy DT bindings only described "control1" register
> > (also referred
> > +	 * as "control MSB" on old documentation). New bindings
> > cover
> > +	 * "control0/control LSB" and "control1/control MSB"
> > registers within
> > +	 * the same resource, which is then of size 8 instead of 4.
> > +	 */
> > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > +		/* ->control0 unavailable in this configuration */
> > +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> > +	} else {
> > +		priv->control0 = control + CONTROL0_OFFSET;
> > +		priv->control1 = control + CONTROL1_OFFSET;
> > +	}  
> 
> The needs_control0 field that you mentioned in the cover page is
> missing here.

Yes, at this point nobody actually *needs* control0 so the limitation
is added with the patch that introduce ap806 support as it is the first
compatible that needs both control0 and control1 to work correctly.
Does this bother you?

Thanks,
Miquèl

> 
> baruch
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel RAYNAL]

Hello Baruch,

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

> Hi Miqu?l,
> 
> On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:
> > Bindings were incomplete for a long time by only exposing one of
> > the two available control registers. To ease the migration to the
> > full bindings (already in use for the Armada 375 SoC), rename the
> > pointers for clarification. This way, it will only be needed to add
> > another pointer to access the other control register when the time
> > comes.
> > 
> > This avoids dangerous situations where the offset 0 of the control
> > area can be either one register or the other depending on the
> > bindings used. After this change, device trees of other SoCs could
> > be migrated to the "full" bindings if they may benefit from
> > features from the unaccessible register, without any change in the
> > driver.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---  
> 
> [...]
> 
> > +	/*
> > +	 * Legacy DT bindings only described "control1" register
> > (also referred
> > +	 * as "control MSB" on old documentation). New bindings
> > cover
> > +	 * "control0/control LSB" and "control1/control MSB"
> > registers within
> > +	 * the same resource, which is then of size 8 instead of 4.
> > +	 */
> > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > +		/* ->control0 unavailable in this configuration */
> > +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> > +	} else {
> > +		priv->control0 = control + CONTROL0_OFFSET;
> > +		priv->control1 = control + CONTROL1_OFFSET;
> > +	}  
> 
> The needs_control0 field that you mentioned in the cover page is
> missing here.

Yes, at this point nobody actually *needs* control0 so the limitation
is added with the patch that introduce ap806 support as it is the first
compatible that needs both control0 and control1 to work correctly.
Does this bother you?

Thanks,
Miqu?l

> 
> baruch
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Miquel RAYNAL]

Hello Baruch,

On Mon, 18 Dec 2017 22:33:24 +0200
Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

> Hi Miquèl,
> 
> On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:
> > From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > 
> > 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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: reword, additional details]
> > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../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>;
> > +	};  
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Miquel RAYNAL]

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 at d0018300 {
> >  		compatible = "marvell,armada370-thermal";
> > -                reg = <0xd0018300 0x4
> > +		reg = <0xd0018300 0x4
> >  		       0xd0018304 0x4>;
> >  	};
> > +
> > +	ap_thermal: thermal at 6f8084 {
> > +		compatible = "marvell,armada-ap806-thermal";
> > +		reg = <0x6f808C 0x4>,
> > +		      <0x6f8084 0x8>;
> > +	};  
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Baruch Siach]

Hi Miquèl,

On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:
> On Mon, 18 Dec 2017 22:35:42 +0200
> Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:
> > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:
> > > Bindings were incomplete for a long time by only exposing one of
> > > the two available control registers. To ease the migration to the
> > > full bindings (already in use for the Armada 375 SoC), rename the
> > > pointers for clarification. This way, it will only be needed to add
> > > another pointer to access the other control register when the time
> > > comes.
> > > 
> > > This avoids dangerous situations where the offset 0 of the control
> > > area can be either one register or the other depending on the
> > > bindings used. After this change, device trees of other SoCs could
> > > be migrated to the "full" bindings if they may benefit from
> > > features from the unaccessible register, without any change in the
> > > driver.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---  
> > 
> > [...]
> > 
> > > +	/*
> > > +	 * Legacy DT bindings only described "control1" register
> > > (also referred
> > > +	 * as "control MSB" on old documentation). New bindings
> > > cover
> > > +	 * "control0/control LSB" and "control1/control MSB"
> > > registers within
> > > +	 * the same resource, which is then of size 8 instead of 4.
> > > +	 */
> > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > +		/* ->control0 unavailable in this configuration */
> > > +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> > > +	} else {
> > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > +	}  
> > 
> > The needs_control0 field that you mentioned in the cover page is
> > missing here.
> 
> Yes, at this point nobody actually *needs* control0 so the limitation
> is added with the patch that introduce ap806 support as it is the first
> compatible that needs both control0 and control1 to work correctly.
> Does this bother you?

No. It is just that we agreed to have a verification here that the size of the 
control registers resource matches the binding. I thought that the 
needs_control0 field that you mention in the cover page is meant to implement 
that. But I'm not sure all that is strictly necessary. It would just make sure 
that no one introduces a DT with the wrong resource size.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Baruch Siach]

Hi Miqu?l,

On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:
> On Mon, 18 Dec 2017 22:35:42 +0200
> Baruch Siach <baruch@tkos.co.il> wrote:
> > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:
> > > Bindings were incomplete for a long time by only exposing one of
> > > the two available control registers. To ease the migration to the
> > > full bindings (already in use for the Armada 375 SoC), rename the
> > > pointers for clarification. This way, it will only be needed to add
> > > another pointer to access the other control register when the time
> > > comes.
> > > 
> > > This avoids dangerous situations where the offset 0 of the control
> > > area can be either one register or the other depending on the
> > > bindings used. After this change, device trees of other SoCs could
> > > be migrated to the "full" bindings if they may benefit from
> > > features from the unaccessible register, without any change in the
> > > driver.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > > ---  
> > 
> > [...]
> > 
> > > +	/*
> > > +	 * Legacy DT bindings only described "control1" register
> > > (also referred
> > > +	 * as "control MSB" on old documentation). New bindings
> > > cover
> > > +	 * "control0/control LSB" and "control1/control MSB"
> > > registers within
> > > +	 * the same resource, which is then of size 8 instead of 4.
> > > +	 */
> > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > +		/* ->control0 unavailable in this configuration */
> > > +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> > > +	} else {
> > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > +	}  
> > 
> > The needs_control0 field that you mentioned in the cover page is
> > missing here.
> 
> Yes, at this point nobody actually *needs* control0 so the limitation
> is added with the patch that introduce ap806 support as it is the first
> compatible that needs both control0 and control1 to work correctly.
> Does this bother you?

No. It is just that we agreed to have a verification here that the size of the 
control registers resource matches the binding. I thought that the 
needs_control0 field that you mention in the cover page is meant to implement 
that. But I'm not sure all that is strictly necessary. It would just make sure 
that no one introduces a DT with the wrong resource size.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

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

[Baruch Siach]

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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:
> > On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:
> > > From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > > 
> > > 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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > > [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: reword, additional details]
> > > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > >  .../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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

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

[Miquel RAYNAL]

Hello Baruch,

On Tue, 19 Dec 2017 08:09:18 +0200
Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> 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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:  
> > > On Mon, Dec 18, 2017 at 03:36:32PM +0100, Miquel Raynal wrote:  
> > > > From: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > > > 
> > > > 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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > > > [<miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: reword, additional details]
> > > > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > > ---
> > > >  .../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
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Miquel RAYNAL]

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
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel RAYNAL]

On Tue, 19 Dec 2017 07:51:54 +0200
Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

> Hi Miquèl,
> 
> On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:
> > On Mon, 18 Dec 2017 22:35:42 +0200
> > Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:  
> > > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:  
> > > > Bindings were incomplete for a long time by only exposing one of
> > > > the two available control registers. To ease the migration to
> > > > the full bindings (already in use for the Armada 375 SoC),
> > > > rename the pointers for clarification. This way, it will only
> > > > be needed to add another pointer to access the other control
> > > > register when the time comes.
> > > > 
> > > > This avoids dangerous situations where the offset 0 of the
> > > > control area can be either one register or the other depending
> > > > on the bindings used. After this change, device trees of other
> > > > SoCs could be migrated to the "full" bindings if they may
> > > > benefit from features from the unaccessible register, without
> > > > any change in the driver.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > > Reviewed-by: Gregory CLEMENT
> > > > <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> ---    
> > > 
> > > [...]
> > >   
> > > > +	/*
> > > > +	 * Legacy DT bindings only described "control1"
> > > > register (also referred
> > > > +	 * as "control MSB" on old documentation). New bindings
> > > > cover
> > > > +	 * "control0/control LSB" and "control1/control MSB"
> > > > registers within
> > > > +	 * the same resource, which is then of size 8 instead
> > > > of 4.
> > > > +	 */
> > > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > > +		/* ->control0 unavailable in this
> > > > configuration */
> > > > +		priv->control1 = control +
> > > > LEGACY_CONTROL1_OFFSET;
> > > > +	} else {
> > > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > > +	}    
> > > 
> > > The needs_control0 field that you mentioned in the cover page is
> > > missing here.  
> > 
> > Yes, at this point nobody actually *needs* control0 so the
> > limitation is added with the patch that introduce ap806 support as
> > it is the first compatible that needs both control0 and control1 to
> > work correctly. Does this bother you?  
> 
> No. It is just that we agreed to have a verification here that the
> size of the control registers resource matches the binding. I thought
> that the needs_control0 field that you mention in the cover page is
> meant to implement that.

That is absolutely right, but at this point in the series, the supported
compatible strings are "marvell,armada[370|375|38x|xp]-thermal". All of
them can use both bindings so I don't see the point to have a
needs_control0 field in this patch. It is introduced in the next patch
that adds support for ap806 by only supporting the new bindings
though.

> necessary. It would just make sure that no one introduces a DT with
> the wrong resource size.

Not sure I understand what exactly you wanna check, can you
give me an example?

Thank you,
Miquèl

> 
> baruch
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel RAYNAL]

On Tue, 19 Dec 2017 07:51:54 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Miqu?l,
> 
> On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:
> > On Mon, 18 Dec 2017 22:35:42 +0200
> > Baruch Siach <baruch@tkos.co.il> wrote:  
> > > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:  
> > > > Bindings were incomplete for a long time by only exposing one of
> > > > the two available control registers. To ease the migration to
> > > > the full bindings (already in use for the Armada 375 SoC),
> > > > rename the pointers for clarification. This way, it will only
> > > > be needed to add another pointer to access the other control
> > > > register when the time comes.
> > > > 
> > > > This avoids dangerous situations where the offset 0 of the
> > > > control area can be either one register or the other depending
> > > > on the bindings used. After this change, device trees of other
> > > > SoCs could be migrated to the "full" bindings if they may
> > > > benefit from features from the unaccessible register, without
> > > > any change in the driver.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > > > Reviewed-by: Gregory CLEMENT
> > > > <gregory.clement@free-electrons.com> ---    
> > > 
> > > [...]
> > >   
> > > > +	/*
> > > > +	 * Legacy DT bindings only described "control1"
> > > > register (also referred
> > > > +	 * as "control MSB" on old documentation). New bindings
> > > > cover
> > > > +	 * "control0/control LSB" and "control1/control MSB"
> > > > registers within
> > > > +	 * the same resource, which is then of size 8 instead
> > > > of 4.
> > > > +	 */
> > > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > > +		/* ->control0 unavailable in this
> > > > configuration */
> > > > +		priv->control1 = control +
> > > > LEGACY_CONTROL1_OFFSET;
> > > > +	} else {
> > > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > > +	}    
> > > 
> > > The needs_control0 field that you mentioned in the cover page is
> > > missing here.  
> > 
> > Yes, at this point nobody actually *needs* control0 so the
> > limitation is added with the patch that introduce ap806 support as
> > it is the first compatible that needs both control0 and control1 to
> > work correctly. Does this bother you?  
> 
> No. It is just that we agreed to have a verification here that the
> size of the control registers resource matches the binding. I thought
> that the needs_control0 field that you mention in the cover page is
> meant to implement that.

That is absolutely right, but at this point in the series, the supported
compatible strings are "marvell,armada[370|375|38x|xp]-thermal". All of
them can use both bindings so I don't see the point to have a
needs_control0 field in this patch. It is introduced in the next patch
that adds support for ap806 by only supporting the new bindings
though.

> necessary. It would just make sure that no one introduces a DT with
> the wrong resource size.

Not sure I understand what exactly you wanna check, can you
give me an example?

Thank you,
Miqu?l

> 
> baruch
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Baruch Siach]

Hi Miquèl,

On Tue, Dec 19, 2017 at 09:08:14AM +0100, Miquel RAYNAL wrote:
> On Tue, 19 Dec 2017 07:51:54 +0200
> Baruch Siach <baruch@tkos.co.il> wrote:
> > On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:
> > > On Mon, 18 Dec 2017 22:35:42 +0200
> > > Baruch Siach <baruch@tkos.co.il> wrote:  
> > > > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:  
> > > > > Bindings were incomplete for a long time by only exposing one of
> > > > > the two available control registers. To ease the migration to
> > > > > the full bindings (already in use for the Armada 375 SoC),
> > > > > rename the pointers for clarification. This way, it will only
> > > > > be needed to add another pointer to access the other control
> > > > > register when the time comes.
> > > > > 
> > > > > This avoids dangerous situations where the offset 0 of the
> > > > > control area can be either one register or the other depending
> > > > > on the bindings used. After this change, device trees of other
> > > > > SoCs could be migrated to the "full" bindings if they may
> > > > > benefit from features from the unaccessible register, without
> > > > > any change in the driver.
> > > > > 
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > > > > Reviewed-by: Gregory CLEMENT
> > > > > <gregory.clement@free-electrons.com> ---    
> > > > 
> > > > [...]
> > > >   
> > > > > +	/*
> > > > > +	 * Legacy DT bindings only described "control1"
> > > > > register (also referred
> > > > > +	 * as "control MSB" on old documentation). New bindings
> > > > > cover
> > > > > +	 * "control0/control LSB" and "control1/control MSB"
> > > > > registers within
> > > > > +	 * the same resource, which is then of size 8 instead
> > > > > of 4.
> > > > > +	 */
> > > > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > > > +		/* ->control0 unavailable in this
> > > > > configuration */
> > > > > +		priv->control1 = control +
> > > > > LEGACY_CONTROL1_OFFSET;
> > > > > +	} else {
> > > > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > > > +	}    
> > > > 
> > > > The needs_control0 field that you mentioned in the cover page is
> > > > missing here.  
> > > 
> > > Yes, at this point nobody actually *needs* control0 so the
> > > limitation is added with the patch that introduce ap806 support as
> > > it is the first compatible that needs both control0 and control1 to
> > > work correctly. Does this bother you?  
> > 
> > No. It is just that we agreed to have a verification here that the
> > size of the control registers resource matches the binding. I thought
> > that the needs_control0 field that you mention in the cover page is
> > meant to implement that.
> 
> That is absolutely right, but at this point in the series, the supported
> compatible strings are "marvell,armada[370|375|38x|xp]-thermal". All of
> them can use both bindings so I don't see the point to have a
> needs_control0 field in this patch. It is introduced in the next patch
> that adds support for ap806 by only supporting the new bindings
> though.

OK. Makes sense.

> > necessary. It would just make sure that no one introduces a DT with
> > the wrong resource size.
> 
> Not sure I understand what exactly you wanna check, can you
> give me an example?

I wrote that before it occurred to me that we can use the control registers 
size the distinguish between the old binding and the new one.

I still think it would be nice to add needs_control0=true to armada375_data, 
for consistency with the ap806 and cp110.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Baruch Siach]

Hi Miqu?l,

On Tue, Dec 19, 2017 at 09:08:14AM +0100, Miquel RAYNAL wrote:
> On Tue, 19 Dec 2017 07:51:54 +0200
> Baruch Siach <baruch@tkos.co.il> wrote:
> > On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:
> > > On Mon, 18 Dec 2017 22:35:42 +0200
> > > Baruch Siach <baruch@tkos.co.il> wrote:  
> > > > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal wrote:  
> > > > > Bindings were incomplete for a long time by only exposing one of
> > > > > the two available control registers. To ease the migration to
> > > > > the full bindings (already in use for the Armada 375 SoC),
> > > > > rename the pointers for clarification. This way, it will only
> > > > > be needed to add another pointer to access the other control
> > > > > register when the time comes.
> > > > > 
> > > > > This avoids dangerous situations where the offset 0 of the
> > > > > control area can be either one register or the other depending
> > > > > on the bindings used. After this change, device trees of other
> > > > > SoCs could be migrated to the "full" bindings if they may
> > > > > benefit from features from the unaccessible register, without
> > > > > any change in the driver.
> > > > > 
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > > > > Reviewed-by: Gregory CLEMENT
> > > > > <gregory.clement@free-electrons.com> ---    
> > > > 
> > > > [...]
> > > >   
> > > > > +	/*
> > > > > +	 * Legacy DT bindings only described "control1"
> > > > > register (also referred
> > > > > +	 * as "control MSB" on old documentation). New bindings
> > > > > cover
> > > > > +	 * "control0/control LSB" and "control1/control MSB"
> > > > > registers within
> > > > > +	 * the same resource, which is then of size 8 instead
> > > > > of 4.
> > > > > +	 */
> > > > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > > > +		/* ->control0 unavailable in this
> > > > > configuration */
> > > > > +		priv->control1 = control +
> > > > > LEGACY_CONTROL1_OFFSET;
> > > > > +	} else {
> > > > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > > > +	}    
> > > > 
> > > > The needs_control0 field that you mentioned in the cover page is
> > > > missing here.  
> > > 
> > > Yes, at this point nobody actually *needs* control0 so the
> > > limitation is added with the patch that introduce ap806 support as
> > > it is the first compatible that needs both control0 and control1 to
> > > work correctly. Does this bother you?  
> > 
> > No. It is just that we agreed to have a verification here that the
> > size of the control registers resource matches the binding. I thought
> > that the needs_control0 field that you mention in the cover page is
> > meant to implement that.
> 
> That is absolutely right, but at this point in the series, the supported
> compatible strings are "marvell,armada[370|375|38x|xp]-thermal". All of
> them can use both bindings so I don't see the point to have a
> needs_control0 field in this patch. It is introduced in the next patch
> that adds support for ap806 by only supporting the new bindings
> though.

OK. Makes sense.

> > necessary. It would just make sure that no one introduces a DT with
> > the wrong resource size.
> 
> Not sure I understand what exactly you wanna check, can you
> give me an example?

I wrote that before it occurred to me that we can use the control registers 
size the distinguish between the old binding and the new one.

I still think it would be nice to add needs_control0=true to armada375_data, 
for consistency with the ap806 and cp110.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel RAYNAL]

Hi Baruch,

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

> Hi Miquèl,
> 
> On Tue, Dec 19, 2017 at 09:08:14AM +0100, Miquel RAYNAL wrote:
> > On Tue, 19 Dec 2017 07:51:54 +0200
> > Baruch Siach <baruch@tkos.co.il> wrote:  
> > > On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:  
> > > > On Mon, 18 Dec 2017 22:35:42 +0200
> > > > Baruch Siach <baruch@tkos.co.il> wrote:    
> > > > > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal
> > > > > wrote:    
> > > > > > Bindings were incomplete for a long time by only exposing
> > > > > > one of the two available control registers. To ease the
> > > > > > migration to the full bindings (already in use for the
> > > > > > Armada 375 SoC), rename the pointers for clarification.
> > > > > > This way, it will only be needed to add another pointer to
> > > > > > access the other control register when the time comes.
> > > > > > 
> > > > > > This avoids dangerous situations where the offset 0 of the
> > > > > > control area can be either one register or the other
> > > > > > depending on the bindings used. After this change, device
> > > > > > trees of other SoCs could be migrated to the "full"
> > > > > > bindings if they may benefit from features from the
> > > > > > unaccessible register, without any change in the driver.
> > > > > > 
> > > > > > Signed-off-by: Miquel Raynal
> > > > > > <miquel.raynal@free-electrons.com> Reviewed-by: Gregory
> > > > > > CLEMENT <gregory.clement@free-electrons.com> ---      
> > > > > 
> > > > > [...]
> > > > >     
> > > > > > +	/*
> > > > > > +	 * Legacy DT bindings only described "control1"
> > > > > > register (also referred
> > > > > > +	 * as "control MSB" on old documentation). New
> > > > > > bindings cover
> > > > > > +	 * "control0/control LSB" and "control1/control
> > > > > > MSB" registers within
> > > > > > +	 * the same resource, which is then of size 8
> > > > > > instead of 4.
> > > > > > +	 */
> > > > > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > > > > +		/* ->control0 unavailable in this
> > > > > > configuration */
> > > > > > +		priv->control1 = control +
> > > > > > LEGACY_CONTROL1_OFFSET;
> > > > > > +	} else {
> > > > > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > > > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > > > > +	}      
> > > > > 
> > > > > The needs_control0 field that you mentioned in the cover page
> > > > > is missing here.    
> > > > 
> > > > Yes, at this point nobody actually *needs* control0 so the
> > > > limitation is added with the patch that introduce ap806 support
> > > > as it is the first compatible that needs both control0 and
> > > > control1 to work correctly. Does this bother you?    
> > > 
> > > No. It is just that we agreed to have a verification here that the
> > > size of the control registers resource matches the binding. I
> > > thought that the needs_control0 field that you mention in the
> > > cover page is meant to implement that.  
> > 
> > That is absolutely right, but at this point in the series, the
> > supported compatible strings are
> > "marvell,armada[370|375|38x|xp]-thermal". All of them can use both
> > bindings so I don't see the point to have a needs_control0 field in
> > this patch. It is introduced in the next patch that adds support
> > for ap806 by only supporting the new bindings though.  
> 
> OK. Makes sense.
> 
> > > necessary. It would just make sure that no one introduces a DT
> > > with the wrong resource size.  
> > 
> > Not sure I understand what exactly you wanna check, can you
> > give me an example?  
> 
> I wrote that before it occurred to me that we can use the control
> registers size the distinguish between the old binding and the new
> one.
> 
> I still think it would be nice to add needs_control0=true to
> armada375_data, for consistency with the ap806 and cp110.

Oh that is right, I forgot about that. I will add it and move the
need_control0 boolean to this patch.

Thank you,
Miquèl

> 
> baruch
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 04/12] thermal: armada: Clarify control registers accesses

[Miquel RAYNAL]

Hi Baruch,

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

> Hi Miqu?l,
> 
> On Tue, Dec 19, 2017 at 09:08:14AM +0100, Miquel RAYNAL wrote:
> > On Tue, 19 Dec 2017 07:51:54 +0200
> > Baruch Siach <baruch@tkos.co.il> wrote:  
> > > On Tue, Dec 19, 2017 at 01:32:33AM +0100, Miquel RAYNAL wrote:  
> > > > On Mon, 18 Dec 2017 22:35:42 +0200
> > > > Baruch Siach <baruch@tkos.co.il> wrote:    
> > > > > On Mon, Dec 18, 2017 at 03:36:35PM +0100, Miquel Raynal
> > > > > wrote:    
> > > > > > Bindings were incomplete for a long time by only exposing
> > > > > > one of the two available control registers. To ease the
> > > > > > migration to the full bindings (already in use for the
> > > > > > Armada 375 SoC), rename the pointers for clarification.
> > > > > > This way, it will only be needed to add another pointer to
> > > > > > access the other control register when the time comes.
> > > > > > 
> > > > > > This avoids dangerous situations where the offset 0 of the
> > > > > > control area can be either one register or the other
> > > > > > depending on the bindings used. After this change, device
> > > > > > trees of other SoCs could be migrated to the "full"
> > > > > > bindings if they may benefit from features from the
> > > > > > unaccessible register, without any change in the driver.
> > > > > > 
> > > > > > Signed-off-by: Miquel Raynal
> > > > > > <miquel.raynal@free-electrons.com> Reviewed-by: Gregory
> > > > > > CLEMENT <gregory.clement@free-electrons.com> ---      
> > > > > 
> > > > > [...]
> > > > >     
> > > > > > +	/*
> > > > > > +	 * Legacy DT bindings only described "control1"
> > > > > > register (also referred
> > > > > > +	 * as "control MSB" on old documentation). New
> > > > > > bindings cover
> > > > > > +	 * "control0/control LSB" and "control1/control
> > > > > > MSB" registers within
> > > > > > +	 * the same resource, which is then of size 8
> > > > > > instead of 4.
> > > > > > +	 */
> > > > > > +	if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > > > > > +		/* ->control0 unavailable in this
> > > > > > configuration */
> > > > > > +		priv->control1 = control +
> > > > > > LEGACY_CONTROL1_OFFSET;
> > > > > > +	} else {
> > > > > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > > > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > > > > +	}      
> > > > > 
> > > > > The needs_control0 field that you mentioned in the cover page
> > > > > is missing here.    
> > > > 
> > > > Yes, at this point nobody actually *needs* control0 so the
> > > > limitation is added with the patch that introduce ap806 support
> > > > as it is the first compatible that needs both control0 and
> > > > control1 to work correctly. Does this bother you?    
> > > 
> > > No. It is just that we agreed to have a verification here that the
> > > size of the control registers resource matches the binding. I
> > > thought that the needs_control0 field that you mention in the
> > > cover page is meant to implement that.  
> > 
> > That is absolutely right, but at this point in the series, the
> > supported compatible strings are
> > "marvell,armada[370|375|38x|xp]-thermal". All of them can use both
> > bindings so I don't see the point to have a needs_control0 field in
> > this patch. It is introduced in the next patch that adds support
> > for ap806 by only supporting the new bindings though.  
> 
> OK. Makes sense.
> 
> > > necessary. It would just make sure that no one introduces a DT
> > > with the wrong resource size.  
> > 
> > Not sure I understand what exactly you wanna check, can you
> > give me an example?  
> 
> I wrote that before it occurred to me that we can use the control
> registers size the distinguish between the old binding and the new
> one.
> 
> I still think it would be nice to add needs_control0=true to
> armada375_data, for consistency with the ap806 and cp110.

Oh that is right, I forgot about that. I will add it and move the
need_control0 boolean to this patch.

Thank you,
Miqu?l

> 
> baruch
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com