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

[PATCH 01/13] 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>
---
 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 bd3572c..8381696 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 01/13] 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>
---
 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 bd3572c..8381696 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/13] 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 | 77 +++++++++++-------------------------------
 1 file changed, 20 insertions(+), 57 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 8381696..92b6889 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,11 +54,10 @@ 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 02/13] 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 | 77 +++++++++++-------------------------------
 1 file changed, 20 insertions(+), 57 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 8381696..92b6889 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,11 +54,10 @@ 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 02/13] thermal/drivers/hisi: Remove the multiple sensors support

[Leo Yan]

Hi Daniel,

On Wed, Aug 30, 2017 at 10:47:26AM +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.
> 
> 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 | 77 +++++++++++-------------------------------
>  1 file changed, 20 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 8381696..92b6889 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

So if we always set the sensor id 2 as default sensor, that means it's
pointless to bind sensor id in dts. E.g. I change to bind thermal
sensor 0, then it's impossible to bind successfully:

-                               thermal-sensors = <&tsensor 2>;
+                               thermal-sensors = <&tsensor 0>;

It's okay for this change, do you think we need reflect this in DT
binding doc [1] ?

[1] Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt

>  struct hisi_thermal_sensor {
>  	struct hisi_thermal_data *thermal;
> @@ -53,11 +54,10 @@ 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;
> -
> +	

Repots whitespace error when I apply patch.

>  	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 02/13] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 01/09/2017 16:05, Leo Yan wrote:
> Hi Daniel,
> 
> On Wed, Aug 30, 2017 at 10:47:26AM +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.
>>
>> 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 | 77 +++++++++++-------------------------------
>>  1 file changed, 20 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> index 8381696..92b6889 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
> 
> So if we always set the sensor id 2 as default sensor, that means it's
> pointless to bind sensor id in dts. E.g. I change to bind thermal
> sensor 0, then it's impossible to bind successfully:
> 
> -                               thermal-sensors = <&tsensor 2>;
> +                               thermal-sensors = <&tsensor 0>;
> 
> It's okay for this change, do you think we need reflect this in DT
> binding doc [1] ?
> 
> [1] Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt

Actually, the current behavior is to go through all the sensors and
register them. By side effect, we end up by registering a sensor with an
id matching the one specified in the DT, others are failing.

That is because there is something missing in the of-thermal API where
we can't call thermal_zone_of_sensor_register() without a matching index.

That is probably why we have in the drivers/thermal/qoriq_thermal.c file
the qoriq_tmu_get_sensor_id() which inspect the thermal-zone DT
structure for the thermal-sensors and read the sensor's id from there in
order to reuse it for thermal_zone_of_sensor_register(). Needless to say
that is not the right place to do that.

IMHO, as nothing gets changed for the moment on hi6220, we can live with
HISI_DEFAULT_SENSOR until we sort out what to do with
thermal_zone_of_sensor_register().


>>  struct hisi_thermal_sensor {
>>  	struct hisi_thermal_data *thermal;
>> @@ -53,11 +54,10 @@ 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;
>> -
>> +	
> 
> Repots whitespace error when I apply patch.

Ok, I will fix that.

[ ... ]

Thanks for the review.

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

[PATCH 03/13] 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>
---
 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 92b6889..67db523 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 03/13] 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>
---
 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 92b6889..67db523 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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:27AM +0200, Daniel Lezcano wrote:
> 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 92b6889..67db523 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;
> +	}
> +

Just reminding for the initialization sequence, could we also move
hisi_thermal_enable_bind_irq_sensor(data) after function
hisi_thermal_register_sensor()? The reason is after we registser
sensor with specified sensor ID, then we can bind irq for the
specified ID, otherwise it will wrongly to bind irq to sensor 0. So
finally we can get result as below:

        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_enable_bind_irq_sensor(data);
        hisi_thermal_toggle_sensor(&data->sensors, true);
        data->irq_enabled = 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/13] 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>
---
 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 67db523..b58ad40 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 04/13] 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>
---
 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 67db523..b58ad40 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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:28AM +0200, Daniel Lezcano wrote:
> 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 67db523..b58ad40 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/13] 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>
---
 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 b58ad40..524310d 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 05/13] 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>
---
 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 b58ad40..524310d 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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:29AM +0200, Daniel Lezcano wrote:
> 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>

This is a good fixing. I do see when the temperature over the tipping
point, if without this patch it's possible to generate interrupt for
2~3 times; after applied this patch it always generate single
interrupt.

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 b58ad40..524310d 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/13] 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>
---
 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 524310d..6f0dab1 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 06/13] 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>
---
 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 524310d..6f0dab1 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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:30AM +0200, Daniel Lezcano wrote:
> 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 524310d..6f0dab1 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/13] 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 | 96 +++++++++++++++++++++++++++++++-----------
 1 file changed, 72 insertions(+), 24 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 6f0dab1..d77a938 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,29 +186,27 @@ 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);
-
-	writel(0x0, data->regs + TEMP0_INT_CLR);
-	writel(0x1, data->regs + TEMP0_INT_EN);
+	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);
@@ -169,10 +217,10 @@ 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 07/13] 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 | 96 +++++++++++++++++++++++++++++++-----------
 1 file changed, 72 insertions(+), 24 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index 6f0dab1..d77a938 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,29 +186,27 @@ 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);
-
-	writel(0x0, data->regs + TEMP0_INT_CLR);
-	writel(0x1, data->regs + TEMP0_INT_EN);
+	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);
@@ -169,10 +217,10 @@ 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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:31AM +0200, Daniel Lezcano wrote:
> 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 | 96 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 72 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 6f0dab1..d77a938 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);
> +}

hdak and sensor id setting share the same one register, so it's
possible to overwrite their value with each other. And for hdak
setting, it should offset 4 bits.

The issues are exists in old code yet but not introduce by this
patch. Could fix these issues as well in this patch?

> +
>  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,29 +186,27 @@ 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);
> -
> -	writel(0x0, data->regs + TEMP0_INT_CLR);
> -	writel(0x1, data->regs + TEMP0_INT_EN);
> +	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);
> +	

whitespace error.

>  	usleep_range(3000, 5000);
>  
>  	mutex_unlock(&data->thermal_lock);
> @@ -169,10 +217,10 @@ 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);
> +	

whitespace error.

>  	mutex_unlock(&data->thermal_lock);
>  }
>  
> -- 
> 2.7.4
> 

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

[Leo Yan]

On Sat, Sep 02, 2017 at 10:09:15AM +0800, Leo Yan wrote:
> On Wed, Aug 30, 2017 at 10:47:31AM +0200, Daniel Lezcano wrote:
> > 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 | 96 +++++++++++++++++++++++++++++++-----------
> >  1 file changed, 72 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> > index 6f0dab1..d77a938 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);
> > +}
> 
> hdak and sensor id setting share the same one register, so it's
> possible to overwrite their value with each other. And for hdak
> setting, it should offset 4 bits.
> 
> The issues are exists in old code yet but not introduce by this
> patch. Could fix these issues as well in this patch?

Have seen the mentioned issues have been fixed in patch 0008. So have
no more question at here.

[...]

Thanks,
Leo Yan

[PATCH 08/13] 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 | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index d77a938..3e03908 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -132,19 +132,39 @@ 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) | (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) | (value << 4), addr + TEMP0_CFG);
 }
 
 static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
-- 
2.7.4

[PATCH 08/13] 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 | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index d77a938..3e03908 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -132,19 +132,39 @@ 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) | (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) | (value << 4), addr + TEMP0_CFG);
 }
 
 static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
-- 
2.7.4

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:32AM +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 | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index d77a938..3e03908 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -132,19 +132,39 @@ 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) | (sensor << 12), addr + TEMP0_CFG);

nitpick: maybe it's better to firstly clear related bits and then set
value?

>  }
>  
> +/*
> + * 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) | (value << 4), addr + TEMP0_CFG);

Ditto.

>  }
>  
>  static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
> -- 
> 2.7.4
> 

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

[Daniel Lezcano]

On 02/09/2017 04:54, Leo Yan wrote:
> On Wed, Aug 30, 2017 at 10:47:32AM +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 | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> index d77a938..3e03908 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -132,19 +132,39 @@ 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) | (sensor << 12), addr + TEMP0_CFG);
> 
> nitpick: maybe it's better to firstly clear related bits and then set
> value?

Sorry, I don't get the comment. Can you elaborate ?


-- 
 <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 08/13] thermal/drivers/hisi: Fix configuration register setting

[Leo Yan]

On Sat, Sep 02, 2017 at 10:34:34AM +0200, Daniel Lezcano wrote:
> On 02/09/2017 04:54, Leo Yan wrote:
> > On Wed, Aug 30, 2017 at 10:47:32AM +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 | 30 +++++++++++++++++++++++++-----
> >>  1 file changed, 25 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> >> index d77a938..3e03908 100644
> >> --- a/drivers/thermal/hisi_thermal.c
> >> +++ b/drivers/thermal/hisi_thermal.c
> >> @@ -132,19 +132,39 @@ 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) | (sensor << 12), addr + TEMP0_CFG);
> > 
> > nitpick: maybe it's better to firstly clear related bits and then set
> > value?
> 
> Sorry, I don't get the comment. Can you elaborate ?

Sure, here I am bit concern there the mixing old bits value and new
setting bits. My suggested code likes below:

  u32 val;

  val = readl(addr + TEMP0_CFG);
  val &= ~0xF000;
  val |= (sensor << 12);
  writel(val, addr + TEMP0_CFG);

Thanks,
Leo Yan

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

[Daniel Lezcano]

On 04/09/2017 02:58, Leo Yan wrote:
> On Sat, Sep 02, 2017 at 10:34:34AM +0200, Daniel Lezcano wrote:
>> On 02/09/2017 04:54, Leo Yan wrote:
>>> On Wed, Aug 30, 2017 at 10:47:32AM +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 | 30 +++++++++++++++++++++++++-----
>>>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>>>> index d77a938..3e03908 100644
>>>> --- a/drivers/thermal/hisi_thermal.c
>>>> +++ b/drivers/thermal/hisi_thermal.c
>>>> @@ -132,19 +132,39 @@ 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) | (sensor << 12), addr + TEMP0_CFG);
>>>
>>> nitpick: maybe it's better to firstly clear related bits and then set
>>> value?
>>
>> Sorry, I don't get the comment. Can you elaborate ?
> 
> Sure, here I am bit concern there the mixing old bits value and new
> setting bits. My suggested code likes below:
> 
>   u32 val;
> 
>   val = readl(addr + TEMP0_CFG);
>   val &= ~0xF000;
>   val |= (sensor << 12);
>   writel(val, addr + TEMP0_CFG);

Oh, yes. Good catch.


-- 
 <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 09/13] 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>
---
 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 3e03908..b038d8a 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -39,6 +39,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
@@ -58,8 +59,6 @@ struct hisi_thermal_data {
 	struct clk *clk;
 	struct hisi_thermal_sensor sensors;
 	int irq;
-	bool irq_enabled;
-	
 	void __iomem *regs;
 };
 
@@ -97,9 +96,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)
@@ -167,71 +197,6 @@ static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
 	writel(readl(addr + TEMP0_CFG) | (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);
@@ -249,25 +214,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;
 }
@@ -276,26 +226,27 @@ static const 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;
 }
@@ -348,6 +299,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;
@@ -390,9 +375,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);
@@ -402,18 +384,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;
 }
@@ -436,7 +421,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);
 
@@ -452,8 +436,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 09/13] 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>
---
 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 3e03908..b038d8a 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -39,6 +39,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
@@ -58,8 +59,6 @@ struct hisi_thermal_data {
 	struct clk *clk;
 	struct hisi_thermal_sensor sensors;
 	int irq;
-	bool irq_enabled;
-	
 	void __iomem *regs;
 };
 
@@ -97,9 +96,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)
@@ -167,71 +197,6 @@ static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
 	writel(readl(addr + TEMP0_CFG) | (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);
@@ -249,25 +214,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;
 }
@@ -276,26 +226,27 @@ static const 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;
 }
@@ -348,6 +299,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;
@@ -390,9 +375,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);
@@ -402,18 +384,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;
 }
@@ -436,7 +421,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);
 
@@ -452,8 +436,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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:33AM +0200, Daniel Lezcano wrote:
> 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>
> ---
>  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 3e03908..b038d8a 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -39,6 +39,7 @@
>  #define HISI_TEMP_BASE			(-60000)
>  #define HISI_TEMP_RESET			(100000)
>  #define HISI_TEMP_STEP			(784)
> +#define HISI_TEMP_LAG			(3500)

So I am curious what's the reason to select 3.5'c for lag value? Is
this a heuristics result?

>  #define HISI_MAX_SENSORS		4
>  #define HISI_DEFAULT_SENSOR		2
> @@ -58,8 +59,6 @@ struct hisi_thermal_data {
>  	struct clk *clk;
>  	struct hisi_thermal_sensor sensors;
>  	int irq;
> -	bool irq_enabled;
> -	
>  	void __iomem *regs;
>  };
>  
> @@ -97,9 +96,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.

So here means if we set a long lag value and the interrupt is delayed,
sometimes we even don't receive the interrupt; but this is acceptable
due we are polling the temperature so we still can get the updated
temperature value, right?

> + *
> + * [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)
> @@ -167,71 +197,6 @@ static inline void hisi_thermal_hdak_set(void __iomem *addr, int value)
>  	writel(readl(addr + TEMP0_CFG) | (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);
> @@ -249,25 +214,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;
>  }
> @@ -276,26 +226,27 @@ static const 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);
> +	}

For temperature increasing and decreasing both cases, can always call
thermal_zone_device_update()?

>  
>  	return IRQ_HANDLED;
>  }
> @@ -348,6 +299,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;
> @@ -390,9 +375,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);
> @@ -402,18 +384,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;
>  }
> @@ -436,7 +421,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);
>  
> @@ -452,8 +436,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
> 

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

[Daniel Lezcano]

On 02/09/2017 05:29, Leo Yan wrote:
> On Wed, Aug 30, 2017 at 10:47:33AM +0200, Daniel Lezcano wrote:
>> 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>
>> ---
>>  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 3e03908..b038d8a 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -39,6 +39,7 @@
>>  #define HISI_TEMP_BASE			(-60000)
>>  #define HISI_TEMP_RESET			(100000)
>>  #define HISI_TEMP_STEP			(784)
>> +#define HISI_TEMP_LAG			(3500)
> 
> So I am curious what's the reason to select 3.5'c for lag value? Is
> this a heuristics result?

Yes, it is. I tried 5°C but I find it too long. After several tests, I
found 3.5°C looked fine.

[ ... ]

>> + * 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.
> 
> So here means if we set a long lag value and the interrupt is delayed,
> sometimes we even don't receive the interrupt; but this is acceptable
> due we are polling the temperature so we still can get the updated
> temperature value, right?
No, what I meant is if the hysteresis is too large, eg. 45 - 65, then
the interrupt will come after the temperature goes below 45 and that may
take a long time, especially if the temperature is fluctuating between
the two hysteresis values.

The only setup preventing an interrupt to happen is when crossing the
low value hysteresis is not possible in practical. For example, the
temperature of the SoC is ~44°C at idle time. If we set the threshold to
65°C and the lag to the max 24°C, the interrupt will raised when we go
below 41°C and that situation can't happen.

In the current situation, the interrupt is only used to raise an alarm.
The get temperature is resulting from a polling, so the delay between
the alarm on/off is not issue for now.

>> + *
>> + * [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);
>>  }


[ ... ]

>> -	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);
>> +	}
> 
> For temperature increasing and decreasing both cases, can always call
> thermal_zone_device_update()?

That is something I asked myself but I finally decided to not change the
current behavior and let that be added in a separate patch.


[ ... ]



-- 
 <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 09/13] thermal/drivers/hisi: Remove costly sensor inspection

[Leo Yan]

On Sat, Sep 02, 2017 at 03:10:29PM +0200, Daniel Lezcano wrote:

[...]

> On 02/09/2017 05:29, Leo Yan wrote:
> > On Wed, Aug 30, 2017 at 10:47:33AM +0200, Daniel Lezcano wrote:
> >> 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>
> >> ---
> >>  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 3e03908..b038d8a 100644
> >> --- a/drivers/thermal/hisi_thermal.c
> >> +++ b/drivers/thermal/hisi_thermal.c
> >> @@ -39,6 +39,7 @@
> >>  #define HISI_TEMP_BASE			(-60000)
> >>  #define HISI_TEMP_RESET			(100000)
> >>  #define HISI_TEMP_STEP			(784)
> >> +#define HISI_TEMP_LAG			(3500)
> > 
> > So I am curious what's the reason to select 3.5'c for lag value? Is
> > this a heuristics result?
> 
> Yes, it is. I tried 5°C but I find it too long. After several tests, I
> found 3.5°C looked fine.
> 
> [ ... ]
> 
> >> + * 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.
> > 
> > So here means if we set a long lag value and the interrupt is delayed,
> > sometimes we even don't receive the interrupt; but this is acceptable
> > due we are polling the temperature so we still can get the updated
> > temperature value, right?
> No, what I meant is if the hysteresis is too large, eg. 45 - 65, then
> the interrupt will come after the temperature goes below 45 and that may
> take a long time, especially if the temperature is fluctuating between
> the two hysteresis values.
> 
> The only setup preventing an interrupt to happen is when crossing the
> low value hysteresis is not possible in practical. For example, the
> temperature of the SoC is ~44°C at idle time. If we set the threshold to
> 65°C and the lag to the max 24°C, the interrupt will raised when we go
> below 41°C and that situation can't happen.
> 
> In the current situation, the interrupt is only used to raise an alarm.

Thanks for the explanation, Ionela (have Cced.) reminded me the
interrupt for 'alarm on' is fatal, if we use polling method then the
delay can be 1000ms, but with interrupt to alarm we can handle the
temperature rasing immediately.

> The get temperature is resulting from a polling, so the delay between
> the alarm on/off is not issue for now.

Understood.

> >> + *
> >> + * [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);
> >>  }
> 
> 
> [ ... ]
> 
> >> -	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);
> >> +	}
> > 
> > For temperature increasing and decreasing both cases, can always call
> > thermal_zone_device_update()?
> 
> That is something I asked myself but I finally decided to not change the
> current behavior and let that be added in a separate patch.

This is fine.

Thanks,
Leo Yan

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

[Daniel Lezcano]

On 04/09/2017 02:50, Leo Yan wrote:

[ ... ]

>> That is something I asked myself but I finally decided to not change the
>> current behavior and let that be added in a separate patch.
> 
> This is fine.

Hi Leo,

can you tag this patch if you agree with it?

-- 
 <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 09/13] thermal/drivers/hisi: Remove costly sensor inspection

[Leo Yan]

On Mon, Sep 04, 2017 at 01:29:19PM +0200, Daniel Lezcano wrote:
> On 04/09/2017 02:50, Leo Yan wrote:
> 
> [ ... ]
> 
> >> That is something I asked myself but I finally decided to not change the
> >> current behavior and let that be added in a separate patch.
> > 
> > This is fine.
> 
> Hi Leo,
> 
> can you tag this patch if you agree with it?

Yeah; I have tested this patch at my side:

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

> 
> -- 
>  <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 10/13] 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>
---
 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 b038d8a..68b625c 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -47,8 +47,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;
 };
@@ -57,9 +55,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;
 };
 
 /*
@@ -229,7 +227,7 @@ static const 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);
@@ -240,7 +238,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) {
@@ -303,7 +301,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);
@@ -376,7 +374,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",
@@ -398,7 +396,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;
 }
@@ -406,7 +404,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 10/13] 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>
---
 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 b038d8a..68b625c 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -47,8 +47,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;
 };
@@ -57,9 +55,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;
 };
 
 /*
@@ -229,7 +227,7 @@ static const 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);
@@ -240,7 +238,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) {
@@ -303,7 +301,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);
@@ -376,7 +374,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",
@@ -398,7 +396,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;
 }
@@ -406,7 +404,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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:34AM +0200, Daniel Lezcano wrote:
> 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 b038d8a..68b625c 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -47,8 +47,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;
>  };
> @@ -57,9 +55,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;
>  };
>  
>  /*
> @@ -229,7 +227,7 @@ static const 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);
> @@ -240,7 +238,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) {
> @@ -303,7 +301,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);
> @@ -376,7 +374,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",
> @@ -398,7 +396,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;
>  }
> @@ -406,7 +404,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/13] 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>
---
 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 68b625c..9eee82b 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -83,12 +83,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 11/13] 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>
---
 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 68b625c..9eee82b 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -83,12 +83,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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:35AM +0200, Daniel Lezcano wrote:
> 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 68b625c..9eee82b 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -83,12 +83,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/13] 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 9eee82b..b1b086ab 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -45,7 +45,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;
@@ -207,10 +206,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);
 
@@ -258,10 +257,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 12/13] 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 9eee82b..b1b086ab 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -45,7 +45,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;
@@ -207,10 +206,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);
 
@@ -258,10 +257,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/13] 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>
---
 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 b1b086ab..b9e8ee2 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -51,7 +51,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;
@@ -196,14 +195,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)
@@ -340,7 +335,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 13/13] 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>
---
 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 b1b086ab..b9e8ee2 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -51,7 +51,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;
@@ -196,14 +195,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)
@@ -340,7 +335,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

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

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:37AM +0200, Daniel Lezcano wrote:
> 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

After applied your previous patches, configuration reigster accessing
only happens in the probe and resume functions. So shouldn't have
chance to access it at the same time.

BTW, I verified this patch with system suspend/resume, so far it works
well after system resume back.

> 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 b1b086ab..b9e8ee2 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -51,7 +51,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;
> @@ -196,14 +195,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)
> @@ -340,7 +335,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
> 

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

[Daniel Lezcano]

On 02/09/2017 06:04, Leo Yan wrote:
> On Wed, Aug 30, 2017 at 10:47:37AM +0200, Daniel Lezcano wrote:
>> 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
> 
> After applied your previous patches, configuration reigster accessing
> only happens in the probe and resume functions. So shouldn't have
> chance to access it at the same time.
> 
> BTW, I verified this patch with system suspend/resume, so far it works
> well after system resume back.

Great, thanks for testing this.

>> 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 b1b086ab..b9e8ee2 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -51,7 +51,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;
>> @@ -196,14 +195,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)
>> @@ -340,7 +335,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
>>


-- 
 <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 01/13] thermal/drivers/hisi: Fix missing interrupt enablement

[Leo Yan]

On Wed, Aug 30, 2017 at 10:47:25AM +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'.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks for patch series, I will verify them one by one. I verified the
interrupt can work well with this patch:

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 bd3572c..8381696 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
>