On Tue, Aug 29, 2017 at 3:23 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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;
