diff mbox series

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

Message ID 20211111153549.29111-2-kabel@kernel.org
State Changes Requested
Delegated to: Stefan Roese
Headers show
Series PCI mvebu and aardvark changes | expand

Commit Message

Marek Behún Nov. 11, 2021, 3:35 p.m. UTC 
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.

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(+)

Comments

Stefan Roese Nov. 12, 2021, 1:59 p.m. UTC | #1 
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
Pali Rohár Nov. 12, 2021, 3:44 p.m. UTC | #2 
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
Stefan Roese Nov. 12, 2021, 4:07 p.m. UTC | #3 
On 11/12/21 16:44, Pali Rohár wrote:
> 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.

Agreed. So let's tick with this 100ms for now.

Thanks,
Stefan

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

Viele Grüße,
Stefan Roese
Pali Rohár Nov. 18, 2021, 6:06 p.m. UTC | #4 
On Friday 12 November 2021 14:59:31 Stefan Roese wrote:
> > 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)

It looks like a good idea!

> 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;
diff mbox series

Patch

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
+
 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;
 }