[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 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 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

[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

[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 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 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

[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.