From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B297CC43603 for ; Fri, 20 Dec 2019 10:08:01 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 68E4924680 for ; Fri, 20 Dec 2019 10:08:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Zt+FIhvt" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 68E4924680 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Z3jHGe8OnJJz2d4ChXywuRnjDRptqIxoeosrV2wZxoY=; b=Zt+FIhvtNRjDmq 1fSiKNVC+OgN8Kp5JaRKcVYTTth/0qxtI/WZD6Jn4AflrWJD3BVBIVRuVFVZgiKWShNC8huS29fZq NLqNbSOCyws7dassBsvntHoMxvxXVnmoEzeKhDD82YO73dMbA9rGbUCgkifIqtTrERPz9LX8xXSo8 tOz+srvRTQ5B927P7u4JGAxCtYV/M7WSVedhUe9Yzqj8h5AswOjWn3Olv5tf6eGq51xgRh3PYy06/ 40U0yyi/L1D/dD+5eu3fKa3V/bL1lyONRNhicZFYOormZVLfnSsz2twD45rR1xr8DMikjlR880hVl fMIzYpbtKNbAw8XsiV9w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iiFCM-0007Ae-Ud; Fri, 20 Dec 2019 10:07:54 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iiFCI-00079K-Dw for linux-arm-kernel@lists.infradead.org; Fri, 20 Dec 2019 10:07:52 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EE1F530E; Fri, 20 Dec 2019 02:07:49 -0800 (PST) Received: from bogus (e107155-lin.cambridge.arm.com [10.1.196.42]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F17EB3F719; Fri, 20 Dec 2019 02:07:47 -0800 (PST) Date: Fri, 20 Dec 2019 10:07:45 +0000 From: Sudeep Holla To: Ulf Hansson Subject: Re: [PATCH v4 13/14] cpuidle: psci: Add support for PM domains by using genpd Message-ID: <20191220100745.GB6731@bogus> References: <20191211154343.29765-1-ulf.hansson@linaro.org> <20191211154343.29765-14-ulf.hansson@linaro.org> <20191219143427.GF20746@bogus> <20191219180629.GC21846@bogus> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191220_020750_565470_B5187B09 X-CRM114-Status: GOOD ( 34.95 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Lorenzo Pieralisi , Linux PM , Stephen Boyd , linux-arm-msm , Daniel Lezcano , "Rafael J . Wysocki" , Andy Gross , Lina Iyer , Bjorn Andersson , Kevin Hilman , Rob Herring , Sudeep Holla , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Dec 19, 2019 at 11:02:40PM +0100, Ulf Hansson wrote: > On Thu, 19 Dec 2019 at 19:06, Sudeep Holla wrote: > > > > On Thu, Dec 19, 2019 at 04:48:39PM +0100, Ulf Hansson wrote: > > > On Thu, 19 Dec 2019 at 15:34, Sudeep Holla wrote: > > > > > > > > On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > > > > > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > > > > > mode is supported by the PSCI FW, let's initialize a corresponding PM > > > > > domain topology by using genpd. This enables a CPU and a group of CPUs, > > > > > when attached to the topology, to be power-managed accordingly. > > > > > > > > > > To trigger the attempt to initialize the genpd data structures let's use a > > > > > subsys_initcall, which should be early enough to allow CPUs, but also other > > > > > devices to be attached. > > > > > > > > > > The initialization consists of parsing the PSCI OF node for the topology > > > > > and the "domain idle states" DT bindings. In case the idle states are > > > > > compatible with "domain-idle-state", the initialized genpd becomes > > > > > responsible of selecting an idle state for the PM domain, via assigning it > > > > > a genpd governor. > > > > > > > > > > Note that, a successful initialization of the genpd data structures, is > > > > > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > > > > > in the PSCI FW. In case this fails, we fall back into a degraded mode > > > > > rather than bailing out and returning an error code. > > > > > > > > > > Co-developed-by: Lina Iyer > > > > > Signed-off-by: Lina Iyer > > > > > Signed-off-by: Ulf Hansson > > > > > --- > > > > > > > > > > Changes in v4: > > > > > - None. > > > > > > > > > > --- > > > > > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > > > > > drivers/cpuidle/cpuidle-psci.c | 4 +- > > > > > drivers/cpuidle/cpuidle-psci.h | 5 + > > > > > 3 files changed, 274 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > index 656ef3d59149..c2f94ba42222 100644 > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > @@ -7,14 +7,281 @@ > > > > > * > > > > > */ > > > > > > > > > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > > > > > + > > > > > #include > > > > > #include > > > > > #include > > > > > #include > > > > > #include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > > > > > > #include "cpuidle-psci.h" > > > > > > > > > > +struct psci_pd_provider { > > > > > + struct list_head link; > > > > > + struct device_node *node; > > > > > +}; > > > > > + > > > > > +static LIST_HEAD(psci_pd_providers); > > > > > +static bool osi_mode_enabled; > > > > > + > > > > > +static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > +{ > > > > > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > > > > > + u32 *pd_state; > > > > > + > > > > > + /* If we have failed to enable OSI mode, then abort power off. */ > > > > > + if (!osi_mode_enabled) > > > > > + return -EBUSY; > > > > > + > > > > > > > > Why is above check needed ? Shouldn't we have disable/remove pd of > > > > OSI is not enabled ? > > > > > > Well, failing to enable OSI should in practice not happen, while it > > > theoretically it could. > > > > > > > I won't assume that. Since it's new and not tested yet, I prefer to assume > > it can fail. > > Yes, I agree. Hence the degraded mode. > > > > > > My approach to this has been to fall back to use a "degraded mode", > > > which seems quite common for these kind of situations. The degraded > > > mode means, we are preventing domain states from being used. > > > > > > > But why can't we just fail registering or remove if already added. > > We can, but there are more problems with that than leaving this in a > degraded mode, I think. See more below. > > > They are useless for "degraded mode" anyways. And it will ensure that > > data->dev is NULL. Sorry now I see why you said it can be NULL but I > > would rather not leave those unused genpd in place in case of error. > > data->dev would not be NULL in this case, because the > dev_pm_domain_attach_by_name() which is called when we attach the CPU > is going to return an error code, not NULL. > > That's because the connection is there in the DTB and thus it must > fail, in this case it would be with -EPROBE_DEFER (waiting for a genpd > provider to be registered). > > That would then lead to that the entire cpuidle-psci driver would fail > to initiate/probe. In my opinion, I think it's better to fall back > into a degraded mode, using all the idle states for the CPUs, but just > preventing the cluster idle states. > > Just wanted to make this more clear for you to consider. I am happy to > change in any way you suggest, but please confirm that you really want > another behaviour than the degraded mode. > Sorry but if OSI set failed in firmware, it will be operating in default/ PC mode and I *don't* want to create genpd for that. It's confusing. Even if you don't create all these genpd domains, it is still degraded mode and we are anyway not changing that. Let me know if my understanding is wrong here. I am sure, DTB may get copied to different platform and the firmware may not support OSI. I know we have logs, but creating and leaving those genpd domains unused will be just confusing. Please change that. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel