On 05/07/2015 04:05 PM, Shilpasri G Bhat wrote:
> 
> 
> On 05/05/2015 02:08 PM, Preeti U Murthy wrote:
>> On 05/05/2015 11:36 AM, Shilpasri G Bhat wrote:
>>> Hi Preeti,
>>>
>>> On 05/05/2015 09:21 AM, Preeti U Murthy wrote:
>>>> Hi Shilpa,
>>>>
>>>> On 05/04/2015 02:24 PM, Shilpasri G Bhat wrote:
>>>>> The On-Chip-Controller(OCC) can throttle cpu frequency by reducing the
>>>>> max allowed frequency for that chip if the chip exceeds its power or
>>>>> temperature limits. As Pmax capping is a chip level condition report
>>>>> this throttling behavior at chip level and also do not set the global
>>>>> 'throttled' on Pmax capping instead set the per-chip throttled
>>>>> variable. Report unthrottling if Pmax is restored after throttling.
>>>>>
>>>>> This patch adds a structure to store chip id and throttled state of
>>>>> the chip.
>>>>>
>>>>> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
>>>>> ---
>>>>>  drivers/cpufreq/powernv-cpufreq.c | 59 
>>>>> ++++++++++++++++++++++++++++++++++++---
>>>>>  1 file changed, 55 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
>>>>> b/drivers/cpufreq/powernv-cpufreq.c
>>>>> index ebef0d8..d0c18c9 100644
>>>>> --- a/drivers/cpufreq/powernv-cpufreq.c
>>>>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>>>>> @@ -27,6 +27,7 @@
>>>>>  #include <linux/smp.h>
>>>>>  #include <linux/of.h>
>>>>>  #include <linux/reboot.h>
>>>>> +#include <linux/slab.h>
>>>>>
>>>>>  #include <asm/cputhreads.h>
>>>>>  #include <asm/firmware.h>
>>>>> @@ -42,6 +43,13 @@
>>>>>  static struct cpufreq_frequency_table 
>>>>> powernv_freqs[POWERNV_MAX_PSTATES+1];
>>>>>  static bool rebooting, throttled;
>>>>>
>>>>> +static struct chip {
>>>>> + unsigned int id;
>>>>> + bool throttled;
>>>>> +} *chips;
>>>>> +
>>>>> +static int nr_chips;
>>>>> +
>>>>>  /*
>>>>>   * Note: The set of pstates consists of contiguous integers, the
>>>>>   * smallest of which is indicated by powernv_pstate_info.min, the
>>>>> @@ -301,22 +309,33 @@ static inline unsigned int get_nominal_index(void)
>>>>>  static void powernv_cpufreq_throttle_check(unsigned int cpu)
>>>>>  {
>>>>>   unsigned long pmsr;
>>>>> - int pmsr_pmax, pmsr_lp;
>>>>> + int pmsr_pmax, pmsr_lp, i;
>>>>>
>>>>>   pmsr = get_pmspr(SPRN_PMSR);
>>>>>
>>>>> + for (i = 0; i < nr_chips; i++)
>>>>> +         if (chips[i].id == cpu_to_chip_id(cpu))
>>>>> +                 break;
>>>>> +
>>>>>   /* Check for Pmax Capping */
>>>>>   pmsr_pmax = (s8)PMSR_MAX(pmsr);
>>>>>   if (pmsr_pmax != powernv_pstate_info.max) {
>>>>> -         throttled = true;
>>>>> -         pr_info("CPU %d Pmax is reduced to %d\n", cpu, pmsr_pmax);
>>>>> -         pr_info("Max allowed Pstate is capped\n");
>>>>> +         if (chips[i].throttled)
>>>>> +                 goto next;
>>>>> +         chips[i].throttled = true;
>>>>> +         pr_info("CPU %d on Chip %u has Pmax reduced to %d\n", cpu,
>>>>> +                 chips[i].id, pmsr_pmax);
>>>>> + } else if (chips[i].throttled) {
>>>>> +         chips[i].throttled = false;
>>>>
>>>> Is this check on pmax sufficient to indicate that the chip is unthrottled ?
>>>
>>> Unthrottling due to Pmax uncapping here is specific to a chip. So it is
>>> sufficient to decide throttling/unthrottling when OCC is active for that 
>>> chip.
>>
>> Ok then we can perhaps exit after detecting unthrottling here.
> 
> This won't work for older firmwares which do not clear "Frequency control
> enabled bit" on OCC reset cycle. So let us check for remaining two conditions 
> on
> unthrottling as well.

ok.

> 
>>>
>>>>
>>>>> +         pr_info("CPU %d on Chip %u has Pmax restored to %d\n", cpu,
>>>>> +                 chips[i].id, pmsr_pmax);
>>>>>   }
>>>>>
>>>>>   /*
>>>>>    * Check for Psafe by reading LocalPstate
>>>>>    * or check if Psafe_mode_active is set in PMSR.
>>>>>    */
>>>>> +next:
>>>>>   pmsr_lp = (s8)PMSR_LP(pmsr);
>>>>>   if ((pmsr_lp < powernv_pstate_info.min) ||
>>>>>                           (pmsr & PMSR_PSAFE_ENABLE)) {
>>>>> @@ -414,6 +433,33 @@ static struct cpufreq_driver powernv_cpufreq_driver 
>>>>> = {
>>>>>   .attr           = powernv_cpu_freq_attr,
>>>>
>>>> What about the situation where although occ is active, this particular
>>>> chip has been throttled and we end up repeatedly reporting "pstate set
>>>> to safe" and "frequency control disabled from OS" ? Should we not have a
>>>> check on (chips[i].throttled) before reporting an anomaly for these two
>>>> scenarios as well just like you have for pmsr_pmax ?
>>>
>>> We will not have "Psafe" and "frequency control disabled" repeatedly printed
>>> because of global variable 'throttled', which is set to true on passing any 
>>> of
>>> these two conditions.
>>>
>>> It is quite unlikely behavior to have only one chip in "Psafe" or "frequency
>>> control disabled" state. These two conditions are most likely to happen 
>>> during
>>> an OCC reset cycle which will occur across all chips.
>>
>> Let us then add a comment to indicate that Psafe and frequency control
>> disabled conditions will fail *only if OCC is inactive* and not
>> otherwise and that this is a system wide phenomenon.
>>
> 
> I agree that adding a comment here will clear global vs local throttling
> scenarios, but this will contradict the architectural design of OCC wherein it
> can independently go to "Psafe" and "frequency control disabled" state. It is
> the implementation in FSP today that has made the above two states global. My
> point is adding a comment here may be confusing if at all for the future
> firmwares this implementation is changed. Having said that the current patch 
> set
> still seems fit for the newer implementation for the following reason:
> 1) The aim here is to identify any sort of throttling and report it to the 
> user
> with least flooding of error messages, which will happen even if OCC can
> independently reset and restore.
> 2) On unthrottling verify throttling on the chips with the exception of Pmax
> capping is also taken care by this patch set.

Ok as long as we are reporting right, its fine.

Reviewed-by: Preeti U Murthy <pre...@linux.vnet.ibm.com>
> 
> Thanks and Regards,
> Shilpa
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to