Re: [PATCHv9 02/20] thermal: introduce device tree parser

[Eduardo Valentin]

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

Attachment: signature.asc
Description: OpenPGP digital signature