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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 47288EB64DA for ; Wed, 19 Jul 2023 14:56:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=9FmG+eqhn7PhBFv5wMwlxb710libeV08nQ9YXdk1930=; b=W8gLl3bL5869bL f1ndQiKglMMVe8QLtu67IF1yy9nhOPqVOhiE2BbxvLp+PPgg+e5g81/dUTcv+GqzxK8SzE1CLo73C eI7/0sGE0SSpXLz0dBmJOjC5Qpv3x39as/uwvKyQ/rkOV8v0jv7qa1TGMppWKX824Xw/0J9OaMW35 pvJRW6MkVJSCHxS3v0CnQyXwDpxp7tIj9Jv/8vP2EOueCcVPSr7GPSLmw3W9gziVd2OpgqlB/DJGX 7xw4JnKk7XIB36AV2tpbQcXvB0+TBlnHu5av/H6gG5YObsVY6ui7B2ZQHqv0/RQfDjfr50ii6PTUF JKlgqCt1hcaRr6v33G9w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qM8aa-007pbv-21; Wed, 19 Jul 2023 14:55:41 +0000 Received: from desiato.infradead.org ([2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qM8aZ-007pag-1F for linux-arm-kernel@bombadil.infradead.org; Wed, 19 Jul 2023 14:55:39 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=Tp3H5pc7kSY1vTwxM+1Q4GuCjqDGy7uWVEZ9bqamdSg=; b=PJlqqfEcQcV/7URsPJuH0te2u9 V5EOpOoNPNg98V2BXjxvDaFNit3StjYDWygeG7e7M5EynezBebKYamVVzn9aQSVGrZqmYZ+icjYD4 OZj/5wsCD5ihn7IJ5V3uLbDugZAkJhRFfoxr0KFObY06YURZZbe5ExX2EB1Bb/60JrL4lxNpxhHyJ bA8FaYhlpX4bmlTaIOIgEVOLksHQobsAgw21c5KqbLFC8oE9rsGj13/B770cw41pSH7nCGxJrTqf/ MwT0J7HDEA3TkzckBOzt0lLjt0rgSIrU32EhwpIbnDnK/uOmBeFLfBZvXELNUO7NcSAWu9p/Y6XlU sBGXM4ng==; Received: from foss.arm.com ([217.140.110.172]) by desiato.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qM8Ww-00DYvY-2b for linux-arm-kernel@lists.infradead.org; Wed, 19 Jul 2023 14:51:57 +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 C26162F4; Wed, 19 Jul 2023 07:52:32 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E5C0E3F6C4; Wed, 19 Jul 2023 07:51:47 -0700 (PDT) Date: Wed, 19 Jul 2023 15:51:45 +0100 From: Cristian Marussi To: Ulf Hansson Cc: Sudeep Holla , Viresh Kumar , Nishanth Menon , Stephen Boyd , Nikunj Kela , Prasad Sodagudi , Alexandre Torgue , linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain Message-ID: References: <20230713141738.23970-1-ulf.hansson@linaro.org> <20230713141738.23970-11-ulf.hansson@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230713141738.23970-11-ulf.hansson@linaro.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230719_155155_346609_5E4B6233 X-CRM114-Status: GOOD ( 34.85 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote: > To enable support for performance scaling (DVFS) for generic devices with > the SCMI performance protocol, let's add an SCMI performance domain. This > is being modelled as a genpd provider, with support for performance scaling > through genpd's ->set_performance_state() callback. > > Note that, this adds the initial support that allows consumer drivers for > attached devices, to vote for a new performance state via calling the > dev_pm_genpd_set_performance_state(). However, this should be avoided as > it's in most cases preferred to use the OPP library to vote for a new OPP > instead. The support using the OPP library isn't part of this change, but > needs to be implemented from subsequent changes. > Hi Ulf, a couple of remarks down below. > Signed-off-by: Ulf Hansson > --- > > Changes in v2: > - Converted to use the new ->domain_info_get() callback. > > --- > drivers/firmware/arm_scmi/Kconfig | 12 ++ > drivers/firmware/arm_scmi/Makefile | 1 + > drivers/firmware/arm_scmi/scmi_perf_domain.c | 155 +++++++++++++++++++ > 3 files changed, 168 insertions(+) > create mode 100644 drivers/firmware/arm_scmi/scmi_perf_domain.c [snip] > +static int scmi_perf_domain_probe(struct scmi_device *sdev) > +{ > + struct device *dev = &sdev->dev; > + const struct scmi_handle *handle = sdev->handle; > + const struct scmi_perf_proto_ops *perf_ops; > + struct scmi_protocol_handle *ph; > + struct scmi_perf_domain *scmi_pd; > + struct genpd_onecell_data *scmi_pd_data; > + struct generic_pm_domain **domains; > + int num_domains, i, ret = 0; > + u32 perf_level; > + > + if (!handle) > + return -ENODEV; > + > + /* The OF node must specify us as a power-domain provider. */ > + if (!of_find_property(dev->of_node, "#power-domain-cells", NULL)) > + return 0; > + > + perf_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PERF, &ph); > + if (IS_ERR(perf_ops)) > + return PTR_ERR(perf_ops); > + > + num_domains = perf_ops->num_domains_get(ph); > + if (num_domains < 0) { > + dev_warn(dev, "Failed with %d when getting num perf domains\n", > + num_domains); > + return num_domains; > + } else if (!num_domains) { > + return 0; > + } > + > + scmi_pd = devm_kcalloc(dev, num_domains, sizeof(*scmi_pd), GFP_KERNEL); > + if (!scmi_pd) > + return -ENOMEM; > + > + scmi_pd_data = devm_kzalloc(dev, sizeof(*scmi_pd_data), GFP_KERNEL); > + if (!scmi_pd_data) > + return -ENOMEM; > + > + domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL); > + if (!domains) > + return -ENOMEM; > + > + for (i = 0; i < num_domains; i++, scmi_pd++) { > + scmi_pd->info = perf_ops->domain_info_get(ph, i); So here you are grabbing all the performance domains exposed by the platform via PERF protocol and then a few lines down below you are registering them with pm_genpd_init(), but the list of domains obtained from the platform will contain NOT only devices but also CPUs possibly, already managed by the SCMI CPUFreq driver. In fact the SCMI CPUFreq driver, on his side, takes care to pick only domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT parsing) but here you are registering all domains with GenPD upfront. Is it not possible that, once registered, GenPD can decide, at some point in the future, to try act on some of these domains associated with a CPU ? (like Clock framework does at the end of boot trying to disable unused clocks...not familiar with internals of GenPD, though) > + scmi_pd->domain_id = i; > + scmi_pd->perf_ops = perf_ops; > + scmi_pd->ph = ph; > + scmi_pd->genpd.name = scmi_pd->info->name; > + scmi_pd->genpd.flags = GENPD_FLAG_OPP_TABLE_FW; > + scmi_pd->genpd.set_performance_state = scmi_pd_set_perf_state; > + > + ret = perf_ops->level_get(ph, i, &perf_level, false); > + if (ret) { > + dev_dbg(dev, "Failed to get perf level for %s", > + scmi_pd->genpd.name); > + perf_level = 0; > + } > + > + /* Let the perf level indicate the power-state too. */ > + ret = pm_genpd_init(&scmi_pd->genpd, NULL, perf_level == 0); In SCMI world PERF levels should have nothing to do with the Power state of a domain: you have the POWER protocol for that, so you should not assume that perf level 0 means OFF, but you can use the POWER protocol operation .state_get() to lookup the power state. (and you can grab both perf and power ops from the same driver) The tricky part would be to match the PERF domain at hand with the related POWER domain to query the state for, I suppose. Indeed, recently, while looking at SCMI v3.2 PERF evolutions, I was tempted to just start rejecting any level_set() or set_freq() request for ZERO since they really can be abused to power off a domain. (if the platform complies...) Apologize if I missed something about how GenPD behaviour... Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel