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 5E711EB64DA for ; Wed, 19 Jul 2023 15:59:52 +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=eCvuzIPJ8N8J0WmikDn3icccCPCBOAJG/icQLiueZWY=; b=sW/+3BU4lcHwUC 7GuzTl3R+yoXc6F2qOy+6PYAtnLQjp/R9HWTS1xXag3Liouqd7pfgpr1VkBl2Yoy95B6giX24Q/P7 xNZ06Misca8eR0aT02KVCcNOox/fEKOh9PWZvMHcuVbtgQ9OQ6iv4qx9x7RgBCJAeYcRWHn43qpde KV49PIsi4EHIA6Xe1EDRwR94ajYY9QR8JO+GL7pzNww85KiXJTzClKPH34jg7X/ACKESOOOrI7Yun c6LRH9pRabay7QkPmH11NjlnVEHAXUvedgTzJsPhPDyikg4IFy/CWbN+XK84/YGP/R4P3bBwsx9LK Jgg20/ZMZxfHdDpbSHaA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qM9aN-00829f-0A; Wed, 19 Jul 2023 15:59:31 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qM9aJ-00828o-37 for linux-arm-kernel@lists.infradead.org; Wed, 19 Jul 2023 15:59:29 +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 1D4F42F4; Wed, 19 Jul 2023 09:00:08 -0700 (PDT) Received: from bogus (e103737-lin.cambridge.arm.com [10.1.197.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 231A13F67D; Wed, 19 Jul 2023 08:59:23 -0700 (PDT) Date: Wed, 19 Jul 2023 16:59:20 +0100 From: Sudeep Holla To: Cristian Marussi Cc: Ulf Hansson , Viresh Kumar , Nishanth Menon , Stephen Boyd , Sudeep Holla , 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: <20230719155920.iuu2ue2co535dfkx@bogus> 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: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230719_085928_123215_2A2CC02D X-CRM114-Status: GOOD ( 33.48 ) 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 Wed, Jul 19, 2023 at 03:51:45PM +0100, Cristian Marussi wrote: > On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote: [...] > > + 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. > Agreed, I pointed out briefly in the previous patch I think. I am not sure how will that work if the performance and power domains are not 1-1 mapping or if they are CPUs then this might confusing ? Not sure but looks like we might be creating a spaghetti here :(. > 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. > +1 > 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 ? IIRC, all unused genpd are turned off right. It may not impact here but still super confusing as we will be creating power domains for the set of domains actually advertised as power domains by the firmware. This will add another set. > (like Clock framework does at the end of boot trying to disable unused > clocks...not familiar with internals of GenPD, though) > Ah, I am reading too much serialised. Just agreed and wrote the same above. > > + 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. > I wanted to ask the same. E.g. on juno, GPU has perf domain 2 and power domain 9. It would be good if we can how it would work there ? What is expected from the gpu driver in terms of managing perf and power ? Does it need to specify 2 power domains now and specify which is perf and which power in its bindings ? > 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...) > Good point I need to dig the spec before I can comment on this. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel