[PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

On Friday 29 July 2022 10:09:52 Francesco Dolcini wrote:
> On Thu, Jul 28, 2022 at 05:30:00PM +0200, Pali Rohár wrote:
> > On Thursday 28 July 2022 17:13:12 Stefan Roese wrote:
> > > On 28.07.22 17:10, Pali Rohár wrote:
> > > > On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
> > > > > On 28.07.22 15:03, Pali Rohár wrote:
> > > > > > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > > > > So you're only releasing the GPIO (setting to inactive) here. Wouldn't
> > > > > it make sense to first use the GPIO (if available via DT) to actually
> > > > > reset the PCI device? How is this done in the Kernel driver(s)?
> > > > 
> > > > This is something which I do not know what is the best. Kernel driver
> > > > pci-mvebu.c has same logic - just release from reset at startup.
> > > 
> > > I see. Could you please check some other PCI Kernel drivers, how they
> > > handle reset-gpios signalling?
> > 
> > Every kernel driver is doing it differently. Touching PERST# in
> > bootloader is for two different purposes (which is board specific):
> > 
> > 1) Board is designed in a way that PCIe stays in reset after CPU starts
> >    and it is up to the SW to release PCIe reset, including reset of
> >    endpoint cards.
> > 
> >    I think that this is recommended design due to how are PCIe
> >    requirements for timings and dependences on other stuff (like PCIe
> >    clock stabilization, power on, etc...).
> > 
> >    This is also what I prefer for designing new boards.
> > 
> > 2) Board is designed in a way that endpoint PCIe card is after CPU reset
> >    immediately released from the reset or maybe that PCIe card is not
> >    reset at all after CPU reset.
> > 
> >    In this case lot of "broken" cards do not work until they are
> >    manually reset from software, which requires flipping PERST# pin
> >    manually (via GPIO).
> 
> Those card that you mention as broken are often just fine, from
> the standard point, they are just a pain ...
> 
> > 
> > Bootloader / U-Boot starts as the first SW and depends on specific state
> > of CPU reset. So it can expects that nothing release PERST# from 1)
> > before it.
> 
> I think in general this last one is not a correct assumption, reset could just
> not be asserted at that point because of the hardware design.

Yes, that is why I marked the last part that applies only for hardware
design from option 1) mentioned above.

> I recently had issue because of that (Linux Kernel, not U-Boot), and
> this is the fix that was merged:
> 
> a6809941c1f1 ("PCI: imx6: Fix PERST# start-up sequence")
> 
> (and I also had myself similar issue while working on Intel platform
> firmware, e.g. UEFI).
> 
> Said that I agree that the topic is way more complicated than it
> should be :-/

Yes, it is :-( I proposed some options for kernel but it is lot of work
for which probably nobody has time...

> > 
> > In operating system it is harder as kernel cannot do this assumption.
> > 
> > And to make it even harder, there is important question: How long should
> > be PERST# signal active to ensure that endpoint card is correctly reset?
> > 
> > Without knowing answer to this question, we do not know and cannot
> > implement 2) properly.
> > 
> > I was not able to find answer for this question in PCIe specs, so I
> > have asked it on mailing linux-pci mailing list:
> > https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > (and it is still without clear answer)
> > 
> > And if we go deeper to the PCIe initialization (as PERST# signal is part
> > of it) I described some proposal for Linux kernel how it should be
> > implemented correctly:
> > https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> > 
> > So all this stuff needs to be rewritten, ideally into some common
> > framework. Until that happens, every driver would continue doing it by
> > its own -- and differently.
> > 
> > Yes, it is a mess, but I do not know what we can do with it.
> > 
> > Btw, in past I did also investigation of those resets and delays, and
> > really every kernel driver is doing it differently:
> > https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> > 
>