Il 26/11/24 14:19, Nícolas F. R. A. Prado ha scritto:
On Tue, Nov 26, 2024 at 10:43:55AM +0100, AngeloGioacchino Del Regno wrote:
Il 25/11/24 22:20, Nícolas F. R. A. Prado ha scritto:
When configured in filtered mode, the LVTS thermal controller will
monitor the temperature from the sensors and trigger an interrupt once a
thermal threshold is crossed.
Currently this is true even during suspend and resume. The problem with
that is that when enabling the internal clock of the LVTS controller in
lvts_ctrl_set_enable() during resume, the temperature reading can glitch
and appear much higher than the real one, resulting in a spurious
interrupt getting generated.
Disable the temperature monitoring and give some time for the signals to
stabilize during suspend in order to prevent such spurious interrupts.
Cc: stable@xxxxxxxxxxxxxxx
Reported-by: Hsin-Te Yuan <yuanhsinte@xxxxxxxxxxxx>
Closes: https://lore.kernel.org/all/20241108-lvts-v1-1-eee339c6ca20@xxxxxxxxxxxx/
Fixes: 8137bb90600d ("thermal/drivers/mediatek/lvts_thermal: Add suspend and resume")
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
---
drivers/thermal/mediatek/lvts_thermal.c | 36 +++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 1997e91bb3be94a3059db619238aa5787edc7675..a92ff2325c40704adc537af6995b34f93c3b0650 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -860,6 +860,32 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
return 0;
}
+static void lvts_ctrl_monitor_enable(struct device *dev, struct lvts_ctrl *lvts_ctrl, bool enable)
+{
+ /*
+ * Bitmaps to enable each sensor on filtered mode in the MONCTL0
+ * register.
+ */
+ u32 sensor_filt_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };
+ u32 sensor_map = 0;
+ int i;
+
+ if (lvts_ctrl->mode != LVTS_MSR_FILTERED_MODE)
+ return;
+
That's easier and shorter:
static void lvts_ctrl_monitor_enable( .... )
{
/* Bitmap to enable each sensor on filtered mode in the MONCTL0 register */
const u32 sensor_map = GENMASK(3, 0);
if (lvts_ctrl->mode != LVTS_MSR_FILTERED_MODE)
return;
/* Bits 0-3: Sensing points - Bit 9: Single point access flow */
if (enable)
writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
Wait, no, here you're enabling all the sensors in the controller. We only want
to enable ones that are valid, otherwise we might get garbage data and irqs from
sensors that aren't actually there. That's why I use the
lvts_for_each_valid_sensor() helper in this patch.
Whoa, my brain actually missed the lvts_for_each_valid_sensor()!
Okay no, then you're right - sorry for the bad example! In that case, though, I
still have one more comment.
You can constify sensor_filt_bitmap, and since the values never go higher than
BIT(3), you should also be able to spare some memory by turning that into a u8:
const u8 sensor_filt_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };
...and then I assume that there's no way valid sensors could ever read from an
index that is more than 4 (so, I assume that there's no way the loop tries to
read out of the array upper boundary).
In which case - after at least constifying the sensor_filt_bitmap array, for v2
feel free to add my
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
...and sorry again for the initial miss :-)
Cheers,
Angelo
