[PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Jim Quinlan]

On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas at 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 at 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 at 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.

Regards,
Jim Quinlan
Broadcom STB

>
> > [1] https://lore.kernel.org/linux-pci/20210329162539.GG5166@sirena.org.uk/
> > [2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4210 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20220720/6310d115/attachment-0001.p7s>