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

[Pali Rohár]

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.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali at kernel.org>
> > > > ---
> > > >    drivers/pci/pci_mvebu.c | 14 ++++++++++++++
> > > >    1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > > > index d80f87e0cfc6..269c027db204 100644
> > > > --- a/drivers/pci/pci_mvebu.c
> > > > +++ b/drivers/pci/pci_mvebu.c
> > > > @@ -22,6 +22,7 @@
> > > >    #include <asm/io.h>
> > > >    #include <asm/arch/cpu.h>
> > > >    #include <asm/arch/soc.h>
> > > > +#include <asm-generic/gpio.h>
> > > >    #include <linux/bitops.h>
> > > >    #include <linux/delay.h>
> > > >    #include <linux/errno.h>
> > > > @@ -60,6 +61,7 @@ struct mvebu_pcie {
> > > >    	struct resource mem;
> > > >    	void __iomem *iobase;
> > > >    	struct resource io;
> > > > +	struct gpio_desc reset_gpio;
> > > >    	u32 intregs;
> > > >    	u32 port;
> > > >    	u32 lane;
> > > > @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	struct udevice *ctlr = pci_get_controller(dev);
> > > >    	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> > > >    	u32 reg;
> > > > +	int ret;
> > > > +
> > > > +	/* Request for optional PERST# GPIO */
> > > > +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> > > > +	if (ret && ret != -ENOENT) {
> > > > +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> > > > +		return ret;
> > > > +	}
> > > >    	/*
> > > >    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> > > > @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
> > > >    		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
> > > > +	/* Release PERST# via GPIO when it was defined */
> > > > +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> > > > +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> > > > +
> > > 
> > > 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)?
> > > 
> > > Thanks,
> > > Stefan
> > 
> > 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?
> 
> Thanks,
> Stefan

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).

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.

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/