From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9765EC04EB8 for ; Fri, 30 Nov 2018 17:12:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 567BA20834 for ; Fri, 30 Nov 2018 17:12:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 567BA20834 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-pci-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726659AbeLAEWB (ORCPT ); Fri, 30 Nov 2018 23:22:01 -0500 Received: from foss.arm.com ([217.140.101.70]:60836 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726647AbeLAEWB (ORCPT ); Fri, 30 Nov 2018 23:22:01 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E8129EBD; Fri, 30 Nov 2018 09:12:02 -0800 (PST) Received: from e107981-ln.cambridge.arm.com (e107981-ln.cambridge.arm.com [10.1.197.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6D6423F5A0; Fri, 30 Nov 2018 09:12:01 -0800 (PST) Date: Fri, 30 Nov 2018 17:11:58 +0000 From: Lorenzo Pieralisi To: Stephen Warren Cc: Jingoo Han , Gustavo Pimentel , Bjorn Helgaas , linux-pci@vger.kernel.org, Vidya Sagar , Manikanta Maddireddy , Stephen Warren Subject: Re: [PATCH V2] PCI: dwc: Don't hard-code DBI/ATU offset Message-ID: <20181130171158.GB23332@e107981-ln.cambridge.arm.com> References: <20181120175750.26141-1-swarren@wwwdotorg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181120175750.26141-1-swarren@wwwdotorg.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Nov 20, 2018 at 10:57:50AM -0700, Stephen Warren wrote: > From: Stephen Warren > > 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 > Acked-by: Gustavo Pimentel > --- > 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 >