Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Bjorn Helgaas]

On Fri, Jul 08, 2022 at 04:38:30PM -0400, Jim Quinlan wrote:
> On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> > > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > > > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > > > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > > > invoking it directly.
> > > > > > >
> > > > > > > Some code that was executed after the PCIe linkup is now placed so that it
> > > > > > > executes prior to linkup, since this code has to run prior to the
> > > > > > > invocation of pci_host_probe().
> > > > > >
> > > > > > This says we need to move some code from brcm_pcie_setup() to
> > > > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > > > I will elaborate in the commit message.
> > > > > >
> > > > > > In brcm_pcie_resume(), they're called together:
> > > > > >
> > > > > >   brcm_pcie_resume
> > > > > >     brcm_pcie_setup
> > > > > >     brcm_pcie_linkup
> > > > > >
> > > > > > In the probe path, they're not called together, but they're in the
> > > > > > same order:
> > > > > >
> > > > > >   brcm_pcie_probe
> > > > > >     brcm_pcie_setup
> > > > > >     pci_host_probe
> > > > > >       ...
> > > > > >         brcm_pcie_add_bus               # bus->ops->add_bus
> > > > > >           brcm_pcie_linkup
> > > > > >
> > > > > > Is there something that must happen *between* them in the probe path?
> > > > >
> > > > > Yes.  In the probe() case, we must do things in this order:
> > > > >
> > > > > 1. brcm_pcie_setup()
> > > > > 2. Turn on regulators
> > > > > 3. brcm_pcie_linkup()
> > > >
> > > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> > > >
> > > >   brcm_pcie_add_bus                    # bus->ops->add_bus
> > > >     pci_subdev_regulators_add_bus
> > > >       regulator_bulk_enable            # turn on regulators
> > > >     brcm_pcie_linkup
> > > >
> > > > > Since the voltage regulators are turned on during enumeration,
> > > > > pci_host_probe() must be invoked prior to 3.  Before regulators, we
> > > > > did not care.
> > > >
> > > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > > > for downstream devices must always be on.
> > > >
> > > > > In the resume case, there is no enumeration of course but our driver
> > > > > has a handle to the regulators and can turn them on/off w/o help.
> > > >
> > > > And I guess we don't need brcm_pcie_setup() in the resume path because
> > > > suspend turns off power only for downstream devices, not for the root
> > > > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > > > to be done again.
> > >
> > > I'm not sure I understand what you are saying -- brcm_pcie_setup()  is
> > > called by brcm_pcie_resume()
> > > because it is needed.  brcm_pcie_setup() isn't concerned with power it
> > >  just does the preparation
> > > required before attempting link-up.
> >
> > Oh, sorry, I totally misread that.
> >
> > But I wonder about the fact that probe and resume do these in
> > different orders:
> >
> >   brcm_pcie_probe
> >     brcm_pcie_setup                          # setup
> >     pci_host_probe
> >       ...
> >         brcm_pcie_add_bus
> >           pci_subdev_regulators_add_bus
> >             regulator_bulk_enable            # regulators on
> >           brcm_pcie_linkup                   # linkup
> >
> >   brcm_pcie_resume
> >     regulator_bulk_enable                    # regulators on
> >     brcm_pcie_setup                          # setup
> >     brcm_pcie_linkup                         # linkup
> >
> brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(),
> but your point is valid -- it is prudent to keep the orders
> consistent. Let me think
> about this.
> 
> > Maybe pci_subdev_regulators_add_bus() could be done directly from
> > brcm_pcie_probe() instead of in brcm_pcie_add_bus()?
> > regulators must be directly under the root port node in DT, it seems
> > like it would be reasonable to look for them in the probe path, which
> > seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
> > pcie-rockchip-host.c do.
> At some point in the original patchset -- IIRC -- the RC driver was
> searching the DT
> tree for regulators.  However, doing a "get" on these regulators is pretty much
> impossible if the "owning" device does not exist.  I even had a version that
> partially created the downstream device;  this pullreq was a mess and
> got feedback which  put me on the current approach.

Ah, I suppose because the regulators are not under the host bridge
itself, but under the *root port*, which is a PCI device that doesn't
exist until we enumerate it.  Although I guess the root port is
described in the DT, and the regulators are connected with that DT
description, not directly with the pci_dev.

> Reviews suggested  that the best location for the regulators should be located
> in the root port DT node(s).  I agree with this. In addition, there
> was a request to allow multiple regulators
> to exist at each of the root ports in the downstream tree.

Makes sense.

> So if the RC driver
> has to  potentially add multiple buses.  I really don't know how it
> would do that,
> and then call the pci_host_probe() w/o it failing.  Perhaps this is what ACPI
> does before boot  -- I'm guessing here -- but I would also guess that it is
> a decent amount of code as it is not far from doing enumeration.
> 
> One thing I could do is to allow the port driver's suspend/resume to do the
> turning off/on of the regulators.  There are two issues with this: (1)
> feedback suggested
> to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable"
> code would also have to live in the port driver and I'm not sure this
> would be welcome.
> 
> > Or maybe brcm_pcie_resume() should enable the regulators after
> > brcm_pcie_setup() so it's the same order as the probe path?
> I  think I'll do this.

Yep, sounds like the right thing.

Bjorn