On 12/6/18 8:18 PM, Lukasz Luba wrote:
> Hi Rui,
>
> On 12/5/18 4:09 PM, Zhang Rui wrote:
>> On 三, 2018-11-07 at 18:09 +0100, Lukasz Luba wrote:
>>> This patch adds support irq mode in trip point.
>>> When that flag is set in DT, there is no need for polling
>>> in thermal framework. Crossing the trip point will rise an IRQ.
>>> The naming convention for tip point 'type' can be confussing
>>> and 'passive' (whic is passive cooling) might be interpretted
>>> wrongly.
>>>
>>> This mechanism prevents from missue and adds explicit setting
>>> for hardware which support interrupts for pre-configured temperature
>>> threshold.
>>>
>>> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
>>> Cc: Eduardo Valentin <edubezval@xxxxxxxxx>
>>> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>>> Signed-off-by: Lukasz Luba <l.luba@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/thermal/of-thermal.c | 17 +++++++++++++++++
>>> drivers/thermal/thermal_core.c | 10 ++++++++--
>>> include/linux/thermal.h | 5 +++++
>>> 3 files changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-
>>> thermal.c
>>> index 4bfdb4a..1a75946a 100644
>>> --- a/drivers/thermal/of-thermal.c
>>> +++ b/drivers/thermal/of-thermal.c
>>> @@ -312,6 +312,20 @@ static int of_thermal_get_trip_type(struct
>>> thermal_zone_device *tz, int trip,
>>> return 0;
>>> }
>>> +static int
>>> +of_thermal_get_trip_irq_mode(struct thermal_zone_device *tz, int
>>> trip,
>>> + bool *mode)
>>> +{
>>> + struct __thermal_zone *data = tz->devdata;
>>> +
>>> + if (trip >= data->ntrips || trip < 0)
>>> + return -EDOM;
>>> +
>>> + *mode = data->trips[trip].irq_mode;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int of_thermal_get_trip_temp(struct thermal_zone_device *tz,
>>> int trip,
>>> int *temp)
>>> {
>>> @@ -394,6 +408,7 @@ static struct thermal_zone_device_ops
>>> of_thermal_ops = {
>>> .set_mode = of_thermal_set_mode,
>>> .get_trip_type = of_thermal_get_trip_type,
>>> + .get_trip_irq_mode = of_thermal_get_trip_irq_mode,
>>> .get_trip_temp = of_thermal_get_trip_temp,
>>> .set_trip_temp = of_thermal_set_trip_temp,
>>> .get_trip_hyst = of_thermal_get_trip_hyst,
>>> @@ -827,6 +842,8 @@ static int thermal_of_populate_trip(struct
>>> device_node *np,
>>> return ret;
>>> }
>>> + trip->irq_mode = of_property_read_bool(np, "irq-mode");
>>> +
>>> /* Required for cooling map matching */
>>> trip->np = np;
>>> of_node_get(np);
>>> diff --git a/drivers/thermal/thermal_core.c
>>> b/drivers/thermal/thermal_core.c
>>> index 39fc812..6d41e08 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -406,6 +406,7 @@ static void handle_critical_trips(struct
>>> thermal_zone_device *tz,
>>> static void handle_thermal_trip(struct thermal_zone_device *tz, int
>>> trip)
>>> {
>>> enum thermal_trip_type type;
>>> + bool irq_mode = false;
>>> /* Ignore disabled trip points */
>>> if (test_bit(trip, &tz->trips_disabled))
>>> @@ -419,9 +420,14 @@ static void handle_thermal_trip(struct
>>> thermal_zone_device *tz, int trip)
>>> handle_non_critical_trips(tz, trip);
>>> /*
>>> * Alright, we handled this trip successfully.
>>> - * So, start monitoring again.
>>> + * So, start monitoring in polling mode if
>>> + * trip is not using irq HW support.
>>> */
>>> - monitor_thermal_zone(tz);
>>> + if (tz->ops->get_trip_irq_mode)
>>> + tz->ops->get_trip_irq_mode(tz, trip, &irq_mode);
>>> +
>>> + if (!irq_mode)
>>> + monitor_thermal_zone(tz);
>>> }
>> handle_thermal_trip() is called from thermal_zone_device_update(), and
>> it is invoked for every trip points.
>> say, you have a passive trip point 1 that supports irq_mode, and
>> another passive trip point 2 that does not support irq_mode,
>> monitor_thermal_zone() is still called in handle_thermal_trip(tz, 2),
>> and the passive timer will be activated anyway, do I miss something?
> Yes, the passive timer will be activated in your example. It is correct
> behavior and does not break anything.
> case 1: there is 'k' passive trip points which have irq_mode and 1
> additional which does not have 'irq_mode', the framework will start
> polling but skip check for those 'k' trip points.
> case 2: all of the passive trip points have irq_mode set, the framework
> will not register 'scheduled_work' because it will not call
> 'monitor_thermal_zone()'.
> This is the case of most Exynos platforms, but there is one exception
> which has 'case 1' with 2 trip points not supporting irq_mode.
Do you suggest to cover the 'case 1'?
It would be possible after adding a new enum THERMAL_FRAMEWORK_POLL,
then function:
thermal_zone_device_check() will call
thermal_zone_device_update(tz, THERMAL_FRAMEWORK_POLL)
and in handle_thermal_trip() implement something like:
--------------8<----------------
static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
{
enum thermal_trip_type type;
bool irq_mode = false;
/* Ignore disabled trip points */
if (test_bit(trip, &tz->trips_disabled))
return;
if (tz->ops->get_trip_irq_mode)
tz->ops->get_trip_irq_mode(tz, trip, &irq_mode)
if (tz->notify_event == THERMAL_FRAMEWORK_POLL && irq_mode)
return;
...
if (!irq_mode)
monitor_thermal_zone(tz);
}
---------->8-----------------------
I could implement it in v3 if you don't see that it add too much of mess
and agree for this approach.
Regards,
Lukasz
>
> Regards,
> Lukasz
>
>>
>> thanks,
>> rui
>>
>>> static void update_temperature(struct thermal_zone_device *tz)
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index 5f4705f..b064565 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -103,6 +103,7 @@ struct thermal_zone_device_ops {
>>> enum thermal_device_mode);
>>> int (*get_trip_type) (struct thermal_zone_device *, int,
>>> enum thermal_trip_type *);
>>> + int (*get_trip_irq_mode) (struct thermal_zone_device *, int,
>>> bool *);
>>> int (*get_trip_temp) (struct thermal_zone_device *, int, int
>>> *);
>>> int (*set_trip_temp) (struct thermal_zone_device *, int,
>>> int);
>>> int (*get_trip_hyst) (struct thermal_zone_device *, int, int
>>> *);
>>> @@ -196,6 +197,7 @@ struct thermal_zone_device {
>>> struct thermal_attr *trip_temp_attrs;
>>> struct thermal_attr *trip_type_attrs;
>>> struct thermal_attr *trip_hyst_attrs;
>>> + struct thermal_attr *trip_irq_mode_attrs;
>>> void *devdata;
>>> int trips;
>>> unsigned long trips_disabled; /* bitmap for disabled
>>> trips */
>>> @@ -364,6 +366,8 @@ struct thermal_zone_of_device_ops {
>>> * @temperature: temperature value in miliCelsius
>>> * @hysteresis: relative hysteresis in miliCelsius
>>> * @type: trip point type
>>> + * @irq_mode: to not use polling in framework set support of HW irq
>>> (which will
>>> + * be triggered when temperature reaches this level).
>>> */
>>> struct thermal_trip {
>>> @@ -371,6 +375,7 @@ struct thermal_trip {
>>> int temperature;
>>> int hysteresis;
>>> enum thermal_trip_type type;
>>> + bool irq_mode;
>>> };
>>> /* Function declarations */
>>
>>
