Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1491642 > unrolled thread

Re: [PATCH] of: thermal: Fixed governor at each thermal zone

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] of: thermal: Fixed governor at each thermal zone Zhang Rui <rui.zhang@intel.com> - 2016-09-27 03:50 +0200
    Re: [PATCH] of: thermal: Fixed governor at each thermal zone Javi Merino <javi.merino@arm.com> - 2016-09-27 13:30 +0200
    Re: [PATCH] of: thermal: Fixed governor at each thermal zone Lukasz Luba <lukasz.luba@arm.com> - 2016-09-27 14:00 +0200
      Re: [PATCH] of: thermal: Fixed governor at each thermal zone Eduardo Valentin <edubezval@gmail.com> - 2016-09-27 15:30 +0200
        Re: [PATCH] of: thermal: Fixed governor at each thermal zone Zhang Rui <rui.zhang@intel.com> - 2016-09-28 03:40 +0200
          RE: [PATCH] of: thermal: Fixed governor at each thermal zone 강인혁/책임연구원/SW Platform(연)AOT팀(hugh.kang@lge.com) <hugh.kang@lge.com> - 2016-09-28 09:20 +0200

#1491642 — Re: [PATCH] of: thermal: Fixed governor at each thermal zone

On 一, 2016-09-19 at 10:18 +0900, Inhyuk Kang wrote:
> It is necessary to be added governor at each thermal_zone.
> Because some governors should be operated in the during the kernel
> booting
> in order to avoid heating problem.
> 
> Default governor cannot be covered all thermal zones policy because
> some thermal zones want to apply different one.
> For example, the power allocator governor operates differently with
> step wise governor.
> Hence, it is better to parse governor parameter from the device tree.
> 
> Signed-off-by: Inhyuk Kang <hugh.kang@lge.com>
> 
The patch looks okay to me.
Eduardo, what do you think of this patch?

thanks,
rui
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-
> thermal.c
> index b8e509c..382c440 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -970,6 +970,7 @@ int __init of_parse_thermal_zones(void)
>  		struct thermal_zone_device *zone;
>  		struct thermal_zone_params *tzp;
>  		int i, mask = 0;
> +		const char *governor;
>  		u32 prop;
>  
>  		tz = thermal_of_build_thermal_zone(child);
> @@ -996,6 +997,9 @@ int __init of_parse_thermal_zones(void)
>  		if (!of_property_read_u32(child, "sustainable-
> power", &prop))
>  			tzp->sustainable_power = prop;
>  
> +		if (!of_property_read_string(child, "governor-name", 
> &governor))
> +			strcpy(tzp->governor_name, governor);
> +
>  		for (i = 0; i < tz->ntrips; i++)
>  			mask |= 1 << i;
>  

[toc] | [next] | [standalone]

#1491829

On Tue, Sep 27, 2016 at 09:46:57AM +0800, Zhang Rui wrote:
> On 一, 2016-09-19 at 10:18 +0900, Inhyuk Kang wrote:
> > It is necessary to be added governor at each thermal_zone.
> > Because some governors should be operated in the during the kernel
> > booting
> > in order to avoid heating problem.
> > 
> > Default governor cannot be covered all thermal zones policy because
> > some thermal zones want to apply different one.
> > For example, the power allocator governor operates differently with
> > step wise governor.
> > Hence, it is better to parse governor parameter from the device tree.
> > 
> > Signed-off-by: Inhyuk Kang <hugh.kang@lge.com>
> > 
> The patch looks okay to me.
> Eduardo, what do you think of this patch?

This has been proposed in the past[0] and Eduardo said no[1] (as did
Krzysztof Kozlowski and Mark Rutland)

[0] https://marc.info/?l=linux-kernel&m=143893141227189&w=4
[1] https://marc.info/?l=linux-pm&m=144649947022547&w=4

Cheers,
Javi

> > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-
> > thermal.c
> > index b8e509c..382c440 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -970,6 +970,7 @@ int __init of_parse_thermal_zones(void)
> >  		struct thermal_zone_device *zone;
> >  		struct thermal_zone_params *tzp;
> >  		int i, mask = 0;
> > +		const char *governor;
> >  		u32 prop;
> >  
> >  		tz = thermal_of_build_thermal_zone(child);
> > @@ -996,6 +997,9 @@ int __init of_parse_thermal_zones(void)
> >  		if (!of_property_read_u32(child, "sustainable-
> > power", &prop))
> >  			tzp->sustainable_power = prop;
> >  
> > +		if (!of_property_read_string(child, "governor-name", 
> > &governor))
> > +			strcpy(tzp->governor_name, governor);
> > +
> >  		for (i = 0; i < tz->ntrips; i++)
> >  			mask |= 1 << i;
> >  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1491848

On 27/09/16 02:46, Zhang Rui wrote:
> On 一, 2016-09-19 at 10:18 +0900, Inhyuk Kang wrote:
>> It is necessary to be added governor at each thermal_zone.
>> Because some governors should be operated in the during the kernel
>> booting
>> in order to avoid heating problem.
>>
>> Default governor cannot be covered all thermal zones policy because
>> some thermal zones want to apply different one.
>> For example, the power allocator governor operates differently with
>> step wise governor.
>> Hence, it is better to parse governor parameter from the device tree.
>>
>> Signed-off-by: Inhyuk Kang <hugh.kang@lge.com>
>>
> The patch looks okay to me.
> Eduardo, what do you think of this patch?
Hi Rui,

Beside the fact which Javi pointed out in his email, there is an issue 
in the patch itself.
The idea behind the patch is good, but the patch should have some 
improvements, i.e:
- strncpy instead of strcpy,
- if the governor name is not found in the registered governor's list by 
__find_governor (and then null is set) we should probably switch to 
default governor,
- add DT documentation,

Regards,
Lukasz

>
> thanks,
> rui
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-
>> thermal.c
>> index b8e509c..382c440 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -970,6 +970,7 @@ int __init of_parse_thermal_zones(void)
>>  		struct thermal_zone_device *zone;
>>  		struct thermal_zone_params *tzp;
>>  		int i, mask = 0;
>> +		const char *governor;
>>  		u32 prop;
>>
>>  		tz = thermal_of_build_thermal_zone(child);
>> @@ -996,6 +997,9 @@ int __init of_parse_thermal_zones(void)
>>  		if (!of_property_read_u32(child, "sustainable-
>> power", &prop))
>>  			tzp->sustainable_power = prop;
>>
>> +		if (!of_property_read_string(child, "governor-name",
>> &governor))
>> +			strcpy(tzp->governor_name, governor);
>> +
>>  		for (i = 0; i < tz->ntrips; i++)
>>  			mask |= 1 << i;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1491898

Hello, Lukasz, Inhyuk, Javi,

On Tue, Sep 27, 2016 at 12:52:04PM +0100, Lukasz Luba wrote:
> 
> On 27/09/16 02:46, Zhang Rui wrote:
> >On 一, 2016-09-19 at 10:18 +0900, Inhyuk Kang wrote:
> >>It is necessary to be added governor at each thermal_zone.
> >>Because some governors should be operated in the during the kernel
> >>booting
> >>in order to avoid heating problem.
> >>
> >>Default governor cannot be covered all thermal zones policy because
> >>some thermal zones want to apply different one.
> >>For example, the power allocator governor operates differently with
> >>step wise governor.
> >>Hence, it is better to parse governor parameter from the device tree.
> >>
> >>Signed-off-by: Inhyuk Kang <hugh.kang@lge.com>
> >>
> >The patch looks okay to me.
> >Eduardo, what do you think of this patch?
> Hi Rui,
> 
> Beside the fact which Javi pointed out in his email, there is an issue in
> the patch itself.
> The idea behind the patch is good, but the patch should have some
> improvements, i.e:
> - strncpy instead of strcpy,
> - if the governor name is not found in the registered governor's list by
> __find_governor (and then null is set) we should probably switch to default
> governor,
> - add DT documentation,

Also, the idea of the patch is good, almost tempting to do it, but
unfortunately, not acceptable from DT perspective. The patch infringes
two of the DT conceptual and design decision of:
(a) DT should describe hardware, not policy;
(b) DT should describe hardware, not OS specific implementations.

As already pointed by Javi, this patch has already been proposed (more
than one time by different people), but, it still continues to be
unacceptable.

Cheers,


> 
> Regards,
> Lukasz
> 

[toc] | [prev] | [next] | [standalone]

#1492230

Hi, Javi, Lukasz and Eduardo,

thanks for your input.

thanks,
rui

On 二, 2016-09-27 at 06:22 -0700, Eduardo Valentin wrote:
> Hello, Lukasz, Inhyuk, Javi,
> 
> On Tue, Sep 27, 2016 at 12:52:04PM +0100, Lukasz Luba wrote:
> > 
> > 
> > On 27/09/16 02:46, Zhang Rui wrote:
> > > 
> > > On 一, 2016-09-19 at 10:18 +0900, Inhyuk Kang wrote:
> > > > 
> > > > It is necessary to be added governor at each thermal_zone.
> > > > Because some governors should be operated in the during the
> > > > kernel
> > > > booting
> > > > in order to avoid heating problem.
> > > > 
> > > > Default governor cannot be covered all thermal zones policy
> > > > because
> > > > some thermal zones want to apply different one.
> > > > For example, the power allocator governor operates differently
> > > > with
> > > > step wise governor.
> > > > Hence, it is better to parse governor parameter from the device
> > > > tree.
> > > > 
> > > > Signed-off-by: Inhyuk Kang <hugh.kang@lge.com>
> > > > 
> > > The patch looks okay to me.
> > > Eduardo, what do you think of this patch?
> > Hi Rui,
> > 
> > Beside the fact which Javi pointed out in his email, there is an
> > issue in
> > the patch itself.
> > The idea behind the patch is good, but the patch should have some
> > improvements, i.e:
> > - strncpy instead of strcpy,
> > - if the governor name is not found in the registered governor's
> > list by
> > __find_governor (and then null is set) we should probably switch to
> > default
> > governor,
> > - add DT documentation,
> Also, the idea of the patch is good, almost tempting to do it, but
> unfortunately, not acceptable from DT perspective. The patch
> infringes
> two of the DT conceptual and design decision of:
> (a) DT should describe hardware, not policy;
> (b) DT should describe hardware, not OS specific implementations.
> 
> As already pointed by Javi, this patch has already been proposed
> (more
> than one time by different people), but, it still continues to be
> unacceptable.
> 
> Cheers,
> 
> 
> > 
> > 
> > Regards,
> > Lukasz
> > 

[toc] | [prev] | [next] | [standalone]

#1492295

Hello Rui, Javi, Lukasz and Eduardo

> -----Original Message-----
> From: Zhang Rui [mailto:rui.zhang@intel.com]
> Sent: Wednesday, September 28, 2016 10:31 AM
> To: Eduardo Valentin; Lukasz Luba
> Cc: Inhyuk Kang; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] of: thermal: Fixed governor at each thermal zone
> 
> Hi, Javi, Lukasz and Eduardo,
> 
> thanks for your input.
> 
> thanks,
> rui
> 
> On 二, 2016-09-27 at 06:22 -0700, Eduardo Valentin wrote:
> > Hello, Lukasz, Inhyuk, Javi,
> >
> > On Tue, Sep 27, 2016 at 12:52:04PM +0100, Lukasz Luba wrote:
> > >
> > >
> > > On 27/09/16 02:46, Zhang Rui wrote:
> > > >
> > > > On 一, 2016-09-19 at 10:18 +0900, Inhyuk Kang wrote:
> > > > >
> > > > > It is necessary to be added governor at each thermal_zone.
> > > > > Because some governors should be operated in the during the
> > > > > kernel booting in order to avoid heating problem.
> > > > >
> > > > > Default governor cannot be covered all thermal zones policy
> > > > > because some thermal zones want to apply different one.
> > > > > For example, the power allocator governor operates differently
> > > > > with step wise governor.
> > > > > Hence, it is better to parse governor parameter from the device
> > > > > tree.
> > > > >
> > > > > Signed-off-by: Inhyuk Kang <hugh.kang@lge.com>
> > > > >
> > > > The patch looks okay to me.
> > > > Eduardo, what do you think of this patch?
> > > Hi Rui,
> > >
> > > Beside the fact which Javi pointed out in his email, there is an
> > > issue in the patch itself.
> > > The idea behind the patch is good, but the patch should have some
> > > improvements, i.e:
> > > - strncpy instead of strcpy,
> > > - if the governor name is not found in the registered governor's
> > > list by __find_governor (and then null is set) we should probably
> > > switch to default governor,
> > > - add DT documentation,
> > Also, the idea of the patch is good, almost tempting to do it, but
> > unfortunately, not acceptable from DT perspective. The patch infringes
> > two of the DT conceptual and design decision of:
> > (a) DT should describe hardware, not policy;
> > (b) DT should describe hardware, not OS specific implementations.
> >
> > As already pointed by Javi, this patch has already been proposed (more
> > than one time by different people), but, it still continues to be
> > unacceptable.
> >
> > Cheers,
> >
> >
> > >
> > >
> > > Regards,
> > > Lukasz
> > >
Thank you for reviewing this patch. I understand your ideas. 
During the thermal testing into device, the thermal policy should be applied in the begging of kernel boot sometimes.
Hence, I have suggested above solution.

Anyway, the DT perspective like above, this patch is better not to be upstream.

Thank you very much.

Regards,
Hugh Kang

No One ever is defeated until defeat has been accepted as a reality.
강인혁 (Hugh Kang) Chief Engineer
SW Platform(연) Advanced OS Technology TEAM (AOT)
Mobile: +82-10-5513-7957

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web