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

[Stephen Warren <swarren@wwwdotorg.org>]

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