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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>> >>
>> >> 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.