Re: [PATCH] of-thermal: Disable polling when interrupt property is found in DT

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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).

-- 
 <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