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

[Bjorn Helgaas]

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@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.

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