| Message ID | 20240104130123.37115-4-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: introduce the concept of power sequencing of PCIe devices | expand |
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 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, 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 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)