Messages in this thread Patch in this message | | | Date | Tue, 26 Jul 2022 16:41:09 -0700 | Subject | Re: [PATCH v3 0/7] PCI: brcmstb: Re-submit reverted patchset | From | Florian Fainelli <> |
| |
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! -- 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); | |