Message-ID: <Z__ljc6WaK8u5kff@google.com>
Date: Wed, 16 Apr 2025 10:14:53 -0700
From: Brian Norris <briannorris@...omium.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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

Hi Manivannan,

On Wed, Apr 16, 2025 at 11:29:32AM +0530, Manivannan Sadhasivam wrote:
> On Tue, Apr 15, 2025 at 11:24:39AM -0700, Brian Norris wrote:
> > We might be talking past each other. Per above, I think we can do better
> > with (1)-(3). But you're bringing up (4). Problem (3) exists for all
> > drivers, although it's more acute with DWC, and I happen to be using it.
> > I also think it's indicative of larger design and ordering problems in
> > pwrctrl.
> > 
> 
> Now I get what you are saying.

Great! I probably didn't include all my thoughts in the first place, but
then, my first email was already long enough :)

> > As an example less-cute way of doing pwrctrl: expose a wrapped version
> > of pci_pwrctrl_create_device() such that drivers can call it earlier. If
> > there is a pwrctrl device created, that means a driver should not yet
> > wait for link-up -- it should defer that until the relevant pwrctrl is
> > marked "ready". (There are likely other problems to solve in here too,
> > but this is just an initial sketch. And to be clear, I suspect this
> > doesn't fit your notion of "generic", because it requires each driver to
> > adapt to it.)
> > 
> 
> This is what I initially had in my mind, but then I opted for a solution which
> allowed the pwrctrl devices to be created in the PCI core itself without any
> modifications in the controller drivers.
> 
> But I totally agree with you that now we don't have any control over PERST# and
> that should be fixed.

Yeah, if we have to handle PERST# and its timing, then we have to touch
essentially every driver anyway, I think. So it's definitely a chance to
go a (slightly) different direction.

(Side note: I think this is potentially a chance to solve the odd I2C
pwrctrl problem I linked in my original post with the same set of hooks.
If a controller driver can know when pwrctrl is finished, then it can
also defer its LTSSM until after that point.

I doubt this will be the last set of "odd" HW where additional
platform-specific dependencies need to be inserted between pwrctrl and
PCI enumeration.)

> > IOW, the pwrctl sequence should be something like:
> > 
> >  (1) power up the slot
> >  (1)(a) delay, per spec
> >  (1)(b) deassert PERST#
> >  (1)(c) wait for link up
> >  (2) rescan bus
> > 
> > Currently, we skip all of (1)(a)-(c). We're probably lucky that (1)(b)'s
> > ordering doesn't matter all the time, as long as we did it earlier. And
> > we're lucky that there are natural delays in software such that lack of
> > (1)(a) and (1)(c) aren't significant.
> > 
> 
> Let me go back to the drawing board and come up with a proposal. There are
> atleast a couple of ways to fix this issue and I need to pick a less intrusive
> one.

That's kind of you. Let me know if I can help at all. Or at least CC me
on any updates you have.

> Thanks for reporting it, appreciated!

Thanks for walking through it with me!

Brian

Powered by blists - more mailing lists