DesignWare ATU PCIE_ATU_TYPE_MEM usage

DesignWare ATU PCIE_ATU_TYPE_MEM usage

[Bjorn Helgaas]

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

RE: DesignWare ATU PCIE_ATU_TYPE_MEM usage

[Minghuan Lian]

SGkgQmpvcm4sDQoNCldlIG5lZWQgYXQgbGVhc3QgZm91ciBraW5kcyBvZiBQQ0llIHRyYW5zYWN0
aW9uczogQ0ZHMCwgQ0ZHMSwgSU8sIE1FTS4NClRoZSBiZXN0IHNvbHV0aW9uIGlzIHRvIGFzc2ln
biA0IGlBVFUgdG8gc3VwcG9ydCB0aGVtIHNlcGFyYXRlbHkgZm9yIGV4YW1wbGU6IEFUVTAtIENG
RzAsIEFUVTEtQ0ZHMSwgQVRVMi1JTyBBVFUzLU1FTS4NClNvLCBmb3IgSU8gYW5kIE1FTSwgd2Ug
ZG9uJ3QgbmVlZCB0byB1cGRhdGUgaUFUVSBhbmQgZm9yIENGRywgd2Ugb25seSBuZWVkIHRvIHVw
ZGF0ZSBhIHJlZ2lzdGVyIHRvIHVwZGF0ZSBCVVMgZGV2aWNlIGFuZCBmdW5jdGlvbiBudW1iZXIu
DQpNb3Jlb3ZlciwgdGhpcyBjYW4gYXZvaWQgYW55IGNvbmZsaWN0Lg0KDQpCdXQgVGhlcmUgYXJl
IHNvbWUgU09DcyB0aGF0IG9ubHkgcHJvdmlkZXMgdHdvIGlBVFVzLg0KU28gdGhlIGxhdGVzdCBk
cml2ZXIgcHJvdmlkZXMgYSBzb2x1dGlvbjogQVRVMCBpcyBzaGFyZWQgYnkgQ0ZHMCBDRkcxIGFu
ZCBJTywgQVRVMSBpcyBkZWRpY2F0ZWQgdG8gTUVNLg0KVGhlIGRyaXZlciBzaG91bGQgZmlyc3Qg
c2V0IEFUVTAgYXMgSU8gYW5kIEFUVTEgYXMgTUVNLiBXaGVuIGFjY2Vzc2luZyBDRkcwIG9yIENG
RzEsIEFUVTAgd2lsbCBiZSBjaGFuZ2VkIHRvIENGRzAvQ0ZHMSwgdGhlbiBiYWNrIHRvIElPIHNl
dHRpbmcuDQpDRkcwIENGRzEgY2FuIHNlcXVlbmNlZCBieSBwY2lfbG9jay4gQnV0IENGRyBJTyBh
bmQgTUVNKG1heSBiZSBnZW5lcmF0ZWQgYnkgUENJZSBkZXZpY2UncyBETUEpIGFyZSBjb21wbGV0
ZWx5IGluZGVwZW5kZW50Lg0KDQpCZWNhdXNlIG1vc3QgUENJZSBkZXZpY2UgZG8gbm90IG5lZWQg
SU8gdHJhbnNhY3Rpb24gYW5kIGluIGdlbmVyYWwgQ0ZHMCBDRkcxIGFuZCBJTyB0cmFuc2FjdGlv
biBpcyBnZW5lcmF0ZWQgYnkgUENJZSBkZXZpY2UgZHJpdmVyIGFuZCByYXJlbHkgb2NjdXJyZWQg
c2ltdWx0YW5lb3VzbHkuDQpTbyB0aGUgY3VycmVudCBzb2x1dGlvbiBjYW4gbWluaW1pemUgY29u
ZmxpY3QuDQpJbiBmYWN0LCBmb3IgdGhlIGN1cnJlbnQgcGNpZS1kZXNpZ253YXJlIGRyaXZlciwg
dGhlcmUgaXMgbm8gd2F5IHRvIGF2b2lkIElPIGFuZCBDRkcgY29uZmxpY3QuDQoNCg0KVGhhbmtz
LA0KTWluZ2h1YW4NCg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IEJq
b3JuIEhlbGdhYXMgW21haWx0bzpoZWxnYWFzQGtlcm5lbC5vcmddDQo+IFNlbnQ6IFdlZG5lc2Rh
eSwgSmFudWFyeSAwNiwgMjAxNiA2OjQyIEFNDQo+IFRvOiBKaW5nb28gSGFuIDxqaW5nb29oYW4x
QGdtYWlsLmNvbT47IFByYXR5dXNoIEFuYW5kDQo+IDxwcmF0eXVzaC5hbmFuZEBnbWFpbC5jb20+
DQo+IENjOiBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBKaXNoZW5nIFpoYW5nIDxqc3poYW5n
QG1hcnZlbGwuY29tPjsNCj4gTWluZ2h1YW4uTGlhbkBmcmVlc2NhbGUuY29tDQo+IFN1YmplY3Q6
IERlc2lnbldhcmUgQVRVIFBDSUVfQVRVX1RZUEVfTUVNIHVzYWdlDQo+IA0KPiBIaSBndXlzLA0K
PiANCj4gVGhpcyBjb2RlIGluIGR3X3BjaWVfaG9zdF9pbml0KCkgbG9va3Mgd3Jvbmc6DQo+IA0K
PiAgICAgaWYgKCFwcC0+b3BzLT5yZF9vdGhlcl9jb25mKQ0KPiAJICAgIGR3X3BjaWVfcHJvZ19v
dXRib3VuZF9hdHUocHAsIFBDSUVfQVRVX1JFR0lPTl9JTkRFWDEsDQo+IAkJCQkgICAgICBQQ0lF
X0FUVV9UWVBFX01FTSwgcHAtPm1lbV9iYXNlLA0KPiAJCQkJICAgICAgcHAtPm1lbV9idXNfYWRk
ciwgcHAtPm1lbV9zaXplKTsNCj4gDQo+ICAgICBkd19wY2llX3dyX293bl9jb25mKHBwLCBQQ0lf
QkFTRV9BRERSRVNTXzAsIDQsIDApOw0KPiAgICAgLi4uDQo+ICAgICBkd19wY2llX3JkX293bl9j
b25mKHBwLCBQQ0lFX0xJTktfV0lEVEhfU1BFRURfQ09OVFJPTCwgNCwgJnZhbCk7DQo+IA0KPiBF
dmlkZW50bHkgeW91IG5lZWQgdG8gcHJvZ3JhbSB0aGUgQVRVIHdpdGggUENJRV9BVFVfVFlQRV9N
RU0gYmVmb3JlDQo+IGRvaW5nIGNvbmZpZyBhY2Nlc3NlcyBvbiB0aGUgcm9vdCBidXM/DQo+IA0K
PiBJZiB0aGF0J3MgdGhlIGNhc2UsIHdoYXQgYWJvdXQgb3RoZXIgY29uZmlnIGFjY2Vzc2VzIGFm
dGVyIHRoZXNlIGZldyBpbg0KPiBkd19wY2llX2hvc3RfaW5pdCgpPyAgSSBkb24ndCBzZWUgYW55
dGhpbmcgdGhhdCBjaGFuZ2VzIHRoZSBBVFUgcHJvZ3JhbW1pbmcNCj4gZm9yIHRoaW5ncyBjb21p
bmcgdGhyb3VnaCBkd19wY2llX3dyX2NvbmYoKS4NCj4gDQo+IEkgYXNzdW1lIHlvdSBuZWVkIHNv
bWUgc29ydCBvZiBsb2NraW5nIGFyb3VuZCB0aGlzIHNlcXVlbmNlOg0KPiANCj4gICAgIGR3X3Bj
aWVfcHJvZ19vdXRib3VuZF9hdHUocHAsIFBDSUVfQVRVX1JFR0lPTl9JTkRFWDAsIHR5cGUsIC4u
Lg0KPiAgICAgcmV0ID0gZHdfcGNpZV9jZmdfcmVhZCh2YV9jZmdfYmFzZSArIHdoZXJlLCBzaXpl
LCB2YWwpOw0KPiAgICAgZHdfcGNpZV9wcm9nX291dGJvdW5kX2F0dShwcCwgUENJRV9BVFVfUkVH
SU9OX0lOREVYMCwNCj4gUENJRV9BVFVfVFlQRV9JTywgLi4uDQo+IA0KPiBBcmUgeW91IHJlbHlp
bmcgb24gcGNpX2xvY2s/ICBJZiBzbywgYSBjb21tZW50IHRvIHRoYXQgZWZmZWN0IHdvdWxkIGJl
IG5pY2UuDQo+IA0KPiBUaGUgd2hvbGUgQVRVIG1hbmFnZW1lbnQgbG9va3MgcHJldHR5IGluZWZm
aWNpZW50LiAgSXQncyBsaWtlbHkgdGhhdCB0aGVyZSdsbCBiZQ0KPiBhIHdob2xlIHNlcXVlbmNl
IG9mIG9wZXJhdGlvbnMgd2hlcmUgd2UgZG9uJ3QgbmVlZCB0byB0b3VjaCB0aGUgQVRVLCBidXQN
Cj4gc2luY2Ugd2UgZG9uJ3QgcmVtZW1iZXIgaXRzIGN1cnJlbnQgc3RhdGUsIHdlIGNvbmZpZ3Vy
ZSB0aGUgd2hvbGUgdGhpbmcgZnJvbQ0KPiBzY3JhdGNoIGVhY2ggdGltZSwgd2hpY2ggaXMgYXQg
bGVhc3QgZWlnaHQgcmVnaXN0ZXIgd3JpdGVzIChwbHVzIGEgcmVhZCBmb3IgdGhlDQo+IG5ldyBz
eW5jaHJvbml6YXRpb24pLg0KPiANCj4gQmpvcm4NCg==

Re: DesignWare ATU PCIE_ATU_TYPE_MEM usage

[Jisheng Zhang]

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  

Re: DesignWare ATU PCIE_ATU_TYPE_MEM usage

[Pratyush Anand]

Hi,

On Thu, Jan 7, 2016 at 12:41 PM, Minghuan Lian <minghuan.lian@nxp.com> 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.
> 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.

Yes, as far as I know there are two SOCs Marvell Berlin and SPEAr1340
which have only 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.

Right, and frankly I do not see any way to rule out even by changing
something in pci core layer.

So, IMHO since there are only few SOCs with 2 ATUs, lets implement one
time ATU programming for 4 ATUs in pcie-designware.c and SOCs having
less than 4 ATUs should implement their own rd/wr_other_conf(). With
this approach we can minimized its ill effect.

~Pratyush