[U-Boot] [PATCH 2/2] boards: ls2080: Disable fdt copying by default

[york sun]

On 03/01/2016 08:01 AM, Scott Wood wrote:
> On Tue, 2016-03-01 at 06:03 +0000, Bhupesh Sharma wrote:
>>> From: york sun
>>> Sent: Tuesday, March 01, 2016 11:30 AM
>>>
>>> On 02/29/2016 09:20 PM, Bhupesh Sharma wrote:
>>>>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Scott
>>>>> Wood
>>>>> Sent: Tuesday, March 01, 2016 7:13 AM
>>>>>
>>>>> On Tue, 2016-03-01 at 00:08 +0000, york sun wrote:
>>>>>> Sorry for top posting. I am on outlook web access.
>>>>>>
>>>>>> There may be some limitation on fdt relocation. Without setting
>>>>>> fdt_high, u -boot relocates the device tree toward the end of
>>>>>> useable memory. I haven't got a chance to debug why it doesn't work.
>>>>>>
>>>>>> This patch is to disable the relocation by default. A magic number
>>>>>> 0xa0000000 doesn't make much sense here.
>>>>>
>>>>> I agree, if the number is arbitrary.  But if Linux has a limitation,
>>>>> as it does on arm32, then it should be expressed here.
>>>>>
>>>>
>>>> There are explicit requirements for placement of DTB for aarch64 linux.
>>>> Linux versions prior to v4.2 also require that the DTB be placed
>>>> within the 512 MB region starting at text_offset bytes below the kernel
>>> Image:
>>>>
>>>> http://lxr.free-electrons.com/source/Documentation/arm64/booting.txt#L
>>>> 43
>>>>
>>>> York - have you tested this patch against older kernels like 3.18?
>>>
>>> Bhupesh,
>>>
>>> Thanks for the link. It may explained why Linux doesn't boot if fdt_high
>>> is not set properly on kernel prior 4.2.
>>>
>>> I proposed to disable fdt copying by default. It doesn't mean we cannot
>>> will won't use fdt_high. My point is, setting an arbitrary value doesn't
>>> make much sense.
> 
> It's not arbitrary if it comes from a Linux requirement.

Even kernel requires device tree to be within 512MB from kernel image, it
doesn't warrant a magic number, because the kernel image location is not fixed.

> 
>>>  It could be in overlap with ramdisk, or something else.
> 
> How?  It's an upper limit.  It's not a directive to put the fdt at a specific
> address.  U-Boot knows where the ramdisk is and should avoid overlapping them.

Yes, it is an upper limit. U-Boot will try to put device tree right under that
limit, if possible, regardless where the kernel image is.

As for the overlapping, you could load FIT image into memory, near the location
of fdt_high. U-Boot knows the location _after_ uncompressing/copying.

> 
>>> Since we are using FIT image for this board, it is easy to set correct
>>> load address for kernel/ramdisk/dtb.  
> 
> How does FIT make that any easier?  Picking correct addresses is the same
> challenge as before -- now it's just specified in a different location.

Picking correct address is the same challenges, but at least not twice. And the
kernel address is also in FIT image, together with device tree address. It may
be less used by us, but the feature is there.

> 
>>> Device tree can be used in place.
>>> However, it is user's choice on how to use the memory. We can write a
>>> README as a guideline but it makes no sense to default to 0xa0000000.
>>
>> Thanks York. I agree that 0xa0000000 was an address we found suitable during
>> our earlier
>> Linux boot attempts and we kept using it.
>>
>> Updating the README to add a suitable guideline seems like a proper approach
>> to me.
> 
> Updating a README that people won't read is better than having U-Boot do the
> right thing by default?

I can't agree having fdt_high at 0xa0000000 is the right thing. Let me explain.
With two patches (v6) I sent earlier regarding FIT image address handling, we
are able to use 64-bit addresses in FIT image. It opens the door to use high
region memory on ls2080 series boards. For example, if I load the FIT image into
location 0x80_c000_0000 (it can be any reachable address, including in IFC),
when U-Boot boots it with bootm, the image is decoded as

=> bootm
## Loading kernel from FIT Image at 80c0000000 ...
   Using 'config at 1' configuration
   Trying 'kernel at 1' kernel subimage
     Description:  ARM64 Linux kernel
     Created:      2016-02-29  21:08:40 UTC
     Type:         Kernel Image
     Compression:  gzip compressed
     Data Start:   0x80c00000e0
     Data Size:    4480978 Bytes = 4.3 MiB
     Architecture: AArch64
     OS:           Linux
     Load Address: 0x8080080000
     Entry Point:  0x8080080000
   Verifying Hash Integrity ... OK
## Loading ramdisk from FIT Image at 80c0000000 ...
   Using 'config at 1' configuration
   Trying 'ramdisk at 1' ramdisk subimage
     Description:  LS2 Ramdisk
     Created:      2016-02-29  21:08:40 UTC
     Type:         RAMDisk Image
     Compression:  uncompressed
     Data Start:   0x80c04495d8
     Data Size:    28700629 Bytes = 27.4 MiB
     Architecture: AArch64
     OS:           Linux
     Load Address: 0x80a0000000
     Entry Point:  unavailable
   Verifying Hash Integrity ... OK
   Loading ramdisk from 0x80c04495d8 to 0x80a0000000
## Loading fdt from FIT Image at 80c0000000 ...
   Using 'config at 1' configuration
   Trying 'fdt at 1' fdt subimage
     Description:  Flattened Device Tree blob
     Created:      2016-02-29  21:08:40 UTC
     Type:         Flat Device Tree
     Compression:  uncompressed
     Data Start:   0x80c0446170
     Data Size:    13279 Bytes = 13 KiB
     Architecture: AArch64
   Verifying Hash Integrity ... OK
   Loading fdt from 0x80c0446170 to 0x8090000000
   Booting using the fdt blob at 0x8090000000
   Uncompressing Kernel Image ... OK
   reserving fdt memory region: addr=80000000 size=10000
   Using Device Tree in place at 0000008090000000, end 00000080900063de

Please note the addresses 0x80_8008_0000, 0x80_a000_0000, 0x80_9000_0000 are put
into the its file by me. I can specify any addresses as far as they meet kernel
requirement. In this case, the kernel load/entry address dictates where the
device tree image should be. The best chance to make it right is to specify
device tree address in the same its file, and use it in place. "fdt_high"
totally defeats this purpose.

> 
>>> As I am working on enabling high region memory, I found it even more
>>> inappropriate to set fdt_high to any arbitrary value. Actually, variables
>>> including kernel_start, kernel_load, kernel_size should be removed. They
>>> don't serve users well if the board doesn't have preloaded image to
>>> specific address, which is almost certain on most boards. Those variables
>>> are only useful for shipping boards from manufacture with preloaded
>>> images.
>>>
>>
>> +1
> 
> This is true regarding kernel_start/kernel_size, but not kernel_load which
> tells you what a sane place would be to load an image, rather than describing
> an image that is already there.

The kernel_load is used by copying image from NOR flash into memory. It is not
totally insane, but not necessary. As I explained in above FIT image example, if
the ramdisk load address is set correctly, there is no need to copy the image at
the first place. U-Boot will copy individual images from FIT to their
destinations. Besides, you can ignore the low region and use high region memory
for kernel if you want (after the FIT image patches are merged). We will discuss
if the low region memory should be kept when we come to this issue later.

York