Hi Stephen, On 14/11/2018 04:31, Stephen Warren wrote: > 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. I think we're talking about the same thing, but with different terms. > > In the future, we'll send a driver for the NVIDIA Tegra SoC which does > explicitly set atu_base to a non-default value. That's what I wanted to know. Because otherwise this patch was just to turn the code more readable. > >> 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. Agree, as long that maintains the retro-compatibility. > >>> 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. > Well seen. Regards, Gustavo
