On 7/20/22 09:18, Rob Herring wrote: > On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <james.quinlan@xxxxxxxxxxxx> wrote: >> >> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: >>> >>> On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote: >>>> On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: >>>>> On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote: >>>>>> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: >>>>>>> On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote: >>>>>>>> Currently, the function does the setup for establishing PCIe link-up >>>>>>>> with the downstream device, and it does the actual link-up as well. >>>>>>>> The calling sequence is (roughly) the following in the probe: >>>>>>>> >>>>>>>> -> brcm_pcie_probe() >>>>>>>> -> brcm_pcie_setup(); /* Set-up and link-up */ >>>>>>>> -> pci_host_probe(bridge); >>>>>>>> >>>>>>>> This commit splits the setup function in two: brcm_pcie_setup(), which only >>>>>>>> does the set-up, and brcm_pcie_start_link(), which only does the link-up. >>>>>>>> The reason why we are doing this is to lay a foundation for subsequent >>>>>>>> commits so that we can turn on any power regulators, as described in the >>>>>>>> root port's DT node, prior to doing link-up. >>>>>>> >>>>>>> All drivers that care about power regulators turn them on before >>>>>>> link-up, but typically those regulators are described directly under >>>>>>> the host bridge itself. >>>>>> >>>>>> Actually, what you describe is what I proposed with my v1 back in Nov 2020. >>>>>> The binding commit message said, >>>>>> >>>>>> "Quite similar to the regulator bindings found in >>>>>> "rockchip-pcie-host.txt", this allows optional regulators to be >>>>>> attached and controlled by the PCIe RC driver." >>>>>> >>>>>>> IIUC the difference here is that you have regulators described under >>>>>>> Root Ports (not the host bridge/Root Complex itself), so you don't >>>>>>> know about them until you've enumerated the Root Ports. >>>>>>> brcm_pcie_probe() can't turn them on directly because it doesn't know >>>>>>> what Root Ports are present and doesn't know about regulators below >>>>>>> them. >>>>>> >>>>>> The reviewer's requested me to move the regulator node(s) >>>>>> elsewhere, and at some point later it was requested to be placed >>>>>> under the Root Port driver. I would love to return them under the >>>>>> host bridge, just say the word! >>>>> >>>>> Actually, I think my understanding is wrong. Even though the PCI core >>>>> hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows >>>>> about it and should be able to look up the regulators and turn them >>>>> on. >>>> >>>> One can do this with >>>> regulator_bulk_get(NULL, ...); >>>> >>>> However, MarkB did not like the idea of a driver getting the >>>> regulator from the global DT namespace [1]. >>>> >>>> For the RC driver, one cannot invoke regulator_bulk_get(dev, ...) >>>> if there is not a direct child regulator node. One needs to use the >>>> Port driver device. The Port driver device does not exist at this >>>> point unless one tries to prematurely create it; I tried this and it >>>> was a mess to say the least. >>>> >>>>> Can you dig up the previous discussion about why the regulators need >>>>> to be under the Root Port and why they can't be turned on before >>>>> calling pci_host_probe()? >>>> >>>> RobH did not want the regulators to be under the RC as he said their >>>> DT location should resemble the HW [2]. The consensus evolved to >>>> place it under the port driver, which can provide a general >>>> mechanism for turning on regulators anywhere in the PCIe tree. >>> >>> I don't want to redesign this whole thing. I just want a crisp >>> rationale for the commit log. >>> >>> All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194, >>> tegra, rockchip-host) have regulators for downstream PCIe power >>> directly under the RC. If putting the regulators under an RP instead >>> is the direction of the future, I guess that might be OK, and I guess >>> the reasons are: >>> >>> 1) Slot or device power regulators that are logically below the RP >>> should be described that way in the DT. >>> >>> 2) Associating regulators with a RP allows the possibility of >>> selectively controlling power to slots/devices below the RP, >>> e.g., to power down devices below RP A when suspending while >>> leaving wakeup devices below RP B powered up. >>> >>> I think in your case the motivating reason is 2). >>> >>> Your commit log for "Add mechanism to turn on subdev regulators" >>> suggests that you want some user control of endpoint power, e.g., via >>> sysfs, but I don't see that implemented yet except possibly via a >>> "remove" file that would unbind the driver and remove the entire >>> device. >> Hi Bjorn, >> >> Initially we wanted to (a) turn on any regulator found under the RC >> node and (b) allow the possibility of the regulator to control the >> EP's power. From the feedback, we realized early on that neither of >> these were going to fly, so we conceded both requests and just wanted >> to turn on standard PCIe regulators. Upon reading the aforementioned >> commit message I realize that there are a couple of leftover sentences >> from these early days that must be removed. >> >> I think when I submitted v1 of the original series that only the >> rockchip driver had regulators under the RC. And my recollection was >> that this was accepted but there was apprehension of this turning into >> the "standard" way of turning on such regulators, as the location of >> the regulator nodes was in question. >> >> In short, I would be quite content to follow the existing examples. > > The existing examples are, in general, incomplete and only work for > the simplest cases. > > There's 2 cases to consider here. The first is standard slots with > standard PCIe signals (e.g. PERST#) and voltage rails. The 2nd is > either non-standard slots or just soldered down devices which could > have any number of device specific resources. In the latter case, > those resources need to go into the node for the device. For the > former case (which we are discussing here), putting the resources in > the upstream (side of the link) node is fine. That's the root port > node(s) or switch port nodes. However, since most host bridges are a > single RP and don't put the RP node in DT, we've ended up with these > properties in host bridge nodes. That's fine as long as it's a single > RP and device. When it is not, we need to do something different. The > only way this scales is putting resources in the port nodes as those > are what have a 1:1 relationship to slots. If that's supported, then > the simple cases are also easily supported with if the resources are > not found in the port node/device, then look for them in the parent > node. That's also the path for how we get the handling of PERST out of > every host bridge driver. This has me confused now, are you suggesting that we pursue what Jim has put together here as a series which describes the regulators in the PCIe end-point device DT node, or that given that we have a single RC single RP configuration it is somewhat acceptable to describe regulators in the PCIe RC node? -- Florian
