[U-Boot] [PATCH v3] PXE: FDT: Add support for fdt in PXE

[Chander Kashyap]

Hi Jason

On 6 September 2012 21:07, Jason Hobbs <jason.hobbs at calxeda.com> wrote:
> Chander,
>
> Comments inline.
>
> On Thu, Sep 06, 2012 at 01:40:04AM -0400, Chander Kashyap wrote:
>> Now DT support is becomming common for all new SoC's. Hence it is better to
>> have option for getting specific FDT from the remote server.
>>
>> This patch adds support for new lable i.e. fdt. If fdt_addr is specified
>> then load fdt blob from the remote server to fdt_address.
>
> If a fdt label is provided AND fdt_addr is specified, then load the blob
> from the remote server to fdt_addr. fdt_addr alone still works on its
> own if the fdt_blob has already been loaded some other way
>
Yes will add it.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap at linaro.org>
>> ---
>> Changes in v2:
>>       - Removed the duplicate code.
>> changes in v3:
>>       - Added documentation for "fdt" lable in doc/README.pxe
>
> s/lable/label - fix globally
Will fix it
>
>>
>>  common/cmd_pxe.c |   23 +++++++++++++++++++++++
>>  doc/README.pxe   |   10 ++++++++--
>>  2 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
>> index 6b31dea..0c81e08 100644
>> --- a/common/cmd_pxe.c
>> +++ b/common/cmd_pxe.c
>> @@ -450,6 +450,7 @@ struct pxe_label {
>>       char *kernel;
>>       char *append;
>>       char *initrd;
>> +     char *fdt;
>>       int attempted;
>>       int localboot;
>>       struct list_head list;
>> @@ -517,6 +518,9 @@ static void label_destroy(struct pxe_label *label)
>>       if (label->initrd)
>>               free(label->initrd);
>>
>> +     if (label->fdt)
>> +             free(label->fdt);
>> +
>>       free(label);
>>  }
>>
>> @@ -541,6 +545,9 @@ static void label_print(void *data)
>>
>>       if (label->initrd)
>>               printf("\t\tinitrd: %s\n", label->initrd);
>> +
>> +     if (label->fdt)
>> +             printf("\tfdt: %s\n", label->fdt);
>>  }
>>
>>  /*
>> @@ -633,6 +640,15 @@ static void label_boot(struct pxe_label *label)
>>        */
>>       bootm_argv[3] = getenv("fdt_addr");
>>
>> +     /* if fdt label is defined then get fdt from server */
>> +     if (bootm_argv[3] && label->fdt) {
>> +             if (get_relfile_envaddr(label->fdt, "fdt_addr") < 0) {
>> +                     printf("Skipping %s for failure retrieving fdt\n",
>> +                                     label->name);
>> +                     return;
>> +             }
>> +     }
>> +
>>       if (bootm_argv[3])
>>               bootm_argc = 4;
>>
>> @@ -658,6 +674,7 @@ enum token_type {
>>       T_DEFAULT,
>>       T_PROMPT,
>>       T_INCLUDE,
>> +     T_FDT,
>>       T_INVALID
>>  };
>>
>> @@ -685,6 +702,7 @@ static const struct token keywords[] = {
>>       {"append", T_APPEND},
>>       {"initrd", T_INITRD},
>>       {"include", T_INCLUDE},
>> +     {"fdt", T_FDT},
>>       {NULL, T_INVALID}
>>  };
>>
>> @@ -1074,6 +1092,11 @@ static int parse_label(char **c, struct pxe_menu *cfg)
>>                               err = parse_sliteral(c, &label->initrd);
>>                       break;
>>
>> +             case T_FDT:
>> +                     if (!label->fdt)
>> +                             err = parse_sliteral(c, &label->fdt);
>> +                     break;
>> +
>>               case T_LOCALBOOT:
>>                       err = parse_integer(c, &label->localboot);
>>                       break;
>> diff --git a/doc/README.pxe b/doc/README.pxe
>> index 2bbf53d..835ca5a 100644
>> --- a/doc/README.pxe
>> +++ b/doc/README.pxe
>> @@ -93,8 +93,9 @@ pxe boot
>>       be passed to the bootm command to boot the kernel. These environment
>>       variables are required to be set.
>>
>> -     fdt_addr - the location of a fdt blob. If this is set, it will be passed
>> -     to bootm when booting a kernel.
>> +     fdt_addr - locations in RAM at which 'pxe boot' will store the fdt blob
>> +     it retrieves from tftp, if "fdt" lable is defined in pxe file. If this is
>> +     set, it will be passed to bootm when booting a kernel.
>
> Thinking about this some more, I don't think you can use fdt_addr as the
> place to store the blob. fdt_addr isn't garaunteed to be writeable RAM -
> it could be a read only non volatile memory like NOR flash.
>
> If you notice, all of the other tftp retrievals go to "_r" suffixed
> variables, which are garaunteed (by convention) to be in writeable RAM.
>
> You may need to take an approach where an addition fdt_addr_r variable
> is used to point at the location to store the fdt blob.
>
Sure, I will take care for this
> Your description also makes it less clear that if fdt_addr is set, it
> points to the blob, whether or not it was retrieved from tftp.
Will make it more clear

>
>>
>>  pxe file format
>>  ===============
>> @@ -156,6 +157,11 @@ initrd <path>        - if this label is chosen, use tftp to retrieve the initrd
>>                     the initrd_addr_r environment variable, and that address
>>                     will be passed to bootm.
>>
>> +fdt <path>       - if this label is chosen, use tftp to retrieve the fdt blob
>> +                   at <path>. it will be stored at the address indicated in
>> +                   the fdt_addr environment variable, and that address will
>> +                   be passed to bootm.
>> +
>
> again, this should be changed to fdt_addr_r.
>
>
>>  localboot <flag>    - Run the command defined by "localcmd" in the environment.
>>                     <flag> is ignored and is only here to match the syntax of
>>                     PXELINUX config files.
>> --
>> 1.7.9.5
>>
Thanks for the comments.


-- 
with warm regards,
Chander Kashyap