Re: [PATCH u-boot-marvell 01/10] pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()

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

On Friday 12 November 2021 14:59:31 Stefan Roese wrote:
> On 11/11/21 16:35, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Function mvebu_pcie_probe() configures PCIe controller and sometimes it
> > takes some time for PCIe card to bring link up. Delay mvebu_pcie_probe()
> > for 100ms to ensure that PCIe link is up after function finish. In the
> > case when no card is connected to the PCIe slot, this will delay probe
> > time by 100ms, which should not be problematic.
> 
> Where does this 100ms come from? From tests with your PCIe devices /
> card?

pci-aardvark.c has similar wait timeout, but up to the 1s.

In PCIe base spec 4.0 in section 6.6.1 Conventional Reset is written:
With a Downstream Port that does not support Link speeds greater than
5.0 GT/s, software must wait a minimum of 100 ms before sending a
Configuration Request to the device immediately below that Port.

So I think that waiting 100ms is reasonable... But I do not know what
should be correct here as proper initialization needs more steps...
For more details see my email sent to linux-pci:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

I saw that more drivers in kernel are using different timeouts at
different levels and they are between 1ms-150ms. It is just mess.

> Please see another comment below...
> 
> > This change fixes detection and initialization of some QCA98xx cards on
> > the first serdes when configured in x1 mode. Default configuration of
> > the first serdes on A385 is x4 mode, so it looks as if some delay is
> > required when x4 is changed to x1 and card correctly links with A385.
> > Other PCIe serdes ports on A385 are x1-only, and so they don't have this
> > problem.
> > 
> > (We need this patch because in the following patch we are moving the
> >   configuration change from x4 to x1 from serdes driver to PCIe mvebu
> >   driver, because the corresponding register lives in the address space
> >   of the PCIe controller. Without that this explicit timeout is not
> >   needed, because there is an implicit timeout between change from x4 to
> >   x1 and probe of PCIe mvebu driver, due to the first being run in SPL
> >   and the second in U-Boot proper.)
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >   drivers/pci/pci_mvebu.c | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index 14cd82db6f..a3364d5a59 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -22,6 +22,7 @@
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> >   #include <linux/bitops.h>
> > +#include <linux/delay.h>
> >   #include <linux/errno.h>
> >   #include <linux/ioport.h>
> >   #include <linux/mbus.h>
> > @@ -70,6 +71,9 @@ DECLARE_GLOBAL_DATA_PTR;
> >   #define PCIE_DEBUG_CTRL			0x1a60
> >   #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
> > +#define LINK_WAIT_RETRIES	100
> > +#define LINK_WAIT_TIMEOUT	1000
> 
> Wouldn't it be easier read, if this was defines like this:
> 
> #define LINK_TIMEOUT_MS		100
> #define LINK_WAIT_TIMEOUT_US	1000
> #define LINK_WAIT_RETRIES	((LINK_TIMEOUT_MS * 1000) / LINK_WAIT_TIMEOUT_US)
> 
> Other than this:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan
> 
> > +
> >   struct mvebu_pcie {
> >   	struct pci_controller hose;
> >   	void __iomem *base;
> > @@ -106,6 +110,23 @@ static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
> >   	return !(val & PCIE_STAT_LINK_DOWN);
> >   }
> > +static void mvebu_pcie_wait_for_link(struct mvebu_pcie *pcie)
> > +{
> > +	int retries;
> > +
> > +	/* check if the link is up or not */
> > +	for (retries = 0; retries < LINK_WAIT_RETRIES; retries++) {
> > +		if (mvebu_pcie_link_up(pcie)) {
> > +			printf("%s: Link up\n", pcie->name);
> > +			return;
> > +		}
> > +
> > +		udelay(LINK_WAIT_TIMEOUT);
> > +	}
> > +
> > +	printf("%s: Link down\n", pcie->name);
> > +}
> > +
> >   static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie *pcie, int busno)
> >   {
> >   	u32 stat;
> > @@ -477,6 +498,8 @@ 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);
> > +	mvebu_pcie_wait_for_link(pcie);
> > +
> >   	return 0;
> >   }
> > 
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de