| Message ID | 20241022-pci-pwrctl-rework-v1-4-94a7e90f58c5@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/pwrctl: Ensure that the pwrctl drivers are probed before PCI client drivers | expand |
On Tue, 22 Oct 2024 at 12:28, Manivannan Sadhasivam via B4 Relay <devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote: > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > This makes the pci_bus_add_device() API easier to maintain. Also add more > comments to the helper to describe how the devices are created. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/bus.c | 59 ++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 41 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 351af581904f..c4cae1775c9e 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -321,6 +321,46 @@ void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { } > > void __weak pcibios_bus_add_device(struct pci_dev *pdev) { } > > +/* > + * Create pwrctl devices (if required) for the PCI devices to handle the power > + * state. > + */ > +static void pci_pwrctl_create_devices(struct pci_dev *dev) > +{ > + struct device_node *np = dev_of_node(&dev->dev); > + struct device *parent = &dev->dev; > + struct platform_device *pdev; > + > + /* > + * First ensure that we are starting from a PCI bridge and it has a > + * corresponding devicetree node. > + */ > + if (np && pci_is_bridge(dev)) { > + /* > + * Now look for the child PCI device nodes and create pwrctl > + * devices for them. The pwrctl device drivers will manage the > + * power state of the devices. > + */ > + for_each_child_of_node_scoped(np, child) { > + /* > + * First check whether the pwrctl device really needs to > + * be created or not. This is decided based on at least > + * one of the power supplies being defined in the > + * devicetree node of the device. > + */ > + if (!of_pci_is_supply_present(child)) { > + pci_dbg(dev, "skipping OF node: %s\n", child->name); > + return; > + } > + > + /* Now create the pwrctl device */ > + pdev = of_platform_device_create(child, NULL, parent); > + if (!pdev) > + pci_err(dev, "failed to create OF node: %s\n", child->name); > + } > + } > +} > + > /** > * pci_bus_add_device - start driver for a single device > * @dev: device to add > @@ -345,24 +385,7 @@ void pci_bus_add_device(struct pci_dev *dev) > pci_proc_attach_device(dev); > pci_bridge_d3_update(dev); > > - if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) { > - for_each_child_of_node_scoped(dn, child) { > - /* > - * First check whether the pwrctl device needs to be > - * created or not. This is decided based on at least > - * one of the power supplies being defined in the > - * devicetree node of the device. > - */ > - if (!of_pci_is_supply_present(child)) { > - pci_dbg(dev, "skipping OF node: %s\n", child->name); > - continue; > - } > - > - pdev = of_platform_device_create(child, NULL, &dev->dev); > - if (!pdev) > - pci_err(dev, "failed to create OF node: %s\n", child->name); > - } > - } > + pci_pwrctl_create_devices(dev); > > /* > * Create a device link between the PCI device and pwrctl device (if > > -- > 2.25.1 > > Would it be possible to move this into drivers/pwrctl/ and provide a header stub for when PCI_PWRCTL is disabled in Kconfig? Bart
On Wed, Oct 23, 2024 at 12:23:57PM +0200, Bartosz Golaszewski wrote: > On Tue, 22 Oct 2024 at 12:28, Manivannan Sadhasivam via B4 Relay > <devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote: > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > This makes the pci_bus_add_device() API easier to maintain. Also add more > > comments to the helper to describe how the devices are created. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/bus.c | 59 ++++++++++++++++++++++++++++++++++++++----------------- > > 1 file changed, 41 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > > index 351af581904f..c4cae1775c9e 100644 > > --- a/drivers/pci/bus.c > > +++ b/drivers/pci/bus.c > > @@ -321,6 +321,46 @@ void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { } > > > > void __weak pcibios_bus_add_device(struct pci_dev *pdev) { } > > > > +/* > > + * Create pwrctl devices (if required) for the PCI devices to handle the power > > + * state. > > + */ > > +static void pci_pwrctl_create_devices(struct pci_dev *dev) > > +{ > > + struct device_node *np = dev_of_node(&dev->dev); > > + struct device *parent = &dev->dev; > > + struct platform_device *pdev; > > + > > + /* > > + * First ensure that we are starting from a PCI bridge and it has a > > + * corresponding devicetree node. > > + */ > > + if (np && pci_is_bridge(dev)) { > > + /* > > + * Now look for the child PCI device nodes and create pwrctl > > + * devices for them. The pwrctl device drivers will manage the > > + * power state of the devices. > > + */ > > + for_each_child_of_node_scoped(np, child) { > > + /* > > + * First check whether the pwrctl device really needs to > > + * be created or not. This is decided based on at least > > + * one of the power supplies being defined in the > > + * devicetree node of the device. > > + */ > > + if (!of_pci_is_supply_present(child)) { > > + pci_dbg(dev, "skipping OF node: %s\n", child->name); > > + return; > > + } > > + > > + /* Now create the pwrctl device */ > > + pdev = of_platform_device_create(child, NULL, parent); > > + if (!pdev) > > + pci_err(dev, "failed to create OF node: %s\n", child->name); > > + } > > + } > > +} > > + > > /** > > * pci_bus_add_device - start driver for a single device > > * @dev: device to add > > @@ -345,24 +385,7 @@ void pci_bus_add_device(struct pci_dev *dev) > > pci_proc_attach_device(dev); > > pci_bridge_d3_update(dev); > > > > - if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) { > > - for_each_child_of_node_scoped(dn, child) { > > - /* > > - * First check whether the pwrctl device needs to be > > - * created or not. This is decided based on at least > > - * one of the power supplies being defined in the > > - * devicetree node of the device. > > - */ > > - if (!of_pci_is_supply_present(child)) { > > - pci_dbg(dev, "skipping OF node: %s\n", child->name); > > - continue; > > - } > > - > > - pdev = of_platform_device_create(child, NULL, &dev->dev); > > - if (!pdev) > > - pci_err(dev, "failed to create OF node: %s\n", child->name); > > - } > > - } > > + pci_pwrctl_create_devices(dev); > > > > /* > > * Create a device link between the PCI device and pwrctl device (if > > > > -- > > 2.25.1 > > > > > > Would it be possible to move this into drivers/pwrctl/ and provide a > header stub for when PCI_PWRCTL is disabled in Kconfig? > Unfortunately, no. Because the pwrctl drivers are modular and PCI core is built-in. So if we use the pwrctl APIs in PCI core, then it will require pwrctl to always be built-in, which we do not want. - Mani
On Fri, Oct 25, 2024 at 10:05 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > > > > Would it be possible to move this into drivers/pwrctl/ and provide a > > header stub for when PCI_PWRCTL is disabled in Kconfig? > > > > Unfortunately, no. Because the pwrctl drivers are modular and PCI core is > built-in. So if we use the pwrctl APIs in PCI core, then it will require pwrctl > to always be built-in, which we do not want. > Got it. Bartosz
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 351af581904f..c4cae1775c9e 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -321,6 +321,46 @@ void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { } void __weak pcibios_bus_add_device(struct pci_dev *pdev) { } +/* + * Create pwrctl devices (if required) for the PCI devices to handle the power + * state. + */ +static void pci_pwrctl_create_devices(struct pci_dev *dev) +{ + struct device_node *np = dev_of_node(&dev->dev); + struct device *parent = &dev->dev; + struct platform_device *pdev; + + /* + * First ensure that we are starting from a PCI bridge and it has a + * corresponding devicetree node. + */ + if (np && pci_is_bridge(dev)) { + /* + * Now look for the child PCI device nodes and create pwrctl + * devices for them. The pwrctl device drivers will manage the + * power state of the devices. + */ + for_each_child_of_node_scoped(np, child) { + /* + * First check whether the pwrctl device really needs to + * be created or not. This is decided based on at least + * one of the power supplies being defined in the + * devicetree node of the device. + */ + if (!of_pci_is_supply_present(child)) { + pci_dbg(dev, "skipping OF node: %s\n", child->name); + return; + } + + /* Now create the pwrctl device */ + pdev = of_platform_device_create(child, NULL, parent); + if (!pdev) + pci_err(dev, "failed to create OF node: %s\n", child->name); + } + } +} + /** * pci_bus_add_device - start driver for a single device * @dev: device to add @@ -345,24 +385,7 @@ void pci_bus_add_device(struct pci_dev *dev) pci_proc_attach_device(dev); pci_bridge_d3_update(dev); - if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) { - for_each_child_of_node_scoped(dn, child) { - /* - * First check whether the pwrctl device needs to be - * created or not. This is decided based on at least - * one of the power supplies being defined in the - * devicetree node of the device. - */ - if (!of_pci_is_supply_present(child)) { - pci_dbg(dev, "skipping OF node: %s\n", child->name); - continue; - } - - pdev = of_platform_device_create(child, NULL, &dev->dev); - if (!pdev) - pci_err(dev, "failed to create OF node: %s\n", child->name); - } - } + pci_pwrctl_create_devices(dev); /* * Create a device link between the PCI device and pwrctl device (if