[U-Boot] [RFC PATCH] driver: pci: Fix regions for local memory

[york sun]

On 03/17/2017 04:43 AM, Simon Glass wrote:
> Hi York,
>
> On 16 March 2017 at 21:29, york sun <york.sun at 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 at chromium.org> wrote:
>>>> On 9 February 2017 at 11:35, York Sun <york.sun at 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 at nxp.com>
>>>>> CC: Simon Glass <sjg at 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 at 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:
>
> 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!
>

Thanks for the explanation.

York