[PATCH] arm: a37xx: pci: Fix handling PIO config error responses

[Stefan Roese]

On 09.08.21 09:53, Pali Rohár wrote:
> Returning fabricated CRS value (0xFFFF0001) by PCIe Root Complex to OS is
> allowed only for 4-byte PCI_VENDOR_ID config read request and only when
> CRSSVE bit in Root Port PCIe device is enabled. In all other error PCIe
> Root Complex must return all-ones.
> 
> So implement this logic in pci-aardvark.c driver properly.
> 
> aardvark HW does not have Root Port PCIe device and U-Boot does not
> implement emulation of this device. So expect that CRSSVE bit is set as
> U-Boot can already handle CRS value for PCI_VENDOR_ID config read request.
> 
> More callers of pci_bus_read_config() function in U-Boot do not check for
> return value, but check readback value. Therefore always fill readback
> value in pcie_advk_read_config() function. On error fill all-ones of
> correct size as it is required for PCIe Root Complex.
> 
> And also correctly propagates error from failed config write request to
> return value of pcie_advk_write_config() function. Most U-Boot callers
> ignores this return value, but it is a good idea to return correct value
> from function.
> 
> These issues about return value of failed config read requests, including
> special handling of CRS were reported by Lorenzo and Bjorn for Linux kernel
> driver pci-aardvark together with quotes from PCIe r4.0 spec, see details:
> https://lore.kernel.org/linux-pci/20210624213345.3617-1-pali@kernel.org/t/#u
> 
> Signed-off-by: Pali Rohár <pali at kernel.org>

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

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 52 +++++++++++++++++++-------------------
>   1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 1b9bae7cca72..815b26162f15 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -177,7 +177,6 @@
>   #define LINK_MAX_RETRIES			10
>   #define LINK_WAIT_TIMEOUT			100000
>   
> -#define CFG_RD_UR_VAL			0xFFFFFFFF
>   #define CFG_RD_CRS_VAL			0xFFFF0001
>   
>   /**
> @@ -263,12 +262,12 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>    * pcie_advk_check_pio_status() - Validate PIO status and get the read result
>    *
>    * @pcie: Pointer to the PCI bus
> - * @read: Read from or write to configuration space - true(read) false(write)
> - * @read_val: Pointer to the read result, only valid when read is true
> + * @allow_crs: Only for read requests, if CRS response is allowed
> + * @read_val: Pointer to the read result
>    *
>    */
>   static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
> -				      bool read,
> +				      bool allow_crs,
>   				      uint *read_val)
>   {
>   	uint reg;
> @@ -286,22 +285,16 @@ static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
>   			break;
>   		}
>   		/* Get the read result */
> -		if (read)
> +		if (read_val)
>   			*read_val = advk_readl(pcie, PIO_RD_DATA);
>   		/* No error */
>   		strcomp_status = NULL;
>   		break;
>   	case PIO_COMPLETION_STATUS_UR:
> -		if (read) {
> -			/* For reading, UR is not an error status. */
> -			*read_val = CFG_RD_UR_VAL;
> -			strcomp_status = NULL;
> -		} else {
> -			strcomp_status = "UR";
> -		}
> +		strcomp_status = "UR";
>   		break;
>   	case PIO_COMPLETION_STATUS_CRS:
> -		if (read) {
> +		if (allow_crs && read_val) {
>   			/* For reading, CRS is not an error status. */
>   			*read_val = CFG_RD_CRS_VAL;
>   			strcomp_status = NULL;
> @@ -352,6 +345,7 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   				 enum pci_size_t size)
>   {
>   	struct pcie_advk *pcie = dev_get_priv(bus);
> +	bool allow_crs;
>   	uint reg;
>   	int ret;
>   
> @@ -364,13 +358,17 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> +	allow_crs = (offset == PCI_VENDOR_ID) && (size == 4);
> +
>   	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;
> +		if (allow_crs) {
> +			*valuep = CFG_RD_CRS_VAL;
> +			return 0;
> +		}
> +		*valuep = pci_get_ff(size);
> +		return -EINVAL;
>   	}
>   
>   	/* Program the control register */
> @@ -392,16 +390,20 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	advk_writel(pcie, 1, PIO_START);
>   
>   	if (!pcie_advk_wait_pio(pcie)) {
> -		if (offset != PCI_VENDOR_ID)
> -			return -EINVAL;
> -		*valuep = CFG_RD_CRS_VAL;
> -		return 0;
> +		if (allow_crs) {
> +			*valuep = CFG_RD_CRS_VAL;
> +			return 0;
> +		}
> +		*valuep = pci_get_ff(size);
> +		return -EINVAL;
>   	}
>   
>   	/* Check PIO status and get the read result */
> -	ret = pcie_advk_check_pio_status(pcie, true, &reg);
> -	if (ret)
> +	ret = pcie_advk_check_pio_status(pcie, allow_crs, &reg);
> +	if (ret) {
> +		*valuep = pci_get_ff(size);
>   		return ret;
> +	}
>   
>   	dev_dbg(pcie->dev, "(addr,size,val)=(0x%04x, %d, 0x%08x)\n",
>   		offset, size, reg);
> @@ -511,9 +513,7 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/* Check PIO status */
> -	pcie_advk_check_pio_status(pcie, false, &reg);
> -
> -	return 0;
> +	return pcie_advk_check_pio_status(pcie, false, NULL);
>   }
>   
>   /**
> 


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