| Message ID | 20220725151258.42574-1-jim2101024@gmail.com |
|---|---|
| Headers | show |
| Series | PCI: brcmstb: Re-submit reverted patchset | expand |
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. Bjorn
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!
On Tue, Jul 26, 2022 at 6:03 PM Bjorn Helgaas <helgaas@kernel.org> 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. Hi Bjorn, Sorry, it seems that some of my future commits have somehow bled code into the driver (e.g. "has_phy"). LGTM. Thanks and regards, Jim Quinlan Broadcom STB > > Bjorn
On Tue, Jul 26, 2022 at 04:41:09PM -0700, Florian Fainelli wrote: > 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. Cyril, any chance you could test this to be sure it fixes the problem you reported? This is in -next and hopefully headed for v5.20/v6.0 soon. I see that we failed to reference https://bugzilla.kernel.org/show_bug.cgi?id=215925 in the commit logs, but IIUC, this *should* fix that. Bjorn
On Mon, Aug 1, 2022 at 6:19 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jul 26, 2022 at 04:41:09PM -0700, Florian Fainelli wrote: > > 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. > > Cyril, any chance you could test this to be sure it fixes the problem > you reported? This is in -next and hopefully headed for v5.20/v6.0 > soon. Hello Bjorn, Cyril sent me an email about a week ago saying that he probably wouldn't have the bandwidth to test this. I immediately ordered an overpriced CM4 via Ebay and it recently arrived. I'm happy to say that this patchset tests successfully, w/ or w/o a device in the slot. That being said, there is an old device, when paired with the CM4, works with RPi Linux but not with upstream Linux. It is unrelated to this patchset; i.e. it fails w/ or w/o this patchset applied. I know the reason for this failure: the current driver assumes clkreq# asserted, which is true for all STB boards. I can add a patch for this now or in the next release cycle, your choice. Regards, Jim Quinlan Broadcom STB > > I see that we failed to reference > https://bugzilla.kernel.org/show_bug.cgi?id=215925 in the commit logs, > but IIUC, this *should* fix that. > > Bjorn
On Thu, Aug 04, 2022 at 01:05:04PM -0400, Jim Quinlan wrote: > On Mon, Aug 1, 2022 at 6:19 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jul 26, 2022 at 04:41:09PM -0700, Florian Fainelli wrote: > > > 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. > > > > Cyril, any chance you could test this to be sure it fixes the problem > > you reported? This is in -next and hopefully headed for v5.20/v6.0 > > soon. > > Cyril sent me an email about a week ago saying that he probably > wouldn't have the bandwidth to test this. > I immediately ordered an overpriced CM4 via Ebay and it recently > arrived. I'm happy to say that this > patchset tests successfully, w/ or w/o a device in the slot. Great, thanks a lot for testing this! > That being said, there is an old device, when paired with the CM4, > works with RPi Linux but not with upstream Linux. It is unrelated > to this patchset; i.e. it fails w/ or w/o this patchset applied. I > know the reason for this failure: the current driver > assumes clkreq# asserted, which is true for all STB boards. I can add > a patch for this now or in the next release > cycle, your choice. It's too late for v5.20-rc1, but if this would fix a regression or otherwise exceptional bug, Lorenzo might still consider it for the v5.20. Bjorn
v3 -- Replaced "refusal mode" commit with one that gates config space accesses based on link status (Bjorn) -- Restrict the brcm_pcie_start_link() code to only those things related to link-up and put the rest in brcm_pcie_setup() -- Added commit to change map functions to end with 'map_bus' (Bjorn) -- s/0x060400/PCI_CLASS_BRIDGE_PCI_NORMAL/ (Pali) -- Limit config space accesess to 4k window. -- Remove wording in a commit message that said the regulators could be endpoint power supplies V2 -- As v1 included the minimal code to fix a regression. v2 does the same but adds some improvements suggested by Bjorn. -- In the unlikely but possible case that some other driver starts using the port driver's dev_data field, do not overwrite it but issue an error and return. -- Functions probe() and resume() do similar operations but did them in a different order; make this order consistent for both. -- New commit to remove forward declarations. -- New commit for only the PCIe config-space access "refusal mode". -- brcm_pcie_linkup renamed to brcm_pcie_start_link -- Add '_noirq' to the brcm_pcie_{suspend,resume} function names to match the dev_pm_ops names. -- Changes to commit messages: o Explain why we are splitting a function in two parts. o s/RB/Root Port/ NOTE for Bjorn: The two commits "add mechanism .." and "add control ..." would probably be more clear if they were squashed. They are kept separate as the first one's code may someday be moved under the Port driver. As such, there's some churn. NOTE for Bjorn: There is no hurry on Broadcom's side wrt which release cycle/phase this patchset is applied. It goes in when you think it is ready. V1 -- Resubmission of patchset after original was reverted for a regression. A submission [1] was made to enable a PCIe root port to turn on regulators for downstream devices. It was accepted. Months later, a regression was discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix came too late in the release cycle. The regression in question is triggered only when the PCIe RC DT node has no root port subnode, which is a perfectly reasonsable configuration. The original commits are now being resubmitted with some modifications to fix the regression. The modifcations on the original commits are described below (the SHA is that of the original commit): [830aa6f29f07 PCI: brcmstb: Split brcm_pcie_setup() into two funcs] NOTE: In the originally submitted patchset, this commit introduced a regression that was corrected by a subsequent commit in the same patchset. Let's not do this again. @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) if (ret) goto fail; + ret = brcm_pcie_linkup(pcie); + if (ret) + goto fail; [67211aadcb4b PCI: brcmstb: Add mechanism to turn on subdev regulators] NOTE: Not related to the regression, the regulators must be freed whenever the PCIe tree is dismantled: @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus) 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; [93e41f3fca3d PCI: brcmstb: Add control of subdevice voltage regulators] NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe link-up was skipped. This is the regression. Fix it by attempting link-up even if the Root Port DT subnode is missing. @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus) static int brcm_pcie_add_bus(struct pci_bus *bus) { - struct device *dev = &bus->dev; struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata; int ret; - if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent)) + if (!bus->parent || !pci_is_root_bus(bus->parent)) return 0; ret = pci_subdev_regulators_add_bus(bus); [1] https://lore.kernel.org/r/20220106160332.2143-1-jim2101024@gmail.com [2] https://bugzilla.kernel.org/show_bug.cgi?id=215925 [3] https://lore.kernel.org/linux-pci/20220511201856.808690-1-helgaas@kernel.org/ 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(-) base-commit: e0dccc3b76fb35bb257b4118367a883073d7390e