On 11/13/2013 03:46 AM, Eduardo Valentin wrote:
> ...
> +
> +/**
> + * of_parse_thermal_zones - parse device tree thermal data
> + *
> + * Initialization function that can be called by machine initialization
> + * code to parse thermal data and populate the thermal framework
> + * with hardware thermal zones info. This function only parses thermal zones.
> + * Cooling devices and sensor devices nodes are supposed to be parsed
> + * by their respective drivers.
> + *
> + * Return: 0 on success, proper error code otherwise
> + *
> + */
> +int __init of_parse_thermal_zones(void)
> +{
> + struct device_node *np, *child;
> + struct __thermal_zone *tz;
> + struct thermal_zone_device_ops *ops;
> +
> + np = of_find_node_by_name(NULL, "thermal-zones");
> + if (!np) {
> + pr_debug("unable to find thermal zones\n");
> + return 0; /* Run successfully on systems without thermal DT */
> + }
> +
> + for_each_child_of_node(np, child) {
> + struct thermal_zone_device *zone;
> + struct thermal_zone_params *tzp;
> +
> + tz = thermal_of_build_thermal_zone(child);
> + if (IS_ERR(tz)) {
> + pr_err("failed to build thermal zone %s: %ld\n",
> + child->name,
> + PTR_ERR(tz));
> + continue;
> + }
> +
> + ops = kmemdup(&of_thermal_ops, sizeof(*ops), GFP_KERNEL);
> + if (!ops)
> + goto exit_free;
> +
> + tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
> + if (!tzp) {
> + kfree(ops);
> + goto exit_free;
> + }
> +
> + /* No hwmon because there might be hwmon drivers registering */
> + tzp->no_hwmon = true;
I think the platform driver may set governor for the thermal zone,
so how about to add a property named as "governor",
and parse it to tzp->governor_name,
something like:
ret = of_property_read_string(child, "governor", &str);
if (ret == 0)
if (strlen(str) < THERMAL_NAME_LENGTH)
strcpy(tzp->governor_name, str);
Thanks.
Wei.
> +
> + zone = thermal_zone_device_register(child->name, tz->ntrips,
> + 0, tz,
> + ops, tzp,
> + tz->passive_delay,
> + tz->polling_delay);
> + if (IS_ERR(zone)) {
> + pr_err("Failed to build %s zone %ld\n", child->name,
> + PTR_ERR(zone));
> + kfree(tzp);
> + kfree(ops);
> + of_thermal_free_zone(tz);
> + /* attempting to build remaining zones still */
> + }
> + }
> +
> + return 0;
> +
> +exit_free:
> + of_thermal_free_zone(tz);
> +
> + /* no memory available, so free what we have built */
> + of_thermal_destroy_zones();
> +
> + return -ENOMEM;
> +}
> +
> +/**
> + * of_thermal_destroy_zones - remove all zones parsed and allocated resources
> + *
> + * Finds all zones parsed and added to the thermal framework and remove them
> + * from the system, together with their resources.
> + *
> + */
> +void __exit of_thermal_destroy_zones(void)
> +{
> + struct device_node *np, *child;
> +
> + np = of_find_node_by_name(NULL, "thermal-zones");
> + if (!np) {
> + pr_err("unable to find thermal zones\n");
> + return;
> + }
> +
> + for_each_child_of_node(np, child) {
> + struct thermal_zone_device *zone;
> +
> + zone = thermal_zone_get_zone_by_name(child->name);
> + if (IS_ERR(zone))
> + continue;
> +
> + thermal_zone_device_unregister(zone);
> + kfree(zone->tzp);
> + kfree(zone->ops);
> + of_thermal_free_zone(zone->devdata);
> + }
> +}
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f4c9021..aba68dc 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1373,7 +1373,7 @@ static void remove_trip_attrs(struct thermal_zone_device *tz)
> */
> struct thermal_zone_device *thermal_zone_device_register(const char *type,
> int trips, int mask, void *devdata,
> - const struct thermal_zone_device_ops *ops,
> + struct thermal_zone_device_ops *ops,
> const struct thermal_zone_params *tzp,
> int passive_delay, int polling_delay)
> {
> @@ -1753,8 +1753,14 @@ static int __init thermal_init(void)
> if (result)
> goto unregister_class;
>
> + result = of_parse_thermal_zones();
> + if (result)
> + goto exit_netlink;
> +
> return 0;
>
> +exit_netlink:
> + genetlink_exit();
> unregister_governors:
> thermal_unregister_governors();
> unregister_class:
> @@ -1770,6 +1776,7 @@ error:
>
> static void __exit thermal_exit(void)
> {
> + of_thermal_destroy_zones();
> genetlink_exit();
> class_unregister(&thermal_class);
> thermal_unregister_governors();
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 7cf2f66..3db339f 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -77,4 +77,13 @@ static inline int thermal_gov_user_space_register(void) { return 0; }
> static inline void thermal_gov_user_space_unregister(void) {}
> #endif /* CONFIG_THERMAL_GOV_USER_SPACE */
>
> +/* device tree support */
> +#ifdef CONFIG_THERMAL_OF
> +int of_parse_thermal_zones(void);
> +void of_thermal_destroy_zones(void);
> +#else
> +static inline int of_parse_thermal_zones(void) { return 0; }
> +static inline void of_thermal_destroy_zones(void) { }
> +#endif
> +
> #endif /* __THERMAL_CORE_H__ */
> diff --git a/include/dt-bindings/thermal/thermal.h b/include/dt-bindings/thermal/thermal.h
> new file mode 100644
> index 0000000..59822a9
> --- /dev/null
> +++ b/include/dt-bindings/thermal/thermal.h
> @@ -0,0 +1,17 @@
> +/*
> + * This header provides constants for most thermal bindings.
> + *
> + * Copyright (C) 2013 Texas Instruments
> + * Eduardo Valentin <[email protected]>
> + *
> + * GPLv2 only
> + */
> +
> +#ifndef _DT_BINDINGS_THERMAL_THERMAL_H
> +#define _DT_BINDINGS_THERMAL_THERMAL_H
> +
> +/* On cooling devices upper and lower limits */
> +#define THERMAL_NO_LIMIT (-1UL)
> +
> +#endif
> +
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index b268d3c..b780c5b 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -143,6 +143,7 @@ struct thermal_cooling_device {
> int id;
> char type[THERMAL_NAME_LENGTH];
> struct device device;
> + struct device_node *np;
> void *devdata;
> const struct thermal_cooling_device_ops *ops;
> bool updated; /* true if the cooling device does not need update */
> @@ -172,7 +173,7 @@ struct thermal_zone_device {
> int emul_temperature;
> int passive;
> unsigned int forced_passive;
> - const struct thermal_zone_device_ops *ops;
> + struct thermal_zone_device_ops *ops;
> const struct thermal_zone_params *tzp;
> struct thermal_governor *governor;
> struct list_head thermal_instances;
> @@ -242,8 +243,31 @@ struct thermal_genl_event {
> };
>
> /* Function declarations */
> +#ifdef CONFIG_THERMAL_OF
> +struct thermal_zone_device *
> +thermal_zone_of_sensor_register(struct device *dev, int id,
> + void *data, int (*get_temp)(void *, long *),
> + int (*get_trend)(void *, long *));
> +void thermal_zone_of_sensor_unregister(struct device *dev,
> + struct thermal_zone_device *tz);
> +#else
> +static inline struct thermal_zone_device *
> +thermal_zone_of_sensor_register(struct device *dev, int id,
> + void *data, int (*get_temp)(void *, long *),
> + int (*get_trend)(void *, long *))
> +{
> + return NULL;
> +}
> +
> +static inline
> +void thermal_zone_of_sensor_unregister(struct device *dev,
> + struct thermal_zone_device *tz)
> +{
> +}
> +
> +#endif
> struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
> - void *, const struct thermal_zone_device_ops *,
> + void *, struct thermal_zone_device_ops *,
> const struct thermal_zone_params *, int, int);
> void thermal_zone_device_unregister(struct thermal_zone_device *);
>
> I think the platform driver may set governor for the thermal zone,
> so how about to add a property named as "governor",
> and parse it to tzp->governor_name,
> something like:
> ret = of_property_read_string(child, "governor", &str);
> if (ret == 0)
> if (strlen(str) < THERMAL_NAME_LENGTH)
> strcpy(tzp->governor_name, str);
>
> Thanks.
> Wei.
DT is supposed to describe the hardware, right? The governor isn't
hardware -- it's a software control policy. On the other hand, that
control policy must be tuned according to the behaviors of the platform
hardware otherwise the system will be unstable.
Is it appropriate to be naming the governor in DT? If so, is it equally
appropriate to describe any governor-specific parameters in DT (even
though they are pure software constructs)?
-Matt
On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
>
> > I think the platform driver may set governor for the thermal zone,
> > so how about to add a property named as "governor",
> > and parse it to tzp->governor_name,
> > something like:
> > ret = of_property_read_string(child, "governor", &str);
> > if (ret == 0)
> > if (strlen(str) < THERMAL_NAME_LENGTH)
> > strcpy(tzp->governor_name, str);
> >
> > Thanks.
> > Wei.
>
> DT is supposed to describe the hardware, right? The governor isn't
> hardware -- it's a software control policy. On the other hand, that
> control policy must be tuned according to the behaviors of the platform
> hardware otherwise the system will be unstable.
>
> Is it appropriate to be naming the governor in DT? If so, is it equally
> appropriate to describe any governor-specific parameters in DT (even
> though they are pure software constructs)?
The dt should be relatively static -- if the hardware doesn't change the
dt shouldn't have to.
The governers are not static. We can introduce new ones and throw away
old ones at any time. Tuning parameters can also change at any time.
I'd prefer to not have governer details described in the dt, and the
choice of governer and configuration of its tuning parameters should be
made at runtime somehow.
Thanks,
Mark.
On 06-01-2014 09:51, Mark Rutland wrote:
> On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
>>
>>> I think the platform driver may set governor for the thermal zone,
>>> so how about to add a property named as "governor",
>>> and parse it to tzp->governor_name,
>>> something like:
>>> ret = of_property_read_string(child, "governor", &str);
>>> if (ret == 0)
>>> if (strlen(str) < THERMAL_NAME_LENGTH)
>>> strcpy(tzp->governor_name, str);
>>>
>>> Thanks.
>>> Wei.
>>
>> DT is supposed to describe the hardware, right? The governor isn't
>> hardware -- it's a software control policy. On the other hand, that
>> control policy must be tuned according to the behaviors of the platform
>> hardware otherwise the system will be unstable.
>>
>> Is it appropriate to be naming the governor in DT? If so, is it equally
>> appropriate to describe any governor-specific parameters in DT (even
>> though they are pure software constructs)?
>
> The dt should be relatively static -- if the hardware doesn't change the
> dt shouldn't have to.
>
> The governers are not static. We can introduce new ones and throw away
> old ones at any time. Tuning parameters can also change at any time.
>
> I'd prefer to not have governer details described in the dt, and the
> choice of governer and configuration of its tuning parameters should be
> made at runtime somehow.
Agreed.
>
> Thanks,
> Mark.
>
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
On 01/06/2014 10:54 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
>
> On 06-01-2014 09:51, Mark Rutland wrote:
>> On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
>>>
>>>> I think the platform driver may set governor for the thermal zone,
>>>> so how about to add a property named as "governor",
>>>> and parse it to tzp->governor_name,
>>>> something like:
>>>> ret = of_property_read_string(child, "governor", &str);
>>>> if (ret == 0)
>>>> if (strlen(str) < THERMAL_NAME_LENGTH)
>>>> strcpy(tzp->governor_name, str);
>>>>
>>>> Thanks.
>>>> Wei.
>>>
>>> DT is supposed to describe the hardware, right? The governor isn't
>>> hardware -- it's a software control policy. On the other hand, that
>>> control policy must be tuned according to the behaviors of the platform
>>> hardware otherwise the system will be unstable.
>>>
>>> Is it appropriate to be naming the governor in DT? If so, is it equally
>>> appropriate to describe any governor-specific parameters in DT (even
>>> though they are pure software constructs)?
>>
>> The dt should be relatively static -- if the hardware doesn't change the
>> dt shouldn't have to.
>>
>> The governers are not static. We can introduce new ones and throw away
>> old ones at any time. Tuning parameters can also change at any time.
>>
>> I'd prefer to not have governer details described in the dt, and the
>> choice of governer and configuration of its tuning parameters should be
>> made at runtime somehow.
>
> Agreed.
Yes, I think so, but the of-thermal driver handle the
thermal_zone_device_register, and pass the "tzp" without governor_name,
so the created thermal_zone's governor will be NULL, then it can't run
into the governor->throttle() if needed. And currently there have no
interface to support updating governor and configuration at runtime.
I think it's better to initialize the governor_name when register the
thermal zone device in the of-thermal driver.
Thanks.
>
>>
>> Thanks,
>> Mark.
>>
>>
>
>
On Tue, Jan 07, 2014 at 02:44:10AM +0000, Wei Ni wrote:
> On 01/06/2014 10:54 PM, Eduardo Valentin wrote:
> > * PGP Signed by an unknown key
> >
> > On 06-01-2014 09:51, Mark Rutland wrote:
> >> On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
> >>>
> >>>> I think the platform driver may set governor for the thermal zone,
> >>>> so how about to add a property named as "governor",
> >>>> and parse it to tzp->governor_name,
> >>>> something like:
> >>>> ret = of_property_read_string(child, "governor", &str);
> >>>> if (ret == 0)
> >>>> if (strlen(str) < THERMAL_NAME_LENGTH)
> >>>> strcpy(tzp->governor_name, str);
> >>>>
> >>>> Thanks.
> >>>> Wei.
> >>>
> >>> DT is supposed to describe the hardware, right? The governor isn't
> >>> hardware -- it's a software control policy. On the other hand, that
> >>> control policy must be tuned according to the behaviors of the platform
> >>> hardware otherwise the system will be unstable.
> >>>
> >>> Is it appropriate to be naming the governor in DT? If so, is it equally
> >>> appropriate to describe any governor-specific parameters in DT (even
> >>> though they are pure software constructs)?
> >>
> >> The dt should be relatively static -- if the hardware doesn't change the
> >> dt shouldn't have to.
> >>
> >> The governers are not static. We can introduce new ones and throw away
> >> old ones at any time. Tuning parameters can also change at any time.
> >>
> >> I'd prefer to not have governer details described in the dt, and the
> >> choice of governer and configuration of its tuning parameters should be
> >> made at runtime somehow.
> >
> > Agreed.
>
> Yes, I think so, but the of-thermal driver handle the
> thermal_zone_device_register, and pass the "tzp" without governor_name,
> so the created thermal_zone's governor will be NULL, then it can't run
> into the governor->throttle() if needed. And currently there have no
> interface to support updating governor and configuration at runtime.
> I think it's better to initialize the governor_name when register the
> thermal zone device in the of-thermal driver.
Initialising it in the driver doesn't mean it has to come from the
device tree. That's still a run-time decision, even though it's made
immediately after parsing the DT.
Thanks,
Mark.
On 02-01-2014 13:50, Matthew Longnecker wrote:
>
>> I think the platform driver may set governor for the thermal zone,
>> so how about to add a property named as "governor",
>> and parse it to tzp->governor_name,
>> something like:
>> ret = of_property_read_string(child, "governor", &str);
>> if (ret == 0)
>> if (strlen(str) < THERMAL_NAME_LENGTH)
>> strcpy(tzp->governor_name, str);
The above is not applicable to DT. The very first version of my proposed
series did include some sort of governor treatment too, fetching such
data from DT.
>>
>> Thanks.
>> Wei.
>
> DT is supposed to describe the hardware, right? The governor isn't
> hardware -- it's a software control policy. On the other hand, that
> control policy must be tuned according to the behaviors of the platform
> hardware otherwise the system will be unstable.
Yes, this is the correct understanding. We can describe hardware,
including thermal constraints, as long as they do not include policy or
OS specific implementation.
>
> Is it appropriate to be naming the governor in DT? If so, is it equally
> appropriate to describe any governor-specific parameters in DT (even
> though they are pure software constructs)?
No for both questions.
However, for the parameters, as long as you can map the software
parameter as a hardware descriptor, then we can discuss if that can be
used as DT properties.
>
> -Matt
>
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
Wei,
On 06-01-2014 22:44, Wei Ni wrote:
> On 01/06/2014 10:54 PM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> On 06-01-2014 09:51, Mark Rutland wrote:
>>> On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
>>>>
>>>>> I think the platform driver may set governor for the thermal zone,
>>>>> so how about to add a property named as "governor",
>>>>> and parse it to tzp->governor_name,
>>>>> something like:
>>>>> ret = of_property_read_string(child, "governor", &str);
>>>>> if (ret == 0)
>>>>> if (strlen(str) < THERMAL_NAME_LENGTH)
>>>>> strcpy(tzp->governor_name, str);
>>>>>
>>>>> Thanks.
>>>>> Wei.
>>>>
>>>> DT is supposed to describe the hardware, right? The governor isn't
>>>> hardware -- it's a software control policy. On the other hand, that
>>>> control policy must be tuned according to the behaviors of the platform
>>>> hardware otherwise the system will be unstable.
>>>>
>>>> Is it appropriate to be naming the governor in DT? If so, is it equally
>>>> appropriate to describe any governor-specific parameters in DT (even
>>>> though they are pure software constructs)?
>>>
>>> The dt should be relatively static -- if the hardware doesn't change the
>>> dt shouldn't have to.
>>>
>>> The governers are not static. We can introduce new ones and throw away
>>> old ones at any time. Tuning parameters can also change at any time.
>>>
>>> I'd prefer to not have governer details described in the dt, and the
>>> choice of governer and configuration of its tuning parameters should be
>>> made at runtime somehow.
>>
>> Agreed.
>
> Yes, I think so, but the of-thermal driver handle the
> thermal_zone_device_register, and pass the "tzp" without governor_name,
In fact, it will fall into the default governor, which is step_wise, by
default config.
> so the created thermal_zone's governor will be NULL, then it can't run
> into the governor->throttle() if needed. And currently there have no
Actually, no, the tz will be set to default governor, and its throttle
call will be called.
> interface to support updating governor and configuration at runtime.
> I think it's better to initialize the governor_name when register the
> thermal zone device in the of-thermal driver.
Still, why would you need to change the governor from a in kernel
decision? There is an ABI to change the thermal zone policy based on
user(land) request. If you need to change the policy from within the
kernel, which seams to be what you are trying to propose, you need to
explain why you need it, say, by giving at least one user of this API or
explaining its use case.
>
> Thanks.
>
>>
>>>
>>> Thanks,
>>> Mark.
>>>
>>>
>>
>>
>
>
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
On 01/14/2014 05:29 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
>
> Wei,
>
> On 06-01-2014 22:44, Wei Ni wrote:
>> On 01/06/2014 10:54 PM, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> On 06-01-2014 09:51, Mark Rutland wrote:
>>>> On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
>>>>>
>>>>>> I think the platform driver may set governor for the thermal zone,
>>>>>> so how about to add a property named as "governor",
>>>>>> and parse it to tzp->governor_name,
>>>>>> something like:
>>>>>> ret = of_property_read_string(child, "governor", &str);
>>>>>> if (ret == 0)
>>>>>> if (strlen(str) < THERMAL_NAME_LENGTH)
>>>>>> strcpy(tzp->governor_name, str);
>>>>>>
>>>>>> Thanks.
>>>>>> Wei.
>>>>>
>>>>> DT is supposed to describe the hardware, right? The governor isn't
>>>>> hardware -- it's a software control policy. On the other hand, that
>>>>> control policy must be tuned according to the behaviors of the platform
>>>>> hardware otherwise the system will be unstable.
>>>>>
>>>>> Is it appropriate to be naming the governor in DT? If so, is it equally
>>>>> appropriate to describe any governor-specific parameters in DT (even
>>>>> though they are pure software constructs)?
>>>>
>>>> The dt should be relatively static -- if the hardware doesn't change the
>>>> dt shouldn't have to.
>>>>
>>>> The governers are not static. We can introduce new ones and throw away
>>>> old ones at any time. Tuning parameters can also change at any time.
>>>>
>>>> I'd prefer to not have governer details described in the dt, and the
>>>> choice of governer and configuration of its tuning parameters should be
>>>> made at runtime somehow.
>>>
>>> Agreed.
>>
>> Yes, I think so, but the of-thermal driver handle the
>> thermal_zone_device_register, and pass the "tzp" without governor_name,
>
> In fact, it will fall into the default governor, which is step_wise, by
> default config.
In the thermal_zone_device_register(), it has following codes:
if (tz->tzp)
tz->governor = __find_governor(tz->tzp->governor_name);
else
tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
It mean if the tz->tzp is not NULL, and the governor_name is NULL, then
the __find_governor() will return NULL, so the tz->governor is NULL, it
can't fall into the default governor. And in the of-thermal driver, it
call the thermal_zone_device_register(), and pass the "tzp" without
governor_name.
I think if we want to change the governor in user space, we need to fix
this first.
>
>> so the created thermal_zone's governor will be NULL, then it can't run
>> into the governor->throttle() if needed. And currently there have no
>
> Actually, no, the tz will be set to default governor, and its throttle
> call will be called.
>
>> interface to support updating governor and configuration at runtime.
>> I think it's better to initialize the governor_name when register the
>> thermal zone device in the of-thermal driver.
>
> Still, why would you need to change the governor from a in kernel
> decision? There is an ABI to change the thermal zone policy based on
> user(land) request. If you need to change the policy from within the
> kernel, which seams to be what you are trying to propose, you need to
> explain why you need it, say, by giving at least one user of this API or
> explaining its use case.
The thermal_zone_device_register() support to set the governor which you
want, but with the of-thermal framework, it only support to set to
default governor, if fix above issue. I think the driver which use the
of-thermal should be able to set to any governors which it want, in its
initialization. So I add the function thermal_update_governor().
>
>>
>> Thanks.
>>
>>>
>>>>
>>>> Thanks,
>>>> Mark.
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
Hello Wei,
On 13-01-2014 22:54, Wei Ni wrote:
> On 01/14/2014 05:29 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> Wei,
>>
>> On 06-01-2014 22:44, Wei Ni wrote:
>>> On 01/06/2014 10:54 PM, Eduardo Valentin wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On 06-01-2014 09:51, Mark Rutland wrote:
>>>>> On Thu, Jan 02, 2014 at 05:50:06PM +0000, Matthew Longnecker wrote:
>>>>>>
>>>>>>> I think the platform driver may set governor for the thermal zone,
>>>>>>> so how about to add a property named as "governor",
>>>>>>> and parse it to tzp->governor_name,
>>>>>>> something like:
>>>>>>> ret = of_property_read_string(child, "governor", &str);
>>>>>>> if (ret == 0)
>>>>>>> if (strlen(str) < THERMAL_NAME_LENGTH)
>>>>>>> strcpy(tzp->governor_name, str);
>>>>>>>
>>>>>>> Thanks.
>>>>>>> Wei.
>>>>>>
>>>>>> DT is supposed to describe the hardware, right? The governor isn't
>>>>>> hardware -- it's a software control policy. On the other hand, that
>>>>>> control policy must be tuned according to the behaviors of the platform
>>>>>> hardware otherwise the system will be unstable.
>>>>>>
>>>>>> Is it appropriate to be naming the governor in DT? If so, is it equally
>>>>>> appropriate to describe any governor-specific parameters in DT (even
>>>>>> though they are pure software constructs)?
>>>>>
>>>>> The dt should be relatively static -- if the hardware doesn't change the
>>>>> dt shouldn't have to.
>>>>>
>>>>> The governers are not static. We can introduce new ones and throw away
>>>>> old ones at any time. Tuning parameters can also change at any time.
>>>>>
>>>>> I'd prefer to not have governer details described in the dt, and the
>>>>> choice of governer and configuration of its tuning parameters should be
>>>>> made at runtime somehow.
>>>>
>>>> Agreed.
>>>
>>> Yes, I think so, but the of-thermal driver handle the
>>> thermal_zone_device_register, and pass the "tzp" without governor_name,
>>
>> In fact, it will fall into the default governor, which is step_wise, by
>> default config.
>
> In the thermal_zone_device_register(), it has following codes:
> if (tz->tzp)
> tz->governor = __find_governor(tz->tzp->governor_name);
> else
> tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
>
> It mean if the tz->tzp is not NULL, and the governor_name is NULL, then
> the __find_governor() will return NULL, so the tz->governor is NULL, it
> can't fall into the default governor. And in the of-thermal driver, it
> call the thermal_zone_device_register(), and pass the "tzp" without
> governor_name.
Yes, it is a bug in the thermal core, thanks for reporting. Something
like [1] should fix it.
> I think if we want to change the governor in user space, we need to fix
> this first.
>
Well no, the above bug does not prevent you to switch governors in
userspace.
[1] - https://patchwork.kernel.org/patch/3487491/
>>
>>> so the created thermal_zone's governor will be NULL, then it can't run
>>> into the governor->throttle() if needed. And currently there have no
>>
>> Actually, no, the tz will be set to default governor, and its throttle
>> call will be called.
>>
>>> interface to support updating governor and configuration at runtime.
>>> I think it's better to initialize the governor_name when register the
>>> thermal zone device in the of-thermal driver.
>>
>> Still, why would you need to change the governor from a in kernel
>> decision? There is an ABI to change the thermal zone policy based on
>> user(land) request. If you need to change the policy from within the
>> kernel, which seams to be what you are trying to propose, you need to
>> explain why you need it, say, by giving at least one user of this API or
>> explaining its use case.
>
> The thermal_zone_device_register() support to set the governor which you
> want, but with the of-thermal framework, it only support to set to
> default governor, if fix above issue. I think the driver which use the
> of-thermal should be able to set to any governors which it want, in its
> initialization. So I add the function thermal_update_governor().
>
>>
>>>
>>> Thanks.
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Mark.
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin