Re: DesignWare ATU PCIE_ATU_TYPE_MEM usage

[Jisheng Zhang <jszhang@marvell.com>]

Dear Minghuan, Bjorn,

On Thu, 7 Jan 2016 07:11:59 +0000 Minghuan Lian wrote:

> Hi Bjorn,
> 
> We need at least four kinds of PCIe transactions: CFG0, CFG1, IO, MEM.
> The best solution is to assign 4 iATU to support them separately for example: ATU0- CFG0, ATU1-CFG1, ATU2-IO ATU3-MEM.

IMHO, 3 ATUs is enough, cfg0 and cfg1 can be sequenced by pci_lock as you
pointed out ;)

> So, for IO and MEM, we don't need to update iATU and for CFG, we only need to update a register to update BUS device and function number.
> Moreover, this can avoid any conflict.
> 
> But There are some SOCs that only provides two iATUs.
> So the latest driver provides a solution: ATU0 is shared by CFG0 CFG1 and IO, ATU1 is dedicated to MEM.
> The driver should first set ATU0 as IO and ATU1 as MEM. When accessing CFG0 or CFG1, ATU0 will be changed to CFG0/CFG1, then back to IO setting.
> CFG0 CFG1 can sequenced by pci_lock. But CFG IO and MEM(may be generated by PCIe device's DMA) are completely independent.
> 
> Because most PCIe device do not need IO transaction and in general CFG0 CFG1 and IO transaction is generated by PCIe device driver and rarely occurred simultaneously.
> So the current solution can minimize conflict.
> In fact, for the current pcie-designware driver, there is no way to avoid IO and CFG conflict.

The key is how to sequence IO and CFG, this may need pci subsystem to provide
the support.

> 
> 
> Thanks,
> Minghuan
> 
> 
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> > Sent: Wednesday, January 06, 2016 6:42 AM
> > To: Jingoo Han <jingoohan1@gmail.com>; Pratyush Anand
> > <pratyush.anand@gmail.com>
> > Cc: linux-pci@vger.kernel.org; Jisheng Zhang <jszhang@marvell.com>;
> > Minghuan.Lian@freescale.com
> > Subject: DesignWare ATU PCIE_ATU_TYPE_MEM usage
> > 
> > Hi guys,
> > 
> > This code in dw_pcie_host_init() looks wrong:
> > 
> >     if (!pp->ops->rd_other_conf)
> > 	    dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> > 				      PCIE_ATU_TYPE_MEM, pp->mem_base,
> > 				      pp->mem_bus_addr, pp->mem_size);
> > 
> >     dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> >     ...
> >     dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
> > 
> > Evidently you need to program the ATU with PCIE_ATU_TYPE_MEM before
> > doing config accesses on the root bus?
> > 
> > If that's the case, what about other config accesses after these few in
> > dw_pcie_host_init()?  I don't see anything that changes the ATU programming
> > for things coming through dw_pcie_wr_conf().
> > 
> > I assume you need some sort of locking around this sequence:
> > 
> >     dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, type, ...
> >     ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
> >     dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> > PCIE_ATU_TYPE_IO, ...
> > 
> > Are you relying on pci_lock?  If so, a comment to that effect would be nice.
> > 
> > The whole ATU management looks pretty inefficient.  It's likely that there'll be
> > a whole sequence of operations where we don't need to touch the ATU, but
> > since we don't remember its current state, we configure the whole thing from
> > scratch each time, which is at least eight register writes (plus a read for the
> > new synchronization).
> > 
> > Bjorn