| Message ID | 20240815-boot-uboot-clean-for-pmufw-v1-1-3f0d9909c0b4@collins.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | boot/uboot: clean-up for ZynqMP pmufw handling | expand |
> 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
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
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
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
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),)
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(+)