Re: [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup

[Kishon Vijay Abraham I]

Hi,

On 17/12/19 6:10 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:41PM +0530, Kishon Vijay Abraham I wrote:
>> Cadence driver uses "mem" memory resource to obtain the offset of
>> configuration space address region, memory space address region and
>> message space address region. The obtained offset is used to program
>> the Address Translation Unit (ATU). However certain platforms like TI's
>> J721E SoC require the absolute address to be programmed in the ATU and not
>> just the offset.
>>
>> The same problem was solved in designware driver using a platform specific
>> ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware:
> 
> Thanks for this reference, though this doesn't need to be in the commit
> log, please put such comments underneath a ---.
> 
>> Add new *ops* for CPU addr fixup"). Follow a similar mechanism in
>> Cadence too instead of directly using "mem" memory resource in Cadence
>> PCIe core.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx>
>> ---
>>  .../pci/controller/cadence/pcie-cadence-host.c    | 15 ++++-----------
>>  drivers/pci/controller/cadence/pcie-cadence.c     |  8 ++++++--
>>  drivers/pci/controller/cadence/pcie-cadence.h     |  1 +
>>  3 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 2efc33b1cade..cf817be237af 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>  static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>>  {
>>  	struct cdns_pcie *pcie = &rc->pcie;
>> -	struct resource *mem_res = pcie->mem_res;
>>  	struct resource *bus_range = rc->bus_range;
>>  	struct resource *cfg_res = rc->cfg_res;
>>  	struct device *dev = pcie->dev;
>>  	struct device_node *np = dev->of_node;
>>  	struct of_pci_range_parser parser;
>> +	u64 cpu_addr = cfg_res->start;
>>  	struct of_pci_range range;
>>  	u32 addr0, addr1, desc1;
>> -	u64 cpu_addr;
>>  	int r, err;
>>  
>>  	/*
>> @@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1);
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1);
>>  
>> -	cpu_addr = cfg_res->start - mem_res->start;
>> +	if (pcie->ops->cpu_addr_fixup)
>> +		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
>> +
> 
> Won't this patch cause a breakage for existing users that won't have defined a
> cpu_addr_fixup? The offset isn't being calculated and so cpu_addr will be wrong?

Correct, this will need an additional patch in pcie-cadence-plat.c.

Thanks
Kishon