* [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.