On Fri, Jul 11, 2025 at 05:04:38PM GMT, Brian Norris wrote:
> Hi,
>
> On Wed, Jul 09, 2025 at 12:18:29PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jul 08, 2025 at 06:39:37PM GMT, Brian Norris wrote:
> > > On Mon, Jul 07, 2025 at 11:48:37PM +0530, Manivannan Sadhasivam wrote:
> > > > Hi,
> > > >
> > > > This series is an RFC to propose pwrctrl framework to control the PERST# GPIO
> > > > instead of letting the controller drivers to do so (which is a mistake btw).
> > > >
> > > > Right now, the pwrctrl framework is controlling the power supplies to the
> > > > components (endpoints and such), but it is not controlling PERST#. This was
> > > > pointed out by Brian during a related conversation [1]. But we cannot just move
> > > > the PERST# control from controller drivers due to the following reasons:
> > > >
> > > > 1. Most of the controller drivers need to assert PERST# during the controller
> > > > initialization sequence. This is mostly as per their hardware reference manual
> > > > and should not be changed.
> > > >
> > > > 2. Controller drivers still need to toggle PERST# when pwrctrl is not used i.e.,
> > > > when the power supplies are not accurately described in PCI DT node. This can
> > > > happen on unsupported platforms and also for platforms with legacy DTs.
> > > >
> > > > For this reason, I've kept the PERST# retrieval logic in the controller drivers
> > > > and just passed the gpio descriptors (for each slot) to the pwrctrl framework.
> > >
> > > How sure are we that GPIOs (and *only* GPIOs) are sufficient for this
> > > feature? I've seen a few drivers that pair a GPIO with some kind of
> > > "internal" reset too, and it's not always clear that they can/should be
> > > operated separately.
> > >
> > > For example, drivers/pci/controller/dwc/pci-imx6.c /
> > > imx_pcie_{,de}assert_core_reset(), and pcie-tegra194.c's
> > > APPL_PINMUX_PEX_RST. The tegra case especially seems pretty clear that
> > > its non-GPIO "pex_rst" is resetting an endpoint.
> > >
> >
> > Right. But GPIO is the most commonly used approach for implementing PERST# and
> > it is the one supported on the platform I'm testing with. So I just went with
> > that. For sure there are other methods exist and the PCIe spec itself doesn't
> > define how PERST# should be implemented in a form factor. It merely defines
> > PERST# as an 'auxiliary signal'. So yes, other form of PERST# can exist. But for
> > the sake of keeping this proposal simple, I'm considering only GPIO based
> > PERST# atm.
>
> Hmm, OK. A simple start is fine, but I'm pointing out this will quickly
> show its limitations.
>
I don't disagree :)
> > Also, Tegra platforms are not converted to use pwrctrl framework and I don't
> > know if the platform maintainers are interested in it or not. But if they start
> > using it, we can tackle this situation by introducing a callback that
> > asserts/deasserts PERST# (yes, callbacks are evil, but I don't know any other
> > sensible way to support vendor specific PERST# implementations).
>
> IMO, it's pretty fair game to at least account for things people are
> doing in upstream drivers today, even if they aren't wholly ready to
> adopt the new thing. It's harder to gain new users when you actively
> don't support things you know the users need.
>
> > Oh and do take a look at pcie-brcmstb driver, which I promised to move to
> > pwrctrl framework for another reason. It uses multiple callbacks per SoC
> > revisions for toggling PERST#. So for these usecases, having a callback in
> > 'struct pci_host_bridge' would be a good fit and I may introduce it after this
> > series gets in.
>
> Sure. I think there are plenty of drivers that will need it. And that's
> why I brought it up.
>
> But if that's a "phase 2" thing, so be it.
>
> > > > This will allow both the controller drivers and pwrctrl framework to share the
> > > > PERST# (which is ugly but can't be avoided). But care must be taken to ensure
> > > > that the controller drivers only assert PERST# and not deassert when pwrctrl is
> > > > used. I've added the change for the Qcom driver as a reference. The Qcom driver
> > > > is a slight mess because, it now has to support both new DT binding (PERST# and
> > > > PHY in Root Port node) and legacy (both in Host Bridge node). So I've allowed
> > > > the PERST# control only for the new binding (which is always going to use
> > > > pwrctrl framework to control the component supplies).
> > > >
> > > > Testing
> > > > =======
> > > >
> > > > This series is tested on Lenovo Thinkpad T14s laptop (with out-of-tree patch
> > > > enabling PCIe WLAN card) and on RB3 Gen2 with TC9563 switch (also with the not
> > > > yet merged series [2]). A big take away from this series is that, it is now
> > > > possible to get rid of the controversial {start/stop}_link() callback proposed
> > > > in the above mentioned switch pwrctrl driver [3].
> > >
> > > This is a tiny bit tangential to the PERST# discussion, but I believe
> > > there are other controller driver features that don't fit into the
> > > sequence model of:
> > >
> > > 1. start LTSSM (controller driver)
> > > 2. pwrctrl eventually turns on power + delay per spec
> > > 3. pwrctrl deasserts PERST#
> > > 4. pwrctrl delays a fixed amount of time, per the CEM spec
> > > 5. pwrctrl rescans bus
> > >
> > > For example, tegra_pcie_dw_start_link() notes some cases where it needs
> > > to take action and retry when the link doesn't come up. Similarly, I've
> > > seen drivers with retry loops for cases where the link comes up, but not
> > > at the expected link rate. None of this is possible if the controller
> > > driver only gets to take care of #1, and has no involvement in between
> > > #3 and #5.
> > >
> >
> > Having this back and forth communication would make the pwrctrl driver a lot
> > messier. But I believe, we could teach pwrctrl driver to detect link up (similar
> > to dw_pcie_wait_for_link()) and if link didn't come up, it could do retry and
> > other steps with help from controller drivers. But these things should be
> > implemented only when platforms like Tegra start to show some love towards
> > pwrctrl.
>
> Never mind the lack of love you feel here :)
> But I'm actively looking at drivers that don't yet fit into what pwrctrl
> supports, and I'd like them to use pwrctrl someday instead of
> reinventing the wheel.
>
I'm not against supporting these controller drivers/platforms in pwrctrl. It's
just that I do not want to implement solutions now without having users. But I'm
glad that you are bringing these up. I'm adding these to my pwrctrl/todo list.
> You're arguing against more callbacks, and start_link()-like
> functionality, but I'm pretty sure some of these things are necessities,
> if you're trying to abstract power control away from controller drivers.
>
Well, that's my personal preference. These days I feel (mostly after spending my
time as a maintainer of PCI controller drivers) that callbacks are evil and they
pay way for 'midlayers'. But having said that, I myself implemented callbacks
whereever I felt that no other options were possible. And here also, I'm just
claiming that this series avoids the '{start/stop}_link' callbacks which was
hated by many (including me) as it ties the Qcom switch driver implementing
these callbacks to be tied to a specific platform.
> Again, maybe this is a problem to be solved later. But I think you're
> kidding yourself that pwrctrl is ready as-is, and that you can avoid
> these kinds of callbacks.
>
No, I never claimed that pwrctrl is perfect and it would solve all the PCI power
issues. But I'm happy that it atleast solves a couple of issues and allows me to
address the rest of them. We had been talking about a framework like this for
several years without any upstream solution. But now we finally have one and I'm
merely trying to make use of it to address issues.
- Mani
--
மணிவண்ணன் சதாசிவம்
