linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jim Quinlan <jim2101024@gmail.com>
To: linux-pci@vger.kernel.org,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Cyril Brulebois <kibi@debian.org>,
	bcm-kernel-feedback-list@broadcom.com, jim2101024@gmail.com,
	james.quinlan@broadcom.com
Cc: "Florian Fainelli" <f.fainelli@gmail.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	linux-rpi-kernel@lists.infradead.org (moderated list:BROADCOM
	BCM2711/BCM2835 ARM ARCHITECTURE),
	linux-arm-kernel@lists.infradead.org (moderated list:BROADCOM
	BCM2711/BCM2835 ARM ARCHITECTURE),
	linux-kernel@vger.kernel.org (open list)
Subject: [PATCH v3 5/7] PCI: brcmstb: Add control of subdevice voltage regulators
Date: Mon, 25 Jul 2022 11:12:54 -0400	[thread overview]
Message-ID: <20220725151258.42574-6-jim2101024@gmail.com> (raw)
In-Reply-To: <20220725151258.42574-1-jim2101024@gmail.com>

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.

The pci_subdev_regulators_remove_bus() is wrapped as well as
we must avoid invoking it if we see that there is a collision
in the use of dev->driver_data.

[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 | 134 ++++++++++++++++++--------
 1 file changed, 95 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 1be6e71142d8..aa199b0a39e2 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -261,6 +261,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			regulator_oops;
+	struct subdev_regulators *sr;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1100,10 +1102,37 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 	struct subdev_regulators *sr;
 	int ret;
 
-	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+	sr = alloc_subdev_regulators(dev);
+	if (!sr)
+		return -ENOMEM;
+
+	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators for downstream device\n");
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators for downstream device\n");
+		regulator_bulk_free(sr->num_supplies, sr->supplies);
+		return ret;
+	}
+	dev->driver_data = sr;
+
+	return 0;
+}
+
+static int brcm_pcie_add_bus(struct pci_bus *bus)
+{
+	struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
+	struct device *dev = &bus->dev;
+	int ret;
+
+	if (!bus->parent || !pci_is_root_bus(bus->parent) || !pcie)
 		return 0;
 
-	if (dev->driver_data) {
+	if (dev->of_node && dev->driver_data) {
 		/*
 		 * Oops, this is unfortunate.  We are using the port
 		 * driver's driver_data field to store our regulator info
@@ -1112,12 +1141,15 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 		 * may still be okay if there are no regulators.
 		 */
 		dev_err(dev, "root port dev.driver_data non-NULL; something wrong\n");
-		return 0;
+
+	} else if (dev->of_node) {
+		ret = pci_subdev_regulators_add_bus(bus);
+		/* Grab the regulators for suspend/resume */
+		pcie->sr = bus->dev.driver_data;
 	}
 
-	sr = alloc_subdev_regulators(dev);
-	if (!sr)
-		return -ENOMEM;
+	/* Try to start the link. */
+	brcm_pcie_start_link(pcie);
 
 	/*
 	 * There is not much of a point to return an error as currently it
@@ -1125,22 +1157,7 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 	 * return the error if it is -ENOMEM.  Note that we are always
 	 * doing a dev_err() for other erros.
 	 */
-	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
-	if (ret) {
-		dev_err(dev, "failed to get regulators for downstream device\n");
-		return 0;
-	}
-
-	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
-	if (ret) {
-		dev_err(dev, "failed to enable regulators for downstream device\n");
-		regulator_bulk_free(sr->num_supplies, sr->supplies);
-		return 0;
-	}
-
-	dev->driver_data = sr;
-
-	return 0;
+	return ret == -ENOMEM ? ret : 0;
 }
 
 static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
@@ -1148,15 +1165,28 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
 	struct device *dev = &bus->dev;
 	struct subdev_regulators *sr = dev->driver_data;
 
-	if (!dev->of_node || !sr || !bus->parent || !pci_is_root_bus(bus->parent))
-		return;
-
 	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
 		dev_err(dev, "failed to disable regulators for downstream device\n");
 	regulator_bulk_free(sr->num_supplies, sr->supplies);
 	dev->driver_data = NULL;
 }
 
+static void brcm_pcie_remove_bus(struct pci_bus *bus)
+{
+	struct device *dev = &bus->dev;
+	struct brcm_pcie *pcie;
+
+	if (!dev->of_node || !dev->driver_data || !bus->parent ||
+	    !pci_is_root_bus(bus->parent))
+		return;
+
+	pcie = (struct brcm_pcie *) bus->sysdata;
+	if (pcie && pcie->sr) {
+		pci_subdev_regulators_remove_bus(bus);
+		pcie->sr = NULL;
+	}
+}
+
 /* L23 is a low-power PCIe link state */
 static void brcm_pcie_enter_l23(struct brcm_pcie *pcie)
 {
@@ -1253,7 +1283,7 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
-static int brcm_pcie_suspend(struct device *dev)
+static int brcm_pcie_suspend_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	int ret;
@@ -1273,12 +1303,20 @@ 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;
 }
 
-static int brcm_pcie_resume(struct device *dev)
+static int brcm_pcie_resume_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	void __iomem *base;
@@ -1313,15 +1351,27 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	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_reset;
+		}
+	}
+
 	ret = brcm_pcie_start_link(pcie);
 	if (ret)
-		goto err_reset;
+		goto err_regulator;
 
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
 	return 0;
 
+err_regulator:
+	if (pcie->sr)
+		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_reset:
 	reset_control_rearm(pcie->rescal);
 err_disable_clk:
@@ -1428,16 +1478,16 @@ 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,
-	.remove_bus = pci_subdev_regulators_remove_bus,
+	.add_bus = brcm_pcie_add_bus,
+	.remove_bus = brcm_pcie_remove_bus
 };
 
 static struct pci_ops brcm_pcie_ops32 = {
 	.map_bus = brcm_pcie_map_conf32,
 	.read = pci_generic_config_read32,
 	.write = pci_generic_config_write32,
-	.add_bus = pci_subdev_regulators_add_bus,
-	.remove_bus = pci_subdev_regulators_remove_bus,
+	.add_bus = brcm_pcie_add_bus,
+	.remove_bus = brcm_pcie_remove_bus
 };
 
 static int brcm_pcie_probe(struct platform_device *pdev)
@@ -1510,10 +1560,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
-	ret = brcm_pcie_start_link(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");
@@ -1535,7 +1581,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;
@@ -1544,8 +1600,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_noirq,
+	.resume_noirq = brcm_pcie_resume_noirq,
 };
 
 static struct platform_driver brcm_pcie_driver = {
-- 
2.17.1


  parent reply	other threads:[~2022-07-25 15:16 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-25 15:12 [PATCH v3 0/7] PCI: brcmstb: Re-submit reverted patchset Jim Quinlan
2022-07-25 15:12 ` [PATCH v3 1/7] PCI: brcmstb: Remove unnecessary forward declarations Jim Quinlan
2022-07-25 15:12 ` [PATCH v3 2/7] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Jim Quinlan
2022-07-25 15:12 ` [PATCH v3 3/7] PCI: brcmstb: Gate config space access on link status Jim Quinlan
2022-07-25 15:12 ` [PATCH v3 4/7] PCI: brcmstb: Add mechanism to turn on subdev regulators Jim Quinlan
2022-07-25 15:12 ` Jim Quinlan [this message]
2022-07-25 15:12 ` [PATCH v3 6/7] PCI: brcmstb: Do not turn off WOL regulators on suspend Jim Quinlan
2022-07-25 15:12 ` [PATCH v3 7/7] PCI: brcmstb: Have .map_bus function names end with 'map_bus' Jim Quinlan
2022-07-26 22:03 ` [PATCH v3 0/7] PCI: brcmstb: Re-submit reverted patchset Bjorn Helgaas
2022-07-26 23:41   ` Florian Fainelli
2022-08-01 22:19     ` Bjorn Helgaas
2022-08-04 17:05       ` Jim Quinlan
2022-08-04 17:27         ` Bjorn Helgaas
2022-07-27  1:01   ` Jim Quinlan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220725151258.42574-6-jim2101024@gmail.com \
    --to=jim2101024@gmail.com \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=bhelgaas@google.com \
    --cc=f.fainelli@gmail.com \
    --cc=james.quinlan@broadcom.com \
    --cc=kibi@debian.org \
    --cc=kw@linux.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=lpieralisi@kernel.org \
    --cc=nsaenz@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).