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
