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

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?

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