Message ID | 20240104130123.37115-4-brgl@bgdev.pl |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-16677-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp5596179dyb; Thu, 4 Jan 2024 05:04:42 -0800 (PST) X-Google-Smtp-Source: AGHT+IHop3KLbghWvOWdkj/fa+2pqhnRXpsad/PL7MqSFgI7OnhQ4743fNwdnqZNnx62z+FbMVyu X-Received: by 2002:a17:906:d0db:b0:9e3:fbab:e091 with SMTP id bq27-20020a170906d0db00b009e3fbabe091mr283729ejb.15.1704373482311; Thu, 04 Jan 2024 05:04:42 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704373482; cv=none; d=google.com; s=arc-20160816; b=DhHCrKnCWtOSR5N1P69xEnHzRFmcFvCsVX6MpeD8T3q9L8IZdVGpt5C7X6IcpbA9Xd pdI7s7GejrTrLlbo8782noMK1itZaiCyMhGoD+wHunslaRooomHXZG5OWkdmlhcZf7ww rC9d4A6hi7vRPQzmJ6zP9T/vy0sVB+ly6Q9ZwEsGo6QDoBUkWZyz2YgcgHj19hNNQ48d qCzGE0CJc2ZOHi0leoGkAO0Fr3eMilwbFh3IMvjug0aH+dAj9sPDtNMe5VAKbp9pwQ26 IVqKsTNZJe+gEVV9UTAhSTMox/TmPLyJGP7ZdSqmiiFWWOxsUNkKzXRnsf53Oa7WUDrq vgng== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=dm8mMOJsxux6ZmWUG3F7cuu+H2vgOte3WZWDy9og3kI=; fh=1uFngekeZT2FVGQaKpK4gjTIex4FU0hdfexTfOUKej8=; b=Z3i1WKvtOuJTpaoA6GYGknOCyqfpQFpyGDJyQqi3RTPPKWidJ9jXzw26kD9vhG+kJr +HC5IKZrEn/mn8AkQW2J5MrvJh7Kqa5Gaz8tuxGSamaJQdUSwVG9fP37BURePhl7Kr0T F7db6pckca5wrao4HBmkxCBal+spSqkIusRv/otJEmuNRYxujyzQwJx1MWW0AwlGcQ/0 34ijqkRXeNl8g4bSSWoVLpd63zRZ6fTQKA8bW7OCE+hyngfqGGrZ30sHIuO5g/7Vzp5c U3wZPo5yR2sFQO+4EkNkMyb+Mdg6h8TmRxJ/Ol51xBgdmY1FPPzpaxo+EfPVh281vHba 3HUg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=zESwugOz; spf=pass (google.com: domain of linux-kernel+bounces-16677-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-16677-ouuuleilei=gmail.com@vger.kernel.org" Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id e22-20020a170906045600b00a1d5063b024si12317229eja.720.2024.01.04.05.04.42 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 Jan 2024 05:04:42 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-16677-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=zESwugOz; spf=pass (google.com: domain of linux-kernel+bounces-16677-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-16677-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id E510F1F21B6F for <ouuuleilei@gmail.com>; Thu, 4 Jan 2024 13:04:41 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 77DD2249EE; Thu, 4 Jan 2024 13:02:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bgdev-pl.20230601.gappssmtp.com header.i=@bgdev-pl.20230601.gappssmtp.com header.b="zESwugOz" X-Original-To: linux-kernel@vger.kernel.org Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C03C522EF0 for <linux-kernel@vger.kernel.org>; Thu, 4 Jan 2024 13:02:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=bgdev.pl Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=bgdev.pl Received: by mail-wr1-f42.google.com with SMTP id ffacd0b85a97d-3373bc6d625so365403f8f.3 for <linux-kernel@vger.kernel.org>; Thu, 04 Jan 2024 05:02:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1704373327; x=1704978127; 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=dm8mMOJsxux6ZmWUG3F7cuu+H2vgOte3WZWDy9og3kI=; b=zESwugOzZQWroLH3AzdFt7/hlzlCWNPWAY42tOmmQ1m+tQ1HgQT13NHkIGN54DaX3O w1aYnbe//4y09pGoNAkqCGL45RrOrRJQBEkvTkizscaQkkbH7nJA/HXjqdYkssHhCUzG aWLRnX93EEBj8JC2RHltW+mnbFvDKrfA8TSLxDjpo0xofw0MTHAsJsnEG1T0PSl90ZHY uX35iKTLEbTNFcMkes0gJ7taDIqUR06yYePDmN1YjJO6/0nlWcxgt5iIHWNAXfwXePJX n3mTT8jWJkaCoGeZ2n2uE2FQ85t68XGguhZCYoSMf0glh68o34oOtdEF21pVeDD2+Dyv iRKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704373327; x=1704978127; 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=dm8mMOJsxux6ZmWUG3F7cuu+H2vgOte3WZWDy9og3kI=; b=L5U4pZ90WqpnNOQJ+XFoQXHSP49JBPC4QqOFPVexiqaBsFOPHEF9HMXauIgVp5G8wc EG+qWurS+XmwOPHW7RqnyJWPuBtuJc21tbnhH2HSkIC30DxHcDNqYjpV+Yp9usYhcIGH Q8GI2pPlXe/OHMabMpCmVldr42dLO2Bz+wYxMY9vQUxoRrsbI3s+aeS0csJ2RAqZozug wLhDsHgxTP0yeuyMZz0TDyc7Pz+GniSAwBfyDpFMzPpLl8E1klLmUH/DqiDdBwLMtf18 zO5MEnC/U6Ns0BhWcy7MKF/UXtAjVi0dz+NPBc6R5e9XnvzyXkyGDzqwwesnEvfPZBKI ihXw== X-Gm-Message-State: AOJu0YwoNqPagkQ6VfuKquVnzdPFWXEz3HjFF4HQPw3chsFuqaThFuqn 8WfszWqhkIYwM8ZtXEYILAyk+OeUbleUsg== X-Received: by 2002:adf:b199:0:b0:337:175:a079 with SMTP id q25-20020adfb199000000b003370175a079mr327095wra.80.1704373327126; Thu, 04 Jan 2024 05:02:07 -0800 (PST) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:5b69:3768:8459:8fee]) by smtp.gmail.com with ESMTPSA id w5-20020a5d5445000000b0033660f75d08sm32887387wrv.116.2024.01.04.05.02.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 Jan 2024 05:02:06 -0800 (PST) From: Bartosz Golaszewski <brgl@bgdev.pl> To: Kalle Valo <kvalo@kernel.org>, "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>, Heiko Stuebner <heiko@sntech.de>, Jernej Skrabec <jernej.skrabec@gmail.com>, Chris Morgan <macromorgan@hotmail.com>, Linus Walleij <linus.walleij@linaro.org>, Geert Uytterhoeven <geert+renesas@glider.be>, Arnd Bergmann <arnd@arndb.de>, Neil Armstrong <neil.armstrong@linaro.org>, =?utf-8?q?N=C3=ADcolas_F_=2E_R_?= =?utf-8?q?=2E_A_=2E_Prado?= <nfraprado@collabora.com>, Marek Szyprowski <m.szyprowski@samsung.com>, Peng Fan <peng.fan@nxp.com>, Robert Richter <rrichter@amd.com>, Dan Williams <dan.j.williams@intel.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, Terry Bowman <terry.bowman@amd.com>, Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>, =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>, Huacai Chen <chenhuacai@kernel.org>, Alex Elder <elder@linaro.org>, Srini Kandagatla <srinivas.kandagatla@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes Date: Thu, 4 Jan 2024 14:01:17 +0100 Message-Id: <20240104130123.37115-4-brgl@bgdev.pl> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20240104130123.37115-1-brgl@bgdev.pl> References: <20240104130123.37115-1-brgl@bgdev.pl> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1787165128487790953 X-GMAIL-MSGID: 1787165128487790953 |
Series |
PCI: introduce the concept of power sequencing of PCIe devices
|
|
Commit Message
Bartosz Golaszewski
Jan. 4, 2024, 1:01 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> In order to introduce PCIe power-sequencing, we need to create platform devices for child nodes of the port driver node. They will get matched against the pwrseq drivers (if one exists) and then the actuak PCIe device will reuse the node once it's detected on the bus. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- drivers/pci/pcie/portdrv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Comments
On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > In order to introduce PCIe power-sequencing, we need to create platform > devices for child nodes of the port driver node. They will get matched > against the pwrseq drivers (if one exists) and then the actuak PCIe nit if you re-spin: s/actuak /actual / > device will reuse the node once it's detected on the bus. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/pci/pcie/portdrv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 14a4b89a3b83..401fb731009d 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -13,6 +13,7 @@ > #include <linux/pci.h> > #include <linux/kernel.h> > #include <linux/errno.h> > +#include <linux/of_platform.h> > #include <linux/pm.h> > #include <linux/pm_runtime.h> > #include <linux/string.h> > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > pm_runtime_allow(&dev->dev); > } > > - return 0; > + return devm_of_platform_populate(&dev->dev); > } > > static void pcie_portdrv_remove(struct pci_dev *dev)
On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote: > In order to introduce PCIe power-sequencing, we need to create platform > devices for child nodes of the port driver node. They will get matched > against the pwrseq drivers (if one exists) and then the actuak PCIe > device will reuse the node once it's detected on the bus. [...] > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > pm_runtime_allow(&dev->dev); > } > > - return 0; > + return devm_of_platform_populate(&dev->dev); > } I think this belongs in of_pci_make_dev_node(), portdrv seems totally the wrong place. Note that you're currently calling this for RCECs (Root Complex Event Collectors) as well, which is likely not what you want. devm functions can't be used in the PCI core, so symmetrically call of_platform_unpopulate() from of_pci_remove_node(). Thanks, Lukas
On Tue, Jan 9, 2024 at 3:43 PM Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote: > > In order to introduce PCIe power-sequencing, we need to create platform > > devices for child nodes of the port driver node. They will get matched > > against the pwrseq drivers (if one exists) and then the actuak PCIe > > device will reuse the node once it's detected on the bus. > [...] > > --- a/drivers/pci/pcie/portdrv.c > > +++ b/drivers/pci/pcie/portdrv.c > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > > pm_runtime_allow(&dev->dev); > > } > > > > - return 0; > > + return devm_of_platform_populate(&dev->dev); > > } > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally > the wrong place. Note that you're currently calling this for RCECs > (Root Complex Event Collectors) as well, which is likely not what > you want. > of_pci_make_dev_node() is only called when the relevant PCI device is instantiated which doesn't happen until it's powered-up and scanned - precisely the problem I'm trying to address. Calling this for whomever isn't really a problem though, is it? We will create a platform device alright - if it's defined on the DT - and at worst it won't match against any driver. It seems harmless IMO. > devm functions can't be used in the PCI core, so symmetrically call > of_platform_unpopulate() from of_pci_remove_node(). I don't doubt what you're saying is true (I've seen worse things) but this is the probe() callback of a driver using the driver model. Why wouldn't devres work? Bart > > Thanks, > > Lukas >
On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote: > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote: > > > In order to introduce PCIe power-sequencing, we need to create platform > > > devices for child nodes of the port driver node. They will get matched > > > against the pwrseq drivers (if one exists) and then the actuak PCIe > > > device will reuse the node once it's detected on the bus. > > [...] > > > --- a/drivers/pci/pcie/portdrv.c > > > +++ b/drivers/pci/pcie/portdrv.c > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > > > pm_runtime_allow(&dev->dev); > > > } > > > > > > - return 0; > > > + return devm_of_platform_populate(&dev->dev); > > > } > > > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally > > the wrong place. Note that you're currently calling this for RCECs > > (Root Complex Event Collectors) as well, which is likely not what > > you want. > > > > of_pci_make_dev_node() is only called when the relevant PCI device is > instantiated which doesn't happen until it's powered-up and scanned - > precisely the problem I'm trying to address. No, of_pci_make_dev_node() is called *before* device_attach(), i.e. before portdrv has even probed. So it seems this should work perfectly well for your use case. > > devm functions can't be used in the PCI core, so symmetrically call > > of_platform_unpopulate() from of_pci_remove_node(). > > I don't doubt what you're saying is true (I've seen worse things) but > this is the probe() callback of a driver using the driver model. Why > wouldn't devres work? The long term plan is to move the functionality in portdrv to the PCI core. Because devm functions can't be used in the PCI core, adding new ones to portdrv will *add* a new roadblock to migrating portdrv to the PCI core. In other words, it makes future maintenance more difficult. Generally, only PCIe port services which share the same interrupt (hotplug, PME, bandwith notification, flit error counter, ...) need to live in portdrv. Arbitrary other stuff should not be shoehorned into portdrv. Thanks, Lukas
On Wed, Jan 10, 2024 at 2:28 PM Lukas Wunner <lukas@wunner.de> wrote: > > On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote: > > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote: > > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote: > > > > In order to introduce PCIe power-sequencing, we need to create platform > > > > devices for child nodes of the port driver node. They will get matched > > > > against the pwrseq drivers (if one exists) and then the actuak PCIe > > > > device will reuse the node once it's detected on the bus. > > > [...] > > > > --- a/drivers/pci/pcie/portdrv.c > > > > +++ b/drivers/pci/pcie/portdrv.c > > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > > > > pm_runtime_allow(&dev->dev); > > > > } > > > > > > > > - return 0; > > > > + return devm_of_platform_populate(&dev->dev); > > > > } > > > > > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally > > > the wrong place. Note that you're currently calling this for RCECs > > > (Root Complex Event Collectors) as well, which is likely not what > > > you want. > > > > > > > of_pci_make_dev_node() is only called when the relevant PCI device is > > instantiated which doesn't happen until it's powered-up and scanned - > > precisely the problem I'm trying to address. > > No, of_pci_make_dev_node() is called *before* device_attach(), > i.e. before portdrv has even probed. So it seems this should > work perfectly well for your use case. > Seems like the following must be true but isn't in my case (from pci_bus_add_device()): if (pci_is_bridge(dev)) of_pci_make_dev_node(dev); Shouldn't it evaluate to true for ports? Bartosz [snip]
On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: > Seems like the following must be true but isn't in my case (from > pci_bus_add_device()): > > if (pci_is_bridge(dev)) > of_pci_make_dev_node(dev); > > Shouldn't it evaluate to true for ports? It should. What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said: > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: >> Seems like the following must be true but isn't in my case (from >> pci_bus_add_device()): >> >> if (pci_is_bridge(dev)) >> of_pci_make_dev_node(dev); >> >> Shouldn't it evaluate to true for ports? > > It should. > > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question? > I cut out the hexdump part, let me know if you really need it. Output follows. Bart -- # lspci -vvvvxxxx -s 0000:00:00.0 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b (prog-if 00 [Normal decode]) Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0 Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 176 IOMMU group: 8 Region 0: Memory at 60300000 (32-bit, non-prefetchable) [size=4K] Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0 I/O behind bridge: f000-0fff [disabled] [16-bit] Memory behind bridge: 60400000-604fffff [size=1M] [32-bit] Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff [disabled] [64-bit] Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR- BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B- PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+ Address: 00000000a1c3f000 Data: 0000 Masking: fffffffe Pending: 00000000 Capabilities: [70] Express (v2) Root Port (Slot+), MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0 ExtTag- RBE+ DevCtl: CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+ RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 8GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <1us, L1 <64us ClockPM- Surprise+ LLActRep+ BwNot+ ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 128 bytes, Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 5GT/s, Width x1 TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt+ SltCap: AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug- Surprise+ Slot #0, PowerLimit 0W; Interlock+ NoCompl- SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg- Control: AttnInd Off, PwrInd Off, Power- Interlock- SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock- Changed: MRL- PresDet- LinkState- RootCap: CRSVisible+ RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible+ RootSta: PME ReqID 0000, PMEStatus- PMEPending- DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ NROPrPrP+ LTR+ 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix- EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit- FRS- LN System CLS Not Supported, TPHComp+ ExtTPHComp- ARIFwd- AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+ 10BitTagReq- OBFF Disabled, ARIFwd- AtomicOpsCtl: ReqEn- EgressBlck- LnkCap2: Supported Link Speeds: 2.5-8GT/s, Crosslink- Retimer- 2Retimers- DRS- LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance Preset/De-emphasis: -6dB de-emphasis, 0dB preshoot LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete- EqualizationPhase1- EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest- Retimer- 2Retimers- CrosslinkRes: unsupported Capabilities: [100 v2] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+ AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn- MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap- HeaderLog: 00000000 00000000 00000000 00000000 RootCmd: CERptEn+ NFERptEn+ FERptEn+ RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd- FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0 ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000 Capabilities: [148 v1] Secondary PCI Express LnkCtl3: LnkEquIntrruptEn- PerformEqu- LaneErrStat: 0 Capabilities: [168 v1] Transaction Processing Hints No steering table available Capabilities: [1fc v1] L1 PM Substates L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+ PortCommonModeRestoreTime=70us PortTPowerOnTime=0us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1- T_CommonMode=70us LTR1.2_Threshold=76800ns L1SubCtl2: T_PwrOn=0us Kernel driver in use: pcieport
[ add Terry ] Lukas Wunner wrote: > On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote: > > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote: > > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote: > > > > In order to introduce PCIe power-sequencing, we need to create platform > > > > devices for child nodes of the port driver node. They will get matched > > > > against the pwrseq drivers (if one exists) and then the actuak PCIe > > > > device will reuse the node once it's detected on the bus. > > > [...] > > > > --- a/drivers/pci/pcie/portdrv.c > > > > +++ b/drivers/pci/pcie/portdrv.c > > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > > > > pm_runtime_allow(&dev->dev); > > > > } > > > > > > > > - return 0; > > > > + return devm_of_platform_populate(&dev->dev); > > > > } > > > > > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally > > > the wrong place. Note that you're currently calling this for RCECs > > > (Root Complex Event Collectors) as well, which is likely not what > > > you want. > > > > > > > of_pci_make_dev_node() is only called when the relevant PCI device is > > instantiated which doesn't happen until it's powered-up and scanned - > > precisely the problem I'm trying to address. > > No, of_pci_make_dev_node() is called *before* device_attach(), > i.e. before portdrv has even probed. So it seems this should > work perfectly well for your use case. > > > > > devm functions can't be used in the PCI core, so symmetrically call > > > of_platform_unpopulate() from of_pci_remove_node(). > > > > I don't doubt what you're saying is true (I've seen worse things) but > > this is the probe() callback of a driver using the driver model. Why > > wouldn't devres work? > > The long term plan is to move the functionality in portdrv to > the PCI core. Because devm functions can't be used in the PCI > core, adding new ones to portdrv will *add* a new roadblock to > migrating portdrv to the PCI core. In other words, it makes > future maintenance more difficult. > > Generally, only PCIe port services which share the same interrupt > (hotplug, PME, bandwith notification, flit error counter, ...) > need to live in portdrv. Arbitrary other stuff should not be > shoehorned into portdrv. I came here to say the same thing. It is already the case that portdrv is not a good model to build new functionality upon [1], and PCI core enlightenment should be considered first. The portdrv model is impeding Terry's CXL Port error handling effort, so I am on the lookout for portdrv growing new entanglements to unwind later. [1]: http://lore.kernel.org/r/20221025232535.GA579167@bhelgaas
On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote: > On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said: > > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: > > > Seems like the following must be true but isn't in my case (from > > > pci_bus_add_device()): > > > > > > if (pci_is_bridge(dev)) > > > of_pci_make_dev_node(dev); > > > > > > Shouldn't it evaluate to true for ports? > > > > It should. > > > > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question? > > I cut out the hexdump part, let me know if you really need it. I really need it.
On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said: > On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote: >> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said: >> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: >> > > Seems like the following must be true but isn't in my case (from >> > > pci_bus_add_device()): >> > > >> > > if (pci_is_bridge(dev)) >> > > of_pci_make_dev_node(dev); >> > > >> > > Shouldn't it evaluate to true for ports? >> > >> > It should. >> > >> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question? >> >> I cut out the hexdump part, let me know if you really need it. > > I really need it. > Ok, one more: # lspci -vvvvxxxx -s 0000:00:00 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b (prog-if 00 [Normal decode]) Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0 Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 188 IOMMU group: 7 Region 0: Memory at 60300000 (32-bit, non-prefetchable) [size=4K] Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0 I/O behind bridge: f000-0fff [disabled] [16-bit] Memory behind bridge: 60400000-604fffff [size=1M] [32-bit] Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff [disabled] [64-bit] Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR- BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B- PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+ Address: 00000000a1c00000 Data: 0000 Masking: fffffffe Pending: 00000000 Capabilities: [70] Express (v2) Root Port (Slot+), MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0 ExtTag- RBE+ DevCtl: CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+ RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 8GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <1us, L1 <64us ClockPM- Surprise+ LLActRep+ BwNot+ ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 128 bytes, Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 5GT/s, Width x1 TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt+ SltCap: AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug- Surprise+ Slot #0, PowerLimit 0W; Interlock+ NoCompl- SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg- Control: AttnInd Off, PwrInd Off, Power- Interlock- SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock- Changed: MRL- PresDet- LinkState- RootCap: CRSVisible+ RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible+ RootSta: PME ReqID 0000, PMEStatus- PMEPending- DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ NROPrPrP+ LTR+ 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix- EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit- FRS- LN System CLS Not Supported, TPHComp+ ExtTPHComp- ARIFwd- AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+ 10BitTagReq- OBFF Disabled, ARIFwd- AtomicOpsCtl: ReqEn- EgressBlck- LnkCap2: Supported Link Speeds: 2.5-8GT/s, Crosslink- Retimer- 2Retimers- DRS- LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance Preset/De-emphasis: -6dB de-emphasis, 0dB preshoot LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete- EqualizationPhase1- EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest- Retimer- 2Retimers- CrosslinkRes: unsupported Capabilities: [100 v2] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+ AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn- MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap- HeaderLog: 00000000 00000000 00000000 00000000 RootCmd: CERptEn+ NFERptEn+ FERptEn+ RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd- FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0 ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000 Capabilities: [148 v1] Secondary PCI Express LnkCtl3: LnkEquIntrruptEn- PerformEqu- LaneErrStat: 0 Capabilities: [168 v1] Transaction Processing Hints No steering table available Capabilities: [1fc v1] L1 PM Substates L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+ PortCommonModeRestoreTime=70us PortTPowerOnTime=0us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1- T_CommonMode=70us LTR1.2_Threshold=76800ns L1SubCtl2: T_PwrOn=0us Kernel driver in use: pcieport 00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00 10: 00 00 30 60 00 00 00 00 00 01 ff 00 f0 00 00 00 20: f0 ff 00 00 f1 ff 01 00 00 00 00 00 00 00 00 00 30: 00 00 00 00 40 00 00 00 00 00 00 00 bb 01 02 00 40: 01 50 03 c8 08 00 00 00 00 00 00 00 00 00 00 00 50: 05 70 8b 01 00 00 c0 a1 00 00 00 00 00 00 00 00 60: fe ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 70: 10 00 42 01 00 80 00 00 1f 28 10 00 13 4c 7b 00 80: 48 00 12 f0 3f 00 02 00 c0 03 00 00 18 00 01 00 90: 00 00 00 00 1f 1c 00 00 00 04 00 00 0e 00 00 00 a0: 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 100: 01 00 82 14 00 00 00 00 00 00 40 00 30 20 46 00 110: 00 00 00 00 00 e0 00 00 a0 00 00 00 00 00 00 00 120: 00 00 00 00 00 00 00 00 00 00 00 00 07 00 00 00 130: 00 00 00 000 00 00 00 00 00 00 00 00 00 00 00 140: 00 00 00 00 00 00 00 00 19 00 81 16 00 00 00 00 150: 00 00 00 00 7f 6f 00 00 00 00 00 00 00 00 00 00 160: 00 00 00 00 00 00 00 00 17 00 c1 1f 01 00 00 00 170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1f0: 00 00 00 00 18 00 c1 1f 00 00 00 00 1e 00 01 00 200: 1f 46 00 00 00 46 4b 40 00 00 00 00 00 00 00 00 210: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 220: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 260: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 270: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5f0: 00 ��0 00 00 00 00 00 00 00 00 00 00 00 00 00 600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 700: 3b 00 b1 00 ff ff ff ff 04 00 80 00 00 90 90 1b 710: 20 01 01 00 00 00 00 00 00 40 01 00 40 01 00 00 720: 00 00 00 00 01 00 00 00 11 74 40 03 10 00 00 08 730: 60 00 01 00 07 a0 01 00 ff ff 0f 00 00 00 00 00 740: 0f 00 00 00 00 00 00 00 60 00 21 45 07 a0 21 05 750: 00 00 20 05 00 00 00 00 00 00 00 00 00 00 00 00 760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 800: 00 00 00 00 00 00 00 00 00 00 00 00 90 01 00 00 810: 00 00 00 00 00 00 00 00 00 00 00 00 7f 00 00 00 820: 00 00 c0 a1 00 00 00 00 ff ff ff ff fe ff ff ff 830: 00 00 00 00 ff ff ff ff 00 3c 02 fe 00 00 00 00 840: ff ff ff ff ff ff ff ff 00 00 00 00 ff ff ff ff 850: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff 860: 00 00 00 00 ff ff ff ff ff ff ff ff 00 00 00 00 870: ff ff ff ff ff ff ff ff 00 00 00 00 ff ff ff ff 880: ff ff ff ff 00 00 00 00 00 00 00 00 01 00 00 00 890: 01 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 8a0: 00 00 00 00 00 00 00 00 60 00 00 04 a0 55 01 00 8b0: 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 8c0: 00 00 00 00 44 00 00 00 00 00 00 00 01 00 00 ff 8d0: 00 9c 00 00 32 00 00 00 00 00 00 00 00 00 00 00 8e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 8f0: 00 00 00 00 00 00 00 00 2a 30 30 35 39 32 61 65 900: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 910: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 920: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 930: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 940: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 950: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 960: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 970: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 980: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 990: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 9a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 9b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 9c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 9d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 9e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 9f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff a90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff aa0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ab0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ac0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ad0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ae0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff af0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 b40: 14 00 00 00 d2 00 00 00 00 00 00 00 00 00 00 00 b50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff b90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ba0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff bb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff bc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff bd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff be0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff bf0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff c90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ca0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff cb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff cc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff cd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ce0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff cf0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff d90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff da0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff db0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff dc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff dd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff de0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff df0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff e90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ea0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff eb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ec0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ed0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ee0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ef0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f20: 90 01 00 00 00 00 00 00 01 14 01 00 00 00 00 00 f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00� 00 f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Bartosz
On Wed, Jan 10, 2024 at 12:41:17PM -0800, Dan Williams wrote: > [ add Terry ] > > > Lukas Wunner wrote: > > On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote: > > > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote: > > > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote: > > > > > In order to introduce PCIe power-sequencing, we need to create platform > > > > > devices for child nodes of the port driver node. They will get matched > > > > > against the pwrseq drivers (if one exists) and then the actuak PCIe > > > > > device will reuse the node once it's detected on the bus. > > > > [...] > > > > > --- a/drivers/pci/pcie/portdrv.c > > > > > +++ b/drivers/pci/pcie/portdrv.c > > > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > > > > > pm_runtime_allow(&dev->dev); > > > > > } > > > > > > > > > > - return 0; > > > > > + return devm_of_platform_populate(&dev->dev); > > > > > } > > > > > > > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally > > > > the wrong place. Note that you're currently calling this for RCECs > > > > (Root Complex Event Collectors) as well, which is likely not what > > > > you want. > > > > > > > > > > of_pci_make_dev_node() is only called when the relevant PCI device is > > > instantiated which doesn't happen until it's powered-up and scanned - > > > precisely the problem I'm trying to address. > > > > No, of_pci_make_dev_node() is called *before* device_attach(), > > i.e. before portdrv has even probed. So it seems this should > > work perfectly well for your use case. > > > > > > > > devm functions can't be used in the PCI core, so symmetrically call > > > > of_platform_unpopulate() from of_pci_remove_node(). > > > > > > I don't doubt what you're saying is true (I've seen worse things) but > > > this is the probe() callback of a driver using the driver model. Why > > > wouldn't devres work? > > > > The long term plan is to move the functionality in portdrv to > > the PCI core. Because devm functions can't be used in the PCI > > core, adding new ones to portdrv will *add* a new roadblock to > > migrating portdrv to the PCI core. In other words, it makes > > future maintenance more difficult. > > > > Generally, only PCIe port services which share the same interrupt > > (hotplug, PME, bandwith notification, flit error counter, ...) > > need to live in portdrv. Arbitrary other stuff should not be > > shoehorned into portdrv. > > I came here to say the same thing. It is already the case that portdrv > is not a good model to build new functionality upon [1], and PCI core > enlightenment should be considered first. > The primary reason for plugging the power sequencing into portdrv is due to portdrv binding with all the bridge devices and acting as management driver for the bridges. This is where exactly the power sequencing part needs to be plugged in IMO. But if the idea of the portdrv is just to expose services based on interrupts, then please suggest a better place to plug this power sequencing part. - Mani > The portdrv model is impeding Terry's CXL Port error handling effort, so > I am on the lookout for portdrv growing new entanglements to unwind > later. > > [1]: http://lore.kernel.org/r/20221025232535.GA579167@bhelgaas >
On Thu, Jan 11, 2024 at 05:09:09AM -0600, Bartosz Golaszewski wrote: > On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said: > > On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote: > >> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said: > >> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: > >> > > Seems like the following must be true but isn't in my case (from > >> > > pci_bus_add_device()): > >> > > > >> > > if (pci_is_bridge(dev)) > >> > > of_pci_make_dev_node(dev); > >> > > > >> > > Shouldn't it evaluate to true for ports? > >> > > >> > It should. > >> > > >> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question? > > # lspci -vvvvxxxx -s 0000:00:00 > 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b > (prog-if 00 [Normal decode]) > Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0 [...] > 00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00 ^^ The Header Type in config space is 0x1, i.e. PCI_HEADER_TYPE_BRIDGE. So pci_is_bridge(dev) does return true (unlike what you write above) and control flow enters of_pci_make_dev_node(). But perhaps of_pci_make_dev_node() returns immediately because: /* * If there is already a device tree node linked to this device, * return immediately. */ if (pci_device_to_OF_node(pdev)) return; ..and lspci does list a devicetree node for that Root Port. In any case, of_pci_make_dev_node() is the right place to add the call to of_platform_populate(). Just make sure it's called even if there is already a DT node for the Root Port itself. Thanks, Lukas
On Thu, Jan 11, 2024 at 06:10:09PM +0530, Manivannan Sadhasivam wrote: > The primary reason for plugging the power sequencing into portdrv is due to > portdrv binding with all the bridge devices and acting as management driver > for the bridges. As I've said before, portdrv not only binds to bridges but also Root Complex Event Collectors. And you most likely don't want to populate child DT nodes for those. > This is where exactly the power sequencing part needs to be plugged > in IMO. But if the idea of the portdrv is just to expose services based on > interrupts, then please suggest a better place to plug this power sequencing > part. Again, I'm suggesting to put this into of_pci_make_dev_node(). Thanks, Lukas
On Thu, Jan 11, 2024 at 4:02 PM Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, Jan 11, 2024 at 05:09:09AM -0600, Bartosz Golaszewski wrote: > > On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said: > > > On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote: > > >> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said: > > >> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: > > >> > > Seems like the following must be true but isn't in my case (from > > >> > > pci_bus_add_device()): > > >> > > > > >> > > if (pci_is_bridge(dev)) > > >> > > of_pci_make_dev_node(dev); > > >> > > > > >> > > Shouldn't it evaluate to true for ports? > > >> > > > >> > It should. > > >> > > > >> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question? > > > > # lspci -vvvvxxxx -s 0000:00:00 > > 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b > > (prog-if 00 [Normal decode]) > > Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0 > [...] > > 00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00 > ^^ > The Header Type in config space is 0x1, i.e. PCI_HEADER_TYPE_BRIDGE. > > So pci_is_bridge(dev) does return true (unlike what you write above) > and control flow enters of_pci_make_dev_node(). > > But perhaps of_pci_make_dev_node() returns immediately because: > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not being set. But this is only available if CONFIG_OF_DYNAMIC is enabled which requires OF_UNITTEST (!). We definitely don't need to enable dynamic OF nodes. We don't want to modify the DT, we want to create devices for existing nodes. Also: with the approach in this RFC we maintain a clear hierarchy of devices with the port device being the parent of the power sequencing device which becomes the parent of the actual PCIe device (the port stays the parent of this device too). Bartosz > /* > * If there is already a device tree node linked to this device, > * return immediately. > */ > if (pci_device_to_OF_node(pdev)) > return; > > ...and lspci does list a devicetree node for that Root Port. > > In any case, of_pci_make_dev_node() is the right place to add > the call to of_platform_populate(). Just make sure it's called > even if there is already a DT node for the Root Port itself. > > Thanks, > > Lukas
Hi Bartosz, On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled > which requires OF_UNITTEST (!). Huh? Config PCI_DYNAMIC_OF_NODES does select OF_DYNAMIC. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Jan 11, 2024 at 10:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Bartosz, > > On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not > > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled > > which requires OF_UNITTEST (!). > > Huh? Config PCI_DYNAMIC_OF_NODES does select OF_DYNAMIC. > Indeed, I got something wrong. But in any case: we *don't* need dynamic OF nodes as we don't create new ones. We use the ones that already exist. This is logically a wrong place to add this. Lukas, Terry: am I getting this right - is the port driver supposed to go away at some point? Because I'm not sure I understand what the problem is here. To me it seems that when we create a real device for the PCIe port, then it's only normal to populate its child devices from the port driver. Bartosz > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
On Thu, Jan 11, 2024 at 05:16:45PM +0100, Bartosz Golaszewski wrote: > On Thu, Jan 11, 2024 at 4:02???PM Lukas Wunner <lukas@wunner.de> wrote: > > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: > > > if (pci_is_bridge(dev)) > > > of_pci_make_dev_node(dev); > > > > But perhaps of_pci_make_dev_node() returns immediately because: > > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled > which requires OF_UNITTEST (!). > > We definitely don't need to enable dynamic OF nodes. We don't want to > modify the DT, we want to create devices for existing nodes. Consider refactoring of_pci_make_dev_node() to suit your needs or add a separate function call inside the "if (pci_is_bridge(dev))" clause which populates the child OF nodes. Thanks, Lukas
On Fri, Jan 12, 2024 at 10:43:04AM +0100, Bartosz Golaszewski wrote: > Lukas, Terry: am I getting this right - is the port driver supposed to > go away at some point? Yes, that's the plan. > Because I'm not sure I understand what the > problem is here. To me it seems that when we create a real device for > the PCIe port, then it's only normal to populate its child devices > from the port driver. portdrv is not creating a real device for the PCIe port. It *binds* to that device. The device is created much earlier. NAK for adding this to portdrv. Thanks, Lukas
On Fri, Jan 12, 2024 at 3:43 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, Jan 11, 2024 at 05:16:45PM +0100, Bartosz Golaszewski wrote: > > On Thu, Jan 11, 2024 at 4:02???PM Lukas Wunner <lukas@wunner.de> wrote: > > > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote: > > > > if (pci_is_bridge(dev)) > > > > of_pci_make_dev_node(dev); > > > > > > But perhaps of_pci_make_dev_node() returns immediately because: > > > > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not > > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled > > which requires OF_UNITTEST (!). > > > > We definitely don't need to enable dynamic OF nodes. We don't want to > > modify the DT, we want to create devices for existing nodes. > > Consider refactoring of_pci_make_dev_node() to suit your needs or > add a separate function call inside the "if (pci_is_bridge(dev))" > clause which populates the child OF nodes. The latter because of_pci_make_dev_node() has absolutely nothing to do with the issue this series solves. The uses are pretty much mutually exclusive. If we have a DT node with power related properties, there is no need to create that node because it already exists. Rob
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c index 14a4b89a3b83..401fb731009d 100644 --- a/drivers/pci/pcie/portdrv.c +++ b/drivers/pci/pcie/portdrv.c @@ -13,6 +13,7 @@ #include <linux/pci.h> #include <linux/kernel.h> #include <linux/errno.h> +#include <linux/of_platform.h> #include <linux/pm.h> #include <linux/pm_runtime.h> #include <linux/string.h> @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, pm_runtime_allow(&dev->dev); } - return 0; + return devm_of_platform_populate(&dev->dev); } static void pcie_portdrv_remove(struct pci_dev *dev)