[v2] thermal: Add support for device tree thermal zones consumers

  Add helpers to support retrieving thermal zones from device tree nodes:
this will allow a device tree consumer to specify phandles to specific
thermal zone(s), including support for specifying thermal-zone-names.
This is useful, for example, for smart voltage scaling drivers that
need to adjust CPU/GPU/other voltages based on temperature, and for
battery charging drivers that need to scale current based on various
aggregated temperature sensor readings which are board-dependant.

Example:
smart-scaling-driver@10000000 {
	[...]

	thermal-zones = <&cluster_big_tz>, <&gpu_tz>, <&vpu_tz>;
	thermal-zone-names = "cpu", "gpu", "vpu";

	[...]
}

battery-charger@20000000 {
	[...]

	thermal-zones = <&battery_temp>, <&device_skin_temp>;
	thermal-zone-names = "batt-ext-sensor", "skin";

	[...]
}

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---

Changes in v2:
 - Added missing static inline for !CONFIG_OF fallback functions

Background story: while I was cleaning up the MediaTek Smart Voltage Scaling
(SVS) driver, I've found out that there's a lot of commonization to be done.
After a rewrite of "this and that" in that driver, I came across a barrier
that didn't allow me to remove another ~100 lines of code, and that was also
anyway breaking the driver, because the thermal zone names are different
from what was originally intended.

I've been looking for thermal zone handle retrieval around the kernel and
found that there currently are at least four other drivers that could make
use this as a cleanup: charger-manager, which is retrieving a thermal zone
to look for with a "cm-thermal-zone" string property, gpu/drm/tiny/repaper.c
that does the same by checking a "pervasive,thermal-zone" string property,
and ab8500_temp and sdhci-omap which are simply hardcoding a "cpu_thermal"
and "battery-thermal" thermal zone names respectively.

There are a number of other devices (mostly embedded, mostly smartphones)
that don't have an upstream driver and that could make use of this as well.

Cheers!


 drivers/thermal/thermal_of.c | 91 ++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h      | 15 ++++++
 2 files changed, 106 insertions(+)
  

Il 15/11/23 15:48, AngeloGioacchino Del Regno ha scritto:
> Add helpers to support retrieving thermal zones from device tree nodes:
> this will allow a device tree consumer to specify phandles to specific
> thermal zone(s), including support for specifying thermal-zone-names.
> This is useful, for example, for smart voltage scaling drivers that
> need to adjust CPU/GPU/other voltages based on temperature, and for
> battery charging drivers that need to scale current based on various
> aggregated temperature sensor readings which are board-dependant.
> 
> Example:
> smart-scaling-driver@10000000 {
> 	[...]
> 
> 	thermal-zones = <&cluster_big_tz>, <&gpu_tz>, <&vpu_tz>;
> 	thermal-zone-names = "cpu", "gpu", "vpu";
> 
> 	[...]
> }
> 
> battery-charger@20000000 {
> 	[...]
> 
> 	thermal-zones = <&battery_temp>, <&device_skin_temp>;
> 	thermal-zone-names = "batt-ext-sensor", "skin";
> 
> 	[...]
> }
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
> 

Hello,

just notifying that the dtschema for thermal consumers was merged[1], hence
totally unblocking this patch.

[1]: 
https://github.com/devicetree-org/dt-schema/commit/414a9f792ff7ae20a54a560bd2e2160b70f7d566

Cheers,
Angelo

> Changes in v2:
>   - Added missing static inline for !CONFIG_OF fallback functions
> 
> Background story: while I was cleaning up the MediaTek Smart Voltage Scaling
> (SVS) driver, I've found out that there's a lot of commonization to be done.
> After a rewrite of "this and that" in that driver, I came across a barrier
> that didn't allow me to remove another ~100 lines of code, and that was also
> anyway breaking the driver, because the thermal zone names are different
> from what was originally intended.
> 
> I've been looking for thermal zone handle retrieval around the kernel and
> found that there currently are at least four other drivers that could make
> use this as a cleanup: charger-manager, which is retrieving a thermal zone
> to look for with a "cm-thermal-zone" string property, gpu/drm/tiny/repaper.c
> that does the same by checking a "pervasive,thermal-zone" string property,
> and ab8500_temp and sdhci-omap which are simply hardcoding a "cpu_thermal"
> and "battery-thermal" thermal zone names respectively.
> 
> There are a number of other devices (mostly embedded, mostly smartphones)
> that don't have an upstream driver and that could make use of this as well.
> 
> Cheers!
> 
> 
>   drivers/thermal/thermal_of.c | 91 ++++++++++++++++++++++++++++++++++++
>   include/linux/thermal.h      | 15 ++++++
>   2 files changed, 106 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 1e0655b63259..d8ead456993e 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -538,6 +538,97 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>   	return ERR_PTR(ret);
>   }
>   
> +/**
> + * __thermal_of_get_zone_by_index() - Get thermal zone handle from the DT
> + *				      thermal-zones index
> + * @dev:   Pointer to the consumer device
> + * @index: Index of thermal-zones
> + *
> + * This function will search for a thermal zone in the thermal-zones phandle
> + * array corresponding to the specified index, then will search for its name
> + * into the registered thermal zones through thermal_zone_get_zone_by_name()
> + *
> + * Please note that this function is for internal use only and expects that
> + * all of the sanity checks are performed by its caller.
> + *
> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
> + * when the API is disabled or the "thermal-zones" DT property is missing.
> + */
> +static struct thermal_zone_device
> +*__thermal_of_get_zone_by_index(struct device *dev, int index)
> +{
> +	struct thermal_zone_device *tzd;
> +	struct device_node *np;
> +
> +	np = of_parse_phandle(dev->of_node, "thermal-zones", index);
> +	if (!np)
> +		return NULL;
> +
> +	tzd = thermal_zone_get_zone_by_name(np->name);
> +	of_node_put(np);
> +
> +	return tzd;
> +}
> +
> +/**
> + * thermal_of_get_zone_by_index() - Get thermal zone handle from a DT node
> + *				    based on index
> + * @dev:   Pointer to the consumer device
> + * @index: Index of thermal-zones
> + *
> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
> + * when the API is disabled or the "thermal-zones" DT property is missing.
> + */
> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int index)
> +{
> +	if (!dev || !dev->of_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (!of_property_present(dev->of_node, "thermal-zones"))
> +		return NULL;
> +
> +	return __thermal_of_get_zone_by_index(dev, index);
> +}
> +
> +/**
> + * thermal_of_get_zone() - Get thermal zone handle from a DT node based
> + *			   on name, or the first handle in list
> + * @dev:   Pointer to the consumer device
> + * @name:  Name as found in thermal-zone-names or NULL
> + *
> + * This function will search for a thermal zone in the thermal-zones phandle
> + * array corresponding to the index of that in the thermal-zone-names array.
> + * If the name is not specified (NULL), it will return the first thermal zone
> + * in the thermal-zones phandle array.
> + *
> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
> + * when the API is disabled or the "thermal-zones" DT property is missing.
> + */
> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char *name)
> +{
> +	int index;
> +
> +	if (!dev || !dev->of_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (!of_property_present(dev->of_node, "thermal-zones")) {
> +		pr_err("thermal zones property not present\n");
> +		return NULL;
> +	}
> +
> +	if (name) {
> +		index = of_property_match_string(dev->of_node, "thermal-zone-names", name);
> +		if (index < 0) {
> +			pr_err("thermal zone names property not present\n");
> +			return ERR_PTR(index);
> +		}
> +	} else {
> +		index = 0;
> +	}
> +
> +	return __thermal_of_get_zone_by_index(dev, index);
> +}
> +
>   static void devm_thermal_of_zone_release(struct device *dev, void *res)
>   {
>   	thermal_of_zone_unregister(*(struct thermal_zone_device **)res);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index cee814d5d1ac..0fceeb7ed08a 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -261,6 +261,9 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
>   
>   void devm_thermal_of_zone_unregister(struct device *dev, struct thermal_zone_device *tz);
>   
> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int index);
> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char *name);
> +
>   #else
>   
>   static inline
> @@ -274,6 +277,18 @@ static inline void devm_thermal_of_zone_unregister(struct device *dev,
>   						   struct thermal_zone_device *tz)
>   {
>   }
> +
> +static inline
> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int index)
> +{
> +	return ERR_PTR(-ENOTSUPP);
> +}
> +
> +static inline
> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char *name)
> +{
> +	return ERR_PTR(-ENOTSUPP);
> +}
>   #endif
>   
>   int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
  

Il 30/11/23 14:22, Daniel Lezcano ha scritto:
> 
> Hi Angelo,
> 
> thanks for your proposal
> 
> On 15/11/2023 15:48, AngeloGioacchino Del Regno wrote:
>> Add helpers to support retrieving thermal zones from device tree nodes:
>> this will allow a device tree consumer to specify phandles to specific
>> thermal zone(s), including support for specifying thermal-zone-names.
>> This is useful, for example, for smart voltage scaling drivers that
>> need to adjust CPU/GPU/other voltages based on temperature, and for
>> battery charging drivers that need to scale current based on various
>> aggregated temperature sensor readings which are board-dependant.
> 
> IMO these changes are trying to solve something from the DT perspective adding more 
> confusion between phandle, names, types etc ... and it does not really help AFAICT.
> 

I honestly don't see how can assigning thermal zones (like we're doing for other
consumers like clocks, etc) to a node can be confusing?
To me, it looks like a pattern that is repeating over and over in device tree, for
multiple types of consumers.

> Overall I'm a bit reluctant to add more API in the thermal.h. From my POV, we 
> should try to remove as much as possible functions from there.
> 

Cleaning up the API is always something that makes sense, but I don't see why this
should prevent useful additions...

> That said, the name of a thermal zone does not really exists and there is confusion 
> in the code between a name and a type. (type being assumed to be a name).

That depends on how you see it. What my brain ticks around is:
A thermal zone is a physical zone on the PCB, or a physical zone on a chip,
which has its own "real name", as in, it can be physically identified.

Example: The "Skin area" of a laptop is something "reachable" from the user as an
externally exposed part. This area's temperature is read by thermistor EXTERNAL_1,
not by thermistor "SKIN0".

Same goes for "big cluster area", "little cluster area", "cpu complex area", etc.

> 
> There could be several thermal zones with the same types for non-DT description. 
> However, the documentation says we should create an unique type in the DT and that 
> is what is happening when registering a thermal zone from the DT [1] as we use the 
> node name.
> 
>  From an external driver, it possible to get the np->name from the phandles and 
> call thermal_zone_get_by_name(np->name).
> 

That'd still require you to pass a thermal zone phandle to the node(driver) though?

> The hardening change which may make sense is to check a thermal zone with the same 
> name is not already registered in thermal_of.c by checking 
> thermal_zone_get_by_name() fails before registering it.
> 

Yes we can harden that, but I don't see how is this relevant to thermal zones
device tree consumers (proposed in this patch)?

Cheers,
Angelo

>    -- Daniel
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_of.c?h=thermal%2Fbleeding-edge#n514
> 
>> Example:
>> smart-scaling-driver@10000000 {
>>     [...]
>>
>>     thermal-zones = <&cluster_big_tz>, <&gpu_tz>, <&vpu_tz>;
>>     thermal-zone-names = "cpu", "gpu", "vpu";
>>
>>     [...]
>> }
>>
>> battery-charger@20000000 {
>>     [...]
>>
>>     thermal-zones = <&battery_temp>, <&device_skin_temp>;
>>     thermal-zone-names = "batt-ext-sensor", "skin";
>>
>>     [...]
>> }
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>
>> Changes in v2:
>>   - Added missing static inline for !CONFIG_OF fallback functions
>>
>> Background story: while I was cleaning up the MediaTek Smart Voltage Scaling
>> (SVS) driver, I've found out that there's a lot of commonization to be done.
>> After a rewrite of "this and that" in that driver, I came across a barrier
>> that didn't allow me to remove another ~100 lines of code, and that was also
>> anyway breaking the driver, because the thermal zone names are different
>> from what was originally intended.
>>
>> I've been looking for thermal zone handle retrieval around the kernel and
>> found that there currently are at least four other drivers that could make
>> use this as a cleanup: charger-manager, which is retrieving a thermal zone
>> to look for with a "cm-thermal-zone" string property, gpu/drm/tiny/repaper.c
>> that does the same by checking a "pervasive,thermal-zone" string property,
>> and ab8500_temp and sdhci-omap which are simply hardcoding a "cpu_thermal"
>> and "battery-thermal" thermal zone names respectively.
>>
>> There are a number of other devices (mostly embedded, mostly smartphones)
>> that don't have an upstream driver and that could make use of this as well.
>>
>> Cheers!
>>
>>
>>   drivers/thermal/thermal_of.c | 91 ++++++++++++++++++++++++++++++++++++
>>   include/linux/thermal.h      | 15 ++++++
>>   2 files changed, 106 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>> index 1e0655b63259..d8ead456993e 100644
>> --- a/drivers/thermal/thermal_of.c
>> +++ b/drivers/thermal/thermal_of.c
>> @@ -538,6 +538,97 @@ static struct thermal_zone_device 
>> *thermal_of_zone_register(struct device_node *
>>       return ERR_PTR(ret);
>>   }
>> +/**
>> + * __thermal_of_get_zone_by_index() - Get thermal zone handle from the DT
>> + *                      thermal-zones index
>> + * @dev:   Pointer to the consumer device
>> + * @index: Index of thermal-zones
>> + *
>> + * This function will search for a thermal zone in the thermal-zones phandle
>> + * array corresponding to the specified index, then will search for its name
>> + * into the registered thermal zones through thermal_zone_get_zone_by_name()
>> + *
>> + * Please note that this function is for internal use only and expects that
>> + * all of the sanity checks are performed by its caller.
>> + *
>> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
>> + * when the API is disabled or the "thermal-zones" DT property is missing.
>> + */
>> +static struct thermal_zone_device
>> +*__thermal_of_get_zone_by_index(struct device *dev, int index)
>> +{
>> +    struct thermal_zone_device *tzd;
>> +    struct device_node *np;
>> +
>> +    np = of_parse_phandle(dev->of_node, "thermal-zones", index);
>> +    if (!np)
>> +        return NULL;
>> +
>> +    tzd = thermal_zone_get_zone_by_name(np->name);
>> +    of_node_put(np);
>> +
>> +    return tzd;
>> +}
>> +
>> +/**
>> + * thermal_of_get_zone_by_index() - Get thermal zone handle from a DT node
>> + *                    based on index
>> + * @dev:   Pointer to the consumer device
>> + * @index: Index of thermal-zones
>> + *
>> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
>> + * when the API is disabled or the "thermal-zones" DT property is missing.
>> + */
>> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int 
>> index)
>> +{
>> +    if (!dev || !dev->of_node)
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    if (!of_property_present(dev->of_node, "thermal-zones"))
>> +        return NULL;
>> +
>> +    return __thermal_of_get_zone_by_index(dev, index);
>> +}
>> +
>> +/**
>> + * thermal_of_get_zone() - Get thermal zone handle from a DT node based
>> + *               on name, or the first handle in list
>> + * @dev:   Pointer to the consumer device
>> + * @name:  Name as found in thermal-zone-names or NULL
>> + *
>> + * This function will search for a thermal zone in the thermal-zones phandle
>> + * array corresponding to the index of that in the thermal-zone-names array.
>> + * If the name is not specified (NULL), it will return the first thermal zone
>> + * in the thermal-zones phandle array.
>> + *
>> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
>> + * when the API is disabled or the "thermal-zones" DT property is missing.
>> + */
>> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char 
>> *name)
>> +{
>> +    int index;
>> +
>> +    if (!dev || !dev->of_node)
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    if (!of_property_present(dev->of_node, "thermal-zones")) {
>> +        pr_err("thermal zones property not present\n");
>> +        return NULL;
>> +    }
>> +
>> +    if (name) {
>> +        index = of_property_match_string(dev->of_node, "thermal-zone-names", name);
>> +        if (index < 0) {
>> +            pr_err("thermal zone names property not present\n");
>> +            return ERR_PTR(index);
>> +        }
>> +    } else {
>> +        index = 0;
>> +    }
>> +
>> +    return __thermal_of_get_zone_by_index(dev, index);
>> +}
>> +
>>   static void devm_thermal_of_zone_release(struct device *dev, void *res)
>>   {
>>       thermal_of_zone_unregister(*(struct thermal_zone_device **)res);
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index cee814d5d1ac..0fceeb7ed08a 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -261,6 +261,9 @@ struct thermal_zone_device 
>> *devm_thermal_of_zone_register(struct device *dev, in
>>   void devm_thermal_of_zone_unregister(struct device *dev, struct 
>> thermal_zone_device *tz);
>> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int 
>> index);
>> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char 
>> *name);
>> +
>>   #else
>>   static inline
>> @@ -274,6 +277,18 @@ static inline void devm_thermal_of_zone_unregister(struct 
>> device *dev,
>>                              struct thermal_zone_device *tz)
>>   {
>>   }
>> +
>> +static inline
>> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int 
>> index)
>> +{
>> +    return ERR_PTR(-ENOTSUPP);
>> +}
>> +
>> +static inline
>> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char 
>> *name)
>> +{
>> +    return ERR_PTR(-ENOTSUPP);
>> +}
>>   #endif
>>   int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
>
  

Hi Angelo,

On 01/12/2023 10:52, AngeloGioacchino Del Regno wrote:
> Il 30/11/23 14:22, Daniel Lezcano ha scritto:
>>
>> Hi Angelo,
>>
>> thanks for your proposal
>>
>> On 15/11/2023 15:48, AngeloGioacchino Del Regno wrote:
>>> Add helpers to support retrieving thermal zones from device tree nodes:
>>> this will allow a device tree consumer to specify phandles to specific
>>> thermal zone(s), including support for specifying thermal-zone-names.
>>> This is useful, for example, for smart voltage scaling drivers that
>>> need to adjust CPU/GPU/other voltages based on temperature, and for
>>> battery charging drivers that need to scale current based on various
>>> aggregated temperature sensor readings which are board-dependant.
>>
>> IMO these changes are trying to solve something from the DT 
>> perspective adding more confusion between phandle, names, types etc 
>> ... and it does not really help AFAICT.
>>
> 
> I honestly don't see how can assigning thermal zones (like we're doing 
> for other
> consumers like clocks, etc) to a node can be confusing?
> To me, it looks like a pattern that is repeating over and over in device 
> tree, for
> multiple types of consumers.

Because there is no need to add anything. Everything is already available.

Add a phandle in the device node wanting to access the thermal zone, get 
the thermal zone device node pointer name and use 
thermal_zone_device_get_by_name(), but see below ...


>> Overall I'm a bit reluctant to add more API in the thermal.h. From my 
>> POV, we should try to remove as much as possible functions from there.
>>
> 
> Cleaning up the API is always something that makes sense, but I don't 
> see why this
> should prevent useful additions...
> 
>> That said, the name of a thermal zone does not really exists and there 
>> is confusion in the code between a name and a type. (type being 
>> assumed to be a name).
> 
> That depends on how you see it. What my brain ticks around is:
> A thermal zone is a physical zone on the PCB, or a physical zone on a chip,
> which has its own "real name", as in, it can be physically identified.

What I meant the thermal framework does not really have a thermal zone 
name, just a type. So it is possible to find several thermal zone with 
the same type like "acpitz"

> Example: The "Skin area" of a laptop is something "reachable" from the 
> user as an
> externally exposed part. This area's temperature is read by thermistor 
> EXTERNAL_1,
> not by thermistor "SKIN0".
> 
> Same goes for "big cluster area", "little cluster area", "cpu complex 
> area", etc.

Today that is solved with a configuration file mapping a specific 
thermal zone to a name but still fragile as we can have duplicate 
thermal zone types.

>> There could be several thermal zones with the same types for non-DT 
>> description. However, the documentation says we should create an 
>> unique type in the DT and that is what is happening when registering a 
>> thermal zone from the DT [1] as we use the node name.
>>
>>  From an external driver, it possible to get the np->name from the 
>> phandles and call thermal_zone_get_by_name(np->name).
>>
> 
> That'd still require you to pass a thermal zone phandle to the 
> node(driver) though?

Yes

>> The hardening change which may make sense is to check a thermal zone 
>> with the same name is not already registered in thermal_of.c by 
>> checking thermal_zone_get_by_name() fails before registering it.
>>
> 
> Yes we can harden that, but I don't see how is this relevant to thermal 
> zones
> device tree consumers (proposed in this patch)?

Putting apart the fact the change you propose is not relevant as there 
is already everything in. My comment is about the current state of the 
thermal framework.

  - A thermal zone does not have a name but a type

  - We use the thermal zone DT node name to register as a name but it is 
a type from the thermal framework point of view

  - We can register several thermal zones with the same type (so we can 
have duplicate names if we use type as name)

  - We use thermal_zone_device_get_by_name() but actually it checks 
against the type and as we can have multiple identical types, the 
function returns the first one found

All this is a bit fuzzy and confusing. So if you add these mapping 
between thermal zone nodes and names, that will be even more confusing.

Ideally, it would make more sense to cleanup this in order to have 
something like an enum type describing the thermal zone (battery, cpu, 
npu, gpu, dsp, ...) which would be used as a type of thermal zone and 
then an unique name (cpu0, cpu1, modem0, modem1, gpu-bottom, gpu-top, 
gpu-center, skin, ...).
  

Il 01/12/23 15:18, Daniel Lezcano ha scritto:
> 
> Hi Angelo,
> 
> On 01/12/2023 10:52, AngeloGioacchino Del Regno wrote:
>> Il 30/11/23 14:22, Daniel Lezcano ha scritto:
>>>
>>> Hi Angelo,
>>>
>>> thanks for your proposal
>>>
>>> On 15/11/2023 15:48, AngeloGioacchino Del Regno wrote:
>>>> Add helpers to support retrieving thermal zones from device tree nodes:
>>>> this will allow a device tree consumer to specify phandles to specific
>>>> thermal zone(s), including support for specifying thermal-zone-names.
>>>> This is useful, for example, for smart voltage scaling drivers that
>>>> need to adjust CPU/GPU/other voltages based on temperature, and for
>>>> battery charging drivers that need to scale current based on various
>>>> aggregated temperature sensor readings which are board-dependant.
>>>
>>> IMO these changes are trying to solve something from the DT perspective adding 
>>> more confusion between phandle, names, types etc ... and it does not really help 
>>> AFAICT.
>>>
>>
>> I honestly don't see how can assigning thermal zones (like we're doing for other
>> consumers like clocks, etc) to a node can be confusing?
>> To me, it looks like a pattern that is repeating over and over in device tree, for
>> multiple types of consumers.
> 
> Because there is no need to add anything. Everything is already available.
> 
> Add a phandle in the device node wanting to access the thermal zone, get the 
> thermal zone device node pointer name and use thermal_zone_device_get_by_name(), 
> but see below ...
> 
> 
>>> Overall I'm a bit reluctant to add more API in the thermal.h. From my POV, we 
>>> should try to remove as much as possible functions from there.
>>>
>>
>> Cleaning up the API is always something that makes sense, but I don't see why this
>> should prevent useful additions...
>>
>>> That said, the name of a thermal zone does not really exists and there is 
>>> confusion in the code between a name and a type. (type being assumed to be a name).
>>
>> That depends on how you see it. What my brain ticks around is:
>> A thermal zone is a physical zone on the PCB, or a physical zone on a chip,
>> which has its own "real name", as in, it can be physically identified.
> 
> What I meant the thermal framework does not really have a thermal zone name, just a 
> type. So it is possible to find several thermal zone with the same type like "acpitz"
> 
>> Example: The "Skin area" of a laptop is something "reachable" from the user as an
>> externally exposed part. This area's temperature is read by thermistor EXTERNAL_1,
>> not by thermistor "SKIN0".
>>
>> Same goes for "big cluster area", "little cluster area", "cpu complex area", etc.
> 
> Today that is solved with a configuration file mapping a specific thermal zone to a 
> name but still fragile as we can have duplicate thermal zone types.
> 
>>> There could be several thermal zones with the same types for non-DT description. 
>>> However, the documentation says we should create an unique type in the DT and 
>>> that is what is happening when registering a thermal zone from the DT [1] as we 
>>> use the node name.
>>>
>>>  From an external driver, it possible to get the np->name from the phandles and 
>>> call thermal_zone_get_by_name(np->name).
>>>
>>
>> That'd still require you to pass a thermal zone phandle to the node(driver) though?
> 
> Yes
> 
>>> The hardening change which may make sense is to check a thermal zone with the 
>>> same name is not already registered in thermal_of.c by checking 
>>> thermal_zone_get_by_name() fails before registering it.
>>>
>>
>> Yes we can harden that, but I don't see how is this relevant to thermal zones
>> device tree consumers (proposed in this patch)?
> 
> Putting apart the fact the change you propose is not relevant as there is already 
> everything in. My comment is about the current state of the thermal framework.
> 

I don't really understand this assertion, and I'm afraid that I'm underestimating
something so, in case, please help me to understand what am I missing here.

For how I see it, in the thermal framewoek I don't see any "somewhat standardized"
helper like the one(s) that I'm introducing with this patch (thermal_of_get_zone(),
thermal_of_get_zone_by_index()), and this is the exact reason why I'm proposing
this patch.

Then again - I mean no disrespect - it's just that I don't understand (yet) why you
are saying that "everything is already available", because I really don't see it.

>   - A thermal zone does not have a name but a type
> 
>   - We use the thermal zone DT node name to register as a name but it is a type 
> from the thermal framework point of view

This is something that I didn't realize before. Thanks for that.

...and yes, we're registering a "name" from DT as a "type" in the framework, this
is highly confusing and needs to be cleaned up.

> 
>   - We can register several thermal zones with the same type (so we can have 
> duplicate names if we use type as name)
> 

...which makes sense, after realizing that we're registering a TYPE and not a NAME,
and I agree about the logic for which that multiple zones can be of the same type.

>   - We use thermal_zone_device_get_by_name() but actually it checks against the 
> type and as we can have multiple identical types, the function returns the first 
> one found
> 

The first thing that comes to mind is to rename thermal_zone_device_get_by_name()
to thermal_zone_device_get_by_type(), but then I'd be reintroducing the former and
this gives me concerns about OOT drivers using that and developers getting highly
confused (name->type, but name exists again, so they might erroneously just fix the
call to xxx_by_name() instead of changing it to xxx_by_type()).

Should I *not* be concerned about that? Any suggestion?


I'd be glad to go on and "make it clear" that we're doing type comparison and not
name comparison (with that rename, or similar), because (again) I see how confusing
that is. I was confused by that as well, so... :-)

> All this is a bit fuzzy and confusing. So if you add these mapping between thermal 
> zone nodes and names, that will be even more confusing.
> 

IMO, not really. The thermal-zone-names are "local to a driver", not to the thermal
framework itself... it's like for clocks, interrupts, etc.: you want to get a TZ
that is declared with name "xyz", but it doesn't matter what the real name of the
actual TZ actually is.

Since I'm not sure I expressed myself in the best possible way, I'm referring to
the following example:

	clock-names = "main";

...but the "real name" for the clock in the clk framework is "mfg_bg3d".

That's the same with what I'm introducing here (forget for just one moment that
there is this name<->type issue):

	thermal-zone-names = "xyz";

...but the "real name" for the TZ in the thermal framework is "gpu0-thermal".

> Ideally, it would make more sense to cleanup this in order to have something like 
> an enum type describing the thermal zone (battery, cpu, npu, gpu, dsp, ...) which 
> would be used as a type of thermal zone and then an unique name (cpu0, cpu1, 
> modem0, modem1, gpu-bottom, gpu-top, gpu-center, skin, ...).
> 

This might get more complicated than how it looks, but would actually make sense
as well: the concern would be about how do we cleanly declare (example, in DT, but
ACPI is the worst case, as ACPI tables are a "set and forget" type of thing,
shipped  with BIOSes/EFI and almost never modified).

Cheers,
Angelo
  

Hi Angelo,

On 05/12/2023 14:48, AngeloGioacchino Del Regno wrote:
> Il 01/12/23 15:18, Daniel Lezcano ha scritto:

[ ... ]

>> Putting apart the fact the change you propose is not relevant as there 
>> is already everything in. My comment is about the current state of the 
>> thermal framework.
>>
> 
> I don't really understand this assertion, and I'm afraid that I'm 
> underestimating
> something so, in case, please help me to understand what am I missing here.

Sure. Let me clarify my understanding of you proposal and my assertion.

  - A driver needs a thermal zone device structure pointer in order to 
read its temperature. But as it is not the creator, it does not have 
this pointer.

  - As a solution, several drivers are using a specific DT bindings to 
map a thermal zone "name/type' with a string to refer from the driver a 
specific thermal zone node name. Then the function 
thermal_zone_device_get_by_name() is used to retrieve the pointer.

  - Your proposal is to provide that mechanism in the thermal_of code 
directly, so the code can be factored out for all these drivers.

Is my understanding correct?

My point is:

  - The driver are mapping a thermal zone with a name but a node name is 
supposed to be unique on DT (but that is implicit)

  - A phandle is enough to get the node name, no need to add a thermal 
zone name to get the node and then get the thermal zone. It is duplicate 
information as the DT uses the node name as a thermal zone name.

We could have:

         thermal-zones {
                 display {
                         polling-delay-passive = <0>;
                         polling-delay = <0>;
                         thermal-sensors = <&display_temp>;
                 };
         };

         papirus27@0{

		[ ... ]

---             pervasive,thermal-zone = "display";
+++             pervasive,thermal-zone = <&display>;
         };

The ux500 is directly calling thermal_zone_device_get_by_name() with the 
thermal zone node name.

> For how I see it, in the thermal framewoek I don't see any "somewhat 
> standardized"
> helper like the one(s) that I'm introducing with this patch 
> (thermal_of_get_zone(),
> thermal_of_get_zone_by_index()), and this is the exact reason why I'm 
> proposing
> this patch.
> 
> Then again - I mean no disrespect - it's just that I don't understand 
> (yet) why you
> are saying that "everything is already available", because I really 
> don't see it.

Ok said differently, thermal zone name and type are messy.

Let's clarify that and then let's see with the result if adding this 
thermal zone node/name mapping still makes sense.

>>   - A thermal zone does not have a name but a type
>>
>>   - We use the thermal zone DT node name to register as a name but it 
>> is a type from the thermal framework point of view
> 
> This is something that I didn't realize before. Thanks for that.
> 
> ...and yes, we're registering a "name" from DT as a "type" in the 
> framework, this
> is highly confusing and needs to be cleaned up.
> 
>>
>>   - We can register several thermal zones with the same type (so we 
>> can have duplicate names if we use type as name)
>>
> 
> ...which makes sense, after realizing that we're registering a TYPE and 
> not a NAME,
> and I agree about the logic for which that multiple zones can be of the 
> same type.
> 
>>   - We use thermal_zone_device_get_by_name() but actually it checks 
>> against the type and as we can have multiple identical types, the 
>> function returns the first one found
>>
> 
> The first thing that comes to mind is to rename 
> thermal_zone_device_get_by_name()
> to thermal_zone_device_get_by_type(), but then I'd be reintroducing the 
> former and
> this gives me concerns about OOT drivers using that and developers 
> getting highly
> confused (name->type, but name exists again, so they might erroneously 
> just fix the
> call to xxx_by_name() instead of changing it to xxx_by_type()).


> Should I *not* be concerned about that?

Not really :)

TBH, OOT drivers do not exist from upstream POV.

 > Any suggestion?

Yes, let's introduce a thermal zone name in the tzd.

  - There are now too many parameters to the function 
thermal_zone_device_register*(), so we can't add a new 'name' parameter. 
Introduce a thermal_zone_device_parameters structure. This structure 
will contain all thermal_zone_device_register_* parameter function. 
There won't be any functional changes, just how the parameters are 
passed. Perhaps, you should use an intermediate function to not have the 
change impacting everywhere.

  - then add a const char *name field in this structure and in the 
thermal_zone_device structure. So we can assign the name to the thermal 
zone. The name must be checked against duplicate. If no name is 
specified when creating a thermal zone, then name = type.

  - In thermal_of, use the node name for the type and the name, that 
will be duplicate but we will sort it out later.

  - Add the name in sysfs

Second step, track down users of thermal_zone_device_get_by_name() and 
check if they can use the name instead of the type. I'm pretty sure it 
is the case for most of the callers.

With that, there will be a nice clarification IMHO.

Then we will be able to restate the 'type' with something different 
without breaking the existing ABI.
  

Il 05/12/23 19:47, Daniel Lezcano ha scritto:
> 
> Hi Angelo,
> 
> On 05/12/2023 14:48, AngeloGioacchino Del Regno wrote:
>> Il 01/12/23 15:18, Daniel Lezcano ha scritto:
> 
> [ ... ]
> 
>>> Putting apart the fact the change you propose is not relevant as there is 
>>> already everything in. My comment is about the current state of the thermal 
>>> framework.
>>>
>>
>> I don't really understand this assertion, and I'm afraid that I'm underestimating
>> something so, in case, please help me to understand what am I missing here.
> 
> Sure. Let me clarify my understanding of you proposal and my assertion.
> 
>   - A driver needs a thermal zone device structure pointer in order to read its 
> temperature. But as it is not the creator, it does not have this pointer.
> 
>   - As a solution, several drivers are using a specific DT bindings to map a 
> thermal zone "name/type' with a string to refer from the driver a specific thermal 
> zone node name. Then the function thermal_zone_device_get_by_name() is used to 
> retrieve the pointer.
> 
>   - Your proposal is to provide that mechanism in the thermal_of code directly, so 
> the code can be factored out for all these drivers.
> 
> Is my understanding correct?
> 

I think that your understanding is 95% correct....

> My point is:
> 
>   - The driver are mapping a thermal zone with a name but a node name is supposed 
> to be unique on DT (but that is implicit)
> 
>   - A phandle is enough to get the node name, no need to add a thermal zone name to 
> get the node and then get the thermal zone. It is duplicate information as the DT 
> uses the node name as a thermal zone name.
> 
> We could have:
> 
>          thermal-zones {
>                  display {
>                          polling-delay-passive = <0>;
>                          polling-delay = <0>;
>                          thermal-sensors = <&display_temp>;
>                  };
>          };
> 
>          papirus27@0{
> 
>          [ ... ]
> 
> ---             pervasive,thermal-zone = "display";
> +++             pervasive,thermal-zone = <&display>;
>          };
> 
> The ux500 is directly calling thermal_zone_device_get_by_name() with the thermal 
> zone node name.
> 

.... but as for the remaining 5%, I'm not sure, so I'll put one full-of-fantasy
example here to make sure that you get my point:


************************************
fantasy soc/board 1:

thermal-zones {
	/*
	 * The type (or name, whatever) of this zone is "dsi-disp-thermal"
	 * and not "display" - this is on purpose. Can be changed to "display"
	 * without any concerns in this fantasy soc/board.
	 */
	display: dsi-disp-thermal {
		polling-delay ....
		thermal-sensors ...
	}

	something_else: some-other-zone {
		stuff ...
	}
}

************************************
fantasy soc/board 2 (qcom vs mtk vs rockchip vs...):

thermal-zones {
	/*
	 * The type (or name, whatever) of this zone is "edp-disp-thermal"
	 * and not "display" - this is on purpose. Can be changed to "display"
	 * without any concerns in this fantasy soc/board.
	 */
	display: edp-disp-thermal {
		polling-delay ....
		thermal-sensors ...
	}

	.....
}

************************************
fantasy soc/board 3 (qcom vs mtk vs rockchip vs...):

thermal-zones {
	/*
	 * The type (or name, whatever) of this zone is "skin-sense-left-thermal"
	 * and not "display" - this is on purpose: on this device the display temp
	 * zone is retrieved from the "bottom" skin temperature zone, because the
	 * display's driver ic is 0.01mm far from that physical zone, so they have
	 * placed a sensor there.
	 *
	 * Lots of fantasy here, but it's just to show why a thermal zone may have
	 * a different name on different boards, and why it is more descriptive to
	 * keep the board-specific TZ name instead of changing them all to have a
	 * driver specific "display" name.
	 */
	display: skin-sense-bottom-thermal {
		polling-delay ....
		thermal-sensors ...
	}

	skin_zone: skin-sense-left-right-top-thermal {
		.....
	}

	.....
}

************************************
whatever dtsi/dts for whatever device based on whatever soc:

device@0 {
	....

	interrupts = <this that blah>
	interrupt-names = "some";

	clocks = <&clkcontroller 22>;
	clock-names = "main";

	dmas = ....
	dma-names = ....

	pwms = ...
	pwm-names = ....

	.... likewise, for thermal zones ....

	thermal-zones = <&display>, <&skin_zone_a>, <&batt_therm>;
	thermal-zone-names = "disp-therm", "skin-temp", "battery-top";
}
************************************

driver code:

device-driver.c:

/* This driver wants disp, batt, skin because it tries to calculate an
  * optimal battery charging current while trying to not burn users' hands
  * or something like that.
  */
enum therm_to_watch {
	THERM_DISPLAY,
	THERM_BATT_TOP,
	THERM_SKIN_TEMP,
	THERM_MAX
}

static const char * const device_therm_to_watch[THERM_MAX] = {
	"disp-therm", "battery-top", "skin-temp", [...]
};



int some_function(params) {
	[... stuff ...]

	/*
	 * Get the zones associated to the thermal-zone-names in device tree
	 *
	 * ------> This is the main reason why I proposed this commit! :-) <------
	 */
	for (i = 0; i < THERM_MAX; i++) {
		ret = thermal_of_get_zone(dev, device_therm_to_watch[i]);
		if (ret)
			return ret;
	}

	[...]
}
************************************

...my target is currently the MediaTek Smart Voltage Scaling driver, where we have
rather huge platform data (similarly to Qualcomm CPR) for [a / an increasing number
of] SoC(s): we have different SoC thermal zones for CPU big(0/1/2/3/all) and
little(0123all), and GPU, which have got different names (currently "types" in the
thermal framework).

The reason why the zone names are different across those SoCs is that those are
actually somewhat defined in the datasheets, so the names in device tree do reflect
those of the datasheet.
The driver would need cpu-big, cpu-little, or cpu, and gpu thermal zones.

But again, there are other cases apart from MTK SVS.

>> For how I see it, in the thermal framewoek I don't see any "somewhat standardized"
>> helper like the one(s) that I'm introducing with this patch (thermal_of_get_zone(),
>> thermal_of_get_zone_by_index()), and this is the exact reason why I'm proposing
>> this patch.
>>
>> Then again - I mean no disrespect - it's just that I don't understand (yet) why you
>> are saying that "everything is already available", because I really don't see it.
> 
> Ok said differently, thermal zone name and type are messy.
> 
> Let's clarify that and then let's see with the result if adding this thermal zone 
> node/name mapping still makes sense.
> 

Yes, I think that it's sensible at this point to just clarify that in the framework
first, and then go on with the rest.

>>>   - A thermal zone does not have a name but a type
>>>
>>>   - We use the thermal zone DT node name to register as a name but it is a type 
>>> from the thermal framework point of view
>>
>> This is something that I didn't realize before. Thanks for that.
>>
>> ...and yes, we're registering a "name" from DT as a "type" in the framework, this
>> is highly confusing and needs to be cleaned up.
>>
>>>
>>>   - We can register several thermal zones with the same type (so we can have 
>>> duplicate names if we use type as name)
>>>
>>
>> ...which makes sense, after realizing that we're registering a TYPE and not a NAME,
>> and I agree about the logic for which that multiple zones can be of the same type.
>>
>>>   - We use thermal_zone_device_get_by_name() but actually it checks against the 
>>> type and as we can have multiple identical types, the function returns the first 
>>> one found
>>>
>>
>> The first thing that comes to mind is to rename thermal_zone_device_get_by_name()
>> to thermal_zone_device_get_by_type(), but then I'd be reintroducing the former and
>> this gives me concerns about OOT drivers using that and developers getting highly
>> confused (name->type, but name exists again, so they might erroneously just fix the
>> call to xxx_by_name() instead of changing it to xxx_by_type()).
> 
> 
>> Should I *not* be concerned about that?
> 
> Not really :)
> 
> TBH, OOT drivers do not exist from upstream POV.
> 

Happy to see this answer.

>  > Any suggestion?
> 
> Yes, let's introduce a thermal zone name in the tzd.
> 

Let's go! I'll start this work ASAP.

>   - There are now too many parameters to the function 
> thermal_zone_device_register*(), so we can't add a new 'name' parameter. Introduce 
> a thermal_zone_device_parameters structure. This structure will contain all 
> thermal_zone_device_register_* parameter function. There won't be any functional 
> changes, just how the parameters are passed. Perhaps, you should use an 
> intermediate function to not have the change impacting everywhere.
> 
>   - then add a const char *name field in this structure and in the 
> thermal_zone_device structure. So we can assign the name to the thermal zone. The 
> name must be checked against duplicate. If no name is specified when creating a 
> thermal zone, then name = type.
> 
>   - In thermal_of, use the node name for the type and the name, that will be 
> duplicate but we will sort it out later.
> 
>   - Add the name in sysfs
> 

I didn't expect a detailed guidance like that! Even though we seem to think alike,
as in, I was imagining to do exactly that -  thank you, this reduces the actual
brainstorming from my side as makes me sure that we want to do the same thing, and
also makes me able to "make my hands dirty with code" sooner than later.

Speeds up the whole process.

> Second step, track down users of thermal_zone_device_get_by_name() and check if 
> they can use the name instead of the type. I'm pretty sure it is the case for most 
> of the callers.
> 
> With that, there will be a nice clarification IMHO.
> 
> Then we will be able to restate the 'type' with something different without 
> breaking the existing ABI.
> 

Yes, I totally agree with that.

Okay - it looks like we have at least 95% of a plan - it's enough for me to start
writing the cleanup.

Cheers!
Angelo