RE: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lorenzo,

Thanks a lot for your comments!

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Sent: 2018年11月29日 2:00
> To: Z.q. Hou <zhiqiang.hou@xxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; jingoohan1@xxxxxxxxx;
> gustavo.pimentel@xxxxxxxxxxxx; Roy Zang <roy.zang@xxxxxxx>; Mingkai Hu
> <mingkai.hu@xxxxxxx>; M.h. Lian <minghuan.lian@xxxxxxx>
> Subject: Re: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
> 
> On Wed, Nov 07, 2018 at 10:09:21AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@xxxxxxx>
> >
> > The current code only support non-prefetchable memory range, as the
> > non-prefetchable memory range must not be greater than 4GiB, one
> > viewport can cover it, which supports upto 4GiB.
> >
> > To support prefetchable memory range, which is upto 64-bit memory
> > space and can be greater than 4GiB, so we need multiple viewports. And
> > added separate vars to store prefetchable memory range info to prevent
> > overriding the non-prefetchable memory range info.
> >
> > And this patch explicitly assigned the last (if there are only
> > 2 viewports) or last 2 viewports for CFG and I/O windows and the rests
> > for MEM windows.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@xxxxxxx>
> > ---
> > V2:
> >  - Reworded the subject and commit description.
> >  - Fix the prefetchable memory range overriding non-perfetchable
> >    memory range issue by adding vars to store prefetchable memory
> >    range info.
> >
> >  .../pci/controller/dwc/pcie-designware-host.c | 107 ++++++++++++++----
> >  drivers/pci/controller/dwc/pcie-designware.h  |   7 ++
> >  2 files changed, 95 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index ecacce016489..328aa40a6609 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -346,6 +346,35 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		dev_err(dev, "Missing *config* reg space\n");
> >  	}
> >
> > +	/*
> > +	 * If vendor's platform driver has set the num_viewport and it is
> > +	 * not less than 2, skip getting the num_viewport from DT here.
> 
> That's not what the device tree bindings specify. If DT does contain the
> property you *must* read it.

Yes, will change back to *must* read.

> 
> > +	 */
> > +	if (pci->num_viewport < 2) {
> > +		ret = of_property_read_u32(np, "num-viewport",
> > +					   &pci->num_viewport);
> > +		if (ret || pci->num_viewport < 2)
> > +			pci->num_viewport = 2;
> > +	}
> > +
> > +	/*
> > +	 * if there are only 2 viewports, assign the last viewport for
> > +	 * both CFG and IO window, otherwise assign the last 2 viewport
> 
> Gah. Can anyone explain to me how this driver works if only two viewports
> are available ? What happens if an IO access happens at the same time of a
> config access (that fiddles with the outbound memory windows) ?

You are right it has potential IO transaction drop issue, but DWC PCIe databook does give a default value 2 of viewport number.

 
> > +	 * for CFG and IO window specific. And the rest viewports are
> > +	 * assigned to MEM windows.
> > +	 */
> > +	if (pci->num_viewport == 2) {
> > +		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
> > +		pp->mem_wins = 1;
> > +	} else {
> > +		pp->cfg_idx = pci->num_viewport - 1;
> > +		pp->io_idx = pci->num_viewport - 2;
> > +		pp->mem_wins = pci->num_viewport - 2;
> > +	}
> > +
> > +	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
> > +		pp->cfg_idx, pp->io_idx, pp->mem_wins);
> > +
> >  	bridge = pci_alloc_host_bridge(0);
> >  	if (!bridge)
> >  		return -ENOMEM;
> > @@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  			}
> >  			break;
> >  		case IORESOURCE_MEM:
> > -			pp->mem = win->res;
> > -			pp->mem->name = "MEM";
> > -			pp->mem_size = resource_size(pp->mem);
> > -			pp->mem_bus_addr = pp->mem->start - win->offset;
> > +			if (win->res->flags & IORESOURCE_PREFETCH) {
> > +				pp->mem_perf = win->res;
> > +				pp->mem_perf->name = "MEM perf";
> 
> Nit: Why "perf" and not "pref" ?
> 
> It is confusing but that's the least of this patch problems.

My bad, it is typo, will fix in next version.

> 
> > +				pp->mem_perf_size = resource_size(pp->mem_perf);
> > +				pp->mem_perf_bus_addr = pp->mem_perf->start -
> > +							win->offset;
> > +				pp->mem_perf_base = pp->mem_perf->start;
> > +			} else {
> > +				pp->mem = win->res;
> > +				pp->mem->name = "MEM";
> > +				pp->mem_size = resource_size(pp->mem);
> > +				pp->mem_bus_addr = pp->mem->start - win->offset;
> > +				pp->mem_base = pp->mem->start;
> > +			}
> >  			break;
> >  		case 0:
> >  			pp->cfg = win->res;
> > @@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		}
> >  	}
> >
> > -	pp->mem_base = pp->mem->start;
> > -
> >  	if (!pp->va_cfg0_base) {
> >  		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
> >  					pp->cfg0_base, pp->cfg0_size);
> > @@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct
> pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	ret = dw_pcie_read(va_cfg_base + where, size, val);
> > -	if (pci->num_viewport <= 2)
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pp->cfg_idx == pp->io_idx)
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> >  					  pp->io_bus_addr, pp->io_size);
> 
> See above, even though this is not related to this patch.

I think it is more reasonable to check if the CFG and IO shared viewport than the viewport number.

> 
> >
> > @@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct
> pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	ret = dw_pcie_write(va_cfg_base + where, size, val);
> > -	if (pci->num_viewport <= 2)
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pp->cfg_idx == pp->io_idx)
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> >  					  pp->io_bus_addr, pp->io_size);
> >
> > @@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct
> > dw_pcie *pci)  void dw_pcie_setup_rc(struct pcie_port *pp)  {
> >  	u32 val, ctrl, num_ctrls;
> > +	u64 remain_size, base, win_size;
> > +	phys_addr_t bus_addr;
> > +	int i;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> >  	dw_pcie_setup(pci);
> > @@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >  		dev_dbg(pci->dev, "iATU unroll: %s\n",
> >  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
> >
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> > -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> > -					  pp->mem_bus_addr, pp->mem_size);
> > -		if (pci->num_viewport > 2)
> > -			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
> > -						  PCIE_ATU_TYPE_IO, pp->io_base,
> > -						  pp->io_bus_addr, pp->io_size);
> > +		/*
> > +		 * The maximum region size is 4 GB, and a region
> > +		 * must not cross a 4 GB boundary.
> > +		 */
> > +		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
> > +		win_size = min(win_size, pp->mem_size);
> > +		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
> > +					  pp->mem_base, pp->mem_bus_addr,
> > +					  win_size);
> > +		dev_dbg(pci->dev,
> > +			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa,
> size = %llx\n",
> > +			0, pp->mem_base, &pp->mem_bus_addr, win_size);
> > +
> > +		/* Prefetchable range can be 64bit space */
> > +		remain_size = pp->mem_perf_size;
> 
> unallocated/free_size ?

Sorry, I don't understand your question.

> 
> > +		base = pp->mem_perf_base;
> > +		bus_addr = pp->mem_perf_bus_addr;
> > +		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
> > +			win_size = SZ_4G - (base & (SZ_4G - 1));
> > +			win_size = min(win_size, remain_size);
> > +			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > +						  base, bus_addr, win_size);
> > +			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx,
> bus_addr = %pa, size = %llx\n",
> > +				i, base, &bus_addr, win_size);
> > +
> > +			base += win_size;
> > +			bus_addr += win_size;
> > +			remain_size -= win_size;
> > +		}
> > +
> > +		if (remain_size > 0)
> > +			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
> > +
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
> > +					  pp->io_base, pp->io_bus_addr,
> > +					  pp->io_size);
> >  	}
> >
> >  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); diff --git
> > a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index a438c3879aa9..0197f67f82b7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -148,15 +148,22 @@ struct pcie_port {
> >  	u64			cfg1_base;
> >  	void __iomem		*va_cfg1_base;
> >  	u32			cfg1_size;
> > +	u32			cfg_idx;
> >  	resource_size_t		io_base;
> >  	phys_addr_t		io_bus_addr;
> >  	u32			io_size;
> > +	u32			io_idx;
> >  	u64			mem_base;
> >  	phys_addr_t		mem_bus_addr;
> >  	u64			mem_size;
> > +	u64			mem_perf_base;
> 
> This is a phys_addr_t
> 
> > +	phys_addr_t		mem_perf_bus_addr;
> 
> pci_bus_addr_t ?

Will fix in next version.

> 
> Lorenzo
> 
> > +	u64			mem_perf_size;
> > +	u32			mem_wins;
> >  	struct resource		*cfg;
> >  	struct resource		*io;
> >  	struct resource		*mem;
> > +	struct resource		*mem_perf;
> >  	struct resource		*busn;
> >  	int			irq;
> >  	const struct dw_pcie_host_ops *ops;
> > --
> > 2.17.1
> >

Thanks,
Zhiqiang




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux