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

[Oza Oza]

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;