Message-ID: <vfjh3xzfhwoppcaxlov5bcmkfngyf6no4zyrgexlcxpfajsw2t@o5nbfcep3auz>
Date: Mon, 14 Apr 2025 16:27:35 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Brian Norris <briannorris@...omium.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Jingoo Han <jingoohan1@...il.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kw@...ux.com>, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, Rob Herring <robh@...nel.org>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, dmitry.baryshkov@...aro.org, 
	Tsai Sung-Fu <danielsftsai@...gle.com>
Subject: Re: [RFC] PCI: pwrctrl and link-up dependencies

On Tue, Apr 08, 2025 at 12:59:36PM -0700, Brian Norris wrote:
> TL;DR: PCIe link-up may depend on pwrctrl; however, link-startup is
> often run before pwrctrl gets involved. I'm exploring options to resolve
> this.
> 
> Hi all,
> 
> I'm currently looking at reworking how some (currently out-of-tree, but I'm
> hoping to change that) pcie-designware based drivers integrate power sequencing
> for their endpoint devices, as well as the corresponding start_link()
> functionality.
> 
> For power sequencing, drivers/pci/pwrctrl/ looks like a very good start at what
> we need, since we have various device-specific regulators, GPIOs, and
> sequencing requirements, which we'd prefer not to encode directly in the
> controller driver.
> 

The naming is a bit confusing, but power sequencing and power control are two
different yet related drivers. Power sequencing drivers
(drivers/power/sequencing) are targeted towards devices having complex resource
topology and often accessed by more than one drivers. Like the WiFI + BT combo
PCIe cards. On the other hand, power control (drivers/pci/pwrctrl) drivers are
used to control power to the PCIe slots/cards having simple resource topology.

> For link startup, pcie-designware-host.c currently
> (a) starts the link via platform-specific means (dw_pcie::ops::start_link()) and
> (b) waits for the link training to complete.
> 
> However, (b) will fail if the other end of the link is not powered up --
> e.g., if the appropriate pwrctrl driver has not yet loaded, or its
> device hasn't finished probing. Today, this can mean the designware
> driver will either fail to probe,

This is not correct. DWC driver will start LTSSM and wait for the link to be up
if the platform has no way of detecting link up. But it will not fail if the
link doesn't come up. It will just continue hoping for the link to come up
later. LTSSM would be in Detect.Quiet/Active state till a link partner is found:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-host.c#n558

> or at least waste time for a condition
> that we can't achieve (link up), depending on the HW/driver
> implementation.
> 

Unfortunately we cannot avoid this waiting time as we don't know if a device is
attached to the bus or not. The 1s wait time predates my involvement with DWC
drivers.

> I'm wondering how any designware-based platforms (on which I believe pwrctrl
> was developed) actually support this, and how I should look to integrate
> additional platforms/drivers. From what I can tell, the only way things would
> work today would either be if:
> (1) a given platform uses the dw_pcie_rp::use_linkup_irq==true functionality,
>     which means pcie-designware-host will only start the link, but not wait for
>     training to succeed. (And presumably the controller will receive its
>     link-up IRQ after power sequencing is done, at which point both pwrctrl and
>     the IRQ may rescan the PCI bus.) Or:
> (2) pci/pwrctrl sequencing only brings up some non-critical power rails for the
>     device in question, so link-up can actually succeed even without
>     pwrctrl.
> 

Again, failing to detect link up will not fail the probe. I don't know how you
derived this conclusion. Even the PCIe spec itself is clear that the link should
stay in Detect.Quiet until it has found the link partner. So failing the probe
means we are introducing a dependency on the devices which would be bizarre.
Imagine how a controller will end up supporting hotplug.

> My guess is that (1) is the case, and specifically that the relevant folks are
> using the pcie-qcom.c, with its "global" IRQ used for link-up events.
> 

We only recently added support for 'Link Up' event through 'global_irq' in the
controller driver. And this was done to avoid waiting for link up during probe
(which is what you/your colleagues also want to avoid I believe). But the
problem in your case is that you are completely skipping the LTSSM and relying
on custom userspace tooling to bring up the device and start LTSSM once done.

> So how should I replicate this on other designware-based platforms? I suppose
> if the platform in question has a link-up IRQ, I can imitate (1). But what if
> it doesn't? (Today, we don't validate and utilize a link-up IRQ, although it's
> possible there is one available. Additionally, we use various retry mechanisms
> today, which don't trivially fit into this framework, as we won't know when
> precisely to retry, if power sequencing is controlled entirely by some other
> entity.)
> 
> Would it make sense to introduce some sort of pwrctrl -> start_link()
> dependency? For example, I see similar work done in this series [1], for
> slightly different reasons. In short, that series adds new
> pci_ops::{start,stop}_link() callbacks, and teaches a single pwrctrl driver to
> stop and restart the bridge link before/after powering things up.
> 

This switch has a crazy requirement for configuring it through I2C. The I2C
configuration has to be done before starting LTSSM. So we disable LTSSM first
since it was enabled way before, then do I2C config and then start LTSSM again.

> I also see that Manivannan has a proposal out [2] to add semi-generic
> link-down + retraining support to core code. It treads somewhat similar
> ground, and I could even imagine that its pci_ops::retrain_link()
> callback could even be reimplemented in terms of the aforementioned
> pci_ops::{start,stop}_link(), or possibly vice versa.
> 

Retrain work is mostly to bring up a broken link, which is completely different
from what you are trying to achieve.

> Any thoughts here? Sorry for a lot of text and no patch, but I didn't just want
> to start off by throwing a 3rd set of patches on top of the existing ones that
> tread similar ground[1][2].
> 

No problem. If you want to use pwrctrl in your platform and get rid of the
custom userspace tooling, I'm all in for it. But for that, I need to understand
your controller design first. All I heard so far is, "we want to skip LTSSM and
let our tool take care of it".

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists