Hi Stephen,
I'd suggest to change the designware on the patch name by dwc and use another
description for the patch instead of don't hard-code DBI/ATU offset.
On 12/11/2018 22:57, Stephen Warren wrote:
> From: Stephen Warren <swarren@xxxxxxxxxx>
>
> 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.
>
If I understood this patch correctly, you basically replace the dbi_base offset
by atu_base offset that still depends of dbi_base offset. I agree the code is
now more readable, but I'm not seeing it by applying this patch what is the
benefit since you are not allowing to change the current atu_base. I though that
was the propose of this patch.
> 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@xxxxxxxxxx>
> ---
> .../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);
> +
> 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..02fb532c5db9 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)
I'd suggest to remove the parenthesis around the region variable, like this:
(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)
I'd suggest to remove the parenthesis around the region variable and to put a
parenthesis in the whole macro computation, like this:
((region << 9) | (0x1 << 8))
Regards,
Gustavo
>
> #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;
>
