[U-Boot] cmd: pxe: Use internal FDT if external one cannot be retrieved
diff mbox series

Message ID 20190823144043.26792-1-scileont@gmail.com
State Superseded
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot] cmd: pxe: Use internal FDT if external one cannot be retrieved
Related show

Commit Message

Anton Leontiev Aug. 23, 2019, 2:40 p.m. UTC
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(-)

Comments

Stephen Warren Aug. 26, 2019, 3:59 p.m. UTC | #1
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.
Anton Leontiev Aug. 29, 2019, 11:20 a.m. UTC | #2
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 $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.

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.
Stephen Warren Aug. 29, 2019, 8:35 p.m. UTC | #3
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 $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.
...
>> 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?
Anton Leontiev Aug. 31, 2019, 7:52 p.m. UTC | #4
чт, 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
Stephen Warren Sept. 3, 2019, 4:16 p.m. UTC | #5
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.

Patch
diff mbox series

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;