[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

X-Gene v1 PCIe controller has a bug in Configuration Request Retry
Status (CRS) logic:
  When CPU tries to read Vendor ID and Device ID of not-existed
  remote device, the controller returns 0xFFFF0001 instead of
  0xFFFFFFFF; this will add significant delay in boot time as
  pci_bus_read_dev_vendor_id will wait for 60 seconds before
  giving up.

So for X-Gene v1 PCIe controllers, disable CRS capability
advertisement by clearing CRS Software Visibility bit before
returning the Root Capability value to the callers. This is done
by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
CFG read accesses to replace the generic default pci_generic_config_read32
function.

Signed-off-by: Duc Dang <dhdang@apm.com>
---
 drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index ee082c0..741a253 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -59,6 +59,12 @@
 #define SZ_1T				(SZ_1G*1024ULL)
 #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
 
+#define ROOT_CAP_AND_CTRL		0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN		0
+#define XGENE_PCIE_IP_VER_1		1
+
 struct xgene_pcie_port {
 	struct device_node	*node;
 	struct device		*dev;
@@ -67,6 +73,7 @@ struct xgene_pcie_port {
 	void __iomem		*cfg_base;
 	unsigned long		cfg_addr;
 	bool			link_up;
+	u32			version;
 };
 
 static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
@@ -140,9 +147,44 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	return xgene_pcie_get_cfg_base(bus) + offset;
 }
 
+int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+			      int where, int size, u32 *val)
+{
+	void __iomem *addr;
+	struct xgene_pcie_port *port = bus->sysdata;
+
+	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (!addr) {
+		*val = ~0;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	*val = readl(addr);
+	/*
+	 * X-Gene v1 PCIe controller has a bug in Configuration Request
+	 * Retry Status (CRS) logic:
+	 *  When CPU tries to read Vendor ID and Device ID of not-existed
+	 *  remote device, the controller returns 0xFFFF0001 instead of
+	 *  0xFFFFFFFF; this will add significant delay in boot time as
+	 *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
+	 *  giving up.
+	 * So for X-Gene v1 PCIe controllers, disable CRS capability
+	 * advertisement by clearing CRS Software Visibility bit before
+	 * returning the Root Capability value to the callers.
+	 */
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops xgene_pcie_ops = {
 	.map_bus = xgene_pcie_map_bus,
-	.read = pci_generic_config_read32,
+	.read = xgene_pcie_config_read32,
 	.write = pci_generic_config_write32,
 };
 
@@ -483,6 +525,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	port->node = of_node_get(pdev->dev.of_node);
 	port->dev = &pdev->dev;
 
+	port->version = XGENE_PCIE_IP_VER_UNKN;
+	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
+		port->version = XGENE_PCIE_IP_VER_1;
+
 	ret = xgene_pcie_map_reg(port, pdev);
 	if (ret)
 		return ret;
-- 
1.9.1

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

X-Gene v1 PCIe controller has a bug in Configuration Request Retry
Status (CRS) logic:
  When CPU tries to read Vendor ID and Device ID of not-existed
  remote device, the controller returns 0xFFFF0001 instead of
  0xFFFFFFFF; this will add significant delay in boot time as
  pci_bus_read_dev_vendor_id will wait for 60 seconds before
  giving up.

So for X-Gene v1 PCIe controllers, disable CRS capability
advertisement by clearing CRS Software Visibility bit before
returning the Root Capability value to the callers. This is done
by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
CFG read accesses to replace the generic default pci_generic_config_read32
function.

Signed-off-by: Duc Dang <dhdang@apm.com>
---
 drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index ee082c0..741a253 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -59,6 +59,12 @@
 #define SZ_1T				(SZ_1G*1024ULL)
 #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
 
+#define ROOT_CAP_AND_CTRL		0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN		0
+#define XGENE_PCIE_IP_VER_1		1
+
 struct xgene_pcie_port {
 	struct device_node	*node;
 	struct device		*dev;
@@ -67,6 +73,7 @@ struct xgene_pcie_port {
 	void __iomem		*cfg_base;
 	unsigned long		cfg_addr;
 	bool			link_up;
+	u32			version;
 };
 
 static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
@@ -140,9 +147,44 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	return xgene_pcie_get_cfg_base(bus) + offset;
 }
 
+int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+			      int where, int size, u32 *val)
+{
+	void __iomem *addr;
+	struct xgene_pcie_port *port = bus->sysdata;
+
+	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (!addr) {
+		*val = ~0;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	*val = readl(addr);
+	/*
+	 * X-Gene v1 PCIe controller has a bug in Configuration Request
+	 * Retry Status (CRS) logic:
+	 *  When CPU tries to read Vendor ID and Device ID of not-existed
+	 *  remote device, the controller returns 0xFFFF0001 instead of
+	 *  0xFFFFFFFF; this will add significant delay in boot time as
+	 *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
+	 *  giving up.
+	 * So for X-Gene v1 PCIe controllers, disable CRS capability
+	 * advertisement by clearing CRS Software Visibility bit before
+	 * returning the Root Capability value to the callers.
+	 */
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops xgene_pcie_ops = {
 	.map_bus = xgene_pcie_map_bus,
-	.read = pci_generic_config_read32,
+	.read = xgene_pcie_config_read32,
 	.write = pci_generic_config_write32,
 };
 
@@ -483,6 +525,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	port->node = of_node_get(pdev->dev.of_node);
 	port->dev = &pdev->dev;
 
+	port->version = XGENE_PCIE_IP_VER_UNKN;
+	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
+		port->version = XGENE_PCIE_IP_VER_1;
+
 	ret = xgene_pcie_map_reg(port, pdev);
 	if (ret)
 		return ret;
-- 
1.9.1

Re: [PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Marcin Juszkiewicz]

W dniu 11.06.2015 o 22:08, Duc Dang pisze:> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>    When CPU tries to read Vendor ID and Device ID of not-existed
>    remote device, the controller returns 0xFFFF0001 instead of
>    0xFFFFFFFF; this will add significant delay in boot time as
>    pci_bus_read_dev_vendor_id will wait for 60 seconds before
>    giving up.
> 
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> ---
>   drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)

Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>

Confirmed. It fixed issue.

[    0.842339] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
[    0.848235] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
[    0.855255] PCI host bridge /soc/pcie@1f2b0000 ranges:
[    0.860678]   No bus range found for /soc/pcie@1f2b0000, using [bus 00-ff]
[    0.867950]    IO 0xe010000000..0xe01000ffff -> 0x00000000
[    0.873753]   MEM 0xe180000000..0xe1ffffffff -> 0x80000000
[    0.879592] xgene-pcie 1f2b0000.pcie: (rc) x1 gen-1 link up
[    0.885529] xgene-pcie 1f2b0000.pcie: PCI host bridge to bus 0000:00
[    0.892248] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.898036] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    0.904556] pci_bus 0000:00: root bus resource [mem 0xe180000000-0xe1ffffffff] (bus address [0x80000000-0xffffffff])
[    0.915741] pci 0000:00:00.0: IOMMU is currently not supported for PCI
[    0.922936] pci 0000:01:00.0: IOMMU is currently not supported for PCI
[    0.929937] pci 0000:01:00.0: of_irq_parse_pci() failed with rc=-19
[    0.936616] pci 0000:01:00.0: disabling ASPM on pre-1.1 PCIe device.  You can enable it with 'pcie_aspm=force'
[    0.947422] pci 0000:02:00.0: IOMMU is currently not supported for PCI
[    0.954439] vgaarb: device added: PCI:0000:02:00.0,decodes=io+mem,owns=none,locks=none
[    0.962946] pci 0000:00:00.0: BAR 14: assigned [mem 0xe180000000-0xe182ffffff]
[    0.970593] pci 0000:01:00.0: BAR 14: assigned [mem 0xe180000000-0xe182ffffff]
[    0.978239] pci 0000:02:00.0: BAR 0: assigned [mem 0xe180000000-0xe181ffffff pref]
[    0.986260] pci 0000:02:00.0: BAR 2: assigned [mem 0xe182000000-0xe1827fffff]
[    0.993828] pci 0000:02:00.0: BAR 6: assigned [mem 0xe182800000-0xe18281ffff pref]
[    1.001852] pci 0000:02:00.0: BAR 1: assigned [mem 0xe182820000-0xe182823fff]
[    1.009397] pci 0000:01:00.0: PCI bridge to [bus 02]
[    1.014637] pci 0000:01:00.0:   bridge window [mem 0xe180000000-0xe182ffffff]
[    1.022200] pci 0000:00:00.0: PCI bridge to [bus 01-02]
[    1.027713] pci 0000:00:00.0:   bridge window [mem 0xe180000000-0xe182ffffff]
[    1.035344] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
[    1.042709] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
[    1.049577] pci 0000:02:00.0: Signaling PME through PCIe PME interrupt
[    1.056538] pci 0000:01:00.0: TI XIO2000a quirk detected; secondary bus fast back-to-back transfers disabled

compared to

[ 2040.117742] pci 0000:00:00.0: BAR 14: assigned [mem 0xe180000000-0xe182ffffff]

Similar with Radeon card.

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Marcin Juszkiewicz]

W dniu 11.06.2015 o 22:08, Duc Dang pisze:> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>    When CPU tries to read Vendor ID and Device ID of not-existed
>    remote device, the controller returns 0xFFFF0001 instead of
>    0xFFFFFFFF; this will add significant delay in boot time as
>    pci_bus_read_dev_vendor_id will wait for 60 seconds before
>    giving up.
> 
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> ---
>   drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)

Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>

Confirmed. It fixed issue.

[    0.842339] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
[    0.848235] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
[    0.855255] PCI host bridge /soc/pcie at 1f2b0000 ranges:
[    0.860678]   No bus range found for /soc/pcie at 1f2b0000, using [bus 00-ff]
[    0.867950]    IO 0xe010000000..0xe01000ffff -> 0x00000000
[    0.873753]   MEM 0xe180000000..0xe1ffffffff -> 0x80000000
[    0.879592] xgene-pcie 1f2b0000.pcie: (rc) x1 gen-1 link up
[    0.885529] xgene-pcie 1f2b0000.pcie: PCI host bridge to bus 0000:00
[    0.892248] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.898036] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    0.904556] pci_bus 0000:00: root bus resource [mem 0xe180000000-0xe1ffffffff] (bus address [0x80000000-0xffffffff])
[    0.915741] pci 0000:00:00.0: IOMMU is currently not supported for PCI
[    0.922936] pci 0000:01:00.0: IOMMU is currently not supported for PCI
[    0.929937] pci 0000:01:00.0: of_irq_parse_pci() failed with rc=-19
[    0.936616] pci 0000:01:00.0: disabling ASPM on pre-1.1 PCIe device.  You can enable it with 'pcie_aspm=force'
[    0.947422] pci 0000:02:00.0: IOMMU is currently not supported for PCI
[    0.954439] vgaarb: device added: PCI:0000:02:00.0,decodes=io+mem,owns=none,locks=none
[    0.962946] pci 0000:00:00.0: BAR 14: assigned [mem 0xe180000000-0xe182ffffff]
[    0.970593] pci 0000:01:00.0: BAR 14: assigned [mem 0xe180000000-0xe182ffffff]
[    0.978239] pci 0000:02:00.0: BAR 0: assigned [mem 0xe180000000-0xe181ffffff pref]
[    0.986260] pci 0000:02:00.0: BAR 2: assigned [mem 0xe182000000-0xe1827fffff]
[    0.993828] pci 0000:02:00.0: BAR 6: assigned [mem 0xe182800000-0xe18281ffff pref]
[    1.001852] pci 0000:02:00.0: BAR 1: assigned [mem 0xe182820000-0xe182823fff]
[    1.009397] pci 0000:01:00.0: PCI bridge to [bus 02]
[    1.014637] pci 0000:01:00.0:   bridge window [mem 0xe180000000-0xe182ffffff]
[    1.022200] pci 0000:00:00.0: PCI bridge to [bus 01-02]
[    1.027713] pci 0000:00:00.0:   bridge window [mem 0xe180000000-0xe182ffffff]
[    1.035344] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
[    1.042709] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
[    1.049577] pci 0000:02:00.0: Signaling PME through PCIe PME interrupt
[    1.056538] pci 0000:01:00.0: TI XIO2000a quirk detected; secondary bus fast back-to-back transfers disabled

compared to

[ 2040.117742] pci 0000:00:00.0: BAR 14: assigned [mem 0xe180000000-0xe182ffffff]

Similar with Radeon card.

Re: [PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Ian Campbell]

On Thu, 2015-06-11 at 13:08 -0700, Duc Dang wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.
> 
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>

Applied onto v4.1-rc7 and:
Tested-by: Ian Campbell <ian.campbell@citrix.com>

Thanks!

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Ian Campbell]

On Thu, 2015-06-11 at 13:08 -0700, Duc Dang wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.
> 
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>

Applied onto v4.1-rc7 and:
Tested-by: Ian Campbell <ian.campbell@citrix.com>

Thanks!

Re: [PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

Hi Duc,

On Thu, Jun 11, 2015 at 01:08:14PM -0700, Duc Dang wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.

OK, help me understand how this works.  I think this is related to the
problem I reported where if the slot is empty, "lspci" doesn't show
anything, not even the Root Port leading to the slot.

I think this happens because when we try to read the Root Port's config
space,

  - the slot below the Root Port is empty
  - the Root Port's link is down
  - xgene_pcie_map_bus() returns NULL because !port->link_up
  - pci_generic_config_read32() returns PCIBIOS_DEVICE_NOT_FOUND

so it looks like the Root Port itself doesn't exist.

I proposed to change xgene_pcie_map_bus() so it didn't check whether the
link was up.  That change makes reads of the Root Port's config space work.

After we learn the Root Port exists, the PCI core enumerates devices below
the Root Port, e.g., on bus 01.  X-Gene advertises that it supports CRS, so
we enable it.  When we try to read the Vendor ID of 01:00.0, there's no
response from the device (because the slot is empty), and the Root Complex
should complete the read by fabricating data of all ones, i.e., 0xFFFFFFFF.
But apparently X-Gene supplies 0xFFFF0001 instead, which means "there's a
device here, but it's not ready yet," so the PCI core retries the read for
60 seconds before timing out.

This patch is basically a quirk that keeps X-Gene from advertising CRS
support, so the PCI core won't enable CRS.  In the example above, I guess
that means the Root Complex will supply 0xFFFFFFFF and the core will see
that the slot is empty.

But this patch leaves the "!port->link_up" test in xgene_pcie_map_bus().
Doesn't that mean the core will still not discover the Root Port when the
slot is empty?

It seems to me that you would want both the xgene_pcie_map_bus() change and
this patch.  The first would fix the problem that we don't enumerate Root
Ports leading to empty slots, and the second would fix the problem that we
enable CRS and timeout when enumerating below those Root Ports.

One more question below:

> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> ---
>  drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index ee082c0..741a253 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T				(SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
>  
> +#define ROOT_CAP_AND_CTRL		0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN		0
> +#define XGENE_PCIE_IP_VER_1		1
> +
>  struct xgene_pcie_port {
>  	struct device_node	*node;
>  	struct device		*dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>  	void __iomem		*cfg_base;
>  	unsigned long		cfg_addr;
>  	bool			link_up;
> +	u32			version;
>  };
>  
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,44 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>  	return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>  
> +int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +			      int where, int size, u32 *val)
> +{
> +	void __iomem *addr;
> +	struct xgene_pcie_port *port = bus->sysdata;
> +
> +	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> +	if (!addr) {
> +		*val = ~0;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	*val = readl(addr);

Can't you just call pci_generic_config_read32() directly instead of
duplicating its code here?

> +	/*
> +	 * X-Gene v1 PCIe controller has a bug in Configuration Request
> +	 * Retry Status (CRS) logic:
> +	 *  When CPU tries to read Vendor ID and Device ID of not-existed
> +	 *  remote device, the controller returns 0xFFFF0001 instead of
> +	 *  0xFFFFFFFF; this will add significant delay in boot time as
> +	 *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
> +	 *  giving up.
> +	 * So for X-Gene v1 PCIe controllers, disable CRS capability
> +	 * advertisement by clearing CRS Software Visibility bit before
> +	 * returning the Root Capability value to the callers.
> +	 */
> +	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>  	.map_bus = xgene_pcie_map_bus,
> -	.read = pci_generic_config_read32,
> +	.read = xgene_pcie_config_read32,
>  	.write = pci_generic_config_write32,
>  };
>  
> @@ -483,6 +525,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>  	port->node = of_node_get(pdev->dev.of_node);
>  	port->dev = &pdev->dev;
>  
> +	port->version = XGENE_PCIE_IP_VER_UNKN;
> +	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +		port->version = XGENE_PCIE_IP_VER_1;
> +
>  	ret = xgene_pcie_map_reg(port, pdev);
>  	if (ret)
>  		return ret;
> -- 
> 1.9.1
> 

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

Hi Duc,

On Thu, Jun 11, 2015 at 01:08:14PM -0700, Duc Dang wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.

OK, help me understand how this works.  I think this is related to the
problem I reported where if the slot is empty, "lspci" doesn't show
anything, not even the Root Port leading to the slot.

I think this happens because when we try to read the Root Port's config
space,

  - the slot below the Root Port is empty
  - the Root Port's link is down
  - xgene_pcie_map_bus() returns NULL because !port->link_up
  - pci_generic_config_read32() returns PCIBIOS_DEVICE_NOT_FOUND

so it looks like the Root Port itself doesn't exist.

I proposed to change xgene_pcie_map_bus() so it didn't check whether the
link was up.  That change makes reads of the Root Port's config space work.

After we learn the Root Port exists, the PCI core enumerates devices below
the Root Port, e.g., on bus 01.  X-Gene advertises that it supports CRS, so
we enable it.  When we try to read the Vendor ID of 01:00.0, there's no
response from the device (because the slot is empty), and the Root Complex
should complete the read by fabricating data of all ones, i.e., 0xFFFFFFFF.
But apparently X-Gene supplies 0xFFFF0001 instead, which means "there's a
device here, but it's not ready yet," so the PCI core retries the read for
60 seconds before timing out.

This patch is basically a quirk that keeps X-Gene from advertising CRS
support, so the PCI core won't enable CRS.  In the example above, I guess
that means the Root Complex will supply 0xFFFFFFFF and the core will see
that the slot is empty.

But this patch leaves the "!port->link_up" test in xgene_pcie_map_bus().
Doesn't that mean the core will still not discover the Root Port when the
slot is empty?

It seems to me that you would want both the xgene_pcie_map_bus() change and
this patch.  The first would fix the problem that we don't enumerate Root
Ports leading to empty slots, and the second would fix the problem that we
enable CRS and timeout when enumerating below those Root Ports.

One more question below:

> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> ---
>  drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index ee082c0..741a253 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T				(SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
>  
> +#define ROOT_CAP_AND_CTRL		0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN		0
> +#define XGENE_PCIE_IP_VER_1		1
> +
>  struct xgene_pcie_port {
>  	struct device_node	*node;
>  	struct device		*dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>  	void __iomem		*cfg_base;
>  	unsigned long		cfg_addr;
>  	bool			link_up;
> +	u32			version;
>  };
>  
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,44 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>  	return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>  
> +int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +			      int where, int size, u32 *val)
> +{
> +	void __iomem *addr;
> +	struct xgene_pcie_port *port = bus->sysdata;
> +
> +	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> +	if (!addr) {
> +		*val = ~0;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	*val = readl(addr);

Can't you just call pci_generic_config_read32() directly instead of
duplicating its code here?

> +	/*
> +	 * X-Gene v1 PCIe controller has a bug in Configuration Request
> +	 * Retry Status (CRS) logic:
> +	 *  When CPU tries to read Vendor ID and Device ID of not-existed
> +	 *  remote device, the controller returns 0xFFFF0001 instead of
> +	 *  0xFFFFFFFF; this will add significant delay in boot time as
> +	 *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
> +	 *  giving up.
> +	 * So for X-Gene v1 PCIe controllers, disable CRS capability
> +	 * advertisement by clearing CRS Software Visibility bit before
> +	 * returning the Root Capability value to the callers.
> +	 */
> +	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>  	.map_bus = xgene_pcie_map_bus,
> -	.read = pci_generic_config_read32,
> +	.read = xgene_pcie_config_read32,
>  	.write = pci_generic_config_write32,
>  };
>  
> @@ -483,6 +525,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>  	port->node = of_node_get(pdev->dev.of_node);
>  	port->dev = &pdev->dev;
>  
> +	port->version = XGENE_PCIE_IP_VER_UNKN;
> +	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +		port->version = XGENE_PCIE_IP_VER_1;
> +
>  	ret = xgene_pcie_map_reg(port, pdev);
>  	if (ret)
>  		return ret;
> -- 
> 1.9.1
> 

Re: [PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

Hi Bjorn,

On Fri, Jun 12, 2015 at 2:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Hi Duc,
>
> On Thu, Jun 11, 2015 at 01:08:14PM -0700, Duc Dang wrote:
>> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
>> Status (CRS) logic:
>>   When CPU tries to read Vendor ID and Device ID of not-existed
>>   remote device, the controller returns 0xFFFF0001 instead of
>>   0xFFFFFFFF; this will add significant delay in boot time as
>>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>>   giving up.
>
> OK, help me understand how this works.  I think this is related to the
> problem I reported where if the slot is empty, "lspci" doesn't show
> anything, not even the Root Port leading to the slot.
>
> I think this happens because when we try to read the Root Port's config
> space,
>
>   - the slot below the Root Port is empty
>   - the Root Port's link is down
>   - xgene_pcie_map_bus() returns NULL because !port->link_up
>   - pci_generic_config_read32() returns PCIBIOS_DEVICE_NOT_FOUND
>
> so it looks like the Root Port itself doesn't exist.
>
> I proposed to change xgene_pcie_map_bus() so it didn't check whether the
> link was up.  That change makes reads of the Root Port's config space work.
>
> After we learn the Root Port exists, the PCI core enumerates devices below
> the Root Port, e.g., on bus 01.  X-Gene advertises that it supports CRS, so
> we enable it.  When we try to read the Vendor ID of 01:00.0, there's no
> response from the device (because the slot is empty), and the Root Complex
> should complete the read by fabricating data of all ones, i.e., 0xFFFFFFFF.
> But apparently X-Gene supplies 0xFFFF0001 instead, which means "there's a
> device here, but it's not ready yet," so the PCI core retries the read for
> 60 seconds before timing out.
>
> This patch is basically a quirk that keeps X-Gene from advertising CRS
> support, so the PCI core won't enable CRS.  In the example above, I guess
> that means the Root Complex will supply 0xFFFFFFFF and the core will see
> that the slot is empty.
>
> But this patch leaves the "!port->link_up" test in xgene_pcie_map_bus().
> Doesn't that mean the core will still not discover the Root Port when the
> slot is empty?
>
> It seems to me that you would want both the xgene_pcie_map_bus() change and
> this patch.  The first would fix the problem that we don't enumerate Root
> Ports leading to empty slots, and the second would fix the problem that we
> enable CRS and timeout when enumerating below those Root Ports.
>
Yes, you are right. I plan to send a follow up patch after this one to
remove the port->link_up check in xgene_pcie_map_bus and make root
port discoverable even when the slot is empty. I can combine this
follow up patch with this one if you feel OK with it.

> One more question below:
>
>> So for X-Gene v1 PCIe controllers, disable CRS capability
>> advertisement by clearing CRS Software Visibility bit before
>> returning the Root Capability value to the callers. This is done
>> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
>> CFG read accesses to replace the generic default pci_generic_config_read32
>> function.
>>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>> ---
>>  drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> index ee082c0..741a253 100644
>> --- a/drivers/pci/host/pci-xgene.c
>> +++ b/drivers/pci/host/pci-xgene.c
>> @@ -59,6 +59,12 @@
>>  #define SZ_1T                                (SZ_1G*1024ULL)
>>  #define PIPE_PHY_RATE_RD(src)                ((0xc000 & (u32)(src)) >> 0xe)
>>
>> +#define ROOT_CAP_AND_CTRL            0x5C
>> +
>> +/* PCIe IP version */
>> +#define XGENE_PCIE_IP_VER_UNKN               0
>> +#define XGENE_PCIE_IP_VER_1          1
>> +
>>  struct xgene_pcie_port {
>>       struct device_node      *node;
>>       struct device           *dev;
>> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>>       void __iomem            *cfg_base;
>>       unsigned long           cfg_addr;
>>       bool                    link_up;
>> +     u32                     version;
>>  };
>>
>>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
>> @@ -140,9 +147,44 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>>       return xgene_pcie_get_cfg_base(bus) + offset;
>>  }
>>
>> +int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>> +                           int where, int size, u32 *val)
>> +{
>> +     void __iomem *addr;
>> +     struct xgene_pcie_port *port = bus->sysdata;
>> +
>> +     addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>> +     if (!addr) {
>> +             *val = ~0;
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +     }
>> +
>> +     *val = readl(addr);
>
> Can't you just call pci_generic_config_read32() directly instead of
> duplicating its code here?
>

Yes, I will replace above code with:

pci_generic_config_read32(bus, devfn, where & ~0x3, 4, *val)

to read 4 bytes to *val;

>> +     /*
>> +      * X-Gene v1 PCIe controller has a bug in Configuration Request
>> +      * Retry Status (CRS) logic:
>> +      *  When CPU tries to read Vendor ID and Device ID of not-existed
>> +      *  remote device, the controller returns 0xFFFF0001 instead of
>> +      *  0xFFFFFFFF; this will add significant delay in boot time as
>> +      *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
>> +      *  giving up.
>> +      * So for X-Gene v1 PCIe controllers, disable CRS capability
>> +      * advertisement by clearing CRS Software Visibility bit before
>> +      * returning the Root Capability value to the callers.
>> +      */
>> +     if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
>> +         ((where & ~0x3) == ROOT_CAP_AND_CTRL))
>> +             *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>> +
>> +     if (size <= 2)
>> +             *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  static struct pci_ops xgene_pcie_ops = {
>>       .map_bus = xgene_pcie_map_bus,
>> -     .read = pci_generic_config_read32,
>> +     .read = xgene_pcie_config_read32,
>>       .write = pci_generic_config_write32,
>>  };
>>
>> @@ -483,6 +525,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>>       port->node = of_node_get(pdev->dev.of_node);
>>       port->dev = &pdev->dev;
>>
>> +     port->version = XGENE_PCIE_IP_VER_UNKN;
>> +     if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
>> +             port->version = XGENE_PCIE_IP_VER_1;
>> +
>>       ret = xgene_pcie_map_reg(port, pdev);
>>       if (ret)
>>               return ret;
>> --
>> 1.9.1
>>

-- 
Regards,
Duc Dang.

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

Hi Bjorn,

On Fri, Jun 12, 2015 at 2:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Hi Duc,
>
> On Thu, Jun 11, 2015 at 01:08:14PM -0700, Duc Dang wrote:
>> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
>> Status (CRS) logic:
>>   When CPU tries to read Vendor ID and Device ID of not-existed
>>   remote device, the controller returns 0xFFFF0001 instead of
>>   0xFFFFFFFF; this will add significant delay in boot time as
>>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>>   giving up.
>
> OK, help me understand how this works.  I think this is related to the
> problem I reported where if the slot is empty, "lspci" doesn't show
> anything, not even the Root Port leading to the slot.
>
> I think this happens because when we try to read the Root Port's config
> space,
>
>   - the slot below the Root Port is empty
>   - the Root Port's link is down
>   - xgene_pcie_map_bus() returns NULL because !port->link_up
>   - pci_generic_config_read32() returns PCIBIOS_DEVICE_NOT_FOUND
>
> so it looks like the Root Port itself doesn't exist.
>
> I proposed to change xgene_pcie_map_bus() so it didn't check whether the
> link was up.  That change makes reads of the Root Port's config space work.
>
> After we learn the Root Port exists, the PCI core enumerates devices below
> the Root Port, e.g., on bus 01.  X-Gene advertises that it supports CRS, so
> we enable it.  When we try to read the Vendor ID of 01:00.0, there's no
> response from the device (because the slot is empty), and the Root Complex
> should complete the read by fabricating data of all ones, i.e., 0xFFFFFFFF.
> But apparently X-Gene supplies 0xFFFF0001 instead, which means "there's a
> device here, but it's not ready yet," so the PCI core retries the read for
> 60 seconds before timing out.
>
> This patch is basically a quirk that keeps X-Gene from advertising CRS
> support, so the PCI core won't enable CRS.  In the example above, I guess
> that means the Root Complex will supply 0xFFFFFFFF and the core will see
> that the slot is empty.
>
> But this patch leaves the "!port->link_up" test in xgene_pcie_map_bus().
> Doesn't that mean the core will still not discover the Root Port when the
> slot is empty?
>
> It seems to me that you would want both the xgene_pcie_map_bus() change and
> this patch.  The first would fix the problem that we don't enumerate Root
> Ports leading to empty slots, and the second would fix the problem that we
> enable CRS and timeout when enumerating below those Root Ports.
>
Yes, you are right. I plan to send a follow up patch after this one to
remove the port->link_up check in xgene_pcie_map_bus and make root
port discoverable even when the slot is empty. I can combine this
follow up patch with this one if you feel OK with it.

> One more question below:
>
>> So for X-Gene v1 PCIe controllers, disable CRS capability
>> advertisement by clearing CRS Software Visibility bit before
>> returning the Root Capability value to the callers. This is done
>> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
>> CFG read accesses to replace the generic default pci_generic_config_read32
>> function.
>>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>> ---
>>  drivers/pci/host/pci-xgene.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> index ee082c0..741a253 100644
>> --- a/drivers/pci/host/pci-xgene.c
>> +++ b/drivers/pci/host/pci-xgene.c
>> @@ -59,6 +59,12 @@
>>  #define SZ_1T                                (SZ_1G*1024ULL)
>>  #define PIPE_PHY_RATE_RD(src)                ((0xc000 & (u32)(src)) >> 0xe)
>>
>> +#define ROOT_CAP_AND_CTRL            0x5C
>> +
>> +/* PCIe IP version */
>> +#define XGENE_PCIE_IP_VER_UNKN               0
>> +#define XGENE_PCIE_IP_VER_1          1
>> +
>>  struct xgene_pcie_port {
>>       struct device_node      *node;
>>       struct device           *dev;
>> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>>       void __iomem            *cfg_base;
>>       unsigned long           cfg_addr;
>>       bool                    link_up;
>> +     u32                     version;
>>  };
>>
>>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
>> @@ -140,9 +147,44 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>>       return xgene_pcie_get_cfg_base(bus) + offset;
>>  }
>>
>> +int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>> +                           int where, int size, u32 *val)
>> +{
>> +     void __iomem *addr;
>> +     struct xgene_pcie_port *port = bus->sysdata;
>> +
>> +     addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>> +     if (!addr) {
>> +             *val = ~0;
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +     }
>> +
>> +     *val = readl(addr);
>
> Can't you just call pci_generic_config_read32() directly instead of
> duplicating its code here?
>

Yes, I will replace above code with:

pci_generic_config_read32(bus, devfn, where & ~0x3, 4, *val)

to read 4 bytes to *val;

>> +     /*
>> +      * X-Gene v1 PCIe controller has a bug in Configuration Request
>> +      * Retry Status (CRS) logic:
>> +      *  When CPU tries to read Vendor ID and Device ID of not-existed
>> +      *  remote device, the controller returns 0xFFFF0001 instead of
>> +      *  0xFFFFFFFF; this will add significant delay in boot time as
>> +      *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
>> +      *  giving up.
>> +      * So for X-Gene v1 PCIe controllers, disable CRS capability
>> +      * advertisement by clearing CRS Software Visibility bit before
>> +      * returning the Root Capability value to the callers.
>> +      */
>> +     if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
>> +         ((where & ~0x3) == ROOT_CAP_AND_CTRL))
>> +             *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>> +
>> +     if (size <= 2)
>> +             *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  static struct pci_ops xgene_pcie_ops = {
>>       .map_bus = xgene_pcie_map_bus,
>> -     .read = pci_generic_config_read32,
>> +     .read = xgene_pcie_config_read32,
>>       .write = pci_generic_config_write32,
>>  };
>>
>> @@ -483,6 +525,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>>       port->node = of_node_get(pdev->dev.of_node);
>>       port->dev = &pdev->dev;
>>
>> +     port->version = XGENE_PCIE_IP_VER_UNKN;
>> +     if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
>> +             port->version = XGENE_PCIE_IP_VER_1;
>> +
>>       ret = xgene_pcie_map_reg(port, pdev);
>>       if (ret)
>>               return ret;
>> --
>> 1.9.1
>>

-- 
Regards,
Duc Dang.

Re: [PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

On Fri, Jun 12, 2015 at 5:10 PM, Duc Dang <dhdang@apm.com> wrote:
> Hi Bjorn,
>
> On Fri, Jun 12, 2015 at 2:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Hi Duc,
>>
>> On Thu, Jun 11, 2015 at 01:08:14PM -0700, Duc Dang wrote:
>>> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
>>> Status (CRS) logic:
>>>   When CPU tries to read Vendor ID and Device ID of not-existed
>>>   remote device, the controller returns 0xFFFF0001 instead of
>>>   0xFFFFFFFF; this will add significant delay in boot time as
>>>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>>>   giving up.
>>
>> OK, help me understand how this works.  I think this is related to the
>> problem I reported where if the slot is empty, "lspci" doesn't show
>> anything, not even the Root Port leading to the slot.
>>
>> I think this happens because when we try to read the Root Port's config
>> space,
>>
>>   - the slot below the Root Port is empty
>>   - the Root Port's link is down
>>   - xgene_pcie_map_bus() returns NULL because !port->link_up
>>   - pci_generic_config_read32() returns PCIBIOS_DEVICE_NOT_FOUND
>>
>> so it looks like the Root Port itself doesn't exist.
>>
>> I proposed to change xgene_pcie_map_bus() so it didn't check whether the
>> link was up.  That change makes reads of the Root Port's config space work.
>>
>> After we learn the Root Port exists, the PCI core enumerates devices below
>> the Root Port, e.g., on bus 01.  X-Gene advertises that it supports CRS, so
>> we enable it.  When we try to read the Vendor ID of 01:00.0, there's no
>> response from the device (because the slot is empty), and the Root Complex
>> should complete the read by fabricating data of all ones, i.e., 0xFFFFFFFF.
>> But apparently X-Gene supplies 0xFFFF0001 instead, which means "there's a
>> device here, but it's not ready yet," so the PCI core retries the read for
>> 60 seconds before timing out.
>>
>> This patch is basically a quirk that keeps X-Gene from advertising CRS
>> support, so the PCI core won't enable CRS.  In the example above, I guess
>> that means the Root Complex will supply 0xFFFFFFFF and the core will see
>> that the slot is empty.
>>
>> But this patch leaves the "!port->link_up" test in xgene_pcie_map_bus().
>> Doesn't that mean the core will still not discover the Root Port when the
>> slot is empty?
>>
>> It seems to me that you would want both the xgene_pcie_map_bus() change and
>> this patch.  The first would fix the problem that we don't enumerate Root
>> Ports leading to empty slots, and the second would fix the problem that we
>> enable CRS and timeout when enumerating below those Root Ports.
>>
> Yes, you are right. I plan to send a follow up patch after this one to
> remove the port->link_up check in xgene_pcie_map_bus and make root
> port discoverable even when the slot is empty. I can combine this
> follow up patch with this one if you feel OK with it.

Two patches would be great.  I just didn't know about your plans for
the follow-up one.

Bjorn

[PATCH 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

On Fri, Jun 12, 2015 at 5:10 PM, Duc Dang <dhdang@apm.com> wrote:
> Hi Bjorn,
>
> On Fri, Jun 12, 2015 at 2:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Hi Duc,
>>
>> On Thu, Jun 11, 2015 at 01:08:14PM -0700, Duc Dang wrote:
>>> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
>>> Status (CRS) logic:
>>>   When CPU tries to read Vendor ID and Device ID of not-existed
>>>   remote device, the controller returns 0xFFFF0001 instead of
>>>   0xFFFFFFFF; this will add significant delay in boot time as
>>>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>>>   giving up.
>>
>> OK, help me understand how this works.  I think this is related to the
>> problem I reported where if the slot is empty, "lspci" doesn't show
>> anything, not even the Root Port leading to the slot.
>>
>> I think this happens because when we try to read the Root Port's config
>> space,
>>
>>   - the slot below the Root Port is empty
>>   - the Root Port's link is down
>>   - xgene_pcie_map_bus() returns NULL because !port->link_up
>>   - pci_generic_config_read32() returns PCIBIOS_DEVICE_NOT_FOUND
>>
>> so it looks like the Root Port itself doesn't exist.
>>
>> I proposed to change xgene_pcie_map_bus() so it didn't check whether the
>> link was up.  That change makes reads of the Root Port's config space work.
>>
>> After we learn the Root Port exists, the PCI core enumerates devices below
>> the Root Port, e.g., on bus 01.  X-Gene advertises that it supports CRS, so
>> we enable it.  When we try to read the Vendor ID of 01:00.0, there's no
>> response from the device (because the slot is empty), and the Root Complex
>> should complete the read by fabricating data of all ones, i.e., 0xFFFFFFFF.
>> But apparently X-Gene supplies 0xFFFF0001 instead, which means "there's a
>> device here, but it's not ready yet," so the PCI core retries the read for
>> 60 seconds before timing out.
>>
>> This patch is basically a quirk that keeps X-Gene from advertising CRS
>> support, so the PCI core won't enable CRS.  In the example above, I guess
>> that means the Root Complex will supply 0xFFFFFFFF and the core will see
>> that the slot is empty.
>>
>> But this patch leaves the "!port->link_up" test in xgene_pcie_map_bus().
>> Doesn't that mean the core will still not discover the Root Port when the
>> slot is empty?
>>
>> It seems to me that you would want both the xgene_pcie_map_bus() change and
>> this patch.  The first would fix the problem that we don't enumerate Root
>> Ports leading to empty slots, and the second would fix the problem that we
>> enable CRS and timeout when enumerating below those Root Ports.
>>
> Yes, you are right. I plan to send a follow up patch after this one to
> remove the port->link_up check in xgene_pcie_map_bus and make root
> port discoverable even when the slot is empty. I can combine this
> follow up patch with this one if you feel OK with it.

Two patches would be great.  I just didn't know about your plans for
the follow-up one.

Bjorn

[PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

X-Gene v1 PCIe controller has a bug in Configuration Request Retry
Status (CRS) logic:
  When CPU tries to read Vendor ID and Device ID of not-existed
  remote device, the controller returns 0xFFFF0001 instead of
  0xFFFFFFFF; this will add significant delay in boot time as
  pci_bus_read_dev_vendor_id will wait for 60 seconds before
  giving up.

So for X-Gene v1 PCIe controllers, disable CRS capability
advertisement by clearing CRS Software Visibility bit before
returning the Root Capability value to the callers. This is done
by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
CFG read accesses to replace the generic default pci_generic_config_read32
function.

v2 changes:
        Use pci_generic_config_read32 to implement xgene_pcie_config_read32

Signed-off-by: Duc Dang <dhdang@apm.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
---
 drivers/pci/host/pci-xgene.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index e6c8864..7e3cf28 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -59,6 +59,12 @@
 #define SZ_1T				(SZ_1G*1024ULL)
 #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
 
+#define ROOT_CAP_AND_CTRL		0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN		0
+#define XGENE_PCIE_IP_VER_1		1
+
 struct xgene_pcie_port {
 	struct device_node	*node;
 	struct device		*dev;
@@ -67,6 +73,7 @@ struct xgene_pcie_port {
 	void __iomem		*cfg_base;
 	unsigned long		cfg_addr;
 	bool			link_up;
+	u32			version;
 };
 
 static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
@@ -140,9 +147,39 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	return xgene_pcie_get_cfg_base(bus) + offset;
 }
 
+int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+			      int where, int size, u32 *val)
+{
+	struct xgene_pcie_port *port = bus->sysdata;
+
+	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
+	    PCIBIOS_SUCCESSFUL)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	/*
+	 * X-Gene v1 PCIe controller has a bug in Configuration Request
+	 * Retry Status (CRS) logic:
+	 *  When CPU tries to read Vendor ID and Device ID of not-existed
+	 *  remote device, the controller returns 0xFFFF0001 instead of
+	 *  0xFFFFFFFF; this will add significant delay in boot time as
+	 *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
+	 *  giving up.
+	 * So for X-Gene v1 PCIe controllers, disable CRS capability
+	 * advertisement by clearing CRS Software Visibility bit before
+	 * returning the Root Capability value to the callers.
+	 */
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops xgene_pcie_ops = {
 	.map_bus = xgene_pcie_map_bus,
-	.read = pci_generic_config_read32,
+	.read = xgene_pcie_config_read32,
 	.write = pci_generic_config_write32,
 };
 
@@ -500,6 +537,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	port->node = of_node_get(pdev->dev.of_node);
 	port->dev = &pdev->dev;
 
+	port->version = XGENE_PCIE_IP_VER_UNKN;
+	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
+		port->version = XGENE_PCIE_IP_VER_1;
+
 	ret = xgene_pcie_map_reg(port, pdev);
 	if (ret)
 		return ret;
-- 
1.9.1

[PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

X-Gene v1 PCIe controller has a bug in Configuration Request Retry
Status (CRS) logic:
  When CPU tries to read Vendor ID and Device ID of not-existed
  remote device, the controller returns 0xFFFF0001 instead of
  0xFFFFFFFF; this will add significant delay in boot time as
  pci_bus_read_dev_vendor_id will wait for 60 seconds before
  giving up.

So for X-Gene v1 PCIe controllers, disable CRS capability
advertisement by clearing CRS Software Visibility bit before
returning the Root Capability value to the callers. This is done
by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
CFG read accesses to replace the generic default pci_generic_config_read32
function.

v2 changes:
        Use pci_generic_config_read32 to implement xgene_pcie_config_read32

Signed-off-by: Duc Dang <dhdang@apm.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
---
 drivers/pci/host/pci-xgene.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index e6c8864..7e3cf28 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -59,6 +59,12 @@
 #define SZ_1T				(SZ_1G*1024ULL)
 #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
 
+#define ROOT_CAP_AND_CTRL		0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN		0
+#define XGENE_PCIE_IP_VER_1		1
+
 struct xgene_pcie_port {
 	struct device_node	*node;
 	struct device		*dev;
@@ -67,6 +73,7 @@ struct xgene_pcie_port {
 	void __iomem		*cfg_base;
 	unsigned long		cfg_addr;
 	bool			link_up;
+	u32			version;
 };
 
 static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
@@ -140,9 +147,39 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	return xgene_pcie_get_cfg_base(bus) + offset;
 }
 
+int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+			      int where, int size, u32 *val)
+{
+	struct xgene_pcie_port *port = bus->sysdata;
+
+	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
+	    PCIBIOS_SUCCESSFUL)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	/*
+	 * X-Gene v1 PCIe controller has a bug in Configuration Request
+	 * Retry Status (CRS) logic:
+	 *  When CPU tries to read Vendor ID and Device ID of not-existed
+	 *  remote device, the controller returns 0xFFFF0001 instead of
+	 *  0xFFFFFFFF; this will add significant delay in boot time as
+	 *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
+	 *  giving up.
+	 * So for X-Gene v1 PCIe controllers, disable CRS capability
+	 * advertisement by clearing CRS Software Visibility bit before
+	 * returning the Root Capability value to the callers.
+	 */
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops xgene_pcie_ops = {
 	.map_bus = xgene_pcie_map_bus,
-	.read = pci_generic_config_read32,
+	.read = xgene_pcie_config_read32,
 	.write = pci_generic_config_write32,
 };
 
@@ -500,6 +537,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	port->node = of_node_get(pdev->dev.of_node);
 	port->dev = &pdev->dev;
 
+	port->version = XGENE_PCIE_IP_VER_UNKN;
+	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
+		port->version = XGENE_PCIE_IP_VER_1;
+
 	ret = xgene_pcie_map_reg(port, pdev);
 	if (ret)
 		return ret;
-- 
1.9.1

Re: [PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

On Fri, Jun 12, 2015 at 05:35:57PM -0700, Duc Dang wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.
> 
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> v2 changes:
>         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
> Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>

I'd like to merge this (as amended below) for v4.2.  But I'm looking for an
ack from Tanmay, and I'm still waiting for the second patch to remove the
link_up test.

Bjorn


commit 49ac8c566d7a37f9f295e1fd4fbdd96517f22d55
Author: Duc Dang <dhdang@apm.com>
Date:   Fri Jun 12 17:35:57 2015 -0700

    PCI: xgene: Disable Configuration Request Retry Status for v1 silicon
    
    When a CPU reads the Vendor and Device ID of a non-existent device, the
    controller should fabricate return data of 0xFFFFFFFF.  Configuration
    Request Retry Status (CRS) is not applicable in this case because the
    device doesn't exist at all.
    
    The X-Gene v1 PCIe controller has a bug in the CRS logic such that when CRS
    is enabled, it fabricates return data of 0xFFFF0001 for this case, which
    means "the device exists but is not ready."  That causes the PCI core to
    retry the read until it times out after 60 seconds.
    
    Disable CRS capability advertisement by clearing the CRS Software
    Visibility bit in the Root Capabilities Register.
    
    [bhelgaas: changelog and comment]
    Tested-by: Ian Campbell <ian.campbell@citrix.com>
    Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
    Signed-off-by: Duc Dang <dhdang@apm.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 3e5a636..70af714 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -59,6 +59,12 @@
 #define SZ_1T				(SZ_1G*1024ULL)
 #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
 
+#define ROOT_CAP_AND_CTRL		0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN		0
+#define XGENE_PCIE_IP_VER_1		1
+
 struct xgene_pcie_port {
 	struct device_node	*node;
 	struct device		*dev;
@@ -67,6 +73,7 @@ struct xgene_pcie_port {
 	void __iomem		*cfg_base;
 	unsigned long		cfg_addr;
 	bool			link_up;
+	u32			version;
 };
 
 static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
@@ -140,9 +147,37 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	return xgene_pcie_get_cfg_base(bus) + offset;
 }
 
+static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct xgene_pcie_port *port = bus->sysdata;
+
+	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
+	    PCIBIOS_SUCCESSFUL)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	/*
+	 * The v1 controller has a bug in its Configuration Request
+	 * Retry Status (CRS) logic: when CRS is enabled and we read the
+	 * Vendor and Device ID of a non-existent device, the controller
+	 * fabricates return data of 0xFFFF0001 ("device exists but is not
+	 * ready") instead of 0xFFFFFFFF ("device does not exist").  This
+	 * causes the PCI core to retry the read until it times out.
+	 * Avoid this by not claiming to support CRS.
+	 */
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops xgene_pcie_ops = {
 	.map_bus = xgene_pcie_map_bus,
-	.read = pci_generic_config_read32,
+	.read = xgene_pcie_config_read32,
 	.write = pci_generic_config_write32,
 };
 
@@ -500,6 +535,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	port->node = of_node_get(pdev->dev.of_node);
 	port->dev = &pdev->dev;
 
+	port->version = XGENE_PCIE_IP_VER_UNKN;
+	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
+		port->version = XGENE_PCIE_IP_VER_1;
+
 	ret = xgene_pcie_map_reg(port, pdev);
 	if (ret)
 		return ret;

[PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

On Fri, Jun 12, 2015 at 05:35:57PM -0700, Duc Dang wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.
> 
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
> 
> v2 changes:
>         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
> Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>

I'd like to merge this (as amended below) for v4.2.  But I'm looking for an
ack from Tanmay, and I'm still waiting for the second patch to remove the
link_up test.

Bjorn


commit 49ac8c566d7a37f9f295e1fd4fbdd96517f22d55
Author: Duc Dang <dhdang@apm.com>
Date:   Fri Jun 12 17:35:57 2015 -0700

    PCI: xgene: Disable Configuration Request Retry Status for v1 silicon
    
    When a CPU reads the Vendor and Device ID of a non-existent device, the
    controller should fabricate return data of 0xFFFFFFFF.  Configuration
    Request Retry Status (CRS) is not applicable in this case because the
    device doesn't exist at all.
    
    The X-Gene v1 PCIe controller has a bug in the CRS logic such that when CRS
    is enabled, it fabricates return data of 0xFFFF0001 for this case, which
    means "the device exists but is not ready."  That causes the PCI core to
    retry the read until it times out after 60 seconds.
    
    Disable CRS capability advertisement by clearing the CRS Software
    Visibility bit in the Root Capabilities Register.
    
    [bhelgaas: changelog and comment]
    Tested-by: Ian Campbell <ian.campbell@citrix.com>
    Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
    Signed-off-by: Duc Dang <dhdang@apm.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 3e5a636..70af714 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -59,6 +59,12 @@
 #define SZ_1T				(SZ_1G*1024ULL)
 #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
 
+#define ROOT_CAP_AND_CTRL		0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN		0
+#define XGENE_PCIE_IP_VER_1		1
+
 struct xgene_pcie_port {
 	struct device_node	*node;
 	struct device		*dev;
@@ -67,6 +73,7 @@ struct xgene_pcie_port {
 	void __iomem		*cfg_base;
 	unsigned long		cfg_addr;
 	bool			link_up;
+	u32			version;
 };
 
 static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
@@ -140,9 +147,37 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 	return xgene_pcie_get_cfg_base(bus) + offset;
 }
 
+static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct xgene_pcie_port *port = bus->sysdata;
+
+	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
+	    PCIBIOS_SUCCESSFUL)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	/*
+	 * The v1 controller has a bug in its Configuration Request
+	 * Retry Status (CRS) logic: when CRS is enabled and we read the
+	 * Vendor and Device ID of a non-existent device, the controller
+	 * fabricates return data of 0xFFFF0001 ("device exists but is not
+	 * ready") instead of 0xFFFFFFFF ("device does not exist").  This
+	 * causes the PCI core to retry the read until it times out.
+	 * Avoid this by not claiming to support CRS.
+	 */
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static struct pci_ops xgene_pcie_ops = {
 	.map_bus = xgene_pcie_map_bus,
-	.read = pci_generic_config_read32,
+	.read = xgene_pcie_config_read32,
 	.write = pci_generic_config_write32,
 };
 
@@ -500,6 +535,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	port->node = of_node_get(pdev->dev.of_node);
 	port->dev = &pdev->dev;
 
+	port->version = XGENE_PCIE_IP_VER_UNKN;
+	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
+		port->version = XGENE_PCIE_IP_VER_1;
+
 	ret = xgene_pcie_map_reg(port, pdev);
 	if (ret)
 		return ret;

Re: [PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Tanmay Inamdar]

Hi,

On Fri, Jun 12, 2015 at 5:35 PM, Duc Dang <dhdang@apm.com> wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.
>
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
>
> v2 changes:
>         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
>
> Signed-off-by: Duc Dang <dhdang@apm.com>
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
> Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>

 Acked-by: Tanmay Inamdar <tinamdar@apm.com

> ---
>  drivers/pci/host/pci-xgene.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index e6c8864..7e3cf28 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T                          (SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)          ((0xc000 & (u32)(src)) >> 0xe)
>
> +#define ROOT_CAP_AND_CTRL              0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN         0
> +#define XGENE_PCIE_IP_VER_1            1
> +
>  struct xgene_pcie_port {
>         struct device_node      *node;
>         struct device           *dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>         void __iomem            *cfg_base;
>         unsigned long           cfg_addr;
>         bool                    link_up;
> +       u32                     version;
>  };
>
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,39 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>         return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>
> +int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +                             int where, int size, u32 *val)
> +{
> +       struct xgene_pcie_port *port = bus->sysdata;
> +
> +       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +           PCIBIOS_SUCCESSFUL)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       /*
> +        * X-Gene v1 PCIe controller has a bug in Configuration Request
> +        * Retry Status (CRS) logic:
> +        *  When CPU tries to read Vendor ID and Device ID of not-existed
> +        *  remote device, the controller returns 0xFFFF0001 instead of
> +        *  0xFFFFFFFF; this will add significant delay in boot time as
> +        *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
> +        *  giving up.
> +        * So for X-Gene v1 PCIe controllers, disable CRS capability
> +        * advertisement by clearing CRS Software Visibility bit before
> +        * returning the Root Capability value to the callers.
> +        */
> +       if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +           ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +               *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +       if (size <= 2)
> +               *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>         .map_bus = xgene_pcie_map_bus,
> -       .read = pci_generic_config_read32,
> +       .read = xgene_pcie_config_read32,
>         .write = pci_generic_config_write32,
>  };
>
> @@ -500,6 +537,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>         port->node = of_node_get(pdev->dev.of_node);
>         port->dev = &pdev->dev;
>
> +       port->version = XGENE_PCIE_IP_VER_UNKN;
> +       if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +               port->version = XGENE_PCIE_IP_VER_1;
> +
>         ret = xgene_pcie_map_reg(port, pdev);
>         if (ret)
>                 return ret;
> --
> 1.9.1
>

[PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Tanmay Inamdar]

Hi,

On Fri, Jun 12, 2015 at 5:35 PM, Duc Dang <dhdang@apm.com> wrote:
> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> Status (CRS) logic:
>   When CPU tries to read Vendor ID and Device ID of not-existed
>   remote device, the controller returns 0xFFFF0001 instead of
>   0xFFFFFFFF; this will add significant delay in boot time as
>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>   giving up.
>
> So for X-Gene v1 PCIe controllers, disable CRS capability
> advertisement by clearing CRS Software Visibility bit before
> returning the Root Capability value to the callers. This is done
> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> CFG read accesses to replace the generic default pci_generic_config_read32
> function.
>
> v2 changes:
>         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
>
> Signed-off-by: Duc Dang <dhdang@apm.com>
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
> Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>

 Acked-by: Tanmay Inamdar <tinamdar@apm.com

> ---
>  drivers/pci/host/pci-xgene.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index e6c8864..7e3cf28 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T                          (SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)          ((0xc000 & (u32)(src)) >> 0xe)
>
> +#define ROOT_CAP_AND_CTRL              0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN         0
> +#define XGENE_PCIE_IP_VER_1            1
> +
>  struct xgene_pcie_port {
>         struct device_node      *node;
>         struct device           *dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>         void __iomem            *cfg_base;
>         unsigned long           cfg_addr;
>         bool                    link_up;
> +       u32                     version;
>  };
>
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,39 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>         return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>
> +int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +                             int where, int size, u32 *val)
> +{
> +       struct xgene_pcie_port *port = bus->sysdata;
> +
> +       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +           PCIBIOS_SUCCESSFUL)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       /*
> +        * X-Gene v1 PCIe controller has a bug in Configuration Request
> +        * Retry Status (CRS) logic:
> +        *  When CPU tries to read Vendor ID and Device ID of not-existed
> +        *  remote device, the controller returns 0xFFFF0001 instead of
> +        *  0xFFFFFFFF; this will add significant delay in boot time as
> +        *  pci_bus_read_dev_vendor_id will wait for 60 seconds before
> +        *  giving up.
> +        * So for X-Gene v1 PCIe controllers, disable CRS capability
> +        * advertisement by clearing CRS Software Visibility bit before
> +        * returning the Root Capability value to the callers.
> +        */
> +       if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +           ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +               *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +       if (size <= 2)
> +               *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>         .map_bus = xgene_pcie_map_bus,
> -       .read = pci_generic_config_read32,
> +       .read = xgene_pcie_config_read32,
>         .write = pci_generic_config_write32,
>  };
>
> @@ -500,6 +537,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>         port->node = of_node_get(pdev->dev.of_node);
>         port->dev = &pdev->dev;
>
> +       port->version = XGENE_PCIE_IP_VER_UNKN;
> +       if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +               port->version = XGENE_PCIE_IP_VER_1;
> +
>         ret = xgene_pcie_map_reg(port, pdev);
>         if (ret)
>                 return ret;
> --
> 1.9.1
>

Re: [PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

On Thu, Jun 18, 2015 at 10:55 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Jun 12, 2015 at 05:35:57PM -0700, Duc Dang wrote:
>> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
>> Status (CRS) logic:
>>   When CPU tries to read Vendor ID and Device ID of not-existed
>>   remote device, the controller returns 0xFFFF0001 instead of
>>   0xFFFFFFFF; this will add significant delay in boot time as
>>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>>   giving up.
>>
>> So for X-Gene v1 PCIe controllers, disable CRS capability
>> advertisement by clearing CRS Software Visibility bit before
>> returning the Root Capability value to the callers. This is done
>> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
>> CFG read accesses to replace the generic default pci_generic_config_read32
>> function.
>>
>> v2 changes:
>>         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
>>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>> Tested-by: Ian Campbell <ian.campbell@citrix.com>
>> Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
>
> I'd like to merge this (as amended below) for v4.2.  But I'm looking for an
> ack from Tanmay, and I'm still waiting for the second patch to remove the
> link_up test.

I will send you the second patch to remove the link_up test shortly.

>
> Bjorn
>
>
> commit 49ac8c566d7a37f9f295e1fd4fbdd96517f22d55
> Author: Duc Dang <dhdang@apm.com>
> Date:   Fri Jun 12 17:35:57 2015 -0700
>
>     PCI: xgene: Disable Configuration Request Retry Status for v1 silicon
>
>     When a CPU reads the Vendor and Device ID of a non-existent device, the
>     controller should fabricate return data of 0xFFFFFFFF.  Configuration
>     Request Retry Status (CRS) is not applicable in this case because the
>     device doesn't exist at all.
>
>     The X-Gene v1 PCIe controller has a bug in the CRS logic such that when CRS
>     is enabled, it fabricates return data of 0xFFFF0001 for this case, which
>     means "the device exists but is not ready."  That causes the PCI core to
>     retry the read until it times out after 60 seconds.
>
>     Disable CRS capability advertisement by clearing the CRS Software
>     Visibility bit in the Root Capabilities Register.
>
>     [bhelgaas: changelog and comment]
>     Tested-by: Ian Campbell <ian.campbell@citrix.com>
>     Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
>     Signed-off-by: Duc Dang <dhdang@apm.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index 3e5a636..70af714 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T                          (SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)          ((0xc000 & (u32)(src)) >> 0xe)
>
> +#define ROOT_CAP_AND_CTRL              0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN         0
> +#define XGENE_PCIE_IP_VER_1            1
> +
>  struct xgene_pcie_port {
>         struct device_node      *node;
>         struct device           *dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>         void __iomem            *cfg_base;
>         unsigned long           cfg_addr;
>         bool                    link_up;
> +       u32                     version;
>  };
>
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,37 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>         return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>
> +static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +                                   int where, int size, u32 *val)
> +{
> +       struct xgene_pcie_port *port = bus->sysdata;
> +
> +       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +           PCIBIOS_SUCCESSFUL)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       /*
> +        * The v1 controller has a bug in its Configuration Request
> +        * Retry Status (CRS) logic: when CRS is enabled and we read the
> +        * Vendor and Device ID of a non-existent device, the controller
> +        * fabricates return data of 0xFFFF0001 ("device exists but is not
> +        * ready") instead of 0xFFFFFFFF ("device does not exist").  This
> +        * causes the PCI core to retry the read until it times out.
> +        * Avoid this by not claiming to support CRS.
> +        */
> +       if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +           ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +               *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +       if (size <= 2)
> +               *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>         .map_bus = xgene_pcie_map_bus,
> -       .read = pci_generic_config_read32,
> +       .read = xgene_pcie_config_read32,
>         .write = pci_generic_config_write32,
>  };
>
> @@ -500,6 +535,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>         port->node = of_node_get(pdev->dev.of_node);
>         port->dev = &pdev->dev;
>
> +       port->version = XGENE_PCIE_IP_VER_UNKN;
> +       if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +               port->version = XGENE_PCIE_IP_VER_1;
> +
>         ret = xgene_pcie_map_reg(port, pdev);
>         if (ret)
>                 return ret;



-- 
Regards,
Duc Dang.

[PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Duc Dang]

On Thu, Jun 18, 2015 at 10:55 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Jun 12, 2015 at 05:35:57PM -0700, Duc Dang wrote:
>> X-Gene v1 PCIe controller has a bug in Configuration Request Retry
>> Status (CRS) logic:
>>   When CPU tries to read Vendor ID and Device ID of not-existed
>>   remote device, the controller returns 0xFFFF0001 instead of
>>   0xFFFFFFFF; this will add significant delay in boot time as
>>   pci_bus_read_dev_vendor_id will wait for 60 seconds before
>>   giving up.
>>
>> So for X-Gene v1 PCIe controllers, disable CRS capability
>> advertisement by clearing CRS Software Visibility bit before
>> returning the Root Capability value to the callers. This is done
>> by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
>> CFG read accesses to replace the generic default pci_generic_config_read32
>> function.
>>
>> v2 changes:
>>         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
>>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>> Tested-by: Ian Campbell <ian.campbell@citrix.com>
>> Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
>
> I'd like to merge this (as amended below) for v4.2.  But I'm looking for an
> ack from Tanmay, and I'm still waiting for the second patch to remove the
> link_up test.

I will send you the second patch to remove the link_up test shortly.

>
> Bjorn
>
>
> commit 49ac8c566d7a37f9f295e1fd4fbdd96517f22d55
> Author: Duc Dang <dhdang@apm.com>
> Date:   Fri Jun 12 17:35:57 2015 -0700
>
>     PCI: xgene: Disable Configuration Request Retry Status for v1 silicon
>
>     When a CPU reads the Vendor and Device ID of a non-existent device, the
>     controller should fabricate return data of 0xFFFFFFFF.  Configuration
>     Request Retry Status (CRS) is not applicable in this case because the
>     device doesn't exist at all.
>
>     The X-Gene v1 PCIe controller has a bug in the CRS logic such that when CRS
>     is enabled, it fabricates return data of 0xFFFF0001 for this case, which
>     means "the device exists but is not ready."  That causes the PCI core to
>     retry the read until it times out after 60 seconds.
>
>     Disable CRS capability advertisement by clearing the CRS Software
>     Visibility bit in the Root Capabilities Register.
>
>     [bhelgaas: changelog and comment]
>     Tested-by: Ian Campbell <ian.campbell@citrix.com>
>     Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
>     Signed-off-by: Duc Dang <dhdang@apm.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index 3e5a636..70af714 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T                          (SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)          ((0xc000 & (u32)(src)) >> 0xe)
>
> +#define ROOT_CAP_AND_CTRL              0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN         0
> +#define XGENE_PCIE_IP_VER_1            1
> +
>  struct xgene_pcie_port {
>         struct device_node      *node;
>         struct device           *dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>         void __iomem            *cfg_base;
>         unsigned long           cfg_addr;
>         bool                    link_up;
> +       u32                     version;
>  };
>
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,37 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>         return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>
> +static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +                                   int where, int size, u32 *val)
> +{
> +       struct xgene_pcie_port *port = bus->sysdata;
> +
> +       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +           PCIBIOS_SUCCESSFUL)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       /*
> +        * The v1 controller has a bug in its Configuration Request
> +        * Retry Status (CRS) logic: when CRS is enabled and we read the
> +        * Vendor and Device ID of a non-existent device, the controller
> +        * fabricates return data of 0xFFFF0001 ("device exists but is not
> +        * ready") instead of 0xFFFFFFFF ("device does not exist").  This
> +        * causes the PCI core to retry the read until it times out.
> +        * Avoid this by not claiming to support CRS.
> +        */
> +       if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +           ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +               *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +       if (size <= 2)
> +               *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>         .map_bus = xgene_pcie_map_bus,
> -       .read = pci_generic_config_read32,
> +       .read = xgene_pcie_config_read32,
>         .write = pci_generic_config_write32,
>  };
>
> @@ -500,6 +535,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>         port->node = of_node_get(pdev->dev.of_node);
>         port->dev = &pdev->dev;
>
> +       port->version = XGENE_PCIE_IP_VER_UNKN;
> +       if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +               port->version = XGENE_PCIE_IP_VER_1;
> +
>         ret = xgene_pcie_map_reg(port, pdev);
>         if (ret)
>                 return ret;



-- 
Regards,
Duc Dang.

Re: [PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

On Thu, Jun 18, 2015 at 12:55:53PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 12, 2015 at 05:35:57PM -0700, Duc Dang wrote:
> > X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> > Status (CRS) logic:
> >   When CPU tries to read Vendor ID and Device ID of not-existed
> >   remote device, the controller returns 0xFFFF0001 instead of
> >   0xFFFFFFFF; this will add significant delay in boot time as
> >   pci_bus_read_dev_vendor_id will wait for 60 seconds before
> >   giving up.
> > 
> > So for X-Gene v1 PCIe controllers, disable CRS capability
> > advertisement by clearing CRS Software Visibility bit before
> > returning the Root Capability value to the callers. This is done
> > by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> > CFG read accesses to replace the generic default pci_generic_config_read32
> > function.
> > 
> > v2 changes:
> >         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
> > 
> > Signed-off-by: Duc Dang <dhdang@apm.com>
> > Tested-by: Ian Campbell <ian.campbell@citrix.com>
> > Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
> 
> I'd like to merge this (as amended below) for v4.2.  But I'm looking for an
> ack from Tanmay, and I'm still waiting for the second patch to remove the
> link_up test.
> 
> Bjorn

Applied to pci/host-xgene for v4.2, thanks very much!

> commit 49ac8c566d7a37f9f295e1fd4fbdd96517f22d55
> Author: Duc Dang <dhdang@apm.com>
> Date:   Fri Jun 12 17:35:57 2015 -0700
> 
>     PCI: xgene: Disable Configuration Request Retry Status for v1 silicon
>     
>     When a CPU reads the Vendor and Device ID of a non-existent device, the
>     controller should fabricate return data of 0xFFFFFFFF.  Configuration
>     Request Retry Status (CRS) is not applicable in this case because the
>     device doesn't exist at all.
>     
>     The X-Gene v1 PCIe controller has a bug in the CRS logic such that when CRS
>     is enabled, it fabricates return data of 0xFFFF0001 for this case, which
>     means "the device exists but is not ready."  That causes the PCI core to
>     retry the read until it times out after 60 seconds.
>     
>     Disable CRS capability advertisement by clearing the CRS Software
>     Visibility bit in the Root Capabilities Register.
>     
>     [bhelgaas: changelog and comment]
>     Tested-by: Ian Campbell <ian.campbell@citrix.com>
>     Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
>     Signed-off-by: Duc Dang <dhdang@apm.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index 3e5a636..70af714 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T				(SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
>  
> +#define ROOT_CAP_AND_CTRL		0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN		0
> +#define XGENE_PCIE_IP_VER_1		1
> +
>  struct xgene_pcie_port {
>  	struct device_node	*node;
>  	struct device		*dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>  	void __iomem		*cfg_base;
>  	unsigned long		cfg_addr;
>  	bool			link_up;
> +	u32			version;
>  };
>  
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,37 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>  	return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>  
> +static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct xgene_pcie_port *port = bus->sysdata;
> +
> +	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +	    PCIBIOS_SUCCESSFUL)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	/*
> +	 * The v1 controller has a bug in its Configuration Request
> +	 * Retry Status (CRS) logic: when CRS is enabled and we read the
> +	 * Vendor and Device ID of a non-existent device, the controller
> +	 * fabricates return data of 0xFFFF0001 ("device exists but is not
> +	 * ready") instead of 0xFFFFFFFF ("device does not exist").  This
> +	 * causes the PCI core to retry the read until it times out.
> +	 * Avoid this by not claiming to support CRS.
> +	 */
> +	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>  	.map_bus = xgene_pcie_map_bus,
> -	.read = pci_generic_config_read32,
> +	.read = xgene_pcie_config_read32,
>  	.write = pci_generic_config_write32,
>  };
>  
> @@ -500,6 +535,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>  	port->node = of_node_get(pdev->dev.of_node);
>  	port->dev = &pdev->dev;
>  
> +	port->version = XGENE_PCIE_IP_VER_UNKN;
> +	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +		port->version = XGENE_PCIE_IP_VER_1;
> +
>  	ret = xgene_pcie_map_reg(port, pdev);
>  	if (ret)
>  		return ret;

[PATCH v2 1/1] PCI: X-Gene: Disable Configuration Request Retry Status for X-Gene v1 PCIe

[Bjorn Helgaas]

On Thu, Jun 18, 2015 at 12:55:53PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 12, 2015 at 05:35:57PM -0700, Duc Dang wrote:
> > X-Gene v1 PCIe controller has a bug in Configuration Request Retry
> > Status (CRS) logic:
> >   When CPU tries to read Vendor ID and Device ID of not-existed
> >   remote device, the controller returns 0xFFFF0001 instead of
> >   0xFFFFFFFF; this will add significant delay in boot time as
> >   pci_bus_read_dev_vendor_id will wait for 60 seconds before
> >   giving up.
> > 
> > So for X-Gene v1 PCIe controllers, disable CRS capability
> > advertisement by clearing CRS Software Visibility bit before
> > returning the Root Capability value to the callers. This is done
> > by implementing X-Gene PCIe specific xgene_pcie_config_read32 for
> > CFG read accesses to replace the generic default pci_generic_config_read32
> > function.
> > 
> > v2 changes:
> >         Use pci_generic_config_read32 to implement xgene_pcie_config_read32
> > 
> > Signed-off-by: Duc Dang <dhdang@apm.com>
> > Tested-by: Ian Campbell <ian.campbell@citrix.com>
> > Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
> 
> I'd like to merge this (as amended below) for v4.2.  But I'm looking for an
> ack from Tanmay, and I'm still waiting for the second patch to remove the
> link_up test.
> 
> Bjorn

Applied to pci/host-xgene for v4.2, thanks very much!

> commit 49ac8c566d7a37f9f295e1fd4fbdd96517f22d55
> Author: Duc Dang <dhdang@apm.com>
> Date:   Fri Jun 12 17:35:57 2015 -0700
> 
>     PCI: xgene: Disable Configuration Request Retry Status for v1 silicon
>     
>     When a CPU reads the Vendor and Device ID of a non-existent device, the
>     controller should fabricate return data of 0xFFFFFFFF.  Configuration
>     Request Retry Status (CRS) is not applicable in this case because the
>     device doesn't exist at all.
>     
>     The X-Gene v1 PCIe controller has a bug in the CRS logic such that when CRS
>     is enabled, it fabricates return data of 0xFFFF0001 for this case, which
>     means "the device exists but is not ready."  That causes the PCI core to
>     retry the read until it times out after 60 seconds.
>     
>     Disable CRS capability advertisement by clearing the CRS Software
>     Visibility bit in the Root Capabilities Register.
>     
>     [bhelgaas: changelog and comment]
>     Tested-by: Ian Campbell <ian.campbell@citrix.com>
>     Tested-by: Marcin Juszkiewicz <mjuszkiewicz@redhat.com>
>     Signed-off-by: Duc Dang <dhdang@apm.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index 3e5a636..70af714 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -59,6 +59,12 @@
>  #define SZ_1T				(SZ_1G*1024ULL)
>  #define PIPE_PHY_RATE_RD(src)		((0xc000 & (u32)(src)) >> 0xe)
>  
> +#define ROOT_CAP_AND_CTRL		0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN		0
> +#define XGENE_PCIE_IP_VER_1		1
> +
>  struct xgene_pcie_port {
>  	struct device_node	*node;
>  	struct device		*dev;
> @@ -67,6 +73,7 @@ struct xgene_pcie_port {
>  	void __iomem		*cfg_base;
>  	unsigned long		cfg_addr;
>  	bool			link_up;
> +	u32			version;
>  };
>  
>  static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
> @@ -140,9 +147,37 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>  	return xgene_pcie_get_cfg_base(bus) + offset;
>  }
>  
> +static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct xgene_pcie_port *port = bus->sysdata;
> +
> +	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +	    PCIBIOS_SUCCESSFUL)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	/*
> +	 * The v1 controller has a bug in its Configuration Request
> +	 * Retry Status (CRS) logic: when CRS is enabled and we read the
> +	 * Vendor and Device ID of a non-existent device, the controller
> +	 * fabricates return data of 0xFFFF0001 ("device exists but is not
> +	 * ready") instead of 0xFFFFFFFF ("device does not exist").  This
> +	 * causes the PCI core to retry the read until it times out.
> +	 * Avoid this by not claiming to support CRS.
> +	 */
> +	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static struct pci_ops xgene_pcie_ops = {
>  	.map_bus = xgene_pcie_map_bus,
> -	.read = pci_generic_config_read32,
> +	.read = xgene_pcie_config_read32,
>  	.write = pci_generic_config_write32,
>  };
>  
> @@ -500,6 +535,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>  	port->node = of_node_get(pdev->dev.of_node);
>  	port->dev = &pdev->dev;
>  
> +	port->version = XGENE_PCIE_IP_VER_UNKN;
> +	if (of_device_is_compatible(port->node, "apm,xgene-pcie"))
> +		port->version = XGENE_PCIE_IP_VER_1;
> +
>  	ret = xgene_pcie_map_reg(port, pdev);
>  	if (ret)
>  		return ret;