[U-Boot] decode_regions() function

[york sun]

On 02/08/2017 02:12 PM, Simon Glass wrote:
> Hi York,
> 
> On 8 February 2017 at 14:58, york sun <york.sun at 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;
+               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