[U-Boot] BUG: bootz/bootm command mandates a fdt blob

[U-Boot] BUG: bootz/bootm command mandates a fdt blob

[Suriyan Ramasami]

Hello Simon,
     This mail is addressed to you as the FDT support was added by
you. I am not sure who else to address it to.

      I find that if CONFIG_OF_LIBFDT is defined then the user is
forced to provide a FDT blob. In most of the cases it makes sense.
However, this removes the ability to boot older linux (non FDT).
    For example, I was looking at the Hardkernel Odroid kernels for
the U2/U3, and they are 3.8 based. Of course newer kernels 3.17 work.
For users to use the same boot loader for 3.8 and for 3.17, they
cannot use the mainline uboot.

    I was wondering if teh third argument to bootz/bootm etc could
also take the route of initrd (optional if - is specified) could be
implemented.

   The fix seems to be trivial (or so I think), in file
common/image-fdt.c, but wanted to know your comments on this. Also the
command help (for bootz etc) states that if the 3rd argument is not
passed, then the bd_info struct is passed, and I do not see it being
passed in the code anywhere. In the absence of the third parameter, it
just gives a "No fdt found" message.

diff --git a/common/image-fdt.c b/common/image-fdt.c
index a39ae1b..e685700 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -243,7 +243,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], u

        if (argc > 2)
                select = argv[2];
-       if (select || genimg_has_config(images)) {
+       if (select && strcmp(select, "-") ==  0) {
+               debug("## Skipping fdt\n");
+               return 0;
+       } else if (select || genimg_has_config(images)) {
 #if defined(CONFIG_FIT)
                if (select) {
                        /*

Regards
- Suriyan

[U-Boot] BUG: bootz/bootm command mandates a fdt blob

[Hans de Goede]

Hi Suriyan,

On 11/20/2014 04:16 PM, Suriyan Ramasami wrote:
> Hello Simon,
>      This mail is addressed to you as the FDT support was added by
> you. I am not sure who else to address it to.
> 
>       I find that if CONFIG_OF_LIBFDT is defined then the user is
> forced to provide a FDT blob. In most of the cases it makes sense.
> However, this removes the ability to boot older linux (non FDT).
>     For example, I was looking at the Hardkernel Odroid kernels for
> the U2/U3, and they are 3.8 based. Of course newer kernels 3.17 work.
> For users to use the same boot loader for 3.8 and for 3.17, they
> cannot use the mainline uboot.

I've hit the same problem myself recently, see the thread titled:

"Booting non devicetree enabled kernels using u-boot build with CONFIG_OF_LIBFDT"

>     I was wondering if teh third argument to bootz/bootm etc could
> also take the route of initrd (optional if - is specified) could be
> implemented.
> 
>    The fix seems to be trivial (or so I think), in file
> common/image-fdt.c, but wanted to know your comments on this. Also the
> command help (for bootz etc) states that if the 3rd argument is not
> passed, then the bd_info struct is passed, and I do not see it being
> passed in the code anywhere. In the absence of the third parameter, it
> just gives a "No fdt found" message.

Thanks for working on a fix, as discussed in the earlier thread, requiring
a third argument which is '-' will break old boot.scr files and the likes,
so a better fix is to:

1) Always try to find an ftd (to keep things like appended ftd-s working)
2) If not found see if there is a third argument, if there is, treat this
as a fatal error, abort the bootm (iow behave as before)
3) If there is not a third argument warn and continue as before.

If you could respin your patch to do this, then that would be great.

Regards,

Hans


> 
> diff --git a/common/image-fdt.c b/common/image-fdt.c
> index a39ae1b..e685700 100644
> --- a/common/image-fdt.c
> +++ b/common/image-fdt.c
> @@ -243,7 +243,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], u
> 
>         if (argc > 2)
>                 select = argv[2];
> -       if (select || genimg_has_config(images)) {
> +       if (select && strcmp(select, "-") ==  0) {
> +               debug("## Skipping fdt\n");
> +               return 0;
> +       } else if (select || genimg_has_config(images)) {
>  #if defined(CONFIG_FIT)
>                 if (select) {
>                         /*
> 
> Regards
> - Suriyan
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] BUG: bootz/bootm command mandates a fdt blob

[Simon Glass]

Hi,

On 20 November 2014 16:04, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Suriyan,
>
> On 11/20/2014 04:16 PM, Suriyan Ramasami wrote:
>> Hello Simon,
>>      This mail is addressed to you as the FDT support was added by
>> you. I am not sure who else to address it to.

Just to be clear, I didn't add FDT support, this predates my
involvement in U-Boot. I added CONFIG_OF_CONTROL etc. though. Han's
solution sounds good to me.

>>
>>       I find that if CONFIG_OF_LIBFDT is defined then the user is
>> forced to provide a FDT blob. In most of the cases it makes sense.
>> However, this removes the ability to boot older linux (non FDT).
>>     For example, I was looking at the Hardkernel Odroid kernels for
>> the U2/U3, and they are 3.8 based. Of course newer kernels 3.17 work.
>> For users to use the same boot loader for 3.8 and for 3.17, they
>> cannot use the mainline uboot.
>
> I've hit the same problem myself recently, see the thread titled:
>
> "Booting non devicetree enabled kernels using u-boot build with CONFIG_OF_LIBFDT"
>
>>     I was wondering if teh third argument to bootz/bootm etc could
>> also take the route of initrd (optional if - is specified) could be
>> implemented.
>>
>>    The fix seems to be trivial (or so I think), in file
>> common/image-fdt.c, but wanted to know your comments on this. Also the
>> command help (for bootz etc) states that if the 3rd argument is not
>> passed, then the bd_info struct is passed, and I do not see it being
>> passed in the code anywhere. In the absence of the third parameter, it
>> just gives a "No fdt found" message.
>
> Thanks for working on a fix, as discussed in the earlier thread, requiring
> a third argument which is '-' will break old boot.scr files and the likes,
> so a better fix is to:
>
> 1) Always try to find an ftd (to keep things like appended ftd-s working)
> 2) If not found see if there is a third argument, if there is, treat this
> as a fatal error, abort the bootm (iow behave as before)
> 3) If there is not a third argument warn and continue as before.
>
> If you could respin your patch to do this, then that would be great.
>
> Regards,
>
> Hans
>
>
>>
>> diff --git a/common/image-fdt.c b/common/image-fdt.c
>> index a39ae1b..e685700 100644
>> --- a/common/image-fdt.c
>> +++ b/common/image-fdt.c
>> @@ -243,7 +243,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], u
>>
>>         if (argc > 2)
>>                 select = argv[2];
>> -       if (select || genimg_has_config(images)) {
>> +       if (select && strcmp(select, "-") ==  0) {
>> +               debug("## Skipping fdt\n");
>> +               return 0;
>> +       } else if (select || genimg_has_config(images)) {
>>  #if defined(CONFIG_FIT)
>>                 if (select) {
>>                         /*
>>
>> Regards
>> - Suriyan
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>

Regards,
Simon

[U-Boot] BUG: bootz/bootm command mandates a fdt blob

[Suriyan Ramasami]

Hello Simon,

On Nov 20, 2014 8:38 AM, "Simon Glass" <sjg@chromium.org> wrote:
>
> Hi,
>
> On 20 November 2014 16:04, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi Suriyan,
> >
> > On 11/20/2014 04:16 PM, Suriyan Ramasami wrote:
> >> Hello Simon,
> >>      This mail is addressed to you as the FDT support was added by
> >> you. I am not sure who else to address it to.
>
> Just to be clear, I didn't add FDT support, this predates my
> involvement in U-Boot. I added CONFIG_OF_CONTROL etc. though. Han's
> solution sounds good to me.
>

Sorry about that. In no way was I accusing you :-) I stand corrected though.

> >>
> >>       I find that if CONFIG_OF_LIBFDT is defined then the user is
> >> forced to provide a FDT blob. In most of the cases it makes sense.
> >> However, this removes the ability to boot older linux (non FDT).
> >>     For example, I was looking at the Hardkernel Odroid kernels for
> >> the U2/U3, and they are 3.8 based. Of course newer kernels 3.17 work.
> >> For users to use the same boot loader for 3.8 and for 3.17, they
> >> cannot use the mainline uboot.
> >
> > I've hit the same problem myself recently, see the thread titled:
> >
> > "Booting non devicetree enabled kernels using u-boot build with
CONFIG_OF_LIBFDT"
> >
> >>     I was wondering if teh third argument to bootz/bootm etc could
> >> also take the route of initrd (optional if - is specified) could be
> >> implemented.
> >>
> >>    The fix seems to be trivial (or so I think), in file
> >> common/image-fdt.c, but wanted to know your comments on this. Also the
> >> command help (for bootz etc) states that if the 3rd argument is not
> >> passed, then the bd_info struct is passed, and I do not see it being
> >> passed in the code anywhere. In the absence of the third parameter, it
> >> just gives a "No fdt found" message.
> >
> > Thanks for working on a fix, as discussed in the earlier thread,
requiring
> > a third argument which is '-' will break old boot.scr files and the
likes,
> > so a better fix is to:
> >
> > 1) Always try to find an ftd (to keep things like appended ftd-s
working)
> > 2) If not found see if there is a third argument, if there is, treat
this
> > as a fatal error, abort the bootm (iow behave as before)
> > 3) If there is not a third argument warn and continue as before.
> >
> > If you could respin your patch to do this, then that would be great.
> >

Hello Hans/Simon,
   I have been tracing the code paths and see that function
bootm_find_fdt() is the final fdt prep function.
   To retain all its previous functionality, but introducing a warning
condition if no third argument is present, but go ahead and boot without
fdt should keep the peace.
    The easiest way to achieve this is: (takes care of 2 and 3 above)

diff --git a/common/image-fdt.c b/common/image-fdt.c
index a39ae1b..1a02166 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -430,6 +430,10 @@ int boot_get_fdt(int flag, int argc, char * const
argv[], u
 error:
        *of_flat_tree = NULL;
        *of_size = 0;
+       if (argc <= 2) {
+               debug("Continuing to boot without FDT\n");
+               return 0;
+       }
        return 1;
 }

For case 1, is this already handled in the code? I need some more
information on this. I did run a 3.17 zImage with an appended dtb and it
did boot up, but I am guessing that it is the linux kernel separating the
dtb out?

Thanks

- Suriyan

> > Regards,
> >
> > Hans
> >
> >
> >>
> >> diff --git a/common/image-fdt.c b/common/image-fdt.c
> >> index a39ae1b..e685700 100644
> >> --- a/common/image-fdt.c
> >> +++ b/common/image-fdt.c
> >> @@ -243,7 +243,10 @@ int boot_get_fdt(int flag, int argc, char * const
argv[], u
> >>
> >>         if (argc > 2)
> >>                 select = argv[2];
> >> -       if (select || genimg_has_config(images)) {
> >> +       if (select && strcmp(select, "-") ==  0) {
> >> +               debug("## Skipping fdt\n");
> >> +               return 0;
> >> +       } else if (select || genimg_has_config(images)) {
> >>  #if defined(CONFIG_FIT)
> >>                 if (select) {
> >>                         /*
> >>
> >> Regards
> >> - Suriyan
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot at lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> >>
>
> Regards,
> Simon

[U-Boot] BUG: bootz/bootm command mandates a fdt blob

[Hans de Goede]

Hi,

On 11/20/2014 08:41 PM, Suriyan Ramasami wrote:
> Hello Simon,
> 
> On Nov 20, 2014 8:38 AM, "Simon Glass" <sjg@chromium.org> wrote:
>>
>> Hi,
>>
>> On 20 November 2014 16:04, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi Suriyan,
>>>
>>> On 11/20/2014 04:16 PM, Suriyan Ramasami wrote:
>>>> Hello Simon,
>>>>      This mail is addressed to you as the FDT support was added by
>>>> you. I am not sure who else to address it to.
>>
>> Just to be clear, I didn't add FDT support, this predates my
>> involvement in U-Boot. I added CONFIG_OF_CONTROL etc. though. Han's
>> solution sounds good to me.
>>
> 
> Sorry about that. In no way was I accusing you :-) I stand corrected though.
> 
>>>>
>>>>       I find that if CONFIG_OF_LIBFDT is defined then the user is
>>>> forced to provide a FDT blob. In most of the cases it makes sense.
>>>> However, this removes the ability to boot older linux (non FDT).
>>>>     For example, I was looking at the Hardkernel Odroid kernels for
>>>> the U2/U3, and they are 3.8 based. Of course newer kernels 3.17 work.
>>>> For users to use the same boot loader for 3.8 and for 3.17, they
>>>> cannot use the mainline uboot.
>>>
>>> I've hit the same problem myself recently, see the thread titled:
>>>
>>> "Booting non devicetree enabled kernels using u-boot build with
> CONFIG_OF_LIBFDT"
>>>
>>>>     I was wondering if teh third argument to bootz/bootm etc could
>>>> also take the route of initrd (optional if - is specified) could be
>>>> implemented.
>>>>
>>>>    The fix seems to be trivial (or so I think), in file
>>>> common/image-fdt.c, but wanted to know your comments on this. Also the
>>>> command help (for bootz etc) states that if the 3rd argument is not
>>>> passed, then the bd_info struct is passed, and I do not see it being
>>>> passed in the code anywhere. In the absence of the third parameter, it
>>>> just gives a "No fdt found" message.
>>>
>>> Thanks for working on a fix, as discussed in the earlier thread,
> requiring
>>> a third argument which is '-' will break old boot.scr files and the
> likes,
>>> so a better fix is to:
>>>
>>> 1) Always try to find an ftd (to keep things like appended ftd-s
> working)
>>> 2) If not found see if there is a third argument, if there is, treat
> this
>>> as a fatal error, abort the bootm (iow behave as before)
>>> 3) If there is not a third argument warn and continue as before.
>>>
>>> If you could respin your patch to do this, then that would be great.
>>>
> 
> Hello Hans/Simon,
>    I have been tracing the code paths and see that function
> bootm_find_fdt() is the final fdt prep function.
>    To retain all its previous functionality, but introducing a warning
> condition if no third argument is present, but go ahead and boot without
> fdt should keep the peace.
>     The easiest way to achieve this is: (takes care of 2 and 3 above)
> 
> diff --git a/common/image-fdt.c b/common/image-fdt.c
> index a39ae1b..1a02166 100644
> --- a/common/image-fdt.c
> +++ b/common/image-fdt.c
> @@ -430,6 +430,10 @@ int boot_get_fdt(int flag, int argc, char * const
> argv[], u
>  error:
>         *of_flat_tree = NULL;
>         *of_size = 0;
> +       if (argc <= 2) {
> +               debug("Continuing to boot without FDT\n");
> +               return 0;
> +       }
>         return 1;
>  }
> 
> For case 1, is this already handled in the code?

Yes it is, so your above patch should do the trick nicely, thanks for working on
this.

Can you re-send as a proper patch, with commit message, rather then inline in
a mail ? (Please use git send-email if possible).

Regards,

Hans