diff mbox series

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

Message ID 1504082857-21702-8-git-send-email-daniel.lezcano@linaro.org
State New
Headers show
Series [01/13] thermal/drivers/hisi: Fix missing interrupt enablement | expand

Commit Message

Daniel Lezcano Aug. 30, 2017, 8:47 a.m. UTC 
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(-)

-- 
2.7.4

Comments

Leo Yan Sept. 2, 2017, 2:54 a.m. UTC | #1 
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

>
Daniel Lezcano Sept. 2, 2017, 8:34 a.m. UTC | #2 
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
Daniel Lezcano Sept. 4, 2017, 9:16 a.m. UTC | #3 
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
diff mbox series

Patch

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,