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.
>
>> iproc_pcie_apb_err_disable(bus, false);
>>
>> --
>> 1.9.1
>>
