Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-05 Thread Daniel Lezcano 
On 05/08/2019 09:42, Martin Kepplinger wrote:
> On 05.08.19 09:39, Daniel Lezcano wrote:
>> On 05/08/2019 08:53, Martin Kepplinger wrote:
>>
>> [ ... ]
>>
> +static s64 cpuidle_cooling_runtime(struct cpuidle_cooling_device 
> *idle_cdev)
> +{
> + s64 next_wakeup;
> + unsigned long state = idle_cdev->state;
> +
> + /*
> +  * The function should not be called when there is no
> +  * mitigation because:
> +  * - that does not make sense
> +  * - we end up with a division by zero
> +  */
> + if (!state)
> + return 0;
> +
> + next_wakeup = (s64)((idle_cdev->idle_cycle * 100) / state) -
> + idle_cdev->idle_cycle;
> +
> + return next_wakeup * NSEC_PER_USEC;
> +}
> +

 There is a bug in your calculation formula here when "state" becomes 100.
 You return 0 for the injection rate, which is the same as "rate" being 0,
 which is dangerous. You stop cooling when it's most necessary :)

 I'm not sure how much sense really being 100% idle makes, so I, when 
 testing
 this, just say if (state == 100) { state = 99 }. Anyways, just don't 
 return 0.

>>>
>>> oh and also, this breaks S3 suspend:
>>
>> What breaks the S3 suspend? The idle cooling device or the bug above ?
> 
> The idle cooling device. I have to configure it out: remove
> CONFIG_CPU_IDLE_THERMAL to test suspend/resume again. Errors in the
> kernel log, see below.

Ok, thanks for reporting. I'll fix the issue.


>>> Aug  5 06:09:20 pureos kernel: [  807.487887] PM: suspend entry (deep)
>>> Aug  5 06:09:40 pureos kernel: [  807.501148] Filesystems sync: 0.013
>>> seconds
>>> Aug  5 06:09:40 pureos kernel: [  807.501591] Freezing user space
>>> processes ... (elapsed 0.003 seconds) done.
>>> Aug  5 06:09:40 pureos kernel: [  807.504741] OOM killer disabled.
>>> Aug  5 06:09:40 pureos kernel: [  807.504744] Freezing remaining
>>> freezable tasks ...
>>> Aug  5 06:09:40 pureos kernel: [  827.517712] Freezing of tasks failed
>>> after 20.002 seconds (4 tasks refusing to freeze, wq_busy=0):
>>> Aug  5 06:09:40 pureos kernel: [  827.527122] thermal-idle/0  S0
>>> 161  2 0x0028
>>> Aug  5 06:09:40 pureos kernel: [  827.527131] Call trace:
>>> Aug  5 06:09:40 pureos kernel: [  827.527148]  __switch_to+0xb4/0x200
>>> Aug  5 06:09:40 pureos kernel: [  827.527156]  __schedule+0x1e0/0x488
>>> Aug  5 06:09:40 pureos kernel: [  827.527162]  schedule+0x38/0xc8
>>> Aug  5 06:09:40 pureos kernel: [  827.527169]  smpboot_thread_fn+0x250/0x2a8
>>> Aug  5 06:09:40 pureos kernel: [  827.527176]  kthread+0xf4/0x120
>>> Aug  5 06:09:40 pureos kernel: [  827.527182]  ret_from_fork+0x10/0x18
>>> Aug  5 06:09:40 pureos kernel: [  827.527186] thermal-idle/1  S0
>>> 162  2 0x0028
>>> Aug  5 06:09:40 pureos kernel: [  827.527192] Call trace:
>>> Aug  5 06:09:40 pureos kernel: [  827.527197]  __switch_to+0x188/0x200
>>> Aug  5 06:09:40 pureos kernel: [  827.527203]  __schedule+0x1e0/0x488
>>> Aug  5 06:09:40 pureos kernel: [  827.527208]  schedule+0x38/0xc8
>>> Aug  5 06:09:40 pureos kernel: [  827.527213]  smpboot_thread_fn+0x250/0x2a8
>>> Aug  5 06:09:40 pureos kernel: [  827.527218]  kthread+0xf4/0x120
>>> Aug  5 06:09:40 pureos kernel: [  827.527222]  ret_from_fork+0x10/0x18
>>> Aug  5 06:09:40 pureos kernel: [  827.527226] thermal-idle/2  S0
>>> 163  2 0x0028
>>> Aug  5 06:09:40 pureos kernel: [  827.527231] Call trace:
>>> Aug  5 06:09:40 pureos kernel: [  827.527237]  __switch_to+0xb4/0x200
>>> Aug  5 06:09:40 pureos kernel: [  827.527242]  __schedule+0x1e0/0x488
>>> Aug  5 06:09:40 pureos kernel: [  827.527247]  schedule+0x38/0xc8
>>> Aug  5 06:09:40 pureos kernel: [  827.527259]  smpboot_thread_fn+0x250/0x2a8
>>> Aug  5 06:09:40 pureos kernel: [  827.527264]  kthread+0xf4/0x120
>>> Aug  5 06:09:40 pureos kernel: [  827.527268]  ret_from_fork+0x10/0x18
>>> Aug  5 06:09:40 pureos kernel: [  827.527272] thermal-idle/3  S0
>>> 164  2 0x0028
>>> Aug  5 06:09:40 pureos kernel: [  827.527278] Call trace:
>>> Aug  5 06:09:40 pureos kernel: [  827.527283]  __switch_to+0xb4/0x200
>>> Aug  5 06:09:40 pureos kernel: [  827.527288]  __schedule+0x1e0/0x488
>>> Aug  5 06:09:40 pureos kernel: [  827.527293]  schedule+0x38/0xc8
>>> Aug  5 06:09:40 pureos kernel: [  827.527298]  smpboot_thread_fn+0x250/0x2a8
>>> Aug  5 06:09:40 pureos kernel: [  827.527303]  kthread+0xf4/0x120
>>> Aug  5 06:09:40 pureos kernel: [  827.527308]  ret_from_fork+0x10/0x18
>>> Aug  5 06:09:40 pureos kernel: [  827.527375] Restarting kernel threads
>>> ... done.
>>> Aug  5 06:09:40 pureos kernel: [  827.527771] OOM killer enabled.
>>> Aug  5 06:09:40 pureos kernel: [  827.527772] Restarting tasks ... done.
>>> Aug  5 06:09:40 pureos kernel: [  827.528926] PM: suspend exit
>>>
>>>
>>> do you know where things might go wrong here?
>>>
>>> thanks,
>>>
>>> martin
>>>
>>
>>
> 


-- 
  Linaro.o

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-05 Thread Martin Kepplinger 
On 05.08.19 09:39, Daniel Lezcano wrote:
> On 05/08/2019 08:53, Martin Kepplinger wrote:
> 
> [ ... ]
> 
 +static s64 cpuidle_cooling_runtime(struct cpuidle_cooling_device 
 *idle_cdev)
 +{
 +  s64 next_wakeup;
 +  unsigned long state = idle_cdev->state;
 +
 +  /*
 +   * The function should not be called when there is no
 +   * mitigation because:
 +   * - that does not make sense
 +   * - we end up with a division by zero
 +   */
 +  if (!state)
 +  return 0;
 +
 +  next_wakeup = (s64)((idle_cdev->idle_cycle * 100) / state) -
 +  idle_cdev->idle_cycle;
 +
 +  return next_wakeup * NSEC_PER_USEC;
 +}
 +
>>>
>>> There is a bug in your calculation formula here when "state" becomes 100.
>>> You return 0 for the injection rate, which is the same as "rate" being 0,
>>> which is dangerous. You stop cooling when it's most necessary :)
>>>
>>> I'm not sure how much sense really being 100% idle makes, so I, when testing
>>> this, just say if (state == 100) { state = 99 }. Anyways, just don't return 
>>> 0.
>>>
>>
>> oh and also, this breaks S3 suspend:
> 
> What breaks the S3 suspend? The idle cooling device or the bug above ?

The idle cooling device. I have to configure it out: remove
CONFIG_CPU_IDLE_THERMAL to test suspend/resume again. Errors in the
kernel log, see below.


> 
>> Aug  5 06:09:20 pureos kernel: [  807.487887] PM: suspend entry (deep)
>> Aug  5 06:09:40 pureos kernel: [  807.501148] Filesystems sync: 0.013
>> seconds
>> Aug  5 06:09:40 pureos kernel: [  807.501591] Freezing user space
>> processes ... (elapsed 0.003 seconds) done.
>> Aug  5 06:09:40 pureos kernel: [  807.504741] OOM killer disabled.
>> Aug  5 06:09:40 pureos kernel: [  807.504744] Freezing remaining
>> freezable tasks ...
>> Aug  5 06:09:40 pureos kernel: [  827.517712] Freezing of tasks failed
>> after 20.002 seconds (4 tasks refusing to freeze, wq_busy=0):
>> Aug  5 06:09:40 pureos kernel: [  827.527122] thermal-idle/0  S0
>> 161  2 0x0028
>> Aug  5 06:09:40 pureos kernel: [  827.527131] Call trace:
>> Aug  5 06:09:40 pureos kernel: [  827.527148]  __switch_to+0xb4/0x200
>> Aug  5 06:09:40 pureos kernel: [  827.527156]  __schedule+0x1e0/0x488
>> Aug  5 06:09:40 pureos kernel: [  827.527162]  schedule+0x38/0xc8
>> Aug  5 06:09:40 pureos kernel: [  827.527169]  smpboot_thread_fn+0x250/0x2a8
>> Aug  5 06:09:40 pureos kernel: [  827.527176]  kthread+0xf4/0x120
>> Aug  5 06:09:40 pureos kernel: [  827.527182]  ret_from_fork+0x10/0x18
>> Aug  5 06:09:40 pureos kernel: [  827.527186] thermal-idle/1  S0
>> 162  2 0x0028
>> Aug  5 06:09:40 pureos kernel: [  827.527192] Call trace:
>> Aug  5 06:09:40 pureos kernel: [  827.527197]  __switch_to+0x188/0x200
>> Aug  5 06:09:40 pureos kernel: [  827.527203]  __schedule+0x1e0/0x488
>> Aug  5 06:09:40 pureos kernel: [  827.527208]  schedule+0x38/0xc8
>> Aug  5 06:09:40 pureos kernel: [  827.527213]  smpboot_thread_fn+0x250/0x2a8
>> Aug  5 06:09:40 pureos kernel: [  827.527218]  kthread+0xf4/0x120
>> Aug  5 06:09:40 pureos kernel: [  827.527222]  ret_from_fork+0x10/0x18
>> Aug  5 06:09:40 pureos kernel: [  827.527226] thermal-idle/2  S0
>> 163  2 0x0028
>> Aug  5 06:09:40 pureos kernel: [  827.527231] Call trace:
>> Aug  5 06:09:40 pureos kernel: [  827.527237]  __switch_to+0xb4/0x200
>> Aug  5 06:09:40 pureos kernel: [  827.527242]  __schedule+0x1e0/0x488
>> Aug  5 06:09:40 pureos kernel: [  827.527247]  schedule+0x38/0xc8
>> Aug  5 06:09:40 pureos kernel: [  827.527259]  smpboot_thread_fn+0x250/0x2a8
>> Aug  5 06:09:40 pureos kernel: [  827.527264]  kthread+0xf4/0x120
>> Aug  5 06:09:40 pureos kernel: [  827.527268]  ret_from_fork+0x10/0x18
>> Aug  5 06:09:40 pureos kernel: [  827.527272] thermal-idle/3  S0
>> 164  2 0x0028
>> Aug  5 06:09:40 pureos kernel: [  827.527278] Call trace:
>> Aug  5 06:09:40 pureos kernel: [  827.527283]  __switch_to+0xb4/0x200
>> Aug  5 06:09:40 pureos kernel: [  827.527288]  __schedule+0x1e0/0x488
>> Aug  5 06:09:40 pureos kernel: [  827.527293]  schedule+0x38/0xc8
>> Aug  5 06:09:40 pureos kernel: [  827.527298]  smpboot_thread_fn+0x250/0x2a8
>> Aug  5 06:09:40 pureos kernel: [  827.527303]  kthread+0xf4/0x120
>> Aug  5 06:09:40 pureos kernel: [  827.527308]  ret_from_fork+0x10/0x18
>> Aug  5 06:09:40 pureos kernel: [  827.527375] Restarting kernel threads
>> ... done.
>> Aug  5 06:09:40 pureos kernel: [  827.527771] OOM killer enabled.
>> Aug  5 06:09:40 pureos kernel: [  827.527772] Restarting tasks ... done.
>> Aug  5 06:09:40 pureos kernel: [  827.528926] PM: suspend exit
>>
>>
>> do you know where things might go wrong here?
>>
>> thanks,
>>
>> martin
>>
> 
>

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-05 Thread Martin Kepplinger 
On 05.08.19 09:37, Daniel Lezcano wrote:
> On 05/08/2019 07:11, Martin Kepplinger wrote:
>> ---
> 
> [ ... ]
> 
>>> +static s64 cpuidle_cooling_runtime(struct cpuidle_cooling_device 
>>> *idle_cdev)
>>> +{
>>> +   s64 next_wakeup;
>>> +   unsigned long state = idle_cdev->state;
>>> +
>>> +   /*
>>> +* The function should not be called when there is no
>>> +* mitigation because:
>>> +* - that does not make sense
>>> +* - we end up with a division by zero
>>> +*/
>>> +   if (!state)
>>> +   return 0;
>>> +
>>> +   next_wakeup = (s64)((idle_cdev->idle_cycle * 100) / state) -
>>> +   idle_cdev->idle_cycle;
>>> +
>>> +   return next_wakeup * NSEC_PER_USEC;
>>> +}
>>> +
>>
>> There is a bug in your calculation formula here when "state" becomes 100.
>> You return 0 for the injection rate, which is the same as "rate" being 0,
>> which is dangerous. You stop cooling when it's most necessary :)
> 
> Right, thanks for spotting this.
> 
>> I'm not sure how much sense really being 100% idle makes, so I, when testing
>> this, just say if (state == 100) { state = 99 }. Anyways, just don't return 
>> 0.
>>
>> Daniel, thanks a lot for these additions! Could you send an update of this?
> 
> Yes, I'm working on a new version.

great.

> 
>> btw, that's what I'm referring to:
>> https://lore.kernel.org/linux-pm/1522945005-7165-1-git-send-email-daniel.lezc...@linaro.org/
>> I know it's a little old already, but it seems like there hasn't been any
>> equivalent solution in the meantime, has it?
>>
>> Using cpuidle for cooling is way more effective than cpufreq (which often
>> hardly is).
> 
> On which platform that happens?
> 
> 

I'm running this on imx8mq.

thanks,

martin

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-05 Thread Daniel Lezcano 
On 05/08/2019 08:53, Martin Kepplinger wrote:

[ ... ]

>>> +static s64 cpuidle_cooling_runtime(struct cpuidle_cooling_device 
>>> *idle_cdev)
>>> +{
>>> +   s64 next_wakeup;
>>> +   unsigned long state = idle_cdev->state;
>>> +
>>> +   /*
>>> +* The function should not be called when there is no
>>> +* mitigation because:
>>> +* - that does not make sense
>>> +* - we end up with a division by zero
>>> +*/
>>> +   if (!state)
>>> +   return 0;
>>> +
>>> +   next_wakeup = (s64)((idle_cdev->idle_cycle * 100) / state) -
>>> +   idle_cdev->idle_cycle;
>>> +
>>> +   return next_wakeup * NSEC_PER_USEC;
>>> +}
>>> +
>>
>> There is a bug in your calculation formula here when "state" becomes 100.
>> You return 0 for the injection rate, which is the same as "rate" being 0,
>> which is dangerous. You stop cooling when it's most necessary :)
>>
>> I'm not sure how much sense really being 100% idle makes, so I, when testing
>> this, just say if (state == 100) { state = 99 }. Anyways, just don't return 
>> 0.
>>
> 
> oh and also, this breaks S3 suspend:

What breaks the S3 suspend? The idle cooling device or the bug above ?

> Aug  5 06:09:20 pureos kernel: [  807.487887] PM: suspend entry (deep)
> Aug  5 06:09:40 pureos kernel: [  807.501148] Filesystems sync: 0.013
> seconds
> Aug  5 06:09:40 pureos kernel: [  807.501591] Freezing user space
> processes ... (elapsed 0.003 seconds) done.
> Aug  5 06:09:40 pureos kernel: [  807.504741] OOM killer disabled.
> Aug  5 06:09:40 pureos kernel: [  807.504744] Freezing remaining
> freezable tasks ...
> Aug  5 06:09:40 pureos kernel: [  827.517712] Freezing of tasks failed
> after 20.002 seconds (4 tasks refusing to freeze, wq_busy=0):
> Aug  5 06:09:40 pureos kernel: [  827.527122] thermal-idle/0  S0
> 161  2 0x0028
> Aug  5 06:09:40 pureos kernel: [  827.527131] Call trace:
> Aug  5 06:09:40 pureos kernel: [  827.527148]  __switch_to+0xb4/0x200
> Aug  5 06:09:40 pureos kernel: [  827.527156]  __schedule+0x1e0/0x488
> Aug  5 06:09:40 pureos kernel: [  827.527162]  schedule+0x38/0xc8
> Aug  5 06:09:40 pureos kernel: [  827.527169]  smpboot_thread_fn+0x250/0x2a8
> Aug  5 06:09:40 pureos kernel: [  827.527176]  kthread+0xf4/0x120
> Aug  5 06:09:40 pureos kernel: [  827.527182]  ret_from_fork+0x10/0x18
> Aug  5 06:09:40 pureos kernel: [  827.527186] thermal-idle/1  S0
> 162  2 0x0028
> Aug  5 06:09:40 pureos kernel: [  827.527192] Call trace:
> Aug  5 06:09:40 pureos kernel: [  827.527197]  __switch_to+0x188/0x200
> Aug  5 06:09:40 pureos kernel: [  827.527203]  __schedule+0x1e0/0x488
> Aug  5 06:09:40 pureos kernel: [  827.527208]  schedule+0x38/0xc8
> Aug  5 06:09:40 pureos kernel: [  827.527213]  smpboot_thread_fn+0x250/0x2a8
> Aug  5 06:09:40 pureos kernel: [  827.527218]  kthread+0xf4/0x120
> Aug  5 06:09:40 pureos kernel: [  827.527222]  ret_from_fork+0x10/0x18
> Aug  5 06:09:40 pureos kernel: [  827.527226] thermal-idle/2  S0
> 163  2 0x0028
> Aug  5 06:09:40 pureos kernel: [  827.527231] Call trace:
> Aug  5 06:09:40 pureos kernel: [  827.527237]  __switch_to+0xb4/0x200
> Aug  5 06:09:40 pureos kernel: [  827.527242]  __schedule+0x1e0/0x488
> Aug  5 06:09:40 pureos kernel: [  827.527247]  schedule+0x38/0xc8
> Aug  5 06:09:40 pureos kernel: [  827.527259]  smpboot_thread_fn+0x250/0x2a8
> Aug  5 06:09:40 pureos kernel: [  827.527264]  kthread+0xf4/0x120
> Aug  5 06:09:40 pureos kernel: [  827.527268]  ret_from_fork+0x10/0x18
> Aug  5 06:09:40 pureos kernel: [  827.527272] thermal-idle/3  S0
> 164  2 0x0028
> Aug  5 06:09:40 pureos kernel: [  827.527278] Call trace:
> Aug  5 06:09:40 pureos kernel: [  827.527283]  __switch_to+0xb4/0x200
> Aug  5 06:09:40 pureos kernel: [  827.527288]  __schedule+0x1e0/0x488
> Aug  5 06:09:40 pureos kernel: [  827.527293]  schedule+0x38/0xc8
> Aug  5 06:09:40 pureos kernel: [  827.527298]  smpboot_thread_fn+0x250/0x2a8
> Aug  5 06:09:40 pureos kernel: [  827.527303]  kthread+0xf4/0x120
> Aug  5 06:09:40 pureos kernel: [  827.527308]  ret_from_fork+0x10/0x18
> Aug  5 06:09:40 pureos kernel: [  827.527375] Restarting kernel threads
> ... done.
> Aug  5 06:09:40 pureos kernel: [  827.527771] OOM killer enabled.
> Aug  5 06:09:40 pureos kernel: [  827.527772] Restarting tasks ... done.
> Aug  5 06:09:40 pureos kernel: [  827.528926] PM: suspend exit
> 
> 
> do you know where things might go wrong here?
> 
> thanks,
> 
> martin
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-05 Thread Daniel Lezcano 
On 05/08/2019 07:11, Martin Kepplinger wrote:
> ---

[ ... ]

>> +static s64 cpuidle_cooling_runtime(struct cpuidle_cooling_device *idle_cdev)
>> +{
>> +s64 next_wakeup;
>> +unsigned long state = idle_cdev->state;
>> +
>> +/*
>> + * The function should not be called when there is no
>> + * mitigation because:
>> + * - that does not make sense
>> + * - we end up with a division by zero
>> + */
>> +if (!state)
>> +return 0;
>> +
>> +next_wakeup = (s64)((idle_cdev->idle_cycle * 100) / state) -
>> +idle_cdev->idle_cycle;
>> +
>> +return next_wakeup * NSEC_PER_USEC;
>> +}
>> +
> 
> There is a bug in your calculation formula here when "state" becomes 100.
> You return 0 for the injection rate, which is the same as "rate" being 0,
> which is dangerous. You stop cooling when it's most necessary :)

Right, thanks for spotting this.

> I'm not sure how much sense really being 100% idle makes, so I, when testing
> this, just say if (state == 100) { state = 99 }. Anyways, just don't return 0.
> 
> Daniel, thanks a lot for these additions! Could you send an update of this?

Yes, I'm working on a new version.

> btw, that's what I'm referring to:
> https://lore.kernel.org/linux-pm/1522945005-7165-1-git-send-email-daniel.lezc...@linaro.org/
> I know it's a little old already, but it seems like there hasn't been any
> equivalent solution in the meantime, has it?
> 
> Using cpuidle for cooling is way more effective than cpufreq (which often
> hardly is).

On which platform that happens?


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-04 Thread Martin Kepplinger 
On 05.08.19 07:11, Martin Kepplinger wrote:
> ---
> 
> On 05-04-18, 18:16, Daniel Lezcano wrote:
>> The cpu idle cooling driver performs synchronized idle injection across all
>> cpus belonging to the same cluster and offers a new method to cool down a 
>> SoC.
>>
>> Each cluster has its own idle cooling device, each core has its own idle
>> injection thread, each idle injection thread uses play_idle to enter idle.  
>> In
>> order to reach the deepest idle state, each cooling device has the idle
>> injection threads synchronized together.
>>
>> It has some similarity with the intel power clamp driver but it is actually
>> designed to work on the ARM architecture via the DT with a mathematical proof
>> with the power model which comes with the Documentation.
>>
>> The idle injection cycle is fixed while the running cycle is variable. That
>> allows to have control on the device reactivity for the user experience. At
>> the mitigation point the idle threads are unparked, they play idle the
>> specified amount of time and they schedule themselves. The last thread sets
>> the next idle injection deadline and when the timer expires it wakes up all
>> the threads which in turn play idle again. Meanwhile the running cycle is
>> changed by set_cur_state.  When the mitigation ends, the threads are parked.
>> The algorithm is self adaptive, so there is no need to handle hotplugging.
>>
>> If we take an example of the balanced point, we can use the DT for the 
>> hi6220.
>>
>> The sustainable power for the SoC is 3326mW to mitigate at 75°C. Eight cores
>> running at full blast at the maximum OPP consumes 5280mW. The first value is
>> given in the DT, the second is calculated from the OPP with the formula:
>>
>>Pdyn = Cdyn x Voltage^2 x Frequency
>>
>> As the SoC vendors don't want to share the static leakage values, we assume
>> it is zero, so the Prun = Pdyn + Pstatic = Pdyn + 0 = Pdyn.
>>
>> In order to reduce the power to 3326mW, we have to apply a ratio to the
>> running time.
>>
>> ratio = (Prun - Ptarget) / Ptarget = (5280 - 3326) / 3326 = 0,5874
>>
>> We know the idle cycle which is fixed, let's assume 10ms. However from this
>> duration we have to substract the wake up latency for the cluster idle state.
>> In our case, it is 1.5ms. So for a 10ms latency for idle, we are really idle
>> 8.5ms.
>>
>> As we know the idle duration and the ratio, we can compute the running cycle.
>>
>>running_cycle = 8.5 / 0.5874 = 14.47ms
>>
>> So for 8.5ms of idle, we have 14.47ms of running cycle, and that brings the
>> SoC to the balanced trip point of 75°C.
>>
>> The driver has been tested on the hi6220 and it appears the temperature
>> stabilizes at 75°C with an idle injection time of 10ms (8.5ms real) and
>> running cycle of 14ms as expected by the theory above.
>>
>> Signed-off-by: Kevin Wangtao 
>> Signed-off-by: Daniel Lezcano 
>> ---
>>  drivers/thermal/Kconfig   |  10 +
>>  drivers/thermal/cpu_cooling.c | 479 
>> ++
>>  include/linux/cpu_cooling.h   |   6 +
>>  3 files changed, 495 insertions(+)
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 5aaae1b..6c34117 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -166,6 +166,16 @@ config CPU_FREQ_THERMAL
>>This will be useful for platforms using the generic thermal interface
>>and not the ACPI interface.
>>  
>> +config CPU_IDLE_THERMAL
>> +   bool "CPU idle cooling strategy"
>> +   depends on CPU_IDLE
>> +   help
>> + This implements the generic CPU cooling mechanism through
>> + idle injection.  This will throttle the CPU by injecting
>> + fixed idle cycle.  All CPUs belonging to the same cluster
>> + will enter idle synchronously to reach the deepest idle
>> + state.
>> +
>>  endchoice
>>  
>>  config CLOCK_THERMAL
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> index 5c219dc..1eec8d6 100644
>> --- a/drivers/thermal/cpu_cooling.c
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -10,18 +10,33 @@
>>   *  Viresh Kumar 
>>   *
>>   */
>> +#define pr_fmt(fmt) "CPU cooling: " fmt
>> +
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>> +#include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>>  
>> +#include 
>> +
>> +#include 
>> +#include 
>> +
>>  #include 
>>  
>> +#include 
>> +
>>  #ifdef CONFIG_CPU_FREQ_THERMAL
>>  /*
>>   * Cooling state <-> CPUFreq frequency
>> @@ -928,3 +943,467 @@ void cpufreq_cooling_unregister(struct 
>> thermal_cooling_device *cdev)
>>  }
>>  EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
>>  #endif /* CONFIG_CPU_FREQ_THERMAL */
>> +
>> +#ifdef CONFIG_CPU_IDLE_THERMAL
>> +/**
>> + * struct cpuidle_cooling_device - data for the idle cooling device
>> + * @cdev: a pointer to a struct thermal_cooling_device
>> + * @cpumask: a cpumask containing the CPU man

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2019-08-04 Thread Martin Kepplinger 
---

On 05-04-18, 18:16, Daniel Lezcano wrote:
> The cpu idle cooling driver performs synchronized idle injection across all
> cpus belonging to the same cluster and offers a new method to cool down a SoC.
> 
> Each cluster has its own idle cooling device, each core has its own idle
> injection thread, each idle injection thread uses play_idle to enter idle.  In
> order to reach the deepest idle state, each cooling device has the idle
> injection threads synchronized together.
> 
> It has some similarity with the intel power clamp driver but it is actually
> designed to work on the ARM architecture via the DT with a mathematical proof
> with the power model which comes with the Documentation.
> 
> The idle injection cycle is fixed while the running cycle is variable. That
> allows to have control on the device reactivity for the user experience. At
> the mitigation point the idle threads are unparked, they play idle the
> specified amount of time and they schedule themselves. The last thread sets
> the next idle injection deadline and when the timer expires it wakes up all
> the threads which in turn play idle again. Meanwhile the running cycle is
> changed by set_cur_state.  When the mitigation ends, the threads are parked.
> The algorithm is self adaptive, so there is no need to handle hotplugging.
> 
> If we take an example of the balanced point, we can use the DT for the hi6220.
> 
> The sustainable power for the SoC is 3326mW to mitigate at 75°C. Eight cores
> running at full blast at the maximum OPP consumes 5280mW. The first value is
> given in the DT, the second is calculated from the OPP with the formula:
> 
>Pdyn = Cdyn x Voltage^2 x Frequency
> 
> As the SoC vendors don't want to share the static leakage values, we assume
> it is zero, so the Prun = Pdyn + Pstatic = Pdyn + 0 = Pdyn.
> 
> In order to reduce the power to 3326mW, we have to apply a ratio to the
> running time.
> 
> ratio = (Prun - Ptarget) / Ptarget = (5280 - 3326) / 3326 = 0,5874
> 
> We know the idle cycle which is fixed, let's assume 10ms. However from this
> duration we have to substract the wake up latency for the cluster idle state.
> In our case, it is 1.5ms. So for a 10ms latency for idle, we are really idle
> 8.5ms.
> 
> As we know the idle duration and the ratio, we can compute the running cycle.
> 
>running_cycle = 8.5 / 0.5874 = 14.47ms
> 
> So for 8.5ms of idle, we have 14.47ms of running cycle, and that brings the
> SoC to the balanced trip point of 75°C.
> 
> The driver has been tested on the hi6220 and it appears the temperature
> stabilizes at 75°C with an idle injection time of 10ms (8.5ms real) and
> running cycle of 14ms as expected by the theory above.
> 
> Signed-off-by: Kevin Wangtao 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/thermal/Kconfig   |  10 +
>  drivers/thermal/cpu_cooling.c | 479 
> ++
>  include/linux/cpu_cooling.h   |   6 +
>  3 files changed, 495 insertions(+)
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 5aaae1b..6c34117 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -166,6 +166,16 @@ config CPU_FREQ_THERMAL
> This will be useful for platforms using the generic thermal interface
> and not the ACPI interface.
>  
> +config CPU_IDLE_THERMAL
> +   bool "CPU idle cooling strategy"
> +   depends on CPU_IDLE
> +   help
> +  This implements the generic CPU cooling mechanism through
> +  idle injection.  This will throttle the CPU by injecting
> +  fixed idle cycle.  All CPUs belonging to the same cluster
> +  will enter idle synchronously to reach the deepest idle
> +  state.
> +
>  endchoice
>  
>  config CLOCK_THERMAL
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 5c219dc..1eec8d6 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -10,18 +10,33 @@
>   *   Viresh Kumar 
>   *
>   */
> +#define pr_fmt(fmt) "CPU cooling: " fmt
> +
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> +#include 
> +
> +#include 
> +#include 
> +
>  #include 
>  
> +#include 
> +
>  #ifdef CONFIG_CPU_FREQ_THERMAL
>  /*
>   * Cooling state <-> CPUFreq frequency
> @@ -928,3 +943,467 @@ void cpufreq_cooling_unregister(struct 
> thermal_cooling_device *cdev)
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
>  #endif /* CONFIG_CPU_FREQ_THERMAL */
> +
> +#ifdef CONFIG_CPU_IDLE_THERMAL
> +/**
> + * struct cpuidle_cooling_device - data for the idle cooling device
> + * @cdev: a pointer to a struct thermal_cooling_device
> + * @cpumask: a cpumask containing the CPU managed by the cooling device
> + * @timer: a hrtimer giving the tempo for the idle injection cycles
> + * @kref: a kernel refcount on this structure
> + * @count: an atomic

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-17 Thread Lorenzo Pieralisi 
On Tue, Apr 17, 2018 at 09:17:36AM +0200, Daniel Lezcano wrote:

[...]

>  Actually there is no impact with the change Sudeep is referring to. It
>  is for ACPI, we are DT based. Confirmed with Jeremy.
> 
>  So AFAICT, it is not a problem.
> >>>
> >>> It is a problem - DT or ACPI alike. Sudeep was referring to the notion
> >>> of "cluster" that has no architectural meaning whatsoever and using
> >>> topology_physical_package_id() to detect a "cluster" was/is/will always
> >>> be the wrong thing to do. The notion of cluster must not appear in the
> >>> kernel at all, it has no architectural meaning. I understand you need
> >>> to group CPUs but that has to be done in a different way, through
> >>> cooling devices, thermal domains or power domains DT/ACPI bindings but
> >>> not by using topology masks.
> >>
> >> I don't get it. What is the cluster concept defined in the ARM
> >> documentation?
> >>
> >> ARM Cortex-A53 MPCore Processor Technical Reference Manual
> >>
> >> 4.5.2. Multiprocessor Affinity Register
> >>
> >> I see the documentation says:
> >>
> >> A cluster with two cores, three cores, ...
> >>
> >> How the kernel can represent that if you kill the
> >> topology_physical_package_id() ?
> > 
> > From an Arm ARM perspective (ARM v8 reference manual), the MPIDR_EL1 has
> > no notion of cluster which means that a cluster is not architecturally
> > defined on Arm systems.
> 
> Sorry, I'm lost :/ You say the MPIDR_EL1 has no notion of cluster but
> the documentation describing this register is all talking about cluster.
> 
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500g/BABHBJCI.html

I pointed you at the documentation I am referring to. You are referring
to A53 TRM, I am referring to the Arm architecture reference manual that
is the reference for all Arm cores.

> > Currently, as Morten explained today, topology_physical_package_id()
> > is supposed to represent a "cluster" and that's completely wrong
> > because a "cluster" cannot be defined from an architectural perspective.
> > 
> > It was a bodge used as a shortcut, wrongly. We should have never used
> > that API for that purpose and there must be no code in the kernel
> > relying on:
> > 
> > topology_physical_package_id()
> > 
> > to define a cluster; the information you require to group CPUs must
> > come from something else, which is firmware bindings(DT or ACPI) as
> > I mentioned.
> 
> Why not ?

I explained why not :). A cluster is not defined architecturally on Arm
- it is as simple as that and you can't rely on a given MPIDR_EL1
subfield to define what a cluster id is.

> diff --git a/arch/arm64/include/asm/topology.h
> b/arch/arm64/include/asm/topology.h
> index c4f2d50..ac0776d 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -14,7 +14,8 @@ struct cpu_topology {
> 
>  extern struct cpu_topology cpu_topology[NR_CPUS];
> 
> -#define topology_physical_package_id(cpu)
> (cpu_topology[cpu].cluster_id)
> +#define topology_physical_package_id(cpu)  (0)
> +#define topology_physical_cluster_id(cpu)

There is no such a thing (and there is no architecturally defined
package id on Arm either).

> (cpu_topology[cpu].cluster_id)
>  #define topology_core_id(cpu)  (cpu_topology[cpu].core_id)
>  #define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling)
>  #define topology_sibling_cpumask(cpu)  (&cpu_topology[cpu].thread_sibling)
> 
> 
> > Please speak to Sudeep who will fill you on the reasoning above.
> 
> Yes, Sudeep is next to me but I would prefer to keep the discussion on
> the mailing list so everyone can get the reasoning.

It is not a reasoning - it is the Arm architecture. There is no
architecturally defined cluster id on Arm. The affinity bits in
MPIDR_EL1 must be treated as a unique number that represents a given
core/thread, how the bits are allocated across affinity levels is not
something that you can rely on architecturally - that's why DT/ACPI
topology bindings exist to group cpus in a hierarchical topology.

HTH,
Lorenzo

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-17 Thread Daniel Lezcano 
On 16/04/2018 16:22, Lorenzo Pieralisi wrote:
> On Mon, Apr 16, 2018 at 03:57:03PM +0200, Daniel Lezcano wrote:
>> On 16/04/2018 14:30, Lorenzo Pieralisi wrote:
>>> On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
 On 16/04/2018 12:10, Viresh Kumar wrote:
> On 16-04-18, 12:03, Daniel Lezcano wrote:
>> On 16/04/2018 11:50, Viresh Kumar wrote:
>>> On 16-04-18, 11:45, Daniel Lezcano wrote:
 Can you elaborate a bit ? I'm not sure to get the point.
>>>
>>> Sure. With your current code on Hikey960 (big/LITTLE), you end up
>>> creating two cooling devices, one for the big cluster and one for
>>> small cluster. Which is the right thing to do, as we also have two
>>> cpufreq cooling devices.
>>>
>>> But with the change Sudeep is referring to, the helper you used to get
>>> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
>>> will end up creating a single cpuidle cooling device for all the CPUs.
>>> Which would be wrong.
>>
>> Is the semantic of topology_physical_package_id changing ?
>
> That's what I understood from his email.
>
>> I don't
>> understand the change Sudeep is referring to.

 Actually there is no impact with the change Sudeep is referring to. It
 is for ACPI, we are DT based. Confirmed with Jeremy.

 So AFAICT, it is not a problem.
>>>
>>> It is a problem - DT or ACPI alike. Sudeep was referring to the notion
>>> of "cluster" that has no architectural meaning whatsoever and using
>>> topology_physical_package_id() to detect a "cluster" was/is/will always
>>> be the wrong thing to do. The notion of cluster must not appear in the
>>> kernel at all, it has no architectural meaning. I understand you need
>>> to group CPUs but that has to be done in a different way, through
>>> cooling devices, thermal domains or power domains DT/ACPI bindings but
>>> not by using topology masks.
>>
>> I don't get it. What is the cluster concept defined in the ARM
>> documentation?
>>
>> ARM Cortex-A53 MPCore Processor Technical Reference Manual
>>
>> 4.5.2. Multiprocessor Affinity Register
>>
>> I see the documentation says:
>>
>> A cluster with two cores, three cores, ...
>>
>> How the kernel can represent that if you kill the
>> topology_physical_package_id() ?
> 
> From an Arm ARM perspective (ARM v8 reference manual), the MPIDR_EL1 has
> no notion of cluster which means that a cluster is not architecturally
> defined on Arm systems.

Sorry, I'm lost :/ You say the MPIDR_EL1 has no notion of cluster but
the documentation describing this register is all talking about cluster.

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500g/BABHBJCI.html

> Currently, as Morten explained today, topology_physical_package_id()
> is supposed to represent a "cluster" and that's completely wrong
> because a "cluster" cannot be defined from an architectural perspective.
> 
> It was a bodge used as a shortcut, wrongly. We should have never used
> that API for that purpose and there must be no code in the kernel
> relying on:
> 
> topology_physical_package_id()
> 
> to define a cluster; the information you require to group CPUs must
> come from something else, which is firmware bindings(DT or ACPI) as
> I mentioned.

Why not ?

diff --git a/arch/arm64/include/asm/topology.h
b/arch/arm64/include/asm/topology.h
index c4f2d50..ac0776d 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -14,7 +14,8 @@ struct cpu_topology {

 extern struct cpu_topology cpu_topology[NR_CPUS];

-#define topology_physical_package_id(cpu)
(cpu_topology[cpu].cluster_id)
+#define topology_physical_package_id(cpu)  (0)
+#define topology_physical_cluster_id(cpu)
(cpu_topology[cpu].cluster_id)
 #define topology_core_id(cpu)  (cpu_topology[cpu].core_id)
 #define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling)
 #define topology_sibling_cpumask(cpu)  (&cpu_topology[cpu].thread_sibling)


> Please speak to Sudeep who will fill you on the reasoning above.

Yes, Sudeep is next to me but I would prefer to keep the discussion on
the mailing list so everyone can get the reasoning.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Lorenzo Pieralisi 
On Mon, Apr 16, 2018 at 03:57:03PM +0200, Daniel Lezcano wrote:
> On 16/04/2018 14:30, Lorenzo Pieralisi wrote:
> > On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
> >> On 16/04/2018 12:10, Viresh Kumar wrote:
> >>> On 16-04-18, 12:03, Daniel Lezcano wrote:
>  On 16/04/2018 11:50, Viresh Kumar wrote:
> > On 16-04-18, 11:45, Daniel Lezcano wrote:
> >> Can you elaborate a bit ? I'm not sure to get the point.
> >
> > Sure. With your current code on Hikey960 (big/LITTLE), you end up
> > creating two cooling devices, one for the big cluster and one for
> > small cluster. Which is the right thing to do, as we also have two
> > cpufreq cooling devices.
> >
> > But with the change Sudeep is referring to, the helper you used to get
> > cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> > will end up creating a single cpuidle cooling device for all the CPUs.
> > Which would be wrong.
> 
>  Is the semantic of topology_physical_package_id changing ?
> >>>
> >>> That's what I understood from his email.
> >>>
>  I don't
>  understand the change Sudeep is referring to.
> >>
> >> Actually there is no impact with the change Sudeep is referring to. It
> >> is for ACPI, we are DT based. Confirmed with Jeremy.
> >>
> >> So AFAICT, it is not a problem.
> > 
> > It is a problem - DT or ACPI alike. Sudeep was referring to the notion
> > of "cluster" that has no architectural meaning whatsoever and using
> > topology_physical_package_id() to detect a "cluster" was/is/will always
> > be the wrong thing to do. The notion of cluster must not appear in the
> > kernel at all, it has no architectural meaning. I understand you need
> > to group CPUs but that has to be done in a different way, through
> > cooling devices, thermal domains or power domains DT/ACPI bindings but
> > not by using topology masks.
> 
> I don't get it. What is the cluster concept defined in the ARM
> documentation?
> 
> ARM Cortex-A53 MPCore Processor Technical Reference Manual
> 
> 4.5.2. Multiprocessor Affinity Register
> 
> I see the documentation says:
> 
> A cluster with two cores, three cores, ...
> 
> How the kernel can represent that if you kill the
> topology_physical_package_id() ?

>From an Arm ARM perspective (ARM v8 reference manual), the MPIDR_EL1 has
no notion of cluster which means that a cluster is not architecturally
defined on Arm systems.

Currently, as Morten explained today, topology_physical_package_id()
is supposed to represent a "cluster" and that's completely wrong
because a "cluster" cannot be defined from an architectural perspective.

It was a bodge used as a shortcut, wrongly. We should have never used
that API for that purpose and there must be no code in the kernel
relying on:

topology_physical_package_id()

to define a cluster; the information you require to group CPUs must
come from something else, which is firmware bindings(DT or ACPI) as
I mentioned.

Please speak to Sudeep who will fill you on the reasoning above.

Thanks,
Lorenzo

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Daniel Lezcano 
On 16/04/2018 14:30, Lorenzo Pieralisi wrote:
> On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
>> On 16/04/2018 12:10, Viresh Kumar wrote:
>>> On 16-04-18, 12:03, Daniel Lezcano wrote:
 On 16/04/2018 11:50, Viresh Kumar wrote:
> On 16-04-18, 11:45, Daniel Lezcano wrote:
>> Can you elaborate a bit ? I'm not sure to get the point.
>
> Sure. With your current code on Hikey960 (big/LITTLE), you end up
> creating two cooling devices, one for the big cluster and one for
> small cluster. Which is the right thing to do, as we also have two
> cpufreq cooling devices.
>
> But with the change Sudeep is referring to, the helper you used to get
> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> will end up creating a single cpuidle cooling device for all the CPUs.
> Which would be wrong.

 Is the semantic of topology_physical_package_id changing ?
>>>
>>> That's what I understood from his email.
>>>
 I don't
 understand the change Sudeep is referring to.
>>
>> Actually there is no impact with the change Sudeep is referring to. It
>> is for ACPI, we are DT based. Confirmed with Jeremy.
>>
>> So AFAICT, it is not a problem.
> 
> It is a problem - DT or ACPI alike. Sudeep was referring to the notion
> of "cluster" that has no architectural meaning whatsoever and using
> topology_physical_package_id() to detect a "cluster" was/is/will always
> be the wrong thing to do. The notion of cluster must not appear in the
> kernel at all, it has no architectural meaning. I understand you need
> to group CPUs but that has to be done in a different way, through
> cooling devices, thermal domains or power domains DT/ACPI bindings but
> not by using topology masks.

I don't get it. What is the cluster concept defined in the ARM
documentation?

ARM Cortex-A53 MPCore Processor Technical Reference Manual

4.5.2. Multiprocessor Affinity Register

I see the documentation says:

A cluster with two cores, three cores, ...

How the kernel can represent that if you kill the
topology_physical_package_id() ?

> You should be able to figure out this week at OSPM the reasoning behind
> what I am saying above.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Sudeep Holla 
On Mon, Apr 16, 2018 at 02:49:35PM +0200, Daniel Lezcano wrote:
> On 16/04/2018 14:31, Sudeep Holla wrote:
> > On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
> >> On 16/04/2018 12:10, Viresh Kumar wrote:
> >>> On 16-04-18, 12:03, Daniel Lezcano wrote:
>  On 16/04/2018 11:50, Viresh Kumar wrote:
> > On 16-04-18, 11:45, Daniel Lezcano wrote:
> >> Can you elaborate a bit ? I'm not sure to get the point.
> >
> > Sure. With your current code on Hikey960 (big/LITTLE), you end up
> > creating two cooling devices, one for the big cluster and one for
> > small cluster. Which is the right thing to do, as we also have two
> > cpufreq cooling devices.
> >
> > But with the change Sudeep is referring to, the helper you used to get
> > cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> > will end up creating a single cpuidle cooling device for all the CPUs.
> > Which would be wrong.
> 
>  Is the semantic of topology_physical_package_id changing ?
> >>>
> >>> That's what I understood from his email.
> >>>
>  I don't
>  understand the change Sudeep is referring to.
> >>
> >> Actually there is no impact with the change Sudeep is referring to. It
> >> is for ACPI, we are DT based. Confirmed with Jeremy.
> >>
> > 
> > No, it will change for DT. The aim is to be consistent irrespective of
> > h/w or f/w description(i.e ADCPI or DT)
> 
> What will happen with the code using topology_physical_package_id ?
> 
> drivers/acpi/processor_thermal.c:   int id = 
> topology_physical_package_id(cpu);
> drivers/acpi/processor_thermal.c:   if 
> (topology_physical_package_id(i) == id)
> drivers/acpi/processor_thermal.c:   if 
> (topology_physical_package_id(i) ==
> drivers/acpi/processor_thermal.c: topology_physical_package_id(cpu))
> 

ACPI and hence will have right notion of package.

> drivers/block/mtip32xx/mtip32xx.c: 
> topology_physical_package_id(cpumask_first(node_mask)),
> 

Not sure if it was ever used on ARM/ARM64

> drivers/cpufreq/arm_big_little.c:   return 
> topology_physical_package_id(cpu);

Yes this is main affected driver. I don't think it's used by any ARM64
platform anymore. This is one of the reason I divorced SCPI driver from
this recently. ARM32 may still retain old definition of package_id(not
sure, depends if anyone as need to change that and maintainers view on
that). If not, we need to fix this driver along with the code changing
the definition.

> 
> drivers/crypto/qat/qat_common/adf_common_drv.h: return 
> topology_physical_package_id(smp_processor_id());
> drivers/crypto/virtio/virtio_crypto_common.h:   node = 
> topology_physical_package_id(cpu);
> 

x86 specific and never assumed cluster id.

> kernel/events/core.c:   event_pkg = 
> topology_physical_package_id(event_cpu);
> kernel/events/core.c:   local_pkg = 
> topology_physical_package_id(local_cpu);
> 

Generic and hence never assumed cluster id.

--
Regards,
Sudeep

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Daniel Lezcano 
On 16/04/2018 14:31, Sudeep Holla wrote:
> On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
>> On 16/04/2018 12:10, Viresh Kumar wrote:
>>> On 16-04-18, 12:03, Daniel Lezcano wrote:
 On 16/04/2018 11:50, Viresh Kumar wrote:
> On 16-04-18, 11:45, Daniel Lezcano wrote:
>> Can you elaborate a bit ? I'm not sure to get the point.
>
> Sure. With your current code on Hikey960 (big/LITTLE), you end up
> creating two cooling devices, one for the big cluster and one for
> small cluster. Which is the right thing to do, as we also have two
> cpufreq cooling devices.
>
> But with the change Sudeep is referring to, the helper you used to get
> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> will end up creating a single cpuidle cooling device for all the CPUs.
> Which would be wrong.

 Is the semantic of topology_physical_package_id changing ?
>>>
>>> That's what I understood from his email.
>>>
 I don't
 understand the change Sudeep is referring to.
>>
>> Actually there is no impact with the change Sudeep is referring to. It
>> is for ACPI, we are DT based. Confirmed with Jeremy.
>>
> 
> No, it will change for DT. The aim is to be consistent irrespective of
> h/w or f/w description(i.e ADCPI or DT)

What will happen with the code using topology_physical_package_id ?

drivers/acpi/processor_thermal.c:   int id =
topology_physical_package_id(cpu);

drivers/acpi/processor_thermal.c:   if
(topology_physical_package_id(i) == id)

drivers/acpi/processor_thermal.c:   if
(topology_physical_package_id(i) ==

drivers/acpi/processor_thermal.c:
topology_physical_package_id(cpu))

drivers/block/mtip32xx/mtip32xx.c:
topology_physical_package_id(cpumask_first(node_mask)),

drivers/cpufreq/arm_big_little.c:   return
topology_physical_package_id(cpu);

drivers/crypto/qat/qat_common/adf_common_drv.h: return
topology_physical_package_id(smp_processor_id());

drivers/crypto/virtio/virtio_crypto_common.h:   node =
topology_physical_package_id(cpu);

kernel/events/core.c:   event_pkg =
topology_physical_package_id(event_cpu);

kernel/events/core.c:   local_pkg =
topology_physical_package_id(local_cpu);





-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Sudeep Holla 
On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
> On 16/04/2018 12:10, Viresh Kumar wrote:
> > On 16-04-18, 12:03, Daniel Lezcano wrote:
> >> On 16/04/2018 11:50, Viresh Kumar wrote:
> >>> On 16-04-18, 11:45, Daniel Lezcano wrote:
>  Can you elaborate a bit ? I'm not sure to get the point.
> >>>
> >>> Sure. With your current code on Hikey960 (big/LITTLE), you end up
> >>> creating two cooling devices, one for the big cluster and one for
> >>> small cluster. Which is the right thing to do, as we also have two
> >>> cpufreq cooling devices.
> >>>
> >>> But with the change Sudeep is referring to, the helper you used to get
> >>> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> >>> will end up creating a single cpuidle cooling device for all the CPUs.
> >>> Which would be wrong.
> >>
> >> Is the semantic of topology_physical_package_id changing ?
> > 
> > That's what I understood from his email.
> > 
> >> I don't
> >> understand the change Sudeep is referring to.
> 
> Actually there is no impact with the change Sudeep is referring to. It
> is for ACPI, we are DT based. Confirmed with Jeremy.
>

No, it will change for DT. The aim is to be consistent irrespective of
h/w or f/w description(i.e ADCPI or DT)

--
Regards,
Sudeep

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Lorenzo Pieralisi 
On Mon, Apr 16, 2018 at 02:10:30PM +0200, Daniel Lezcano wrote:
> On 16/04/2018 12:10, Viresh Kumar wrote:
> > On 16-04-18, 12:03, Daniel Lezcano wrote:
> >> On 16/04/2018 11:50, Viresh Kumar wrote:
> >>> On 16-04-18, 11:45, Daniel Lezcano wrote:
>  Can you elaborate a bit ? I'm not sure to get the point.
> >>>
> >>> Sure. With your current code on Hikey960 (big/LITTLE), you end up
> >>> creating two cooling devices, one for the big cluster and one for
> >>> small cluster. Which is the right thing to do, as we also have two
> >>> cpufreq cooling devices.
> >>>
> >>> But with the change Sudeep is referring to, the helper you used to get
> >>> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> >>> will end up creating a single cpuidle cooling device for all the CPUs.
> >>> Which would be wrong.
> >>
> >> Is the semantic of topology_physical_package_id changing ?
> > 
> > That's what I understood from his email.
> > 
> >> I don't
> >> understand the change Sudeep is referring to.
> 
> Actually there is no impact with the change Sudeep is referring to. It
> is for ACPI, we are DT based. Confirmed with Jeremy.
> 
> So AFAICT, it is not a problem.

It is a problem - DT or ACPI alike. Sudeep was referring to the notion
of "cluster" that has no architectural meaning whatsoever and using
topology_physical_package_id() to detect a "cluster" was/is/will always
be the wrong thing to do. The notion of cluster must not appear in the
kernel at all, it has no architectural meaning. I understand you need
to group CPUs but that has to be done in a different way, through
cooling devices, thermal domains or power domains DT/ACPI bindings but
not by using topology masks.

You should be able to figure out this week at OSPM the reasoning behind
what I am saying above.

Cheers,
Lorenzo

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Sudeep Holla 
On Mon, Apr 16, 2018 at 03:20:06PM +0530, Viresh Kumar wrote:
> On 16-04-18, 11:45, Daniel Lezcano wrote:
> > Can you elaborate a bit ? I'm not sure to get the point.
> 
> Sure. With your current code on Hikey960 (big/LITTLE), you end up
> creating two cooling devices, one for the big cluster and one for
> small cluster. Which is the right thing to do, as we also have two
> cpufreq cooling devices.
> 
> But with the change Sudeep is referring to, the helper you used to get
> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> will end up creating a single cpuidle cooling device for all the CPUs.
> Which would be wrong.
> 
> > BTW, Am I asked to change my code to stick to something which is not
> > merged ?
> 
> Sudeep looked pretty confident on how the meaning of this routine is
> going to change very soon. I will let him respond on what guarantees
> we have that it will get merged :)
> 

Viresh has already summarised it correctly. I don't have much to add.
Unless the system has multiple package, this routine will return 0(or some
fixed socket number, I don't think it can be different from 0 but
theoretically it can be anything but has to be same for all the CPUs)

--
Regards,
Sudeep

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Daniel Lezcano 
On 16/04/2018 12:10, Viresh Kumar wrote:
> On 16-04-18, 12:03, Daniel Lezcano wrote:
>> On 16/04/2018 11:50, Viresh Kumar wrote:
>>> On 16-04-18, 11:45, Daniel Lezcano wrote:
 Can you elaborate a bit ? I'm not sure to get the point.
>>>
>>> Sure. With your current code on Hikey960 (big/LITTLE), you end up
>>> creating two cooling devices, one for the big cluster and one for
>>> small cluster. Which is the right thing to do, as we also have two
>>> cpufreq cooling devices.
>>>
>>> But with the change Sudeep is referring to, the helper you used to get
>>> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
>>> will end up creating a single cpuidle cooling device for all the CPUs.
>>> Which would be wrong.
>>
>> Is the semantic of topology_physical_package_id changing ?
> 
> That's what I understood from his email.
> 
>> I don't
>> understand the change Sudeep is referring to.

Actually there is no impact with the change Sudeep is referring to. It
is for ACPI, we are DT based. Confirmed with Jeremy.

So AFAICT, it is not a problem.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Viresh Kumar 
On 16-04-18, 12:03, Daniel Lezcano wrote:
> On 16/04/2018 11:50, Viresh Kumar wrote:
> > On 16-04-18, 11:45, Daniel Lezcano wrote:
> >> Can you elaborate a bit ? I'm not sure to get the point.
> > 
> > Sure. With your current code on Hikey960 (big/LITTLE), you end up
> > creating two cooling devices, one for the big cluster and one for
> > small cluster. Which is the right thing to do, as we also have two
> > cpufreq cooling devices.
> > 
> > But with the change Sudeep is referring to, the helper you used to get
> > cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> > will end up creating a single cpuidle cooling device for all the CPUs.
> > Which would be wrong.
> 
> Is the semantic of topology_physical_package_id changing ?

That's what I understood from his email.

> I don't
> understand the change Sudeep is referring to.
> 
> I saw this attempt:
> 
> https://patchwork.kernel.org/patch/9959977/

@Sudeep: Is using topology_cod_id() is okay in that case ?

-- 
viresh

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Daniel Lezcano 
On 16/04/2018 11:50, Viresh Kumar wrote:
> On 16-04-18, 11:45, Daniel Lezcano wrote:
>> Can you elaborate a bit ? I'm not sure to get the point.
> 
> Sure. With your current code on Hikey960 (big/LITTLE), you end up
> creating two cooling devices, one for the big cluster and one for
> small cluster. Which is the right thing to do, as we also have two
> cpufreq cooling devices.
> 
> But with the change Sudeep is referring to, the helper you used to get
> cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
> will end up creating a single cpuidle cooling device for all the CPUs.
> Which would be wrong.

Is the semantic of topology_physical_package_id changing ? I don't
understand the change Sudeep is referring to.

I saw this attempt:

https://patchwork.kernel.org/patch/9959977/

>> BTW, Am I asked to change my code to stick to something which is not
>> merged ?
> 
> Sudeep looked pretty confident on how the meaning of this routine is
> going to change very soon. I will let him respond on what guarantees
> we have that it will get merged :)



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Viresh Kumar 
On 16-04-18, 11:45, Daniel Lezcano wrote:
> Can you elaborate a bit ? I'm not sure to get the point.

Sure. With your current code on Hikey960 (big/LITTLE), you end up
creating two cooling devices, one for the big cluster and one for
small cluster. Which is the right thing to do, as we also have two
cpufreq cooling devices.

But with the change Sudeep is referring to, the helper you used to get
cluster id will return 0 (SoC id) for all the 8 CPUs. So your code
will end up creating a single cpuidle cooling device for all the CPUs.
Which would be wrong.

> BTW, Am I asked to change my code to stick to something which is not
> merged ?

Sudeep looked pretty confident on how the meaning of this routine is
going to change very soon. I will let him respond on what guarantees
we have that it will get merged :)

-- 
viresh

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Daniel Lezcano 
On 16/04/2018 11:37, Viresh Kumar wrote:
> On 16-04-18, 09:44, Daniel Lezcano wrote:
>> Because we rely on the number to identify the cluster and flag it
>> 'processed'. The number itself is not important.
> 
> It is, because you are creating multiple groups (like cpufreq
> policies) right now based on cluster id. Which will be zero for all
> the CPUs. So in a big little system we will have two cpufreq cooling
> devices and one cpuidle cooling device.

Can you elaborate a bit ? I'm not sure to get the point.

BTW, Am I asked to change my code to stick to something which is not
merged ?


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Viresh Kumar 
On 16-04-18, 09:44, Daniel Lezcano wrote:
> Because we rely on the number to identify the cluster and flag it
> 'processed'. The number itself is not important.

It is, because you are creating multiple groups (like cpufreq
policies) right now based on cluster id. Which will be zero for all
the CPUs. So in a big little system we will have two cpufreq cooling
devices and one cpuidle cooling device.

-- 
viresh

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Sudeep Holla 
On Mon, Apr 16, 2018 at 09:44:51AM +0200, Daniel Lezcano wrote:
> On 16/04/2018 09:37, Viresh Kumar wrote:
> > On 13-04-18, 13:47, Daniel Lezcano wrote:
> >> Ok, noted. At the first glance, it should not be a problem.
> > 
> > Why do you think it wouldn't be a problem ?
> 
> Because we rely on the number to identify the cluster and flag it
> 'processed'. The number itself is not important.
> 

In that case, why can't cpu_possible_mask be used for simplicity if all
you need is the tracking and that need not be grouped under the "cluster"
banner.

--
Regards,
Sudeep

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Daniel Lezcano 
On 16/04/2018 09:37, Viresh Kumar wrote:
> On 13-04-18, 13:47, Daniel Lezcano wrote:
>> Ok, noted. At the first glance, it should not be a problem.
> 
> Why do you think it wouldn't be a problem ?

Because we rely on the number to identify the cluster and flag it
'processed'. The number itself is not important.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-16 Thread Viresh Kumar 
On 13-04-18, 13:47, Daniel Lezcano wrote:
> Ok, noted. At the first glance, it should not be a problem.

Why do you think it wouldn't be a problem ?

-- 
viresh

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-13 Thread Daniel Lezcano 
On 13/04/2018 13:23, Sudeep Holla wrote:
> Hi Daniel,
> 
> On 05/04/18 17:16, Daniel Lezcano wrote:
> 
> [...]
> 
>> +/**
>> + * cpuidle_cooling_register - Idle cooling device initialization function
>> + *
>> + * This function is in charge of creating a cooling device per cluster
>> + * and register it to thermal framework. For this we rely on the
>> + * topology as there is nothing yet describing better the idle state
>> + * power domains.
>> + *
>> + * We create a cpuidle cooling device per cluster. For this reason we
>> + * must, for each cluster, allocate and initialize the cooling device
>> + * and for each cpu belonging to this cluster, do the initialization
>> + * on a cpu basis.
>> + *
>> + * This approach for creating the cooling device is needed as we don't
>> + * have the guarantee the CPU numbering is sequential.
>> + *
>> + * Unfortunately, there is no API to browse from top to bottom the
>> + * topology, cluster->cpu, only the usual for_each_possible_cpu loop.
>> + * In order to solve that, we use a cpumask to flag the cluster_id we
>> + * already processed. The cpumask will always have enough room for all
>> + * the cluster because it is based on NR_CPUS and it is not possible
>> + * to have more clusters than cpus.
>> + *
>> + */
>> +void __init cpuidle_cooling_register(void)
>> +{
>> +struct cpuidle_cooling_device *idle_cdev = NULL;
>> +struct thermal_cooling_device *cdev;
>> +struct device_node *np;
>> +cpumask_var_t cpumask;
>> +char dev_name[THERMAL_NAME_LENGTH];
>> +int ret = -ENOMEM, cpu;
>> +int cluster_id;
>> +
>> +if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
>> +return;
>> +
>> +for_each_possible_cpu(cpu) {
>> +
>> +cluster_id = topology_physical_package_id(cpu);
>> +if (cpumask_test_cpu(cluster_id, cpumask))
>> +continue;
> 
> Sorry for chiming in randomly, I haven't read the patches in detail.
> But it was brought to my notice that topology_physical_package_id is
> being used for cluster ID here. It's completely wrong and will
> changesoon with ACPI topology related changes Jeremy is working on.
> 
> You will get the physical socket number(which is mostly 0 on single SoC
> system). Makes sure that this won't break with that change.
> 
> Please note with cluster id not defined architecturally, relying on that
> is simply problematic.

Ok, noted. At the first glance, it should not be a problem.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-13 Thread Daniel Lezcano 
On 13/04/2018 13:38, Daniel Thompson wrote:

[ ... ]

>> +/*
>> + * Allocate the cpuidle cooling device with the list
>> + * of the cpus belonging to the cluster.
>> + */
>> +idle_cdev = cpuidle_cooling_alloc(topology_core_cpumask(cpu));
>> +if (!idle_cdev)
>> +goto out;
> 
> Perhaps nitpicking but it might be better to set ret to -ENOMEM here 
> and avoid initializing it during the declarations.
> 
> There's no bug in the current form since ret is never assigned to
> outside of the error paths, but the unwritten assumption that ret keeps
> its original value throughout the loop seems like a bit of a landmine
> w.r.t. future maintainance.

Agree.

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-13 Thread Daniel Thompson 
On Thu, Apr 05, 2018 at 06:16:43PM +0200, Daniel Lezcano wrote:
> +/**
> + * cpuidle_cooling_register - Idle cooling device initialization function
> + *
> + * This function is in charge of creating a cooling device per cluster
> + * and register it to thermal framework. For this we rely on the
> + * topology as there is nothing yet describing better the idle state
> + * power domains.
> + *
> + * We create a cpuidle cooling device per cluster. For this reason we
> + * must, for each cluster, allocate and initialize the cooling device
> + * and for each cpu belonging to this cluster, do the initialization
> + * on a cpu basis.
> + *
> + * This approach for creating the cooling device is needed as we don't
> + * have the guarantee the CPU numbering is sequential.
> + *
> + * Unfortunately, there is no API to browse from top to bottom the
> + * topology, cluster->cpu, only the usual for_each_possible_cpu loop.
> + * In order to solve that, we use a cpumask to flag the cluster_id we
> + * already processed. The cpumask will always have enough room for all
> + * the cluster because it is based on NR_CPUS and it is not possible
> + * to have more clusters than cpus.
> + *
> + */
> +void __init cpuidle_cooling_register(void)
> +{
> + struct cpuidle_cooling_device *idle_cdev = NULL;
> + struct thermal_cooling_device *cdev;
> + struct device_node *np;
> + cpumask_var_t cpumask;
> + char dev_name[THERMAL_NAME_LENGTH];
> + int ret = -ENOMEM, cpu;
> + int cluster_id;
> +
> + if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
> + return;
> +
> + for_each_possible_cpu(cpu) {
> +
> + cluster_id = topology_physical_package_id(cpu);
> + if (cpumask_test_cpu(cluster_id, cpumask))
> + continue;
> +
> + /*
> +  * Allocate the cpuidle cooling device with the list
> +  * of the cpus belonging to the cluster.
> +  */
> + idle_cdev = cpuidle_cooling_alloc(topology_core_cpumask(cpu));
> + if (!idle_cdev)
> + goto out;

Perhaps nitpicking but it might be better to set ret to -ENOMEM here 
and avoid initializing it during the declarations.

There's no bug in the current form since ret is never assigned to
outside of the error paths, but the unwritten assumption that ret keeps
its original value throughout the loop seems like a bit of a landmine
w.r.t. future maintainance.


Daniel.

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-13 Thread Sudeep Holla 
Hi Daniel,

On 05/04/18 17:16, Daniel Lezcano wrote:

[...]

> +/**
> + * cpuidle_cooling_register - Idle cooling device initialization function
> + *
> + * This function is in charge of creating a cooling device per cluster
> + * and register it to thermal framework. For this we rely on the
> + * topology as there is nothing yet describing better the idle state
> + * power domains.
> + *
> + * We create a cpuidle cooling device per cluster. For this reason we
> + * must, for each cluster, allocate and initialize the cooling device
> + * and for each cpu belonging to this cluster, do the initialization
> + * on a cpu basis.
> + *
> + * This approach for creating the cooling device is needed as we don't
> + * have the guarantee the CPU numbering is sequential.
> + *
> + * Unfortunately, there is no API to browse from top to bottom the
> + * topology, cluster->cpu, only the usual for_each_possible_cpu loop.
> + * In order to solve that, we use a cpumask to flag the cluster_id we
> + * already processed. The cpumask will always have enough room for all
> + * the cluster because it is based on NR_CPUS and it is not possible
> + * to have more clusters than cpus.
> + *
> + */
> +void __init cpuidle_cooling_register(void)
> +{
> + struct cpuidle_cooling_device *idle_cdev = NULL;
> + struct thermal_cooling_device *cdev;
> + struct device_node *np;
> + cpumask_var_t cpumask;
> + char dev_name[THERMAL_NAME_LENGTH];
> + int ret = -ENOMEM, cpu;
> + int cluster_id;
> +
> + if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
> + return;
> +
> + for_each_possible_cpu(cpu) {
> +
> + cluster_id = topology_physical_package_id(cpu);
> + if (cpumask_test_cpu(cluster_id, cpumask))
> + continue;

Sorry for chiming in randomly, I haven't read the patches in detail.
But it was brought to my notice that topology_physical_package_id is
being used for cluster ID here. It's completely wrong and will
changesoon with ACPI topology related changes Jeremy is working on.

You will get the physical socket number(which is mostly 0 on single SoC
system). Makes sure that this won't break with that change.

Please note with cluster id not defined architecturally, relying on that
is simply problematic.
-- 
Regards,
Sudeep

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-11 Thread Daniel Lezcano 

Hi Viresh,

thanks for the review.


On 11/04/2018 10:51, Viresh Kumar wrote:

[ ... ]

>> +void __init cpuidle_cooling_register(void)
>> +{
>> +struct cpuidle_cooling_device *idle_cdev = NULL;
>> +struct thermal_cooling_device *cdev;
>> +struct device_node *np;
>> +cpumask_var_t cpumask;
> 
> maybe call it clustermask ?

Yeah, sounds better.

>> +cdev = thermal_of_cooling_device_register(np, dev_name,
>> +  idle_cdev,
>> +  &cpuidle_cooling_ops);
>> +if (IS_ERR(cdev)) {
>> +ret = PTR_ERR(cdev);
>> +goto out;
>> +}
>> +
>> +idle_cdev->cdev = cdev;
>> +cpumask_set_cpu(cluster_id, cpumask);
>> +}
>> +
>> +ret = smpboot_register_percpu_thread(&cpuidle_cooling_threads);
>> +if (ret)
>> +goto out;
>> +
>> +pr_info("Created cpuidle cooling device\n");
>> +out:
>> +free_cpumask_var(cpumask);
>> +
>> +if (ret) {
>> +cpuidle_cooling_unregister();
>> +pr_err("Failed to create idle cooling device (%d)\n", ret);
>> +}
> 
> Maybe rearrange it a bit:
> 
> + ret = smpboot_register_percpu_thread(&cpuidle_cooling_threads);
> +
> +out:
> + if (ret) {
> + cpuidle_cooling_unregister();
> + pr_err("Failed to create idle cooling device (%d)\n", ret);
> + } else {
> + pr_info("Created cpuidle cooling devices\n");
> +   }
> +
> + free_cpumask_var(cpumask);
> 
> ??

I usually tend to avoid using 'else' statements when possible (a coding
style practice) but if you prefer this version I'm fine with that.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-11 Thread Viresh Kumar 
On 05-04-18, 18:16, Daniel Lezcano wrote:
> The cpu idle cooling driver performs synchronized idle injection across all
> cpus belonging to the same cluster and offers a new method to cool down a SoC.
> 
> Each cluster has its own idle cooling device, each core has its own idle
> injection thread, each idle injection thread uses play_idle to enter idle.  In
> order to reach the deepest idle state, each cooling device has the idle
> injection threads synchronized together.
> 
> It has some similarity with the intel power clamp driver but it is actually
> designed to work on the ARM architecture via the DT with a mathematical proof
> with the power model which comes with the Documentation.
> 
> The idle injection cycle is fixed while the running cycle is variable. That
> allows to have control on the device reactivity for the user experience. At
> the mitigation point the idle threads are unparked, they play idle the
> specified amount of time and they schedule themselves. The last thread sets
> the next idle injection deadline and when the timer expires it wakes up all
> the threads which in turn play idle again. Meanwhile the running cycle is
> changed by set_cur_state.  When the mitigation ends, the threads are parked.
> The algorithm is self adaptive, so there is no need to handle hotplugging.
> 
> If we take an example of the balanced point, we can use the DT for the hi6220.
> 
> The sustainable power for the SoC is 3326mW to mitigate at 75°C. Eight cores
> running at full blast at the maximum OPP consumes 5280mW. The first value is
> given in the DT, the second is calculated from the OPP with the formula:
> 
>Pdyn = Cdyn x Voltage^2 x Frequency
> 
> As the SoC vendors don't want to share the static leakage values, we assume
> it is zero, so the Prun = Pdyn + Pstatic = Pdyn + 0 = Pdyn.
> 
> In order to reduce the power to 3326mW, we have to apply a ratio to the
> running time.
> 
> ratio = (Prun - Ptarget) / Ptarget = (5280 - 3326) / 3326 = 0,5874
> 
> We know the idle cycle which is fixed, let's assume 10ms. However from this
> duration we have to substract the wake up latency for the cluster idle state.
> In our case, it is 1.5ms. So for a 10ms latency for idle, we are really idle
> 8.5ms.
> 
> As we know the idle duration and the ratio, we can compute the running cycle.
> 
>running_cycle = 8.5 / 0.5874 = 14.47ms
> 
> So for 8.5ms of idle, we have 14.47ms of running cycle, and that brings the
> SoC to the balanced trip point of 75°C.
> 
> The driver has been tested on the hi6220 and it appears the temperature
> stabilizes at 75°C with an idle injection time of 10ms (8.5ms real) and
> running cycle of 14ms as expected by the theory above.
> 
> Signed-off-by: Kevin Wangtao 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/thermal/Kconfig   |  10 +
>  drivers/thermal/cpu_cooling.c | 479 
> ++
>  include/linux/cpu_cooling.h   |   6 +
>  3 files changed, 495 insertions(+)
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 5aaae1b..6c34117 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -166,6 +166,16 @@ config CPU_FREQ_THERMAL
> This will be useful for platforms using the generic thermal interface
> and not the ACPI interface.
>  
> +config CPU_IDLE_THERMAL
> +   bool "CPU idle cooling strategy"
> +   depends on CPU_IDLE
> +   help
> +  This implements the generic CPU cooling mechanism through
> +  idle injection.  This will throttle the CPU by injecting
> +  fixed idle cycle.  All CPUs belonging to the same cluster
> +  will enter idle synchronously to reach the deepest idle
> +  state.
> +
>  endchoice
>  
>  config CLOCK_THERMAL
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 5c219dc..1eec8d6 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -10,18 +10,33 @@
>   *   Viresh Kumar 
>   *
>   */
> +#define pr_fmt(fmt) "CPU cooling: " fmt
> +
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> +#include 

What part of the code is using this one ?

> +

Why add above 2 blank lines ?

> +#include 
> +#include 
> +
>  #include 
>  
> +#include 
> +
>  #ifdef CONFIG_CPU_FREQ_THERMAL
>  /*
>   * Cooling state <-> CPUFreq frequency
> @@ -928,3 +943,467 @@ void cpufreq_cooling_unregister(struct 
> thermal_cooling_device *cdev)
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
>  #endif /* CONFIG_CPU_FREQ_THERMAL */
> +
> +#ifdef CONFIG_CPU_IDLE_THERMAL
> +/**
> + * struct cpuidle_cooling_device - data for the idle cooling device
> + * @cdev: a pointer to a struct thermal_cooling_device
> + * @cpumask: a cpumask containing the CPU managed by the cooling device
> + * @timer: a hrtimer giving the tempo for the idle injection cycles
>

[PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver

2018-04-05 Thread Daniel Lezcano 
The cpu idle cooling driver performs synchronized idle injection across all
cpus belonging to the same cluster and offers a new method to cool down a SoC.

Each cluster has its own idle cooling device, each core has its own idle
injection thread, each idle injection thread uses play_idle to enter idle.  In
order to reach the deepest idle state, each cooling device has the idle
injection threads synchronized together.

It has some similarity with the intel power clamp driver but it is actually
designed to work on the ARM architecture via the DT with a mathematical proof
with the power model which comes with the Documentation.

The idle injection cycle is fixed while the running cycle is variable. That
allows to have control on the device reactivity for the user experience. At
the mitigation point the idle threads are unparked, they play idle the
specified amount of time and they schedule themselves. The last thread sets
the next idle injection deadline and when the timer expires it wakes up all
the threads which in turn play idle again. Meanwhile the running cycle is
changed by set_cur_state.  When the mitigation ends, the threads are parked.
The algorithm is self adaptive, so there is no need to handle hotplugging.

If we take an example of the balanced point, we can use the DT for the hi6220.

The sustainable power for the SoC is 3326mW to mitigate at 75°C. Eight cores
running at full blast at the maximum OPP consumes 5280mW. The first value is
given in the DT, the second is calculated from the OPP with the formula:

   Pdyn = Cdyn x Voltage^2 x Frequency

As the SoC vendors don't want to share the static leakage values, we assume
it is zero, so the Prun = Pdyn + Pstatic = Pdyn + 0 = Pdyn.

In order to reduce the power to 3326mW, we have to apply a ratio to the
running time.

ratio = (Prun - Ptarget) / Ptarget = (5280 - 3326) / 3326 = 0,5874

We know the idle cycle which is fixed, let's assume 10ms. However from this
duration we have to substract the wake up latency for the cluster idle state.
In our case, it is 1.5ms. So for a 10ms latency for idle, we are really idle
8.5ms.

As we know the idle duration and the ratio, we can compute the running cycle.

   running_cycle = 8.5 / 0.5874 = 14.47ms

So for 8.5ms of idle, we have 14.47ms of running cycle, and that brings the
SoC to the balanced trip point of 75°C.

The driver has been tested on the hi6220 and it appears the temperature
stabilizes at 75°C with an idle injection time of 10ms (8.5ms real) and
running cycle of 14ms as expected by the theory above.

Signed-off-by: Kevin Wangtao 
Signed-off-by: Daniel Lezcano 
---
 drivers/thermal/Kconfig   |  10 +
 drivers/thermal/cpu_cooling.c | 479 ++
 include/linux/cpu_cooling.h   |   6 +
 3 files changed, 495 insertions(+)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 5aaae1b..6c34117 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -166,6 +166,16 @@ config CPU_FREQ_THERMAL
  This will be useful for platforms using the generic thermal interface
  and not the ACPI interface.
 
+config CPU_IDLE_THERMAL
+   bool "CPU idle cooling strategy"
+   depends on CPU_IDLE
+   help
+This implements the generic CPU cooling mechanism through
+idle injection.  This will throttle the CPU by injecting
+fixed idle cycle.  All CPUs belonging to the same cluster
+will enter idle synchronously to reach the deepest idle
+state.
+
 endchoice
 
 config CLOCK_THERMAL
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 5c219dc..1eec8d6 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -10,18 +10,33 @@
  * Viresh Kumar 
  *
  */
+#define pr_fmt(fmt) "CPU cooling: " fmt
+
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+
+#include 
+#include 
+
 #include 
 
+#include 
+
 #ifdef CONFIG_CPU_FREQ_THERMAL
 /*
  * Cooling state <-> CPUFreq frequency
@@ -928,3 +943,467 @@ void cpufreq_cooling_unregister(struct 
thermal_cooling_device *cdev)
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
 #endif /* CONFIG_CPU_FREQ_THERMAL */
+
+#ifdef CONFIG_CPU_IDLE_THERMAL
+/**
+ * struct cpuidle_cooling_device - data for the idle cooling device
+ * @cdev: a pointer to a struct thermal_cooling_device
+ * @cpumask: a cpumask containing the CPU managed by the cooling device
+ * @timer: a hrtimer giving the tempo for the idle injection cycles
+ * @kref: a kernel refcount on this structure
+ * @count: an atomic to keep track of the last task exiting the idle cycle
+ * @idle_cycle: an integer defining the duration of the idle injection
+ * @state: an normalized integer giving the state of the cooling device
+ */
+struct cpuidle_cooling_device {
+   struct thermal_cooling_device *cdev;
+   struct cpum