diff mbox series

[RFC,6/9] PCI: create platform devices for child OF nodes of the port node

Message ID 20240201155532.49707-7-brgl@bgdev.pl
State New
Headers show
Series power: sequencing: implement the subsystem and add first users | expand

Commit Message

Bartosz Golaszewski Feb. 1, 2024, 3:55 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to introduce PCI power-sequencing, we need to create platform
devices for child nodes of the port node. They will get matched against
the pwrseq drivers (if one exists) and then the actual PCI device will
reuse the node once it's detected on the bus.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/bus.c    | 9 ++++++++-
 drivers/pci/remove.c | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson Feb. 2, 2024, 2:59 a.m. UTC | #1 
On Thu, Feb 01, 2024 at 04:55:29PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> In order to introduce PCI power-sequencing,

Please provide a proper problem description.

> we need to create platform

And properly express why this is a "need".

> devices for child nodes of the port node. They will get matched against
> the pwrseq drivers

That's not what happens in your code, the child nodes of the bridge node
in DeviceTree will match against arbitrary platform_drivers.

I also would like this commit message to express that the job of the
matched device is to:

1) power up said device, followed by triggering a scan on the parent PCI
bus during it's probe function.

2)  power down said device, during its remove function.

> (if one exists) and then the actual PCI device will
> reuse the node once it's detected on the bus.

I think the "reuse" deserves to be clarified as there will be both a pci
and a platform device associated with the same of_node.

> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/bus.c    | 9 ++++++++-
>  drivers/pci/remove.c | 2 ++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 826b5016a101..17ab41094c4e 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -12,6 +12,7 @@
>  #include <linux/errno.h>
>  #include <linux/ioport.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/proc_fs.h>
>  #include <linux/slab.h>
>  
> @@ -342,8 +343,14 @@ void pci_bus_add_device(struct pci_dev *dev)
>  	 */
>  	pcibios_bus_add_device(dev);
>  	pci_fixup_device(pci_fixup_final, dev);
> -	if (pci_is_bridge(dev))
> +	if (pci_is_bridge(dev)) {
>  		of_pci_make_dev_node(dev);
> +		retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
> +					      &dev->dev);

I'm not familiar enough with the ins and outs of the PCI code. Can you
confirm that there are no problems with this (possibly) calling
pci_rescan_bus() before the bridge device is fully initialized below?

Regards,
Bjorn

> +		if (retval)
> +			pci_err(dev, "failed to populate child OF nodes (%d)\n",
> +				retval);
> +	}
>  	pci_create_sysfs_dev_files(dev);
>  	pci_proc_attach_device(dev);
>  	pci_bridge_d3_update(dev);
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index d749ea8250d6..fc9db2805888 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/of_platform.h>
>  #include "pci.h"
>  
>  static void pci_free_resources(struct pci_dev *dev)
> @@ -22,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
>  		device_release_driver(&dev->dev);
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
> +		of_platform_depopulate(&dev->dev);
>  		of_pci_remove_node(dev);
>  
>  		pci_dev_assign_added(dev, false);
> -- 
> 2.40.1
>
Bartosz Golaszewski Feb. 2, 2024, 9:03 a.m. UTC | #2 
On Fri, Feb 2, 2024 at 3:59 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:29PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > In order to introduce PCI power-sequencing,
>
> Please provide a proper problem description.
>
> > we need to create platform
>
> And properly express why this is a "need".
>
> > devices for child nodes of the port node. They will get matched against
> > the pwrseq drivers
>
> That's not what happens in your code, the child nodes of the bridge node
> in DeviceTree will match against arbitrary platform_drivers.
>
> I also would like this commit message to express that the job of the
> matched device is to:
>
> 1) power up said device, followed by triggering a scan on the parent PCI
> bus during it's probe function.
>
> 2)  power down said device, during its remove function.
>
> > (if one exists) and then the actual PCI device will
> > reuse the node once it's detected on the bus.
>
> I think the "reuse" deserves to be clarified as there will be both a pci
> and a platform device associated with the same of_node.
>

Noted all of the above. Thanks!

> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/pci/bus.c    | 9 ++++++++-
> >  drivers/pci/remove.c | 2 ++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 826b5016a101..17ab41094c4e 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/ioport.h>
> >  #include <linux/of.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/proc_fs.h>
> >  #include <linux/slab.h>
> >
> > @@ -342,8 +343,14 @@ void pci_bus_add_device(struct pci_dev *dev)
> >        */
> >       pcibios_bus_add_device(dev);
> >       pci_fixup_device(pci_fixup_final, dev);
> > -     if (pci_is_bridge(dev))
> > +     if (pci_is_bridge(dev)) {
> >               of_pci_make_dev_node(dev);
> > +             retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
> > +                                           &dev->dev);
>
> I'm not familiar enough with the ins and outs of the PCI code. Can you
> confirm that there are no problems with this (possibly) calling
> pci_rescan_bus() before the bridge device is fully initialized below?
>

I'll clarify that. I'm not that well versed with PCI code either but
will get help from the right people.

Bart

> Regards,
> Bjorn
>
> > +             if (retval)
> > +                     pci_err(dev, "failed to populate child OF nodes (%d)\n",
> > +                             retval);
> > +     }
> >       pci_create_sysfs_dev_files(dev);
> >       pci_proc_attach_device(dev);
> >       pci_bridge_d3_update(dev);
> > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> > index d749ea8250d6..fc9db2805888 100644
> > --- a/drivers/pci/remove.c
> > +++ b/drivers/pci/remove.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/pci.h>
> >  #include <linux/module.h>
> > +#include <linux/of_platform.h>
> >  #include "pci.h"
> >
> >  static void pci_free_resources(struct pci_dev *dev)
> > @@ -22,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
> >               device_release_driver(&dev->dev);
> >               pci_proc_detach_device(dev);
> >               pci_remove_sysfs_dev_files(dev);
> > +             of_platform_depopulate(&dev->dev);
> >               of_pci_remove_node(dev);
> >
> >               pci_dev_assign_added(dev, false);
> > --
> > 2.40.1
> >
diff mbox series

Patch

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 826b5016a101..17ab41094c4e 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -12,6 +12,7 @@ 
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/proc_fs.h>
 #include <linux/slab.h>
 
@@ -342,8 +343,14 @@  void pci_bus_add_device(struct pci_dev *dev)
 	 */
 	pcibios_bus_add_device(dev);
 	pci_fixup_device(pci_fixup_final, dev);
-	if (pci_is_bridge(dev))
+	if (pci_is_bridge(dev)) {
 		of_pci_make_dev_node(dev);
+		retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
+					      &dev->dev);
+		if (retval)
+			pci_err(dev, "failed to populate child OF nodes (%d)\n",
+				retval);
+	}
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index d749ea8250d6..fc9db2805888 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include "pci.h"
 
 static void pci_free_resources(struct pci_dev *dev)
@@ -22,6 +23,7 @@  static void pci_stop_dev(struct pci_dev *dev)
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
+		of_platform_depopulate(&dev->dev);
 		of_pci_remove_node(dev);
 
 		pci_dev_assign_added(dev, false);