Message ID | f709e9e273004be43efe3a2854a7e7b51a777f99.1697710527.git.viresh.kumar@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2010:b0:403:3b70:6f57 with SMTP id fe16csp281922vqb; Thu, 19 Oct 2023 03:22:52 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHROGrS4nZzZ+gdrKhwmwqRyMEp2SSrdCPiVbBxmSU3Zxp7penhc9ph1oLHZVjNJG9XAVjm X-Received: by 2002:a17:90b:118:b0:27d:bc2:7c6e with SMTP id p24-20020a17090b011800b0027d0bc27c6emr1675079pjz.20.1697710971843; Thu, 19 Oct 2023 03:22:51 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697710971; cv=none; d=google.com; s=arc-20160816; b=U2iJT6gmTQCLaz1rmiUcC50ch8XSt79j7p//XBKWVHILzFQEhv/pnuckrhvNJMAkqD 9ppq8Yd8f6SGpLhwt5fh3zPIPy8HVCMkUovp2mE+uW93cQjl++pyWNpxH51eEodoDuou U/xGFxUMjCP1FdfxPNeFdx0jaXsJ/3AvUbSeJpHL/GlSZxqQF5IeJLWjgGmuPAJUx35V qoM6yiXXtymjrjiPCSGy3MxjUCtsXuYKGgSBo0RNkuDT08HreMqEppAz6kDMHHp08qJQ /VdTtv49Fqo7C2uWlm/SXyY5/OezaYAjuMBB7mq7Rcb1xevPcGU3dI9688SOvstaHlKD dcCw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=hmjA0HEYlUuwJKncR5UsQ51jJen21NHSaXoRnKSgrrY=; fh=lVFod2Nh/bVLd9f3LnZD5svEoMpRA0jQsUMvfqIGMU8=; b=tElZnr/vQIPFH5qhvu3MHEXLGcyt+JsHFUBIBYTyA8MWqzmYfcwN+V7ZfgRhC1EPXV Gdbv6YHj67s1LO3vfMfVF1D+NbbGzwZqDv2gRQaKuUimt3y+NkXrHSK0S0j4VfzzVXkK N6JLfHWa46HDryUB1sy6MMgScARcCZ24w7OQ4ZgL4WWpPbixa2FjsOaU99C76fBxDEnd ckHLjxEbaSRlRxuot5vTf0Tn1B8L2Zov6E8vWX/0/mLlQuSpvwr5RtI2mfmsEJ8IcC4N 2XMxebFRu6Wga7vPAqU4ETpQOkQVAgAFeyYpXSEgQapPTt8fUgO6dj1pEpPHE6BtWifk nGTg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=VJ8GXYbi; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id v19-20020a17090ac91300b0027e022bd420si664841pjt.77.2023.10.19.03.22.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Oct 2023 03:22:51 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=VJ8GXYbi; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 95E4E811F26F; Thu, 19 Oct 2023 03:22:49 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345274AbjJSKWR (ORCPT <rfc822;zwp10758@gmail.com> + 24 others); Thu, 19 Oct 2023 06:22:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48188 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345249AbjJSKWP (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 19 Oct 2023 06:22:15 -0400 Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4E044FA for <linux-kernel@vger.kernel.org>; Thu, 19 Oct 2023 03:22:13 -0700 (PDT) Received: by mail-pj1-x1030.google.com with SMTP id 98e67ed59e1d1-27d0251d305so4784253a91.2 for <linux-kernel@vger.kernel.org>; Thu, 19 Oct 2023 03:22:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1697710933; x=1698315733; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=hmjA0HEYlUuwJKncR5UsQ51jJen21NHSaXoRnKSgrrY=; b=VJ8GXYbixIqdEPgD7WbhMfgUrcvQOnLomDlFXdlE1s2/8hF7HwrQbxChbqkSJ8L6/n Sdpz+o5NMw8diFBAZvIniynKBeFbbpW6BpqYRQBPqMqbar528XYSs5Gvd6/jYTUqB5Ix 2mWilVgA+Pgxlz69EeA4o9ZTmk+D3iNb5DVkScLkKWLo4qE5d6AkyutzAfVsowaKnRnN WNlNihmb8kp5+/LTfYvbFbuIwFnbbP6J4S71YGHQqM/KUg/w3yktq+unwsEZzFi9YrBG buAL6DDxG5IXIqI9JuHGFiojQqawVXtV27UW+EQXgUBurZ5Fug7mJ1Swv0lQPf+sHVcG SNQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697710933; x=1698315733; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hmjA0HEYlUuwJKncR5UsQ51jJen21NHSaXoRnKSgrrY=; b=apryUP8VO6RVyR95/JmQ0qBINI/+xxc2AZ79CnpVYSdjKrjHBqJij3ukCzqWiH6jjF X1XL7ozO4DK9sXWD4rsWgJ3qFRAUAHCy1D4B+zwx/tlvVteqPJ/sXg2D+3cN3ZWz11r9 +Ni1i2f3rd/VmYOSE/YrIlFEGQloMZc+Aza3VmJw+pyOkbXeK70YlHvY5e3zjVlJHvzQ Vl1ZgRNA+2p1TMnSmz0rNUSP6HFDoys1pm9SXTzgMoJNzdzAYCMnZ3hHifQeS0niKL6b xtFWmAqCSOL+AFpv4IH767VkMOyyKdya3UlT/WMMsFsCKOQJb53vsZkeWk/tIyOlytFM adnQ== X-Gm-Message-State: AOJu0Yzv3DpbuPffK5TbkDtt1FfFxxjIkY3C5Pqfxu7eVMkFTQjfTfNl 1C5PoW0Gs0T5W5TgncGsWaRwdw== X-Received: by 2002:a17:90b:3793:b0:27d:2364:44f6 with SMTP id mz19-20020a17090b379300b0027d236444f6mr1559112pjb.6.1697710932749; Thu, 19 Oct 2023 03:22:12 -0700 (PDT) Received: from localhost ([122.172.80.14]) by smtp.gmail.com with ESMTPSA id 20-20020a17090a005400b0026cecddfc58sm1430945pjb.42.2023.10.19.03.22.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Oct 2023 03:22:12 -0700 (PDT) From: Viresh Kumar <viresh.kumar@linaro.org> To: Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>, Stephen Boyd <sboyd@kernel.org> Cc: Viresh Kumar <viresh.kumar@linaro.org>, linux-pm@vger.kernel.org, Vincent Guittot <vincent.guittot@linaro.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Ulf Hansson <ulf.hansson@linaro.org>, Stephan Gerhold <stephan.gerhold@kernkonzept.com>, Konrad Dybcio <konrad.dybcio@linaro.org>, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, linux-kernel@vger.kernel.org Subject: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case Date: Thu, 19 Oct 2023 15:52:00 +0530 Message-Id: <f709e9e273004be43efe3a2854a7e7b51a777f99.1697710527.git.viresh.kumar@linaro.org> X-Mailer: git-send-email 2.31.1.272.g89b43f80a514 In-Reply-To: <cover.1697710527.git.viresh.kumar@linaro.org> References: <cover.1697710527.git.viresh.kumar@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Thu, 19 Oct 2023 03:22:49 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780178979554134036 X-GMAIL-MSGID: 1780178979554134036 |
Series |
OPP: Simplify required-opp handling
|
|
Commit Message
Viresh Kumar
Oct. 19, 2023, 10:22 a.m. UTC
There are two genpd (as required-opp) cases that we need to handle,
devices with a single genpd and ones with multiple genpds.
The multiple genpds case is clear, where the OPP core calls
dev_pm_domain_attach_by_name() for them and uses the virtual devices
returned by this helper to call dev_pm_domain_set_performance_state()
later to change the performance state.
The single genpd case however requires special handling as we need to
use the same `dev` structure (instead of a virtual one provided by genpd
core) for setting the performance state via
dev_pm_domain_set_performance_state().
As we move towards more generic code to take care of the required OPPs,
where we will recursively call dev_pm_opp_set_opp() for all the required
OPPs, the above special case becomes a problem.
Eventually we want to handle all performance state changes via
_set_opp_level(), so lets move the single genpd case to that right away.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 6 ++++--
drivers/opp/of.c | 25 ++++++++++++++++++++++---
2 files changed, 26 insertions(+), 5 deletions(-)
Comments
On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > There are two genpd (as required-opp) cases that we need to handle, > devices with a single genpd and ones with multiple genpds. > > The multiple genpds case is clear, where the OPP core calls > dev_pm_domain_attach_by_name() for them and uses the virtual devices > returned by this helper to call dev_pm_domain_set_performance_state() > later to change the performance state. > > The single genpd case however requires special handling as we need to > use the same `dev` structure (instead of a virtual one provided by genpd > core) for setting the performance state via > dev_pm_domain_set_performance_state(). > > As we move towards more generic code to take care of the required OPPs, > where we will recursively call dev_pm_opp_set_opp() for all the required > OPPs, the above special case becomes a problem. > > Eventually we want to handle all performance state changes via > _set_opp_level(), so lets move the single genpd case to that right away. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/core.c | 6 ++++-- > drivers/opp/of.c | 25 ++++++++++++++++++++++--- > 2 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 84f345c69ea5..aab8c8e79146 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1074,10 +1074,12 @@ static int _opp_set_required_opps_generic(struct device *dev, > static int _opp_set_required_opps_genpd(struct device *dev, > struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) > { > - struct device **genpd_virt_devs = > - opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev; > + struct device **genpd_virt_devs = opp_table->genpd_virt_devs; > int index, target, delta, ret; > > + if (!genpd_virt_devs) > + return 0; > + > /* Scaling up? Set required OPPs in normal order, else reverse */ > if (!scaling_down) { > index = 0; > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 81fa27599d58..e056f31a48b5 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -296,7 +296,7 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp) > of_node_put(opp->np); > } > > -static int _link_required_opps(struct dev_pm_opp *opp, > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, > struct opp_table *required_table, int index) > { > struct device_node *np; > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, > return -ENODEV; > } > > + /* > + * There are two genpd (as required-opp) cases that we need to handle, > + * devices with a single genpd and ones with multiple genpds. > + * > + * The single genpd case requires special handling as we need to use the > + * same `dev` structure (instead of a virtual one provided by genpd > + * core) for setting the performance state. Lets treat this as a case > + * where the OPP's level is directly available without required genpd > + * link in the DT. > + * > + * Just update the `level` with the right value, which > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > + */ > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > + !opp_table->genpd_virt_devs) { > + if (!WARN_ON(opp->level)) Hmm. Doesn't this introduce an unnecessary limitation? An opp node that has a required-opps phande, may have "opp-hz", "opp-microvolt", etc. Why would we not allow the "opp-level" to be used too? > + opp->level = opp->required_opps[0]->level; > + } > + > return 0; > } > > @@ -338,7 +357,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table, > if (IS_ERR_OR_NULL(required_table)) > continue; > > - ret = _link_required_opps(opp, required_table, i); > + ret = _link_required_opps(opp, opp_table, required_table, i); > if (ret) > goto free_required_opps; > } > @@ -359,7 +378,7 @@ static int lazy_link_required_opps(struct opp_table *opp_table, > int ret; > > list_for_each_entry(opp, &opp_table->opp_list, node) { > - ret = _link_required_opps(opp, new_table, index); > + ret = _link_required_opps(opp, opp_table, new_table, index); > if (ret) > return ret; > } Kind regards Uffe
On 19-10-23, 13:16, Ulf Hansson wrote: > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > + /* > > + * There are two genpd (as required-opp) cases that we need to handle, > > + * devices with a single genpd and ones with multiple genpds. > > + * > > + * The single genpd case requires special handling as we need to use the > > + * same `dev` structure (instead of a virtual one provided by genpd > > + * core) for setting the performance state. Lets treat this as a case > > + * where the OPP's level is directly available without required genpd > > + * link in the DT. > > + * > > + * Just update the `level` with the right value, which > > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > > + */ > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > + !opp_table->genpd_virt_devs) { > > + if (!WARN_ON(opp->level)) > > Hmm. Doesn't this introduce an unnecessary limitation? > > An opp node that has a required-opps phande, may have "opp-hz", > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > used too? Such platforms need to call dev_pm_opp_set_config() with genpd names and it should all work just fine. The point is that we can't use the same `dev` pointer with another OPP table, i.e. device's dev pointer for the genpd's table here.
On Fri, 20 Oct 2023 at 05:45, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-10-23, 13:16, Ulf Hansson wrote: > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > + /* > > > + * There are two genpd (as required-opp) cases that we need to handle, > > > + * devices with a single genpd and ones with multiple genpds. > > > + * > > > + * The single genpd case requires special handling as we need to use the > > > + * same `dev` structure (instead of a virtual one provided by genpd > > > + * core) for setting the performance state. Lets treat this as a case > > > + * where the OPP's level is directly available without required genpd > > > + * link in the DT. > > > + * > > > + * Just update the `level` with the right value, which > > > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > > > + */ > > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > > + !opp_table->genpd_virt_devs) { > > > + if (!WARN_ON(opp->level)) > > > > Hmm. Doesn't this introduce an unnecessary limitation? > > > > An opp node that has a required-opps phande, may have "opp-hz", > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > > used too? > > Such platforms need to call dev_pm_opp_set_config() with genpd names > and it should all work just fine. The point is that we can't use the > same `dev` pointer with another OPP table, i.e. device's dev pointer > for the genpd's table here. For the single PM domain case, consumer drivers are often not able to use dev_pm_opp_set_config(). That's because the PM domain has already been attached from some of the generic buses, through dev_pm_domain_attach(). In this case, as dev_pm_opp_set_config() ends up trying to attach again, via dev_pm_domain_attach_by_name() it would receive "ERR_PTR(-EEXIST)". Or maybe I didn't quite understand your point? Kind regards Uffe
On 20-10-23, 12:02, Ulf Hansson wrote: > For the single PM domain case, consumer drivers are often not able to > use dev_pm_opp_set_config(). That's because the PM domain has already > been attached from some of the generic buses, through > dev_pm_domain_attach(). > > In this case, as dev_pm_opp_set_config() ends up trying to attach > again, via dev_pm_domain_attach_by_name() it would receive > "ERR_PTR(-EEXIST)". > > Or maybe I didn't quite understand your point? So the thing is that I _really_ want to call dev_pm_opp_set_opp() for each OPP we want to configure, primary or required. For example, the required OPP may want to do more than just performance state and we aren't touching them right now. Now, in order to call dev_pm_opp_set_opp() for any device, we need a device pointer and an OPP table associated with it. I can take care of it for the multi genpd case as there are extra device structures (which we get from dev_pm_domain_attach_by_name()), but there is no clean way out for single PM domain devices, unless they also call dev_pm_opp_set_config() to get a virtual structure. This is why I had to get this hackish code in place to make it work with the recursive calls to dev_pm_opp_set_opp(), where I could just reuse the opp-level thing for the primary device. How do you suggest we take care of this now ?
On Fri, 20 Oct 2023 at 12:57, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 20-10-23, 12:02, Ulf Hansson wrote: > > For the single PM domain case, consumer drivers are often not able to > > use dev_pm_opp_set_config(). That's because the PM domain has already > > been attached from some of the generic buses, through > > dev_pm_domain_attach(). > > > > In this case, as dev_pm_opp_set_config() ends up trying to attach > > again, via dev_pm_domain_attach_by_name() it would receive > > "ERR_PTR(-EEXIST)". > > > > Or maybe I didn't quite understand your point? > > So the thing is that I _really_ want to call dev_pm_opp_set_opp() for > each OPP we want to configure, primary or required. For example, the > required OPP may want to do more than just performance state and we > aren't touching them right now. I understand - and it makes perfect sense to me too! > > Now, in order to call dev_pm_opp_set_opp() for any device, we need a > device pointer and an OPP table associated with it. > > I can take care of it for the multi genpd case as there are extra > device structures (which we get from dev_pm_domain_attach_by_name()), > but there is no clean way out for single PM domain devices, unless > they also call dev_pm_opp_set_config() to get a virtual structure. > > This is why I had to get this hackish code in place to make it work > with the recursive calls to dev_pm_opp_set_opp(), where I could just > reuse the opp-level thing for the primary device. > > How do you suggest we take care of this now ? Honestly, I don't know yet. But I am certainly willing to help. Allow me to have a closer look and see if I can propose a way forward. Kind regards Uffe
On 19-10-23, 13:16, Ulf Hansson wrote: > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, > > struct opp_table *required_table, int index) > > { > > struct device_node *np; > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, > > return -ENODEV; > > } > > > > + /* > > + * There are two genpd (as required-opp) cases that we need to handle, > > + * devices with a single genpd and ones with multiple genpds. > > + * > > + * The single genpd case requires special handling as we need to use the > > + * same `dev` structure (instead of a virtual one provided by genpd > > + * core) for setting the performance state. Lets treat this as a case > > + * where the OPP's level is directly available without required genpd > > + * link in the DT. > > + * > > + * Just update the `level` with the right value, which > > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > > + */ > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > + !opp_table->genpd_virt_devs) { > > + if (!WARN_ON(opp->level)) > > Hmm. Doesn't this introduce an unnecessary limitation? > > An opp node that has a required-opps phande, may have "opp-hz", > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > used too? Coming back to this, why would we ever want a device to have "opp-level" and "required-opp" (set to genpd's table) ? That would mean we will call: dev_pm_domain_set_performance_state() twice to set different level values. And so it should be safe to force that if required-opp table is set to a genpd, then opp-level shouldn't be set. Maybe we should fail in that case, which isn't happening currently.
On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-10-23, 13:16, Ulf Hansson wrote: > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, > > > struct opp_table *required_table, int index) > > > { > > > struct device_node *np; > > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, > > > return -ENODEV; > > > } > > > > > > + /* > > > + * There are two genpd (as required-opp) cases that we need to handle, > > > + * devices with a single genpd and ones with multiple genpds. > > > + * > > > + * The single genpd case requires special handling as we need to use the > > > + * same `dev` structure (instead of a virtual one provided by genpd > > > + * core) for setting the performance state. Lets treat this as a case > > > + * where the OPP's level is directly available without required genpd > > > + * link in the DT. > > > + * > > > + * Just update the `level` with the right value, which > > > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > > > + */ > > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > > + !opp_table->genpd_virt_devs) { > > > + if (!WARN_ON(opp->level)) > > > > Hmm. Doesn't this introduce an unnecessary limitation? > > > > An opp node that has a required-opps phande, may have "opp-hz", > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > > used too? > > Coming back to this, why would we ever want a device to have "opp-level" and > "required-opp" (set to genpd's table) ? That would mean we will call: > > dev_pm_domain_set_performance_state() twice to set different level values. Yes - and that would be weird, especially since the PM domain (genpd) is already managing the aggregation and propagation to parent domains. I guess I got a bit confused by the commit message for patch2/2, where it sounded like you were striving towards introducing recursive calls to set OPPs. Having a closer look, I realize that isn't the case, which I think makes sense. > > And so it should be safe to force that if required-opp table is set to a genpd, > then opp-level shouldn't be set. Maybe we should fail in that case, which isn't > happening currently. Yes, it seems better to fail earlier during the OF parsing of the required-opps or when adding an OPP dynamically. In that way, the WARN_ON above could be removed. That said, sorry for the noise and either way, feel free to add (for $subject patch): Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe
On 25-10-23, 12:40, Ulf Hansson wrote: > On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-10-23, 13:16, Ulf Hansson wrote: > > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > > > + !opp_table->genpd_virt_devs) { > > > > + if (!WARN_ON(opp->level)) > > > > > > Hmm. Doesn't this introduce an unnecessary limitation? > > > > > > An opp node that has a required-opps phande, may have "opp-hz", > > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > > > used too? > > > > Coming back to this, why would we ever want a device to have "opp-level" and > > "required-opp" (set to genpd's table) ? That would mean we will call: > > > > dev_pm_domain_set_performance_state() twice to set different level values. > > Yes - and that would be weird, especially since the PM domain (genpd) > is already managing the aggregation and propagation to parent domains. > > I guess I got a bit confused by the commit message for patch2/2, where > it sounded like you were striving towards introducing recursive calls > to set OPPs. Having a closer look, I realize that isn't the case, > which I think makes sense. > > > > > And so it should be safe to force that if required-opp table is set to a genpd, > > then opp-level shouldn't be set. Maybe we should fail in that case, which isn't > > happening currently. > > Yes, it seems better to fail earlier during the OF parsing of the > required-opps or when adding an OPP dynamically. In that way, the > WARN_ON above could be removed. For now I will leave the WARN as is, will reconsider if it makes more sense to fail and return early. And send a separate patch for that. > That said, sorry for the noise and either way, feel free to add (for > $subject patch): > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Thanks.
On Wed, Oct 25, 2023 at 12:40:26PM +0200, Ulf Hansson wrote: > On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 19-10-23, 13:16, Ulf Hansson wrote: > > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, > > > > struct opp_table *required_table, int index) > > > > { > > > > struct device_node *np; > > > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, > > > > return -ENODEV; > > > > } > > > > > > > > + /* > > > > + * There are two genpd (as required-opp) cases that we need to handle, > > > > + * devices with a single genpd and ones with multiple genpds. > > > > + * > > > > + * The single genpd case requires special handling as we need to use the > > > > + * same `dev` structure (instead of a virtual one provided by genpd > > > > + * core) for setting the performance state. Lets treat this as a case > > > > + * where the OPP's level is directly available without required genpd > > > > + * link in the DT. > > > > + * > > > > + * Just update the `level` with the right value, which > > > > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > > > > + */ > > > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > > > + !opp_table->genpd_virt_devs) { > > > > + if (!WARN_ON(opp->level)) > > > > > > Hmm. Doesn't this introduce an unnecessary limitation? > > > > > > An opp node that has a required-opps phande, may have "opp-hz", > > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > > > used too? > > > > Coming back to this, why would we ever want a device to have "opp-level" and > > "required-opp" (set to genpd's table) ? That would mean we will call: > > > > dev_pm_domain_set_performance_state() twice to set different level values. > > Yes - and that would be weird, especially since the PM domain (genpd) > is already managing the aggregation and propagation to parent domains. > FWIW I'm hitting this WARNing when trying to set up the parent domain setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently [1]. I know, me and all my weird OPP setups. :'D Basically, I have cpufreq voting for performance states of the CPR genpd (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO> as parent genpd and translates to the parent performance state using the "required-opps" in the *CPR* OPP table: cpr: power-controller@b018000 { compatible = "qcom,msm8916-cpr", "qcom,cpr"; reg = <0x0b018000 0x1000>; /* ... */ #power-domain-cells = <0>; operating-points-v2 = <&cpr_opp_table>; /* Supposed to be parent domain, not consumer */ power-domains = <&rpmpd MSM8916_VDDMX_AO>; cpr_opp_table: opp-table { compatible = "operating-points-v2-qcom-level"; cpr_opp1: opp1 { opp-level = <1>; qcom,opp-fuse-level = <1>; required-opps = <&rpmpd_opp_svs_soc>; }; cpr_opp2: opp2 { opp-level = <2>; qcom,opp-fuse-level = <2>; required-opps = <&rpmpd_opp_nom>; }; cpr_opp3: opp3 { opp-level = <3>; qcom,opp-fuse-level = <3>; required-opps = <&rpmpd_opp_super_turbo>; }; }; }; There are two problems with this: 1. (Unrelated to $subject patch) Since there is only a single entry in "power-domains", the genpd core code automatically attaches the CPR platform device as consumer of the VDDMX_AO power domain. I don't want this, I want it to become child of the VDDMX_AO genpd. I added some hacky code to workaround this. One option that works is to add a second dummy entry to "power-domains", which will prevent the genpd core from attaching the power domain: power-domains = <&rpmpd MSM8916_VDDMX_AO>, <0>; The other option is detaching the power domain again in probe(), after setting it up as parent domain: struct of_phandle_args parent, child; child.np = dev->of_node; child.args_count = 0; of_parse_phandle_with_args(dev->of_node, "power-domains", "#power-domain-cells", 0, &parent)); of_genpd_add_subdomain(&parent, &child); /* Detach power domain since it's managed via the subdomain */ dev_pm_domain_detach(dev, false); Is there a cleaner way to handle this? Mainly a question for Uffe. 2. The OPP WARNing triggers with both variants because it just checks if "required-opps" has a single entry. I guess we need extra checks to exclude the "parent genpd" case compared to the "OPP" case. [ 1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc [ 1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) [ 1.146887] pc : _link_required_opps+0x180/0x1cc [ 1.146902] lr : _link_required_opps+0xdc/0x1cc [ 1.276408] Call trace: [ 1.283519] _link_required_opps+0x180/0x1cc [ 1.285779] _of_add_table_indexed+0x61c/0xd40 [ 1.290292] dev_pm_opp_of_add_table+0x10/0x18 [ 1.294546] of_genpd_add_provider_simple+0x80/0x160 [ 1.298974] cpr_probe+0x6a0/0x97c [ 1.304092] platform_probe+0x64/0xbc It does seem to work correctly, with and without this patch. So I guess another option might be to simply silence this WARN_ON(). :') Thanks, Stephan [1]: https://lore.kernel.org/linux-pm/CAPDyKFoH5EOvRRKy-Bgp_B9B3rf=PUKK5N45s5PNgfBi55PaOQ@mail.gmail.com/
On 25-10-23, 15:47, Stephan Gerhold wrote: > FWIW I'm hitting this WARNing when trying to set up the parent domain > setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently > [1]. I know, me and all my weird OPP setups. :'D > > Basically, I have cpufreq voting for performance states of the CPR genpd > (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO> > as parent genpd and translates to the parent performance state using the > "required-opps" in the *CPR* OPP table: > > cpr: power-controller@b018000 { > compatible = "qcom,msm8916-cpr", "qcom,cpr"; > reg = <0x0b018000 0x1000>; > /* ... */ > #power-domain-cells = <0>; > operating-points-v2 = <&cpr_opp_table>; > /* Supposed to be parent domain, not consumer */ > power-domains = <&rpmpd MSM8916_VDDMX_AO>; > > cpr_opp_table: opp-table { > compatible = "operating-points-v2-qcom-level"; > > cpr_opp1: opp1 { > opp-level = <1>; > qcom,opp-fuse-level = <1>; > required-opps = <&rpmpd_opp_svs_soc>; > }; > cpr_opp2: opp2 { > opp-level = <2>; > qcom,opp-fuse-level = <2>; > required-opps = <&rpmpd_opp_nom>; > }; > cpr_opp3: opp3 { > opp-level = <3>; > qcom,opp-fuse-level = <3>; > required-opps = <&rpmpd_opp_super_turbo>; > }; > }; > }; I have forgotten a bit about this usecase. How exactly does the configurations work currently for this ? I mean genpd core must be setting the vote finally for only one of them or something else ?
On Wed, Oct 25, 2023 at 08:54:31PM +0530, Viresh Kumar wrote: > On 25-10-23, 15:47, Stephan Gerhold wrote: > > FWIW I'm hitting this WARNing when trying to set up the parent domain > > setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently > > [1]. I know, me and all my weird OPP setups. :'D > > > > Basically, I have cpufreq voting for performance states of the CPR genpd > > (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO> > > as parent genpd and translates to the parent performance state using the > > "required-opps" in the *CPR* OPP table: > > > > cpr: power-controller@b018000 { > > compatible = "qcom,msm8916-cpr", "qcom,cpr"; > > reg = <0x0b018000 0x1000>; > > /* ... */ > > #power-domain-cells = <0>; > > operating-points-v2 = <&cpr_opp_table>; > > /* Supposed to be parent domain, not consumer */ > > power-domains = <&rpmpd MSM8916_VDDMX_AO>; > > > > cpr_opp_table: opp-table { > > compatible = "operating-points-v2-qcom-level"; > > > > cpr_opp1: opp1 { > > opp-level = <1>; > > qcom,opp-fuse-level = <1>; > > required-opps = <&rpmpd_opp_svs_soc>; > > }; > > cpr_opp2: opp2 { > > opp-level = <2>; > > qcom,opp-fuse-level = <2>; > > required-opps = <&rpmpd_opp_nom>; > > }; > > cpr_opp3: opp3 { > > opp-level = <3>; > > qcom,opp-fuse-level = <3>; > > required-opps = <&rpmpd_opp_super_turbo>; > > }; > > }; > > }; > > I have forgotten a bit about this usecase. How exactly does the > configurations work currently for this ? I mean genpd core must be > setting the vote finally for only one of them or something else ? > I'm not sure if I understand your question correctly. Basically, setting <&rpmpd MSM8916_VDDMX_AO> as "parent genpd" of <&cpr> is supposed to describe that there is a direct relationship between the performance states of CPR and VDDMX. When changing the CPR performance state, VDDMX should also be adjusted accordingly. This is implemented in the genpd core in _genpd_set_performance_state(). It loops over the parent genpds, and re-evaluates the performance states of each of them. Translation happens using genpd_xlate_performance_state() which is just a direct call to dev_pm_opp_xlate_performance_state(). This will look up the required-opps from the OPP table above. However, the genpd core calls ->set_performance_state() on the parent genpd directly, so dev_pm_opp_set_opp() isn't involved in this case. Overall the call sequence for a CPUfreq switch will look something like: - cpu0: dev_pm_opp_set_rate(998.4 MHz) - cpu0: _set_required_opps(opp-998400000) - genpd:1:cpu0: dev_pm_opp_set_opp(&cpr_opp3) - genpd:1:cpu0: _set_opp_level(&cpr_opp3) - cpr: _genpd_set_performance_state(3) # genpd: translate & adjust parent performance states - cpr: genpd_xlate_performance_state(parent=VDDMX_AO) => &rpmpd_opp_super_turbo = 6 - VDDMX_AO: _genpd_set_performance_state(6) - rpmpd: ->set_performance_state(VDDMX_AO, 6) # genpd: change actual performance state - cpr: ->set_performance_state(cpr, 3) Before the discussion with Uffe I did not describe this relationship between CPR<->VDDMX as parent-child, I just had them as two separate power domains in the CPU OPP table. That worked fine too but Uffe suggested the parent-child representation might be better. Does that help or were you looking for something else? :D Thanks, Stephan
On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Wed, Oct 25, 2023 at 12:40:26PM +0200, Ulf Hansson wrote: > > On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 19-10-23, 13:16, Ulf Hansson wrote: > > > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, > > > > > struct opp_table *required_table, int index) > > > > > { > > > > > struct device_node *np; > > > > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, > > > > > return -ENODEV; > > > > > } > > > > > > > > > > + /* > > > > > + * There are two genpd (as required-opp) cases that we need to handle, > > > > > + * devices with a single genpd and ones with multiple genpds. > > > > > + * > > > > > + * The single genpd case requires special handling as we need to use the > > > > > + * same `dev` structure (instead of a virtual one provided by genpd > > > > > + * core) for setting the performance state. Lets treat this as a case > > > > > + * where the OPP's level is directly available without required genpd > > > > > + * link in the DT. > > > > > + * > > > > > + * Just update the `level` with the right value, which > > > > > + * dev_pm_opp_set_opp() will take care of in the normal path itself. > > > > > + */ > > > > > + if (required_table->is_genpd && opp_table->required_opp_count == 1 && > > > > > + !opp_table->genpd_virt_devs) { > > > > > + if (!WARN_ON(opp->level)) > > > > > > > > Hmm. Doesn't this introduce an unnecessary limitation? > > > > > > > > An opp node that has a required-opps phande, may have "opp-hz", > > > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be > > > > used too? > > > > > > Coming back to this, why would we ever want a device to have "opp-level" and > > > "required-opp" (set to genpd's table) ? That would mean we will call: > > > > > > dev_pm_domain_set_performance_state() twice to set different level values. > > > > Yes - and that would be weird, especially since the PM domain (genpd) > > is already managing the aggregation and propagation to parent domains. > > > > FWIW I'm hitting this WARNing when trying to set up the parent domain > setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently > [1]. I know, me and all my weird OPP setups. :'D > > Basically, I have cpufreq voting for performance states of the CPR genpd > (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO> > as parent genpd and translates to the parent performance state using the > "required-opps" in the *CPR* OPP table: > > cpr: power-controller@b018000 { > compatible = "qcom,msm8916-cpr", "qcom,cpr"; > reg = <0x0b018000 0x1000>; > /* ... */ > #power-domain-cells = <0>; > operating-points-v2 = <&cpr_opp_table>; > /* Supposed to be parent domain, not consumer */ > power-domains = <&rpmpd MSM8916_VDDMX_AO>; > > cpr_opp_table: opp-table { > compatible = "operating-points-v2-qcom-level"; > > cpr_opp1: opp1 { > opp-level = <1>; > qcom,opp-fuse-level = <1>; > required-opps = <&rpmpd_opp_svs_soc>; > }; > cpr_opp2: opp2 { > opp-level = <2>; > qcom,opp-fuse-level = <2>; > required-opps = <&rpmpd_opp_nom>; > }; > cpr_opp3: opp3 { > opp-level = <3>; > qcom,opp-fuse-level = <3>; > required-opps = <&rpmpd_opp_super_turbo>; > }; > }; > }; > > There are two problems with this: > > 1. (Unrelated to $subject patch) > Since there is only a single entry in "power-domains", the genpd > core code automatically attaches the CPR platform device as consumer > of the VDDMX_AO power domain. I don't want this, I want it to become > child of the VDDMX_AO genpd. > > I added some hacky code to workaround this. One option that works is > to add a second dummy entry to "power-domains", which will prevent > the genpd core from attaching the power domain: > > power-domains = <&rpmpd MSM8916_VDDMX_AO>, <0>; Hmm, looks a bit hackish to me. > > The other option is detaching the power domain again in probe(), > after setting it up as parent domain: Yes, if needed. > > struct of_phandle_args parent, child; > > child.np = dev->of_node; > child.args_count = 0; > > of_parse_phandle_with_args(dev->of_node, "power-domains", > "#power-domain-cells", 0, &parent)); > of_genpd_add_subdomain(&parent, &child); > > /* Detach power domain since it's managed via the subdomain */ > dev_pm_domain_detach(dev, false); > > Is there a cleaner way to handle this? Mainly a question for Uffe. At the moment, I don't think so. In fact, we have situations when the attachment is really useful. For example, during ->probe(), one can do a pm_runtime_get_sync() to power-on the "parent" domain. This may be needed to synchronize the power-states between the child/parent-domains, before calling of_genpd_add_subdomain(). > > 2. The OPP WARNing triggers with both variants because it just checks > if "required-opps" has a single entry. I guess we need extra checks > to exclude the "parent genpd" case compared to the "OPP" case. > > [ 1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc > [ 1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > [ 1.146887] pc : _link_required_opps+0x180/0x1cc > [ 1.146902] lr : _link_required_opps+0xdc/0x1cc > [ 1.276408] Call trace: > [ 1.283519] _link_required_opps+0x180/0x1cc > [ 1.285779] _of_add_table_indexed+0x61c/0xd40 > [ 1.290292] dev_pm_opp_of_add_table+0x10/0x18 > [ 1.294546] of_genpd_add_provider_simple+0x80/0x160 > [ 1.298974] cpr_probe+0x6a0/0x97c > [ 1.304092] platform_probe+0x64/0xbc > > It does seem to work correctly, with and without this patch. So I guess > another option might be to simply silence this WARN_ON(). :') Oh, thanks for pointing this out! This case haven't crossed my mind yet! Allow me to think a bit more about it. I will get back to you again with a suggestion soon, unless Viresh comes back first. :-) [...] Kind regards Uffe
On 26-10-23, 11:53, Ulf Hansson wrote: > On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote: > > 2. The OPP WARNing triggers with both variants because it just checks > > if "required-opps" has a single entry. I guess we need extra checks > > to exclude the "parent genpd" case compared to the "OPP" case. > > > > [ 1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc > > [ 1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > > [ 1.146887] pc : _link_required_opps+0x180/0x1cc > > [ 1.146902] lr : _link_required_opps+0xdc/0x1cc > > [ 1.276408] Call trace: > > [ 1.283519] _link_required_opps+0x180/0x1cc > > [ 1.285779] _of_add_table_indexed+0x61c/0xd40 > > [ 1.290292] dev_pm_opp_of_add_table+0x10/0x18 > > [ 1.294546] of_genpd_add_provider_simple+0x80/0x160 > > [ 1.298974] cpr_probe+0x6a0/0x97c > > [ 1.304092] platform_probe+0x64/0xbc > > > > It does seem to work correctly, with and without this patch. So I guess > > another option might be to simply silence this WARN_ON(). :') > > Oh, thanks for pointing this out! This case haven't crossed my mind yet! > > Allow me to think a bit more about it. I will get back to you again > with a suggestion soon, unless Viresh comes back first. :-) I have resent the series now. Stephan, please give it a try again. Thanks. Regarding this case where a genpd's table points to a parent genpd's table via the required-opps, it is a bit tricky to solve and the only way around that I could think of is that someone needs to call dev_pm_opp_set_config() with the right device pointer, with that we won't hit the warning anymore and things will work as expected. In this case the OPP core needs to call dev_pm_domain_set_performance_state() for device and then its genpd. We need the right device pointers :( Ulf, also another important thing here is that maybe we would want the genpd core to not propagate the voting anymore to the parent genpd's ? The dev_pm_opp_set_opp() call is better placed at handling all things and not just the performance state, like clk, regulator, bandwidth and so the recursion should happen at OPP level only. For now my series shouldn't break anything, just that we will try to set performance state twice for the parent genpd, the second call should silently return as the target state should be equal to current state.
On Mon, 30 Oct 2023 at 11:29, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-10-23, 11:53, Ulf Hansson wrote: > > On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote: > > > 2. The OPP WARNing triggers with both variants because it just checks > > > if "required-opps" has a single entry. I guess we need extra checks > > > to exclude the "parent genpd" case compared to the "OPP" case. > > > > > > [ 1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc > > > [ 1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > > > [ 1.146887] pc : _link_required_opps+0x180/0x1cc > > > [ 1.146902] lr : _link_required_opps+0xdc/0x1cc > > > [ 1.276408] Call trace: > > > [ 1.283519] _link_required_opps+0x180/0x1cc > > > [ 1.285779] _of_add_table_indexed+0x61c/0xd40 > > > [ 1.290292] dev_pm_opp_of_add_table+0x10/0x18 > > > [ 1.294546] of_genpd_add_provider_simple+0x80/0x160 > > > [ 1.298974] cpr_probe+0x6a0/0x97c > > > [ 1.304092] platform_probe+0x64/0xbc > > > > > > It does seem to work correctly, with and without this patch. So I guess > > > another option might be to simply silence this WARN_ON(). :') > > > > Oh, thanks for pointing this out! This case haven't crossed my mind yet! > > > > Allow me to think a bit more about it. I will get back to you again > > with a suggestion soon, unless Viresh comes back first. :-) > > I have resent the series now. > > Stephan, please give it a try again. Thanks. > > Regarding this case where a genpd's table points to a parent genpd's table via > the required-opps, it is a bit tricky to solve and the only way around that I > could think of is that someone needs to call dev_pm_opp_set_config() with the > right device pointer, with that we won't hit the warning anymore and things will > work as expected. > > In this case the OPP core needs to call dev_pm_domain_set_performance_state() > for device and then its genpd. We need the right device pointers :( > > Ulf, also another important thing here is that maybe we would want the genpd > core to not propagate the voting anymore to the parent genpd's ? The > dev_pm_opp_set_opp() call is better placed at handling all things and not just > the performance state, like clk, regulator, bandwidth and so the recursion > should happen at OPP level only. Are you saying that the OPP library should be capable of managing the parent-clock-rates too, when there is a new rate being requested for a clock that belongs to an OPP? To me, that sounds like replicating framework specific knowledge into the OPP library, no? Why do we want this? Unless I totally misunderstood your suggestion, I think it would be better if the OPP library remained simple and didn't run recursive calls, but instead relied on each framework to manage the aggregation and propagation to parents. > For now my series shouldn't break anything, > just that we will try to set performance state twice for the parent genpd, the > second call should silently return as the target state should be equal to > current state. > > -- > viresh Kind regards Uffe
On 03-11-23, 12:58, Ulf Hansson wrote: > Are you saying that the OPP library should be capable of managing the > parent-clock-rates too, when there is a new rate being requested for a > clock that belongs to an OPP? To me, that sounds like replicating > framework specific knowledge into the OPP library, no? Why do we want > this? I am surely not touching clocks or any other framework :) > Unless I totally misunderstood your suggestion, I think it would be > better if the OPP library remained simple and didn't run recursive > calls, but instead relied on each framework to manage the aggregation > and propagation to parents. I see your point and agree with it. Here is the problem and I am not very sure what's the way forward for this then: - Devices can have other devices (like caches) or genpds mentioned via required-opps. - Same is true for genpds, they can also have required-opps, which may or may not be other genpds. - When OPP core is asked to set a device's OPP, it isn't only about performance level, but clk, level, regulator, bw, etc. And so a full call to dev_pm_opp_set_opp() is required. - The OPP core is going to run the helper recursively only for required-opps and hence it won't affect clock or regulators. - But it currently affects genpds as they are mentioned in required-opps. - Skipping the recursive call to a parent genpd will require a special hack, maybe we should add it, I am just discussing it if we should or if there is another way around this.
On Mon, 6 Nov 2023 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 03-11-23, 12:58, Ulf Hansson wrote: > > Are you saying that the OPP library should be capable of managing the > > parent-clock-rates too, when there is a new rate being requested for a > > clock that belongs to an OPP? To me, that sounds like replicating > > framework specific knowledge into the OPP library, no? Why do we want > > this? > > I am surely not touching clocks or any other framework :) > > > Unless I totally misunderstood your suggestion, I think it would be > > better if the OPP library remained simple and didn't run recursive > > calls, but instead relied on each framework to manage the aggregation > > and propagation to parents. > > I see your point and agree with it. Okay! > > Here is the problem and I am not very sure what's the way forward for this then: > > - Devices can have other devices (like caches) or genpds mentioned via > required-opps. > > - Same is true for genpds, they can also have required-opps, which may or may not > be other genpds. > > - When OPP core is asked to set a device's OPP, it isn't only about performance > level, but clk, level, regulator, bw, etc. And so a full call to > dev_pm_opp_set_opp() is required. > > - The OPP core is going to run the helper recursively only for required-opps and > hence it won't affect clock or regulators. What if a required-opps has a clock or regulator? Doesn't that mean that clocks/regulators could be called too? > > - But it currently affects genpds as they are mentioned in required-opps. > > - Skipping the recursive call to a parent genpd will require a special hack, > maybe we should add it, I am just discussing it if we should or if there is > another way around this. Right, I see. If this is only for required-opps and devices being hooked up to a PM domain (genpd), my suggestion would be to keep avoiding doing the propagation to required-opps-parents. For the similar reasons to why we don't do it for clock/regulators, the propagation and aggregation, seems to me, to belong better in genpd. Did that make sense? Kind regards Uffe
On 10-11-23, 14:50, Ulf Hansson wrote: > If this is only for required-opps and devices being hooked up to a PM > domain (genpd), my suggestion would be to keep avoiding doing the > propagation to required-opps-parents. For the similar reasons to why > we don't do it for clock/regulators, the propagation and aggregation, > seems to me, to belong better in genpd. > > Did that make sense? Hmm, it does. Let me see what I can do on this..
On 15-11-23, 11:02, Viresh Kumar wrote: > On 10-11-23, 14:50, Ulf Hansson wrote: > > If this is only for required-opps and devices being hooked up to a PM > > domain (genpd), my suggestion would be to keep avoiding doing the > > propagation to required-opps-parents. For the similar reasons to why > > we don't do it for clock/regulators, the propagation and aggregation, > > seems to me, to belong better in genpd. > > > > Did that make sense? > > Hmm, it does. Let me see what I can do on this.. Hi Ulf, I have sent V3 with a new commit at the end to take care of this.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 84f345c69ea5..aab8c8e79146 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1074,10 +1074,12 @@ static int _opp_set_required_opps_generic(struct device *dev, static int _opp_set_required_opps_genpd(struct device *dev, struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) { - struct device **genpd_virt_devs = - opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev; + struct device **genpd_virt_devs = opp_table->genpd_virt_devs; int index, target, delta, ret; + if (!genpd_virt_devs) + return 0; + /* Scaling up? Set required OPPs in normal order, else reverse */ if (!scaling_down) { index = 0; diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 81fa27599d58..e056f31a48b5 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -296,7 +296,7 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp) of_node_put(opp->np); } -static int _link_required_opps(struct dev_pm_opp *opp, +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, struct opp_table *required_table, int index) { struct device_node *np; @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, return -ENODEV; } + /* + * There are two genpd (as required-opp) cases that we need to handle, + * devices with a single genpd and ones with multiple genpds. + * + * The single genpd case requires special handling as we need to use the + * same `dev` structure (instead of a virtual one provided by genpd + * core) for setting the performance state. Lets treat this as a case + * where the OPP's level is directly available without required genpd + * link in the DT. + * + * Just update the `level` with the right value, which + * dev_pm_opp_set_opp() will take care of in the normal path itself. + */ + if (required_table->is_genpd && opp_table->required_opp_count == 1 && + !opp_table->genpd_virt_devs) { + if (!WARN_ON(opp->level)) + opp->level = opp->required_opps[0]->level; + } + return 0; } @@ -338,7 +357,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table, if (IS_ERR_OR_NULL(required_table)) continue; - ret = _link_required_opps(opp, required_table, i); + ret = _link_required_opps(opp, opp_table, required_table, i); if (ret) goto free_required_opps; } @@ -359,7 +378,7 @@ static int lazy_link_required_opps(struct opp_table *opp_table, int ret; list_for_each_entry(opp, &opp_table->opp_list, node) { - ret = _link_required_opps(opp, new_table, index); + ret = _link_required_opps(opp, opp_table, new_table, index); if (ret) return ret; }