Hi Jason
On 6 September 2012 21:07, Jason Hobbs <jason.ho...@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.kash...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
