Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752700AbbEGMPa (ORCPT ); Thu, 7 May 2015 08:15:30 -0400 Received: from e18.ny.us.ibm.com ([129.33.205.208]:47068 "EHLO e18.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751967AbbEGMP0 (ORCPT ); Thu, 7 May 2015 08:15:26 -0400 Message-ID: <554B5754.6010902@linux.vnet.ibm.com> Date: Thu, 07 May 2015 17:45:16 +0530 From: Preeti U Murthy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Shilpasri G Bhat , linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org CC: viresh.kumar@linaro.org, rjw@rjwysocki.net, linux-pm@vger.kernel.org Subject: Re: [PATCH v3 1/6] cpufreq: poowernv: Handle throttling due to Pmax capping at chip level References: <1430729652-14813-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com> <1430729652-14813-2-git-send-email-shilpa.bhat@linux.vnet.ibm.com> <55483E4B.4080405@linux.vnet.ibm.com> <55485DE9.4070609@linux.vnet.ibm.com> <55488188.9070908@linux.vnet.ibm.com> <554B3FE9.7050908@linux.vnet.ibm.com> In-Reply-To: <554B3FE9.7050908@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15050712-0045-0000-0000-0000001FE912 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6372 Lines: 160 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 >>>>> --- >>>>> 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include >>>>> #include >>>>> @@ -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 > > Thanks and Regards, > Shilpa > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/