lkml.org 
[lkml]   [2022]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs
    From
    On 7/20/22 09:18, Rob Herring wrote:
    > On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
    >>
    >> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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@kernel.org> 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

    \
     
     \ /
      Last update: 2022-07-20 23:34    [W:3.511 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site