| Message ID | 20240626-qps615-v1-3-2ade7bd91e02@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI: enable Power and configure the QPS615 PCIe switch | expand |
On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru <quic_krichai@quicinc.com> wrote: > > Currently the power control driver is child of pci-pci bridge driver, > this will cause issue when suspend resume is introduced in the pwr > control driver. If the supply is removed to the endpoint in the > power control driver then the config space access initaited by the > pci-pci bridge driver can cause issues like Timeouts. > > For this reason change the parent to controller from pci-pci bridge. Newline before trailers please. > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/pci/bus.c | 5 +++-- > drivers/pci/pwrctl/core.c | 7 ++++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 3e3517567721..eedab4aabd81 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -335,6 +335,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { } > void pci_bus_add_device(struct pci_dev *dev) > { > struct device_node *dn = dev->dev.of_node; > + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > int retval; > > /* > @@ -356,9 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev) > > pci_dev_assign_added(dev, true); > > - if (pci_is_bridge(dev)) { > + if (pci_is_bridge(dev) && host) { I know I told you to check the return value of pci_find_host_bridge() in private but now after a second look I see it's just a multi-layer wrapper around container_of() and it looks like it cannot fail so - correct me if I'm wrong - this can be dropped after all. > retval = of_platform_populate(dev->dev.of_node, NULL, NULL, > - &dev->dev); > + host->dev.parent); > if (retval) > pci_err(dev, "failed to populate child OF nodes (%d)\n", > retval); > diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c > index feca26ad2f6a..4c0d0f3b15f8 100644 > --- a/drivers/pci/pwrctl/core.c > +++ b/drivers/pci/pwrctl/core.c > @@ -10,6 +10,7 @@ > #include <linux/pci-pwrctl.h> > #include <linux/property.h> > #include <linux/slab.h> New line here, please. > +#include "../pci.h" > > static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action, > void *data) > @@ -64,18 +65,22 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action, > */ > int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl) > { > + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0); > int ret; > > if (!pwrctl->dev) > return -ENODEV; > > + if (!bus) > + return -ENODEV; This - on the other hand - can fail, so the check if valid. Could you assign it and then test it in a single spot for better readability? Bart > + > pwrctl->nb.notifier_call = pci_pwrctl_notify; > ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb); > if (ret) > return ret; > > pci_lock_rescan_remove(); > - pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus); > + pci_rescan_bus(bus); > pci_unlock_rescan_remove(); > > return 0; > > -- > 2.42.0 >
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 3e3517567721..eedab4aabd81 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -335,6 +335,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { } void pci_bus_add_device(struct pci_dev *dev) { struct device_node *dn = dev->dev.of_node; + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); int retval; /* @@ -356,9 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev) pci_dev_assign_added(dev, true); - if (pci_is_bridge(dev)) { + if (pci_is_bridge(dev) && host) { retval = of_platform_populate(dev->dev.of_node, NULL, NULL, - &dev->dev); + host->dev.parent); if (retval) pci_err(dev, "failed to populate child OF nodes (%d)\n", retval); diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c index feca26ad2f6a..4c0d0f3b15f8 100644 --- a/drivers/pci/pwrctl/core.c +++ b/drivers/pci/pwrctl/core.c @@ -10,6 +10,7 @@ #include <linux/pci-pwrctl.h> #include <linux/property.h> #include <linux/slab.h> +#include "../pci.h" static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action, void *data) @@ -64,18 +65,22 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action, */ int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl) { + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0); int ret; if (!pwrctl->dev) return -ENODEV; + if (!bus) + return -ENODEV; + pwrctl->nb.notifier_call = pci_pwrctl_notify; ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb); if (ret) return ret; pci_lock_rescan_remove(); - pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus); + pci_rescan_bus(bus); pci_unlock_rescan_remove(); return 0;
Currently the power control driver is child of pci-pci bridge driver, this will cause issue when suspend resume is introduced in the pwr control driver. If the supply is removed to the endpoint in the power control driver then the config space access initaited by the pci-pci bridge driver can cause issues like Timeouts. For this reason change the parent to controller from pci-pci bridge. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/pci/bus.c | 5 +++-- drivers/pci/pwrctl/core.c | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-)