thermal: ti-soc-thermal: Fix stuck sensor with continuous mode for 4430

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

Also, trying to read the temperature shows a stuck value with:

$ while true; do cat /sys/class/thermal/thermal_zone0/temp; done

Where the temperature is not rising at all with the busy loop.

Additionally, the EOCZ (end of conversion) bit is not rising on 4430 in single conversion mode while it works fine in continuous conversion mode. It is also possible that the hung temperature sensor can affect the thermal shutdown alert too.

Let's fix the issue by adding TI_BANDGAP_FEATURE_CONT_MODE_ONLY flag and use it for 4430.

Note that we also need to add udelay to for the EOCZ (end of conversion) bit polling as otherwise we have it time out too early on 4430. We'll be changing the loop to use iopoll in the following clean-up patch.

Cc Adam Ford <[email protected]>

Cc Carl Philipp Klemm <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Sebastian Reichel <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/thermal/ti-soc-thermal/omap4-thermal-data.c | 3 ++-
 drivers/thermal/ti-soc-thermal/ti-bandgap.c         | 9 +++++++--
 drivers/thermal/ti-soc-thermal/ti-bandgap.h         | 2 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

drivers/thermal/ti-soc-thermal/omap4-thermal-data.c +2 -1

@@ -58,7 +58,8 @@ omap4430_adc_to_temp[OMAP4430_ADC_END_VALUE - OMAP4430_ADC_START_VALUE + 1] = {
 const struct ti_bandgap_data omap4430_data = {
        .features = TI_BANDGAP_FEATURE_MODE_CONFIG |
                        TI_BANDGAP_FEATURE_CLK_CTRL |
-                       TI_BANDGAP_FEATURE_POWER_SWITCH,
+                       TI_BANDGAP_FEATURE_POWER_SWITCH |
+                       TI_BANDGAP_FEATURE_CONT_MODE_ONLY,
        .fclock_name = "bandgap_fclk",
        .div_ck_name = "bandgap_fclk",
        .conv_table = omap4430_adc_to_temp,

drivers/thermal/ti-soc-thermal/ti-bandgap.c +7 -2

@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>

@@ -605,8 +606,10 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
        u32 counter = 1000;
        struct temp_sensor_registers *tsr;
 
-       /* Select single conversion mode */
-       if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
+       /* Select continuous or single conversion mode */
+       if (TI_BANDGAP_HAS(bgp, CONT_MODE_ONLY))
+               RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 1);
+       else if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
                RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 0);
 
        /* Start of Conversion = 1 */

@@ -619,6 +622,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
                if (ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
                    tsr->bgap_eocz_mask)
                        break;
+               udelay(1);
        }
 
        /* Start of Conversion = 0 */

@@ -630,6 +634,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
                if (!(ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
                      tsr->bgap_eocz_mask))
                        break;
+               udelay(1);
        }
 
        return 0;

drivers/thermal/ti-soc-thermal/ti-bandgap.h +2 -1

@@ -280,6 +280,7 @@ struct ti_temp_sensor {
  *     has Errata 814
  * TI_BANDGAP_FEATURE_UNRELIABLE - used when the sensor readings are too
  *     inaccurate.
+ * TI_BANDGAP_FEATURE_CONT_MODE_ONLY - used when single mode hangs the sensor
  * TI_BANDGAP_HAS(b, f) - macro to check if a bandgap device is capable of a
  *      specific feature (above) or not. Return non-zero, if yes.
  */

@@ -295,6 +296,7 @@ struct ti_temp_sensor {
 #define TI_BANDGAP_FEATURE_HISTORY_BUFFER      BIT(9)
 #define TI_BANDGAP_FEATURE_ERRATA_814          BIT(10)
 #define TI_BANDGAP_FEATURE_UNRELIABLE          BIT(11)
+#define TI_BANDGAP_FEATURE_CONT_MODE_ONLY      BIT(12)
 #define TI_BANDGAP_HAS(b, f)                   \
                        ((b)->conf->features & TI_BANDGAP_FEATURE_ ## f)
 
--

2.29.2

Am 30.12.2020 um 13:55 schrieb Adam Ford <[email protected]>:

Quoted 20 lines · Tony Lindgren

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

Also, trying to read the temperature shows a stuck value with:

$ while true; do cat /sys/class/thermal/thermal_zone0/temp; done

Where the temperature is not rising at all with the busy loop.

Additionally, the EOCZ (end of conversion) bit is not rising on 4430 in single conversion mode while it works fine in continuous conversion mode. It is also possible that the hung temperature sensor can affect the thermal shutdown alert too.

Let's fix the issue by adding TI_BANDGAP_FEATURE_CONT_MODE_ONLY flag and use it for 4430.

Quoted 5 lines

Note that we also need to add udelay to for the EOCZ (end of conversion) bit polling as otherwise we have it time out too early on 4430. We'll be changing the loop to use iopoll in the following clean-up patch.

Cc Adam Ford <[email protected]>

I don't have an OMAP4, but if you want, I can test a DM3730.

adam

Quoted 87 lines

Cc Carl Philipp Klemm <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Sebastian Reichel <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---

drivers/thermal/ti-soc-thermal/omap4-thermal-data.c | 3 ++-
drivers/thermal/ti-soc-thermal/ti-bandgap.c         | 9 +++++++--
drivers/thermal/ti-soc-thermal/ti-bandgap.h         | 2 ++
3 files changed, 11 insertions(+), 3 deletions(-)

drivers/thermal/ti-soc-thermal/omap4-thermal-data.c +2 -1

@@ -58,7 +58,8 @@ omap4430_adc_to_temp[OMAP4430_ADC_END_VALUE - OMAP4430_ADC_START_VALUE + 1] = {
 const struct ti_bandgap_data omap4430_data = {
       .features = TI_BANDGAP_FEATURE_MODE_CONFIG |
                       TI_BANDGAP_FEATURE_CLK_CTRL |
-                       TI_BANDGAP_FEATURE_POWER_SWITCH,
+                       TI_BANDGAP_FEATURE_POWER_SWITCH |
+                       TI_BANDGAP_FEATURE_CONT_MODE_ONLY,
       .fclock_name = "bandgap_fclk",
       .div_ck_name = "bandgap_fclk",
       .conv_table = omap4430_adc_to_temp,

drivers/thermal/ti-soc-thermal/ti-bandgap.c +7 -2

@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>

@@ -605,8 +606,10 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
       u32 counter = 1000;
       struct temp_sensor_registers *tsr;
 
-       /* Select single conversion mode */
-       if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
+       /* Select continuous or single conversion mode */
+       if (TI_BANDGAP_HAS(bgp, CONT_MODE_ONLY))
+               RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 1);
+       else if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
               RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 0);
 
       /* Start of Conversion = 1 */

@@ -619,6 +622,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
               if (ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
                   tsr->bgap_eocz_mask)
                       break;
+               udelay(1);
       }
 
       /* Start of Conversion = 0 */

@@ -630,6 +634,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
               if (!(ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
                     tsr->bgap_eocz_mask))
                       break;
+               udelay(1);
       }
 
       return 0;

drivers/thermal/ti-soc-thermal/ti-bandgap.h +2 -1

@@ -280,6 +280,7 @@ struct ti_temp_sensor {
 *     has Errata 814
 * TI_BANDGAP_FEATURE_UNRELIABLE - used when the sensor readings are too
 *     inaccurate.
+ * TI_BANDGAP_FEATURE_CONT_MODE_ONLY - used when single mode hangs the sensor
 * TI_BANDGAP_HAS(b, f) - macro to check if a bandgap device is capable of a
 *      specific feature (above) or not. Return non-zero, if yes.
 */

@@ -295,6 +296,7 @@ struct ti_temp_sensor {
 #define TI_BANDGAP_FEATURE_HISTORY_BUFFER      BIT(9)
 #define TI_BANDGAP_FEATURE_ERRATA_814          BIT(10)
 #define TI_BANDGAP_FEATURE_UNRELIABLE          BIT(11)
+#define TI_BANDGAP_FEATURE_CONT_MODE_ONLY      BIT(12)
 #define TI_BANDGAP_HAS(b, f)                   \
                       ((b)->conf->features & TI_BANDGAP_FEATURE_ ## f)
 
--

2.29.2

Quoted 7 lines

Am 30.12.2020 um 13:55 schrieb Adam Ford <[email protected]>:

Quoted 5 lines · Tony Lindgren

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

Quoted 1 line

I don't have an OMAP4, but if you want, I can test a DM3730.

Indeed I remember a similar discussion from the DM3730 [1]. temp values were always those from the last measurement. E.g. the first one was done during (cold) boot and the first request after 10 minutes did show a quite cold system... The next one did show a hot system independent of what had been between (suspend or high activity).

It seems as if it was even reproducible with a very old kernel on a BeagleBoard. So it is quite fundamental.

We tried to fix it but did not come to a solution [2]. So we opened an issue in our tracker [3] and decided to stay with continuous conversion although this raises idle mode processor load.

Quoted 7 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 30.12.2020 um 13:55 schrieb Adam Ford <[email protected]>:

Quoted 5 lines · Tony Lindgren

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

...

Quoted 8 lines

Quoted 1 line

I don't have an OMAP4, but if you want, I can test a DM3730.

Indeed I remember a similar discussion from the DM3730 [1]. temp values were always those from the last measurement. E.g. the first one was done during (cold) boot and the first request after 10 minutes did show a quite cold system... The next one did show a hot system independent of what had been between (suspend or high activity).

Quoted 6 lines

It seems as if it was even reproducible with a very old kernel on a BeagleBoard. So it is quite fundamental.

We tried to fix it but did not come to a solution [2]. So we opened an issue in our tracker [3] and decided to stay with continuous conversion although this raises idle mode processor load.

Hmm so maybe eocz high always times out in single mode since it also triggers at least on dra7?

Yes it would be great if you guys can the $subject patch a try at least on your omap36xx and omap5 boards and see if you see eocz time out warnings in dmesg.

Regards,

Tony

Quoted 26 lines · Tony Lindgren

Quoted 7 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 30.12.2020 um 13:55 schrieb Adam Ford <[email protected]>:

Quoted 5 lines · Tony Lindgren

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

...

Quoted 14 lines

Quoted 1 line

I don't have an OMAP4, but if you want, I can test a DM3730.

Indeed I remember a similar discussion from the DM3730 [1]. temp values were always those from the last measurement. E.g. the first one was done during (cold) boot and the first request after 10 minutes did show a quite cold system... The next one did show a hot system independent of what had been between (suspend or high activity).

It seems as if it was even reproducible with a very old kernel on a BeagleBoard. So it is quite fundamental.

We tried to fix it but did not come to a solution [2]. So we opened an issue in our tracker [3] and decided to stay with continuous conversion although this raises idle mode processor load.

Quoted 6 lines

Hmm so maybe eocz high always times out in single mode since it also triggers at least on dra7?

Yes it would be great if you guys can the $subject patch a try at least on your omap36xx and omap5 boards and see if you see eocz time out warnings in dmesg.

I should be able to try it on the dm3730 logicpd-torpedo kit this weekend.

adam

Quoted 4 lines

Regards,

Tony

Quoted 28 lines · Adam Ford

Quoted 26 lines · Tony Lindgren

Quoted 7 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 30.12.2020 um 13:55 schrieb Adam Ford <[email protected]>:

Quoted 5 lines · Tony Lindgren

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

...

Quoted 14 lines

Quoted 1 line

I don't have an OMAP4, but if you want, I can test a DM3730.

Indeed I remember a similar discussion from the DM3730 [1]. temp values were always those from the last measurement. E.g. the first one was done during (cold) boot and the first request after 10 minutes did show a quite cold system... The next one did show a hot system independent of what had been between (suspend or high activity).

It seems as if it was even reproducible with a very old kernel on a BeagleBoard. So it is quite fundamental.

We tried to fix it but did not come to a solution [2]. So we opened an issue in our tracker [3] and decided to stay with continuous conversion although this raises idle mode processor load.

Quoted 6 lines

Quoted 6 lines

Hmm so maybe eocz high always times out in single mode since it also triggers at least on dra7?

Yes it would be great if you guys can the $subject patch a try at least on your omap36xx and omap5 boards and see if you see eocz time out warnings in dmesg.

Quoted 2 lines

I should be able to try it on the dm3730 logicpd-torpedo kit this weekend.

I am going to be a bit delayed testing this. I cannot boot omap2plus using Linux version 5.11.0-rc2.

[    2.666748] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xbc
[    2.673309] nand: Micron MT29F4G16ABBDA3W
[    2.677368] nand: 512 MiB, SLC, erase size: 128 KiB, page size:
2048, OOB size: 64
[    2.685119] nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
[    2.693237] Invalid ECC layout
[    2.696350] omap2-nand 30000000.nand: unable to use BCH library
[    2.702575] omap2-nand: probe of 30000000.nand failed with error -22
[    2.716094] 8<--- cut here ---
[    2.719207] Unable to handle kernel NULL pointer dereference at
virtual address 00000018
[    2.727600] pgd = (ptrval)
...
[    3.050933] ---[ end trace 59640c7399a80a07 ]---
[    3.055603] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b
[    3.063323] ---[ end Kernel panic - not syncing: Attempted to kill

init! exitcode=0x0000000b ]---

Once I get past this, I'll try to test the thermal stuff.

adam

Quoted 6 lines

adam

Quoted 4 lines

Regards,

Tony

At least for 4430, trying to use the single conversion mode eventually hangs the thermal sensor. This can be quite easily seen with errors:

thermal thermal_zone0: failed to read out thermal zone (-5)

Also, trying to read the temperature shows a stuck value with:

$ while true; do cat /sys/class/thermal/thermal_zone0/temp; done

Where the temperature is not rising at all with the busy loop.

Additionally, the EOCZ (end of conversion) bit is not rising on 4430 in single conversion mode while it works fine in continuous conversion mode. It is also possible that the hung temperature sensor can affect the thermal shutdown alert too.

Let's fix the issue by adding TI_BANDGAP_FEATURE_CONT_MODE_ONLY flag and use it for 4430.

Note that we also need to add udelay to for the EOCZ (end of conversion) bit polling as otherwise we have it time out too early on 4430. We'll be changing the loop to use iopoll in the following clean-up patch.

Cc Adam Ford <[email protected]>

Cc Carl Philipp Klemm <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Sebastian Reichel <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/thermal/ti-soc-thermal/omap4-thermal-data.c | 3 ++-
 drivers/thermal/ti-soc-thermal/ti-bandgap.c         | 9 +++++++--
 drivers/thermal/ti-soc-thermal/ti-bandgap.h         | 2 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

drivers/thermal/ti-soc-thermal/omap4-thermal-data.c +0 -0

 	.fclock_name = "bandgap_fclk",
 	.div_ck_name = "bandgap_fclk",
 	.conv_table = omap4430_adc_to_temp,

drivers/thermal/ti-soc-thermal/ti-bandgap.c +1 -0

@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>

drivers/thermal/ti-soc-thermal/ti-bandgap.c +2 -0

 	/* Start of Conversion = 1 */

@@ -619,6 +622,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
 		if (ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
 		    tsr->bgap_eocz_mask)
 			break;
+		udelay(1);
 	}
 
 	/* Start of Conversion = 0 */

@@ -630,6 +634,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
 		if (!(ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
 		      tsr->bgap_eocz_mask))
 			break;
+		udelay(1);
 	}
 
 	return 0;

drivers/thermal/ti-soc-thermal/ti-bandgap.h +2 -0

@@ -280,6 +280,7 @@ struct ti_temp_sensor {
  *	has Errata 814
  * TI_BANDGAP_FEATURE_UNRELIABLE - used when the sensor readings are too
  *	inaccurate.
+ * TI_BANDGAP_FEATURE_CONT_MODE_ONLY - used when single mode hangs the sensor
  * TI_BANDGAP_HAS(b, f) - macro to check if a bandgap device is capable of a
  *      specific feature (above) or not. Return non-zero, if yes.
  */

@@ -295,6 +296,7 @@ struct ti_temp_sensor {
 #define TI_BANDGAP_FEATURE_HISTORY_BUFFER	BIT(9)
 #define TI_BANDGAP_FEATURE_ERRATA_814		BIT(10)
 #define TI_BANDGAP_FEATURE_UNRELIABLE		BIT(11)
+#define TI_BANDGAP_FEATURE_CONT_MODE_ONLY	BIT(12)
 #define TI_BANDGAP_HAS(b, f)			\
 			((b)->conf->features & TI_BANDGAP_FEATURE_ ## f)
 

drivers/thermal/ti-soc-thermal/omap4-thermal-data.c omap4430_adc_to_temp[OMAP4430_ADC_END_VALUE - OMAP4430_ADC_START_VALUE + 1] = { · line 62

Quoted 7 lines · Tony Lindgren

drivers/thermal/ti-soc-thermal/omap4-thermal-data.c +2 -1

@@ -58,7 +58,8 @@ omap4430_adc_to_temp[OMAP4430_ADC_END_VALUE - OMAP4430_ADC_START_VALUE + 1] = {
 const struct ti_bandgap_data omap4430_data = {
 	.features = TI_BANDGAP_FEATURE_MODE_CONFIG |
 			TI_BANDGAP_FEATURE_CLK_CTRL |
-			TI_BANDGAP_FEATURE_POWER_SWITCH,
+			TI_BANDGAP_FEATURE_POWER_SWITCH |
+			TI_BANDGAP_FEATURE_CONT_MODE_ONLY,

Can we add a comment with the observations?

drivers/thermal/ti-soc-thermal/ti-bandgap.c ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id) · line 613

Quoted 11 lines

drivers/thermal/ti-soc-thermal/ti-bandgap.c +4 -2

@@ -605,8 +606,10 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
 	u32 counter = 1000;
 	struct temp_sensor_registers *tsr;
 
-	/* Select single conversion mode */
-	if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
+	/* Select continuous or single conversion mode */
+	if (TI_BANDGAP_HAS(bgp, CONT_MODE_ONLY))
+		RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 1);
+	else if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
 		RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 0);

Would not be better to:

if (TI_BANDGAP_HAS(bgp, MODE_CONFIG)) {
	if (TI_BANDGAP_HAS(bgp, CONT_MODE_ONLY))
		RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 1);
	else
		RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 0);
}

One can only switch to cont/single mode if the mode config is possible.