RE: [RFC PATCH 1/3] thermal/cpuplog_cooling: Add CPU hotplug cooling driver

[John Madieu <john.madieu.xa@bp.renesas.com>]

Hi Krzysztof,

Thanks for the review.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, March 11, 2025 8:31 AM
> To: John Madieu <john.madieu.xa@bp.renesas.com>; geert+renesas@glider.be;
> niklas.soderlund+renesas@ragnatech.se; conor+dt@kernel.org;
> krzk+dt@kernel.org; robh@kernel.org; rafael@kernel.org;
> daniel.lezcano@linaro.org
> Subject: Re: [RFC PATCH 1/3] thermal/cpuplog_cooling: Add CPU hotplug
> cooling driver
> 
> On 09/03/2025 13:13, John Madieu wrote:
> > +
> > +/* Check if a trip point is of type "plug" */ static bool
> > +is_plug_trip_point(struct device_node *trip_node) {
> > +	const char *trip_type_str;
> > +
> > +	if (!trip_node) {
> > +		pr_err("Trip node is NULL\n");
> > +		return false;
> > +	}
> > +
> > +	if (of_property_read_string(trip_node, "type", &trip_type_str)) {
> > +		pr_err("Trip node missing 'type' property\n");
> > +		return false;
> > +	}
> > +
> > +	pr_info("Trip type: '%s'\n", trip_type_str);
> > +
> > +	if (strcmp(trip_type_str, "plug") != 0) {
> 
> type is object, not string. Where is ABI documented? For the type and its
> value?

I'll prepare it for v2.

> 
> 
> > +		pr_debug("Trip type is '%s', not 'plug' - skipping\n",
> > +			 trip_type_str);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/* Init function */
> > +static int __init cpu_hotplug_cooling_init(void) {
> > +	struct device_node *thermal_zones, *thermal_zone;
> > +	int ret = 0;
> > +	int count = 0;
> > +
> > +	bitmap_zero(cpu_cooling_registered, NR_CPUS);
> > +
> > +	thermal_zones = of_find_node_by_name(NULL, "thermal-zones");
> > +	if (!thermal_zones) {
> > +		pr_err("Missing thermal-zones node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Process each thermal zone */
> > +	for_each_child_of_node(thermal_zones, thermal_zone) {
> > +		struct device_node *trips, *trip;
> > +		struct device_node *maps, *map;
> > +		bool found_plug = false;
> > +
> > +		/* First find trips and get a specific plug trip */
> > +		trips = of_find_node_by_name(thermal_zone, "trips");
> > +		if (!trips)
> > +			continue;
> > +
> > +		/* Find the emergency trip with type="plug" */
> > +		for_each_child_of_node(trips, trip) {
> > +			if (is_plug_trip_point(trip)) {
> > +				found_plug = true;
> > +				break;
> > +			}
> > +		}
> > +
> > +		/* If we didn't find a plug trip, no need to process this zone
> */
> > +		if (!found_plug) {
> > +			of_node_put(trips);
> > +			continue;
> > +		}
> > +
> > +		maps = of_find_node_by_name(thermal_zone, "cooling-maps");
> > +		if (!maps) {
> > +			of_node_put(trip);
> > +			of_node_put(trips);
> > +			continue;
> > +		}
> > +
> > +		pr_info("Found 'plug' trip point, processing cooling
> devices\n");
> 
> dev_info, or just drop. You are not supposed to print successes of
> standard DT parsing.

Noted. Thanks!

> 
> > +
> > +		/* Find the specific cooling map that references our plug trip
> */
> > +		for_each_child_of_node(maps, map) {
> > +			struct device_node *trip_ref;
> > +			struct of_phandle_args cooling_spec;
> > +			int idx = 0;
> > +
> > +			trip_ref = of_parse_phandle(map, "trip", 0);
> > +			if (!trip_ref || trip_ref != trip) {
> > +				if (trip_ref)
> > +					of_node_put(trip_ref);
> > +				continue;
> > +			}
> > +			of_node_put(trip_ref);
> > +
> > +			if (!of_find_property(map, "cooling-device", NULL)) {
> > +				pr_err("Missing cooling-device property\n");
> > +				continue;
> > +			}
> > +
> > +			/* Iterate through all cooling-device entries */
> > +			while (of_parse_phandle_with_args(
> > +				       map, "cooling-device",
> > +				       "#cooling-cells", idx++,
> > +				       &cooling_spec) == 0) {
> > +				struct device_node *cpu_node = cooling_spec.np;
> > +				int cpu;
> > +
> > +				if (!cpu_node) {
> > +					pr_err("CPU node at index %d is NULL\n",
> > +					       idx - 1);
> > +					continue;
> > +				}
> > +
> > +				cpu = of_cpu_node_to_id(cpu_node);
> > +				if (cpu < 0) {
> > +					pr_err("Failed to map CPU node %pOF to
> logical ID\n",
> > +					       cpu_node);
> > +					of_node_put(cpu_node);
> > +					continue;
> > +				}
> > +
> > +				if (cpu >= num_possible_cpus()) {
> > +					pr_err("Invalid CPU ID %d (max %d)\n",
> > +					       cpu, num_possible_cpus() - 1);
> > +					of_node_put(cpu_node);
> > +					continue;
> > +				}
> > +
> > +				pr_info("Processing cooling device for CPU%d\n",
> cpu);
> > +				ret = register_cpu_hotplug_cooling(cpu_node, cpu);
> > +				if (ret == 0)
> > +					count++;
> > +
> > +				of_node_put(cpu_node);
> > +			}
> > +			break; /* Only process the first map that references our
> trip */
> > +		}
> > +		of_node_put(maps);
> > +		of_node_put(trip);
> > +		of_node_put(trips);
> > +	}
> > +	of_node_put(thermal_zones);
> > +
> > +	if (count == 0) {
> > +		pr_err("No cooling devices registered\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	pr_info("CPU hotplug cooling driver initialized with %d devices\n",
> > +count);
> 
> Drop. Why would you print this on MIPS machine which does not care about
> it, just because someone loaded a module?
> 

Will remove this in v2.

> > +	return 0;
> > +}
> > +
> > +/* Exit function */
> > +static void __exit cpu_hotplug_cooling_exit(void) {
> > +	cleanup_cooling_devices();
> > +	pr_info("CPU hotplug cooling driver removed\n");
> 
> No, drop
> 

Got it.

> 
> > +}
> > +
> > +module_init(cpu_hotplug_cooling_init);
> > +module_exit(cpu_hotplug_cooling_exit);
> > +
> > +MODULE_AUTHOR("John Madieu <john.madieu.xa@bp.renesas.com>");
> > +MODULE_DESCRIPTION("CPU Hotplug Thermal Cooling Device");
> > +MODULE_LICENSE("GPL");
> > \ No newline at end of file
> 
> Warning here
> 

Will be fixed in v2.

> > diff --git a/drivers/thermal/thermal_of.c
> > b/drivers/thermal/thermal_of.c index 0eb92d57a1e2..41655af1e419 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -28,6 +28,7 @@ static const char * const trip_types[] = {
> >  	[THERMAL_TRIP_ACTIVE]	= "active",
> >  	[THERMAL_TRIP_PASSIVE]	= "passive",
> >  	[THERMAL_TRIP_HOT]	= "hot",
> > +	[THERMAL_TRIP_PLUG]	= "plug",
> >  	[THERMAL_TRIP_CRITICAL]	= "critical",
> >  };
> >
> > diff --git a/drivers/thermal/thermal_trace.h
> > b/drivers/thermal/thermal_trace.h index df8f4edd6068..c26a3aa7de5f
> > 100644
> > --- a/drivers/thermal/thermal_trace.h
> > +++ b/drivers/thermal/thermal_trace.h
> > @@ -12,6 +12,7 @@
> >  #include "thermal_core.h"
> >
> >  TRACE_DEFINE_ENUM(THERMAL_TRIP_CRITICAL);
> > +TRACE_DEFINE_ENUM(THERMAL_TRIP_PLUG);
> >  TRACE_DEFINE_ENUM(THERMAL_TRIP_HOT);
> >  TRACE_DEFINE_ENUM(THERMAL_TRIP_PASSIVE);
> >  TRACE_DEFINE_ENUM(THERMAL_TRIP_ACTIVE);
> > @@ -19,6 +20,7 @@ TRACE_DEFINE_ENUM(THERMAL_TRIP_ACTIVE);
> >  #define show_tzt_type(type)					\
> >  	__print_symbolic(type,					\
> >  			 { THERMAL_TRIP_CRITICAL, "CRITICAL"},	\
> > +			 { THERMAL_TRIP_PLUG,     "PLUG"},	\
> >  			 { THERMAL_TRIP_HOT,      "HOT"},	\
> >  			 { THERMAL_TRIP_PASSIVE,  "PASSIVE"},	\
> >  			 { THERMAL_TRIP_ACTIVE,   "ACTIVE"})
> > diff --git a/drivers/thermal/thermal_trip.c
> > b/drivers/thermal/thermal_trip.c index 4b8238468b53..373f6aaaf0da
> > 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -13,6 +13,7 @@ static const char *trip_type_names[] = {
> >  	[THERMAL_TRIP_ACTIVE] = "active",
> >  	[THERMAL_TRIP_PASSIVE] = "passive",
> >  	[THERMAL_TRIP_HOT] = "hot",
> > +	[THERMAL_TRIP_PLUG]	= "plug",
> >  	[THERMAL_TRIP_CRITICAL] = "critical",  };
> >
> > diff --git a/include/uapi/linux/thermal.h
> > b/include/uapi/linux/thermal.h index 46a2633d33aa..5f76360c6f69 100644
> > --- a/include/uapi/linux/thermal.h
> > +++ b/include/uapi/linux/thermal.h
> > @@ -15,6 +15,7 @@ enum thermal_trip_type {
> >  	THERMAL_TRIP_ACTIVE = 0,
> >  	THERMAL_TRIP_PASSIVE,
> >  	THERMAL_TRIP_HOT,
> > +	THERMAL_TRIP_PLUG,
> >  	THERMAL_TRIP_CRITICAL,
> >  };
> >
> 
> 
> Best regards,
> Krzysztof

Regards,
John