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

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

[Oza Pawandeep]

PCI: iproc: Retry request when CRS returned from EP Above patch adds
support for CRS in PCI RC driver, otherwise if not handled at lower
level, the user space PMD (poll mode drivers) can timeout.

PCI: iproc: add device shutdown for PCI RC This fixes the issue where
certian PCI endpoints are not getting detected on Stingray SOC after
reboot.

Changes Since v7:
Factor out the ep config access code.

Changes Since v6:
Rebased patches on top of Lorenzo's patches.
Bjorn's comments addressed.
now the confg retry returns 0xffffffff as data.
Added reference to PCIe spec and iproc Controller spec in Changelog.

Changes Since v5:
Ray's comments addressed.

Changes Since v4:
Bjorn's comments addressed.

Changes Since v3:
[re-send]

Changes Since v2:
Fix compilation errors for pcie-iproc-platform.ko which was caught
by kbuild.

Oza Pawandeep (3):
  PCI: iproc: factor-out ep configuration access
  PCI: iproc: Retry request when CRS returned from EP
  PCI: iproc: add device shutdown for PCI RC

 drivers/pci/host/pcie-iproc-platform.c |   8 ++
 drivers/pci/host/pcie-iproc.c          | 143 ++++++++++++++++++++++++++-------
 drivers/pci/host/pcie-iproc.h          |   1 +
 3 files changed, 124 insertions(+), 28 deletions(-)

-- 
1.9.1

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

[Oza Pawandeep]

PCI: iproc: Retry request when CRS returned from EP Above patch adds
support for CRS in PCI RC driver, otherwise if not handled at lower
level, the user space PMD (poll mode drivers) can timeout.

PCI: iproc: add device shutdown for PCI RC This fixes the issue where
certian PCI endpoints are not getting detected on Stingray SOC after
reboot.

Changes Since v7:
Factor out the ep config access code.

Changes Since v6:
Rebased patches on top of Lorenzo's patches.
Bjorn's comments addressed.
now the confg retry returns 0xffffffff as data.
Added reference to PCIe spec and iproc Controller spec in Changelog.

Changes Since v5:
Ray's comments addressed.

Changes Since v4:
Bjorn's comments addressed.

Changes Since v3:
[re-send]

Changes Since v2:
Fix compilation errors for pcie-iproc-platform.ko which was caught
by kbuild.

Oza Pawandeep (3):
  PCI: iproc: factor-out ep configuration access
  PCI: iproc: Retry request when CRS returned from EP
  PCI: iproc: add device shutdown for PCI RC

 drivers/pci/host/pcie-iproc-platform.c |   8 ++
 drivers/pci/host/pcie-iproc.c          | 143 ++++++++++++++++++++++++++-------
 drivers/pci/host/pcie-iproc.h          |   1 +
 3 files changed, 124 insertions(+), 28 deletions(-)

-- 
1.9.1

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

[Oza Pawandeep]

PCI: iproc: Retry request when CRS returned from EP Above patch adds
support for CRS in PCI RC driver, otherwise if not handled at lower
level, the user space PMD (poll mode drivers) can timeout.

PCI: iproc: add device shutdown for PCI RC This fixes the issue where
certian PCI endpoints are not getting detected on Stingray SOC after
reboot.

Changes Since v7:
Factor out the ep config access code.

Changes Since v6:
Rebased patches on top of Lorenzo's patches.
Bjorn's comments addressed.
now the confg retry returns 0xffffffff as data.
Added reference to PCIe spec and iproc Controller spec in Changelog.

Changes Since v5:
Ray's comments addressed.

Changes Since v4:
Bjorn's comments addressed.

Changes Since v3:
[re-send]

Changes Since v2:
Fix compilation errors for pcie-iproc-platform.ko which was caught
by kbuild.

Oza Pawandeep (3):
  PCI: iproc: factor-out ep configuration access
  PCI: iproc: Retry request when CRS returned from EP
  PCI: iproc: add device shutdown for PCI RC

 drivers/pci/host/pcie-iproc-platform.c |   8 ++
 drivers/pci/host/pcie-iproc.c          | 143 ++++++++++++++++++++++++++-------
 drivers/pci/host/pcie-iproc.h          |   1 +
 3 files changed, 124 insertions(+), 28 deletions(-)

-- 
1.9.1

[PATCH v8 1/3] PCI: iproc: factor out ep configuration access

[Oza Pawandeep]

This patch factors out ep configuration access
as a separate function.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index c574863..61d9be6 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -448,6 +448,31 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
 	}
 }
 
+static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
+					       unsigned int busno,
+					       unsigned int slot,
+					       unsigned int fn,
+					       int where)
+{
+	u16 offset;
+	u32 val;
+
+	/* EP device access */
+	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
+		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
+		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
+		(where & CFG_ADDR_REG_NUM_MASK) |
+		(1 & CFG_ADDR_CFG_TYPE_MASK);
+
+	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
+	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
+
+	if (iproc_pcie_reg_is_invalid(offset))
+		return NULL;
+
+	return (pcie->base + offset);
+}
+
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -459,7 +484,6 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
 {
 	unsigned slot = PCI_SLOT(devfn);
 	unsigned fn = PCI_FUNC(devfn);
-	u32 val;
 	u16 offset;
 
 	/* root complex access */
@@ -484,18 +508,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
 		if (slot > 0)
 			return NULL;
 
-	/* EP device access */
-	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
-		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
-		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
-		(where & CFG_ADDR_REG_NUM_MASK) |
-		(1 & CFG_ADDR_CFG_TYPE_MASK);
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
-	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
-	if (iproc_pcie_reg_is_invalid(offset))
-		return NULL;
-	else
-		return (pcie->base + offset);
+	return iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
 }
 
 static void __iomem *iproc_pcie_bus_map_cfg_bus(struct pci_bus *bus,
-- 
1.9.1

[PATCH v8 1/3] PCI: iproc: factor out ep configuration access

[Oza Pawandeep]

This patch factors out ep configuration access
as a separate function.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index c574863..61d9be6 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -448,6 +448,31 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
 	}
 }
 
+static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
+					       unsigned int busno,
+					       unsigned int slot,
+					       unsigned int fn,
+					       int where)
+{
+	u16 offset;
+	u32 val;
+
+	/* EP device access */
+	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
+		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
+		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
+		(where & CFG_ADDR_REG_NUM_MASK) |
+		(1 & CFG_ADDR_CFG_TYPE_MASK);
+
+	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
+	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
+
+	if (iproc_pcie_reg_is_invalid(offset))
+		return NULL;
+
+	return (pcie->base + offset);
+}
+
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -459,7 +484,6 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
 {
 	unsigned slot = PCI_SLOT(devfn);
 	unsigned fn = PCI_FUNC(devfn);
-	u32 val;
 	u16 offset;
 
 	/* root complex access */
@@ -484,18 +508,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
 		if (slot > 0)
 			return NULL;
 
-	/* EP device access */
-	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
-		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
-		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
-		(where & CFG_ADDR_REG_NUM_MASK) |
-		(1 & CFG_ADDR_CFG_TYPE_MASK);
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
-	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
-	if (iproc_pcie_reg_is_invalid(offset))
-		return NULL;
-	else
-		return (pcie->base + offset);
+	return iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
 }
 
 static void __iomem *iproc_pcie_bus_map_cfg_bus(struct pci_bus *bus,
-- 
1.9.1

[PATCH v8 1/3] PCI: iproc: factor out ep configuration access

[Oza Pawandeep]

This patch factors out ep configuration access
as a separate function.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index c574863..61d9be6 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -448,6 +448,31 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
 	}
 }
 
+static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
+					       unsigned int busno,
+					       unsigned int slot,
+					       unsigned int fn,
+					       int where)
+{
+	u16 offset;
+	u32 val;
+
+	/* EP device access */
+	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
+		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
+		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
+		(where & CFG_ADDR_REG_NUM_MASK) |
+		(1 & CFG_ADDR_CFG_TYPE_MASK);
+
+	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
+	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
+
+	if (iproc_pcie_reg_is_invalid(offset))
+		return NULL;
+
+	return (pcie->base + offset);
+}
+
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -459,7 +484,6 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
 {
 	unsigned slot = PCI_SLOT(devfn);
 	unsigned fn = PCI_FUNC(devfn);
-	u32 val;
 	u16 offset;
 
 	/* root complex access */
@@ -484,18 +508,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
 		if (slot > 0)
 			return NULL;
 
-	/* EP device access */
-	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
-		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
-		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
-		(where & CFG_ADDR_REG_NUM_MASK) |
-		(1 & CFG_ADDR_CFG_TYPE_MASK);
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
-	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
-	if (iproc_pcie_reg_is_invalid(offset))
-		return NULL;
-	else
-		return (pcie->base + offset);
+	return iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
 }
 
 static void __iomem *iproc_pcie_bus_map_cfg_bus(struct pci_bus *bus,
-- 
1.9.1

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Pawandeep]

PCIe spec r3.1, sec 2.3.2
If CRS software visibility is not enabled, the RC must reissue the
config request as a new request.

- If CRS software visibility is enabled,
- for a config read of Vendor ID, the RC must return 0x0001 data
- for all other config reads/writes, the RC must reissue the
  request

iproc PCIe Controller spec:
4.7.3.3. Retry Status On Configuration Cycle
Endpoints are allowed to generate retry status on configuration
cycles. In this case, the RC needs to re-issue the request. The IP
does not handle this because the number of configuration cycles needed
will probably be less than the total number of non-posted operations
needed.

When a retry status is received on the User RX interface for a
configuration request that was sent on the User TX interface,
it will be indicated with a completion with the CMPL_STATUS field set
to 2=CRS, and the user will have to find the address and data values
and send a new transaction on the User TX interface.
When the internal configuration space returns a retry status during a
configuration cycle (user_cscfg = 1) on the Command/Status interface,
the pcie_cscrs will assert with the pcie_csack signal to indicate the
CRS status.
When the CRS Software Visibility Enable register in the Root Control
register is enabled, the IP will return the data value to 0x0001 for
the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
the request for reads of offset 0 that return with CRS status.  This
is true for both the User RX Interface and for the Command/Status
interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
field of the completion on the User RX Interface will not be 2=CRS and
the pcie_cscrs signal will not assert on the Command/Status interface.

Per PCIe r3.1, sec 2.3.2, config requests that receive completions
with Configuration Request Retry Status (CRS) should be reissued by
the hardware except reads of the Vendor ID when CRS Software
Visibility is enabled.

This hardware never reissues configuration requests when it receives
CRS completions.
Note that, neither PCIe host bridge nor PCIe core re-issues the
request for any configuration offset.

For config reads, this hardware returns CFG_RETRY_STATUS data when
it receives a CRS completion for a config read, regardless of the
address of the read or the CRS Software Visibility Enable bit.

This patch implements iproc_pcie_config_read which gets called for
Stingray, if it receives a CRS completion, it retries reading it again.
In case of timeout, it returns 0xffffffff.
For other iproc based SOC, it falls back to PCI generic APIs.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 61d9be6..37f4adf 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -68,6 +68,9 @@
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
+#define CFG_RETRY_STATUS             0xffff0001
+#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
+
 /* derive the enum index of the outbound/inbound mapping registers */
 #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
 
@@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
 	return (pcie->base + offset);
 }
 
+static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
+{
+	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
+	unsigned int data;
+
+	/*
+	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
+	 * Visibility only affects config read of the Vendor ID.
+	 * For config write or any other config read the Root must
+	 * automatically re-issue configuration request again as a
+	 * new request.
+	 *
+	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
+	 * it receives a CRS completion for a config read, regardless of the
+	 * address of the read or the CRS Software Visibility Enable bit. As a
+	 * partial workaround for this, we retry in software any read that
+	 * returns CFG_RETRY_STATUS.
+	 */
+	data = readl(cfg_data_p);
+	while (data == CFG_RETRY_STATUS && timeout--) {
+		udelay(1);
+		data = readl(cfg_data_p);
+	}
+
+	if (data == CFG_RETRY_STATUS)
+		data = 0xffffffff;
+
+	return data;
+}
+
+static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	unsigned int slot = PCI_SLOT(devfn);
+	unsigned int fn = PCI_FUNC(devfn);
+	unsigned int busno = bus->number;
+	void __iomem *cfg_data_p;
+	unsigned int data;
+
+	/* root complex access. */
+	if (busno == 0)
+		return pci_generic_config_read32(bus, devfn, where, size, val);
+
+	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
+
+	if (!cfg_data_p)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	data = iproc_pcie_cfg_retry(cfg_data_p);
+
+	*val = data;
+	if (size <= 2)
+		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 *val)
 {
 	int ret;
+	struct iproc_pcie *pcie = iproc_data(bus);
 
 	iproc_pcie_apb_err_disable(bus, true);
+	if (pcie->type == IPROC_PCIE_PAXB_V2)
+		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
+	else
+		ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
-- 
1.9.1

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Pawandeep]

PCIe spec r3.1, sec 2.3.2
If CRS software visibility is not enabled, the RC must reissue the
config request as a new request.

- If CRS software visibility is enabled,
- for a config read of Vendor ID, the RC must return 0x0001 data
- for all other config reads/writes, the RC must reissue the
  request

iproc PCIe Controller spec:
4.7.3.3. Retry Status On Configuration Cycle
Endpoints are allowed to generate retry status on configuration
cycles. In this case, the RC needs to re-issue the request. The IP
does not handle this because the number of configuration cycles needed
will probably be less than the total number of non-posted operations
needed.

When a retry status is received on the User RX interface for a
configuration request that was sent on the User TX interface,
it will be indicated with a completion with the CMPL_STATUS field set
to 2=CRS, and the user will have to find the address and data values
and send a new transaction on the User TX interface.
When the internal configuration space returns a retry status during a
configuration cycle (user_cscfg = 1) on the Command/Status interface,
the pcie_cscrs will assert with the pcie_csack signal to indicate the
CRS status.
When the CRS Software Visibility Enable register in the Root Control
register is enabled, the IP will return the data value to 0x0001 for
the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
the request for reads of offset 0 that return with CRS status.  This
is true for both the User RX Interface and for the Command/Status
interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
field of the completion on the User RX Interface will not be 2=CRS and
the pcie_cscrs signal will not assert on the Command/Status interface.

Per PCIe r3.1, sec 2.3.2, config requests that receive completions
with Configuration Request Retry Status (CRS) should be reissued by
the hardware except reads of the Vendor ID when CRS Software
Visibility is enabled.

This hardware never reissues configuration requests when it receives
CRS completions.
Note that, neither PCIe host bridge nor PCIe core re-issues the
request for any configuration offset.

For config reads, this hardware returns CFG_RETRY_STATUS data when
it receives a CRS completion for a config read, regardless of the
address of the read or the CRS Software Visibility Enable bit.

This patch implements iproc_pcie_config_read which gets called for
Stingray, if it receives a CRS completion, it retries reading it again.
In case of timeout, it returns 0xffffffff.
For other iproc based SOC, it falls back to PCI generic APIs.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 61d9be6..37f4adf 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -68,6 +68,9 @@
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
+#define CFG_RETRY_STATUS             0xffff0001
+#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
+
 /* derive the enum index of the outbound/inbound mapping registers */
 #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
 
@@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
 	return (pcie->base + offset);
 }
 
+static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
+{
+	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
+	unsigned int data;
+
+	/*
+	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
+	 * Visibility only affects config read of the Vendor ID.
+	 * For config write or any other config read the Root must
+	 * automatically re-issue configuration request again as a
+	 * new request.
+	 *
+	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
+	 * it receives a CRS completion for a config read, regardless of the
+	 * address of the read or the CRS Software Visibility Enable bit. As a
+	 * partial workaround for this, we retry in software any read that
+	 * returns CFG_RETRY_STATUS.
+	 */
+	data = readl(cfg_data_p);
+	while (data == CFG_RETRY_STATUS && timeout--) {
+		udelay(1);
+		data = readl(cfg_data_p);
+	}
+
+	if (data == CFG_RETRY_STATUS)
+		data = 0xffffffff;
+
+	return data;
+}
+
+static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	unsigned int slot = PCI_SLOT(devfn);
+	unsigned int fn = PCI_FUNC(devfn);
+	unsigned int busno = bus->number;
+	void __iomem *cfg_data_p;
+	unsigned int data;
+
+	/* root complex access. */
+	if (busno == 0)
+		return pci_generic_config_read32(bus, devfn, where, size, val);
+
+	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
+
+	if (!cfg_data_p)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	data = iproc_pcie_cfg_retry(cfg_data_p);
+
+	*val = data;
+	if (size <= 2)
+		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 *val)
 {
 	int ret;
+	struct iproc_pcie *pcie = iproc_data(bus);
 
 	iproc_pcie_apb_err_disable(bus, true);
+	if (pcie->type == IPROC_PCIE_PAXB_V2)
+		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
+	else
+		ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
-- 
1.9.1

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Pawandeep]

PCIe spec r3.1, sec 2.3.2
If CRS software visibility is not enabled, the RC must reissue the
config request as a new request.

- If CRS software visibility is enabled,
- for a config read of Vendor ID, the RC must return 0x0001 data
- for all other config reads/writes, the RC must reissue the
  request

iproc PCIe Controller spec:
4.7.3.3. Retry Status On Configuration Cycle
Endpoints are allowed to generate retry status on configuration
cycles. In this case, the RC needs to re-issue the request. The IP
does not handle this because the number of configuration cycles needed
will probably be less than the total number of non-posted operations
needed.

When a retry status is received on the User RX interface for a
configuration request that was sent on the User TX interface,
it will be indicated with a completion with the CMPL_STATUS field set
to 2=CRS, and the user will have to find the address and data values
and send a new transaction on the User TX interface.
When the internal configuration space returns a retry status during a
configuration cycle (user_cscfg = 1) on the Command/Status interface,
the pcie_cscrs will assert with the pcie_csack signal to indicate the
CRS status.
When the CRS Software Visibility Enable register in the Root Control
register is enabled, the IP will return the data value to 0x0001 for
the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
the request for reads of offset 0 that return with CRS status.  This
is true for both the User RX Interface and for the Command/Status
interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
field of the completion on the User RX Interface will not be 2=CRS and
the pcie_cscrs signal will not assert on the Command/Status interface.

Per PCIe r3.1, sec 2.3.2, config requests that receive completions
with Configuration Request Retry Status (CRS) should be reissued by
the hardware except reads of the Vendor ID when CRS Software
Visibility is enabled.

This hardware never reissues configuration requests when it receives
CRS completions.
Note that, neither PCIe host bridge nor PCIe core re-issues the
request for any configuration offset.

For config reads, this hardware returns CFG_RETRY_STATUS data when
it receives a CRS completion for a config read, regardless of the
address of the read or the CRS Software Visibility Enable bit.

This patch implements iproc_pcie_config_read which gets called for
Stingray, if it receives a CRS completion, it retries reading it again.
In case of timeout, it returns 0xffffffff.
For other iproc based SOC, it falls back to PCI generic APIs.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 61d9be6..37f4adf 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -68,6 +68,9 @@
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
+#define CFG_RETRY_STATUS             0xffff0001
+#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
+
 /* derive the enum index of the outbound/inbound mapping registers */
 #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
 
@@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
 	return (pcie->base + offset);
 }
 
+static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
+{
+	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
+	unsigned int data;
+
+	/*
+	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
+	 * Visibility only affects config read of the Vendor ID.
+	 * For config write or any other config read the Root must
+	 * automatically re-issue configuration request again as a
+	 * new request.
+	 *
+	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
+	 * it receives a CRS completion for a config read, regardless of the
+	 * address of the read or the CRS Software Visibility Enable bit. As a
+	 * partial workaround for this, we retry in software any read that
+	 * returns CFG_RETRY_STATUS.
+	 */
+	data = readl(cfg_data_p);
+	while (data == CFG_RETRY_STATUS && timeout--) {
+		udelay(1);
+		data = readl(cfg_data_p);
+	}
+
+	if (data == CFG_RETRY_STATUS)
+		data = 0xffffffff;
+
+	return data;
+}
+
+static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	unsigned int slot = PCI_SLOT(devfn);
+	unsigned int fn = PCI_FUNC(devfn);
+	unsigned int busno = bus->number;
+	void __iomem *cfg_data_p;
+	unsigned int data;
+
+	/* root complex access. */
+	if (busno == 0)
+		return pci_generic_config_read32(bus, devfn, where, size, val);
+
+	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
+
+	if (!cfg_data_p)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	data = iproc_pcie_cfg_retry(cfg_data_p);
+
+	*val = data;
+	if (size <= 2)
+		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 /**
  * Note access to the configuration registers are protected@the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 *val)
 {
 	int ret;
+	struct iproc_pcie *pcie = iproc_data(bus);
 
 	iproc_pcie_apb_err_disable(bus, true);
+	if (pcie->type == IPROC_PCIE_PAXB_V2)
+		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
+	else
+		ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
-- 
1.9.1

[PATCH v8 3/3] PCI: iproc: add device shutdown for PCI RC

[Oza Pawandeep]

PERST must be asserted around ~500ms before the reboot is applied.

During soft reset (e.g., "reboot" from Linux) on some iproc based SOCs
LCPLL clock and PERST both goes off simultaneously.
This will cause certain Endpoints Intel NVMe not get detected, upon
next boot sequence.

This is specifically happening with Intel P3700 400GB series.
Endpoint is expecting the clock for some amount of time after PERST is
asserted, which is not happening in Stingray (iproc based SOC).
This causes NVMe to behave in undefined way.

On the contrary, Intel x86 boards will have ref clock running, so we
do not see this behavior there.

Besides, PCI spec does not stipulate about such timings.
In-fact it does not tell us, whether PCIe device should consider
refclk to be available or not to be.

It is completely up to vendor to design their EP the way they want
with respect to ref clock availability.

500ms is just based on the observation and taken as safe margin.
This patch adds platform shutdown where it should be
called in device_shutdown while reboot command is issued.
So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
followed by RC shutdown, which issues safe PERST assertion.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index 2253119..a5073a9 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -134,6 +134,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	return iproc_pcie_remove(pcie);
 }
 
+static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
+{
+	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
+
+	iproc_pcie_shutdown(pcie);
+}
+
 static struct platform_driver iproc_pcie_pltfm_driver = {
 	.driver = {
 		.name = "iproc-pcie",
@@ -141,6 +148,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	},
 	.probe = iproc_pcie_pltfm_probe,
 	.remove = iproc_pcie_pltfm_remove,
+	.shutdown = iproc_pcie_pltfm_shutdown,
 };
 module_platform_driver(iproc_pcie_pltfm_driver);
 
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 37f4adf..cbdabe8 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -659,7 +659,7 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
 	.write = iproc_pcie_config_write32,
 };
 
-static void iproc_pcie_reset(struct iproc_pcie *pcie)
+static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
 {
 	u32 val;
 
@@ -671,19 +671,26 @@ static void iproc_pcie_reset(struct iproc_pcie *pcie)
 	if (pcie->ep_is_internal)
 		return;
 
-	/*
-	 * Select perst_b signal as reset source. Put the device into reset,
-	 * and then bring it out of reset
-	 */
-	val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
-	val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
-		~RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	udelay(250);
-
-	val |= RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	msleep(100);
+	if (assert) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
+			~RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		udelay(250);
+	} else {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val |= RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		msleep(100);
+	}
+}
+
+int iproc_pcie_shutdown(struct iproc_pcie *pcie)
+{
+	iproc_pcie_perst_ctrl(pcie, true);
+	msleep(500);
+
+	return 0;
 }
 
 static int iproc_pcie_check_link(struct iproc_pcie *pcie)
@@ -1365,7 +1372,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
-	iproc_pcie_reset(pcie);
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
 
 	if (pcie->need_ob_cfg) {
 		ret = iproc_pcie_map_ranges(pcie, res);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 0bbe2ea..a6b55ce 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -110,6 +110,7 @@ struct iproc_pcie {
 
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
+int iproc_pcie_shutdown(struct iproc_pcie *pcie);
 
 #ifdef CONFIG_PCIE_IPROC_MSI
 int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
-- 
1.9.1

[PATCH v8 3/3] PCI: iproc: add device shutdown for PCI RC

[Oza Pawandeep]

PERST must be asserted around ~500ms before the reboot is applied.

During soft reset (e.g., "reboot" from Linux) on some iproc based SOCs
LCPLL clock and PERST both goes off simultaneously.
This will cause certain Endpoints Intel NVMe not get detected, upon
next boot sequence.

This is specifically happening with Intel P3700 400GB series.
Endpoint is expecting the clock for some amount of time after PERST is
asserted, which is not happening in Stingray (iproc based SOC).
This causes NVMe to behave in undefined way.

On the contrary, Intel x86 boards will have ref clock running, so we
do not see this behavior there.

Besides, PCI spec does not stipulate about such timings.
In-fact it does not tell us, whether PCIe device should consider
refclk to be available or not to be.

It is completely up to vendor to design their EP the way they want
with respect to ref clock availability.

500ms is just based on the observation and taken as safe margin.
This patch adds platform shutdown where it should be
called in device_shutdown while reboot command is issued.
So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
followed by RC shutdown, which issues safe PERST assertion.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index 2253119..a5073a9 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -134,6 +134,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	return iproc_pcie_remove(pcie);
 }
 
+static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
+{
+	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
+
+	iproc_pcie_shutdown(pcie);
+}
+
 static struct platform_driver iproc_pcie_pltfm_driver = {
 	.driver = {
 		.name = "iproc-pcie",
@@ -141,6 +148,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	},
 	.probe = iproc_pcie_pltfm_probe,
 	.remove = iproc_pcie_pltfm_remove,
+	.shutdown = iproc_pcie_pltfm_shutdown,
 };
 module_platform_driver(iproc_pcie_pltfm_driver);
 
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 37f4adf..cbdabe8 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -659,7 +659,7 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
 	.write = iproc_pcie_config_write32,
 };
 
-static void iproc_pcie_reset(struct iproc_pcie *pcie)
+static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
 {
 	u32 val;
 
@@ -671,19 +671,26 @@ static void iproc_pcie_reset(struct iproc_pcie *pcie)
 	if (pcie->ep_is_internal)
 		return;
 
-	/*
-	 * Select perst_b signal as reset source. Put the device into reset,
-	 * and then bring it out of reset
-	 */
-	val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
-	val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
-		~RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	udelay(250);
-
-	val |= RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	msleep(100);
+	if (assert) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
+			~RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		udelay(250);
+	} else {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val |= RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		msleep(100);
+	}
+}
+
+int iproc_pcie_shutdown(struct iproc_pcie *pcie)
+{
+	iproc_pcie_perst_ctrl(pcie, true);
+	msleep(500);
+
+	return 0;
 }
 
 static int iproc_pcie_check_link(struct iproc_pcie *pcie)
@@ -1365,7 +1372,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
-	iproc_pcie_reset(pcie);
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
 
 	if (pcie->need_ob_cfg) {
 		ret = iproc_pcie_map_ranges(pcie, res);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 0bbe2ea..a6b55ce 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -110,6 +110,7 @@ struct iproc_pcie {
 
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
+int iproc_pcie_shutdown(struct iproc_pcie *pcie);
 
 #ifdef CONFIG_PCIE_IPROC_MSI
 int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
-- 
1.9.1

[PATCH v8 3/3] PCI: iproc: add device shutdown for PCI RC

[Oza Pawandeep]

PERST must be asserted around ~500ms before the reboot is applied.

During soft reset (e.g., "reboot" from Linux) on some iproc based SOCs
LCPLL clock and PERST both goes off simultaneously.
This will cause certain Endpoints Intel NVMe not get detected, upon
next boot sequence.

This is specifically happening with Intel P3700 400GB series.
Endpoint is expecting the clock for some amount of time after PERST is
asserted, which is not happening in Stingray (iproc based SOC).
This causes NVMe to behave in undefined way.

On the contrary, Intel x86 boards will have ref clock running, so we
do not see this behavior there.

Besides, PCI spec does not stipulate about such timings.
In-fact it does not tell us, whether PCIe device should consider
refclk to be available or not to be.

It is completely up to vendor to design their EP the way they want
with respect to ref clock availability.

500ms is just based on the observation and taken as safe margin.
This patch adds platform shutdown where it should be
called in device_shutdown while reboot command is issued.
So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
followed by RC shutdown, which issues safe PERST assertion.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index 2253119..a5073a9 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -134,6 +134,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	return iproc_pcie_remove(pcie);
 }
 
+static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
+{
+	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
+
+	iproc_pcie_shutdown(pcie);
+}
+
 static struct platform_driver iproc_pcie_pltfm_driver = {
 	.driver = {
 		.name = "iproc-pcie",
@@ -141,6 +148,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	},
 	.probe = iproc_pcie_pltfm_probe,
 	.remove = iproc_pcie_pltfm_remove,
+	.shutdown = iproc_pcie_pltfm_shutdown,
 };
 module_platform_driver(iproc_pcie_pltfm_driver);
 
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 37f4adf..cbdabe8 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -659,7 +659,7 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
 	.write = iproc_pcie_config_write32,
 };
 
-static void iproc_pcie_reset(struct iproc_pcie *pcie)
+static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
 {
 	u32 val;
 
@@ -671,19 +671,26 @@ static void iproc_pcie_reset(struct iproc_pcie *pcie)
 	if (pcie->ep_is_internal)
 		return;
 
-	/*
-	 * Select perst_b signal as reset source. Put the device into reset,
-	 * and then bring it out of reset
-	 */
-	val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
-	val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
-		~RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	udelay(250);
-
-	val |= RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	msleep(100);
+	if (assert) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
+			~RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		udelay(250);
+	} else {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val |= RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		msleep(100);
+	}
+}
+
+int iproc_pcie_shutdown(struct iproc_pcie *pcie)
+{
+	iproc_pcie_perst_ctrl(pcie, true);
+	msleep(500);
+
+	return 0;
 }
 
 static int iproc_pcie_check_link(struct iproc_pcie *pcie)
@@ -1365,7 +1372,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
-	iproc_pcie_reset(pcie);
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
 
 	if (pcie->need_ob_cfg) {
 		ret = iproc_pcie_map_ranges(pcie, res);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 0bbe2ea..a6b55ce 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -110,6 +110,7 @@ struct iproc_pcie {
 
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
+int iproc_pcie_shutdown(struct iproc_pcie *pcie);
 
 #ifdef CONFIG_PCIE_IPROC_MSI
 int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
-- 
1.9.1

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> PCIe spec r3.1, sec 2.3.2
> If CRS software visibility is not enabled, the RC must reissue the
> config request as a new request.
> 
> - If CRS software visibility is enabled,
> - for a config read of Vendor ID, the RC must return 0x0001 data
> - for all other config reads/writes, the RC must reissue the
>   request
> 
> iproc PCIe Controller spec:
> 4.7.3.3. Retry Status On Configuration Cycle
> Endpoints are allowed to generate retry status on configuration
> cycles. In this case, the RC needs to re-issue the request. The IP
> does not handle this because the number of configuration cycles needed
> will probably be less than the total number of non-posted operations
> needed.
> 
> When a retry status is received on the User RX interface for a
> configuration request that was sent on the User TX interface,
> it will be indicated with a completion with the CMPL_STATUS field set
> to 2=CRS, and the user will have to find the address and data values
> and send a new transaction on the User TX interface.
> When the internal configuration space returns a retry status during a
> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> CRS status.
> When the CRS Software Visibility Enable register in the Root Control
> register is enabled, the IP will return the data value to 0x0001 for
> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
> the request for reads of offset 0 that return with CRS status.  This
> is true for both the User RX Interface and for the Command/Status
> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> field of the completion on the User RX Interface will not be 2=CRS and
> the pcie_cscrs signal will not assert on the Command/Status interface.
> 
> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> with Configuration Request Retry Status (CRS) should be reissued by
> the hardware except reads of the Vendor ID when CRS Software
> Visibility is enabled.
> 
> This hardware never reissues configuration requests when it receives
> CRS completions.
> Note that, neither PCIe host bridge nor PCIe core re-issues the
> request for any configuration offset.
> 
> For config reads, this hardware returns CFG_RETRY_STATUS data when
> it receives a CRS completion for a config read, regardless of the
> address of the read or the CRS Software Visibility Enable bit.

I can't remember how Stingray handles the CRS Software Visibility Enable
bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
it all (I think not)?

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> PCIe spec r3.1, sec 2.3.2
> If CRS software visibility is not enabled, the RC must reissue the
> config request as a new request.
> 
> - If CRS software visibility is enabled,
> - for a config read of Vendor ID, the RC must return 0x0001 data
> - for all other config reads/writes, the RC must reissue the
>   request
> 
> iproc PCIe Controller spec:
> 4.7.3.3. Retry Status On Configuration Cycle
> Endpoints are allowed to generate retry status on configuration
> cycles. In this case, the RC needs to re-issue the request. The IP
> does not handle this because the number of configuration cycles needed
> will probably be less than the total number of non-posted operations
> needed.
> 
> When a retry status is received on the User RX interface for a
> configuration request that was sent on the User TX interface,
> it will be indicated with a completion with the CMPL_STATUS field set
> to 2=CRS, and the user will have to find the address and data values
> and send a new transaction on the User TX interface.
> When the internal configuration space returns a retry status during a
> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> CRS status.
> When the CRS Software Visibility Enable register in the Root Control
> register is enabled, the IP will return the data value to 0x0001 for
> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
> the request for reads of offset 0 that return with CRS status.  This
> is true for both the User RX Interface and for the Command/Status
> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> field of the completion on the User RX Interface will not be 2=CRS and
> the pcie_cscrs signal will not assert on the Command/Status interface.
> 
> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> with Configuration Request Retry Status (CRS) should be reissued by
> the hardware except reads of the Vendor ID when CRS Software
> Visibility is enabled.
> 
> This hardware never reissues configuration requests when it receives
> CRS completions.
> Note that, neither PCIe host bridge nor PCIe core re-issues the
> request for any configuration offset.
> 
> For config reads, this hardware returns CFG_RETRY_STATUS data when
> it receives a CRS completion for a config read, regardless of the
> address of the read or the CRS Software Visibility Enable bit.

I can't remember how Stingray handles the CRS Software Visibility Enable
bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
it all (I think not)?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

T24gVGh1LCBBdWcgMjQsIDIwMTcgYXQgMTA6MzQ6MjVBTSArMDUzMCwgT3phIFBhd2FuZGVlcCB3
cm90ZToKPiBQQ0llIHNwZWMgcjMuMSwgc2VjIDIuMy4yCj4gSWYgQ1JTIHNvZnR3YXJlIHZpc2li
aWxpdHkgaXMgbm90IGVuYWJsZWQsIHRoZSBSQyBtdXN0IHJlaXNzdWUgdGhlCj4gY29uZmlnIHJl
cXVlc3QgYXMgYSBuZXcgcmVxdWVzdC4KPiAKPiAtIElmIENSUyBzb2Z0d2FyZSB2aXNpYmlsaXR5
IGlzIGVuYWJsZWQsCj4gLSBmb3IgYSBjb25maWcgcmVhZCBvZiBWZW5kb3IgSUQsIHRoZSBSQyBt
dXN0IHJldHVybiAweDAwMDEgZGF0YQo+IC0gZm9yIGFsbCBvdGhlciBjb25maWcgcmVhZHMvd3Jp
dGVzLCB0aGUgUkMgbXVzdCByZWlzc3VlIHRoZQo+ICAgcmVxdWVzdAo+IAo+IGlwcm9jIFBDSWUg
Q29udHJvbGxlciBzcGVjOgo+IDQuNy4zLjMuIFJldHJ5IFN0YXR1cyBPbiBDb25maWd1cmF0aW9u
IEN5Y2xlCj4gRW5kcG9pbnRzIGFyZSBhbGxvd2VkIHRvIGdlbmVyYXRlIHJldHJ5IHN0YXR1cyBv
biBjb25maWd1cmF0aW9uCj4gY3ljbGVzLiBJbiB0aGlzIGNhc2UsIHRoZSBSQyBuZWVkcyB0byBy
ZS1pc3N1ZSB0aGUgcmVxdWVzdC4gVGhlIElQCj4gZG9lcyBub3QgaGFuZGxlIHRoaXMgYmVjYXVz
ZSB0aGUgbnVtYmVyIG9mIGNvbmZpZ3VyYXRpb24gY3ljbGVzIG5lZWRlZAo+IHdpbGwgcHJvYmFi
bHkgYmUgbGVzcyB0aGFuIHRoZSB0b3RhbCBudW1iZXIgb2Ygbm9uLXBvc3RlZCBvcGVyYXRpb25z
Cj4gbmVlZGVkLgo+IAo+IFdoZW4gYSByZXRyeSBzdGF0dXMgaXMgcmVjZWl2ZWQgb24gdGhlIFVz
ZXIgUlggaW50ZXJmYWNlIGZvciBhCj4gY29uZmlndXJhdGlvbiByZXF1ZXN0IHRoYXQgd2FzIHNl
bnQgb24gdGhlIFVzZXIgVFggaW50ZXJmYWNlLAo+IGl0IHdpbGwgYmUgaW5kaWNhdGVkIHdpdGgg
YSBjb21wbGV0aW9uIHdpdGggdGhlIENNUExfU1RBVFVTIGZpZWxkIHNldAo+IHRvIDI9Q1JTLCBh
bmQgdGhlIHVzZXIgd2lsbCBoYXZlIHRvIGZpbmQgdGhlIGFkZHJlc3MgYW5kIGRhdGEgdmFsdWVz
Cj4gYW5kIHNlbmQgYSBuZXcgdHJhbnNhY3Rpb24gb24gdGhlIFVzZXIgVFggaW50ZXJmYWNlLgo+
IFdoZW4gdGhlIGludGVybmFsIGNvbmZpZ3VyYXRpb24gc3BhY2UgcmV0dXJucyBhIHJldHJ5IHN0
YXR1cyBkdXJpbmcgYQo+IGNvbmZpZ3VyYXRpb24gY3ljbGUgKHVzZXJfY3NjZmcgPSAxKSBvbiB0
aGUgQ29tbWFuZC9TdGF0dXMgaW50ZXJmYWNlLAo+IHRoZSBwY2llX2NzY3JzIHdpbGwgYXNzZXJ0
IHdpdGggdGhlIHBjaWVfY3NhY2sgc2lnbmFsIHRvIGluZGljYXRlIHRoZQo+IENSUyBzdGF0dXMu
Cj4gV2hlbiB0aGUgQ1JTIFNvZnR3YXJlIFZpc2liaWxpdHkgRW5hYmxlIHJlZ2lzdGVyIGluIHRo
ZSBSb290IENvbnRyb2wKPiByZWdpc3RlciBpcyBlbmFibGVkLCB0aGUgSVAgd2lsbCByZXR1cm4g
dGhlIGRhdGEgdmFsdWUgdG8gMHgwMDAxIGZvcgo+IHRoZSBWZW5kb3IgSUQgdmFsdWUgYW5kIDB4
ZmZmZiAgKGFsbCAx4oCZcykgZm9yIHRoZSByZXN0IG9mIHRoZSBkYXRhIGluCj4gdGhlIHJlcXVl
c3QgZm9yIHJlYWRzIG9mIG9mZnNldCAwIHRoYXQgcmV0dXJuIHdpdGggQ1JTIHN0YXR1cy4gIFRo
aXMKPiBpcyB0cnVlIGZvciBib3RoIHRoZSBVc2VyIFJYIEludGVyZmFjZSBhbmQgZm9yIHRoZSBD
b21tYW5kL1N0YXR1cwo+IGludGVyZmFjZS4gIFdoZW4gQ1JTIFNvZnR3YXJlIFZpc2liaWxpdHkg
aXMgZW5hYmxlZCwgdGhlIENNUExfU1RBVFVTCj4gZmllbGQgb2YgdGhlIGNvbXBsZXRpb24gb24g
dGhlIFVzZXIgUlggSW50ZXJmYWNlIHdpbGwgbm90IGJlIDI9Q1JTIGFuZAo+IHRoZSBwY2llX2Nz
Y3JzIHNpZ25hbCB3aWxsIG5vdCBhc3NlcnQgb24gdGhlIENvbW1hbmQvU3RhdHVzIGludGVyZmFj
ZS4KPiAKPiBQZXIgUENJZSByMy4xLCBzZWMgMi4zLjIsIGNvbmZpZyByZXF1ZXN0cyB0aGF0IHJl
Y2VpdmUgY29tcGxldGlvbnMKPiB3aXRoIENvbmZpZ3VyYXRpb24gUmVxdWVzdCBSZXRyeSBTdGF0
dXMgKENSUykgc2hvdWxkIGJlIHJlaXNzdWVkIGJ5Cj4gdGhlIGhhcmR3YXJlIGV4Y2VwdCByZWFk
cyBvZiB0aGUgVmVuZG9yIElEIHdoZW4gQ1JTIFNvZnR3YXJlCj4gVmlzaWJpbGl0eSBpcyBlbmFi
bGVkLgo+IAo+IFRoaXMgaGFyZHdhcmUgbmV2ZXIgcmVpc3N1ZXMgY29uZmlndXJhdGlvbiByZXF1
ZXN0cyB3aGVuIGl0IHJlY2VpdmVzCj4gQ1JTIGNvbXBsZXRpb25zLgo+IE5vdGUgdGhhdCwgbmVp
dGhlciBQQ0llIGhvc3QgYnJpZGdlIG5vciBQQ0llIGNvcmUgcmUtaXNzdWVzIHRoZQo+IHJlcXVl
c3QgZm9yIGFueSBjb25maWd1cmF0aW9uIG9mZnNldC4KPiAKPiBGb3IgY29uZmlnIHJlYWRzLCB0
aGlzIGhhcmR3YXJlIHJldHVybnMgQ0ZHX1JFVFJZX1NUQVRVUyBkYXRhIHdoZW4KPiBpdCByZWNl
aXZlcyBhIENSUyBjb21wbGV0aW9uIGZvciBhIGNvbmZpZyByZWFkLCByZWdhcmRsZXNzIG9mIHRo
ZQo+IGFkZHJlc3Mgb2YgdGhlIHJlYWQgb3IgdGhlIENSUyBTb2Z0d2FyZSBWaXNpYmlsaXR5IEVu
YWJsZSBiaXQuCgpJIGNhbid0IHJlbWVtYmVyIGhvdyBTdGluZ3JheSBoYW5kbGVzIHRoZSBDUlMg
U29mdHdhcmUgVmlzaWJpbGl0eSBFbmFibGUKYml0LiAgSXMgaXQgYSByZWFkLW9ubHkgemVybz8g
IElzIGl0IHdyaXRhYmxlPyAgRG9lcyB0aGUgaGFyZHdhcmUgbG9vayBhdAppdCBhbGwgKEkgdGhp
bmsgbm90KT8KCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
CmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5m
cmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xp
bnV4LWFybS1rZXJuZWwK

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> PCIe spec r3.1, sec 2.3.2
> If CRS software visibility is not enabled, the RC must reissue the
> config request as a new request.
> 
> - If CRS software visibility is enabled,
> - for a config read of Vendor ID, the RC must return 0x0001 data
> - for all other config reads/writes, the RC must reissue the
>   request
> 
> iproc PCIe Controller spec:
> 4.7.3.3. Retry Status On Configuration Cycle
> Endpoints are allowed to generate retry status on configuration
> cycles. In this case, the RC needs to re-issue the request. The IP
> does not handle this because the number of configuration cycles needed
> will probably be less than the total number of non-posted operations
> needed.
> 
> When a retry status is received on the User RX interface for a
> configuration request that was sent on the User TX interface,
> it will be indicated with a completion with the CMPL_STATUS field set
> to 2=CRS, and the user will have to find the address and data values
> and send a new transaction on the User TX interface.
> When the internal configuration space returns a retry status during a
> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> CRS status.
> When the CRS Software Visibility Enable register in the Root Control
> register is enabled, the IP will return the data value to 0x0001 for
> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
> the request for reads of offset 0 that return with CRS status.  This
> is true for both the User RX Interface and for the Command/Status
> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> field of the completion on the User RX Interface will not be 2=CRS and
> the pcie_cscrs signal will not assert on the Command/Status interface.
> 
> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> with Configuration Request Retry Status (CRS) should be reissued by
> the hardware except reads of the Vendor ID when CRS Software
> Visibility is enabled.
> 
> This hardware never reissues configuration requests when it receives
> CRS completions.
> Note that, neither PCIe host bridge nor PCIe core re-issues the
> request for any configuration offset.
> 
> For config reads, this hardware returns CFG_RETRY_STATUS data when
> it receives a CRS completion for a config read, regardless of the
> address of the read or the CRS Software Visibility Enable bit.

I can't remember how Stingray handles the CRS Software Visibility Enable
bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
it all (I think not)?

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> PCIe spec r3.1, sec 2.3.2
>> If CRS software visibility is not enabled, the RC must reissue the
>> config request as a new request.
>>
>> - If CRS software visibility is enabled,
>> - for a config read of Vendor ID, the RC must return 0x0001 data
>> - for all other config reads/writes, the RC must reissue the
>>   request
>>
>> iproc PCIe Controller spec:
>> 4.7.3.3. Retry Status On Configuration Cycle
>> Endpoints are allowed to generate retry status on configuration
>> cycles. In this case, the RC needs to re-issue the request. The IP
>> does not handle this because the number of configuration cycles needed
>> will probably be less than the total number of non-posted operations
>> needed.
>>
>> When a retry status is received on the User RX interface for a
>> configuration request that was sent on the User TX interface,
>> it will be indicated with a completion with the CMPL_STATUS field set
>> to 2=CRS, and the user will have to find the address and data values
>> and send a new transaction on the User TX interface.
>> When the internal configuration space returns a retry status during a
>> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> CRS status.
>> When the CRS Software Visibility Enable register in the Root Control
>> register is enabled, the IP will return the data value to 0x0001 for
>> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
>> the request for reads of offset 0 that return with CRS status.  This
>> is true for both the User RX Interface and for the Command/Status
>> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> field of the completion on the User RX Interface will not be 2=CRS and
>> the pcie_cscrs signal will not assert on the Command/Status interface.
>>
>> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> with Configuration Request Retry Status (CRS) should be reissued by
>> the hardware except reads of the Vendor ID when CRS Software
>> Visibility is enabled.
>>
>> This hardware never reissues configuration requests when it receives
>> CRS completions.
>> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> request for any configuration offset.
>>
>> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> it receives a CRS completion for a config read, regardless of the
>> address of the read or the CRS Software Visibility Enable bit.
>
> I can't remember how Stingray handles the CRS Software Visibility Enable
> bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
> it all (I think not)?

HW doesnt look it at all, it is "dont care" bit.

Regards,
Oza.

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> PCIe spec r3.1, sec 2.3.2
>> If CRS software visibility is not enabled, the RC must reissue the
>> config request as a new request.
>>
>> - If CRS software visibility is enabled,
>> - for a config read of Vendor ID, the RC must return 0x0001 data
>> - for all other config reads/writes, the RC must reissue the
>>   request
>>
>> iproc PCIe Controller spec:
>> 4.7.3.3. Retry Status On Configuration Cycle
>> Endpoints are allowed to generate retry status on configuration
>> cycles. In this case, the RC needs to re-issue the request. The IP
>> does not handle this because the number of configuration cycles needed
>> will probably be less than the total number of non-posted operations
>> needed.
>>
>> When a retry status is received on the User RX interface for a
>> configuration request that was sent on the User TX interface,
>> it will be indicated with a completion with the CMPL_STATUS field set
>> to 2=3DCRS, and the user will have to find the address and data values
>> and send a new transaction on the User TX interface.
>> When the internal configuration space returns a retry status during a
>> configuration cycle (user_cscfg =3D 1) on the Command/Status interface,
>> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> CRS status.
>> When the CRS Software Visibility Enable register in the Root Control
>> register is enabled, the IP will return the data value to 0x0001 for
>> the Vendor ID value and 0xffff  (all 1=E2=80=99s) for the rest of the da=
ta in
>> the request for reads of offset 0 that return with CRS status.  This
>> is true for both the User RX Interface and for the Command/Status
>> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> field of the completion on the User RX Interface will not be 2=3DCRS and
>> the pcie_cscrs signal will not assert on the Command/Status interface.
>>
>> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> with Configuration Request Retry Status (CRS) should be reissued by
>> the hardware except reads of the Vendor ID when CRS Software
>> Visibility is enabled.
>>
>> This hardware never reissues configuration requests when it receives
>> CRS completions.
>> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> request for any configuration offset.
>>
>> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> it receives a CRS completion for a config read, regardless of the
>> address of the read or the CRS Software Visibility Enable bit.
>
> I can't remember how Stingray handles the CRS Software Visibility Enable
> bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
> it all (I think not)?

HW doesnt look it at all, it is "dont care" bit.

Regards,
Oza.

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> PCIe spec r3.1, sec 2.3.2
>> If CRS software visibility is not enabled, the RC must reissue the
>> config request as a new request.
>>
>> - If CRS software visibility is enabled,
>> - for a config read of Vendor ID, the RC must return 0x0001 data
>> - for all other config reads/writes, the RC must reissue the
>>   request
>>
>> iproc PCIe Controller spec:
>> 4.7.3.3. Retry Status On Configuration Cycle
>> Endpoints are allowed to generate retry status on configuration
>> cycles. In this case, the RC needs to re-issue the request. The IP
>> does not handle this because the number of configuration cycles needed
>> will probably be less than the total number of non-posted operations
>> needed.
>>
>> When a retry status is received on the User RX interface for a
>> configuration request that was sent on the User TX interface,
>> it will be indicated with a completion with the CMPL_STATUS field set
>> to 2=CRS, and the user will have to find the address and data values
>> and send a new transaction on the User TX interface.
>> When the internal configuration space returns a retry status during a
>> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> CRS status.
>> When the CRS Software Visibility Enable register in the Root Control
>> register is enabled, the IP will return the data value to 0x0001 for
>> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
>> the request for reads of offset 0 that return with CRS status.  This
>> is true for both the User RX Interface and for the Command/Status
>> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> field of the completion on the User RX Interface will not be 2=CRS and
>> the pcie_cscrs signal will not assert on the Command/Status interface.
>>
>> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> with Configuration Request Retry Status (CRS) should be reissued by
>> the hardware except reads of the Vendor ID when CRS Software
>> Visibility is enabled.
>>
>> This hardware never reissues configuration requests when it receives
>> CRS completions.
>> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> request for any configuration offset.
>>
>> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> it receives a CRS completion for a config read, regardless of the
>> address of the read or the CRS Software Visibility Enable bit.
>
> I can't remember how Stingray handles the CRS Software Visibility Enable
> bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
> it all (I think not)?

HW doesnt look it at all, it is "dont care" bit.

Regards,
Oza.

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Tue, Aug 29, 2017 at 01:09:53AM +0530, Oza Oza wrote:
> On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> >> PCIe spec r3.1, sec 2.3.2
> >> If CRS software visibility is not enabled, the RC must reissue the
> >> config request as a new request.
> >>
> >> - If CRS software visibility is enabled,
> >> - for a config read of Vendor ID, the RC must return 0x0001 data
> >> - for all other config reads/writes, the RC must reissue the
> >>   request
> >>
> >> iproc PCIe Controller spec:
> >> 4.7.3.3. Retry Status On Configuration Cycle
> >> Endpoints are allowed to generate retry status on configuration
> >> cycles. In this case, the RC needs to re-issue the request. The IP
> >> does not handle this because the number of configuration cycles needed
> >> will probably be less than the total number of non-posted operations
> >> needed.
> >>
> >> When a retry status is received on the User RX interface for a
> >> configuration request that was sent on the User TX interface,
> >> it will be indicated with a completion with the CMPL_STATUS field set
> >> to 2=CRS, and the user will have to find the address and data values
> >> and send a new transaction on the User TX interface.
> >> When the internal configuration space returns a retry status during a
> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> >> CRS status.
> >> When the CRS Software Visibility Enable register in the Root Control
> >> register is enabled, the IP will return the data value to 0x0001 for
> >> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
> >> the request for reads of offset 0 that return with CRS status.  This
> >> is true for both the User RX Interface and for the Command/Status
> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> >> field of the completion on the User RX Interface will not be 2=CRS and
> >> the pcie_cscrs signal will not assert on the Command/Status interface.
> >>
> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> >> with Configuration Request Retry Status (CRS) should be reissued by
> >> the hardware except reads of the Vendor ID when CRS Software
> >> Visibility is enabled.
> >>
> >> This hardware never reissues configuration requests when it receives
> >> CRS completions.
> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
> >> request for any configuration offset.
> >>
> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
> >> it receives a CRS completion for a config read, regardless of the
> >> address of the read or the CRS Software Visibility Enable bit.
> >
> > I can't remember how Stingray handles the CRS Software Visibility Enable
> > bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
> > it all (I think not)?
> 
> HW doesnt look it at all, it is "dont care" bit.

Sigh, I made the classic mistake of asking more than one question, and
I guess I'm about to do it again :)  It'll save time if you can answer
all the questions at once.

Linux enables PCI_EXP_RTCTL_CRSSVE if PCI_EXP_RTCAP says it is
supported (see pci_enable_crs()).

  - What does PCI_EXP_RTCAP say?

  - Is PCI_EXP_RTCTL_CRSSVE writable?

With all the CRS-related work going on, I suspect we may someday want to
read PCI_EXP_RTCTL_CRSSVE to see if CRS SV is enabled.

Bjorn

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

T24gVHVlLCBBdWcgMjksIDIwMTcgYXQgMDE6MDk6NTNBTSArMDUzMCwgT3phIE96YSB3cm90ZToK
PiBPbiBUdWUsIEF1ZyAyOSwgMjAxNyBhdCAxMjo0NyBBTSwgQmpvcm4gSGVsZ2FhcyA8aGVsZ2Fh
c0BrZXJuZWwub3JnPiB3cm90ZToKPiA+IE9uIFRodSwgQXVnIDI0LCAyMDE3IGF0IDEwOjM0OjI1
QU0gKzA1MzAsIE96YSBQYXdhbmRlZXAgd3JvdGU6Cj4gPj4gUENJZSBzcGVjIHIzLjEsIHNlYyAy
LjMuMgo+ID4+IElmIENSUyBzb2Z0d2FyZSB2aXNpYmlsaXR5IGlzIG5vdCBlbmFibGVkLCB0aGUg
UkMgbXVzdCByZWlzc3VlIHRoZQo+ID4+IGNvbmZpZyByZXF1ZXN0IGFzIGEgbmV3IHJlcXVlc3Qu
Cj4gPj4KPiA+PiAtIElmIENSUyBzb2Z0d2FyZSB2aXNpYmlsaXR5IGlzIGVuYWJsZWQsCj4gPj4g
LSBmb3IgYSBjb25maWcgcmVhZCBvZiBWZW5kb3IgSUQsIHRoZSBSQyBtdXN0IHJldHVybiAweDAw
MDEgZGF0YQo+ID4+IC0gZm9yIGFsbCBvdGhlciBjb25maWcgcmVhZHMvd3JpdGVzLCB0aGUgUkMg
bXVzdCByZWlzc3VlIHRoZQo+ID4+ICAgcmVxdWVzdAo+ID4+Cj4gPj4gaXByb2MgUENJZSBDb250
cm9sbGVyIHNwZWM6Cj4gPj4gNC43LjMuMy4gUmV0cnkgU3RhdHVzIE9uIENvbmZpZ3VyYXRpb24g
Q3ljbGUKPiA+PiBFbmRwb2ludHMgYXJlIGFsbG93ZWQgdG8gZ2VuZXJhdGUgcmV0cnkgc3RhdHVz
IG9uIGNvbmZpZ3VyYXRpb24KPiA+PiBjeWNsZXMuIEluIHRoaXMgY2FzZSwgdGhlIFJDIG5lZWRz
IHRvIHJlLWlzc3VlIHRoZSByZXF1ZXN0LiBUaGUgSVAKPiA+PiBkb2VzIG5vdCBoYW5kbGUgdGhp
cyBiZWNhdXNlIHRoZSBudW1iZXIgb2YgY29uZmlndXJhdGlvbiBjeWNsZXMgbmVlZGVkCj4gPj4g
d2lsbCBwcm9iYWJseSBiZSBsZXNzIHRoYW4gdGhlIHRvdGFsIG51bWJlciBvZiBub24tcG9zdGVk
IG9wZXJhdGlvbnMKPiA+PiBuZWVkZWQuCj4gPj4KPiA+PiBXaGVuIGEgcmV0cnkgc3RhdHVzIGlz
IHJlY2VpdmVkIG9uIHRoZSBVc2VyIFJYIGludGVyZmFjZSBmb3IgYQo+ID4+IGNvbmZpZ3VyYXRp
b24gcmVxdWVzdCB0aGF0IHdhcyBzZW50IG9uIHRoZSBVc2VyIFRYIGludGVyZmFjZSwKPiA+PiBp
dCB3aWxsIGJlIGluZGljYXRlZCB3aXRoIGEgY29tcGxldGlvbiB3aXRoIHRoZSBDTVBMX1NUQVRV
UyBmaWVsZCBzZXQKPiA+PiB0byAyPUNSUywgYW5kIHRoZSB1c2VyIHdpbGwgaGF2ZSB0byBmaW5k
IHRoZSBhZGRyZXNzIGFuZCBkYXRhIHZhbHVlcwo+ID4+IGFuZCBzZW5kIGEgbmV3IHRyYW5zYWN0
aW9uIG9uIHRoZSBVc2VyIFRYIGludGVyZmFjZS4KPiA+PiBXaGVuIHRoZSBpbnRlcm5hbCBjb25m
aWd1cmF0aW9uIHNwYWNlIHJldHVybnMgYSByZXRyeSBzdGF0dXMgZHVyaW5nIGEKPiA+PiBjb25m
aWd1cmF0aW9uIGN5Y2xlICh1c2VyX2NzY2ZnID0gMSkgb24gdGhlIENvbW1hbmQvU3RhdHVzIGlu
dGVyZmFjZSwKPiA+PiB0aGUgcGNpZV9jc2NycyB3aWxsIGFzc2VydCB3aXRoIHRoZSBwY2llX2Nz
YWNrIHNpZ25hbCB0byBpbmRpY2F0ZSB0aGUKPiA+PiBDUlMgc3RhdHVzLgo+ID4+IFdoZW4gdGhl
IENSUyBTb2Z0d2FyZSBWaXNpYmlsaXR5IEVuYWJsZSByZWdpc3RlciBpbiB0aGUgUm9vdCBDb250
cm9sCj4gPj4gcmVnaXN0ZXIgaXMgZW5hYmxlZCwgdGhlIElQIHdpbGwgcmV0dXJuIHRoZSBkYXRh
IHZhbHVlIHRvIDB4MDAwMSBmb3IKPiA+PiB0aGUgVmVuZG9yIElEIHZhbHVlIGFuZCAweGZmZmYg
IChhbGwgMeKAmXMpIGZvciB0aGUgcmVzdCBvZiB0aGUgZGF0YSBpbgo+ID4+IHRoZSByZXF1ZXN0
IGZvciByZWFkcyBvZiBvZmZzZXQgMCB0aGF0IHJldHVybiB3aXRoIENSUyBzdGF0dXMuICBUaGlz
Cj4gPj4gaXMgdHJ1ZSBmb3IgYm90aCB0aGUgVXNlciBSWCBJbnRlcmZhY2UgYW5kIGZvciB0aGUg
Q29tbWFuZC9TdGF0dXMKPiA+PiBpbnRlcmZhY2UuICBXaGVuIENSUyBTb2Z0d2FyZSBWaXNpYmls
aXR5IGlzIGVuYWJsZWQsIHRoZSBDTVBMX1NUQVRVUwo+ID4+IGZpZWxkIG9mIHRoZSBjb21wbGV0
aW9uIG9uIHRoZSBVc2VyIFJYIEludGVyZmFjZSB3aWxsIG5vdCBiZSAyPUNSUyBhbmQKPiA+PiB0
aGUgcGNpZV9jc2NycyBzaWduYWwgd2lsbCBub3QgYXNzZXJ0IG9uIHRoZSBDb21tYW5kL1N0YXR1
cyBpbnRlcmZhY2UuCj4gPj4KPiA+PiBQZXIgUENJZSByMy4xLCBzZWMgMi4zLjIsIGNvbmZpZyBy
ZXF1ZXN0cyB0aGF0IHJlY2VpdmUgY29tcGxldGlvbnMKPiA+PiB3aXRoIENvbmZpZ3VyYXRpb24g
UmVxdWVzdCBSZXRyeSBTdGF0dXMgKENSUykgc2hvdWxkIGJlIHJlaXNzdWVkIGJ5Cj4gPj4gdGhl
IGhhcmR3YXJlIGV4Y2VwdCByZWFkcyBvZiB0aGUgVmVuZG9yIElEIHdoZW4gQ1JTIFNvZnR3YXJl
Cj4gPj4gVmlzaWJpbGl0eSBpcyBlbmFibGVkLgo+ID4+Cj4gPj4gVGhpcyBoYXJkd2FyZSBuZXZl
ciByZWlzc3VlcyBjb25maWd1cmF0aW9uIHJlcXVlc3RzIHdoZW4gaXQgcmVjZWl2ZXMKPiA+PiBD
UlMgY29tcGxldGlvbnMuCj4gPj4gTm90ZSB0aGF0LCBuZWl0aGVyIFBDSWUgaG9zdCBicmlkZ2Ug
bm9yIFBDSWUgY29yZSByZS1pc3N1ZXMgdGhlCj4gPj4gcmVxdWVzdCBmb3IgYW55IGNvbmZpZ3Vy
YXRpb24gb2Zmc2V0Lgo+ID4+Cj4gPj4gRm9yIGNvbmZpZyByZWFkcywgdGhpcyBoYXJkd2FyZSBy
ZXR1cm5zIENGR19SRVRSWV9TVEFUVVMgZGF0YSB3aGVuCj4gPj4gaXQgcmVjZWl2ZXMgYSBDUlMg
Y29tcGxldGlvbiBmb3IgYSBjb25maWcgcmVhZCwgcmVnYXJkbGVzcyBvZiB0aGUKPiA+PiBhZGRy
ZXNzIG9mIHRoZSByZWFkIG9yIHRoZSBDUlMgU29mdHdhcmUgVmlzaWJpbGl0eSBFbmFibGUgYml0
Lgo+ID4KPiA+IEkgY2FuJ3QgcmVtZW1iZXIgaG93IFN0aW5ncmF5IGhhbmRsZXMgdGhlIENSUyBT
b2Z0d2FyZSBWaXNpYmlsaXR5IEVuYWJsZQo+ID4gYml0LiAgSXMgaXQgYSByZWFkLW9ubHkgemVy
bz8gIElzIGl0IHdyaXRhYmxlPyAgRG9lcyB0aGUgaGFyZHdhcmUgbG9vayBhdAo+ID4gaXQgYWxs
IChJIHRoaW5rIG5vdCk/Cj4gCj4gSFcgZG9lc250IGxvb2sgaXQgYXQgYWxsLCBpdCBpcyAiZG9u
dCBjYXJlIiBiaXQuCgpTaWdoLCBJIG1hZGUgdGhlIGNsYXNzaWMgbWlzdGFrZSBvZiBhc2tpbmcg
bW9yZSB0aGFuIG9uZSBxdWVzdGlvbiwgYW5kCkkgZ3Vlc3MgSSdtIGFib3V0IHRvIGRvIGl0IGFn
YWluIDopICBJdCdsbCBzYXZlIHRpbWUgaWYgeW91IGNhbiBhbnN3ZXIKYWxsIHRoZSBxdWVzdGlv
bnMgYXQgb25jZS4KCkxpbnV4IGVuYWJsZXMgUENJX0VYUF9SVENUTF9DUlNTVkUgaWYgUENJX0VY
UF9SVENBUCBzYXlzIGl0IGlzCnN1cHBvcnRlZCAoc2VlIHBjaV9lbmFibGVfY3JzKCkpLgoKICAt
IFdoYXQgZG9lcyBQQ0lfRVhQX1JUQ0FQIHNheT8KCiAgLSBJcyBQQ0lfRVhQX1JUQ1RMX0NSU1NW
RSB3cml0YWJsZT8KCldpdGggYWxsIHRoZSBDUlMtcmVsYXRlZCB3b3JrIGdvaW5nIG9uLCBJIHN1
c3BlY3Qgd2UgbWF5IHNvbWVkYXkgd2FudCB0bwpyZWFkIFBDSV9FWFBfUlRDVExfQ1JTU1ZFIHRv
IHNlZSBpZiBDUlMgU1YgaXMgZW5hYmxlZC4KCkJqb3JuCgpfX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdAps
aW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVh
ZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Tue, Aug 29, 2017 at 01:09:53AM +0530, Oza Oza wrote:
> On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> >> PCIe spec r3.1, sec 2.3.2
> >> If CRS software visibility is not enabled, the RC must reissue the
> >> config request as a new request.
> >>
> >> - If CRS software visibility is enabled,
> >> - for a config read of Vendor ID, the RC must return 0x0001 data
> >> - for all other config reads/writes, the RC must reissue the
> >>   request
> >>
> >> iproc PCIe Controller spec:
> >> 4.7.3.3. Retry Status On Configuration Cycle
> >> Endpoints are allowed to generate retry status on configuration
> >> cycles. In this case, the RC needs to re-issue the request. The IP
> >> does not handle this because the number of configuration cycles needed
> >> will probably be less than the total number of non-posted operations
> >> needed.
> >>
> >> When a retry status is received on the User RX interface for a
> >> configuration request that was sent on the User TX interface,
> >> it will be indicated with a completion with the CMPL_STATUS field set
> >> to 2=CRS, and the user will have to find the address and data values
> >> and send a new transaction on the User TX interface.
> >> When the internal configuration space returns a retry status during a
> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> >> CRS status.
> >> When the CRS Software Visibility Enable register in the Root Control
> >> register is enabled, the IP will return the data value to 0x0001 for
> >> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
> >> the request for reads of offset 0 that return with CRS status.  This
> >> is true for both the User RX Interface and for the Command/Status
> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> >> field of the completion on the User RX Interface will not be 2=CRS and
> >> the pcie_cscrs signal will not assert on the Command/Status interface.
> >>
> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> >> with Configuration Request Retry Status (CRS) should be reissued by
> >> the hardware except reads of the Vendor ID when CRS Software
> >> Visibility is enabled.
> >>
> >> This hardware never reissues configuration requests when it receives
> >> CRS completions.
> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
> >> request for any configuration offset.
> >>
> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
> >> it receives a CRS completion for a config read, regardless of the
> >> address of the read or the CRS Software Visibility Enable bit.
> >
> > I can't remember how Stingray handles the CRS Software Visibility Enable
> > bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
> > it all (I think not)?
> 
> HW doesnt look it at all, it is "dont care" bit.

Sigh, I made the classic mistake of asking more than one question, and
I guess I'm about to do it again :)  It'll save time if you can answer
all the questions at once.

Linux enables PCI_EXP_RTCTL_CRSSVE if PCI_EXP_RTCAP says it is
supported (see pci_enable_crs()).

  - What does PCI_EXP_RTCAP say?

  - Is PCI_EXP_RTCTL_CRSSVE writable?

With all the CRS-related work going on, I suspect we may someday want to
read PCI_EXP_RTCTL_CRSSVE to see if CRS SV is enabled.

Bjorn

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 1:24 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Aug 29, 2017 at 01:09:53AM +0530, Oza Oza wrote:
>> On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> >> PCIe spec r3.1, sec 2.3.2
>> >> If CRS software visibility is not enabled, the RC must reissue the
>> >> config request as a new request.
>> >>
>> >> - If CRS software visibility is enabled,
>> >> - for a config read of Vendor ID, the RC must return 0x0001 data
>> >> - for all other config reads/writes, the RC must reissue the
>> >>   request
>> >>
>> >> iproc PCIe Controller spec:
>> >> 4.7.3.3. Retry Status On Configuration Cycle
>> >> Endpoints are allowed to generate retry status on configuration
>> >> cycles. In this case, the RC needs to re-issue the request. The IP
>> >> does not handle this because the number of configuration cycles needed
>> >> will probably be less than the total number of non-posted operations
>> >> needed.
>> >>
>> >> When a retry status is received on the User RX interface for a
>> >> configuration request that was sent on the User TX interface,
>> >> it will be indicated with a completion with the CMPL_STATUS field set
>> >> to 2=CRS, and the user will have to find the address and data values
>> >> and send a new transaction on the User TX interface.
>> >> When the internal configuration space returns a retry status during a
>> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> >> CRS status.
>> >> When the CRS Software Visibility Enable register in the Root Control
>> >> register is enabled, the IP will return the data value to 0x0001 for
>> >> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
>> >> the request for reads of offset 0 that return with CRS status.  This
>> >> is true for both the User RX Interface and for the Command/Status
>> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> >> field of the completion on the User RX Interface will not be 2=CRS and
>> >> the pcie_cscrs signal will not assert on the Command/Status interface.
>> >>
>> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> >> with Configuration Request Retry Status (CRS) should be reissued by
>> >> the hardware except reads of the Vendor ID when CRS Software
>> >> Visibility is enabled.
>> >>
>> >> This hardware never reissues configuration requests when it receives
>> >> CRS completions.
>> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> >> request for any configuration offset.
>> >>
>> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> it receives a CRS completion for a config read, regardless of the
>> >> address of the read or the CRS Software Visibility Enable bit.
>> >
>> > I can't remember how Stingray handles the CRS Software Visibility Enable
>> > bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
>> > it all (I think not)?
>>
>> HW doesnt look it at all, it is "dont care" bit.
>
> Sigh, I made the classic mistake of asking more than one question, and
> I guess I'm about to do it again :)  It'll save time if you can answer
> all the questions at once.
>
> Linux enables PCI_EXP_RTCTL_CRSSVE if PCI_EXP_RTCAP says it is
> supported (see pci_enable_crs()).
>
>   - What does PCI_EXP_RTCAP say?
it is rea as set: value :0x1
>
>   - Is PCI_EXP_RTCTL_CRSSVE writable?
yes: it is:
read as 0 and written as 1.
>
> With all the CRS-related work going on, I suspect we may someday want to
> read PCI_EXP_RTCTL_CRSSVE to see if CRS SV is enabled.
>
> Bjorn

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 1:24 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Aug 29, 2017 at 01:09:53AM +0530, Oza Oza wrote:
>> On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wro=
te:
>> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> >> PCIe spec r3.1, sec 2.3.2
>> >> If CRS software visibility is not enabled, the RC must reissue the
>> >> config request as a new request.
>> >>
>> >> - If CRS software visibility is enabled,
>> >> - for a config read of Vendor ID, the RC must return 0x0001 data
>> >> - for all other config reads/writes, the RC must reissue the
>> >>   request
>> >>
>> >> iproc PCIe Controller spec:
>> >> 4.7.3.3. Retry Status On Configuration Cycle
>> >> Endpoints are allowed to generate retry status on configuration
>> >> cycles. In this case, the RC needs to re-issue the request. The IP
>> >> does not handle this because the number of configuration cycles neede=
d
>> >> will probably be less than the total number of non-posted operations
>> >> needed.
>> >>
>> >> When a retry status is received on the User RX interface for a
>> >> configuration request that was sent on the User TX interface,
>> >> it will be indicated with a completion with the CMPL_STATUS field set
>> >> to 2=3DCRS, and the user will have to find the address and data value=
s
>> >> and send a new transaction on the User TX interface.
>> >> When the internal configuration space returns a retry status during a
>> >> configuration cycle (user_cscfg =3D 1) on the Command/Status interfac=
e,
>> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> >> CRS status.
>> >> When the CRS Software Visibility Enable register in the Root Control
>> >> register is enabled, the IP will return the data value to 0x0001 for
>> >> the Vendor ID value and 0xffff  (all 1=E2=80=99s) for the rest of the=
 data in
>> >> the request for reads of offset 0 that return with CRS status.  This
>> >> is true for both the User RX Interface and for the Command/Status
>> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> >> field of the completion on the User RX Interface will not be 2=3DCRS =
and
>> >> the pcie_cscrs signal will not assert on the Command/Status interface=
.
>> >>
>> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> >> with Configuration Request Retry Status (CRS) should be reissued by
>> >> the hardware except reads of the Vendor ID when CRS Software
>> >> Visibility is enabled.
>> >>
>> >> This hardware never reissues configuration requests when it receives
>> >> CRS completions.
>> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> >> request for any configuration offset.
>> >>
>> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> it receives a CRS completion for a config read, regardless of the
>> >> address of the read or the CRS Software Visibility Enable bit.
>> >
>> > I can't remember how Stingray handles the CRS Software Visibility Enab=
le
>> > bit.  Is it a read-only zero?  Is it writable?  Does the hardware look=
 at
>> > it all (I think not)?
>>
>> HW doesnt look it at all, it is "dont care" bit.
>
> Sigh, I made the classic mistake of asking more than one question, and
> I guess I'm about to do it again :)  It'll save time if you can answer
> all the questions at once.
>
> Linux enables PCI_EXP_RTCTL_CRSSVE if PCI_EXP_RTCAP says it is
> supported (see pci_enable_crs()).
>
>   - What does PCI_EXP_RTCAP say?
it is rea as set: value :0x1
>
>   - Is PCI_EXP_RTCTL_CRSSVE writable?
yes: it is:
read as 0 and written as 1.
>
> With all the CRS-related work going on, I suspect we may someday want to
> read PCI_EXP_RTCTL_CRSSVE to see if CRS SV is enabled.
>
> Bjorn

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 1:24 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Aug 29, 2017 at 01:09:53AM +0530, Oza Oza wrote:
>> On Tue, Aug 29, 2017 at 12:47 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> >> PCIe spec r3.1, sec 2.3.2
>> >> If CRS software visibility is not enabled, the RC must reissue the
>> >> config request as a new request.
>> >>
>> >> - If CRS software visibility is enabled,
>> >> - for a config read of Vendor ID, the RC must return 0x0001 data
>> >> - for all other config reads/writes, the RC must reissue the
>> >>   request
>> >>
>> >> iproc PCIe Controller spec:
>> >> 4.7.3.3. Retry Status On Configuration Cycle
>> >> Endpoints are allowed to generate retry status on configuration
>> >> cycles. In this case, the RC needs to re-issue the request. The IP
>> >> does not handle this because the number of configuration cycles needed
>> >> will probably be less than the total number of non-posted operations
>> >> needed.
>> >>
>> >> When a retry status is received on the User RX interface for a
>> >> configuration request that was sent on the User TX interface,
>> >> it will be indicated with a completion with the CMPL_STATUS field set
>> >> to 2=CRS, and the user will have to find the address and data values
>> >> and send a new transaction on the User TX interface.
>> >> When the internal configuration space returns a retry status during a
>> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> >> CRS status.
>> >> When the CRS Software Visibility Enable register in the Root Control
>> >> register is enabled, the IP will return the data value to 0x0001 for
>> >> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
>> >> the request for reads of offset 0 that return with CRS status.  This
>> >> is true for both the User RX Interface and for the Command/Status
>> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> >> field of the completion on the User RX Interface will not be 2=CRS and
>> >> the pcie_cscrs signal will not assert on the Command/Status interface.
>> >>
>> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> >> with Configuration Request Retry Status (CRS) should be reissued by
>> >> the hardware except reads of the Vendor ID when CRS Software
>> >> Visibility is enabled.
>> >>
>> >> This hardware never reissues configuration requests when it receives
>> >> CRS completions.
>> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> >> request for any configuration offset.
>> >>
>> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> it receives a CRS completion for a config read, regardless of the
>> >> address of the read or the CRS Software Visibility Enable bit.
>> >
>> > I can't remember how Stingray handles the CRS Software Visibility Enable
>> > bit.  Is it a read-only zero?  Is it writable?  Does the hardware look at
>> > it all (I think not)?
>>
>> HW doesnt look it at all, it is "dont care" bit.
>
> Sigh, I made the classic mistake of asking more than one question, and
> I guess I'm about to do it again :)  It'll save time if you can answer
> all the questions at once.
>
> Linux enables PCI_EXP_RTCTL_CRSSVE if PCI_EXP_RTCAP says it is
> supported (see pci_enable_crs()).
>
>   - What does PCI_EXP_RTCAP say?
it is rea as set: value :0x1
>
>   - Is PCI_EXP_RTCTL_CRSSVE writable?
yes: it is:
read as 0 and written as 1.
>
> With all the CRS-related work going on, I suspect we may someday want to
> read PCI_EXP_RTCTL_CRSSVE to see if CRS SV is enabled.
>
> Bjorn

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> PCIe spec r3.1, sec 2.3.2
> If CRS software visibility is not enabled, the RC must reissue the
> config request as a new request.
> 
> - If CRS software visibility is enabled,
> - for a config read of Vendor ID, the RC must return 0x0001 data
> - for all other config reads/writes, the RC must reissue the
>   request
> 
> iproc PCIe Controller spec:
> 4.7.3.3. Retry Status On Configuration Cycle
> Endpoints are allowed to generate retry status on configuration
> cycles. In this case, the RC needs to re-issue the request. The IP
> does not handle this because the number of configuration cycles needed
> will probably be less than the total number of non-posted operations
> needed.
> 
> When a retry status is received on the User RX interface for a
> configuration request that was sent on the User TX interface,
> it will be indicated with a completion with the CMPL_STATUS field set
> to 2=CRS, and the user will have to find the address and data values
> and send a new transaction on the User TX interface.
> When the internal configuration space returns a retry status during a
> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> CRS status.
> When the CRS Software Visibility Enable register in the Root Control
> register is enabled, the IP will return the data value to 0x0001 for
> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
> the request for reads of offset 0 that return with CRS status.  This
> is true for both the User RX Interface and for the Command/Status
> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> field of the completion on the User RX Interface will not be 2=CRS and
> the pcie_cscrs signal will not assert on the Command/Status interface.
> 
> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> with Configuration Request Retry Status (CRS) should be reissued by
> the hardware except reads of the Vendor ID when CRS Software
> Visibility is enabled.
> 
> This hardware never reissues configuration requests when it receives
> CRS completions.
> Note that, neither PCIe host bridge nor PCIe core re-issues the
> request for any configuration offset.
> 
> For config reads, this hardware returns CFG_RETRY_STATUS data when
> it receives a CRS completion for a config read, regardless of the
> address of the read or the CRS Software Visibility Enable bit.
> 
> This patch implements iproc_pcie_config_read which gets called for
> Stingray, if it receives a CRS completion, it retries reading it again.
> In case of timeout, it returns 0xffffffff.
> For other iproc based SOC, it falls back to PCI generic APIs.
> 
> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 61d9be6..37f4adf 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -68,6 +68,9 @@
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> +#define CFG_RETRY_STATUS             0xffff0001
> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> +
>  /* derive the enum index of the outbound/inbound mapping registers */
>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>  
> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>  	return (pcie->base + offset);
>  }
>  
> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> +{
> +	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> +	unsigned int data;
> +
> +	/*
> +	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
> +	 * Visibility only affects config read of the Vendor ID.
> +	 * For config write or any other config read the Root must
> +	 * automatically re-issue configuration request again as a
> +	 * new request.
> +	 *
> +	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
> +	 * it receives a CRS completion for a config read, regardless of the
> +	 * address of the read or the CRS Software Visibility Enable bit. As a
> +	 * partial workaround for this, we retry in software any read that
> +	 * returns CFG_RETRY_STATUS.
> +	 */
> +	data = readl(cfg_data_p);
> +	while (data == CFG_RETRY_STATUS && timeout--) {
> +		udelay(1);
> +		data = readl(cfg_data_p);
> +	}
> +
> +	if (data == CFG_RETRY_STATUS)
> +		data = 0xffffffff;
> +
> +	return data;
> +}
> +
> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	unsigned int slot = PCI_SLOT(devfn);
> +	unsigned int fn = PCI_FUNC(devfn);
> +	unsigned int busno = bus->number;
> +	void __iomem *cfg_data_p;
> +	unsigned int data;
> +
> +	/* root complex access. */
> +	if (busno == 0)
> +		return pci_generic_config_read32(bus, devfn, where, size, val);

It sounds like Stingray advertises CRS SV support in its Root Capabilities
register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
advertise it.  That will keep Linux from trying to enable it.  I know the
hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
confusing users reading the lspci output.

We did something similar with f09f8735fb9c ("PCI: xgene: Disable
Configuration Request Retry Status for v1 silicon").

I tried to do this in the patch I pushed to pci/host-iproc.

> +
> +	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> +
> +	if (!cfg_data_p)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	data = iproc_pcie_cfg_retry(cfg_data_p);
> +
> +	*val = data;
> +	if (size <= 2)
> +		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  /**
>   * Note access to the configuration registers are protected at the higher layer
>   * by 'pci_lock' in drivers/pci/access.c
> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>  				    int where, int size, u32 *val)
>  {
>  	int ret;
> +	struct iproc_pcie *pcie = iproc_data(bus);
>  
>  	iproc_pcie_apb_err_disable(bus, true);
> +	if (pcie->type == IPROC_PCIE_PAXB_V2)
> +		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
> +	else
> +		ret = pci_generic_config_read32(bus, devfn, where, size, val);
>  	ret = pci_generic_config_read32(bus, devfn, where, size, val);

This last pci_generic_config_read32() call looks like a duplicate.

>  	iproc_pcie_apb_err_disable(bus, false);
>  
> -- 
> 1.9.1
> 

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> PCIe spec r3.1, sec 2.3.2
> If CRS software visibility is not enabled, the RC must reissue the
> config request as a new request.
> 
> - If CRS software visibility is enabled,
> - for a config read of Vendor ID, the RC must return 0x0001 data
> - for all other config reads/writes, the RC must reissue the
>   request
> 
> iproc PCIe Controller spec:
> 4.7.3.3. Retry Status On Configuration Cycle
> Endpoints are allowed to generate retry status on configuration
> cycles. In this case, the RC needs to re-issue the request. The IP
> does not handle this because the number of configuration cycles needed
> will probably be less than the total number of non-posted operations
> needed.
> 
> When a retry status is received on the User RX interface for a
> configuration request that was sent on the User TX interface,
> it will be indicated with a completion with the CMPL_STATUS field set
> to 2=CRS, and the user will have to find the address and data values
> and send a new transaction on the User TX interface.
> When the internal configuration space returns a retry status during a
> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> CRS status.
> When the CRS Software Visibility Enable register in the Root Control
> register is enabled, the IP will return the data value to 0x0001 for
> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
> the request for reads of offset 0 that return with CRS status.  This
> is true for both the User RX Interface and for the Command/Status
> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> field of the completion on the User RX Interface will not be 2=CRS and
> the pcie_cscrs signal will not assert on the Command/Status interface.
> 
> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> with Configuration Request Retry Status (CRS) should be reissued by
> the hardware except reads of the Vendor ID when CRS Software
> Visibility is enabled.
> 
> This hardware never reissues configuration requests when it receives
> CRS completions.
> Note that, neither PCIe host bridge nor PCIe core re-issues the
> request for any configuration offset.
> 
> For config reads, this hardware returns CFG_RETRY_STATUS data when
> it receives a CRS completion for a config read, regardless of the
> address of the read or the CRS Software Visibility Enable bit.
> 
> This patch implements iproc_pcie_config_read which gets called for
> Stingray, if it receives a CRS completion, it retries reading it again.
> In case of timeout, it returns 0xffffffff.
> For other iproc based SOC, it falls back to PCI generic APIs.
> 
> Signed-off-by: Oza Pawandeep <oza.oza-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 61d9be6..37f4adf 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -68,6 +68,9 @@
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> +#define CFG_RETRY_STATUS             0xffff0001
> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> +
>  /* derive the enum index of the outbound/inbound mapping registers */
>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>  
> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>  	return (pcie->base + offset);
>  }
>  
> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> +{
> +	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> +	unsigned int data;
> +
> +	/*
> +	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
> +	 * Visibility only affects config read of the Vendor ID.
> +	 * For config write or any other config read the Root must
> +	 * automatically re-issue configuration request again as a
> +	 * new request.
> +	 *
> +	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
> +	 * it receives a CRS completion for a config read, regardless of the
> +	 * address of the read or the CRS Software Visibility Enable bit. As a
> +	 * partial workaround for this, we retry in software any read that
> +	 * returns CFG_RETRY_STATUS.
> +	 */
> +	data = readl(cfg_data_p);
> +	while (data == CFG_RETRY_STATUS && timeout--) {
> +		udelay(1);
> +		data = readl(cfg_data_p);
> +	}
> +
> +	if (data == CFG_RETRY_STATUS)
> +		data = 0xffffffff;
> +
> +	return data;
> +}
> +
> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	unsigned int slot = PCI_SLOT(devfn);
> +	unsigned int fn = PCI_FUNC(devfn);
> +	unsigned int busno = bus->number;
> +	void __iomem *cfg_data_p;
> +	unsigned int data;
> +
> +	/* root complex access. */
> +	if (busno == 0)
> +		return pci_generic_config_read32(bus, devfn, where, size, val);

It sounds like Stingray advertises CRS SV support in its Root Capabilities
register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
advertise it.  That will keep Linux from trying to enable it.  I know the
hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
confusing users reading the lspci output.

We did something similar with f09f8735fb9c ("PCI: xgene: Disable
Configuration Request Retry Status for v1 silicon").

I tried to do this in the patch I pushed to pci/host-iproc.

> +
> +	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> +
> +	if (!cfg_data_p)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	data = iproc_pcie_cfg_retry(cfg_data_p);
> +
> +	*val = data;
> +	if (size <= 2)
> +		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  /**
>   * Note access to the configuration registers are protected at the higher layer
>   * by 'pci_lock' in drivers/pci/access.c
> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>  				    int where, int size, u32 *val)
>  {
>  	int ret;
> +	struct iproc_pcie *pcie = iproc_data(bus);
>  
>  	iproc_pcie_apb_err_disable(bus, true);
> +	if (pcie->type == IPROC_PCIE_PAXB_V2)
> +		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
> +	else
> +		ret = pci_generic_config_read32(bus, devfn, where, size, val);
>  	ret = pci_generic_config_read32(bus, devfn, where, size, val);

This last pci_generic_config_read32() call looks like a duplicate.

>  	iproc_pcie_apb_err_disable(bus, false);
>  
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

T24gVGh1LCBBdWcgMjQsIDIwMTcgYXQgMTA6MzQ6MjVBTSArMDUzMCwgT3phIFBhd2FuZGVlcCB3
cm90ZToKPiBQQ0llIHNwZWMgcjMuMSwgc2VjIDIuMy4yCj4gSWYgQ1JTIHNvZnR3YXJlIHZpc2li
aWxpdHkgaXMgbm90IGVuYWJsZWQsIHRoZSBSQyBtdXN0IHJlaXNzdWUgdGhlCj4gY29uZmlnIHJl
cXVlc3QgYXMgYSBuZXcgcmVxdWVzdC4KPiAKPiAtIElmIENSUyBzb2Z0d2FyZSB2aXNpYmlsaXR5
IGlzIGVuYWJsZWQsCj4gLSBmb3IgYSBjb25maWcgcmVhZCBvZiBWZW5kb3IgSUQsIHRoZSBSQyBt
dXN0IHJldHVybiAweDAwMDEgZGF0YQo+IC0gZm9yIGFsbCBvdGhlciBjb25maWcgcmVhZHMvd3Jp
dGVzLCB0aGUgUkMgbXVzdCByZWlzc3VlIHRoZQo+ICAgcmVxdWVzdAo+IAo+IGlwcm9jIFBDSWUg
Q29udHJvbGxlciBzcGVjOgo+IDQuNy4zLjMuIFJldHJ5IFN0YXR1cyBPbiBDb25maWd1cmF0aW9u
IEN5Y2xlCj4gRW5kcG9pbnRzIGFyZSBhbGxvd2VkIHRvIGdlbmVyYXRlIHJldHJ5IHN0YXR1cyBv
biBjb25maWd1cmF0aW9uCj4gY3ljbGVzLiBJbiB0aGlzIGNhc2UsIHRoZSBSQyBuZWVkcyB0byBy
ZS1pc3N1ZSB0aGUgcmVxdWVzdC4gVGhlIElQCj4gZG9lcyBub3QgaGFuZGxlIHRoaXMgYmVjYXVz
ZSB0aGUgbnVtYmVyIG9mIGNvbmZpZ3VyYXRpb24gY3ljbGVzIG5lZWRlZAo+IHdpbGwgcHJvYmFi
bHkgYmUgbGVzcyB0aGFuIHRoZSB0b3RhbCBudW1iZXIgb2Ygbm9uLXBvc3RlZCBvcGVyYXRpb25z
Cj4gbmVlZGVkLgo+IAo+IFdoZW4gYSByZXRyeSBzdGF0dXMgaXMgcmVjZWl2ZWQgb24gdGhlIFVz
ZXIgUlggaW50ZXJmYWNlIGZvciBhCj4gY29uZmlndXJhdGlvbiByZXF1ZXN0IHRoYXQgd2FzIHNl
bnQgb24gdGhlIFVzZXIgVFggaW50ZXJmYWNlLAo+IGl0IHdpbGwgYmUgaW5kaWNhdGVkIHdpdGgg
YSBjb21wbGV0aW9uIHdpdGggdGhlIENNUExfU1RBVFVTIGZpZWxkIHNldAo+IHRvIDI9Q1JTLCBh
bmQgdGhlIHVzZXIgd2lsbCBoYXZlIHRvIGZpbmQgdGhlIGFkZHJlc3MgYW5kIGRhdGEgdmFsdWVz
Cj4gYW5kIHNlbmQgYSBuZXcgdHJhbnNhY3Rpb24gb24gdGhlIFVzZXIgVFggaW50ZXJmYWNlLgo+
IFdoZW4gdGhlIGludGVybmFsIGNvbmZpZ3VyYXRpb24gc3BhY2UgcmV0dXJucyBhIHJldHJ5IHN0
YXR1cyBkdXJpbmcgYQo+IGNvbmZpZ3VyYXRpb24gY3ljbGUgKHVzZXJfY3NjZmcgPSAxKSBvbiB0
aGUgQ29tbWFuZC9TdGF0dXMgaW50ZXJmYWNlLAo+IHRoZSBwY2llX2NzY3JzIHdpbGwgYXNzZXJ0
IHdpdGggdGhlIHBjaWVfY3NhY2sgc2lnbmFsIHRvIGluZGljYXRlIHRoZQo+IENSUyBzdGF0dXMu
Cj4gV2hlbiB0aGUgQ1JTIFNvZnR3YXJlIFZpc2liaWxpdHkgRW5hYmxlIHJlZ2lzdGVyIGluIHRo
ZSBSb290IENvbnRyb2wKPiByZWdpc3RlciBpcyBlbmFibGVkLCB0aGUgSVAgd2lsbCByZXR1cm4g
dGhlIGRhdGEgdmFsdWUgdG8gMHgwMDAxIGZvcgo+IHRoZSBWZW5kb3IgSUQgdmFsdWUgYW5kIDB4
ZmZmZiAgKGFsbCAx4oCZcykgZm9yIHRoZSByZXN0IG9mIHRoZSBkYXRhIGluCj4gdGhlIHJlcXVl
c3QgZm9yIHJlYWRzIG9mIG9mZnNldCAwIHRoYXQgcmV0dXJuIHdpdGggQ1JTIHN0YXR1cy4gIFRo
aXMKPiBpcyB0cnVlIGZvciBib3RoIHRoZSBVc2VyIFJYIEludGVyZmFjZSBhbmQgZm9yIHRoZSBD
b21tYW5kL1N0YXR1cwo+IGludGVyZmFjZS4gIFdoZW4gQ1JTIFNvZnR3YXJlIFZpc2liaWxpdHkg
aXMgZW5hYmxlZCwgdGhlIENNUExfU1RBVFVTCj4gZmllbGQgb2YgdGhlIGNvbXBsZXRpb24gb24g
dGhlIFVzZXIgUlggSW50ZXJmYWNlIHdpbGwgbm90IGJlIDI9Q1JTIGFuZAo+IHRoZSBwY2llX2Nz
Y3JzIHNpZ25hbCB3aWxsIG5vdCBhc3NlcnQgb24gdGhlIENvbW1hbmQvU3RhdHVzIGludGVyZmFj
ZS4KPiAKPiBQZXIgUENJZSByMy4xLCBzZWMgMi4zLjIsIGNvbmZpZyByZXF1ZXN0cyB0aGF0IHJl
Y2VpdmUgY29tcGxldGlvbnMKPiB3aXRoIENvbmZpZ3VyYXRpb24gUmVxdWVzdCBSZXRyeSBTdGF0
dXMgKENSUykgc2hvdWxkIGJlIHJlaXNzdWVkIGJ5Cj4gdGhlIGhhcmR3YXJlIGV4Y2VwdCByZWFk
cyBvZiB0aGUgVmVuZG9yIElEIHdoZW4gQ1JTIFNvZnR3YXJlCj4gVmlzaWJpbGl0eSBpcyBlbmFi
bGVkLgo+IAo+IFRoaXMgaGFyZHdhcmUgbmV2ZXIgcmVpc3N1ZXMgY29uZmlndXJhdGlvbiByZXF1
ZXN0cyB3aGVuIGl0IHJlY2VpdmVzCj4gQ1JTIGNvbXBsZXRpb25zLgo+IE5vdGUgdGhhdCwgbmVp
dGhlciBQQ0llIGhvc3QgYnJpZGdlIG5vciBQQ0llIGNvcmUgcmUtaXNzdWVzIHRoZQo+IHJlcXVl
c3QgZm9yIGFueSBjb25maWd1cmF0aW9uIG9mZnNldC4KPiAKPiBGb3IgY29uZmlnIHJlYWRzLCB0
aGlzIGhhcmR3YXJlIHJldHVybnMgQ0ZHX1JFVFJZX1NUQVRVUyBkYXRhIHdoZW4KPiBpdCByZWNl
aXZlcyBhIENSUyBjb21wbGV0aW9uIGZvciBhIGNvbmZpZyByZWFkLCByZWdhcmRsZXNzIG9mIHRo
ZQo+IGFkZHJlc3Mgb2YgdGhlIHJlYWQgb3IgdGhlIENSUyBTb2Z0d2FyZSBWaXNpYmlsaXR5IEVu
YWJsZSBiaXQuCj4gCj4gVGhpcyBwYXRjaCBpbXBsZW1lbnRzIGlwcm9jX3BjaWVfY29uZmlnX3Jl
YWQgd2hpY2ggZ2V0cyBjYWxsZWQgZm9yCj4gU3RpbmdyYXksIGlmIGl0IHJlY2VpdmVzIGEgQ1JT
IGNvbXBsZXRpb24sIGl0IHJldHJpZXMgcmVhZGluZyBpdCBhZ2Fpbi4KPiBJbiBjYXNlIG9mIHRp
bWVvdXQsIGl0IHJldHVybnMgMHhmZmZmZmZmZi4KPiBGb3Igb3RoZXIgaXByb2MgYmFzZWQgU09D
LCBpdCBmYWxscyBiYWNrIHRvIFBDSSBnZW5lcmljIEFQSXMuCj4gCj4gU2lnbmVkLW9mZi1ieTog
T3phIFBhd2FuZGVlcCA8b3phLm96YUBicm9hZGNvbS5jb20+Cj4gCj4gZGlmZiAtLWdpdCBhL2Ry
aXZlcnMvcGNpL2hvc3QvcGNpZS1pcHJvYy5jIGIvZHJpdmVycy9wY2kvaG9zdC9wY2llLWlwcm9j
LmMKPiBpbmRleCA2MWQ5YmU2Li4zN2Y0YWRmIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvcGNpL2hv
c3QvcGNpZS1pcHJvYy5jCj4gKysrIGIvZHJpdmVycy9wY2kvaG9zdC9wY2llLWlwcm9jLmMKPiBA
QCAtNjgsNiArNjgsOSBAQAo+ICAjZGVmaW5lIEFQQl9FUlJfRU5fU0hJRlQgICAgICAgICAgICAg
MAo+ICAjZGVmaW5lIEFQQl9FUlJfRU4gICAgICAgICAgICAgICAgICAgQklUKEFQQl9FUlJfRU5f
U0hJRlQpCj4gIAo+ICsjZGVmaW5lIENGR19SRVRSWV9TVEFUVVMgICAgICAgICAgICAgMHhmZmZm
MDAwMQo+ICsjZGVmaW5lIENGR19SRVRSWV9TVEFUVVNfVElNRU9VVF9VUyAgNTAwMDAwIC8qIDUw
MCBtaWxsaS1zZWNvbmRzLiAqLwo+ICsKPiAgLyogZGVyaXZlIHRoZSBlbnVtIGluZGV4IG9mIHRo
ZSBvdXRib3VuZC9pbmJvdW5kIG1hcHBpbmcgcmVnaXN0ZXJzICovCj4gICNkZWZpbmUgTUFQX1JF
RyhiYXNlX3JlZywgaW5kZXgpICAgICAgKChiYXNlX3JlZykgKyAoaW5kZXgpICogMikKPiAgCj4g
QEAgLTQ3Myw2ICs0NzYsNjQgQEAgc3RhdGljIHZvaWQgX19pb21lbSAqaXByb2NfcGNpZV9tYXBf
ZXBfY2ZnX3JlZyhzdHJ1Y3QgaXByb2NfcGNpZSAqcGNpZSwKPiAgCXJldHVybiAocGNpZS0+YmFz
ZSArIG9mZnNldCk7Cj4gIH0KPiAgCj4gK3N0YXRpYyB1bnNpZ25lZCBpbnQgaXByb2NfcGNpZV9j
ZmdfcmV0cnkodm9pZCBfX2lvbWVtICpjZmdfZGF0YV9wKQo+ICt7Cj4gKwlpbnQgdGltZW91dCA9
IENGR19SRVRSWV9TVEFUVVNfVElNRU9VVF9VUzsKPiArCXVuc2lnbmVkIGludCBkYXRhOwo+ICsK
PiArCS8qCj4gKwkgKiBBcyBwZXIgUENJZSBzcGVjIHIzLjEsIHNlYyAyLjMuMiwgQ1JTIFNvZnR3
YXJlCj4gKwkgKiBWaXNpYmlsaXR5IG9ubHkgYWZmZWN0cyBjb25maWcgcmVhZCBvZiB0aGUgVmVu
ZG9yIElELgo+ICsJICogRm9yIGNvbmZpZyB3cml0ZSBvciBhbnkgb3RoZXIgY29uZmlnIHJlYWQg
dGhlIFJvb3QgbXVzdAo+ICsJICogYXV0b21hdGljYWxseSByZS1pc3N1ZSBjb25maWd1cmF0aW9u
IHJlcXVlc3QgYWdhaW4gYXMgYQo+ICsJICogbmV3IHJlcXVlc3QuCj4gKwkgKgo+ICsJICogRm9y
IGNvbmZpZyByZWFkcywgdGhpcyBoYXJkd2FyZSByZXR1cm5zIENGR19SRVRSWV9TVEFUVVMgZGF0
YSB3aGVuCj4gKwkgKiBpdCByZWNlaXZlcyBhIENSUyBjb21wbGV0aW9uIGZvciBhIGNvbmZpZyBy
ZWFkLCByZWdhcmRsZXNzIG9mIHRoZQo+ICsJICogYWRkcmVzcyBvZiB0aGUgcmVhZCBvciB0aGUg
Q1JTIFNvZnR3YXJlIFZpc2liaWxpdHkgRW5hYmxlIGJpdC4gQXMgYQo+ICsJICogcGFydGlhbCB3
b3JrYXJvdW5kIGZvciB0aGlzLCB3ZSByZXRyeSBpbiBzb2Z0d2FyZSBhbnkgcmVhZCB0aGF0Cj4g
KwkgKiByZXR1cm5zIENGR19SRVRSWV9TVEFUVVMuCj4gKwkgKi8KPiArCWRhdGEgPSByZWFkbChj
ZmdfZGF0YV9wKTsKPiArCXdoaWxlIChkYXRhID09IENGR19SRVRSWV9TVEFUVVMgJiYgdGltZW91
dC0tKSB7Cj4gKwkJdWRlbGF5KDEpOwo+ICsJCWRhdGEgPSByZWFkbChjZmdfZGF0YV9wKTsKPiAr
CX0KPiArCj4gKwlpZiAoZGF0YSA9PSBDRkdfUkVUUllfU1RBVFVTKQo+ICsJCWRhdGEgPSAweGZm
ZmZmZmZmOwo+ICsKPiArCXJldHVybiBkYXRhOwo+ICt9Cj4gKwo+ICtzdGF0aWMgaW50IGlwcm9j
X3BjaWVfY29uZmlnX3JlYWQoc3RydWN0IHBjaV9idXMgKmJ1cywgdW5zaWduZWQgaW50IGRldmZu
LAo+ICsJCQkJICAgIGludCB3aGVyZSwgaW50IHNpemUsIHUzMiAqdmFsKQo+ICt7Cj4gKwlzdHJ1
Y3QgaXByb2NfcGNpZSAqcGNpZSA9IGlwcm9jX2RhdGEoYnVzKTsKPiArCXVuc2lnbmVkIGludCBz
bG90ID0gUENJX1NMT1QoZGV2Zm4pOwo+ICsJdW5zaWduZWQgaW50IGZuID0gUENJX0ZVTkMoZGV2
Zm4pOwo+ICsJdW5zaWduZWQgaW50IGJ1c25vID0gYnVzLT5udW1iZXI7Cj4gKwl2b2lkIF9faW9t
ZW0gKmNmZ19kYXRhX3A7Cj4gKwl1bnNpZ25lZCBpbnQgZGF0YTsKPiArCj4gKwkvKiByb290IGNv
bXBsZXggYWNjZXNzLiAqLwo+ICsJaWYgKGJ1c25vID09IDApCj4gKwkJcmV0dXJuIHBjaV9nZW5l
cmljX2NvbmZpZ19yZWFkMzIoYnVzLCBkZXZmbiwgd2hlcmUsIHNpemUsIHZhbCk7CgpJdCBzb3Vu
ZHMgbGlrZSBTdGluZ3JheSBhZHZlcnRpc2VzIENSUyBTViBzdXBwb3J0IGluIGl0cyBSb290IENh
cGFiaWxpdGllcwpyZWdpc3Rlci4gIEkgdGhpbmsgd2Ugc2hvdWxkIG1hc2sgb3V0IFBDSV9FWFBf
UlRDQVBfQ1JTVklTIHNvIHdlIGRvbid0CmFkdmVydGlzZSBpdC4gIFRoYXQgd2lsbCBrZWVwIExp
bnV4IGZyb20gdHJ5aW5nIHRvIGVuYWJsZSBpdC4gIEkga25vdyB0aGUKaGFyZHdhcmUgZG9lc24n
dCBsb29rIGF0IFBDSV9FWFBfUlRDVExfQ1JTU1ZFLCBidXQgdGhlcmUncyBubyBwb2ludCBpbgpj
b25mdXNpbmcgdXNlcnMgcmVhZGluZyB0aGUgbHNwY2kgb3V0cHV0LgoKV2UgZGlkIHNvbWV0aGlu
ZyBzaW1pbGFyIHdpdGggZjA5Zjg3MzVmYjljICgiUENJOiB4Z2VuZTogRGlzYWJsZQpDb25maWd1
cmF0aW9uIFJlcXVlc3QgUmV0cnkgU3RhdHVzIGZvciB2MSBzaWxpY29uIikuCgpJIHRyaWVkIHRv
IGRvIHRoaXMgaW4gdGhlIHBhdGNoIEkgcHVzaGVkIHRvIHBjaS9ob3N0LWlwcm9jLgoKPiArCj4g
KwljZmdfZGF0YV9wID0gaXByb2NfcGNpZV9tYXBfZXBfY2ZnX3JlZyhwY2llLCBidXNubywgc2xv
dCwgZm4sIHdoZXJlKTsKPiArCj4gKwlpZiAoIWNmZ19kYXRhX3ApCj4gKwkJcmV0dXJuIFBDSUJJ
T1NfREVWSUNFX05PVF9GT1VORDsKPiArCj4gKwlkYXRhID0gaXByb2NfcGNpZV9jZmdfcmV0cnko
Y2ZnX2RhdGFfcCk7Cj4gKwo+ICsJKnZhbCA9IGRhdGE7Cj4gKwlpZiAoc2l6ZSA8PSAyKQo+ICsJ
CSp2YWwgPSAoZGF0YSA+PiAoOCAqICh3aGVyZSAmIDMpKSkgJiAoKDEgPDwgKHNpemUgKiA4KSkg
LSAxKTsKPiArCj4gKwlyZXR1cm4gUENJQklPU19TVUNDRVNTRlVMOwo+ICt9Cj4gKwo+ICAvKioK
PiAgICogTm90ZSBhY2Nlc3MgdG8gdGhlIGNvbmZpZ3VyYXRpb24gcmVnaXN0ZXJzIGFyZSBwcm90
ZWN0ZWQgYXQgdGhlIGhpZ2hlciBsYXllcgo+ICAgKiBieSAncGNpX2xvY2snIGluIGRyaXZlcnMv
cGNpL2FjY2Vzcy5jCj4gQEAgLTU2Nyw4ICs2MjgsMTMgQEAgc3RhdGljIGludCBpcHJvY19wY2ll
X2NvbmZpZ19yZWFkMzIoc3RydWN0IHBjaV9idXMgKmJ1cywgdW5zaWduZWQgaW50IGRldmZuLAo+
ICAJCQkJICAgIGludCB3aGVyZSwgaW50IHNpemUsIHUzMiAqdmFsKQo+ICB7Cj4gIAlpbnQgcmV0
Owo+ICsJc3RydWN0IGlwcm9jX3BjaWUgKnBjaWUgPSBpcHJvY19kYXRhKGJ1cyk7Cj4gIAo+ICAJ
aXByb2NfcGNpZV9hcGJfZXJyX2Rpc2FibGUoYnVzLCB0cnVlKTsKPiArCWlmIChwY2llLT50eXBl
ID09IElQUk9DX1BDSUVfUEFYQl9WMikKPiArCQlyZXQgPSBpcHJvY19wY2llX2NvbmZpZ19yZWFk
KGJ1cywgZGV2Zm4sIHdoZXJlLCBzaXplLCB2YWwpOwo+ICsJZWxzZQo+ICsJCXJldCA9IHBjaV9n
ZW5lcmljX2NvbmZpZ19yZWFkMzIoYnVzLCBkZXZmbiwgd2hlcmUsIHNpemUsIHZhbCk7Cj4gIAly
ZXQgPSBwY2lfZ2VuZXJpY19jb25maWdfcmVhZDMyKGJ1cywgZGV2Zm4sIHdoZXJlLCBzaXplLCB2
YWwpOwoKVGhpcyBsYXN0IHBjaV9nZW5lcmljX2NvbmZpZ19yZWFkMzIoKSBjYWxsIGxvb2tzIGxp
a2UgYSBkdXBsaWNhdGUuCgo+ICAJaXByb2NfcGNpZV9hcGJfZXJyX2Rpc2FibGUoYnVzLCBmYWxz
ZSk7Cj4gIAo+IC0tIAo+IDEuOS4xCj4gCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0t
a2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFp
bG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> PCIe spec r3.1, sec 2.3.2
> If CRS software visibility is not enabled, the RC must reissue the
> config request as a new request.
> 
> - If CRS software visibility is enabled,
> - for a config read of Vendor ID, the RC must return 0x0001 data
> - for all other config reads/writes, the RC must reissue the
>   request
> 
> iproc PCIe Controller spec:
> 4.7.3.3. Retry Status On Configuration Cycle
> Endpoints are allowed to generate retry status on configuration
> cycles. In this case, the RC needs to re-issue the request. The IP
> does not handle this because the number of configuration cycles needed
> will probably be less than the total number of non-posted operations
> needed.
> 
> When a retry status is received on the User RX interface for a
> configuration request that was sent on the User TX interface,
> it will be indicated with a completion with the CMPL_STATUS field set
> to 2=CRS, and the user will have to find the address and data values
> and send a new transaction on the User TX interface.
> When the internal configuration space returns a retry status during a
> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> CRS status.
> When the CRS Software Visibility Enable register in the Root Control
> register is enabled, the IP will return the data value to 0x0001 for
> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
> the request for reads of offset 0 that return with CRS status.  This
> is true for both the User RX Interface and for the Command/Status
> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> field of the completion on the User RX Interface will not be 2=CRS and
> the pcie_cscrs signal will not assert on the Command/Status interface.
> 
> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> with Configuration Request Retry Status (CRS) should be reissued by
> the hardware except reads of the Vendor ID when CRS Software
> Visibility is enabled.
> 
> This hardware never reissues configuration requests when it receives
> CRS completions.
> Note that, neither PCIe host bridge nor PCIe core re-issues the
> request for any configuration offset.
> 
> For config reads, this hardware returns CFG_RETRY_STATUS data when
> it receives a CRS completion for a config read, regardless of the
> address of the read or the CRS Software Visibility Enable bit.
> 
> This patch implements iproc_pcie_config_read which gets called for
> Stingray, if it receives a CRS completion, it retries reading it again.
> In case of timeout, it returns 0xffffffff.
> For other iproc based SOC, it falls back to PCI generic APIs.
> 
> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 61d9be6..37f4adf 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -68,6 +68,9 @@
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> +#define CFG_RETRY_STATUS             0xffff0001
> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> +
>  /* derive the enum index of the outbound/inbound mapping registers */
>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>  
> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>  	return (pcie->base + offset);
>  }
>  
> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> +{
> +	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> +	unsigned int data;
> +
> +	/*
> +	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software
> +	 * Visibility only affects config read of the Vendor ID.
> +	 * For config write or any other config read the Root must
> +	 * automatically re-issue configuration request again as a
> +	 * new request.
> +	 *
> +	 * For config reads, this hardware returns CFG_RETRY_STATUS data when
> +	 * it receives a CRS completion for a config read, regardless of the
> +	 * address of the read or the CRS Software Visibility Enable bit. As a
> +	 * partial workaround for this, we retry in software any read that
> +	 * returns CFG_RETRY_STATUS.
> +	 */
> +	data = readl(cfg_data_p);
> +	while (data == CFG_RETRY_STATUS && timeout--) {
> +		udelay(1);
> +		data = readl(cfg_data_p);
> +	}
> +
> +	if (data == CFG_RETRY_STATUS)
> +		data = 0xffffffff;
> +
> +	return data;
> +}
> +
> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	unsigned int slot = PCI_SLOT(devfn);
> +	unsigned int fn = PCI_FUNC(devfn);
> +	unsigned int busno = bus->number;
> +	void __iomem *cfg_data_p;
> +	unsigned int data;
> +
> +	/* root complex access. */
> +	if (busno == 0)
> +		return pci_generic_config_read32(bus, devfn, where, size, val);

It sounds like Stingray advertises CRS SV support in its Root Capabilities
register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
advertise it.  That will keep Linux from trying to enable it.  I know the
hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
confusing users reading the lspci output.

We did something similar with f09f8735fb9c ("PCI: xgene: Disable
Configuration Request Retry Status for v1 silicon").

I tried to do this in the patch I pushed to pci/host-iproc.

> +
> +	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> +
> +	if (!cfg_data_p)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	data = iproc_pcie_cfg_retry(cfg_data_p);
> +
> +	*val = data;
> +	if (size <= 2)
> +		*val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  /**
>   * Note access to the configuration registers are protected at the higher layer
>   * by 'pci_lock' in drivers/pci/access.c
> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>  				    int where, int size, u32 *val)
>  {
>  	int ret;
> +	struct iproc_pcie *pcie = iproc_data(bus);
>  
>  	iproc_pcie_apb_err_disable(bus, true);
> +	if (pcie->type == IPROC_PCIE_PAXB_V2)
> +		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
> +	else
> +		ret = pci_generic_config_read32(bus, devfn, where, size, val);
>  	ret = pci_generic_config_read32(bus, devfn, where, size, val);

This last pci_generic_config_read32() call looks like a duplicate.

>  	iproc_pcie_apb_err_disable(bus, false);
>  
> -- 
> 1.9.1
> 

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

[Bjorn Helgaas]

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

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

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

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

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

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

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

[Bjorn Helgaas]

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

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

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

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

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

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

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

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

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

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

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

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

[Oza Oza]

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

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

so things look okay to me.

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

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

[Oza Oza]

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

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

so things look okay to me.

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

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

[Oza Oza]

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

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

so things look okay to me.

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

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> PCIe spec r3.1, sec 2.3.2
>> If CRS software visibility is not enabled, the RC must reissue the
>> config request as a new request.
>>
>> - If CRS software visibility is enabled,
>> - for a config read of Vendor ID, the RC must return 0x0001 data
>> - for all other config reads/writes, the RC must reissue the
>>   request
>>
>> iproc PCIe Controller spec:
>> 4.7.3.3. Retry Status On Configuration Cycle
>> Endpoints are allowed to generate retry status on configuration
>> cycles. In this case, the RC needs to re-issue the request. The IP
>> does not handle this because the number of configuration cycles needed
>> will probably be less than the total number of non-posted operations
>> needed.
>>
>> When a retry status is received on the User RX interface for a
>> configuration request that was sent on the User TX interface,
>> it will be indicated with a completion with the CMPL_STATUS field set
>> to 2=CRS, and the user will have to find the address and data values
>> and send a new transaction on the User TX interface.
>> When the internal configuration space returns a retry status during a
>> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> CRS status.
>> When the CRS Software Visibility Enable register in the Root Control
>> register is enabled, the IP will return the data value to 0x0001 for
>> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
>> the request for reads of offset 0 that return with CRS status.  This
>> is true for both the User RX Interface and for the Command/Status
>> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> field of the completion on the User RX Interface will not be 2=CRS and
>> the pcie_cscrs signal will not assert on the Command/Status interface.
>>
>> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> with Configuration Request Retry Status (CRS) should be reissued by
>> the hardware except reads of the Vendor ID when CRS Software
>> Visibility is enabled.
>>
>> This hardware never reissues configuration requests when it receives
>> CRS completions.
>> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> request for any configuration offset.
>>
>> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> it receives a CRS completion for a config read, regardless of the
>> address of the read or the CRS Software Visibility Enable bit.
>>
>> This patch implements iproc_pcie_config_read which gets called for
>> Stingray, if it receives a CRS completion, it retries reading it again.
>> In case of timeout, it returns 0xffffffff.
>> For other iproc based SOC, it falls back to PCI generic APIs.
>>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 61d9be6..37f4adf 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -68,6 +68,9 @@
>>  #define APB_ERR_EN_SHIFT             0
>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>
>> +#define CFG_RETRY_STATUS             0xffff0001
>> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> +
>>  /* derive the enum index of the outbound/inbound mapping registers */
>>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>>
>> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>>       return (pcie->base + offset);
>>  }
>>
>> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> +{
>> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
>> +     unsigned int data;
>> +
>> +     /*
>> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
>> +      * Visibility only affects config read of the Vendor ID.
>> +      * For config write or any other config read the Root must
>> +      * automatically re-issue configuration request again as a
>> +      * new request.
>> +      *
>> +      * For config reads, this hardware returns CFG_RETRY_STATUS data when
>> +      * it receives a CRS completion for a config read, regardless of the
>> +      * address of the read or the CRS Software Visibility Enable bit. As a
>> +      * partial workaround for this, we retry in software any read that
>> +      * returns CFG_RETRY_STATUS.
>> +      */
>> +     data = readl(cfg_data_p);
>> +     while (data == CFG_RETRY_STATUS && timeout--) {
>> +             udelay(1);
>> +             data = readl(cfg_data_p);
>> +     }
>> +
>> +     if (data == CFG_RETRY_STATUS)
>> +             data = 0xffffffff;
>> +
>> +     return data;
>> +}
>> +
>> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>> +                                 int where, int size, u32 *val)
>> +{
>> +     struct iproc_pcie *pcie = iproc_data(bus);
>> +     unsigned int slot = PCI_SLOT(devfn);
>> +     unsigned int fn = PCI_FUNC(devfn);
>> +     unsigned int busno = bus->number;
>> +     void __iomem *cfg_data_p;
>> +     unsigned int data;
>> +
>> +     /* root complex access. */
>> +     if (busno == 0)
>> +             return pci_generic_config_read32(bus, devfn, where, size, val);
>
> It sounds like Stingray advertises CRS SV support in its Root Capabilities
> register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
> advertise it.  That will keep Linux from trying to enable it.  I know the
> hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
> confusing users reading the lspci output.
>
> We did something similar with f09f8735fb9c ("PCI: xgene: Disable
> Configuration Request Retry Status for v1 silicon").
>
> I tried to do this in the patch I pushed to pci/host-iproc.
>
>> +
>> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>> +
>> +     if (!cfg_data_p)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +     data = iproc_pcie_cfg_retry(cfg_data_p);
>> +
>> +     *val = data;
>> +     if (size <= 2)
>> +             *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  /**
>>   * Note access to the configuration registers are protected at the higher layer
>>   * by 'pci_lock' in drivers/pci/access.c
>> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>>                                   int where, int size, u32 *val)
>>  {
>>       int ret;
>> +     struct iproc_pcie *pcie = iproc_data(bus);
>>
>>       iproc_pcie_apb_err_disable(bus, true);
>> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
>> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>> +     else
>> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>       ret = pci_generic_config_read32(bus, devfn, where, size, val);
>
> This last pci_generic_config_read32() call looks like a duplicate.

yes indeed.
I have tested your CRS visibility bit changes; and it works fine.

do you want me to post new patch-set by removing the duplicate call
along with the changes you have made ?

or since, you have already applied patches, with your changes, you
will take care of removing this last duplicate call ?
I think this is the last change for this patch-set, If I did not miss anything.

please let me know.

>
>>       iproc_pcie_apb_err_disable(bus, false);
>>
>> --
>> 1.9.1
>>

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> PCIe spec r3.1, sec 2.3.2
>> If CRS software visibility is not enabled, the RC must reissue the
>> config request as a new request.
>>
>> - If CRS software visibility is enabled,
>> - for a config read of Vendor ID, the RC must return 0x0001 data
>> - for all other config reads/writes, the RC must reissue the
>>   request
>>
>> iproc PCIe Controller spec:
>> 4.7.3.3. Retry Status On Configuration Cycle
>> Endpoints are allowed to generate retry status on configuration
>> cycles. In this case, the RC needs to re-issue the request. The IP
>> does not handle this because the number of configuration cycles needed
>> will probably be less than the total number of non-posted operations
>> needed.
>>
>> When a retry status is received on the User RX interface for a
>> configuration request that was sent on the User TX interface,
>> it will be indicated with a completion with the CMPL_STATUS field set
>> to 2=3DCRS, and the user will have to find the address and data values
>> and send a new transaction on the User TX interface.
>> When the internal configuration space returns a retry status during a
>> configuration cycle (user_cscfg =3D 1) on the Command/Status interface,
>> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> CRS status.
>> When the CRS Software Visibility Enable register in the Root Control
>> register is enabled, the IP will return the data value to 0x0001 for
>> the Vendor ID value and 0xffff  (all 1=E2=80=99s) for the rest of the da=
ta in
>> the request for reads of offset 0 that return with CRS status.  This
>> is true for both the User RX Interface and for the Command/Status
>> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> field of the completion on the User RX Interface will not be 2=3DCRS and
>> the pcie_cscrs signal will not assert on the Command/Status interface.
>>
>> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> with Configuration Request Retry Status (CRS) should be reissued by
>> the hardware except reads of the Vendor ID when CRS Software
>> Visibility is enabled.
>>
>> This hardware never reissues configuration requests when it receives
>> CRS completions.
>> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> request for any configuration offset.
>>
>> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> it receives a CRS completion for a config read, regardless of the
>> address of the read or the CRS Software Visibility Enable bit.
>>
>> This patch implements iproc_pcie_config_read which gets called for
>> Stingray, if it receives a CRS completion, it retries reading it again.
>> In case of timeout, it returns 0xffffffff.
>> For other iproc based SOC, it falls back to PCI generic APIs.
>>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc=
.c
>> index 61d9be6..37f4adf 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -68,6 +68,9 @@
>>  #define APB_ERR_EN_SHIFT             0
>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>
>> +#define CFG_RETRY_STATUS             0xffff0001
>> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> +
>>  /* derive the enum index of the outbound/inbound mapping registers */
>>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>>
>> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(stru=
ct iproc_pcie *pcie,
>>       return (pcie->base + offset);
>>  }
>>
>> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> +{
>> +     int timeout =3D CFG_RETRY_STATUS_TIMEOUT_US;
>> +     unsigned int data;
>> +
>> +     /*
>> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
>> +      * Visibility only affects config read of the Vendor ID.
>> +      * For config write or any other config read the Root must
>> +      * automatically re-issue configuration request again as a
>> +      * new request.
>> +      *
>> +      * For config reads, this hardware returns CFG_RETRY_STATUS data w=
hen
>> +      * it receives a CRS completion for a config read, regardless of t=
he
>> +      * address of the read or the CRS Software Visibility Enable bit. =
As a
>> +      * partial workaround for this, we retry in software any read that
>> +      * returns CFG_RETRY_STATUS.
>> +      */
>> +     data =3D readl(cfg_data_p);
>> +     while (data =3D=3D CFG_RETRY_STATUS && timeout--) {
>> +             udelay(1);
>> +             data =3D readl(cfg_data_p);
>> +     }
>> +
>> +     if (data =3D=3D CFG_RETRY_STATUS)
>> +             data =3D 0xffffffff;
>> +
>> +     return data;
>> +}
>> +
>> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int dev=
fn,
>> +                                 int where, int size, u32 *val)
>> +{
>> +     struct iproc_pcie *pcie =3D iproc_data(bus);
>> +     unsigned int slot =3D PCI_SLOT(devfn);
>> +     unsigned int fn =3D PCI_FUNC(devfn);
>> +     unsigned int busno =3D bus->number;
>> +     void __iomem *cfg_data_p;
>> +     unsigned int data;
>> +
>> +     /* root complex access. */
>> +     if (busno =3D=3D 0)
>> +             return pci_generic_config_read32(bus, devfn, where, size, =
val);
>
> It sounds like Stingray advertises CRS SV support in its Root Capabilitie=
s
> register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
> advertise it.  That will keep Linux from trying to enable it.  I know the
> hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
> confusing users reading the lspci output.
>
> We did something similar with f09f8735fb9c ("PCI: xgene: Disable
> Configuration Request Retry Status for v1 silicon").
>
> I tried to do this in the patch I pushed to pci/host-iproc.
>
>> +
>> +     cfg_data_p =3D iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, wh=
ere);
>> +
>> +     if (!cfg_data_p)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +     data =3D iproc_pcie_cfg_retry(cfg_data_p);
>> +
>> +     *val =3D data;
>> +     if (size <=3D 2)
>> +             *val =3D (data >> (8 * (where & 3))) & ((1 << (size * 8)) =
- 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  /**
>>   * Note access to the configuration registers are protected at the high=
er layer
>>   * by 'pci_lock' in drivers/pci/access.c
>> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus =
*bus, unsigned int devfn,
>>                                   int where, int size, u32 *val)
>>  {
>>       int ret;
>> +     struct iproc_pcie *pcie =3D iproc_data(bus);
>>
>>       iproc_pcie_apb_err_disable(bus, true);
>> +     if (pcie->type =3D=3D IPROC_PCIE_PAXB_V2)
>> +             ret =3D iproc_pcie_config_read(bus, devfn, where, size, va=
l);
>> +     else
>> +             ret =3D pci_generic_config_read32(bus, devfn, where, size,=
 val);
>>       ret =3D pci_generic_config_read32(bus, devfn, where, size, val);
>
> This last pci_generic_config_read32() call looks like a duplicate.

yes indeed.
I have tested your CRS visibility bit changes; and it works fine.

do you want me to post new patch-set by removing the duplicate call
along with the changes you have made ?

or since, you have already applied patches, with your changes, you
will take care of removing this last duplicate call ?
I think this is the last change for this patch-set, If I did not miss anyth=
ing.

please let me know.

>
>>       iproc_pcie_apb_err_disable(bus, false);
>>
>> --
>> 1.9.1
>>

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> PCIe spec r3.1, sec 2.3.2
>> If CRS software visibility is not enabled, the RC must reissue the
>> config request as a new request.
>>
>> - If CRS software visibility is enabled,
>> - for a config read of Vendor ID, the RC must return 0x0001 data
>> - for all other config reads/writes, the RC must reissue the
>>   request
>>
>> iproc PCIe Controller spec:
>> 4.7.3.3. Retry Status On Configuration Cycle
>> Endpoints are allowed to generate retry status on configuration
>> cycles. In this case, the RC needs to re-issue the request. The IP
>> does not handle this because the number of configuration cycles needed
>> will probably be less than the total number of non-posted operations
>> needed.
>>
>> When a retry status is received on the User RX interface for a
>> configuration request that was sent on the User TX interface,
>> it will be indicated with a completion with the CMPL_STATUS field set
>> to 2=CRS, and the user will have to find the address and data values
>> and send a new transaction on the User TX interface.
>> When the internal configuration space returns a retry status during a
>> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> CRS status.
>> When the CRS Software Visibility Enable register in the Root Control
>> register is enabled, the IP will return the data value to 0x0001 for
>> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
>> the request for reads of offset 0 that return with CRS status.  This
>> is true for both the User RX Interface and for the Command/Status
>> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> field of the completion on the User RX Interface will not be 2=CRS and
>> the pcie_cscrs signal will not assert on the Command/Status interface.
>>
>> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> with Configuration Request Retry Status (CRS) should be reissued by
>> the hardware except reads of the Vendor ID when CRS Software
>> Visibility is enabled.
>>
>> This hardware never reissues configuration requests when it receives
>> CRS completions.
>> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> request for any configuration offset.
>>
>> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> it receives a CRS completion for a config read, regardless of the
>> address of the read or the CRS Software Visibility Enable bit.
>>
>> This patch implements iproc_pcie_config_read which gets called for
>> Stingray, if it receives a CRS completion, it retries reading it again.
>> In case of timeout, it returns 0xffffffff.
>> For other iproc based SOC, it falls back to PCI generic APIs.
>>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 61d9be6..37f4adf 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -68,6 +68,9 @@
>>  #define APB_ERR_EN_SHIFT             0
>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>
>> +#define CFG_RETRY_STATUS             0xffff0001
>> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> +
>>  /* derive the enum index of the outbound/inbound mapping registers */
>>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>>
>> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>>       return (pcie->base + offset);
>>  }
>>
>> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> +{
>> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
>> +     unsigned int data;
>> +
>> +     /*
>> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
>> +      * Visibility only affects config read of the Vendor ID.
>> +      * For config write or any other config read the Root must
>> +      * automatically re-issue configuration request again as a
>> +      * new request.
>> +      *
>> +      * For config reads, this hardware returns CFG_RETRY_STATUS data when
>> +      * it receives a CRS completion for a config read, regardless of the
>> +      * address of the read or the CRS Software Visibility Enable bit. As a
>> +      * partial workaround for this, we retry in software any read that
>> +      * returns CFG_RETRY_STATUS.
>> +      */
>> +     data = readl(cfg_data_p);
>> +     while (data == CFG_RETRY_STATUS && timeout--) {
>> +             udelay(1);
>> +             data = readl(cfg_data_p);
>> +     }
>> +
>> +     if (data == CFG_RETRY_STATUS)
>> +             data = 0xffffffff;
>> +
>> +     return data;
>> +}
>> +
>> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>> +                                 int where, int size, u32 *val)
>> +{
>> +     struct iproc_pcie *pcie = iproc_data(bus);
>> +     unsigned int slot = PCI_SLOT(devfn);
>> +     unsigned int fn = PCI_FUNC(devfn);
>> +     unsigned int busno = bus->number;
>> +     void __iomem *cfg_data_p;
>> +     unsigned int data;
>> +
>> +     /* root complex access. */
>> +     if (busno == 0)
>> +             return pci_generic_config_read32(bus, devfn, where, size, val);
>
> It sounds like Stingray advertises CRS SV support in its Root Capabilities
> register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
> advertise it.  That will keep Linux from trying to enable it.  I know the
> hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
> confusing users reading the lspci output.
>
> We did something similar with f09f8735fb9c ("PCI: xgene: Disable
> Configuration Request Retry Status for v1 silicon").
>
> I tried to do this in the patch I pushed to pci/host-iproc.
>
>> +
>> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>> +
>> +     if (!cfg_data_p)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +     data = iproc_pcie_cfg_retry(cfg_data_p);
>> +
>> +     *val = data;
>> +     if (size <= 2)
>> +             *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  /**
>>   * Note access to the configuration registers are protected at the higher layer
>>   * by 'pci_lock' in drivers/pci/access.c
>> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>>                                   int where, int size, u32 *val)
>>  {
>>       int ret;
>> +     struct iproc_pcie *pcie = iproc_data(bus);
>>
>>       iproc_pcie_apb_err_disable(bus, true);
>> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
>> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>> +     else
>> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>       ret = pci_generic_config_read32(bus, devfn, where, size, val);
>
> This last pci_generic_config_read32() call looks like a duplicate.

yes indeed.
I have tested your CRS visibility bit changes; and it works fine.

do you want me to post new patch-set by removing the duplicate call
along with the changes you have made ?

or since, you have already applied patches, with your changes, you
will take care of removing this last duplicate call ?
I think this is the last change for this patch-set, If I did not miss anything.

please let me know.

>
>>       iproc_pcie_apb_err_disable(bus, false);
>>
>> --
>> 1.9.1
>>

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Tue, Aug 29, 2017 at 11:02:23AM +0530, Oza Oza wrote:
> On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> >> PCIe spec r3.1, sec 2.3.2
> >> If CRS software visibility is not enabled, the RC must reissue the
> >> config request as a new request.
> >>
> >> - If CRS software visibility is enabled,
> >> - for a config read of Vendor ID, the RC must return 0x0001 data
> >> - for all other config reads/writes, the RC must reissue the
> >>   request
> >>
> >> iproc PCIe Controller spec:
> >> 4.7.3.3. Retry Status On Configuration Cycle
> >> Endpoints are allowed to generate retry status on configuration
> >> cycles. In this case, the RC needs to re-issue the request. The IP
> >> does not handle this because the number of configuration cycles needed
> >> will probably be less than the total number of non-posted operations
> >> needed.
> >>
> >> When a retry status is received on the User RX interface for a
> >> configuration request that was sent on the User TX interface,
> >> it will be indicated with a completion with the CMPL_STATUS field set
> >> to 2=CRS, and the user will have to find the address and data values
> >> and send a new transaction on the User TX interface.
> >> When the internal configuration space returns a retry status during a
> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> >> CRS status.
> >> When the CRS Software Visibility Enable register in the Root Control
> >> register is enabled, the IP will return the data value to 0x0001 for
> >> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
> >> the request for reads of offset 0 that return with CRS status.  This
> >> is true for both the User RX Interface and for the Command/Status
> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> >> field of the completion on the User RX Interface will not be 2=CRS and
> >> the pcie_cscrs signal will not assert on the Command/Status interface.
> >>
> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> >> with Configuration Request Retry Status (CRS) should be reissued by
> >> the hardware except reads of the Vendor ID when CRS Software
> >> Visibility is enabled.
> >>
> >> This hardware never reissues configuration requests when it receives
> >> CRS completions.
> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
> >> request for any configuration offset.
> >>
> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
> >> it receives a CRS completion for a config read, regardless of the
> >> address of the read or the CRS Software Visibility Enable bit.
> >>
> >> This patch implements iproc_pcie_config_read which gets called for
> >> Stingray, if it receives a CRS completion, it retries reading it again.
> >> In case of timeout, it returns 0xffffffff.
> >> For other iproc based SOC, it falls back to PCI generic APIs.
> >>
> >> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> >>
> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> >> index 61d9be6..37f4adf 100644
> >> --- a/drivers/pci/host/pcie-iproc.c
> >> +++ b/drivers/pci/host/pcie-iproc.c
> >> @@ -68,6 +68,9 @@
> >>  #define APB_ERR_EN_SHIFT             0
> >>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
> >>
> >> +#define CFG_RETRY_STATUS             0xffff0001
> >> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> >> +
> >>  /* derive the enum index of the outbound/inbound mapping registers */
> >>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
> >>
> >> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
> >>       return (pcie->base + offset);
> >>  }
> >>
> >> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> >> +{
> >> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> >> +     unsigned int data;
> >> +
> >> +     /*
> >> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
> >> +      * Visibility only affects config read of the Vendor ID.
> >> +      * For config write or any other config read the Root must
> >> +      * automatically re-issue configuration request again as a
> >> +      * new request.
> >> +      *
> >> +      * For config reads, this hardware returns CFG_RETRY_STATUS data when
> >> +      * it receives a CRS completion for a config read, regardless of the
> >> +      * address of the read or the CRS Software Visibility Enable bit. As a
> >> +      * partial workaround for this, we retry in software any read that
> >> +      * returns CFG_RETRY_STATUS.
> >> +      */
> >> +     data = readl(cfg_data_p);
> >> +     while (data == CFG_RETRY_STATUS && timeout--) {
> >> +             udelay(1);
> >> +             data = readl(cfg_data_p);
> >> +     }
> >> +
> >> +     if (data == CFG_RETRY_STATUS)
> >> +             data = 0xffffffff;
> >> +
> >> +     return data;
> >> +}
> >> +
> >> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> >> +                                 int where, int size, u32 *val)
> >> +{
> >> +     struct iproc_pcie *pcie = iproc_data(bus);
> >> +     unsigned int slot = PCI_SLOT(devfn);
> >> +     unsigned int fn = PCI_FUNC(devfn);
> >> +     unsigned int busno = bus->number;
> >> +     void __iomem *cfg_data_p;
> >> +     unsigned int data;
> >> +
> >> +     /* root complex access. */
> >> +     if (busno == 0)
> >> +             return pci_generic_config_read32(bus, devfn, where, size, val);
> >
> > It sounds like Stingray advertises CRS SV support in its Root Capabilities
> > register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
> > advertise it.  That will keep Linux from trying to enable it.  I know the
> > hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
> > confusing users reading the lspci output.
> >
> > We did something similar with f09f8735fb9c ("PCI: xgene: Disable
> > Configuration Request Retry Status for v1 silicon").
> >
> > I tried to do this in the patch I pushed to pci/host-iproc.
> >
> >> +
> >> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> >> +
> >> +     if (!cfg_data_p)
> >> +             return PCIBIOS_DEVICE_NOT_FOUND;
> >> +
> >> +     data = iproc_pcie_cfg_retry(cfg_data_p);
> >> +
> >> +     *val = data;
> >> +     if (size <= 2)
> >> +             *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> >> +
> >> +     return PCIBIOS_SUCCESSFUL;
> >> +}
> >> +
> >>  /**
> >>   * Note access to the configuration registers are protected at the higher layer
> >>   * by 'pci_lock' in drivers/pci/access.c
> >> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> >>                                   int where, int size, u32 *val)
> >>  {
> >>       int ret;
> >> +     struct iproc_pcie *pcie = iproc_data(bus);
> >>
> >>       iproc_pcie_apb_err_disable(bus, true);
> >> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
> >> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
> >> +     else
> >> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
> >>       ret = pci_generic_config_read32(bus, devfn, where, size, val);
> >
> > This last pci_generic_config_read32() call looks like a duplicate.
> 
> yes indeed.
> I have tested your CRS visibility bit changes; and it works fine.
> 
> do you want me to post new patch-set by removing the duplicate call
> along with the changes you have made ?
> 
> or since, you have already applied patches, with your changes, you
> will take care of removing this last duplicate call ?
> I think this is the last change for this patch-set, If I did not miss anything.
> 
> please let me know.

I already removed that duplicate call.  It should be in the next -next.
Let me know if there's anything wrong with it.

Bjorn

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

T24gVHVlLCBBdWcgMjksIDIwMTcgYXQgMTE6MDI6MjNBTSArMDUzMCwgT3phIE96YSB3cm90ZToK
PiBPbiBUdWUsIEF1ZyAyOSwgMjAxNyBhdCAzOjE3IEFNLCBCam9ybiBIZWxnYWFzIDxoZWxnYWFz
QGtlcm5lbC5vcmc+IHdyb3RlOgo+ID4gT24gVGh1LCBBdWcgMjQsIDIwMTcgYXQgMTA6MzQ6MjVB
TSArMDUzMCwgT3phIFBhd2FuZGVlcCB3cm90ZToKPiA+PiBQQ0llIHNwZWMgcjMuMSwgc2VjIDIu
My4yCj4gPj4gSWYgQ1JTIHNvZnR3YXJlIHZpc2liaWxpdHkgaXMgbm90IGVuYWJsZWQsIHRoZSBS
QyBtdXN0IHJlaXNzdWUgdGhlCj4gPj4gY29uZmlnIHJlcXVlc3QgYXMgYSBuZXcgcmVxdWVzdC4K
PiA+Pgo+ID4+IC0gSWYgQ1JTIHNvZnR3YXJlIHZpc2liaWxpdHkgaXMgZW5hYmxlZCwKPiA+PiAt
IGZvciBhIGNvbmZpZyByZWFkIG9mIFZlbmRvciBJRCwgdGhlIFJDIG11c3QgcmV0dXJuIDB4MDAw
MSBkYXRhCj4gPj4gLSBmb3IgYWxsIG90aGVyIGNvbmZpZyByZWFkcy93cml0ZXMsIHRoZSBSQyBt
dXN0IHJlaXNzdWUgdGhlCj4gPj4gICByZXF1ZXN0Cj4gPj4KPiA+PiBpcHJvYyBQQ0llIENvbnRy
b2xsZXIgc3BlYzoKPiA+PiA0LjcuMy4zLiBSZXRyeSBTdGF0dXMgT24gQ29uZmlndXJhdGlvbiBD
eWNsZQo+ID4+IEVuZHBvaW50cyBhcmUgYWxsb3dlZCB0byBnZW5lcmF0ZSByZXRyeSBzdGF0dXMg
b24gY29uZmlndXJhdGlvbgo+ID4+IGN5Y2xlcy4gSW4gdGhpcyBjYXNlLCB0aGUgUkMgbmVlZHMg
dG8gcmUtaXNzdWUgdGhlIHJlcXVlc3QuIFRoZSBJUAo+ID4+IGRvZXMgbm90IGhhbmRsZSB0aGlz
IGJlY2F1c2UgdGhlIG51bWJlciBvZiBjb25maWd1cmF0aW9uIGN5Y2xlcyBuZWVkZWQKPiA+PiB3
aWxsIHByb2JhYmx5IGJlIGxlc3MgdGhhbiB0aGUgdG90YWwgbnVtYmVyIG9mIG5vbi1wb3N0ZWQg
b3BlcmF0aW9ucwo+ID4+IG5lZWRlZC4KPiA+Pgo+ID4+IFdoZW4gYSByZXRyeSBzdGF0dXMgaXMg
cmVjZWl2ZWQgb24gdGhlIFVzZXIgUlggaW50ZXJmYWNlIGZvciBhCj4gPj4gY29uZmlndXJhdGlv
biByZXF1ZXN0IHRoYXQgd2FzIHNlbnQgb24gdGhlIFVzZXIgVFggaW50ZXJmYWNlLAo+ID4+IGl0
IHdpbGwgYmUgaW5kaWNhdGVkIHdpdGggYSBjb21wbGV0aW9uIHdpdGggdGhlIENNUExfU1RBVFVT
IGZpZWxkIHNldAo+ID4+IHRvIDI9Q1JTLCBhbmQgdGhlIHVzZXIgd2lsbCBoYXZlIHRvIGZpbmQg
dGhlIGFkZHJlc3MgYW5kIGRhdGEgdmFsdWVzCj4gPj4gYW5kIHNlbmQgYSBuZXcgdHJhbnNhY3Rp
b24gb24gdGhlIFVzZXIgVFggaW50ZXJmYWNlLgo+ID4+IFdoZW4gdGhlIGludGVybmFsIGNvbmZp
Z3VyYXRpb24gc3BhY2UgcmV0dXJucyBhIHJldHJ5IHN0YXR1cyBkdXJpbmcgYQo+ID4+IGNvbmZp
Z3VyYXRpb24gY3ljbGUgKHVzZXJfY3NjZmcgPSAxKSBvbiB0aGUgQ29tbWFuZC9TdGF0dXMgaW50
ZXJmYWNlLAo+ID4+IHRoZSBwY2llX2NzY3JzIHdpbGwgYXNzZXJ0IHdpdGggdGhlIHBjaWVfY3Nh
Y2sgc2lnbmFsIHRvIGluZGljYXRlIHRoZQo+ID4+IENSUyBzdGF0dXMuCj4gPj4gV2hlbiB0aGUg
Q1JTIFNvZnR3YXJlIFZpc2liaWxpdHkgRW5hYmxlIHJlZ2lzdGVyIGluIHRoZSBSb290IENvbnRy
b2wKPiA+PiByZWdpc3RlciBpcyBlbmFibGVkLCB0aGUgSVAgd2lsbCByZXR1cm4gdGhlIGRhdGEg
dmFsdWUgdG8gMHgwMDAxIGZvcgo+ID4+IHRoZSBWZW5kb3IgSUQgdmFsdWUgYW5kIDB4ZmZmZiAg
KGFsbCAx4oCZcykgZm9yIHRoZSByZXN0IG9mIHRoZSBkYXRhIGluCj4gPj4gdGhlIHJlcXVlc3Qg
Zm9yIHJlYWRzIG9mIG9mZnNldCAwIHRoYXQgcmV0dXJuIHdpdGggQ1JTIHN0YXR1cy4gIFRoaXMK
PiA+PiBpcyB0cnVlIGZvciBib3RoIHRoZSBVc2VyIFJYIEludGVyZmFjZSBhbmQgZm9yIHRoZSBD
b21tYW5kL1N0YXR1cwo+ID4+IGludGVyZmFjZS4gIFdoZW4gQ1JTIFNvZnR3YXJlIFZpc2liaWxp
dHkgaXMgZW5hYmxlZCwgdGhlIENNUExfU1RBVFVTCj4gPj4gZmllbGQgb2YgdGhlIGNvbXBsZXRp
b24gb24gdGhlIFVzZXIgUlggSW50ZXJmYWNlIHdpbGwgbm90IGJlIDI9Q1JTIGFuZAo+ID4+IHRo
ZSBwY2llX2NzY3JzIHNpZ25hbCB3aWxsIG5vdCBhc3NlcnQgb24gdGhlIENvbW1hbmQvU3RhdHVz
IGludGVyZmFjZS4KPiA+Pgo+ID4+IFBlciBQQ0llIHIzLjEsIHNlYyAyLjMuMiwgY29uZmlnIHJl
cXVlc3RzIHRoYXQgcmVjZWl2ZSBjb21wbGV0aW9ucwo+ID4+IHdpdGggQ29uZmlndXJhdGlvbiBS
ZXF1ZXN0IFJldHJ5IFN0YXR1cyAoQ1JTKSBzaG91bGQgYmUgcmVpc3N1ZWQgYnkKPiA+PiB0aGUg
aGFyZHdhcmUgZXhjZXB0IHJlYWRzIG9mIHRoZSBWZW5kb3IgSUQgd2hlbiBDUlMgU29mdHdhcmUK
PiA+PiBWaXNpYmlsaXR5IGlzIGVuYWJsZWQuCj4gPj4KPiA+PiBUaGlzIGhhcmR3YXJlIG5ldmVy
IHJlaXNzdWVzIGNvbmZpZ3VyYXRpb24gcmVxdWVzdHMgd2hlbiBpdCByZWNlaXZlcwo+ID4+IENS
UyBjb21wbGV0aW9ucy4KPiA+PiBOb3RlIHRoYXQsIG5laXRoZXIgUENJZSBob3N0IGJyaWRnZSBu
b3IgUENJZSBjb3JlIHJlLWlzc3VlcyB0aGUKPiA+PiByZXF1ZXN0IGZvciBhbnkgY29uZmlndXJh
dGlvbiBvZmZzZXQuCj4gPj4KPiA+PiBGb3IgY29uZmlnIHJlYWRzLCB0aGlzIGhhcmR3YXJlIHJl
dHVybnMgQ0ZHX1JFVFJZX1NUQVRVUyBkYXRhIHdoZW4KPiA+PiBpdCByZWNlaXZlcyBhIENSUyBj
b21wbGV0aW9uIGZvciBhIGNvbmZpZyByZWFkLCByZWdhcmRsZXNzIG9mIHRoZQo+ID4+IGFkZHJl
c3Mgb2YgdGhlIHJlYWQgb3IgdGhlIENSUyBTb2Z0d2FyZSBWaXNpYmlsaXR5IEVuYWJsZSBiaXQu
Cj4gPj4KPiA+PiBUaGlzIHBhdGNoIGltcGxlbWVudHMgaXByb2NfcGNpZV9jb25maWdfcmVhZCB3
aGljaCBnZXRzIGNhbGxlZCBmb3IKPiA+PiBTdGluZ3JheSwgaWYgaXQgcmVjZWl2ZXMgYSBDUlMg
Y29tcGxldGlvbiwgaXQgcmV0cmllcyByZWFkaW5nIGl0IGFnYWluLgo+ID4+IEluIGNhc2Ugb2Yg
dGltZW91dCwgaXQgcmV0dXJucyAweGZmZmZmZmZmLgo+ID4+IEZvciBvdGhlciBpcHJvYyBiYXNl
ZCBTT0MsIGl0IGZhbGxzIGJhY2sgdG8gUENJIGdlbmVyaWMgQVBJcy4KPiA+Pgo+ID4+IFNpZ25l
ZC1vZmYtYnk6IE96YSBQYXdhbmRlZXAgPG96YS5vemFAYnJvYWRjb20uY29tPgo+ID4+Cj4gPj4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2hvc3QvcGNpZS1pcHJvYy5jIGIvZHJpdmVycy9wY2kv
aG9zdC9wY2llLWlwcm9jLmMKPiA+PiBpbmRleCA2MWQ5YmU2Li4zN2Y0YWRmIDEwMDY0NAo+ID4+
IC0tLSBhL2RyaXZlcnMvcGNpL2hvc3QvcGNpZS1pcHJvYy5jCj4gPj4gKysrIGIvZHJpdmVycy9w
Y2kvaG9zdC9wY2llLWlwcm9jLmMKPiA+PiBAQCAtNjgsNiArNjgsOSBAQAo+ID4+ICAjZGVmaW5l
IEFQQl9FUlJfRU5fU0hJRlQgICAgICAgICAgICAgMAo+ID4+ICAjZGVmaW5lIEFQQl9FUlJfRU4g
ICAgICAgICAgICAgICAgICAgQklUKEFQQl9FUlJfRU5fU0hJRlQpCj4gPj4KPiA+PiArI2RlZmlu
ZSBDRkdfUkVUUllfU1RBVFVTICAgICAgICAgICAgIDB4ZmZmZjAwMDEKPiA+PiArI2RlZmluZSBD
RkdfUkVUUllfU1RBVFVTX1RJTUVPVVRfVVMgIDUwMDAwMCAvKiA1MDAgbWlsbGktc2Vjb25kcy4g
Ki8KPiA+PiArCj4gPj4gIC8qIGRlcml2ZSB0aGUgZW51bSBpbmRleCBvZiB0aGUgb3V0Ym91bmQv
aW5ib3VuZCBtYXBwaW5nIHJlZ2lzdGVycyAqLwo+ID4+ICAjZGVmaW5lIE1BUF9SRUcoYmFzZV9y
ZWcsIGluZGV4KSAgICAgICgoYmFzZV9yZWcpICsgKGluZGV4KSAqIDIpCj4gPj4KPiA+PiBAQCAt
NDczLDYgKzQ3Niw2NCBAQCBzdGF0aWMgdm9pZCBfX2lvbWVtICppcHJvY19wY2llX21hcF9lcF9j
ZmdfcmVnKHN0cnVjdCBpcHJvY19wY2llICpwY2llLAo+ID4+ICAgICAgIHJldHVybiAocGNpZS0+
YmFzZSArIG9mZnNldCk7Cj4gPj4gIH0KPiA+Pgo+ID4+ICtzdGF0aWMgdW5zaWduZWQgaW50IGlw
cm9jX3BjaWVfY2ZnX3JldHJ5KHZvaWQgX19pb21lbSAqY2ZnX2RhdGFfcCkKPiA+PiArewo+ID4+
ICsgICAgIGludCB0aW1lb3V0ID0gQ0ZHX1JFVFJZX1NUQVRVU19USU1FT1VUX1VTOwo+ID4+ICsg
ICAgIHVuc2lnbmVkIGludCBkYXRhOwo+ID4+ICsKPiA+PiArICAgICAvKgo+ID4+ICsgICAgICAq
IEFzIHBlciBQQ0llIHNwZWMgcjMuMSwgc2VjIDIuMy4yLCBDUlMgU29mdHdhcmUKPiA+PiArICAg
ICAgKiBWaXNpYmlsaXR5IG9ubHkgYWZmZWN0cyBjb25maWcgcmVhZCBvZiB0aGUgVmVuZG9yIElE
Lgo+ID4+ICsgICAgICAqIEZvciBjb25maWcgd3JpdGUgb3IgYW55IG90aGVyIGNvbmZpZyByZWFk
IHRoZSBSb290IG11c3QKPiA+PiArICAgICAgKiBhdXRvbWF0aWNhbGx5IHJlLWlzc3VlIGNvbmZp
Z3VyYXRpb24gcmVxdWVzdCBhZ2FpbiBhcyBhCj4gPj4gKyAgICAgICogbmV3IHJlcXVlc3QuCj4g
Pj4gKyAgICAgICoKPiA+PiArICAgICAgKiBGb3IgY29uZmlnIHJlYWRzLCB0aGlzIGhhcmR3YXJl
IHJldHVybnMgQ0ZHX1JFVFJZX1NUQVRVUyBkYXRhIHdoZW4KPiA+PiArICAgICAgKiBpdCByZWNl
aXZlcyBhIENSUyBjb21wbGV0aW9uIGZvciBhIGNvbmZpZyByZWFkLCByZWdhcmRsZXNzIG9mIHRo
ZQo+ID4+ICsgICAgICAqIGFkZHJlc3Mgb2YgdGhlIHJlYWQgb3IgdGhlIENSUyBTb2Z0d2FyZSBW
aXNpYmlsaXR5IEVuYWJsZSBiaXQuIEFzIGEKPiA+PiArICAgICAgKiBwYXJ0aWFsIHdvcmthcm91
bmQgZm9yIHRoaXMsIHdlIHJldHJ5IGluIHNvZnR3YXJlIGFueSByZWFkIHRoYXQKPiA+PiArICAg
ICAgKiByZXR1cm5zIENGR19SRVRSWV9TVEFUVVMuCj4gPj4gKyAgICAgICovCj4gPj4gKyAgICAg
ZGF0YSA9IHJlYWRsKGNmZ19kYXRhX3ApOwo+ID4+ICsgICAgIHdoaWxlIChkYXRhID09IENGR19S
RVRSWV9TVEFUVVMgJiYgdGltZW91dC0tKSB7Cj4gPj4gKyAgICAgICAgICAgICB1ZGVsYXkoMSk7
Cj4gPj4gKyAgICAgICAgICAgICBkYXRhID0gcmVhZGwoY2ZnX2RhdGFfcCk7Cj4gPj4gKyAgICAg
fQo+ID4+ICsKPiA+PiArICAgICBpZiAoZGF0YSA9PSBDRkdfUkVUUllfU1RBVFVTKQo+ID4+ICsg
ICAgICAgICAgICAgZGF0YSA9IDB4ZmZmZmZmZmY7Cj4gPj4gKwo+ID4+ICsgICAgIHJldHVybiBk
YXRhOwo+ID4+ICt9Cj4gPj4gKwo+ID4+ICtzdGF0aWMgaW50IGlwcm9jX3BjaWVfY29uZmlnX3Jl
YWQoc3RydWN0IHBjaV9idXMgKmJ1cywgdW5zaWduZWQgaW50IGRldmZuLAo+ID4+ICsgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICBpbnQgd2hlcmUsIGludCBzaXplLCB1MzIgKnZhbCkK
PiA+PiArewo+ID4+ICsgICAgIHN0cnVjdCBpcHJvY19wY2llICpwY2llID0gaXByb2NfZGF0YShi
dXMpOwo+ID4+ICsgICAgIHVuc2lnbmVkIGludCBzbG90ID0gUENJX1NMT1QoZGV2Zm4pOwo+ID4+
ICsgICAgIHVuc2lnbmVkIGludCBmbiA9IFBDSV9GVU5DKGRldmZuKTsKPiA+PiArICAgICB1bnNp
Z25lZCBpbnQgYnVzbm8gPSBidXMtPm51bWJlcjsKPiA+PiArICAgICB2b2lkIF9faW9tZW0gKmNm
Z19kYXRhX3A7Cj4gPj4gKyAgICAgdW5zaWduZWQgaW50IGRhdGE7Cj4gPj4gKwo+ID4+ICsgICAg
IC8qIHJvb3QgY29tcGxleCBhY2Nlc3MuICovCj4gPj4gKyAgICAgaWYgKGJ1c25vID09IDApCj4g
Pj4gKyAgICAgICAgICAgICByZXR1cm4gcGNpX2dlbmVyaWNfY29uZmlnX3JlYWQzMihidXMsIGRl
dmZuLCB3aGVyZSwgc2l6ZSwgdmFsKTsKPiA+Cj4gPiBJdCBzb3VuZHMgbGlrZSBTdGluZ3JheSBh
ZHZlcnRpc2VzIENSUyBTViBzdXBwb3J0IGluIGl0cyBSb290IENhcGFiaWxpdGllcwo+ID4gcmVn
aXN0ZXIuICBJIHRoaW5rIHdlIHNob3VsZCBtYXNrIG91dCBQQ0lfRVhQX1JUQ0FQX0NSU1ZJUyBz
byB3ZSBkb24ndAo+ID4gYWR2ZXJ0aXNlIGl0LiAgVGhhdCB3aWxsIGtlZXAgTGludXggZnJvbSB0
cnlpbmcgdG8gZW5hYmxlIGl0LiAgSSBrbm93IHRoZQo+ID4gaGFyZHdhcmUgZG9lc24ndCBsb29r
IGF0IFBDSV9FWFBfUlRDVExfQ1JTU1ZFLCBidXQgdGhlcmUncyBubyBwb2ludCBpbgo+ID4gY29u
ZnVzaW5nIHVzZXJzIHJlYWRpbmcgdGhlIGxzcGNpIG91dHB1dC4KPiA+Cj4gPiBXZSBkaWQgc29t
ZXRoaW5nIHNpbWlsYXIgd2l0aCBmMDlmODczNWZiOWMgKCJQQ0k6IHhnZW5lOiBEaXNhYmxlCj4g
PiBDb25maWd1cmF0aW9uIFJlcXVlc3QgUmV0cnkgU3RhdHVzIGZvciB2MSBzaWxpY29uIikuCj4g
Pgo+ID4gSSB0cmllZCB0byBkbyB0aGlzIGluIHRoZSBwYXRjaCBJIHB1c2hlZCB0byBwY2kvaG9z
dC1pcHJvYy4KPiA+Cj4gPj4gKwo+ID4+ICsgICAgIGNmZ19kYXRhX3AgPSBpcHJvY19wY2llX21h
cF9lcF9jZmdfcmVnKHBjaWUsIGJ1c25vLCBzbG90LCBmbiwgd2hlcmUpOwo+ID4+ICsKPiA+PiAr
ICAgICBpZiAoIWNmZ19kYXRhX3ApCj4gPj4gKyAgICAgICAgICAgICByZXR1cm4gUENJQklPU19E
RVZJQ0VfTk9UX0ZPVU5EOwo+ID4+ICsKPiA+PiArICAgICBkYXRhID0gaXByb2NfcGNpZV9jZmdf
cmV0cnkoY2ZnX2RhdGFfcCk7Cj4gPj4gKwo+ID4+ICsgICAgICp2YWwgPSBkYXRhOwo+ID4+ICsg
ICAgIGlmIChzaXplIDw9IDIpCj4gPj4gKyAgICAgICAgICAgICAqdmFsID0gKGRhdGEgPj4gKDgg
KiAod2hlcmUgJiAzKSkpICYgKCgxIDw8IChzaXplICogOCkpIC0gMSk7Cj4gPj4gKwo+ID4+ICsg
ICAgIHJldHVybiBQQ0lCSU9TX1NVQ0NFU1NGVUw7Cj4gPj4gK30KPiA+PiArCj4gPj4gIC8qKgo+
ID4+ICAgKiBOb3RlIGFjY2VzcyB0byB0aGUgY29uZmlndXJhdGlvbiByZWdpc3RlcnMgYXJlIHBy
b3RlY3RlZCBhdCB0aGUgaGlnaGVyIGxheWVyCj4gPj4gICAqIGJ5ICdwY2lfbG9jaycgaW4gZHJp
dmVycy9wY2kvYWNjZXNzLmMKPiA+PiBAQCAtNTY3LDggKzYyOCwxMyBAQCBzdGF0aWMgaW50IGlw
cm9jX3BjaWVfY29uZmlnX3JlYWQzMihzdHJ1Y3QgcGNpX2J1cyAqYnVzLCB1bnNpZ25lZCBpbnQg
ZGV2Zm4sCj4gPj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGludCB3aGVyZSwg
aW50IHNpemUsIHUzMiAqdmFsKQo+ID4+ICB7Cj4gPj4gICAgICAgaW50IHJldDsKPiA+PiArICAg
ICBzdHJ1Y3QgaXByb2NfcGNpZSAqcGNpZSA9IGlwcm9jX2RhdGEoYnVzKTsKPiA+Pgo+ID4+ICAg
ICAgIGlwcm9jX3BjaWVfYXBiX2Vycl9kaXNhYmxlKGJ1cywgdHJ1ZSk7Cj4gPj4gKyAgICAgaWYg
KHBjaWUtPnR5cGUgPT0gSVBST0NfUENJRV9QQVhCX1YyKQo+ID4+ICsgICAgICAgICAgICAgcmV0
ID0gaXByb2NfcGNpZV9jb25maWdfcmVhZChidXMsIGRldmZuLCB3aGVyZSwgc2l6ZSwgdmFsKTsK
PiA+PiArICAgICBlbHNlCj4gPj4gKyAgICAgICAgICAgICByZXQgPSBwY2lfZ2VuZXJpY19jb25m
aWdfcmVhZDMyKGJ1cywgZGV2Zm4sIHdoZXJlLCBzaXplLCB2YWwpOwo+ID4+ICAgICAgIHJldCA9
IHBjaV9nZW5lcmljX2NvbmZpZ19yZWFkMzIoYnVzLCBkZXZmbiwgd2hlcmUsIHNpemUsIHZhbCk7
Cj4gPgo+ID4gVGhpcyBsYXN0IHBjaV9nZW5lcmljX2NvbmZpZ19yZWFkMzIoKSBjYWxsIGxvb2tz
IGxpa2UgYSBkdXBsaWNhdGUuCj4gCj4geWVzIGluZGVlZC4KPiBJIGhhdmUgdGVzdGVkIHlvdXIg
Q1JTIHZpc2liaWxpdHkgYml0IGNoYW5nZXM7IGFuZCBpdCB3b3JrcyBmaW5lLgo+IAo+IGRvIHlv
dSB3YW50IG1lIHRvIHBvc3QgbmV3IHBhdGNoLXNldCBieSByZW1vdmluZyB0aGUgZHVwbGljYXRl
IGNhbGwKPiBhbG9uZyB3aXRoIHRoZSBjaGFuZ2VzIHlvdSBoYXZlIG1hZGUgPwo+IAo+IG9yIHNp
bmNlLCB5b3UgaGF2ZSBhbHJlYWR5IGFwcGxpZWQgcGF0Y2hlcywgd2l0aCB5b3VyIGNoYW5nZXMs
IHlvdQo+IHdpbGwgdGFrZSBjYXJlIG9mIHJlbW92aW5nIHRoaXMgbGFzdCBkdXBsaWNhdGUgY2Fs
bCA/Cj4gSSB0aGluayB0aGlzIGlzIHRoZSBsYXN0IGNoYW5nZSBmb3IgdGhpcyBwYXRjaC1zZXQs
IElmIEkgZGlkIG5vdCBtaXNzIGFueXRoaW5nLgo+IAo+IHBsZWFzZSBsZXQgbWUga25vdy4KCkkg
YWxyZWFkeSByZW1vdmVkIHRoYXQgZHVwbGljYXRlIGNhbGwuICBJdCBzaG91bGQgYmUgaW4gdGhl
IG5leHQgLW5leHQuCkxldCBtZSBrbm93IGlmIHRoZXJlJ3MgYW55dGhpbmcgd3Jvbmcgd2l0aCBp
dC4KCkJqb3JuCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
XwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmlu
ZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9s
aW51eC1hcm0ta2VybmVsCg==

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Bjorn Helgaas]

On Tue, Aug 29, 2017 at 11:02:23AM +0530, Oza Oza wrote:
> On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
> >> PCIe spec r3.1, sec 2.3.2
> >> If CRS software visibility is not enabled, the RC must reissue the
> >> config request as a new request.
> >>
> >> - If CRS software visibility is enabled,
> >> - for a config read of Vendor ID, the RC must return 0x0001 data
> >> - for all other config reads/writes, the RC must reissue the
> >>   request
> >>
> >> iproc PCIe Controller spec:
> >> 4.7.3.3. Retry Status On Configuration Cycle
> >> Endpoints are allowed to generate retry status on configuration
> >> cycles. In this case, the RC needs to re-issue the request. The IP
> >> does not handle this because the number of configuration cycles needed
> >> will probably be less than the total number of non-posted operations
> >> needed.
> >>
> >> When a retry status is received on the User RX interface for a
> >> configuration request that was sent on the User TX interface,
> >> it will be indicated with a completion with the CMPL_STATUS field set
> >> to 2=CRS, and the user will have to find the address and data values
> >> and send a new transaction on the User TX interface.
> >> When the internal configuration space returns a retry status during a
> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
> >> CRS status.
> >> When the CRS Software Visibility Enable register in the Root Control
> >> register is enabled, the IP will return the data value to 0x0001 for
> >> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
> >> the request for reads of offset 0 that return with CRS status.  This
> >> is true for both the User RX Interface and for the Command/Status
> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
> >> field of the completion on the User RX Interface will not be 2=CRS and
> >> the pcie_cscrs signal will not assert on the Command/Status interface.
> >>
> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
> >> with Configuration Request Retry Status (CRS) should be reissued by
> >> the hardware except reads of the Vendor ID when CRS Software
> >> Visibility is enabled.
> >>
> >> This hardware never reissues configuration requests when it receives
> >> CRS completions.
> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
> >> request for any configuration offset.
> >>
> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
> >> it receives a CRS completion for a config read, regardless of the
> >> address of the read or the CRS Software Visibility Enable bit.
> >>
> >> This patch implements iproc_pcie_config_read which gets called for
> >> Stingray, if it receives a CRS completion, it retries reading it again.
> >> In case of timeout, it returns 0xffffffff.
> >> For other iproc based SOC, it falls back to PCI generic APIs.
> >>
> >> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> >>
> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> >> index 61d9be6..37f4adf 100644
> >> --- a/drivers/pci/host/pcie-iproc.c
> >> +++ b/drivers/pci/host/pcie-iproc.c
> >> @@ -68,6 +68,9 @@
> >>  #define APB_ERR_EN_SHIFT             0
> >>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
> >>
> >> +#define CFG_RETRY_STATUS             0xffff0001
> >> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> >> +
> >>  /* derive the enum index of the outbound/inbound mapping registers */
> >>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
> >>
> >> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
> >>       return (pcie->base + offset);
> >>  }
> >>
> >> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> >> +{
> >> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> >> +     unsigned int data;
> >> +
> >> +     /*
> >> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
> >> +      * Visibility only affects config read of the Vendor ID.
> >> +      * For config write or any other config read the Root must
> >> +      * automatically re-issue configuration request again as a
> >> +      * new request.
> >> +      *
> >> +      * For config reads, this hardware returns CFG_RETRY_STATUS data when
> >> +      * it receives a CRS completion for a config read, regardless of the
> >> +      * address of the read or the CRS Software Visibility Enable bit. As a
> >> +      * partial workaround for this, we retry in software any read that
> >> +      * returns CFG_RETRY_STATUS.
> >> +      */
> >> +     data = readl(cfg_data_p);
> >> +     while (data == CFG_RETRY_STATUS && timeout--) {
> >> +             udelay(1);
> >> +             data = readl(cfg_data_p);
> >> +     }
> >> +
> >> +     if (data == CFG_RETRY_STATUS)
> >> +             data = 0xffffffff;
> >> +
> >> +     return data;
> >> +}
> >> +
> >> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> >> +                                 int where, int size, u32 *val)
> >> +{
> >> +     struct iproc_pcie *pcie = iproc_data(bus);
> >> +     unsigned int slot = PCI_SLOT(devfn);
> >> +     unsigned int fn = PCI_FUNC(devfn);
> >> +     unsigned int busno = bus->number;
> >> +     void __iomem *cfg_data_p;
> >> +     unsigned int data;
> >> +
> >> +     /* root complex access. */
> >> +     if (busno == 0)
> >> +             return pci_generic_config_read32(bus, devfn, where, size, val);
> >
> > It sounds like Stingray advertises CRS SV support in its Root Capabilities
> > register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
> > advertise it.  That will keep Linux from trying to enable it.  I know the
> > hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
> > confusing users reading the lspci output.
> >
> > We did something similar with f09f8735fb9c ("PCI: xgene: Disable
> > Configuration Request Retry Status for v1 silicon").
> >
> > I tried to do this in the patch I pushed to pci/host-iproc.
> >
> >> +
> >> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> >> +
> >> +     if (!cfg_data_p)
> >> +             return PCIBIOS_DEVICE_NOT_FOUND;
> >> +
> >> +     data = iproc_pcie_cfg_retry(cfg_data_p);
> >> +
> >> +     *val = data;
> >> +     if (size <= 2)
> >> +             *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> >> +
> >> +     return PCIBIOS_SUCCESSFUL;
> >> +}
> >> +
> >>  /**
> >>   * Note access to the configuration registers are protected at the higher layer
> >>   * by 'pci_lock' in drivers/pci/access.c
> >> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> >>                                   int where, int size, u32 *val)
> >>  {
> >>       int ret;
> >> +     struct iproc_pcie *pcie = iproc_data(bus);
> >>
> >>       iproc_pcie_apb_err_disable(bus, true);
> >> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
> >> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
> >> +     else
> >> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
> >>       ret = pci_generic_config_read32(bus, devfn, where, size, val);
> >
> > This last pci_generic_config_read32() call looks like a duplicate.
> 
> yes indeed.
> I have tested your CRS visibility bit changes; and it works fine.
> 
> do you want me to post new patch-set by removing the duplicate call
> along with the changes you have made ?
> 
> or since, you have already applied patches, with your changes, you
> will take care of removing this last duplicate call ?
> I think this is the last change for this patch-set, If I did not miss anything.
> 
> please let me know.

I already removed that duplicate call.  It should be in the next -next.
Let me know if there's anything wrong with it.

Bjorn

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Wed, Aug 30, 2017 at 1:32 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Aug 29, 2017 at 11:02:23AM +0530, Oza Oza wrote:
>> On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> >> PCIe spec r3.1, sec 2.3.2
>> >> If CRS software visibility is not enabled, the RC must reissue the
>> >> config request as a new request.
>> >>
>> >> - If CRS software visibility is enabled,
>> >> - for a config read of Vendor ID, the RC must return 0x0001 data
>> >> - for all other config reads/writes, the RC must reissue the
>> >>   request
>> >>
>> >> iproc PCIe Controller spec:
>> >> 4.7.3.3. Retry Status On Configuration Cycle
>> >> Endpoints are allowed to generate retry status on configuration
>> >> cycles. In this case, the RC needs to re-issue the request. The IP
>> >> does not handle this because the number of configuration cycles needed
>> >> will probably be less than the total number of non-posted operations
>> >> needed.
>> >>
>> >> When a retry status is received on the User RX interface for a
>> >> configuration request that was sent on the User TX interface,
>> >> it will be indicated with a completion with the CMPL_STATUS field set
>> >> to 2=CRS, and the user will have to find the address and data values
>> >> and send a new transaction on the User TX interface.
>> >> When the internal configuration space returns a retry status during a
>> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> >> CRS status.
>> >> When the CRS Software Visibility Enable register in the Root Control
>> >> register is enabled, the IP will return the data value to 0x0001 for
>> >> the Vendor ID value and 0xffff  (all 1’s) for the rest of the data in
>> >> the request for reads of offset 0 that return with CRS status.  This
>> >> is true for both the User RX Interface and for the Command/Status
>> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> >> field of the completion on the User RX Interface will not be 2=CRS and
>> >> the pcie_cscrs signal will not assert on the Command/Status interface.
>> >>
>> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> >> with Configuration Request Retry Status (CRS) should be reissued by
>> >> the hardware except reads of the Vendor ID when CRS Software
>> >> Visibility is enabled.
>> >>
>> >> This hardware never reissues configuration requests when it receives
>> >> CRS completions.
>> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> >> request for any configuration offset.
>> >>
>> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> it receives a CRS completion for a config read, regardless of the
>> >> address of the read or the CRS Software Visibility Enable bit.
>> >>
>> >> This patch implements iproc_pcie_config_read which gets called for
>> >> Stingray, if it receives a CRS completion, it retries reading it again.
>> >> In case of timeout, it returns 0xffffffff.
>> >> For other iproc based SOC, it falls back to PCI generic APIs.
>> >>
>> >> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> >>
>> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> >> index 61d9be6..37f4adf 100644
>> >> --- a/drivers/pci/host/pcie-iproc.c
>> >> +++ b/drivers/pci/host/pcie-iproc.c
>> >> @@ -68,6 +68,9 @@
>> >>  #define APB_ERR_EN_SHIFT             0
>> >>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>> >>
>> >> +#define CFG_RETRY_STATUS             0xffff0001
>> >> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> >> +
>> >>  /* derive the enum index of the outbound/inbound mapping registers */
>> >>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>> >>
>> >> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>> >>       return (pcie->base + offset);
>> >>  }
>> >>
>> >> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> >> +{
>> >> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
>> >> +     unsigned int data;
>> >> +
>> >> +     /*
>> >> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
>> >> +      * Visibility only affects config read of the Vendor ID.
>> >> +      * For config write or any other config read the Root must
>> >> +      * automatically re-issue configuration request again as a
>> >> +      * new request.
>> >> +      *
>> >> +      * For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> +      * it receives a CRS completion for a config read, regardless of the
>> >> +      * address of the read or the CRS Software Visibility Enable bit. As a
>> >> +      * partial workaround for this, we retry in software any read that
>> >> +      * returns CFG_RETRY_STATUS.
>> >> +      */
>> >> +     data = readl(cfg_data_p);
>> >> +     while (data == CFG_RETRY_STATUS && timeout--) {
>> >> +             udelay(1);
>> >> +             data = readl(cfg_data_p);
>> >> +     }
>> >> +
>> >> +     if (data == CFG_RETRY_STATUS)
>> >> +             data = 0xffffffff;
>> >> +
>> >> +     return data;
>> >> +}
>> >> +
>> >> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>> >> +                                 int where, int size, u32 *val)
>> >> +{
>> >> +     struct iproc_pcie *pcie = iproc_data(bus);
>> >> +     unsigned int slot = PCI_SLOT(devfn);
>> >> +     unsigned int fn = PCI_FUNC(devfn);
>> >> +     unsigned int busno = bus->number;
>> >> +     void __iomem *cfg_data_p;
>> >> +     unsigned int data;
>> >> +
>> >> +     /* root complex access. */
>> >> +     if (busno == 0)
>> >> +             return pci_generic_config_read32(bus, devfn, where, size, val);
>> >
>> > It sounds like Stingray advertises CRS SV support in its Root Capabilities
>> > register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
>> > advertise it.  That will keep Linux from trying to enable it.  I know the
>> > hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
>> > confusing users reading the lspci output.
>> >
>> > We did something similar with f09f8735fb9c ("PCI: xgene: Disable
>> > Configuration Request Retry Status for v1 silicon").
>> >
>> > I tried to do this in the patch I pushed to pci/host-iproc.
>> >
>> >> +
>> >> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>> >> +
>> >> +     if (!cfg_data_p)
>> >> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> >> +
>> >> +     data = iproc_pcie_cfg_retry(cfg_data_p);
>> >> +
>> >> +     *val = data;
>> >> +     if (size <= 2)
>> >> +             *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> >> +
>> >> +     return PCIBIOS_SUCCESSFUL;
>> >> +}
>> >> +
>> >>  /**
>> >>   * Note access to the configuration registers are protected at the higher layer
>> >>   * by 'pci_lock' in drivers/pci/access.c
>> >> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>> >>                                   int where, int size, u32 *val)
>> >>  {
>> >>       int ret;
>> >> +     struct iproc_pcie *pcie = iproc_data(bus);
>> >>
>> >>       iproc_pcie_apb_err_disable(bus, true);
>> >> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
>> >> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>> >> +     else
>> >> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> >>       ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> >
>> > This last pci_generic_config_read32() call looks like a duplicate.
>>
>> yes indeed.
>> I have tested your CRS visibility bit changes; and it works fine.
>>
>> do you want me to post new patch-set by removing the duplicate call
>> along with the changes you have made ?
>>
>> or since, you have already applied patches, with your changes, you
>> will take care of removing this last duplicate call ?
>> I think this is the last change for this patch-set, If I did not miss anything.
>>
>> please let me know.
>
> I already removed that duplicate call.  It should be in the next -next.
> Let me know if there's anything wrong with it.
>
> Bjorn

Its all well I think. I see no problem.
once they are in, I have to base my PCI hotplug patches on those.

Regards,
Oza.

Re: [PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Wed, Aug 30, 2017 at 1:32 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Aug 29, 2017 at 11:02:23AM +0530, Oza Oza wrote:
>> On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrot=
e:
>> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> >> PCIe spec r3.1, sec 2.3.2
>> >> If CRS software visibility is not enabled, the RC must reissue the
>> >> config request as a new request.
>> >>
>> >> - If CRS software visibility is enabled,
>> >> - for a config read of Vendor ID, the RC must return 0x0001 data
>> >> - for all other config reads/writes, the RC must reissue the
>> >>   request
>> >>
>> >> iproc PCIe Controller spec:
>> >> 4.7.3.3. Retry Status On Configuration Cycle
>> >> Endpoints are allowed to generate retry status on configuration
>> >> cycles. In this case, the RC needs to re-issue the request. The IP
>> >> does not handle this because the number of configuration cycles neede=
d
>> >> will probably be less than the total number of non-posted operations
>> >> needed.
>> >>
>> >> When a retry status is received on the User RX interface for a
>> >> configuration request that was sent on the User TX interface,
>> >> it will be indicated with a completion with the CMPL_STATUS field set
>> >> to 2=3DCRS, and the user will have to find the address and data value=
s
>> >> and send a new transaction on the User TX interface.
>> >> When the internal configuration space returns a retry status during a
>> >> configuration cycle (user_cscfg =3D 1) on the Command/Status interfac=
e,
>> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> >> CRS status.
>> >> When the CRS Software Visibility Enable register in the Root Control
>> >> register is enabled, the IP will return the data value to 0x0001 for
>> >> the Vendor ID value and 0xffff  (all 1=E2=80=99s) for the rest of the=
 data in
>> >> the request for reads of offset 0 that return with CRS status.  This
>> >> is true for both the User RX Interface and for the Command/Status
>> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> >> field of the completion on the User RX Interface will not be 2=3DCRS =
and
>> >> the pcie_cscrs signal will not assert on the Command/Status interface=
.
>> >>
>> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> >> with Configuration Request Retry Status (CRS) should be reissued by
>> >> the hardware except reads of the Vendor ID when CRS Software
>> >> Visibility is enabled.
>> >>
>> >> This hardware never reissues configuration requests when it receives
>> >> CRS completions.
>> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> >> request for any configuration offset.
>> >>
>> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> it receives a CRS completion for a config read, regardless of the
>> >> address of the read or the CRS Software Visibility Enable bit.
>> >>
>> >> This patch implements iproc_pcie_config_read which gets called for
>> >> Stingray, if it receives a CRS completion, it retries reading it agai=
n.
>> >> In case of timeout, it returns 0xffffffff.
>> >> For other iproc based SOC, it falls back to PCI generic APIs.
>> >>
>> >> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> >>
>> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-ip=
roc.c
>> >> index 61d9be6..37f4adf 100644
>> >> --- a/drivers/pci/host/pcie-iproc.c
>> >> +++ b/drivers/pci/host/pcie-iproc.c
>> >> @@ -68,6 +68,9 @@
>> >>  #define APB_ERR_EN_SHIFT             0
>> >>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>> >>
>> >> +#define CFG_RETRY_STATUS             0xffff0001
>> >> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> >> +
>> >>  /* derive the enum index of the outbound/inbound mapping registers *=
/
>> >>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>> >>
>> >> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(s=
truct iproc_pcie *pcie,
>> >>       return (pcie->base + offset);
>> >>  }
>> >>
>> >> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> >> +{
>> >> +     int timeout =3D CFG_RETRY_STATUS_TIMEOUT_US;
>> >> +     unsigned int data;
>> >> +
>> >> +     /*
>> >> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
>> >> +      * Visibility only affects config read of the Vendor ID.
>> >> +      * For config write or any other config read the Root must
>> >> +      * automatically re-issue configuration request again as a
>> >> +      * new request.
>> >> +      *
>> >> +      * For config reads, this hardware returns CFG_RETRY_STATUS dat=
a when
>> >> +      * it receives a CRS completion for a config read, regardless o=
f the
>> >> +      * address of the read or the CRS Software Visibility Enable bi=
t. As a
>> >> +      * partial workaround for this, we retry in software any read t=
hat
>> >> +      * returns CFG_RETRY_STATUS.
>> >> +      */
>> >> +     data =3D readl(cfg_data_p);
>> >> +     while (data =3D=3D CFG_RETRY_STATUS && timeout--) {
>> >> +             udelay(1);
>> >> +             data =3D readl(cfg_data_p);
>> >> +     }
>> >> +
>> >> +     if (data =3D=3D CFG_RETRY_STATUS)
>> >> +             data =3D 0xffffffff;
>> >> +
>> >> +     return data;
>> >> +}
>> >> +
>> >> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int =
devfn,
>> >> +                                 int where, int size, u32 *val)
>> >> +{
>> >> +     struct iproc_pcie *pcie =3D iproc_data(bus);
>> >> +     unsigned int slot =3D PCI_SLOT(devfn);
>> >> +     unsigned int fn =3D PCI_FUNC(devfn);
>> >> +     unsigned int busno =3D bus->number;
>> >> +     void __iomem *cfg_data_p;
>> >> +     unsigned int data;
>> >> +
>> >> +     /* root complex access. */
>> >> +     if (busno =3D=3D 0)
>> >> +             return pci_generic_config_read32(bus, devfn, where, siz=
e, val);
>> >
>> > It sounds like Stingray advertises CRS SV support in its Root Capabili=
ties
>> > register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
>> > advertise it.  That will keep Linux from trying to enable it.  I know =
the
>> > hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
>> > confusing users reading the lspci output.
>> >
>> > We did something similar with f09f8735fb9c ("PCI: xgene: Disable
>> > Configuration Request Retry Status for v1 silicon").
>> >
>> > I tried to do this in the patch I pushed to pci/host-iproc.
>> >
>> >> +
>> >> +     cfg_data_p =3D iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn,=
 where);
>> >> +
>> >> +     if (!cfg_data_p)
>> >> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> >> +
>> >> +     data =3D iproc_pcie_cfg_retry(cfg_data_p);
>> >> +
>> >> +     *val =3D data;
>> >> +     if (size <=3D 2)
>> >> +             *val =3D (data >> (8 * (where & 3))) & ((1 << (size * 8=
)) - 1);
>> >> +
>> >> +     return PCIBIOS_SUCCESSFUL;
>> >> +}
>> >> +
>> >>  /**
>> >>   * Note access to the configuration registers are protected at the h=
igher layer
>> >>   * by 'pci_lock' in drivers/pci/access.c
>> >> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_b=
us *bus, unsigned int devfn,
>> >>                                   int where, int size, u32 *val)
>> >>  {
>> >>       int ret;
>> >> +     struct iproc_pcie *pcie =3D iproc_data(bus);
>> >>
>> >>       iproc_pcie_apb_err_disable(bus, true);
>> >> +     if (pcie->type =3D=3D IPROC_PCIE_PAXB_V2)
>> >> +             ret =3D iproc_pcie_config_read(bus, devfn, where, size,=
 val);
>> >> +     else
>> >> +             ret =3D pci_generic_config_read32(bus, devfn, where, si=
ze, val);
>> >>       ret =3D pci_generic_config_read32(bus, devfn, where, size, val)=
;
>> >
>> > This last pci_generic_config_read32() call looks like a duplicate.
>>
>> yes indeed.
>> I have tested your CRS visibility bit changes; and it works fine.
>>
>> do you want me to post new patch-set by removing the duplicate call
>> along with the changes you have made ?
>>
>> or since, you have already applied patches, with your changes, you
>> will take care of removing this last duplicate call ?
>> I think this is the last change for this patch-set, If I did not miss an=
ything.
>>
>> please let me know.
>
> I already removed that duplicate call.  It should be in the next -next.
> Let me know if there's anything wrong with it.
>
> Bjorn

Its all well I think. I see no problem.
once they are in, I have to base my PCI hotplug patches on those.

Regards,
Oza.

[PATCH v8 2/3] PCI: iproc: retry request when CRS returned from EP

[Oza Oza]

On Wed, Aug 30, 2017 at 1:32 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Aug 29, 2017 at 11:02:23AM +0530, Oza Oza wrote:
>> On Tue, Aug 29, 2017 at 3:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Thu, Aug 24, 2017 at 10:34:25AM +0530, Oza Pawandeep wrote:
>> >> PCIe spec r3.1, sec 2.3.2
>> >> If CRS software visibility is not enabled, the RC must reissue the
>> >> config request as a new request.
>> >>
>> >> - If CRS software visibility is enabled,
>> >> - for a config read of Vendor ID, the RC must return 0x0001 data
>> >> - for all other config reads/writes, the RC must reissue the
>> >>   request
>> >>
>> >> iproc PCIe Controller spec:
>> >> 4.7.3.3. Retry Status On Configuration Cycle
>> >> Endpoints are allowed to generate retry status on configuration
>> >> cycles. In this case, the RC needs to re-issue the request. The IP
>> >> does not handle this because the number of configuration cycles needed
>> >> will probably be less than the total number of non-posted operations
>> >> needed.
>> >>
>> >> When a retry status is received on the User RX interface for a
>> >> configuration request that was sent on the User TX interface,
>> >> it will be indicated with a completion with the CMPL_STATUS field set
>> >> to 2=CRS, and the user will have to find the address and data values
>> >> and send a new transaction on the User TX interface.
>> >> When the internal configuration space returns a retry status during a
>> >> configuration cycle (user_cscfg = 1) on the Command/Status interface,
>> >> the pcie_cscrs will assert with the pcie_csack signal to indicate the
>> >> CRS status.
>> >> When the CRS Software Visibility Enable register in the Root Control
>> >> register is enabled, the IP will return the data value to 0x0001 for
>> >> the Vendor ID value and 0xffff  (all 1?s) for the rest of the data in
>> >> the request for reads of offset 0 that return with CRS status.  This
>> >> is true for both the User RX Interface and for the Command/Status
>> >> interface.  When CRS Software Visibility is enabled, the CMPL_STATUS
>> >> field of the completion on the User RX Interface will not be 2=CRS and
>> >> the pcie_cscrs signal will not assert on the Command/Status interface.
>> >>
>> >> Per PCIe r3.1, sec 2.3.2, config requests that receive completions
>> >> with Configuration Request Retry Status (CRS) should be reissued by
>> >> the hardware except reads of the Vendor ID when CRS Software
>> >> Visibility is enabled.
>> >>
>> >> This hardware never reissues configuration requests when it receives
>> >> CRS completions.
>> >> Note that, neither PCIe host bridge nor PCIe core re-issues the
>> >> request for any configuration offset.
>> >>
>> >> For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> it receives a CRS completion for a config read, regardless of the
>> >> address of the read or the CRS Software Visibility Enable bit.
>> >>
>> >> This patch implements iproc_pcie_config_read which gets called for
>> >> Stingray, if it receives a CRS completion, it retries reading it again.
>> >> In case of timeout, it returns 0xffffffff.
>> >> For other iproc based SOC, it falls back to PCI generic APIs.
>> >>
>> >> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> >>
>> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> >> index 61d9be6..37f4adf 100644
>> >> --- a/drivers/pci/host/pcie-iproc.c
>> >> +++ b/drivers/pci/host/pcie-iproc.c
>> >> @@ -68,6 +68,9 @@
>> >>  #define APB_ERR_EN_SHIFT             0
>> >>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>> >>
>> >> +#define CFG_RETRY_STATUS             0xffff0001
>> >> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> >> +
>> >>  /* derive the enum index of the outbound/inbound mapping registers */
>> >>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>> >>
>> >> @@ -473,6 +476,64 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>> >>       return (pcie->base + offset);
>> >>  }
>> >>
>> >> +static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> >> +{
>> >> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
>> >> +     unsigned int data;
>> >> +
>> >> +     /*
>> >> +      * As per PCIe spec r3.1, sec 2.3.2, CRS Software
>> >> +      * Visibility only affects config read of the Vendor ID.
>> >> +      * For config write or any other config read the Root must
>> >> +      * automatically re-issue configuration request again as a
>> >> +      * new request.
>> >> +      *
>> >> +      * For config reads, this hardware returns CFG_RETRY_STATUS data when
>> >> +      * it receives a CRS completion for a config read, regardless of the
>> >> +      * address of the read or the CRS Software Visibility Enable bit. As a
>> >> +      * partial workaround for this, we retry in software any read that
>> >> +      * returns CFG_RETRY_STATUS.
>> >> +      */
>> >> +     data = readl(cfg_data_p);
>> >> +     while (data == CFG_RETRY_STATUS && timeout--) {
>> >> +             udelay(1);
>> >> +             data = readl(cfg_data_p);
>> >> +     }
>> >> +
>> >> +     if (data == CFG_RETRY_STATUS)
>> >> +             data = 0xffffffff;
>> >> +
>> >> +     return data;
>> >> +}
>> >> +
>> >> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>> >> +                                 int where, int size, u32 *val)
>> >> +{
>> >> +     struct iproc_pcie *pcie = iproc_data(bus);
>> >> +     unsigned int slot = PCI_SLOT(devfn);
>> >> +     unsigned int fn = PCI_FUNC(devfn);
>> >> +     unsigned int busno = bus->number;
>> >> +     void __iomem *cfg_data_p;
>> >> +     unsigned int data;
>> >> +
>> >> +     /* root complex access. */
>> >> +     if (busno == 0)
>> >> +             return pci_generic_config_read32(bus, devfn, where, size, val);
>> >
>> > It sounds like Stingray advertises CRS SV support in its Root Capabilities
>> > register.  I think we should mask out PCI_EXP_RTCAP_CRSVIS so we don't
>> > advertise it.  That will keep Linux from trying to enable it.  I know the
>> > hardware doesn't look at PCI_EXP_RTCTL_CRSSVE, but there's no point in
>> > confusing users reading the lspci output.
>> >
>> > We did something similar with f09f8735fb9c ("PCI: xgene: Disable
>> > Configuration Request Retry Status for v1 silicon").
>> >
>> > I tried to do this in the patch I pushed to pci/host-iproc.
>> >
>> >> +
>> >> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>> >> +
>> >> +     if (!cfg_data_p)
>> >> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> >> +
>> >> +     data = iproc_pcie_cfg_retry(cfg_data_p);
>> >> +
>> >> +     *val = data;
>> >> +     if (size <= 2)
>> >> +             *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> >> +
>> >> +     return PCIBIOS_SUCCESSFUL;
>> >> +}
>> >> +
>> >>  /**
>> >>   * Note access to the configuration registers are protected at the higher layer
>> >>   * by 'pci_lock' in drivers/pci/access.c
>> >> @@ -567,8 +628,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>> >>                                   int where, int size, u32 *val)
>> >>  {
>> >>       int ret;
>> >> +     struct iproc_pcie *pcie = iproc_data(bus);
>> >>
>> >>       iproc_pcie_apb_err_disable(bus, true);
>> >> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
>> >> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>> >> +     else
>> >> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> >>       ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> >
>> > This last pci_generic_config_read32() call looks like a duplicate.
>>
>> yes indeed.
>> I have tested your CRS visibility bit changes; and it works fine.
>>
>> do you want me to post new patch-set by removing the duplicate call
>> along with the changes you have made ?
>>
>> or since, you have already applied patches, with your changes, you
>> will take care of removing this last duplicate call ?
>> I think this is the last change for this patch-set, If I did not miss anything.
>>
>> please let me know.
>
> I already removed that duplicate call.  It should be in the next -next.
> Let me know if there's anything wrong with it.
>
> Bjorn

Its all well I think. I see no problem.
once they are in, I have to base my PCI hotplug patches on those.

Regards,
Oza.