thermal: add new flag irq-mode for trip point

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 <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lukasz Luba <[email protected]>

---
 drivers/thermal/of-thermal.c   | 17 +++++++++++++++++ drivers/thermal/thermal_core.c | 10 ++++++++-- include/linux/thermal.h        |  5 +++++

 3 files changed, 30 insertions(+), 2 deletions(-)

drivers/thermal/of- renamed from drivers/thermal/of-thermal.c +0 -0

thermal.c
index 4bfdb4a..1a75946a 100644

drivers/thermal/of-thermal.c +0 -0

@@ -312,6 +312,20 @@ static int of_thermal_get_trip_type(struct
 thermal_zone_device *tz, int trip,
  	return 0;
  }

drivers/thermal/of-thermal.c +2 -0

+static int
+of_thermal_get_trip_irq_mode(struct thermal_zone_device *tz, int

trip,

drivers/thermal/of-thermal.c +12 -0

+			     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)
 {

drivers/thermal/of-thermal.c +0 -0

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

drivers/thermal/of-thermal.c +1 -0

+	.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,

drivers/thermal/of-thermal.c +0 -0

@@ -827,6 +842,8 @@ static int thermal_of_populate_trip(struct
 device_node *np,
  		return ret;
  	}

drivers/thermal/of-thermal.c +2 -0

+	trip->irq_mode = of_property_read_bool(np, "irq-mode");
+

  /* Required for cooling map matching */
  trip->np = np;
  of_node_get(np);

drivers/thermal/thermal_core.c +0 -0

index 39fc812..6d41e08 100644
@@ -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;

drivers/thermal/thermal_core.c +1 -0

+	bool irq_mode = false;

  /* Ignore disabled trip points */
  if (test_bit(trip, &tz->trips_disabled))

drivers/thermal/thermal_core.c +7 -2

@@ -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);
  }

 static void update_temperature(struct thermal_zone_device *tz)

include/linux/thermal.h +1 -0

index 5f4705f..b064565 100644
@@ -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 *);

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

@@ -371,6 +375,7 @@ struct thermal_trip {
  	int temperature;
  	int hysteresis;
  	enum thermal_trip_type type;
+	bool irq_mode;
  };

 /* Function declarations */

Quoted 99 lines · Lukasz Luba

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 <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lukasz Luba <[email protected]>

---
  drivers/thermal/of-thermal.c   | 17 +++++++++++++++++
  drivers/thermal/thermal_core.c | 10 ++++++++--
  include/linux/thermal.h        |  5 +++++
  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/thermal/of- renamed from drivers/thermal/of-thermal.c +0 -0

thermal.c
index 4bfdb4a..1a75946a 100644

drivers/thermal/of-thermal.c +14 -0

@@ -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)
 {

drivers/thermal/of-thermal.c +1 -0

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

drivers/thermal/of-thermal.c +2 -0

@@ -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);

drivers/thermal/thermal_core.c +1 -0

index 39fc812..6d41e08 100644
@@ -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))

drivers/thermal/thermal_core.c +7 -2

@@ -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);

drivers/thermal/thermal_core.c static void handle_thermal_trip(struct · line 433

Quoted 2 lines

drivers/thermal/thermal_core.c +0 -0

  }
 

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?

thanks,
rui

Quoted 44 lines

 static void update_temperature(struct thermal_zone_device *tz)

include/linux/thermal.h +1 -0

index 5f4705f..b064565 100644
@@ -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
*);

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

@@ -371,6 +375,7 @@ struct thermal_trip {
  	int temperature;
  	int hysteresis;
  	enum thermal_trip_type type;
+	bool irq_mode;
  };
 
  /* Function declarations */

Hi Rui,

Quoted 95 lines · Zhang Rui

Quoted 94 lines · Lukasz Luba

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 <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lukasz Luba <[email protected]>

---
  drivers/thermal/of-thermal.c   | 17 +++++++++++++++++ drivers/thermal/thermal_core.c | 10 ++++++++-- include/linux/thermal.h        |  5 +++++

  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/thermal/of- renamed from drivers/thermal/of-thermal.c +0 -0

thermal.c
index 4bfdb4a..1a75946a 100644

drivers/thermal/of-thermal.c +14 -0

@@ -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)
  {

drivers/thermal/of-thermal.c +3 -0

@@ -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);

drivers/thermal/thermal_core.c +0 -0

index 39fc812..6d41e08 100644
@@ -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))

drivers/thermal/thermal_core.c +6 -2

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

Quoted 8 lines

drivers/thermal/thermal_core.c static void handle_thermal_trip(struct thermal_zone_device *tz, int trip) · line 431

Quoted 2 lines

drivers/thermal/thermal_core.c +1 -0

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

Regards,
Lukasz

Quoted 48 lines

thanks,
rui

Quoted 42 lines

  static void update_temperature(struct thermal_zone_device *tz)

include/linux/thermal.h +1 -0

index 5f4705f..b064565 100644
@@ -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 *);

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

@@ -371,6 +375,7 @@ struct thermal_trip {
       int temperature;
       int hysteresis;
       enum thermal_trip_type type;
+    bool irq_mode;
   };
   /* Function declarations */

Quoted 108 lines · Lukasz Luba

Hi Rui,

Quoted 104 lines · Zhang Rui

Quoted 91 lines · Lukasz Luba

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 <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lukasz Luba <[email protected]>

---
  drivers/thermal/of-thermal.c   | 17 +++++++++++++++++ drivers/thermal/thermal_core.c | 10 ++++++++-- include/linux/thermal.h        |  5 +++++

  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/thermal/of- renamed from drivers/thermal/of-thermal.c +0 -0

thermal.c
index 4bfdb4a..1a75946a 100644

drivers/thermal/of-thermal.c +2 -0

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

drivers/thermal/of-thermal.c +11 -0

+{
+    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)
  {

drivers/thermal/of-thermal.c +3 -0

@@ -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);

drivers/thermal/thermal_core.c +0 -0

index 39fc812..6d41e08 100644
@@ -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))

drivers/thermal/thermal_core.c +0 -1

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

drivers/thermal/thermal_core.c static void handle_thermal_trip(struct · line 432

Quoted 10 lines

drivers/thermal/thermal_core.c +7 -1

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

Quoted 8 lines

Quoted 8 lines

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?

Quoted 5 lines

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

Quoted 8 lines

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

Quoted 59 lines

Regards,
Lukasz

Quoted 54 lines

thanks,
rui

Quoted 48 lines

  static void update_temperature(struct thermal_zone_device *tz)

include/linux/thermal.h +1 -0

index 5f4705f..b064565 100644
@@ -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
*);

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

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

include/linux/thermal.h +1 -0

@@ -371,6 +375,7 @@ struct thermal_trip {
       int temperature;
       int hysteresis;
       enum thermal_trip_type type;
+    bool irq_mode;
   };
   /* Function declarations */

Thermal trip point gets new flag in DT: irq-mode.
Trip point may have a new explicit flag which indicate IRQ support when the temperature is met (so the thermal framework deos not need to set polling for it).
It is useful for 'passive' cooling trip point,
which now will not register for polling the temperature.

Update documentation about irq-mode for trip points.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Rob Herring <[email protected]>

Cc Mark Rutland <[email protected]>

Cc devicetree at vger.kernel.org

Signed-off-by Lukasz Luba <[email protected]>

---
 Documentation/devicetree/bindings/thermal/thermal.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/thermal/thermal.txt +0 -0

index ca14ba9..bee21e3 100644

Documentation/devicetree/bindings/thermal/thermal.txt +6 -1

+  Type: bool           need of polling in thermal framework.
+  Size: one cell
+
 * Cooling device maps
 
 The cooling device maps node is a node to describe how cooling devices

@@ -256,16 +260,19 @@ thermal-zones {
                                temperature = <90000>; /* millicelsius */
                                hysteresis = <2000>; /* millicelsius */
                                type = "active";
+                               irq-mode;
                        };
                        cpu_alert1: cpu-alert1 {
                                temperature = <100000>; /* millicelsius */
                                hysteresis = <2000>; /* millicelsius */
                                type = "passive";
+                               irq-mode;
                        };
                        cpu_crit: cpu-crit {
                                temperature = <125000>; /* millicelsius */
                                hysteresis = <2000>; /* millicelsius */
                                type = "critical";
+                               irq-mode;
                        };
                };
 
--

2.7.4

Quoted 1 line · Lukasz Luba

Subject prefix:
dt-bindings: thermal:

Quoted 6 lines

Thermal trip point gets new flag in DT: irq-mode.
Trip point may have a new explicit flag which indicate IRQ support when the temperature is met (so the thermal framework deos not need to set polling for it).
It is useful for 'passive' cooling trip point,
which now will not register for polling the temperature.

You wrap lines in weird way making it more difficult to read. I already asked about this while reviewing v1. Please fix it.
https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L633

Quoted 22 lines

Update documentation about irq-mode for trip points.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Rob Herring <[email protected]>

Cc Mark Rutland <[email protected]>

Cc devicetree at vger.kernel.org

Signed-off-by Lukasz Luba <[email protected]>

---
  Documentation/devicetree/bindings/thermal/thermal.txt | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/thermal/thermal.txt +0 -0

index ca14ba9..bee21e3 100644
@@ -90,6 +90,10 @@ Required properties:
         "critical":     Hardware not reliable.
    Type: string
 

Documentation/devicetree/bindings/thermal/thermal.txt Required properties: · line 93

Quoted 1 line

Documentation/devicetree/bindings/thermal/thermal.txt +1 -0

+- irq-mode:            A flag indicating that trip rises irq, so there is no

"rises IRQ" (it is an abbreviation).

Best regards,
Krzysztof

Thermal trip point gets new flag in DT: irq-mode.
Trip point may have a new explicit flag which indicate IRQ support when the temperature is met (so the thermal framework deos not need to set polling for it).
It is useful for 'passive' cooling trip point,
which now will not register for polling the temperature.

Update documentation about irq-mode for trip points.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Rob Herring <[email protected]>

Cc Mark Rutland <[email protected]>

Cc devicetree at vger.kernel.org

Signed-off-by Lukasz Luba <[email protected]>

---
 Documentation/devicetree/bindings/thermal/thermal.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/thermal/thermal.txt +0 -0

index ca14ba9..bee21e3 100644

Documentation/devicetree/bindings/thermal/thermal.txt +4 -0

+
 * Cooling device maps
 
 The cooling device maps node is a node to describe how cooling devices

@@ -256,16 +260,19 @@ thermal-zones {
 				temperature = <90000>; /* millicelsius */
 				hysteresis = <2000>; /* millicelsius */
 				type = "active";
+				irq-mode;
 			};
 			cpu_alert1: cpu-alert1 {
 				temperature = <100000>; /* millicelsius */
 				hysteresis = <2000>; /* millicelsius */
 				type = "passive";
+				irq-mode;
 			};
 			cpu_crit: cpu-crit {
 				temperature = <125000>; /* millicelsius */
 				hysteresis = <2000>; /* millicelsius */
 				type = "critical";
+				irq-mode;
 			};
 		};
 

Signature · 1 line

2.7.4

Quoted 8 lines · Lukasz Luba

Thermal trip point gets new flag in DT: irq-mode.
Trip point may have a new explicit flag which indicate IRQ support when the temperature is met (so the thermal framework deos not need to set polling for it).
It is useful for 'passive' cooling trip point,
which now will not register for polling the temperature.

Update documentation about irq-mode for trip points.

This patch should come before you use it.

Quoted 21 lines

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Rob Herring <[email protected]>

Cc Mark Rutland <[email protected]>

Cc devicetree at vger.kernel.org

Signed-off-by Lukasz Luba <[email protected]>

---
  Documentation/devicetree/bindings/thermal/thermal.txt | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/thermal/thermal.txt +0 -0

index ca14ba9..bee21e3 100644
@@ -90,6 +90,10 @@ Required properties:
  	"critical":	Hardware not reliable.
    Type: string
 

Documentation/devicetree/bindings/thermal/thermal.txt Required properties: · line 95

Quoted 3 lines

Documentation/devicetree/bindings/thermal/thermal.txt +3 -0

+- irq-mode:		A flag indicating that trip rises irq, so there is no
+  Type: bool		need of polling in thermal framework.
+  Size: one cell

Should be optional, right?

This patch adds support for new flag which indicates

that trip point triggers IRQ when temperature is met. Exynos5433 supports 8 trip point which will trigger IRQ.

Above that number other trip points should be registered without 'irq-mode' flag.

Cc Kukjin Kim <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc devicetree at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc linux-samsung-soc at vger.kernel.org

Signed-off-by Lukasz Luba <[email protected]>

---
 arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi | 105 ++++++++++++++++---------
 1 file changed, 70 insertions(+), 35 deletions(-)

arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi +0 -0

index fe3a0b1..c4330f6 100644

Thanks Lukasz for patches. I like your work!

I have few more comments which will probably apply for all the DTS commits.

Quoted 2 lines · Lukasz Luba

This patch adds support for new flag which indicates

This patch does not add support. Support for flag was added in your first drivers/thermal patches in this set. This patch adds new flag. (and does something more, so read on)

Quoted 3 lines

that trip point triggers IRQ when temperature is met. Exynos5433 supports 8 trip point which will trigger IRQ.

/8 trip point/8 trip points/

Quoted 2 lines

Above that number other trip points should be registered without 'irq-mode' flag.

Please fix the line-wrap.

Quoted 17 lines

Cc Kukjin Kim <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc devicetree at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc linux-samsung-soc at vger.kernel.org

Signed-off-by Lukasz Luba <[email protected]>

---
  arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi | 105 ++++++++++++++++---------
  1 file changed, 70 insertions(+), 35 deletions(-)

arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi +0 -0

index fe3a0b1..c4330f6 100644

arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi thermal-zones { · line 20

Quoted 5 lines

arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi +1 -1

@@ -17,37 +17,44 @@ thermal-zones {
                         atlas0_alert_0: atlas0-alert-0 {
                                 temperature = <65000>;  /* millicelsius */
                                 hysteresis = <1000>;    /* millicelsius */
-                               type = "active";
+                               type = "passive";

This change is not explained in commit msg.

Basically you are doing here two things (related to each other). You clearly define which trip points are IRQ-type and you correct the type of trip point. Active is incorrect. This second thing is missing in your commit msg and I believe it is main reason behind this patch. You should focus then on this reason - start from it. Subject could be like "Use proper passive type for thermal trip points".

Without this explanation I don't see the strong reason behind that patch. IOW, everything was working fine before... so why adding this new flag? Or maybe something was not fine... and then it is real reason why you are sending the patch. Usually commit message should answer to the most important question "WHY". Now, it explains "WHAT"... but I can see it from the source code. However from the code it is not easy to guess WHY.

Best regards,
Krzysztof

Hi all,

This patch set adds a new flag and mechanism in thermal trip points in
DT.  The old implementation in thermal zone in DT sets the same configuration for all internal trip points. It does not work for all HW.  There are SoCs which support IRQs for some trip points (i.e. Exynos 4 has 4 trip points with IRQs). For additional one defined inside the thermal zone there is a need of 'polling'. When developer adds polling mode settings inside the thermal zone, all the trip points will be registered for polling, even those supporting IRQs, which does not make sense.

Thus, developers create workarounds, which
are confusing for some other developers.  To workaround, people declare some trip points as 'active' (those with IRQ support). It allows to bypass polling mode in thermal framework applied for all thermal zone's trip points.

Thermal framework defines 4 types of trip points. The 'passive' means passive cooling using DVFS, 'active' is designed for fan and other devices actively changing the outside conditions. Therefore, a workaround
mentioned earlier is confusing when someone does not know about the framework limitations.

This patch set tries to solve the issue by adding one flag inside the trip point: 'irq-mode;'.  The trip point 'passive' declared in DT with
explicit flag 'irq-mode;' will not register itself as polling mode. Thermal framework will skip it during scheduling next read out work. The old global-polling-mode-configuration-inside-thermal-zone is still
valid.  Patch set does not break existing design for trip points which
do not have 'irq-mode' flag - they will use polling.

As an example please check patch #10 for Exynos4 SoC family, where there
is 4 HW supported trip points and there is a need of 6. The rest 2 are
declared as 'passive' without 'irq-mode;' flag, which means polling mode needed for them.

Patch #1 is a small cleanup in thermal framework.

Change log:
v2
- changed description in cover letter
- change commit messages according to Krzysztof comments
- rebase on top of current mainline (v4.20-rc1)

Regards,
Lukasz Luba

Lukasz Luba (11):
  thermal: remove unused function parameter
  thermal: add irq-mode configuration for trip point
  thermal: add new sysfs file for irq-mode
  Doc: thermal: new irq-mode for trip point
  Doc: DT: thermal: new irq-mode for trip point
  arm64: dts: exynos5433: add support for thermal trip irq-mode
  arm64: dts: exynos7: add support for thermal trip irq-mode
  arm: dts: exynos4: add support for thermal trip irq-mode
  arm: dts: exynos5420: add support for thermal trip irq-mode
  arm: dts: exynos5422: add support for thermal trip irq-mode
  arm: dts: exynos5410: add support for thermal trip irq-mode

 .../devicetree/bindings/thermal/thermal.txt        |   7 ++ Documentation/thermal/sysfs-api.txt                |   9 ++ arch/arm/boot/dts/exynos4-cpu-thermal.dtsi         |  10 +- arch/arm/boot/dts/exynos5410-odroidxu.dts          |  10 +- arch/arm/boot/dts/exynos5420-trip-points.dtsi      |  10 +- arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi |  40 +++++--- arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi     | 105
++++++++++++++-------
 .../arm64/boot/dts/exynos/exynos7-trip-points.dtsi |   8 ++ drivers/thermal/of-thermal.c                       |  17 ++++ drivers/thermal/thermal_core.c                     |  16 ++-- drivers/thermal/thermal_sysfs.c                    |  53 ++++++++++- include/linux/thermal.h                            |   5 +

 12 files changed, 226 insertions(+), 64 deletions(-)

Quoted 12 lines · Lukasz Luba

Hi all,

This patch set adds a new flag and mechanism in thermal trip points in
DT.  The old implementation in thermal zone in DT sets the same configuration for all internal trip points. It does not work for all HW.  There are SoCs which support IRQs for some trip points (i.e. Exynos 4 has 4 trip points with IRQs). For additional one defined inside the thermal zone there is a need of 'polling'. When developer adds polling mode settings inside the thermal zone, all the trip points will be registered for polling, even those supporting IRQs, which does not make sense.

we have two timers, one for polling, and one for passive cooling. I think we are talking about passive cooling timer only, right? And the real problem is that we have multiple passive trip points and only part of them support irq_mode, and we don't want to start the passive polling timer for all of the passive trip points.

thanks,
rui

Quoted 70 lines

Thus, developers create workarounds, which
are confusing for some other developers.  To workaround, people declare some trip points as 'active' (those with IRQ support). It allows to bypass polling mode in thermal framework applied for all thermal zone's trip points.

Thermal framework defines 4 types of trip points. The 'passive' means passive cooling using DVFS, 'active' is designed for fan and other devices actively changing the outside conditions. Therefore, a workaround
mentioned earlier is confusing when someone does not know about the framework limitations.

This patch set tries to solve the issue by adding one flag inside the trip point: 'irq-mode;'.  The trip point 'passive' declared in DT with
explicit flag 'irq-mode;' will not register itself as polling mode. Thermal framework will skip it during scheduling next read out work. The old global-polling-mode-configuration-inside-thermal-zone is still
valid.  Patch set does not break existing design for trip points which
do not have 'irq-mode' flag - they will use polling.

As an example please check patch #10 for Exynos4 SoC family, where there
is 4 HW supported trip points and there is a need of 6. The rest 2 are
declared as 'passive' without 'irq-mode;' flag, which means polling mode needed for them.

Patch #1 is a small cleanup in thermal framework.

Change log:
v2
- changed description in cover letter
- change commit messages according to Krzysztof comments
- rebase on top of current mainline (v4.20-rc1)

Regards,
Lukasz Luba

Lukasz Luba (11):
  thermal: remove unused function parameter
  thermal: add irq-mode configuration for trip point
  thermal: add new sysfs file for irq-mode
  Doc: thermal: new irq-mode for trip point
  Doc: DT: thermal: new irq-mode for trip point
  arm64: dts: exynos5433: add support for thermal trip irq-mode
  arm64: dts: exynos7: add support for thermal trip irq-mode
  arm: dts: exynos4: add support for thermal trip irq-mode
  arm: dts: exynos5420: add support for thermal trip irq-mode
  arm: dts: exynos5422: add support for thermal trip irq-mode
  arm: dts: exynos5410: add support for thermal trip irq-mode

  .../devicetree/bindings/thermal/thermal.txt        |   7 ++
  Documentation/thermal/sysfs-api.txt                |   9 ++
  arch/arm/boot/dts/exynos4-cpu-thermal.dtsi         |  10 +-
  arch/arm/boot/dts/exynos5410-odroidxu.dts          |  10 +-
  arch/arm/boot/dts/exynos5420-trip-points.dtsi      |  10 +-
  arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi |  40 +++++---
  arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi     | 105
++++++++++++++-------
  .../arm64/boot/dts/exynos/exynos7-trip-points.dtsi |   8 ++
  drivers/thermal/of-thermal.c                       |  17 ++++
  drivers/thermal/thermal_core.c                     |  16 ++--
  drivers/thermal/thermal_sysfs.c                    |  53 ++++++++++-
  include/linux/thermal.h                            |   5 +
  12 files changed, 226 insertions(+), 64 deletions(-)