[PATCH 01/25] thermal/drivers/hisi: Fix missing interrupt enablement

[PATCH 01/25] thermal/drivers/hisi: Fix missing interrupt enablement

[Daniel Lezcano]

The interrupt for the temperature threshold is not enabled at the end of the
probe function, enable it after the setup is complete.

On the other side, the irq_enabled is not correctly set as we are checking if
the interrupt is masked where 'yes' means irq_enabled=false.

	irq_get_irqchip_state(data->irq, IRQCHIP_STATE_MASKED,
				&data->irq_enabled);

As we are always enabling the interrupt, it is pointless to check if
the interrupt is masked or not, just set irq_enabled to 'true'.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 9c3ce34..f3b50b0 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -345,8 +345,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 	}
 
 	hisi_thermal_enable_bind_irq_sensor(data);
-	irq_get_irqchip_state(data->irq, IRQCHIP_STATE_MASKED,
-			      &data->irq_enabled);
+	data->irq_enabled = true;
 
 	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
 		ret = hisi_thermal_register_sensor(pdev, data,
@@ -358,6 +357,8 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 			hisi_thermal_toggle_sensor(&data->sensors[i], true);
 	}
 
+	enable_irq(data->irq);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

By essence, the tsensor does not really support multiple sensor at the same
time. It allows to set a sensor and use it to get the temperature, another
sensor could be switched but with a delay of 3-5ms. It is difficult to read
simultaneously several sensors without a big delay.

Today, just one sensor is used, it is not necessary to deal with multiple
sensors in the code. Remove them and if it is needed in the future add them
on top of a code which will be clean up in the meantime.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 75 +++++++++++-------------------------------
 1 file changed, 19 insertions(+), 56 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index f3b50b0..687efd4 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -39,6 +39,7 @@
 #define HISI_TEMP_RESET			(100000)
 
 #define HISI_MAX_SENSORS		4
+#define HISI_DEFAULT_SENSOR		2
 
 struct hisi_thermal_sensor {
 	struct hisi_thermal_data *thermal;
@@ -53,9 +54,8 @@ struct hisi_thermal_data {
 	struct mutex thermal_lock;    /* protects register data */
 	struct platform_device *pdev;
 	struct clk *clk;
-	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
-
-	int irq, irq_bind_sensor;
+	struct hisi_thermal_sensor sensors;
+	int irq;
 	bool irq_enabled;
 
 	void __iomem *regs;
@@ -113,7 +113,7 @@ static void hisi_thermal_enable_bind_irq_sensor
 
 	mutex_lock(&data->thermal_lock);
 
-	sensor = &data->sensors[data->irq_bind_sensor];
+	sensor = &data->sensors;
 
 	/* setting the hdak time */
 	writel(0x0, data->regs + TEMP0_CFG);
@@ -160,31 +160,8 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
 	struct hisi_thermal_sensor *sensor = _sensor;
 	struct hisi_thermal_data *data = sensor->thermal;
 
-	int sensor_id = -1, i;
-	long max_temp = 0;
-
 	*temp = hisi_thermal_get_sensor_temp(data, sensor);
 
-	sensor->sensor_temp = *temp;
-
-	for (i = 0; i < HISI_MAX_SENSORS; i++) {
-		if (!data->sensors[i].tzd)
-			continue;
-
-		if (data->sensors[i].sensor_temp >= max_temp) {
-			max_temp = data->sensors[i].sensor_temp;
-			sensor_id = i;
-		}
-	}
-
-	/* If no sensor has been enabled, then skip to enable irq */
-	if (sensor_id == -1)
-		return 0;
-
-	mutex_lock(&data->thermal_lock);
-	data->irq_bind_sensor = sensor_id;
-	mutex_unlock(&data->thermal_lock);
-
 	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
 		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
 	/*
@@ -197,7 +174,7 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
 		return 0;
 	}
 
-	if (max_temp < sensor->thres_temp) {
+	if (*temp < sensor->thres_temp) {
 		data->irq_enabled = true;
 		hisi_thermal_enable_bind_irq_sensor(data);
 		enable_irq(data->irq);
@@ -224,22 +201,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 {
 	struct hisi_thermal_data *data = dev;
 	struct hisi_thermal_sensor *sensor;
-	int i;
 
 	mutex_lock(&data->thermal_lock);
-	sensor = &data->sensors[data->irq_bind_sensor];
+	sensor = &data->sensors;
 
 	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
 		 sensor->thres_temp / 1000);
 	mutex_unlock(&data->thermal_lock);
 
-	for (i = 0; i < HISI_MAX_SENSORS; i++) {
-		if (!data->sensors[i].tzd)
-			continue;
-
-		thermal_zone_device_update(data->sensors[i].tzd,
-					   THERMAL_EVENT_UNSPECIFIED);
-	}
+	thermal_zone_device_update(data->sensors.tzd,
+				   THERMAL_EVENT_UNSPECIFIED);
 
 	return IRQ_HANDLED;
 }
@@ -296,7 +267,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data;
 	struct resource *res;
-	int i;
 	int ret;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
@@ -347,16 +317,17 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 	hisi_thermal_enable_bind_irq_sensor(data);
 	data->irq_enabled = true;
 
-	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
-		ret = hisi_thermal_register_sensor(pdev, data,
-						   &data->sensors[i], i);
-		if (ret)
-			dev_err(&pdev->dev,
-				"failed to register thermal sensor: %d\n", ret);
-		else
-			hisi_thermal_toggle_sensor(&data->sensors[i], true);
+	ret = hisi_thermal_register_sensor(pdev, data,
+					   &data->sensors,
+					   HISI_DEFAULT_SENSOR);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
+			ret);
+		return ret;
 	}
 
+	hisi_thermal_toggle_sensor(&data->sensors, true);
+
 	enable_irq(data->irq);
 
 	return 0;
@@ -365,17 +336,9 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 static int hisi_thermal_remove(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < HISI_MAX_SENSORS; i++) {
-		struct hisi_thermal_sensor *sensor = &data->sensors[i];
-
-		if (!sensor->tzd)
-			continue;
-
-		hisi_thermal_toggle_sensor(sensor, false);
-	}
+	struct hisi_thermal_sensor *sensor = &data->sensors;
 
+	hisi_thermal_toggle_sensor(sensor, false);
 	hisi_thermal_disable_sensor(data);
 	clk_disable_unprepare(data->clk);
 
-- 
2.7.4

[PATCH 03/25] thermal/drivers/hisi: Fix kernel panic on alarm interrupt

[Daniel Lezcano]

The threaded interrupt for the alarm interrupt is requested before the
temperature controller is setup. This one can fire an interrupt immediately
leading to a kernel panic as the sensor data is not initialized.

In order to prevent that, move the threaded irq after the Tsensor is setup.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 687efd4..feae552 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -287,15 +287,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 	if (data->irq < 0)
 		return data->irq;
 
-	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
-					hisi_thermal_alarm_irq,
-					hisi_thermal_alarm_irq_thread,
-					0, "hisi_thermal", data);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, data);
 
 	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
@@ -328,6 +319,15 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 
 	hisi_thermal_toggle_sensor(&data->sensors, true);
 
+	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
+					hisi_thermal_alarm_irq,
+					hisi_thermal_alarm_irq_thread,
+					0, "hisi_thermal", data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
+		return ret;
+	}
+
 	enable_irq(data->irq);
 
 	return 0;
-- 
2.7.4

[PATCH 04/25] thermal/drivers/hisi: Simplify the temperature/step computation

[Daniel Lezcano]

The step and the base temperature are fixed values, we can simplify the
computation by converting the base temperature to milli celsius and use a
pre-computed step value. That saves us a lot of mult + div for nothing at
runtime.

Take also the opportunity to change the function names to be consistent with
the rest of the code.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index feae552..04eb5e2 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -35,8 +35,9 @@
 #define TEMP0_RST_MSK			(0x1C)
 #define TEMP0_VALUE			(0x28)
 
-#define HISI_TEMP_BASE			(-60)
+#define HISI_TEMP_BASE			(-60000)
 #define HISI_TEMP_RESET			(100000)
+#define HISI_TEMP_STEP			(784)
 
 #define HISI_MAX_SENSORS		4
 #define HISI_DEFAULT_SENSOR		2
@@ -61,19 +62,32 @@ struct hisi_thermal_data {
 	void __iomem *regs;
 };
 
-/* in millicelsius */
-static inline int _step_to_temp(int step)
+/*
+ * The temperature computation on the tsensor is as follow:
+ *	Unit: millidegree Celsius
+ *	Step: 255/200 (0.7843)
+ *	Temperature base: -60°C
+ *
+ * The register is programmed in temperature steps, every step is 784
+ * millidegree and begins at -60 000 m°C
+ *
+ * The temperature from the steps:
+ *
+ *	Temp = TempBase + (steps x 784)
+ *
+ * and the steps from the temperature:
+ *
+ *	steps = (Temp - TempBase) / 784
+ *
+ */
+static inline int hisi_thermal_step_to_temp(int step)
 {
-	/*
-	 * Every step equals (1 * 200) / 255 celsius, and finally
-	 * need convert to millicelsius.
-	 */
-	return (HISI_TEMP_BASE * 1000 + (step * 200000 / 255));
+	return HISI_TEMP_BASE + (step * HISI_TEMP_STEP);
 }
 
-static inline long _temp_to_step(long temp)
+static inline long hisi_thermal_temp_to_step(long temp)
 {
-	return ((temp - HISI_TEMP_BASE * 1000) * 255) / 200000;
+	return (temp - HISI_TEMP_BASE) / HISI_TEMP_STEP;
 }
 
 static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
@@ -99,7 +113,7 @@ static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
 	usleep_range(3000, 5000);
 
 	val = readl(data->regs + TEMP0_VALUE);
-	val = _step_to_temp(val);
+	val = hisi_thermal_step_to_temp(val);
 
 	mutex_unlock(&data->thermal_lock);
 
@@ -126,10 +140,11 @@ static void hisi_thermal_enable_bind_irq_sensor
 	writel((sensor->id << 12), data->regs + TEMP0_CFG);
 
 	/* enable for interrupt */
-	writel(_temp_to_step(sensor->thres_temp) | 0x0FFFFFF00,
+	writel(hisi_thermal_temp_to_step(sensor->thres_temp) | 0x0FFFFFF00,
 	       data->regs + TEMP0_TH);
 
-	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
+	writel(hisi_thermal_temp_to_step(HISI_TEMP_RESET),
+	       data->regs + TEMP0_RST_TH);
 
 	/* enable module */
 	writel(0x1, data->regs + TEMP0_RST_MSK);
-- 
2.7.4

[PATCH 05/25] thermal/drivers/hisi: Fix multiple alarm interrupts firing

[Daniel Lezcano]

The DT specifies a threshold of 65000, we setup the register with a value in
the temperature resolution for the controller, 64656.

When we reach 64656, the interrupt fires, the interrupt is disabled. Then the
irq thread runs and calls thermal_zone_device_update() which will call in turn
hisi_thermal_get_temp().

The function will look if the temperature decreased, assuming it was more than
65000, but that is not the case because the current temperature is 64656
(because of the rounding when setting the threshold). This condition being
true, we re-enable the interrupt which fires immediately after exiting the irq
thread. That happens again and again until the temperature goes to more than
65000.

Potentially, there is here an interrupt storm if the temperature stabilizes at
this temperature. A very unlikely case but possible.

In any case, it does not make sense to handle dozens of alarm interrupt for
nothing.

Fix this by rounding the threshold value to the controller resolution so the
check against the threshold is consistent with the one set in the controller.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 04eb5e2..1b44bfe 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -90,6 +90,12 @@ static inline long hisi_thermal_temp_to_step(long temp)
 	return (temp - HISI_TEMP_BASE) / HISI_TEMP_STEP;
 }
 
+static inline long hisi_thermal_round_temp(int temp)
+{
+	return hisi_thermal_step_to_temp(
+		hisi_thermal_temp_to_step(temp));
+}
+
 static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
 					 struct hisi_thermal_sensor *sensor)
 {
@@ -221,7 +227,7 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 	sensor = &data->sensors;
 
 	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
-		 sensor->thres_temp / 1000);
+		 sensor->thres_temp);
 	mutex_unlock(&data->thermal_lock);
 
 	thermal_zone_device_update(data->sensors.tzd,
@@ -255,7 +261,7 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 
 	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
 		if (trip[i].type == THERMAL_TRIP_PASSIVE) {
-			sensor->thres_temp = trip[i].temperature;
+			sensor->thres_temp = hisi_thermal_round_temp(trip[i].temperature);
 			break;
 		}
 	}
-- 
2.7.4

[PATCH 06/25] thermal/drivers/hisi: Remove pointless lock

[Daniel Lezcano]

The threaded interrupt inspect the sensors structure to look in the temp
threshold field, but this field is read-only in all the code, except in the
probe function before the threaded interrupt is set. In other words there
is not race window in the threaded interrupt when reading the field value.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 1b44bfe..b657ae4 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -221,14 +221,10 @@ static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
 static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 {
 	struct hisi_thermal_data *data = dev;
-	struct hisi_thermal_sensor *sensor;
-
-	mutex_lock(&data->thermal_lock);
-	sensor = &data->sensors;
+	struct hisi_thermal_sensor *sensor = &data->sensors;
 
 	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
 		 sensor->thres_temp);
-	mutex_unlock(&data->thermal_lock);
 
 	thermal_zone_device_update(data->sensors.tzd,
 				   THERMAL_EVENT_UNSPECIFIED);
-- 
2.7.4

[PATCH 07/25] thermal/drivers/hisi: Encapsulate register writes into helpers

[Daniel Lezcano]

Hopefully, the function name can help to clarify the semantic of the operations
when writing in the register.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 92 ++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index b657ae4..8e8a117 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -26,6 +26,7 @@
 
 #include "thermal_core.h"
 
+#define TEMP0_LAG			(0x0)
 #define TEMP0_TH			(0x4)
 #define TEMP0_RST_TH			(0x8)
 #define TEMP0_CFG			(0xC)
@@ -96,6 +97,56 @@ static inline long hisi_thermal_round_temp(int temp)
 		hisi_thermal_temp_to_step(temp));
 }
 
+static inline void hisi_thermal_set_lag(void __iomem *addr, int value)
+{
+	writel(value, addr + TEMP0_LAG);
+}
+
+static inline void hisi_thermal_alarm_clear(void __iomem *addr, int value)
+{
+	writel(value, addr + TEMP0_INT_CLR);
+}
+
+static inline void hisi_thermal_alarm_enable(void __iomem *addr, int value)
+{
+	writel(value, addr + TEMP0_INT_EN);
+}
+
+static inline void hisi_thermal_alarm_set(void __iomem *addr, int temp)
+{
+	writel(hisi_thermal_temp_to_step(temp) | 0x0FFFFFF00, addr + TEMP0_TH);
+}
+
+static inline void hisi_thermal_reset_set(void __iomem *addr, int temp)
+{
+	writel(hisi_thermal_temp_to_step(temp), addr + TEMP0_RST_TH);
+}
+
+static inline void hisi_thermal_reset_enable(void __iomem *addr, int value)
+{
+	writel(value, addr + TEMP0_RST_MSK);
+}
+
+static inline void hisi_thermal_enable(void __iomem *addr, int value)
+{
+	writel(value, addr + TEMP0_EN);
+}
+
+static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
+{
+	writel((sensor << 12), addr + TEMP0_CFG);
+}
+
+static inline int hisi_thermal_get_temperature(void __iomem *addr)
+{
+	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
+}
+
+static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
+{
+	writel(value, addr + TEMP0_CFG);
+}
+
 static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
 					 struct hisi_thermal_sensor *sensor)
 {
@@ -104,22 +155,21 @@ static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
 	mutex_lock(&data->thermal_lock);
 
 	/* disable interrupt */
-	writel(0x0, data->regs + TEMP0_INT_EN);
-	writel(0x1, data->regs + TEMP0_INT_CLR);
+	hisi_thermal_alarm_enable(data->regs, 0);
+	hisi_thermal_alarm_clear(data->regs, 1);
 
 	/* disable module firstly */
-	writel(0x0, data->regs + TEMP0_EN);
+	hisi_thermal_enable(data->regs, 0);
 
 	/* select sensor id */
-	writel((sensor->id << 12), data->regs + TEMP0_CFG);
+	hisi_thermal_sensor_select(data->regs, sensor->id);
 
 	/* enable module */
-	writel(0x1, data->regs + TEMP0_EN);
+	hisi_thermal_enable(data->regs, 1);
 
 	usleep_range(3000, 5000);
 
-	val = readl(data->regs + TEMP0_VALUE);
-	val = hisi_thermal_step_to_temp(val);
+	val = hisi_thermal_get_temperature(data->regs);
 
 	mutex_unlock(&data->thermal_lock);
 
@@ -136,28 +186,26 @@ static void hisi_thermal_enable_bind_irq_sensor
 	sensor = &data->sensors;
 
 	/* setting the hdak time */
-	writel(0x0, data->regs + TEMP0_CFG);
+	hisi_thermal_hdak_set(data->regs, 0);
 
 	/* disable module firstly */
-	writel(0x0, data->regs + TEMP0_RST_MSK);
-	writel(0x0, data->regs + TEMP0_EN);
+	hisi_thermal_reset_enable(data->regs, 0);
+	hisi_thermal_enable(data->regs, 0);
 
 	/* select sensor id */
-	writel((sensor->id << 12), data->regs + TEMP0_CFG);
+	hisi_thermal_sensor_select(data->regs, sensor->id);
 
 	/* enable for interrupt */
-	writel(hisi_thermal_temp_to_step(sensor->thres_temp) | 0x0FFFFFF00,
-	       data->regs + TEMP0_TH);
+	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
 
-	writel(hisi_thermal_temp_to_step(HISI_TEMP_RESET),
-	       data->regs + TEMP0_RST_TH);
+	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
 
 	/* enable module */
-	writel(0x1, data->regs + TEMP0_RST_MSK);
-	writel(0x1, data->regs + TEMP0_EN);
+	hisi_thermal_reset_enable(data->regs, 1);
+	hisi_thermal_enable(data->regs, 1);
 
-	writel(0x0, data->regs + TEMP0_INT_CLR);
-	writel(0x1, data->regs + TEMP0_INT_EN);
+	hisi_thermal_alarm_clear(data->regs, 0);
+	hisi_thermal_alarm_enable(data->regs, 1);
 
 	usleep_range(3000, 5000);
 
@@ -169,9 +217,9 @@ static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
 	mutex_lock(&data->thermal_lock);
 
 	/* disable sensor module */
-	writel(0x0, data->regs + TEMP0_INT_EN);
-	writel(0x0, data->regs + TEMP0_RST_MSK);
-	writel(0x0, data->regs + TEMP0_EN);
+	hisi_thermal_enable(data->regs, 0);
+	hisi_thermal_alarm_enable(data->regs, 0);
+	hisi_thermal_reset_enable(data->regs, 0);
 
 	mutex_unlock(&data->thermal_lock);
 }
-- 
2.7.4

[PATCH 08/25] thermal/drivers/hisi: Fix configuration register setting

[Daniel Lezcano]

The TEMP0_CFG configuration register contains different field to set up the
temperature controller. However in the code, nothing prevents a setup to
overwrite the previous one: eg. writing the hdak value overwrites the sensor
selection, the sensor selection overwrites the hdak value.

In order to prevent such thing, use a regmap-like mechanism by reading the
value before, set the corresponding bits and write the result.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 8e8a117..5688556 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -30,6 +30,8 @@
 #define TEMP0_TH			(0x4)
 #define TEMP0_RST_TH			(0x8)
 #define TEMP0_CFG			(0xC)
+#define TEMP0_CFG_SS_MSK		(0xF000)
+#define TEMP0_CFG_HDAK_MSK		(0x30)
 #define TEMP0_EN			(0x10)
 #define TEMP0_INT_EN			(0x14)
 #define TEMP0_INT_CLR			(0x18)
@@ -132,19 +134,41 @@ static inline void hisi_thermal_enable(void __iomem *addr, int value)
 	writel(value, addr + TEMP0_EN);
 }
 
-static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
+static inline int hisi_thermal_get_temperature(void __iomem *addr)
 {
-	writel((sensor << 12), addr + TEMP0_CFG);
+	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
 }
 
-static inline int hisi_thermal_get_temperature(void __iomem *addr)
+/*
+ * Temperature configuration register - Sensor selection
+ *
+ * Bits [19:12]
+ *
+ * 0x0: local sensor (default)
+ * 0x1: remote sensor 1 (ACPU cluster 1)
+ * 0x2: remote sensor 2 (ACPU cluster 0)
+ * 0x3: remote sensor 3 (G3D)
+ */
+static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
 {
-	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
+	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_SS_MSK ) |
+	       (sensor << 12), addr + TEMP0_CFG);
 }
 
+/*
+ * Temperature configuration register - Hdak conversion polling interval
+ *
+ * Bits [5:4]
+ *
+ * 0x0 :   0.768 ms
+ * 0x1 :   6.144 ms
+ * 0x2 :  49.152 ms
+ * 0x3 : 393.216 ms
+ */
 static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
 {
-	writel(value, addr + TEMP0_CFG);
+	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_HDAK_MSK) |
+	       (value << 4), addr + TEMP0_CFG);
 }
 
 static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
-- 
2.7.4

[PATCH 09/25] thermal/drivers/hisi: Remove costly sensor inspection

[Daniel Lezcano]

The sensor is all setup, bind, resetted, acked, etc... every single second.

That was the way to workaround a problem with the interrupt bouncing again and
again.

With the following changes, we fix all in one:

 - Do the setup, one time, at probe time

 - Add the IRQF_ONESHOT, ack the interrupt in the threaded handler

 - Remove the interrupt handler

 - Set the correct value for the LAG register

 - Remove all the irq_enabled stuff in the code as the interruption
   handling is fixed

 - Remove the 3ms delay

 - Reorder the initialization routine to be in the right order

It ends up to a nicer code and more efficient, the 3-5ms delay is removed from
the get_temp() path.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 203 +++++++++++++++++++----------------------
 1 file changed, 93 insertions(+), 110 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 5688556..5b3e681 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -41,6 +41,7 @@
 #define HISI_TEMP_BASE			(-60000)
 #define HISI_TEMP_RESET			(100000)
 #define HISI_TEMP_STEP			(784)
+#define HISI_TEMP_LAG			(3500)
 
 #define HISI_MAX_SENSORS		4
 #define HISI_DEFAULT_SENSOR		2
@@ -60,8 +61,6 @@ struct hisi_thermal_data {
 	struct clk *clk;
 	struct hisi_thermal_sensor sensors;
 	int irq;
-	bool irq_enabled;
-
 	void __iomem *regs;
 };
 
@@ -99,9 +98,40 @@ static inline long hisi_thermal_round_temp(int temp)
 		hisi_thermal_temp_to_step(temp));
 }
 
+/*
+ * The lag register contains 5 bits encoding the temperature in steps.
+ *
+ * Each time the temperature crosses the threshold boundary, an
+ * interrupt is raised. It could be when the temperature is going
+ * above the threshold or below. However, if the temperature is
+ * fluctuating around this value due to the load, we can receive
+ * several interrupts which may not desired.
+ *
+ * We can setup a temperature representing the delta between the
+ * threshold and the current temperature when the temperature is
+ * decreasing.
+ *
+ * For instance: the lag register is 5°C, the threshold is 65°C, when
+ * the temperature reaches 65°C an interrupt is raised and when the
+ * temperature decrease to 65°C - 5°C another interrupt is raised.
+ *
+ * A very short lag can lead to an interrupt storm, a long lag
+ * increase the latency to react to the temperature changes.  In our
+ * case, that is not really a problem as we are polling the
+ * temperature.
+ *
+ * [0:4] : lag register
+ *
+ * The temperature is coded in steps, cf. HISI_TEMP_STEP.
+ *
+ * Min : 0x00 :  0.0 °C
+ * Max : 0x1F : 24.3 °C
+ *
+ * The 'value' parameter is in milliCelsius.
+ */
 static inline void hisi_thermal_set_lag(void __iomem *addr, int value)
 {
-	writel(value, addr + TEMP0_LAG);
+	writel((value / HISI_TEMP_STEP) & 0x1F, addr + TEMP0_LAG);
 }
 
 static inline void hisi_thermal_alarm_clear(void __iomem *addr, int value)
@@ -171,71 +201,6 @@ static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
 	       (value << 4), addr + TEMP0_CFG);
 }
 
-static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
-					 struct hisi_thermal_sensor *sensor)
-{
-	long val;
-
-	mutex_lock(&data->thermal_lock);
-
-	/* disable interrupt */
-	hisi_thermal_alarm_enable(data->regs, 0);
-	hisi_thermal_alarm_clear(data->regs, 1);
-
-	/* disable module firstly */
-	hisi_thermal_enable(data->regs, 0);
-
-	/* select sensor id */
-	hisi_thermal_sensor_select(data->regs, sensor->id);
-
-	/* enable module */
-	hisi_thermal_enable(data->regs, 1);
-
-	usleep_range(3000, 5000);
-
-	val = hisi_thermal_get_temperature(data->regs);
-
-	mutex_unlock(&data->thermal_lock);
-
-	return val;
-}
-
-static void hisi_thermal_enable_bind_irq_sensor
-			(struct hisi_thermal_data *data)
-{
-	struct hisi_thermal_sensor *sensor;
-
-	mutex_lock(&data->thermal_lock);
-
-	sensor = &data->sensors;
-
-	/* setting the hdak time */
-	hisi_thermal_hdak_set(data->regs, 0);
-
-	/* disable module firstly */
-	hisi_thermal_reset_enable(data->regs, 0);
-	hisi_thermal_enable(data->regs, 0);
-
-	/* select sensor id */
-	hisi_thermal_sensor_select(data->regs, sensor->id);
-
-	/* enable for interrupt */
-	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
-
-	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
-
-	/* enable module */
-	hisi_thermal_reset_enable(data->regs, 1);
-	hisi_thermal_enable(data->regs, 1);
-
-	hisi_thermal_alarm_clear(data->regs, 0);
-	hisi_thermal_alarm_enable(data->regs, 1);
-
-	usleep_range(3000, 5000);
-
-	mutex_unlock(&data->thermal_lock);
-}
-
 static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
 {
 	mutex_lock(&data->thermal_lock);
@@ -253,25 +218,10 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
 	struct hisi_thermal_sensor *sensor = _sensor;
 	struct hisi_thermal_data *data = sensor->thermal;
 
-	*temp = hisi_thermal_get_sensor_temp(data, sensor);
-
-	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
-		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
-	/*
-	 * Bind irq to sensor for two cases:
-	 *   Reenable alarm IRQ if temperature below threshold;
-	 *   if irq has been enabled, always set it;
-	 */
-	if (data->irq_enabled) {
-		hisi_thermal_enable_bind_irq_sensor(data);
-		return 0;
-	}
+	*temp = hisi_thermal_get_temperature(data->regs);
 
-	if (*temp < sensor->thres_temp) {
-		data->irq_enabled = true;
-		hisi_thermal_enable_bind_irq_sensor(data);
-		enable_irq(data->irq);
-	}
+	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
+		sensor->id, *temp, sensor->thres_temp);
 
 	return 0;
 }
@@ -280,26 +230,27 @@ static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
 	.get_temp = hisi_thermal_get_temp,
 };
 
-static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
+static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 {
 	struct hisi_thermal_data *data = dev;
+	struct hisi_thermal_sensor *sensor = &data->sensors;
+	int temp;
 
-	disable_irq_nosync(irq);
-	data->irq_enabled = false;
+	hisi_thermal_alarm_clear(data->regs, 1);
 
-	return IRQ_WAKE_THREAD;
-}
+	temp = hisi_thermal_get_temperature(data->regs);
 
-static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
-{
-	struct hisi_thermal_data *data = dev;
-	struct hisi_thermal_sensor *sensor = &data->sensors;
+	if (temp >= sensor->thres_temp) {
+		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
+			 temp, sensor->thres_temp);
 
-	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
-		 sensor->thres_temp);
+		thermal_zone_device_update(data->sensors.tzd,
+					   THERMAL_EVENT_UNSPECIFIED);
 
-	thermal_zone_device_update(data->sensors.tzd,
-				   THERMAL_EVENT_UNSPECIFIED);
+	} else if (temp < sensor->thres_temp) {
+		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
+			 temp, sensor->thres_temp);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -352,6 +303,40 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
 		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
 }
 
+static int hisi_thermal_setup(struct hisi_thermal_data *data)
+{
+	struct hisi_thermal_sensor *sensor;
+
+	sensor = &data->sensors;
+
+	/* disable module firstly */
+	hisi_thermal_reset_enable(data->regs, 0);
+	hisi_thermal_enable(data->regs, 0);
+
+	/* select sensor id */
+	hisi_thermal_sensor_select(data->regs, sensor->id);
+
+	/* setting the hdak time */
+	hisi_thermal_hdak_set(data->regs, 0);
+
+	/* setting lag value between current temp and the threshold */
+	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
+
+	/* enable for interrupt */
+	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
+
+	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
+
+	/* enable module */
+	hisi_thermal_reset_enable(data->regs, 1);
+	hisi_thermal_enable(data->regs, 1);
+
+	hisi_thermal_alarm_clear(data->regs, 0);
+	hisi_thermal_alarm_enable(data->regs, 1);
+
+	return 0;
+}
+
 static int hisi_thermal_probe(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data;
@@ -394,9 +379,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	hisi_thermal_enable_bind_irq_sensor(data);
-	data->irq_enabled = true;
-
 	ret = hisi_thermal_register_sensor(pdev, data,
 					   &data->sensors,
 					   HISI_DEFAULT_SENSOR);
@@ -406,18 +388,21 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	hisi_thermal_toggle_sensor(&data->sensors, true);
+	ret = hisi_thermal_setup(data);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to setup the sensor: %d\n", ret);
+		return ret;
+	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
-					hisi_thermal_alarm_irq,
+	ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
 					hisi_thermal_alarm_irq_thread,
-					0, "hisi_thermal", data);
+					IRQF_ONESHOT, "hisi_thermal", data);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
 		return ret;
 	}
 
-	enable_irq(data->irq);
+	hisi_thermal_toggle_sensor(&data->sensors, true);
 
 	return 0;
 }
@@ -440,7 +425,6 @@ static int hisi_thermal_suspend(struct device *dev)
 	struct hisi_thermal_data *data = dev_get_drvdata(dev);
 
 	hisi_thermal_disable_sensor(data);
-	data->irq_enabled = false;
 
 	clk_disable_unprepare(data->clk);
 
@@ -456,8 +440,7 @@ static int hisi_thermal_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	data->irq_enabled = true;
-	hisi_thermal_enable_bind_irq_sensor(data);
+	hisi_thermal_setup(data);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 10/25] thermal/drivers/hisi: Rename and remove unused field

[Daniel Lezcano]

Rename the 'sensors' field to 'sensor' as we describe only one sensor.
Remove the 'sensor_temp' as it is no longer used.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 5b3e681..70dc373 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -49,8 +49,6 @@
 struct hisi_thermal_sensor {
 	struct hisi_thermal_data *thermal;
 	struct thermal_zone_device *tzd;
-
-	long sensor_temp;
 	uint32_t id;
 	uint32_t thres_temp;
 };
@@ -59,9 +57,9 @@ struct hisi_thermal_data {
 	struct mutex thermal_lock;    /* protects register data */
 	struct platform_device *pdev;
 	struct clk *clk;
-	struct hisi_thermal_sensor sensors;
-	int irq;
+	struct hisi_thermal_sensor sensor;
 	void __iomem *regs;
+	int irq;
 };
 
 /*
@@ -233,7 +231,7 @@ static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
 static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 {
 	struct hisi_thermal_data *data = dev;
-	struct hisi_thermal_sensor *sensor = &data->sensors;
+	struct hisi_thermal_sensor *sensor = &data->sensor;
 	int temp;
 
 	hisi_thermal_alarm_clear(data->regs, 1);
@@ -244,7 +242,7 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
 			 temp, sensor->thres_temp);
 
-		thermal_zone_device_update(data->sensors.tzd,
+		thermal_zone_device_update(data->sensor.tzd,
 					   THERMAL_EVENT_UNSPECIFIED);
 
 	} else if (temp < sensor->thres_temp) {
@@ -307,7 +305,7 @@ static int hisi_thermal_setup(struct hisi_thermal_data *data)
 {
 	struct hisi_thermal_sensor *sensor;
 
-	sensor = &data->sensors;
+	sensor = &data->sensor;
 
 	/* disable module firstly */
 	hisi_thermal_reset_enable(data->regs, 0);
@@ -380,7 +378,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 	}
 
 	ret = hisi_thermal_register_sensor(pdev, data,
-					   &data->sensors,
+					   &data->sensor,
 					   HISI_DEFAULT_SENSOR);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
@@ -402,7 +400,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	hisi_thermal_toggle_sensor(&data->sensors, true);
+	hisi_thermal_toggle_sensor(&data->sensor, true);
 
 	return 0;
 }
@@ -410,7 +408,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 static int hisi_thermal_remove(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
-	struct hisi_thermal_sensor *sensor = &data->sensors;
+	struct hisi_thermal_sensor *sensor = &data->sensor;
 
 	hisi_thermal_toggle_sensor(sensor, false);
 	hisi_thermal_disable_sensor(data);
-- 
2.7.4

[PATCH 11/25] thermal/drivers/hisi: Convert long to int

[Daniel Lezcano]

There is no point to specify the temperature as long variable, the int is
enough.

Replace all long variables to int, so making the code consistent.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 70dc373..dabe9a8 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -85,12 +85,12 @@ static inline int hisi_thermal_step_to_temp(int step)
 	return HISI_TEMP_BASE + (step * HISI_TEMP_STEP);
 }
 
-static inline long hisi_thermal_temp_to_step(long temp)
+static inline int hisi_thermal_temp_to_step(int temp)
 {
 	return (temp - HISI_TEMP_BASE) / HISI_TEMP_STEP;
 }
 
-static inline long hisi_thermal_round_temp(int temp)
+static inline int hisi_thermal_round_temp(int temp)
 {
 	return hisi_thermal_step_to_temp(
 		hisi_thermal_temp_to_step(temp));
-- 
2.7.4

[PATCH 12/25] thermal/drivers/hisi: Remove thermal data back pointer

[Daniel Lezcano]

The presence of the thermal data pointer in the sensor structure has the unique
purpose of accessing the thermal data in the interrupt handler.

The sensor pointer is passed when registering the interrupt handler, replace the
cookie by the thermal data pointer, so the back pointer is no longer needed.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index dabe9a8..5b0b5be 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -47,7 +47,6 @@
 #define HISI_DEFAULT_SENSOR		2
 
 struct hisi_thermal_sensor {
-	struct hisi_thermal_data *thermal;
 	struct thermal_zone_device *tzd;
 	uint32_t id;
 	uint32_t thres_temp;
@@ -211,10 +210,10 @@ static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
 	mutex_unlock(&data->thermal_lock);
 }
 
-static int hisi_thermal_get_temp(void *_sensor, int *temp)
+static int hisi_thermal_get_temp(void *__data, int *temp)
 {
-	struct hisi_thermal_sensor *sensor = _sensor;
-	struct hisi_thermal_data *data = sensor->thermal;
+	struct hisi_thermal_data *data = __data;
+	struct hisi_thermal_sensor *sensor = &data->sensor;
 
 	*temp = hisi_thermal_get_temperature(data->regs);
 
@@ -262,10 +261,10 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 	const struct thermal_trip *trip;
 
 	sensor->id = index;
-	sensor->thermal = data;
 
 	sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
-				sensor->id, sensor, &hisi_of_thermal_ops);
+							   sensor->id, data,
+							   &hisi_of_thermal_ops);
 	if (IS_ERR(sensor->tzd)) {
 		ret = PTR_ERR(sensor->tzd);
 		sensor->tzd = NULL;
-- 
2.7.4

[PATCH 13/25] thermal/drivers/hisi: Remove mutex_lock in the code

[Daniel Lezcano]

The mutex is used to protect against writes in the configuration register.

That happens at probe time, with no possible race yet.

Then when the module is unloaded and at suspend/resume.

When the module is unloaded, it is an userspace operation, thus via a process.
Suspending the system goes through the freezer to suspend all the tasks
synchronously before continuing. So it is not possible to hit the suspend ops
in this driver while we are unloading it.

The resume is the same situation than the probe.

In other words, even if there are several places where we write the
configuration register, there is no situation where we can write it at the same
time, so far as I can judge

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 5b0b5be..241d42c 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -53,7 +53,6 @@ struct hisi_thermal_sensor {
 };
 
 struct hisi_thermal_data {
-	struct mutex thermal_lock;    /* protects register data */
 	struct platform_device *pdev;
 	struct clk *clk;
 	struct hisi_thermal_sensor sensor;
@@ -200,14 +199,10 @@ static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
 
 static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
 {
-	mutex_lock(&data->thermal_lock);
-
 	/* disable sensor module */
 	hisi_thermal_enable(data->regs, 0);
 	hisi_thermal_alarm_enable(data->regs, 0);
 	hisi_thermal_reset_enable(data->regs, 0);
-
-	mutex_unlock(&data->thermal_lock);
 }
 
 static int hisi_thermal_get_temp(void *__data, int *temp)
@@ -344,7 +339,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
-	mutex_init(&data->thermal_lock);
 	data->pdev = pdev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
2.7.4

[PATCH 14/25] thermal/drivers/generic-iio-adc: Switch tz request to devm version

[Daniel Lezcano]

Everything mentionned here:
 https://lkml.org/lkml/2016/4/20/850

This driver was added before the devm_iio_channel_get() function version was
merged. The sensor should be released before the iio channel, thus we had to
use the non-devm version of thermal_zone_of_sensor_register().

Now the devm_iio_channel_get() is available, do the corresponding change in
this driver and remove gadc_thermal_remove().

[Compiled tested only]

Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal-generic-adc.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
index 73f55d6..46d3005 100644
--- a/drivers/thermal/thermal-generic-adc.c
+++ b/drivers/thermal/thermal-generic-adc.c
@@ -126,38 +126,23 @@ static int gadc_thermal_probe(struct platform_device *pdev)
 	gti->dev = &pdev->dev;
 	platform_set_drvdata(pdev, gti);
 
-	gti->channel = iio_channel_get(&pdev->dev, "sensor-channel");
+	gti->channel = devm_iio_channel_get(&pdev->dev, "sensor-channel");
 	if (IS_ERR(gti->channel)) {
 		ret = PTR_ERR(gti->channel);
 		dev_err(&pdev->dev, "IIO channel not found: %d\n", ret);
 		return ret;
 	}
 
-	gti->tz_dev = thermal_zone_of_sensor_register(&pdev->dev, 0,
-						      gti, &gadc_thermal_ops);
+	gti->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, gti,
+							   &gadc_thermal_ops);
 	if (IS_ERR(gti->tz_dev)) {
 		ret = PTR_ERR(gti->tz_dev);
 		dev_err(&pdev->dev, "Thermal zone sensor register failed: %d\n",
 			ret);
-		goto sensor_fail;
+		return ret;
 	}
 
 	return 0;
-
-sensor_fail:
-	iio_channel_release(gti->channel);
-
-	return ret;
-}
-
-static int gadc_thermal_remove(struct platform_device *pdev)
-{
-	struct gadc_thermal_info *gti = platform_get_drvdata(pdev);
-
-	thermal_zone_of_sensor_unregister(&pdev->dev, gti->tz_dev);
-	iio_channel_release(gti->channel);
-
-	return 0;
 }
 
 static const struct of_device_id of_adc_thermal_match[] = {
@@ -172,7 +157,6 @@ static struct platform_driver gadc_thermal_driver = {
 		.of_match_table = of_adc_thermal_match,
 	},
 	.probe = gadc_thermal_probe,
-	.remove = gadc_thermal_remove,
 };
 
 module_platform_driver(gadc_thermal_driver);
-- 
2.7.4

[GIT PULL] thermal: new material for hikey for 4.15

[Daniel Lezcano]

Hi Rui, Eduardo,

** this is my first pull request for thermal, so I may have choose the
wrong branch **

The changes are based on top of the thermal-soc branch.

The pull request contains the following changes:

 - Reworked the hikey6220 thermal driver by improving the code, fixing
the interrupt setup issues and removed the locks. Set the scene for the
hikey3660 support (Daniel Lezcano)

 - Switched tz request to devm version in order to be consistent with
the devm API used in the driver (Daniel Lezcano)

 - Fixed temperature regulation misbehavior with the step wise governor
leading to a hikey hard reset (Daniel Lezcano)

 - Added support for the hikey3660 thermal driver (Kevin Wangtao)

Thanks!

  -- Daniel

-------------------------------------------------------------------------

The following changes since commit 7a348799d5d9087bbc2e60767f5e6da9f70cc7ca:

  Merge branches 'mediatek-mt2712', 'rockchip-rk3328' and
'uniphier-thermal' into thermal-soc (2017-09-08 11:17:53 +0800)

are available in the git repository at:

  https://git.linaro.org/people/daniel.lezcano/linux.git thermal/hikey-next

for you to fetch changes up to 0b860d941472c97d16d65d4f1acd7fd2c2b29cb0:

  arm64: dts: Register Hi3660's thermal sensor (2017-10-10 19:53:42 +0200)

----------------------------------------------------------------
Daniel Lezcano (16):
      thermal/drivers/hisi: Fix missing interrupt enablement
      thermal/drivers/hisi: Remove the multiple sensors support
      thermal/drivers/hisi: Fix kernel panic on alarm interrupt
      thermal/drivers/hisi: Simplify the temperature/step computation
      thermal/drivers/hisi: Fix multiple alarm interrupts firing
      thermal/drivers/hisi: Remove pointless lock
      thermal/drivers/hisi: Encapsulate register writes into helpers
      thermal/drivers/hisi: Fix configuration register setting
      thermal/drivers/hisi: Remove costly sensor inspection
      thermal/drivers/hisi: Rename and remove unused field
      thermal/drivers/hisi: Convert long to int
      thermal/drivers/hisi: Remove thermal data back pointer
      thermal/drivers/hisi: Remove mutex_lock in the code
      thermal/drivers/generic-iio-adc: Switch tz request to devm version
      thermal/drivers/step_wise: Fix temperature regulation misbehavior
      thermal/drivers/qcom-spmi: Use devm_iio_channel_get

Kevin Wangtao (9):
      thermal/drivers/hisi: Move the clk setup in the corresponding
functions
      thermal/drivers/hisi: Use round up step value
      thermal/drivers/hisi: Put platform code together
      thermal/drivers/hisi: Add platform prefix to function name
      thermal/drivers/hisi: Prepare to add support for other hisi platforms
      thermal/drivers/hisi: Add support for multi temp threshold
      dt-bindings: Document the hi3660 thermal sensor binding
      thermal/drivers/hisi: Add support for hi3660 SoC
      arm64: dts: Register Hi3660's thermal sensor

 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt |   9 ++
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi                       |   8 ++
 drivers/thermal/hisi_thermal.c                                  | 623
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------
 drivers/thermal/qcom-spmi-temp-alarm.c                          |  43
+++------
 drivers/thermal/step_wise.c                                     |  11 ++-
 drivers/thermal/thermal-generic-adc.c                           |  24 +----
 6 files changed, 453 insertions(+), 265 deletions(-)


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 15/25] thermal/drivers/step_wise: Fix temperature regulation misbehavior

[Daniel Lezcano]

There is a particular situation when the cooling device is cpufreq and the heat
dissipation is not efficient enough where the temperature increases little by
little until reaching the critical threshold and leading to a SoC reset.

The behavior is reproducible on a hikey6220 with bad heat dissipation (eg.
stacked with other boards).

Running a simple C program doing while(1); for each CPU of the SoC makes the
temperature to reach the passive regulation trip point and ends up to the
maximum allowed temperature followed by a reset.

This issue has been also reported by running the libhugetlbfs test suite.

What is observed is a ping pong between two cpu frequencies, 1.2GHz and 900MHz
while the temperature continues to grow.

It appears the step wise governor calls get_target_state() the first time with
the throttle set to true and the trend to 'raising'. The code selects logically
the next state, so the cpu frequency decreases from 1.2GHz to 900MHz, so far so
good. The temperature decreases immediately but still stays greater than the
trip point, then get_target_state() is called again, this time with the
throttle set to true *and* the trend to 'dropping'. From there the algorithm
assumes we have to step down the state and the cpu frequency jumps back to
1.2GHz. But the temperature is still higher than the trip point, so
get_target_state() is called with throttle=1 and trend='raising' again, we jump
to 900MHz, then get_target_state() is called with throttle=1 and
trend='dropping', we jump to 1.2GHz, etc ... but the temperature does not
stabilizes and continues to increase.

[  237.922654] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=1,throttle=1
[  237.922678] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=1,throttle=1
[  237.922690] thermal cooling_device0: cur_state=0
[  237.922701] thermal cooling_device0: old_target=0, target=1
[  238.026656] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=2,throttle=1
[  238.026680] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=2,throttle=1
[  238.026694] thermal cooling_device0: cur_state=1
[  238.026707] thermal cooling_device0: old_target=1, target=0
[  238.134647] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=1,throttle=1
[  238.134667] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=1,throttle=1
[  238.134679] thermal cooling_device0: cur_state=0
[  238.134690] thermal cooling_device0: old_target=0, target=1

In this situation the temperature continues to increase while the trend is
oscillating between 'dropping' and 'raising'. We need to keep the current state
untouched if the throttle is set, so the temperature can decrease or a higher
state could be selected, thus preventing this oscillation.

Keeping the next_target untouched when 'throttle' is true at 'dropping' time
fixes the issue.

The following traces show the governor does not change the next state if
trend==2 (dropping) and throttle==1.

[ 2306.127987] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=1,throttle=1
[ 2306.128009] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=1,throttle=1
[ 2306.128021] thermal cooling_device0: cur_state=0
[ 2306.128031] thermal cooling_device0: old_target=0, target=1
[ 2306.231991] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=2,throttle=1
[ 2306.232016] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=2,throttle=1
[ 2306.232030] thermal cooling_device0: cur_state=1
[ 2306.232042] thermal cooling_device0: old_target=1, target=1
[ 2306.335982] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=0,throttle=1
[ 2306.336006] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=0,throttle=1
[ 2306.336021] thermal cooling_device0: cur_state=1
[ 2306.336034] thermal cooling_device0: old_target=1, target=1
[ 2306.439984] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=2,throttle=1
[ 2306.440008] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=2,throttle=0
[ 2306.440022] thermal cooling_device0: cur_state=1
[ 2306.440034] thermal cooling_device0: old_target=1, target=0

[ ... ]

After a while, if the temperature continues to increase, the next state becomes
2 which is 720MHz on the hikey. That results in the temperature stabilizing
around the trip point.

[ 2455.831982] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=1,throttle=1
[ 2455.832006] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=1,throttle=0
[ 2455.832019] thermal cooling_device0: cur_state=1
[ 2455.832032] thermal cooling_device0: old_target=1, target=1
[ 2455.935985] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=0,throttle=1
[ 2455.936013] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=0,throttle=0
[ 2455.936027] thermal cooling_device0: cur_state=1
[ 2455.936040] thermal cooling_device0: old_target=1, target=1
[ 2456.043984] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=0,throttle=1
[ 2456.044009] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=0,throttle=0
[ 2456.044023] thermal cooling_device0: cur_state=1
[ 2456.044036] thermal cooling_device0: old_target=1, target=1
[ 2456.148001] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=1,throttle=1
[ 2456.148028] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=1,throttle=1
[ 2456.148042] thermal cooling_device0: cur_state=1
[ 2456.148055] thermal cooling_device0: old_target=1, target=2
[ 2456.252009] thermal thermal_zone0: Trip0[type=1,temp=65000]:trend=2,throttle=1
[ 2456.252041] thermal thermal_zone0: Trip1[type=1,temp=75000]:trend=2,throttle=0
[ 2456.252058] thermal cooling_device0: cur_state=2
[ 2456.252075] thermal cooling_device0: old_target=2, target=1

IOW, this change is needed to keep the state for a cooling device if the
temperature trend is oscillating while the temperature increases slightly.

Without this change, the situation above leads to a catastrophic crash by a
hardware reset on hikey. This issue has been reported to happen on an OMAP
dra7xx also.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Keerthy <j-keerthy@ti.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Tested-by: Keerthy <j-keerthy@ti.com>
Reviewed-by: Keerthy <j-keerthy@ti.com>
---
 drivers/thermal/step_wise.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index be95826..ee047ca 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -31,8 +31,7 @@
  * If the temperature is higher than a trip point,
  *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
  *       state for this trip point
- *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
- *       state for this trip point
+ *    b. if the trend is THERMAL_TREND_DROPPING, do nothing
  *    c. if the trend is THERMAL_TREND_RAISE_FULL, use upper limit
  *       for this trip point
  *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
@@ -94,9 +93,11 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 			if (!throttle)
 				next_target = THERMAL_NO_TARGET;
 		} else {
-			next_target = cur_state - 1;
-			if (next_target > instance->upper)
-				next_target = instance->upper;
+			if (!throttle) {
+				next_target = cur_state - 1;
+				if (next_target > instance->upper)
+					next_target = instance->upper;
+			}
 		}
 		break;
 	case THERMAL_TREND_DROP_FULL:
-- 
2.7.4

[PATCH 16/25] thermal/drivers/qcom-spmi: Use devm_iio_channel_get

[Daniel Lezcano]

The iio_channel_get() function has now its devm_ version.

Use it and remove all the rollback code for iio_channel_release() as well
as the .remove ops.

[Compiled tested only]

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/qcom-spmi-temp-alarm.c | 43 +++++++++++-----------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/drivers/thermal/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom-spmi-temp-alarm.c
index f502419..95f987d 100644
--- a/drivers/thermal/qcom-spmi-temp-alarm.c
+++ b/drivers/thermal/qcom-spmi-temp-alarm.c
@@ -125,7 +125,7 @@ static int qpnp_tm_get_temp(void *data, int *temp)
 	if (!temp)
 		return -EINVAL;
 
-	if (IS_ERR(chip->adc)) {
+	if (!chip->adc) {
 		ret = qpnp_tm_update_temp_no_adc(chip);
 		if (ret < 0)
 			return ret;
@@ -224,67 +224,53 @@ static int qpnp_tm_probe(struct platform_device *pdev)
 		return irq;
 
 	/* ADC based measurements are optional */
-	chip->adc = iio_channel_get(&pdev->dev, "thermal");
-	if (PTR_ERR(chip->adc) == -EPROBE_DEFER)
-		return PTR_ERR(chip->adc);
+	chip->adc = devm_iio_channel_get(&pdev->dev, "thermal");
+	if (IS_ERR(chip->adc)) {
+		ret = PTR_ERR(chip->adc);
+		chip->adc = NULL;
+		if (ret == -EPROBE_DEFER)
+			return ret;
+	}
 
 	chip->base = res;
 
 	ret = qpnp_tm_read(chip, QPNP_TM_REG_TYPE, &type);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not read type\n");
-		goto fail;
+		return ret;
 	}
 
 	ret = qpnp_tm_read(chip, QPNP_TM_REG_SUBTYPE, &subtype);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not read subtype\n");
-		goto fail;
+		return ret;
 	}
 
 	if (type != QPNP_TM_TYPE || subtype != QPNP_TM_SUBTYPE) {
 		dev_err(&pdev->dev, "invalid type 0x%02x or subtype 0x%02x\n",
 			type, subtype);
-		ret = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
 	ret = qpnp_tm_init(chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "init failed\n");
-		goto fail;
+		return ret;
 	}
 
 	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, qpnp_tm_isr,
 					IRQF_ONESHOT, node->name, chip);
 	if (ret < 0)
-		goto fail;
+		return ret;
 
 	chip->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, chip,
 							&qpnp_tm_sensor_ops);
 	if (IS_ERR(chip->tz_dev)) {
 		dev_err(&pdev->dev, "failed to register sensor\n");
-		ret = PTR_ERR(chip->tz_dev);
-		goto fail;
+		return PTR_ERR(chip->tz_dev);
 	}
 
 	return 0;
-
-fail:
-	if (!IS_ERR(chip->adc))
-		iio_channel_release(chip->adc);
-
-	return ret;
-}
-
-static int qpnp_tm_remove(struct platform_device *pdev)
-{
-	struct qpnp_tm_chip *chip = dev_get_drvdata(&pdev->dev);
-
-	if (!IS_ERR(chip->adc))
-		iio_channel_release(chip->adc);
-
-	return 0;
 }
 
 static const struct of_device_id qpnp_tm_match_table[] = {
@@ -299,7 +285,6 @@ static struct platform_driver qpnp_tm_driver = {
 		.of_match_table = qpnp_tm_match_table,
 	},
 	.probe  = qpnp_tm_probe,
-	.remove = qpnp_tm_remove,
 };
 module_platform_driver(qpnp_tm_driver);
 
-- 
2.7.4

[PATCH 17/25] thermal/drivers/hisi: Move the clk setup in the corresponding functions

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

The sensor's clock is enabled and disabled outside of the probe and
disable function. Moving the corresponding action in the
hisi_thermal_setup() and hisi_thermal_disable_sensor(), factors out
some lines of code and makes the code more symmetric.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 241d42c..02d0ad8 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -203,6 +203,8 @@ static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
 	hisi_thermal_enable(data->regs, 0);
 	hisi_thermal_alarm_enable(data->regs, 0);
 	hisi_thermal_reset_enable(data->regs, 0);
+
+	clk_disable_unprepare(data->clk);
 }
 
 static int hisi_thermal_get_temp(void *__data, int *temp)
@@ -297,9 +299,13 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
 
 static int hisi_thermal_setup(struct hisi_thermal_data *data)
 {
-	struct hisi_thermal_sensor *sensor;
+	struct hisi_thermal_sensor *sensor = &data->sensor;
+	int ret;
 
-	sensor = &data->sensor;
+	/* enable clock for tsensor */
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
 
 	/* disable module firstly */
 	hisi_thermal_reset_enable(data->regs, 0);
@@ -363,13 +369,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* enable clock for thermal */
-	ret = clk_prepare_enable(data->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
-		return ret;
-	}
-
 	ret = hisi_thermal_register_sensor(pdev, data,
 					   &data->sensor,
 					   HISI_DEFAULT_SENSOR);
@@ -405,7 +404,6 @@ static int hisi_thermal_remove(struct platform_device *pdev)
 
 	hisi_thermal_toggle_sensor(sensor, false);
 	hisi_thermal_disable_sensor(data);
-	clk_disable_unprepare(data->clk);
 
 	return 0;
 }
@@ -417,23 +415,14 @@ static int hisi_thermal_suspend(struct device *dev)
 
 	hisi_thermal_disable_sensor(data);
 
-	clk_disable_unprepare(data->clk);
-
 	return 0;
 }
 
 static int hisi_thermal_resume(struct device *dev)
 {
 	struct hisi_thermal_data *data = dev_get_drvdata(dev);
-	int ret;
 
-	ret = clk_prepare_enable(data->clk);
-	if (ret)
-		return ret;
-
-	hisi_thermal_setup(data);
-
-	return 0;
+	return hisi_thermal_setup(data);
 }
 #endif
 
-- 
2.7.4

[PATCH 18/25] thermal/drivers/hisi: Use round up step value

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

Use round up division to ensure the programmed value of threshold and the lag
are not less than what we set, and in order to keep the accuracy while using
round up division, the step value should be a rounded up value.  There is
no need to use hisi_thermal_round_temp.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 02d0ad8..befdb28 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -40,7 +40,7 @@
 
 #define HISI_TEMP_BASE			(-60000)
 #define HISI_TEMP_RESET			(100000)
-#define HISI_TEMP_STEP			(784)
+#define HISI_TEMP_STEP			(785)
 #define HISI_TEMP_LAG			(3500)
 
 #define HISI_MAX_SENSORS		4
@@ -63,19 +63,19 @@ struct hisi_thermal_data {
 /*
  * The temperature computation on the tsensor is as follow:
  *	Unit: millidegree Celsius
- *	Step: 255/200 (0.7843)
+ *	Step: 200/255 (0.7843)
  *	Temperature base: -60°C
  *
- * The register is programmed in temperature steps, every step is 784
+ * The register is programmed in temperature steps, every step is 785
  * millidegree and begins at -60 000 m°C
  *
  * The temperature from the steps:
  *
- *	Temp = TempBase + (steps x 784)
+ *	Temp = TempBase + (steps x 785)
  *
  * and the steps from the temperature:
  *
- *	steps = (Temp - TempBase) / 784
+ *	steps = (Temp - TempBase) / 785
  *
  */
 static inline int hisi_thermal_step_to_temp(int step)
@@ -85,13 +85,7 @@ static inline int hisi_thermal_step_to_temp(int step)
 
 static inline int hisi_thermal_temp_to_step(int temp)
 {
-	return (temp - HISI_TEMP_BASE) / HISI_TEMP_STEP;
-}
-
-static inline int hisi_thermal_round_temp(int temp)
-{
-	return hisi_thermal_step_to_temp(
-		hisi_thermal_temp_to_step(temp));
+	return DIV_ROUND_UP(temp - HISI_TEMP_BASE, HISI_TEMP_STEP);
 }
 
 /*
@@ -127,7 +121,7 @@ static inline int hisi_thermal_round_temp(int temp)
  */
 static inline void hisi_thermal_set_lag(void __iomem *addr, int value)
 {
-	writel((value / HISI_TEMP_STEP) & 0x1F, addr + TEMP0_LAG);
+	writel(DIV_ROUND_UP(value, HISI_TEMP_STEP) & 0x1F, addr + TEMP0_LAG);
 }
 
 static inline void hisi_thermal_alarm_clear(void __iomem *addr, int value)
@@ -274,7 +268,7 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 
 	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
 		if (trip[i].type == THERMAL_TRIP_PASSIVE) {
-			sensor->thres_temp = hisi_thermal_round_temp(trip[i].temperature);
+			sensor->thres_temp = trip[i].temperature;
 			break;
 		}
 	}
-- 
2.7.4

[PATCH 19/25] thermal/drivers/hisi: Put platform code together

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

Reorganize the code for next patches by moving the functions upper in
the file which will prevent a forward declaration. There is no functional
change here.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 76 +++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index befdb28..ff9055a 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -201,6 +201,44 @@ static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
 	clk_disable_unprepare(data->clk);
 }
 
+
+static int hisi_thermal_setup(struct hisi_thermal_data *data)
+{
+	struct hisi_thermal_sensor *sensor = &data->sensor;
+	int ret;
+
+	/* enable clock for tsensor */
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
+	/* disable module firstly */
+	hisi_thermal_reset_enable(data->regs, 0);
+	hisi_thermal_enable(data->regs, 0);
+
+	/* select sensor id */
+	hisi_thermal_sensor_select(data->regs, sensor->id);
+
+	/* setting the hdak time */
+	hisi_thermal_hdak_set(data->regs, 0);
+
+	/* setting lag value between current temp and the threshold */
+	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
+
+	/* enable for interrupt */
+	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
+
+	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
+
+	/* enable module */
+	hisi_thermal_reset_enable(data->regs, 1);
+	hisi_thermal_enable(data->regs, 1);
+
+	hisi_thermal_alarm_clear(data->regs, 0);
+	hisi_thermal_alarm_enable(data->regs, 1);
+
+	return 0;
+}
 static int hisi_thermal_get_temp(void *__data, int *temp)
 {
 	struct hisi_thermal_data *data = __data;
@@ -291,44 +329,6 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
 		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
 }
 
-static int hisi_thermal_setup(struct hisi_thermal_data *data)
-{
-	struct hisi_thermal_sensor *sensor = &data->sensor;
-	int ret;
-
-	/* enable clock for tsensor */
-	ret = clk_prepare_enable(data->clk);
-	if (ret)
-		return ret;
-
-	/* disable module firstly */
-	hisi_thermal_reset_enable(data->regs, 0);
-	hisi_thermal_enable(data->regs, 0);
-
-	/* select sensor id */
-	hisi_thermal_sensor_select(data->regs, sensor->id);
-
-	/* setting the hdak time */
-	hisi_thermal_hdak_set(data->regs, 0);
-
-	/* setting lag value between current temp and the threshold */
-	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
-
-	/* enable for interrupt */
-	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
-
-	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
-
-	/* enable module */
-	hisi_thermal_reset_enable(data->regs, 1);
-	hisi_thermal_enable(data->regs, 1);
-
-	hisi_thermal_alarm_clear(data->regs, 0);
-	hisi_thermal_alarm_enable(data->regs, 1);
-
-	return 0;
-}
-
 static int hisi_thermal_probe(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data;
-- 
2.7.4

[PATCH 20/25] thermal/drivers/hisi: Add platform prefix to function name

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

As the next patches will provide support for the hikey3660's sensor, several
functions with the same purpose but for different platforms will be introduced.
In order to make a clear distinction between them, let's prefix the function
names with the platform name.

This patch has no functional changes.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 145 +++++++++++++++++++++--------------------
 1 file changed, 73 insertions(+), 72 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index ff9055a..8a70ab7 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -26,25 +26,24 @@
 
 #include "thermal_core.h"
 
-#define TEMP0_LAG			(0x0)
-#define TEMP0_TH			(0x4)
-#define TEMP0_RST_TH			(0x8)
-#define TEMP0_CFG			(0xC)
-#define TEMP0_CFG_SS_MSK		(0xF000)
-#define TEMP0_CFG_HDAK_MSK		(0x30)
-#define TEMP0_EN			(0x10)
-#define TEMP0_INT_EN			(0x14)
-#define TEMP0_INT_CLR			(0x18)
-#define TEMP0_RST_MSK			(0x1C)
-#define TEMP0_VALUE			(0x28)
-
-#define HISI_TEMP_BASE			(-60000)
-#define HISI_TEMP_RESET			(100000)
-#define HISI_TEMP_STEP			(785)
-#define HISI_TEMP_LAG			(3500)
-
-#define HISI_MAX_SENSORS		4
-#define HISI_DEFAULT_SENSOR		2
+#define HI6220_TEMP0_LAG			(0x0)
+#define HI6220_TEMP0_TH				(0x4)
+#define HI6220_TEMP0_RST_TH			(0x8)
+#define HI6220_TEMP0_CFG			(0xC)
+#define HI6220_TEMP0_CFG_SS_MSK		(0xF000)
+#define HI6220_TEMP0_CFG_HDAK_MSK		(0x30)
+#define HI6220_TEMP0_EN				(0x10)
+#define HI6220_TEMP0_INT_EN			(0x14)
+#define HI6220_TEMP0_INT_CLR			(0x18)
+#define HI6220_TEMP0_RST_MSK			(0x1C)
+#define HI6220_TEMP0_VALUE			(0x28)
+
+#define HI6220_TEMP_BASE			(-60000)
+#define HI6220_TEMP_RESET			(100000)
+#define HI6220_TEMP_STEP			(785)
+#define HI6220_TEMP_LAG			(3500)
+
+#define HI6220_DEFAULT_SENSOR		2
 
 struct hisi_thermal_sensor {
 	struct thermal_zone_device *tzd;
@@ -78,14 +77,14 @@ struct hisi_thermal_data {
  *	steps = (Temp - TempBase) / 785
  *
  */
-static inline int hisi_thermal_step_to_temp(int step)
+static inline int hi6220_thermal_step_to_temp(int step)
 {
-	return HISI_TEMP_BASE + (step * HISI_TEMP_STEP);
+	return HI6220_TEMP_BASE + (step * HI6220_TEMP_STEP);
 }
 
-static inline int hisi_thermal_temp_to_step(int temp)
+static inline int hi6220_thermal_temp_to_step(int temp)
 {
-	return DIV_ROUND_UP(temp - HISI_TEMP_BASE, HISI_TEMP_STEP);
+	return DIV_ROUND_UP(temp - HI6220_TEMP_BASE, HI6220_TEMP_STEP);
 }
 
 /*
@@ -112,51 +111,53 @@ static inline int hisi_thermal_temp_to_step(int temp)
  *
  * [0:4] : lag register
  *
- * The temperature is coded in steps, cf. HISI_TEMP_STEP.
+ * The temperature is coded in steps, cf. HI6220_TEMP_STEP.
  *
  * Min : 0x00 :  0.0 °C
  * Max : 0x1F : 24.3 °C
  *
  * The 'value' parameter is in milliCelsius.
  */
-static inline void hisi_thermal_set_lag(void __iomem *addr, int value)
+static inline void hi6220_thermal_set_lag(void __iomem *addr, int value)
 {
-	writel(DIV_ROUND_UP(value, HISI_TEMP_STEP) & 0x1F, addr + TEMP0_LAG);
+	writel(DIV_ROUND_UP(value, HI6220_TEMP_STEP) & 0x1F,
+			addr + HI6220_TEMP0_LAG);
 }
 
-static inline void hisi_thermal_alarm_clear(void __iomem *addr, int value)
+static inline void hi6220_thermal_alarm_clear(void __iomem *addr, int value)
 {
-	writel(value, addr + TEMP0_INT_CLR);
+	writel(value, addr + HI6220_TEMP0_INT_CLR);
 }
 
-static inline void hisi_thermal_alarm_enable(void __iomem *addr, int value)
+static inline void hi6220_thermal_alarm_enable(void __iomem *addr, int value)
 {
-	writel(value, addr + TEMP0_INT_EN);
+	writel(value, addr + HI6220_TEMP0_INT_EN);
 }
 
-static inline void hisi_thermal_alarm_set(void __iomem *addr, int temp)
+static inline void hi6220_thermal_alarm_set(void __iomem *addr, int temp)
 {
-	writel(hisi_thermal_temp_to_step(temp) | 0x0FFFFFF00, addr + TEMP0_TH);
+	writel(hi6220_thermal_temp_to_step(temp) | 0x0FFFFFF00,
+			addr + HI6220_TEMP0_TH);
 }
 
-static inline void hisi_thermal_reset_set(void __iomem *addr, int temp)
+static inline void hi6220_thermal_reset_set(void __iomem *addr, int temp)
 {
-	writel(hisi_thermal_temp_to_step(temp), addr + TEMP0_RST_TH);
+	writel(hi6220_thermal_temp_to_step(temp), addr + HI6220_TEMP0_RST_TH);
 }
 
-static inline void hisi_thermal_reset_enable(void __iomem *addr, int value)
+static inline void hi6220_thermal_reset_enable(void __iomem *addr, int value)
 {
-	writel(value, addr + TEMP0_RST_MSK);
+	writel(value, addr + HI6220_TEMP0_RST_MSK);
 }
 
-static inline void hisi_thermal_enable(void __iomem *addr, int value)
+static inline void hi6220_thermal_enable(void __iomem *addr, int value)
 {
-	writel(value, addr + TEMP0_EN);
+	writel(value, addr + HI6220_TEMP0_EN);
 }
 
-static inline int hisi_thermal_get_temperature(void __iomem *addr)
+static inline int hi6220_thermal_get_temperature(void __iomem *addr)
 {
-	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
+	return hi6220_thermal_step_to_temp(readl(addr + HI6220_TEMP0_VALUE));
 }
 
 /*
@@ -169,10 +170,10 @@ static inline int hisi_thermal_get_temperature(void __iomem *addr)
  * 0x2: remote sensor 2 (ACPU cluster 0)
  * 0x3: remote sensor 3 (G3D)
  */
-static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
+static inline void hi6220_thermal_sensor_select(void __iomem *addr, int sensor)
 {
-	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_SS_MSK ) |
-	       (sensor << 12), addr + TEMP0_CFG);
+	writel((readl(addr + HI6220_TEMP0_CFG) & ~HI6220_TEMP0_CFG_SS_MSK) |
+	       (sensor << 12), addr + HI6220_TEMP0_CFG);
 }
 
 /*
@@ -185,24 +186,24 @@ static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
  * 0x2 :  49.152 ms
  * 0x3 : 393.216 ms
  */
-static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
+static inline void hi6220_thermal_hdak_set(void __iomem *addr, int value)
 {
-	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_HDAK_MSK) |
-	       (value << 4), addr + TEMP0_CFG);
+	writel((readl(addr + HI6220_TEMP0_CFG) & ~HI6220_TEMP0_CFG_HDAK_MSK) |
+	       (value << 4), addr + HI6220_TEMP0_CFG);
 }
 
-static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
+static void hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
 {
 	/* disable sensor module */
-	hisi_thermal_enable(data->regs, 0);
-	hisi_thermal_alarm_enable(data->regs, 0);
-	hisi_thermal_reset_enable(data->regs, 0);
+	hi6220_thermal_enable(data->regs, 0);
+	hi6220_thermal_alarm_enable(data->regs, 0);
+	hi6220_thermal_reset_enable(data->regs, 0);
 
 	clk_disable_unprepare(data->clk);
 }
 
 
-static int hisi_thermal_setup(struct hisi_thermal_data *data)
+static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
 {
 	struct hisi_thermal_sensor *sensor = &data->sensor;
 	int ret;
@@ -213,29 +214,29 @@ static int hisi_thermal_setup(struct hisi_thermal_data *data)
 		return ret;
 
 	/* disable module firstly */
-	hisi_thermal_reset_enable(data->regs, 0);
-	hisi_thermal_enable(data->regs, 0);
+	hi6220_thermal_reset_enable(data->regs, 0);
+	hi6220_thermal_enable(data->regs, 0);
 
 	/* select sensor id */
-	hisi_thermal_sensor_select(data->regs, sensor->id);
+	hi6220_thermal_sensor_select(data->regs, sensor->id);
 
 	/* setting the hdak time */
-	hisi_thermal_hdak_set(data->regs, 0);
+	hi6220_thermal_hdak_set(data->regs, 0);
 
 	/* setting lag value between current temp and the threshold */
-	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
+	hi6220_thermal_set_lag(data->regs, HI6220_TEMP_LAG);
 
 	/* enable for interrupt */
-	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
+	hi6220_thermal_alarm_set(data->regs, sensor->thres_temp);
 
-	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
+	hi6220_thermal_reset_set(data->regs, HI6220_TEMP_RESET);
 
 	/* enable module */
-	hisi_thermal_reset_enable(data->regs, 1);
-	hisi_thermal_enable(data->regs, 1);
+	hi6220_thermal_reset_enable(data->regs, 1);
+	hi6220_thermal_enable(data->regs, 1);
 
-	hisi_thermal_alarm_clear(data->regs, 0);
-	hisi_thermal_alarm_enable(data->regs, 1);
+	hi6220_thermal_alarm_clear(data->regs, 0);
+	hi6220_thermal_alarm_enable(data->regs, 1);
 
 	return 0;
 }
@@ -244,7 +245,7 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
 	struct hisi_thermal_data *data = __data;
 	struct hisi_thermal_sensor *sensor = &data->sensor;
 
-	*temp = hisi_thermal_get_temperature(data->regs);
+	*temp = hi6220_thermal_get_temperature(data->regs);
 
 	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
 		sensor->id, *temp, sensor->thres_temp);
@@ -260,11 +261,11 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 {
 	struct hisi_thermal_data *data = dev;
 	struct hisi_thermal_sensor *sensor = &data->sensor;
-	int temp;
+	int temp = 0;
 
-	hisi_thermal_alarm_clear(data->regs, 1);
+	hi6220_thermal_alarm_clear(data->regs, 1);
 
-	temp = hisi_thermal_get_temperature(data->regs);
+	hisi_thermal_get_temp(data, &temp);
 
 	if (temp >= sensor->thres_temp) {
 		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
@@ -273,7 +274,7 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 		thermal_zone_device_update(data->sensor.tzd,
 					   THERMAL_EVENT_UNSPECIFIED);
 
-	} else if (temp < sensor->thres_temp) {
+	} else {
 		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
 			 temp, sensor->thres_temp);
 	}
@@ -365,14 +366,14 @@ static int hisi_thermal_probe(struct platform_device *pdev)
 
 	ret = hisi_thermal_register_sensor(pdev, data,
 					   &data->sensor,
-					   HISI_DEFAULT_SENSOR);
+					   HI6220_DEFAULT_SENSOR);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
 			ret);
 		return ret;
 	}
 
-	ret = hisi_thermal_setup(data);
+	ret = hi6220_thermal_enable_sensor(data);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to setup the sensor: %d\n", ret);
 		return ret;
@@ -397,7 +398,7 @@ static int hisi_thermal_remove(struct platform_device *pdev)
 	struct hisi_thermal_sensor *sensor = &data->sensor;
 
 	hisi_thermal_toggle_sensor(sensor, false);
-	hisi_thermal_disable_sensor(data);
+	hi6220_thermal_disable_sensor(data);
 
 	return 0;
 }
@@ -407,7 +408,7 @@ static int hisi_thermal_suspend(struct device *dev)
 {
 	struct hisi_thermal_data *data = dev_get_drvdata(dev);
 
-	hisi_thermal_disable_sensor(data);
+	hi6220_thermal_disable_sensor(data);
 
 	return 0;
 }
@@ -416,7 +417,7 @@ static int hisi_thermal_resume(struct device *dev)
 {
 	struct hisi_thermal_data *data = dev_get_drvdata(dev);
 
-	return hisi_thermal_setup(data);
+	return hi6220_thermal_enable_sensor(data);
 }
 #endif
 
-- 
2.7.4

[PATCH 21/25] thermal/drivers/hisi: Prepare to add support for other hisi platforms

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

For platform compatibility, add the tsensor ops to a thermal data structure.
Each platform has its own probe function to register proper tsensor ops
function to the pointer, platform related resource request are also implemented
in the platform probe function.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 135 +++++++++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 46 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 8a70ab7..b5a7159 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/of_device.h>
 
 #include "thermal_core.h"
 
@@ -30,7 +31,7 @@
 #define HI6220_TEMP0_TH				(0x4)
 #define HI6220_TEMP0_RST_TH			(0x8)
 #define HI6220_TEMP0_CFG			(0xC)
-#define HI6220_TEMP0_CFG_SS_MSK		(0xF000)
+#define HI6220_TEMP0_CFG_SS_MSK			(0xF000)
 #define HI6220_TEMP0_CFG_HDAK_MSK		(0x30)
 #define HI6220_TEMP0_EN				(0x10)
 #define HI6220_TEMP0_INT_EN			(0x14)
@@ -41,7 +42,7 @@
 #define HI6220_TEMP_BASE			(-60000)
 #define HI6220_TEMP_RESET			(100000)
 #define HI6220_TEMP_STEP			(785)
-#define HI6220_TEMP_LAG			(3500)
+#define HI6220_TEMP_LAG				(3500)
 
 #define HI6220_DEFAULT_SENSOR		2
 
@@ -52,6 +53,10 @@ struct hisi_thermal_sensor {
 };
 
 struct hisi_thermal_data {
+	int (*get_temp)(struct hisi_thermal_data *data);
+	int (*enable_sensor)(struct hisi_thermal_data *data);
+	int (*disable_sensor)(struct hisi_thermal_data *data);
+	int (*irq_handler)(struct hisi_thermal_data *data);
 	struct platform_device *pdev;
 	struct clk *clk;
 	struct hisi_thermal_sensor sensor;
@@ -59,6 +64,7 @@ struct hisi_thermal_data {
 	int irq;
 };
 
+
 /*
  * The temperature computation on the tsensor is as follow:
  *	Unit: millidegree Celsius
@@ -192,7 +198,18 @@ static inline void hi6220_thermal_hdak_set(void __iomem *addr, int value)
 	       (value << 4), addr + HI6220_TEMP0_CFG);
 }
 
-static void hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
+static int hi6220_thermal_irq_handler(struct hisi_thermal_data *data)
+{
+	hi6220_thermal_alarm_clear(data->regs, 1);
+	return 0;
+}
+
+static int hi6220_thermal_get_temp(struct hisi_thermal_data *data)
+{
+	return hi6220_thermal_get_temperature(data->regs);
+}
+
+static int hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
 {
 	/* disable sensor module */
 	hi6220_thermal_enable(data->regs, 0);
@@ -200,9 +217,9 @@ static void hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
 	hi6220_thermal_reset_enable(data->regs, 0);
 
 	clk_disable_unprepare(data->clk);
+	return 0;
 }
 
-
 static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
 {
 	struct hisi_thermal_sensor *sensor = &data->sensor;
@@ -240,12 +257,50 @@ static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
 
 	return 0;
 }
+
+static int hi6220_thermal_probe(struct hisi_thermal_data *data)
+{
+	struct platform_device *pdev = data->pdev;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	data->get_temp = hi6220_thermal_get_temp;
+	data->enable_sensor = hi6220_thermal_enable_sensor;
+	data->disable_sensor = hi6220_thermal_disable_sensor;
+	data->irq_handler = hi6220_thermal_irq_handler;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->regs)) {
+		dev_err(dev, "failed to get io address\n");
+		return PTR_ERR(data->regs);
+	}
+
+	data->clk = devm_clk_get(dev, "thermal_clk");
+	if (IS_ERR(data->clk)) {
+		ret = PTR_ERR(data->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get thermal clk: %d\n", ret);
+		return ret;
+	}
+
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq < 0)
+		return data->irq;
+
+	data->sensor.id = HI6220_DEFAULT_SENSOR;
+
+	return 0;
+}
+
+
 static int hisi_thermal_get_temp(void *__data, int *temp)
 {
 	struct hisi_thermal_data *data = __data;
 	struct hisi_thermal_sensor *sensor = &data->sensor;
 
-	*temp = hi6220_thermal_get_temperature(data->regs);
+	*temp = data->get_temp(data);
 
 	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
 		sensor->id, *temp, sensor->thres_temp);
@@ -263,7 +318,7 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 	struct hisi_thermal_sensor *sensor = &data->sensor;
 	int temp = 0;
 
-	hi6220_thermal_alarm_clear(data->regs, 1);
+	data->irq_handler(data);
 
 	hisi_thermal_get_temp(data, &temp);
 
@@ -284,14 +339,11 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 
 static int hisi_thermal_register_sensor(struct platform_device *pdev,
 					struct hisi_thermal_data *data,
-					struct hisi_thermal_sensor *sensor,
-					int index)
+					struct hisi_thermal_sensor *sensor)
 {
 	int ret, i;
 	const struct thermal_trip *trip;
 
-	sensor->id = index;
-
 	sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
 							   sensor->id, data,
 							   &hisi_of_thermal_ops);
@@ -316,7 +368,7 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 }
 
 static const struct of_device_id of_hisi_thermal_match[] = {
-	{ .compatible = "hisilicon,tsensor" },
+	{ .compatible = "hisilicon,tsensor", .data = hi6220_thermal_probe },
 	{ /* end */ }
 };
 MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
@@ -333,58 +385,48 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
 static int hisi_thermal_probe(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data;
-	struct resource *res;
+	int (*platform_probe)(struct hisi_thermal_data *);
+	struct device *dev = &pdev->dev;
 	int ret;
 
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	data->pdev = pdev;
+	platform_set_drvdata(pdev, data);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data->regs = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(data->regs)) {
-		dev_err(&pdev->dev, "failed to get io address\n");
-		return PTR_ERR(data->regs);
+	platform_probe = of_device_get_match_data(dev);
+	if (!platform_probe) {
+		dev_err(dev, "failed to get probe func\n");
+		return -EINVAL;
 	}
 
-	data->irq = platform_get_irq(pdev, 0);
-	if (data->irq < 0)
-		return data->irq;
-
-	platform_set_drvdata(pdev, data);
-
-	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
-	if (IS_ERR(data->clk)) {
-		ret = PTR_ERR(data->clk);
-		if (ret != -EPROBE_DEFER)
-			dev_err(&pdev->dev,
-				"failed to get thermal clk: %d\n", ret);
+	ret = platform_probe(data);
+	if (ret)
 		return ret;
-	}
 
 	ret = hisi_thermal_register_sensor(pdev, data,
-					   &data->sensor,
-					   HI6220_DEFAULT_SENSOR);
+					   &data->sensor);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
-			ret);
+		dev_err(dev, "failed to register thermal sensor: %d\n", ret);
 		return ret;
 	}
 
-	ret = hi6220_thermal_enable_sensor(data);
+	ret = data->enable_sensor(data);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to setup the sensor: %d\n", ret);
+		dev_err(dev, "Failed to setup the sensor: %d\n", ret);
 		return ret;
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
-					hisi_thermal_alarm_irq_thread,
-					IRQF_ONESHOT, "hisi_thermal", data);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
-		return ret;
+	if (data->irq) {
+		ret = devm_request_threaded_irq(dev, data->irq, NULL,
+				hisi_thermal_alarm_irq_thread,
+				IRQF_ONESHOT, "hisi_thermal", data);
+		if (ret < 0) {
+			dev_err(dev, "failed to request alarm irq: %d\n", ret);
+			return ret;
+		}
 	}
 
 	hisi_thermal_toggle_sensor(&data->sensor, true);
@@ -398,7 +440,8 @@ static int hisi_thermal_remove(struct platform_device *pdev)
 	struct hisi_thermal_sensor *sensor = &data->sensor;
 
 	hisi_thermal_toggle_sensor(sensor, false);
-	hi6220_thermal_disable_sensor(data);
+
+	data->disable_sensor(data);
 
 	return 0;
 }
@@ -408,7 +451,7 @@ static int hisi_thermal_suspend(struct device *dev)
 {
 	struct hisi_thermal_data *data = dev_get_drvdata(dev);
 
-	hi6220_thermal_disable_sensor(data);
+	data->disable_sensor(data);
 
 	return 0;
 }
@@ -417,7 +460,7 @@ static int hisi_thermal_resume(struct device *dev)
 {
 	struct hisi_thermal_data *data = dev_get_drvdata(dev);
 
-	return hi6220_thermal_enable_sensor(data);
+	return data->enable_sensor(data);
 }
 #endif
 
-- 
2.7.4

[PATCH 22/25] thermal/drivers/hisi: Add support for multi temp threshold

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

The next patches will provide the support for the hi3660 where the temperature
sensor can have multiple alarm levels. In order to set the scene to support it,
we have to convert the current code to be able to support multiple threshold
values.

[Daniel Lezcano: Restated the log]

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index b5a7159..e87ca6c 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -46,10 +46,12 @@
 
 #define HI6220_DEFAULT_SENSOR		2
 
+#define MAX_THRES_NUM			2
+
 struct hisi_thermal_sensor {
 	struct thermal_zone_device *tzd;
 	uint32_t id;
-	uint32_t thres_temp;
+	uint32_t thres_temp[MAX_THRES_NUM];
 };
 
 struct hisi_thermal_data {
@@ -244,7 +246,7 @@ static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
 	hi6220_thermal_set_lag(data->regs, HI6220_TEMP_LAG);
 
 	/* enable for interrupt */
-	hi6220_thermal_alarm_set(data->regs, sensor->thres_temp);
+	hi6220_thermal_alarm_set(data->regs, sensor->thres_temp[0]);
 
 	hi6220_thermal_reset_set(data->regs, HI6220_TEMP_RESET);
 
@@ -303,7 +305,7 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
 	*temp = data->get_temp(data);
 
 	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
-		sensor->id, *temp, sensor->thres_temp);
+		sensor->id, *temp, sensor->thres_temp[0]);
 
 	return 0;
 }
@@ -322,16 +324,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 
 	hisi_thermal_get_temp(data, &temp);
 
-	if (temp >= sensor->thres_temp) {
+	if (temp >= sensor->thres_temp[0]) {
 		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
-			 temp, sensor->thres_temp);
+			 temp, sensor->thres_temp[0]);
 
 		thermal_zone_device_update(data->sensor.tzd,
 					   THERMAL_EVENT_UNSPECIFIED);
 
 	} else {
 		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
-			 temp, sensor->thres_temp);
+			 temp, sensor->thres_temp[0]);
 	}
 
 	return IRQ_HANDLED;
@@ -341,7 +343,7 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 					struct hisi_thermal_data *data,
 					struct hisi_thermal_sensor *sensor)
 {
-	int ret, i;
+	int ret, i, thres_idx = 0;
 	const struct thermal_trip *trip;
 
 	sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
@@ -359,8 +361,9 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 
 	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
 		if (trip[i].type == THERMAL_TRIP_PASSIVE) {
-			sensor->thres_temp = trip[i].temperature;
-			break;
+			sensor->thres_temp[thres_idx++] = trip[i].temperature;
+			if (thres_idx >= MAX_THRES_NUM)
+				break;
 		}
 	}
 
-- 
2.7.4

[PATCH 23/25] dt-bindings: Document the hi3660 thermal sensor binding

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

This adds documentation of device tree bindings for the
thermal sensor controller of hi3660 SoC.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
index d48fc52..cef716a 100644
--- a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
@@ -13,6 +13,7 @@
 
 Example :
 
+for Hi6220:
 	tsensor: tsensor@0,f7030700 {
 		compatible = "hisilicon,tsensor";
 		reg = <0x0 0xf7030700 0x0 0x1000>;
@@ -21,3 +22,11 @@ Example :
 		clock-names = "thermal_clk";
 		#thermal-sensor-cells = <1>;
 	}
+
+for Hi3660:
+	tsensor: tsensor@fff30000 {
+		compatible = "hisilicon,hi3660-tsensor";
+		reg = <0x0 0xfff30000 0x0 0x1000>;
+		interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+		#thermal-sensor-cells = <1>;
+	};
-- 
2.7.4

[PATCH 23/25] dt-bindings: Document the hi3660 thermal sensor binding

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

This adds documentation of device tree bindings for the
thermal sensor controller of hi3660 SoC.

Signed-off-by: Kevin Wangtao <kevin.wangtao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
index d48fc52..cef716a 100644
--- a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
@@ -13,6 +13,7 @@
 
 Example :
 
+for Hi6220:
 	tsensor: tsensor@0,f7030700 {
 		compatible = "hisilicon,tsensor";
 		reg = <0x0 0xf7030700 0x0 0x1000>;
@@ -21,3 +22,11 @@ Example :
 		clock-names = "thermal_clk";
 		#thermal-sensor-cells = <1>;
 	}
+
+for Hi3660:
+	tsensor: tsensor@fff30000 {
+		compatible = "hisilicon,hi3660-tsensor";
+		reg = <0x0 0xfff30000 0x0 0x1000>;
+		interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+		#thermal-sensor-cells = <1>;
+	};
-- 
2.7.4

--
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 24/25] thermal/drivers/hisi: Add support for hi3660 SoC

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

This patch adds the support for thermal sensor on the Hi3660 SoC.
Hi3660 tsensor support alarm interrupt and have three configurable
alarm thresholds, it also has a configurable hysteresis interval,
interrupt will be triggered when temperature rise above the alarm
threshold or fall below the hysteresis threshold.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 146 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 145 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index e87ca6c..10276f0 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -39,12 +39,24 @@
 #define HI6220_TEMP0_RST_MSK			(0x1C)
 #define HI6220_TEMP0_VALUE			(0x28)
 
+#define HI3660_OFFSET(chan)		((chan) * 0x40)
+#define HI3660_TEMP(chan)		(HI3660_OFFSET(chan) + 0x1C)
+#define HI3660_TH(chan)			(HI3660_OFFSET(chan) + 0x20)
+#define HI3660_LAG(chan)		(HI3660_OFFSET(chan) + 0x28)
+#define HI3660_INT_EN(chan)		(HI3660_OFFSET(chan) + 0x2C)
+#define HI3660_INT_CLR(chan)		(HI3660_OFFSET(chan) + 0x30)
+
 #define HI6220_TEMP_BASE			(-60000)
 #define HI6220_TEMP_RESET			(100000)
 #define HI6220_TEMP_STEP			(785)
 #define HI6220_TEMP_LAG				(3500)
 
+#define HI3660_TEMP_BASE		(-63780)
+#define HI3660_TEMP_STEP		(205)
+#define HI3660_TEMP_LAG			(4000)
+
 #define HI6220_DEFAULT_SENSOR		2
+#define HI3660_DEFAULT_SENSOR		1
 
 #define MAX_THRES_NUM			2
 
@@ -96,6 +108,24 @@ static inline int hi6220_thermal_temp_to_step(int temp)
 }
 
 /*
+ * for Hi3660,
+ *	Step: 189/922 (0.205)
+ *	Temperature base: -63.780°C
+ *
+ * The register is programmed in temperature steps, every step is 205
+ * millidegree and begins at -63 780 m°C
+ */
+static inline int hi3660_thermal_step_to_temp(int step)
+{
+	return HI3660_TEMP_BASE + step * HI3660_TEMP_STEP;
+}
+
+static inline int hi3660_thermal_temp_to_step(int temp)
+{
+	return DIV_ROUND_UP(temp - HI3660_TEMP_BASE, HI3660_TEMP_STEP);
+}
+
+/*
  * The lag register contains 5 bits encoding the temperature in steps.
  *
  * Each time the temperature crosses the threshold boundary, an
@@ -169,6 +199,45 @@ static inline int hi6220_thermal_get_temperature(void __iomem *addr)
 }
 
 /*
+ * [0:6] lag register
+ *
+ * The temperature is coded in steps, cf. HI3660_TEMP_STEP.
+ *
+ * Min : 0x00 :  0.0 °C
+ * Max : 0x7F : 26.0 °C
+ *
+ */
+static inline void hi3660_thermal_set_lag(void __iomem *addr,
+					  int id, int value)
+{
+	writel(DIV_ROUND_UP(value, HI3660_TEMP_STEP) & 0x7F,
+			addr + HI3660_LAG(id));
+}
+
+static inline void hi3660_thermal_alarm_clear(void __iomem *addr,
+					      int id, int value)
+{
+	writel(value, addr + HI3660_INT_CLR(id));
+}
+
+static inline void hi3660_thermal_alarm_enable(void __iomem *addr,
+					       int id, int value)
+{
+	writel(value, addr + HI3660_INT_EN(id));
+}
+
+static inline void hi3660_thermal_alarm_set(void __iomem *addr,
+					    int id, int value)
+{
+	writel(value, addr + HI3660_TH(id));
+}
+
+static inline int hi3660_thermal_get_temperature(void __iomem *addr, int id)
+{
+	return hi3660_thermal_step_to_temp(readl(addr + HI3660_TEMP(id)));
+}
+
+/*
  * Temperature configuration register - Sensor selection
  *
  * Bits [19:12]
@@ -206,11 +275,22 @@ static int hi6220_thermal_irq_handler(struct hisi_thermal_data *data)
 	return 0;
 }
 
+static int hi3660_thermal_irq_handler(struct hisi_thermal_data *data)
+{
+	hi3660_thermal_alarm_clear(data->regs, data->sensor.id, 1);
+	return 0;
+}
+
 static int hi6220_thermal_get_temp(struct hisi_thermal_data *data)
 {
 	return hi6220_thermal_get_temperature(data->regs);
 }
 
+static int hi3660_thermal_get_temp(struct hisi_thermal_data *data)
+{
+	return hi3660_thermal_get_temperature(data->regs, data->sensor.id);
+}
+
 static int hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
 {
 	/* disable sensor module */
@@ -222,6 +302,13 @@ static int hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
 	return 0;
 }
 
+static int hi3660_thermal_disable_sensor(struct hisi_thermal_data *data)
+{
+	/* disable sensor module */
+	hi3660_thermal_alarm_enable(data->regs, data->sensor.id, 0);
+	return 0;
+}
+
 static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
 {
 	struct hisi_thermal_sensor *sensor = &data->sensor;
@@ -260,6 +347,29 @@ static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
 	return 0;
 }
 
+static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
+{
+	unsigned int value;
+	struct hisi_thermal_sensor *sensor = &data->sensor;
+
+	/* disable interrupt */
+	hi3660_thermal_alarm_enable(data->regs, sensor->id, 0);
+
+	/* setting lag value between current temp and the threshold */
+	hi3660_thermal_set_lag(data->regs, sensor->id, HI3660_TEMP_LAG);
+
+	/* set interrupt threshold */
+	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
+	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
+	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
+
+	/* enable interrupt */
+	hi3660_thermal_alarm_clear(data->regs, sensor->id, 1);
+	hi3660_thermal_alarm_enable(data->regs, sensor->id, 1);
+
+	return 0;
+}
+
 static int hi6220_thermal_probe(struct hisi_thermal_data *data)
 {
 	struct platform_device *pdev = data->pdev;
@@ -296,6 +406,33 @@ static int hi6220_thermal_probe(struct hisi_thermal_data *data)
 	return 0;
 }
 
+static int hi3660_thermal_probe(struct hisi_thermal_data *data)
+{
+	struct platform_device *pdev = data->pdev;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+
+	data->get_temp = hi3660_thermal_get_temp;
+	data->enable_sensor = hi3660_thermal_enable_sensor;
+	data->disable_sensor = hi3660_thermal_disable_sensor;
+	data->irq_handler = hi3660_thermal_irq_handler;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->regs)) {
+		dev_err(dev, "failed to get io address\n");
+		return PTR_ERR(data->regs);
+	}
+
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq < 0)
+		return data->irq;
+
+	data->sensor.id = HI3660_DEFAULT_SENSOR;
+
+	return 0;
+}
+
 
 static int hisi_thermal_get_temp(void *__data, int *temp)
 {
@@ -371,7 +508,14 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
 }
 
 static const struct of_device_id of_hisi_thermal_match[] = {
-	{ .compatible = "hisilicon,tsensor", .data = hi6220_thermal_probe },
+	{
+		.compatible = "hisilicon,tsensor",
+		.data = hi6220_thermal_probe
+	},
+	{
+		.compatible = "hisilicon,hi3660-tsensor",
+		.data = hi3660_thermal_probe
+	},
 	{ /* end */ }
 };
 MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
-- 
2.7.4

[PATCH 25/25] arm64: dts: Register Hi3660's thermal sensor

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

Add binding for tsensor on H3660, this tsensor is used for
SoC thermal control, it supports alarm interrupt.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index c6a1961..d9c0cf3 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -848,5 +848,13 @@
 				     &sdio_cfg_func>;
 			status = "disabled";
 		};
+
+		tsensor: tsensor@fff30000 {
+			compatible = "hisilicon,hi3660-tsensor";
+			reg = <0x0 0xfff30000 0x0 0x1000>;
+			interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+			#thermal-sensor-cells = <1>;
+		};
+
 	};
 };
-- 
2.7.4

[PATCH 25/25] arm64: dts: Register Hi3660's thermal sensor

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Add binding for tsensor on H3660, this tsensor is used for
SoC thermal control, it supports alarm interrupt.

Signed-off-by: Kevin Wangtao <kevin.wangtao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index c6a1961..d9c0cf3 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -848,5 +848,13 @@
 				     &sdio_cfg_func>;
 			status = "disabled";
 		};
+
+		tsensor: tsensor@fff30000 {
+			compatible = "hisilicon,hi3660-tsensor";
+			reg = <0x0 0xfff30000 0x0 0x1000>;
+			interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+			#thermal-sensor-cells = <1>;
+		};
+
 	};
 };
-- 
2.7.4

--
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 25/25] arm64: dts: Register Hi3660's thermal sensor

[Daniel Lezcano]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

Add binding for tsensor on H3660, this tsensor is used for
SoC thermal control, it supports alarm interrupt.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index c6a1961..d9c0cf3 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -848,5 +848,13 @@
 				     &sdio_cfg_func>;
 			status = "disabled";
 		};
+
+		tsensor: tsensor at fff30000 {
+			compatible = "hisilicon,hi3660-tsensor";
+			reg = <0x0 0xfff30000 0x0 0x1000>;
+			interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
+			#thermal-sensor-cells = <1>;
+		};
+
 	};
 };
-- 
2.7.4

Re: [PATCH 25/25] arm64: dts: Register Hi3660's thermal sensor

[Wei Xu]

Hi Daniel,

On 2017/10/10 19:02, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> Add binding for tsensor on H3660, this tsensor is used for
> SoC thermal control, it supports alarm interrupt.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Applied this and 23rd into hisilicon dt tree.
Thanks!

BR,
Wei

>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index c6a1961..d9c0cf3 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -848,5 +848,13 @@
>  				     &sdio_cfg_func>;
>  			status = "disabled";
>  		};
> +
> +		tsensor: tsensor@fff30000 {
> +			compatible = "hisilicon,hi3660-tsensor";
> +			reg = <0x0 0xfff30000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
>  	};
>  };
> 

Re: [PATCH 25/25] arm64: dts: Register Hi3660's thermal sensor

[Wei Xu]

Hi Daniel,

On 2017/10/10 19:02, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> Add binding for tsensor on H3660, this tsensor is used for
> SoC thermal control, it supports alarm interrupt.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Applied this and 23rd into hisilicon dt tree.
Thanks!

BR,
Wei

>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index c6a1961..d9c0cf3 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -848,5 +848,13 @@
>  				     &sdio_cfg_func>;
>  			status = "disabled";
>  		};
> +
> +		tsensor: tsensor@fff30000 {
> +			compatible = "hisilicon,hi3660-tsensor";
> +			reg = <0x0 0xfff30000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
>  	};
>  };
> 

[PATCH 25/25] arm64: dts: Register Hi3660's thermal sensor

[Wei Xu]

Hi Daniel,

On 2017/10/10 19:02, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> Add binding for tsensor on H3660, this tsensor is used for
> SoC thermal control, it supports alarm interrupt.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Applied this and 23rd into hisilicon dt tree.
Thanks!

BR,
Wei

>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index c6a1961..d9c0cf3 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -848,5 +848,13 @@
>  				     &sdio_cfg_func>;
>  			status = "disabled";
>  		};
> +
> +		tsensor: tsensor at fff30000 {
> +			compatible = "hisilicon,hi3660-tsensor";
> +			reg = <0x0 0xfff30000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
>  	};
>  };
> 

Re: [PATCH 01/25] thermal/drivers/hisi: Fix missing interrupt enablement

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:26PM +0200, Daniel Lezcano wrote:
> The interrupt for the temperature threshold is not enabled at the end of the
> probe function, enable it after the setup is complete.
> 
> On the other side, the irq_enabled is not correctly set as we are checking if
> the interrupt is masked where 'yes' means irq_enabled=false.
> 
> 	irq_get_irqchip_state(data->irq, IRQCHIP_STATE_MASKED,
> 				&data->irq_enabled);
> 
> As we are always enabling the interrupt, it is pointless to check if
> the interrupt is masked or not, just set irq_enabled to 'true'.

If no objections, I will start collecting this series.

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> Tested-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 9c3ce34..f3b50b0 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -345,8 +345,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  	}
>  
>  	hisi_thermal_enable_bind_irq_sensor(data);
> -	irq_get_irqchip_state(data->irq, IRQCHIP_STATE_MASKED,
> -			      &data->irq_enabled);
> +	data->irq_enabled = true;
>  
>  	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>  		ret = hisi_thermal_register_sensor(pdev, data,
> @@ -358,6 +357,8 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  			hisi_thermal_toggle_sensor(&data->sensors[i], true);
>  	}
>  
> +	enable_irq(data->irq);
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> By essence, the tsensor does not really support multiple sensor at the same
> time. It allows to set a sensor and use it to get the temperature, another
> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> simultaneously several sensors without a big delay.
> 

Is 3-5 ms enough to loose an event? Is this really a problem?

> Today, just one sensor is used, it is not necessary to deal with multiple

How come? Today the driver exposes all sensors to userspace. Removing
them, means you are changing the amount of sensors userspace really
sees.

Are you sure about this?

Can you please elaborate how we are only using one of the four sensors?

> sensors in the code. Remove them and if it is needed in the future add them
> on top of a code which will be clean up in the meantime.

It is just not clear to me why having 1 sensor support is better than
having 4, as per the hw supported tsensor.

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I would prefer to get confirmation from folks from hisilicon here.

BTW, you should be copying previous author of the code.

> ---
>  drivers/thermal/hisi_thermal.c | 75 +++++++++++-------------------------------
>  1 file changed, 19 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index f3b50b0..687efd4 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -39,6 +39,7 @@
>  #define HISI_TEMP_RESET			(100000)
>  
>  #define HISI_MAX_SENSORS		4
> +#define HISI_DEFAULT_SENSOR		2
>  
>  struct hisi_thermal_sensor {
>  	struct hisi_thermal_data *thermal;
> @@ -53,9 +54,8 @@ struct hisi_thermal_data {
>  	struct mutex thermal_lock;    /* protects register data */
>  	struct platform_device *pdev;
>  	struct clk *clk;
> -	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
> -
> -	int irq, irq_bind_sensor;
> +	struct hisi_thermal_sensor sensors;
> +	int irq;
>  	bool irq_enabled;
>  
>  	void __iomem *regs;
> @@ -113,7 +113,7 @@ static void hisi_thermal_enable_bind_irq_sensor
>  
>  	mutex_lock(&data->thermal_lock);
>  
> -	sensor = &data->sensors[data->irq_bind_sensor];
> +	sensor = &data->sensors;
>  
>  	/* setting the hdak time */
>  	writel(0x0, data->regs + TEMP0_CFG);
> @@ -160,31 +160,8 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
>  	struct hisi_thermal_sensor *sensor = _sensor;
>  	struct hisi_thermal_data *data = sensor->thermal;
>  
> -	int sensor_id = -1, i;
> -	long max_temp = 0;
> -
>  	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>  
> -	sensor->sensor_temp = *temp;
> -
> -	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> -		if (!data->sensors[i].tzd)
> -			continue;
> -
> -		if (data->sensors[i].sensor_temp >= max_temp) {
> -			max_temp = data->sensors[i].sensor_temp;
> -			sensor_id = i;
> -		}
> -	}
> -
> -	/* If no sensor has been enabled, then skip to enable irq */
> -	if (sensor_id == -1)
> -		return 0;
> -
> -	mutex_lock(&data->thermal_lock);
> -	data->irq_bind_sensor = sensor_id;
> -	mutex_unlock(&data->thermal_lock);
> -
>  	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
>  		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>  	/*
> @@ -197,7 +174,7 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
>  		return 0;
>  	}
>  
> -	if (max_temp < sensor->thres_temp) {
> +	if (*temp < sensor->thres_temp) {
>  		data->irq_enabled = true;
>  		hisi_thermal_enable_bind_irq_sensor(data);
>  		enable_irq(data->irq);
> @@ -224,22 +201,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  {
>  	struct hisi_thermal_data *data = dev;
>  	struct hisi_thermal_sensor *sensor;
> -	int i;
>  
>  	mutex_lock(&data->thermal_lock);
> -	sensor = &data->sensors[data->irq_bind_sensor];
> +	sensor = &data->sensors;
>  
>  	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>  		 sensor->thres_temp / 1000);
>  	mutex_unlock(&data->thermal_lock);
>  
> -	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> -		if (!data->sensors[i].tzd)
> -			continue;
> -
> -		thermal_zone_device_update(data->sensors[i].tzd,
> -					   THERMAL_EVENT_UNSPECIFIED);
> -	}
> +	thermal_zone_device_update(data->sensors.tzd,
> +				   THERMAL_EVENT_UNSPECIFIED);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -296,7 +267,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data;
>  	struct resource *res;
> -	int i;
>  	int ret;
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> @@ -347,16 +317,17 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  	hisi_thermal_enable_bind_irq_sensor(data);
>  	data->irq_enabled = true;
>  
> -	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
> -		ret = hisi_thermal_register_sensor(pdev, data,
> -						   &data->sensors[i], i);
> -		if (ret)
> -			dev_err(&pdev->dev,
> -				"failed to register thermal sensor: %d\n", ret);
> -		else
> -			hisi_thermal_toggle_sensor(&data->sensors[i], true);
> +	ret = hisi_thermal_register_sensor(pdev, data,
> +					   &data->sensors,
> +					   HISI_DEFAULT_SENSOR);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
> +			ret);
> +		return ret;
>  	}
>  
> +	hisi_thermal_toggle_sensor(&data->sensors, true);
> +
>  	enable_irq(data->irq);
>  
>  	return 0;
> @@ -365,17 +336,9 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  static int hisi_thermal_remove(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
> -	int i;
> -
> -	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> -		struct hisi_thermal_sensor *sensor = &data->sensors[i];
> -
> -		if (!sensor->tzd)
> -			continue;
> -
> -		hisi_thermal_toggle_sensor(sensor, false);
> -	}
> +	struct hisi_thermal_sensor *sensor = &data->sensors;
>  
> +	hisi_thermal_toggle_sensor(sensor, false);
>  	hisi_thermal_disable_sensor(data);
>  	clk_disable_unprepare(data->clk);
>  
> -- 
> 2.7.4
> 

Re: [PATCH 08/25] thermal/drivers/hisi: Fix configuration register setting

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:33PM +0200, Daniel Lezcano wrote:
> The TEMP0_CFG configuration register contains different field to set up the
> temperature controller. However in the code, nothing prevents a setup to
> overwrite the previous one: eg. writing the hdak value overwrites the sensor
> selection, the sensor selection overwrites the hdak value.
> 
> In order to prevent such thing, use a regmap-like mechanism by reading the
> value before, set the corresponding bits and write the result.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 8e8a117..5688556 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -30,6 +30,8 @@
>  #define TEMP0_TH			(0x4)
>  #define TEMP0_RST_TH			(0x8)
>  #define TEMP0_CFG			(0xC)
> +#define TEMP0_CFG_SS_MSK		(0xF000)
> +#define TEMP0_CFG_HDAK_MSK		(0x30)
>  #define TEMP0_EN			(0x10)
>  #define TEMP0_INT_EN			(0x14)
>  #define TEMP0_INT_CLR			(0x18)
> @@ -132,19 +134,41 @@ static inline void hisi_thermal_enable(void __iomem *addr, int value)
>  	writel(value, addr + TEMP0_EN);
>  }
>  
> -static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
> +static inline int hisi_thermal_get_temperature(void __iomem *addr)
>  {
> -	writel((sensor << 12), addr + TEMP0_CFG);
> +	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
>  }
>  
> -static inline int hisi_thermal_get_temperature(void __iomem *addr)
> +/*
> + * Temperature configuration register - Sensor selection
> + *
> + * Bits [19:12]
> + *
> + * 0x0: local sensor (default)
> + * 0x1: remote sensor 1 (ACPU cluster 1)
> + * 0x2: remote sensor 2 (ACPU cluster 0)
> + * 0x3: remote sensor 3 (G3D)
> + */
> +static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
>  {
> -	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
> +	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_SS_MSK ) |


ERROR: space prohibited before that close parenthesis ')'
#135: FILE: drivers/thermal/hisi_thermal.c:154:
+	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_SS_MSK ) |



> +	       (sensor << 12), addr + TEMP0_CFG);
>  }
>  
> +/*
> + * Temperature configuration register - Hdak conversion polling interval
> + *
> + * Bits [5:4]
> + *
> + * 0x0 :   0.768 ms
> + * 0x1 :   6.144 ms
> + * 0x2 :  49.152 ms
> + * 0x3 : 393.216 ms
> + */
>  static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
>  {
> -	writel(value, addr + TEMP0_CFG);
> +	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_HDAK_MSK) |
> +	       (value << 4), addr + TEMP0_CFG);
>  }
>  
>  static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
> -- 
> 2.7.4
> 

Re: [PATCH 21/25] thermal/drivers/hisi: Prepare to add support for other hisi platforms

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:46PM +0200, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> For platform compatibility, add the tsensor ops to a thermal data structure.
> Each platform has its own probe function to register proper tsensor ops
> function to the pointer, platform related resource request are also implemented
> in the platform probe function.

Please fix minor check patch issues like:
CHECK: Please don't use multiple blank lines
#142: FILE: drivers/thermal/hisi_thermal.c:67:
 
 +

 CHECK: Please don't use multiple blank lines
 #218: FILE: drivers/thermal/hisi_thermal.c:297:
 +
 +

 CHECK: Alignment should match open parenthesis
 #335: FILE: drivers/thermal/hisi_thermal.c:424:
 +		ret = devm_request_threaded_irq(dev, data->irq, NULL,
 +				hisi_thermal_alarm_irq_thread,



> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 135 +++++++++++++++++++++++++++--------------
>  1 file changed, 89 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 8a70ab7..b5a7159 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
> +#include <linux/of_device.h>
>  
>  #include "thermal_core.h"
>  
> @@ -30,7 +31,7 @@
>  #define HI6220_TEMP0_TH				(0x4)
>  #define HI6220_TEMP0_RST_TH			(0x8)
>  #define HI6220_TEMP0_CFG			(0xC)
> -#define HI6220_TEMP0_CFG_SS_MSK		(0xF000)
> +#define HI6220_TEMP0_CFG_SS_MSK			(0xF000)
>  #define HI6220_TEMP0_CFG_HDAK_MSK		(0x30)
>  #define HI6220_TEMP0_EN				(0x10)
>  #define HI6220_TEMP0_INT_EN			(0x14)
> @@ -41,7 +42,7 @@
>  #define HI6220_TEMP_BASE			(-60000)
>  #define HI6220_TEMP_RESET			(100000)
>  #define HI6220_TEMP_STEP			(785)
> -#define HI6220_TEMP_LAG			(3500)
> +#define HI6220_TEMP_LAG				(3500)
>  
>  #define HI6220_DEFAULT_SENSOR		2
>  
> @@ -52,6 +53,10 @@ struct hisi_thermal_sensor {
>  };
>  
>  struct hisi_thermal_data {
> +	int (*get_temp)(struct hisi_thermal_data *data);
> +	int (*enable_sensor)(struct hisi_thermal_data *data);
> +	int (*disable_sensor)(struct hisi_thermal_data *data);
> +	int (*irq_handler)(struct hisi_thermal_data *data);
>  	struct platform_device *pdev;
>  	struct clk *clk;
>  	struct hisi_thermal_sensor sensor;
> @@ -59,6 +64,7 @@ struct hisi_thermal_data {
>  	int irq;
>  };
>  
> +
>  /*
>   * The temperature computation on the tsensor is as follow:
>   *	Unit: millidegree Celsius
> @@ -192,7 +198,18 @@ static inline void hi6220_thermal_hdak_set(void __iomem *addr, int value)
>  	       (value << 4), addr + HI6220_TEMP0_CFG);
>  }
>  
> -static void hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
> +static int hi6220_thermal_irq_handler(struct hisi_thermal_data *data)
> +{
> +	hi6220_thermal_alarm_clear(data->regs, 1);
> +	return 0;
> +}
> +
> +static int hi6220_thermal_get_temp(struct hisi_thermal_data *data)
> +{
> +	return hi6220_thermal_get_temperature(data->regs);
> +}
> +
> +static int hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
>  {
>  	/* disable sensor module */
>  	hi6220_thermal_enable(data->regs, 0);
> @@ -200,9 +217,9 @@ static void hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
>  	hi6220_thermal_reset_enable(data->regs, 0);
>  
>  	clk_disable_unprepare(data->clk);
> +	return 0;
>  }
>  
> -
>  static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
>  {
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
> @@ -240,12 +257,50 @@ static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
>  
>  	return 0;
>  }
> +
> +static int hi6220_thermal_probe(struct hisi_thermal_data *data)
> +{
> +	struct platform_device *pdev = data->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +
> +	data->get_temp = hi6220_thermal_get_temp;
> +	data->enable_sensor = hi6220_thermal_enable_sensor;
> +	data->disable_sensor = hi6220_thermal_disable_sensor;
> +	data->irq_handler = hi6220_thermal_irq_handler;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(dev, "failed to get io address\n");
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	data->clk = devm_clk_get(dev, "thermal_clk");
> +	if (IS_ERR(data->clk)) {
> +		ret = PTR_ERR(data->clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get thermal clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	data->irq = platform_get_irq(pdev, 0);
> +	if (data->irq < 0)
> +		return data->irq;
> +
> +	data->sensor.id = HI6220_DEFAULT_SENSOR;
> +
> +	return 0;
> +}
> +
> +
>  static int hisi_thermal_get_temp(void *__data, int *temp)
>  {
>  	struct hisi_thermal_data *data = __data;
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
>  
> -	*temp = hi6220_thermal_get_temperature(data->regs);
> +	*temp = data->get_temp(data);
>  
>  	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
>  		sensor->id, *temp, sensor->thres_temp);
> @@ -263,7 +318,7 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
>  	int temp = 0;
>  
> -	hi6220_thermal_alarm_clear(data->regs, 1);
> +	data->irq_handler(data);
>  
>  	hisi_thermal_get_temp(data, &temp);
>  
> @@ -284,14 +339,11 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  
>  static int hisi_thermal_register_sensor(struct platform_device *pdev,
>  					struct hisi_thermal_data *data,
> -					struct hisi_thermal_sensor *sensor,
> -					int index)
> +					struct hisi_thermal_sensor *sensor)
>  {
>  	int ret, i;
>  	const struct thermal_trip *trip;
>  
> -	sensor->id = index;
> -
>  	sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
>  							   sensor->id, data,
>  							   &hisi_of_thermal_ops);
> @@ -316,7 +368,7 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>  }
>  
>  static const struct of_device_id of_hisi_thermal_match[] = {
> -	{ .compatible = "hisilicon,tsensor" },
> +	{ .compatible = "hisilicon,tsensor", .data = hi6220_thermal_probe },
>  	{ /* end */ }
>  };
>  MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
> @@ -333,58 +385,48 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>  static int hisi_thermal_probe(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data;
> -	struct resource *res;
> +	int (*platform_probe)(struct hisi_thermal_data *);
> +	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
>  	data->pdev = pdev;
> +	platform_set_drvdata(pdev, data);
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	data->regs = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(data->regs)) {
> -		dev_err(&pdev->dev, "failed to get io address\n");
> -		return PTR_ERR(data->regs);
> +	platform_probe = of_device_get_match_data(dev);

I get the compilation warn below:

  CHECK   drivers/thermal/hisi_thermal.c
drivers/thermal/hisi_thermal.c:399:24: warning: incorrect type in assignment (different modifiers)
drivers/thermal/hisi_thermal.c:399:24:    expected int ( *platform_probe )( ... )
drivers/thermal/hisi_thermal.c:399:24:    got void const *
  CC [M]  drivers/thermal/hisi_thermal.o

Please fix it.




> +	if (!platform_probe) {
> +		dev_err(dev, "failed to get probe func\n");
> +		return -EINVAL;
>  	}
>  
> -	data->irq = platform_get_irq(pdev, 0);
> -	if (data->irq < 0)
> -		return data->irq;
> -
> -	platform_set_drvdata(pdev, data);
> -
> -	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
> -	if (IS_ERR(data->clk)) {
> -		ret = PTR_ERR(data->clk);
> -		if (ret != -EPROBE_DEFER)
> -			dev_err(&pdev->dev,
> -				"failed to get thermal clk: %d\n", ret);
> +	ret = platform_probe(data);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	ret = hisi_thermal_register_sensor(pdev, data,
> -					   &data->sensor,
> -					   HI6220_DEFAULT_SENSOR);
> +					   &data->sensor);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
> -			ret);
> +		dev_err(dev, "failed to register thermal sensor: %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = hi6220_thermal_enable_sensor(data);
> +	ret = data->enable_sensor(data);
>  	if (ret) {
> -		dev_err(&pdev->dev, "Failed to setup the sensor: %d\n", ret);
> +		dev_err(dev, "Failed to setup the sensor: %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
> -					hisi_thermal_alarm_irq_thread,
> -					IRQF_ONESHOT, "hisi_thermal", data);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
> -		return ret;
> +	if (data->irq) {
> +		ret = devm_request_threaded_irq(dev, data->irq, NULL,
> +				hisi_thermal_alarm_irq_thread,
> +				IRQF_ONESHOT, "hisi_thermal", data);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to request alarm irq: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	hisi_thermal_toggle_sensor(&data->sensor, true);
> @@ -398,7 +440,8 @@ static int hisi_thermal_remove(struct platform_device *pdev)
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
>  
>  	hisi_thermal_toggle_sensor(sensor, false);
> -	hi6220_thermal_disable_sensor(data);
> +
> +	data->disable_sensor(data);
>  
>  	return 0;
>  }
> @@ -408,7 +451,7 @@ static int hisi_thermal_suspend(struct device *dev)
>  {
>  	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>  
> -	hi6220_thermal_disable_sensor(data);
> +	data->disable_sensor(data);
>  
>  	return 0;
>  }
> @@ -417,7 +460,7 @@ static int hisi_thermal_resume(struct device *dev)
>  {
>  	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>  
> -	return hi6220_thermal_enable_sensor(data);
> +	return data->enable_sensor(data);
>  }
>  #endif
>  
> -- 
> 2.7.4
> 

Re: [PATCH 20/25] thermal/drivers/hisi: Add platform prefix to function name

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:45PM +0200, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> As the next patches will provide support for the hikey3660's sensor, several
> functions with the same purpose but for different platforms will be introduced.
> In order to make a clear distinction between them, let's prefix the function
> names with the platform name.
> 
> This patch has no functional changes.


CHECK: Alignment should match open parenthesis
#188: FILE: drivers/thermal/hisi_thermal.c:124:
+	writel(DIV_ROUND_UP(value, HI6220_TEMP_STEP) & 0x1F,
+			addr + HI6220_TEMP0_LAG);

CHECK: Alignment should match open parenthesis
#210: FILE: drivers/thermal/hisi_thermal.c:140:
+	writel(hi6220_thermal_temp_to_step(temp) | 0x0FFFFFF00,
+			addr + HI6220_TEMP0_TH);

total: 0 errors, 1 warnings, 2 checks, 286 lines checked



> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 145 +++++++++++++++++++++--------------------
>  1 file changed, 73 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index ff9055a..8a70ab7 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -26,25 +26,24 @@
>  
>  #include "thermal_core.h"
>  
> -#define TEMP0_LAG			(0x0)
> -#define TEMP0_TH			(0x4)
> -#define TEMP0_RST_TH			(0x8)
> -#define TEMP0_CFG			(0xC)
> -#define TEMP0_CFG_SS_MSK		(0xF000)
> -#define TEMP0_CFG_HDAK_MSK		(0x30)
> -#define TEMP0_EN			(0x10)
> -#define TEMP0_INT_EN			(0x14)
> -#define TEMP0_INT_CLR			(0x18)
> -#define TEMP0_RST_MSK			(0x1C)
> -#define TEMP0_VALUE			(0x28)
> -
> -#define HISI_TEMP_BASE			(-60000)
> -#define HISI_TEMP_RESET			(100000)
> -#define HISI_TEMP_STEP			(785)
> -#define HISI_TEMP_LAG			(3500)
> -
> -#define HISI_MAX_SENSORS		4
> -#define HISI_DEFAULT_SENSOR		2
> +#define HI6220_TEMP0_LAG			(0x0)
> +#define HI6220_TEMP0_TH				(0x4)
> +#define HI6220_TEMP0_RST_TH			(0x8)
> +#define HI6220_TEMP0_CFG			(0xC)
> +#define HI6220_TEMP0_CFG_SS_MSK		(0xF000)
> +#define HI6220_TEMP0_CFG_HDAK_MSK		(0x30)
> +#define HI6220_TEMP0_EN				(0x10)
> +#define HI6220_TEMP0_INT_EN			(0x14)
> +#define HI6220_TEMP0_INT_CLR			(0x18)
> +#define HI6220_TEMP0_RST_MSK			(0x1C)
> +#define HI6220_TEMP0_VALUE			(0x28)
> +
> +#define HI6220_TEMP_BASE			(-60000)
> +#define HI6220_TEMP_RESET			(100000)
> +#define HI6220_TEMP_STEP			(785)
> +#define HI6220_TEMP_LAG			(3500)
> +
> +#define HI6220_DEFAULT_SENSOR		2
>  
>  struct hisi_thermal_sensor {
>  	struct thermal_zone_device *tzd;
> @@ -78,14 +77,14 @@ struct hisi_thermal_data {
>   *	steps = (Temp - TempBase) / 785
>   *
>   */
> -static inline int hisi_thermal_step_to_temp(int step)
> +static inline int hi6220_thermal_step_to_temp(int step)
>  {
> -	return HISI_TEMP_BASE + (step * HISI_TEMP_STEP);
> +	return HI6220_TEMP_BASE + (step * HI6220_TEMP_STEP);
>  }
>  
> -static inline int hisi_thermal_temp_to_step(int temp)
> +static inline int hi6220_thermal_temp_to_step(int temp)
>  {
> -	return DIV_ROUND_UP(temp - HISI_TEMP_BASE, HISI_TEMP_STEP);
> +	return DIV_ROUND_UP(temp - HI6220_TEMP_BASE, HI6220_TEMP_STEP);
>  }
>  
>  /*
> @@ -112,51 +111,53 @@ static inline int hisi_thermal_temp_to_step(int temp)
>   *
>   * [0:4] : lag register
>   *
> - * The temperature is coded in steps, cf. HISI_TEMP_STEP.
> + * The temperature is coded in steps, cf. HI6220_TEMP_STEP.
>   *
>   * Min : 0x00 :  0.0 °C
>   * Max : 0x1F : 24.3 °C
>   *
>   * The 'value' parameter is in milliCelsius.
>   */
> -static inline void hisi_thermal_set_lag(void __iomem *addr, int value)
> +static inline void hi6220_thermal_set_lag(void __iomem *addr, int value)
>  {
> -	writel(DIV_ROUND_UP(value, HISI_TEMP_STEP) & 0x1F, addr + TEMP0_LAG);
> +	writel(DIV_ROUND_UP(value, HI6220_TEMP_STEP) & 0x1F,
> +			addr + HI6220_TEMP0_LAG);
>  }
>  
> -static inline void hisi_thermal_alarm_clear(void __iomem *addr, int value)
> +static inline void hi6220_thermal_alarm_clear(void __iomem *addr, int value)
>  {
> -	writel(value, addr + TEMP0_INT_CLR);
> +	writel(value, addr + HI6220_TEMP0_INT_CLR);
>  }
>  
> -static inline void hisi_thermal_alarm_enable(void __iomem *addr, int value)
> +static inline void hi6220_thermal_alarm_enable(void __iomem *addr, int value)
>  {
> -	writel(value, addr + TEMP0_INT_EN);
> +	writel(value, addr + HI6220_TEMP0_INT_EN);
>  }
>  
> -static inline void hisi_thermal_alarm_set(void __iomem *addr, int temp)
> +static inline void hi6220_thermal_alarm_set(void __iomem *addr, int temp)
>  {
> -	writel(hisi_thermal_temp_to_step(temp) | 0x0FFFFFF00, addr + TEMP0_TH);
> +	writel(hi6220_thermal_temp_to_step(temp) | 0x0FFFFFF00,
> +			addr + HI6220_TEMP0_TH);
>  }
>  
> -static inline void hisi_thermal_reset_set(void __iomem *addr, int temp)
> +static inline void hi6220_thermal_reset_set(void __iomem *addr, int temp)
>  {
> -	writel(hisi_thermal_temp_to_step(temp), addr + TEMP0_RST_TH);
> +	writel(hi6220_thermal_temp_to_step(temp), addr + HI6220_TEMP0_RST_TH);
>  }
>  
> -static inline void hisi_thermal_reset_enable(void __iomem *addr, int value)
> +static inline void hi6220_thermal_reset_enable(void __iomem *addr, int value)
>  {
> -	writel(value, addr + TEMP0_RST_MSK);
> +	writel(value, addr + HI6220_TEMP0_RST_MSK);
>  }
>  
> -static inline void hisi_thermal_enable(void __iomem *addr, int value)
> +static inline void hi6220_thermal_enable(void __iomem *addr, int value)
>  {
> -	writel(value, addr + TEMP0_EN);
> +	writel(value, addr + HI6220_TEMP0_EN);
>  }
>  
> -static inline int hisi_thermal_get_temperature(void __iomem *addr)
> +static inline int hi6220_thermal_get_temperature(void __iomem *addr)
>  {
> -	return hisi_thermal_step_to_temp(readl(addr + TEMP0_VALUE));
> +	return hi6220_thermal_step_to_temp(readl(addr + HI6220_TEMP0_VALUE));
>  }
>  
>  /*
> @@ -169,10 +170,10 @@ static inline int hisi_thermal_get_temperature(void __iomem *addr)
>   * 0x2: remote sensor 2 (ACPU cluster 0)
>   * 0x3: remote sensor 3 (G3D)
>   */
> -static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
> +static inline void hi6220_thermal_sensor_select(void __iomem *addr, int sensor)
>  {
> -	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_SS_MSK ) |
> -	       (sensor << 12), addr + TEMP0_CFG);
> +	writel((readl(addr + HI6220_TEMP0_CFG) & ~HI6220_TEMP0_CFG_SS_MSK) |
> +	       (sensor << 12), addr + HI6220_TEMP0_CFG);
>  }
>  
>  /*
> @@ -185,24 +186,24 @@ static inline void hisi_thermal_sensor_select(void __iomem *addr, int sensor)
>   * 0x2 :  49.152 ms
>   * 0x3 : 393.216 ms
>   */
> -static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
> +static inline void hi6220_thermal_hdak_set(void __iomem *addr, int value)
>  {
> -	writel((readl(addr + TEMP0_CFG) & ~TEMP0_CFG_HDAK_MSK) |
> -	       (value << 4), addr + TEMP0_CFG);
> +	writel((readl(addr + HI6220_TEMP0_CFG) & ~HI6220_TEMP0_CFG_HDAK_MSK) |
> +	       (value << 4), addr + HI6220_TEMP0_CFG);
>  }
>  
> -static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
> +static void hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
>  {
>  	/* disable sensor module */
> -	hisi_thermal_enable(data->regs, 0);
> -	hisi_thermal_alarm_enable(data->regs, 0);
> -	hisi_thermal_reset_enable(data->regs, 0);
> +	hi6220_thermal_enable(data->regs, 0);
> +	hi6220_thermal_alarm_enable(data->regs, 0);
> +	hi6220_thermal_reset_enable(data->regs, 0);
>  
>  	clk_disable_unprepare(data->clk);
>  }
>  
>  
> -static int hisi_thermal_setup(struct hisi_thermal_data *data)
> +static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
>  {
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
>  	int ret;
> @@ -213,29 +214,29 @@ static int hisi_thermal_setup(struct hisi_thermal_data *data)
>  		return ret;
>  
>  	/* disable module firstly */
> -	hisi_thermal_reset_enable(data->regs, 0);
> -	hisi_thermal_enable(data->regs, 0);
> +	hi6220_thermal_reset_enable(data->regs, 0);
> +	hi6220_thermal_enable(data->regs, 0);
>  
>  	/* select sensor id */
> -	hisi_thermal_sensor_select(data->regs, sensor->id);
> +	hi6220_thermal_sensor_select(data->regs, sensor->id);
>  
>  	/* setting the hdak time */
> -	hisi_thermal_hdak_set(data->regs, 0);
> +	hi6220_thermal_hdak_set(data->regs, 0);
>  
>  	/* setting lag value between current temp and the threshold */
> -	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
> +	hi6220_thermal_set_lag(data->regs, HI6220_TEMP_LAG);
>  
>  	/* enable for interrupt */
> -	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
> +	hi6220_thermal_alarm_set(data->regs, sensor->thres_temp);
>  
> -	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
> +	hi6220_thermal_reset_set(data->regs, HI6220_TEMP_RESET);
>  
>  	/* enable module */
> -	hisi_thermal_reset_enable(data->regs, 1);
> -	hisi_thermal_enable(data->regs, 1);
> +	hi6220_thermal_reset_enable(data->regs, 1);
> +	hi6220_thermal_enable(data->regs, 1);
>  
> -	hisi_thermal_alarm_clear(data->regs, 0);
> -	hisi_thermal_alarm_enable(data->regs, 1);
> +	hi6220_thermal_alarm_clear(data->regs, 0);
> +	hi6220_thermal_alarm_enable(data->regs, 1);
>  
>  	return 0;
>  }
> @@ -244,7 +245,7 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
>  	struct hisi_thermal_data *data = __data;
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
>  
> -	*temp = hisi_thermal_get_temperature(data->regs);
> +	*temp = hi6220_thermal_get_temperature(data->regs);
>  
>  	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
>  		sensor->id, *temp, sensor->thres_temp);
> @@ -260,11 +261,11 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  {
>  	struct hisi_thermal_data *data = dev;
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
> -	int temp;
> +	int temp = 0;
>  
> -	hisi_thermal_alarm_clear(data->regs, 1);
> +	hi6220_thermal_alarm_clear(data->regs, 1);
>  
> -	temp = hisi_thermal_get_temperature(data->regs);
> +	hisi_thermal_get_temp(data, &temp);
>  
>  	if (temp >= sensor->thres_temp) {
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
> @@ -273,7 +274,7 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  		thermal_zone_device_update(data->sensor.tzd,
>  					   THERMAL_EVENT_UNSPECIFIED);
>  
> -	} else if (temp < sensor->thres_temp) {
> +	} else {
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
>  			 temp, sensor->thres_temp);
>  	}
> @@ -365,14 +366,14 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  
>  	ret = hisi_thermal_register_sensor(pdev, data,
>  					   &data->sensor,
> -					   HISI_DEFAULT_SENSOR);
> +					   HI6220_DEFAULT_SENSOR);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
>  			ret);
>  		return ret;
>  	}
>  
> -	ret = hisi_thermal_setup(data);
> +	ret = hi6220_thermal_enable_sensor(data);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to setup the sensor: %d\n", ret);
>  		return ret;
> @@ -397,7 +398,7 @@ static int hisi_thermal_remove(struct platform_device *pdev)
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
>  
>  	hisi_thermal_toggle_sensor(sensor, false);
> -	hisi_thermal_disable_sensor(data);
> +	hi6220_thermal_disable_sensor(data);
>  
>  	return 0;
>  }
> @@ -407,7 +408,7 @@ static int hisi_thermal_suspend(struct device *dev)
>  {
>  	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>  
> -	hisi_thermal_disable_sensor(data);
> +	hi6220_thermal_disable_sensor(data);
>  
>  	return 0;
>  }
> @@ -416,7 +417,7 @@ static int hisi_thermal_resume(struct device *dev)
>  {
>  	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>  
> -	return hisi_thermal_setup(data);
> +	return hi6220_thermal_enable_sensor(data);
>  }
>  #endif
>  
> -- 
> 2.7.4
> 

Re: [PATCH 19/25] thermal/drivers/hisi: Put platform code together

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:44PM +0200, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> Reorganize the code for next patches by moving the functions upper in
> the file which will prevent a forward declaration. There is no functional
> change here.
> 


CHECK: Please don't use multiple blank lines
#104: FILE: drivers/thermal/hisi_thermal.c:204:
 
 +

 total: 0 errors, 0 warnings, 1 checks, 88 lines checked


> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 76 +++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index befdb28..ff9055a 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -201,6 +201,44 @@ static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>  	clk_disable_unprepare(data->clk);
>  }
>  
> +
> +static int hisi_thermal_setup(struct hisi_thermal_data *data)
> +{
> +	struct hisi_thermal_sensor *sensor = &data->sensor;
> +	int ret;
> +
> +	/* enable clock for tsensor */
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* disable module firstly */
> +	hisi_thermal_reset_enable(data->regs, 0);
> +	hisi_thermal_enable(data->regs, 0);
> +
> +	/* select sensor id */
> +	hisi_thermal_sensor_select(data->regs, sensor->id);
> +
> +	/* setting the hdak time */
> +	hisi_thermal_hdak_set(data->regs, 0);
> +
> +	/* setting lag value between current temp and the threshold */
> +	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
> +
> +	/* enable for interrupt */
> +	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
> +
> +	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
> +
> +	/* enable module */
> +	hisi_thermal_reset_enable(data->regs, 1);
> +	hisi_thermal_enable(data->regs, 1);
> +
> +	hisi_thermal_alarm_clear(data->regs, 0);
> +	hisi_thermal_alarm_enable(data->regs, 1);
> +
> +	return 0;
> +}
>  static int hisi_thermal_get_temp(void *__data, int *temp)
>  {
>  	struct hisi_thermal_data *data = __data;
> @@ -291,44 +329,6 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>  		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>  }
>  
> -static int hisi_thermal_setup(struct hisi_thermal_data *data)
> -{
> -	struct hisi_thermal_sensor *sensor = &data->sensor;
> -	int ret;
> -
> -	/* enable clock for tsensor */
> -	ret = clk_prepare_enable(data->clk);
> -	if (ret)
> -		return ret;
> -
> -	/* disable module firstly */
> -	hisi_thermal_reset_enable(data->regs, 0);
> -	hisi_thermal_enable(data->regs, 0);
> -
> -	/* select sensor id */
> -	hisi_thermal_sensor_select(data->regs, sensor->id);
> -
> -	/* setting the hdak time */
> -	hisi_thermal_hdak_set(data->regs, 0);
> -
> -	/* setting lag value between current temp and the threshold */
> -	hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG);
> -
> -	/* enable for interrupt */
> -	hisi_thermal_alarm_set(data->regs, sensor->thres_temp);
> -
> -	hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET);
> -
> -	/* enable module */
> -	hisi_thermal_reset_enable(data->regs, 1);
> -	hisi_thermal_enable(data->regs, 1);
> -
> -	hisi_thermal_alarm_clear(data->regs, 0);
> -	hisi_thermal_alarm_enable(data->regs, 1);
> -
> -	return 0;
> -}
> -
>  static int hisi_thermal_probe(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data;
> -- 
> 2.7.4
> 

Re: [PATCH 22/25] thermal/drivers/hisi: Add support for multi temp threshold

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:47PM +0200, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> The next patches will provide the support for the hi3660 where the temperature
> sensor can have multiple alarm levels. In order to set the scene to support it,
> we have to convert the current code to be able to support multiple threshold
> values.
> 
> [Daniel Lezcano: Restated the log]

CHECK: Prefer kernel type 'u32' over 'uint32_t'
#113: FILE: drivers/thermal/hisi_thermal.c:54:
+	uint32_t thres_temp[MAX_THRES_NUM];

total: 0 errors, 1 warnings, 1 checks, 67 lines checked



> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index b5a7159..e87ca6c 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -46,10 +46,12 @@
>  
>  #define HI6220_DEFAULT_SENSOR		2
>  
> +#define MAX_THRES_NUM			2
> +
>  struct hisi_thermal_sensor {
>  	struct thermal_zone_device *tzd;
>  	uint32_t id;
> -	uint32_t thres_temp;
> +	uint32_t thres_temp[MAX_THRES_NUM];
>  };
>  
>  struct hisi_thermal_data {
> @@ -244,7 +246,7 @@ static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
>  	hi6220_thermal_set_lag(data->regs, HI6220_TEMP_LAG);
>  
>  	/* enable for interrupt */
> -	hi6220_thermal_alarm_set(data->regs, sensor->thres_temp);
> +	hi6220_thermal_alarm_set(data->regs, sensor->thres_temp[0]);
>  
>  	hi6220_thermal_reset_set(data->regs, HI6220_TEMP_RESET);
>  
> @@ -303,7 +305,7 @@ static int hisi_thermal_get_temp(void *__data, int *temp)
>  	*temp = data->get_temp(data);
>  
>  	dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n",
> -		sensor->id, *temp, sensor->thres_temp);
> +		sensor->id, *temp, sensor->thres_temp[0]);
>  
>  	return 0;
>  }
> @@ -322,16 +324,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  
>  	hisi_thermal_get_temp(data, &temp);
>  
> -	if (temp >= sensor->thres_temp) {
> +	if (temp >= sensor->thres_temp[0]) {
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n",
> -			 temp, sensor->thres_temp);
> +			 temp, sensor->thres_temp[0]);
>  
>  		thermal_zone_device_update(data->sensor.tzd,
>  					   THERMAL_EVENT_UNSPECIFIED);
>  
>  	} else {
>  		dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n",
> -			 temp, sensor->thres_temp);
> +			 temp, sensor->thres_temp[0]);
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -341,7 +343,7 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>  					struct hisi_thermal_data *data,
>  					struct hisi_thermal_sensor *sensor)
>  {
> -	int ret, i;
> +	int ret, i, thres_idx = 0;
>  	const struct thermal_trip *trip;
>  
>  	sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
> @@ -359,8 +361,9 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>  
>  	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>  		if (trip[i].type == THERMAL_TRIP_PASSIVE) {
> -			sensor->thres_temp = trip[i].temperature;
> -			break;
> +			sensor->thres_temp[thres_idx++] = trip[i].temperature;
> +			if (thres_idx >= MAX_THRES_NUM)
> +				break;
>  		}
>  	}
>  
> -- 
> 2.7.4
> 

Re: [PATCH 24/25] thermal/drivers/hisi: Add support for hi3660 SoC

[Eduardo Valentin]

On Tue, Oct 10, 2017 at 08:02:49PM +0200, Daniel Lezcano wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> This patch adds the support for thermal sensor on the Hi3660 SoC.
> Hi3660 tsensor support alarm interrupt and have three configurable
> alarm thresholds, it also has a configurable hysteresis interval,
> interrupt will be triggered when temperature rise above the alarm
> threshold or fall below the hysteresis threshold.

Minor:


CHECK: Alignment should match open parenthesis
#173: FILE: drivers/thermal/hisi_thermal.c:214:
+	writel(DIV_ROUND_UP(value, HI3660_TEMP_STEP) & 0x7F,
+			addr + HI3660_LAG(id));

total: 0 errors, 0 warnings, 1 checks, 205 lines checked

> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/hisi_thermal.c | 146 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 145 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index e87ca6c..10276f0 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -39,12 +39,24 @@
>  #define HI6220_TEMP0_RST_MSK			(0x1C)
>  #define HI6220_TEMP0_VALUE			(0x28)
>  
> +#define HI3660_OFFSET(chan)		((chan) * 0x40)
> +#define HI3660_TEMP(chan)		(HI3660_OFFSET(chan) + 0x1C)
> +#define HI3660_TH(chan)			(HI3660_OFFSET(chan) + 0x20)
> +#define HI3660_LAG(chan)		(HI3660_OFFSET(chan) + 0x28)
> +#define HI3660_INT_EN(chan)		(HI3660_OFFSET(chan) + 0x2C)
> +#define HI3660_INT_CLR(chan)		(HI3660_OFFSET(chan) + 0x30)
> +
>  #define HI6220_TEMP_BASE			(-60000)
>  #define HI6220_TEMP_RESET			(100000)
>  #define HI6220_TEMP_STEP			(785)
>  #define HI6220_TEMP_LAG				(3500)
>  
> +#define HI3660_TEMP_BASE		(-63780)
> +#define HI3660_TEMP_STEP		(205)
> +#define HI3660_TEMP_LAG			(4000)
> +
>  #define HI6220_DEFAULT_SENSOR		2
> +#define HI3660_DEFAULT_SENSOR		1
>  
>  #define MAX_THRES_NUM			2
>  
> @@ -96,6 +108,24 @@ static inline int hi6220_thermal_temp_to_step(int temp)
>  }
>  
>  /*
> + * for Hi3660,
> + *	Step: 189/922 (0.205)
> + *	Temperature base: -63.780°C
> + *
> + * The register is programmed in temperature steps, every step is 205
> + * millidegree and begins at -63 780 m°C
> + */
> +static inline int hi3660_thermal_step_to_temp(int step)
> +{
> +	return HI3660_TEMP_BASE + step * HI3660_TEMP_STEP;
> +}
> +
> +static inline int hi3660_thermal_temp_to_step(int temp)
> +{
> +	return DIV_ROUND_UP(temp - HI3660_TEMP_BASE, HI3660_TEMP_STEP);
> +}
> +
> +/*
>   * The lag register contains 5 bits encoding the temperature in steps.
>   *
>   * Each time the temperature crosses the threshold boundary, an
> @@ -169,6 +199,45 @@ static inline int hi6220_thermal_get_temperature(void __iomem *addr)
>  }
>  
>  /*
> + * [0:6] lag register
> + *
> + * The temperature is coded in steps, cf. HI3660_TEMP_STEP.
> + *
> + * Min : 0x00 :  0.0 °C
> + * Max : 0x7F : 26.0 °C
> + *
> + */
> +static inline void hi3660_thermal_set_lag(void __iomem *addr,
> +					  int id, int value)
> +{
> +	writel(DIV_ROUND_UP(value, HI3660_TEMP_STEP) & 0x7F,
> +			addr + HI3660_LAG(id));
> +}
> +
> +static inline void hi3660_thermal_alarm_clear(void __iomem *addr,
> +					      int id, int value)
> +{
> +	writel(value, addr + HI3660_INT_CLR(id));
> +}
> +
> +static inline void hi3660_thermal_alarm_enable(void __iomem *addr,
> +					       int id, int value)
> +{
> +	writel(value, addr + HI3660_INT_EN(id));
> +}
> +
> +static inline void hi3660_thermal_alarm_set(void __iomem *addr,
> +					    int id, int value)
> +{
> +	writel(value, addr + HI3660_TH(id));
> +}
> +
> +static inline int hi3660_thermal_get_temperature(void __iomem *addr, int id)
> +{
> +	return hi3660_thermal_step_to_temp(readl(addr + HI3660_TEMP(id)));
> +}
> +
> +/*
>   * Temperature configuration register - Sensor selection
>   *
>   * Bits [19:12]
> @@ -206,11 +275,22 @@ static int hi6220_thermal_irq_handler(struct hisi_thermal_data *data)
>  	return 0;
>  }
>  
> +static int hi3660_thermal_irq_handler(struct hisi_thermal_data *data)
> +{
> +	hi3660_thermal_alarm_clear(data->regs, data->sensor.id, 1);
> +	return 0;
> +}
> +
>  static int hi6220_thermal_get_temp(struct hisi_thermal_data *data)
>  {
>  	return hi6220_thermal_get_temperature(data->regs);
>  }
>  
> +static int hi3660_thermal_get_temp(struct hisi_thermal_data *data)
> +{
> +	return hi3660_thermal_get_temperature(data->regs, data->sensor.id);
> +}
> +
>  static int hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
>  {
>  	/* disable sensor module */
> @@ -222,6 +302,13 @@ static int hi6220_thermal_disable_sensor(struct hisi_thermal_data *data)
>  	return 0;
>  }
>  
> +static int hi3660_thermal_disable_sensor(struct hisi_thermal_data *data)
> +{
> +	/* disable sensor module */
> +	hi3660_thermal_alarm_enable(data->regs, data->sensor.id, 0);
> +	return 0;
> +}
> +
>  static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
>  {
>  	struct hisi_thermal_sensor *sensor = &data->sensor;
> @@ -260,6 +347,29 @@ static int hi6220_thermal_enable_sensor(struct hisi_thermal_data *data)
>  	return 0;
>  }
>  
> +static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
> +{
> +	unsigned int value;
> +	struct hisi_thermal_sensor *sensor = &data->sensor;
> +
> +	/* disable interrupt */
> +	hi3660_thermal_alarm_enable(data->regs, sensor->id, 0);
> +
> +	/* setting lag value between current temp and the threshold */
> +	hi3660_thermal_set_lag(data->regs, sensor->id, HI3660_TEMP_LAG);
> +
> +	/* set interrupt threshold */
> +	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
> +	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
> +	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
> +
> +	/* enable interrupt */
> +	hi3660_thermal_alarm_clear(data->regs, sensor->id, 1);
> +	hi3660_thermal_alarm_enable(data->regs, sensor->id, 1);
> +
> +	return 0;
> +}
> +
>  static int hi6220_thermal_probe(struct hisi_thermal_data *data)
>  {
>  	struct platform_device *pdev = data->pdev;
> @@ -296,6 +406,33 @@ static int hi6220_thermal_probe(struct hisi_thermal_data *data)
>  	return 0;
>  }
>  
> +static int hi3660_thermal_probe(struct hisi_thermal_data *data)
> +{
> +	struct platform_device *pdev = data->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +
> +	data->get_temp = hi3660_thermal_get_temp;
> +	data->enable_sensor = hi3660_thermal_enable_sensor;
> +	data->disable_sensor = hi3660_thermal_disable_sensor;
> +	data->irq_handler = hi3660_thermal_irq_handler;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(dev, "failed to get io address\n");
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	data->irq = platform_get_irq(pdev, 0);
> +	if (data->irq < 0)
> +		return data->irq;
> +
> +	data->sensor.id = HI3660_DEFAULT_SENSOR;
> +
> +	return 0;
> +}
> +
>  
>  static int hisi_thermal_get_temp(void *__data, int *temp)
>  {
> @@ -371,7 +508,14 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>  }
>  
>  static const struct of_device_id of_hisi_thermal_match[] = {
> -	{ .compatible = "hisilicon,tsensor", .data = hi6220_thermal_probe },
> +	{
> +		.compatible = "hisilicon,tsensor",
> +		.data = hi6220_thermal_probe
> +	},
> +	{
> +		.compatible = "hisilicon,hi3660-tsensor",
> +		.data = hi3660_thermal_probe
> +	},
>  	{ /* end */ }
>  };
>  MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
> -- 
> 2.7.4
> 

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 17/10/2017 05:54, Eduardo Valentin wrote:
> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>> By essence, the tsensor does not really support multiple sensor at the same
>> time. It allows to set a sensor and use it to get the temperature, another
>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>> simultaneously several sensors without a big delay.
>>
> 
> Is 3-5 ms enough to loose an event? Is this really a problem?

There are several aspects:

 - the multiple sensors is not needed here

 - the temperature controller is not designed to read several sensors at
the same time, we switch the sensor and that clears some internal
buffers and re-init the controller

 - some boards can take 40°C in 1 sec, the temperature increase is
insanely fast and reading several sensors add an extra 15ms.

So from my point of view, trying to read multiple sensors with *this*
tsensor is pointless.

>> Today, just one sensor is used, it is not necessary to deal with multiple
> 
> How come? Today the driver exposes all sensors to userspace. Removing
> them, means you are changing the amount of sensors userspace really
> sees.
> 
> Are you sure about this?
> 
> Can you please elaborate how we are only using one of the four sensors?

Only one has been defined in the DT since the beginning and no one is
using multiple sensors.

>> sensors in the code. Remove them and if it is needed in the future add them
>> on top of a code which will be clean up in the meantime.
> 
> It is just not clear to me why having 1 sensor support is better than
> having 4, as per the hw supported tsensor.

The tsensor supports 4 sensors but not at the same time. You choose one
and use it, AFAICS from the documentation and behavior.

If multiple sensors is needed later, I will be happy to re-introduce it
on top of a sanitized driver.

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> I would prefer to get confirmation from folks from hisilicon here.
> 
> BTW, you should be copying previous author of the code.

Hmm, I thought Leo was in copy (added). Kevin is from hisilicon and in
charge of the thermal aspect of the hikey.





-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Eduardo Valentin]

Hello,

On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> On 17/10/2017 05:54, Eduardo Valentin wrote:
> > On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> >> By essence, the tsensor does not really support multiple sensor at the same
> >> time. It allows to set a sensor and use it to get the temperature, another
> >> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> >> simultaneously several sensors without a big delay.
> >>
> > 
> > Is 3-5 ms enough to loose an event? Is this really a problem?
> 
> There are several aspects:
> 
>  - the multiple sensors is not needed here

Well, that is debatable, I cannot really agree or disagree with the
above statement without understanding the use cases and most important,
the location of each sensor. What is the location of each sensor?

> 
>  - the temperature controller is not designed to read several sensors at
> the same time, we switch the sensor and that clears some internal
> buffers and re-init the controller

Which is still very helpful in case you have multiple hotspots that you
want to track and they are exposed on different workloads. Sacrificing
the availability of sensors is something needs a better justification
other than "current code uses only one".


> 
>  - some boards can take 40°C in 1 sec, the temperature increase is
> insanely fast and reading several sensors add an extra 15ms.
> 


Ok... What is the difference in update rate with and without the switch
of sensors? With the above worst case, you have about 4/6 mC/ms. Can
your tsensor support that resolution for a single sensor? What is the
maximum resolution a tsensor can support? What is the penalty added with
switch?

Based on this data, and the above 3-5ms, that  means you would miss about
~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
enough justification to drop three extra sensors?

> So from my point of view, trying to read multiple sensors with *this*
> tsensor is pointless.
> 
> >> Today, just one sensor is used, it is not necessary to deal with multiple
> > 
> > How come? Today the driver exposes all sensors to userspace. Removing
> > them, means you are changing the amount of sensors userspace really
> > sees.
> > 
> > Are you sure about this?
> > 
> > Can you please elaborate how we are only using one of the four sensors?
> 
> Only one has been defined in the DT since the beginning and no one is
> using multiple sensors.
> 
> >> sensors in the code. Remove them and if it is needed in the future add them
> >> on top of a code which will be clean up in the meantime.
> > 
> > It is just not clear to me why having 1 sensor support is better than
> > having 4, as per the hw supported tsensor.
> 
> The tsensor supports 4 sensors but not at the same time. You choose one
> and use it, AFAICS from the documentation and behavior.
> 
> If multiple sensors is needed later, I will be happy to re-introduce it
> on top of a sanitized driver.

Once again, having one at time is better than only one, depending on
sensor location and usage of the chip. Say you have two sensors, one at
CPU domain another at a coprocessor, say a DSP. The representation of
the CPU sensor is typically not representative of the DSP, and
vice-versa, even though one may still collect a few of the heat of what
the other detects first. Sometimes takes several hundreds of ms  
(sometimes seconds) to see heat wave from one to the other. Having both,
but the reads one at a time, may give you the chance to see a heat
increase on one side (say DSP) within 5 ms time frame (assuming your
reported worst case above), faster than physically waiting the heat
to really reach the other sensor (say CPU) after several hundreds of
ms (sometimes seconds).


> 
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 
> > I would prefer to get confirmation from folks from hisilicon here.
> > 
> > BTW, you should be copying previous author of the code.
> 
> Hmm, I thought Leo was in copy (added). Kevin is from hisilicon and in
> charge of the thermal aspect of the hikey.
> 
> 
> 
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 17/10/2017 20:25, Eduardo Valentin wrote:
> Hello,
> 
> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>> simultaneously several sensors without a big delay.
>>>>
>>>
>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>
>> There are several aspects:
>>
>>  - the multiple sensors is not needed here
> 
> Well, that is debatable, I cannot really agree or disagree with the
> above statement without understanding the use cases and most important,
> the location of each sensor. What is the location of each sensor?
> 
>>
>>  - the temperature controller is not designed to read several sensors at
>> the same time, we switch the sensor and that clears some internal
>> buffers and re-init the controller
> 
> Which is still very helpful in case you have multiple hotspots that you
> want to track and they are exposed on different workloads. Sacrificing
> the availability of sensors is something needs a better justification
> other than "current code uses only one".
> 
> 
>>
>>  - some boards can take 40°C in 1 sec, the temperature increase is
>> insanely fast and reading several sensors add an extra 15ms.
>>
> 
> 
> Ok... What is the difference in update rate with and without the switch
> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> your tsensor support that resolution for a single sensor? What is the
> maximum resolution a tsensor can support? What is the penalty added with
> switch?
> 
> Based on this data, and the above 3-5ms, that  means you would miss about
> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> enough justification to drop three extra sensors?

Ok if I refer to the documentation the rate is 0.768 ms with the current
configuration.

The driver is currently bogus: register overwritten, bouncing interrupt,
unneeded lock, ... So the proposition was to remove the multiple sensors
support, clean the driver, and re-introduce it if there is a need.

If I remember correctly Leo, author of the driver, agreed on this. Leo ?

Note, I'm not strongly against multiple sensors support in the driver if
you think it is convenient but it is much simpler to remove the current
code as it is not used and put it back on top of a sane foundation
instead of circumventing that on the existing code.





-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Eduardo Valentin]

On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
> On 17/10/2017 20:25, Eduardo Valentin wrote:
> > Hello,
> > 
> > On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> >> On 17/10/2017 05:54, Eduardo Valentin wrote:
> >>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> >>>> By essence, the tsensor does not really support multiple sensor at the same
> >>>> time. It allows to set a sensor and use it to get the temperature, another
> >>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> >>>> simultaneously several sensors without a big delay.
> >>>>
> >>>
> >>> Is 3-5 ms enough to loose an event? Is this really a problem?
> >>
> >> There are several aspects:
> >>
> >>  - the multiple sensors is not needed here
> > 
> > Well, that is debatable, I cannot really agree or disagree with the
> > above statement without understanding the use cases and most important,
> > the location of each sensor. What is the location of each sensor?
> > 
> >>
> >>  - the temperature controller is not designed to read several sensors at
> >> the same time, we switch the sensor and that clears some internal
> >> buffers and re-init the controller
> > 
> > Which is still very helpful in case you have multiple hotspots that you
> > want to track and they are exposed on different workloads. Sacrificing
> > the availability of sensors is something needs a better justification
> > other than "current code uses only one".
> > 
> > 
> >>
> >>  - some boards can take 40°C in 1 sec, the temperature increase is
> >> insanely fast and reading several sensors add an extra 15ms.
> >>
> > 
> > 
> > Ok... What is the difference in update rate with and without the switch
> > of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> > your tsensor support that resolution for a single sensor? What is the
> > maximum resolution a tsensor can support? What is the penalty added with
> > switch?
> > 
> > Based on this data, and the above 3-5ms, that  means you would miss about
> > ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> > above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> > enough justification to drop three extra sensors?
> 
> Ok if I refer to the documentation the rate is 0.768 ms with the current
> configuration.
> 
> The driver is currently bogus: register overwritten, bouncing interrupt,
> unneeded lock, ... So the proposition was to remove the multiple sensors
> support, clean the driver, and re-introduce it if there is a need.
> 
> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
> 
> Note, I'm not strongly against multiple sensors support in the driver if
> you think it is convenient but it is much simpler to remove the current
> code as it is not used and put it back on top of a sane foundation
> instead of circumventing that on the existing code.
> 
> 

I am also fine with the above strategy, as long as you are sure you are
not breaking anyone (specially userspace). Also, it would be good to get
a reviewed-by from hisilicon just to confirm (Leo?).

Besides, once you get his reviewed-by, and add it to the patches,
can you please resend the series with the minor issues I
mentioned (a few minor checkpatch issues and one compilation warn that
is added to the driver after the series is applied).

> 
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 17/10/2017 23:07, Eduardo Valentin wrote:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>> Hello,
>>>
>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>> simultaneously several sensors without a big delay.
>>>>>>
>>>>>
>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>
>>>> There are several aspects:
>>>>
>>>>  - the multiple sensors is not needed here
>>>
>>> Well, that is debatable, I cannot really agree or disagree with the
>>> above statement without understanding the use cases and most important,
>>> the location of each sensor. What is the location of each sensor?
>>>
>>>>
>>>>  - the temperature controller is not designed to read several sensors at
>>>> the same time, we switch the sensor and that clears some internal
>>>> buffers and re-init the controller
>>>
>>> Which is still very helpful in case you have multiple hotspots that you
>>> want to track and they are exposed on different workloads. Sacrificing
>>> the availability of sensors is something needs a better justification
>>> other than "current code uses only one".
>>>
>>>
>>>>
>>>>  - some boards can take 40°C in 1 sec, the temperature increase is
>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>
>>>
>>>
>>> Ok... What is the difference in update rate with and without the switch
>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>> your tsensor support that resolution for a single sensor? What is the
>>> maximum resolution a tsensor can support? What is the penalty added with
>>> switch?
>>>
>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>> enough justification to drop three extra sensors?
>>
>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>> configuration.
>>
>> The driver is currently bogus: register overwritten, bouncing interrupt,
>> unneeded lock, ... So the proposition was to remove the multiple sensors
>> support, clean the driver, and re-introduce it if there is a need.
>>
>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>
>> Note, I'm not strongly against multiple sensors support in the driver if
>> you think it is convenient but it is much simpler to remove the current
>> code as it is not used and put it back on top of a sane foundation
>> instead of circumventing that on the existing code.
>>
>>
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).
> 
> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).

Yes, sure. I'm on it.

I will send it tomorrow.

Thanks.

  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Leo Yan]

On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
> > On 17/10/2017 20:25, Eduardo Valentin wrote:
> > > Hello,
> > > 
> > > On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> > >> On 17/10/2017 05:54, Eduardo Valentin wrote:
> > >>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> > >>>> By essence, the tsensor does not really support multiple sensor at the same
> > >>>> time. It allows to set a sensor and use it to get the temperature, another
> > >>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> > >>>> simultaneously several sensors without a big delay.
> > >>>>
> > >>>
> > >>> Is 3-5 ms enough to loose an event? Is this really a problem?
> > >>
> > >> There are several aspects:
> > >>
> > >>  - the multiple sensors is not needed here
> > > 
> > > Well, that is debatable, I cannot really agree or disagree with the
> > > above statement without understanding the use cases and most important,
> > > the location of each sensor. What is the location of each sensor?
> > > 
> > >>
> > >>  - the temperature controller is not designed to read several sensors at
> > >> the same time, we switch the sensor and that clears some internal
> > >> buffers and re-init the controller
> > > 
> > > Which is still very helpful in case you have multiple hotspots that you
> > > want to track and they are exposed on different workloads. Sacrificing
> > > the availability of sensors is something needs a better justification
> > > other than "current code uses only one".
> > > 
> > > 
> > >>
> > >>  - some boards can take 40°C in 1 sec, the temperature increase is
> > >> insanely fast and reading several sensors add an extra 15ms.
> > >>
> > > 
> > > 
> > > Ok... What is the difference in update rate with and without the switch
> > > of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> > > your tsensor support that resolution for a single sensor? What is the
> > > maximum resolution a tsensor can support? What is the penalty added with
> > > switch?
> > > 
> > > Based on this data, and the above 3-5ms, that  means you would miss about
> > > ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> > > above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> > > enough justification to drop three extra sensors?
> > 
> > Ok if I refer to the documentation the rate is 0.768 ms with the current
> > configuration.
> > 
> > The driver is currently bogus: register overwritten, bouncing interrupt,
> > unneeded lock, ... So the proposition was to remove the multiple sensors
> > support, clean the driver, and re-introduce it if there is a need.
> > 
> > If I remember correctly Leo, author of the driver, agreed on this. Leo ?
> > 
> > Note, I'm not strongly against multiple sensors support in the driver if
> > you think it is convenient but it is much simpler to remove the current
> > code as it is not used and put it back on top of a sane foundation
> > instead of circumventing that on the existing code.
> > 
> > 
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).

Sorry I missed to reply this patch. And yes, I have tested and
reviewed it at my side:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

P.s. I am working for Linaro; I am continously co-working with
Hisilicon to maintain this driver due it's important for Hikey/Hikey960
two boards stability; this driver also is important for our daily
profiling for power and performance. Eduardo, so please let us know if
you still need ack from Hisilicon engineer.

> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).
> 
> > 
> > 
> > 
> > -- 
> >  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> > 
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog
> > 

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Wangtao (Kevin, Kirin)]

在 2017/10/18 5:07, Eduardo Valentin 写道:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>> Hello,
>>>
>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>> simultaneously several sensors without a big delay.
>>>>>>
>>>>>
>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>
>>>> There are several aspects:
>>>>
>>>>   - the multiple sensors is not needed here
>>>
>>> Well, that is debatable, I cannot really agree or disagree with the
>>> above statement without understanding the use cases and most important,
>>> the location of each sensor. What is the location of each sensor?
>>>
>>>>
>>>>   - the temperature controller is not designed to read several sensors at
>>>> the same time, we switch the sensor and that clears some internal
>>>> buffers and re-init the controller
>>>
>>> Which is still very helpful in case you have multiple hotspots that you
>>> want to track and they are exposed on different workloads. Sacrificing
>>> the availability of sensors is something needs a better justification
>>> other than "current code uses only one".
>>>
>>>
>>>>
>>>>   - some boards can take 40°C in 1 sec, the temperature increase is
>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>
>>>
>>>
>>> Ok... What is the difference in update rate with and without the switch
>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>> your tsensor support that resolution for a single sensor? What is the
>>> maximum resolution a tsensor can support? What is the penalty added with
>>> switch?
>>>
>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>> enough justification to drop three extra sensors?
>>
>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>> configuration.
>>
>> The driver is currently bogus: register overwritten, bouncing interrupt,
>> unneeded lock, ... So the proposition was to remove the multiple sensors
>> support, clean the driver, and re-introduce it if there is a need.
>>
>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>
>> Note, I'm not strongly against multiple sensors support in the driver if
>> you think it is convenient but it is much simpler to remove the current
>> code as it is not used and put it back on top of a sane foundation
>> instead of circumventing that on the existing code.
>>
>>
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).
I agree with Daniel, we only care about CPU temperature on hikey, and currently
mutiple sensors support are actually not used.
> 
> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).
> 
>>
>>
>>
>> -- 
>>   <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>
> 
> 

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Wangtao (Kevin, Kirin)]

在 2017/10/18 5:07, Eduardo Valentin 写道:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>> Hello,
>>>
>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>> simultaneously several sensors without a big delay.
>>>>>>
>>>>>
>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>
>>>> There are several aspects:
>>>>
>>>>   - the multiple sensors is not needed here
>>>
>>> Well, that is debatable, I cannot really agree or disagree with the
>>> above statement without understanding the use cases and most important,
>>> the location of each sensor. What is the location of each sensor?
>>>
>>>>
>>>>   - the temperature controller is not designed to read several sensors at
>>>> the same time, we switch the sensor and that clears some internal
>>>> buffers and re-init the controller
>>>
>>> Which is still very helpful in case you have multiple hotspots that you
>>> want to track and they are exposed on different workloads. Sacrificing
>>> the availability of sensors is something needs a better justification
>>> other than "current code uses only one".
>>>
>>>
>>>>
>>>>   - some boards can take 40°C in 1 sec, the temperature increase is
>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>
>>>
>>>
>>> Ok... What is the difference in update rate with and without the switch
>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>> your tsensor support that resolution for a single sensor? What is the
>>> maximum resolution a tsensor can support? What is the penalty added with
>>> switch?
>>>
>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>> enough justification to drop three extra sensors?
>>
>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>> configuration.
>>
>> The driver is currently bogus: register overwritten, bouncing interrupt,
>> unneeded lock, ... So the proposition was to remove the multiple sensors
>> support, clean the driver, and re-introduce it if there is a need.
>>
>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>
>> Note, I'm not strongly against multiple sensors support in the driver if
>> you think it is convenient but it is much simpler to remove the current
>> code as it is not used and put it back on top of a sane foundation
>> instead of circumventing that on the existing code.
>>
>>
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).
I agree with Daniel, we only care about CPU temperature on hikey, and currently
mutiple sensors support are actually not used.
> 
> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).
> 
>>
>>
>>
>> -- 
>>   <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>
> 
> 

[PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Tao Wang]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

multi alarm interrupt forced a re-trigger of power_allocator_throttle
which changes the PID's actual sampling rate, this isn't optimal for
IPA, it is best to disable multi alarm support now and sort out this
issue later.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 133238a..3b74c12 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -360,7 +360,6 @@ static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
 
 	/* set interrupt threshold */
 	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
-	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
 	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
 
 	/* enable interrupt */
-- 
2.8.1

[PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Tao Wang]

From: Kevin Wangtao <kevin.wangtao@linaro.org>

multi alarm interrupt forced a re-trigger of power_allocator_throttle
which changes the PID's actual sampling rate, this isn't optimal for
IPA, it is best to disable multi alarm support now and sort out this
issue later.

Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 133238a..3b74c12 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -360,7 +360,6 @@ static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
 
 	/* set interrupt threshold */
 	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
-	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
 	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
 
 	/* enable interrupt */
-- 
2.8.1

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Eduardo Valentin]

On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote:
> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
> > On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
> > > On 17/10/2017 20:25, Eduardo Valentin wrote:
> > > > Hello,
> > > > 
> > > > On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> > > >> On 17/10/2017 05:54, Eduardo Valentin wrote:
> > > >>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> > > >>>> By essence, the tsensor does not really support multiple sensor at the same
> > > >>>> time. It allows to set a sensor and use it to get the temperature, another
> > > >>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> > > >>>> simultaneously several sensors without a big delay.
> > > >>>>
> > > >>>
> > > >>> Is 3-5 ms enough to loose an event? Is this really a problem?
> > > >>
> > > >> There are several aspects:
> > > >>
> > > >>  - the multiple sensors is not needed here
> > > > 
> > > > Well, that is debatable, I cannot really agree or disagree with the
> > > > above statement without understanding the use cases and most important,
> > > > the location of each sensor. What is the location of each sensor?
> > > > 
> > > >>
> > > >>  - the temperature controller is not designed to read several sensors at
> > > >> the same time, we switch the sensor and that clears some internal
> > > >> buffers and re-init the controller
> > > > 
> > > > Which is still very helpful in case you have multiple hotspots that you
> > > > want to track and they are exposed on different workloads. Sacrificing
> > > > the availability of sensors is something needs a better justification
> > > > other than "current code uses only one".
> > > > 
> > > > 
> > > >>
> > > >>  - some boards can take 40°C in 1 sec, the temperature increase is
> > > >> insanely fast and reading several sensors add an extra 15ms.
> > > >>
> > > > 
> > > > 
> > > > Ok... What is the difference in update rate with and without the switch
> > > > of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> > > > your tsensor support that resolution for a single sensor? What is the
> > > > maximum resolution a tsensor can support? What is the penalty added with
> > > > switch?
> > > > 
> > > > Based on this data, and the above 3-5ms, that  means you would miss about
> > > > ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> > > > above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> > > > enough justification to drop three extra sensors?
> > > 
> > > Ok if I refer to the documentation the rate is 0.768 ms with the current
> > > configuration.
> > > 
> > > The driver is currently bogus: register overwritten, bouncing interrupt,
> > > unneeded lock, ... So the proposition was to remove the multiple sensors
> > > support, clean the driver, and re-introduce it if there is a need.
> > > 
> > > If I remember correctly Leo, author of the driver, agreed on this. Leo ?
> > > 
> > > Note, I'm not strongly against multiple sensors support in the driver if
> > > you think it is convenient but it is much simpler to remove the current
> > > code as it is not used and put it back on top of a sane foundation
> > > instead of circumventing that on the existing code.
> > > 
> > > 
> > 
> > I am also fine with the above strategy, as long as you are sure you are
> > not breaking anyone (specially userspace). Also, it would be good to get
> > a reviewed-by from hisilicon just to confirm (Leo?).
> 
> Sorry I missed to reply this patch. And yes, I have tested and
> reviewed it at my side:
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> 
> P.s. I am working for Linaro; I am continously co-working with
> Hisilicon to maintain this driver due it's important for Hikey/Hikey960
> two boards stability; this driver also is important for our daily
> profiling for power and performance. Eduardo, so please let us know if
> you still need ack from Hisilicon engineer.


Yeah, I think adding your Reviewed-by and Kevin's is enough for this
series to go through. As I asked Daniel already, only few minor stuff
needs to be fixed along with the addition of the reviewed-by's.

> 
> > Besides, once you get his reviewed-by, and add it to the patches,
> > can you please resend the series with the minor issues I
> > mentioned (a few minor checkpatch issues and one compilation warn that
> > is added to the driver after the series is applied).
> > 
> > > 
> > > 
> > > 
> > > -- 
> > >  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> > > 
> > > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > > <http://twitter.com/#!/linaroorg> Twitter |
> > > <http://www.linaro.org/linaro-blog/> Blog
> > > 

Re: [PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Daniel Lezcano]

On 18/10/2017 11:15, Tao Wang wrote:
> From: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> multi alarm interrupt forced a re-trigger of power_allocator_throttle
> which changes the PID's actual sampling rate, this isn't optimal for
> IPA, it is best to disable multi alarm support now and sort out this
> issue later.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>

Did you test the series with this change?

> ---
>  drivers/thermal/hisi_thermal.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 133238a..3b74c12 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -360,7 +360,6 @@ static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
>  
>  	/* set interrupt threshold */
>  	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
> -	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
>  	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
>  
>  	/* enable interrupt */
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 18/10/2017 17:51, Eduardo Valentin wrote:
> On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote:
>> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
>>> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>>>> Hello,
>>>>>
>>>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>>>> simultaneously several sensors without a big delay.
>>>>>>>>
>>>>>>>
>>>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>>>
>>>>>> There are several aspects:
>>>>>>
>>>>>>  - the multiple sensors is not needed here
>>>>>
>>>>> Well, that is debatable, I cannot really agree or disagree with the
>>>>> above statement without understanding the use cases and most important,
>>>>> the location of each sensor. What is the location of each sensor?
>>>>>
>>>>>>
>>>>>>  - the temperature controller is not designed to read several sensors at
>>>>>> the same time, we switch the sensor and that clears some internal
>>>>>> buffers and re-init the controller
>>>>>
>>>>> Which is still very helpful in case you have multiple hotspots that you
>>>>> want to track and they are exposed on different workloads. Sacrificing
>>>>> the availability of sensors is something needs a better justification
>>>>> other than "current code uses only one".
>>>>>
>>>>>
>>>>>>
>>>>>>  - some boards can take 40°C in 1 sec, the temperature increase is
>>>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>>>
>>>>>
>>>>>
>>>>> Ok... What is the difference in update rate with and without the switch
>>>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>>>> your tsensor support that resolution for a single sensor? What is the
>>>>> maximum resolution a tsensor can support? What is the penalty added with
>>>>> switch?
>>>>>
>>>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>>>> enough justification to drop three extra sensors?
>>>>
>>>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>>>> configuration.
>>>>
>>>> The driver is currently bogus: register overwritten, bouncing interrupt,
>>>> unneeded lock, ... So the proposition was to remove the multiple sensors
>>>> support, clean the driver, and re-introduce it if there is a need.
>>>>
>>>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>>>
>>>> Note, I'm not strongly against multiple sensors support in the driver if
>>>> you think it is convenient but it is much simpler to remove the current
>>>> code as it is not used and put it back on top of a sane foundation
>>>> instead of circumventing that on the existing code.
>>>>
>>>>
>>>
>>> I am also fine with the above strategy, as long as you are sure you are
>>> not breaking anyone (specially userspace). Also, it would be good to get
>>> a reviewed-by from hisilicon just to confirm (Leo?).
>>
>> Sorry I missed to reply this patch. And yes, I have tested and
>> reviewed it at my side:
>>
>> Reviewed-by: Leo Yan <leo.yan@linaro.org>
>>
>> P.s. I am working for Linaro; I am continously co-working with
>> Hisilicon to maintain this driver due it's important for Hikey/Hikey960
>> two boards stability; this driver also is important for our daily
>> profiling for power and performance. Eduardo, so please let us know if
>> you still need ack from Hisilicon engineer.
> 
> 
> Yeah, I think adding your Reviewed-by and Kevin's is enough for this
> series to go through. As I asked Daniel already, only few minor stuff
> needs to be fixed along with the addition of the reviewed-by's.

The different warnings you reported are fixed and the reviewed-by's /
acked-by's added. I think the patches 19-25 may need an extra look, so I
will resend all the other patches meanwhile.

Does it sound good?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Wangtao (Kevin, Kirin)]

在 2017/10/18 23:54, Daniel Lezcano 写道:
> On 18/10/2017 11:15, Tao Wang wrote:
>> From: Kevin Wangtao <kevin.wangtao@linaro.org>
>>
>> multi alarm interrupt forced a re-trigger of power_allocator_throttle
>> which changes the PID's actual sampling rate, this isn't optimal for
>> IPA, it is best to disable multi alarm support now and sort out this
>> issue later.
>>
>> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> Did you test the series with this change?
Yes
> 
>> ---
>>   drivers/thermal/hisi_thermal.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> index 133238a..3b74c12 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -360,7 +360,6 @@ static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
>>   
>>   	/* set interrupt threshold */
>>   	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
>> -	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
>>   	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
>>   
>>   	/* enable interrupt */
>>
> 
> 

Re: [PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Wangtao (Kevin, Kirin)]

在 2017/10/18 23:54, Daniel Lezcano 写道:
> On 18/10/2017 11:15, Tao Wang wrote:
>> From: Kevin Wangtao <kevin.wangtao@linaro.org>
>>
>> multi alarm interrupt forced a re-trigger of power_allocator_throttle
>> which changes the PID's actual sampling rate, this isn't optimal for
>> IPA, it is best to disable multi alarm support now and sort out this
>> issue later.
>>
>> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> 
> Did you test the series with this change?
Yes
> 
>> ---
>>   drivers/thermal/hisi_thermal.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> index 133238a..3b74c12 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -360,7 +360,6 @@ static int hi3660_thermal_enable_sensor(struct hisi_thermal_data *data)
>>   
>>   	/* set interrupt threshold */
>>   	value = hi3660_thermal_temp_to_step(sensor->thres_temp[0]);
>> -	value |= hi3660_thermal_temp_to_step(sensor->thres_temp[1]) << 10;
>>   	hi3660_thermal_alarm_set(data->regs, sensor->id, value);
>>   
>>   	/* enable interrupt */
>>
> 
> 

Re: [PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Eduardo Valentin]

Hello,

On Thu, Oct 19, 2017 at 09:31:24AM +0800, Wangtao (Kevin, Kirin) wrote:
> 
> 
> 在 2017/10/18 23:54, Daniel Lezcano 写道:
> >On 18/10/2017 11:15, Tao Wang wrote:
> >>From: Kevin Wangtao <kevin.wangtao@linaro.org>
> >>
> >>multi alarm interrupt forced a re-trigger of power_allocator_throttle
> >>which changes the PID's actual sampling rate, this isn't optimal for
> >>IPA, it is best to disable multi alarm support now and sort out this
> >>issue later.
> >>
> >>Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
> >
> >Did you test the series with this change?
> Yes

Is this patch still valid with the latest linus master?

Re: [PATCH] thermal/drivers/hisi: disable multi alarm support for hi3660 SoC

[Daniel Lezcano]

On 05/12/2017 03:02, Eduardo Valentin wrote:
> Hello,
> 
> On Thu, Oct 19, 2017 at 09:31:24AM +0800, Wangtao (Kevin, Kirin) wrote:
>>
>>
>> 在 2017/10/18 23:54, Daniel Lezcano 写道:
>>> On 18/10/2017 11:15, Tao Wang wrote:
>>>> From: Kevin Wangtao <kevin.wangtao@linaro.org>
>>>>
>>>> multi alarm interrupt forced a re-trigger of power_allocator_throttle
>>>> which changes the PID's actual sampling rate, this isn't optimal for
>>>> IPA, it is best to disable multi alarm support now and sort out this
>>>> issue later.
>>>>
>>>> Signed-off-by: Kevin Wangtao <kevin.wangtao@linaro.org>
>>>
>>> Did you test the series with this change?
>> Yes
> 
> Is this patch still valid with the latest linus master?

No, it was folded in the next iteration.

All patches for hisilicon are up-to-date.

Thanks.

 -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog