From patchwork Tue May 30 07:13:57 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicholas Piggin X-Patchwork-Id: 768417 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wcPyd1TZ6z9ryv for ; Tue, 30 May 2017 17:15:37 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="OcZeZosT"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3wcPyd0LLyzDqCv for ; Tue, 30 May 2017 17:15:37 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="OcZeZosT"; dkim-atps=neutral X-Original-To: linuxppc-dev@lists.ozlabs.org Delivered-To: linuxppc-dev@lists.ozlabs.org Received: from mail-pf0-x241.google.com (mail-pf0-x241.google.com [IPv6:2607:f8b0:400e:c00::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3wcPx06nYkzDq5x for ; Tue, 30 May 2017 17:14:12 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="OcZeZosT"; dkim-atps=neutral Received: by mail-pf0-x241.google.com with SMTP id f27so15970988pfe.0 for ; Tue, 30 May 2017 00:14:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :organization:mime-version:content-transfer-encoding; bh=epRPpZb2WIiV+Buco9REMRELgNF3hMXb1gVWNJvO8rA=; b=OcZeZosTms61sxAVI226mjCaVBa27TfbxZ8gbd5rR0WMRRCadRXEONcsa2TPeSOPnZ h8ZVb0t9c1Jafr6bk6Uk18bLlJZzqdVtFn5P9v9JAxKIcWwDUyopDr2JijPNglJ2bnna J8IfiGSdKzjBoHJUJ5Y2PjrpqlusbiREN8/Mrv/fGo5TU9pn/9TRoPSL2g2tVKw2pBNc f1nyut4HczgPBopZU0Tt5I/WmiqzJnq3UXTSm7w4pMnQtQW5LOIAIobZF6D9Yzzc0/rB zkg2xhR6JwtUXkfRD71jSx08/jwhdQLHLDZl5nNB1pQUyYM3nTQYHu8ASGci9KSpMdXh +5vA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=epRPpZb2WIiV+Buco9REMRELgNF3hMXb1gVWNJvO8rA=; b=modUR2CtYO6xHk+WoSm85QRlxR1HjI7+3SqzBJbmip/5Zh5nUgAyay6FsgE6EbLk4H hcLbZ/ei9kIldOq4F0NrL/mlLMGvIKsgUkxID4vYwTyNu43lpKTkdMvJ1PvkcwzPxAK+ vcjNvaP9GHui3UUcMKdKeS3bxYbZTw+ITX7q6PtdLw8e5+FB/LND21bsy8gFFzhjaOIH RBcllLshrxgHoj/9gl7tMGvrk3in9fkPSakoMPvBti1APEYO2kHXtJFPD2u6ADEupaR0 mv57dfSL4wXoMWo3JDsvws2hKO7J/kg8CITEbSw/sWyYJo23jA1z0KOi5xEQ4LxMQ/as 4IXQ== X-Gm-Message-State: AODbwcA9LXr1VZcSwPnDfjcZ8XNtZhgNW62fgu/e9PYtdm2UZXuUUVWV DFJxeeEs8VS4KbS6 X-Received: by 10.84.238.15 with SMTP id u15mr49515499plk.126.1496128450638; Tue, 30 May 2017 00:14:10 -0700 (PDT) Received: from roar.ozlabs.ibm.com (14-202-186-188.tpgi.com.au. [14.202.186.188]) by smtp.gmail.com with ESMTPSA id i68sm21252276pfi.72.2017.05.30.00.14.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 30 May 2017 00:14:09 -0700 (PDT) Date: Tue, 30 May 2017 17:13:57 +1000 From: Nicholas Piggin To: "Gautham R. Shenoy" Subject: Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time Message-ID: <20170530171357.4e0b87a7@roar.ozlabs.ibm.com> In-Reply-To: <3429547945465851cfcbf81f6e762037c395c8ac.1494585671.git.ego@linux.vnet.ibm.com> References: <3429547945465851cfcbf81f6e762037c395c8ac.1494585671.git.ego@linux.vnet.ibm.com> Organization: IBM X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Michael Neuling , linux-kernel@vger.kernel.org, Shilpasri G Bhat , linuxppc-dev@lists.ozlabs.org, Akshay Adiga Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Tue, 16 May 2017 14:19:48 +0530 "Gautham R. Shenoy" wrote: > From: "Gautham R. Shenoy" > > The current code in the cpuidle-powernv intialization only allows deep > stop states (indicated by OPAL_PM_STOP_INST_DEEP) which lose timebase > (indicated by OPAL_PM_TIMEBASE_STOP). This assumption goes back to > POWER8 time where deep states used to lose the timebase. However, on > POWER9, we do have stop states that are deep (they lose hypervisor > state) but retain the timebase. > > Fix the initialization code in the cpuidle-powernv driver to allow > such deep states. > > Further, there is a bug in cpuidle-powernv driver with > CONFIG_TICK_ONESHOT=n where we end up incrementing the nr_idle_states > even if a platform idle state which loses time base was not added to > the cpuidle table. > > Fix this by ensuring that the nr_idle_states variable gets incremented > only when the platform idle state was added to the cpuidle table. Should this be a separate patch? Stable? > > Signed-off-by: Gautham R. Shenoy > --- > drivers/cpuidle/cpuidle-powernv.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c > index 12409a5..45eaf06 100644 > --- a/drivers/cpuidle/cpuidle-powernv.c > +++ b/drivers/cpuidle/cpuidle-powernv.c > @@ -354,6 +354,7 @@ static int powernv_add_idle_states(void) > > for (i = 0; i < dt_idle_states; i++) { > unsigned int exit_latency, target_residency; > + bool stops_timebase = false; > /* > * If an idle state has exit latency beyond > * POWERNV_THRESHOLD_LATENCY_NS then don't use it > @@ -381,6 +382,9 @@ static int powernv_add_idle_states(void) > } > } > > + if (flags[i] & OPAL_PM_TIMEBASE_STOP) > + stops_timebase = true; > + > /* > * For nap and fastsleep, use default target_residency > * values if f/w does not expose it. > @@ -392,8 +396,7 @@ static int powernv_add_idle_states(void) > add_powernv_state(nr_idle_states, "Nap", > CPUIDLE_FLAG_NONE, nap_loop, > target_residency, exit_latency, 0, 0); > - } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) && > - !(flags[i] & OPAL_PM_TIMEBASE_STOP)) { > + } else if (has_stop_states && !stops_timebase) { > add_powernv_state(nr_idle_states, names[i], > CPUIDLE_FLAG_NONE, stop_loop, > target_residency, exit_latency, > @@ -405,8 +408,8 @@ static int powernv_add_idle_states(void) > * within this config dependency check. > */ > #ifdef CONFIG_TICK_ONESHOT > - if (flags[i] & OPAL_PM_SLEEP_ENABLED || > - flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) { > + else if (flags[i] & OPAL_PM_SLEEP_ENABLED || > + flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) { Hmm, seems okay but readability is isn't the best with the ifdef and mixing power8 and 9 cases IMO. Particularly with the nice regular POWER9 states, we're not doing much logic in this loop besides checking for the timebase stop flag, right? Would it be clearer if it was changed to something like this (untested quick hack)? --- drivers/cpuidle/cpuidle-powernv.c | 76 +++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index 12409a519cc5..77291389f9ac 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -353,7 +353,9 @@ static int powernv_add_idle_states(void) } for (i = 0; i < dt_idle_states; i++) { + unsigned int cpuidle_flags = CPUIDLE_FLAG_NONE; unsigned int exit_latency, target_residency; + /* * If an idle state has exit latency beyond * POWERNV_THRESHOLD_LATENCY_NS then don't use it @@ -371,6 +373,16 @@ static int powernv_add_idle_states(void) else target_residency = 0; + if (flags[i] & OPAL_PM_TIMEBASE_STOP) { + /* + * All cpuidle states with CPUIDLE_FLAG_TIMER_STOP set + * depend on CONFIG_TICK_ONESHOT. + */ + if (!IS_ENABLED(CONFIG_TICK_ONESHOT)) + continue; + cpuidle_flags = CPUIDLE_FLAG_TIMER_STOP; + } + if (has_stop_states) { int err = validate_psscr_val_mask(&psscr_val[i], &psscr_mask[i], @@ -379,50 +391,36 @@ static int powernv_add_idle_states(void) report_invalid_psscr_val(psscr_val[i], err); continue; } - } - /* - * For nap and fastsleep, use default target_residency - * values if f/w does not expose it. - */ - if (flags[i] & OPAL_PM_NAP_ENABLED) { - if (!rc) - target_residency = 100; - /* Add NAP state */ - add_powernv_state(nr_idle_states, "Nap", - CPUIDLE_FLAG_NONE, nap_loop, - target_residency, exit_latency, 0, 0); - } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) && - !(flags[i] & OPAL_PM_TIMEBASE_STOP)) { add_powernv_state(nr_idle_states, names[i], - CPUIDLE_FLAG_NONE, stop_loop, - target_residency, exit_latency, - psscr_val[i], psscr_mask[i]); - } - - /* - * All cpuidle states with CPUIDLE_FLAG_TIMER_STOP set must come - * within this config dependency check. - */ -#ifdef CONFIG_TICK_ONESHOT - if (flags[i] & OPAL_PM_SLEEP_ENABLED || - flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) { - if (!rc) - target_residency = 300000; - /* Add FASTSLEEP state */ - add_powernv_state(nr_idle_states, "FastSleep", - CPUIDLE_FLAG_TIMER_STOP, - fastsleep_loop, - target_residency, exit_latency, 0, 0); - } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) && - (flags[i] & OPAL_PM_TIMEBASE_STOP)) { - add_powernv_state(nr_idle_states, names[i], - CPUIDLE_FLAG_TIMER_STOP, stop_loop, + cpuidle_flags, stop_loop, target_residency, exit_latency, psscr_val[i], psscr_mask[i]); + nr_idle_states++; + } else { + /* + * For nap and fastsleep, use default target_residency + * values if f/w does not expose it. + */ + if (flags[i] & OPAL_PM_NAP_ENABLED) { + if (!rc) + target_residency = 100; + /* Add NAP state */ + add_powernv_state(nr_idle_states, "Nap", + cpuidle_flags, nap_loop, + target_residency, exit_latency, 0, 0); + nr_idle_states++; + } else if (flags[i] & (OPAL_PM_SLEEP_ENABLED | + OPAL_PM_SLEEP_ENABLED_ER1)) { + if (!rc) + target_residency = 300000; + /* Add FASTSLEEP state */ + add_powernv_state(nr_idle_states, "FastSleep", + cpuidle_flags, fastsleep_loop, + target_residency, exit_latency, 0, 0); + nr_idle_states++; + } } -#endif - nr_idle_states++; } out: return nr_idle_states;