diff mbox

[U-Boot,7/9] mx53loco: Add support to dynamically choose between ftd use or not

Message ID CAP9ODKpxWGee9wuE5gQp8yKUWH0E3VVEAiMCHW0h_WbycN9gKg@mail.gmail.com
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Otavio Salvador Dec. 27, 2012, 9:22 p.m. UTC 
On Thu, Dec 27, 2012 at 7:44 AM, Stefano Babic <sbabic@denx.de> wrote:
> On 26/12/2012 19:23, Otavio Salvador wrote:
>> On Wed, Dec 26, 2012 at 10:14 AM, Fabio Estevam <festevam@gmail.com> wrote:
>>> On Wed, Dec 26, 2012 at 9:31 AM, Stefano Babic <sbabic@denx.de> wrote:
>>>
>>>> Only to remark that this is the correct behavior. The kernel with fdt is
>>>> booted independently if this is wanted or not, but if the fdt file is
>>>> loaded successfully from MMC or network. This can have some drawback
>>>> effects if, for example, the fdt is simply stored on the TFTP server,
>>>> but we want to boot a kernel without DT. For example, when someone wants
>>>> to test both kernels or there are multiple instances of the same board
>>>> type (mx53loco in this case) loading from the same TFTP server.
>>>>
>>>> In your patch, the behavior depends if the fdt is simply present on the
>>>> media, but this does not always mean that the file must be loaded.
>>>> Should be not better to set variable as flag to force the desired
>>>> behavior and to be sure that the system does not boot in a different way
>>>> as the user thinks ?
>>>>
>>>> In other words, something like :
>>>>
>>>>         "if boot_fdt; then " \
>>>>                 "if dhcp ${ftd_addr} ${ftd_file}; then " \
>>>>                         "bootm ${loadaddr} - ${ftd_addr}; " \
>>>>                 "else " \
>>>>                         "echo Error: I cannot boot a DT kernel; \"
>>>>                 "fi; \"
>>>>         " else bootm; fi;\0"
>>>
>>>
>>> Yes, I think this is a good idea and would allow us to easily switch
>>> from dt to non-dt kernel during tests.
>>
>> I like the idea but maybe we could have three states?
>>
>> boot_fdt=yes
>>
>> It'd imply force fdt and would behave as you said above;
>>
>> boot_fdt=no
>>
>> Would ignore the fdt completely
>>
>> boot_fdt=auto
>>
>> The current code, which try to load fdt and do not fail otherwise.
>>
>> Comments?
>
> Reading again your environment, it seems to me that you strictly bind
> the network configuration with fdt. But they are two distinct setup.
> Let's say, if I desire to start a fdt kernel but I have a static ip
> address. In this case, I should load with "tftp" instead using "dhcp".
> So the necessity to have a three state is due to the fact you are
> setting more as one state in one shot: fdt or not, and dhcp or not. IMHO
> you need two variables to manage them independently. I think I did
> something like this in another board, but I do not remember which one ;-(.
>
> So you can have:
>         boot_fdt
>         ip_dyn
>
> to discriminate what the script should do.
>
>          "if boot_fdt; then " \
>                 "if ip_dyn;then" \
>                    "if dhcp ${ftd_addr} ${ftd_file}; then " \
>                        "bootm ${loadaddr} - ${ftd_addr}; " \
>                     "else " \
>                        "echo Error: I cannot boot a DT kernel; \"
>                     "fi;" \
>                 "else tftp ${ftd_addr} ${ftd_file}
>                         .....

Right; I have did the bellow changes:

    "mmc dev ${mmcdev}; if mmc rescan; then " \

What do you think? I don't have the board to test but except by the
get_cmd command, the rest should be fine.

--
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

Comments

Stefano Babic Dec. 28, 2012, 8:52 a.m. UTC | #1 
On 27/12/2012 22:22, Otavio Salvador wrote:

> 
> Right; I have did the bellow changes:
> 
> diff --git a/include/configs/mx28evk.h b/include/configs/mx28evk.h

ok, you changed the mx28evk, but we are really talking about
mx53loco...never mind !

> index ac9522f..45ac072 100644
> --- a/include/configs/mx28evk.h
> +++ b/include/configs/mx28evk.h
> @@ -290,6 +290,10 @@
>     "uimage=uImage\0" \
>     "console_fsl=ttyAM0\0" \
>     "console_mainline=ttyAMA0\0" \
> +   "ftd_file=imx28-evk.dtb\0" \
> +   "ftd_addr=0x41000000\0" \
> +   "boot_fdt=auto\0" \
> +   "ip_dyn=yes\0" \

Only my personal taste. I find simpler, specially with U-Boot
environment, to not full describe the variable with strings like "yes"
or "no". This lets me to simply test with
	
	if test -n ${ip_dyn}

and it works if ip_dyn is not set at all.

>     "mmcdev=0\0" \
>     "mmcpart=2\0" \
>     "mmcroot=/dev/mmcblk0p3 rw rootwait\0" \
> @@ -302,13 +306,43 @@
>     "loaduimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${uimage}\0" \
>     "mmcboot=echo Booting from mmc ...; " \
>         "run mmcargs; " \
> -       "bootm\0" \
> +       "if test ${boot_fdt} = no; then " \
> +           "bootm; " \
> +       "else " \
> +           "if fatload mmc ${mmcdev}:${mmcpart} ${ftd_addr}
> ${ftd_file}; then " \
> +               "bootm ${loadaddr} - ${ftd_addr}; " \

I think you can better rewrite it as :

       "if test ${boot_fdt} = yes; then " \
           "if fatload mmc ${mmcdev}:${mmcpart} ${ftd_addr} 		
		${ftd_file}; then " \
               "bootm ${loadaddr} - ${ftd_addr}; " \
            "else " \
                   "echo ERROR: Cannot load the DT, aborting...; " \
            "fi;\0" \
            "else "
                   "bootm; " \
            "fi;\0" \

Not sure what you mean with "auto". You try to start a  DT kernel
without FDT. Why is it helpful ?

> +           "else " \
> +               "if test ${boot_fdt} = auto; then " \
> +                   "bootm; " \
> +               "else " \
> +                   "echo ERROR: Cannot load the DT, aborting...; " \
> +               "fi;\0" \
> +           "fi;\0" \
> +       "fi;\0" \
>     "netargs=setenv bootargs console=${console_mainline},${baudrate} " \
>         "root=/dev/nfs " \
>         "ip=dhcp nfsroot=${serverip}:${nfsroot},v3,tcp\0" \
>     "netboot=echo Booting from net ...; " \
>         "run netargs; " \
> -       "dhcp ${uimage}; bootm\0"
> +       "if test ${ip_dyn} = yes; then " \
> +           "setenv get_cmd dhcp; " \
> +       "else " \
> +           "setenv get_cmd tftp; " \
> +       "fi;\0" \
> +       ${get_cmd} ${uimage}; " \

This does not work - you must use "run ${get_cmd}"

> +       "if test ${boot_fdt} = no; then " \
> +           "bootm; " \
> +       "else " \
> +           "if ${get_cmd} ${ftd_addr} ${ftd_file}; then "  \
> +               "bootm ${loadaddr} - ${ftd_addr}; " \
> +           "else " \
> +               "if test ${boot_fdt} = auto; then " \
> +                   "bootm; " \
> +               "else " \
> +                   "echo ERROR: Cannot load the DT, aborting...; " \
> +               "fi;\0" \
> +           "fi;\0" \
> +       "fi;\0"
> 
>  #define CONFIG_BOOTCOMMAND \
>     "mmc dev ${mmcdev}; if mmc rescan; then " \
> 
> What do you think? I don't have the board

I can test it on mx53loco

Regards,
Stefano
Otavio Salvador Dec. 28, 2012, 5:47 p.m. UTC | #2 
On Fri, Dec 28, 2012 at 6:52 AM, Stefano Babic <sbabic@denx.de> wrote:
> On 27/12/2012 22:22, Otavio Salvador wrote:
>
>>
>> Right; I have did the bellow changes:
>>
>> diff --git a/include/configs/mx28evk.h b/include/configs/mx28evk.h
>
> ok, you changed the mx28evk, but we are really talking about
> mx53loco...never mind !

Indeed; I will redo it for mx53qsb and send it ... so you can try. I
will attach it in the e-mail so I resend it in a proper pull request
later.

>> index ac9522f..45ac072 100644
>> --- a/include/configs/mx28evk.h
>> +++ b/include/configs/mx28evk.h
>> @@ -290,6 +290,10 @@
>>     "uimage=uImage\0" \
>>     "console_fsl=ttyAM0\0" \
>>     "console_mainline=ttyAMA0\0" \
>> +   "ftd_file=imx28-evk.dtb\0" \
>> +   "ftd_addr=0x41000000\0" \
>> +   "boot_fdt=auto\0" \
>> +   "ip_dyn=yes\0" \
>
> Only my personal taste. I find simpler, specially with U-Boot
> environment, to not full describe the variable with strings like "yes"
> or "no". This lets me to simply test with
>
>         if test -n ${ip_dyn}
>
> and it works if ip_dyn is not set at all.

Yes; I just don't know if it us good to remove it, instead of set it
to something as 0 or no so user doesn't need to read the code to
identify the need of setting the var to enable dynamic ip again.

>>     "mmcdev=0\0" \
>>     "mmcpart=2\0" \
>>     "mmcroot=/dev/mmcblk0p3 rw rootwait\0" \
>> @@ -302,13 +306,43 @@
>>     "loaduimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${uimage}\0" \
>>     "mmcboot=echo Booting from mmc ...; " \
>>         "run mmcargs; " \
>> -       "bootm\0" \
>> +       "if test ${boot_fdt} = no; then " \
>> +           "bootm; " \
>> +       "else " \
>> +           "if fatload mmc ${mmcdev}:${mmcpart} ${ftd_addr}
>> ${ftd_file}; then " \
>> +               "bootm ${loadaddr} - ${ftd_addr}; " \
>
> I think you can better rewrite it as :
>
>        "if test ${boot_fdt} = yes; then " \
>            "if fatload mmc ${mmcdev}:${mmcpart} ${ftd_addr}
>                 ${ftd_file}; then " \
>                "bootm ${loadaddr} - ${ftd_addr}; " \
>             "else " \
>                    "echo ERROR: Cannot load the DT, aborting...; " \
>             "fi;\0" \
>             "else "
>                    "bootm; " \
>             "fi;\0" \

I will rework it inverting the logic.

> Not sure what you mean with "auto". You try to start a  DT kernel
> without FDT. Why is it helpful ?

The idea is to allow to it to try to load fdt and if it fails boot a
non-fdt kernel. This allow for e.g OE to use same environment to be
used to fdt and non-fdt kernel. The only difference is the inclusion
or not of the dtb file in the filesystem. I can name it 'try' if you
think it is clearer.

>> +           "else " \
>> +               "if test ${boot_fdt} = auto; then " \
>> +                   "bootm; " \
>> +               "else " \
>> +                   "echo ERROR: Cannot load the DT, aborting...; " \
>> +               "fi;\0" \
>> +           "fi;\0" \
>> +       "fi;\0" \
>>     "netargs=setenv bootargs console=${console_mainline},${baudrate} " \
>>         "root=/dev/nfs " \
>>         "ip=dhcp nfsroot=${serverip}:${nfsroot},v3,tcp\0" \
>>     "netboot=echo Booting from net ...; " \
>>         "run netargs; " \
>> -       "dhcp ${uimage}; bootm\0"
>> +       "if test ${ip_dyn} = yes; then " \
>> +           "setenv get_cmd dhcp; " \
>> +       "else " \
>> +           "setenv get_cmd tftp; " \
>> +       "fi;\0" \
>> +       ${get_cmd} ${uimage}; " \
>
> This does not work - you must use "run ${get_cmd}"

I see, good.

>> +       "if test ${boot_fdt} = no; then " \
>> +           "bootm; " \
>> +       "else " \
>> +           "if ${get_cmd} ${ftd_addr} ${ftd_file}; then "  \
>> +               "bootm ${loadaddr} - ${ftd_addr}; " \
>> +           "else " \
>> +               "if test ${boot_fdt} = auto; then " \
>> +                   "bootm; " \
>> +               "else " \
>> +                   "echo ERROR: Cannot load the DT, aborting...; " \
>> +               "fi;\0" \
>> +           "fi;\0" \
>> +       "fi;\0"
>>
>>  #define CONFIG_BOOTCOMMAND \
>>     "mmc dev ${mmcdev}; if mmc rescan; then " \
>>
>> What do you think? I don't have the board
>
> I can test it on mx53loco

I'll redo it and send.

--
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
diff mbox

Patch

diff --git a/include/configs/mx28evk.h b/include/configs/mx28evk.h
index ac9522f..45ac072 100644
--- a/include/configs/mx28evk.h
+++ b/include/configs/mx28evk.h
@@ -290,6 +290,10 @@ 
    "uimage=uImage\0" \
    "console_fsl=ttyAM0\0" \
    "console_mainline=ttyAMA0\0" \
+   "ftd_file=imx28-evk.dtb\0" \
+   "ftd_addr=0x41000000\0" \
+   "boot_fdt=auto\0" \
+   "ip_dyn=yes\0" \
    "mmcdev=0\0" \
    "mmcpart=2\0" \
    "mmcroot=/dev/mmcblk0p3 rw rootwait\0" \
@@ -302,13 +306,43 @@ 
    "loaduimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${uimage}\0" \
    "mmcboot=echo Booting from mmc ...; " \
        "run mmcargs; " \
-       "bootm\0" \
+       "if test ${boot_fdt} = no; then " \
+           "bootm; " \
+       "else " \
+           "if fatload mmc ${mmcdev}:${mmcpart} ${ftd_addr}
${ftd_file}; then " \
+               "bootm ${loadaddr} - ${ftd_addr}; " \
+           "else " \
+               "if test ${boot_fdt} = auto; then " \
+                   "bootm; " \
+               "else " \
+                   "echo ERROR: Cannot load the DT, aborting...; " \
+               "fi;\0" \
+           "fi;\0" \
+       "fi;\0" \
    "netargs=setenv bootargs console=${console_mainline},${baudrate} " \
        "root=/dev/nfs " \
        "ip=dhcp nfsroot=${serverip}:${nfsroot},v3,tcp\0" \
    "netboot=echo Booting from net ...; " \
        "run netargs; " \
-       "dhcp ${uimage}; bootm\0"
+       "if test ${ip_dyn} = yes; then " \
+           "setenv get_cmd dhcp; " \
+       "else " \
+           "setenv get_cmd tftp; " \
+       "fi;\0" \
+       ${get_cmd} ${uimage}; " \
+       "if test ${boot_fdt} = no; then " \
+           "bootm; " \
+       "else " \
+           "if ${get_cmd} ${ftd_addr} ${ftd_file}; then "  \
+               "bootm ${loadaddr} - ${ftd_addr}; " \
+           "else " \
+               "if test ${boot_fdt} = auto; then " \
+                   "bootm; " \
+               "else " \
+                   "echo ERROR: Cannot load the DT, aborting...; " \
+               "fi;\0" \
+           "fi;\0" \
+       "fi;\0"

 #define CONFIG_BOOTCOMMAND \