| Message ID | 20220701162726.31346-4-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: brcmstb: Re-submit reverted patchset | expand |
On Fri, Jul 01, 2022 at 12:27:24PM -0400, Jim Quinlan wrote: > This Broadcom STB PCIe RC driver has one port and connects directly to one > device, be it a switch or an endpoint. We want to be able to leverage the > recently added mechanism that allocates and turns on/off subdevice > regulators. > > All that needs to be done is to put the regulator DT nodes in the bridge > below host and to set the pci_ops methods add_bus and remove_bus. > > Note that the pci_subdev_regulators_add_bus() method is wrapped for two > reasons: > > 1. To achieve link up after the voltage regulators are turned on. > > 2. If, in the case of an unsuccessful link up, to redirect any PCIe > accesses to subdevices, e.g. the scan for DEV/ID. This redirection > is needed because the Broadcom PCIe HW will issue a CPU abort if such > an access is made when the link is down. > > [bhelgaas: fold in > https://lore.kernel.org/r/20220112013100.48029-1-jim2101024@gmail.com] > Link: https://lore.kernel.org/r/20220106160332.2143-7-jim2101024@gmail.com > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 86 ++++++++++++++++++++++++--- > 1 file changed, 77 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 661d3834c6da..a86bf502a265 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -196,6 +196,8 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, > static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); > static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); > static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); > +static int brcm_pcie_linkup(struct brcm_pcie *pcie); > +static int brcm_pcie_add_bus(struct pci_bus *bus); I think the brcm_pcie_add_bus() declaration is unnecessary. The brcm_pcie_linkup() one is probably unnecessary, too, but would require a lot of reordering that I don't think we should do in this series. > enum { > RGR1_SW_INIT_1, > @@ -329,6 +331,8 @@ struct brcm_pcie { > u32 hw_rev; > void (*perst_set)(struct brcm_pcie *pcie, u32 val); > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); > + bool refusal_mode; > + struct subdev_regulators *sr; > }; > > static inline bool is_bmips(const struct brcm_pcie *pcie) > @@ -497,6 +501,33 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus) > return 0; > } > > +static int brcm_pcie_add_bus(struct pci_bus *bus) > +{ > + struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata; > + int ret; > + > + if (!bus->parent || !pci_is_root_bus(bus->parent)) > + return 0; > + > + ret = pci_subdev_regulators_add_bus(bus); > + if (ret) > + return ret; > + > + /* Grab the regulators for suspend/resume */ > + pcie->sr = bus->dev.driver_data; > + > + /* > + * If we have failed linkup there is no point to return an error as > + * currently it will cause a WARNING() from pci_alloc_child_bus(). > + * We return 0 and turn on the "refusal_mode" so that any further > + * accesses to the pci_dev just get 0xffffffff > + */ > + if (brcm_pcie_linkup(pcie) != 0) > + pcie->refusal_mode = true; Is there a bisection hole between the previous patch and this one? The previous patch sets .add_bus() to pci_subdev_regulators_add_bus(), so we'll turn on the regulators, but we don't know whether the link came up. If it didn't come up, it looks like we might get a CPU abort when enumerating? I think we should split out the refusal_mode patch: - Add refusal mode - Add subdev regulator mechanism - This patch (which would then be clearly about managing regulators in suspend/resume, IIUC) > + return 0; > +} > + > static void pci_subdev_regulators_remove_bus(struct pci_bus *bus) > { > struct device *dev = &bus->dev; > @@ -826,6 +857,18 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn, > /* Accesses to the RC go right to the RC registers if slot==0 */ > if (pci_is_root_bus(bus)) > return PCI_SLOT(devfn) ? NULL : base + where; > + if (pcie->refusal_mode) { > + /* > + * At this point we do not have link. There will be a CPU > + * abort -- a quirk with this controller --if Linux tries > + * to read any config-space registers besides those > + * targeting the host bridge. To prevent this we hijack > + * the address to point to a safe access that will return > + * 0xffffffff. > + */ > + writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI); > + return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3); > + } > > /* For devices, write to the config space index register */ > idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0); > @@ -854,7 +897,7 @@ static struct pci_ops brcm_pcie_ops = { > .map_bus = brcm_pcie_map_conf, > .read = pci_generic_config_read, > .write = pci_generic_config_write, > - .add_bus = pci_subdev_regulators_add_bus, > + .add_bus = brcm_pcie_add_bus, > .remove_bus = pci_subdev_regulators_remove_bus, > }; > > @@ -1327,6 +1370,14 @@ static int brcm_pcie_suspend(struct device *dev) > return ret; > } > > + if (pcie->sr) { > + ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies); > + if (ret) { > + dev_err(dev, "Could not turn off regulators\n"); > + reset_control_reset(pcie->rescal); > + return ret; > + } > + } > clk_disable_unprepare(pcie->clk); > > return 0; > @@ -1344,9 +1395,17 @@ static int brcm_pcie_resume(struct device *dev) > if (ret) > return ret; > > + if (pcie->sr) { > + ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies); > + if (ret) { > + dev_err(dev, "Could not turn on regulators\n"); > + goto err_disable_clk; > + } > + } > + > ret = reset_control_reset(pcie->rescal); > if (ret) > - goto err_disable_clk; > + goto err_regulator; > > ret = brcm_phy_start(pcie); > if (ret) > @@ -1378,6 +1437,9 @@ static int brcm_pcie_resume(struct device *dev) > > err_reset: > reset_control_rearm(pcie->rescal); > +err_regulator: > + if (pcie->sr) > + regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies); > err_disable_clk: > clk_disable_unprepare(pcie->clk); > return ret; > @@ -1488,10 +1550,6 @@ static int brcm_pcie_probe(struct platform_device *pdev) > if (ret) > goto fail; > > - ret = brcm_pcie_linkup(pcie); > - if (ret) > - goto fail; > - > pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION); > if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) { > dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n"); > @@ -1513,7 +1571,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pcie); > > - return pci_host_probe(bridge); > + ret = pci_host_probe(bridge); > + if (!ret && !brcm_pcie_link_up(pcie)) > + ret = -ENODEV; > + > + if (ret) { > + brcm_pcie_remove(pdev); > + return ret; > + } > + > + return 0; > + > fail: > __brcm_pcie_remove(pcie); > return ret; > @@ -1522,8 +1590,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) > MODULE_DEVICE_TABLE(of, brcm_pcie_match); > > static const struct dev_pm_ops brcm_pcie_pm_ops = { > - .suspend = brcm_pcie_suspend, > - .resume = brcm_pcie_resume, > + .suspend_noirq = brcm_pcie_suspend, > + .resume_noirq = brcm_pcie_resume, Can you name these brcm_pcie_suspend_noirq() and brcm_pcie_resume_noirq() to match the hook names? > }; > > static struct platform_driver brcm_pcie_driver = { > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jul 6, 2022 at 7:13 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 01, 2022 at 12:27:24PM -0400, Jim Quinlan wrote: > > This Broadcom STB PCIe RC driver has one port and connects directly to one > > device, be it a switch or an endpoint. We want to be able to leverage the > > recently added mechanism that allocates and turns on/off subdevice > > regulators. > > > > All that needs to be done is to put the regulator DT nodes in the bridge > > below host and to set the pci_ops methods add_bus and remove_bus. > > > > Note that the pci_subdev_regulators_add_bus() method is wrapped for two > > reasons: > > > > 1. To achieve link up after the voltage regulators are turned on. > > > > 2. If, in the case of an unsuccessful link up, to redirect any PCIe > > accesses to subdevices, e.g. the scan for DEV/ID. This redirection > > is needed because the Broadcom PCIe HW will issue a CPU abort if such > > an access is made when the link is down. > > > > [bhelgaas: fold in > > https://lore.kernel.org/r/20220112013100.48029-1-jim2101024@gmail.com] > > Link: https://lore.kernel.org/r/20220106160332.2143-7-jim2101024@gmail.com > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 86 ++++++++++++++++++++++++--- > > 1 file changed, 77 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 661d3834c6da..a86bf502a265 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -196,6 +196,8 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, > > static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); > > static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); > > static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); > > +static int brcm_pcie_linkup(struct brcm_pcie *pcie); > > +static int brcm_pcie_add_bus(struct pci_bus *bus); > > I think the brcm_pcie_add_bus() declaration is unnecessary. Will remove. > > The brcm_pcie_linkup() one is probably unnecessary, too, but would > require a lot of reordering that I don't think we should do in this > series. I have a future commit that will remove all forward declarations. I just wanted to keep this the un-revert patchset simple. > > > enum { > > RGR1_SW_INIT_1, > > @@ -329,6 +331,8 @@ struct brcm_pcie { > > u32 hw_rev; > > void (*perst_set)(struct brcm_pcie *pcie, u32 val); > > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); > > + bool refusal_mode; > > + struct subdev_regulators *sr; > > }; > > > > static inline bool is_bmips(const struct brcm_pcie *pcie) > > @@ -497,6 +501,33 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus) > > return 0; > > } > > > > +static int brcm_pcie_add_bus(struct pci_bus *bus) > > +{ > > + struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata; > > + int ret; > > + > > + if (!bus->parent || !pci_is_root_bus(bus->parent)) > > + return 0; > > + > > + ret = pci_subdev_regulators_add_bus(bus); > > + if (ret) > > + return ret; > > + > > + /* Grab the regulators for suspend/resume */ > > + pcie->sr = bus->dev.driver_data; > > + > > + /* > > + * If we have failed linkup there is no point to return an error as > > + * currently it will cause a WARNING() from pci_alloc_child_bus(). > > + * We return 0 and turn on the "refusal_mode" so that any further > > + * accesses to the pci_dev just get 0xffffffff > > + */ > > + if (brcm_pcie_linkup(pcie) != 0) > > + pcie->refusal_mode = true; > > Is there a bisection hole between the previous patch and this one? > The previous patch sets .add_bus() to pci_subdev_regulators_add_bus(), > so we'll turn on the regulators, but we don't know whether the link > came up. If it didn't come up, it looks like we might get a CPU abort > when enumerating? I don't think so. At this commit, attempting the link-up is still done before the call to pci_host_probe(). Since there is no power there will be no link, the probe will fail and pci_host_probe() will never be invoked. > > I think we should split out the refusal_mode patch: > > - Add refusal mode > - Add subdev regulator mechanism > - This patch (which would then be clearly about managing regulators > in suspend/resume, IIUC) Will do. > > > + return 0; > > +} > > + > > static void pci_subdev_regulators_remove_bus(struct pci_bus *bus) > > { > > struct device *dev = &bus->dev; > > @@ -826,6 +857,18 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn, > > /* Accesses to the RC go right to the RC registers if slot==0 */ > > if (pci_is_root_bus(bus)) > > return PCI_SLOT(devfn) ? NULL : base + where; > > + if (pcie->refusal_mode) { > > + /* > > + * At this point we do not have link. There will be a CPU > > + * abort -- a quirk with this controller --if Linux tries > > + * to read any config-space registers besides those > > + * targeting the host bridge. To prevent this we hijack > > + * the address to point to a safe access that will return > > + * 0xffffffff. > > + */ > > + writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI); > > + return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3); > > + } > > > > /* For devices, write to the config space index register */ > > idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0); > > @@ -854,7 +897,7 @@ static struct pci_ops brcm_pcie_ops = { > > .map_bus = brcm_pcie_map_conf, > > .read = pci_generic_config_read, > > .write = pci_generic_config_write, > > - .add_bus = pci_subdev_regulators_add_bus, > > + .add_bus = brcm_pcie_add_bus, > > .remove_bus = pci_subdev_regulators_remove_bus, > > }; > > > > @@ -1327,6 +1370,14 @@ static int brcm_pcie_suspend(struct device *dev) > > return ret; > > } > > > > + if (pcie->sr) { > > + ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies); > > + if (ret) { > > + dev_err(dev, "Could not turn off regulators\n"); > > + reset_control_reset(pcie->rescal); > > + return ret; > > + } > > + } > > clk_disable_unprepare(pcie->clk); > > > > return 0; > > @@ -1344,9 +1395,17 @@ static int brcm_pcie_resume(struct device *dev) > > if (ret) > > return ret; > > > > + if (pcie->sr) { > > + ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies); > > + if (ret) { > > + dev_err(dev, "Could not turn on regulators\n"); > > + goto err_disable_clk; > > + } > > + } > > + > > ret = reset_control_reset(pcie->rescal); > > if (ret) > > - goto err_disable_clk; > > + goto err_regulator; > > > > ret = brcm_phy_start(pcie); > > if (ret) > > @@ -1378,6 +1437,9 @@ static int brcm_pcie_resume(struct device *dev) > > > > err_reset: > > reset_control_rearm(pcie->rescal); > > +err_regulator: > > + if (pcie->sr) > > + regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies); > > err_disable_clk: > > clk_disable_unprepare(pcie->clk); > > return ret; > > @@ -1488,10 +1550,6 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > if (ret) > > goto fail; > > > > - ret = brcm_pcie_linkup(pcie); > > - if (ret) > > - goto fail; > > - > > pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION); > > if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) { > > dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n"); > > @@ -1513,7 +1571,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, pcie); > > > > - return pci_host_probe(bridge); > > + ret = pci_host_probe(bridge); > > + if (!ret && !brcm_pcie_link_up(pcie)) > > + ret = -ENODEV; > > + > > + if (ret) { > > + brcm_pcie_remove(pdev); > > + return ret; > > + } > > + > > + return 0; > > + > > fail: > > __brcm_pcie_remove(pcie); > > return ret; > > @@ -1522,8 +1590,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > MODULE_DEVICE_TABLE(of, brcm_pcie_match); > > > > static const struct dev_pm_ops brcm_pcie_pm_ops = { > > - .suspend = brcm_pcie_suspend, > > - .resume = brcm_pcie_resume, > > + .suspend_noirq = brcm_pcie_suspend, > > + .resume_noirq = brcm_pcie_resume, > > Can you name these brcm_pcie_suspend_noirq() and > brcm_pcie_resume_noirq() to match the hook names? Yes. Jim Quintlan Broadcom STB > > > }; > > > > static struct platform_driver brcm_pcie_driver = { > > -- > > 2.17.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jul 08, 2022 at 11:16:08AM -0400, Jim Quinlan wrote: > On Wed, Jul 6, 2022 at 7:13 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 01, 2022 at 12:27:24PM -0400, Jim Quinlan wrote: > > > This Broadcom STB PCIe RC driver has one port and connects directly to one > > > device, be it a switch or an endpoint. We want to be able to leverage the > > > recently added mechanism that allocates and turns on/off subdevice > > > regulators. > > > ... > > > + * If we have failed linkup there is no point to return an error as > > > + * currently it will cause a WARNING() from pci_alloc_child_bus(). > > > + * We return 0 and turn on the "refusal_mode" so that any further > > > + * accesses to the pci_dev just get 0xffffffff > > > + */ > > > + if (brcm_pcie_linkup(pcie) != 0) > > > + pcie->refusal_mode = true; > > > > Is there a bisection hole between the previous patch and this one? > > The previous patch sets .add_bus() to pci_subdev_regulators_add_bus(), > > so we'll turn on the regulators, but we don't know whether the link > > came up. If it didn't come up, it looks like we might get a CPU abort > > when enumerating? > > I don't think so. At this commit, attempting the link-up is still > done before the call > to pci_host_probe(). Since there is no power there will be no link, > the probe will > fail and pci_host_probe() will never be invoked. Ah, OK, thanks for the explanation. > > I think we should split out the refusal_mode patch: > > > > - Add refusal mode > > - Add subdev regulator mechanism > > - This patch (which would then be clearly about managing regulators > > in suspend/resume, IIUC) > > Will do. If it's not too hard to do, I think splitting it will make the patches easier to read. Bjorn
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 661d3834c6da..a86bf502a265 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -196,6 +196,8 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); +static int brcm_pcie_linkup(struct brcm_pcie *pcie); +static int brcm_pcie_add_bus(struct pci_bus *bus); enum { RGR1_SW_INIT_1, @@ -329,6 +331,8 @@ struct brcm_pcie { u32 hw_rev; void (*perst_set)(struct brcm_pcie *pcie, u32 val); void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); + bool refusal_mode; + struct subdev_regulators *sr; }; static inline bool is_bmips(const struct brcm_pcie *pcie) @@ -497,6 +501,33 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus) return 0; } +static int brcm_pcie_add_bus(struct pci_bus *bus) +{ + struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata; + int ret; + + if (!bus->parent || !pci_is_root_bus(bus->parent)) + return 0; + + ret = pci_subdev_regulators_add_bus(bus); + if (ret) + return ret; + + /* Grab the regulators for suspend/resume */ + pcie->sr = bus->dev.driver_data; + + /* + * If we have failed linkup there is no point to return an error as + * currently it will cause a WARNING() from pci_alloc_child_bus(). + * We return 0 and turn on the "refusal_mode" so that any further + * accesses to the pci_dev just get 0xffffffff + */ + if (brcm_pcie_linkup(pcie) != 0) + pcie->refusal_mode = true; + + return 0; +} + static void pci_subdev_regulators_remove_bus(struct pci_bus *bus) { struct device *dev = &bus->dev; @@ -826,6 +857,18 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn, /* Accesses to the RC go right to the RC registers if slot==0 */ if (pci_is_root_bus(bus)) return PCI_SLOT(devfn) ? NULL : base + where; + if (pcie->refusal_mode) { + /* + * At this point we do not have link. There will be a CPU + * abort -- a quirk with this controller --if Linux tries + * to read any config-space registers besides those + * targeting the host bridge. To prevent this we hijack + * the address to point to a safe access that will return + * 0xffffffff. + */ + writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI); + return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3); + } /* For devices, write to the config space index register */ idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0); @@ -854,7 +897,7 @@ static struct pci_ops brcm_pcie_ops = { .map_bus = brcm_pcie_map_conf, .read = pci_generic_config_read, .write = pci_generic_config_write, - .add_bus = pci_subdev_regulators_add_bus, + .add_bus = brcm_pcie_add_bus, .remove_bus = pci_subdev_regulators_remove_bus, }; @@ -1327,6 +1370,14 @@ static int brcm_pcie_suspend(struct device *dev) return ret; } + if (pcie->sr) { + ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies); + if (ret) { + dev_err(dev, "Could not turn off regulators\n"); + reset_control_reset(pcie->rescal); + return ret; + } + } clk_disable_unprepare(pcie->clk); return 0; @@ -1344,9 +1395,17 @@ static int brcm_pcie_resume(struct device *dev) if (ret) return ret; + if (pcie->sr) { + ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies); + if (ret) { + dev_err(dev, "Could not turn on regulators\n"); + goto err_disable_clk; + } + } + ret = reset_control_reset(pcie->rescal); if (ret) - goto err_disable_clk; + goto err_regulator; ret = brcm_phy_start(pcie); if (ret) @@ -1378,6 +1437,9 @@ static int brcm_pcie_resume(struct device *dev) err_reset: reset_control_rearm(pcie->rescal); +err_regulator: + if (pcie->sr) + regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies); err_disable_clk: clk_disable_unprepare(pcie->clk); return ret; @@ -1488,10 +1550,6 @@ static int brcm_pcie_probe(struct platform_device *pdev) if (ret) goto fail; - ret = brcm_pcie_linkup(pcie); - if (ret) - goto fail; - pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION); if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) { dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n"); @@ -1513,7 +1571,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pcie); - return pci_host_probe(bridge); + ret = pci_host_probe(bridge); + if (!ret && !brcm_pcie_link_up(pcie)) + ret = -ENODEV; + + if (ret) { + brcm_pcie_remove(pdev); + return ret; + } + + return 0; + fail: __brcm_pcie_remove(pcie); return ret; @@ -1522,8 +1590,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) MODULE_DEVICE_TABLE(of, brcm_pcie_match); static const struct dev_pm_ops brcm_pcie_pm_ops = { - .suspend = brcm_pcie_suspend, - .resume = brcm_pcie_resume, + .suspend_noirq = brcm_pcie_suspend, + .resume_noirq = brcm_pcie_resume, }; static struct platform_driver brcm_pcie_driver = {
This Broadcom STB PCIe RC driver has one port and connects directly to one device, be it a switch or an endpoint. We want to be able to leverage the recently added mechanism that allocates and turns on/off subdevice regulators. All that needs to be done is to put the regulator DT nodes in the bridge below host and to set the pci_ops methods add_bus and remove_bus. Note that the pci_subdev_regulators_add_bus() method is wrapped for two reasons: 1. To achieve link up after the voltage regulators are turned on. 2. If, in the case of an unsuccessful link up, to redirect any PCIe accesses to subdevices, e.g. the scan for DEV/ID. This redirection is needed because the Broadcom PCIe HW will issue a CPU abort if such an access is made when the link is down. [bhelgaas: fold in https://lore.kernel.org/r/20220112013100.48029-1-jim2101024@gmail.com] Link: https://lore.kernel.org/r/20220106160332.2143-7-jim2101024@gmail.com Signed-off-by: Jim Quinlan <jim2101024@gmail.com> --- drivers/pci/controller/pcie-brcmstb.c | 86 ++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 9 deletions(-)