Re: [PATCH] PCI: designware: don't hard-code DBI/ATU offset

[Stephen Warren <swarren@wwwdotorg.org>]

On 11/12/18 9:19 PM, Gustavo Pimentel wrote:
> Hi Stephen,
> 
> I'd suggest to change the designware on the patch name by dwc and use another
> description for the patch instead of don't hard-code DBI/ATU offset.

What issue do you see with the patch description? I believe it's
correct/accurate. Perhaps the confusion is due to the question you
raised below; see my answer there.

> On 12/11/2018 22:57, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> 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.
> 
> If I understood this patch correctly, you basically replace the dbi_base offset
> by atu_base offset that still depends of dbi_base offset.

That's not what the patch does.

The patch leaves most DBI accesses still using dbi_base, but updates
just a few accesses to use atu_base. Thus, after the patch, all accesses
use the correct base address for the register being accessed.

There is a default value supplied for atu_base, which does indeed depend
on dbi_base. This maintains backwards compatibility, so that all the
existing drivers don't need to be updated to explicitly set atu_base,
and will continue to use the existing offset between DBI and ATU base.

In the future, we'll send a driver for the NVIDIA Tegra SoC which does
explicitly set atu_base to a non-default value.

> I agree the code is
> now more readable, but I'm not seeing it by applying this patch what is the
> benefit since you are not allowing to change the current atu_base. I though that
> was the propose of this patch.

atu_base may be set by the implementation-specific driver before calling
dw_pcie_ep_init(), or the host controller equivalent.

>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h

>> -#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)	\
>> -			((0x3 << 20) | ((region) << 9))
>> +#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region) \
>> +		((region) << 9)
> 
> I'd suggest to remove the parenthesis around the region variable, like this:
> (region << 9)

Note that this is a pre-existing issue, and not caused/influenced by
this patch. The existing code is correct though; it's best practice to
always bracket all macro parameters, so that if the expression passed to
the macro contains operators, the macro implementation can guarantee the
correct calculation bracketing.