Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, Nov 20, 2018 at 10:57:50AM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> The DWC PCIe core contains various separate register spaces: DBI, DBI2,
> ATU, DMA, etc. The relationship between the addresses of these register
> spaces is entirely determined by the implementation of the IP block, not
> by the IP block design itself. Hence, the DWC driver must not make
> assumptions that one register space can be accessed at a fixed offset from
> any other register space. To avoid such assumptions, introduce an
> explicit/separate register pointer for the ATU register space. In
> particular, the current assumption is not valid for NVIDIA's T194 SoC.
> 
> The ATU register space is only used on systems that require unrolled ATU
> access. This property is detected at run-time for host controllers, and
> when this is detected, this patch provides a default value for atu_base
> that matches the previous assumption re: register layout. An alternative
> would be to update all drivers for HW that requires unrolled access to
> explicitly set atu_base. However, it's hard to tell which drivers would
> require atu_base to be set. The unrolled property is not detected for
> endpoint systems, and so any endpoint driver that requires unrolled access
> must explicitly set the iatu_unroll_enabled flag (none do at present), and
> so a check is added to require the driver to also set atu_base while at
> it.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> v2:
> * Modified patch subject
> * Added missing outer brackets to PCIE_GET_ATU_INB_UNR_REG_OFFSET macro
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  4 ++++
>  .../pci/controller/dwc/pcie-designware-host.c |  3 +++
>  drivers/pci/controller/dwc/pcie-designware.c  |  8 ++++----
>  drivers/pci/controller/dwc/pcie-designware.h  | 20 +++++++++++++++----
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1e7b02221eac..880210366e71 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -504,6 +504,10 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
>  		return -EINVAL;
>  	}
> +	if (pci->iatu_unroll_enabled && !pci->atu_base) {
> +		dev_err(dev, "atu_base is not populated\n");
> +		return -EINVAL;
> +	}
>  
>  	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
>  	if (ret < 0) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 29a05759a294..2ebb7f4768cf 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -699,6 +699,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		dev_dbg(pci->dev, "iATU unroll: %s\n",
>  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
>  
> +		if (pci->iatu_unroll_enabled && !pci->atu_base)
> +			pci->atu_base = pci->dbi_base + (0x3 << 20);

Hi Stephen,

I think the patch is sound, minor nit, do you think that making

(0x3 << 20)

a preprocessor macro (and a comment on the default initialization above)
can help clarify the code ?

eg DEFAULT_ATU_BASE_OFFSET

or something like that.

Please let me know, I can merge the patch as-is, it is just to improve
readability.

Thanks,
Lorenzo

> +
>  		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>  					  pp->mem_bus_addr, pp->mem_size);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 2153956a0b20..93ef8c31fb39 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -93,7 +93,7 @@ static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
>  {
>  	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
>  
> -	return dw_pcie_readl_dbi(pci, offset + reg);
> +	return dw_pcie_readl_atu(pci, offset + reg);
>  }
>  
>  static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
> @@ -101,7 +101,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
>  {
>  	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
>  
> -	dw_pcie_writel_dbi(pci, offset + reg, val);
> +	dw_pcie_writel_atu(pci, offset + reg, val);
>  }
>  
>  static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
> @@ -187,7 +187,7 @@ static u32 dw_pcie_readl_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg)
>  {
>  	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
>  
> -	return dw_pcie_readl_dbi(pci, offset + reg);
> +	return dw_pcie_readl_atu(pci, offset + reg);
>  }
>  
>  static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
> @@ -195,7 +195,7 @@ static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg,
>  {
>  	u32 offset = PCIE_GET_ATU_INB_UNR_REG_OFFSET(index);
>  
> -	dw_pcie_writel_dbi(pci, offset + reg, val);
> +	dw_pcie_writel_atu(pci, offset + reg, val);
>  }
>  
>  static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 0989d880ac46..e895f37b1dee 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -93,11 +93,11 @@
>  #define PCIE_ATU_UNR_UPPER_TARGET	0x18
>  
>  /* Register address builder */
> -#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)	\
> -			((0x3 << 20) | ((region) << 9))
> +#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
> +		((region) << 9)
>  
> -#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region)				\
> -			((0x3 << 20) | ((region) << 9) | (0x1 << 8))
> +#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
> +		(((region) << 9) | (0x1 << 8))
>  
>  #define MAX_MSI_IRQS			256
>  #define MAX_MSI_IRQS_PER_CTRL		32
> @@ -219,6 +219,8 @@ struct dw_pcie {
>  	struct device		*dev;
>  	void __iomem		*dbi_base;
>  	void __iomem		*dbi_base2;
> +	/* Used when iatu_unroll_enabled is true */
> +	void __iomem		*atu_base;
>  	u32			num_viewport;
>  	u8			iatu_unroll_enabled;
>  	struct pcie_port	pp;
> @@ -289,6 +291,16 @@ static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg)
>  	return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4);
>  }
>  
> +static inline void dw_pcie_writel_atu(struct dw_pcie *pci, u32 reg, u32 val)
> +{
> +	__dw_pcie_write_dbi(pci, pci->atu_base, reg, 0x4, val);
> +}
> +
> +static inline u32 dw_pcie_readl_atu(struct dw_pcie *pci, u32 reg)
> +{
> +	return __dw_pcie_read_dbi(pci, pci->atu_base, reg, 0x4);
> +}
> +
>  static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci)
>  {
>  	u32 reg;
> -- 
> 2.19.1
>