* [U-Boot] decode_regions() function
@ 2017-02-08 21:58 york sun
2017-02-08 22:12 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: york sun @ 2017-02-08 21:58 UTC (permalink / raw)
To: u-boot
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?
York
^ permalink raw reply [flat|nested] 4+ messages in thread* [U-Boot] decode_regions() function 2017-02-08 21:58 [U-Boot] decode_regions() function york sun @ 2017-02-08 22:12 ` Simon Glass 2017-02-08 22:56 ` york sun 0 siblings, 1 reply; 4+ messages in thread From: Simon Glass @ 2017-02-08 22:12 UTC (permalink / raw) To: u-boot 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. Regards, Simon ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] decode_regions() function 2017-02-08 22:12 ` Simon Glass @ 2017-02-08 22:56 ` york sun 2017-02-09 17:36 ` Simon Glass 0 siblings, 1 reply; 4+ messages in thread From: york sun @ 2017-02-08 22:56 UTC (permalink / raw) To: u-boot 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; + 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] decode_regions() function 2017-02-08 22:56 ` york sun @ 2017-02-09 17:36 ` Simon Glass 0 siblings, 0 replies; 4+ messages in thread From: Simon Glass @ 2017-02-09 17:36 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-09 17:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-08 21:58 [U-Boot] decode_regions() function york sun 2017-02-08 22:12 ` Simon Glass 2017-02-08 22:56 ` york sun 2017-02-09 17:36 ` Simon Glass
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.