| Message ID | 1346910005-19356-1-git-send-email-chander.kashyap@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 > > Signed-off-by: Chander Kashyap <chander.kashyap@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 > > 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. 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. > > 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 >
Hi Jason On 6 September 2012 21:07, Jason Hobbs <jason.hobbs@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@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.
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. 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. + 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.
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. Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> --- Changes in v2: - Removed the duplicate code. changes in v3: - Added documentation for "fdt" lable in doc/README.pxe common/cmd_pxe.c | 23 +++++++++++++++++++++++ doc/README.pxe | 10 ++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-)