[U-Boot,1/5] mx6sabre{auto, sd}: Change FDT loading address to avoid overlaping

The new FSL 3.10.9_1.0.0-alpha kernel requires more memory space and
with the previous loading address we had ovelap; change it for the
same address used in 2013.04-3.10.9_1.0.0-alpha U-Boot.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
Tested-by: Daiane Angolini <daiane.angolini@freescale.com>
---
 include/configs/mx6sabre_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Otavio,

On Fri, Nov 1, 2013 at 9:25 AM, Otavio Salvador <otavio@ossystems.com.br> wrote:
> The new FSL 3.10.9_1.0.0-alpha kernel requires more memory space and
> with the previous loading address we had ovelap; change it for the
> same address used in 2013.04-3.10.9_1.0.0-alpha U-Boot.

Care to explain in more details about the overlap?

Currently we use the following addresses:

- dtb: 0x11000000
- kernel: 0x12000000

Now you propose:

- kernel: 0x12000000
- dtb: 0x18000000

If the kernel is larger in FSL 3.10.9 kernel, how it can overlap with
dtb, since in the original code the kernel goes after the dtb?

I have never used FSL 3.10.9 kernel, so I am curious about this fix.

Regards,

Fabio Estevam

On Fri, Nov 1, 2013 at 10:25 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Otavio,
>
> On Fri, Nov 1, 2013 at 9:25 AM, Otavio Salvador <otavio@ossystems.com.br> wrote:
>> The new FSL 3.10.9_1.0.0-alpha kernel requires more memory space and
>> with the previous loading address we had ovelap; change it for the
>> same address used in 2013.04-3.10.9_1.0.0-alpha U-Boot.
>
> Care to explain in more details about the overlap?
>
> Currently we use the following addresses:
>
> - dtb: 0x11000000
> - kernel: 0x12000000
>
> Now you propose:
>
> - kernel: 0x12000000
> - dtb: 0x18000000
>
> If the kernel is larger in FSL 3.10.9 kernel, how it can overlap with
> dtb, since in the original code the kernel goes after the dtb?
>
> I have never used FSL 3.10.9 kernel, so I am curious about this fix.

I am quoting the original commit change:

    ENGR00268032 config/sabre_common: change the fdt_addr to a high address

    current the fdt_addr is just 16MB offset from the ddr base address, which
    will cause the dtb will be overritten by linux kernel(include .bss section)
    if the linux kernel is bigger than 16MB, which cause setup_machine_fdt
    failed and thus kernel failed to boot.

    This patch change the defaut fdt_addr to 128MB offset from the ddr base
    address, which should be enough for common user case. user can change it
    to other value according to their system needs.

    Signed-off-by: Jason Liu <r64343@freescale.com>

I think I can rework the commit log and include this there.

On Fri, Nov 1, 2013 at 11:00 AM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

> I am quoting the original commit change:
>
>     ENGR00268032 config/sabre_common: change the fdt_addr to a high address
>
>     current the fdt_addr is just 16MB offset from the ddr base address, which
>     will cause the dtb will be overritten by linux kernel(include .bss section)
>     if the linux kernel is bigger than 16MB, which cause setup_machine_fdt
>     failed and thus kernel failed to boot.

What is the size of the FSL 3.10 kernel?

I still do not understand the fix after reading the commit log you pointed to.

Adding Jason, in case he could help clarifying it.

Regards,

Fabio Estevam

On Mon, Nov 4, 2013 at 1:27 AM, Hui Liu <r64343@freescale.com> wrote:
>> -----Original Message-----
>> From: Fabio Estevam [mailto:festevam@gmail.com]
>> Sent: Friday, November 01, 2013 10:19 PM
>> To: Otavio Salvador
>> Cc: U-Boot Mailing List; Estevam Fabio-R49496; Liu Hui-R64343
>> Subject: Re: [U-Boot] [PATCH 1/5] mx6sabre{auto, sd}: Change FDT loading
>> address to avoid overlaping
>>
>> On Fri, Nov 1, 2013 at 11:00 AM, Otavio Salvador <otavio@ossystems.com.br>
>> wrote:
>>
>> > I am quoting the original commit change:
>> >
>> >     ENGR00268032 config/sabre_common: change the fdt_addr to a high
>> > address
>> >
>> >     current the fdt_addr is just 16MB offset from the ddr base address,
>> which
>> >     will cause the dtb will be overritten by linux kernel(include .bss
>> section)
>> >     if the linux kernel is bigger than 16MB, which cause
>> setup_machine_fdt
>> >     failed and thus kernel failed to boot.
>>
>> What is the size of the FSL 3.10 kernel?
>>
>> I still do not understand the fix after reading the commit log you
>> pointed to.
>>
>> Adding Jason, in case he could help clarifying it.
>
> This is due to that the dtb will be overridden by the Linux kernel if the dtb is just 16MB offset
> While Linux kernel is big enough. We have found this issue when Linux kernel image is bigger since
> the decompressed kernel will also start from the beginning of the DDR.
>
> To enlarge the dtb offset is just one workaround for this kernel common issue.
>
> The ideal solution is to fix it in Linux kernel but don't have much time to do it.

What is the final decision on this? I have the same change for many
boards which are/will be supported by 3.10.

Hi Otavio, Jason, Fabio,

On 18/11/2013 20:39, Otavio Salvador wrote:

> 
> What is the final decision on this? I have the same change for many
> boards which are/will be supported by 3.10.
> 

You're right, we need a decision. As far as I read, there is no
explanation why this change is required. As Fabio points out, dtb is
loaded before the kernel and this should avoid an overlap. It will be
very nice if Jason could better explain the issue, because I admit I
have not understood why it happens.

Maybe is DTB loaded after the kernel on Freescale's U-Boot ?

This is clearly a work-around for a bug in Freescale's kernel 3.10, as
we have no problems with other kernels. I booted mainline kernel
3.11/3.12 on a i.MX6 without this issue.

However, I could still merge the patch if we can at least have the
following answers:

- why is there the overlap if the kernel is loaded after DTB (Fabio's
question) ?
- add the explanation to the commit message (so V2 is required, this is
also a Wolfgang's comment).
- of course, the patch should generate a problem with other kernels. It
should not be, but a couple of tested-by will be nice.

As the patchset fixes the same issue, I would prefer if the patchset is
squashed into a single patch because it fixes a single issue and we can
have only one "extended" commit message with the whole explanation.

Best regards,
Stefano Babic

On Thu, Nov 21, 2013 at 7:00 AM, Stefano Babic <sbabic@denx.de> wrote:
> Hi Otavio, Jason, Fabio,
>
> On 18/11/2013 20:39, Otavio Salvador wrote:
>
>>
>> What is the final decision on this? I have the same change for many
>> boards which are/will be supported by 3.10.
>>
>
> You're right, we need a decision. As far as I read, there is no
> explanation why this change is required. As Fabio points out, dtb is
> loaded before the kernel and this should avoid an overlap. It will be
> very nice if Jason could better explain the issue, because I admit I
> have not understood why it happens.
>
> Maybe is DTB loaded after the kernel on Freescale's U-Boot ?
>
> This is clearly a work-around for a bug in Freescale's kernel 3.10, as
> we have no problems with other kernels. I booted mainline kernel
> 3.11/3.12 on a i.MX6 without this issue.
>
> However, I could still merge the patch if we can at least have the
> following answers:
>
> - why is there the overlap if the kernel is loaded after DTB (Fabio's
> question) ?
> - add the explanation to the commit message (so V2 is required, this is
> also a Wolfgang's comment).
> - of course, the patch should generate a problem with other kernels. It
> should not be, but a couple of tested-by will be nice.
>
> As the patchset fixes the same issue, I would prefer if the patchset is
> squashed into a single patch because it fixes a single issue and we can
> have only one "extended" commit message with the whole explanation.

Ok; once we get Jason's feedback I rework the patch and send.

> -----Original Message-----
> From: Stefano Babic [mailto:sbabic@denx.de]
> Sent: Thursday, November 21, 2013 5:01 PM
> To: Otavio Salvador; Liu Hui-R64343
> Cc: U-Boot Mailing List; Estevam Fabio-R49496
> Subject: Re: [U-Boot] [PATCH 1/5] mx6sabre{auto, sd}: Change FDT loading
> address to avoid overlaping
> 
> Hi Otavio, Jason, Fabio,
> 
> On 18/11/2013 20:39, Otavio Salvador wrote:
> 
> >
> > What is the final decision on this? I have the same change for many
> > boards which are/will be supported by 3.10.
> >
> 
> You're right, we need a decision. As far as I read, there is no
> explanation why this change is required. As Fabio points out, dtb is
> loaded before the kernel and this should avoid an overlap. It will be
> very nice if Jason could better explain the issue, because I admit I have
> not understood why it happens.
> 
> Maybe is DTB loaded after the kernel on Freescale's U-Boot ?
> 
> This is clearly a work-around for a bug in Freescale's kernel 3.10, as we
> have no problems with other kernels. I booted mainline kernel
> 3.11/3.12 on a i.MX6 without this issue.
> 
> However, I could still merge the patch if we can at least have the
> following answers:
> 
> - why is there the overlap if the kernel is loaded after DTB (Fabio's
> question) ?

Let me explain it: 
since we defined the fdt_high=0xffffffff at include/configs/mx6qsabre_common.h, 
which means we disable the fdt re-allocation, which you can see when boot up:

## Flattened Device Tree blob at 11000000
   Booting using the fdt blob at 0x11000000
   Using Device Tree in place at 11000000, end 1800e37e

The FDT blob will be placed at DDR physical addr: 0x11000000. When Linux kernel
Boot up, it will decompress the compressed kernel image and place the decompressed
kernel image at the low end of the DDR memory and start running from it. If the
decompressed kernel image is bigger for example than 16M, it may over written the 
fdt blob which u-boot loaded to the DDR memory @0x11000000 with fdt_addr=0x11000000

To expand the fdt_addr from 0x11000000 to 0x18000000, which can avoid the override
Since we will not likely have one kernel image larger than 128MB. 

The other solution is to enable the FDT blob re-allocation by remove the fdt_high=0xffffffff

I check the history and found the definition introduced by the following commit:

commit 7e9603e74ef95a0900771d63ba499b3e80300e55
Author: Dirk Behme <dirk.behme@de.bosch.com>
Date:   Thu Jan 12 23:49:24 2012 +0000

    i.mx6q: configs: Add fdt_high and initrd_high variables

    To be able to load the device tree and initrd correctly, set
    the fdt_high and initrd_high environment variables.

    Using 0xffffffff implies that the device tree and the initrd
    are initially copied to working addresses. This will avoid an
    additional copy.

    Loading the device tree to 0x30000000 and the initrd to 0x3c000000
    should work for both boards, the ARM2 and SabreLite.

    Example (SabreLite):

    fatload mmc 0:2 0x10000000 uImage
    fatload mmc 0:2 0x3c000000 uInitrd
    fatload mmc 0:2 0x30000000 board.dtb
    bootm 0x10000000 0x3c000000 0x30000000

    Note: This requires that the kernel has CONFIG_HIGHMEM enabled.

    Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
    CC: Jason Liu <jason.hui@linaro.org>
    CC: Stefano Babic <sbabic@denx.de>
    Acked-by: Jason Liu <jason.hui@linaro.org>

So, from the commit, if we enable the fdt reallocation, it will have some additional
Copy thus impact the boot up time.

> - add the explanation to the commit message (so V2 is required, this is
> also a Wolfgang's comment).
> - of course, the patch should generate a problem with other kernels. It
> should not be, but a couple of tested-by will be nice.

Yes, agree. 

> 
> As the patchset fixes the same issue, I would prefer if the patchset is
> squashed into a single patch because it fixes a single issue and we can
> have only one "extended" commit message with the whole explanation.

Ditto, 

> 
> Best regards,
> Stefano Babic
> 
> --
> =====================================================================
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================