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@xxxxxxxxxx> >> >> 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.
