Hi York,
On 8 February 2017 at 15:56, york sun york.sun@nxp.com wrote:
On 02/08/2017 02:12 PM, Simon Glass wrote:
Hi York,
On 8 February 2017 at 14:58, york sun york.sun@nxp.com wrote:
Simon,
I stumped on this issue when I was rewriting the code to reserve secure memory. I didn't realize gd->ram_size was used in the driver. I made the top of memory secure so EL2 code wouldn't be able to access. All of the sudden my PCI device failed. By reducing the gd->ram_size, PCI works again.
Can you help me to understand a function in drivers/pci/pci-uclass.c? Around line 818 in function
static int decode_regions(struct pci_controller *hose, const void *blob, int parent_node, int node)
/* Add a region for our local memory */ size = gd->ram_size;#ifdef CONFIG_SYS_SDRAM_BASE base = CONFIG_SYS_SDRAM_BASE; #endif if (gd->pci_ram_top && gd->pci_ram_top < base + size) size = gd->pci_ram_top - base; pci_set_region(hose->regions + hose->region_count++, base, base, size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
What would happen if pci_ram_top is not set, and the memory is split into banks? gd->ram_size would have the total memory, but not in continuous space. Is adding a single region correct here?
It is assuming a simple contiguous memory setup. If it is not contiguous then it isn't right. It would need to add several regions, I suppose.
Simon,
Please see the proposed change below. I did a quick test on LS2080ARDB. PCIe still works. For multi-bank case, I am not sure if we should add a single region up to pci_ram_top, or continue to add all other banks.
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 3b00e6a..582c039 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -814,7 +814,19 @@ static int decode_regions(struct pci_controller *hose, const void *blob, pci_set_region(hose->regions + pos, pci_addr, addr, size, type); }
/* Add a region for our local memory */
/* Add region(s) for our local memory */+#ifdef CONFIG_NR_DRAM_BANKS
for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {base = gd->bd->bi_dram[i].start;size = gd->bd->bi_dram[i].size;if (gd->pci_ram_top &&gd->pci_ram_top >= base &&gd->pci_ram_top < base + size)size = gd->pci_ram_top - base;
It seems reasonable to me, but do check that size is > 0.
pci_set_region(hose->regions + hose->region_count++, base, base,size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);}+#else size = gd->ram_size; #ifdef CONFIG_SYS_SDRAM_BASE base = CONFIG_SYS_SDRAM_BASE; @@ -823,6 +835,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob, size = gd->pci_ram_top - base; pci_set_region(hose->regions + hose->region_count++, base, base, size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); +#endif
return 0;}
York
Regards, Simon