Re: [PATCH] PCI: tegra: Do not allocate MSI target memory

[Vidya Sagar <vidyas@nvidia.com>]

On 3/1/2019 12:32 AM, Lucas Stach wrote:
> Am Donnerstag, den 28.02.2019, 20:30 +0530 schrieb Vidya Sagar:
>> The PCI host bridge found on Tegra SoCs doesn't require the MSI target
>> address to be backed by physical system memory. Writes are intercepted
>> within the controller and never make it to the memory pointed to.
>>
>> Since no actual system memory is required, remove the allocation of a
>> single page and hardcode the MSI target address with a special address
>> on a per-SoC basis. Ideally this would be an address to an MMIO memory
>> region (such as where the controller's register are located). However,
>> those addresses don't work reliably across all Tegra generations. The
>> only set of addresses that work consistently are those that point to
>> external memory.
>>
>> This is not ideal, since those addresses could technically be used for
>> DMA and hence be confusing. However, the first page of external memory
>> is unlikely to be used and special enough to avoid confusion.
> So you are trading a slight memory waste of a single page against a
> sporadic (and probably hard to debug) DMA failure if any device happens
> to initiate DMA to the first page of physical memory? That does not
> sound like a good deal...
>
> Also why would the first page of external memory be unlikely to be
> used?
>
> Regards,
> Lucas

We are not wasting single page of memory here and if any device's DMA is 
trying to

access it, it will still go through. Its just that we are using that 
same address for MSI (note that

MSI writes don't go beyond PCIe IP as they get decoded at PCIe IP level 
itself and only an interrupt

goes to CPU) and that might be a bit confusing as same address is used 
as normal memory as well

as MSI target address. Since there can never be any issue with this, 
would you suggest to

remove the last paragraph from commit description?

>> Original base patch was posted by Thierry Reding <treding@nvidia.com>
>> ( http://patchwork.ozlabs.org/patch/848569/ )
>> Current patch removes hardcoding of external RAM starting address instead
>> gets it using memblock_start_of_DRAM() API
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   drivers/pci/controller/pci-tegra.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>> index f4f53d092e00..b33de7c78425 100644
>> --- a/drivers/pci/controller/pci-tegra.c
>> +++ b/drivers/pci/controller/pci-tegra.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/irqdomain.h>
>>   #include <linux/kernel.h>
>>   #include <linux/init.h>
>> +#include <linux/memblock.h>
>>   #include <linux/module.h>
>>   #include <linux/msi.h>
>>   #include <linux/of_address.h>
>> @@ -231,9 +232,8 @@ struct tegra_msi {
>>   	struct msi_controller chip;
>>   	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>>   	struct irq_domain *domain;
>> -	unsigned long pages;
>>   	struct mutex lock;
>> -	u64 phys;
>> +	phys_addr_t phys;
>>   	int irq;
>>   };
>>   
>> @@ -1548,9 +1548,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>   		goto err;
>>   	}
>>   
>> -	/* setup AFI/FPCI range */
>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> -	msi->phys = virt_to_phys((void *)msi->pages);
>> +	msi->phys = memblock_start_of_DRAM();
>>   	host->msi = &msi->chip;
>>   
>>   	return 0;
>> @@ -1592,8 +1590,6 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>>   	struct tegra_msi *msi = &pcie->msi;
>>   	unsigned int i, irq;
>>   
>> -	free_pages(msi->pages, 0);
>> -
>>   	if (msi->irq > 0)
>>   		free_irq(msi->irq, pcie);
>>