Re: [PATCH v3 0/7] PCI: brcmstb: Re-submit reverted patchset

[Florian Fainelli <f.fainelli@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]

On 7/26/22 15:03, Bjorn Helgaas wrote:
> On Mon, Jul 25, 2022 at 11:12:49AM -0400, Jim Quinlan wrote:
>> ...
>> Jim Quinlan (7):
>>   PCI: brcmstb: Remove unnecessary forward declarations
>>   PCI: brcmstb: Split brcm_pcie_setup() into two funcs
>>   PCI: brcmstb: Gate config space access on link status
>>   PCI: brcmstb: Add mechanism to turn on subdev regulators
>>   PCI: brcmstb: Add control of subdevice voltage regulators
>>   PCI: brcmstb: Do not turn off WOL regulators on suspend
>>   PCI: brcmstb: Have .map_bus function names end with 'map_bus'
>>
>>  drivers/pci/controller/pcie-brcmstb.c | 476 ++++++++++++++++++--------
>>  1 file changed, 341 insertions(+), 135 deletions(-)
> 
> I reworked these and put them on pci/ctrl/brcm for v5.20.  This is a
> proposal, not something set in stone.  But time is of the essence to
> figure out how we want to proceed.
> 
> I changed a lot of stuff and it's likely I broke something in the
> process, so please take a look and test this out.  Here's an outline
> of what I changed:
> 
>   - Moved the config access "link up" check earlier because it's not
>     related to the power regulator patches.
> 
>   - Changed config access "link up" checks to use PCIE_ECAM_REG()
>     instead of hard-coding 0xfff masks.  The 32-bit accessors already
>     mask out the low two bits, so we don't need to do that here.
> 
>   - Squashed pci_subdev_regulators_add_bus() directly into
>     brcm_pcie_add_bus() for readability.  Similarly for
>     pci_subdev_regulators_remove_bus().
> 
>   - This makes a clear split between:
> 
>     * A patch that adds get/enable of regulators, and starting the
>       link after enabling regulators, and
> 
>     * A patch that disables/enables regulators for suspend/resume.
> 
>   - Since we only support one set of subregulator info (for one Root
>     Port, and brcm_pcie_suspend_noirq() depends on this since it uses
>     the pcie->sr pointer), use pcie->sr always instead of
>     dev->driver_data.
> 
>   - Squashed wakeup device checking into the suspend/resume patch so
>     there's not a time when suspend might turn off power to a wakeup
>     device.
> 
>   - Renamed brcm_pcie_map_bus32() to brcm7425_pcie_map_bus() so it
>     ends in "_map_bus()" like other drivers.  Also,
>     brcm7425_pcie_map_bus() doesn't actually depend on the 32-bitness.

Attached is the diff between Jim's and your branch just so it is easier to see what moved around.

Initial testing on an ARCH_BRCMSTB system with PCIe appears to be good, we don't have any regulator on that board so the dummy ones get picked up which is expected. Same thing with a Raspberry Pi 4B system.

I could unbind and bind again and there were no reference count leaks on the regulators, so this looks good to me.

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

of course, we should have Jim's test results as well as Cyril's ideally to make sure there are no regressions on the CM4 board.

Thanks Bjorn!
-- 
Florian

[-- Attachment #2: jq-vs-bh.diff --]
[-- Type: text/x-patch, Size: 8082 bytes --]

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index b40733dd253c..521acd632f1a 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -261,7 +261,6 @@ 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;
 	bool			ep_wakeup_capable;
 };
@@ -685,8 +684,8 @@ static bool brcm_pcie_link_up(struct brcm_pcie *pcie)
 	return dla && plu;
 }
 
-static void __iomem *brcm_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
-					int where)
+static void __iomem *brcm_pcie_map_bus(struct pci_bus *bus,
+				       unsigned int devfn, int where)
 {
 	struct brcm_pcie *pcie = bus->sysdata;
 	void __iomem *base = pcie->base;
@@ -694,7 +693,7 @@ static void __iomem *brcm_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 
 	/* Accesses to the RC go right to the RC registers if !devfn */
 	if (pci_is_root_bus(bus))
-		return devfn ? NULL : base + (where & 0xfff);
+		return devfn ? NULL : base + PCIE_ECAM_REG(where);
 
 	/* An access to our HW w/o link-up will cause a CPU Abort */
 	if (!brcm_pcie_link_up(pcie))
@@ -703,11 +702,11 @@ static void __iomem *brcm_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	/* For devices, write to the config space index register */
 	idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
 	writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
-	return base + PCIE_EXT_CFG_DATA + (where & 0xfff);
+	return base + PCIE_EXT_CFG_DATA + PCIE_ECAM_REG(where);
 }
 
-static void __iomem *brcm_pcie_map_bus32(struct pci_bus *bus, unsigned int devfn,
-					 int where)
+static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
+					   unsigned int devfn, int where)
 {
 	struct brcm_pcie *pcie = bus->sysdata;
 	void __iomem *base = pcie->base;
@@ -715,14 +714,14 @@ static void __iomem *brcm_pcie_map_bus32(struct pci_bus *bus, unsigned int devfn
 
 	/* Accesses to the RC go right to the RC registers if !devfn */
 	if (pci_is_root_bus(bus))
-		return devfn ? NULL : base + (where & 0xffc);
+		return devfn ? NULL : base + PCIE_ECAM_REG(where);
 
 	/* An access to our HW w/o link-up will cause a CPU Abort */
 	if (!brcm_pcie_link_up(pcie))
 		return NULL;
 
 	/* For devices, write to the config space index register */
-	idx = PCIE_ECAM_OFFSET(bus->number, devfn, (where & 0xffc));
+	idx = PCIE_ECAM_OFFSET(bus->number, devfn, where);
 	writel(idx, base + IDX_ADDR(pcie));
 	return base + DATA_ADDR(pcie);
 }
@@ -1082,8 +1081,8 @@ static const char * const supplies[] = {
 
 static void *alloc_subdev_regulators(struct device *dev)
 {
-	const size_t size = sizeof(struct subdev_regulators)
-		+ sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
+	const size_t size = sizeof(struct subdev_regulators) +
+		sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
 	struct subdev_regulators *sr;
 	int i;
 
@@ -1097,95 +1096,57 @@ static void *alloc_subdev_regulators(struct device *dev)
 	return sr;
 }
 
-static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
+static int brcm_pcie_add_bus(struct pci_bus *bus)
 {
+	struct brcm_pcie *pcie = bus->sysdata;
 	struct device *dev = &bus->dev;
 	struct subdev_regulators *sr;
 	int ret;
 
-	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;
-}
+	if (!bus->parent || !pci_is_root_bus(bus->parent))
+		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 (dev->of_node) {
+		sr = alloc_subdev_regulators(dev);
+		if (!sr) {
+			dev_info(dev, "Can't allocate regulators for downstream device\n");
+			goto no_regulators;
+		}
 
-	if (!bus->parent || !pci_is_root_bus(bus->parent) || !pcie)
-		return 0;
+		pcie->sr = sr;
 
-	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
-		 * and it appears that another driver started using it as
-		 * well.  If so, be a team player do not overwrite it.  We
-		 * may still be okay if there are no regulators.
-		 */
-		dev_err(dev, "root port dev.driver_data non-NULL; something wrong\n");
+		ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+		if (ret) {
+			dev_info(dev, "No regulators for downstream device\n");
+			goto no_regulators;
+		}
 
-	} else if (dev->of_node) {
-		ret = pci_subdev_regulators_add_bus(bus);
-		/* Grab the regulators for suspend/resume */
-		pcie->sr = bus->dev.driver_data;
+		ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+		if (ret) {
+			dev_err(dev, "Can't enable regulators for downstream device\n");
+			regulator_bulk_free(sr->num_supplies, sr->supplies);
+			pcie->sr = NULL;
+		}
 	}
 
-	/* Try to start the link. */
+no_regulators:
 	brcm_pcie_start_link(pcie);
-
-	/*
-	 * There is not much of a point to return an error as currently it
-	 * will cause a WARNING() from pci_alloc_child_bus().  So only
-	 * return the error if it is -ENOMEM.  Note that we are always
-	 * doing a dev_err() for other erros.
-	 */
-	return ret == -ENOMEM ? ret : 0;
-}
-
-static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
-{
-	struct device *dev = &bus->dev;
-	struct subdev_regulators *sr = dev->driver_data;
-
-	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;
+	return 0;
 }
 
 static void brcm_pcie_remove_bus(struct pci_bus *bus)
 {
+	struct brcm_pcie *pcie = bus->sysdata;
+	struct subdev_regulators *sr = pcie->sr;
 	struct device *dev = &bus->dev;
-	struct brcm_pcie *pcie;
 
-	if (!dev->of_node || !dev->driver_data || !bus->parent ||
-	    !pci_is_root_bus(bus->parent))
+	if (!sr)
 		return;
 
-	pcie = (struct brcm_pcie *) bus->sysdata;
-	if (pcie && pcie->sr) {
-		pci_subdev_regulators_remove_bus(bus);
-		pcie->sr = NULL;
-	}
+	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);
+	pcie->sr = NULL;
 }
 
 /* L23 is a low-power PCIe link state */
@@ -1290,7 +1251,7 @@ static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 
 	if (device_may_wakeup(&dev->dev)) {
 		*ret = true;
-		dev_info(&dev->dev, "disable cancelled for wake-up device\n");
+		dev_info(&dev->dev, "Possible wake-up device; regulators will not be disabled\n");
 	}
 	return (int) *ret;
 }
@@ -1513,15 +1474,15 @@ static struct pci_ops brcm_pcie_ops = {
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
 	.add_bus = brcm_pcie_add_bus,
-	.remove_bus = brcm_pcie_remove_bus
+	.remove_bus = brcm_pcie_remove_bus,
 };
 
-static struct pci_ops brcm_pcie_ops32 = {
-	.map_bus = brcm_pcie_map_bus32,
+static struct pci_ops brcm7425_pcie_ops = {
+	.map_bus = brcm7425_pcie_map_bus,
 	.read = pci_generic_config_read32,
 	.write = pci_generic_config_write32,
 	.add_bus = brcm_pcie_add_bus,
-	.remove_bus = brcm_pcie_remove_bus
+	.remove_bus = brcm_pcie_remove_bus,
 };
 
 static int brcm_pcie_probe(struct platform_device *pdev)
@@ -1610,7 +1571,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	bridge->ops = pcie->type == BCM7425 ? &brcm_pcie_ops32 : &brcm_pcie_ops;
+	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
 	bridge->sysdata = pcie;
 
 	platform_set_drvdata(pdev, pcie);

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel