Hi Tony, On 12/30/20 10:43 AM, Tony Lindgren wrote: > 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.
I don't yet have my setup in working condition, so I can not test these. > 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(-) > > diff --git a/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c > b/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c > --- a/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c > +++ b/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c > @@ -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? > .fclock_name = "bandgap_fclk", > .div_ck_name = "bandgap_fclk", > .conv_table = omap4430_adc_to_temp, > diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c > b/drivers/thermal/ti-soc-thermal/ti-bandgap.c > --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c > +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c > @@ -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); 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. > > /* 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; > diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.h > b/drivers/thermal/ti-soc-thermal/ti-bandgap.h > --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.h > +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.h > @@ -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) > > -- Péter
