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 5F676C52D7B for ; Tue, 13 Aug 2024 15:10:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=6Gpr4JomZa9oMJWnIhkLX96KwVNRYGen8FZEkCeV2so=; b=wAlLfzxuAUl8q7MWnZ5H/09YF9 0oM0Co9AI3UXcHKsGJYDh52gSyOG8kIQmOknm3lP64e77ZDYQSMw9+JPPUHRV24aB/S5oHIX58pnH iOTKX79r2HWzjvhSGhGi5cHGiYhi2VM/KxCgcLd2QhrWOc3Z58klgkKEuoH6jFnKdbPstWVxFOkSL DtY5GcRpnLyMMJsg0leNGEEUutQQh5wZwxJxwccr0SMCmRVPrfEZMTNSLnlScwLGCpat7+zjSXg2W DTkoeuEpCsTsQBGJUvCzZe/4ZJNsN5LadPAtZb3/cTVkxYd2+b9Ahcal3V1EkdOjeN1DbQDYa+bt0 GKE7WIaQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sdtAR-000000049o8-2OZ3; Tue, 13 Aug 2024 15:10:35 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sdt9p-000000049iF-0ACw for linux-arm-kernel@lists.infradead.org; Tue, 13 Aug 2024 15:09:58 +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 145941596; Tue, 13 Aug 2024 08:10:22 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A63743F6A8; Tue, 13 Aug 2024 08:09:54 -0700 (PDT) Date: Tue, 13 Aug 2024 16:09:51 +0100 From: Cristian Marussi To: Peng Fan Cc: Cristian Marussi , Dhruva Gole , "Peng Fan (OSS)" , "saravanak@google.com" , "sudeep.holla@arm.com" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "arm-scmi@vger.kernel.org" Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode for scmi cpufreq Message-ID: References: <20240729070325.2065286-1-peng.fan@oss.nxp.com> <20240813085703.zz6ltcxmrrbdgt77@lcpd911> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240813_080957_204825_9BD538B1 X-CRM114-Status: GOOD ( 53.10 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote: > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode > > for scmi cpufreq > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote: > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set > > fwnode > > > > for scmi cpufreq > > > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote: > > > > > From: Peng Fan > > > > > > > > > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use > > > > > SCMI_PROTCOL_PERF protocol, but with different name, so two > > scmi > > > > > devices will be created. But the fwnode->dev could only point to > > > > > one > > > > device. > > > > > > > > > > If scmi cpufreq device created earlier, the fwnode->dev will point > > > > > to the scmi cpufreq device. Then the fw_devlink will link > > > > > performance domain user device(consumer) to the scmi cpufreq > > device(supplier). > > > > > But actually the performance domain user device, such as GPU, > > > > should > > > > > use the scmi perf device as supplier. Also if 'cpufreq.off=1' in > > > > > bootargs, the GPU driver will defer probe always, because of the > > > > > scmi cpufreq > > > > > > > > The commit message itself seems very specific to some platform to > > me. > > > > What about platforms that don't atall have a GPU? Why would > > they > > > > care about this? > > > > > > It is a generic issue if a platform has performance domain to serve > > > scmi cpufreq and device performance level. > > > > > > > > > > > > device not ready. > > > > > > > > > > Because for cpufreq, no need use fw_devlink. So bypass setting > > > > fwnode > > > > > for scmi cpufreq device. > > > > > > > > > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the > > > > > scmi_device") > > > > > Signed-off-by: Peng Fan > > > > > --- > > > > > > > > > > V2: > > > > > Use A!=B to replace !(A == B) > > > > > Add fixes tag > > > > > This might be a workaround, but since this is a fix, it is simple > > > > > for backporting. > > > > > > > > More than a workaround, it feels like a HACK to me. > > > > > > > > > > > > > > V1: > > > > > > > > > > > > > > > > > > > > drivers/firmware/arm_scmi/bus.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c > > > > > b/drivers/firmware/arm_scmi/bus.c index > > > > 96b2e5f9a8ef..be91a82e0cda > > > > > 100644 > > > > > --- a/drivers/firmware/arm_scmi/bus.c > > > > > +++ b/drivers/firmware/arm_scmi/bus.c > > > > > @@ -395,7 +395,8 @@ __scmi_device_create(struct > > device_node > > > > *np, struct device *parent, > > > > > scmi_dev->id = id; > > > > > scmi_dev->protocol_id = protocol; > > > > > scmi_dev->dev.parent = parent; > > > > > - device_set_node(&scmi_dev->dev, of_fwnode_handle(np)); > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name, > > > > "cpufreq")) > > > > > + device_set_node(&scmi_dev->dev, > > > > of_fwnode_handle(np)); > > > > > > > > I kind of disagree with the idea here to be specific about the > > > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver > > right? > > > > Why bring in specific code into a bus driver? We will never fix the > > > > actual root cause of the issue this way. > > > > > > The root cause is fwnode devlink only supports one fwnode, one > > device. > > > 1:1 match. But current arm scmi driver use one fwnode for two > > devices. > > > > > > If your platform has scmi cpufreq and scmi device performance > > domain, > > > you might see that some devices are consumer of scmi cpufreq, but > > > actually they should be consumer of scmi device performance > > domain. > > > > > > I not have a good idea that this is fw devlink design that only allows > > > 1 fwnode has 1 device or not. If yes, that arm scmi should be fixed. > > > If not, fw devlink should be updated. > > > > > > The current patch is the simplest method for stable tree fixes as I > > > could work out. > > > > So this is the same root cause at the end of the issues you had with > > IMX pinctrl coexistence...i.e. the SCMI stack creates scmi devices that > > embeds the protocol node, BUT since you can have multiple > > device/drivers doing different things on different resources within the > > same protocol you can end with 2 devices having the same embedded > > device_node, since we dont really have anything else to use as > > device_node, I rigth ? > > I think, yes. And you remind me that with PINCTRL_SCMI and > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node > will only take one to set the fwnode device pointer depends on > the order to run __scmi_device_create. > > So not only perf, pinctrl also has issue here, fwnode devlink will > not work properly for pinctrl/perf. ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are mutually exclusive in the code (@probe time) itself as far as I can remember about what you did, so why devlink should have that issue there ? Have you seen any issue in this regards while having loaded pinctrl_scmi and pinctrl_imx_scmi ? I want to have a better look next days about this devlink issue that you reported...it still not clear to me...from device_link_add() docs, it seems indeed that it will return the old existing link if a link exist already between that same supplier/consumer devices pair....but from the code (at first sight) it seems that the check is made agains the devices not their embeded device_nodes, but here (and in pinctrl/imx case) you will have 2 distinct consumer devices (with same device_node)...I may have missed something in your exaplanation.... Thanks, Cristian