Re: [PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex config registers

[poza@codeaurora.org]

On 2018-06-12 05:51, Ray Jui wrote:
> On certain versions of Broadcom PAXC based root complexes, certain
> regions of the configuration space are corrupted. As a result, it
> prevents the Linux PCIe stack from traversing the linked list of the
> capability registers completely and therefore the root complex is
> not advertised as "PCIe capable". This prevents the correct PCIe RID
> from being parsed in the kernel PCIe stack. A correct RID is required
> for mapping to a stream ID from the SMMU or the device ID from the
> GICv3 ITS
> 
> This patch fixes up the issue by manually populating the related
> PCIe capabilities
> 
> Signed-off-by: Ray Jui <rjui@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c | 65 
> +++++++++++++++++++++++++++++++++++++++----
>  drivers/pci/host/pcie-iproc.h |  3 ++
>  2 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 3c76c5f..680f6b1 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -85,6 +85,8 @@
>  #define IMAP_VALID_SHIFT		0
>  #define IMAP_VALID			BIT(IMAP_VALID_SHIFT)
> 
> +#define IPROC_PCI_PM_CAP		0x48
> +#define IPROC_PCI_PM_CAP_MASK		0xffff
>  #define IPROC_PCI_EXP_CAP		0xac
> 
>  #define IPROC_PCIE_REG_INVALID		0xffff
> @@ -375,6 +377,17 @@ static const u16 iproc_pcie_reg_paxc_v2[] = {
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  };
> 
> +/*
> + * List of device IDs of controllers that have corrupted capability 
> list that
> + * require SW fixup
> + */
> +static const u16 iproc_pcie_corrupt_cap_did[] = {
> +	0x16cd,
> +	0x16f0,
> +	0xd802,
> +	0xd804
> +};
> +
>  static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
>  {
>  	struct iproc_pcie *pcie = bus->sysdata;
> @@ -495,6 +508,49 @@ static unsigned int iproc_pcie_cfg_retry(void
> __iomem *cfg_data_p)
>  	return data;
>  }
> 
> +static void iproc_pcie_fix_cap(struct iproc_pcie *pcie, int where, u32 
> *val)
> +{
> +	u32 i, dev_id;
> +
> +	switch (where & ~0x3) {
> +	case PCI_VENDOR_ID:
> +		dev_id = *val >> 16;
> +
> +		/*
> +		 * Activate fixup for those controllers that have corrupted
> +		 * capability list registers
> +		 */
> +		for (i = 0; i < ARRAY_SIZE(iproc_pcie_corrupt_cap_did); i++)
> +			if (dev_id == iproc_pcie_corrupt_cap_did[i])
> +				pcie->fix_paxc_cap = true;

and I think this code will try to fix up every time config space is 
read.
Does this get corrupted often, randomly ?
Can it not be solved by using one time Quirk ?
and if not Quirk, you dont want to be setting pcie->fix_paxc_cap = false 
somewhere

besides, pcie->fix_paxc_cap = true; is set if PCI_VENDOR_ID is read 
first.
and rest cases stay with the assumption that PCI_VENDOR_ID will be read 
first.
which is infact read first during enumeration
(that is the assumption code is making), but that is safe assumption to 
make I think.

> +		break;
> +
> +	case IPROC_PCI_PM_CAP:
> +		if (pcie->fix_paxc_cap) {
> +			/* advertise PM, force next capability to PCIe */
> +			*val &= ~IPROC_PCI_PM_CAP_MASK;
> +			*val |= IPROC_PCI_EXP_CAP << 8 | PCI_CAP_ID_PM;
> +		}
> +		break;
> +
> +	case IPROC_PCI_EXP_CAP:
> +		if (pcie->fix_paxc_cap) {
> +			/* advertise root port, version 2, terminate here */
> +			*val = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2) << 16 |
> +				PCI_CAP_ID_EXP;
> +		}
> +		break;
> +
> +	case IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL:
> +		/* Don't advertise CRS SV support */
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +}
> +
>  static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int 
> devfn,
>  				  int where, int size, u32 *val)
>  {
> @@ -509,13 +565,10 @@ static int iproc_pcie_config_read(struct pci_bus
> *bus, unsigned int devfn,
>  	/* root complex access */
>  	if (busno == 0) {
>  		ret = pci_generic_config_read32(bus, devfn, where, size, val);
> -		if (ret != PCIBIOS_SUCCESSFUL)
> -			return ret;
> +		if (ret == PCIBIOS_SUCCESSFUL)
> +			iproc_pcie_fix_cap(pcie, where, val);
> 
> -		/* Don't advertise CRS SV support */
> -		if ((where & ~0x3) == IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL)
> -			*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> -		return PCIBIOS_SUCCESSFUL;
> +		return ret;
>  	}
> 
>  	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> diff --git a/drivers/pci/host/pcie-iproc.h 
> b/drivers/pci/host/pcie-iproc.h
> index 814b600..9d5cfee 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -60,6 +60,8 @@ struct iproc_msi;
>   * @ep_is_internal: indicates an internal emulated endpoint device is 
> connected
>   * @has_apb_err_disable: indicates the controller can be configured to 
> prevent
>   * unsupported request from being forwarded as an APB bus error
> + * @fix_paxc_cap: indicates the controller has corrupted capability 
> list in its
> + * config space registers and requires SW based fixup
>   *
>   * @need_ob_cfg: indicates SW needs to configure the outbound mapping 
> window
>   * @ob: outbound mapping related parameters
> @@ -85,6 +87,7 @@ struct iproc_pcie {
>  	int (*map_irq)(const struct pci_dev *, u8, u8);
>  	bool ep_is_internal;
>  	bool has_apb_err_disable;
> +	bool fix_paxc_cap;
> 
>  	bool need_ob_cfg;
>  	struct iproc_pcie_ob ob;