diff mbox series

[1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend"

Message ID 20220511201856.808690-2-helgaas@kernel.org
State New
Headers show
Series PCI: brcmstb: Revert subdevice regulator stuff | expand

Commit Message

Bjorn Helgaas May 11, 2022, 8:18 p.m. UTC
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
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 53 +++++----------------------
 1 file changed, 9 insertions(+), 44 deletions(-)

Comments

Linux regression tracking (Thorsten Leemhuis) May 12, 2022, 6:24 a.m. UTC | #1
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
Bjorn Helgaas May 12, 2022, 12:45 p.m. UTC | #2
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 mbox series

Patch

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;
 		}
 	}