Re: [RESEND PATCH] MAINTAINERS: update thermal CPU cooling section

[Javi Merino]

On Fri, Apr 02, 2021 at 12:53:08PM +0200, Daniel Lezcano wrote:
> On 02/04/2021 12:25, Lukasz Luba wrote:
> > Hi Viresh, Daniel
> > 
> > On 2/18/21 4:18 AM, Viresh Kumar wrote:
> >> On 17-02-21, 11:59, Lukasz Luba wrote:
> >>> Update maintainers responsible for CPU cooling on Arm side.
> >>>
> >>> Signed-off-by: Lukasz Luba 
> >>> ---
> >>> Hi Daniel,
> >>>
> >>> Please ignore the previous email and that this change with 'R'.
> >>> Javi will ack it later.
> >>>
> >>> Regards,
> >>> Lukasz
> >>>
> >>>   MAINTAINERS | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index f32ebcff37d2..fe34f56acb0f 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -17774,7 +17774,7 @@ THERMAL/CPU_COOLING
> >>>   M:    Amit Daniel Kachhap 
> >>>   M:    Daniel Lezcano 
> >>>   M:    Viresh Kumar 
> >>> -M:    Javi Merino 
> >>> +R:    Lukasz Luba 
> >>>   L:    linux...@vger.kernel.org
> >>>   S:    Supported
> >>>   F:    Documentation/driver-api/thermal/cpu-cooling-api.rst
> >>
> >> Good that we have one more reviewer for this :)
> >>
> >> Acked-by: Viresh Kumar 
> >>
> > 
> > I believe it has lost somewhere in people mailboxes.
> > 
> > Thank you Viresh for the ACK.
> > 
> > Could you Daniel (or you Viresh) take this patch, please?
> 
> I was expecting Javi to ack it.

I did, but it looks like my replies never made it to the mailing
list.  Anyway, here it is:

Acked-by: Javi Merino 


signature.asc
Description: PGP signature

Re: [PATCH] thermal: cpu_cooling: Actually trace CPU load in thermal_power_cpu_get_power

[Javi Merino]

On Thu, May 02, 2019 at 11:32:38AM -0700, Matthias Kaehlcke wrote:
> The CPU load values passed to the thermal_power_cpu_get_power
> tracepoint are zero for all CPUs, unless, unless the
> thermal_power_cpu_limit tracepoint is enabled too:
> 
>   irq/41-rockchip-98[000]    290.972410: thermal_power_cpu_get_power:
>   cpus=000f freq=180 load={{0x0,0x0,0x0,0x0}} dynamic_power=4815
> 
> vs
> 
>   irq/41-rockchip-96[000] 95.773585: thermal_power_cpu_get_power:
>   cpus=000f freq=180 load={{0x56,0x64,0x64,0x5e}} dynamic_power=4959
>   irq/41-rockchip-96[000] 95.773596: thermal_power_cpu_limit:
>   cpus=000f freq=408000 cdev_state=10 power=416
> 
> There seems to be no good reason for omitting the CPU load information
> depending on another tracepoint. My guess is that the intention was to
> check whether thermal_power_cpu_get_power is (still) enabled, however
> 'load_cpu != NULL' already indicates that it was at least enabled when
> cpufreq_get_requested_power() was entered, there seems little gain
> from omitting the assignment if the tracepoint was just disabled, so
> just remove the check.
> 
> Fixes: 6828a4711f99 ("thermal: add trace events to the power allocator 
> governor")
> Signed-off-by: Matthias Kaehlcke 

Yep, looks good to me.

Acked-by: Javi Merino 

> ---
>  drivers/thermal/cpu_cooling.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index f7c1f49ec87f..b437804e099b 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -458,7 +458,7 @@ static int cpufreq_get_requested_power(struct 
> thermal_cooling_device *cdev,
>   load = 0;
>  
>   total_load += load;
> - if (trace_thermal_power_cpu_limit_enabled() && load_cpu)
> + if (load_cpu)
>   load_cpu[i] = load;
>  
>   i++;
> -- 
> 2.21.0.593.g511ec345e18-goog
> 

Re: [RFC PATCH 6/7] sched/fair: update cpu_capcity to reflect thermal pressure

[Javi Merino]

On Tue, Oct 09, 2018 at 12:25:01PM -0400, Thara Gopinath wrote:
> cpu_capacity relflects the maximum available capacity of a cpu. Thermal
> pressure on a cpu means this maximum available capacity is reduced. This
> patch reduces the average thermal pressure for a cpu from its maximum
> available capacity so that cpu_capacity reflects the actual
> available capacity.
> 
> Signed-off-by: Thara Gopinath 
> ---
>  kernel/sched/fair.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7deb1d0..8651e55 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7497,6 +7497,7 @@ static unsigned long scale_rt_capacity(int cpu)
>  
>   used = READ_ONCE(rq->avg_rt.util_avg);
>   used += READ_ONCE(rq->avg_dl.util_avg);
> + used += READ_ONCE(rq->avg_thermal.load_avg);

IIUIC, you are treating thermal pressure as an artificial load on the
cpu.  If so, this sounds like a hard to maintain hack.  Thermal
pressure have different characteristics to utilization.  What happens
if thermal sets the cpu cooling state back to 0 because there is
thermal headroom again?  Do we keep adding this artificial load to the
cpu just because there was thermal pressure in the past and let it
decay as if it was cpu load?

Cheers,
Javi

Re: [RFC PATCH 6/7] sched/fair: update cpu_capcity to reflect thermal pressure

[Javi Merino]

On Tue, Oct 09, 2018 at 12:25:01PM -0400, Thara Gopinath wrote:
> cpu_capacity relflects the maximum available capacity of a cpu. Thermal
> pressure on a cpu means this maximum available capacity is reduced. This
> patch reduces the average thermal pressure for a cpu from its maximum
> available capacity so that cpu_capacity reflects the actual
> available capacity.
> 
> Signed-off-by: Thara Gopinath 
> ---
>  kernel/sched/fair.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7deb1d0..8651e55 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7497,6 +7497,7 @@ static unsigned long scale_rt_capacity(int cpu)
>  
>   used = READ_ONCE(rq->avg_rt.util_avg);
>   used += READ_ONCE(rq->avg_dl.util_avg);
> + used += READ_ONCE(rq->avg_thermal.load_avg);

IIUIC, you are treating thermal pressure as an artificial load on the
cpu.  If so, this sounds like a hard to maintain hack.  Thermal
pressure have different characteristics to utilization.  What happens
if thermal sets the cpu cooling state back to 0 because there is
thermal headroom again?  Do we keep adding this artificial load to the
cpu just because there was thermal pressure in the past and let it
decay as if it was cpu load?

Cheers,
Javi

Re: [RFC PATCH 0/7] Introduce thermal pressure

[Javi Merino]

On Tue, Oct 09, 2018 at 12:24:55PM -0400, Thara Gopinath wrote:
> Thermal governors can respond to an overheat event for a cpu by
> capping the cpu's maximum possible frequency. This in turn
> means that the maximum available compute capacity of the
> cpu is restricted. But today in linux kernel, in event of maximum
> frequency capping of a cpu, the maximum available compute
> capacity of the cpu is not adjusted at all. In other words, scheduler
> is unware maximum cpu capacity restrictions placed due to thermal
> activity.

Interesting, I would have sworn that I tested this years ago by
lowering the maximum frequency of a cpufreq domain, and the scheduler
reacted accordingly to the new maximum capacities of the cpus.

>   This patch series attempts to address this issue.
> The benefits identified are better task placement among available
> cpus in event of overheating which in turn leads to better
> performance numbers.
> 
> The delta between the maximum possible capacity of a cpu and
> maximum available capacity of a cpu due to thermal event can
> be considered as thermal pressure. Instantaneous thermal pressure
> is hard to record and can sometime be erroneous as there can be mismatch
> between the actual capping of capacity and scheduler recording it.
> Thus solution is to have a weighted average per cpu value for thermal
> pressure over time. The weight reflects the amount of time the cpu has
> spent at a capped maximum frequency. To accumulate, average and
> appropriately decay thermal pressure, this patch series uses pelt
> signals and reuses the available framework that does a similar
> bookkeeping of rt/dl task utilization.
> 
> Regarding testing, basic build, boot and sanity testing have been
> performed on hikey960 mainline kernel with debian file system.
> Further aobench (An occlusion renderer for benchmarking realworld
> floating point performance) showed the following results on hikey960
> with debain.
> 
> Result  Standard
> Standard
> (Time secs) Error   
> Deviation
> Hikey 960 - no thermal pressure applied 138.67  6.5211.52%
> Hikey 960 -  thermal pressure applied   122.37  5.7811.57%
> 
> Thara Gopinath (7):
>   sched/pelt: Add option to make load and util calculations frequency
> invariant
>   sched/pelt.c: Add support to track thermal pressure
>   sched: Add infrastructure to store and update instantaneous thermal
> pressure
>   sched: Initialize per cpu thermal pressure structure
>   sched/fair: Enable CFS periodic tick to update thermal pressure
>   sched/fair: update cpu_capcity to reflect thermal pressure
>   thermal/cpu-cooling: Update thermal pressure in case of a maximum
> frequency capping
> 
>  drivers/base/arch_topology.c  |  1 +
>  drivers/thermal/cpu_cooling.c | 20 -

thermal?  There are other ways in which the maximum frequency of a cpu
can be limited, for example from userspace via scaling_max_freq.

When something (anything) changes the maximum frequency of a cpufreq
policy, the scheduler should be notified.  I think this change should
be done in cpufreq instead to make it generic and not particular to
a given maximum frequency "capper".

Cheers,
Javi

Re: [RFC PATCH 0/7] Introduce thermal pressure

[Javi Merino]

On Tue, Oct 09, 2018 at 12:24:55PM -0400, Thara Gopinath wrote:
> Thermal governors can respond to an overheat event for a cpu by
> capping the cpu's maximum possible frequency. This in turn
> means that the maximum available compute capacity of the
> cpu is restricted. But today in linux kernel, in event of maximum
> frequency capping of a cpu, the maximum available compute
> capacity of the cpu is not adjusted at all. In other words, scheduler
> is unware maximum cpu capacity restrictions placed due to thermal
> activity.

Interesting, I would have sworn that I tested this years ago by
lowering the maximum frequency of a cpufreq domain, and the scheduler
reacted accordingly to the new maximum capacities of the cpus.

>   This patch series attempts to address this issue.
> The benefits identified are better task placement among available
> cpus in event of overheating which in turn leads to better
> performance numbers.
> 
> The delta between the maximum possible capacity of a cpu and
> maximum available capacity of a cpu due to thermal event can
> be considered as thermal pressure. Instantaneous thermal pressure
> is hard to record and can sometime be erroneous as there can be mismatch
> between the actual capping of capacity and scheduler recording it.
> Thus solution is to have a weighted average per cpu value for thermal
> pressure over time. The weight reflects the amount of time the cpu has
> spent at a capped maximum frequency. To accumulate, average and
> appropriately decay thermal pressure, this patch series uses pelt
> signals and reuses the available framework that does a similar
> bookkeeping of rt/dl task utilization.
> 
> Regarding testing, basic build, boot and sanity testing have been
> performed on hikey960 mainline kernel with debian file system.
> Further aobench (An occlusion renderer for benchmarking realworld
> floating point performance) showed the following results on hikey960
> with debain.
> 
> Result  Standard
> Standard
> (Time secs) Error   
> Deviation
> Hikey 960 - no thermal pressure applied 138.67  6.5211.52%
> Hikey 960 -  thermal pressure applied   122.37  5.7811.57%
> 
> Thara Gopinath (7):
>   sched/pelt: Add option to make load and util calculations frequency
> invariant
>   sched/pelt.c: Add support to track thermal pressure
>   sched: Add infrastructure to store and update instantaneous thermal
> pressure
>   sched: Initialize per cpu thermal pressure structure
>   sched/fair: Enable CFS periodic tick to update thermal pressure
>   sched/fair: update cpu_capcity to reflect thermal pressure
>   thermal/cpu-cooling: Update thermal pressure in case of a maximum
> frequency capping
> 
>  drivers/base/arch_topology.c  |  1 +
>  drivers/thermal/cpu_cooling.c | 20 -

thermal?  There are other ways in which the maximum frequency of a cpu
can be limited, for example from userspace via scaling_max_freq.

When something (anything) changes the maximum frequency of a cpufreq
policy, the scheduler should be notified.  I think this change should
be done in cpufreq instead to make it generic and not particular to
a given maximum frequency "capper".

Cheers,
Javi

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Fri, Jun 08, 2018 at 04:47:39PM +0100, Quentin Perret wrote:
> Hi Javi,
> 
> On Friday 08 Jun 2018 at 14:39:42 (+0100), Javi Merino wrote:
> > On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> > > On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > > > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > > > This brings me to another question. Let's say there are multiple 
> > > > > > > users of
> > > > > > > the Energy Model in the system. Shouldn't the units of frequency 
> > > > > > > and power
> > > > > > > not standardized, maybe Mhz and mW?
> > > > > > > The task scheduler doesn't care since it is only interested in 
> > > > > > > power diffs
> > > > > > > but other user might do.
> > > > > > 
> > > > > > So the good thing about specifying units is that we can probably 
> > > > > > assume
> > > > > > ranges on the values. If the power is in mW, assuming that we're 
> > > > > > talking
> > > > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should 
> > > > > > be
> > > > > > a reasonable upper-bound ?
> > > > > > But there are also vendors who might not be happy with disclosing 
> > > > > > absolute
> > > > > > values ... These are sometimes considered sensitive and only 
> > > > > > relative
> > > > > > numbers are discussed publicly. Now, you can also argue that we 
> > > > > > already
> > > > > > have units specified in IPA for ex, and that it doesn't really 
> > > > > > matter if
> > > > > > a driver "lies" about the real value, as long as the ratios are 
> > > > > > correct.
> > > > > > And I guess that anyone can do measurement on the hardware and get 
> > > > > > those
> > > > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > > > good idea.
> > > > > 
> > > > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > > > binding accepted, and one of the musts was that the values were going 
> > > > > to
> > > > > be normalized. So, normalized power values again maybe?
> > > > 
> > > > Hmmm, that's a very good point ... There should be no problems on the
> > > > scheduler side -- we're only interested in correct ratios. But I'm not
> > > > sure on the thermal side ... I will double check that.
> > > 
> > > So, IPA needs to compare the power of the CPUs with the power of other
> > > things (e.g. GPUs). So we can't normalize the power of the CPUs without
> > > normalizing in the same scale the power of the other devices. I see two
> > > possibilities:
> > > 
> > > 1) we don't normalize the CPU power values, we specify them in mW, and
> > >we document (and maybe throw a warning if we see an issue at runtime)
> > >the max range of values. The max expected power for a single core
> > >could be 65K for ex (16bits). And based on that we can verify
> > >overflow and precision issues in the algorithms, and we keep it easy
> > >to compare the CPU power numbers with other devices.
> > > 
> > > 2) we normalize the power values, but that means that the EM framework
> > >has to manage not only CPUs, but also other types of devices, and
> > >normalized their power values as well. That's required to keep the
> > >scale consistent across all of them, and keep comparisons doable.
> > >But if we do this, we still have to keep a normalized and a "raw"
> > >version of the power for all devices. And the "raw" power must still
> > >be in the same unit across all devices, otherwise the re-scaling is
> > >broken. The main benefit of doing this is that the range of
> > >acceptable "raw" power values can be larger, probably 32bits, and
> > >that the precision of the normalized range is arbitrary.
> > > 
> > > I feel like 2) involves a lot of complexity, and not so many benefits,
> > > so I'd be happy to go with 1). Unless I forgot something ?
> > 
> > From the thermal point of view, the power values don't need to have
> > any given unit, as long as the values are comparable to each other.
> 
> OK, thanks for confirming that :-)
> 
> > Do we need to normalize anything in the kernel though?  Can't we just
> > assume that whatever the platform is telling us is correct?  Quentin
> > mentioned it earlier: sometimes absolute values are considered
> > sensitive and we only get ones that are correct relative to the rest
> > of the system.
> 
> I'm happy to specify the units as mW and let the drivers lie about the
> true values. At least that helps them lie coherently if another
> subsystem requires power in uW for example.

I think this is a good option.

Cheers,
Javi

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Fri, Jun 08, 2018 at 04:47:39PM +0100, Quentin Perret wrote:
> Hi Javi,
> 
> On Friday 08 Jun 2018 at 14:39:42 (+0100), Javi Merino wrote:
> > On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> > > On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > > > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > > > This brings me to another question. Let's say there are multiple 
> > > > > > > users of
> > > > > > > the Energy Model in the system. Shouldn't the units of frequency 
> > > > > > > and power
> > > > > > > not standardized, maybe Mhz and mW?
> > > > > > > The task scheduler doesn't care since it is only interested in 
> > > > > > > power diffs
> > > > > > > but other user might do.
> > > > > > 
> > > > > > So the good thing about specifying units is that we can probably 
> > > > > > assume
> > > > > > ranges on the values. If the power is in mW, assuming that we're 
> > > > > > talking
> > > > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should 
> > > > > > be
> > > > > > a reasonable upper-bound ?
> > > > > > But there are also vendors who might not be happy with disclosing 
> > > > > > absolute
> > > > > > values ... These are sometimes considered sensitive and only 
> > > > > > relative
> > > > > > numbers are discussed publicly. Now, you can also argue that we 
> > > > > > already
> > > > > > have units specified in IPA for ex, and that it doesn't really 
> > > > > > matter if
> > > > > > a driver "lies" about the real value, as long as the ratios are 
> > > > > > correct.
> > > > > > And I guess that anyone can do measurement on the hardware and get 
> > > > > > those
> > > > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > > > good idea.
> > > > > 
> > > > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > > > binding accepted, and one of the musts was that the values were going 
> > > > > to
> > > > > be normalized. So, normalized power values again maybe?
> > > > 
> > > > Hmmm, that's a very good point ... There should be no problems on the
> > > > scheduler side -- we're only interested in correct ratios. But I'm not
> > > > sure on the thermal side ... I will double check that.
> > > 
> > > So, IPA needs to compare the power of the CPUs with the power of other
> > > things (e.g. GPUs). So we can't normalize the power of the CPUs without
> > > normalizing in the same scale the power of the other devices. I see two
> > > possibilities:
> > > 
> > > 1) we don't normalize the CPU power values, we specify them in mW, and
> > >we document (and maybe throw a warning if we see an issue at runtime)
> > >the max range of values. The max expected power for a single core
> > >could be 65K for ex (16bits). And based on that we can verify
> > >overflow and precision issues in the algorithms, and we keep it easy
> > >to compare the CPU power numbers with other devices.
> > > 
> > > 2) we normalize the power values, but that means that the EM framework
> > >has to manage not only CPUs, but also other types of devices, and
> > >normalized their power values as well. That's required to keep the
> > >scale consistent across all of them, and keep comparisons doable.
> > >But if we do this, we still have to keep a normalized and a "raw"
> > >version of the power for all devices. And the "raw" power must still
> > >be in the same unit across all devices, otherwise the re-scaling is
> > >broken. The main benefit of doing this is that the range of
> > >acceptable "raw" power values can be larger, probably 32bits, and
> > >that the precision of the normalized range is arbitrary.
> > > 
> > > I feel like 2) involves a lot of complexity, and not so many benefits,
> > > so I'd be happy to go with 1). Unless I forgot something ?
> > 
> > From the thermal point of view, the power values don't need to have
> > any given unit, as long as the values are comparable to each other.
> 
> OK, thanks for confirming that :-)
> 
> > Do we need to normalize anything in the kernel though?  Can't we just
> > assume that whatever the platform is telling us is correct?  Quentin
> > mentioned it earlier: sometimes absolute values are considered
> > sensitive and we only get ones that are correct relative to the rest
> > of the system.
> 
> I'm happy to specify the units as mW and let the drivers lie about the
> true values. At least that helps them lie coherently if another
> subsystem requires power in uW for example.

I think this is a good option.

Cheers,
Javi

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Thu, Jun 07, 2018 at 06:04:19PM +0200, Juri Lelli wrote:
> On 07/06/18 16:19, Quentin Perret wrote:
> > Hi Juri,
> > 
> > On Thursday 07 Jun 2018 at 16:44:09 (+0200), Juri Lelli wrote:
> > > On 21/05/18 15:24, Quentin Perret wrote:
> 
> [...]
> 
> > > > +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> > > > +{
> > > > +   unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> > > > +   int max_cap_state = cs_table->nr_cap_states - 1;
> > >  ^
> > > You don't need this on the stack, right?
> > 
> > Oh, why not ?
> > 
> 
> Because you use it only once here below? Anyway, more a (debatable)
> nitpick than anything.

The compiler optimizes that for you because it knows that it is used
only once.  It doesn't put it in the stack, it uses a register.  As it
is, it's more readable so I'd rather keep it.

For reference, this is the code gcc 7.3 generates for arm64 for
fd_update_cstable() (which is inlined in em_rescale_cpu_capacity():

x27 holds the address to cs_table

 1ac:   b9400b63ldr w3, [x27, #8]   ; w3 = cs_table->nr_cap_states
 1b0:   b9406fa4ldr w4, [x29, #108] ; w4 = 0x18 (sizeof(struct 
em_cap_state))
 1b4:   f9400362ldr x2, [x27]   ; x2 = _table[state]
 1b8:   51000461sub w1, w3, #0x1; w1 max_cap_state = 
cs_table->nr_cap_states - 1
[...]
 1cc:   9b240821smaddl  x1, w1, w4, x2  ; x1 = 
_table->state[max_cap_state]
[...]
 1d4:   f9400427ldr x7, [x1, #8]; x7 fmax = 
cs_table->state[max_cap_state].frequency
[...]   ; calculates cmax * 
cs_table->state[i].frequency in x0
 200:   9ac70800udivx0, x0, x7  ; x0 = x0 / fmax
; x0 is then stored to 
cs_table->state[i].capacity

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Thu, Jun 07, 2018 at 06:04:19PM +0200, Juri Lelli wrote:
> On 07/06/18 16:19, Quentin Perret wrote:
> > Hi Juri,
> > 
> > On Thursday 07 Jun 2018 at 16:44:09 (+0200), Juri Lelli wrote:
> > > On 21/05/18 15:24, Quentin Perret wrote:
> 
> [...]
> 
> > > > +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> > > > +{
> > > > +   unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> > > > +   int max_cap_state = cs_table->nr_cap_states - 1;
> > >  ^
> > > You don't need this on the stack, right?
> > 
> > Oh, why not ?
> > 
> 
> Because you use it only once here below? Anyway, more a (debatable)
> nitpick than anything.

The compiler optimizes that for you because it knows that it is used
only once.  It doesn't put it in the stack, it uses a register.  As it
is, it's more readable so I'd rather keep it.

For reference, this is the code gcc 7.3 generates for arm64 for
fd_update_cstable() (which is inlined in em_rescale_cpu_capacity():

x27 holds the address to cs_table

 1ac:   b9400b63ldr w3, [x27, #8]   ; w3 = cs_table->nr_cap_states
 1b0:   b9406fa4ldr w4, [x29, #108] ; w4 = 0x18 (sizeof(struct 
em_cap_state))
 1b4:   f9400362ldr x2, [x27]   ; x2 = _table[state]
 1b8:   51000461sub w1, w3, #0x1; w1 max_cap_state = 
cs_table->nr_cap_states - 1
[...]
 1cc:   9b240821smaddl  x1, w1, w4, x2  ; x1 = 
_table->state[max_cap_state]
[...]
 1d4:   f9400427ldr x7, [x1, #8]; x7 fmax = 
cs_table->state[max_cap_state].frequency
[...]   ; calculates cmax * 
cs_table->state[i].frequency in x0
 200:   9ac70800udivx0, x0, x7  ; x0 = x0 / fmax
; x0 is then stored to 
cs_table->state[i].capacity

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > This brings me to another question. Let's say there are multiple 
> > > > > users of
> > > > > the Energy Model in the system. Shouldn't the units of frequency and 
> > > > > power
> > > > > not standardized, maybe Mhz and mW?
> > > > > The task scheduler doesn't care since it is only interested in power 
> > > > > diffs
> > > > > but other user might do.
> > > > 
> > > > So the good thing about specifying units is that we can probably assume
> > > > ranges on the values. If the power is in mW, assuming that we're talking
> > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > > > a reasonable upper-bound ?
> > > > But there are also vendors who might not be happy with disclosing 
> > > > absolute
> > > > values ... These are sometimes considered sensitive and only relative
> > > > numbers are discussed publicly. Now, you can also argue that we already
> > > > have units specified in IPA for ex, and that it doesn't really matter if
> > > > a driver "lies" about the real value, as long as the ratios are correct.
> > > > And I guess that anyone can do measurement on the hardware and get those
> > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > good idea.
> > > 
> > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > binding accepted, and one of the musts was that the values were going to
> > > be normalized. So, normalized power values again maybe?
> > 
> > Hmmm, that's a very good point ... There should be no problems on the
> > scheduler side -- we're only interested in correct ratios. But I'm not
> > sure on the thermal side ... I will double check that.
> 
> So, IPA needs to compare the power of the CPUs with the power of other
> things (e.g. GPUs). So we can't normalize the power of the CPUs without
> normalizing in the same scale the power of the other devices. I see two
> possibilities:
> 
> 1) we don't normalize the CPU power values, we specify them in mW, and
>we document (and maybe throw a warning if we see an issue at runtime)
>the max range of values. The max expected power for a single core
>could be 65K for ex (16bits). And based on that we can verify
>overflow and precision issues in the algorithms, and we keep it easy
>to compare the CPU power numbers with other devices.
> 
> 2) we normalize the power values, but that means that the EM framework
>has to manage not only CPUs, but also other types of devices, and
>normalized their power values as well. That's required to keep the
>scale consistent across all of them, and keep comparisons doable.
>But if we do this, we still have to keep a normalized and a "raw"
>version of the power for all devices. And the "raw" power must still
>be in the same unit across all devices, otherwise the re-scaling is
>broken. The main benefit of doing this is that the range of
>acceptable "raw" power values can be larger, probably 32bits, and
>that the precision of the normalized range is arbitrary.
> 
> I feel like 2) involves a lot of complexity, and not so many benefits,
> so I'd be happy to go with 1). Unless I forgot something ?

>From the thermal point of view, the power values don't need to have
any given unit, as long as the values are comparable to each other.
Do we need to normalize anything in the kernel though?  Can't we just
assume that whatever the platform is telling us is correct?  Quentin
mentioned it earlier: sometimes absolute values are considered
sensitive and we only get ones that are correct relative to the rest
of the system.

(This reminds me that the units in
Documentation/thermal/cpu-cooling-api.txt are wrong and I need to fix
it :-X )

Cheers,
Javi

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > This brings me to another question. Let's say there are multiple 
> > > > > users of
> > > > > the Energy Model in the system. Shouldn't the units of frequency and 
> > > > > power
> > > > > not standardized, maybe Mhz and mW?
> > > > > The task scheduler doesn't care since it is only interested in power 
> > > > > diffs
> > > > > but other user might do.
> > > > 
> > > > So the good thing about specifying units is that we can probably assume
> > > > ranges on the values. If the power is in mW, assuming that we're talking
> > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > > > a reasonable upper-bound ?
> > > > But there are also vendors who might not be happy with disclosing 
> > > > absolute
> > > > values ... These are sometimes considered sensitive and only relative
> > > > numbers are discussed publicly. Now, you can also argue that we already
> > > > have units specified in IPA for ex, and that it doesn't really matter if
> > > > a driver "lies" about the real value, as long as the ratios are correct.
> > > > And I guess that anyone can do measurement on the hardware and get those
> > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > good idea.
> > > 
> > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > binding accepted, and one of the musts was that the values were going to
> > > be normalized. So, normalized power values again maybe?
> > 
> > Hmmm, that's a very good point ... There should be no problems on the
> > scheduler side -- we're only interested in correct ratios. But I'm not
> > sure on the thermal side ... I will double check that.
> 
> So, IPA needs to compare the power of the CPUs with the power of other
> things (e.g. GPUs). So we can't normalize the power of the CPUs without
> normalizing in the same scale the power of the other devices. I see two
> possibilities:
> 
> 1) we don't normalize the CPU power values, we specify them in mW, and
>we document (and maybe throw a warning if we see an issue at runtime)
>the max range of values. The max expected power for a single core
>could be 65K for ex (16bits). And based on that we can verify
>overflow and precision issues in the algorithms, and we keep it easy
>to compare the CPU power numbers with other devices.
> 
> 2) we normalize the power values, but that means that the EM framework
>has to manage not only CPUs, but also other types of devices, and
>normalized their power values as well. That's required to keep the
>scale consistent across all of them, and keep comparisons doable.
>But if we do this, we still have to keep a normalized and a "raw"
>version of the power for all devices. And the "raw" power must still
>be in the same unit across all devices, otherwise the re-scaling is
>broken. The main benefit of doing this is that the range of
>acceptable "raw" power values can be larger, probably 32bits, and
>that the precision of the normalized range is arbitrary.
> 
> I feel like 2) involves a lot of complexity, and not so many benefits,
> so I'd be happy to go with 1). Unless I forgot something ?

>From the thermal point of view, the power values don't need to have
any given unit, as long as the values are comparable to each other.
Do we need to normalize anything in the kernel though?  Can't we just
assume that whatever the platform is telling us is correct?  Quentin
mentioned it earlier: sometimes absolute values are considered
sensitive and we only get ones that are correct relative to the rest
of the system.

(This reminds me that the units in
Documentation/thermal/cpu-cooling-api.txt are wrong and I need to fix
it :-X )

Cheers,
Javi

Re: [PATCH V2 4/4] cpu_cooling: Drop static-power related stuff

[Javi Merino]

On Tue, Dec 05, 2017 at 11:02:46AM +0530, Viresh Kumar wrote:
> No one has used it for the last two and half years (since it was
> introduced by commit c36cf0717631 ("thermal: cpu_cooling: implement the
> power cooling device API")), get rid of it.

That commit c36cf0717631 ("thermal: cpu_cooling: implement the power
cooling device API") also added documentation for it.  You should
remove the documentation, to keep it in sync.

> Cc: Javi Merino <javi.mer...@arm.com>
> Cc: Punit Agrawal <punit.agra...@arm.com>
> Acked-by: Eduardo Valentin <edubez...@gmail.com>
> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c  | 106 
> +
>  include/linux/cpu_cooling.h|   3 --
>  include/trace/events/thermal.h |  10 ++--
>  3 files changed, 16 insertions(+), 103 deletions(-)

Re: [PATCH V2 4/4] cpu_cooling: Drop static-power related stuff

[Javi Merino]

On Tue, Dec 05, 2017 at 11:02:46AM +0530, Viresh Kumar wrote:
> No one has used it for the last two and half years (since it was
> introduced by commit c36cf0717631 ("thermal: cpu_cooling: implement the
> power cooling device API")), get rid of it.

That commit c36cf0717631 ("thermal: cpu_cooling: implement the power
cooling device API") also added documentation for it.  You should
remove the documentation, to keep it in sync.

> Cc: Javi Merino 
> Cc: Punit Agrawal 
> Acked-by: Eduardo Valentin 
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/thermal/cpu_cooling.c  | 106 
> +
>  include/linux/cpu_cooling.h|   3 --
>  include/trace/events/thermal.h |  10 ++--
>  3 files changed, 16 insertions(+), 103 deletions(-)

Re: [PATCH 4/4] cpu_cooling: Drop static-power related stuff

[Javi Merino]

Hi,

On Tue, Nov 21, 2017 at 08:57:06AM -0800, Eduardo Valentin wrote:

[snip]

> As I said before, the minimal you guys (ARM and Linaro) can do is to at
> least upstream the Juno code! as a reference. Come on guys?  what is
> preventing you to upstream Juno model?

As Ionela pointed out earlier in the thread, the cpufreq driver for Juno
was not acceptable for mainline because it used platform specific code.
When it was converted to cpufreq-dt, the static power was left behind
because it can't be represented in device tree.  This is because there
isn't a function that works for every SoC, different process nodes
(among other things) will need different functions.  So it can't be just
a bunch of coefficients in DT, we need a function.  Hence the callback.

In a nutshell, mainline does not want platform specific code, but we
haven't figured out how to calculate static power without platform
specific code.

Cheers,
Javi

Re: [PATCH 4/4] cpu_cooling: Drop static-power related stuff

[Javi Merino]

Hi,

On Tue, Nov 21, 2017 at 08:57:06AM -0800, Eduardo Valentin wrote:

[snip]

> As I said before, the minimal you guys (ARM and Linaro) can do is to at
> least upstream the Juno code! as a reference. Come on guys?  what is
> preventing you to upstream Juno model?

As Ionela pointed out earlier in the thread, the cpufreq driver for Juno
was not acceptable for mainline because it used platform specific code.
When it was converted to cpufreq-dt, the static power was left behind
because it can't be represented in device tree.  This is because there
isn't a function that works for every SoC, different process nodes
(among other things) will need different functions.  So it can't be just
a bunch of coefficients in DT, we need a function.  Hence the callback.

In a nutshell, mainline does not want platform specific code, but we
haven't figured out how to calculate static power without platform
specific code.

Cheers,
Javi

Re: [PATCH 4/4] cpu_cooling: Drop static-power related stuff

[Javi Merino]

On Wed, Nov 15, 2017 at 02:49:48PM +0530, Viresh Kumar wrote:
> No one has used it for the last two and half years (since it was
> introduced by commit c36cf0717631 ("thermal: cpu_cooling: implement the
> power cooling device API")), get rid of it.

Linaro used it in lsk 3.18 for the cpufreq driver for Juno.  The cpufreq
driver was converted to the generic one from dt in lsk 4.4, but the
generic cpufreq driver does not support static power because everything
has to come from device tree and we don't have a way to specify it there.

Cheers,
Javi

Re: [PATCH 4/4] cpu_cooling: Drop static-power related stuff

[Javi Merino]

On Wed, Nov 15, 2017 at 02:49:48PM +0530, Viresh Kumar wrote:
> No one has used it for the last two and half years (since it was
> introduced by commit c36cf0717631 ("thermal: cpu_cooling: implement the
> power cooling device API")), get rid of it.

Linaro used it in lsk 3.18 for the cpufreq driver for Juno.  The cpufreq
driver was converted to the generic one from dt in lsk 4.4, but the
generic cpufreq driver does not support static power because everything
has to come from device tree and we don't have a way to specify it there.

Cheers,
Javi

Re: [PATCH] thermal: cpu_cooling: pr_err() strings should end with newlines

[Javi Merino]

On Tue, Oct 24, 2017 at 01:20:39PM +0530, Arvind Yadav wrote:
> pr_err() messages should end with a new-line to avoid other messages
> being concatenated.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>

FWIW,

Acked-by: Javi Merino <javi.mer...@kernel.org>

> ---
>  drivers/thermal/cpu_cooling.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 908a801..dc63aba 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -696,7 +696,7 @@ static unsigned int find_next_max(struct 
> cpufreq_frequency_table *table,
>   bool first;
>  
>   if (IS_ERR_OR_NULL(policy)) {
> - pr_err("%s: cpufreq policy isn't valid: %p", __func__, policy);
> + pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, 
> policy);
>   return ERR_PTR(-EINVAL);
>   }
>  
> -- 
> 1.9.1
> 

Re: [PATCH] thermal: cpu_cooling: pr_err() strings should end with newlines

[Javi Merino]

On Tue, Oct 24, 2017 at 01:20:39PM +0530, Arvind Yadav wrote:
> pr_err() messages should end with a new-line to avoid other messages
> being concatenated.
> 
> Signed-off-by: Arvind Yadav 

FWIW,

Acked-by: Javi Merino 

> ---
>  drivers/thermal/cpu_cooling.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 908a801..dc63aba 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -696,7 +696,7 @@ static unsigned int find_next_max(struct 
> cpufreq_frequency_table *table,
>   bool first;
>  
>   if (IS_ERR_OR_NULL(policy)) {
> - pr_err("%s: cpufreq policy isn't valid: %p", __func__, policy);
> + pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, 
> policy);
>   return ERR_PTR(-EINVAL);
>   }
>  
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] tracing, thermal: Hide cpu cooling trace events when not in use

[Javi Merino]

On Fri, Oct 13, 2017 at 10:23:09AM -0400, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rost...@goodmis.org>
> 
> As trace events when defined create data structures and functions to
> process them, defining trace events when not using them is a waste of
> memory.
> 
> The trace events thermal_power_cpu_get_power and
> thermal_power_cpu_limit are only used when CONFIG_CPU_THERMAL is set.
> Make those events only defined when that is set as well.
> 
> Signed-off-by: Steven Rostedt (VMware) <rost...@goodmis.org>

Acked-by: Javi Merino <javi.mer...@kernel.org>

> ---
> Index: linux-trace.git/include/trace/events/thermal.h
> ===
> --- linux-trace.git.orig/include/trace/events/thermal.h
> +++ linux-trace.git/include/trace/events/thermal.h
> @@ -90,6 +90,7 @@ TRACE_EVENT(thermal_zone_trip,
>   show_tzt_type(__entry->trip_type))
>  );
>  
> +#ifdef CONFIG_CPU_THERMAL
>  TRACE_EVENT(thermal_power_cpu_get_power,
>   TP_PROTO(const struct cpumask *cpus, unsigned long freq, u32 *load,
>   size_t load_len, u32 dynamic_power, u32 static_power),
> @@ -147,6 +148,7 @@ TRACE_EVENT(thermal_power_cpu_limit,
>   __get_bitmask(cpumask), __entry->freq, __entry->cdev_state,
>   __entry->power)
>  );
> +#endif /* CONFIG_CPU_THERMAL */
>  
>  #ifdef CONFIG_DEVFREQ_THERMAL
>  TRACE_EVENT(thermal_power_devfreq_get_power,

Re: [PATCH 2/2] tracing, thermal: Hide cpu cooling trace events when not in use

[Javi Merino]

On Fri, Oct 13, 2017 at 10:23:09AM -0400, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) 
> 
> As trace events when defined create data structures and functions to
> process them, defining trace events when not using them is a waste of
> memory.
> 
> The trace events thermal_power_cpu_get_power and
> thermal_power_cpu_limit are only used when CONFIG_CPU_THERMAL is set.
> Make those events only defined when that is set as well.
> 
> Signed-off-by: Steven Rostedt (VMware) 

Acked-by: Javi Merino 

> ---
> Index: linux-trace.git/include/trace/events/thermal.h
> ===
> --- linux-trace.git.orig/include/trace/events/thermal.h
> +++ linux-trace.git/include/trace/events/thermal.h
> @@ -90,6 +90,7 @@ TRACE_EVENT(thermal_zone_trip,
>   show_tzt_type(__entry->trip_type))
>  );
>  
> +#ifdef CONFIG_CPU_THERMAL
>  TRACE_EVENT(thermal_power_cpu_get_power,
>   TP_PROTO(const struct cpumask *cpus, unsigned long freq, u32 *load,
>   size_t load_len, u32 dynamic_power, u32 static_power),
> @@ -147,6 +148,7 @@ TRACE_EVENT(thermal_power_cpu_limit,
>   __get_bitmask(cpumask), __entry->freq, __entry->cdev_state,
>   __entry->power)
>  );
> +#endif /* CONFIG_CPU_THERMAL */
>  
>  #ifdef CONFIG_DEVFREQ_THERMAL
>  TRACE_EVENT(thermal_power_devfreq_get_power,

Re: [PATCH 1/2] tracing, thermal: Hide devfreq trace events when not in use

[Javi Merino]

On Fri, Oct 13, 2017 at 10:21:50AM -0400, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rost...@goodmis.org>
> 
> As trace events when defined create data structures and functions to
> process them, defining trace events when not using them is a waste of
> memory.
> 
> The trace events thermal_power_devfreq_get_power and
> thermal_power_devfreq_limit are only used when CONFIG_DEVFREQ_THERMAL
> is set. Make those events only defined when that is set as well.
> 
> Signed-off-by: Steven Rostedt (VMware) <rost...@goodmis.org>

Acked-by: Javi Merino <javi.mer...@kernel.org>

> ---
> Index: linux-trace.git/include/trace/events/thermal.h
> ===
> --- linux-trace.git.orig/include/trace/events/thermal.h
> +++ linux-trace.git/include/trace/events/thermal.h
> @@ -148,6 +148,7 @@ TRACE_EVENT(thermal_power_cpu_limit,
>   __entry->power)
>  );
>  
> +#ifdef CONFIG_DEVFREQ_THERMAL
>  TRACE_EVENT(thermal_power_devfreq_get_power,
>   TP_PROTO(struct thermal_cooling_device *cdev,
>struct devfreq_dev_status *status, unsigned long freq,
> @@ -203,6 +204,7 @@ TRACE_EVENT(thermal_power_devfreq_limit,
>   __get_str(type), __entry->freq, __entry->cdev_state,
>   __entry->power)
>  );
> +#endif /* CONFIG_DEVFREQ_THERMAL */
>  #endif /* _TRACE_THERMAL_H */
>  
>  /* This part must be outside protection */

Re: [PATCH 1/2] tracing, thermal: Hide devfreq trace events when not in use

[Javi Merino]

On Fri, Oct 13, 2017 at 10:21:50AM -0400, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) 
> 
> As trace events when defined create data structures and functions to
> process them, defining trace events when not using them is a waste of
> memory.
> 
> The trace events thermal_power_devfreq_get_power and
> thermal_power_devfreq_limit are only used when CONFIG_DEVFREQ_THERMAL
> is set. Make those events only defined when that is set as well.
> 
> Signed-off-by: Steven Rostedt (VMware) 

Acked-by: Javi Merino 

> ---
> Index: linux-trace.git/include/trace/events/thermal.h
> ===
> --- linux-trace.git.orig/include/trace/events/thermal.h
> +++ linux-trace.git/include/trace/events/thermal.h
> @@ -148,6 +148,7 @@ TRACE_EVENT(thermal_power_cpu_limit,
>   __entry->power)
>  );
>  
> +#ifdef CONFIG_DEVFREQ_THERMAL
>  TRACE_EVENT(thermal_power_devfreq_get_power,
>   TP_PROTO(struct thermal_cooling_device *cdev,
>struct devfreq_dev_status *status, unsigned long freq,
> @@ -203,6 +204,7 @@ TRACE_EVENT(thermal_power_devfreq_limit,
>   __get_str(type), __entry->freq, __entry->cdev_state,
>   __entry->power)
>  );
> +#endif /* CONFIG_DEVFREQ_THERMAL */
>  #endif /* _TRACE_THERMAL_H */
>  
>  /* This part must be outside protection */

[PATCH RESEND] drm: use .hword to represent 16-bit numbers

[Javi Merino]

The size of .word is the size of a word in the given platform, which
for intel systems is 16-bits but other architectures use different
sizes.  However, .hword emits 16-bit numbers regardless of the
platform (and despite the name).  The quantities specified in EDID are
platform independent, so they should work in spite of the default
target of the cc you are using, so use .hword where EDID specifies
16-bit numbers.

Cc: Carsten Emde <c.e...@osadl.org>
Cc: David Airlie <airl...@linux.ie>
Signed-off-by: Javi Merino <javi.mer...@kernel.org>
---
 Documentation/EDID/edid.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/EDID/edid.S b/Documentation/EDID/edid.S
index 7ac0327..ef082dc 100644
--- a/Documentation/EDID/edid.S
+++ b/Documentation/EDID/edid.S
@@ -59,9 +59,9 @@
 /* Fixed header pattern */
 header:.byte   0x00,0xff,0xff,0xff,0xff,0xff,0xff,0x00
 
-mfg_id:.word   swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
+mfg_id:.hword  swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
 
-prod_code: .word   0
+prod_code: .hword  0
 
 /* Serial number. 32 bits, little endian. */
 serial_number: .long   SERIAL
@@ -177,7 +177,7 @@ std_vres:   .byte   (XY_RATIO<<6)+VFREQ-60
 
 descriptor1:
 /* Pixel clock in 10 kHz units. (0.-655.35 MHz, little-endian) */
-clock: .word   CLOCK/10
+clock: .hword  CLOCK/10
 
 /* Horizontal active pixels 8 lsbits (0-4095) */
 x_act_lsb: .byte   XPIX&0xff
-- 
2.1.4

[PATCH RESEND] drm: use .hword to represent 16-bit numbers

[Javi Merino]

The size of .word is the size of a word in the given platform, which
for intel systems is 16-bits but other architectures use different
sizes.  However, .hword emits 16-bit numbers regardless of the
platform (and despite the name).  The quantities specified in EDID are
platform independent, so they should work in spite of the default
target of the cc you are using, so use .hword where EDID specifies
16-bit numbers.

Cc: Carsten Emde 
Cc: David Airlie 
Signed-off-by: Javi Merino 
---
 Documentation/EDID/edid.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/EDID/edid.S b/Documentation/EDID/edid.S
index 7ac0327..ef082dc 100644
--- a/Documentation/EDID/edid.S
+++ b/Documentation/EDID/edid.S
@@ -59,9 +59,9 @@
 /* Fixed header pattern */
 header:.byte   0x00,0xff,0xff,0xff,0xff,0xff,0xff,0x00
 
-mfg_id:.word   swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
+mfg_id:.hword  swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
 
-prod_code: .word   0
+prod_code: .hword  0
 
 /* Serial number. 32 bits, little endian. */
 serial_number: .long   SERIAL
@@ -177,7 +177,7 @@ std_vres:   .byte   (XY_RATIO<<6)+VFREQ-60
 
 descriptor1:
 /* Pixel clock in 10 kHz units. (0.-655.35 MHz, little-endian) */
-clock: .word   CLOCK/10
+clock: .hword  CLOCK/10
 
 /* Horizontal active pixels 8 lsbits (0-4095) */
 x_act_lsb: .byte   XPIX&0xff
-- 
2.1.4

Re: [PATCH 0/2] Documentation/EDID fixes

[Javi Merino]

On Tue, Mar 07, 2017 at 06:16:51PM +0200, Jani Nikula wrote:
> On Mon, 06 Mar 2017, Javi Merino <javi.mer...@kernel.org> wrote:
> > I found these two minor issues while building an EDID.  I'm not sure
> > whether the second patch (Add O= to support) is upstream material, but
> > I'm sending it just in case.
> 
> I'm not opposed to fixing existing issues like this, but really I think
> there should be an userspace tool for this. Definitely outside of the
> Documentation directory, perhaps even outside the kernel tree
> altogether.

I am not sure whether this should be in the kernel or not, but I
agree that Documentation/ doesn't look like the right place to build
these files.

Cheers,
Javi

Re: [PATCH 0/2] Documentation/EDID fixes

[Javi Merino]

On Tue, Mar 07, 2017 at 06:16:51PM +0200, Jani Nikula wrote:
> On Mon, 06 Mar 2017, Javi Merino  wrote:
> > I found these two minor issues while building an EDID.  I'm not sure
> > whether the second patch (Add O= to support) is upstream material, but
> > I'm sending it just in case.
> 
> I'm not opposed to fixing existing issues like this, but really I think
> there should be an userspace tool for this. Definitely outside of the
> Documentation directory, perhaps even outside the kernel tree
> altogether.

I am not sure whether this should be in the kernel or not, but I
agree that Documentation/ doesn't look like the right place to build
these files.

Cheers,
Javi

[PATCH 2/2] drm: Add O= support

[Javi Merino]

Add an option to put all output files in a given directory, similar to
what kbuild does.

Cc: Carsten Emde <c.e...@osadl.org>
Cc: David Airlie <airl...@linux.ie>
Signed-off-by: Javi Merino <javi.mer...@kernel.org>
---
 Documentation/EDID/Makefile | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/Documentation/EDID/Makefile b/Documentation/EDID/Makefile
index 17763ca..76e8ef5 100644
--- a/Documentation/EDID/Makefile
+++ b/Documentation/EDID/Makefile
@@ -1,26 +1,29 @@
 
+# use "make O=dir" to locate all output files in "dir"
+O ?= .
+
 SOURCES:= $(wildcard [0-9]*x[0-9]*.S)
 
-BIN:= $(patsubst %.S, %.bin, $(SOURCES))
+BIN:= $(patsubst %.S, $(O)/%.bin, $(SOURCES))
 
-IHEX   := $(patsubst %.S, %.bin.ihex, $(SOURCES))
+IHEX   := $(patsubst %.S, $(O)/%.bin.ihex, $(SOURCES))
 
-CODE   := $(patsubst %.S, %.c, $(SOURCES))
+CODE   := $(patsubst %.S, $(O)/%.c, $(SOURCES))
 
 all:   $(BIN) $(IHEX) $(CODE)
 
 clean:
-   @rm -f *.o *.bin.ihex *.bin *.c
+   @rm -f $(O)/*.o $(O)/*.bin.ihex $(O)/*.bin $(O)/*.c
 
-%.o:   %.S
-   @cc -c $^
+$(O)/%.o:  %.S
+   @cc -c $^ -o $@
 
-%.bin: %.o
+$(O)/%.bin:$(O)/%.o
@objcopy -Obinary $^ $@
 
-%.bin.ihex:%.o
+$(O)/%.bin.ihex:   $(O)/%.o
@objcopy -Oihex $^ $@
@dos2unix $@ 2>/dev/null
 
-%.c:   %.bin
+$(O)/%.c:  $(O)/%.bin
@echo "{" >$@; hexdump -f hex $^ >>$@; echo "};" >>$@
-- 
2.1.4

[PATCH 2/2] drm: Add O= support

[Javi Merino]

Add an option to put all output files in a given directory, similar to
what kbuild does.

Cc: Carsten Emde 
Cc: David Airlie 
Signed-off-by: Javi Merino 
---
 Documentation/EDID/Makefile | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/Documentation/EDID/Makefile b/Documentation/EDID/Makefile
index 17763ca..76e8ef5 100644
--- a/Documentation/EDID/Makefile
+++ b/Documentation/EDID/Makefile
@@ -1,26 +1,29 @@
 
+# use "make O=dir" to locate all output files in "dir"
+O ?= .
+
 SOURCES:= $(wildcard [0-9]*x[0-9]*.S)
 
-BIN:= $(patsubst %.S, %.bin, $(SOURCES))
+BIN:= $(patsubst %.S, $(O)/%.bin, $(SOURCES))
 
-IHEX   := $(patsubst %.S, %.bin.ihex, $(SOURCES))
+IHEX   := $(patsubst %.S, $(O)/%.bin.ihex, $(SOURCES))
 
-CODE   := $(patsubst %.S, %.c, $(SOURCES))
+CODE   := $(patsubst %.S, $(O)/%.c, $(SOURCES))
 
 all:   $(BIN) $(IHEX) $(CODE)
 
 clean:
-   @rm -f *.o *.bin.ihex *.bin *.c
+   @rm -f $(O)/*.o $(O)/*.bin.ihex $(O)/*.bin $(O)/*.c
 
-%.o:   %.S
-   @cc -c $^
+$(O)/%.o:  %.S
+   @cc -c $^ -o $@
 
-%.bin: %.o
+$(O)/%.bin:$(O)/%.o
@objcopy -Obinary $^ $@
 
-%.bin.ihex:%.o
+$(O)/%.bin.ihex:   $(O)/%.o
@objcopy -Oihex $^ $@
@dos2unix $@ 2>/dev/null
 
-%.c:   %.bin
+$(O)/%.c:  $(O)/%.bin
@echo "{" >$@; hexdump -f hex $^ >>$@; echo "};" >>$@
-- 
2.1.4

[PATCH 1/2] drm: use .hword to represent 16-bit numbers

[Javi Merino]

The size of .word is the size of a word in the given platform, which
for intel systems is 16-bits but other architectures use different
sizes.  However, .hword emits 16-bit numbers regardless of the
platform (and despite the name).  The quantities specified in EDID are
platform independent, so they should work in spite of the default
target of the cc you are using, so use .hword where EDID specifies
16-bit numbers.

Cc: Carsten Emde <c.e...@osadl.org>
Cc: David Airlie <airl...@linux.ie>
Signed-off-by: Javi Merino <javi.mer...@kernel.org>
---
 Documentation/EDID/edid.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/EDID/edid.S b/Documentation/EDID/edid.S
index 7ac0327..ef082dc 100644
--- a/Documentation/EDID/edid.S
+++ b/Documentation/EDID/edid.S
@@ -59,9 +59,9 @@
 /* Fixed header pattern */
 header:.byte   0x00,0xff,0xff,0xff,0xff,0xff,0xff,0x00
 
-mfg_id:.word   swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
+mfg_id:.hword  swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
 
-prod_code: .word   0
+prod_code: .hword  0
 
 /* Serial number. 32 bits, little endian. */
 serial_number: .long   SERIAL
@@ -177,7 +177,7 @@ std_vres:   .byte   (XY_RATIO<<6)+VFREQ-60
 
 descriptor1:
 /* Pixel clock in 10 kHz units. (0.-655.35 MHz, little-endian) */
-clock: .word   CLOCK/10
+clock: .hword  CLOCK/10
 
 /* Horizontal active pixels 8 lsbits (0-4095) */
 x_act_lsb: .byte   XPIX&0xff
-- 
2.1.4

[PATCH 1/2] drm: use .hword to represent 16-bit numbers

[Javi Merino]

The size of .word is the size of a word in the given platform, which
for intel systems is 16-bits but other architectures use different
sizes.  However, .hword emits 16-bit numbers regardless of the
platform (and despite the name).  The quantities specified in EDID are
platform independent, so they should work in spite of the default
target of the cc you are using, so use .hword where EDID specifies
16-bit numbers.

Cc: Carsten Emde 
Cc: David Airlie 
Signed-off-by: Javi Merino 
---
 Documentation/EDID/edid.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/EDID/edid.S b/Documentation/EDID/edid.S
index 7ac0327..ef082dc 100644
--- a/Documentation/EDID/edid.S
+++ b/Documentation/EDID/edid.S
@@ -59,9 +59,9 @@
 /* Fixed header pattern */
 header:.byte   0x00,0xff,0xff,0xff,0xff,0xff,0xff,0x00
 
-mfg_id:.word   swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
+mfg_id:.hword  swap16(mfgname2id(MFG_LNX1, MFG_LNX2, MFG_LNX3))
 
-prod_code: .word   0
+prod_code: .hword  0
 
 /* Serial number. 32 bits, little endian. */
 serial_number: .long   SERIAL
@@ -177,7 +177,7 @@ std_vres:   .byte   (XY_RATIO<<6)+VFREQ-60
 
 descriptor1:
 /* Pixel clock in 10 kHz units. (0.-655.35 MHz, little-endian) */
-clock: .word   CLOCK/10
+clock: .hword  CLOCK/10
 
 /* Horizontal active pixels 8 lsbits (0-4095) */
 x_act_lsb: .byte   XPIX&0xff
-- 
2.1.4

[PATCH 0/2] Documentation/EDID fixes

[Javi Merino]

Hi,

I found these two minor issues while building an EDID.  I'm not sure
whether the second patch (Add O= to support) is upstream material, but
I'm sending it just in case.

Thanks,
Javi

Javi Merino (2):
  drm: use .hword to represent 16-bit numbers
  drm: Add O= support

 Documentation/EDID/Makefile | 21 -
 Documentation/EDID/edid.S   |  6 +++---
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.1.4

[PATCH 0/2] Documentation/EDID fixes

[Javi Merino]

Hi,

I found these two minor issues while building an EDID.  I'm not sure
whether the second patch (Add O= to support) is upstream material, but
I'm sending it just in case.

Thanks,
Javi

Javi Merino (2):
  drm: use .hword to represent 16-bit numbers
  drm: Add O= support

 Documentation/EDID/Makefile | 21 -
 Documentation/EDID/edid.S   |  6 +++---
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.1.4

Re: [PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay

[Javi Merino]

On Mon, Dec 05, 2016 at 10:13:38AM -0300, Javier Martinez Canillas wrote:
> Hello Javi,
> 
> On 12/05/2016 07:09 AM, Javi Merino wrote:
> > In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
> > part of a devicetree overlay, for example:
> > 
> > _bridge {
> > ...
> > my_port: port@0 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0>;
> > ep: endpoint@0 {
> > remote-endpoint = <>;
> > };
> > };
> > };
> > 
> > / {
> > fragment@0 {
> > target = <>;
> > __overlay__ {
> > my_cam {
> > compatible = "foo,bar";
> > port {
> > camera0: endpoint {
> > remote-endpoint = <_port>;
> > ...
> > };
> > };
> > };
> > };
> > };
> > };
> > 
> > Each time the overlay is applied, its of_node pointer will be
> > different.  We are not interested in matching the pointer, what we
> > want to match is that the path is the one we are expecting.  Change to
> > use of_node_cmp() so that we continue matching after the overlay has
> > been removed and reapplied.
> > 
> > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> > Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> > Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> > Signed-off-by: Javi Merino <javi.mer...@kernel.org>
> > ---
> 
> I already reviewed v1 but you didn't carry the tag. So again:

I forgot to add it :(

> Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Thanks!
Javi

Re: [PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay

[Javi Merino]

On Mon, Dec 05, 2016 at 10:13:38AM -0300, Javier Martinez Canillas wrote:
> Hello Javi,
> 
> On 12/05/2016 07:09 AM, Javi Merino wrote:
> > In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
> > part of a devicetree overlay, for example:
> > 
> > _bridge {
> > ...
> > my_port: port@0 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0>;
> > ep: endpoint@0 {
> > remote-endpoint = <>;
> > };
> > };
> > };
> > 
> > / {
> > fragment@0 {
> > target = <>;
> > __overlay__ {
> > my_cam {
> > compatible = "foo,bar";
> > port {
> > camera0: endpoint {
> > remote-endpoint = <_port>;
> > ...
> > };
> > };
> > };
> > };
> > };
> > };
> > 
> > Each time the overlay is applied, its of_node pointer will be
> > different.  We are not interested in matching the pointer, what we
> > want to match is that the path is the one we are expecting.  Change to
> > use of_node_cmp() so that we continue matching after the overlay has
> > been removed and reapplied.
> > 
> > Cc: Mauro Carvalho Chehab 
> > Cc: Javier Martinez Canillas 
> > Cc: Sakari Ailus 
> > Signed-off-by: Javi Merino 
> > ---
> 
> I already reviewed v1 but you didn't carry the tag. So again:

I forgot to add it :(

> Reviewed-by: Javier Martinez Canillas 

Thanks!
Javi

[PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay

[Javi Merino]

In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
part of a devicetree overlay, for example:

_bridge {
...
my_port: port@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
ep: endpoint@0 {
remote-endpoint = <>;
};
};
};

/ {
fragment@0 {
target = <>;
__overlay__ {
my_cam {
compatible = "foo,bar";
port {
camera0: endpoint {
remote-endpoint = <_port>;
...
};
};
};
};
};
};

Each time the overlay is applied, its of_node pointer will be
different.  We are not interested in matching the pointer, what we
want to match is that the path is the one we are expecting.  Change to
use of_node_cmp() so that we continue matching after the overlay has
been removed and reapplied.

Cc: Mauro Carvalho Chehab <mche...@kernel.org>
Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
Signed-off-by: Javi Merino <javi.mer...@kernel.org>
---
 drivers/media/v4l2-core/v4l2-async.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 5bada20..d33a17c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
 
 static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-   return sd->of_node == asd->match.of.node;
+   return !of_node_cmp(of_node_full_name(sd->of_node),
+   of_node_full_name(asd->match.of.node));
 }
 
 static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-- 
2.1.4

[PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay

[Javi Merino]

In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
part of a devicetree overlay, for example:

_bridge {
...
my_port: port@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
ep: endpoint@0 {
remote-endpoint = <>;
};
};
};

/ {
fragment@0 {
target = <>;
__overlay__ {
my_cam {
compatible = "foo,bar";
port {
camera0: endpoint {
remote-endpoint = <_port>;
...
};
};
};
};
};
};

Each time the overlay is applied, its of_node pointer will be
different.  We are not interested in matching the pointer, what we
want to match is that the path is the one we are expecting.  Change to
use of_node_cmp() so that we continue matching after the overlay has
been removed and reapplied.

Cc: Mauro Carvalho Chehab 
Cc: Javier Martinez Canillas 
Cc: Sakari Ailus 
Signed-off-by: Javi Merino 
---
 drivers/media/v4l2-core/v4l2-async.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 5bada20..d33a17c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
 
 static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-   return sd->of_node == asd->match.of.node;
+   return !of_node_cmp(of_node_full_name(sd->of_node),
+   of_node_full_name(asd->match.of.node));
 }
 
 static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-- 
2.1.4

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Fri, Nov 25, 2016 at 10:21:21AM +0200, Sakari Ailus wrote:
Hi Sakari,

> On Wed, Nov 23, 2016 at 04:15:11PM +0000, Javi Merino wrote:
> > On Wed, Nov 23, 2016 at 05:10:42PM +0200, Sakari Ailus wrote:
> > > Hi Javi,
> > 
> > Hi Sakari,
> > 
> > > On Wed, Nov 23, 2016 at 10:09:57AM +, Javi Merino wrote:
> > > > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > > > a devicetree overlay, its of_node pointer will be different each time
> > > > the overlay is applied.  We are not interested in matching the
> > > > pointer, what we want to match is that the path is the one we are
> > > > expecting.  Change to use of_node_cmp() so that we continue matching
> > > > after the overlay has been removed and reapplied.
> > > > 
> > > > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> > > > Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> > > > Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> > > > Signed-off-by: Javi Merino <javi.mer...@kernel.org>
> > > > ---
> > > > Hi,
> > > > 
> > > > I feel it is a bit of a hack, but I couldn't think of anything better.
> > > > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > > > solution.
> > > 
> > > First I have to admit that I'm not an expert when it comes to DT overlays.
> > > 
> > > That said, my understanding is that the sub-device and the async 
> > > sub-device
> > > are supposed to point to the exactly same DT node. I wonder if there's
> > > actually anything wrong in the current code.
> > > 
> > > If the overlay has changed between probing the driver for the async 
> > > notifier
> > > and the async sub-device, there should be no match here, should there? The
> > > two nodes actually point to a node in a different overlay in that case.
> > 
> > Overlays are parts of the devicetree that can be added and removed.
> > When the overlay is applied, the camera driver is probed and does
> > v4l2_async_register_subdev().  However, v4l2_async_belongs() fails.
> > The problem is with comparing pointers.  I haven't looked at the
> > implementation of overlays in detail, but what I see is that the
> > of_node pointer changes when you remove and reapply an overlay (I
> > guess it's dynamically allocated and when you remove the overlay, it's
> > freed).
> 
> The concern here which we were discussing was whether the overlay should be
> relied on having compliant configuration compared to the part which was not
> part of the overlay.
> 
> As external components are involved, quite possibly also the ISP DT node
> will require changes, not just the image source (TV tuner, camera sensor
> etc.). This could be because of number of CSI-2 lanes or parallel bus width,
> for instance.
> 
> I'd also be interested in having an actual driver implement support for
> removing and adding a DT overlay first so we'd see how this would actually
> work. We need both in order to be able to actually remove and add DT
> overlays _without_ unbinding the ISP driver. Otherwise it should already
> work in the current codebase.

Unfortunately, the driver I'm working on is not upstream and I can't
submit it to mainline.  This patch fixes the issue for me, so I
thought it could be useful fix for the kernel.

Cheers,
Javi

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Fri, Nov 25, 2016 at 10:21:21AM +0200, Sakari Ailus wrote:
Hi Sakari,

> On Wed, Nov 23, 2016 at 04:15:11PM +0000, Javi Merino wrote:
> > On Wed, Nov 23, 2016 at 05:10:42PM +0200, Sakari Ailus wrote:
> > > Hi Javi,
> > 
> > Hi Sakari,
> > 
> > > On Wed, Nov 23, 2016 at 10:09:57AM +, Javi Merino wrote:
> > > > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > > > a devicetree overlay, its of_node pointer will be different each time
> > > > the overlay is applied.  We are not interested in matching the
> > > > pointer, what we want to match is that the path is the one we are
> > > > expecting.  Change to use of_node_cmp() so that we continue matching
> > > > after the overlay has been removed and reapplied.
> > > > 
> > > > Cc: Mauro Carvalho Chehab 
> > > > Cc: Javier Martinez Canillas 
> > > > Cc: Sakari Ailus 
> > > > Signed-off-by: Javi Merino 
> > > > ---
> > > > Hi,
> > > > 
> > > > I feel it is a bit of a hack, but I couldn't think of anything better.
> > > > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > > > solution.
> > > 
> > > First I have to admit that I'm not an expert when it comes to DT overlays.
> > > 
> > > That said, my understanding is that the sub-device and the async 
> > > sub-device
> > > are supposed to point to the exactly same DT node. I wonder if there's
> > > actually anything wrong in the current code.
> > > 
> > > If the overlay has changed between probing the driver for the async 
> > > notifier
> > > and the async sub-device, there should be no match here, should there? The
> > > two nodes actually point to a node in a different overlay in that case.
> > 
> > Overlays are parts of the devicetree that can be added and removed.
> > When the overlay is applied, the camera driver is probed and does
> > v4l2_async_register_subdev().  However, v4l2_async_belongs() fails.
> > The problem is with comparing pointers.  I haven't looked at the
> > implementation of overlays in detail, but what I see is that the
> > of_node pointer changes when you remove and reapply an overlay (I
> > guess it's dynamically allocated and when you remove the overlay, it's
> > freed).
> 
> The concern here which we were discussing was whether the overlay should be
> relied on having compliant configuration compared to the part which was not
> part of the overlay.
> 
> As external components are involved, quite possibly also the ISP DT node
> will require changes, not just the image source (TV tuner, camera sensor
> etc.). This could be because of number of CSI-2 lanes or parallel bus width,
> for instance.
> 
> I'd also be interested in having an actual driver implement support for
> removing and adding a DT overlay first so we'd see how this would actually
> work. We need both in order to be able to actually remove and add DT
> overlays _without_ unbinding the ISP driver. Otherwise it should already
> work in the current codebase.

Unfortunately, the driver I'm working on is not upstream and I can't
submit it to mainline.  This patch fixes the issue for me, so I
thought it could be useful fix for the kernel.

Cheers,
Javi

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Wed, Nov 23, 2016 at 05:10:42PM +0200, Sakari Ailus wrote:
> Hi Javi,

Hi Sakari,

> On Wed, Nov 23, 2016 at 10:09:57AM +, Javi Merino wrote:
> > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > a devicetree overlay, its of_node pointer will be different each time
> > the overlay is applied.  We are not interested in matching the
> > pointer, what we want to match is that the path is the one we are
> > expecting.  Change to use of_node_cmp() so that we continue matching
> > after the overlay has been removed and reapplied.
> > 
> > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> > Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> > Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> > Signed-off-by: Javi Merino <javi.mer...@kernel.org>
> > ---
> > Hi,
> > 
> > I feel it is a bit of a hack, but I couldn't think of anything better.
> > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > solution.
> 
> First I have to admit that I'm not an expert when it comes to DT overlays.
> 
> That said, my understanding is that the sub-device and the async sub-device
> are supposed to point to the exactly same DT node. I wonder if there's
> actually anything wrong in the current code.
> 
> If the overlay has changed between probing the driver for the async notifier
> and the async sub-device, there should be no match here, should there? The
> two nodes actually point to a node in a different overlay in that case.

Overlays are parts of the devicetree that can be added and removed.
When the overlay is applied, the camera driver is probed and does
v4l2_async_register_subdev().  However, v4l2_async_belongs() fails.
The problem is with comparing pointers.  I haven't looked at the
implementation of overlays in detail, but what I see is that the
of_node pointer changes when you remove and reapply an overlay (I
guess it's dynamically allocated and when you remove the overlay, it's
freed).

Cheers,
Javi

> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 5bada20..d33a17c 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -   return sd->of_node == asd->match.of.node;
> > +   return !of_node_cmp(of_node_full_name(sd->of_node),
> > +   of_node_full_name(asd->match.of.node));
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Wed, Nov 23, 2016 at 05:10:42PM +0200, Sakari Ailus wrote:
> Hi Javi,

Hi Sakari,

> On Wed, Nov 23, 2016 at 10:09:57AM +, Javi Merino wrote:
> > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > a devicetree overlay, its of_node pointer will be different each time
> > the overlay is applied.  We are not interested in matching the
> > pointer, what we want to match is that the path is the one we are
> > expecting.  Change to use of_node_cmp() so that we continue matching
> > after the overlay has been removed and reapplied.
> > 
> > Cc: Mauro Carvalho Chehab 
> > Cc: Javier Martinez Canillas 
> > Cc: Sakari Ailus 
> > Signed-off-by: Javi Merino 
> > ---
> > Hi,
> > 
> > I feel it is a bit of a hack, but I couldn't think of anything better.
> > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > solution.
> 
> First I have to admit that I'm not an expert when it comes to DT overlays.
> 
> That said, my understanding is that the sub-device and the async sub-device
> are supposed to point to the exactly same DT node. I wonder if there's
> actually anything wrong in the current code.
> 
> If the overlay has changed between probing the driver for the async notifier
> and the async sub-device, there should be no match here, should there? The
> two nodes actually point to a node in a different overlay in that case.

Overlays are parts of the devicetree that can be added and removed.
When the overlay is applied, the camera driver is probed and does
v4l2_async_register_subdev().  However, v4l2_async_belongs() fails.
The problem is with comparing pointers.  I haven't looked at the
implementation of overlays in detail, but what I see is that the
of_node pointer changes when you remove and reapply an overlay (I
guess it's dynamically allocated and when you remove the overlay, it's
freed).

Cheers,
Javi

> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 5bada20..d33a17c 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -   return sd->of_node == asd->match.of.node;
> > +   return !of_node_cmp(of_node_full_name(sd->of_node),
> > +   of_node_full_name(asd->match.of.node));
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Wed, Nov 23, 2016 at 11:25:39AM -0300, Javier Martinez Canillas wrote:
> Hello Javi,
> 
> On 11/23/2016 07:09 AM, Javi Merino wrote:
> > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > a devicetree overlay, its of_node pointer will be different each time
> > the overlay is applied.  We are not interested in matching the
> > pointer, what we want to match is that the path is the one we are
> > expecting.  Change to use of_node_cmp() so that we continue matching
> > after the overlay has been removed and reapplied.
> >
> 
> I'm still not that familiar with DT overlays (and I guess others aren't
> either) so I think that including an example of a base tree and overlay
> DTS where this is an issue, could make things more clear in the commit.
> 
> IIUC, it should be something like this?
> 
> -- base tree --
> 
>  {
>   camera: camera@10 {
>   reg = <0x10>;
>   port {
>   cam_ep: endpoint {
>   ...
>   };
>   };
>   };
> };
> 
> _bridge {
>   ...
>   ports {
>   port@0 {
>   reg = <0>;
>   ep: endpoint {
>   remote-endpoint = <_ep>;
>   };
>   };
>   };
> };
> 
> -- overlay --
> 
> /plugin/;
> / {
>   ...
>   fragment@0 {
>   target = <>;
>   __overlay__ {
>   compatible = "foo,bar";
>   ...
>   port {
>   cam_ep: endpoint {
>   ...
>   };
>   };
>   };
>   }
> }

Yes, that's right.  What I have is that the whole camera can be
plugged or unplugged, so the overlay adds/removes the camera node:

/ {
fragment@0 {
target-path = "/i2c0";
__overlay__ {
my_cam {
compatible = "foo,bar";
port {
camera0: endpoint {
remote-endpoint = <>;
...
};
};
    };
};
};

I will add it to the commit message.

> > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> > Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> > Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> > Signed-off-by: Javi Merino <javi.mer...@kernel.org>
> > ---
> > Hi,
> > 
> > I feel it is a bit of a hack, but I couldn't think of anything better.
> > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > solution.
> >
> 
> I also couldn't think a better way to do this, since IIUC the node's name is
> the only thing that doesn't change, and is available at the time the bridge
> driver calls v4l2_async_notifier_register() when parsing the base tree.
> 
> >  drivers/media/v4l2-core/v4l2-async.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 5bada20..d33a17c 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -   return sd->of_node == asd->match.of.node;
> > +   return !of_node_cmp(of_node_full_name(sd->of_node),
> > +   of_node_full_name(asd->match.of.node));
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> > 
> 
> Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> Best regards,
> -- 
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Wed, Nov 23, 2016 at 11:25:39AM -0300, Javier Martinez Canillas wrote:
> Hello Javi,
> 
> On 11/23/2016 07:09 AM, Javi Merino wrote:
> > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > a devicetree overlay, its of_node pointer will be different each time
> > the overlay is applied.  We are not interested in matching the
> > pointer, what we want to match is that the path is the one we are
> > expecting.  Change to use of_node_cmp() so that we continue matching
> > after the overlay has been removed and reapplied.
> >
> 
> I'm still not that familiar with DT overlays (and I guess others aren't
> either) so I think that including an example of a base tree and overlay
> DTS where this is an issue, could make things more clear in the commit.
> 
> IIUC, it should be something like this?
> 
> -- base tree --
> 
>  {
>   camera: camera@10 {
>   reg = <0x10>;
>   port {
>   cam_ep: endpoint {
>   ...
>   };
>   };
>   };
> };
> 
> _bridge {
>   ...
>   ports {
>   port@0 {
>   reg = <0>;
>   ep: endpoint {
>   remote-endpoint = <_ep>;
>   };
>   };
>   };
> };
> 
> -- overlay --
> 
> /plugin/;
> / {
>   ...
>   fragment@0 {
>   target = <>;
>   __overlay__ {
>   compatible = "foo,bar";
>   ...
>   port {
>   cam_ep: endpoint {
>   ...
>   };
>   };
>   };
>   }
> }

Yes, that's right.  What I have is that the whole camera can be
plugged or unplugged, so the overlay adds/removes the camera node:

/ {
fragment@0 {
target-path = "/i2c0";
__overlay__ {
my_cam {
compatible = "foo,bar";
port {
camera0: endpoint {
remote-endpoint = <>;
...
        };
};
};
};
};

I will add it to the commit message.

> > Cc: Mauro Carvalho Chehab 
> > Cc: Javier Martinez Canillas 
> > Cc: Sakari Ailus 
> > Signed-off-by: Javi Merino 
> > ---
> > Hi,
> > 
> > I feel it is a bit of a hack, but I couldn't think of anything better.
> > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > solution.
> >
> 
> I also couldn't think a better way to do this, since IIUC the node's name is
> the only thing that doesn't change, and is available at the time the bridge
> driver calls v4l2_async_notifier_register() when parsing the base tree.
> 
> >  drivers/media/v4l2-core/v4l2-async.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 5bada20..d33a17c 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -   return sd->of_node == asd->match.of.node;
> > +   return !of_node_cmp(of_node_full_name(sd->of_node),
> > +   of_node_full_name(asd->match.of.node));
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> > 
> 
> Reviewed-by: Javier Martinez Canillas 
> 
> Best regards,
> -- 
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America

[PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
a devicetree overlay, its of_node pointer will be different each time
the overlay is applied.  We are not interested in matching the
pointer, what we want to match is that the path is the one we are
expecting.  Change to use of_node_cmp() so that we continue matching
after the overlay has been removed and reapplied.

Cc: Mauro Carvalho Chehab <mche...@kernel.org>
Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
Signed-off-by: Javi Merino <javi.mer...@kernel.org>
---
Hi,

I feel it is a bit of a hack, but I couldn't think of anything better.
I'm ccing devicetree@ and Pantelis because there may be a simpler
solution.

 drivers/media/v4l2-core/v4l2-async.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 5bada20..d33a17c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
 
 static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-   return sd->of_node == asd->match.of.node;
+   return !of_node_cmp(of_node_full_name(sd->of_node),
+   of_node_full_name(asd->match.of.node));
 }
 
 static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-- 
2.1.4

[PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
a devicetree overlay, its of_node pointer will be different each time
the overlay is applied.  We are not interested in matching the
pointer, what we want to match is that the path is the one we are
expecting.  Change to use of_node_cmp() so that we continue matching
after the overlay has been removed and reapplied.

Cc: Mauro Carvalho Chehab 
Cc: Javier Martinez Canillas 
Cc: Sakari Ailus 
Signed-off-by: Javi Merino 
---
Hi,

I feel it is a bit of a hack, but I couldn't think of anything better.
I'm ccing devicetree@ and Pantelis because there may be a simpler
solution.

 drivers/media/v4l2-core/v4l2-async.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 5bada20..d33a17c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
 
 static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-   return sd->of_node == asd->match.of.node;
+   return !of_node_cmp(of_node_full_name(sd->of_node),
+   of_node_full_name(asd->match.of.node));
 }
 
 static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-- 
2.1.4

[PATCH] MAINTAINERS: Switch to kernel.org email address for Javi Merino

[Javi Merino]

Change my email address to my kernel.org account instead of the ARM one.

Signed-off-by: Javi Merino <javi.mer...@arm.com>
---
 .mailmap| 1 +
 MAINTAINERS | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index de22daefd9da..1dab0a156489 100644
--- a/.mailmap
+++ b/.mailmap
@@ -69,6 +69,7 @@ James Bottomley <jejb@mulgrave.(none)>
 James Bottomley <j...@titanic.il.steeleye.com>
 James E Wilson <wil...@specifix.com>
 James Ketrenos <jketreno@io.(none)>
+Javi Merino <javi.mer...@kernel.org> <javi.mer...@arm.com>
 <jav...@osg.samsung.com> <javier.marti...@collabora.co.uk>
 Jean Tourrilhes <j...@hpl.hp.com>
 Jeff Garzik <jgar...@pretzel.yyz.us>
diff --git a/MAINTAINERS b/MAINTAINERS
index b003d0ca6238..f593300e310b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11626,7 +11626,7 @@ F:  Documentation/devicetree/bindings/thermal/
 THERMAL/CPU_COOLING
 M: Amit Daniel Kachhap <amit.kach...@gmail.com>
 M: Viresh Kumar <viresh.ku...@linaro.org>
-M: Javi Merino <javi.mer...@arm.com>
+M: Javi Merino <javi.mer...@kernel.org>
 L: linux...@vger.kernel.org
 S: Supported
 F: Documentation/thermal/cpu-cooling-api.txt
-- 
1.9.1

[PATCH] MAINTAINERS: Switch to kernel.org email address for Javi Merino

[Javi Merino]

Change my email address to my kernel.org account instead of the ARM one.

Signed-off-by: Javi Merino 
---
 .mailmap| 1 +
 MAINTAINERS | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index de22daefd9da..1dab0a156489 100644
--- a/.mailmap
+++ b/.mailmap
@@ -69,6 +69,7 @@ James Bottomley 
 James Bottomley 
 James E Wilson 
 James Ketrenos 
+Javi Merino  
  
 Jean Tourrilhes 
 Jeff Garzik 
diff --git a/MAINTAINERS b/MAINTAINERS
index b003d0ca6238..f593300e310b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11626,7 +11626,7 @@ F:  Documentation/devicetree/bindings/thermal/
 THERMAL/CPU_COOLING
 M: Amit Daniel Kachhap 
 M: Viresh Kumar 
-M: Javi Merino 
+M: Javi Merino 
 L: linux...@vger.kernel.org
 S: Supported
 F: Documentation/thermal/cpu-cooling-api.txt
-- 
1.9.1

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

[Javi Merino]

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 
> > 
> 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=143893141227189=4
[1] https://marc.info/?l=linux-pm=144649947022547=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", ))
> >     tzp->sustainable_power = prop;
> >  
> > +   if (!of_property_read_string(child, "governor-name", 
> > ))
> > +   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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Javi Merino]

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 
> > 
> 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=143893141227189=4
[1] https://marc.info/?l=linux-pm=144649947022547=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", ))
> >     tzp->sustainable_power = prop;
> >  
> > +   if (!of_property_read_string(child, "governor-name", 
> > ))
> > +   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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] thermal: cpu_cooling: Fix wrong comment call function name

[Javi Merino]

On Wed, Sep 07, 2016 at 09:35:39AM +0900, Inhyuk Kang wrote:
> The last_load is updated not cpufreq_get_actual_power() function call
> but cpufreq_get_requested_power() function call.

Yep, my bad.  Thanks for fixing it!

> Signed-off-by: Inhyuk Kang <hugh.k...@lge.com>

Acked-by: Javi Merino <javi.mer...@arm.com>

> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index a32b417..9ce0e9e 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -74,7 +74,7 @@ struct power_table {
>   *   cpufreq frequencies.
>   * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
>   * @node: list_head to link all cpufreq_cooling_device together.
> - * @last_load: load measured by the latest call to cpufreq_get_actual_power()
> + * @last_load: load measured by the latest call to 
> cpufreq_get_requested_power()
>   * @time_in_idle: previous reading of the absolute time that this cpu was 
> idle
>   * @time_in_idle_timestamp: wall time of the last invocation of
>   *   get_cpu_idle_time_us()
> -- 
> 1.9.1
> 

Re: [PATCH] thermal: cpu_cooling: Fix wrong comment call function name

[Javi Merino]

On Wed, Sep 07, 2016 at 09:35:39AM +0900, Inhyuk Kang wrote:
> The last_load is updated not cpufreq_get_actual_power() function call
> but cpufreq_get_requested_power() function call.

Yep, my bad.  Thanks for fixing it!

> Signed-off-by: Inhyuk Kang 

Acked-by: Javi Merino 

> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index a32b417..9ce0e9e 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -74,7 +74,7 @@ struct power_table {
>   *   cpufreq frequencies.
>   * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
>   * @node: list_head to link all cpufreq_cooling_device together.
> - * @last_load: load measured by the latest call to cpufreq_get_actual_power()
> + * @last_load: load measured by the latest call to 
> cpufreq_get_requested_power()
>   * @time_in_idle: previous reading of the absolute time that this cpu was 
> idle
>   * @time_in_idle_timestamp: wall time of the last invocation of
>   *   get_cpu_idle_time_us()
> -- 
> 1.9.1
> 

Re: [PATCH] devfreq_cooling: pass a pointer to devfreq in the power model callbacks

[Javi Merino]

Eduardo, Rui,

On Fri, Jun 03, 2016 at 10:25:31AM +0100, Javi Merino wrote:
> When the devfreq cooling device was designed, it was an oversight not to
> pass a pointer to the struct devfreq as the first parameters of the
> callbacks.  The design patterns of the kernel suggest it for a good
> reason.
> 
> By passing a pointer to struct devfreq, the driver can register one
> function that works with multiple devices.  With the current
> implementation, a driver that can work with multiple devices has to
> create multiple copies of the same function with different parameters so
> that each devfreq_cooling_device can use the appropriate one.  By
> passing a pointer to struct devfreq, the driver can identify which
> device it's referring to.
> 
> Cc: Zhang Rui <rui.zh...@intel.com>
> Cc: Eduardo Valentin <edubez...@gmail.com>
> Reviewed-by: Punit Agrawal <punit.agra...@arm.com>
> Signed-off-by: Javi Merino <javi.mer...@arm.com>
> ---
>  drivers/thermal/devfreq_cooling.c | 5 +++--
>  include/linux/devfreq_cooling.h   | 6 --
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/devfreq_cooling.c 
> b/drivers/thermal/devfreq_cooling.c
> index 01f0015f80dc..c549d83a0c7d 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -238,7 +238,7 @@ get_static_power(struct devfreq_cooling_device *dfc, 
> unsigned long freq)
>   return 0;
>   }
>  
> - return dfc->power_ops->get_static_power(voltage);
> + return dfc->power_ops->get_static_power(df, voltage);
>  }
>  
>  /**
> @@ -262,7 +262,8 @@ get_dynamic_power(struct devfreq_cooling_device *dfc, 
> unsigned long freq,
>   struct devfreq_cooling_power *dfc_power = dfc->power_ops;
>  
>   if (dfc_power->get_dynamic_power)
> - return dfc_power->get_dynamic_power(freq, voltage);
> + return dfc_power->get_dynamic_power(dfc->devfreq, freq,
> + voltage);
>  
>   freq_mhz = freq / 100;
>   power = (u64)dfc_power->dyn_power_coeff * freq_mhz * voltage * voltage;
> diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
> index 7adf6cc4b305..959714e93e5b 100644
> --- a/include/linux/devfreq_cooling.h
> +++ b/include/linux/devfreq_cooling.h
> @@ -37,8 +37,10 @@
>   *   @dyn_power_coeff * frequency * voltage^2
>   */
>  struct devfreq_cooling_power {
> - unsigned long (*get_static_power)(unsigned long voltage);
> - unsigned long (*get_dynamic_power)(unsigned long freq,
> + unsigned long (*get_static_power)(struct devfreq *devfreq,
> +   unsigned long voltage);
> + unsigned long (*get_dynamic_power)(struct devfreq *devfreq,
> +unsigned long freq,
>  unsigned long voltage);
>   unsigned long dyn_power_coeff;
>  };

If there are no objections, can you pick this for the next merge
window?

Thanks,
Javi

Re: [PATCH] devfreq_cooling: pass a pointer to devfreq in the power model callbacks

[Javi Merino]

Eduardo, Rui,

On Fri, Jun 03, 2016 at 10:25:31AM +0100, Javi Merino wrote:
> When the devfreq cooling device was designed, it was an oversight not to
> pass a pointer to the struct devfreq as the first parameters of the
> callbacks.  The design patterns of the kernel suggest it for a good
> reason.
> 
> By passing a pointer to struct devfreq, the driver can register one
> function that works with multiple devices.  With the current
> implementation, a driver that can work with multiple devices has to
> create multiple copies of the same function with different parameters so
> that each devfreq_cooling_device can use the appropriate one.  By
> passing a pointer to struct devfreq, the driver can identify which
> device it's referring to.
> 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Reviewed-by: Punit Agrawal 
> Signed-off-by: Javi Merino 
> ---
>  drivers/thermal/devfreq_cooling.c | 5 +++--
>  include/linux/devfreq_cooling.h   | 6 --
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/devfreq_cooling.c 
> b/drivers/thermal/devfreq_cooling.c
> index 01f0015f80dc..c549d83a0c7d 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -238,7 +238,7 @@ get_static_power(struct devfreq_cooling_device *dfc, 
> unsigned long freq)
>   return 0;
>   }
>  
> - return dfc->power_ops->get_static_power(voltage);
> + return dfc->power_ops->get_static_power(df, voltage);
>  }
>  
>  /**
> @@ -262,7 +262,8 @@ get_dynamic_power(struct devfreq_cooling_device *dfc, 
> unsigned long freq,
>   struct devfreq_cooling_power *dfc_power = dfc->power_ops;
>  
>   if (dfc_power->get_dynamic_power)
> - return dfc_power->get_dynamic_power(freq, voltage);
> + return dfc_power->get_dynamic_power(dfc->devfreq, freq,
> + voltage);
>  
>   freq_mhz = freq / 100;
>   power = (u64)dfc_power->dyn_power_coeff * freq_mhz * voltage * voltage;
> diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
> index 7adf6cc4b305..959714e93e5b 100644
> --- a/include/linux/devfreq_cooling.h
> +++ b/include/linux/devfreq_cooling.h
> @@ -37,8 +37,10 @@
>   *   @dyn_power_coeff * frequency * voltage^2
>   */
>  struct devfreq_cooling_power {
> - unsigned long (*get_static_power)(unsigned long voltage);
> - unsigned long (*get_dynamic_power)(unsigned long freq,
> + unsigned long (*get_static_power)(struct devfreq *devfreq,
> +   unsigned long voltage);
> + unsigned long (*get_dynamic_power)(struct devfreq *devfreq,
> +unsigned long freq,
>  unsigned long voltage);
>   unsigned long dyn_power_coeff;
>  };

If there are no objections, can you pick this for the next merge
window?

Thanks,
Javi

[PATCH 1/2] arm64: defconfig: enable SENSORS_ARM_SCPI

[Javi Merino]

ARM SCPI Sensors were merged for v4.4 and they are defined in the Juno
dts.  Enable it in the defconfig to get them registered automatically in
Juno by default.

Signed-off-by: Javi Merino <javi.mer...@arm.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index fd2d74d0491e..1152c6d9f6e6 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -197,6 +197,7 @@ CONFIG_POWER_RESET_XGENE=y
 CONFIG_POWER_RESET_SYSCON=y
 CONFIG_SENSORS_LM90=m
 CONFIG_SENSORS_INA2XX=m
+CONFIG_SENSORS_ARM_SCPI=y
 CONFIG_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
 CONFIG_EXYNOS_THERMAL=y
-- 
1.9.1

[PATCH 2/2] arm64: dts: juno: add thermal zones for scpi sensors

[Javi Merino]

The juno dts have entries for the hwmon scpi, let's create thermal zones
for the temperature sensors described in the Juno ARM Development
Platform Implementation Details.

Cc: Liviu Dudau <liviu.du...@arm.com>
Cc: Sudeep Holla <sudeep.ho...@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Cc: Rob Herring <robh...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: Catalin Marinas <catalin.mari...@arm.com>
Cc: Will Deacon <will.dea...@arm.com>
Signed-off-by: Javi Merino <javi.mer...@arm.com>
---
 arch/arm64/boot/dts/arm/juno-base.dtsi | 42 ++
 arch/arm64/boot/dts/arm/juno-r1.dts| 16 +
 arch/arm64/boot/dts/arm/juno-r2.dts| 16 +
 3 files changed, 74 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi 
b/arch/arm64/boot/dts/arm/juno-base.dtsi
index dee2386d3b9b..61ec045125ce 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -125,6 +125,48 @@
};
};
 
+   thermal-zones {
+   pmic {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 0>;
+   };
+
+   soc {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 3>;
+   };
+
+   big_cluster_thermal_zone: big_cluster {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 21>;
+   status = "disabled";
+   };
+
+   little_cluster_thermal_zone: little_cluster {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 22>;
+   status = "disabled";
+   };
+
+   gpu0_thermal_zone: gpu0 {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 23>;
+   status = "disabled";
+   };
+
+   gpu1_thermal_zone: gpu1 {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 24>;
+   status = "disabled";
+   };
+   };
+
/include/ "juno-clocks.dtsi"
 
dma@7ff0 {
diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts 
b/arch/arm64/boot/dts/arm/juno-r1.dts
index d95d9e7e2dc0..0f9c32ae1ce9 100644
--- a/arch/arm64/boot/dts/arm/juno-r1.dts
+++ b/arch/arm64/boot/dts/arm/juno-r1.dts
@@ -181,3 +181,19 @@
 _ctlr {
status = "okay";
 };
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts 
b/arch/arm64/boot/dts/arm/juno-r2.dts
index 88ecd6182b67..f20cf5654cca 100644
--- a/arch/arm64/boot/dts/arm/juno-r2.dts
+++ b/arch/arm64/boot/dts/arm/juno-r2.dts
@@ -181,3 +181,19 @@
 _ctlr {
status = "okay";
 };
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
-- 
1.9.1

[PATCH 1/2] arm64: defconfig: enable SENSORS_ARM_SCPI

[Javi Merino]

ARM SCPI Sensors were merged for v4.4 and they are defined in the Juno
dts.  Enable it in the defconfig to get them registered automatically in
Juno by default.

Signed-off-by: Javi Merino 
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index fd2d74d0491e..1152c6d9f6e6 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -197,6 +197,7 @@ CONFIG_POWER_RESET_XGENE=y
 CONFIG_POWER_RESET_SYSCON=y
 CONFIG_SENSORS_LM90=m
 CONFIG_SENSORS_INA2XX=m
+CONFIG_SENSORS_ARM_SCPI=y
 CONFIG_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
 CONFIG_EXYNOS_THERMAL=y
-- 
1.9.1

[PATCH 2/2] arm64: dts: juno: add thermal zones for scpi sensors

[Javi Merino]

The juno dts have entries for the hwmon scpi, let's create thermal zones
for the temperature sensors described in the Juno ARM Development
Platform Implementation Details.

Cc: Liviu Dudau 
Cc: Sudeep Holla 
Cc: Lorenzo Pieralisi 
Cc: Rob Herring 
Cc: Mark Rutland 
Cc: Catalin Marinas 
Cc: Will Deacon 
Signed-off-by: Javi Merino 
---
 arch/arm64/boot/dts/arm/juno-base.dtsi | 42 ++
 arch/arm64/boot/dts/arm/juno-r1.dts| 16 +
 arch/arm64/boot/dts/arm/juno-r2.dts| 16 +
 3 files changed, 74 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi 
b/arch/arm64/boot/dts/arm/juno-base.dtsi
index dee2386d3b9b..61ec045125ce 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -125,6 +125,48 @@
};
};
 
+   thermal-zones {
+   pmic {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 0>;
+   };
+
+   soc {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 3>;
+   };
+
+   big_cluster_thermal_zone: big_cluster {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 21>;
+   status = "disabled";
+   };
+
+   little_cluster_thermal_zone: little_cluster {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 22>;
+   status = "disabled";
+   };
+
+   gpu0_thermal_zone: gpu0 {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 23>;
+   status = "disabled";
+   };
+
+   gpu1_thermal_zone: gpu1 {
+   polling-delay = <1000>;
+   polling-delay-passive = <100>;
+   thermal-sensors = <_sensors0 24>;
+   status = "disabled";
+   };
+   };
+
/include/ "juno-clocks.dtsi"
 
dma@7ff0 {
diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts 
b/arch/arm64/boot/dts/arm/juno-r1.dts
index d95d9e7e2dc0..0f9c32ae1ce9 100644
--- a/arch/arm64/boot/dts/arm/juno-r1.dts
+++ b/arch/arm64/boot/dts/arm/juno-r1.dts
@@ -181,3 +181,19 @@
 _ctlr {
status = "okay";
 };
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts 
b/arch/arm64/boot/dts/arm/juno-r2.dts
index 88ecd6182b67..f20cf5654cca 100644
--- a/arch/arm64/boot/dts/arm/juno-r2.dts
+++ b/arch/arm64/boot/dts/arm/juno-r2.dts
@@ -181,3 +181,19 @@
 _ctlr {
status = "okay";
 };
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_cluster_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
+
+_thermal_zone {
+   status = "okay";
+};
-- 
1.9.1

[PATCH 0/2] Enable SCPI sensors for Juno

[Javi Merino]

Enable the ARM SCPI sensors in arm64 defconfig and add thermal zones
for the thermal scpi sensors on Juno.  The hwmon support for SCPI
sensors was merged for v4.4, this just enables it by default.

Javi Merino (2):
  arm64: defconfig: enable SENSORS_ARM_SCPI
  arm64: dts: juno: add thermal zones for scpi sensors

 arch/arm64/boot/dts/arm/juno-base.dtsi | 42 ++
 arch/arm64/boot/dts/arm/juno-r1.dts| 16 +
 arch/arm64/boot/dts/arm/juno-r2.dts| 16 +
 arch/arm64/configs/defconfig   |  1 +
 4 files changed, 75 insertions(+)

-- 
1.9.1

[PATCH 0/2] Enable SCPI sensors for Juno

[Javi Merino]

Enable the ARM SCPI sensors in arm64 defconfig and add thermal zones
for the thermal scpi sensors on Juno.  The hwmon support for SCPI
sensors was merged for v4.4, this just enables it by default.

Javi Merino (2):
  arm64: defconfig: enable SENSORS_ARM_SCPI
  arm64: dts: juno: add thermal zones for scpi sensors

 arch/arm64/boot/dts/arm/juno-base.dtsi | 42 ++
 arch/arm64/boot/dts/arm/juno-r1.dts| 16 +
 arch/arm64/boot/dts/arm/juno-r2.dts| 16 +
 arch/arm64/configs/defconfig   |  1 +
 4 files changed, 75 insertions(+)

-- 
1.9.1

Re: [PATCH] thermal: fix race condition when updating cooling device

[Javi Merino]

On Thu, Jun 02, 2016 at 03:25:31PM +0100, Michele Di Giorgio wrote:
> When multiple thermal zones are bound to the same cooling device, multiple
> kernel threads may want to update the cooling device state by calling
> thermal_cdev_update(). Having cdev not protected by a mutex can lead to a race
> condition. Consider the following situation with two kernel threads k1 and k2:
> 
>   Thread k1   Thread k2
> ||
> ||  call thermal_cdev_update()
> ||  ...
> ||  set_cur_state(cdev, target);
> call power_actor_set_power()||
> ... ||
> instance->target = state;   ||
> cdev->updated = false;  ||
> ||  cdev->updated = true;
> ||  // completes execution
> call thermal_cdev_update()  ||
> // cdev->updated == true||
> return; ||
> \/
> time
> 
> k2 has already looped through the thermal instances looking for the deepest
> cooling device state and is preempted right before setting cdev->updated to
> true. Now, k1 runs, modifies the thermal instance state and sets cdev->updated
> to false. Then, k1 is preempted and k2 continues the execution by setting
> cdev->updated to true, therefore preventing k1 from performing the update.
> Notice that this is not an issue if k2 looks at the instance->target modified 
> by
> k1 "after" it is assigned by k1. In fact, in this case the update will happen
> anyway and k1 can safely return immediately from thermal_cdev_update().
> 
> This may lead to a situation where a thermal governor never updates the 
> cooling
> device. For example, this is the case for the step_wise governor: when calling
> the function thermal_zone_trip_update(), the governor may always get a new 
> state
> equal to the old one (which, however, wasn't notified to the cooling device) 
> and
> will therefore skip the update.
> 
> CC: Zhang Rui <rui.zh...@intel.com>
> CC: Eduardo Valentin <edubez...@gmail.com>
> CC: Peter Feuerer <pe...@piie.net>
> Reported-by: Toby Huang <toby.hu...@arm.com>
> Signed-off-by: Michele Di Giorgio <michele.digior...@arm.com>
> ---
> Protecting only the assignment of cdev->updated with mutexes may look
> suspicious, but it is necessary to guarantee synchronization and avoiding the
> situation described in the commit message.
> 
> There are other two possible solutions.
> 
> Moving the cdev->lock mutex outside thermal_cdev_update() and protect both the
> assignment and the function. This would work, but will probably cause many
> issues when updating all the modules that use thermal_cdev_update().
> 
> The other solution is to make cdev->updated an atomic_t, change the if
> condition to an atomic_cmpxchg and extend the critical section to include the
> call to cdev->ops->set_cur_state().

True.  In any case, the mutex needs to cover set_cur_state() in
thermal_cdev_update().  This fixes the race condition, so I'm happy
with it.

Reviewed-by: Javi Merino <javi.mer...@arm.com>

>  drivers/thermal/fair_share.c  |  2 ++
>  drivers/thermal/gov_bang_bang.c   |  2 ++
>  drivers/thermal/power_allocator.c |  2 ++
>  drivers/thermal/step_wise.c   |  2 ++
>  drivers/thermal/thermal_core.c| 10 +++---
>  5 files changed, 15 insertions(+), 3 deletions(-)

Re: [PATCH] thermal: fix race condition when updating cooling device

[Javi Merino]

On Thu, Jun 02, 2016 at 03:25:31PM +0100, Michele Di Giorgio wrote:
> When multiple thermal zones are bound to the same cooling device, multiple
> kernel threads may want to update the cooling device state by calling
> thermal_cdev_update(). Having cdev not protected by a mutex can lead to a race
> condition. Consider the following situation with two kernel threads k1 and k2:
> 
>   Thread k1   Thread k2
> ||
> ||  call thermal_cdev_update()
> ||  ...
> ||  set_cur_state(cdev, target);
> call power_actor_set_power()||
> ... ||
> instance->target = state;   ||
> cdev->updated = false;  ||
> ||  cdev->updated = true;
> ||  // completes execution
> call thermal_cdev_update()  ||
> // cdev->updated == true||
> return; ||
> \/
> time
> 
> k2 has already looped through the thermal instances looking for the deepest
> cooling device state and is preempted right before setting cdev->updated to
> true. Now, k1 runs, modifies the thermal instance state and sets cdev->updated
> to false. Then, k1 is preempted and k2 continues the execution by setting
> cdev->updated to true, therefore preventing k1 from performing the update.
> Notice that this is not an issue if k2 looks at the instance->target modified 
> by
> k1 "after" it is assigned by k1. In fact, in this case the update will happen
> anyway and k1 can safely return immediately from thermal_cdev_update().
> 
> This may lead to a situation where a thermal governor never updates the 
> cooling
> device. For example, this is the case for the step_wise governor: when calling
> the function thermal_zone_trip_update(), the governor may always get a new 
> state
> equal to the old one (which, however, wasn't notified to the cooling device) 
> and
> will therefore skip the update.
> 
> CC: Zhang Rui 
> CC: Eduardo Valentin 
> CC: Peter Feuerer 
> Reported-by: Toby Huang 
> Signed-off-by: Michele Di Giorgio 
> ---
> Protecting only the assignment of cdev->updated with mutexes may look
> suspicious, but it is necessary to guarantee synchronization and avoiding the
> situation described in the commit message.
> 
> There are other two possible solutions.
> 
> Moving the cdev->lock mutex outside thermal_cdev_update() and protect both the
> assignment and the function. This would work, but will probably cause many
> issues when updating all the modules that use thermal_cdev_update().
> 
> The other solution is to make cdev->updated an atomic_t, change the if
> condition to an atomic_cmpxchg and extend the critical section to include the
> call to cdev->ops->set_cur_state().

True.  In any case, the mutex needs to cover set_cur_state() in
thermal_cdev_update().  This fixes the race condition, so I'm happy
with it.

Reviewed-by: Javi Merino 

>  drivers/thermal/fair_share.c  |  2 ++
>  drivers/thermal/gov_bang_bang.c   |  2 ++
>  drivers/thermal/power_allocator.c |  2 ++
>  drivers/thermal/step_wise.c   |  2 ++
>  drivers/thermal/thermal_core.c| 10 +++---
>  5 files changed, 15 insertions(+), 3 deletions(-)

Re: [PATCH v5 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

On Mon, Jun 06, 2016 at 07:44:45PM +0800, Caesar Wang wrote:
> From: Sascha Hauer <s.ha...@pengutronix.de>
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> <mikko.perttu...@kapsi.fi>
> 
> Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
> Signed-off-by: Caesar Wang <w...@rock-chips.com>
> Cc: Zhang Rui <rui.zh...@intel.com>
> Cc: Eduardo Valentin <edubez...@gmail.com>
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v5:
> - add the lock for thermal_zone_set_trips function.

Looks good to me!

Reviewed-by: Javi Merino <javi.mer...@arm.com>

> - change based on next kernel.
> 
> Changes in v4:
> - Missing the lock added in v3.
> 
> Changes in v3:
> - as Javi comments on https://patchwork.kernel.org/patch/9001281/.
> - add the lock for preventing the called from multi placce
> - add the note for pre_low/high_trip.
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips.
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 54 
> +
>  drivers/thermal/thermal_sysfs.c |  3 +++
>  include/linux/thermal.h | 10 +++
>  4 files changed, 74 insertions(+)
> 

Re: [PATCH v5 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

On Mon, Jun 06, 2016 at 07:44:45PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v5:
> - add the lock for thermal_zone_set_trips function.

Looks good to me!

Reviewed-by: Javi Merino 

> - change based on next kernel.
> 
> Changes in v4:
> - Missing the lock added in v3.
> 
> Changes in v3:
> - as Javi comments on https://patchwork.kernel.org/patch/9001281/.
> - add the lock for preventing the called from multi placce
> - add the note for pre_low/high_trip.
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips.
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 54 
> +
>  drivers/thermal/thermal_sysfs.c |  3 +++
>  include/linux/thermal.h | 10 +++
>  4 files changed, 74 insertions(+)
> 

[PATCH] devfreq_cooling: pass a pointer to devfreq in the power model callbacks

[Javi Merino]

When the devfreq cooling device was designed, it was an oversight not to
pass a pointer to the struct devfreq as the first parameters of the
callbacks.  The design patterns of the kernel suggest it for a good
reason.

By passing a pointer to struct devfreq, the driver can register one
function that works with multiple devices.  With the current
implementation, a driver that can work with multiple devices has to
create multiple copies of the same function with different parameters so
that each devfreq_cooling_device can use the appropriate one.  By
passing a pointer to struct devfreq, the driver can identify which
device it's referring to.

Cc: Zhang Rui <rui.zh...@intel.com>
Cc: Eduardo Valentin <edubez...@gmail.com>
Reviewed-by: Punit Agrawal <punit.agra...@arm.com>
Signed-off-by: Javi Merino <javi.mer...@arm.com>
---
 drivers/thermal/devfreq_cooling.c | 5 +++--
 include/linux/devfreq_cooling.h   | 6 --
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/devfreq_cooling.c 
b/drivers/thermal/devfreq_cooling.c
index 01f0015f80dc..c549d83a0c7d 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -238,7 +238,7 @@ get_static_power(struct devfreq_cooling_device *dfc, 
unsigned long freq)
return 0;
}
 
-   return dfc->power_ops->get_static_power(voltage);
+   return dfc->power_ops->get_static_power(df, voltage);
 }
 
 /**
@@ -262,7 +262,8 @@ get_dynamic_power(struct devfreq_cooling_device *dfc, 
unsigned long freq,
struct devfreq_cooling_power *dfc_power = dfc->power_ops;
 
if (dfc_power->get_dynamic_power)
-   return dfc_power->get_dynamic_power(freq, voltage);
+   return dfc_power->get_dynamic_power(dfc->devfreq, freq,
+   voltage);
 
freq_mhz = freq / 100;
power = (u64)dfc_power->dyn_power_coeff * freq_mhz * voltage * voltage;
diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
index 7adf6cc4b305..959714e93e5b 100644
--- a/include/linux/devfreq_cooling.h
+++ b/include/linux/devfreq_cooling.h
@@ -37,8 +37,10 @@
  * @dyn_power_coeff * frequency * voltage^2
  */
 struct devfreq_cooling_power {
-   unsigned long (*get_static_power)(unsigned long voltage);
-   unsigned long (*get_dynamic_power)(unsigned long freq,
+   unsigned long (*get_static_power)(struct devfreq *devfreq,
+ unsigned long voltage);
+   unsigned long (*get_dynamic_power)(struct devfreq *devfreq,
+  unsigned long freq,
   unsigned long voltage);
unsigned long dyn_power_coeff;
 };
-- 
1.9.1

[PATCH] devfreq_cooling: pass a pointer to devfreq in the power model callbacks

[Javi Merino]

When the devfreq cooling device was designed, it was an oversight not to
pass a pointer to the struct devfreq as the first parameters of the
callbacks.  The design patterns of the kernel suggest it for a good
reason.

By passing a pointer to struct devfreq, the driver can register one
function that works with multiple devices.  With the current
implementation, a driver that can work with multiple devices has to
create multiple copies of the same function with different parameters so
that each devfreq_cooling_device can use the appropriate one.  By
passing a pointer to struct devfreq, the driver can identify which
device it's referring to.

Cc: Zhang Rui 
Cc: Eduardo Valentin 
Reviewed-by: Punit Agrawal 
Signed-off-by: Javi Merino 
---
 drivers/thermal/devfreq_cooling.c | 5 +++--
 include/linux/devfreq_cooling.h   | 6 --
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/devfreq_cooling.c 
b/drivers/thermal/devfreq_cooling.c
index 01f0015f80dc..c549d83a0c7d 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -238,7 +238,7 @@ get_static_power(struct devfreq_cooling_device *dfc, 
unsigned long freq)
return 0;
}
 
-   return dfc->power_ops->get_static_power(voltage);
+   return dfc->power_ops->get_static_power(df, voltage);
 }
 
 /**
@@ -262,7 +262,8 @@ get_dynamic_power(struct devfreq_cooling_device *dfc, 
unsigned long freq,
struct devfreq_cooling_power *dfc_power = dfc->power_ops;
 
if (dfc_power->get_dynamic_power)
-   return dfc_power->get_dynamic_power(freq, voltage);
+   return dfc_power->get_dynamic_power(dfc->devfreq, freq,
+   voltage);
 
freq_mhz = freq / 100;
power = (u64)dfc_power->dyn_power_coeff * freq_mhz * voltage * voltage;
diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
index 7adf6cc4b305..959714e93e5b 100644
--- a/include/linux/devfreq_cooling.h
+++ b/include/linux/devfreq_cooling.h
@@ -37,8 +37,10 @@
  * @dyn_power_coeff * frequency * voltage^2
  */
 struct devfreq_cooling_power {
-   unsigned long (*get_static_power)(unsigned long voltage);
-   unsigned long (*get_dynamic_power)(unsigned long freq,
+   unsigned long (*get_static_power)(struct devfreq *devfreq,
+ unsigned long voltage);
+   unsigned long (*get_dynamic_power)(struct devfreq *devfreq,
+  unsigned long freq,
   unsigned long voltage);
unsigned long dyn_power_coeff;
 };
-- 
1.9.1

Re: [PATCH V2 2/6] cpufreq: Remove cpufreq_frequency_get_table()

[Javi Merino]

On Thu, Jun 02, 2016 at 09:06:26PM +0530, Viresh Kumar wrote:
> On 2 June 2016 at 20:29, Javi Merino <javi.mer...@arm.com> wrote:
> > In 5a31d594a973 ("cpufreq: Allow freq_table to be obtained for offline
> > CPUs") you did the opposite: don't use cpufreq_cpu_get_raw() because
> > it won't give you the policy of a cpu that is offline.  Now you are
> > arguing that we should go back to cpufreq_cpu_get() which implicitly
> > calls cpufreq_cpu_get_raw().  Won't we hit the same issue that
> > 5a31d594a973 was trying to prevent: that we can't get a freq_table for
> > a cpu that is offline?
> 
> Yes, that should be fixed. Thanks for letting me know about it :)

Ok, that was my only nit.  Other than that, it looks good to me.  For 
cpu_cooling.c 

Acked-by: Javi Merino <javi.mer...@arm.com>

Re: [PATCH V2 2/6] cpufreq: Remove cpufreq_frequency_get_table()

[Javi Merino]

On Thu, Jun 02, 2016 at 09:06:26PM +0530, Viresh Kumar wrote:
> On 2 June 2016 at 20:29, Javi Merino  wrote:
> > In 5a31d594a973 ("cpufreq: Allow freq_table to be obtained for offline
> > CPUs") you did the opposite: don't use cpufreq_cpu_get_raw() because
> > it won't give you the policy of a cpu that is offline.  Now you are
> > arguing that we should go back to cpufreq_cpu_get() which implicitly
> > calls cpufreq_cpu_get_raw().  Won't we hit the same issue that
> > 5a31d594a973 was trying to prevent: that we can't get a freq_table for
> > a cpu that is offline?
> 
> Yes, that should be fixed. Thanks for letting me know about it :)

Ok, that was my only nit.  Other than that, it looks good to me.  For 
cpu_cooling.c 

Acked-by: Javi Merino 

Re: [RESEND PATCH v4 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Fri, May 27, 2016 at 04:36:44PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v4:
> - Missing the lock added in v3.
> 
> Changes in v3:
> - as Javi comments on https://patchwork.kernel.org/patch/9001281/.
> - add the lock for preventing the called from multi placce
> - add the note for pre_low/high_trip.
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips.
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 56 
> +
>  include/linux/thermal.h |  7 +
>  3 files changed, 70 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index efc3f3d..75d8838 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -49,6 +49,9 @@ temperature) and throttle appropriate devices.
>   .bind: bind the thermal zone device with a thermal cooling device.
>   .unbind: unbind the thermal zone device with a thermal cooling device.
>   .get_temp: get the current temperature of the thermal zone.
> + .set_trips: set the trip points window. Whenever the current temperature
> + is updated, the trip points immediately below and above the
> + current temperature are found.
>   .get_mode: get the current mode (enabled/disabled) of the thermal zone.
>   - "enabled" means the kernel thermal management is enabled.
>   - "disabled" will prevent kernel thermal driver action upon trip 
> points
> @@ -95,6 +98,10 @@ temperature) and throttle appropriate devices.
>   get_temp:   a pointer to a function that reads the
>   sensor temperature. This is mandatory
>   callback provided by sensor driver.
> + set_trips:  a pointer to a function that sets a
> + temperature window. When this window is
> + left the driver must inform the thermal
> + core via thermal_zone_device_update.
>   get_trend:  a pointer to a function that reads the
>   sensor temperature trend.
>   set_emul_temp:  a pointer to a function that sets
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5133cd1..0591438 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,55 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> + int low = -INT_MAX;
> + int high = INT_MAX;
> + int trip_temp, hysteresis;
> + int temp = tz->temperature;
> + int i, ret;
> +
> + if (!tz->ops->set_trips)
> + return;
> +
> + for (i = 0; i < tz->trips; i++) {
> + int trip_low;
> +
> + tz->ops->get_trip_temp(tz, i, _temp);
> + tz->ops->get_trip_hyst(tz, i, );
> +
> + trip_low = trip_temp - hysteresis;

You only use the hysteresis for the low trip point and not for the
high trip point.  Shouldn't you also add:

trip_high = trip_temp + hysteresis;

...

> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip_temp > temp && trip_temp < high)
> + high = trip_temp;

... and here:

if (trip_high > temp && trip_high < high)
high = trip_high;

> + }
> +
> + /* No need to change trip points */
> + if 

Re: [RESEND PATCH v4 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Fri, May 27, 2016 at 04:36:44PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v4:
> - Missing the lock added in v3.
> 
> Changes in v3:
> - as Javi comments on https://patchwork.kernel.org/patch/9001281/.
> - add the lock for preventing the called from multi placce
> - add the note for pre_low/high_trip.
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips.
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 56 
> +
>  include/linux/thermal.h |  7 +
>  3 files changed, 70 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index efc3f3d..75d8838 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -49,6 +49,9 @@ temperature) and throttle appropriate devices.
>   .bind: bind the thermal zone device with a thermal cooling device.
>   .unbind: unbind the thermal zone device with a thermal cooling device.
>   .get_temp: get the current temperature of the thermal zone.
> + .set_trips: set the trip points window. Whenever the current temperature
> + is updated, the trip points immediately below and above the
> + current temperature are found.
>   .get_mode: get the current mode (enabled/disabled) of the thermal zone.
>   - "enabled" means the kernel thermal management is enabled.
>   - "disabled" will prevent kernel thermal driver action upon trip 
> points
> @@ -95,6 +98,10 @@ temperature) and throttle appropriate devices.
>   get_temp:   a pointer to a function that reads the
>   sensor temperature. This is mandatory
>   callback provided by sensor driver.
> + set_trips:  a pointer to a function that sets a
> + temperature window. When this window is
> + left the driver must inform the thermal
> + core via thermal_zone_device_update.
>   get_trend:  a pointer to a function that reads the
>   sensor temperature trend.
>   set_emul_temp:  a pointer to a function that sets
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5133cd1..0591438 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,55 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> + int low = -INT_MAX;
> + int high = INT_MAX;
> + int trip_temp, hysteresis;
> + int temp = tz->temperature;
> + int i, ret;
> +
> + if (!tz->ops->set_trips)
> + return;
> +
> + for (i = 0; i < tz->trips; i++) {
> + int trip_low;
> +
> + tz->ops->get_trip_temp(tz, i, _temp);
> + tz->ops->get_trip_hyst(tz, i, );
> +
> + trip_low = trip_temp - hysteresis;

You only use the hysteresis for the low trip point and not for the
high trip point.  Shouldn't you also add:

trip_high = trip_temp + hysteresis;

...

> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip_temp > temp && trip_temp < high)
> + high = trip_temp;

... and here:

if (trip_high > temp && trip_high < high)
high = trip_high;

> + }
> +
> + /* No need to change trip points */
> + if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> + return;
> +
> + mutex_lock(>lock);

I think you should get 

Re: [PATCH V2 2/6] cpufreq: Remove cpufreq_frequency_get_table()

[Javi Merino]

Hi Viresh,

On Thu, Jun 02, 2016 at 07:34:56PM +0530, Viresh Kumar wrote:
> Most of the callers of cpufreq_frequency_get_table() already have the
> pointer to a valid 'policy' structure and they don't really need to go
> through the per-cpu variable first and then a check to validate the
> frequency, in order to find the freq-table for the policy.
> 
> Directly use the policy->freq_table field instead for them.
> 
> Only one user of that API is left after above changes, cpu_cooling.c and
> it accesses the freq_table in a racy way as the policy can get freed in
> between.
> 
> Fix it by using cpufreq_cpu_get() properly.

In 5a31d594a973 ("cpufreq: Allow freq_table to be obtained for offline
CPUs") you did the opposite: don't use cpufreq_cpu_get_raw() because
it won't give you the policy of a cpu that is offline.  Now you are
arguing that we should go back to cpufreq_cpu_get() which implicitly
calls cpufreq_cpu_get_raw().  Won't we hit the same issue that
5a31d594a973 was trying to prevent: that we can't get a freq_table for
a cpu that is offline?

Cheers,
Javi

> Signed-off-by: Viresh Kumar 
> ---
>  drivers/cpufreq/cpufreq.c | 38 
> +--
>  drivers/cpufreq/cpufreq_ondemand.c|  2 +-
>  drivers/cpufreq/cpufreq_stats.c   |  3 +--
>  drivers/cpufreq/freq_table.c  |  9 +++--
>  drivers/cpufreq/ppc_cbe_cpufreq_pmi.c |  3 +--
>  drivers/thermal/cpu_cooling.c | 22 +++-
>  include/linux/cpufreq.h   |  2 --
>  7 files changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index fbc97b1fa371..1833eda1f9d4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -126,15 +126,6 @@ struct kobject *get_governor_parent_kobj(struct 
> cpufreq_policy *policy)
>  }
>  EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
>  
> -struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> -{
> - struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> -
> - return policy && !policy_is_inactive(policy) ?
> - policy->freq_table : NULL;
> -}
> -EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> -
>  static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
>  {
>   u64 idle_time;
> @@ -1950,7 +1941,7 @@ int __cpufreq_driver_target(struct cpufreq_policy 
> *policy,
>   if (!cpufreq_driver->target_index)
>   return -EINVAL;
>  
> - freq_table = cpufreq_frequency_get_table(policy->cpu);
> + freq_table = policy->freq_table;
>   if (unlikely(!freq_table)) {
>   pr_err("%s: Unable to find freq_table\n", __func__);
>   return -EINVAL;
> @@ -2345,26 +2336,25 @@ static struct notifier_block __refdata 
> cpufreq_cpu_notifier = {
>   */
>  static int cpufreq_boost_set_sw(int state)
>  {
> - struct cpufreq_frequency_table *freq_table;
>   struct cpufreq_policy *policy;
>   int ret = -EINVAL;
>  
>   for_each_active_policy(policy) {
> - freq_table = cpufreq_frequency_get_table(policy->cpu);
> - if (freq_table) {
> - ret = cpufreq_frequency_table_cpuinfo(policy,
> - freq_table);
> - if (ret) {
> - pr_err("%s: Policy frequency update failed\n",
> -__func__);
> - break;
> - }
> + if (!policy->freq_table)
> + continue;
>  
> - down_write(>rwsem);
> - policy->user_policy.max = policy->max;
> - cpufreq_governor_limits(policy);
> - up_write(>rwsem);
> + ret = cpufreq_frequency_table_cpuinfo(policy,
> +   policy->freq_table);
> + if (ret) {
> + pr_err("%s: Policy frequency update failed\n",
> +__func__);
> + break;
>   }
> +
> + down_write(>rwsem);
> + policy->user_policy.max = policy->max;
> + cpufreq_governor_limits(policy);
> + up_write(>rwsem);
>   }
>  
>   return ret;
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
> b/drivers/cpufreq/cpufreq_ondemand.c
> index c84fc2240d49..4d2fe2710c5d 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -113,7 +113,7 @@ static void ondemand_powersave_bias_init(struct 
> cpufreq_policy *policy)
>  {
>   struct od_policy_dbs_info *dbs_info = 
> to_dbs_info(policy->governor_data);
>  
> - dbs_info->freq_table = cpufreq_frequency_get_table(policy->cpu);
> + dbs_info->freq_table = policy->freq_table;
>   

Re: [PATCH V2 2/6] cpufreq: Remove cpufreq_frequency_get_table()

[Javi Merino]

Hi Viresh,

On Thu, Jun 02, 2016 at 07:34:56PM +0530, Viresh Kumar wrote:
> Most of the callers of cpufreq_frequency_get_table() already have the
> pointer to a valid 'policy' structure and they don't really need to go
> through the per-cpu variable first and then a check to validate the
> frequency, in order to find the freq-table for the policy.
> 
> Directly use the policy->freq_table field instead for them.
> 
> Only one user of that API is left after above changes, cpu_cooling.c and
> it accesses the freq_table in a racy way as the policy can get freed in
> between.
> 
> Fix it by using cpufreq_cpu_get() properly.

In 5a31d594a973 ("cpufreq: Allow freq_table to be obtained for offline
CPUs") you did the opposite: don't use cpufreq_cpu_get_raw() because
it won't give you the policy of a cpu that is offline.  Now you are
arguing that we should go back to cpufreq_cpu_get() which implicitly
calls cpufreq_cpu_get_raw().  Won't we hit the same issue that
5a31d594a973 was trying to prevent: that we can't get a freq_table for
a cpu that is offline?

Cheers,
Javi

> Signed-off-by: Viresh Kumar 
> ---
>  drivers/cpufreq/cpufreq.c | 38 
> +--
>  drivers/cpufreq/cpufreq_ondemand.c|  2 +-
>  drivers/cpufreq/cpufreq_stats.c   |  3 +--
>  drivers/cpufreq/freq_table.c  |  9 +++--
>  drivers/cpufreq/ppc_cbe_cpufreq_pmi.c |  3 +--
>  drivers/thermal/cpu_cooling.c | 22 +++-
>  include/linux/cpufreq.h   |  2 --
>  7 files changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index fbc97b1fa371..1833eda1f9d4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -126,15 +126,6 @@ struct kobject *get_governor_parent_kobj(struct 
> cpufreq_policy *policy)
>  }
>  EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
>  
> -struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> -{
> - struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> -
> - return policy && !policy_is_inactive(policy) ?
> - policy->freq_table : NULL;
> -}
> -EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> -
>  static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
>  {
>   u64 idle_time;
> @@ -1950,7 +1941,7 @@ int __cpufreq_driver_target(struct cpufreq_policy 
> *policy,
>   if (!cpufreq_driver->target_index)
>   return -EINVAL;
>  
> - freq_table = cpufreq_frequency_get_table(policy->cpu);
> + freq_table = policy->freq_table;
>   if (unlikely(!freq_table)) {
>   pr_err("%s: Unable to find freq_table\n", __func__);
>   return -EINVAL;
> @@ -2345,26 +2336,25 @@ static struct notifier_block __refdata 
> cpufreq_cpu_notifier = {
>   */
>  static int cpufreq_boost_set_sw(int state)
>  {
> - struct cpufreq_frequency_table *freq_table;
>   struct cpufreq_policy *policy;
>   int ret = -EINVAL;
>  
>   for_each_active_policy(policy) {
> - freq_table = cpufreq_frequency_get_table(policy->cpu);
> - if (freq_table) {
> - ret = cpufreq_frequency_table_cpuinfo(policy,
> - freq_table);
> - if (ret) {
> - pr_err("%s: Policy frequency update failed\n",
> -__func__);
> - break;
> - }
> + if (!policy->freq_table)
> + continue;
>  
> - down_write(>rwsem);
> - policy->user_policy.max = policy->max;
> - cpufreq_governor_limits(policy);
> - up_write(>rwsem);
> + ret = cpufreq_frequency_table_cpuinfo(policy,
> +   policy->freq_table);
> + if (ret) {
> + pr_err("%s: Policy frequency update failed\n",
> +__func__);
> + break;
>   }
> +
> + down_write(>rwsem);
> + policy->user_policy.max = policy->max;
> + cpufreq_governor_limits(policy);
> + up_write(>rwsem);
>   }
>  
>   return ret;
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
> b/drivers/cpufreq/cpufreq_ondemand.c
> index c84fc2240d49..4d2fe2710c5d 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -113,7 +113,7 @@ static void ondemand_powersave_bias_init(struct 
> cpufreq_policy *policy)
>  {
>   struct od_policy_dbs_info *dbs_info = 
> to_dbs_info(policy->governor_data);
>  
> - dbs_info->freq_table = cpufreq_frequency_get_table(policy->cpu);
> + dbs_info->freq_table = policy->freq_table;
>   dbs_info->freq_lo = 0;
>  }
>  
> diff 

Re: [PATCH v3 2/5] thermal: of: implement .set_trips for device tree thermal zones

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:47:46AM +0800, Caesar Wang wrote:
> From: Sascha Hauer <s.ha...@pengutronix.de>
> 
> This patch implemnets .set_trips for device tree thermal zones.
 ^
 implements

> As the hardware-tracked trip points is supported by thermal core patch[0].
> 
> patch[0]
> "thermal: Add support for hardware-tracked trip points".
> 
> Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
> Signed-off-by: Caesar Wang <w...@rock-chips.com>
> Cc: Zhang Rui <rui.zh...@intel.com>
> Cc: Eduardo Valentin <edubez...@gmail.com>

Other than that, it looks good to me.  You can add my

Reviewed-by: Javi Merino <javi.mer...@arm.com>

Re: [PATCH v3 2/5] thermal: of: implement .set_trips for device tree thermal zones

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:47:46AM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This patch implemnets .set_trips for device tree thermal zones.
 ^
 implements

> As the hardware-tracked trip points is supported by thermal core patch[0].
> 
> patch[0]
> "thermal: Add support for hardware-tracked trip points".
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 

Other than that, it looks good to me.  You can add my

Reviewed-by: Javi Merino 

Re: [PATCH v2 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:27:24AM +0800, Caesar Wang wrote:
> On 2016年05月24日 20:57, Javi Merino wrote:
> >On Tue, May 03, 2016 at 05:33:29PM +0800, Caesar Wang wrote:
> >>From: Sascha Hauer <s.ha...@pengutronix.de>
> >>
> >>This adds support for hardware-tracked trip points to the device tree
> >>thermal sensor framework.
> >>
> >>The framework supports an arbitrary number of trip points. Whenever
> >>the current temperature is updated, the trip points immediately
> >>below and above the current temperature are found. A .set_trips
> >>callback is then called with the temperatures. If there is no trip
> >>point above or below the current temperature, the passed trip
> >>temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> >>the driver should program the hardware such that it is notified
> >>when either of these trip points are triggered. When a trip point
> >>is triggered, the driver should call `thermal_zone_device_update'
> >>for the respective thermal zone. This will cause the trip points
> >>to be updated again.
> >>
> >>If .set_trips is not implemented, the framework behaves as before.
> >>
> >>This patch is based on an earlier version from Mikko Perttunen
> >><mikko.perttu...@kapsi.fi>
> >>
> >>Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
> >>Signed-off-by: Caesar Wang <w...@rock-chips.com>
> >>Cc: Zhang Rui <rui.zh...@intel.com>
> >>Cc: Eduardo Valentin <edubez...@gmail.com>
> >>Cc: linux...@vger.kernel.org
> >>
> >>---
> >>
> >>Changes in v2:
> >>- update the sysfs-api.txt for set_trips
> >>
> >>  Documentation/thermal/sysfs-api.txt |  7 +
> >>  drivers/thermal/thermal_core.c  | 52 
> >> +
> >>  include/linux/thermal.h |  3 +++
> >>  3 files changed, 62 insertions(+)
> 
> >>+   /*
> >>+* Set a temperature window. When this window is left the driver
> >>+* must inform the thermal core via thermal_zone_device_update.
> >>+*/
> >>+   ret = tz->ops->set_trips(tz, low, high);
> >>+   if (ret)
> >>+   dev_err(>device, "Failed to set trips: %d\n", ret);
> >This function can be called at the same time from multiple places so
> >it should be reentrant.  I think you should call mutex_lock(tz->lock)
> >before "if (tz->prev_low_trip == low && ..." and unlock it here.
> 
> Sound reasonable, fixes it in next version.
> 
> >>+}
> >>+
> >>  static void update_temperature(struct thermal_zone_device *tz)
> >>  {
> >>int temp, ret;
> >>@@ -569,6 +614,8 @@ void thermal_zone_device_update(struct 
> >>thermal_zone_device *tz)
> >>update_temperature(tz);
> >>+   thermal_zone_set_trips(tz);
> >>+
> >>for (count = 0; count < tz->trips; count++)
> >>handle_thermal_trip(tz, count);
> >>  }
> >>@@ -754,6 +801,9 @@ trip_point_hyst_store(struct device *dev, struct 
> >>device_attribute *attr,
> >> */
> >>ret = tz->ops->set_trip_hyst(tz, trip, temperature);
> >>+   if (!ret)
> >>+   thermal_zone_set_trips(tz);
> >>+
> >You should add a similar call to thermal_zone_set_trips() in 
> >trip_point_temp_store()
> 
> No, this patch has been done.
> 
> if you see the linux next kernel.
> 72f3ada UPSTREAM: thermal: trip_point_temp_store() calls
> thermal_zone_device_update()
> 
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -733,8 +733,12 @@ trip_point_temp_store(struct device *dev,
> struct device_attribute *attr,
> return -EINVAL;
> 
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
> + if (ret)
> + return ret;
> 
> - return ret ? ret : count;
> + thermal_zone_device_update(tz);
> +
> + return count;
> }
> 
> So the "thermal_zone_set_trips(tz)" have been set in
> thermal_zone_device_update.

Ah, right, I missed that.  Thanks!
Javi

Re: [PATCH v2 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:27:24AM +0800, Caesar Wang wrote:
> On 2016年05月24日 20:57, Javi Merino wrote:
> >On Tue, May 03, 2016 at 05:33:29PM +0800, Caesar Wang wrote:
> >>From: Sascha Hauer 
> >>
> >>This adds support for hardware-tracked trip points to the device tree
> >>thermal sensor framework.
> >>
> >>The framework supports an arbitrary number of trip points. Whenever
> >>the current temperature is updated, the trip points immediately
> >>below and above the current temperature are found. A .set_trips
> >>callback is then called with the temperatures. If there is no trip
> >>point above or below the current temperature, the passed trip
> >>temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> >>the driver should program the hardware such that it is notified
> >>when either of these trip points are triggered. When a trip point
> >>is triggered, the driver should call `thermal_zone_device_update'
> >>for the respective thermal zone. This will cause the trip points
> >>to be updated again.
> >>
> >>If .set_trips is not implemented, the framework behaves as before.
> >>
> >>This patch is based on an earlier version from Mikko Perttunen
> >>
> >>
> >>Signed-off-by: Sascha Hauer 
> >>Signed-off-by: Caesar Wang 
> >>Cc: Zhang Rui 
> >>Cc: Eduardo Valentin 
> >>Cc: linux...@vger.kernel.org
> >>
> >>---
> >>
> >>Changes in v2:
> >>- update the sysfs-api.txt for set_trips
> >>
> >>  Documentation/thermal/sysfs-api.txt |  7 +
> >>  drivers/thermal/thermal_core.c  | 52 
> >> +
> >>  include/linux/thermal.h |  3 +++
> >>  3 files changed, 62 insertions(+)
> 
> >>+   /*
> >>+* Set a temperature window. When this window is left the driver
> >>+* must inform the thermal core via thermal_zone_device_update.
> >>+*/
> >>+   ret = tz->ops->set_trips(tz, low, high);
> >>+   if (ret)
> >>+   dev_err(>device, "Failed to set trips: %d\n", ret);
> >This function can be called at the same time from multiple places so
> >it should be reentrant.  I think you should call mutex_lock(tz->lock)
> >before "if (tz->prev_low_trip == low && ..." and unlock it here.
> 
> Sound reasonable, fixes it in next version.
> 
> >>+}
> >>+
> >>  static void update_temperature(struct thermal_zone_device *tz)
> >>  {
> >>int temp, ret;
> >>@@ -569,6 +614,8 @@ void thermal_zone_device_update(struct 
> >>thermal_zone_device *tz)
> >>update_temperature(tz);
> >>+   thermal_zone_set_trips(tz);
> >>+
> >>for (count = 0; count < tz->trips; count++)
> >>handle_thermal_trip(tz, count);
> >>  }
> >>@@ -754,6 +801,9 @@ trip_point_hyst_store(struct device *dev, struct 
> >>device_attribute *attr,
> >> */
> >>ret = tz->ops->set_trip_hyst(tz, trip, temperature);
> >>+   if (!ret)
> >>+   thermal_zone_set_trips(tz);
> >>+
> >You should add a similar call to thermal_zone_set_trips() in 
> >trip_point_temp_store()
> 
> No, this patch has been done.
> 
> if you see the linux next kernel.
> 72f3ada UPSTREAM: thermal: trip_point_temp_store() calls
> thermal_zone_device_update()
> 
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -733,8 +733,12 @@ trip_point_temp_store(struct device *dev,
> struct device_attribute *attr,
> return -EINVAL;
> 
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
> + if (ret)
> + return ret;
> 
> - return ret ? ret : count;
> + thermal_zone_device_update(tz);
> +
> + return count;
> }
> 
> So the "thermal_zone_set_trips(tz)" have been set in
> thermal_zone_device_update.

Ah, right, I missed that.  Thanks!
Javi

Re: [PATCH v3 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:47:45AM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v3:
> - as Javi comments on https://patchwork.kernel.org/patch/9001281/.
> - add the lock for preventing the called from multi placce
> - add the note for pre_low/high_trip.
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips.
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 52 
> +
>  include/linux/thermal.h |  7 +
>  3 files changed, 66 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index efc3f3d..75d8838 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -49,6 +49,9 @@ temperature) and throttle appropriate devices.
>   .bind: bind the thermal zone device with a thermal cooling device.
>   .unbind: unbind the thermal zone device with a thermal cooling device.
>   .get_temp: get the current temperature of the thermal zone.
> + .set_trips: set the trip points window. Whenever the current temperature
> + is updated, the trip points immediately below and above the
> + current temperature are found.
>   .get_mode: get the current mode (enabled/disabled) of the thermal zone.
>   - "enabled" means the kernel thermal management is enabled.
>   - "disabled" will prevent kernel thermal driver action upon trip 
> points
> @@ -95,6 +98,10 @@ temperature) and throttle appropriate devices.
>   get_temp:   a pointer to a function that reads the
>   sensor temperature. This is mandatory
>   callback provided by sensor driver.
> + set_trips:  a pointer to a function that sets a
> + temperature window. When this window is
> + left the driver must inform the thermal
> + core via thermal_zone_device_update.
>   get_trend:  a pointer to a function that reads the
>   sensor temperature trend.
>   set_emul_temp:  a pointer to a function that sets
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5133cd1..e5bfbd3 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,51 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> + int low = -INT_MAX;
> + int high = INT_MAX;
> + int trip_temp, hysteresis;
> + int temp = tz->temperature;
> + int i, ret;
> +
> + if (!tz->ops->set_trips)
> + return;
> +
> + for (i = 0; i < tz->trips; i++) {
> + int trip_low;
> +
> + tz->ops->get_trip_temp(tz, i, _temp);
> + tz->ops->get_trip_hyst(tz, i, );
> +
> + trip_low = trip_temp - hysteresis;
> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip_temp > temp && trip_temp < high)
> + high = trip_temp;
> + }
> +
> + /* No need to change trip points */
> + if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> + return;
> +
> + tz->prev_low_trip = low;
> + tz->prev_high_trip = high;
> +
> + dev_dbg(>device, "new temperature boundaries: %d < x < %d\n",
> + low, high);
> +
> + /*
> +  * Set a temperature window. When this window is left 

Re: [PATCH v3 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:47:45AM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v3:
> - as Javi comments on https://patchwork.kernel.org/patch/9001281/.
> - add the lock for preventing the called from multi placce
> - add the note for pre_low/high_trip.
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips.
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 52 
> +
>  include/linux/thermal.h |  7 +
>  3 files changed, 66 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index efc3f3d..75d8838 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -49,6 +49,9 @@ temperature) and throttle appropriate devices.
>   .bind: bind the thermal zone device with a thermal cooling device.
>   .unbind: unbind the thermal zone device with a thermal cooling device.
>   .get_temp: get the current temperature of the thermal zone.
> + .set_trips: set the trip points window. Whenever the current temperature
> + is updated, the trip points immediately below and above the
> + current temperature are found.
>   .get_mode: get the current mode (enabled/disabled) of the thermal zone.
>   - "enabled" means the kernel thermal management is enabled.
>   - "disabled" will prevent kernel thermal driver action upon trip 
> points
> @@ -95,6 +98,10 @@ temperature) and throttle appropriate devices.
>   get_temp:   a pointer to a function that reads the
>   sensor temperature. This is mandatory
>   callback provided by sensor driver.
> + set_trips:  a pointer to a function that sets a
> + temperature window. When this window is
> + left the driver must inform the thermal
> + core via thermal_zone_device_update.
>   get_trend:  a pointer to a function that reads the
>   sensor temperature trend.
>   set_emul_temp:  a pointer to a function that sets
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5133cd1..e5bfbd3 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,51 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> + int low = -INT_MAX;
> + int high = INT_MAX;
> + int trip_temp, hysteresis;
> + int temp = tz->temperature;
> + int i, ret;
> +
> + if (!tz->ops->set_trips)
> + return;
> +
> + for (i = 0; i < tz->trips; i++) {
> + int trip_low;
> +
> + tz->ops->get_trip_temp(tz, i, _temp);
> + tz->ops->get_trip_hyst(tz, i, );
> +
> + trip_low = trip_temp - hysteresis;
> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip_temp > temp && trip_temp < high)
> + high = trip_temp;
> + }
> +
> + /* No need to change trip points */
> + if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> + return;
> +
> + tz->prev_low_trip = low;
> + tz->prev_high_trip = high;
> +
> + dev_dbg(>device, "new temperature boundaries: %d < x < %d\n",
> + low, high);
> +
> + /*
> +  * Set a temperature window. When this window is left the driver
> +  * must inform the thermal core via thermal_zone_device_update.
> +  */
> + ret = tz->ops->set_trips(tz, low, 

Re: [PATCH v2 4/5] thermal: bang-bang governor: act on lower trip boundary

[Javi Merino]

Ccing Peter Feuerer, author of the bang bang governor.

On Tue, May 03, 2016 at 05:33:32PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> With interrupt driven thermal zones we pass the lower and upper
> temperature on which shall be acted, so in the governor we have to act on
> the exact lower temperature to be consistent. Otherwise an interrupt maybe
> generated on the exact lower temperature, but the bang bang governor does
> not react since The polling driven zones have to be one step cooler before
> the governor reacts.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - Update the commit for patch[v2 4/5].
> 
>  drivers/thermal/gov_bang_bang.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
> index 70836c5..9d1dfea 100644
> --- a/drivers/thermal/gov_bang_bang.c
> +++ b/drivers/thermal/gov_bang_bang.c
> @@ -59,7 +59,7 @@ static void thermal_zone_trip_update(struct 
> thermal_zone_device *tz, int trip)
>   if (instance->target == 0 && tz->temperature >= trip_temp)
>   instance->target = 1;
>   else if (instance->target == 1 &&
> - tz->temperature < trip_temp - trip_hyst)
> + tz->temperature <= trip_temp - trip_hyst)
>   instance->target = 0;
>  
>   dev_dbg(>cdev->device, "target=%d\n",
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 4/5] thermal: bang-bang governor: act on lower trip boundary

[Javi Merino]

Ccing Peter Feuerer, author of the bang bang governor.

On Tue, May 03, 2016 at 05:33:32PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> With interrupt driven thermal zones we pass the lower and upper
> temperature on which shall be acted, so in the governor we have to act on
> the exact lower temperature to be consistent. Otherwise an interrupt maybe
> generated on the exact lower temperature, but the bang bang governor does
> not react since The polling driven zones have to be one step cooler before
> the governor reacts.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - Update the commit for patch[v2 4/5].
> 
>  drivers/thermal/gov_bang_bang.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
> index 70836c5..9d1dfea 100644
> --- a/drivers/thermal/gov_bang_bang.c
> +++ b/drivers/thermal/gov_bang_bang.c
> @@ -59,7 +59,7 @@ static void thermal_zone_trip_update(struct 
> thermal_zone_device *tz, int trip)
>   if (instance->target == 0 && tz->temperature >= trip_temp)
>   instance->target = 1;
>   else if (instance->target == 1 &&
> - tz->temperature < trip_temp - trip_hyst)
> + tz->temperature <= trip_temp - trip_hyst)
>   instance->target = 0;
>  
>   dev_dbg(>cdev->device, "target=%d\n",
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 2/5] thermal: of: implement .set_trips for device tree thermal zones

[Javi Merino]

On Tue, May 03, 2016 at 05:33:30PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This patch implemnets .set_trips for device tree thermal zones.
> As the hardware-tracked trip points is supported by thermal core patch[0].
> 
> patch[0]
> "thermal: Add support for hardware-tracked trip points".
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - add the commit in patch[v2 2/5].
> 
>  drivers/thermal/of-thermal.c | 12 
>  include/linux/thermal.h  |  4 
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b8e509c..8722e63 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -101,6 +101,17 @@ static int of_thermal_get_temp(struct 
> thermal_zone_device *tz,
>   return data->ops->get_temp(data->sensor_data, temp);
>  }
>  
> +static int of_thermal_set_trips(struct thermal_zone_device *tz,
> + int low, int high)
> +{
> + struct __thermal_zone *data = tz->devdata;
> +
> + if (!data->ops || !data->ops->set_trips)
> + return -EINVAL;
> +
> + return data->ops->set_trips(data->sensor_data, low, high);
> +}
> +
>  /**
>   * of_thermal_get_ntrips - function to export number of available trip
>   *  points.
> @@ -427,6 +438,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  
>   tzd->ops->get_temp = of_thermal_get_temp;
>   tzd->ops->get_trend = of_thermal_get_trend;
> + tzd->ops->set_trips = of_thermal_set_trips;

Why not do it only if ops->set_trips is set?  Something like

if (ops->set_trips)
tzd->ops->set_trips = of_thermal_set_trips;

Otherwise, the thermal zone core will calculate the window for all
device tree thermal zones, even if they haven't set the optional
set_trips pointer.

Cheers,
Javi

>   tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
>   mutex_unlock(>lock);
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e258359..09053eb 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -336,12 +336,16 @@ struct thermal_genl_event {
>   *
>   * Optional:
>   * @get_trend: a pointer to a function that reads the sensor temperature 
> trend.
> + * @set_trips: a pointer to a function that sets a temperature window. When
> + *  this window is left the driver must inform the thermal core via
> + *  thermal_zone_device_update.
>   * @set_emul_temp: a pointer to a function that sets sensor emulated
>   *  temperature.
>   */
>  struct thermal_zone_of_device_ops {
>   int (*get_temp)(void *, int *);
>   int (*get_trend)(void *, long *);
> + int (*set_trips)(void *, int, int);
>   int (*set_emul_temp)(void *, int);
>   int (*set_trip_temp)(void *, int, int);
>  };
> -- 
> 1.9.1
> 

Re: [PATCH v2 2/5] thermal: of: implement .set_trips for device tree thermal zones

[Javi Merino]

On Tue, May 03, 2016 at 05:33:30PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This patch implemnets .set_trips for device tree thermal zones.
> As the hardware-tracked trip points is supported by thermal core patch[0].
> 
> patch[0]
> "thermal: Add support for hardware-tracked trip points".
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - add the commit in patch[v2 2/5].
> 
>  drivers/thermal/of-thermal.c | 12 
>  include/linux/thermal.h  |  4 
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b8e509c..8722e63 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -101,6 +101,17 @@ static int of_thermal_get_temp(struct 
> thermal_zone_device *tz,
>   return data->ops->get_temp(data->sensor_data, temp);
>  }
>  
> +static int of_thermal_set_trips(struct thermal_zone_device *tz,
> + int low, int high)
> +{
> + struct __thermal_zone *data = tz->devdata;
> +
> + if (!data->ops || !data->ops->set_trips)
> + return -EINVAL;
> +
> + return data->ops->set_trips(data->sensor_data, low, high);
> +}
> +
>  /**
>   * of_thermal_get_ntrips - function to export number of available trip
>   *  points.
> @@ -427,6 +438,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  
>   tzd->ops->get_temp = of_thermal_get_temp;
>   tzd->ops->get_trend = of_thermal_get_trend;
> + tzd->ops->set_trips = of_thermal_set_trips;

Why not do it only if ops->set_trips is set?  Something like

if (ops->set_trips)
tzd->ops->set_trips = of_thermal_set_trips;

Otherwise, the thermal zone core will calculate the window for all
device tree thermal zones, even if they haven't set the optional
set_trips pointer.

Cheers,
Javi

>   tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
>   mutex_unlock(>lock);
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e258359..09053eb 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -336,12 +336,16 @@ struct thermal_genl_event {
>   *
>   * Optional:
>   * @get_trend: a pointer to a function that reads the sensor temperature 
> trend.
> + * @set_trips: a pointer to a function that sets a temperature window. When
> + *  this window is left the driver must inform the thermal core via
> + *  thermal_zone_device_update.
>   * @set_emul_temp: a pointer to a function that sets sensor emulated
>   *  temperature.
>   */
>  struct thermal_zone_of_device_ops {
>   int (*get_temp)(void *, int *);
>   int (*get_trend)(void *, long *);
> + int (*set_trips)(void *, int, int);
>   int (*set_emul_temp)(void *, int);
>   int (*set_trip_temp)(void *, int, int);
>  };
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

On Tue, May 03, 2016 at 05:33:29PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 52 
> +
>  include/linux/thermal.h |  3 +++
>  3 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index efc3f3d..75d8838 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -49,6 +49,9 @@ temperature) and throttle appropriate devices.
>   .bind: bind the thermal zone device with a thermal cooling device.
>   .unbind: unbind the thermal zone device with a thermal cooling device.
>   .get_temp: get the current temperature of the thermal zone.
> + .set_trips: set the trip points window. Whenever the current temperature
> + is updated, the trip points immediately below and above the
> + current temperature are found.
>   .get_mode: get the current mode (enabled/disabled) of the thermal zone.
>   - "enabled" means the kernel thermal management is enabled.
>   - "disabled" will prevent kernel thermal driver action upon trip 
> points
> @@ -95,6 +98,10 @@ temperature) and throttle appropriate devices.
>   get_temp:   a pointer to a function that reads the
>   sensor temperature. This is mandatory
>   callback provided by sensor driver.
> + set_trips:  a pointer to a function that sets a
> + temperature window. When this window is
> + left the driver must inform the thermal
> + core via thermal_zone_device_update.
>   get_trend:  a pointer to a function that reads the
>   sensor temperature trend.
>   set_emul_temp:  a pointer to a function that sets
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5133cd1..e5bfbd3 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,51 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> + int low = -INT_MAX;
> + int high = INT_MAX;
> + int trip_temp, hysteresis;
> + int temp = tz->temperature;
> + int i, ret;
> +
> + if (!tz->ops->set_trips)
> + return;
> +
> + for (i = 0; i < tz->trips; i++) {
> + int trip_low;
> +
> + tz->ops->get_trip_temp(tz, i, _temp);
> + tz->ops->get_trip_hyst(tz, i, );
> +
> + trip_low = trip_temp - hysteresis;
> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip_temp > temp && trip_temp < high)
> + high = trip_temp;
> + }
> +
> + /* No need to change trip points */
> + if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> + return;
> +
> + tz->prev_low_trip = low;
> + tz->prev_high_trip = high;
> +
> + dev_dbg(>device, "new temperature boundaries: %d < x < %d\n",
> + low, high);
> +
> + /*
> +  * Set a temperature window. When this window is left the driver
> +  * must inform the thermal core via thermal_zone_device_update.
> +  */
> + ret = tz->ops->set_trips(tz, low, high);
> + if (ret)
> + dev_err(>device, "Failed to 

Re: [PATCH v2 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

On Tue, May 03, 2016 at 05:33:29PM +0800, Caesar Wang wrote:
> From: Sascha Hauer 
> 
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A .set_trips
> callback is then called with the temperatures. If there is no trip
> point above or below the current temperature, the passed trip
> temperature will be -INT_MAX or INT_MAX respectively. In this callback,
> the driver should program the hardware such that it is notified
> when either of these trip points are triggered. When a trip point
> is triggered, the driver should call `thermal_zone_device_update'
> for the respective thermal zone. This will cause the trip points
> to be updated again.
> 
> If .set_trips is not implemented, the framework behaves as before.
> 
> This patch is based on an earlier version from Mikko Perttunen
> 
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Caesar Wang 
> Cc: Zhang Rui 
> Cc: Eduardo Valentin 
> Cc: linux...@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - update the sysfs-api.txt for set_trips
> 
>  Documentation/thermal/sysfs-api.txt |  7 +
>  drivers/thermal/thermal_core.c  | 52 
> +
>  include/linux/thermal.h |  3 +++
>  3 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index efc3f3d..75d8838 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -49,6 +49,9 @@ temperature) and throttle appropriate devices.
>   .bind: bind the thermal zone device with a thermal cooling device.
>   .unbind: unbind the thermal zone device with a thermal cooling device.
>   .get_temp: get the current temperature of the thermal zone.
> + .set_trips: set the trip points window. Whenever the current temperature
> + is updated, the trip points immediately below and above the
> + current temperature are found.
>   .get_mode: get the current mode (enabled/disabled) of the thermal zone.
>   - "enabled" means the kernel thermal management is enabled.
>   - "disabled" will prevent kernel thermal driver action upon trip 
> points
> @@ -95,6 +98,10 @@ temperature) and throttle appropriate devices.
>   get_temp:   a pointer to a function that reads the
>   sensor temperature. This is mandatory
>   callback provided by sensor driver.
> + set_trips:  a pointer to a function that sets a
> + temperature window. When this window is
> + left the driver must inform the thermal
> + core via thermal_zone_device_update.
>   get_trend:  a pointer to a function that reads the
>   sensor temperature trend.
>   set_emul_temp:  a pointer to a function that sets
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5133cd1..e5bfbd3 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -520,6 +520,51 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> + int low = -INT_MAX;
> + int high = INT_MAX;
> + int trip_temp, hysteresis;
> + int temp = tz->temperature;
> + int i, ret;
> +
> + if (!tz->ops->set_trips)
> + return;
> +
> + for (i = 0; i < tz->trips; i++) {
> + int trip_low;
> +
> + tz->ops->get_trip_temp(tz, i, _temp);
> + tz->ops->get_trip_hyst(tz, i, );
> +
> + trip_low = trip_temp - hysteresis;
> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip_temp > temp && trip_temp < high)
> + high = trip_temp;
> + }
> +
> + /* No need to change trip points */
> + if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> + return;
> +
> + tz->prev_low_trip = low;
> + tz->prev_high_trip = high;
> +
> + dev_dbg(>device, "new temperature boundaries: %d < x < %d\n",
> + low, high);
> +
> + /*
> +  * Set a temperature window. When this window is left the driver
> +  * must inform the thermal core via thermal_zone_device_update.
> +  */
> + ret = tz->ops->set_trips(tz, low, high);
> + if (ret)
> + dev_err(>device, "Failed to set trips: %d\n", ret);

This function can be called at the same time from multiple places so
it should be reentrant.  I think you 

Re: [PATCH] Syntactic and factual errors in the API document

[Javi Merino]

On Thu, Mar 31, 2016 at 08:32:26AM +, Champ, Andy wrote:
> I thought that _was_ the current kernel (not the obsolete one our chip vendor 
> supports).
> 
> Which one do you mean by current? (URL & commit ID would be great!)

You can find it in the MAINTAINERS file.  I think Jon refers to 

git://git.lwn.net/linux.git docs-next

Cheers,
Javi

> 
> From: Jonathan Corbet 
> Sent: 31 March 2016 07:43
> To: Champ, Andy
> Cc: edubez...@gmail.com; javi.mer...@arm.com; durgados...@intel.com; 
> leo@linaro.org; kapileshwar.si...@arm.com; w...@nvidia.com; 
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Syntactic and factual errors in the API document
> 
> On Tue, 22 Mar 2016 12:37:25 +
> Andy Champ  wrote:
> 
> > There are several places where the English in the document is syntactically
> > invalid, or unclear. There are also one or two factual errors.
> 
> I went to apply this one to the docs tree, but it no longer applies to
> current kernels.  Any chance of getting a respin?
> 
> Thanks,
> 
> jon
> 
> 
> 

Re: [PATCH] Syntactic and factual errors in the API document

[Javi Merino]

On Thu, Mar 31, 2016 at 08:32:26AM +, Champ, Andy wrote:
> I thought that _was_ the current kernel (not the obsolete one our chip vendor 
> supports).
> 
> Which one do you mean by current? (URL & commit ID would be great!)

You can find it in the MAINTAINERS file.  I think Jon refers to 

git://git.lwn.net/linux.git docs-next

Cheers,
Javi

> 
> From: Jonathan Corbet 
> Sent: 31 March 2016 07:43
> To: Champ, Andy
> Cc: edubez...@gmail.com; javi.mer...@arm.com; durgados...@intel.com; 
> leo@linaro.org; kapileshwar.si...@arm.com; w...@nvidia.com; 
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Syntactic and factual errors in the API document
> 
> On Tue, 22 Mar 2016 12:37:25 +
> Andy Champ  wrote:
> 
> > There are several places where the English in the document is syntactically
> > invalid, or unclear. There are also one or two factual errors.
> 
> I went to apply this one to the docs tree, but it no longer applies to
> current kernels.  Any chance of getting a respin?
> 
> Thanks,
> 
> jon
> 
> 
> 

Re: [PATCH] Syntactic and factual errors in the API document

[Javi Merino]

Hi Andy,

Thanks for improving the documentation!  One minor nit below.  Other
than that, you can add my reviewed-by.

On Tue, Mar 22, 2016 at 12:37:25PM +, Andy Champ wrote:
> There are several places where the English in the document is syntactically
> invalid, or unclear. There are also one or two factual errors.
> 
> ---
>  Documentation/thermal/sysfs-api.txt | 44 
> ++---
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index 8c745c8..5bc73ef 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -69,8 +69,8 @@ temperature) and throttle appropriate devices.
>  1.1.2 void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  
>  This interface function removes the thermal zone device.
> -It deletes the corresponding entry form /sys/class/thermal folder and
> -unbind all the thermal cooling devices it uses.
> +It deletes the corresponding entry from /sys/class/thermal folder and
> +unbinds all the thermal cooling devices it uses.
>  
>  1.2 thermal cooling device interface
>  1.2.1 struct thermal_cooling_device *thermal_cooling_device_register(char 
> *name,
> @@ -78,32 +78,32 @@ temperature) and throttle appropriate devices.
>  
>  This interface function adds a new thermal cooling device 
> (fan/processor/...)
>  to /sys/class/thermal/ folder as cooling_device[0-*]. It tries to bind 
> itself
> -to all the thermal zone devices register at the same time.
> +to all the thermal zone devices registered at the same time.
>  name: the cooling device name.
>  devdata: device private data.
>  ops: thermal cooling devices call-backs.
>   .get_max_state: get the Maximum throttle state of the cooling device.
> - .get_cur_state: get the Current throttle state of the cooling device.
> + .get_cur_state: get the Currently requested throttle state of the 
> cooling device.

Not sure about this one.  It's good that it uses the same language as
set_cur_state.  What's the problem with "Current throttle state"?

>   .set_cur_state: set the Current throttle state of the cooling device.
>  
>  1.2.2 void thermal_cooling_device_unregister(struct thermal_cooling_device 
> *cdev)
>  
> -This interface function remove the thermal cooling device.
> -It deletes the corresponding entry form /sys/class/thermal folder and
> -unbind itself from all the thermal zone devices using it.
> +This interface function removes the thermal cooling device.
> +It deletes the corresponding entry from /sys/class/thermal folder and
> +unbinds itself from all the thermal zone devices using it.
>  
>  1.3 interface for binding a thermal zone device with a thermal cooling device
>  1.3.1 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
>   int trip, struct thermal_cooling_device *cdev,
>   unsigned long upper, unsigned long lower, unsigned int weight);
>  
> -This interface function bind a thermal cooling device to the certain trip
> +This interface function binds a thermal cooling device to a particular 
> trip
>  point of a thermal zone device.
>  This function is usually called in the thermal zone device .bind 
> callback.
>  tz: the thermal zone device
>  cdev: thermal cooling device
> -trip: indicates which trip point the cooling devices is associated with
> -   in this thermal zone.
> +trip: indicates which trip point in this thermal zone the cooling device
> +  is associated with.
>  upper:the Maximum cooling state for this trip point.
>THERMAL_NO_LIMIT means no upper limit,
> and the cooling device can be in max_state.
> @@ -116,13 +116,13 @@ temperature) and throttle appropriate devices.
>  1.3.2 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
>   int trip, struct thermal_cooling_device *cdev);
>  
> -This interface function unbind a thermal cooling device from the certain
> +This interface function unbinds a thermal cooling device from a 
> particular
>  trip point of a thermal zone device. This function is usually called in
>  the thermal zone device .unbind callback.
>  tz: the thermal zone device
>  cdev: thermal cooling device
> -trip: indicates which trip point the cooling devices is associated with
> -   in this thermal zone.
> +trip: indicates which trip point in this thermal zone the cooling device
> +  is associated with.
>  
>  1.4 Thermal Zone Parameters
>  1.4.1 struct thermal_bind_params
> @@ -142,13 +142,13 @@ temperature) and throttle appropriate devices.
> this thermal zone and cdev, for a particular trip point.
> If nth bit is set, then the cdev and thermal zone are bound
> for trip point n.
> -.limits: This is an array of 

Re: [PATCH] Syntactic and factual errors in the API document

[Javi Merino]

Hi Andy,

Thanks for improving the documentation!  One minor nit below.  Other
than that, you can add my reviewed-by.

On Tue, Mar 22, 2016 at 12:37:25PM +, Andy Champ wrote:
> There are several places where the English in the document is syntactically
> invalid, or unclear. There are also one or two factual errors.
> 
> ---
>  Documentation/thermal/sysfs-api.txt | 44 
> ++---
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt 
> b/Documentation/thermal/sysfs-api.txt
> index 8c745c8..5bc73ef 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -69,8 +69,8 @@ temperature) and throttle appropriate devices.
>  1.1.2 void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  
>  This interface function removes the thermal zone device.
> -It deletes the corresponding entry form /sys/class/thermal folder and
> -unbind all the thermal cooling devices it uses.
> +It deletes the corresponding entry from /sys/class/thermal folder and
> +unbinds all the thermal cooling devices it uses.
>  
>  1.2 thermal cooling device interface
>  1.2.1 struct thermal_cooling_device *thermal_cooling_device_register(char 
> *name,
> @@ -78,32 +78,32 @@ temperature) and throttle appropriate devices.
>  
>  This interface function adds a new thermal cooling device 
> (fan/processor/...)
>  to /sys/class/thermal/ folder as cooling_device[0-*]. It tries to bind 
> itself
> -to all the thermal zone devices register at the same time.
> +to all the thermal zone devices registered at the same time.
>  name: the cooling device name.
>  devdata: device private data.
>  ops: thermal cooling devices call-backs.
>   .get_max_state: get the Maximum throttle state of the cooling device.
> - .get_cur_state: get the Current throttle state of the cooling device.
> + .get_cur_state: get the Currently requested throttle state of the 
> cooling device.

Not sure about this one.  It's good that it uses the same language as
set_cur_state.  What's the problem with "Current throttle state"?

>   .set_cur_state: set the Current throttle state of the cooling device.
>  
>  1.2.2 void thermal_cooling_device_unregister(struct thermal_cooling_device 
> *cdev)
>  
> -This interface function remove the thermal cooling device.
> -It deletes the corresponding entry form /sys/class/thermal folder and
> -unbind itself from all the thermal zone devices using it.
> +This interface function removes the thermal cooling device.
> +It deletes the corresponding entry from /sys/class/thermal folder and
> +unbinds itself from all the thermal zone devices using it.
>  
>  1.3 interface for binding a thermal zone device with a thermal cooling device
>  1.3.1 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
>   int trip, struct thermal_cooling_device *cdev,
>   unsigned long upper, unsigned long lower, unsigned int weight);
>  
> -This interface function bind a thermal cooling device to the certain trip
> +This interface function binds a thermal cooling device to a particular 
> trip
>  point of a thermal zone device.
>  This function is usually called in the thermal zone device .bind 
> callback.
>  tz: the thermal zone device
>  cdev: thermal cooling device
> -trip: indicates which trip point the cooling devices is associated with
> -   in this thermal zone.
> +trip: indicates which trip point in this thermal zone the cooling device
> +  is associated with.
>  upper:the Maximum cooling state for this trip point.
>THERMAL_NO_LIMIT means no upper limit,
> and the cooling device can be in max_state.
> @@ -116,13 +116,13 @@ temperature) and throttle appropriate devices.
>  1.3.2 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
>   int trip, struct thermal_cooling_device *cdev);
>  
> -This interface function unbind a thermal cooling device from the certain
> +This interface function unbinds a thermal cooling device from a 
> particular
>  trip point of a thermal zone device. This function is usually called in
>  the thermal zone device .unbind callback.
>  tz: the thermal zone device
>  cdev: thermal cooling device
> -trip: indicates which trip point the cooling devices is associated with
> -   in this thermal zone.
> +trip: indicates which trip point in this thermal zone the cooling device
> +  is associated with.
>  
>  1.4 Thermal Zone Parameters
>  1.4.1 struct thermal_bind_params
> @@ -142,13 +142,13 @@ temperature) and throttle appropriate devices.
> this thermal zone and cdev, for a particular trip point.
> If nth bit is set, then the cdev and thermal zone are bound
> for trip point n.
> -.limits: This is an array of 

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Javi Merino]

On Wed, Mar 02, 2016 at 07:21:53PM -0800, Eduardo Valentin wrote:
> On Mon, Jan 04, 2016 at 03:17:09PM +0100, Sascha Hauer wrote:
> > On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > > The thermal-sensor property of the thermal zone node accepts phandles to
> > > thermal sensors.  However, thermal zones can be created as an
> > > +
> > > + thermal-sensors = <_thermal _thermal _thermal>
> > 
> > This seems inconsistent. Why can a thermal zone only have multiple
> > thermal sensors when they are thermal zones themselves? Either we assume
> > that one thermal zone has a single sensor or we assume that it can have
> > multiple sensors, but this should not depend on the zone being a sub zone
> > or not.
> > 
> > I think the thermal-sensors property should always point to one or
> > multiple sensors. I see no point in "This property either points to
> > exactly one sensor or multiple other thermal zones (from which we only
> > use the temperature)"
> 
> 
> Agreed here. In fact, if we are going to allow thermal zones to be
> treated as sensors, it means there should be no limits on what you put
> of there, as long as all items have #thermal-sensor-cells. So, mixing
> one (or more) regular sensors, with other thermal zones shall be
> allowed, if we agree in this semantics.

Eduardo, thanks for the review.  There doesn't seem to be much
interest in this and I currently have no time to work on it so I am
dropping this series for the time being.  I'm happy for this to be
picked up by somebody else (or who knows, maybe I will be able to work
on it again in the future).

Javi

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Javi Merino]

On Wed, Mar 02, 2016 at 07:21:53PM -0800, Eduardo Valentin wrote:
> On Mon, Jan 04, 2016 at 03:17:09PM +0100, Sascha Hauer wrote:
> > On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > > The thermal-sensor property of the thermal zone node accepts phandles to
> > > thermal sensors.  However, thermal zones can be created as an
> > > +
> > > + thermal-sensors = <_thermal _thermal _thermal>
> > 
> > This seems inconsistent. Why can a thermal zone only have multiple
> > thermal sensors when they are thermal zones themselves? Either we assume
> > that one thermal zone has a single sensor or we assume that it can have
> > multiple sensors, but this should not depend on the zone being a sub zone
> > or not.
> > 
> > I think the thermal-sensors property should always point to one or
> > multiple sensors. I see no point in "This property either points to
> > exactly one sensor or multiple other thermal zones (from which we only
> > use the temperature)"
> 
> 
> Agreed here. In fact, if we are going to allow thermal zones to be
> treated as sensors, it means there should be no limits on what you put
> of there, as long as all items have #thermal-sensor-cells. So, mixing
> one (or more) regular sensors, with other thermal zones shall be
> allowed, if we agree in this semantics.

Eduardo, thanks for the review.  There doesn't seem to be much
interest in this and I currently have no time to work on it so I am
dropping this series for the time being.  I'm happy for this to be
picked up by somebody else (or who knows, maybe I will be able to work
on it again in the future).

Javi

Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

[Javi Merino]

On Tue, Mar 08, 2016 at 12:55:59PM -0800, Eduardo Valentin wrote:
> On Tue, Mar 08, 2016 at 09:57:43PM +0800, Leo Yan wrote:
> > Hi Eduardo,
> > 
> > On Fri, Mar 04, 2016 at 11:57:53AM +, Javi Merino wrote:
> > > On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> > > > On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > > > > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > 
> > [...]
> > 
> > > > > > The property "hysteresis" is mandatory for trip points, so if 
> > > > > > without
> > > > > > it the thermal zone cannot register successfully. But "hysteresis" 
> > > > > > is
> > > > > > ignored in the thermal subsystem and only inquired by several 
> > > > > > thermal
> > > > > > sensor drivers.
> > > > > 
> > > > > If the Linux thermal subsystem has a problem with handling 
> > > > > hysteresis, I
> > > > > would rather fix Linux code than relaxing the DT binding. Or if you
> > > > > still believe hysteresis is really optional, I would prefer to see a
> > > > > better justification than "Linux ignores it".
> > > 
> > > I see it the other way round, Is hysteresis a property that, without
> > > it, the thermal code can't configure itself so it fails to create the
> > > trip point?  The current code goes "There is no hysteresis for this
> > > property, I don't know how to set up this trip point!".  I think we
> > > can do better than this.
> > 
> > Do you agree with Javi's suggestion? If you think it's okay, I will
> > move on to send out a new version patch based on Javi's comments.
> 
> No I don't. This discussion so far has been about Linux code. I still
> havent seen an argument explaining why hysteresis has to be optional.

Fair enough.  Looks like I'm holding this driver from being
upstreamed, so I'll back off.

Leo, sorry for misguiding you.  Please bring back the hysteresis
property you had in v1.

Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

[Javi Merino]

On Tue, Mar 08, 2016 at 12:55:59PM -0800, Eduardo Valentin wrote:
> On Tue, Mar 08, 2016 at 09:57:43PM +0800, Leo Yan wrote:
> > Hi Eduardo,
> > 
> > On Fri, Mar 04, 2016 at 11:57:53AM +, Javi Merino wrote:
> > > On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> > > > On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > > > > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > 
> > [...]
> > 
> > > > > > The property "hysteresis" is mandatory for trip points, so if 
> > > > > > without
> > > > > > it the thermal zone cannot register successfully. But "hysteresis" 
> > > > > > is
> > > > > > ignored in the thermal subsystem and only inquired by several 
> > > > > > thermal
> > > > > > sensor drivers.
> > > > > 
> > > > > If the Linux thermal subsystem has a problem with handling 
> > > > > hysteresis, I
> > > > > would rather fix Linux code than relaxing the DT binding. Or if you
> > > > > still believe hysteresis is really optional, I would prefer to see a
> > > > > better justification than "Linux ignores it".
> > > 
> > > I see it the other way round, Is hysteresis a property that, without
> > > it, the thermal code can't configure itself so it fails to create the
> > > trip point?  The current code goes "There is no hysteresis for this
> > > property, I don't know how to set up this trip point!".  I think we
> > > can do better than this.
> > 
> > Do you agree with Javi's suggestion? If you think it's okay, I will
> > move on to send out a new version patch based on Javi's comments.
> 
> No I don't. This discussion so far has been about Linux code. I still
> havent seen an argument explaining why hysteresis has to be optional.

Fair enough.  Looks like I'm holding this driver from being
upstreamed, so I'll back off.

Leo, sorry for misguiding you.  Please bring back the hysteresis
property you had in v1.

Re: [PATCH] thermal: trace: migrating thermal traces to use TRACE_DEFINE_ENUM() macros

[Javi Merino]

On Tue, Mar 08, 2016 at 09:48:42AM +, Zhang, Rui wrote:
> Punit and Javi,
> 
> What do you think of this? I'd like to get your ACK before taking it.

Sorry, I didn't realize you were waiting for us.  I'm quite happy for
this to go in:

Acked-by: Javi Merino <javi.mer...@arm.com>

> >  -Original Message-
> > From: linux-pm-ow...@vger.kernel.org [mailto:linux-pm-
> > ow...@vger.kernel.org] On Behalf Of Michele DiGiorgio
> > Sent: Friday, March 04, 2016 1:11 AM
> > To: Steven Rostedt <rost...@goodmis.org>
> > Cc: linux...@vger.kernel.org; Ingo Molnar <mi...@redhat.com>; Javi
> > Merino <javi.mer...@arm.com>; linux-kernel@vger.kernel.org; Punit
> > Agrawal <punit.agra...@arm.com>; Eduardo Valentin <edubez...@gmail.com>
> > Subject: Re: [PATCH] thermal: trace: migrating thermal traces to use
> > TRACE_DEFINE_ENUM() macros
> > 
> > Hello Steven,
> > 
> > Thanks for acknowledging the patch.
> > 
> > Will you be merging it or should I refer to somebody else?
> > 
> > Kind regards,
> > Michele
> > 
> > On 01/03/16 18:20, Steven Rostedt wrote:
> > > On Tue,  1 Mar 2016 17:38:54 +
> > > Michele Di Giorgio <michele.digior...@arm.com> wrote:
> > >
> > > > Userspace tools are not aware of how to convert the enums provided
> > > > by the tracepoints to their corresponding strings.
> > > >
> > > > Adding TRACE_DEFINE_ENUM() macros allows to make the enums available
> > > > to userspace to let the tools know what those enum values represent.
> > > >
> > > > In particular, for thermal zone trip types what we obtained before
> > > > was something like:
> > > >
> > > > kworker/1:1-460   [001]   320.372732: thermal_zone_trip:
> > thermal_zone=soc
> > > > id=0 trip=1 trip_type=1
> > > >
> > > > Unfortunately, userspace tools do not know how to convert enum
> > > > values to strings and as a consequence they can only forward the
> > > > enum value to the output. By using TRACE_DEFINE_ENUM() macros for
> > > > thermal traces we get the following trace line:
> > > >
> > > > kworker/1:1-460   [001]   320.372732: thermal_zone_trip:
> > thermal_zone=soc
> > > > id=0 trip=1 trip_type=PASSIVE
> > > >
> > > > Userspace tools are now able to better understand the meaning of the
> > > > trip_type and provide the user with more readable information.
> > > >
> > >
> > > Acked-by: Steven Rostedt <rost...@goodmis.org>
> > >
> > > -- Steve
> > >
> > 
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended 
> > recipient,
> > please notify the sender immediately and do not disclose the contents to any
> > other person, use it for any purpose, or store or copy the information in 
> > any
> > medium. Thank you.
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in the 
> > body
> > of a message to majord...@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html

Re: [PATCH] thermal: trace: migrating thermal traces to use TRACE_DEFINE_ENUM() macros

[Javi Merino]

On Tue, Mar 08, 2016 at 09:48:42AM +, Zhang, Rui wrote:
> Punit and Javi,
> 
> What do you think of this? I'd like to get your ACK before taking it.

Sorry, I didn't realize you were waiting for us.  I'm quite happy for
this to go in:

Acked-by: Javi Merino 

> >  -Original Message-
> > From: linux-pm-ow...@vger.kernel.org [mailto:linux-pm-
> > ow...@vger.kernel.org] On Behalf Of Michele DiGiorgio
> > Sent: Friday, March 04, 2016 1:11 AM
> > To: Steven Rostedt 
> > Cc: linux...@vger.kernel.org; Ingo Molnar ; Javi
> > Merino ; linux-kernel@vger.kernel.org; Punit
> > Agrawal ; Eduardo Valentin 
> > Subject: Re: [PATCH] thermal: trace: migrating thermal traces to use
> > TRACE_DEFINE_ENUM() macros
> > 
> > Hello Steven,
> > 
> > Thanks for acknowledging the patch.
> > 
> > Will you be merging it or should I refer to somebody else?
> > 
> > Kind regards,
> > Michele
> > 
> > On 01/03/16 18:20, Steven Rostedt wrote:
> > > On Tue,  1 Mar 2016 17:38:54 +
> > > Michele Di Giorgio  wrote:
> > >
> > > > Userspace tools are not aware of how to convert the enums provided
> > > > by the tracepoints to their corresponding strings.
> > > >
> > > > Adding TRACE_DEFINE_ENUM() macros allows to make the enums available
> > > > to userspace to let the tools know what those enum values represent.
> > > >
> > > > In particular, for thermal zone trip types what we obtained before
> > > > was something like:
> > > >
> > > > kworker/1:1-460   [001]   320.372732: thermal_zone_trip:
> > thermal_zone=soc
> > > > id=0 trip=1 trip_type=1
> > > >
> > > > Unfortunately, userspace tools do not know how to convert enum
> > > > values to strings and as a consequence they can only forward the
> > > > enum value to the output. By using TRACE_DEFINE_ENUM() macros for
> > > > thermal traces we get the following trace line:
> > > >
> > > > kworker/1:1-460   [001]   320.372732: thermal_zone_trip:
> > thermal_zone=soc
> > > > id=0 trip=1 trip_type=PASSIVE
> > > >
> > > > Userspace tools are now able to better understand the meaning of the
> > > > trip_type and provide the user with more readable information.
> > > >
> > >
> > > Acked-by: Steven Rostedt 
> > >
> > > -- Steve
> > >
> > 
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended 
> > recipient,
> > please notify the sender immediately and do not disclose the contents to any
> > other person, use it for any purpose, or store or copy the information in 
> > any
> > medium. Thank you.
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in the 
> > body
> > of a message to majord...@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

[Javi Merino]

On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> Hi Eduardo,
> 
> On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > Hi Leo,
> > 
> > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > > The property "hysteresis" is mandatory for trip points, so if without
> > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > ignored in the thermal subsystem and only inquired by several thermal
> > > sensor drivers.
> > 
> > I am not sure this a good direction to go. Remember that Linux
> > implementation not necessarily has to be the implication of the DT
> > binding. Hysteresis is a property that plays a significant role on
> > thermal control systems, which in many cases avoid overshooting cooling
> > actions. Having the DT writer to explicitly set it to 0 means that zone
> > does not suffer of overshooting and does not need hysteresis.
> 
> After review current code, the "hysteresis" is used to calculate
> temperature falling threshold with a more conservative value; so that
> finally avoid overshooting issue.
> 
> Please confirm if is my understanding correct or not?
> 
> > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > would rather fix Linux code than relaxing the DT binding. Or if you
> > still believe hysteresis is really optional, I would prefer to see a
> > better justification than "Linux ignores it".

I see it the other way round, Is hysteresis a property that, without
it, the thermal code can't configure itself so it fails to create the
trip point?  The current code goes "There is no hysteresis for this
property, I don't know how to set up this trip point!".  I think we
can do better than this.

> If we think about power allocator governor, PID's two parameters are
> also used to dismiss overshooting issue: one is k_po (proportional
> term), another is k_i (integral term). So that means after we apply
> power allocator governor, we don't need parameter "hysteresis" due PID
> algorithm can automatically dismiss potential errors.

I disagree.  We shouldn't base DT decisions based on only one
governor in linux.  Having said that, AFAICS all governors currently
ignore hysteresis.

Cheers,
Javi

Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

[Javi Merino]

On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> Hi Eduardo,
> 
> On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > Hi Leo,
> > 
> > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > > The property "hysteresis" is mandatory for trip points, so if without
> > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > ignored in the thermal subsystem and only inquired by several thermal
> > > sensor drivers.
> > 
> > I am not sure this a good direction to go. Remember that Linux
> > implementation not necessarily has to be the implication of the DT
> > binding. Hysteresis is a property that plays a significant role on
> > thermal control systems, which in many cases avoid overshooting cooling
> > actions. Having the DT writer to explicitly set it to 0 means that zone
> > does not suffer of overshooting and does not need hysteresis.
> 
> After review current code, the "hysteresis" is used to calculate
> temperature falling threshold with a more conservative value; so that
> finally avoid overshooting issue.
> 
> Please confirm if is my understanding correct or not?
> 
> > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > would rather fix Linux code than relaxing the DT binding. Or if you
> > still believe hysteresis is really optional, I would prefer to see a
> > better justification than "Linux ignores it".

I see it the other way round, Is hysteresis a property that, without
it, the thermal code can't configure itself so it fails to create the
trip point?  The current code goes "There is no hysteresis for this
property, I don't know how to set up this trip point!".  I think we
can do better than this.

> If we think about power allocator governor, PID's two parameters are
> also used to dismiss overshooting issue: one is k_po (proportional
> term), another is k_i (integral term). So that means after we apply
> power allocator governor, we don't need parameter "hysteresis" due PID
> algorithm can automatically dismiss potential errors.

I disagree.  We shouldn't base DT decisions based on only one
governor in linux.  Having said that, AFAICS all governors currently
ignore hysteresis.

Cheers,
Javi

Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

[Javi Merino]

On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> The property "hysteresis" is mandatory for trip points, so if without
> it the thermal zone cannot register successfully. But "hysteresis" is
> ignored in the thermal subsystem and only inquired by several thermal
> sensor drivers.
> 
> So change "hysteresis" as optional properties.

I had forgotten that hysteresis is enforced in DT.  Thanks for fixing
this!
 
> Signed-off-by: Leo Yan <leo@linaro.org>
> ---
>  Documentation/devicetree/bindings/thermal/thermal.txt | 9 +
>  drivers/thermal/of-thermal.c  | 9 -
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt 
> b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 41b817f..7d79e77 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -89,10 +89,6 @@ Required properties:
>Type: signed   in millicelsius.
>Size: one cell
>  
> -- hysteresis:A low hysteresis value on temperature property 
> (above).
> -  Type: unsigned This is a relative value, in millicelsius.
> -  Size: one cell
> -
>  - type:  a string containing the trip type. Expected 
> values are:
>   "active":   A trip point to enable active cooling
>   "passive":  A trip point to enable passive cooling
> @@ -100,6 +96,11 @@ Required properties:
>   "critical": Hardware not reliable.
>Type: string
>  
> +Optional properties:
> +- hysteresis:A low hysteresis value on temperature property 
> (above).
> +  Type: unsigned This is a relative value, in millicelsius.
> +  Size: one cell
> +
>  * Cooling device maps
>  
>  The cooling device maps node is a node to describe how cooling devices
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 9043f8f..ab05500 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -689,11 +689,10 @@ static int thermal_of_populate_trip(struct device_node 
> *np,
>   trip->temperature = prop;
>  
>   ret = of_property_read_u32(np, "hysteresis", );
> - if (ret < 0) {
> - pr_err("missing hysteresis property\n");
> - return ret;
> - }
> - trip->hysteresis = prop;
> + if (ret < 0)
> + pr_warning("missing hysteresis property\n");

I'd remove the warning.  It is an optional parameter, so there is no
need to warn about something going wrong.  As you say in the commit
log, it is ignored in the thermal subsystem and only used by some
sensors so no need to warn about it missing.

Other than that, I'm happy to see this merged.

Acked-by: Javi Merino <javi.mer...@arm.com>

> + else
> + trip->hysteresis = prop;
>  
>   ret = thermal_of_get_trip_type(np, >type);
>   if (ret < 0) {
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

[Javi Merino]

On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> The property "hysteresis" is mandatory for trip points, so if without
> it the thermal zone cannot register successfully. But "hysteresis" is
> ignored in the thermal subsystem and only inquired by several thermal
> sensor drivers.
> 
> So change "hysteresis" as optional properties.

I had forgotten that hysteresis is enforced in DT.  Thanks for fixing
this!
 
> Signed-off-by: Leo Yan 
> ---
>  Documentation/devicetree/bindings/thermal/thermal.txt | 9 +
>  drivers/thermal/of-thermal.c  | 9 -
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt 
> b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 41b817f..7d79e77 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -89,10 +89,6 @@ Required properties:
>Type: signed   in millicelsius.
>Size: one cell
>  
> -- hysteresis:A low hysteresis value on temperature property 
> (above).
> -  Type: unsigned This is a relative value, in millicelsius.
> -  Size: one cell
> -
>  - type:  a string containing the trip type. Expected 
> values are:
>   "active":   A trip point to enable active cooling
>   "passive":  A trip point to enable passive cooling
> @@ -100,6 +96,11 @@ Required properties:
>   "critical": Hardware not reliable.
>Type: string
>  
> +Optional properties:
> +- hysteresis:A low hysteresis value on temperature property 
> (above).
> +  Type: unsigned This is a relative value, in millicelsius.
> +  Size: one cell
> +
>  * Cooling device maps
>  
>  The cooling device maps node is a node to describe how cooling devices
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 9043f8f..ab05500 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -689,11 +689,10 @@ static int thermal_of_populate_trip(struct device_node 
> *np,
>   trip->temperature = prop;
>  
>   ret = of_property_read_u32(np, "hysteresis", );
> - if (ret < 0) {
> - pr_err("missing hysteresis property\n");
> - return ret;
> - }
> - trip->hysteresis = prop;
> + if (ret < 0)
> + pr_warning("missing hysteresis property\n");

I'd remove the warning.  It is an optional parameter, so there is no
need to warn about something going wrong.  As you say in the commit
log, it is ignored in the thermal subsystem and only used by some
sensors so no need to warn about it missing.

Other than that, I'm happy to see this merged.

Acked-by: Javi Merino 

> + else
> + trip->hysteresis = prop;
>  
>   ret = thermal_of_get_trip_type(np, >type);
>   if (ret < 0) {
> -- 
> 1.9.1
> 

[PATCH] doc: fix grammar

[Javi Merino]

Some minor typos:

  - make is unbindable -> make it unbindable
  - a underlying -> an underlying
  - different version -> different versions

Cc: Jonathan Corbet <cor...@lwn.net>
Signed-off-by: Javi Merino <javi.mer...@arm.com>
---
 Documentation/filesystems/sharedsubtree.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/filesystems/sharedsubtree.txt 
b/Documentation/filesystems/sharedsubtree.txt
index e3f4c778eb98..8ccfbd55244b 100644
--- a/Documentation/filesystems/sharedsubtree.txt
+++ b/Documentation/filesystems/sharedsubtree.txt
@@ -123,7 +123,7 @@ replicas continue to be exactly same.
 
 2d) A unbindable mount is a unbindable private mount
 
-   let's say we have a mount at /mnt and we make is unbindable
+   let's say we have a mount at /mnt and we make it unbindable
 
# mount --make-unbindable /mnt
 
@@ -197,13 +197,13 @@ replicas continue to be exactly same.
namespaces are made first class objects with user API to
associate/disassociate a namespace with userid, then each user
could have his/her own namespace and tailor it to his/her
-   requirements. Offcourse its needs support from PAM.
+   requirements. This needs to be supported in PAM.
 
D)  Versioned files
 
If the entire mount tree is visible at multiple locations, then
-   a underlying versioning file system can return different
-   version of the file depending on the path used to access that
+   an underlying versioning file system can return different
+   versions of the file depending on the path used to access that
file.
 
An example is:
-- 
1.9.1

[PATCH] doc: fix grammar

[Javi Merino]

Some minor typos:

  - make is unbindable -> make it unbindable
  - a underlying -> an underlying
  - different version -> different versions

Cc: Jonathan Corbet 
Signed-off-by: Javi Merino 
---
 Documentation/filesystems/sharedsubtree.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/filesystems/sharedsubtree.txt 
b/Documentation/filesystems/sharedsubtree.txt
index e3f4c778eb98..8ccfbd55244b 100644
--- a/Documentation/filesystems/sharedsubtree.txt
+++ b/Documentation/filesystems/sharedsubtree.txt
@@ -123,7 +123,7 @@ replicas continue to be exactly same.
 
 2d) A unbindable mount is a unbindable private mount
 
-   let's say we have a mount at /mnt and we make is unbindable
+   let's say we have a mount at /mnt and we make it unbindable
 
# mount --make-unbindable /mnt
 
@@ -197,13 +197,13 @@ replicas continue to be exactly same.
namespaces are made first class objects with user API to
associate/disassociate a namespace with userid, then each user
could have his/her own namespace and tailor it to his/her
-   requirements. Offcourse its needs support from PAM.
+   requirements. This needs to be supported in PAM.
 
D)  Versioned files
 
If the entire mount tree is visible at multiple locations, then
-   a underlying versioning file system can return different
-   version of the file depending on the path used to access that
+   an underlying versioning file system can return different
+   versions of the file depending on the path used to access that
file.
 
An example is:
-- 
1.9.1