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
