| Message ID | 20240201155532.49707-7-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: sequencing: implement the subsystem and add first users | expand |
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 >
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 --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);