[PATCH 0/7] Minor cleanup for thermal gov power allocator

[PATCH 0/7] Minor cleanup for thermal gov power allocator

[Lukasz Luba]

Hi all,

The patch set does some small clean up for Intelligent Power Allocator.
Those changes are not expected to alter the general functionality. They just
improve the code reading. Only patch 3/7 might improve the use case for
binding the governor to thermal zone (very unlikely in real products, but
it's needed for correctness).

The changes are based on top of current PM thermal branch, so with the
new trip points.

Regards,
Lukasz

Lukasz Luba (7):
  thermal: gov_power_allocator: Rename trip_max_desired_temperature
  thermal: gov_power_allocator: Setup trip points earlier
  thermal: gov_power_allocator: Check the cooling devices only for
    trip_max
  thermal: gov_power_allocator: Rearrange the order of variables
  thermal: gov_power_allocator: Use shorter variable when possible
  thermal: gov_power_allocator: Remove unneeded local variables
  thermal: gov_power_allocator: Clean needed variables at the beginning

 drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
 1 file changed, 64 insertions(+), 59 deletions(-)

-- 
2.25.1

[PATCH 1/7] thermal: gov_power_allocator: Rename trip_max_desired_temperature

[Lukasz Luba]

Refactor the code and drop the long name for the trip point. There is
a comment describing the field properly. Use shorten variable name so that
allow to make the code cleaner a bit.

This change is not expected to alter the general functionality.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 40 ++++++++++++---------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 83d4f451b1a97..97a8a6e4e1b0b 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -59,9 +59,8 @@ static inline s64 div_frac(s64 x, s64 y)
  *			governor switches on when this trip point is crossed.
  *			If the thermal zone only has one passive trip point,
  *			@trip_switch_on should be NULL.
- * @trip_max_desired_temperature:	last passive trip point of the thermal
- *					zone.  The temperature we are
- *					controlling for.
+ * @trip_max:		last passive trip point of the thermal zone. The
+ *			temperature we are controlling for.
  */
 struct power_allocator_params {
 	bool allocated_tzp;
@@ -69,7 +68,7 @@ struct power_allocator_params {
 	s32 prev_err;
 	u32 sustainable_power;
 	const struct thermal_trip *trip_switch_on;
-	const struct thermal_trip *trip_max_desired_temperature;
+	const struct thermal_trip *trip_max;
 };
 
 /**
@@ -93,7 +92,7 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
 		struct thermal_cooling_device *cdev = instance->cdev;
 		u32 min_power;
 
-		if (instance->trip != params->trip_max_desired_temperature)
+		if (instance->trip != params->trip_max)
 			continue;
 
 		if (!cdev_is_power_actor(cdev))
@@ -379,8 +378,7 @@ static int allocate_power(struct thermal_zone_device *tz,
 {
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
-	const struct thermal_trip *trip_max_desired_temperature =
-					params->trip_max_desired_temperature;
+	const struct thermal_trip *trip_max = params->trip_max;
 	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
 	u32 *weighted_req_power;
 	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
@@ -390,7 +388,7 @@ static int allocate_power(struct thermal_zone_device *tz,
 	num_actors = 0;
 	total_weight = 0;
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if ((instance->trip == trip_max_desired_temperature) &&
+		if ((instance->trip == trip_max) &&
 		    cdev_is_power_actor(instance->cdev)) {
 			num_actors++;
 			total_weight += instance->weight;
@@ -429,7 +427,7 @@ static int allocate_power(struct thermal_zone_device *tz,
 		int weight;
 		struct thermal_cooling_device *cdev = instance->cdev;
 
-		if (instance->trip != trip_max_desired_temperature)
+		if (instance->trip != trip_max)
 			continue;
 
 		if (!cdev_is_power_actor(cdev))
@@ -465,7 +463,7 @@ static int allocate_power(struct thermal_zone_device *tz,
 	total_granted_power = 0;
 	i = 0;
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != trip_max_desired_temperature)
+		if (instance->trip != trip_max)
 			continue;
 
 		if (!cdev_is_power_actor(instance->cdev))
@@ -531,13 +529,13 @@ static void get_governor_trips(struct thermal_zone_device *tz,
 
 	if (last_passive) {
 		params->trip_switch_on = first_passive;
-		params->trip_max_desired_temperature = last_passive;
+		params->trip_max = last_passive;
 	} else if (first_passive) {
 		params->trip_switch_on = NULL;
-		params->trip_max_desired_temperature = first_passive;
+		params->trip_max = first_passive;
 	} else {
 		params->trip_switch_on = NULL;
-		params->trip_max_desired_temperature = last_active;
+		params->trip_max = last_active;
 	}
 }
 
@@ -556,8 +554,8 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		struct thermal_cooling_device *cdev = instance->cdev;
 
-		if (instance->trip != params->trip_max_desired_temperature ||
-		    (!cdev_is_power_actor(instance->cdev)))
+		if (instance->trip != params->trip_max ||
+		    !cdev_is_power_actor(instance->cdev))
 			continue;
 
 		instance->target = 0;
@@ -642,12 +640,10 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 
 	get_governor_trips(tz, params);
 
-	if (params->trip_max_desired_temperature) {
-		int temp = params->trip_max_desired_temperature->temperature;
-
+	if (params->trip_max)
 		estimate_pid_constants(tz, tz->tzp->sustainable_power,
-				       params->trip_switch_on, temp);
-	}
+				       params->trip_switch_on,
+				       params->trip_max->temperature);
 
 	reset_pid_controller(params);
 
@@ -688,7 +684,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz,
 	 * We get called for every trip point but we only need to do
 	 * our calculations once
 	 */
-	if (trip != params->trip_max_desired_temperature)
+	if (trip != params->trip_max)
 		return 0;
 
 	trip = params->trip_switch_on;
@@ -702,7 +698,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz,
 
 	tz->passive = 1;
 
-	return allocate_power(tz, params->trip_max_desired_temperature->temperature);
+	return allocate_power(tz, params->trip_max->temperature);
 }
 
 static struct thermal_governor thermal_gov_power_allocator = {
-- 
2.25.1

[PATCH 2/7] thermal: gov_power_allocator: Setup trip points earlier

[Lukasz Luba]

Setup the trip points at the beginning of the binding function. This
simplifies the code a bit and allows further cleaning. Add also the
check if the last passive trip point is found and fail binding if not.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 97a8a6e4e1b0b..0dfc5b5ab5235 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -617,14 +617,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	int ret;
 	struct power_allocator_params *params;
 
-	ret = check_power_actors(tz);
-	if (ret)
-		return ret;
-
 	params = kzalloc(sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
+	get_governor_trips(tz, params);
+	if (!params->trip_max) {
+		dev_warn(&tz->device, "power_allocator: missing trip_max\n");
+		kfree(params);
+		return -EINVAL;
+	}
+
+	ret = check_power_actors(tz);
+	if (ret) {
+		dev_warn(&tz->device, "power_allocator: binding failed\n");
+		kfree(params);
+		return ret;
+	}
+
 	if (!tz->tzp) {
 		tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
 		if (!tz->tzp) {
@@ -638,12 +648,9 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	if (!tz->tzp->sustainable_power)
 		dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
 
-	get_governor_trips(tz, params);
-
-	if (params->trip_max)
-		estimate_pid_constants(tz, tz->tzp->sustainable_power,
-				       params->trip_switch_on,
-				       params->trip_max->temperature);
+	estimate_pid_constants(tz, tz->tzp->sustainable_power,
+			       params->trip_switch_on,
+			       params->trip_max->temperature);
 
 	reset_pid_controller(params);
 
-- 
2.25.1

[PATCH 3/7] thermal: gov_power_allocator: Check the cooling devices only for trip_max

[Lukasz Luba]

The throttling logic takes care only for the last passive trip point and
attached cooling devices to it. Therefore, there is no need to bail out if
other trip points have a cooling device which is not a supported by IPA.

Check the cooling devices only for the 'trip_max' during the binding.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 0dfc5b5ab5235..4b9ef04577a9a 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -578,6 +578,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
  * check_power_actors() - Check all cooling devices and warn when they are
  *			not power actors
  * @tz:		thermal zone to operate on
+ * @params:	power allocator private data
  *
  * Check all cooling devices in the @tz and warn every time they are missing
  * power actor API. The warning should help to investigate the issue, which
@@ -586,12 +587,16 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
  * Return: 0 on success, -EINVAL if any cooling device does not implement
  * the power actor API.
  */
-static int check_power_actors(struct thermal_zone_device *tz)
+static int check_power_actors(struct thermal_zone_device *tz,
+			      struct power_allocator_params *params)
 {
 	struct thermal_instance *instance;
 	int ret = 0;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		if (instance->trip != params->trip_max)
+			continue;
+
 		if (!cdev_is_power_actor(instance->cdev)) {
 			dev_warn(&tz->device, "power_allocator: %s is not a power actor\n",
 				 instance->cdev->type);
@@ -628,7 +633,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 		return -EINVAL;
 	}
 
-	ret = check_power_actors(tz);
+	ret = check_power_actors(tz, params);
 	if (ret) {
 		dev_warn(&tz->device, "power_allocator: binding failed\n");
 		kfree(params);
-- 
2.25.1

[PATCH 4/7] thermal: gov_power_allocator: Rearrange the order of variables

[Lukasz Luba]

Rearrange the order of variables in function and make them aligned with
the standard order for the kernel coding style. Also, move some variables
defined in nested code into the top of the function, to improve visibility
off all variables used.

This change is not expected to alter the general functionality.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 39 ++++++++++++++-------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 4b9ef04577a9a..79621b42ead38 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -84,13 +84,14 @@ struct power_allocator_params {
  */
 static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
 {
-	u32 sustainable_power = 0;
-	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
+	struct thermal_cooling_device *cdev;
+	struct thermal_instance *instance;
+	u32 sustainable_power = 0;
+	u32 min_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		struct thermal_cooling_device *cdev = instance->cdev;
-		u32 min_power;
+		cdev = instance->cdev;
 
 		if (instance->trip != params->trip_max)
 			continue;
@@ -211,10 +212,10 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 			  int control_temp,
 			  u32 max_allocatable_power)
 {
+	struct power_allocator_params *params = tz->governor_data;
 	s64 p, i, d, power_range;
 	s32 err, max_power_frac;
 	u32 sustainable_power;
-	struct power_allocator_params *params = tz->governor_data;
 
 	max_power_frac = int_to_frac(max_allocatable_power);
 
@@ -373,20 +374,20 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
 		}
 }
 
-static int allocate_power(struct thermal_zone_device *tz,
-			  int control_temp)
+static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 {
-	struct thermal_instance *instance;
+	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
+	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
 	struct power_allocator_params *params = tz->governor_data;
 	const struct thermal_trip *trip_max = params->trip_max;
-	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
-	u32 *weighted_req_power;
-	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
 	u32 total_granted_power, power_range;
-	int i, num_actors, total_weight, ret = 0;
+	struct thermal_cooling_device *cdev;
+	struct thermal_instance *instance;
+	u32 *weighted_req_power;
+	int i, weight, ret = 0;
+	int total_weight = 0;
+	int num_actors = 0;
 
-	num_actors = 0;
-	total_weight = 0;
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if ((instance->trip == trip_max) &&
 		    cdev_is_power_actor(instance->cdev)) {
@@ -424,8 +425,7 @@ static int allocate_power(struct thermal_zone_device *tz,
 	max_allocatable_power = 0;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		int weight;
-		struct thermal_cooling_device *cdev = instance->cdev;
+		cdev = instance->cdev;
 
 		if (instance->trip != trip_max)
 			continue;
@@ -547,12 +547,13 @@ static void reset_pid_controller(struct power_allocator_params *params)
 
 static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 {
-	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
+	struct thermal_cooling_device *cdev;
+	struct thermal_instance *instance;
 	u32 req_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		struct thermal_cooling_device *cdev = instance->cdev;
+		cdev = instance->cdev;
 
 		if (instance->trip != params->trip_max ||
 		    !cdev_is_power_actor(instance->cdev))
@@ -619,8 +620,8 @@ static int check_power_actors(struct thermal_zone_device *tz,
  */
 static int power_allocator_bind(struct thermal_zone_device *tz)
 {
-	int ret;
 	struct power_allocator_params *params;
+	int ret;
 
 	params = kzalloc(sizeof(*params), GFP_KERNEL);
 	if (!params)
-- 
2.25.1

[PATCH 5/7] thermal: gov_power_allocator: Use shorter variable when possible

[Lukasz Luba]

The 'cdev' pointer is initialed, so there is no need to use
'instance->cdev'.

This change is not expected to alter the general functionality.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 79621b42ead38..0f7f8278eacc5 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -560,7 +560,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 			continue;
 
 		instance->target = 0;
-		mutex_lock(&instance->cdev->lock);
+		mutex_lock(&cdev->lock);
 		/*
 		 * Call for updating the cooling devices local stats and avoid
 		 * periods of dozen of seconds when those have not been
@@ -569,9 +569,9 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 		cdev->ops->get_requested_power(cdev, &req_power);
 
 		if (update)
-			__thermal_cdev_update(instance->cdev);
+			__thermal_cdev_update(cdev);
 
-		mutex_unlock(&instance->cdev->lock);
+		mutex_unlock(&cdev->lock);
 	}
 }
 
-- 
2.25.1

[PATCH 6/7] thermal: gov_power_allocator: Remove unneeded local variables

[Lukasz Luba]

There is no need to keep some variables. We have a shorter name variables
in the governor private structure 'trip_max'. Remove the local 'trip_max'
and 'ret' variables from the allocate_power().

This change is not expected to alter the general functionality.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 0f7f8278eacc5..e6d2f0fe8d2fd 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -379,17 +379,16 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
 	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
 	struct power_allocator_params *params = tz->governor_data;
-	const struct thermal_trip *trip_max = params->trip_max;
 	u32 total_granted_power, power_range;
 	struct thermal_cooling_device *cdev;
 	struct thermal_instance *instance;
 	u32 *weighted_req_power;
-	int i, weight, ret = 0;
 	int total_weight = 0;
 	int num_actors = 0;
+	int i, weight;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if ((instance->trip == trip_max) &&
+		if ((instance->trip == params->trip_max) &&
 		    cdev_is_power_actor(instance->cdev)) {
 			num_actors++;
 			total_weight += instance->weight;
@@ -427,7 +426,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		cdev = instance->cdev;
 
-		if (instance->trip != trip_max)
+		if (instance->trip != params->trip_max)
 			continue;
 
 		if (!cdev_is_power_actor(cdev))
@@ -463,7 +462,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	total_granted_power = 0;
 	i = 0;
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != trip_max)
+		if (instance->trip != params->trip_max)
 			continue;
 
 		if (!cdev_is_power_actor(instance->cdev))
@@ -484,7 +483,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 
 	kfree(req_power);
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.25.1

[PATCH 7/7] thermal: gov_power_allocator: Clean needed variables at the beginning

[Lukasz Luba]

Rearrange the local variables setup. This improves the reading of the code
and allows to better see the initial values;

This change is not expected to alter the general functionality.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index e6d2f0fe8d2fd..785fff14223d8 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -376,16 +376,19 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
 
 static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 {
-	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
 	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
 	struct power_allocator_params *params = tz->governor_data;
-	u32 total_granted_power, power_range;
 	struct thermal_cooling_device *cdev;
 	struct thermal_instance *instance;
+	u32 total_weighted_req_power = 0;
+	u32 max_allocatable_power = 0;
+	u32 total_granted_power = 0;
+	u32 total_req_power = 0;
 	u32 *weighted_req_power;
+	u32 power_range, weight;
 	int total_weight = 0;
 	int num_actors = 0;
-	int i, weight;
+	int i = 0;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if ((instance->trip == params->trip_max) &&
@@ -418,11 +421,6 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	extra_actor_power = &req_power[3 * num_actors];
 	weighted_req_power = &req_power[4 * num_actors];
 
-	i = 0;
-	total_weighted_req_power = 0;
-	total_req_power = 0;
-	max_allocatable_power = 0;
-
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		cdev = instance->cdev;
 
@@ -459,7 +457,6 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 		       total_weighted_req_power, power_range, granted_power,
 		       extra_actor_power);
 
-	total_granted_power = 0;
 	i = 0;
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != params->trip_max)
-- 
2.25.1

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Rafael J. Wysocki]

On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi all,
>
> The patch set does some small clean up for Intelligent Power Allocator.
> Those changes are not expected to alter the general functionality. They just
> improve the code reading. Only patch 3/7 might improve the use case for
> binding the governor to thermal zone (very unlikely in real products, but
> it's needed for correctness).
>
> The changes are based on top of current PM thermal branch, so with the
> new trip points.
>
> Regards,
> Lukasz
>
> Lukasz Luba (7):
>   thermal: gov_power_allocator: Rename trip_max_desired_temperature
>   thermal: gov_power_allocator: Setup trip points earlier
>   thermal: gov_power_allocator: Check the cooling devices only for
>     trip_max
>   thermal: gov_power_allocator: Rearrange the order of variables
>   thermal: gov_power_allocator: Use shorter variable when possible
>   thermal: gov_power_allocator: Remove unneeded local variables
>   thermal: gov_power_allocator: Clean needed variables at the beginning
>
>  drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>  1 file changed, 64 insertions(+), 59 deletions(-)
>
> --

The series looks good to me overall, but I'd prefer to make these
changes in the 6.8 cycle, because the 6.7 merge window is around the
corner and there is quite a bit of thermal material in this cycle
already.

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Lukasz Luba]

On 10/26/23 09:54, Rafael J. Wysocki wrote:
> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi all,
>>
>> The patch set does some small clean up for Intelligent Power Allocator.
>> Those changes are not expected to alter the general functionality. They just
>> improve the code reading. Only patch 3/7 might improve the use case for
>> binding the governor to thermal zone (very unlikely in real products, but
>> it's needed for correctness).
>>
>> The changes are based on top of current PM thermal branch, so with the
>> new trip points.
>>
>> Regards,
>> Lukasz
>>
>> Lukasz Luba (7):
>>    thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>    thermal: gov_power_allocator: Setup trip points earlier
>>    thermal: gov_power_allocator: Check the cooling devices only for
>>      trip_max
>>    thermal: gov_power_allocator: Rearrange the order of variables
>>    thermal: gov_power_allocator: Use shorter variable when possible
>>    thermal: gov_power_allocator: Remove unneeded local variables
>>    thermal: gov_power_allocator: Clean needed variables at the beginning
>>
>>   drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>   1 file changed, 64 insertions(+), 59 deletions(-)
>>
>> --
> 
> The series looks good to me overall, but I'd prefer to make these
> changes in the 6.8 cycle, because the 6.7 merge window is around the
> corner and there is quite a bit of thermal material in this cycle
> already.

Thanks for having a look! Yes, I agree, we can wait after the
merge window. It just have to be cleaned one day a bit and I postponed
this a few times, so no rush ;)

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Lukasz Luba]

Hi Rafael,

Gentle ping

On 10/26/23 13:22, Lukasz Luba wrote:
> 
> 
> On 10/26/23 09:54, Rafael J. Wysocki wrote:
>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>
>>> Hi all,
>>>
>>> The patch set does some small clean up for Intelligent Power Allocator.
>>> Those changes are not expected to alter the general functionality. 
>>> They just
>>> improve the code reading. Only patch 3/7 might improve the use case for
>>> binding the governor to thermal zone (very unlikely in real products, 
>>> but
>>> it's needed for correctness).
>>>
>>> The changes are based on top of current PM thermal branch, so with the
>>> new trip points.
>>>
>>> Regards,
>>> Lukasz
>>>
>>> Lukasz Luba (7):
>>>    thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>>    thermal: gov_power_allocator: Setup trip points earlier
>>>    thermal: gov_power_allocator: Check the cooling devices only for
>>>      trip_max
>>>    thermal: gov_power_allocator: Rearrange the order of variables
>>>    thermal: gov_power_allocator: Use shorter variable when possible
>>>    thermal: gov_power_allocator: Remove unneeded local variables
>>>    thermal: gov_power_allocator: Clean needed variables at the beginning
>>>
>>>   drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>>   1 file changed, 64 insertions(+), 59 deletions(-)
>>>
>>> -- 
>>
>> The series looks good to me overall, but I'd prefer to make these
>> changes in the 6.8 cycle, because the 6.7 merge window is around the
>> corner and there is quite a bit of thermal material in this cycle
>> already.
> 
> Thanks for having a look! Yes, I agree, we can wait after the
> merge window. It just have to be cleaned one day a bit and I postponed
> this a few times, so no rush ;)

I've seen you've created the new pm/thermal. Could you consider to take
those in, please?

I would send some RFC on top showing the issue with reading back the CPU
max frequency from the PM_QoS chain.

Regards,
Lukasz

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Rafael J. Wysocki]

Hi Lukasz,

On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> Gentle ping
>
> On 10/26/23 13:22, Lukasz Luba wrote:
> >
> >
> > On 10/26/23 09:54, Rafael J. Wysocki wrote:
> >> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>
> >>> Hi all,
> >>>
> >>> The patch set does some small clean up for Intelligent Power Allocator.
> >>> Those changes are not expected to alter the general functionality.
> >>> They just
> >>> improve the code reading. Only patch 3/7 might improve the use case for
> >>> binding the governor to thermal zone (very unlikely in real products,
> >>> but
> >>> it's needed for correctness).
> >>>
> >>> The changes are based on top of current PM thermal branch, so with the
> >>> new trip points.
> >>>
> >>> Regards,
> >>> Lukasz
> >>>
> >>> Lukasz Luba (7):
> >>>    thermal: gov_power_allocator: Rename trip_max_desired_temperature
> >>>    thermal: gov_power_allocator: Setup trip points earlier
> >>>    thermal: gov_power_allocator: Check the cooling devices only for
> >>>      trip_max
> >>>    thermal: gov_power_allocator: Rearrange the order of variables
> >>>    thermal: gov_power_allocator: Use shorter variable when possible
> >>>    thermal: gov_power_allocator: Remove unneeded local variables
> >>>    thermal: gov_power_allocator: Clean needed variables at the beginning
> >>>
> >>>   drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
> >>>   1 file changed, 64 insertions(+), 59 deletions(-)
> >>>
> >>> --
> >>
> >> The series looks good to me overall, but I'd prefer to make these
> >> changes in the 6.8 cycle, because the 6.7 merge window is around the
> >> corner and there is quite a bit of thermal material in this cycle
> >> already.
> >
> > Thanks for having a look! Yes, I agree, we can wait after the
> > merge window. It just have to be cleaned one day a bit and I postponed
> > this a few times, so no rush ;)
>
> I've seen you've created the new pm/thermal. Could you consider to take
> those in, please?

Sure, I'll get to them presumably tomorrow and if not then early next week.

> I would send some RFC on top showing the issue with reading back the CPU
> max frequency from the PM_QoS chain.

Sounds good.

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Lukasz Luba]

On 11/23/23 19:50, Rafael J. Wysocki wrote:
> Hi Lukasz,
> 
> On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>> Gentle ping
>>
>> On 10/26/23 13:22, Lukasz Luba wrote:
>>>
>>>
>>> On 10/26/23 09:54, Rafael J. Wysocki wrote:
>>>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> The patch set does some small clean up for Intelligent Power Allocator.
>>>>> Those changes are not expected to alter the general functionality.
>>>>> They just
>>>>> improve the code reading. Only patch 3/7 might improve the use case for
>>>>> binding the governor to thermal zone (very unlikely in real products,
>>>>> but
>>>>> it's needed for correctness).
>>>>>
>>>>> The changes are based on top of current PM thermal branch, so with the
>>>>> new trip points.
>>>>>
>>>>> Regards,
>>>>> Lukasz
>>>>>
>>>>> Lukasz Luba (7):
>>>>>     thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>>>>     thermal: gov_power_allocator: Setup trip points earlier
>>>>>     thermal: gov_power_allocator: Check the cooling devices only for
>>>>>       trip_max
>>>>>     thermal: gov_power_allocator: Rearrange the order of variables
>>>>>     thermal: gov_power_allocator: Use shorter variable when possible
>>>>>     thermal: gov_power_allocator: Remove unneeded local variables
>>>>>     thermal: gov_power_allocator: Clean needed variables at the beginning
>>>>>
>>>>>    drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>>>>    1 file changed, 64 insertions(+), 59 deletions(-)
>>>>>
>>>>> --
>>>>
>>>> The series looks good to me overall, but I'd prefer to make these
>>>> changes in the 6.8 cycle, because the 6.7 merge window is around the
>>>> corner and there is quite a bit of thermal material in this cycle
>>>> already.
>>>
>>> Thanks for having a look! Yes, I agree, we can wait after the
>>> merge window. It just have to be cleaned one day a bit and I postponed
>>> this a few times, so no rush ;)
>>
>> I've seen you've created the new pm/thermal. Could you consider to take
>> those in, please?
> 
> Sure, I'll get to them presumably tomorrow and if not then early next week.

OK, thank you Rafael!

> 
>> I would send some RFC on top showing the issue with reading back the CPU
>> max frequency from the PM_QoS chain.
> 
> Sounds good.

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Rafael J. Wysocki]

On Fri, Nov 24, 2023 at 8:44 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 11/23/23 19:50, Rafael J. Wysocki wrote:
> > Hi Lukasz,
> >
> > On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> Gentle ping
> >>
> >> On 10/26/23 13:22, Lukasz Luba wrote:
> >>>
> >>>
> >>> On 10/26/23 09:54, Rafael J. Wysocki wrote:
> >>>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>>
> >>>>> Hi all,
> >>>>>
> >>>>> The patch set does some small clean up for Intelligent Power Allocator.
> >>>>> Those changes are not expected to alter the general functionality.
> >>>>> They just
> >>>>> improve the code reading. Only patch 3/7 might improve the use case for
> >>>>> binding the governor to thermal zone (very unlikely in real products,
> >>>>> but
> >>>>> it's needed for correctness).
> >>>>>
> >>>>> The changes are based on top of current PM thermal branch, so with the
> >>>>> new trip points.
> >>>>>
> >>>>> Regards,
> >>>>> Lukasz
> >>>>>
> >>>>> Lukasz Luba (7):
> >>>>>     thermal: gov_power_allocator: Rename trip_max_desired_temperature
> >>>>>     thermal: gov_power_allocator: Setup trip points earlier
> >>>>>     thermal: gov_power_allocator: Check the cooling devices only for
> >>>>>       trip_max
> >>>>>     thermal: gov_power_allocator: Rearrange the order of variables
> >>>>>     thermal: gov_power_allocator: Use shorter variable when possible
> >>>>>     thermal: gov_power_allocator: Remove unneeded local variables
> >>>>>     thermal: gov_power_allocator: Clean needed variables at the beginning
> >>>>>
> >>>>>    drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
> >>>>>    1 file changed, 64 insertions(+), 59 deletions(-)
> >>>>>
> >>>>> --
> >>>>
> >>>> The series looks good to me overall, but I'd prefer to make these
> >>>> changes in the 6.8 cycle, because the 6.7 merge window is around the
> >>>> corner and there is quite a bit of thermal material in this cycle
> >>>> already.
> >>>
> >>> Thanks for having a look! Yes, I agree, we can wait after the
> >>> merge window. It just have to be cleaned one day a bit and I postponed
> >>> this a few times, so no rush ;)
> >>
> >> I've seen you've created the new pm/thermal. Could you consider to take
> >> those in, please?
> >
> > Sure, I'll get to them presumably tomorrow and if not then early next week.
>
> OK, thank you Rafael!

I've queued up the whole lot for 6.8 and in the process I've edited
all of the changelogs and some subjects for clarity and English
grammar improvements.

Thanks!

Re: [PATCH 0/7] Minor cleanup for thermal gov power allocator

[Lukasz Luba]

On 11/28/23 15:17, Rafael J. Wysocki wrote:
> On Fri, Nov 24, 2023 at 8:44 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 11/23/23 19:50, Rafael J. Wysocki wrote:
>>> Hi Lukasz,
>>>
>>> On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> Gentle ping
>>>>
>>>> On 10/26/23 13:22, Lukasz Luba wrote:
>>>>>
>>>>>
>>>>> On 10/26/23 09:54, Rafael J. Wysocki wrote:
>>>>>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> The patch set does some small clean up for Intelligent Power Allocator.
>>>>>>> Those changes are not expected to alter the general functionality.
>>>>>>> They just
>>>>>>> improve the code reading. Only patch 3/7 might improve the use case for
>>>>>>> binding the governor to thermal zone (very unlikely in real products,
>>>>>>> but
>>>>>>> it's needed for correctness).
>>>>>>>
>>>>>>> The changes are based on top of current PM thermal branch, so with the
>>>>>>> new trip points.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Lukasz
>>>>>>>
>>>>>>> Lukasz Luba (7):
>>>>>>>      thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>>>>>>      thermal: gov_power_allocator: Setup trip points earlier
>>>>>>>      thermal: gov_power_allocator: Check the cooling devices only for
>>>>>>>        trip_max
>>>>>>>      thermal: gov_power_allocator: Rearrange the order of variables
>>>>>>>      thermal: gov_power_allocator: Use shorter variable when possible
>>>>>>>      thermal: gov_power_allocator: Remove unneeded local variables
>>>>>>>      thermal: gov_power_allocator: Clean needed variables at the beginning
>>>>>>>
>>>>>>>     drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>>>>>>     1 file changed, 64 insertions(+), 59 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>
>>>>>> The series looks good to me overall, but I'd prefer to make these
>>>>>> changes in the 6.8 cycle, because the 6.7 merge window is around the
>>>>>> corner and there is quite a bit of thermal material in this cycle
>>>>>> already.
>>>>>
>>>>> Thanks for having a look! Yes, I agree, we can wait after the
>>>>> merge window. It just have to be cleaned one day a bit and I postponed
>>>>> this a few times, so no rush ;)
>>>>
>>>> I've seen you've created the new pm/thermal. Could you consider to take
>>>> those in, please?
>>>
>>> Sure, I'll get to them presumably tomorrow and if not then early next week.
>>
>> OK, thank you Rafael!
> 
> I've queued up the whole lot for 6.8 and in the process I've edited
> all of the changelogs and some subjects for clarity and English
> grammar improvements.

Thank you and I really appreciate the improvements!