diff mbox

[v8,0/3] PCI: iproc: SOC specific fixes

Message ID 20170828215309.GM8154@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bjorn Helgaas Aug. 28, 2017, 9:53 p.m. UTC 
On Thu, Aug 24, 2017 at 10:34:23AM +0530, Oza Pawandeep wrote:
> PCI: iproc: Retry request when CRS returned from EP Above patch adds
> support for CRS in PCI RC driver, otherwise if not handled at lower
> level, the user space PMD (poll mode drivers) can timeout.
> 
> PCI: iproc: add device shutdown for PCI RC This fixes the issue where
> certian PCI endpoints are not getting detected on Stingray SOC after
> reboot.
> 
> Changes Since v7:
> Factor out the ep config access code.
> 
> Changes Since v6:
> Rebased patches on top of Lorenzo's patches.
> Bjorn's comments addressed.
> now the confg retry returns 0xffffffff as data.
> Added reference to PCIe spec and iproc Controller spec in Changelog.
> 
> Changes Since v5:
> Ray's comments addressed.
> 
> Changes Since v4:
> Bjorn's comments addressed.
> 
> Changes Since v3:
> [re-send]
> 
> Changes Since v2:
> Fix compilation errors for pcie-iproc-platform.ko which was caught
> by kbuild.
> 
> Oza Pawandeep (3):
>   PCI: iproc: factor-out ep configuration access
>   PCI: iproc: Retry request when CRS returned from EP
>   PCI: iproc: add device shutdown for PCI RC
> 
>  drivers/pci/host/pcie-iproc-platform.c |   8 ++
>  drivers/pci/host/pcie-iproc.c          | 143 ++++++++++++++++++++++++++-------
>  drivers/pci/host/pcie-iproc.h          |   1 +
>  3 files changed, 124 insertions(+), 28 deletions(-)

I applied these to pci/host-iproc for v4.14.  Man, this is ugly.

I reworked the changelog to try to make it more readable.  I also tried to
disable the PCI_EXP_RTCAP_CRSVIS bit, which advertises CRS SV support.  And
I removed what looked like a duplicate pci_generic_config_read32() call.
And I added a warning about the fact that we corrupt reads of config
registers that happen to contain 0xffff0001.

I'm pretty sure I broke something, so please take a look.

Incremental diff from your v8 to what's on pci/host-iproc:

Comments

Oza Pawandeep Aug. 29, 2017, 5:25 a.m. UTC | #1 
On Tue, Aug 29, 2017 at 3:23 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:23AM +0530, Oza Pawandeep wrote:
>> PCI: iproc: Retry request when CRS returned from EP Above patch adds
>> support for CRS in PCI RC driver, otherwise if not handled at lower
>> level, the user space PMD (poll mode drivers) can timeout.
>>
>> PCI: iproc: add device shutdown for PCI RC This fixes the issue where
>> certian PCI endpoints are not getting detected on Stingray SOC after
>> reboot.
>>
>> Changes Since v7:
>> Factor out the ep config access code.
>>
>> Changes Since v6:
>> Rebased patches on top of Lorenzo's patches.
>> Bjorn's comments addressed.
>> now the confg retry returns 0xffffffff as data.
>> Added reference to PCIe spec and iproc Controller spec in Changelog.
>>
>> Changes Since v5:
>> Ray's comments addressed.
>>
>> Changes Since v4:
>> Bjorn's comments addressed.
>>
>> Changes Since v3:
>> [re-send]
>>
>> Changes Since v2:
>> Fix compilation errors for pcie-iproc-platform.ko which was caught
>> by kbuild.
>>
>> Oza Pawandeep (3):
>>   PCI: iproc: factor-out ep configuration access
>>   PCI: iproc: Retry request when CRS returned from EP
>>   PCI: iproc: add device shutdown for PCI RC
>>
>>  drivers/pci/host/pcie-iproc-platform.c |   8 ++
>>  drivers/pci/host/pcie-iproc.c          | 143 ++++++++++++++++++++++++++-------
>>  drivers/pci/host/pcie-iproc.h          |   1 +
>>  3 files changed, 124 insertions(+), 28 deletions(-)
>
> I applied these to pci/host-iproc for v4.14.  Man, this is ugly.
>
> I reworked the changelog to try to make it more readable.  I also tried to
> disable the PCI_EXP_RTCAP_CRSVIS bit, which advertises CRS SV support.  And
> I removed what looked like a duplicate pci_generic_config_read32() call.
> And I added a warning about the fact that we corrupt reads of config
> registers that happen to contain 0xffff0001.
>
> I'm pretty sure I broke something, so please take a look.

Appreciate your time in adding PCI_EXP_RTCAP_CRSVIS and other changes.
I just tested the patch, and it works fine.
which tells us, that CRS visibility bit has no effect.

so things look okay to me.

Regards,
Oza.
>
> Incremental diff from your v8 to what's on pci/host-iproc:
>
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index cbdabe8a073e..8bd5e544b1c1 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -69,7 +69,7 @@
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>
>  #define CFG_RETRY_STATUS             0xffff0001
> -#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milliseconds */
>
>  /* derive the enum index of the outbound/inbound mapping registers */
>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
> @@ -482,17 +482,21 @@ static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>         unsigned int data;
>
>         /*
> -        * As per PCIe spec r3.1, sec 2.3.2, CRS Software
> -        * Visibility only affects config read of the Vendor ID.
> -        * For config write or any other config read the Root must
> -        * automatically re-issue configuration request again as a
> -        * new request.
> +        * As per PCIe spec r3.1, sec 2.3.2, CRS Software Visibility only
> +        * affects config reads of the Vendor ID.  For config writes or any
> +        * other config reads, the Root may automatically reissue the
> +        * configuration request again as a new request.
>          *
> -        * For config reads, this hardware returns CFG_RETRY_STATUS data when
> -        * it receives a CRS completion for a config read, regardless of the
> -        * address of the read or the CRS Software Visibility Enable bit. As a
> +        * For config reads, this hardware returns CFG_RETRY_STATUS data
> +        * when it receives a CRS completion, regardless of the address of
> +        * the read or the CRS Software Visibility Enable bit.  As a
>          * partial workaround for this, we retry in software any read that
>          * returns CFG_RETRY_STATUS.
> +        *
> +        * Note that a non-Vendor ID config register may have a value of
> +        * CFG_RETRY_STATUS.  If we read that, we can't distinguish it from
> +        * a CRS completion, so we will incorrectly retry the read and
> +        * eventually return the wrong data (0xffffffff).
>          */
>         data = readl(cfg_data_p);
>         while (data == CFG_RETRY_STATUS && timeout--) {
> @@ -515,10 +519,19 @@ static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>         unsigned int busno = bus->number;
>         void __iomem *cfg_data_p;
>         unsigned int data;
> +       int ret;
>
> -       /* root complex access. */
> -       if (busno == 0)
> -               return pci_generic_config_read32(bus, devfn, where, size, val);
> +       /* root complex access */
> +       if (busno == 0) {
> +               ret = pci_generic_config_read32(bus, devfn, where, size, val);
> +               if (ret != PCIBIOS_SUCCESSFUL)
> +                       return ret;
> +
> +               /* Don't advertise CRS SV support */
> +               if ((where & ~0x3) == PCI_EXP_CAP + PCI_EXP_RTCAP)
> +                       *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +               return PCIBIOS_SUCCESSFUL;
> +       }
>
>         cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>
> @@ -635,7 +648,6 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>                 ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>         else
>                 ret = pci_generic_config_read32(bus, devfn, where, size, val);
> -       ret = pci_generic_config_read32(bus, devfn, where, size, val);
>         iproc_pcie_apb_err_disable(bus, false);
>
>         return ret;
> @@ -1309,6 +1321,8 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>                 pcie->ib.nr_regions = ARRAY_SIZE(paxb_v2_ib_map);
>                 pcie->ib_map = paxb_v2_ib_map;
>                 pcie->need_msi_steer = true;
> +               dev_warn(dev, "reads of config registers that contain %#x return incorrect data\n",
> +                        CFG_RETRY_STATUS);
>                 break;
>         case IPROC_PCIE_PAXC:
>                 regs = iproc_pcie_reg_paxc;
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index cbdabe8a073e..8bd5e544b1c1 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -69,7 +69,7 @@ 
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
 #define CFG_RETRY_STATUS             0xffff0001
-#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
+#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milliseconds */
 
 /* derive the enum index of the outbound/inbound mapping registers */
 #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
@@ -482,17 +482,21 @@  static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
 	unsigned int data;
 
 	/*
-	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
-	 * Visibility only affects config read of the Vendor ID.
-	 * For config write or any other config read the Root must
-	 * automatically re-issue configuration request again as a
-	 * new request.
+	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software Visibility only
+	 * affects config reads of the Vendor ID.  For config writes or any
+	 * other config reads, the Root may automatically reissue the
+	 * configuration request again as a new request.
 	 *
-	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
-	 * it receives a CRS completion for a config read, regardless of the
-	 * address of the read or the CRS Software Visibility Enable bit. As a
+	 * For config reads, this hardware returns CFG_RETRY_STATUS data
+	 * when it receives a CRS completion, regardless of the address of
+	 * the read or the CRS Software Visibility Enable bit.  As a
 	 * partial workaround for this, we retry in software any read that
 	 * returns CFG_RETRY_STATUS.
+	 *
+	 * Note that a non-Vendor ID config register may have a value of
+	 * CFG_RETRY_STATUS.  If we read that, we can't distinguish it from
+	 * a CRS completion, so we will incorrectly retry the read and
+	 * eventually return the wrong data (0xffffffff).
 	 */
 	data = readl(cfg_data_p);
 	while (data == CFG_RETRY_STATUS && timeout--) {
@@ -515,10 +519,19 @@  static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 	unsigned int busno = bus->number;
 	void __iomem *cfg_data_p;
 	unsigned int data;
+	int ret;
 
-	/* root complex access. */
-	if (busno == 0)
-		return pci_generic_config_read32(bus, devfn, where, size, val);
+	/* root complex access */
+	if (busno == 0) {
+		ret = pci_generic_config_read32(bus, devfn, where, size, val);
+		if (ret != PCIBIOS_SUCCESSFUL)
+			return ret;
+
+		/* Don't advertise CRS SV support */
+		if ((where & ~0x3) == PCI_EXP_CAP + PCI_EXP_RTCAP)
+			*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+		return PCIBIOS_SUCCESSFUL;
+	}
 
 	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
 
@@ -635,7 +648,6 @@  static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
 	else
 		ret = pci_generic_config_read32(bus, devfn, where, size, val);
-	ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
 	return ret;
@@ -1309,6 +1321,8 @@  static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
 		pcie->ib.nr_regions = ARRAY_SIZE(paxb_v2_ib_map);
 		pcie->ib_map = paxb_v2_ib_map;
 		pcie->need_msi_steer = true;
+		dev_warn(dev, "reads of config registers that contain %#x return incorrect data\n",
+			 CFG_RETRY_STATUS);
 		break;
 	case IPROC_PCIE_PAXC:
 		regs = iproc_pcie_reg_paxc;