On 11/14/18 9:33 PM, Gustavo Pimentel wrote: > 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. So it sounds like I've addressed your questions? If so, is the next step for you to ack the patch and Bjorn to apply it? Thanks!
