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

[Jim Quinlan]

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.

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

Thanks,
Jim Quinlan
Broadcom STB
>
> Bjorn

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature