Re: [PATCH] PCI: apple: Reset the port for 100ms on probe

["Pali Rohár" <pali@kernel.org>]

On Thursday 18 November 2021 12:57:46 Marc Zyngier wrote:
> On Thu, 18 Nov 2021 10:31:56 +0000,
> Pali Rohár <pali@kernel.org> wrote:
> > For power-on it is probably overkill, but I think that delay between
> > flipping PERST# should be there. IIRC Compex WLE1216 wifi card needs to
> > be at least 10-11ms in reset. Last year, during testing of this card I
> > saw that if PERST#-based reset was shorter then card was completely
> > undetected.
> 
> The only delay we really need is Tperst-clk. Random bugs on random
> devices don't apply here, as the system is completely closed (there is
> no slot to add anything). Once we have TB running one of these days,
> we will see whether this still holds.

Ok!

> > > In practice, I can completely remove the initial Tpvperl delay (we
> > > have been powered-on for a long time already, and the clock is stable
> > > when we come back from setting it up), and cut the second one by half
> > > without observing any ill effect (though I feel safer keeping it to
> > > its nominal value).
> > 
> > My opinion is that this patch does not power on/off card in PCIe slot.
> > And because card is powered-on for a long time (as you wrote), it means
> > that Tpvperl delay does not apply here. That is why I think that
> > different delay (How long should be PCIe card in Warm Reset state)
> > should be used _between_ flipping PERST# signal.
> 
> My reading of the spec is that the only thing we need while #PERST is
> asserted is Tperst-clk. The value you keep arguing about doesn't seem
> to exist as such in the spec, because it appears to be endpoint
> specific.

Well, I was not able to find it in the spec too, that is why I do not
know...

> > And of course after the releasing PERST# that 100ms post-PERST# delay is
> > required.
> 
> That we agree on.
> 
> > I have an idea to move PERST# handling (with all delays) from controller
> > drivers to pci core functions. Because basically every driver
> > re-implements these delays in its probe function. I wrote this idea with
> > some details in email. If you have a time, could you look at it? I
> > summarized here also details about delays (like Tpvperl, Tperstclk, ..):
> > https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> 
> That's a laudable goal. What isn't clear to me is whether you intend
> to move the whole state machine into core code, or just have a set of
> helpers that the driver calls into. IMO, the former is what we really
> need, while the latter only rids us of the simple stuff.

Now I'm just collecting comments and feedbacks for this idea. I think
that state machine in core code is what we need.