| Message ID | 20220511201856.808690-2-helgaas@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcmstb: Revert subdevice regulator stuff | expand |
On 11.05.22 22:18, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > This reverts commit 11ed8b8624b8085f706864b4addcd304b1e4fc38. > > This is part of a revert of the following commits: > > 11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend") > 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators") > 67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators") > 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs") > > Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() > into two funcs"), which appeared in v5.17-rc1, broke booting on the > Raspberry Pi Compute Module 4. Apparently 830aa6f29f07 panics with an > Asynchronous SError Interrupt, and after further commits here is a black > screen on HDMI and no output on the serial console. > > This does not seem to affect the Raspberry Pi 4 B. > > Reported-by: Cyril Brulebois <kibi@debian.org> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925 A "Bugzilla" tag? Why don't you just use a proper "Link:" tag, as explained by the documentation to use in this case (I clarified the docs recently with regards to this). Such inventions (some people use "References:", others "BugLink:" and there were a few others I already forget about) make my regression tracking efforts hard. :-/ Side note: Linus in the past few days two times reminded people to use proper Link: tags, but that was in a totally different context (when there was no link at all or just a Link: pointing to the submission of a patch) > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > [...] Ciao, Thorsten
On Thu, May 12, 2022 at 08:24:00AM +0200, Thorsten Leemhuis wrote: > On 11.05.22 22:18, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > This reverts commit 11ed8b8624b8085f706864b4addcd304b1e4fc38. > > > > This is part of a revert of the following commits: > > > > 11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend") > > 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators") > > 67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators") > > 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs") > > > > Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() > > into two funcs"), which appeared in v5.17-rc1, broke booting on the > > Raspberry Pi Compute Module 4. Apparently 830aa6f29f07 panics with an > > Asynchronous SError Interrupt, and after further commits here is a black > > screen on HDMI and no output on the serial console. > > > > This does not seem to affect the Raspberry Pi 4 B. > > > > Reported-by: Cyril Brulebois <kibi@debian.org> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925 > > A "Bugzilla" tag? Why don't you just use a proper "Link:" tag, as > explained by the documentation to use in this case (I clarified the docs > recently with regards to this). Such inventions (some people use > "References:", others "BugLink:" and there were a few others I already > forget about) make my regression tracking efforts hard. :-/ In this case, I actually looked through the history to try to figure out the most common way to cite bugzilla but didn't see much uniformity. Anyway, I updated these to "Link:".
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 375c0c40bbf8..3edd63735948 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -333,7 +333,6 @@ struct brcm_pcie { void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); bool refusal_mode; struct subdev_regulators *sr; - bool ep_wakeup_capable; }; static inline bool is_bmips(const struct brcm_pcie *pcie) @@ -1351,21 +1350,9 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) pcie->bridge_sw_init_set(pcie, 1); } -static int pci_dev_may_wakeup(struct pci_dev *dev, void *data) -{ - bool *ret = data; - - if (device_may_wakeup(&dev->dev)) { - *ret = true; - dev_info(&dev->dev, "disable cancelled for wake-up device\n"); - } - return (int) *ret; -} - static int brcm_pcie_suspend(struct device *dev) { struct brcm_pcie *pcie = dev_get_drvdata(dev); - struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); int ret; brcm_pcie_turn_off(pcie); @@ -1384,22 +1371,11 @@ static int brcm_pcie_suspend(struct device *dev) } if (pcie->sr) { - /* - * Now turn off the regulators, but if at least one - * downstream device is enabled as a wake-up source, do not - * turn off regulators. - */ - pcie->ep_wakeup_capable = false; - pci_walk_bus(bridge->bus, pci_dev_may_wakeup, - &pcie->ep_wakeup_capable); - if (!pcie->ep_wakeup_capable) { - 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; - } + 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); @@ -1420,21 +1396,10 @@ static int brcm_pcie_resume(struct device *dev) return ret; if (pcie->sr) { - if (pcie->ep_wakeup_capable) { - /* - * We are resuming from a suspend. In the suspend we - * did not disable the power supplies, so there is - * no need to enable them (and falsely increase their - * usage count). - */ - pcie->ep_wakeup_capable = false; - } else { - 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 = 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; } }