Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Xiaowei Bao <xiaowei.bao@nxp.com>
To: Andrew Murray <andrew.murray@arm.com>
Cc: "jingoohan1@gmail.com" <jingoohan1@gmail.com>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	Leo Li <leoyang.li@nxp.com>, "kishon@ti.com" <kishon@ti.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"M.h. Lian" <minghuan.lian@nxp.com>, Roy Zang <roy.zang@nxp.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"Z.q. Hou" <zhiqiang.hou@nxp.com>
Subject: RE: [PATCH 01/10] PCI: designware-ep: Add multiple PFs support for DWC
Date: Fri, 16 Aug 2019 02:55:41 +0000
Message-ID: <AM5PR04MB3299396885AC4B54B9E8D0AEF5AF0@AM5PR04MB3299.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20190815113129.GF43882@e119886-lin.cambridge.arm.com>



> -----Original Message-----
> From: Andrew Murray <andrew.murray@arm.com>
> Sent: 2019Äê8ÔÂ15ÈÕ 19:32
> To: Xiaowei Bao <xiaowei.bao@nxp.com>
> Cc: jingoohan1@gmail.com; gustavo.pimentel@synopsys.com;
> bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> lorenzo.pieralisi@arm.com; arnd@arndb.de; gregkh@linuxfoundation.org;
> M.h. Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> Roy Zang <roy.zang@nxp.com>; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 01/10] PCI: designware-ep: Add multiple PFs support for
> DWC
> 
> On Thu, Aug 15, 2019 at 04:37:07PM +0800, Xiaowei Bao wrote:
> > Add multiple PFs support for DWC, different PF have different config
> > space, we use pf-offset property which get from the DTS to access the
> > different pF config space.
> 
> Thanks for the patch. I haven't seen a cover letter for this series, is there one
> missing?
Maybe I miss, I will add you to review next time, thanks a lot for your comments.
> 
> 
> >
> > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c |  97
> +++++++++++++---------
> >  drivers/pci/controller/dwc/pcie-designware.c    | 105
> ++++++++++++++++++++++--
> >  drivers/pci/controller/dwc/pcie-designware.h    |  10 ++-
> >  include/linux/pci-epc.h                         |   1 +
> >  4 files changed, 164 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 2bf5a35..75e2955 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -19,12 +19,14 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> >  	pci_epc_linkup(epc);
> >  }
> >
> > -static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno
> bar,
> > -				   int flags)
> > +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> > +				   enum pci_barno bar, int flags)
> >  {
> >  	u32 reg;
> > +	struct pci_epc *epc = pci->ep.epc;
> > +	u32 pf_base = func_no * epc->pf_offset;
> >
> > -	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > +	reg = pf_base + PCI_BASE_ADDRESS_0 + (4 * bar);
> 
> I think I'd rather see this arithmetic (and the one for determining pf_base)
> inside a macro or inline header function. This would make this code more
> readable and reduce the chances of an error by avoiding duplication of code.
> 
> For example look at cdns_pcie_ep_fn_writeb and
> ROCKCHIP_PCIE_EP_FUNC_BASE for examples of other EP drivers that do
> this.
Agree, this looks fine, thanks a lot for your comments, I will use this way to access
the registers in next version patch.
> 
> 
> >  	dw_pcie_dbi_ro_wr_en(pci);
> >  	dw_pcie_writel_dbi2(pci, reg, 0x0);
> >  	dw_pcie_writel_dbi(pci, reg, 0x0);
> > @@ -37,7 +39,12 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie
> > *pci, enum pci_barno bar,
> >
> >  void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)  {
> > -	__dw_pcie_ep_reset_bar(pci, bar, 0);
> > +	u8 func_no, funcs;
> > +
> > +	funcs = pci->ep.epc->max_functions;
> > +
> > +	for (func_no = 0; func_no < funcs; func_no++)
> > +		__dw_pcie_ep_reset_bar(pci, func_no, bar, 0);
> >  }
> >
> >  static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> > @@ -78,28 +85,29 @@ static int dw_pcie_ep_write_header(struct pci_epc
> > *epc, u8 func_no,  {
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	u32 pf_base = func_no * epc->pf_offset;
> >
> >  	dw_pcie_dbi_ro_wr_en(pci);
> > -	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, hdr->vendorid);
> > -	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, hdr->deviceid);
> > -	dw_pcie_writeb_dbi(pci, PCI_REVISION_ID, hdr->revid);
> > -	dw_pcie_writeb_dbi(pci, PCI_CLASS_PROG, hdr->progif_code);
> > -	dw_pcie_writew_dbi(pci, PCI_CLASS_DEVICE,
> > +	dw_pcie_writew_dbi(pci, pf_base + PCI_VENDOR_ID, hdr->vendorid);
> > +	dw_pcie_writew_dbi(pci, pf_base + PCI_DEVICE_ID, hdr->deviceid);
> > +	dw_pcie_writeb_dbi(pci, pf_base + PCI_REVISION_ID, hdr->revid);
> > +	dw_pcie_writeb_dbi(pci, pf_base + PCI_CLASS_PROG, hdr->progif_code);
> > +	dw_pcie_writew_dbi(pci, pf_base + PCI_CLASS_DEVICE,
> >  			   hdr->subclass_code | hdr->baseclass_code << 8);
> > -	dw_pcie_writeb_dbi(pci, PCI_CACHE_LINE_SIZE,
> > +	dw_pcie_writeb_dbi(pci, pf_base + PCI_CACHE_LINE_SIZE,
> >  			   hdr->cache_line_size);
> > -	dw_pcie_writew_dbi(pci, PCI_SUBSYSTEM_VENDOR_ID,
> > +	dw_pcie_writew_dbi(pci, pf_base + PCI_SUBSYSTEM_VENDOR_ID,
> >  			   hdr->subsys_vendor_id);
> > -	dw_pcie_writew_dbi(pci, PCI_SUBSYSTEM_ID, hdr->subsys_id);
> > -	dw_pcie_writeb_dbi(pci, PCI_INTERRUPT_PIN,
> > +	dw_pcie_writew_dbi(pci, pf_base + PCI_SUBSYSTEM_ID,
> hdr->subsys_id);
> > +	dw_pcie_writeb_dbi(pci, pf_base + PCI_INTERRUPT_PIN,
> >  			   hdr->interrupt_pin);
> >  	dw_pcie_dbi_ro_wr_dis(pci);
> >
> >  	return 0;
> >  }
> >
> > -static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum
> pci_barno bar,
> > -				  dma_addr_t cpu_addr,
> > +static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > +				  enum pci_barno bar, dma_addr_t cpu_addr,
> >  				  enum dw_pcie_as_type as_type)
> >  {
> >  	int ret;
> > @@ -112,7 +120,7 @@ static int dw_pcie_ep_inbound_atu(struct
> dw_pcie_ep *ep, enum pci_barno bar,
> >  		return -EINVAL;
> >  	}
> >
> > -	ret = dw_pcie_prog_inbound_atu(pci, free_win, bar, cpu_addr,
> > +	ret = dw_pcie_prog_inbound_atu(pci, func_no, free_win, bar,
> > +cpu_addr,
> >  				       as_type);
> >  	if (ret < 0) {
> >  		dev_err(pci->dev, "Failed to program IB window\n"); @@ -125,7
> > +133,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep,
> enum pci_barno bar,
> >  	return 0;
> >  }
> >
> > -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t
> > phys_addr,
> > +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > +				   phys_addr_t phys_addr,
> >  				   u64 pci_addr, size_t size)
> >  {
> >  	u32 free_win;
> > @@ -137,8 +146,8 @@ static int dw_pcie_ep_outbound_atu(struct
> dw_pcie_ep *ep, phys_addr_t phys_addr,
> >  		return -EINVAL;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM,
> > -				  phys_addr, pci_addr, size);
> > +	dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win,
> PCIE_ATU_TYPE_MEM,
> > +				     phys_addr, pci_addr, size);
> >
> >  	set_bit(free_win, ep->ob_window_map);
> >  	ep->outbound_addr[free_win] = phys_addr; @@ -154,7 +163,7 @@
> static
> > void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
> >  	enum pci_barno bar = epf_bar->barno;
> >  	u32 atu_index = ep->bar_to_atu[bar];
> >
> > -	__dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
> > +	__dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
> >
> >  	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
> >  	clear_bit(atu_index, ep->ib_window_map); @@ -170,14 +179,16 @@
> > static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> >  	size_t size = epf_bar->size;
> >  	int flags = epf_bar->flags;
> >  	enum dw_pcie_as_type as_type;
> > -	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > +	u32 pf_base = func_no * epc->pf_offset;
> > +	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar) + pf_base;
> >
> >  	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> >  		as_type = DW_PCIE_AS_MEM;
> >  	else
> >  		as_type = DW_PCIE_AS_IO;
> >
> > -	ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
> > +	ret = dw_pcie_ep_inbound_atu(ep, func_no, bar,
> > +				     epf_bar->phys_addr, as_type);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -235,7 +246,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc
> *epc, u8 func_no,
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >
> > -	ret = dw_pcie_ep_outbound_atu(ep, addr, pci_addr, size);
> > +	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> >  	if (ret) {
> >  		dev_err(pci->dev, "Failed to enable address\n");
> >  		return ret;
> > @@ -248,12 +259,13 @@ static int dw_pcie_ep_get_msi(struct pci_epc
> > *epc, u8 func_no)  {
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	u32 pf_base = func_no * epc->pf_offset;
> >  	u32 val, reg;
> >
> >  	if (!ep->msi_cap)
> >  		return -EINVAL;
> >
> > -	reg = ep->msi_cap + PCI_MSI_FLAGS;
> > +	reg = ep->msi_cap + pf_base + PCI_MSI_FLAGS;
> >  	val = dw_pcie_readw_dbi(pci, reg);
> >  	if (!(val & PCI_MSI_FLAGS_ENABLE))
> >  		return -EINVAL;
> > @@ -267,12 +279,13 @@ static int dw_pcie_ep_set_msi(struct pci_epc
> > *epc, u8 func_no, u8 interrupts)  {
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	u32 pf_base = func_no * epc->pf_offset;
> >  	u32 val, reg;
> >
> >  	if (!ep->msi_cap)
> >  		return -EINVAL;
> >
> > -	reg = ep->msi_cap + PCI_MSI_FLAGS;
> > +	reg = ep->msi_cap + pf_base + PCI_MSI_FLAGS;
> >  	val = dw_pcie_readw_dbi(pci, reg);
> >  	val &= ~PCI_MSI_FLAGS_QMASK;
> >  	val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK; @@ -287,12 +300,13
> > @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no)  {
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	u32 pf_base = func_no * epc->pf_offset;
> >  	u32 val, reg;
> >
> >  	if (!ep->msix_cap)
> >  		return -EINVAL;
> >
> > -	reg = ep->msix_cap + PCI_MSIX_FLAGS;
> > +	reg = ep->msix_cap + pf_base + PCI_MSIX_FLAGS;
> >  	val = dw_pcie_readw_dbi(pci, reg);
> >  	if (!(val & PCI_MSIX_FLAGS_ENABLE))
> >  		return -EINVAL;
> > @@ -306,12 +320,13 @@ static int dw_pcie_ep_set_msix(struct pci_epc
> > *epc, u8 func_no, u16 interrupts)  {
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	u32 pf_base = func_no * epc->pf_offset;
> >  	u32 val, reg;
> >
> >  	if (!ep->msix_cap)
> >  		return -EINVAL;
> >
> > -	reg = ep->msix_cap + PCI_MSIX_FLAGS;
> > +	reg = ep->msix_cap + pf_base + PCI_MSIX_FLAGS;
> >  	val = dw_pcie_readw_dbi(pci, reg);
> >  	val &= ~PCI_MSIX_FLAGS_QSIZE;
> >  	val |= interrupts;
> > @@ -400,6 +415,7 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep
> *ep, u8 func_no,
> >  	unsigned int aligned_offset;
> >  	u16 msg_ctrl, msg_data;
> >  	u32 msg_addr_lower, msg_addr_upper, reg;
> > +	u32 pf_base = func_no * epc->pf_offset;
> >  	u64 msg_addr;
> >  	bool has_upper;
> >  	int ret;
> > @@ -408,19 +424,19 @@ int dw_pcie_ep_raise_msi_irq(struct
> dw_pcie_ep *ep, u8 func_no,
> >  		return -EINVAL;
> >
> >  	/* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
> > -	reg = ep->msi_cap + PCI_MSI_FLAGS;
> > +	reg = ep->msi_cap + pf_base + PCI_MSI_FLAGS;
> >  	msg_ctrl = dw_pcie_readw_dbi(pci, reg);
> >  	has_upper = !!(msg_ctrl & PCI_MSI_FLAGS_64BIT);
> > -	reg = ep->msi_cap + PCI_MSI_ADDRESS_LO;
> > +	reg = ep->msi_cap + pf_base + PCI_MSI_ADDRESS_LO;
> >  	msg_addr_lower = dw_pcie_readl_dbi(pci, reg);
> >  	if (has_upper) {
> > -		reg = ep->msi_cap + PCI_MSI_ADDRESS_HI;
> > +		reg = ep->msi_cap + pf_base + PCI_MSI_ADDRESS_HI;
> >  		msg_addr_upper = dw_pcie_readl_dbi(pci, reg);
> > -		reg = ep->msi_cap + PCI_MSI_DATA_64;
> > +		reg = ep->msi_cap + pf_base + PCI_MSI_DATA_64;
> >  		msg_data = dw_pcie_readw_dbi(pci, reg);
> >  	} else {
> >  		msg_addr_upper = 0;
> > -		reg = ep->msi_cap + PCI_MSI_DATA_32;
> > +		reg = ep->msi_cap + pf_base + PCI_MSI_DATA_32;
> >  		msg_data = dw_pcie_readw_dbi(pci, reg);
> >  	}
> >  	aligned_offset = msg_addr_lower & (epc->mem->page_size - 1); @@
> > -439,7 +455,7 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
> > u8 func_no,  }
> >
> >  int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> > -			     u16 interrupt_num)
> > +			      u16 interrupt_num)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  	struct pci_epc *epc = ep->epc;
> > @@ -447,16 +463,17 @@ int dw_pcie_ep_raise_msix_irq(struct
> dw_pcie_ep *ep, u8 func_no,
> >  	u32 bar_addr_upper, bar_addr_lower;
> >  	u32 msg_addr_upper, msg_addr_lower;
> >  	u32 reg, msg_data, vec_ctrl;
> > +	u32 pf_base = func_no * epc->pf_offset;
> >  	u64 tbl_addr, msg_addr, reg_u64;
> >  	void __iomem *msix_tbl;
> >  	int ret;
> >
> > -	reg = ep->msix_cap + PCI_MSIX_TABLE;
> > +	reg = ep->msix_cap + pf_base + PCI_MSIX_TABLE;
> >  	tbl_offset = dw_pcie_readl_dbi(pci, reg);
> >  	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> >  	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
> >
> > -	reg = PCI_BASE_ADDRESS_0 + (4 * bir);
> > +	reg = PCI_BASE_ADDRESS_0 + pf_base + (4 * bir);
> >  	bar_addr_upper = 0;
> >  	bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
> >  	reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
> @@
> > -592,13 +609,17 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  	ep->epc = epc;
> >  	epc_set_drvdata(epc, ep);
> >
> > -	if (ep->ops->ep_init)
> > -		ep->ops->ep_init(ep);
> > -
> >  	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
> >  	if (ret < 0)
> >  		epc->max_functions = 1;
> >
> > +	ret = of_property_read_u32(np, "pf-offset", &epc->pf_offset);
> > +	if (ret < 0)
> > +		epc->pf_offset = 0;
> 
> Bad things will likely happen if max_functions > 1 and pf-offset isn't set.
> I think the driver should bail in this situation. It would be very easy for
> someone to misconfigure this.
Yes, you are right, but if the max-functions have defined in DTS, require the pf-offset
must define in DTS, I am not sure the correct value of pf-offsetfor other platforms, 
so I think the max-functions and pf-offset should not have the dependence.
even though I didn't define pf-offset when I defined max-functions, the pf-offset is 0, 
the DWC ep driver can continue run the progress of INIT but not return, of course, 
thus the PF1 will not work, I don't know which way is better.
> 
> 
> > +
> > +	if (ep->ops->ep_init)
> > +		ep->ops->ep_init(ep);
> > +
> >  	ret = __pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
> >  				 ep->page_size);
> >  	if (ret < 0) {
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > b/drivers/pci/controller/dwc/pcie-designware.c
> > index 7d25102..c99cee4 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -158,6 +158,43 @@ static void dw_pcie_writel_ob_unroll(struct
> dw_pcie *pci, u32 index, u32 reg,
> >  	dw_pcie_writel_atu(pci, offset + reg, val);  }
> >
> > +static void dw_pcie_prog_ep_outbound_atu_unroll(struct dw_pcie *pci, u8
> func_no,
> > +						int index, int type,
> > +						u64 cpu_addr, u64 pci_addr,
> > +						u32 size)
> > +{
> > +	u32 retries, val;
> > +
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_BASE,
> > +				 lower_32_bits(cpu_addr));
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_BASE,
> > +				 upper_32_bits(cpu_addr));
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LIMIT,
> > +				 lower_32_bits(cpu_addr + size - 1));
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_TARGET,
> > +				 lower_32_bits(pci_addr));
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
> > +				 upper_32_bits(pci_addr));
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
> > +				 type | PCIE_ATU_FUNC_NUM(func_no));
> 
> With the exception of this line, the rest of this function is identical to
> dw_pcie_prog_outbound_atu_unroll.
Yes, I can integrate the same code, but I think we'd better use the different outbound 
window set function between RC and EP, because the RC don't need the func_num parameter.
> 
> > +	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> > +				 PCIE_ATU_ENABLE);
> > +
> > +	/*
> > +	 * Make sure ATU enable takes effect before any subsequent config
> > +	 * and I/O accesses.
> > +	 */
> > +	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > +		val = dw_pcie_readl_ob_unroll(pci, index,
> > +					      PCIE_ATU_UNR_REGION_CTRL2);
> > +		if (val & PCIE_ATU_ENABLE)
> > +			return;
> > +
> > +		mdelay(LINK_WAIT_IATU);
> > +	}
> > +	dev_err(pci->dev, "Outbound iATU is not being enabled\n"); }
> > +
> >  static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int
> index,
> >  					     int type, u64 cpu_addr,
> >  					     u64 pci_addr, u32 size)
> > @@ -194,6 +231,51 @@ static void
> dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
> >  	dev_err(pci->dev, "Outbound iATU is not being enabled\n");  }
> >
> > +void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int
> index,
> > +				  int type, u64 cpu_addr, u64 pci_addr,
> > +				  u32 size)
> > +{
> > +	u32 retries, val;
> > +
> > +	if (pci->ops->cpu_addr_fixup)
> > +		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > +
> > +	if (pci->iatu_unroll_enabled) {
> > +		dw_pcie_prog_ep_outbound_atu_unroll(pci, func_no, index, type,
> > +						    cpu_addr, pci_addr, size);
> > +		return;
> > +	}
> > +
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT,
> > +			   PCIE_ATU_REGION_OUTBOUND | index);
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_BASE,
> > +			   lower_32_bits(cpu_addr));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_BASE,
> > +			   upper_32_bits(cpu_addr));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
> > +			   lower_32_bits(cpu_addr + size - 1));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
> > +			   lower_32_bits(pci_addr));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
> > +			   upper_32_bits(pci_addr));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
> > +			   PCIE_ATU_FUNC_NUM(func_no));
> 
> The same here, this is identical to dw_pcie_prog_outbound_atu with the
> exception of this line.
> 
> Is there a way you can avoid all of this duplicated code?
As above, I can integrate the same code, but I keep to think the different outbound 
Window set function should be used between RC and EP.
> 
> Thanks,
> 
> Andrew Murray
> 
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
> > +
> > +	/*
> > +	 * Make sure ATU enable takes effect before any subsequent config
> > +	 * and I/O accesses.
> > +	 */
> > +	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > +		val = dw_pcie_readl_dbi(pci, PCIE_ATU_CR2);
> > +		if (val & PCIE_ATU_ENABLE)
> > +			return;
> > +
> > +		mdelay(LINK_WAIT_IATU);
> > +	}
> > +	dev_err(pci->dev, "Outbound iATU is not being enabled\n"); }
> > +
> >  void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> >  			       u64 cpu_addr, u64 pci_addr, u32 size)  { @@ -252,8
> +334,8
> > @@ static void dw_pcie_writel_ib_unroll(struct dw_pcie *pci, u32 index,
> u32 reg,
> >  	dw_pcie_writel_atu(pci, offset + reg, val);  }
> >
> > -static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
> > -					   int bar, u64 cpu_addr,
> > +static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, u8
> func_no,
> > +					   int index, int bar, u64 cpu_addr,
> >  					   enum dw_pcie_as_type as_type)  {
> >  	int type;
> > @@ -275,8 +357,10 @@ static int dw_pcie_prog_inbound_atu_unroll(struct
> dw_pcie *pci, int index,
> >  		return -EINVAL;
> >  	}
> >
> > -	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
> type);
> > +	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
> type |
> > +				 PCIE_ATU_FUNC_NUM(func_no));
> >  	dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> > +				 PCIE_ATU_FUNC_NUM_MATCH_EN |
> >  				 PCIE_ATU_ENABLE |
> >  				 PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
> >
> > @@ -297,14 +381,15 @@ static int
> dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
> >  	return -EBUSY;
> >  }
> >
> > -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
> > -			     u64 cpu_addr, enum dw_pcie_as_type as_type)
> > +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > +			     int bar, u64 cpu_addr,
> > +			     enum dw_pcie_as_type as_type)
> >  {
> >  	int type;
> >  	u32 retries, val;
> >
> >  	if (pci->iatu_unroll_enabled)
> > -		return dw_pcie_prog_inbound_atu_unroll(pci, index, bar,
> > +		return dw_pcie_prog_inbound_atu_unroll(pci, func_no, index, bar,
> >  						       cpu_addr, as_type);
> >
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT,
> PCIE_ATU_REGION_INBOUND |
> > @@ -323,9 +408,11 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie
> *pci, int index, int bar,
> >  		return -EINVAL;
> >  	}
> >
> > -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type);
> > -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE
> > -			   | PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
> > +			   PCIE_ATU_FUNC_NUM(func_no));
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE |
> > +			   PCIE_ATU_FUNC_NUM_MATCH_EN |
> > +			   PCIE_ATU_BAR_MODE_ENABLE | (bar << 8));
> >
> >  	/*
> >  	 * Make sure ATU enable takes effect before any subsequent config
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index ffed084..2b291e8 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -71,9 +71,11 @@
> >  #define PCIE_ATU_TYPE_IO		0x2
> >  #define PCIE_ATU_TYPE_CFG0		0x4
> >  #define PCIE_ATU_TYPE_CFG1		0x5
> > +#define PCIE_ATU_FUNC_NUM(pf)           (pf << 20)
> >  #define PCIE_ATU_CR2			0x908
> >  #define PCIE_ATU_ENABLE			BIT(31)
> >  #define PCIE_ATU_BAR_MODE_ENABLE	BIT(30)
> > +#define PCIE_ATU_FUNC_NUM_MATCH_EN      BIT(19)
> >  #define PCIE_ATU_LOWER_BASE		0x90C
> >  #define PCIE_ATU_UPPER_BASE		0x910
> >  #define PCIE_ATU_LIMIT			0x914
> > @@ -265,8 +267,12 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
> >  			       int type, u64 cpu_addr, u64 pci_addr,
> >  			       u32 size);
> > -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
> > -			     u64 cpu_addr, enum dw_pcie_as_type as_type);
> > +void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int
> index,
> > +				  int type, u64 cpu_addr, u64 pci_addr,
> > +				  u32 size);
> > +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > +			     int bar, u64 cpu_addr,
> > +			     enum dw_pcie_as_type as_type);
> >  void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> >  			 enum dw_pcie_region_type type);
> >  void dw_pcie_setup(struct dw_pcie *pci); diff --git
> > a/include/linux/pci-epc.h b/include/linux/pci-epc.h index
> > f641bad..fc2feee 100644
> > --- a/include/linux/pci-epc.h
> > +++ b/include/linux/pci-epc.h
> > @@ -96,6 +96,7 @@ struct pci_epc {
> >  	const struct pci_epc_ops	*ops;
> >  	struct pci_epc_mem		*mem;
> >  	u8				max_functions;
> > +	u32				pf_offset;
> >  	struct config_group		*group;
> >  	/* spinlock to protect against concurrent access of EP controller */
> >  	spinlock_t			lock;
> > --
> > 2.9.5
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=02%
> 7C0
> >
> 1%7Cxiaowei.bao%40nxp.com%7C0e39168f6f144db6840308d721742040%7
> C686ea1d
> >
> 3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637014654998524452&amp;sd
> ata=bP7eh
> > cjlGXCMVFE2b4f12Q6fGV7lQ%2F5i9qIi9FoPlbI%3D&amp;reserved=0

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15  8:37 Xiaowei Bao
2019-08-15  8:37 ` [PATCH 02/10] PCI: designware-ep: Add the doorbell mode of MSI-X in EP mode Xiaowei Bao
2019-08-15 11:53   ` Andrew Murray
2019-08-16  2:58     ` Xiaowei Bao
2019-08-16 10:20       ` Andrew Murray
2019-08-16 11:01         ` Xiaowei Bao
2019-08-16 10:49       ` Kishon Vijay Abraham I
2019-08-16 11:14         ` Xiaowei Bao
2019-08-15  8:37 ` [PATCH 03/10] PCI: designware-ep: Move the function of getting MSI capability forward Xiaowei Bao
2019-08-15  8:37 ` [PATCH 04/10] dt-bindings: pci: layerscape-pci: add compatible strings for ls1088a and ls2088a Xiaowei Bao
2019-08-15  8:37 ` [PATCH 05/10] PCI: layerscape: Modify the way of getting capability with different PEX Xiaowei Bao
2019-08-15 12:51   ` Andrew Murray
2019-08-16  3:00     ` Xiaowei Bao
2019-08-16 10:25       ` Andrew Murray
2019-08-16 11:03         ` Xiaowei Bao
2019-08-15  8:37 ` [PATCH 06/10] PCI: layerscape: Modify the MSIX to the doorbell way Xiaowei Bao
2019-08-15  8:37 ` [PATCH 07/10] PCI: layerscape: Fix some format issue of the code Xiaowei Bao
2019-08-15  8:37 ` [PATCH 08/10] dt-bindings: PCI: Add the pf-offset property Xiaowei Bao
2019-08-27 16:26   ` Rob Herring
2019-08-15  8:37 ` [PATCH 09/10] arm64: dts: layerscape: Add PCIe EP node for ls1088a Xiaowei Bao
2019-08-15  8:37 ` [PATCH 10/10] misc: pci_endpoint_test: Add LS1088a in pci_device_id table Xiaowei Bao
2019-08-15 11:31 ` [PATCH 01/10] PCI: designware-ep: Add multiple PFs support for DWC Andrew Murray
2019-08-16  2:55   ` Xiaowei Bao [this message]
2019-08-16  9:44     ` Andrew Murray
2019-08-16 11:00       ` Xiaowei Bao
2019-08-16 12:35         ` Andrew Murray
2019-08-16 15:11           ` Xiaowei Bao

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM5PR04MB3299396885AC4B54B9E8D0AEF5AF0@AM5PR04MB3299.eurprd04.prod.outlook.com \
    --to=xiaowei.bao@nxp.com \
    --cc=andrew.murray@arm.com \
    --cc=arnd@arndb.de \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=jingoohan1@gmail.com \
    --cc=kishon@ti.com \
    --cc=leoyang.li@nxp.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=minghuan.lian@nxp.com \
    --cc=roy.zang@nxp.com \
    --cc=shawnguo@kernel.org \
    --cc=zhiqiang.hou@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox