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
