[Buildroot] [PATCH 0/2] boot/uboot: clean-up for ZynqMP pmufw handling

[Buildroot] [PATCH 0/2] boot/uboot: clean-up for ZynqMP pmufw handling

[Brandon Maier via buildroot]

Signed-off-by: Brandon Maier <brandon.maier@collins.com>
---
Brandon Maier (2):
      boot/uboot: avoid setting PMUFW_INIT_FILE when not used
      boot/uboot: use $(TARGET_OBJCOPY) for ZynqMP pmufw.elf

 boot/uboot/uboot.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
---
base-commit: 48e7affffa06bbe23a1cd190d0a765956cf179b0
change-id: 20240815-boot-uboot-clean-for-pmufw-c0c401a9fec5

Best regards,
-- 
Brandon Maier <brandon.maier@collins.com>

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 1/2] boot/uboot: avoid setting PMUFW_INIT_FILE when not used

[Brandon Maier via buildroot]

The CONFIG_PMUFW_INIT_FILE is always set, even when
BR2_TARGET_UBOOT_ZYNQMP_PMUFW is an empty string, which prevents a user
from setting this directly in their U-Boot defconfig or frag.

Signed-off-by: Brandon Maier <brandon.maier@collins.com>
---
 boot/uboot/uboot.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index cdb9f435f7..9de0776ace 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -469,9 +469,12 @@ UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
 else
 UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH)
 endif
+
+ifneq ($(UBOOT_ZYNQMP_PMUFW_PATH_FINAL),)
 define UBOOT_ZYNQMP_KCONFIG_PMUFW
 	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")
 endef
+endif
 
 UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG))
 ifneq ($(UBOOT_ZYNQMP_PM_CFG),)

-- 
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 2/2] boot/uboot: use $(TARGET_OBJCOPY) for ZynqMP pmufw.elf

[Brandon Maier via buildroot]

Converting the pmufw.elf to a binary works with any objcopy, regardless
if it's from the host or cross-compiler. Prefer to use the
$(TARGET_OBJCOPY) as it's always available and reproducible.

Signed-off-by: Brandon Maier <brandon.maier@collins.com>
---
 boot/uboot/uboot.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 9de0776ace..08493996e5 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -463,7 +463,7 @@ endif #BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
 ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
 UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
 define UBOOT_ZYNQMP_PMUFW_CONVERT
-	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
+	$(TARGET_OBJCOPY) -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
 endef
 UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
 else

-- 
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 2/2] boot/uboot: use $(TARGET_OBJCOPY) for ZynqMP pmufw.elf

[Frager, Neal via buildroot]

Hi Brandon,

> Converting the pmufw.elf to a binary works with any objcopy, regardless
> if it's from the host or cross-compiler. Prefer to use the
> $(TARGET_OBJCOPY) as it's always available and reproducible.

> Signed-off-by: Brandon Maier <brandon.maier@collins.com>

I confirm that objcopy is arch agnostic, so it does not matter whether it is
an x86 objcopy or arm objcopy when converting a microblaze .elf to .bin.

You have convinced me that this is worth changing because of your
reproducibility argument.

Reviewed-by: Neal Frager <neal.frager@amd.com>

Best regards,
Neal Frager
AMD
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/2] boot/uboot: avoid setting PMUFW_INIT_FILE when not used

[Frager, Neal via buildroot]

> The CONFIG_PMUFW_INIT_FILE is always set, even when
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW is an empty string, which prevents a user
> from setting this directly in their U-Boot defconfig or frag.

> Signed-off-by: Brandon Maier <brandon.maier@collins.com>

Reviewed-by: Neal Frager <neal.frager@amd.com>

Best regards,
Neal Frager
AMD
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/2] boot/uboot: avoid setting PMUFW_INIT_FILE when not used

[Luca Ceresoli via buildroot]

Hello Brandon,

On Thu, 15 Aug 2024 14:22:30 +0000
Brandon Maier <brandon.maier@collins.com> wrote:

> The CONFIG_PMUFW_INIT_FILE is always set, even when
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW is an empty string, which prevents a user
> from setting this directly in their U-Boot defconfig or frag.

I think there are two potential reasons for applying this patch:

 1. setting an empty string lets the build finish apparently
    successfully, but the output binaries are not working

 2. allowing users to set CONFIG_PMUFW_INIT_FILE in their U-Boot config
    and have Buildroot let it pass through

For point 1, as I wrote earlier today I think this won't happen, based
on the U-Boot code.

For point 2, which is the motivation you present in this patch, I don't
see a valid use case. Without a use case, I'd rather not add code to
uboot.mk, no matter if it is only a few lines. With a use case, I'd be
totally OK.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 2/2] boot/uboot: use $(TARGET_OBJCOPY) for ZynqMP pmufw.elf

[Luca Ceresoli via buildroot]

Hello Brandon,

On Thu, 15 Aug 2024 14:22:31 +0000
Brandon Maier <brandon.maier@collins.com> wrote:

> Converting the pmufw.elf to a binary works with any objcopy, regardless
> if it's from the host or cross-compiler. Prefer to use the
> $(TARGET_OBJCOPY) as it's always available and reproducible.
> 
> Signed-off-by: Brandon Maier <brandon.maier@collins.com>
> ---
>  boot/uboot/uboot.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 9de0776ace..08493996e5 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -463,7 +463,7 @@ endif #BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
>  ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
>  UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
>  define UBOOT_ZYNQMP_PMUFW_CONVERT
> -	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
> +	$(TARGET_OBJCOPY) -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)

As I wrote earlier today (way after you sent this patch, so surely not
blaming you!) a short comment would help understand why we are using
the "wrong" objcopy.

E.g.:

 # objcopy is arch-agnostic so we can use $(TARGET_OBJCOPY) in lack of
 # a microblaze objcopy

But this can be added later, or in v2 in case you send one, so:

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/2] boot/uboot: avoid setting PMUFW_INIT_FILE when not used

[Thomas Petazzoni via buildroot]

Hello,

On Mon, 19 Aug 2024 10:32:26 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> I think there are two potential reasons for applying this patch:
> 
>  1. setting an empty string lets the build finish apparently
>     successfully, but the output binaries are not working
> 
>  2. allowing users to set CONFIG_PMUFW_INIT_FILE in their U-Boot config
>     and have Buildroot let it pass through
> 
> For point 1, as I wrote earlier today I think this won't happen, based
> on the U-Boot code.
> 
> For point 2, which is the motivation you present in this patch, I don't
> see a valid use case. Without a use case, I'd rather not add code to
> uboot.mk, no matter if it is only a few lines. With a use case, I'd be
> totally OK.

My initial point was that I found it odd that:

	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")

was done unconditionally. But in fact, it's done under the condition:

ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)

so I think the code is indeed fine as it is.

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/2] boot/uboot: avoid setting PMUFW_INIT_FILE when not used

[Brandon Maier via buildroot]

Hi Thomas, Luca,

On Mon Aug 19, 2024 at 8:43 AM UTC, Thomas Petazzoni via buildroot wrote:
> Hello,
>
> On Mon, 19 Aug 2024 10:32:26 +0200
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>
> > I think there are two potential reasons for applying this patch:
> >
> >  1. setting an empty string lets the build finish apparently
> >     successfully, but the output binaries are not working
> >
> >  2. allowing users to set CONFIG_PMUFW_INIT_FILE in their U-Boot config
> >     and have Buildroot let it pass through
> >
> > For point 1, as I wrote earlier today I think this won't happen, based
> > on the U-Boot code.
> >
> > For point 2, which is the motivation you present in this patch, I don't
> > see a valid use case. Without a use case, I'd rather not add code to
> > uboot.mk, no matter if it is only a few lines. With a use case, I'd be
> > totally OK.
>
> My initial point was that I found it odd that:
>
> 	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")
>
> was done unconditionally. But in fact, it's done under the condition:
>
> ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)
>
> so I think the code is indeed fine as it is.

Sounds good to me, I don't have a use case either. I'll drop this patch
when I send the v2.

Thanks,
Brandon Maier

>
> Thomas
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 2/2] boot/uboot: use $(TARGET_OBJCOPY) for ZynqMP pmufw.elf

[Brandon Maier via buildroot]

Hi Luca,

On Mon Aug 19, 2024 at 8:38 AM UTC, Luca Ceresoli via buildroot wrote:
> Hello Brandon,
>
> On Thu, 15 Aug 2024 14:22:31 +0000
> Brandon Maier <brandon.maier@collins.com> wrote:
>
> > Converting the pmufw.elf to a binary works with any objcopy, regardless
> > if it's from the host or cross-compiler. Prefer to use the
> > $(TARGET_OBJCOPY) as it's always available and reproducible.
> >
> > Signed-off-by: Brandon Maier <brandon.maier@collins.com>
> > ---
> >  boot/uboot/uboot.mk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> > index 9de0776ace..08493996e5 100644
> > --- a/boot/uboot/uboot.mk
> > +++ b/boot/uboot/uboot.mk
> > @@ -463,7 +463,7 @@ endif #BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
> >  ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
> >  UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
> >  define UBOOT_ZYNQMP_PMUFW_CONVERT
> > -	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
> > +	$(TARGET_OBJCOPY) -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
>
> As I wrote earlier today (way after you sent this patch, so surely not
> blaming you!) a short comment would help understand why we are using
> the "wrong" objcopy.
>
> E.g.:
>
>  # objcopy is arch-agnostic so we can use $(TARGET_OBJCOPY) in lack of
>  # a microblaze objcopy

Agreed a comment would be good, I sent a v2 with this change.

Thank you!
Brandon Maier

>
> But this can be added later, or in v2 in case you send one, so:
>
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot