[RESEND PATCH 0/3] PCI: aardvark: PIO fixes

[RESEND PATCH 0/3] PCI: aardvark: PIO fixes

[Pali Rohár]

Per Lorenzo's request [1] I'm resending PCI aardvark patches from big
patch series [2] which fixes just one small thing - aardvark PIO code.

[1] - https://lore.kernel.org/linux-pci/20210603151605.GA18917@lpieralisi/
[2] - https://lore.kernel.org/linux-pci/20210506153153.30454-1-pali@kernel.org/

Evan Wang (1):
  PCI: aardvark: Fix checking for PIO status

Pali Rohár (2):
  PCI: aardvark: Fix checking for PIO Non-posted Request
  PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO
    response

 drivers/pci/controller/pci-aardvark.c | 97 ++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 15 deletions(-)

-- 
2.20.1

[RESEND PATCH 1/3] PCI: aardvark: Fix checking for PIO Non-posted Request

[Pali Rohár]

PIO_NON_POSTED_REQ for PIO_STAT register is incorrectly defined. Bit 10 in
register PIO_STAT indicates the response is to a non-posted request.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e3f5e7ab7606..2f8380a1f84f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -57,7 +57,7 @@
 #define   PIO_COMPLETION_STATUS_UR		1
 #define   PIO_COMPLETION_STATUS_CRS		2
 #define   PIO_COMPLETION_STATUS_CA		4
-#define   PIO_NON_POSTED_REQ			BIT(0)
+#define   PIO_NON_POSTED_REQ			BIT(10)
 #define PIO_ADDR_LS				(PIO_BASE_ADDR + 0x8)
 #define PIO_ADDR_MS				(PIO_BASE_ADDR + 0xc)
 #define PIO_WR_DATA				(PIO_BASE_ADDR + 0x10)
-- 
2.20.1

[RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Pali Rohár]

From: Evan Wang <xswang@marvell.com>

There is an issue that when PCIe switch is connected to an Armada 3700
board, there will be lots of warnings about PIO errors when reading the
config space. According to Aardvark PIO read and write sequence in HW
specification, the current way to check PIO status has the following
issues:

1) For PIO read operation, it reports the error message, which should be
   avoided according to HW specification.

2) For PIO read and write operations, it only checks PIO operation complete
   status, which is not enough, and error status should also be checked.

This patch aligns the code with Aardvark PIO read and write sequence in HW
specification on PIO status check and fix the warnings when reading config
space.

This patch also returns Completion Retry Status value when the read request
timeout, to give the caller a chance to send the request again instead of
failing.

Signed-off-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Victor Gu <xigu@marvell.com>
Tested-by: Victor Gu <xigu@marvell.com>
[pali: Return CRS also after timeout]
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # b1bd5714472c ("PCI: aardvark: Indicate error in 'val' when config read fails")
---
 drivers/pci/controller/pci-aardvark.c | 93 +++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2f8380a1f84f..a37ba86f1b2d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -58,6 +58,7 @@
 #define   PIO_COMPLETION_STATUS_CRS		2
 #define   PIO_COMPLETION_STATUS_CA		4
 #define   PIO_NON_POSTED_REQ			BIT(10)
+#define   PIO_ERR_STATUS			BIT(11)
 #define PIO_ADDR_LS				(PIO_BASE_ADDR + 0x8)
 #define PIO_ADDR_MS				(PIO_BASE_ADDR + 0xc)
 #define PIO_WR_DATA				(PIO_BASE_ADDR + 0x10)
@@ -176,6 +177,9 @@
 
 #define MSI_IRQ_NUM			32
 
+#define CFG_RD_UR_VAL			0xffffffff
+#define CFG_RD_CRS_VAL			0xffff0001
+
 struct advk_pcie {
 	struct platform_device *pdev;
 	void __iomem *base;
@@ -461,7 +465,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
 }
 
-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;
@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {
+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;
+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "UR";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		strcomp_status = "CRS";
+		if (val) {
+			/* For reading, CRS is not an error status */
+			*val = CFG_RD_CRS_VAL;
+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";
@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else
@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)
@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 
 	if (advk_pcie_pio_is_running(pcie)) {
-		*val = 0xffffffff;
-		return PCIBIOS_SET_FAILED;
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again insted of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */
@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)
@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }
-- 
2.20.1

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Lorenzo Pieralisi]

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

> -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
>  {
>  	struct device *dev = &pcie->pdev->dev;
>  	u32 reg;
> @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
>  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
>  		PIO_COMPLETION_STATUS_SHIFT;
>  
> -	if (!status)
> -		return;
> -
> +	/*
> +	 * According to HW spec, the PIO status check sequence as below:
> +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> +	 *    it still needs to check Error Status(bit11), only when this bit
> +	 *    indicates no error happen, the operation is successful.
> +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> +	 *    means a PIO write error, and for PIO read it is successful with
> +	 *    a read value of 0xFFFFFFFF.
> +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> +	 *    only means a PIO write error, and for PIO read it is successful
> +	 *    with a read value of 0xFFFF0001.
> +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> +	 *    error for both PIO read and PIO write operation.
> +	 * 5) other errors are indicated as 'unknown'.
> +	 */
>  	switch (status) {
> +	case PIO_COMPLETION_STATUS_OK:
> +		if (reg & PIO_ERR_STATUS) {
> +			strcomp_status = "COMP_ERR";
> +			break;
> +		}
> +		/* Get the read result */
> +		if (val)
> +			*val = advk_readl(pcie, PIO_RD_DATA);
> +		/* No error */
> +		strcomp_status = NULL;
> +		break;
>  	case PIO_COMPLETION_STATUS_UR:
> -		strcomp_status = "UR";
> +		if (val) {
> +			/* For reading, UR is not an error status */
> +			*val = CFG_RD_UR_VAL;
> +			strcomp_status = NULL;
> +		} else {
> +			strcomp_status = "UR";
> +		}
>  		break;
>  	case PIO_COMPLETION_STATUS_CRS:
> -		strcomp_status = "CRS";
> +		if (val) {
> +			/* For reading, CRS is not an error status */
> +			*val = CFG_RD_CRS_VAL;

Need Bjorn's input on this. I don't think this is what is expected from
from a root complex according to the PCI specifications (depending on
whether CSR software visibility is supported or not).

Here we are fabricating a CRS completion value for all PCI config read
transactions that are hitting a CRS completion status (and that's not
the expected behaviour according to the PCI specifications and I don't
think that's correct).

> +			strcomp_status = NULL;
> +		} else {
> +			strcomp_status = "CRS";
> +		}
>  		break;
>  	case PIO_COMPLETION_STATUS_CA:
>  		strcomp_status = "CA";
> @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
>  		break;
>  	}
>  
> +	if (!strcomp_status)
> +		return 0;
> +
>  	if (reg & PIO_NON_POSTED_REQ)
>  		str_posted = "Non-posted";
>  	else
> @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
>  
>  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
>  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> +
> +	return -EFAULT;
>  }
>  
>  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  						 size, val);
>  
>  	if (advk_pcie_pio_is_running(pcie)) {
> -		*val = 0xffffffff;
> -		return PCIBIOS_SET_FAILED;
> +		/*
> +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> +		 * so caller tries to issue the request again insted of failing
> +		 */
> +		if (where == PCI_VENDOR_ID) {
> +			*val = CFG_RD_CRS_VAL;
> +			return PCIBIOS_SUCCESSFUL;

Mmmm..here we are faking a CRS completion value to coerce the kernel
into believing a CRS completion was received (which is not necessarily
true) ?

if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

Lorenzo

> +		} else {
> +			*val = 0xffffffff;
> +			return PCIBIOS_SET_FAILED;
> +		}
>  	}
>  
>  	/* Program the control register */
> @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	advk_writel(pcie, 1, PIO_START);
>  
>  	ret = advk_pcie_wait_pio(pcie);
> +	if (ret < 0) {
> +		/*
> +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> +		 * so caller tries to issue the request again instead of failing
> +		 */
> +		if (where == PCI_VENDOR_ID) {
> +			*val = CFG_RD_CRS_VAL;
> +			return PCIBIOS_SUCCESSFUL;
> +		} else {
> +			*val = 0xffffffff;
> +			return PCIBIOS_SET_FAILED;
> +		}
> +	}
> +
> +	/* Check PIO status and get the read result */
> +	ret = advk_pcie_check_pio_status(pcie, val);
>  	if (ret < 0) {
>  		*val = 0xffffffff;
>  		return PCIBIOS_SET_FAILED;
>  	}
>  
> -	advk_pcie_check_pio_status(pcie);
> -
> -	/* Get the read result */
> -	*val = advk_readl(pcie, PIO_RD_DATA);
>  	if (size == 1)
>  		*val = (*val >> (8 * (where & 3))) & 0xff;
>  	else if (size == 2)
> @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	if (ret < 0)
>  		return PCIBIOS_SET_FAILED;
>  
> -	advk_pcie_check_pio_status(pcie);
> +	ret = advk_pcie_check_pio_status(pcie, NULL);
> +	if (ret < 0)
> +		return PCIBIOS_SET_FAILED;
>  
>  	return PCIBIOS_SUCCESSFUL;
>  }
> -- 
> 2.20.1
> 

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Pali Rohár]

On Friday 25 June 2021 12:04:29 Lorenzo Pieralisi wrote:
> On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
> 
> [...]
> 
> > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> >  {
> >  	struct device *dev = &pcie->pdev->dev;
> >  	u32 reg;
> > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> >  		PIO_COMPLETION_STATUS_SHIFT;
> >  
> > -	if (!status)
> > -		return;
> > -
> > +	/*
> > +	 * According to HW spec, the PIO status check sequence as below:
> > +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > +	 *    it still needs to check Error Status(bit11), only when this bit
> > +	 *    indicates no error happen, the operation is successful.
> > +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > +	 *    means a PIO write error, and for PIO read it is successful with
> > +	 *    a read value of 0xFFFFFFFF.
> > +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > +	 *    only means a PIO write error, and for PIO read it is successful
> > +	 *    with a read value of 0xFFFF0001.
> > +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > +	 *    error for both PIO read and PIO write operation.
> > +	 * 5) other errors are indicated as 'unknown'.
> > +	 */
> >  	switch (status) {
> > +	case PIO_COMPLETION_STATUS_OK:
> > +		if (reg & PIO_ERR_STATUS) {
> > +			strcomp_status = "COMP_ERR";
> > +			break;
> > +		}
> > +		/* Get the read result */
> > +		if (val)
> > +			*val = advk_readl(pcie, PIO_RD_DATA);
> > +		/* No error */
> > +		strcomp_status = NULL;
> > +		break;
> >  	case PIO_COMPLETION_STATUS_UR:
> > -		strcomp_status = "UR";
> > +		if (val) {
> > +			/* For reading, UR is not an error status */
> > +			*val = CFG_RD_UR_VAL;
> > +			strcomp_status = NULL;
> > +		} else {
> > +			strcomp_status = "UR";
> > +		}
> >  		break;
> >  	case PIO_COMPLETION_STATUS_CRS:
> > -		strcomp_status = "CRS";
> > +		if (val) {
> > +			/* For reading, CRS is not an error status */
> > +			*val = CFG_RD_CRS_VAL;
> 
> Need Bjorn's input on this.

Ok.

> I don't think this is what is expected from
> from a root complex according to the PCI specifications (depending on
> whether CSR software visibility is supported or not).

This patch / logic was written and reviewed by Marvell people as is
mentioned in commit description. But I was not able to get any feedback
from them about aardvark, so I have not put them into recipients of this
patch...

> Here we are fabricating a CRS completion value for all PCI config read
> transactions that are hitting a CRS completion status (and that's not
> the expected behaviour according to the PCI specifications and I don't
> think that's correct).

I see what what you mean. I think that for PCI_VENDOR_ID read request it
is correct. But question is what we should return for other read
requests.

> > +			strcomp_status = NULL;
> > +		} else {
> > +			strcomp_status = "CRS";
> > +		}
> >  		break;
> >  	case PIO_COMPLETION_STATUS_CA:
> >  		strcomp_status = "CA";
> > @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >  		break;
> >  	}
> >  
> > +	if (!strcomp_status)
> > +		return 0;
> > +
> >  	if (reg & PIO_NON_POSTED_REQ)
> >  		str_posted = "Non-posted";
> >  	else
> > @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >  
> >  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> >  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> > +
> > +	return -EFAULT;
> >  }
> >  
> >  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  						 size, val);
> >  
> >  	if (advk_pcie_pio_is_running(pcie)) {
> > -		*val = 0xffffffff;
> > -		return PCIBIOS_SET_FAILED;
> > +		/*
> > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > +		 * so caller tries to issue the request again insted of failing
> > +		 */
> > +		if (where == PCI_VENDOR_ID) {
> > +			*val = CFG_RD_CRS_VAL;
> > +			return PCIBIOS_SUCCESSFUL;
> 
> Mmmm..here we are faking a CRS completion value to coerce the kernel
> into believing a CRS completion was received (which is not necessarily
> true) ?

This part of patch was written by me. I chose to return "fake CRS" to
let kernel / software to issue a new PCI_VENDOR_ID read request again
after timeout. After some timeout previous PIO transfer should complete
and therefore advk_pcie_pio_is_running returns false.

> if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

No. It indicates that software (kernel) was impatient for previous
config read / write request and did not wait for previous completion. So
at the time when kernel tried to issue a new (this) config read request,
previous one was still running (advk_pcie_pio_is_running returned true)
and therefore driver was not able to issue a new config read request.

In patch 3/3 I increased wait timeout so this situation when
advk_pcie_pio_is_running returns true should not happen. Or rather to
say, I was not able to reproduce it anymore.

> Lorenzo
> 
> > +		} else {
> > +			*val = 0xffffffff;
> > +			return PCIBIOS_SET_FAILED;
> > +		}
> >  	}
> >  
> >  	/* Program the control register */
> > @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  	advk_writel(pcie, 1, PIO_START);
> >  
> >  	ret = advk_pcie_wait_pio(pcie);
> > +	if (ret < 0) {
> > +		/*
> > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > +		 * so caller tries to issue the request again instead of failing
> > +		 */
> > +		if (where == PCI_VENDOR_ID) {
> > +			*val = CFG_RD_CRS_VAL;
> > +			return PCIBIOS_SUCCESSFUL;
> > +		} else {
> > +			*val = 0xffffffff;
> > +			return PCIBIOS_SET_FAILED;
> > +		}
> > +	}
> > +
> > +	/* Check PIO status and get the read result */
> > +	ret = advk_pcie_check_pio_status(pcie, val);
> >  	if (ret < 0) {
> >  		*val = 0xffffffff;
> >  		return PCIBIOS_SET_FAILED;
> >  	}
> >  
> > -	advk_pcie_check_pio_status(pcie);
> > -
> > -	/* Get the read result */
> > -	*val = advk_readl(pcie, PIO_RD_DATA);
> >  	if (size == 1)
> >  		*val = (*val >> (8 * (where & 3))) & 0xff;
> >  	else if (size == 2)
> > @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> >  	if (ret < 0)
> >  		return PCIBIOS_SET_FAILED;
> >  
> > -	advk_pcie_check_pio_status(pcie);
> > +	ret = advk_pcie_check_pio_status(pcie, NULL);
> > +	if (ret < 0)
> > +		return PCIBIOS_SET_FAILED;
> >  
> >  	return PCIBIOS_SUCCESSFUL;
> >  }
> > -- 
> > 2.20.1
> > 

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Bjorn Helgaas]

On Fri, Jun 25, 2021 at 12:04:29PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
>
> [...]
> 
> > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> >  {
> >  	struct device *dev = &pcie->pdev->dev;
> >  	u32 reg;
> > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> >  		PIO_COMPLETION_STATUS_SHIFT;
> >  
> > -	if (!status)
> > -		return;
> > -
> > +	/*
> > +	 * According to HW spec, the PIO status check sequence as below:
> > +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > +	 *    it still needs to check Error Status(bit11), only when this bit
> > +	 *    indicates no error happen, the operation is successful.
> > +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > +	 *    means a PIO write error, and for PIO read it is successful with
> > +	 *    a read value of 0xFFFFFFFF.
> > +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > +	 *    only means a PIO write error, and for PIO read it is successful
> > +	 *    with a read value of 0xFFFF0001.
> > +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > +	 *    error for both PIO read and PIO write operation.
> > +	 * 5) other errors are indicated as 'unknown'.
> > +	 */
> >  	switch (status) {
> > +	case PIO_COMPLETION_STATUS_OK:
> > +		if (reg & PIO_ERR_STATUS) {
> > +			strcomp_status = "COMP_ERR";
> > +			break;
> > +		}
> > +		/* Get the read result */
> > +		if (val)
> > +			*val = advk_readl(pcie, PIO_RD_DATA);
> > +		/* No error */
> > +		strcomp_status = NULL;
> > +		break;
> >  	case PIO_COMPLETION_STATUS_UR:
> > -		strcomp_status = "UR";
> > +		if (val) {
> > +			/* For reading, UR is not an error status */
> > +			*val = CFG_RD_UR_VAL;

I think the comment is incorrect.  Unsupported Request *is* an error
status.  But most platforms log it and fabricate ~0 data
(CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're
doing here.  So I think the code is fine, but the "not an error
status" comment is wrong.

Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the
hardware should be setting the "Unsupported Request Detected" bit in
the Device Status register when this occurs.

> > +			strcomp_status = NULL;
> > +		} else {
> > +			strcomp_status = "UR";
> > +		}
> >  		break;
> >  	case PIO_COMPLETION_STATUS_CRS:
> > -		strcomp_status = "CRS";
> > +		if (val) {
> > +			/* For reading, CRS is not an error status */
> > +			*val = CFG_RD_CRS_VAL;
> 
> Need Bjorn's input on this. I don't think this is what is expected from
> from a root complex according to the PCI specifications (depending on
> whether CSR software visibility is supported or not).
> 
> Here we are fabricating a CRS completion value for all PCI config read
> transactions that are hitting a CRS completion status (and that's not
> the expected behaviour according to the PCI specifications and I don't
> think that's correct).

Right.  I think any config access (read or write) can be completed
with a CRS completion (sec 2.3.1).

Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is
enabled and a config read that includes both bytes of the Vendor ID
receives a CRS completion, we must return 0x0001 for the Vendor ID and
0xff for any additional bytes.  Note that a config read of only the
two Vendor ID bytes is legal and should receive 0x0001 data.

But if CRS SV is disabled, I think config reads that receive CRS
completions should fail the normal way, i.e., fabricate ~0 data.

> > +			strcomp_status = NULL;
> > +		} else {
> > +			strcomp_status = "CRS";
> > +		}
> >  		break;
> >  	case PIO_COMPLETION_STATUS_CA:
> >  		strcomp_status = "CA";
> > @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >  		break;
> >  	}
> >  
> > +	if (!strcomp_status)
> > +		return 0;
> > +
> >  	if (reg & PIO_NON_POSTED_REQ)
> >  		str_posted = "Non-posted";
> >  	else
> > @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >  
> >  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> >  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> > +
> > +	return -EFAULT;
> >  }
> >  
> >  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  						 size, val);
> >  
> >  	if (advk_pcie_pio_is_running(pcie)) {
> > -		*val = 0xffffffff;
> > -		return PCIBIOS_SET_FAILED;
> > +		/*
> > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > +		 * so caller tries to issue the request again insted of failing
> > +		 */
> > +		if (where == PCI_VENDOR_ID) {
> > +			*val = CFG_RD_CRS_VAL;
> > +			return PCIBIOS_SUCCESSFUL;
> 
> Mmmm..here we are faking a CRS completion value to coerce the kernel
> into believing a CRS completion was received (which is not necessarily
> true) ?
> 
> if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?
> 
> Lorenzo
> 
> > +		} else {
> > +			*val = 0xffffffff;
> > +			return PCIBIOS_SET_FAILED;
> > +		}
> >  	}
> >  
> >  	/* Program the control register */
> > @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  	advk_writel(pcie, 1, PIO_START);
> >  
> >  	ret = advk_pcie_wait_pio(pcie);
> > +	if (ret < 0) {
> > +		/*
> > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > +		 * so caller tries to issue the request again instead of failing
> > +		 */
> > +		if (where == PCI_VENDOR_ID) {
> > +			*val = CFG_RD_CRS_VAL;
> > +			return PCIBIOS_SUCCESSFUL;
> > +		} else {
> > +			*val = 0xffffffff;
> > +			return PCIBIOS_SET_FAILED;
> > +		}
> > +	}
> > +
> > +	/* Check PIO status and get the read result */
> > +	ret = advk_pcie_check_pio_status(pcie, val);
> >  	if (ret < 0) {
> >  		*val = 0xffffffff;
> >  		return PCIBIOS_SET_FAILED;
> >  	}
> >  
> > -	advk_pcie_check_pio_status(pcie);
> > -
> > -	/* Get the read result */
> > -	*val = advk_readl(pcie, PIO_RD_DATA);
> >  	if (size == 1)
> >  		*val = (*val >> (8 * (where & 3))) & 0xff;
> >  	else if (size == 2)
> > @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> >  	if (ret < 0)
> >  		return PCIBIOS_SET_FAILED;
> >  
> > -	advk_pcie_check_pio_status(pcie);
> > +	ret = advk_pcie_check_pio_status(pcie, NULL);
> > +	if (ret < 0)
> > +		return PCIBIOS_SET_FAILED;
> >  
> >  	return PCIBIOS_SUCCESSFUL;
> >  }
> > -- 
> > 2.20.1
> > 

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Pali Rohár]

On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:
> On Fri, Jun 25, 2021 at 12:04:29PM +0100, Lorenzo Pieralisi wrote:
> > On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
> >
> > [...]
> > 
> > > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> > >  {
> > >  	struct device *dev = &pcie->pdev->dev;
> > >  	u32 reg;
> > > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> > >  		PIO_COMPLETION_STATUS_SHIFT;
> > >  
> > > -	if (!status)
> > > -		return;
> > > -
> > > +	/*
> > > +	 * According to HW spec, the PIO status check sequence as below:
> > > +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > > +	 *    it still needs to check Error Status(bit11), only when this bit
> > > +	 *    indicates no error happen, the operation is successful.
> > > +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > > +	 *    means a PIO write error, and for PIO read it is successful with
> > > +	 *    a read value of 0xFFFFFFFF.
> > > +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > > +	 *    only means a PIO write error, and for PIO read it is successful
> > > +	 *    with a read value of 0xFFFF0001.
> > > +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > > +	 *    error for both PIO read and PIO write operation.
> > > +	 * 5) other errors are indicated as 'unknown'.
> > > +	 */
> > >  	switch (status) {
> > > +	case PIO_COMPLETION_STATUS_OK:
> > > +		if (reg & PIO_ERR_STATUS) {
> > > +			strcomp_status = "COMP_ERR";
> > > +			break;
> > > +		}
> > > +		/* Get the read result */
> > > +		if (val)
> > > +			*val = advk_readl(pcie, PIO_RD_DATA);
> > > +		/* No error */
> > > +		strcomp_status = NULL;
> > > +		break;
> > >  	case PIO_COMPLETION_STATUS_UR:
> > > -		strcomp_status = "UR";
> > > +		if (val) {
> > > +			/* For reading, UR is not an error status */
> > > +			*val = CFG_RD_UR_VAL;
> 
> I think the comment is incorrect.  Unsupported Request *is* an error
> status.  But most platforms log it and fabricate ~0 data
> (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're
> doing here.  So I think the code is fine, but the "not an error
> status" comment is wrong.

Ok, and what we should driver set as return value for pci_ops.read
callback in this case?

> Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the
> hardware should be setting the "Unsupported Request Detected" bit in
> the Device Status register when this occurs.

Yes there is register in kernel's emulated PCIe bridge which at bit 19
has: Unsupported Request Detected - The core sets this bit to 1 when an
unsupported request is received. Write this bit to 1 to clear.

> > > +			strcomp_status = NULL;
> > > +		} else {
> > > +			strcomp_status = "UR";
> > > +		}
> > >  		break;
> > >  	case PIO_COMPLETION_STATUS_CRS:
> > > -		strcomp_status = "CRS";
> > > +		if (val) {
> > > +			/* For reading, CRS is not an error status */
> > > +			*val = CFG_RD_CRS_VAL;
> > 
> > Need Bjorn's input on this. I don't think this is what is expected from
> > from a root complex according to the PCI specifications (depending on
> > whether CSR software visibility is supported or not).
> > 
> > Here we are fabricating a CRS completion value for all PCI config read
> > transactions that are hitting a CRS completion status (and that's not
> > the expected behaviour according to the PCI specifications and I don't
> > think that's correct).
> 
> Right.  I think any config access (read or write) can be completed
> with a CRS completion (sec 2.3.1).
> 
> Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is
> enabled and a config read that includes both bytes of the Vendor ID
> receives a CRS completion, we must return 0x0001 for the Vendor ID and
> 0xff for any additional bytes.  Note that a config read of only the
> two Vendor ID bytes is legal and should receive 0x0001 data.
> 
> But if CRS SV is disabled, I think config reads that receive CRS
> completions should fail the normal way, i.e., fabricate ~0 data.

In PCIe base 2.0 is:

For other Configuration Requests, or when CRS Software Visibility is not
enabled, the Root Complex will generally re-issue the Configuration
Request until it completes with a status other than CRS as described in
Section 2.3.2.

So what should pci-aardvark driver in this case do? Return ~0 or re-send
this config read request (and how many times)?

Also this relates to previous discussion about PCI_EXP_RTCTL_CRSSVE:
https://lore.kernel.org/linux-pci/20210507152542.sd54lk7bk56qapf3@pali/

> > > +			strcomp_status = NULL;
> > > +		} else {
> > > +			strcomp_status = "CRS";
> > > +		}
> > >  		break;
> > >  	case PIO_COMPLETION_STATUS_CA:
> > >  		strcomp_status = "CA";
> > > @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > >  		break;
> > >  	}
> > >  
> > > +	if (!strcomp_status)
> > > +		return 0;
> > > +
> > >  	if (reg & PIO_NON_POSTED_REQ)
> > >  		str_posted = "Non-posted";
> > >  	else
> > > @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > >  
> > >  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> > >  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> > > +
> > > +	return -EFAULT;
> > >  }
> > >  
> > >  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > > @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > >  						 size, val);
> > >  
> > >  	if (advk_pcie_pio_is_running(pcie)) {
> > > -		*val = 0xffffffff;
> > > -		return PCIBIOS_SET_FAILED;
> > > +		/*
> > > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > > +		 * so caller tries to issue the request again insted of failing
> > > +		 */
> > > +		if (where == PCI_VENDOR_ID) {
> > > +			*val = CFG_RD_CRS_VAL;
> > > +			return PCIBIOS_SUCCESSFUL;
> > 
> > Mmmm..here we are faking a CRS completion value to coerce the kernel
> > into believing a CRS completion was received (which is not necessarily
> > true) ?
> > 
> > if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?
> > 
> > Lorenzo
> > 
> > > +		} else {
> > > +			*val = 0xffffffff;
> > > +			return PCIBIOS_SET_FAILED;
> > > +		}
> > >  	}
> > >  
> > >  	/* Program the control register */
> > > @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > >  	advk_writel(pcie, 1, PIO_START);
> > >  
> > >  	ret = advk_pcie_wait_pio(pcie);
> > > +	if (ret < 0) {
> > > +		/*
> > > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > > +		 * so caller tries to issue the request again instead of failing
> > > +		 */
> > > +		if (where == PCI_VENDOR_ID) {
> > > +			*val = CFG_RD_CRS_VAL;
> > > +			return PCIBIOS_SUCCESSFUL;
> > > +		} else {
> > > +			*val = 0xffffffff;
> > > +			return PCIBIOS_SET_FAILED;
> > > +		}
> > > +	}
> > > +
> > > +	/* Check PIO status and get the read result */
> > > +	ret = advk_pcie_check_pio_status(pcie, val);
> > >  	if (ret < 0) {
> > >  		*val = 0xffffffff;
> > >  		return PCIBIOS_SET_FAILED;
> > >  	}
> > >  
> > > -	advk_pcie_check_pio_status(pcie);
> > > -
> > > -	/* Get the read result */
> > > -	*val = advk_readl(pcie, PIO_RD_DATA);
> > >  	if (size == 1)
> > >  		*val = (*val >> (8 * (where & 3))) & 0xff;
> > >  	else if (size == 2)
> > > @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> > >  	if (ret < 0)
> > >  		return PCIBIOS_SET_FAILED;
> > >  
> > > -	advk_pcie_check_pio_status(pcie);
> > > +	ret = advk_pcie_check_pio_status(pcie, NULL);
> > > +	if (ret < 0)
> > > +		return PCIBIOS_SET_FAILED;
> > >  
> > >  	return PCIBIOS_SUCCESSFUL;
> > >  }
> > > -- 
> > > 2.20.1
> > > 

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Bjorn Helgaas]

On Tue, Jul 20, 2021 at 04:49:55PM +0200, Pali Rohár wrote:
> On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:
> > On Fri, Jun 25, 2021 at 12:04:29PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
> > >
> > > [...]
> > > 
> > > > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> > > >  {
> > > >  	struct device *dev = &pcie->pdev->dev;
> > > >  	u32 reg;
> > > > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> > > >  		PIO_COMPLETION_STATUS_SHIFT;
> > > >  
> > > > -	if (!status)
> > > > -		return;
> > > > -
> > > > +	/*
> > > > +	 * According to HW spec, the PIO status check sequence as below:
> > > > +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > > > +	 *    it still needs to check Error Status(bit11), only when this bit
> > > > +	 *    indicates no error happen, the operation is successful.
> > > > +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > > > +	 *    means a PIO write error, and for PIO read it is successful with
> > > > +	 *    a read value of 0xFFFFFFFF.
> > > > +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > > > +	 *    only means a PIO write error, and for PIO read it is successful
> > > > +	 *    with a read value of 0xFFFF0001.
> > > > +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > > > +	 *    error for both PIO read and PIO write operation.
> > > > +	 * 5) other errors are indicated as 'unknown'.
> > > > +	 */
> > > >  	switch (status) {
> > > > +	case PIO_COMPLETION_STATUS_OK:
> > > > +		if (reg & PIO_ERR_STATUS) {
> > > > +			strcomp_status = "COMP_ERR";
> > > > +			break;
> > > > +		}
> > > > +		/* Get the read result */
> > > > +		if (val)
> > > > +			*val = advk_readl(pcie, PIO_RD_DATA);
> > > > +		/* No error */
> > > > +		strcomp_status = NULL;
> > > > +		break;
> > > >  	case PIO_COMPLETION_STATUS_UR:
> > > > -		strcomp_status = "UR";
> > > > +		if (val) {
> > > > +			/* For reading, UR is not an error status */
> > > > +			*val = CFG_RD_UR_VAL;
> > 
> > I think the comment is incorrect.  Unsupported Request *is* an error
> > status.  But most platforms log it and fabricate ~0 data
> > (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're
> > doing here.  So I think the code is fine, but the "not an error
> > status" comment is wrong.
> 
> Ok, and what we should driver set as return value for pci_ops.read
> callback in this case?

On most platforms, pci_ops.read() does not check for failure, so it
returns PCIBIOS_SUCCESSFUL in this case.

> > Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the
> > hardware should be setting the "Unsupported Request Detected" bit in
> > the Device Status register when this occurs.
> 
> Yes there is register in kernel's emulated PCIe bridge which at bit 19
> has: Unsupported Request Detected - The core sets this bit to 1 when an
> unsupported request is received. Write this bit to 1 to clear.

I guess this bit 19 is the same as bit 3 in the 16-bit Device Status
register?  Bit 19 if you look at Device Control + Device Status as a
single 32-bit register?

> > > > +			strcomp_status = NULL;
> > > > +		} else {
> > > > +			strcomp_status = "UR";
> > > > +		}
> > > >  		break;
> > > >  	case PIO_COMPLETION_STATUS_CRS:
> > > > -		strcomp_status = "CRS";
> > > > +		if (val) {
> > > > +			/* For reading, CRS is not an error status */
> > > > +			*val = CFG_RD_CRS_VAL;
> > > 
> > > Need Bjorn's input on this. I don't think this is what is expected from
> > > from a root complex according to the PCI specifications (depending on
> > > whether CSR software visibility is supported or not).
> > > 
> > > Here we are fabricating a CRS completion value for all PCI config read
> > > transactions that are hitting a CRS completion status (and that's not
> > > the expected behaviour according to the PCI specifications and I don't
> > > think that's correct).
> > 
> > Right.  I think any config access (read or write) can be completed
> > with a CRS completion (sec 2.3.1).
> > 
> > Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is
> > enabled and a config read that includes both bytes of the Vendor ID
> > receives a CRS completion, we must return 0x0001 for the Vendor ID and
> > 0xff for any additional bytes.  Note that a config read of only the
> > two Vendor ID bytes is legal and should receive 0x0001 data.
> > 
> > But if CRS SV is disabled, I think config reads that receive CRS
> > completions should fail the normal way, i.e., fabricate ~0 data.
> 
> In PCIe base 2.0 is:
> 
> For other Configuration Requests, or when CRS Software Visibility is not
> enabled, the Root Complex will generally re-issue the Configuration
> Request until it completes with a status other than CRS as described in
> Section 2.3.2.

That text is from the "Configuration Request Retry Status"
implementation note in PCIe r2.0, sec 2.3.1, "Request Handling Rules",
and PCIe r5.0 contains the same text.

PCIe r4.0, sec 2.3.2, says (r5.0 contains the same text but with a
formatting error that changes the meaning):

  Root Complex handling of a Completion with Configuration Request
  Retry Status for a Configuration Request is implementation specific,
  except for the period following system reset (see Section 6.6). For
  Root Complexes that support CRS Software Visibility, the following
  rules apply:

    * If CRS Software Visibility is not enabled, the Root Complex must
      re-issue the Configuration Request as a new Request.

    * If CRS Software Visibility is enabled (see below):

      - For a Configuration Read Request that includes both bytes of
	the Vendor ID field of a device Function's Configuration Space
	Header, the Root Complex must complete the Request to the host
	by returning a read-data value of 0001h for the Vendor ID
	field and all ‘1’s for any additional bytes included in the
	request. This read-data value has been reserved specifically
	for this use by the PCI-SIG and does not correspond to any
	assigned Vendor ID.

      - For a Configuration Write Request or for any other
	Configuration Read Request, the Root Complex must re-issue the
	Configuration Request as a new Request.

  A Root Complex implementation may choose to limit the number of
  Configuration Request/CRS Completion Status loops before determining
  that something is wrong with the target of the Request and taking
  appropriate action, e.g., complete the Request to the host as a
  failed transaction.

> So what should pci-aardvark driver in this case do? Return ~0 or re-send
> this config read request (and how many times)?

That's a good question.  I don't know what other hardware
implementations do, but I doubt they retry forever.  Since the spec
doesn't specify a number of retries, I think you can choose to do
none and immediately return ~0.

> Also this relates to previous discussion about PCI_EXP_RTCTL_CRSSVE:
> https://lore.kernel.org/linux-pci/20210507152542.sd54lk7bk56qapf3@pali/
> 
> > > > +			strcomp_status = NULL;
> > > > +		} else {
> > > > +			strcomp_status = "CRS";
> > > > +		}
> > > >  		break;
> > > >  	case PIO_COMPLETION_STATUS_CA:
> > > >  		strcomp_status = "CA";
> > > > @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > >  		break;
> > > >  	}
> > > >  
> > > > +	if (!strcomp_status)
> > > > +		return 0;
> > > > +
> > > >  	if (reg & PIO_NON_POSTED_REQ)
> > > >  		str_posted = "Non-posted";
> > > >  	else
> > > > @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > >  
> > > >  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> > > >  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> > > > +
> > > > +	return -EFAULT;
> > > >  }
> > > >  
> > > >  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > > > @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > >  						 size, val);
> > > >  
> > > >  	if (advk_pcie_pio_is_running(pcie)) {
> > > > -		*val = 0xffffffff;
> > > > -		return PCIBIOS_SET_FAILED;
> > > > +		/*
> > > > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > > > +		 * so caller tries to issue the request again insted of failing
> > > > +		 */
> > > > +		if (where == PCI_VENDOR_ID) {
> > > > +			*val = CFG_RD_CRS_VAL;
> > > > +			return PCIBIOS_SUCCESSFUL;
> > > 
> > > Mmmm..here we are faking a CRS completion value to coerce the kernel
> > > into believing a CRS completion was received (which is not necessarily
> > > true) ?
> > > 
> > > if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?
> > > 
> > > Lorenzo
> > > 
> > > > +		} else {
> > > > +			*val = 0xffffffff;
> > > > +			return PCIBIOS_SET_FAILED;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	/* Program the control register */
> > > > @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > >  	advk_writel(pcie, 1, PIO_START);
> > > >  
> > > >  	ret = advk_pcie_wait_pio(pcie);
> > > > +	if (ret < 0) {
> > > > +		/*
> > > > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > > > +		 * so caller tries to issue the request again instead of failing
> > > > +		 */
> > > > +		if (where == PCI_VENDOR_ID) {
> > > > +			*val = CFG_RD_CRS_VAL;
> > > > +			return PCIBIOS_SUCCESSFUL;
> > > > +		} else {
> > > > +			*val = 0xffffffff;
> > > > +			return PCIBIOS_SET_FAILED;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/* Check PIO status and get the read result */
> > > > +	ret = advk_pcie_check_pio_status(pcie, val);
> > > >  	if (ret < 0) {
> > > >  		*val = 0xffffffff;
> > > >  		return PCIBIOS_SET_FAILED;
> > > >  	}
> > > >  
> > > > -	advk_pcie_check_pio_status(pcie);
> > > > -
> > > > -	/* Get the read result */
> > > > -	*val = advk_readl(pcie, PIO_RD_DATA);
> > > >  	if (size == 1)
> > > >  		*val = (*val >> (8 * (where & 3))) & 0xff;
> > > >  	else if (size == 2)
> > > > @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> > > >  	if (ret < 0)
> > > >  		return PCIBIOS_SET_FAILED;
> > > >  
> > > > -	advk_pcie_check_pio_status(pcie);
> > > > +	ret = advk_pcie_check_pio_status(pcie, NULL);
> > > > +	if (ret < 0)
> > > > +		return PCIBIOS_SET_FAILED;
> > > >  
> > > >  	return PCIBIOS_SUCCESSFUL;
> > > >  }
> > > > -- 
> > > > 2.20.1
> > > > 

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Pali Rohár]

On Tuesday 20 July 2021 11:34:51 Bjorn Helgaas wrote:
> On Tue, Jul 20, 2021 at 04:49:55PM +0200, Pali Rohár wrote:
> > On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:
> > > On Fri, Jun 25, 2021 at 12:04:29PM +0100, Lorenzo Pieralisi wrote:
> > > > On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
> > > >
> > > > [...]
> > > > 
> > > > > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> > > > >  {
> > > > >  	struct device *dev = &pcie->pdev->dev;
> > > > >  	u32 reg;
> > > > > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> > > > >  		PIO_COMPLETION_STATUS_SHIFT;
> > > > >  
> > > > > -	if (!status)
> > > > > -		return;
> > > > > -
> > > > > +	/*
> > > > > +	 * According to HW spec, the PIO status check sequence as below:
> > > > > +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > > > > +	 *    it still needs to check Error Status(bit11), only when this bit
> > > > > +	 *    indicates no error happen, the operation is successful.
> > > > > +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > > > > +	 *    means a PIO write error, and for PIO read it is successful with
> > > > > +	 *    a read value of 0xFFFFFFFF.
> > > > > +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > > > > +	 *    only means a PIO write error, and for PIO read it is successful
> > > > > +	 *    with a read value of 0xFFFF0001.
> > > > > +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > > > > +	 *    error for both PIO read and PIO write operation.
> > > > > +	 * 5) other errors are indicated as 'unknown'.
> > > > > +	 */
> > > > >  	switch (status) {
> > > > > +	case PIO_COMPLETION_STATUS_OK:
> > > > > +		if (reg & PIO_ERR_STATUS) {
> > > > > +			strcomp_status = "COMP_ERR";
> > > > > +			break;
> > > > > +		}
> > > > > +		/* Get the read result */
> > > > > +		if (val)
> > > > > +			*val = advk_readl(pcie, PIO_RD_DATA);
> > > > > +		/* No error */
> > > > > +		strcomp_status = NULL;
> > > > > +		break;
> > > > >  	case PIO_COMPLETION_STATUS_UR:
> > > > > -		strcomp_status = "UR";
> > > > > +		if (val) {
> > > > > +			/* For reading, UR is not an error status */
> > > > > +			*val = CFG_RD_UR_VAL;
> > > 
> > > I think the comment is incorrect.  Unsupported Request *is* an error
> > > status.  But most platforms log it and fabricate ~0 data
> > > (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're
> > > doing here.  So I think the code is fine, but the "not an error
> > > status" comment is wrong.
> > 
> > Ok, and what we should driver set as return value for pci_ops.read
> > callback in this case?
> 
> On most platforms, pci_ops.read() does not check for failure, so it
> returns PCIBIOS_SUCCESSFUL in this case.

Ok. Most platforms do not check for failure. But it is generally
correct? Probably more platforms do not provide error flag and only
return value. But aardvark hw provides this kind error information, so
should pci-aardvark's pci_ops.read() on error returns PCIBIOS_SUCCESSFUL
on some other return value?

> > > Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the
> > > hardware should be setting the "Unsupported Request Detected" bit in
> > > the Device Status register when this occurs.
> > 
> > Yes there is register in kernel's emulated PCIe bridge which at bit 19
> > has: Unsupported Request Detected - The core sets this bit to 1 when an
> > unsupported request is received. Write this bit to 1 to clear.
> 
> I guess this bit 19 is the same as bit 3 in the 16-bit Device Status
> register?  Bit 19 if you look at Device Control + Device Status as a
> single 32-bit register?

Yes. Access to (emulated) registers is possible only via 4-byte aligned
read / write operations. So this bit 19 is bit 3 in Device Status reg.

> > > > > +			strcomp_status = NULL;
> > > > > +		} else {
> > > > > +			strcomp_status = "UR";
> > > > > +		}
> > > > >  		break;
> > > > >  	case PIO_COMPLETION_STATUS_CRS:
> > > > > -		strcomp_status = "CRS";
> > > > > +		if (val) {
> > > > > +			/* For reading, CRS is not an error status */
> > > > > +			*val = CFG_RD_CRS_VAL;
> > > > 
> > > > Need Bjorn's input on this. I don't think this is what is expected from
> > > > from a root complex according to the PCI specifications (depending on
> > > > whether CSR software visibility is supported or not).
> > > > 
> > > > Here we are fabricating a CRS completion value for all PCI config read
> > > > transactions that are hitting a CRS completion status (and that's not
> > > > the expected behaviour according to the PCI specifications and I don't
> > > > think that's correct).
> > > 
> > > Right.  I think any config access (read or write) can be completed
> > > with a CRS completion (sec 2.3.1).
> > > 
> > > Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is
> > > enabled and a config read that includes both bytes of the Vendor ID
> > > receives a CRS completion, we must return 0x0001 for the Vendor ID and
> > > 0xff for any additional bytes.  Note that a config read of only the
> > > two Vendor ID bytes is legal and should receive 0x0001 data.
> > > 
> > > But if CRS SV is disabled, I think config reads that receive CRS
> > > completions should fail the normal way, i.e., fabricate ~0 data.
> > 
> > In PCIe base 2.0 is:
> > 
> > For other Configuration Requests, or when CRS Software Visibility is not
> > enabled, the Root Complex will generally re-issue the Configuration
> > Request until it completes with a status other than CRS as described in
> > Section 2.3.2.
> 
> That text is from the "Configuration Request Retry Status"
> implementation note in PCIe r2.0, sec 2.3.1, "Request Handling Rules",
> and PCIe r5.0 contains the same text.
> 
> PCIe r4.0, sec 2.3.2, says (r5.0 contains the same text but with a
> formatting error that changes the meaning):
> 
>   Root Complex handling of a Completion with Configuration Request
>   Retry Status for a Configuration Request is implementation specific,
>   except for the period following system reset (see Section 6.6). For
>   Root Complexes that support CRS Software Visibility, the following
>   rules apply:
> 
>     * If CRS Software Visibility is not enabled, the Root Complex must
>       re-issue the Configuration Request as a new Request.
> 
>     * If CRS Software Visibility is enabled (see below):
> 
>       - For a Configuration Read Request that includes both bytes of
> 	the Vendor ID field of a device Function's Configuration Space
> 	Header, the Root Complex must complete the Request to the host
> 	by returning a read-data value of 0001h for the Vendor ID
> 	field and all ‘1’s for any additional bytes included in the
> 	request. This read-data value has been reserved specifically
> 	for this use by the PCI-SIG and does not correspond to any
> 	assigned Vendor ID.
> 
>       - For a Configuration Write Request or for any other
> 	Configuration Read Request, the Root Complex must re-issue the
> 	Configuration Request as a new Request.
> 
>   A Root Complex implementation may choose to limit the number of
>   Configuration Request/CRS Completion Status loops before determining
>   that something is wrong with the target of the Request and taking
>   appropriate action, e.g., complete the Request to the host as a
>   failed transaction.
> 
> > So what should pci-aardvark driver in this case do? Return ~0 or re-send
> > this config read request (and how many times)?
> 
> That's a good question.  I don't know what other hardware
> implementations do, but I doubt they retry forever.  Since the spec
> doesn't specify a number of retries, I think you can choose to do
> none and immediately return ~0.

Ok, so I will change implementation to return ~0 without any retry. This
is simple and easy implementation. Anyway, it would be nice to know what
other real-hardware implementations of PCIe controllers are doing.

PCI_EXP_RTCTL_CRSSVE bit is only part of kernel emulated PCIe Bridge and
has no real register in aardvark hw.

So for me it looks like that aardvark's pci_ops.read() should set read
value to ~0 or 0xffff0001 based on what is stored in kernel emulated
PCI_EXP_RTCTL_CRSSVE bit. Right?

> > Also this relates to previous discussion about PCI_EXP_RTCTL_CRSSVE:
> > https://lore.kernel.org/linux-pci/20210507152542.sd54lk7bk56qapf3@pali/
> > 
> > > > > +			strcomp_status = NULL;
> > > > > +		} else {
> > > > > +			strcomp_status = "CRS";
> > > > > +		}
> > > > >  		break;
> > > > >  	case PIO_COMPLETION_STATUS_CA:
> > > > >  		strcomp_status = "CA";
> > > > > @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > >  		break;
> > > > >  	}
> > > > >  
> > > > > +	if (!strcomp_status)
> > > > > +		return 0;
> > > > > +
> > > > >  	if (reg & PIO_NON_POSTED_REQ)
> > > > >  		str_posted = "Non-posted";
> > > > >  	else
> > > > > @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > >  
> > > > >  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> > > > >  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> > > > > +
> > > > > +	return -EFAULT;
> > > > >  }
> > > > >  
> > > > >  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > > > > @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > > >  						 size, val);
> > > > >  
> > > > >  	if (advk_pcie_pio_is_running(pcie)) {
> > > > > -		*val = 0xffffffff;
> > > > > -		return PCIBIOS_SET_FAILED;
> > > > > +		/*
> > > > > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > > > > +		 * so caller tries to issue the request again insted of failing
> > > > > +		 */
> > > > > +		if (where == PCI_VENDOR_ID) {
> > > > > +			*val = CFG_RD_CRS_VAL;
> > > > > +			return PCIBIOS_SUCCESSFUL;
> > > > 
> > > > Mmmm..here we are faking a CRS completion value to coerce the kernel
> > > > into believing a CRS completion was received (which is not necessarily
> > > > true) ?
> > > > 
> > > > if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?
> > > > 
> > > > Lorenzo
> > > > 
> > > > > +		} else {
> > > > > +			*val = 0xffffffff;
> > > > > +			return PCIBIOS_SET_FAILED;
> > > > > +		}
> > > > >  	}
> > > > >  
> > > > >  	/* Program the control register */
> > > > > @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > > >  	advk_writel(pcie, 1, PIO_START);
> > > > >  
> > > > >  	ret = advk_pcie_wait_pio(pcie);
> > > > > +	if (ret < 0) {
> > > > > +		/*
> > > > > +		 * For PCI_VENDOR_ID register, return Completion Retry Status
> > > > > +		 * so caller tries to issue the request again instead of failing
> > > > > +		 */
> > > > > +		if (where == PCI_VENDOR_ID) {
> > > > > +			*val = CFG_RD_CRS_VAL;
> > > > > +			return PCIBIOS_SUCCESSFUL;
> > > > > +		} else {
> > > > > +			*val = 0xffffffff;
> > > > > +			return PCIBIOS_SET_FAILED;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	/* Check PIO status and get the read result */
> > > > > +	ret = advk_pcie_check_pio_status(pcie, val);
> > > > >  	if (ret < 0) {
> > > > >  		*val = 0xffffffff;
> > > > >  		return PCIBIOS_SET_FAILED;
> > > > >  	}
> > > > >  
> > > > > -	advk_pcie_check_pio_status(pcie);
> > > > > -
> > > > > -	/* Get the read result */
> > > > > -	*val = advk_readl(pcie, PIO_RD_DATA);
> > > > >  	if (size == 1)
> > > > >  		*val = (*val >> (8 * (where & 3))) & 0xff;
> > > > >  	else if (size == 2)
> > > > > @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> > > > >  	if (ret < 0)
> > > > >  		return PCIBIOS_SET_FAILED;
> > > > >  
> > > > > -	advk_pcie_check_pio_status(pcie);
> > > > > +	ret = advk_pcie_check_pio_status(pcie, NULL);
> > > > > +	if (ret < 0)
> > > > > +		return PCIBIOS_SET_FAILED;
> > > > >  
> > > > >  	return PCIBIOS_SUCCESSFUL;
> > > > >  }
> > > > > -- 
> > > > > 2.20.1
> > > > > 

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Bjorn Helgaas]

On Tue, Jul 20, 2021 at 08:11:55PM +0200, Pali Rohár wrote:
> On Tuesday 20 July 2021 11:34:51 Bjorn Helgaas wrote:
> > On Tue, Jul 20, 2021 at 04:49:55PM +0200, Pali Rohár wrote:

> > > So what should pci-aardvark driver in this case do? Return ~0 or re-send
> > > this config read request (and how many times)?
> > 
> > That's a good question.  I don't know what other hardware
> > implementations do, but I doubt they retry forever.  Since the spec
> > doesn't specify a number of retries, I think you can choose to do
> > none and immediately return ~0.
> 
> Ok, so I will change implementation to return ~0 without any retry. This
> is simple and easy implementation. Anyway, it would be nice to know what
> other real-hardware implementations of PCIe controllers are doing.
> 
> PCI_EXP_RTCTL_CRSSVE bit is only part of kernel emulated PCIe Bridge and
> has no real register in aardvark hw.
> 
> So for me it looks like that aardvark's pci_ops.read() should set read
> value to ~0 or 0xffff0001 based on what is stored in kernel emulated
> PCI_EXP_RTCTL_CRSSVE bit. Right?

Yes, that would be my advice.  Of course it's only if we're reading
the Vendor ID *and* PCI_EXP_RTCTL_CRSSVE is set.

Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status

[Bjorn Helgaas]

On Tue, Jul 20, 2021 at 08:11:55PM +0200, Pali Rohár wrote:
> On Tuesday 20 July 2021 11:34:51 Bjorn Helgaas wrote:
> > On Tue, Jul 20, 2021 at 04:49:55PM +0200, Pali Rohár wrote:
> > > On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:
> > > > On Fri, Jun 25, 2021 at 12:04:29PM +0100, Lorenzo Pieralisi wrote:
> > > > > On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
> > > > >
> > > > > [...]
> > > > > 
> > > > > > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > > > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> > > > > >  {
> > > > > >  	struct device *dev = &pcie->pdev->dev;
> > > > > >  	u32 reg;
> > > > > > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > > > > >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> > > > > >  		PIO_COMPLETION_STATUS_SHIFT;
> > > > > >  
> > > > > > -	if (!status)
> > > > > > -		return;
> > > > > > -
> > > > > > +	/*
> > > > > > +	 * According to HW spec, the PIO status check sequence as below:
> > > > > > +	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > > > > > +	 *    it still needs to check Error Status(bit11), only when this bit
> > > > > > +	 *    indicates no error happen, the operation is successful.
> > > > > > +	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > > > > > +	 *    means a PIO write error, and for PIO read it is successful with
> > > > > > +	 *    a read value of 0xFFFFFFFF.
> > > > > > +	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > > > > > +	 *    only means a PIO write error, and for PIO read it is successful
> > > > > > +	 *    with a read value of 0xFFFF0001.
> > > > > > +	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > > > > > +	 *    error for both PIO read and PIO write operation.
> > > > > > +	 * 5) other errors are indicated as 'unknown'.
> > > > > > +	 */
> > > > > >  	switch (status) {
> > > > > > +	case PIO_COMPLETION_STATUS_OK:
> > > > > > +		if (reg & PIO_ERR_STATUS) {
> > > > > > +			strcomp_status = "COMP_ERR";
> > > > > > +			break;
> > > > > > +		}
> > > > > > +		/* Get the read result */
> > > > > > +		if (val)
> > > > > > +			*val = advk_readl(pcie, PIO_RD_DATA);
> > > > > > +		/* No error */
> > > > > > +		strcomp_status = NULL;
> > > > > > +		break;
> > > > > >  	case PIO_COMPLETION_STATUS_UR:
> > > > > > -		strcomp_status = "UR";
> > > > > > +		if (val) {
> > > > > > +			/* For reading, UR is not an error status */
> > > > > > +			*val = CFG_RD_UR_VAL;
> > > > 
> > > > I think the comment is incorrect.  Unsupported Request *is* an error
> > > > status.  But most platforms log it and fabricate ~0 data
> > > > (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're
> > > > doing here.  So I think the code is fine, but the "not an error
> > > > status" comment is wrong.
> > > 
> > > Ok, and what we should driver set as return value for pci_ops.read
> > > callback in this case?
> > 
> > On most platforms, pci_ops.read() does not check for failure, so it
> > returns PCIBIOS_SUCCESSFUL in this case.
> 
> Ok. Most platforms do not check for failure. But it is generally
> correct? Probably more platforms do not provide error flag and only
> return value. But aardvark hw provides this kind error information, so
> should pci-aardvark's pci_ops.read() on error returns PCIBIOS_SUCCESSFUL
> on some other return value?

By all means, if you have the error information handy, return
PCIBIOS_DEVICE_NOT_FOUND or whatever you think is appropriate.  Of
course, most callers of pci_read_config_word() et al don't check.  I
think you should set the returned data to ~0 in this case, too.

[RESEND PATCH 3/3] PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO response

[Pali Rohár]

Measurements in different conditions showed that aardvark hardware PIO
response can take up to 1.44s. Increase wait timeout from 1ms to 1.5s to
ensure that we do not miss responses from hardware. After 1.44s hardware
returns errors (e.g. Completer abort).

The previous two patches fixed checking for PIO status, so now we can use
it to also catch errors which are reported by hardware after 1.44s.

After applying this patch, kernel can detect and print PIO errors to dmesg:

    [    6.879999] advk-pcie d0070000.pcie: Non-posted PIO Response Status: CA, 0xe00 @ 0x100004
    [    6.896436] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100004
    [    6.913049] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100010
    [    6.929663] advk-pcie d0070000.pcie: Non-posted PIO Response Status: CA, 0xe00 @ 0x100010
    [    6.953558] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100014
    [    6.970170] advk-pcie d0070000.pcie: Non-posted PIO Response Status: CA, 0xe00 @ 0x100014
    [    6.994328] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100004

Without this patch kernel prints only a generic error to dmesg:

    [    5.246847] advk-pcie d0070000.pcie: config read/write timed out

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # 7fbcb5da811b ("PCI: aardvark: Don't rely on jiffies while holding spinlock")
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a37ba86f1b2d..3f3c72927afb 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -166,7 +166,7 @@
 #define PCIE_CONFIG_WR_TYPE0			0xa
 #define PCIE_CONFIG_WR_TYPE1			0xb
 
-#define PIO_RETRY_CNT			500
+#define PIO_RETRY_CNT			750000 /* 1.5 s */
 #define PIO_RETRY_DELAY			2 /* 2 us*/
 
 #define LINK_WAIT_MAX_RETRIES		10
-- 
2.20.1

Re: [RESEND PATCH 0/3] PCI: aardvark: PIO fixes

[Lorenzo Pieralisi]

On Thu, 24 Jun 2021 23:33:42 +0200, Pali Rohár wrote:
> Per Lorenzo's request [1] I'm resending PCI aardvark patches from big
> patch series [2] which fixes just one small thing - aardvark PIO code.
> 
> [1] - https://lore.kernel.org/linux-pci/20210603151605.GA18917@lpieralisi/
> [2] - https://lore.kernel.org/linux-pci/20210506153153.30454-1-pali@kernel.org/
> 
> Evan Wang (1):
>   PCI: aardvark: Fix checking for PIO status
> 
> [...]

Given that we are close to a release, I am trying to cherry-pick
some aardvark patches to cut the current delta.

Applied to pci/aardvark:

[1/1] PCI: aardvark: Fix checking for PIO Non-posted Request
      https://git.kernel.org/lpieralisi/pci/c/8ceeac307a

Thanks,
Lorenzo

[PATCH v2 0/4] PCI: aardvark: PIO fixes

[Pali Rohár]

This patch series fix PIO functions used for accessing PCI config space.

In v2 is fixed processing of CRS response which depends on emulation of
CRSVIS and CRSSVE bits in config space of emulated PCIe bridge.

Patch "PCI: aardvark: Fix checking for PIO Non-posted Request" was
dropped from v2 as it was already applied.

Evan Wang (1):
  PCI: aardvark: Fix checking for PIO status

Pali Rohár (3):
  PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO
    response
  PCI: pci-bridge-emul: Add PCIe Root Capabilities Register
  PCI: aardvark: Fix reporting CRS value

 drivers/pci/controller/pci-aardvark.c | 125 +++++++++++++++++++++++---
 drivers/pci/pci-bridge-emul.h         |   2 +-
 2 files changed, 116 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] PCI: aardvark: Fix checking for PIO status

[Pali Rohár]

From: Evan Wang <xswang@marvell.com>

There is an issue that when PCIe switch is connected to an Armada 3700
board, there will be lots of warnings about PIO errors when reading the
config space. According to Aardvark PIO read and write sequence in HW
specification, the current way to check PIO status has the following
issues:

1) For PIO read operation, it reports the error message, which should be
   avoided according to HW specification.

2) For PIO read and write operations, it only checks PIO operation complete
   status, which is not enough, and error status should also be checked.

This patch aligns the code with Aardvark PIO read and write sequence in HW
specification on PIO status check and fix the warnings when reading config
space.

Signed-off-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Victor Gu <xigu@marvell.com>
Tested-by: Victor Gu <xigu@marvell.com>
[pali: Fix CRS handling when CRSSVE is not enabled]
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # b1bd5714472c ("PCI: aardvark: Indicate error in 'val' when config read fails")
---
Changes in v2:
* Fixed CRS handling when CRSSVE is not enabled (which is by default as
  support for CRSSVE is in followup patch)
---
 drivers/pci/controller/pci-aardvark.c | 62 +++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2f8380a1f84f..a74ac8addeae 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -58,6 +58,7 @@
 #define   PIO_COMPLETION_STATUS_CRS		2
 #define   PIO_COMPLETION_STATUS_CA		4
 #define   PIO_NON_POSTED_REQ			BIT(10)
+#define   PIO_ERR_STATUS			BIT(11)
 #define PIO_ADDR_LS				(PIO_BASE_ADDR + 0x8)
 #define PIO_ADDR_MS				(PIO_BASE_ADDR + 0xc)
 #define PIO_WR_DATA				(PIO_BASE_ADDR + 0x10)
@@ -461,7 +462,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
 }
 
-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;
@@ -472,14 +473,49 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
 		strcomp_status = "UR";
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
+		/* PCIe r4.0, sec 2.3.2, says:
+		 * If CRS Software Visibility is not enabled, the Root Complex
+		 * must re-issue the Configuration Request as a new Request.
+		 * A Root Complex implementation may choose to limit the number
+		 * of Configuration Request/CRS Completion Status loops before
+		 * determining that something is wrong with the target of the
+		 * Request and taking appropriate action, e.g., complete the
+		 * Request to the host as a failed transaction.
+		 *
+		 * To simplify implementation do not re-issue the Configuration
+		 * Request and complete the Request as a failed transaction.
+		 */
 		strcomp_status = "CRS";
 		break;
 	case PIO_COMPLETION_STATUS_CA:
@@ -490,6 +526,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else
@@ -497,6 +536,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)
@@ -734,10 +775,13 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
+	if (ret < 0) {
+		*val = 0xffffffff;
+		return PCIBIOS_SET_FAILED;
+	}
 
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)
@@ -801,7 +845,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }
-- 
2.20.1

[PATCH v2 2/4] PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO response

[Pali Rohár]

Measurements in different conditions showed that aardvark hardware PIO
response can take up to 1.44s. Increase wait timeout from 1ms to 1.5s to
ensure that we do not miss responses from hardware. After 1.44s hardware
returns errors (e.g. Completer abort).

The previous two patches fixed checking for PIO status, so now we can use
it to also catch errors which are reported by hardware after 1.44s.

After applying this patch, kernel can detect and print PIO errors to dmesg:

    [    6.879999] advk-pcie d0070000.pcie: Non-posted PIO Response Status: CA, 0xe00 @ 0x100004
    [    6.896436] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100004
    [    6.913049] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100010
    [    6.929663] advk-pcie d0070000.pcie: Non-posted PIO Response Status: CA, 0xe00 @ 0x100010
    [    6.953558] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100014
    [    6.970170] advk-pcie d0070000.pcie: Non-posted PIO Response Status: CA, 0xe00 @ 0x100014
    [    6.994328] advk-pcie d0070000.pcie: Posted PIO Response Status: COMP_ERR, 0x804 @ 0x100004

Without this patch kernel prints only a generic error to dmesg:

    [    5.246847] advk-pcie d0070000.pcie: config read/write timed out

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # 7fbcb5da811b ("PCI: aardvark: Don't rely on jiffies while holding spinlock")
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a74ac8addeae..ebb48530024c 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -166,7 +166,7 @@
 #define PCIE_CONFIG_WR_TYPE0			0xa
 #define PCIE_CONFIG_WR_TYPE1			0xb
 
-#define PIO_RETRY_CNT			500
+#define PIO_RETRY_CNT			750000 /* 1.5 s */
 #define PIO_RETRY_DELAY			2 /* 2 us*/
 
 #define LINK_WAIT_MAX_RETRIES		10
-- 
2.20.1

[PATCH v2 3/4] PCI: pci-bridge-emul: Add PCIe Root Capabilities Register

[Pali Rohár]

The 16-bit Root Capabilities register is at offset 0x1e in the PCIe
Capability. Rename current 'rsvd' struct member to 'rootcap'.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/pci-bridge-emul.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h
index b31883022a8e..49bbd37ee318 100644
--- a/drivers/pci/pci-bridge-emul.h
+++ b/drivers/pci/pci-bridge-emul.h
@@ -54,7 +54,7 @@ struct pci_bridge_emul_pcie_conf {
 	__le16 slotctl;
 	__le16 slotsta;
 	__le16 rootctl;
-	__le16 rsvd;
+	__le16 rootcap;
 	__le32 rootsta;
 	__le32 devcap2;
 	__le16 devctl2;
-- 
2.20.1

[PATCH v2 4/4] PCI: aardvark: Fix reporting CRS value

[Pali Rohár]

Set CRSVIS flag in emulated root PCI bridge to indicate support for
Completion Retry Status.

Add check for CRSSVE flag from root PCI brige when issuing Configuration
Read Request via PIO to correctly returns fabricated CRS value as it is
required by PCIe spec.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
Cc: stable@vger.kernel.org # e0d9d30b7354 ("PCI: pci-bridge-emul: Fix big-endian support")
---
 drivers/pci/controller/pci-aardvark.c | 67 +++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ebb48530024c..abc93225ba20 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -177,6 +177,8 @@
 
 #define MSI_IRQ_NUM			32
 
+#define CFG_RD_CRS_VAL			0xffff0001
+
 struct advk_pcie {
 	struct platform_device *pdev;
 	void __iomem *base;
@@ -462,7 +464,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
 }
 
-static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;
@@ -504,9 +506,30 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 		strcomp_status = "UR";
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
+		if (allow_crs && val) {
+			/* PCIe r4.0, sec 2.3.2, says:
+			 * If CRS Software Visibility is enabled:
+			 * For a Configuration Read Request that includes both
+			 * bytes of the Vendor ID field of a device Function's
+			 * Configuration Space Header, the Root Complex must
+			 * complete the Request to the host by returning a
+			 * read-data value of 0001h for the Vendor ID field and
+			 * all '1's for any additional bytes included in the
+			 * request.
+			 *
+			 * So CRS in this case is not an error status.
+			 */
+			*val = CFG_RD_CRS_VAL;
+			strcomp_status = NULL;
+			break;
+		}
 		/* PCIe r4.0, sec 2.3.2, says:
 		 * If CRS Software Visibility is not enabled, the Root Complex
 		 * must re-issue the Configuration Request as a new Request.
+		 * If CRS Software Visibility is enabled: For a Configuration
+		 * Write Request or for any other Configuration Read Request,
+		 * the Root Complex must re-issue the Configuration Request as
+		 * a new Request.
 		 * A Root Complex implementation may choose to limit the number
 		 * of Configuration Request/CRS Completion Status loops before
 		 * determining that something is wrong with the target of the
@@ -575,6 +598,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 	case PCI_EXP_RTCTL: {
 		u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG);
 		*value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE;
+		*value |= PCI_EXP_RTCAP_CRSVIS << 16;
 		return PCI_BRIDGE_EMUL_HANDLED;
 	}
 
@@ -656,6 +680,7 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
 static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 {
 	struct pci_bridge_emul *bridge = &pcie->bridge;
+	int ret;
 
 	bridge->conf.vendor =
 		cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff);
@@ -679,7 +704,15 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 	bridge->data = pcie;
 	bridge->ops = &advk_pci_bridge_emul_ops;
 
-	return pci_bridge_emul_init(bridge, 0);
+	/* PCIe config space can be initialized after pci_bridge_emul_init() */
+	ret = pci_bridge_emul_init(bridge, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Indicates supports for Completion Retry Status */
+	bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
+
+	return 0;
 }
 
 static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
@@ -731,6 +764,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 			     int where, int size, u32 *val)
 {
 	struct advk_pcie *pcie = bus->sysdata;
+	bool allow_crs;
 	u32 reg;
 	int ret;
 
@@ -743,7 +777,24 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 		return pci_bridge_emul_conf_read(&pcie->bridge, where,
 						 size, val);
 
+	/*
+	 * Completion Retry Status is possible to return only when reading all
+	 * 4 bytes from PCI_VENDOR_ID and PCI_DEVICE_ID registers at once and
+	 * CRSSVE flag on Root Bridge is enabled.
+	 */
+	allow_crs = (where == PCI_VENDOR_ID) && (size == 4) &&
+		    (le16_to_cpu(pcie->bridge.pcie_conf.rootctl) &
+		     PCI_EXP_RTCTL_CRSSVE);
+
 	if (advk_pcie_pio_is_running(pcie)) {
+		/*
+		 * If it is possible return Completion Retry Status so caller
+		 * tries to issue the request again instead of failing.
+		 */
+		if (allow_crs) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		}
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
@@ -771,12 +822,20 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 
 	ret = advk_pcie_wait_pio(pcie);
 	if (ret < 0) {
+		/*
+		 * If it is possible return Completion Retry Status so caller
+		 * tries to issue the request again instead of failing.
+		 */
+		if (allow_crs) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		}
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
 	/* Check PIO status and get the read result */
-	ret = advk_pcie_check_pio_status(pcie, val);
+	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
@@ -845,7 +904,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	ret = advk_pcie_check_pio_status(pcie, NULL);
+	ret = advk_pcie_check_pio_status(pcie, false, NULL);
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-- 
2.20.1

Re: [PATCH v2 0/4] PCI: aardvark: PIO fixes

[Lorenzo Pieralisi]

On Thu, 22 Jul 2021 16:40:37 +0200, Pali Rohár wrote:
> This patch series fix PIO functions used for accessing PCI config space.
> 
> In v2 is fixed processing of CRS response which depends on emulation of
> CRSVIS and CRSSVE bits in config space of emulated PCIe bridge.
> 
> Patch "PCI: aardvark: Fix checking for PIO Non-posted Request" was
> dropped from v2 as it was already applied.
> 
> [...]

Applied to pci/aardvark, thanks!

[1/4] PCI: aardvark: Fix checking for PIO status
      https://git.kernel.org/lpieralisi/pci/c/fcb461e2bc
[2/4] PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO response
      https://git.kernel.org/lpieralisi/pci/c/02bcec3ea5
[3/4] PCI: pci-bridge-emul: Add PCIe Root Capabilities Register
      https://git.kernel.org/lpieralisi/pci/c/e902bb7c24
[4/4] PCI: aardvark: Fix reporting CRS value
      https://git.kernel.org/lpieralisi/pci/c/43f5c77bcb

Thanks,
Lorenzo