[U-Boot] decode_regions() function

[Simon Glass]

Hi York,

On 8 February 2017 at 15:56, york sun <york.sun at 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 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;

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