Fixes in the interrupt handling of the LVTS thermal driver noticed while
testing it on the Spherion Chromebook (mt8192-asurada-spherion) with the
MT8192 support series [1].
These are standalone fixes and don't depend on anything else.
[1] https://lore.kernel.org/all/[email protected]/
Thanks,
Nícolas
Nícolas F. R. A. Prado (3):
thermal/drivers/mediatek/lvts_thermal: Handle IRQ on all controllers
thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode
thermal/drivers/mediatek/lvts_thermal: Disable undesired interrupts
drivers/thermal/mediatek/lvts_thermal.c | 54 +++++++++++++------------
1 file changed, 28 insertions(+), 26 deletions(-)
--
2.40.0
There is a single IRQ handler for each LVTS thermal domain, and it is
supposed to check each of its underlying controllers for the origin of
the interrupt and clear its status. However due to a typo, only the
first controller was ever being handled, which resulted in the interrupt
never being cleared when it happened on the other controllers. Add the
missing index so interrupts are handled for all controllers.
Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/thermal/mediatek/lvts_thermal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index d0a3f95b7884..56b24c5b645f 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -449,7 +449,7 @@ static irqreturn_t lvts_irq_handler(int irq, void *data)
for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
- aux = lvts_ctrl_irq_handler(lvts_td->lvts_ctrl);
+ aux = lvts_ctrl_irq_handler(&lvts_td->lvts_ctrl[i]);
if (aux != IRQ_HANDLED)
continue;
--
2.40.0
Out of the many interrupts supported by the hardware, the only ones of
interest to the driver currently are:
* The temperature went over the hot threshold, for any of the sensors
* The temperature went over the hot to normal threshold, for any of the
sensors
* The temperature went over the stage3 threshold
These are the only thresholds configured by the driver through the
HTHRE, H2NTHRE, and PROTTC registers, respectively.
The current interrupt mask in LVTS_MONINT_CONF, enables many more
interrupts, including offset detection and data ready on sensors for
both filtered and immediate mode. These are not only not handled by the
driver, but they are also triggered too often, causing unneeded
overhead. Disable these unnecessary interrupts.
The meaning of each bit can be seen in the comment describing
LVTS_MONINTST in the IRQ handler.
Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/thermal/mediatek/lvts_thermal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 29d10eaa8dfc..300ce22a7471 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -63,7 +63,7 @@
#define LVTS_HW_FILTER 0x2
#define LVTS_TSSEL_CONF 0x13121110
#define LVTS_CALSCALE_CONF 0x300
-#define LVTS_MONINT_CONF 0x9FBF7BDE
+#define LVTS_MONINT_CONF 0x84804A52
#define LVTS_INT_SENSOR0 0x0009001F
#define LVTS_INT_SENSOR1 0x001203E0
--
2.40.0
Each controller can be configured to operate on immediate or filtered
mode. On filtered mode, the sensors are enabled by setting the
corresponding bits in MONCTL0, while on immediate mode, by setting
MSRCTL1.
Previously, the code would set MSRCTL1 for all four sensors when
configured to immediate mode, but given that the controller might not
have all four sensors connected, this would cause interrupts to trigger
for non-existent sensors. Fix this by handling the MSRCTL1 register
analogously to the MONCTL0: only enable the sensors that were declared.
Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/thermal/mediatek/lvts_thermal.c | 50 +++++++++++++------------
1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 56b24c5b645f..29d10eaa8dfc 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -894,24 +894,6 @@ static int lvts_ctrl_configure(struct device *dev, struct lvts_ctrl *lvts_ctrl)
LVTS_HW_FILTER << 3 | LVTS_HW_FILTER;
writel(value, LVTS_MSRCTL0(lvts_ctrl->base));
- /*
- * LVTS_MSRCTL1 : Measurement control
- *
- * Bits:
- *
- * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
- * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
- * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
- * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
- *
- * That configuration will ignore the filtering and the delays
- * introduced below in MONCTL1 and MONCTL2
- */
- if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
- value = BIT(9) | BIT(6) | BIT(5) | BIT(4);
- writel(value, LVTS_MSRCTL1(lvts_ctrl->base));
- }
-
/*
* LVTS_MONCTL1 : Period unit and group interval configuration
*
@@ -976,6 +958,8 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
struct lvts_sensor *lvts_sensors = lvts_ctrl->sensors;
struct thermal_zone_device *tz;
u32 sensor_map = 0;
+ u32 sensor_map_bits[][4] = {{BIT(4), BIT(5), BIT(6), BIT(9)},
+ {BIT(0), BIT(1), BIT(2), BIT(3)}};
int i;
for (i = 0; i < lvts_ctrl->num_lvts_sensor; i++) {
@@ -1012,20 +996,38 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
* map, so we can enable the temperature monitoring in
* the hardware thermal controller.
*/
- sensor_map |= BIT(i);
+ sensor_map |= sensor_map_bits[lvts_ctrl->mode][i];
}
/*
- * Bits:
- * 9: Single point access flow
- * 0-3: Enable sensing point 0-3
- *
* The initialization of the thermal zones give us
* which sensor point to enable. If any thermal zone
* was not described in the device tree, it won't be
* enabled here in the sensor map.
*/
- writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
+ if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
+ /*
+ * LVTS_MSRCTL1 : Measurement control
+ *
+ * Bits:
+ *
+ * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
+ * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
+ * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
+ * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
+ *
+ * That configuration will ignore the filtering and the delays
+ * introduced in MONCTL1 and MONCTL2
+ */
+ writel(sensor_map, LVTS_MSRCTL1(lvts_ctrl->base));
+ } else {
+ /*
+ * Bits:
+ * 9: Single point access flow
+ * 0-3: Enable sensing point 0-3
+ */
+ writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
+ }
return 0;
}
--
2.40.0
On 28/04/2023 21:53, Nícolas F. R. A. Prado wrote:
> There is a single IRQ handler for each LVTS thermal domain, and it is
> supposed to check each of its underlying controllers for the origin of
> the interrupt and clear its status. However due to a typo, only the
> first controller was ever being handled, which resulted in the interrupt
> never being cleared when it happened on the other controllers. Add the
> missing index so interrupts are handled for all controllers.
>
> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
>
> drivers/thermal/mediatek/lvts_thermal.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index d0a3f95b7884..56b24c5b645f 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -449,7 +449,7 @@ static irqreturn_t lvts_irq_handler(int irq, void *data)
>
> for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
>
> - aux = lvts_ctrl_irq_handler(lvts_td->lvts_ctrl);
> + aux = lvts_ctrl_irq_handler(&lvts_td->lvts_ctrl[i]);
> if (aux != IRQ_HANDLED)
> continue;
>
On Fri, Apr 28, 2023 at 03:53:44PM -0400, N?colas F. R. A. Prado wrote:
>
> Fixes in the interrupt handling of the LVTS thermal driver noticed while
> testing it on the Spherion Chromebook (mt8192-asurada-spherion) with the
> MT8192 support series [1].
>
> These are standalone fixes and don't depend on anything else.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Thanks,
> N?colas
>
>
> N?colas F. R. A. Prado (3):
> thermal/drivers/mediatek/lvts_thermal: Handle IRQ on all controllers
> thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode
> thermal/drivers/mediatek/lvts_thermal: Disable undesired interrupts
This series seems to have solved all interrupt storm issue I ran into, so
Tested-by: Chen-Yu Tsai <[email protected]>
Il 28/04/23 21:53, Nícolas F. R. A. Prado ha scritto:
> There is a single IRQ handler for each LVTS thermal domain, and it is
> supposed to check each of its underlying controllers for the origin of
> the interrupt and clear its status. However due to a typo, only the
> first controller was ever being handled, which resulted in the interrupt
> never being cleared when it happened on the other controllers. Add the
> missing index so interrupts are handled for all controllers.
>
> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 28/04/23 21:53, Nícolas F. R. A. Prado ha scritto:
> Each controller can be configured to operate on immediate or filtered
> mode. On filtered mode, the sensors are enabled by setting the
> corresponding bits in MONCTL0, while on immediate mode, by setting
> MSRCTL1.
>
> Previously, the code would set MSRCTL1 for all four sensors when
> configured to immediate mode, but given that the controller might not
> have all four sensors connected, this would cause interrupts to trigger
> for non-existent sensors. Fix this by handling the MSRCTL1 register
> analogously to the MONCTL0: only enable the sensors that were declared.
>
> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
Awesome!!!!!
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 28/04/23 21:53, Nícolas F. R. A. Prado ha scritto:
> Out of the many interrupts supported by the hardware, the only ones of
> interest to the driver currently are:
> * The temperature went over the hot threshold, for any of the sensors
> * The temperature went over the hot to normal threshold, for any of the
> sensors
> * The temperature went over the stage3 threshold
>
> These are the only thresholds configured by the driver through the
> HTHRE, H2NTHRE, and PROTTC registers, respectively.
>
> The current interrupt mask in LVTS_MONINT_CONF, enables many more
> interrupts, including offset detection and data ready on sensors for
> both filtered and immediate mode. These are not only not handled by the
> driver, but they are also triggered too often, causing unneeded
> overhead. Disable these unnecessary interrupts.
>
> The meaning of each bit can be seen in the comment describing
> LVTS_MONINTST in the IRQ handler.
>
> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Hi,
On 02/05/2023 12:33, Chen-Yu Tsai wrote:
> On Fri, Apr 28, 2023 at 03:53:44PM -0400, Nícolas F. R. A. Prado wrote:
>>
>> Fixes in the interrupt handling of the LVTS thermal driver noticed while
>> testing it on the Spherion Chromebook (mt8192-asurada-spherion) with the
>> MT8192 support series [1].
>>
>> These are standalone fixes and don't depend on anything else.
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>>
>> Thanks,
>> Nícolas
>>
>>
>> Nícolas F. R. A. Prado (3):
>> thermal/drivers/mediatek/lvts_thermal: Handle IRQ on all controllers
>> thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode
>> thermal/drivers/mediatek/lvts_thermal: Disable undesired interrupts
>
> This series seems to have solved all interrupt storm issue I ran into, so
>
> Tested-by: Chen-Yu Tsai <[email protected]>
I gave a try on a mt8195 board and I don't see any interrupt firing when
crossing the temperature thresholds.
Did I miss something ?
--
<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
On Tue, May 30, 2023 at 02:27:36PM +0200, Daniel Lezcano wrote:
>
> Hi,
>
> On 02/05/2023 12:33, Chen-Yu Tsai wrote:
> > On Fri, Apr 28, 2023 at 03:53:44PM -0400, N?colas F. R. A. Prado wrote:
> > >
> > > Fixes in the interrupt handling of the LVTS thermal driver noticed while
> > > testing it on the Spherion Chromebook (mt8192-asurada-spherion) with the
> > > MT8192 support series [1].
> > >
> > > These are standalone fixes and don't depend on anything else.
> > >
> > > [1] https://lore.kernel.org/all/[email protected]/
> > >
> > > Thanks,
> > > N?colas
> > >
> > >
> > > N?colas F. R. A. Prado (3):
> > > thermal/drivers/mediatek/lvts_thermal: Handle IRQ on all controllers
> > > thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode
> > > thermal/drivers/mediatek/lvts_thermal: Disable undesired interrupts
> >
> > This series seems to have solved all interrupt storm issue I ran into, so
> >
> > Tested-by: Chen-Yu Tsai <[email protected]>
>
> I gave a try on a mt8195 board and I don't see any interrupt firing when
> crossing the temperature thresholds.
>
> Did I miss something ?
No, indeed interrupts seem to be completely disabled on mt8195, even after
setting the controllers to filtered mode (a requirement to get interrupts). I
haven't investigated that further yet. This series was validated on mt8192,
which did have working interrupts, but they were being triggered too often.
Also note that I've sent a v2 with even more fixes:
https://lore.kernel.org/all/[email protected]/
Thanks,
N?colas
On 30/05/2023 21:46, Nícolas F. R. A. Prado wrote:
> On Tue, May 30, 2023 at 02:27:36PM +0200, Daniel Lezcano wrote:
>>
>> Hi,
>>
>> On 02/05/2023 12:33, Chen-Yu Tsai wrote:
>>> On Fri, Apr 28, 2023 at 03:53:44PM -0400, Nícolas F. R. A. Prado wrote:
>>>>
>>>> Fixes in the interrupt handling of the LVTS thermal driver noticed while
>>>> testing it on the Spherion Chromebook (mt8192-asurada-spherion) with the
>>>> MT8192 support series [1].
>>>>
>>>> These are standalone fixes and don't depend on anything else.
>>>>
>>>> [1] https://lore.kernel.org/all/[email protected]/
>>>>
>>>> Thanks,
>>>> Nícolas
>>>>
>>>>
>>>> Nícolas F. R. A. Prado (3):
>>>> thermal/drivers/mediatek/lvts_thermal: Handle IRQ on all controllers
>>>> thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode
>>>> thermal/drivers/mediatek/lvts_thermal: Disable undesired interrupts
>>>
>>> This series seems to have solved all interrupt storm issue I ran into, so
>>>
>>> Tested-by: Chen-Yu Tsai <[email protected]>
>>
>> I gave a try on a mt8195 board and I don't see any interrupt firing when
>> crossing the temperature thresholds.
>>
>> Did I miss something ?
>
> No, indeed interrupts seem to be completely disabled on mt8195, even after
> setting the controllers to filtered mode (a requirement to get interrupts).
Really? interrupts work only on filtered mode? That sounds strange
What board are you using for testing?
> I
> haven't investigated that further yet. This series was validated on mt8192,
> which did have working interrupts, but they were being triggered too often.
Ok.
> Also note that I've sent a v2 with even more fixes:
> https://lore.kernel.org/all/[email protected]/
Yes, I'm reviewing it closely
--
<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
On Fri, Jun 02, 2023 at 10:07:29AM +0200, Daniel Lezcano wrote:
> On 30/05/2023 21:46, N?colas F. R. A. Prado wrote:
> > On Tue, May 30, 2023 at 02:27:36PM +0200, Daniel Lezcano wrote:
> > >
> > > Hi,
> > >
> > > On 02/05/2023 12:33, Chen-Yu Tsai wrote:
> > > > On Fri, Apr 28, 2023 at 03:53:44PM -0400, N?colas F. R. A. Prado wrote:
> > > > >
> > > > > Fixes in the interrupt handling of the LVTS thermal driver noticed while
> > > > > testing it on the Spherion Chromebook (mt8192-asurada-spherion) with the
> > > > > MT8192 support series [1].
> > > > >
> > > > > These are standalone fixes and don't depend on anything else.
> > > > >
> > > > > [1] https://lore.kernel.org/all/[email protected]/
> > > > >
> > > > > Thanks,
> > > > > N?colas
> > > > >
> > > > >
> > > > > N?colas F. R. A. Prado (3):
> > > > > thermal/drivers/mediatek/lvts_thermal: Handle IRQ on all controllers
> > > > > thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode
> > > > > thermal/drivers/mediatek/lvts_thermal: Disable undesired interrupts
> > > >
> > > > This series seems to have solved all interrupt storm issue I ran into, so
> > > >
> > > > Tested-by: Chen-Yu Tsai <[email protected]>
> > >
> > > I gave a try on a mt8195 board and I don't see any interrupt firing when
> > > crossing the temperature thresholds.
> > >
> > > Did I miss something ?
> >
> > No, indeed interrupts seem to be completely disabled on mt8195, even after
> > setting the controllers to filtered mode (a requirement to get interrupts).
>
> Really? interrupts work only on filtered mode? That sounds strange
Sorry my reply was confusing, let me clarify. What I meant to say is that
the threshold interrupts (cold, hot2normal, hot, low offset, high offset) only
trigger in filtered mode. AFAICT that's by design, since immediate mode is meant
only for one-off temperature readings, and filtered mode is the one meant to be
used for temperature monitoring. But in immediate mode you could still get the
data ready for immediate mode (bits 16, 17, 18, 27) interrupts triggering.
Though note that I have disabled those in my series, since they are triggered
constantly.
>
> What board are you using for testing?
I'm testing on the Acer Chromebook 514 (mt8192-asurada-spherion-r0). And noticed
the interrupts aren't triggered on Acer Chromebook Spin 513
(mt8195-cherry-tomato-r2).
>
> > I
> > haven't investigated that further yet. This series was validated on mt8192,
> > which did have working interrupts, but they were being triggered too often.
>
> Ok.
>
> > Also note that I've sent a v2 with even more fixes:
> > https://lore.kernel.org/all/[email protected]/
>
> Yes, I'm reviewing it closely
Thanks!
N?colas