Armada thermal: improvements and A7K/A8K SoCs support

Hi Miquel

Quoted 4 lines · Miquel Raynal

+- marvell,thermal-zone-name: The name to identify the thermal zone

(quoted diff) +2 -0

+                             within the sysfs, useful when multiple
+                             thermal zones are registered (AP,

CPx...).

I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'.

Quoted 7 lines · On Fri, 15 Dec 2017 10:27:59 +0200 Baruch Siach <[email protected]> wrote:

Quoted 4 lines · Miquel Raynal

+- marvell,thermal-zone-name: The name to identify the thermal zone

(quoted diff) +2 -0

+                             within the sysfs, useful when multiple
+                             thermal zones are registered (AP,

CPx...).

Quoted 4 lines

I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'.

You are right the sysfs mention should not appear in the description.

Otherwise for the naming I'm not sure "linux," is a valid prefix in that case.

Hello Baruch,

Quoted 8 lines · On Fri, 15 Dec 2017 10:27:59 +0200 Baruch Siach <[email protected]> wrote:

Hi Miquel

Quoted 4 lines · Miquel Raynal

+- marvell,thermal-zone-name: The name to identify the thermal zone

(quoted diff) +2 -0

+                             within the sysfs, useful when multiple
+                             thermal zones are registered (AP,

CPx...).

Quoted 4 lines

I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'.

You are right the sysfs mention should not appear in the description.

Otherwise for the naming I'm not sure "linux," is a valid prefix in that case.

Miqu?l

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

Hi Miquel,

Quoted 22 lines · Miquel RAYNAL

Hello Baruch,

Quoted 10 lines · On Fri, 15 Dec 2017 10:27:59 +0200 Baruch Siach <[email protected]> wrote:

Hi Miquel

Quoted 6 lines · Miquel Raynal

+- marvell,thermal-zone-name: The name to identify the thermal zone
+ within the sysfs, useful when
multiple
+ thermal zones are registered (AP,
CPx...).

Quoted 4 lines

I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'.

You are right the sysfs mention should not appear in the description.

Quoted 3 lines

Otherwise for the naming I'm not sure "linux," is a valid prefix in that case.

Actually the choice between linux or marvell make me realize that there is something wrong. Having a name associated to a device is something pretty usual with the device tree, however it is as the class device level, such as clock-names, line-name, or regulator-name. So in my opinion if we want to support naming from device tree it would be done for all the thermal device not just for the Marvell one.

However I don't think we need it. For example for the clocks we created the name dynamically using of the base address of the register to keep them unique.

Hello Baruch and Gregory,

Quoted 27 lines · On Fri, 15 Dec 2017 09:44:19 +0100 Gregory CLEMENT…

Hi Miquel,

Quoted 22 lines · Miquel RAYNAL

Hello Baruch,

Quoted 10 lines · On Fri, 15 Dec 2017 10:27:59 +0200 Baruch Siach <[email protected]> wrote:

Hi Miquel

Quoted 6 lines · Miquel Raynal

+- marvell,thermal-zone-name: The name to identify the thermal zone
+ within the sysfs, useful when
multiple
+ thermal zones are registered (AP,
CPx...).

Quoted 4 lines

I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'.

You are right the sysfs mention should not appear in the description.

Actually, you are right for all of it, this property should not exist, sorry for my too quick answer.

Quoted 3 lines

Quoted 3 lines

Otherwise for the naming I'm not sure "linux," is a valid prefix in that case.

Thank you both for your explanations, I was also wrong about the prefix.

Quoted 9 lines

Actually the choice between linux or marvell make me realize that there is something wrong. Having a name associated to a device is something pretty usual with the device tree, however it is as the class device level, such as clock-names, line-name, or regulator-name. So in my opinion if we want to support naming from device tree it would be done for all the thermal device not just for the Marvell one.

Quoted 3 lines

However I don't think we need it. For example for the clocks we created the name dynamically using of the base address of the register to keep them unique.

I was convinced that dev_name's would be the same but after trying it on a 8040-DB, using dev_name(&pdev->dev) gives:

    f06f808c.thermal
    f2400078.thermal
    f4400078.thermal

which I found meaningful enough.

I will drop the property and use dev_name instead. I still need your help to solve one problem though: how to make the distinction between using "armada_thermal" (the previous name) and dev_name() ? If I don't it kind of breaks userspace, doesn't it ?

Quoted 63 lines · Miquel RAYNAL

Hello Baruch and Gregory,

Quoted 28 lines · On Fri, 15 Dec 2017 09:44:19 +0100 Gregory CLEMENT…

Hi Miquel,

Quoted 23 lines · Miquel RAYNAL

Hello Baruch,

Quoted 11 lines · On Fri, 15 Dec 2017 10:27:59 +0200 Baruch Siach <[email protected]> wrote:

Hi Miquel

Quoted 6 lines · Miquel Raynal

+- marvell,thermal-zone-name: The name to identify the thermal zone
+ within the sysfs, useful when
multiple
+ thermal zones are registered
(AP, CPx...).

Quoted 4 lines

I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'.

You are right the sysfs mention should not appear in the description.

Actually, you are right for all of it, this property should not exist, sorry for my too quick answer.

Quoted 3 lines

Quoted 3 lines

Otherwise for the naming I'm not sure "linux," is a valid prefix in that case.

Thank you both for your explanations, I was also wrong about the prefix.

Quoted 9 lines

Actually the choice between linux or marvell make me realize that there is something wrong. Having a name associated to a device is something pretty usual with the device tree, however it is as the class device level, such as clock-names, line-name, or regulator-name. So in my opinion if we want to support naming from device tree it would be done for all the thermal device not just for the Marvell one.

Quoted 3 lines

However I don't think we need it. For example for the clocks we created the name dynamically using of the base address of the register to keep them unique.

I was convinced that dev_name's would be the same but after trying it on a 8040-DB, using dev_name(&pdev->dev) gives:

    f06f808c.thermal
    f2400078.thermal
    f4400078.thermal

Quoted 6 lines

which I found meaningful enough.

I will drop the property and use dev_name instead. I still need your help to solve one problem though: how to make the distinction between using "armada_thermal" (the previous name) and dev_name() ? If I don't it kind of breaks userspace, doesn't it ?

No. The /sys/devices/... or /sys/bus/platform/... paths and names are not guaranteed to be stable. These changed for every platform converted to DT for example. Userspace should be accessing things through /sys/class/... (or deal with changes).

From: Baruch Siach <[email protected]>

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 <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/thermal/armada_thermal.c +4 -4

index 706d74798cbe..6c4af2622d4f 100644
@@ -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);
 	}
 }
 

Signature · 7 lines

2.11.0


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

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 <[email protected]>

---
 drivers/thermal/armada_thermal.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/thermal/armada_thermal.c +5 -7

index 6c4af2622d4f..26698f2d3ca7 100644
@@ -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;
+	unsigned int is_valid_bit;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -151,7 +149,7 @@ static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
 	unsigned long 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,

Signature · 7 lines

2.11.0


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

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 <[email protected]>

---
 drivers/thermal/armada_thermal.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/thermal/armada_thermal.c +0 -2

index 6c4af2622d4f..26698f2d3ca7 100644
@@ -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

drivers/thermal/armada_thermal.c +3 -3

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

Hi Miquel,

Quoted 21 lines · 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 <[email protected]>

---
 drivers/thermal/armada_thermal.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 6c4af2622d4f..26698f2d3ca7 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +0 -0

@@ -24,8 +24,6 @@
 #include <linux/of_device.h>
 #include <linux/thermal.h>
 

drivers/thermal/armada_thermal.c struct armada_thermal_data { · line 68

Quoted 11 lines

drivers/thermal/armada_thermal.c +1 -3

-#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;
+	unsigned int is_valid_bit;  

Type should be u32 now, I think.

drivers/thermal/armada_thermal.c static bool armada_is_valid(struct · line 150

Quoted 7 lines

drivers/thermal/armada_thermal.c +0 -0

 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -151,7 +149,7 @@ static bool armada_is_valid(struct
 armada_thermal_priv *priv) {
 	unsigned long reg = readl_relaxed(priv->sensor);  

u32 here also, I think. But that's unrelated to this patch.

Quoted 28 lines

drivers/thermal/armada_thermal.c +1 -1

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

drivers/thermal/armada_thermal.c +1 -1

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

drivers/thermal/armada_thermal.c +1 -1

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

drivers/thermal/armada_thermal.c static const struct armada_thermal_data armada375_data = { · line 225

Quoted 6 lines

drivers/thermal/armada_thermal.c +1 -1

 	.init_sensor = armada380_init_sensor,
-	.is_valid_shift = 10,
+	.is_valid_bit = BIT(10),
 	.temp_shift = 0,
 	.temp_mask = 0x3ff,
 	.coef_b = 1172499100UL,  

baruch

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 <[email protected]>

---
 drivers/thermal/armada_thermal.c | 86 +++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 31 deletions(-)

drivers/thermal/armada_thermal.c +55 -31

index 26698f2d3ca7..e5b184cee79b 100644
@@ -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 *status;
+	void __iomem *control0;
+	void __iomem *control1;
 	struct armada_thermal_data *data;
 };
 

@@ -71,45 +80,45 @@ 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);
+	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,
 				  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,37 +126,37 @@ 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);
 	}
 }
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->status);
 
 	return reg & priv->data->is_valid_bit;
 }

@@ -156,7 +165,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 			  int *temp)
 {
 	struct armada_thermal_priv *priv = thermal->devdata;
-	unsigned long reg;
+	u32 reg;
 	unsigned long m, b, div;
 
 	/* Valid check */

@@ -166,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 */

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

@@ -267,14 +277,28 @@ 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);
-	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);
+
+	/*
+	 * 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 ((res->end - res->start) == 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 = (struct armada_thermal_data *)match->data;
 	priv->data->init_sensor(pdev, priv);

Signature · 7 lines

2.11.0


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

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 <[email protected]>

---
 drivers/thermal/armada_thermal.c | 86 +++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 31 deletions(-)

drivers/thermal/armada_thermal.c +7 -0

index 26698f2d3ca7..e5b184cee79b 100644
@@ -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

drivers/thermal/armada_thermal.c +44 -29

 	struct armada_thermal_data *data;
 };
 

@@ -71,45 +80,45 @@ 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);
+	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,
 				  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,37 +126,37 @@ 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);
 	}
 }
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->status);
 
 	return reg & priv->data->is_valid_bit;
 }

@@ -156,7 +165,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 			  int *temp)
 {
 	struct armada_thermal_priv *priv = thermal->devdata;
-	unsigned long reg;
+	u32 reg;
 	unsigned long m, b, div;
 
 	/* Valid check */

@@ -166,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 */

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

@@ -267,14 +277,28 @@ 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);
-	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);
+
+	/*
+	 * 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 ((res->end - res->start) == 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 = (struct armada_thermal_data *)match->data;
 	priv->data->init_sensor(pdev, priv);

Hi Miqu?l,

Quoted 38 lines · 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 <[email protected]>

---
 drivers/thermal/armada_thermal.c | 86

+++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 31 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 26698f2d3ca7..e5b184cee79b 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +4 -0

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

drivers/thermal/armada_thermal.c +3 -0

+#define CONTROL0_OFFSET			0x0
+#define CONTROL1_OFFSET			0x4
+
 struct armada_thermal_data;

drivers/thermal/armada_thermal.c line 56

Quoted 7 lines

/* Marvell EBU Thermal Sensor Dev Structure */
struct armada_thermal_priv {

drivers/thermal/armada_thermal.c +3 -2

-	void __iomem *sensor;
-	void __iomem *control;
+	void __iomem *status;
+	void __iomem *control0;
+	void __iomem *control1;  

The 'status' -> 'sensor' rename is not mentioned in the commit log. I'd say it is a matter for a separate patch.

Otherwise, good cleanup.

baruch

Quoted 172 lines

drivers/thermal/armada_thermal.c +14 -14

 	struct armada_thermal_data *data;
 };

@@ -71,45 +80,45 @@ 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);
+	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,
 				  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,37 +126,37 @@ static void armada370_init_sensor(struct

platform_device *pdev, static void armada375_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv)
{

drivers/thermal/armada_thermal.c +2 -2

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

drivers/thermal/armada_thermal.c +17 -13

 	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);
 	}
 }
 
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
-	unsigned long reg = readl_relaxed(priv->sensor);
+	u32 reg = readl_relaxed(priv->status);
 
 	return reg & priv->data->is_valid_bit;
 }

@@ -156,7 +165,7 @@ static int armada_get_temp(struct
 thermal_zone_device *thermal, int *temp)
 {
 	struct armada_thermal_priv *priv = thermal->devdata;
-	unsigned long reg;
+	u32 reg;
 	unsigned long m, b, div;
 
 	/* Valid check */

@@ -166,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 */

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

@@ -267,14 +277,28 @@ 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);
-	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);
+
+	/*
+	 * Legacy DT bindings only described "control1" register

(also referred

(quoted diff) +1 -0

+	 * as "control MSB" on old documentation). New bindings

cover

(quoted diff) +1 -0

+	 * "control0/control LSB" and "control1/control MSB"

registers within

drivers/thermal/armada_thermal.c +9 -0

+	 * the same resource, which is then of size 8 instead of 4.
+	 */
+	if ((res->end - res->start) == 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 = (struct armada_thermal_data *)match->data;
 	priv->data->init_sensor(pdev, priv);  

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 <[email protected]>

---

Quoted 14 lines · 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 <[email protected]>

---

[...]

Quoted 9 lines

drivers/thermal/armada_thermal.c +9 -0

+	/*
+	 * 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 ((res->end - res->start) == LEGACY_CONTROL_MEM_LEN) {
+		/* ->control0 unavailable in this configuration */
+		priv->control1 = control + LEGACY_CONTROL1_OFFSET;

drivers/thermal/armada_thermal.c static int armada_thermal_probe(struct platform_device *pdev) · line 301

Quoted 4 lines

drivers/thermal/armada_thermal.c +4 -0

+	} else {
+		priv->control0 = control + CONTROL0_OFFSET;
+		priv->control1 = control + CONTROL1_OFFSET;
+	}

I think we need to add a check here that the control registers area size matches the expected value given the compatible string. In case of mismatch probe should fail.

drivers/thermal/armada_thermal.c static int armada_thermal_probe(struct platform_device *pdev) · line 304

Quoted 2 lines

drivers/thermal/armada_thermal.c +0 -0

 	priv->data = (struct armada_thermal_data *)match->data;
 	priv->data->init_sensor(pdev, priv);

Hi Miqu?l,

Quoted 43 lines · Baruch Siach

Quoted 16 lines · 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 <[email protected]>

---

[...]

Quoted 15 lines

(quoted diff) +2 -0

+	/*
+	 * Legacy DT bindings only described "control1" register

(also referred

(quoted diff) +1 -0

+	 * as "control MSB" on old documentation). New bindings

cover

(quoted diff) +1 -0

+	 * "control0/control LSB" and "control1/control MSB"

registers within

(quoted diff) +1 -0

+	 * the same resource, which is then of size 8 instead of

4.

drivers/thermal/armada_thermal.c +3 -0

+	 */
+	if ((res->end - res->start) == LEGACY_CONTROL_MEM_LEN) {
+		/* ->control0 unavailable in this configuration

*/

(quoted diff) +1 -0

+		priv->control1 = control +

LEGACY_CONTROL1_OFFSET;

drivers/thermal/armada_thermal.c static int armada_thermal_probe(struct platform_device *pdev) · line 301

Quoted 4 lines

drivers/thermal/armada_thermal.c +4 -0

+	} else {
+		priv->control0 = control + CONTROL0_OFFSET;
+		priv->control1 = control + CONTROL1_OFFSET;
+	}  

I think we need to add a check here that the control registers area size matches the expected value given the compatible string. In case of mismatch probe should fail.

One more thing. You should probably use resource_size() instead of open coding it. resource_size() does "res->end - res->start + 1". Are you sure your code is correct?

Quoted 3 lines

drivers/thermal/armada_thermal.c static int armada_thermal_probe(struct platform_device *pdev) · line 304

Quoted 3 lines

drivers/thermal/armada_thermal.c +0 -0

 	priv->data = (struct armada_thermal_data *)match->data;
 	priv->data->init_sensor(pdev, priv);  

baruch

From: Baruch Siach <[email protected]>

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() to handle signed values.

Signed-off-by Baruch Siach <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 51 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

drivers/thermal/armada_thermal.c +32 -2

index e5b184cee79b..279d01937bb8 100644
@@ -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 */

@@ -70,6 +75,7 @@ struct armada_thermal_data {
 	unsigned long coef_m;
 	unsigned long coef_div;
 	bool inverted;
+	bool signed_sample;
 
 	/* Register shift and mask to access the sensor temperature */
 	unsigned int temp_shift;

@@ -154,6 +160,24 @@ 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;
+
+	if (!priv->control0) {
+		dev_err(&pdev->dev,
+			"Cannot access to control0 (control LSB) register\n");
+		return;
+	}
+
+	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);

@@ -167,6 +191,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 	struct armada_thermal_priv *priv = thermal->devdata;
 	u32 reg;
 	unsigned long m, b, div;
+	int sample;
 
 	/* Valid check */
 	if (priv->data->is_valid && !priv->data->is_valid(priv)) {

@@ -177,6 +202,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 +214,9 @@ 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) / div;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = (b - (m * sample)) / div;
 	return 0;
 }
 

drivers/thermal/armada_thermal.c +10 -0

+	.coef_m = 423UL,
+	.coef_div = 1,
+	.inverted = true,
+	.signed_sample = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",

@@ -255,6 +298,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.data       = &armada380_data,
 	},
 	{
+		.compatible = "marvell,armada-ap806-thermal",
+		.data       = &armada_ap806_data,
+	},
+	{
 		/* sentinel */
 	},
 };

Signature · 7 lines

2.11.0


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

Quoted 16 lines

drivers/thermal/armada_thermal.c +2 -2

@@ -184,9 +214,9 @@ 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) / div;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = (b - (m * sample)) / div;
 	return 0;
 }
 

@@ -237,6 +267,19 @@ static const struct armada_thermal_data
 armada380_data = { .inverted = true,
 };
 

drivers/thermal/armada_thermal.c static const struct armada_thermal_data armada380_data = { · line 276

Quoted 7 lines

drivers/thermal/armada_thermal.c +7 -0

+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 = -150000,  

Don't you expect any side effect by storing a negative value in a unsigned variable?

Hello Gregory & Baruch,

Quoted 16 lines · On Thu, 14 Dec 2017 12:05:43 +0100 Gregory CLEMENT…

Quoted 16 lines

drivers/thermal/armada_thermal.c +2 -2

@@ -184,9 +214,9 @@ 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) / div;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = (b - (m * sample)) / div;
 	return 0;
 }
 

@@ -237,6 +267,19 @@ static const struct armada_thermal_data
 armada380_data = { .inverted = true,
 };
 

Quoted 10 lines

drivers/thermal/armada_thermal.c static const struct armada_thermal_data armada380_data = { · line 276

Quoted 7 lines

drivers/thermal/armada_thermal.c +7 -0

+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 = -150000,  

Don't you expect any side effect by storing a negative value in a unsigned variable?

That is a fair question, I did not spot that.

As other values are really close to 2^32 I don't know what is the best option for us in this case. Should I:
- don't care?
- use signed values? (dangerous IMHO)
- use a union with a signed and an unsigned value? (problem moved to ->get_temp())

Hi Miqu?l,

Quoted 34 lines · Miquel RAYNAL

Hello Gregory & Baruch,

Quoted 16 lines · On Thu, 14 Dec 2017 12:05:43 +0100 Gregory CLEMENT…

Quoted 16 lines

drivers/thermal/armada_thermal.c +2 -2

@@ -184,9 +214,9 @@ 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) / div;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = (b - (m * sample)) / div;
 	return 0;
 }
 

@@ -237,6 +267,19 @@ static const struct armada_thermal_data
 armada380_data = { .inverted = true,
 };
 

Quoted 10 lines

drivers/thermal/armada_thermal.c static const struct armada_thermal_data armada380_data = { · line 276

Quoted 7 lines

drivers/thermal/armada_thermal.c +7 -0

+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 = -150000,    

Don't you expect any side effect by storing a negative value in a unsigned variable?

That is a fair question, I did not spot that.

Quoted 6 lines

As other values are really close to 2^32 I don't know what is the best option for us in this case. Should I:
- don't care?
- use signed values? (dangerous IMHO)
- use a union with a signed and an unsigned value? (problem moved to ->get_temp())

Another option is to use s64 type.

baruch

From: Baruch Siach <[email protected]>

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() to handle signed values.

Signed-off-by Baruch Siach <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---

drivers/thermal/armada_thermal.c +7 -0

+
+	reg = readl_relaxed(priv->control0);
+	reg &= ~CONTROL0_TSEN_RESET;
+	reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
+	writel(reg, priv->control0);
+	msleep(10);
+}

From: Baruch Siach <[email protected]>

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

Signed-off-by Baruch Siach <[email protected]>

[<[email protected]>: renamed the register pointers]

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

drivers/thermal/armada_thermal.c +0 -1

index 279d01937bb8..f5c911524656 100644
@@ -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

drivers/thermal/armada_thermal.c +1 -0

+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */

@@ -153,11 +156,10 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	u32 reg = readl_relaxed(priv->control1);

drivers/thermal/armada_thermal.c +17 -0

+	msleep(10);
 }
 
 static void armada_ap806_init_sensor(struct platform_device *pdev,

@@ -280,6 +282,18 @@ static const struct armada_thermal_data armada_ap806_data = {
 	.signed_sample = 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 = 1172499100UL,
+	.coef_m = 2000096UL,
+	.coef_div = 4201,
+	.inverted = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",

@@ -302,6 +316,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 */
 	},
 };

Signature · 7 lines

2.11.0


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

Hi Miquel,

Quoted 8 lines · Miquel Raynal

From: Baruch Siach <[email protected]>

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

Signed-off-by Baruch Siach <[email protected]>

[<[email protected]>: renamed the register pointers]

Actually you did more thant this see below

Quoted 19 lines

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 30

++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 279d01937bb8..f5c911524656 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +0 -1

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

drivers/thermal/armada_thermal.c line 56

Quoted 6 lines

drivers/thermal/armada_thermal.c +2 -0

@@ -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)  

You added or rename these values

Quoted 7 lines

drivers/thermal/armada_thermal.c +1 -0

+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */

@@ -153,11 +156,10 @@ static void armada380_init_sensor(struct
 platform_device *pdev, u32 reg = readl_relaxed(priv->control1);
 

drivers/thermal/armada_thermal.c static void armada380_init_sensor(struct platform_device *pdev, · line 161

Quoted 9 lines

drivers/thermal/armada_thermal.c +3 -5

 	/* Reset hardware once */
-	if (!(reg & A380_HW_RESET)) {
-		reg |= A380_HW_RESET;
-		writel(reg, priv->control1);
-		msleep(10);
-	}
+	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
+	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
+	writel(reg, priv->control1);  

And here you modified the behavior of this function. Did you checked that it is valid for Armada 38x?

Given the comment we had, I thought we should not do anything if CONTROL1_EXT_TSEN_HW_RESETn was not set.

By the way, if the new sequence is valid, this comment should be removed or at least updated.

Quoted 51 lines · On Thu, 14 Dec 2017 12:11:49 +0100 Gregory CLEMENT…

Hi Miquel,

Quoted 8 lines · Miquel Raynal

From: Baruch Siach <[email protected]>

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

Signed-off-by Baruch Siach <[email protected]>

[<[email protected]>: renamed the register pointers]

Actually you did more thant this see below

Quoted 19 lines

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 30

++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 279d01937bb8..f5c911524656 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +0 -1

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

drivers/thermal/armada_thermal.c line 56

Quoted 6 lines

drivers/thermal/armada_thermal.c +2 -0

@@ -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)  

You added or rename these values

Quoted 7 lines

drivers/thermal/armada_thermal.c +1 -0

+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */

@@ -153,11 +156,10 @@ static void armada380_init_sensor(struct
 platform_device *pdev, u32 reg = readl_relaxed(priv->control1);
 

Quoted 12 lines

drivers/thermal/armada_thermal.c static void armada380_init_sensor(struct platform_device *pdev, · line 161

Quoted 9 lines

drivers/thermal/armada_thermal.c +3 -5

 	/* Reset hardware once */
-	if (!(reg & A380_HW_RESET)) {
-		reg |= A380_HW_RESET;
-		writel(reg, priv->control1);
-		msleep(10);
-	}
+	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
+	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
+	writel(reg, priv->control1);  

And here you modified the behavior of this function. Did you checked that it is valid for Armada 38x?

There is nothing about it the documentation and anyway this register can be accessed later, so writing it is harmless ayway.

Quoted 1 line

Quoted 2 lines

Given the comment we had, I thought we should not do anything if CONTROL1_EXT_TSEN_HW_RESETn was not set.

That is the opposite, if it is not set (ie. reset is active), you have to set it (reset is then disabled).

Quoted 4 lines

By the way, if the new sequence is valid, this comment should be removed or at least updated.

That's right, I will in v4.

Thanks for reviewing,
Miqu?l

Hi Miquel,

Quoted 74 lines · Miquel RAYNAL

Quoted 51 lines · On Thu, 14 Dec 2017 12:11:49 +0100 Gregory CLEMENT…

Hi Miquel,

Quoted 8 lines · Miquel Raynal

From: Baruch Siach <[email protected]>

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

Signed-off-by Baruch Siach <[email protected]>

[<[email protected]>: renamed the register pointers]

Actually you did more thant this see below

Quoted 19 lines

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 30

++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 279d01937bb8..f5c911524656 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)

drivers/thermal/armada_thermal.c +0 -1

-#define A380_HW_RESET			BIT(8)
 
 /* Legacy bindings */
 #define LEGACY_CONTROL_MEM_LEN		0x4

drivers/thermal/armada_thermal.c line 56

Quoted 6 lines

drivers/thermal/armada_thermal.c +2 -0

@@ -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)    

You added or rename these values

Quoted 7 lines

drivers/thermal/armada_thermal.c +1 -0

+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */

@@ -153,11 +156,10 @@ static void armada380_init_sensor(struct
 platform_device *pdev, u32 reg = readl_relaxed(priv->control1);
 

Quoted 12 lines

drivers/thermal/armada_thermal.c static void armada380_init_sensor(struct platform_device *pdev, · line 161

Quoted 9 lines

drivers/thermal/armada_thermal.c +3 -5

 	/* Reset hardware once */
-	if (!(reg & A380_HW_RESET)) {
-		reg |= A380_HW_RESET;
-		writel(reg, priv->control1);
-		msleep(10);
-	}
+	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
+	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
+	writel(reg, priv->control1);    

And here you modified the behavior of this function. Did you checked that it is valid for Armada 38x?

There is nothing about it the documentation and anyway this register can be accessed later, so writing it is harmless ayway.

Quoted 3 lines

Given the comment we had, I thought we should not do anything if CONTROL1_EXT_TSEN_HW_RESETn was not set.

Quoted 2 lines

That is the opposite, if it is not set (ie. reset is active), you have to set it (reset is then disabled).

Actually I was concerned by the "once" for me it means "only one time", but maybe it just meant it was useless to reset it again but not harmful.

Gregory

Quoted 11 lines

Quoted 4 lines

By the way, if the new sequence is valid, this comment should be removed or at least updated.

That's right, I will in v4.

Thanks for reviewing,
Miqu?l

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.

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/Kconfig          | 4 ++--
 drivers/thermal/armada_thermal.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/thermal/Kconfig +0 -0

index 315ae2926e20..44cad046f272 100644

drivers/thermal/Kconfig +1 -1

 	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 Armada 370, Armada XP, Armada 7K/8K

SoCs.

config DA9062_THERMAL

drivers/thermal/Kconfig +0 -0

 	tristate "DA9062/DA9061 Dialog Semiconductor thermal driver"

drivers/thermal/armada_thermal.c +1 -1

index f5c911524656..da58a8961a37 100644
@@ -403,5 +403,5 @@ static struct platform_driver armada_thermal_driver = {
 module_platform_driver(armada_thermal_driver);
 
 MODULE_AUTHOR("Ezequiel Garcia <[email protected]>");
-MODULE_DESCRIPTION("Armada 370/XP thermal driver");
+MODULE_DESCRIPTION("Armada SoCs thermal driver");
 MODULE_LICENSE("GPL v2");

Signature · 7 lines

2.11.0


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

Hi Miquel,

Quoted 17 lines · 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.

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/Kconfig          | 4 ++--
 drivers/thermal/armada_thermal.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/thermal/Kconfig +0 -0

index 315ae2926e20..44cad046f272 100644
@@ -301,13 +301,13 @@ config DB8500_THERMAL
 	  thermal zone if trip points reached.
 

drivers/thermal/Kconfig config DB8500_THERMAL · line 304

Quoted 3 lines

drivers/thermal/Kconfig +1 -1

 config ARMADA_THERMAL
-	tristate "Armada 370/XP thermal management"
+	tristate "Armada 370/XP/7K/8K thermal management"  

if you want to be exhaustive you should also add Armada 375 and 38x.

Hi Gregory,

Quoted 22 lines · On Thu, 14 Dec 2017 12:13:51 +0100 Gregory CLEMENT…

Hi Miquel,

Quoted 17 lines · 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.

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/Kconfig          | 4 ++--
 drivers/thermal/armada_thermal.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/thermal/Kconfig +0 -0

index 315ae2926e20..44cad046f272 100644
@@ -301,13 +301,13 @@ config DB8500_THERMAL
 	  thermal zone if trip points reached.
 

Quoted 4 lines

drivers/thermal/Kconfig config DB8500_THERMAL · line 304

Quoted 3 lines

drivers/thermal/Kconfig +1 -1

 config ARMADA_THERMAL
-	tristate "Armada 370/XP thermal management"
+	tristate "Armada 370/XP/7K/8K thermal management"  

if you want to be exhaustive you should also add Armada 375 and 38x.

That is right, both are missing from the description, but wouldn't it be better to just state "Armada SoCs" instead of patching that file everythime a new SoC reuses this IP?

Thanks,
Miqu?l

Hi Miquel,

Quoted 33 lines · Miquel RAYNAL

Hi Gregory,

Quoted 22 lines · On Thu, 14 Dec 2017 12:13:51 +0100 Gregory CLEMENT…

Hi Miquel,

Quoted 17 lines · 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.

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/Kconfig          | 4 ++--
 drivers/thermal/armada_thermal.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/thermal/Kconfig +0 -0

index 315ae2926e20..44cad046f272 100644
@@ -301,13 +301,13 @@ config DB8500_THERMAL
 	  thermal zone if trip points reached.
 

Quoted 5 lines

drivers/thermal/Kconfig config DB8500_THERMAL · line 304

Quoted 3 lines

drivers/thermal/Kconfig +1 -1

 config ARMADA_THERMAL
-	tristate "Armada 370/XP thermal management"
+	tristate "Armada 370/XP/7K/8K thermal management"    

if you want to be exhaustive you should also add Armada 375 and 38x.

Quoted 3 lines

That is right, both are missing from the description, but wouldn't it be better to just state "Armada SoCs" instead of patching that file everythime a new SoC reuses this IP?

Unfortunately Armada SoCs is more that just these SoC!

Have a look on Documentation/arm/Marvell/README to see how the Marvell marketing guys had been creative :)

Some kirkwood are called Armada 300 and Armada 310.
The Dove is also called Armada 510.
Some PXA are called Armada too such as Armada 168 or Armada 610. And finally the Berlin also use Aramda as code name: Armada 1000 or Aramda 1500

Unfortunately Armada SoCs is more that just these SoC!

Have a look on Documentation/arm/Marvell/README to see how the Marvell marketing guys had been creative :)

Some kirkwood are called Armada 300 and Armada 310.
The Dove is also called Armada 510.
Some PXA are called Armada too such as Armada 168 or Armada 610. And finally the Berlin also use Aramda as code name: Armada 1000 or Aramda 1500

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 <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/thermal/armada_thermal.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

drivers/thermal/armada_thermal.c +20 -3

index be141ca53e83..1c4122f7377c 100644
@@ -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 */

@@ -154,6 +158,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)
 {

@@ -163,7 +177,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) {

@@ -171,8 +184,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,

@@ -190,7 +205,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)

Signature · 7 lines

2.11.0


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

Hi Miquel,

Quoted 6 lines · 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.

You only modified the armada 380 and ap806 init function. Does it means that this feature is not supported on the other variant?