[Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add pmufw.elf support

[Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add pmufw.elf support

[Neal Frager via buildroot]

This patch adds support for zynqmp pmufw.elf files.
It will allow buildroot to use pmufw.elf binaries directly
from the Xilinx git repository built by petalinux.

https://github.com/Xilinx/ubuntu-firmware/tree/v2022.1_22.04_1/xlnx-firmware

It will solve the problem of the pmufw built by the
zynqmp-pmufw-builder where soft resets crash.  Details of the issue
can be found here:

https://lore.kernel.org/buildroot/87ilqccu3k.fsf@dell.be.48ers.dk/

Instead of using a pmufw binary built by the zynqmp-pmufw-builder, this patch
enables a pmufw binary built by petalinux to solve the issue.

Signed-off-by: Neal Frager <neal.frager@amd.com>
---
V1->V2:
  - corrected objcopy instruction for converting elf to bin
---
 boot/uboot/uboot.mk | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index a9f9b1bf16..ab7dedb4b6 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -392,8 +392,17 @@ else ifneq ($(UBOOT_ZYNQMP_PMUFW),)
 UBOOT_ZYNQMP_PMUFW_PATH = $(shell readlink -f $(UBOOT_ZYNQMP_PMUFW))
 endif
 
+UBOOT_ZYNQMP_PMUFW_BASENAME=$(basename $(UBOOT_ZYNQMP_PMUFW_PATH))
+UBOOT_ZYNQMP_PMUFW_SUFFIX=$(suffix $(UBOOT_ZYNQMP_PMUFW_PATH))
+
+ifeq ($(UBOOT_ZYNQMP_PMUFW_SUFFIX),.elf)
+UBOOT_ZYNQMP_PMUFW_CONVERT=y
+endif
+
 define UBOOT_ZYNQMP_KCONFIG_PMUFW
-	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")
+	$(if $(UBOOT_ZYNQMP_PMUFW_CONVERT),
+		objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin)
+	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin")
 endef
 
 UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG))
-- 
2.17.1

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

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add pmufw.elf support

[Thomas Petazzoni via buildroot]

Hello,

On Wed, 12 Oct 2022 11:54:26 -0600
Neal Frager via buildroot <buildroot@buildroot.org> wrote:

> This patch adds support for zynqmp pmufw.elf files.
> It will allow buildroot to use pmufw.elf binaries directly
> from the Xilinx git repository built by petalinux.
> 
> https://github.com/Xilinx/ubuntu-firmware/tree/v2022.1_22.04_1/xlnx-firmware
> 
> It will solve the problem of the pmufw built by the
> zynqmp-pmufw-builder where soft resets crash.  Details of the issue
> can be found here:
> 
> https://lore.kernel.org/buildroot/87ilqccu3k.fsf@dell.be.48ers.dk/
> 
> Instead of using a pmufw binary built by the zynqmp-pmufw-builder, this patch
> enables a pmufw binary built by petalinux to solve the issue.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>

I think the whole commit log is a bit confusing. What this commit
allows is to use a PMU FW as an ELF binary in addition to allowing to
use a PMU FW provided as a raw binary.

And indeed, this will allow using the PMU FW provided by Petalinux in
addition to the ones of zynqmp-pmufw-builder, which is the motivation
for the change.

> +UBOOT_ZYNQMP_PMUFW_BASENAME=$(basename $(UBOOT_ZYNQMP_PMUFW_PATH))
> +UBOOT_ZYNQMP_PMUFW_SUFFIX=$(suffix $(UBOOT_ZYNQMP_PMUFW_PATH))

Spaces around = sign.

> +
> +ifeq ($(UBOOT_ZYNQMP_PMUFW_SUFFIX),.elf)
> +UBOOT_ZYNQMP_PMUFW_CONVERT=y

Ditto.

> +endif
> +
>  define UBOOT_ZYNQMP_KCONFIG_PMUFW
> -	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")
> +	$(if $(UBOOT_ZYNQMP_PMUFW_CONVERT),
> +		objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin)

You could also write this like this:

	$(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
		objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin)

and get rid of UBOOT_ZYNQMP_PMUFW_CONVERT entirely.

I was wondering if the usage of objcopy was correct or whether
$(TARGET_OBJCOPY) would be more correct. But in fact none of them is
perfect I guess: the PMU FW is a Microblaze ELF binary, while
$(TARGET_OBJCOPY) will be an ARM64 objcopy, and plain objcopy will be
the machine native objcopy, very unlikely to be a Microblaze one :-)

So overall, I guess using objcopy is fine here.

Best regards,

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 v2 1/1] boot/uboot/uboot.mk: add pmufw.elf support

[Frager, Neal via buildroot]

Hi Thomas,

Thank you for the review.

> This patch adds support for zynqmp pmufw.elf files.
> It will allow buildroot to use pmufw.elf binaries directly from the 
> Xilinx git repository built by petalinux.
> 
> https://github.com/Xilinx/ubuntu-firmware/tree/v2022.1_22.04_1/xlnx-fi
> rmware
> 
> It will solve the problem of the pmufw built by the 
> zynqmp-pmufw-builder where soft resets crash.  Details of the issue 
> can be found here:
> 
> https://lore.kernel.org/buildroot/87ilqccu3k.fsf@dell.be.48ers.dk/
> 
> Instead of using a pmufw binary built by the zynqmp-pmufw-builder, 
> this patch enables a pmufw binary built by petalinux to solve the issue.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>

> I think the whole commit log is a bit confusing. What this commit allows is to use a PMU FW as an ELF binary in addition to allowing to use a PMU FW provided as a raw binary.

> And indeed, this will allow using the PMU FW provided by Petalinux in addition to the ones of zynqmp-pmufw-builder, which is the motivation for the change.

I will simplify the commit log.  No problem.

> +UBOOT_ZYNQMP_PMUFW_BASENAME=$(basename $(UBOOT_ZYNQMP_PMUFW_PATH)) 
> +UBOOT_ZYNQMP_PMUFW_SUFFIX=$(suffix $(UBOOT_ZYNQMP_PMUFW_PATH))

> Spaces around = sign.

I will take out the unnecessary spaces.

> +
> +ifeq ($(UBOOT_ZYNQMP_PMUFW_SUFFIX),.elf)
> +UBOOT_ZYNQMP_PMUFW_CONVERT=y

> Ditto.

> +endif
> +
>  define UBOOT_ZYNQMP_KCONFIG_PMUFW
> -	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")
> +	$(if $(UBOOT_ZYNQMP_PMUFW_CONVERT),
> +		objcopy -O binary -I elf32-little 
> +$(UBOOT_ZYNQMP_PMUFW_BASENAME).elf 
> +$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin)

> You could also write this like this:

>	$(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
>		objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin)

> and get rid of UBOOT_ZYNQMP_PMUFW_CONVERT entirely.

Thank you for this!  I was trying to find a simpler solution, but when I could not think of one, I figured someone from the community would help.

> I was wondering if the usage of objcopy was correct or whether
> $(TARGET_OBJCOPY) would be more correct. But in fact none of them is perfect I guess: the PMU FW is a Microblaze ELF binary, while
> $(TARGET_OBJCOPY) will be an ARM64 objcopy, and plain objcopy will be the machine native objcopy, very unlikely to be a Microblaze one :-)

> So overall, I guess using objcopy is fine here.

I ultimately chose the host objcopy because you are right, the pmufw is not an ARM64 binary.
I think using $(TARGET_OBJCOPY) is a bit misleading.

> Best regards,

> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin Embedded Linux and Kernel engineering and training https://bootlin.com

Thank you for your support!

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

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add pmufw.elf support

[Thomas Petazzoni via buildroot]

On Thu, 13 Oct 2022 05:57:23 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> > +UBOOT_ZYNQMP_PMUFW_BASENAME=$(basename $(UBOOT_ZYNQMP_PMUFW_PATH)) 
> > +UBOOT_ZYNQMP_PMUFW_SUFFIX=$(suffix $(UBOOT_ZYNQMP_PMUFW_PATH))  
> 
> > Spaces around = sign.  
> 
> I will take out the unnecessary spaces.

It is the opposite: you need to add spaces around the = signs.

Thanks!

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 v2 1/1] boot/uboot/uboot.mk: add pmufw.elf support

[Frager, Neal via buildroot]

Hi Thomas,

> > +UBOOT_ZYNQMP_PMUFW_BASENAME=$(basename $(UBOOT_ZYNQMP_PMUFW_PATH)) 
> > +UBOOT_ZYNQMP_PMUFW_SUFFIX=$(suffix $(UBOOT_ZYNQMP_PMUFW_PATH))
> 
> > Spaces around = sign.  
> 
> I will take out the unnecessary spaces.

> It is the opposite: you need to add spaces around the = signs.

Thank you for clarifying.  I will fix that.

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