[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved
From: Anton Leontiev aleontiev@elvees.com
Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag") states, that if FDT file cannot be retrieved then FDT packaged in firmware should be used.
If FDT file cannot be retrieved and it is specified explicitly using FDT keyword then the label is skipped. If it cannot be found in FDTDIR then internal FDT is tried first.
Signed-off-by: Anton Leontiev aleontiev@elvees.com --- cmd/pxe.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/cmd/pxe.c b/cmd/pxe.c index 2059975446..28390c114c 100644 --- a/cmd/pxe.c +++ b/cmd/pxe.c @@ -795,9 +795,13 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label) int err = get_relfile_envaddr(cmdtp, fdtfile, "fdt_addr_r"); free(fdtfilefree); if (err < 0) { - printf("Skipping %s for failure retrieving fdt\n", - label->name); - goto cleanup; + bootm_argv[3] = NULL; + + if (label->fdt) { + printf("Skipping %s for failure retrieving FDT\n", + label->name); + goto cleanup; + } } } else { bootm_argv[3] = NULL;
On 8/23/19 8:40 AM, Anton Leontiev wrote:
From: Anton Leontiev aleontiev@elvees.com
Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag") states, that if FDT file cannot be retrieved then FDT packaged in firmware should be used.
It's not meant to say that. I believe the part of the description you're referring to is:
if no FDT file was loaded, and $fdtaddr is set: # This indicates an FDT packaged with firmware use the FDT at $fdtaddr
That wasn't meant to say anything about "if there was an error loading the FDT file", but rather is meant to mean "if no FDT file was loaded because extlinux.conf contained no fdt or fdtdir statement". Nothing there is intended to refer to errors loading a specified FDT file.
If FDT file cannot be retrieved and it is specified explicitly using FDT keyword then the label is skipped. If it cannot be found in FDTDIR then internal FDT is tried first.
This makes the fdt/fdtdir keywords work differently. I don't think we want that.
What specific problem are you trying to solve?
And note that if we do change anything here, we should update the comment at around line 730, which describes the algorithm that's implemented.
2019-08-26 at 18:59, Stephen Warren swarren@wwwdotorg.org:
On 8/23/19 8:40 AM, Anton Leontiev wrote:
From: Anton Leontiev aleontiev@elvees.com
Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag") states, that if FDT file cannot be retrieved then FDT packaged in firmware should be used.
It's not meant to say that. I believe the part of the description you're referring to is:
if no FDT file was loaded, and $fdtaddr is set: # This indicates an FDT packaged with firmware use the FDT at $fdtaddrThat wasn't meant to say anything about "if there was an error loading the FDT file", but rather is meant to mean "if no FDT file was loaded because extlinux.conf contained no fdt or fdtdir statement". Nothing there is intended to refer to errors loading a specified FDT file.
Indeed, I read it strictly as "if no FDT file was loaded" regardless the reason. Thank you for clarification.
If FDT file cannot be retrieved and it is specified explicitly using FDT keyword then the label is skipped. If it cannot be found in FDTDIR then internal FDT is tried first.
This makes the fdt/fdtdir keywords work differently. I don't think we want that.
FDT will work as always. FDTDIR will be less strict. It doesn't specify exact file to be loaded, that's why it should not fail if there is no such file.
What specific problem are you trying to solve?
We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf to support several boards. But some boards have FDT embedded in U-Boot and it is't present in FDTDIR. In such configuration U-Boot fails to boot an entry, despite no exact FDT is specified in it. Distribution itself is designed to work on any board.
And note that if we do change anything here, we should update the comment at around line 730, which describes the algorithm that's implemented.
Thank you, I'll update the comment.
On 8/29/19 5:20 AM, Anton Leontiev wrote:
2019-08-26 at 18:59, Stephen Warren swarren@wwwdotorg.org:
On 8/23/19 8:40 AM, Anton Leontiev wrote:
From: Anton Leontiev aleontiev@elvees.com
Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag") states, that if FDT file cannot be retrieved then FDT packaged in firmware should be used.
It's not meant to say that. I believe the part of the description you're referring to is:
if no FDT file was loaded, and $fdtaddr is set: # This indicates an FDT packaged with firmware use the FDT at $fdtaddrThat wasn't meant to say anything about "if there was an error loading the FDT file", but rather is meant to mean "if no FDT file was loaded because extlinux.conf contained no fdt or fdtdir statement". Nothing there is intended to refer to errors loading a specified FDT file.
...
What specific problem are you trying to solve?
We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf to support several boards. But some boards have FDT embedded in U-Boot and it is't present in FDTDIR. In such configuration U-Boot fails to boot an entry, despite no exact FDT is specified in it. Distribution itself is designed to work on any board.
I lookead at that referenced commit description in full and the code, and I believe what you want is for U-Boot to set fdt_addr to the location of the in-RAM DT, and leave fdt_addr_r unset. This will indicate to the pxe code that no FDT should be loaded when parsing extlinux.conf, but instead to use fdt_addr directly.
Does that work for you, or does it break some other script/use-case on this board?
чт, 29 авг. 2019 г. в 23:35, Stephen Warren swarren@wwwdotorg.org:
On 8/29/19 5:20 AM, Anton Leontiev wrote:
2019-08-26 at 18:59, Stephen Warren swarren@wwwdotorg.org: We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf to support several boards. But some boards have FDT embedded in U-Boot and it is't present in FDTDIR. In such configuration U-Boot fails to boot an entry, despite no exact FDT is specified in it. Distribution itself is designed to work on any board.
I lookead at that referenced commit description in full and the code, and I believe what you want is for U-Boot to set fdt_addr to the location of the in-RAM DT, and leave fdt_addr_r unset. This will indicate to the pxe code that no FDT should be loaded when parsing extlinux.conf, but instead to use fdt_addr directly.
Does that work for you, or does it break some other script/use-case on this board?
Indeed, it's a possible option. However, if fdt_addr_r is not set, user can't override embedded FDT using extlinux.conf. README.distro says about fdt_addr_r: "This is mandatory even when fdt_addr is provided, since extlinux.conf must always be able to provide a DTB which overrides any copy provided by the HW."
So it should be considered as a workaround rather than a solution.
Best regards, Anton Leontiev
On 8/31/19 1:52 PM, Anton Leontiev wrote:
чт, 29 авг. 2019 г. в 23:35, Stephen Warren swarren@wwwdotorg.org:
On 8/29/19 5:20 AM, Anton Leontiev wrote:
2019-08-26 at 18:59, Stephen Warren swarren@wwwdotorg.org: We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf to support several boards. But some boards have FDT embedded in U-Boot and it is't present in FDTDIR. In such configuration U-Boot fails to boot an entry, despite no exact FDT is specified in it. Distribution itself is designed to work on any board.
I lookead at that referenced commit description in full and the code, and I believe what you want is for U-Boot to set fdt_addr to the location of the in-RAM DT, and leave fdt_addr_r unset. This will indicate to the pxe code that no FDT should be loaded when parsing extlinux.conf, but instead to use fdt_addr directly.
Does that work for you, or does it break some other script/use-case on this board?
Indeed, it's a possible option. However, if fdt_addr_r is not set, user can't override embedded FDT using extlinux.conf. README.distro says about fdt_addr_r: "This is mandatory even when fdt_addr is provided, since extlinux.conf must always be able to provide a DTB which overrides any copy provided by the HW."
So it should be considered as a workaround rather than a solution.
OK, I guess that makes sense.
I suppose it's not reasonable to ask that extlinux.conf doesn't contain an FDTDIR statement in the case where the specific board has a built-in DT, since the extlinux.conf content is supposed to be generic, and not tailored to the current HW.
From: Anton Leontiev aleontiev@elvees.com
As FDTDIR label doesn't specify exact file to be loaded, it should not fail if no file exists in the directory. In this case try to boot with internal FDT if it exists.
Signed-off-by: Anton Leontiev aleontiev@elvees.com --- cmd/pxe.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/cmd/pxe.c b/cmd/pxe.c index 2059975446..1eec6d29bf 100644 --- a/cmd/pxe.c +++ b/cmd/pxe.c @@ -727,11 +727,14 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
/* * fdt usage is optional: - * It handles the following scenarios. All scenarios are exclusive + * It handles the following scenarios. * - * Scenario 1: If fdt_addr_r specified and "fdt" label is defined in - * pxe file, retrieve fdt blob from server. Pass fdt_addr_r to bootm, - * and adjust argc appropriately. + * Scenario 1: If fdt_addr_r specified and "fdt" or "fdtdir" label is + * defined in pxe file, retrieve fdt blob from server. Pass fdt_addr_r to + * bootm, and adjust argc appropriately. + * + * If retrieve fails and no exact fdt blob is specified in pxe file with + * "fdt" label, try Scenario 2. * * Scenario 2: If there is an fdt_addr specified, pass it along to * bootm, and adjust argc appropriately. @@ -795,9 +798,13 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label) int err = get_relfile_envaddr(cmdtp, fdtfile, "fdt_addr_r"); free(fdtfilefree); if (err < 0) { - printf("Skipping %s for failure retrieving fdt\n", - label->name); - goto cleanup; + bootm_argv[3] = NULL; + + if (label->fdt) { + printf("Skipping %s for failure retrieving FDT\n", + label->name); + goto cleanup; + } } } else { bootm_argv[3] = NULL;
On 9/3/19 1:52 AM, Anton Leontiev wrote:
From: Anton Leontiev aleontiev@elvees.com
As FDTDIR label doesn't specify exact file to be loaded, it should not fail if no file exists in the directory. In this case try to boot with internal FDT if it exists.
Signed-off-by: Anton Leontiev aleontiev@elvees.com
cmd/pxe.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/cmd/pxe.c b/cmd/pxe.c index 2059975446..1eec6d29bf 100644 --- a/cmd/pxe.c +++ b/cmd/pxe.c @@ -727,11 +727,14 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
/* * fdt usage is optional:
* It handles the following scenarios. All scenarios are exclusive
* It handles the following scenarios.
* Scenario 1: If fdt_addr_r specified and "fdt" label is defined in* pxe file, retrieve fdt blob from server. Pass fdt_addr_r to bootm,* and adjust argc appropriately.
* Scenario 1: If fdt_addr_r specified and "fdt" or "fdtdir" label is* defined in pxe file, retrieve fdt blob from server. Pass fdt_addr_r to* bootm, and adjust argc appropriately.** If retrieve fails and no exact fdt blob is specified in pxe file with* "fdt" label, try Scenario 2.
Is it possible/sensible to distinguish between "file not found" and "error during retrieval"? "File not found" indicates the case you care about, and continuing to use a built-in DT makes sense here. "Error during retrieval" indicates that the file was found, and hence really should be use, yet couldn't be due to download failure. In this case, I wonder if we shouldn't outright fail this boot option, just like the existing code does?
But either way, I suppose this patch is reasonable.
* Scenario 2: If there is an fdt_addr specified, pass it along to * bootm, and adjust argc appropriately.@@ -795,9 +798,13 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label) int err = get_relfile_envaddr(cmdtp, fdtfile, "fdt_addr_r"); free(fdtfilefree); if (err < 0) {
printf("Skipping %s for failure retrieving fdt\n",label->name);goto cleanup;
bootm_argv[3] = NULL;if (label->fdt) {printf("Skipping %s for failure retrieving FDT\n",label->name);goto cleanup;} }
2019-09-03 19:18, Stephen Warren swarren@wwwdotorg.org:
Is it possible/sensible to distinguish between "file not found" and "error during retrieval"? "File not found" indicates the case you care about, and continuing to use a built-in DT makes sense here. "Error during retrieval" indicates that the file was found, and hence really should be use, yet couldn't be due to download failure. In this case, I wonder if we shouldn't outright fail this boot option, just like the existing code does?
But either way, I suppose this patch is reasonable.
As I see, for example from do_get_ext2(), currently it is not possible to distinguish between "file not found" and "error during retrieval". It seems possible to update all do_get_*() functions to check file existence before retrieving, but it will require extensive testing and I prefer this to be changed independently to current patch.
Best regards,
2019-09-03 10:52, Anton Leontiev scileont@gmail.com:
From: Anton Leontiev aleontiev@elvees.com
As FDTDIR label doesn't specify exact file to be loaded, it should not fail if no file exists in the directory. In this case try to boot with internal FDT if it exists.
Signed-off-by: Anton Leontiev aleontiev@elvees.com
Tom, could you take a look at my patch during the merge window? Or should I pass it through some custodian?
Best regards, Anton Leontiev
On Tue, Sep 03, 2019 at 10:52:24AM +0300, Anton Leontiev wrote:
From: Anton Leontiev aleontiev@elvees.com
As FDTDIR label doesn't specify exact file to be loaded, it should not fail if no file exists in the directory. In this case try to boot with internal FDT if it exists.
Signed-off-by: Anton Leontiev aleontiev@elvees.com
Sorry for the delay, applied to u-boot/next, thanks!
participants (3)
-
Anton Leontiev -
Stephen Warren -
Tom Rini