[PATCH] arm: a37xx: pci: Fix processing PIO transfers

[Stefan Roese]

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

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