[U-Boot] [RFC PATCH] driver: pci: Fix regions for local memory
When adding local memory to PCI region, gd->ram_size is correct only if the memory is in one continuous block. In case memory is split into several banks, each bank should be added separately.
Signed-off-by: York Sun york.sun@nxp.com CC: Simon Glass sjg@chromium.org --- It was spotted when I was rewriting the code to reserve secure memory and forgot to reduce gd->ram_size. PCIe resumes working after fixing gd->ram_size. For my case, the memory is split into two banks. So base + gd->ram_size is not in memory. I don't know how it worked before. This change seems reasonable without digging into PCI code.
drivers/pci/pci-uclass.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 3b00e6a..eb80198 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -814,7 +814,22 @@ 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; + if (size) { + 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 +838,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; }
On 9 February 2017 at 11:35, York Sun york.sun@nxp.com wrote:
When adding local memory to PCI region, gd->ram_size is correct only if the memory is in one continuous block. In case memory is split into several banks, each bank should be added separately.
Signed-off-by: York Sun york.sun@nxp.com CC: Simon Glass sjg@chromium.org
It was spotted when I was rewriting the code to reserve secure memory and forgot to reduce gd->ram_size. PCIe resumes working after fixing gd->ram_size. For my case, the memory is split into two banks. So base + gd->ram_size is not in memory. I don't know how it worked before. This change seems reasonable without digging into PCI code.
drivers/pci/pci-uclass.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
Hi York,
On 16 March 2017 at 16:47, Simon Glass sjg@chromium.org wrote:
On 9 February 2017 at 11:35, York Sun york.sun@nxp.com wrote:
When adding local memory to PCI region, gd->ram_size is correct only if the memory is in one continuous block. In case memory is split into several banks, each bank should be added separately.
Signed-off-by: York Sun york.sun@nxp.com CC: Simon Glass sjg@chromium.org
It was spotted when I was rewriting the code to reserve secure memory and forgot to reduce gd->ram_size. PCIe resumes working after fixing gd->ram_size. For my case, the memory is split into two banks. So base + gd->ram_size is not in memory. I don't know how it worked before. This change seems reasonable without digging into PCI code.
drivers/pci/pci-uclass.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
Unfortunately this breaks chromebook_link (x86).
Do you have any ideas or should I dig into it?
Regards, Simon
On 03/16/2017 08:14 PM, Simon Glass wrote:
Hi York,
On 16 March 2017 at 16:47, Simon Glass sjg@chromium.org wrote:
On 9 February 2017 at 11:35, York Sun york.sun@nxp.com wrote:
When adding local memory to PCI region, gd->ram_size is correct only if the memory is in one continuous block. In case memory is split into several banks, each bank should be added separately.
Signed-off-by: York Sun york.sun@nxp.com CC: Simon Glass sjg@chromium.org
It was spotted when I was rewriting the code to reserve secure memory and forgot to reduce gd->ram_size. PCIe resumes working after fixing gd->ram_size. For my case, the memory is split into two banks. So base + gd->ram_size is not in memory. I don't know how it worked before. This change seems reasonable without digging into PCI code.
drivers/pci/pci-uclass.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
Unfortunately this breaks chromebook_link (x86).
Do you have any ideas or should I dig into it?
Sorry I have no idea. If you can look into it, that will be great.
York
Hi York,
On 16 March 2017 at 21:29, york sun york.sun@nxp.com wrote:
On 03/16/2017 08:14 PM, Simon Glass wrote:
Hi York,
On 16 March 2017 at 16:47, Simon Glass sjg@chromium.org wrote:
On 9 February 2017 at 11:35, York Sun york.sun@nxp.com wrote:
When adding local memory to PCI region, gd->ram_size is correct only if the memory is in one continuous block. In case memory is split into several banks, each bank should be added separately.
Signed-off-by: York Sun york.sun@nxp.com CC: Simon Glass sjg@chromium.org
It was spotted when I was rewriting the code to reserve secure memory and forgot to reduce gd->ram_size. PCIe resumes working after fixing gd->ram_size. For my case, the memory is split into two banks. So base + gd->ram_size is not in memory. I don't know how it worked before. This change seems reasonable without digging into PCI code.
drivers/pci/pci-uclass.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
Unfortunately this breaks chromebook_link (x86).
Do you have any ideas or should I dig into it?
Sorry I have no idea. If you can look into it, that will be great.
The problem is that x86 sets up PCI before relocation, thus before gd->bd is available.
I think several changes are needed:
1. Something like:
bool use_dram_banks = false;
#ifdef CONFIG_NR_DRAM_BANKS use_dram_banks = gd->bd != NULL; #endif if (use_dram_banks) { your new code } else { old code }
2. DRAM banks can have a size of 0, so check for that and skip if needed.
3. Check you don't overflow the size of the pci controller regions[] array.
Sorry I didn't notice this when reviewing it. It looked fine to me!
Regards, Simon
On 03/17/2017 04:43 AM, Simon Glass wrote:
Hi York,
On 16 March 2017 at 21:29, york sun york.sun@nxp.com wrote:
On 03/16/2017 08:14 PM, Simon Glass wrote:
Hi York,
On 16 March 2017 at 16:47, Simon Glass sjg@chromium.org wrote:
On 9 February 2017 at 11:35, York Sun york.sun@nxp.com wrote:
When adding local memory to PCI region, gd->ram_size is correct only if the memory is in one continuous block. In case memory is split into several banks, each bank should be added separately.
Signed-off-by: York Sun york.sun@nxp.com CC: Simon Glass sjg@chromium.org
It was spotted when I was rewriting the code to reserve secure memory and forgot to reduce gd->ram_size. PCIe resumes working after fixing gd->ram_size. For my case, the memory is split into two banks. So base + gd->ram_size is not in memory. I don't know how it worked before. This change seems reasonable without digging into PCI code.
drivers/pci/pci-uclass.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
Unfortunately this breaks chromebook_link (x86).
Do you have any ideas or should I dig into it?
Sorry I have no idea. If you can look into it, that will be great.
The problem is that x86 sets up PCI before relocation, thus before gd->bd is available.
That explains it.
I think several changes are needed:
- Something like:
bool use_dram_banks = false;
#ifdef CONFIG_NR_DRAM_BANKS use_dram_banks = gd->bd != NULL; #endif if (use_dram_banks) { your new code } else { old code }
DRAM banks can have a size of 0, so check for that and skip if needed.
Check you don't overflow the size of the pci controller regions[] array.
Sorry I didn't notice this when reviewing it. It looked fine to me!
Thanks for the explanation.
York
participants (3)
-
Simon Glass -
York Sun -
york sun