Re: [PATCH v3 0/7] PCI: brcmstb: Re-submit reverted patchset
From: Florian Fainelli
Date: Tue Jul 26 2022 - 19:41:19 EST
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@xxxxxxxxx>
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!
--
Floriandiff --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);