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 <p...@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 <p...@kernel.org> > > Signed-off-by: Marek Behún <marek.be...@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 <s...@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: s...@denx.de
