diff mbox series

arm: a37xx: pci: Fix processing PIO transfers

Message ID 20210422142304.31065-1-pali@kernel.org
State Accepted
Commit eccbd4ad8e4e182638eafbfb87ac139c04f24a01
Delegated to: Stefan Roese
Headers show
Series arm: a37xx: pci: Fix processing PIO transfers | expand

Commit Message

Pali Rohár April 22, 2021, 2:23 p.m. UTC 
Trying to clear PIO_START register when it is non-zero (which indicates
that previous PIO transfer has not finished yet) causes an External
Abort with SError 0xbf000002.

This bug is currently worked around in TF-A by handling External Aborts
in EL3 and ignoring this particular SError.

This workaround was also discussed at:
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=3c7dcdac5c50
https://lore.kernel.org/linux-pci/20190316161243.29517-1-repk@triplefau.lt/
https://lore.kernel.org/linux-pci/971be151d24312cc533989a64bd454b4@www.loen.fr/
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/1541

Implement a proper fix to prevent this External Abort. As it is not
possible to cancel a pending PIO transfer, simply do not start a new one
if previous has not finished yet. In this case return an error to the
caller.

In most cases this SError happens when there is no PCIe card connected
or when PCIe link is down. The reason is that in these cases a PIO
transfer takes about 1.44 seconds. For this reason we also increase the
wait timeout in pcie_advk_wait_pio() to 1.5 seconds.

If PIO read transfer for PCI_VENDOR_ID register times out, or if it
isn't possible to read it yet because previous transfer is not finished,
return Completion Retry Status value instead of failing, to give the
caller a chance to send a new read request.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci-aardvark.c | 42 +++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Stefan Roese April 26, 2021, 9:22 a.m. UTC | #1 
On 22.04.21 16:23, Pali Rohár wrote:
> Trying to clear PIO_START register when it is non-zero (which indicates
> that previous PIO transfer has not finished yet) causes an External
> Abort with SError 0xbf000002.
> 
> This bug is currently worked around in TF-A by handling External Aborts
> in EL3 and ignoring this particular SError.
> 
> This workaround was also discussed at:
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=3c7dcdac5c50
> https://lore.kernel.org/linux-pci/20190316161243.29517-1-repk@triplefau.lt/
> https://lore.kernel.org/linux-pci/971be151d24312cc533989a64bd454b4@www.loen.fr/
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/1541
> 
> Implement a proper fix to prevent this External Abort. As it is not
> possible to cancel a pending PIO transfer, simply do not start a new one
> if previous has not finished yet. In this case return an error to the
> caller.
> 
> In most cases this SError happens when there is no PCIe card connected
> or when PCIe link is down. The reason is that in these cases a PIO
> transfer takes about 1.44 seconds. For this reason we also increase the
> wait timeout in pcie_advk_wait_pio() to 1.5 seconds.
> 
> If PIO read transfer for PCI_VENDOR_ID register times out, or if it
> isn't possible to read it yet because previous transfer is not finished,
> return Completion Retry Status value instead of failing, to give the
> caller a chance to send a new read request.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 42 +++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 3b9309f52c..c43d4f309b 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -132,8 +132,9 @@
>   	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
>   
>   /* PCIe Retries & Timeout definitions */
> -#define MAX_RETRIES				10
> -#define PIO_WAIT_TIMEOUT			100
> +#define PIO_MAX_RETRIES				1500
> +#define PIO_WAIT_TIMEOUT			1000
> +#define LINK_MAX_RETRIES			10
>   #define LINK_WAIT_TIMEOUT			100000
>   
>   #define CFG_RD_UR_VAL			0xFFFFFFFF
> @@ -192,7 +193,7 @@ static int pcie_advk_addr_valid(pci_dev_t bdf, int first_busno)
>    *
>    * @pcie: The PCI device to access
>    *
> - * Wait up to 1 micro second for PIO access to be accomplished.
> + * Wait up to 1.5 seconds for PIO access to be accomplished.
>    *
>    * Return 1 (true) if PIO access is accomplished.
>    * Return 0 (false) if PIO access is timed out.
> @@ -202,7 +203,7 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>   	uint start, isr;
>   	uint count;
>   
> -	for (count = 0; count < MAX_RETRIES; count++) {
> +	for (count = 0; count < PIO_MAX_RETRIES; count++) {
>   		start = advk_readl(pcie, PIO_START);
>   		isr = advk_readl(pcie, PIO_ISR);
>   		if (!start && isr)
> @@ -214,7 +215,7 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>   		udelay(PIO_WAIT_TIMEOUT);
>   	}
>   
> -	dev_err(pcie->dev, "config read/write timed out\n");
> +	dev_err(pcie->dev, "PIO read/write transfer time out\n");
>   	return 0;
>   }
>   
> @@ -323,9 +324,14 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> -	/* Start PIO */
> -	advk_writel(pcie, 0, PIO_START);
> -	advk_writel(pcie, 1, PIO_ISR);
> +	if (advk_readl(pcie, PIO_START)) {
> +		dev_err(pcie->dev,
> +			"Previous PIO read/write transfer is still running\n");
> +		if (offset != PCI_VENDOR_ID)
> +			return -EINVAL;
> +		*valuep = CFG_RD_CRS_VAL;
> +		return 0;
> +	}
>   
>   	/* Program the control register */
>   	reg = advk_readl(pcie, PIO_CTRL);
> @@ -342,10 +348,15 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	advk_writel(pcie, 0, PIO_ADDR_MS);
>   
>   	/* Start the transfer */
> +	advk_writel(pcie, 1, PIO_ISR);
>   	advk_writel(pcie, 1, PIO_START);
>   
> -	if (!pcie_advk_wait_pio(pcie))
> -		return -EINVAL;
> +	if (!pcie_advk_wait_pio(pcie)) {
> +		if (offset != PCI_VENDOR_ID)
> +			return -EINVAL;
> +		*valuep = CFG_RD_CRS_VAL;
> +		return 0;
> +	}
>   
>   	/* Check PIO status and get the read result */
>   	ret = pcie_advk_check_pio_status(pcie, true, &reg);
> @@ -420,9 +431,11 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> -	/* Start PIO */
> -	advk_writel(pcie, 0, PIO_START);
> -	advk_writel(pcie, 1, PIO_ISR);
> +	if (advk_readl(pcie, PIO_START)) {
> +		dev_err(pcie->dev,
> +			"Previous PIO read/write transfer is still running\n");
> +		return -EINVAL;
> +	}
>   
>   	/* Program the control register */
>   	reg = advk_readl(pcie, PIO_CTRL);
> @@ -450,6 +463,7 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   	dev_dbg(pcie->dev, "\tPIO req. - strb = 0x%02x\n", reg);
>   
>   	/* Start the transfer */
> +	advk_writel(pcie, 1, PIO_ISR);
>   	advk_writel(pcie, 1, PIO_START);
>   
>   	if (!pcie_advk_wait_pio(pcie)) {
> @@ -494,7 +508,7 @@ static int pcie_advk_wait_for_link(struct pcie_advk *pcie)
>   	int retries;
>   
>   	/* check if the link is up or not */
> -	for (retries = 0; retries < MAX_RETRIES; retries++) {
> +	for (retries = 0; retries < LINK_MAX_RETRIES; retries++) {
>   		if (pcie_advk_link_up(pcie)) {
>   			printf("PCIE-%d: Link up\n", pcie->first_busno);
>   			return 0;
> 


Viele Grüße,
Stefan
Stefan Roese April 29, 2021, 6:46 a.m. UTC | #2 
On 22.04.21 16:23, Pali Rohár wrote:
> Trying to clear PIO_START register when it is non-zero (which indicates
> that previous PIO transfer has not finished yet) causes an External
> Abort with SError 0xbf000002.
> 
> This bug is currently worked around in TF-A by handling External Aborts
> in EL3 and ignoring this particular SError.
> 
> This workaround was also discussed at:
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=3c7dcdac5c50
> https://lore.kernel.org/linux-pci/20190316161243.29517-1-repk@triplefau.lt/
> https://lore.kernel.org/linux-pci/971be151d24312cc533989a64bd454b4@www.loen.fr/
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/1541
> 
> Implement a proper fix to prevent this External Abort. As it is not
> possible to cancel a pending PIO transfer, simply do not start a new one
> if previous has not finished yet. In this case return an error to the
> caller.
> 
> In most cases this SError happens when there is no PCIe card connected
> or when PCIe link is down. The reason is that in these cases a PIO
> transfer takes about 1.44 seconds. For this reason we also increase the
> wait timeout in pcie_advk_wait_pio() to 1.5 seconds.
> 
> If PIO read transfer for PCI_VENDOR_ID register times out, or if it
> isn't possible to read it yet because previous transfer is not finished,
> return Completion Retry Status value instead of failing, to give the
> caller a chance to send a new read request.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>
> ---
>   drivers/pci/pci-aardvark.c | 42 +++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 14 deletions(-)

Applied to u-boot-marvell/master

Thanks,
Stefan

> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 3b9309f52c..c43d4f309b 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -132,8 +132,9 @@
>   	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
>   
>   /* PCIe Retries & Timeout definitions */
> -#define MAX_RETRIES				10
> -#define PIO_WAIT_TIMEOUT			100
> +#define PIO_MAX_RETRIES				1500
> +#define PIO_WAIT_TIMEOUT			1000
> +#define LINK_MAX_RETRIES			10
>   #define LINK_WAIT_TIMEOUT			100000
>   
>   #define CFG_RD_UR_VAL			0xFFFFFFFF
> @@ -192,7 +193,7 @@ static int pcie_advk_addr_valid(pci_dev_t bdf, int first_busno)
>    *
>    * @pcie: The PCI device to access
>    *
> - * Wait up to 1 micro second for PIO access to be accomplished.
> + * Wait up to 1.5 seconds for PIO access to be accomplished.
>    *
>    * Return 1 (true) if PIO access is accomplished.
>    * Return 0 (false) if PIO access is timed out.
> @@ -202,7 +203,7 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>   	uint start, isr;
>   	uint count;
>   
> -	for (count = 0; count < MAX_RETRIES; count++) {
> +	for (count = 0; count < PIO_MAX_RETRIES; count++) {
>   		start = advk_readl(pcie, PIO_START);
>   		isr = advk_readl(pcie, PIO_ISR);
>   		if (!start && isr)
> @@ -214,7 +215,7 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>   		udelay(PIO_WAIT_TIMEOUT);
>   	}
>   
> -	dev_err(pcie->dev, "config read/write timed out\n");
> +	dev_err(pcie->dev, "PIO read/write transfer time out\n");
>   	return 0;
>   }
>   
> @@ -323,9 +324,14 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> -	/* Start PIO */
> -	advk_writel(pcie, 0, PIO_START);
> -	advk_writel(pcie, 1, PIO_ISR);
> +	if (advk_readl(pcie, PIO_START)) {
> +		dev_err(pcie->dev,
> +			"Previous PIO read/write transfer is still running\n");
> +		if (offset != PCI_VENDOR_ID)
> +			return -EINVAL;
> +		*valuep = CFG_RD_CRS_VAL;
> +		return 0;
> +	}
>   
>   	/* Program the control register */
>   	reg = advk_readl(pcie, PIO_CTRL);
> @@ -342,10 +348,15 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	advk_writel(pcie, 0, PIO_ADDR_MS);
>   
>   	/* Start the transfer */
> +	advk_writel(pcie, 1, PIO_ISR);
>   	advk_writel(pcie, 1, PIO_START);
>   
> -	if (!pcie_advk_wait_pio(pcie))
> -		return -EINVAL;
> +	if (!pcie_advk_wait_pio(pcie)) {
> +		if (offset != PCI_VENDOR_ID)
> +			return -EINVAL;
> +		*valuep = CFG_RD_CRS_VAL;
> +		return 0;
> +	}
>   
>   	/* Check PIO status and get the read result */
>   	ret = pcie_advk_check_pio_status(pcie, true, &reg);
> @@ -420,9 +431,11 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> -	/* Start PIO */
> -	advk_writel(pcie, 0, PIO_START);
> -	advk_writel(pcie, 1, PIO_ISR);
> +	if (advk_readl(pcie, PIO_START)) {
> +		dev_err(pcie->dev,
> +			"Previous PIO read/write transfer is still running\n");
> +		return -EINVAL;
> +	}
>   
>   	/* Program the control register */
>   	reg = advk_readl(pcie, PIO_CTRL);
> @@ -450,6 +463,7 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   	dev_dbg(pcie->dev, "\tPIO req. - strb = 0x%02x\n", reg);
>   
>   	/* Start the transfer */
> +	advk_writel(pcie, 1, PIO_ISR);
>   	advk_writel(pcie, 1, PIO_START);
>   
>   	if (!pcie_advk_wait_pio(pcie)) {
> @@ -494,7 +508,7 @@ static int pcie_advk_wait_for_link(struct pcie_advk *pcie)
>   	int retries;
>   
>   	/* check if the link is up or not */
> -	for (retries = 0; retries < MAX_RETRIES; retries++) {
> +	for (retries = 0; retries < LINK_MAX_RETRIES; retries++) {
>   		if (pcie_advk_link_up(pcie)) {
>   			printf("PCIE-%d: Link up\n", pcie->first_busno);
>   			return 0;
> 


Viele Grüße,
Stefan
diff mbox series

Patch

diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
index 3b9309f52c..c43d4f309b 100644
--- a/drivers/pci/pci-aardvark.c
+++ b/drivers/pci/pci-aardvark.c
@@ -132,8 +132,9 @@ 
 	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
 
 /* PCIe Retries & Timeout definitions */
-#define MAX_RETRIES				10
-#define PIO_WAIT_TIMEOUT			100
+#define PIO_MAX_RETRIES				1500
+#define PIO_WAIT_TIMEOUT			1000
+#define LINK_MAX_RETRIES			10
 #define LINK_WAIT_TIMEOUT			100000
 
 #define CFG_RD_UR_VAL			0xFFFFFFFF
@@ -192,7 +193,7 @@  static int pcie_advk_addr_valid(pci_dev_t bdf, int first_busno)
  *
  * @pcie: The PCI device to access
  *
- * Wait up to 1 micro second for PIO access to be accomplished.
+ * Wait up to 1.5 seconds for PIO access to be accomplished.
  *
  * Return 1 (true) if PIO access is accomplished.
  * Return 0 (false) if PIO access is timed out.
@@ -202,7 +203,7 @@  static int pcie_advk_wait_pio(struct pcie_advk *pcie)
 	uint start, isr;
 	uint count;
 
-	for (count = 0; count < MAX_RETRIES; count++) {
+	for (count = 0; count < PIO_MAX_RETRIES; count++) {
 		start = advk_readl(pcie, PIO_START);
 		isr = advk_readl(pcie, PIO_ISR);
 		if (!start && isr)
@@ -214,7 +215,7 @@  static int pcie_advk_wait_pio(struct pcie_advk *pcie)
 		udelay(PIO_WAIT_TIMEOUT);
 	}
 
-	dev_err(pcie->dev, "config read/write timed out\n");
+	dev_err(pcie->dev, "PIO read/write transfer time out\n");
 	return 0;
 }
 
@@ -323,9 +324,14 @@  static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
 		return 0;
 	}
 
-	/* Start PIO */
-	advk_writel(pcie, 0, PIO_START);
-	advk_writel(pcie, 1, PIO_ISR);
+	if (advk_readl(pcie, PIO_START)) {
+		dev_err(pcie->dev,
+			"Previous PIO read/write transfer is still running\n");
+		if (offset != PCI_VENDOR_ID)
+			return -EINVAL;
+		*valuep = CFG_RD_CRS_VAL;
+		return 0;
+	}
 
 	/* Program the control register */
 	reg = advk_readl(pcie, PIO_CTRL);
@@ -342,10 +348,15 @@  static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
 	advk_writel(pcie, 0, PIO_ADDR_MS);
 
 	/* Start the transfer */
+	advk_writel(pcie, 1, PIO_ISR);
 	advk_writel(pcie, 1, PIO_START);
 
-	if (!pcie_advk_wait_pio(pcie))
-		return -EINVAL;
+	if (!pcie_advk_wait_pio(pcie)) {
+		if (offset != PCI_VENDOR_ID)
+			return -EINVAL;
+		*valuep = CFG_RD_CRS_VAL;
+		return 0;
+	}
 
 	/* Check PIO status and get the read result */
 	ret = pcie_advk_check_pio_status(pcie, true, &reg);
@@ -420,9 +431,11 @@  static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
 		return 0;
 	}
 
-	/* Start PIO */
-	advk_writel(pcie, 0, PIO_START);
-	advk_writel(pcie, 1, PIO_ISR);
+	if (advk_readl(pcie, PIO_START)) {
+		dev_err(pcie->dev,
+			"Previous PIO read/write transfer is still running\n");
+		return -EINVAL;
+	}
 
 	/* Program the control register */
 	reg = advk_readl(pcie, PIO_CTRL);
@@ -450,6 +463,7 @@  static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
 	dev_dbg(pcie->dev, "\tPIO req. - strb = 0x%02x\n", reg);
 
 	/* Start the transfer */
+	advk_writel(pcie, 1, PIO_ISR);
 	advk_writel(pcie, 1, PIO_START);
 
 	if (!pcie_advk_wait_pio(pcie)) {
@@ -494,7 +508,7 @@  static int pcie_advk_wait_for_link(struct pcie_advk *pcie)
 	int retries;
 
 	/* check if the link is up or not */
-	for (retries = 0; retries < MAX_RETRIES; retries++) {
+	for (retries = 0; retries < LINK_MAX_RETRIES; retries++) {
 		if (pcie_advk_link_up(pcie)) {
 			printf("PCIE-%d: Link up\n", pcie->first_busno);
 			return 0;