All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
@ 2021-10-14  8:40 Peter Hoyes
  2021-10-14 18:24 ` Simon Glass
  2022-05-18  8:40 ` Neil Armstrong
  0 siblings, 2 replies; 13+ messages in thread
From: Peter Hoyes @ 2021-10-14  8:40 UTC (permalink / raw)
  To: u-boot
  Cc: bmeng.cn, sjg, kory.maincent, andre.przywara, diego.sueiro, Peter Hoyes

From: Peter Hoyes <Peter.Hoyes@arm.com>

If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
U-Boot. When using PXE, if no fdt is defined in the menu file, and
there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.

We are developing board support for the Armv8r64 FVP using
config_distro_bootcmd. We are also using OF_BOARD and would like the
PXE boot option to default to the fdt provided by board_fdt_blob_setup.

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
 cmd/pxe_utils.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
index 067c24e5ff..8f8e69ca97 100644
--- a/cmd/pxe_utils.c
+++ b/cmd/pxe_utils.c
@@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
 	 * Scenario 2: If there is an fdt_addr specified, pass it along to
 	 * bootm, and adjust argc appropriately.
 	 *
-	 * Scenario 3: fdt blob is not available.
+	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
+	 * bootm, and adjust argc appropriately.
+	 *
+	 * Scenario 4: fdt blob is not available.
 	 */
 	bootm_argv[3] = env_get("fdt_addr_r");
 
@@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
 	if (!bootm_argv[3])
 		bootm_argv[3] = env_get("fdt_addr");
 
+	if (!bootm_argv[3])
+		bootm_argv[3] = env_get("fdtcontroladdr");
+
 	if (bootm_argv[3]) {
 		if (!bootm_argv[2])
 			bootm_argv[2] = "-";
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-10-14  8:40 [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot Peter Hoyes
@ 2021-10-14 18:24 ` Simon Glass
  2021-10-15 11:20   ` Andre Przywara
  2022-05-18  8:40 ` Neil Armstrong
  1 sibling, 1 reply; 13+ messages in thread
From: Simon Glass @ 2021-10-14 18:24 UTC (permalink / raw)
  To: Peter Hoyes
  Cc: U-Boot Mailing List, Bin Meng, Kory Maincent, Andre Przywara,
	diego.sueiro

Hi Peter,

On Thu, 14 Oct 2021 at 02:40, Peter Hoyes <peter.hoyes@arm.com> wrote:
>
> From: Peter Hoyes <Peter.Hoyes@arm.com>
>
> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> U-Boot. When using PXE, if no fdt is defined in the menu file, and
> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
>
> We are developing board support for the Armv8r64 FVP using
> config_distro_bootcmd. We are also using OF_BOARD and would like the
> PXE boot option to default to the fdt provided by board_fdt_blob_setup.
>
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> ---
>  cmd/pxe_utils.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> index 067c24e5ff..8f8e69ca97 100644
> --- a/cmd/pxe_utils.c
> +++ b/cmd/pxe_utils.c
> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>          * Scenario 2: If there is an fdt_addr specified, pass it along to
>          * bootm, and adjust argc appropriately.
>          *
> -        * Scenario 3: fdt blob is not available.
> +        * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> +        * bootm, and adjust argc appropriately.
> +        *
> +        * Scenario 4: fdt blob is not available.
>          */
>         bootm_argv[3] = env_get("fdt_addr_r");
>
> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>         if (!bootm_argv[3])
>                 bootm_argv[3] = env_get("fdt_addr");
>
> +       if (!bootm_argv[3])
> +               bootm_argv[3] = env_get("fdtcontroladdr");
> +
>         if (bootm_argv[3]) {
>                 if (!bootm_argv[2])
>                         bootm_argv[2] = "-";
> --
> 2.25.1
>

This is a bit strange as fdtcontroladdr is the control dtb that U-Boot
uses. Can you not set fdt_addr to that, if you need to? Is it not
already set to that?

Also, there is a clean-up series for this code outstanding. I will
send v3 and copy you, if you are able to review it.

Regards,
Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-10-14 18:24 ` Simon Glass
@ 2021-10-15 11:20   ` Andre Przywara
  2021-10-24 19:53     ` Simon Glass
  0 siblings, 1 reply; 13+ messages in thread
From: Andre Przywara @ 2021-10-15 11:20 UTC (permalink / raw)
  To: Simon Glass
  Cc: Peter Hoyes, U-Boot Mailing List, Bin Meng, Kory Maincent, diego.sueiro

On Thu, 14 Oct 2021 12:24:45 -0600
Simon Glass <sjg@chromium.org> wrote:

Hi Simon,

> On Thu, 14 Oct 2021 at 02:40, Peter Hoyes <peter.hoyes@arm.com> wrote:
> >
> > From: Peter Hoyes <Peter.Hoyes@arm.com>
> >
> > If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> > U-Boot. When using PXE, if no fdt is defined in the menu file, and
> > there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> >
> > We are developing board support for the Armv8r64 FVP using
> > config_distro_bootcmd. We are also using OF_BOARD and would like the
> > PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> >
> > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> > ---
> >  cmd/pxe_utils.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> > index 067c24e5ff..8f8e69ca97 100644
> > --- a/cmd/pxe_utils.c
> > +++ b/cmd/pxe_utils.c
> > @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> >          * Scenario 2: If there is an fdt_addr specified, pass it along to
> >          * bootm, and adjust argc appropriately.
> >          *
> > -        * Scenario 3: fdt blob is not available.
> > +        * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> > +        * bootm, and adjust argc appropriately.
> > +        *
> > +        * Scenario 4: fdt blob is not available.
> >          */
> >         bootm_argv[3] = env_get("fdt_addr_r");
> >
> > @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> >         if (!bootm_argv[3])
> >                 bootm_argv[3] = env_get("fdt_addr");
> >
> > +       if (!bootm_argv[3])
> > +               bootm_argv[3] = env_get("fdtcontroladdr");
> > +
> >         if (bootm_argv[3]) {
> >                 if (!bootm_argv[2])
> >                         bootm_argv[2] = "-";
> > --
> > 2.25.1
> >  
> 
> This is a bit strange as fdtcontroladdr is the control dtb that U-Boot
> uses.

Yes, that is the idea. This DTB should be as good as any, and this is the
last check before giving up on DTs completely, so "U-Boot's DTB" (actually
there is no such thing) is better than none at all. In fact EFI boot does
the very same thing:
https://source.denx.de/u-boot/u-boot/-/blob/master/include/config_distro_bootcmd.h#L144-L148

> Can you not set fdt_addr to that, if you need to? Is it not
> already set to that?

According to doc/README.distro, fdt_addr is reserved for DTBs residing at
a (fixed?) ROM address. We don't really know yet where the DT comes from,
the cleanest seems to be to be passed down from TF-A, in which case this
would definitely not be ROM. board_fdt_blob_setup() knows this address,
and this is what will be written to $fdtcontroladdr.

FWIW, this patch looks good to me.

Cheers,
Andre

> Also, there is a clean-up series for this code outstanding. I will
> send v3 and copy you, if you are able to review it.
> 
> Regards,
> Simon


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-10-15 11:20   ` Andre Przywara
@ 2021-10-24 19:53     ` Simon Glass
  2021-10-28 18:55       ` Ramon Fried
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2021-10-24 19:53 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Peter Hoyes, U-Boot Mailing List, Bin Meng, Kory Maincent, diego.sueiro

Hi,

On Fri, 15 Oct 2021 at 05:21, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 14 Oct 2021 12:24:45 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> > On Thu, 14 Oct 2021 at 02:40, Peter Hoyes <peter.hoyes@arm.com> wrote:
> > >
> > > From: Peter Hoyes <Peter.Hoyes@arm.com>
> > >
> > > If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> > > U-Boot. When using PXE, if no fdt is defined in the menu file, and
> > > there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> > >
> > > We are developing board support for the Armv8r64 FVP using
> > > config_distro_bootcmd. We are also using OF_BOARD and would like the
> > > PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> > >
> > > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> > > ---
> > >  cmd/pxe_utils.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> > > index 067c24e5ff..8f8e69ca97 100644
> > > --- a/cmd/pxe_utils.c
> > > +++ b/cmd/pxe_utils.c
> > > @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> > >          * Scenario 2: If there is an fdt_addr specified, pass it along to
> > >          * bootm, and adjust argc appropriately.
> > >          *
> > > -        * Scenario 3: fdt blob is not available.
> > > +        * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> > > +        * bootm, and adjust argc appropriately.
> > > +        *
> > > +        * Scenario 4: fdt blob is not available.
> > >          */
> > >         bootm_argv[3] = env_get("fdt_addr_r");
> > >
> > > @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> > >         if (!bootm_argv[3])
> > >                 bootm_argv[3] = env_get("fdt_addr");
> > >
> > > +       if (!bootm_argv[3])
> > > +               bootm_argv[3] = env_get("fdtcontroladdr");
> > > +
> > >         if (bootm_argv[3]) {
> > >                 if (!bootm_argv[2])
> > >                         bootm_argv[2] = "-";
> > > --
> > > 2.25.1
> > >
> >
> > This is a bit strange as fdtcontroladdr is the control dtb that U-Boot
> > uses.
>
> Yes, that is the idea. This DTB should be as good as any, and this is the
> last check before giving up on DTs completely, so "U-Boot's DTB" (actually
> there is no such thing) is better than none at all. In fact EFI boot does
> the very same thing:
> https://source.denx.de/u-boot/u-boot/-/blob/master/include/config_distro_bootcmd.h#L144-L148
>
> > Can you not set fdt_addr to that, if you need to? Is it not
> > already set to that?
>
> According to doc/README.distro, fdt_addr is reserved for DTBs residing at
> a (fixed?) ROM address. We don't really know yet where the DT comes from,
> the cleanest seems to be to be passed down from TF-A, in which case this
> would definitely not be ROM. board_fdt_blob_setup() knows this address,
> and this is what will be written to $fdtcontroladdr.
>
> FWIW, this patch looks good to me.

OK.

But note that OF_BOARD is going to become an option, not something
that let's us leave out the DT. Board vendors must provide a DT in
U-Boot. So long as that is understood:

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simom

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-10-24 19:53     ` Simon Glass
@ 2021-10-28 18:55       ` Ramon Fried
  2021-11-08 16:52         ` Peter Hoyes
  0 siblings, 1 reply; 13+ messages in thread
From: Ramon Fried @ 2021-10-28 18:55 UTC (permalink / raw)
  To: Simon Glass
  Cc: Andre Przywara, Peter Hoyes, U-Boot Mailing List, Bin Meng,
	Kory Maincent, diego.sueiro

On Sun, Oct 24, 2021 at 10:56 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Fri, 15 Oct 2021 at 05:21, Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Thu, 14 Oct 2021 12:24:45 -0600
> > Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Simon,
> >
> > > On Thu, 14 Oct 2021 at 02:40, Peter Hoyes <peter.hoyes@arm.com> wrote:
> > > >
> > > > From: Peter Hoyes <Peter.Hoyes@arm.com>
> > > >
> > > > If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> > > > U-Boot. When using PXE, if no fdt is defined in the menu file, and
> > > > there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> > > >
> > > > We are developing board support for the Armv8r64 FVP using
> > > > config_distro_bootcmd. We are also using OF_BOARD and would like the
> > > > PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> > > >
> > > > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> > > > ---
> > > >  cmd/pxe_utils.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> > > > index 067c24e5ff..8f8e69ca97 100644
> > > > --- a/cmd/pxe_utils.c
> > > > +++ b/cmd/pxe_utils.c
> > > > @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> > > >          * Scenario 2: If there is an fdt_addr specified, pass it along to
> > > >          * bootm, and adjust argc appropriately.
> > > >          *
> > > > -        * Scenario 3: fdt blob is not available.
> > > > +        * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> > > > +        * bootm, and adjust argc appropriately.
> > > > +        *
> > > > +        * Scenario 4: fdt blob is not available.
> > > >          */
> > > >         bootm_argv[3] = env_get("fdt_addr_r");
> > > >
> > > > @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> > > >         if (!bootm_argv[3])
> > > >                 bootm_argv[3] = env_get("fdt_addr");
> > > >
> > > > +       if (!bootm_argv[3])
> > > > +               bootm_argv[3] = env_get("fdtcontroladdr");
> > > > +
> > > >         if (bootm_argv[3]) {
> > > >                 if (!bootm_argv[2])
> > > >                         bootm_argv[2] = "-";
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > This is a bit strange as fdtcontroladdr is the control dtb that U-Boot
> > > uses.
> >
> > Yes, that is the idea. This DTB should be as good as any, and this is the
> > last check before giving up on DTs completely, so "U-Boot's DTB" (actually
> > there is no such thing) is better than none at all. In fact EFI boot does
> > the very same thing:
> > https://source.denx.de/u-boot/u-boot/-/blob/master/include/config_distro_bootcmd.h#L144-L148
> >
> > > Can you not set fdt_addr to that, if you need to? Is it not
> > > already set to that?
> >
> > According to doc/README.distro, fdt_addr is reserved for DTBs residing at
> > a (fixed?) ROM address. We don't really know yet where the DT comes from,
> > the cleanest seems to be to be passed down from TF-A, in which case this
> > would definitely not be ROM. board_fdt_blob_setup() knows this address,
> > and this is what will be written to $fdtcontroladdr.
> >
> > FWIW, this patch looks good to me.
>
> OK.
>
> But note that OF_BOARD is going to become an option, not something
> that let's us leave out the DT. Board vendors must provide a DT in
> U-Boot. So long as that is understood:
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Regards,
> Simom
applied to u-boot-net/next
Thanks,
Ramon.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-10-28 18:55       ` Ramon Fried
@ 2021-11-08 16:52         ` Peter Hoyes
  2021-11-09  6:26           ` Ramon Fried
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Hoyes @ 2021-11-08 16:52 UTC (permalink / raw)
  To: Ramon Fried; +Cc: Andre Przywara, U-Boot Mailing List, diego.sueiro

On 28/10/2021 19:55, Ramon Fried wrote:
> On Sun, Oct 24, 2021 at 10:56 PM Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On Fri, 15 Oct 2021 at 05:21, Andre Przywara <andre.przywara@arm.com> wrote:
>>> On Thu, 14 Oct 2021 12:24:45 -0600
>>> Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi Simon,
>>>
>>>> On Thu, 14 Oct 2021 at 02:40, Peter Hoyes <peter.hoyes@arm.com> wrote:
>>>>> From: Peter Hoyes <Peter.Hoyes@arm.com>
>>>>>
>>>>> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
>>>>> U-Boot. When using PXE, if no fdt is defined in the menu file, and
>>>>> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
>>>>>
>>>>> We are developing board support for the Armv8r64 FVP using
>>>>> config_distro_bootcmd. We are also using OF_BOARD and would like the
>>>>> PXE boot option to default to the fdt provided by board_fdt_blob_setup.
>>>>>
>>>>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
>>>>> ---
>>>>>   cmd/pxe_utils.c | 8 +++++++-
>>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
>>>>> index 067c24e5ff..8f8e69ca97 100644
>>>>> --- a/cmd/pxe_utils.c
>>>>> +++ b/cmd/pxe_utils.c
>>>>> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>>>>>           * Scenario 2: If there is an fdt_addr specified, pass it along to
>>>>>           * bootm, and adjust argc appropriately.
>>>>>           *
>>>>> -        * Scenario 3: fdt blob is not available.
>>>>> +        * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
>>>>> +        * bootm, and adjust argc appropriately.
>>>>> +        *
>>>>> +        * Scenario 4: fdt blob is not available.
>>>>>           */
>>>>>          bootm_argv[3] = env_get("fdt_addr_r");
>>>>>
>>>>> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>>>>>          if (!bootm_argv[3])
>>>>>                  bootm_argv[3] = env_get("fdt_addr");
>>>>>
>>>>> +       if (!bootm_argv[3])
>>>>> +               bootm_argv[3] = env_get("fdtcontroladdr");
>>>>> +
>>>>>          if (bootm_argv[3]) {
>>>>>                  if (!bootm_argv[2])
>>>>>                          bootm_argv[2] = "-";
>>>>> --
>>>>> 2.25.1
>>>>>
>>>> This is a bit strange as fdtcontroladdr is the control dtb that U-Boot
>>>> uses.
>>> Yes, that is the idea. This DTB should be as good as any, and this is the
>>> last check before giving up on DTs completely, so "U-Boot's DTB" (actually
>>> there is no such thing) is better than none at all. In fact EFI boot does
>>> the very same thing:
>>> https://source.denx.de/u-boot/u-boot/-/blob/master/include/config_distro_bootcmd.h#L144-L148
>>>
>>>> Can you not set fdt_addr to that, if you need to? Is it not
>>>> already set to that?
>>> According to doc/README.distro, fdt_addr is reserved for DTBs residing at
>>> a (fixed?) ROM address. We don't really know yet where the DT comes from,
>>> the cleanest seems to be to be passed down from TF-A, in which case this
>>> would definitely not be ROM. board_fdt_blob_setup() knows this address,
>>> and this is what will be written to $fdtcontroladdr.
>>>
>>> FWIW, this patch looks good to me.
>> OK.
>>
>> But note that OF_BOARD is going to become an option, not something
>> that let's us leave out the DT. Board vendors must provide a DT in
>> U-Boot. So long as that is understood:
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Regards,
>> Simom
> applied to u-boot-net/next
> Thanks,
> Ramon.

Hi,

Thanks for accepting this patch. I wondered if you could give an
indication when it will reach u-boot/master?

Cheers,

Peter

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-11-08 16:52         ` Peter Hoyes
@ 2021-11-09  6:26           ` Ramon Fried
  0 siblings, 0 replies; 13+ messages in thread
From: Ramon Fried @ 2021-11-09  6:26 UTC (permalink / raw)
  To: Peter Hoyes; +Cc: Andre Przywara, U-Boot Mailing List, diego.sueiro

On Mon, Nov 8, 2021 at 6:53 PM Peter Hoyes <Peter.Hoyes@arm.com> wrote:
>
> On 28/10/2021 19:55, Ramon Fried wrote:
> > On Sun, Oct 24, 2021 at 10:56 PM Simon Glass <sjg@chromium.org> wrote:
> >> Hi,
> >>
> >> On Fri, 15 Oct 2021 at 05:21, Andre Przywara <andre.przywara@arm.com> wrote:
> >>> On Thu, 14 Oct 2021 12:24:45 -0600
> >>> Simon Glass <sjg@chromium.org> wrote:
> >>>
> >>> Hi Simon,
> >>>
> >>>> On Thu, 14 Oct 2021 at 02:40, Peter Hoyes <peter.hoyes@arm.com> wrote:
> >>>>> From: Peter Hoyes <Peter.Hoyes@arm.com>
> >>>>>
> >>>>> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> >>>>> U-Boot. When using PXE, if no fdt is defined in the menu file, and
> >>>>> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> >>>>>
> >>>>> We are developing board support for the Armv8r64 FVP using
> >>>>> config_distro_bootcmd. We are also using OF_BOARD and would like the
> >>>>> PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> >>>>>
> >>>>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> >>>>> ---
> >>>>>   cmd/pxe_utils.c | 8 +++++++-
> >>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> >>>>> index 067c24e5ff..8f8e69ca97 100644
> >>>>> --- a/cmd/pxe_utils.c
> >>>>> +++ b/cmd/pxe_utils.c
> >>>>> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> >>>>>           * Scenario 2: If there is an fdt_addr specified, pass it along to
> >>>>>           * bootm, and adjust argc appropriately.
> >>>>>           *
> >>>>> -        * Scenario 3: fdt blob is not available.
> >>>>> +        * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> >>>>> +        * bootm, and adjust argc appropriately.
> >>>>> +        *
> >>>>> +        * Scenario 4: fdt blob is not available.
> >>>>>           */
> >>>>>          bootm_argv[3] = env_get("fdt_addr_r");
> >>>>>
> >>>>> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> >>>>>          if (!bootm_argv[3])
> >>>>>                  bootm_argv[3] = env_get("fdt_addr");
> >>>>>
> >>>>> +       if (!bootm_argv[3])
> >>>>> +               bootm_argv[3] = env_get("fdtcontroladdr");
> >>>>> +
> >>>>>          if (bootm_argv[3]) {
> >>>>>                  if (!bootm_argv[2])
> >>>>>                          bootm_argv[2] = "-";
> >>>>> --
> >>>>> 2.25.1
> >>>>>
> >>>> This is a bit strange as fdtcontroladdr is the control dtb that U-Boot
> >>>> uses.
> >>> Yes, that is the idea. This DTB should be as good as any, and this is the
> >>> last check before giving up on DTs completely, so "U-Boot's DTB" (actually
> >>> there is no such thing) is better than none at all. In fact EFI boot does
> >>> the very same thing:
> >>> https://source.denx.de/u-boot/u-boot/-/blob/master/include/config_distro_bootcmd.h#L144-L148
> >>>
> >>>> Can you not set fdt_addr to that, if you need to? Is it not
> >>>> already set to that?
> >>> According to doc/README.distro, fdt_addr is reserved for DTBs residing at
> >>> a (fixed?) ROM address. We don't really know yet where the DT comes from,
> >>> the cleanest seems to be to be passed down from TF-A, in which case this
> >>> would definitely not be ROM. board_fdt_blob_setup() knows this address,
> >>> and this is what will be written to $fdtcontroladdr.
> >>>
> >>> FWIW, this patch looks good to me.
> >> OK.
> >>
> >> But note that OF_BOARD is going to become an option, not something
> >> that let's us leave out the DT. Board vendors must provide a DT in
> >> U-Boot. So long as that is understood:
> >>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>
> >> Regards,
> >> Simom
> > applied to u-boot-net/next
> > Thanks,
> > Ramon.
>
> Hi,
>
> Thanks for accepting this patch. I wondered if you could give an
> indication when it will reach u-boot/master?
>
> Cheers,
>
> Peter
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Follow the pull request I sent to Tom.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2021-10-14  8:40 [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot Peter Hoyes
  2021-10-14 18:24 ` Simon Glass
@ 2022-05-18  8:40 ` Neil Armstrong
  2022-05-18 15:52   ` Tom Rini
  1 sibling, 1 reply; 13+ messages in thread
From: Neil Armstrong @ 2022-05-18  8:40 UTC (permalink / raw)
  To: Peter Hoyes, u-boot, Tom Rini
  Cc: bmeng.cn, sjg, kory.maincent, andre.przywara, diego.sueiro

Hi,

On 14/10/2021 10:40, Peter Hoyes wrote:
> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> U-Boot. When using PXE, if no fdt is defined in the menu file, and
> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> 
> We are developing board support for the Armv8r64 FVP using
> config_distro_bootcmd. We are also using OF_BOARD and would like the
> PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> 
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> ---
>   cmd/pxe_utils.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> index 067c24e5ff..8f8e69ca97 100644
> --- a/cmd/pxe_utils.c
> +++ b/cmd/pxe_utils.c
> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>   	 * Scenario 2: If there is an fdt_addr specified, pass it along to
>   	 * bootm, and adjust argc appropriately.
>   	 *
> -	 * Scenario 3: fdt blob is not available.
> +	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> +	 * bootm, and adjust argc appropriately.
> +	 *
> +	 * Scenario 4: fdt blob is not available.
>   	 */
>   	bootm_argv[3] = env_get("fdt_addr_r");
>   
> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>   	if (!bootm_argv[3])
>   		bootm_argv[3] = env_get("fdt_addr");
>   
> +	if (!bootm_argv[3])
> +		bootm_argv[3] = env_get("fdtcontroladdr");
> +
>   	if (bootm_argv[3]) {
>   		if (!bootm_argv[2])
>   			bootm_argv[2] = "-";

This change makes a regression when using a FIT image as kernel image within an extlinux.conf

Before this change, when a DT wasn't specified, the DT in the FIT image was selected,
not the u-boot DT is selected.

While it should work because the U-Boot DT should work in Linux, it's not always the case.

Before:
Found /extlinux/extlinux.conf
Retrieving file: /extlinux/extlinux.conf
1:	Yocto
Retrieving file: /fitImage
append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
## Loading kernel from FIT Image at 08080000 ...
    Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
    Trying 'kernel-1' kernel subimage
      Description:  Linux kernel
      Type:         Kernel Image
      Compression:  gzip compressed
      Data Start:   0x08080120
      Data Size:    9926956 Bytes = 9.5 MiB
      Architecture: AArch64
      OS:           Linux
      Load Address: 0x01080000
      Entry Point:  0x01080000
      Hash algo:    sha256
      Hash value:   5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
    Verifying Hash Integrity ... sha256+ OK
## Loading fdt from FIT Image at 08080000 ...
    Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
    Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage
      Description:  Flattened Device Tree blob
      Type:         Flat Device Tree
      Compression:  uncompressed
      Data Start:   0x089f7b78
      Data Size:    29092 Bytes = 28.4 KiB
      Architecture: AArch64
      Hash algo:    sha256
      Hash value:   72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67
    Verifying Hash Integrity ... sha256+ OK
    Booting using the fdt blob at 0x89f7b78
    Uncompressing Kernel Image
    Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 ... OK


After:
Found /extlinux/extlinux.conf
Retrieving file: /extlinux/extlinux.conf
1:	Yocto
Retrieving file: /fitImage
append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
## Loading kernel from FIT Image at 08080000 ...
    Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
    Trying 'kernel-1' kernel subimage
      Description:  Linux kernel
      Type:         Kernel Image
      Compression:  gzip compressed
      Data Start:   0x08080120
      Data Size:    9926956 Bytes = 9.5 MiB
      Architecture: AArch64
      OS:           Linux
      Load Address: 0x01080000
      Entry Point:  0x01080000
      Hash algo:    sha256
      Hash value:   5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
    Verifying Hash Integrity ... sha256+ OK
## Flattened Device Tree blob at 7bf40210
    Booting using the fdt blob at 0x7bf40210
    Uncompressing Kernel Image
    Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 ... OK


Note this is the "best in town" boot method in Yocto/OE when using WIC, FIT Image and
generating extlinux.conf from the WIC "bootimg-partition" plugin.

So this change will regress all platforms using this scheme.

In case we only want the FIT image and not DTB, the fdtdir is ommited at:
https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150

Neil

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2022-05-18  8:40 ` Neil Armstrong
@ 2022-05-18 15:52   ` Tom Rini
  2022-05-18 18:15     ` Neil Armstrong
  0 siblings, 1 reply; 13+ messages in thread
From: Tom Rini @ 2022-05-18 15:52 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Peter Hoyes, u-boot, bmeng.cn, sjg, kory.maincent,
	andre.przywara, diego.sueiro

[-- Attachment #1: Type: text/plain, Size: 5577 bytes --]

On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote:
> Hi,
> 
> On 14/10/2021 10:40, Peter Hoyes wrote:
> > From: Peter Hoyes <Peter.Hoyes@arm.com>
> > 
> > If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> > U-Boot. When using PXE, if no fdt is defined in the menu file, and
> > there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> > 
> > We are developing board support for the Armv8r64 FVP using
> > config_distro_bootcmd. We are also using OF_BOARD and would like the
> > PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> > 
> > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> > ---
> >   cmd/pxe_utils.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> > index 067c24e5ff..8f8e69ca97 100644
> > --- a/cmd/pxe_utils.c
> > +++ b/cmd/pxe_utils.c
> > @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> >   	 * Scenario 2: If there is an fdt_addr specified, pass it along to
> >   	 * bootm, and adjust argc appropriately.
> >   	 *
> > -	 * Scenario 3: fdt blob is not available.
> > +	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> > +	 * bootm, and adjust argc appropriately.
> > +	 *
> > +	 * Scenario 4: fdt blob is not available.
> >   	 */
> >   	bootm_argv[3] = env_get("fdt_addr_r");
> > @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> >   	if (!bootm_argv[3])
> >   		bootm_argv[3] = env_get("fdt_addr");
> > +	if (!bootm_argv[3])
> > +		bootm_argv[3] = env_get("fdtcontroladdr");
> > +
> >   	if (bootm_argv[3]) {
> >   		if (!bootm_argv[2])
> >   			bootm_argv[2] = "-";
> 
> This change makes a regression when using a FIT image as kernel image within an extlinux.conf
> 
> Before this change, when a DT wasn't specified, the DT in the FIT image was selected,
> not the u-boot DT is selected.
> 
> While it should work because the U-Boot DT should work in Linux, it's not always the case.
> 
> Before:
> Found /extlinux/extlinux.conf
> Retrieving file: /extlinux/extlinux.conf
> 1:	Yocto
> Retrieving file: /fitImage
> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
> ## Loading kernel from FIT Image at 08080000 ...
>    Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>    Trying 'kernel-1' kernel subimage
>      Description:  Linux kernel
>      Type:         Kernel Image
>      Compression:  gzip compressed
>      Data Start:   0x08080120
>      Data Size:    9926956 Bytes = 9.5 MiB
>      Architecture: AArch64
>      OS:           Linux
>      Load Address: 0x01080000
>      Entry Point:  0x01080000
>      Hash algo:    sha256
>      Hash value:   5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>    Verifying Hash Integrity ... sha256+ OK
> ## Loading fdt from FIT Image at 08080000 ...
>    Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>    Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage
>      Description:  Flattened Device Tree blob
>      Type:         Flat Device Tree
>      Compression:  uncompressed
>      Data Start:   0x089f7b78
>      Data Size:    29092 Bytes = 28.4 KiB
>      Architecture: AArch64
>      Hash algo:    sha256
>      Hash value:   72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67
>    Verifying Hash Integrity ... sha256+ OK
>    Booting using the fdt blob at 0x89f7b78
>    Uncompressing Kernel Image
>    Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 ... OK
> 
> 
> After:
> Found /extlinux/extlinux.conf
> Retrieving file: /extlinux/extlinux.conf
> 1:	Yocto
> Retrieving file: /fitImage
> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
> ## Loading kernel from FIT Image at 08080000 ...
>    Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>    Trying 'kernel-1' kernel subimage
>      Description:  Linux kernel
>      Type:         Kernel Image
>      Compression:  gzip compressed
>      Data Start:   0x08080120
>      Data Size:    9926956 Bytes = 9.5 MiB
>      Architecture: AArch64
>      OS:           Linux
>      Load Address: 0x01080000
>      Entry Point:  0x01080000
>      Hash algo:    sha256
>      Hash value:   5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>    Verifying Hash Integrity ... sha256+ OK
> ## Flattened Device Tree blob at 7bf40210
>    Booting using the fdt blob at 0x7bf40210
>    Uncompressing Kernel Image
>    Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 ... OK
> 
> 
> Note this is the "best in town" boot method in Yocto/OE when using WIC, FIT Image and
> generating extlinux.conf from the WIC "bootimg-partition" plugin.
> 
> So this change will regress all platforms using this scheme.
> 
> In case we only want the FIT image and not DTB, the fdtdir is ommited at:
> https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150

Well this is certainly tricky.  Do we be "boring" and say that the
built-in device tree should be used, or "boring" and say that the distro
provided device tree should be used.  I assume that in the OE case we're
not always going to be booting U-Boot that was built, so updating the
logic there to have the correct dtb isn't right either.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2022-05-18 15:52   ` Tom Rini
@ 2022-05-18 18:15     ` Neil Armstrong
  2022-05-19 12:58       ` Peter Hoyes
  0 siblings, 1 reply; 13+ messages in thread
From: Neil Armstrong @ 2022-05-18 18:15 UTC (permalink / raw)
  To: Tom Rini
  Cc: Peter Hoyes, u-boot, bmeng.cn, sjg, kory.maincent,
	andre.przywara, diego.sueiro

On 18/05/2022 17:52, Tom Rini wrote:
> On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote:
>> Hi,
>>
>> On 14/10/2021 10:40, Peter Hoyes wrote:
>>> From: Peter Hoyes <Peter.Hoyes@arm.com>
>>>
>>> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
>>> U-Boot. When using PXE, if no fdt is defined in the menu file, and
>>> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
>>>
>>> We are developing board support for the Armv8r64 FVP using
>>> config_distro_bootcmd. We are also using OF_BOARD and would like the
>>> PXE boot option to default to the fdt provided by board_fdt_blob_setup.
>>>
>>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
>>> ---
>>>    cmd/pxe_utils.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
>>> index 067c24e5ff..8f8e69ca97 100644
>>> --- a/cmd/pxe_utils.c
>>> +++ b/cmd/pxe_utils.c
>>> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>>>    	 * Scenario 2: If there is an fdt_addr specified, pass it along to
>>>    	 * bootm, and adjust argc appropriately.
>>>    	 *
>>> -	 * Scenario 3: fdt blob is not available.
>>> +	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
>>> +	 * bootm, and adjust argc appropriately.
>>> +	 *
>>> +	 * Scenario 4: fdt blob is not available.
>>>    	 */
>>>    	bootm_argv[3] = env_get("fdt_addr_r");
>>> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>>>    	if (!bootm_argv[3])
>>>    		bootm_argv[3] = env_get("fdt_addr");
>>> +	if (!bootm_argv[3])
>>> +		bootm_argv[3] = env_get("fdtcontroladdr");
>>> +
>>>    	if (bootm_argv[3]) {
>>>    		if (!bootm_argv[2])
>>>    			bootm_argv[2] = "-";
>>
>> This change makes a regression when using a FIT image as kernel image within an extlinux.conf
>>
>> Before this change, when a DT wasn't specified, the DT in the FIT image was selected,
>> not the u-boot DT is selected.
>>
>> While it should work because the U-Boot DT should work in Linux, it's not always the case.
>>
>> Before:
>> Found /extlinux/extlinux.conf
>> Retrieving file: /extlinux/extlinux.conf
>> 1:	Yocto
>> Retrieving file: /fitImage
>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
>> ## Loading kernel from FIT Image at 08080000 ...
>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>     Trying 'kernel-1' kernel subimage
>>       Description:  Linux kernel
>>       Type:         Kernel Image
>>       Compression:  gzip compressed
>>       Data Start:   0x08080120
>>       Data Size:    9926956 Bytes = 9.5 MiB
>>       Architecture: AArch64
>>       OS:           Linux
>>       Load Address: 0x01080000
>>       Entry Point:  0x01080000
>>       Hash algo:    sha256
>>       Hash value:   5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>>     Verifying Hash Integrity ... sha256+ OK
>> ## Loading fdt from FIT Image at 08080000 ...
>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>     Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage
>>       Description:  Flattened Device Tree blob
>>       Type:         Flat Device Tree
>>       Compression:  uncompressed
>>       Data Start:   0x089f7b78
>>       Data Size:    29092 Bytes = 28.4 KiB
>>       Architecture: AArch64
>>       Hash algo:    sha256
>>       Hash value:   72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67
>>     Verifying Hash Integrity ... sha256+ OK
>>     Booting using the fdt blob at 0x89f7b78
>>     Uncompressing Kernel Image
>>     Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 ... OK
>>
>>
>> After:
>> Found /extlinux/extlinux.conf
>> Retrieving file: /extlinux/extlinux.conf
>> 1:	Yocto
>> Retrieving file: /fitImage
>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
>> ## Loading kernel from FIT Image at 08080000 ...
>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>     Trying 'kernel-1' kernel subimage
>>       Description:  Linux kernel
>>       Type:         Kernel Image
>>       Compression:  gzip compressed
>>       Data Start:   0x08080120
>>       Data Size:    9926956 Bytes = 9.5 MiB
>>       Architecture: AArch64
>>       OS:           Linux
>>       Load Address: 0x01080000
>>       Entry Point:  0x01080000
>>       Hash algo:    sha256
>>       Hash value:   5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>>     Verifying Hash Integrity ... sha256+ OK
>> ## Flattened Device Tree blob at 7bf40210
>>     Booting using the fdt blob at 0x7bf40210
>>     Uncompressing Kernel Image
>>     Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 ... OK
>>
>>
>> Note this is the "best in town" boot method in Yocto/OE when using WIC, FIT Image and
>> generating extlinux.conf from the WIC "bootimg-partition" plugin.
>>
>> So this change will regress all platforms using this scheme.
>>
>> In case we only want the FIT image and not DTB, the fdtdir is ommited at:
>> https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150
> 
> Well this is certainly tricky.  Do we be "boring" and say that the
> built-in device tree should be used, or "boring" and say that the distro
> provided device tree should be used.  I assume that in the OE case we're
> not always going to be booting U-Boot that was built, so updating the
> logic there to have the correct dtb isn't right either.
> 

I'm not against this logic, it should definitely be aligned on EFI and permit
using built-in DT, but the logic breaks fitImage loading via extlinux, because
now we have no way to say "use the fitImage DT".

This change should have been elsewhere, like in the non-FIT bootm path when
no DT was supplied and DT is mandatory for the platform.

Or by passing a keyword like fdt-built-in or ftdfile=builtin.

==><=======================================================
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 0c24becae3..22040c2340 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -682,7 +682,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
                         fdtfile = fdtfilefree;
                 }

-               if (fdtfile) {
+               if (strcmp(fdtfile, "builtin") && fdtfile) {
                         int err = get_relfile_envaddr(ctx, fdtfile,
                                                       "fdt_addr_r", NULL);

@@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
         if (!bootm_argv[3])
                 bootm_argv[3] = env_get("fdt_addr");

-       if (!bootm_argv[3])
+       if (!strcmp(fdtfile, "builtin"))
                 bootm_argv[3] = env_get("fdtcontroladdr");

         if (bootm_argv[3]) {
==><=======================================================

Or by checking the image format before using fdtcontroladdr like:

==><=======================================================
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 0c24becae3..f4f6369de6 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
         if (!bootm_argv[3])
                 bootm_argv[3] = env_get("fdt_addr");

-       if (!bootm_argv[3])
+       if (genimg_get_format(buf) != IMAGE_FORMAT_FIT && !bootm_argv[3])
                 bootm_argv[3] = env_get("fdtcontroladdr");

         if (bootm_argv[3]) {
==><=======================================================

Neil

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2022-05-18 18:15     ` Neil Armstrong
@ 2022-05-19 12:58       ` Peter Hoyes
  2022-05-23  7:25         ` Neil Armstrong
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Hoyes @ 2022-05-19 12:58 UTC (permalink / raw)
  To: Neil Armstrong, Tom Rini
  Cc: u-boot, bmeng.cn, sjg, kory.maincent, Andre Przywara, Diego Sueiro

On 18/05/2022 19:15, Neil Armstrong wrote:
> On 18/05/2022 17:52, Tom Rini wrote:
>> On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On 14/10/2021 10:40, Peter Hoyes wrote:
>>>> From: Peter Hoyes <Peter.Hoyes@arm.com>
>>>>
>>>> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to 
>>>> configure
>>>> U-Boot. When using PXE, if no fdt is defined in the menu file, and
>>>> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
>>>>
>>>> We are developing board support for the Armv8r64 FVP using
>>>> config_distro_bootcmd. We are also using OF_BOARD and would like the
>>>> PXE boot option to default to the fdt provided by 
>>>> board_fdt_blob_setup.
>>>>
>>>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
>>>> ---
>>>>    cmd/pxe_utils.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
>>>> index 067c24e5ff..8f8e69ca97 100644
>>>> --- a/cmd/pxe_utils.c
>>>> +++ b/cmd/pxe_utils.c
>>>> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, 
>>>> struct pxe_label *label)
>>>>         * Scenario 2: If there is an fdt_addr specified, pass it 
>>>> along to
>>>>         * bootm, and adjust argc appropriately.
>>>>         *
>>>> -     * Scenario 3: fdt blob is not available.
>>>> +     * Scenario 3: If there is an fdtcontroladdr specified, pass 
>>>> it along to
>>>> +     * bootm, and adjust argc appropriately.
>>>> +     *
>>>> +     * Scenario 4: fdt blob is not available.
>>>>         */
>>>>        bootm_argv[3] = env_get("fdt_addr_r");
>>>> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, 
>>>> struct pxe_label *label)
>>>>        if (!bootm_argv[3])
>>>>            bootm_argv[3] = env_get("fdt_addr");
>>>> +    if (!bootm_argv[3])
>>>> +        bootm_argv[3] = env_get("fdtcontroladdr");
>>>> +
>>>>        if (bootm_argv[3]) {
>>>>            if (!bootm_argv[2])
>>>>                bootm_argv[2] = "-";
>>>
>>> This change makes a regression when using a FIT image as kernel 
>>> image within an extlinux.conf
>>>
>>> Before this change, when a DT wasn't specified, the DT in the FIT 
>>> image was selected,
>>> not the u-boot DT is selected.
>>>
>>> While it should work because the U-Boot DT should work in Linux, 
>>> it's not always the case.
>>>
>>> Before:
>>> Found /extlinux/extlinux.conf
>>> Retrieving file: /extlinux/extlinux.conf
>>> 1:    Yocto
>>> Retrieving file: /fitImage
>>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
>>> ## Loading kernel from FIT Image at 08080000 ...
>>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>>     Trying 'kernel-1' kernel subimage
>>>       Description:  Linux kernel
>>>       Type:         Kernel Image
>>>       Compression:  gzip compressed
>>>       Data Start:   0x08080120
>>>       Data Size:    9926956 Bytes = 9.5 MiB
>>>       Architecture: AArch64
>>>       OS:           Linux
>>>       Load Address: 0x01080000
>>>       Entry Point:  0x01080000
>>>       Hash algo:    sha256
>>>       Hash value: 
>>> 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>>>     Verifying Hash Integrity ... sha256+ OK
>>> ## Loading fdt from FIT Image at 08080000 ...
>>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>>     Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage
>>>       Description:  Flattened Device Tree blob
>>>       Type:         Flat Device Tree
>>>       Compression:  uncompressed
>>>       Data Start:   0x089f7b78
>>>       Data Size:    29092 Bytes = 28.4 KiB
>>>       Architecture: AArch64
>>>       Hash algo:    sha256
>>>       Hash value: 
>>> 72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67
>>>     Verifying Hash Integrity ... sha256+ OK
>>>     Booting using the fdt blob at 0x89f7b78
>>>     Uncompressing Kernel Image
>>>     Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 
>>> ... OK
>>>
>>>
>>> After:
>>> Found /extlinux/extlinux.conf
>>> Retrieving file: /extlinux/extlinux.conf
>>> 1:    Yocto
>>> Retrieving file: /fitImage
>>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
>>> ## Loading kernel from FIT Image at 08080000 ...
>>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>>     Trying 'kernel-1' kernel subimage
>>>       Description:  Linux kernel
>>>       Type:         Kernel Image
>>>       Compression:  gzip compressed
>>>       Data Start:   0x08080120
>>>       Data Size:    9926956 Bytes = 9.5 MiB
>>>       Architecture: AArch64
>>>       OS:           Linux
>>>       Load Address: 0x01080000
>>>       Entry Point:  0x01080000
>>>       Hash algo:    sha256
>>>       Hash value: 
>>> 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>>>     Verifying Hash Integrity ... sha256+ OK
>>> ## Flattened Device Tree blob at 7bf40210
>>>     Booting using the fdt blob at 0x7bf40210
>>>     Uncompressing Kernel Image
>>>     Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 
>>> ... OK
>>>
>>>
>>> Note this is the "best in town" boot method in Yocto/OE when using 
>>> WIC, FIT Image and
>>> generating extlinux.conf from the WIC "bootimg-partition" plugin.
>>>
>>> So this change will regress all platforms using this scheme.
>>>
>>> In case we only want the FIT image and not DTB, the fdtdir is 
>>> ommited at:
>>> https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150 
>>>
>>
>> Well this is certainly tricky.  Do we be "boring" and say that the
>> built-in device tree should be used, or "boring" and say that the distro
>> provided device tree should be used.  I assume that in the OE case we're
>> not always going to be booting U-Boot that was built, so updating the
>> logic there to have the correct dtb isn't right either.
>>
>
> I'm not against this logic, it should definitely be aligned on EFI and 
> permit
> using built-in DT, but the logic breaks fitImage loading via extlinux, 
> because
> now we have no way to say "use the fitImage DT".
>
> This change should have been elsewhere, like in the non-FIT bootm path 
> when
> no DT was supplied and DT is mandatory for the platform.
>
> Or by passing a keyword like fdt-built-in or ftdfile=builtin.

I agree the correct logic should be:

  * Use the extlinux.conf fdt file, if defined
  * Use a FIT fdt, if available
  * Use fdtcontroladdr, as a last resort

, so FIT images was an oversight.

>
> ==><=======================================================
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index 0c24becae3..22040c2340 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -682,7 +682,7 @@ static int label_boot(struct pxe_context *ctx, 
> struct pxe_label *label)
>                         fdtfile = fdtfilefree;
>                 }
>
> -               if (fdtfile) {
> +               if (strcmp(fdtfile, "builtin") && fdtfile) {
>                         int err = get_relfile_envaddr(ctx, fdtfile,
> "fdt_addr_r", NULL);
>
> @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, 
> struct pxe_label *label)
>         if (!bootm_argv[3])
>                 bootm_argv[3] = env_get("fdt_addr");
>
> -       if (!bootm_argv[3])
> +       if (!strcmp(fdtfile, "builtin"))
>                 bootm_argv[3] = env_get("fdtcontroladdr");
>
>         if (bootm_argv[3]) {
> ==><=======================================================
>
> Or by checking the image format before using fdtcontroladdr like:
>
> ==><=======================================================
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index 0c24becae3..f4f6369de6 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, 
> struct pxe_label *label)
>         if (!bootm_argv[3])
>                 bootm_argv[3] = env_get("fdt_addr");
>
> -       if (!bootm_argv[3])
> +       if (genimg_get_format(buf) != IMAGE_FORMAT_FIT && !bootm_argv[3])
>                 bootm_argv[3] = env_get("fdtcontroladdr");
>
>         if (bootm_argv[3]) {
> ==><=======================================================
>
> Neil

Your second diff above seems like the least invasive way of achieving 
this behavior.

I did wonder whether it would make sense to add a global 
'fdtcontroladdr' fallback for bootm (i.e. in boot_get_fdt in 
image-fdt.c), so that:

    bootm $kernel_addr_r

falls back to $fdtcontroladdr (if valid) as a last resort. Then we could 
remove the branch in pxe_utils, and the FIT logic would happen in the 
right order. It looks like bootefi already does something similar, but I 
guess modifying the bootm behavior would affect a lot of existing boards.

Peter


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2022-05-19 12:58       ` Peter Hoyes
@ 2022-05-23  7:25         ` Neil Armstrong
  2022-05-23 13:23           ` Tom Rini
  0 siblings, 1 reply; 13+ messages in thread
From: Neil Armstrong @ 2022-05-23  7:25 UTC (permalink / raw)
  To: Peter Hoyes, Tom Rini
  Cc: u-boot, bmeng.cn, sjg, kory.maincent, Andre Przywara, Diego Sueiro

Hi,

On 19/05/2022 14:58, Peter Hoyes wrote:
> On 18/05/2022 19:15, Neil Armstrong wrote:
>> On 18/05/2022 17:52, Tom Rini wrote:
>>> On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote:
>>>> Hi,
>>>>
>>>> On 14/10/2021 10:40, Peter Hoyes wrote:
>>>>> From: Peter Hoyes <Peter.Hoyes@arm.com>
>>>>>
>>>>> If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
>>>>> U-Boot. When using PXE, if no fdt is defined in the menu file, and
>>>>> there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
>>>>>
>>>>> We are developing board support for the Armv8r64 FVP using
>>>>> config_distro_bootcmd. We are also using OF_BOARD and would like the
>>>>> PXE boot option to default to the fdt provided by board_fdt_blob_setup.
>>>>>
>>>>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
>>>>> ---
>>>>>    cmd/pxe_utils.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
>>>>> index 067c24e5ff..8f8e69ca97 100644
>>>>> --- a/cmd/pxe_utils.c
>>>>> +++ b/cmd/pxe_utils.c
>>>>> @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>>>>>         * Scenario 2: If there is an fdt_addr specified, pass it along to
>>>>>         * bootm, and adjust argc appropriately.
>>>>>         *
>>>>> -     * Scenario 3: fdt blob is not available.
>>>>> +     * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
>>>>> +     * bootm, and adjust argc appropriately.
>>>>> +     *
>>>>> +     * Scenario 4: fdt blob is not available.
>>>>>         */
>>>>>        bootm_argv[3] = env_get("fdt_addr_r");
>>>>> @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
>>>>>        if (!bootm_argv[3])
>>>>>            bootm_argv[3] = env_get("fdt_addr");
>>>>> +    if (!bootm_argv[3])
>>>>> +        bootm_argv[3] = env_get("fdtcontroladdr");
>>>>> +
>>>>>        if (bootm_argv[3]) {
>>>>>            if (!bootm_argv[2])
>>>>>                bootm_argv[2] = "-";
>>>>
>>>> This change makes a regression when using a FIT image as kernel image within an extlinux.conf
>>>>
>>>> Before this change, when a DT wasn't specified, the DT in the FIT image was selected,
>>>> not the u-boot DT is selected.
>>>>
>>>> While it should work because the U-Boot DT should work in Linux, it's not always the case.
>>>>
>>>> Before:
>>>> Found /extlinux/extlinux.conf
>>>> Retrieving file: /extlinux/extlinux.conf
>>>> 1:    Yocto
>>>> Retrieving file: /fitImage
>>>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
>>>> ## Loading kernel from FIT Image at 08080000 ...
>>>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>>>     Trying 'kernel-1' kernel subimage
>>>>       Description:  Linux kernel
>>>>       Type:         Kernel Image
>>>>       Compression:  gzip compressed
>>>>       Data Start:   0x08080120
>>>>       Data Size:    9926956 Bytes = 9.5 MiB
>>>>       Architecture: AArch64
>>>>       OS:           Linux
>>>>       Load Address: 0x01080000
>>>>       Entry Point:  0x01080000
>>>>       Hash algo:    sha256
>>>>       Hash value: 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>>>>     Verifying Hash Integrity ... sha256+ OK
>>>> ## Loading fdt from FIT Image at 08080000 ...
>>>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>>>     Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage
>>>>       Description:  Flattened Device Tree blob
>>>>       Type:         Flat Device Tree
>>>>       Compression:  uncompressed
>>>>       Data Start:   0x089f7b78
>>>>       Data Size:    29092 Bytes = 28.4 KiB
>>>>       Architecture: AArch64
>>>>       Hash algo:    sha256
>>>>       Hash value: 72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67
>>>>     Verifying Hash Integrity ... sha256+ OK
>>>>     Booting using the fdt blob at 0x89f7b78
>>>>     Uncompressing Kernel Image
>>>>     Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 ... OK
>>>>
>>>>
>>>> After:
>>>> Found /extlinux/extlinux.conf
>>>> Retrieving file: /extlinux/extlinux.conf
>>>> 1:    Yocto
>>>> Retrieving file: /fitImage
>>>> append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
>>>> ## Loading kernel from FIT Image at 08080000 ...
>>>>     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
>>>>     Trying 'kernel-1' kernel subimage
>>>>       Description:  Linux kernel
>>>>       Type:         Kernel Image
>>>>       Compression:  gzip compressed
>>>>       Data Start:   0x08080120
>>>>       Data Size:    9926956 Bytes = 9.5 MiB
>>>>       Architecture: AArch64
>>>>       OS:           Linux
>>>>       Load Address: 0x01080000
>>>>       Entry Point:  0x01080000
>>>>       Hash algo:    sha256
>>>>       Hash value: 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
>>>>     Verifying Hash Integrity ... sha256+ OK
>>>> ## Flattened Device Tree blob at 7bf40210
>>>>     Booting using the fdt blob at 0x7bf40210
>>>>     Uncompressing Kernel Image
>>>>     Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 ... OK
>>>>
>>>>
>>>> Note this is the "best in town" boot method in Yocto/OE when using WIC, FIT Image and
>>>> generating extlinux.conf from the WIC "bootimg-partition" plugin.
>>>>
>>>> So this change will regress all platforms using this scheme.
>>>>
>>>> In case we only want the FIT image and not DTB, the fdtdir is ommited at:
>>>> https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150
>>>
>>> Well this is certainly tricky.  Do we be "boring" and say that the
>>> built-in device tree should be used, or "boring" and say that the distro
>>> provided device tree should be used.  I assume that in the OE case we're
>>> not always going to be booting U-Boot that was built, so updating the
>>> logic there to have the correct dtb isn't right either.
>>>
>>
>> I'm not against this logic, it should definitely be aligned on EFI and permit
>> using built-in DT, but the logic breaks fitImage loading via extlinux, because
>> now we have no way to say "use the fitImage DT".
>>
>> This change should have been elsewhere, like in the non-FIT bootm path when
>> no DT was supplied and DT is mandatory for the platform.
>>
>> Or by passing a keyword like fdt-built-in or ftdfile=builtin.
> 
> I agree the correct logic should be:
> 
>   * Use the extlinux.conf fdt file, if defined
>   * Use a FIT fdt, if available
>   * Use fdtcontroladdr, as a last resort
> 
> , so FIT images was an oversight.
> 
>>
>> ==><=======================================================
>> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
>> index 0c24becae3..22040c2340 100644
>> --- a/boot/pxe_utils.c
>> +++ b/boot/pxe_utils.c
>> @@ -682,7 +682,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>>                         fdtfile = fdtfilefree;
>>                 }
>>
>> -               if (fdtfile) {
>> +               if (strcmp(fdtfile, "builtin") && fdtfile) {
>>                         int err = get_relfile_envaddr(ctx, fdtfile,
>> "fdt_addr_r", NULL);
>>
>> @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>>         if (!bootm_argv[3])
>>                 bootm_argv[3] = env_get("fdt_addr");
>>
>> -       if (!bootm_argv[3])
>> +       if (!strcmp(fdtfile, "builtin"))
>>                 bootm_argv[3] = env_get("fdtcontroladdr");
>>
>>         if (bootm_argv[3]) {
>> ==><=======================================================
>>
>> Or by checking the image format before using fdtcontroladdr like:
>>
>> ==><=======================================================
>> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
>> index 0c24becae3..f4f6369de6 100644
>> --- a/boot/pxe_utils.c
>> +++ b/boot/pxe_utils.c
>> @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>>         if (!bootm_argv[3])
>>                 bootm_argv[3] = env_get("fdt_addr");
>>
>> -       if (!bootm_argv[3])
>> +       if (genimg_get_format(buf) != IMAGE_FORMAT_FIT && !bootm_argv[3])
>>                 bootm_argv[3] = env_get("fdtcontroladdr");
>>
>>         if (bootm_argv[3]) {
>> ==><=======================================================
>>
>> Neil
> 
> Your second diff above seems like the least invasive way of achieving this behavior.
> 
> I did wonder whether it would make sense to add a global 'fdtcontroladdr' fallback for bootm (i.e. in boot_get_fdt in image-fdt.c), so that:
> 
>     bootm $kernel_addr_r
> 
> falls back to $fdtcontroladdr (if valid) as a last resort. Then we could remove the branch in pxe_utils, and the FIT logic would happen in the right order. It looks like bootefi already does something similar, but I guess modifying the bootm behavior would affect a lot of existing boards.

Probably, but I think getting back fit image working would be the priority.

Tom, should I send the last change ?

Neil

> 
> Peter
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot
  2022-05-23  7:25         ` Neil Armstrong
@ 2022-05-23 13:23           ` Tom Rini
  0 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2022-05-23 13:23 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Peter Hoyes, u-boot, bmeng.cn, sjg, kory.maincent,
	Andre Przywara, Diego Sueiro

[-- Attachment #1: Type: text/plain, Size: 10324 bytes --]

On Mon, May 23, 2022 at 09:25:55AM +0200, Neil Armstrong wrote:
> Hi,
> 
> On 19/05/2022 14:58, Peter Hoyes wrote:
> > On 18/05/2022 19:15, Neil Armstrong wrote:
> > > On 18/05/2022 17:52, Tom Rini wrote:
> > > > On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote:
> > > > > Hi,
> > > > > 
> > > > > On 14/10/2021 10:40, Peter Hoyes wrote:
> > > > > > From: Peter Hoyes <Peter.Hoyes@arm.com>
> > > > > > 
> > > > > > If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure
> > > > > > U-Boot. When using PXE, if no fdt is defined in the menu file, and
> > > > > > there is no fdt at fdt_addr, add fall back on fdtcontroladdr too.
> > > > > > 
> > > > > > We are developing board support for the Armv8r64 FVP using
> > > > > > config_distro_bootcmd. We are also using OF_BOARD and would like the
> > > > > > PXE boot option to default to the fdt provided by board_fdt_blob_setup.
> > > > > > 
> > > > > > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> > > > > > ---
> > > > > >    cmd/pxe_utils.c | 8 +++++++-
> > > > > >    1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> > > > > > index 067c24e5ff..8f8e69ca97 100644
> > > > > > --- a/cmd/pxe_utils.c
> > > > > > +++ b/cmd/pxe_utils.c
> > > > > > @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> > > > > >         * Scenario 2: If there is an fdt_addr specified, pass it along to
> > > > > >         * bootm, and adjust argc appropriately.
> > > > > >         *
> > > > > > -     * Scenario 3: fdt blob is not available.
> > > > > > +     * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> > > > > > +     * bootm, and adjust argc appropriately.
> > > > > > +     *
> > > > > > +     * Scenario 4: fdt blob is not available.
> > > > > >         */
> > > > > >        bootm_argv[3] = env_get("fdt_addr_r");
> > > > > > @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> > > > > >        if (!bootm_argv[3])
> > > > > >            bootm_argv[3] = env_get("fdt_addr");
> > > > > > +    if (!bootm_argv[3])
> > > > > > +        bootm_argv[3] = env_get("fdtcontroladdr");
> > > > > > +
> > > > > >        if (bootm_argv[3]) {
> > > > > >            if (!bootm_argv[2])
> > > > > >                bootm_argv[2] = "-";
> > > > > 
> > > > > This change makes a regression when using a FIT image as kernel image within an extlinux.conf
> > > > > 
> > > > > Before this change, when a DT wasn't specified, the DT in the FIT image was selected,
> > > > > not the u-boot DT is selected.
> > > > > 
> > > > > While it should work because the U-Boot DT should work in Linux, it's not always the case.
> > > > > 
> > > > > Before:
> > > > > Found /extlinux/extlinux.conf
> > > > > Retrieving file: /extlinux/extlinux.conf
> > > > > 1:    Yocto
> > > > > Retrieving file: /fitImage
> > > > > append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
> > > > > ## Loading kernel from FIT Image at 08080000 ...
> > > > >     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
> > > > >     Trying 'kernel-1' kernel subimage
> > > > >       Description:  Linux kernel
> > > > >       Type:         Kernel Image
> > > > >       Compression:  gzip compressed
> > > > >       Data Start:   0x08080120
> > > > >       Data Size:    9926956 Bytes = 9.5 MiB
> > > > >       Architecture: AArch64
> > > > >       OS:           Linux
> > > > >       Load Address: 0x01080000
> > > > >       Entry Point:  0x01080000
> > > > >       Hash algo:    sha256
> > > > >       Hash value: 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
> > > > >     Verifying Hash Integrity ... sha256+ OK
> > > > > ## Loading fdt from FIT Image at 08080000 ...
> > > > >     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
> > > > >     Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage
> > > > >       Description:  Flattened Device Tree blob
> > > > >       Type:         Flat Device Tree
> > > > >       Compression:  uncompressed
> > > > >       Data Start:   0x089f7b78
> > > > >       Data Size:    29092 Bytes = 28.4 KiB
> > > > >       Architecture: AArch64
> > > > >       Hash algo:    sha256
> > > > >       Hash value: 72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67
> > > > >     Verifying Hash Integrity ... sha256+ OK
> > > > >     Booting using the fdt blob at 0x89f7b78
> > > > >     Uncompressing Kernel Image
> > > > >     Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 ... OK
> > > > > 
> > > > > 
> > > > > After:
> > > > > Found /extlinux/extlinux.conf
> > > > > Retrieving file: /extlinux/extlinux.conf
> > > > > 1:    Yocto
> > > > > Retrieving file: /fitImage
> > > > > append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200
> > > > > ## Loading kernel from FIT Image at 08080000 ...
> > > > >     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration
> > > > >     Trying 'kernel-1' kernel subimage
> > > > >       Description:  Linux kernel
> > > > >       Type:         Kernel Image
> > > > >       Compression:  gzip compressed
> > > > >       Data Start:   0x08080120
> > > > >       Data Size:    9926956 Bytes = 9.5 MiB
> > > > >       Architecture: AArch64
> > > > >       OS:           Linux
> > > > >       Load Address: 0x01080000
> > > > >       Entry Point:  0x01080000
> > > > >       Hash algo:    sha256
> > > > >       Hash value: 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e
> > > > >     Verifying Hash Integrity ... sha256+ OK
> > > > > ## Flattened Device Tree blob at 7bf40210
> > > > >     Booting using the fdt blob at 0x7bf40210
> > > > >     Uncompressing Kernel Image
> > > > >     Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 ... OK
> > > > > 
> > > > > 
> > > > > Note this is the "best in town" boot method in Yocto/OE when using WIC, FIT Image and
> > > > > generating extlinux.conf from the WIC "bootimg-partition" plugin.
> > > > > 
> > > > > So this change will regress all platforms using this scheme.
> > > > > 
> > > > > In case we only want the FIT image and not DTB, the fdtdir is ommited at:
> > > > > https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150
> > > > 
> > > > Well this is certainly tricky.  Do we be "boring" and say that the
> > > > built-in device tree should be used, or "boring" and say that the distro
> > > > provided device tree should be used.  I assume that in the OE case we're
> > > > not always going to be booting U-Boot that was built, so updating the
> > > > logic there to have the correct dtb isn't right either.
> > > > 
> > > 
> > > I'm not against this logic, it should definitely be aligned on EFI and permit
> > > using built-in DT, but the logic breaks fitImage loading via extlinux, because
> > > now we have no way to say "use the fitImage DT".
> > > 
> > > This change should have been elsewhere, like in the non-FIT bootm path when
> > > no DT was supplied and DT is mandatory for the platform.
> > > 
> > > Or by passing a keyword like fdt-built-in or ftdfile=builtin.
> > 
> > I agree the correct logic should be:
> > 
> >   * Use the extlinux.conf fdt file, if defined
> >   * Use a FIT fdt, if available
> >   * Use fdtcontroladdr, as a last resort
> > 
> > , so FIT images was an oversight.
> > 
> > > 
> > > ==><=======================================================
> > > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> > > index 0c24becae3..22040c2340 100644
> > > --- a/boot/pxe_utils.c
> > > +++ b/boot/pxe_utils.c
> > > @@ -682,7 +682,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
> > >                         fdtfile = fdtfilefree;
> > >                 }
> > > 
> > > -               if (fdtfile) {
> > > +               if (strcmp(fdtfile, "builtin") && fdtfile) {
> > >                         int err = get_relfile_envaddr(ctx, fdtfile,
> > > "fdt_addr_r", NULL);
> > > 
> > > @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
> > >         if (!bootm_argv[3])
> > >                 bootm_argv[3] = env_get("fdt_addr");
> > > 
> > > -       if (!bootm_argv[3])
> > > +       if (!strcmp(fdtfile, "builtin"))
> > >                 bootm_argv[3] = env_get("fdtcontroladdr");
> > > 
> > >         if (bootm_argv[3]) {
> > > ==><=======================================================
> > > 
> > > Or by checking the image format before using fdtcontroladdr like:
> > > 
> > > ==><=======================================================
> > > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> > > index 0c24becae3..f4f6369de6 100644
> > > --- a/boot/pxe_utils.c
> > > +++ b/boot/pxe_utils.c
> > > @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
> > >         if (!bootm_argv[3])
> > >                 bootm_argv[3] = env_get("fdt_addr");
> > > 
> > > -       if (!bootm_argv[3])
> > > +       if (genimg_get_format(buf) != IMAGE_FORMAT_FIT && !bootm_argv[3])
> > >                 bootm_argv[3] = env_get("fdtcontroladdr");
> > > 
> > >         if (bootm_argv[3]) {
> > > ==><=======================================================
> > > 
> > > Neil
> > 
> > Your second diff above seems like the least invasive way of achieving this behavior.
> > 
> > I did wonder whether it would make sense to add a global 'fdtcontroladdr' fallback for bootm (i.e. in boot_get_fdt in image-fdt.c), so that:
> > 
> >     bootm $kernel_addr_r
> > 
> > falls back to $fdtcontroladdr (if valid) as a last resort. Then we could remove the branch in pxe_utils, and the FIT logic would happen in the right order. It looks like bootefi already does something similar, but I guess modifying the bootm behavior would affect a lot of existing boards.
> 
> Probably, but I think getting back fit image working would be the priority.
> 
> Tom, should I send the last change ?

Yes please, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-05-23 13:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14  8:40 [PATCH] cmd: pxe_utils: Check fdtcontroladdr in label_boot Peter Hoyes
2021-10-14 18:24 ` Simon Glass
2021-10-15 11:20   ` Andre Przywara
2021-10-24 19:53     ` Simon Glass
2021-10-28 18:55       ` Ramon Fried
2021-11-08 16:52         ` Peter Hoyes
2021-11-09  6:26           ` Ramon Fried
2022-05-18  8:40 ` Neil Armstrong
2022-05-18 15:52   ` Tom Rini
2022-05-18 18:15     ` Neil Armstrong
2022-05-19 12:58       ` Peter Hoyes
2022-05-23  7:25         ` Neil Armstrong
2022-05-23 13:23           ` Tom Rini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.