| Message ID | 1b53ef537203e629328285b4597a09e4a586d688.1571181041.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
| Series | of-thermal: Disable polling when interrupt property is found in DT | expand |
Quoting Amit Kucheria (2019-10-15 16:13:16) > Currently, in order to enable interrupt-only mode, one must set > polling-delay-passive and polling-delay properties in the DT to 0, > otherwise the thermal framework will continue to setup a periodic timers > to monitor the thermal zones. > > Change the behaviour, so that on DT-based systems, we no longer have to > set the properties to zero if we find an 'interrupt' property in the > sensor. > > Following data shows the number of times > thermal_zone_device_set_polling() is invoked with and without this > patch. So the patch achieves the same behaviour as setting the delay > properties to 0. > > Current behaviour (without setting delay properties to 0): > FUNC COUNT > thermal_zone_device_update 302 > thermal_zone_device_set_pollin 7911 thermal_zone_device_set_polling? > > Current behaviour (with delay properties set to 0): > FUNC COUNT > thermal_zone_device_update 3 > thermal_zone_device_set_pollin 6 > > With this patch (without setting delay properties to 0): > FUNC COUNT > thermal_zone_device_update 3 > thermal_zone_device_set_pollin 6 > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Wed, Oct 16, 2019 at 10:27 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Amit Kucheria (2019-10-15 16:13:16) > > Currently, in order to enable interrupt-only mode, one must set > > polling-delay-passive and polling-delay properties in the DT to 0, > > otherwise the thermal framework will continue to setup a periodic timers > > to monitor the thermal zones. > > > > Change the behaviour, so that on DT-based systems, we no longer have to > > set the properties to zero if we find an 'interrupt' property in the > > sensor. > > > > Following data shows the number of times > > thermal_zone_device_set_polling() is invoked with and without this > > patch. So the patch achieves the same behaviour as setting the delay > > properties to 0. > > > > Current behaviour (without setting delay properties to 0): > > FUNC COUNT > > thermal_zone_device_update 302 > > thermal_zone_device_set_pollin 7911 > > thermal_zone_device_set_polling? Yes, the script I was using restricted the width of the fn name while printing. > > > > > Current behaviour (with delay properties set to 0): > > FUNC COUNT > > thermal_zone_device_update 3 > > thermal_zone_device_set_pollin 6 > > > > With this patch (without setting delay properties to 0): > > FUNC COUNT > > thermal_zone_device_update 3 > > thermal_zone_device_set_pollin 6 > > > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> >
On 16/10/2019 01:13, Amit Kucheria wrote: > Currently, in order to enable interrupt-only mode, one must set > polling-delay-passive and polling-delay properties in the DT to 0, > otherwise the thermal framework will continue to setup a periodic timers > to monitor the thermal zones. > > Change the behaviour, so that on DT-based systems, we no longer have to > set the properties to zero if we find an 'interrupt' property in the > sensor. > > Following data shows the number of times > thermal_zone_device_set_polling() is invoked with and without this > patch. So the patch achieves the same behaviour as setting the delay > properties to 0. > > Current behaviour (without setting delay properties to 0): > FUNC COUNT > thermal_zone_device_update 302 > thermal_zone_device_set_pollin 7911 > > Current behaviour (with delay properties set to 0): > FUNC COUNT > thermal_zone_device_update 3 > thermal_zone_device_set_pollin 6 > > With this patch (without setting delay properties to 0): > FUNC COUNT > thermal_zone_device_update 3 > thermal_zone_device_set_pollin 6 > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/of-thermal.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index dc5093be553e..79ad587462b1 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -412,7 +412,8 @@ static struct thermal_zone_device_ops of_thermal_ops = { > static struct thermal_zone_device * > thermal_zone_of_add_sensor(struct device_node *zone, > struct device_node *sensor, void *data, > - const struct thermal_zone_of_device_ops *ops) > + const struct thermal_zone_of_device_ops *ops, > + bool force_interrupts) > { > struct thermal_zone_device *tzd; > struct __thermal_zone *tz; > @@ -433,6 +434,11 @@ thermal_zone_of_add_sensor(struct device_node *zone, > tzd->ops->get_temp = of_thermal_get_temp; > tzd->ops->get_trend = of_thermal_get_trend; > > + if (force_interrupts) { > + tz->passive_delay = 0; > + tz->polling_delay = 0; > + } > + > /* > * The thermal zone core will calculate the window if they have set the > * optional set_trips pointer. > @@ -486,6 +492,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, > { > struct device_node *np, *child, *sensor_np; > struct thermal_zone_device *tzd = ERR_PTR(-ENODEV); > + bool force_interrupts = false; > > np = of_find_node_by_name(NULL, "thermal-zones"); > if (!np) > @@ -498,6 +505,9 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, > > sensor_np = of_node_get(dev->of_node); > > + if (of_find_property(sensor_np, "interrupts", NULL)) > + force_interrupts = true; > + This is hackish. It does cover only DT drivers. It would be cleaner to add a specific flag describing the thermal sensor driver mode and then in the core code do not start the timers if the associated works in interrupt. Moreover the interrupt mode can be used to activate faster the passive delay monitoring after reaching a threshold like the hikey and hikey960. So this patch will disable the mitigation on those boards. This is another argument to add flags for the thermal sensor mode. [ ... ]
On Wed, Oct 30, 2019 at 12:21 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 16/10/2019 01:13, Amit Kucheria wrote: > > Currently, in order to enable interrupt-only mode, one must set > > polling-delay-passive and polling-delay properties in the DT to 0, > > otherwise the thermal framework will continue to setup a periodic timers > > to monitor the thermal zones. > > > > Change the behaviour, so that on DT-based systems, we no longer have to > > set the properties to zero if we find an 'interrupt' property in the > > sensor. > > > > Following data shows the number of times > > thermal_zone_device_set_polling() is invoked with and without this > > patch. So the patch achieves the same behaviour as setting the delay > > properties to 0. > > > > Current behaviour (without setting delay properties to 0): > > FUNC COUNT > > thermal_zone_device_update 302 > > thermal_zone_device_set_pollin 7911 > > > > Current behaviour (with delay properties set to 0): > > FUNC COUNT > > thermal_zone_device_update 3 > > thermal_zone_device_set_pollin 6 > > > > With this patch (without setting delay properties to 0): > > FUNC COUNT > > thermal_zone_device_update 3 > > thermal_zone_device_set_pollin 6 > > > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > drivers/thermal/of-thermal.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > > index dc5093be553e..79ad587462b1 100644 > > --- a/drivers/thermal/of-thermal.c > > +++ b/drivers/thermal/of-thermal.c > > @@ -412,7 +412,8 @@ static struct thermal_zone_device_ops of_thermal_ops = { > > static struct thermal_zone_device * > > thermal_zone_of_add_sensor(struct device_node *zone, > > struct device_node *sensor, void *data, > > - const struct thermal_zone_of_device_ops *ops) > > + const struct thermal_zone_of_device_ops *ops, > > + bool force_interrupts) > > { > > struct thermal_zone_device *tzd; > > struct __thermal_zone *tz; > > @@ -433,6 +434,11 @@ thermal_zone_of_add_sensor(struct device_node *zone, > > tzd->ops->get_temp = of_thermal_get_temp; > > tzd->ops->get_trend = of_thermal_get_trend; > > > > + if (force_interrupts) { > > + tz->passive_delay = 0; > > + tz->polling_delay = 0; > > + } > > + > > /* > > * The thermal zone core will calculate the window if they have set the > > * optional set_trips pointer. > > @@ -486,6 +492,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, > > { > > struct device_node *np, *child, *sensor_np; > > struct thermal_zone_device *tzd = ERR_PTR(-ENODEV); > > + bool force_interrupts = false; > > > > np = of_find_node_by_name(NULL, "thermal-zones"); > > if (!np) > > @@ -498,6 +505,9 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, > > > > sensor_np = of_node_get(dev->of_node); > > > > + if (of_find_property(sensor_np, "interrupts", NULL)) > > + force_interrupts = true; > > + > > This is hackish. It does cover only DT drivers. OK. > It would be cleaner to add a specific flag describing the thermal sensor > driver mode and then in the core code do not start the timers if the > associated works in interrupt. > > Moreover the interrupt mode can be used to activate faster the passive > delay monitoring after reaching a threshold like the hikey and hikey960. > So this patch will disable the mitigation on those boards. This is > another argument to add flags for the thermal sensor mode. > So each driver then needs to set this flag at interrupt registration time. Or do you want to set that automatically in core code with some wrapper function? Regards, Amit
On 04/11/2019 07:26, Amit Kucheria wrote: > On Wed, Oct 30, 2019 at 12:21 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 16/10/2019 01:13, Amit Kucheria wrote: >>> Currently, in order to enable interrupt-only mode, one must set >>> polling-delay-passive and polling-delay properties in the DT to 0, >>> otherwise the thermal framework will continue to setup a periodic timers >>> to monitor the thermal zones. >>> >>> Change the behaviour, so that on DT-based systems, we no longer have to >>> set the properties to zero if we find an 'interrupt' property in the >>> sensor. >>> >>> Following data shows the number of times >>> thermal_zone_device_set_polling() is invoked with and without this >>> patch. So the patch achieves the same behaviour as setting the delay >>> properties to 0. >>> >>> Current behaviour (without setting delay properties to 0): >>> FUNC COUNT >>> thermal_zone_device_update 302 >>> thermal_zone_device_set_pollin 7911 >>> >>> Current behaviour (with delay properties set to 0): >>> FUNC COUNT >>> thermal_zone_device_update 3 >>> thermal_zone_device_set_pollin 6 >>> >>> With this patch (without setting delay properties to 0): >>> FUNC COUNT >>> thermal_zone_device_update 3 >>> thermal_zone_device_set_pollin 6 >>> >>> Suggested-by: Stephen Boyd <swboyd@chromium.org> >>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> >>> --- >>> drivers/thermal/of-thermal.c | 15 +++++++++++++-- >>> 1 file changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c >>> index dc5093be553e..79ad587462b1 100644 >>> --- a/drivers/thermal/of-thermal.c >>> +++ b/drivers/thermal/of-thermal.c >>> @@ -412,7 +412,8 @@ static struct thermal_zone_device_ops of_thermal_ops = { >>> static struct thermal_zone_device * >>> thermal_zone_of_add_sensor(struct device_node *zone, >>> struct device_node *sensor, void *data, >>> - const struct thermal_zone_of_device_ops *ops) >>> + const struct thermal_zone_of_device_ops *ops, >>> + bool force_interrupts) >>> { >>> struct thermal_zone_device *tzd; >>> struct __thermal_zone *tz; >>> @@ -433,6 +434,11 @@ thermal_zone_of_add_sensor(struct device_node *zone, >>> tzd->ops->get_temp = of_thermal_get_temp; >>> tzd->ops->get_trend = of_thermal_get_trend; >>> >>> + if (force_interrupts) { >>> + tz->passive_delay = 0; >>> + tz->polling_delay = 0; >>> + } >>> + >>> /* >>> * The thermal zone core will calculate the window if they have set the >>> * optional set_trips pointer. >>> @@ -486,6 +492,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, >>> { >>> struct device_node *np, *child, *sensor_np; >>> struct thermal_zone_device *tzd = ERR_PTR(-ENODEV); >>> + bool force_interrupts = false; >>> >>> np = of_find_node_by_name(NULL, "thermal-zones"); >>> if (!np) >>> @@ -498,6 +505,9 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, >>> >>> sensor_np = of_node_get(dev->of_node); >>> >>> + if (of_find_property(sensor_np, "interrupts", NULL)) >>> + force_interrupts = true; >>> + >> >> This is hackish. It does cover only DT drivers. > > OK. > >> It would be cleaner to add a specific flag describing the thermal sensor >> driver mode and then in the core code do not start the timers if the >> associated works in interrupt. >> >> Moreover the interrupt mode can be used to activate faster the passive >> delay monitoring after reaching a threshold like the hikey and hikey960. >> So this patch will disable the mitigation on those boards. This is >> another argument to add flags for the thermal sensor mode. >> > > So each driver then needs to set this flag at interrupt registration > time. Or do you want to set that automatically in core code with some > wrapper function? I'm not sure. Actually the 'mode' is now different from what we had previously as we can use the interrupt mode and polling mode used together, it is no longer 'enabled' / 'disabled'. We can have the following: ------------------------------------------------------------- | interrupt | passive interrupt | polling | passive polling | ------------------------------------------------------------- | X | | | X | ------------------------------------------------------------- | X | X | | | ------------------------------------------------------------- | | | X | X | ------------------------------------------------------------- The set_mode should be changed first in the of-thermal IMO, so we end up with something: #define THERMAL_DEVICE_MODE_SHUTDOWN 0x0 #define THERMAL_DEVICE_MODE_POLL 0x1 #define THERMAL_DEVICE_MODE_POLL_PASSIVE 0x2 #define THERMAL_DEVICE_MODE_INTERRUPT 0x4 #define THERMAL_DEVICE_MODE_INTERRUPT_PASSIVE 0x8 #define THERMAL_DEVICE_DISABLED THERMAL_DEVICE_MODE_SHUTDOWN #define THERMAL_DEVICE_ENABLED THERMAL_DEVICE_MODE_POLL|THERMAL_DEVICE_MODE_POLL_PASSIVE Then the function of_thermal_set_mode can be changed accordingly: tz->polling_delay = 0; tz->passive_delay = 0; if (mode & THERMAL_DEVICE_MODE_POLL) tz->polling_delay = data->polling_delay; if (mode & THERMAL_DEVICE_MODE_POLL_PASSIVE) tz->passive_delay = data->passive_delay; Then the drivers can call the tz's set_mode callback with the right parameter. But as I previously commented, that will be only for DT drivers. IMHO, it may make sense to add a mode field in the thermal_zone_device structure and a thermal_zone_set_mode() function (and take the opportunity to kill the set_mode callbacks in of-thermal and acpi/thermal.c as they are pointless with the new function).
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index dc5093be553e..79ad587462b1 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -412,7 +412,8 @@ static struct thermal_zone_device_ops of_thermal_ops = { static struct thermal_zone_device * thermal_zone_of_add_sensor(struct device_node *zone, struct device_node *sensor, void *data, - const struct thermal_zone_of_device_ops *ops) + const struct thermal_zone_of_device_ops *ops, + bool force_interrupts) { struct thermal_zone_device *tzd; struct __thermal_zone *tz; @@ -433,6 +434,11 @@ thermal_zone_of_add_sensor(struct device_node *zone, tzd->ops->get_temp = of_thermal_get_temp; tzd->ops->get_trend = of_thermal_get_trend; + if (force_interrupts) { + tz->passive_delay = 0; + tz->polling_delay = 0; + } + /* * The thermal zone core will calculate the window if they have set the * optional set_trips pointer. @@ -486,6 +492,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, { struct device_node *np, *child, *sensor_np; struct thermal_zone_device *tzd = ERR_PTR(-ENODEV); + bool force_interrupts = false; np = of_find_node_by_name(NULL, "thermal-zones"); if (!np) @@ -498,6 +505,9 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, sensor_np = of_node_get(dev->of_node); + if (of_find_property(sensor_np, "interrupts", NULL)) + force_interrupts = true; + for_each_available_child_of_node(np, child) { struct of_phandle_args sensor_specs; int ret, id; @@ -520,7 +530,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, if (sensor_specs.np == sensor_np && id == sensor_id) { tzd = thermal_zone_of_add_sensor(child, sensor_np, - data, ops); + data, ops, + force_interrupts); if (!IS_ERR(tzd)) tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
Currently, in order to enable interrupt-only mode, one must set polling-delay-passive and polling-delay properties in the DT to 0, otherwise the thermal framework will continue to setup a periodic timers to monitor the thermal zones. Change the behaviour, so that on DT-based systems, we no longer have to set the properties to zero if we find an 'interrupt' property in the sensor. Following data shows the number of times thermal_zone_device_set_polling() is invoked with and without this patch. So the patch achieves the same behaviour as setting the delay properties to 0. Current behaviour (without setting delay properties to 0): FUNC COUNT thermal_zone_device_update 302 thermal_zone_device_set_pollin 7911 Current behaviour (with delay properties set to 0): FUNC COUNT thermal_zone_device_update 3 thermal_zone_device_set_pollin 6 With this patch (without setting delay properties to 0): FUNC COUNT thermal_zone_device_update 3 thermal_zone_device_set_pollin 6 Suggested-by: Stephen Boyd <swboyd@chromium.org> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> --- drivers/thermal/of-thermal.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)