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

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali at 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?

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 at kernel.org>
> Signed-off-by: Marek Behún <marek.behun at 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 at 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 at denx.de