Re: [PATCH v2 02/11] thermal: add irq-mode configuration for trip point

From: Zhang Rui
Date: Thu Jan 10 2019 - 09:20:23 EST

Next message: Daniel Thompson: "Re: [PATCH v2 3/4] tee: add supp_nowait flag in tee_context struct"
Previous message: Paul E. McKenney: "Re: Possible use of RCU while in extended QS: idle vs RCU read-side in interrupt vs rcu_eqs_exit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Lukasz,

On å, 2018-12-06 at 20:55 +0100, Lukasz Luba wrote:
>
> 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?

sorry that I missed this thread.

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

So this solution does not cover case 1.
And for case 2, why not set passive_delay to 0? are they sharing the
same DT file?

> It would be possible after adding a new enum THERMAL_FRAMEWORK_POLL,Â
> then function:

hmmm, I think we can
1. use tz->passive to count the passive trip points that need passive
polling.
2. count tz->passive properly in the governors
3. always do passive polling when tz->passive > 0.

This will cover both cases, right?

some sample code to handle this in step_wise governor attached below,

what do you think?

thanks,
rui

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index ee047ca..59d9a1d 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -121,8 +121,15 @@ static void update_passive_instance(struct
thermal_zone_device *tz,
ÂÂÂÂÂÂÂÂÂ* If value is +1, activate a passive instance.
ÂÂÂÂÂÂÂÂÂ* If value is -1, deactivate a passive instance.
ÂÂÂÂÂÂÂÂÂ*/
-ÂÂÂÂÂÂÂif (type == THERMAL_TRIP_PASSIVE || type == THERMAL_TRIPS_NONE)
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtz->passive += value;
+ÂÂÂÂÂÂÂif (type != THERMAL_TRIP_PASSIVE && type != THERMAL_TRIPS_NONE)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
+ÂÂÂÂÂÂÂif (tz->ops->get_trip_irq_mode) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (tz->ops->get_trip_irq_mode(tz, trip, &irq_mode))
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (irq_mode)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
+ÂÂÂÂÂÂÂ}
+ÂÂÂÂÂÂÂtz->passive += value;
Â}

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

Next message: Daniel Thompson: "Re: [PATCH v2 3/4] tee: add supp_nowait flag in tee_context struct"
Previous message: Paul E. McKenney: "Re: Possible use of RCU while in extended QS: idle vs RCU read-side in interrupt vs rcu_eqs_exit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]