Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
@ 2022-12-09 19:40 Brandon Maier via buildroot
  2022-12-10  9:55 ` Frager, Neal via buildroot
  2022-12-12  9:31 ` Luca Ceresoli via buildroot
  0 siblings, 2 replies; 11+ messages in thread
From: Brandon Maier via buildroot @ 2022-12-09 19:40 UTC (permalink / raw)
  To: buildroot; +Cc: Luca Ceresoli, Brandon Maier, Neal Frager

BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always
tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. Rework
the logic so if the pm_cfg_obj isn't a C file, it is provided directly
to U-Boot.

This mimics changes done to the ZYNQMP_PMUFW which had a similar issue.

Signed-off-by: Brandon Maier <brandon.maier@collins.com>
---
 boot/uboot/Config.in | 10 ++++++----
 boot/uboot/uboot.mk  | 17 +++++++----------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index a729280060..75bfaef2a8 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -496,10 +496,8 @@ config BR2_TARGET_UBOOT_ZYNQMP_PM_CFG
 	help
 	  Location of a PMU configuration file.
 
-	  If not empty, Buildroot will convert the PMU configuration
-	  file into a loadable blob and pass it to U-Boot. The blob gets
-	  embedded into the U-Boot SPL and is used to configure the PMU
-	  during board initialization.
+	  If not empty, the blob gets embedded into the U-Boot SPL and
+	  is used to configure the PMU during board initialization.
 
 	  Unlike the PMU firmware, the PMU configuration file is unique
 	  to each board configuration. A PMU configuration file can be
@@ -507,6 +505,10 @@ config BR2_TARGET_UBOOT_ZYNQMP_PM_CFG
 	  the BSP source, for example at
 	    ./psu_cortexa53_0/libsrc/xilpm_v2_4/src/pm_cfg_obj.c
 
+	  If the file is a C source file, Buildroot will convert the PMU
+	  configuration file into a loadable blob before handing off to
+	  U-Boot.
+
 	  Leave this option empty if your PMU firmware has a hard-coded
 	  configuration object or you are loading it by any other means.
 
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 99e80ea5a1..31093992f6 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -406,18 +406,15 @@ endef
 
 UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG))
 ifneq ($(UBOOT_ZYNQMP_PM_CFG),)
-UBOOT_ZYNQMP_PM_CFG_BIN = $(UBOOT_DIR)/pm_cfg_obj.bin
-define UBOOT_ZYNQMP_KCONFIG_PM_CFG
-	$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_BIN)", \
-		$(@D)/.config)
-endef
+UBOOT_ZYNQMP_PM_CFG_PATH = $(shell readlink -f $(UBOOT_ZYNQMP_PM_CFG))
+UBOOT_ZYNQMP_PM_CFG_BASENAME = $(basename $(UBOOT_ZYNQMP_PM_CFG_PATH))
 
-define UBOOT_ZYNQMP_PM_CFG_CONVERT
-	$(UBOOT_DIR)/tools/zynqmp_pm_cfg_obj_convert.py \
-		"$(UBOOT_ZYNQMP_PM_CFG)" \
-		"$(UBOOT_ZYNQMP_PM_CFG_BIN)"
+define UBOOT_ZYNQMP_KCONFIG_PM_CFG
+	$(if $(filter %.c,$(UBOOT_ZYNQMP_PM_CFG_PATH)),
+		$(UBOOT_DIR)/tools/zynqmp_pm_cfg_obj_convert.py $(UBOOT_ZYNQMP_PM_CFG_BASENAME).c $(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin
+		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin"),
+		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_PATH)"))
 endef
-UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PM_CFG_CONVERT
 endif
 
 UBOOT_ZYNQMP_PSU_INIT = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_FILE))
-- 
2.38.1

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

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

* Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-09 19:40 [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj Brandon Maier via buildroot
@ 2022-12-10  9:55 ` Frager, Neal via buildroot
  2022-12-12  9:35   ` Luca Ceresoli via buildroot
  2022-12-12  9:31 ` Luca Ceresoli via buildroot
  1 sibling, 1 reply; 11+ messages in thread
From: Frager, Neal via buildroot @ 2022-12-10  9:55 UTC (permalink / raw)
  To: Brandon Maier, buildroot@buildroot.org; +Cc: Luca Ceresoli

Hi Brandon,

> BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. Rework the logic so if the pm_cfg_obj isn't a C file, it is provided directly to U-Boot.

Good idea to add this support.  It is true that since developers cannot build the pmufw with buildroot, it makes sense that they may also
build the pm_cfg_obj.bin binary outside of buildroot as well.

> +	$(if $(filter %.c,$(UBOOT_ZYNQMP_PM_CFG_PATH)),
> +		$(UBOOT_DIR)/tools/zynqmp_pm_cfg_obj_convert.py $(UBOOT_ZYNQMP_PM_CFG_BASENAME).c $(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin
> +		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin"),
> +		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_PATH)"))
>  endef

Just a question to the community.  Should we handle the error when a user configures the BR2_TARGET_UBOOT_ZYNQMP_PM_CFG to be a file
that is not a c file nor a binary file?  With the above if statement, the file would get fed directly into u-boot and the build process will succeed.
However, the user will get a run-time error at boot time when the spl attempts to load an invalid pmu configuration blob.

In the previous version, when we only accepted c files, the zynqmp_pm_cfg_obj_convert.py would error out causing a build failure if the input
file was not an expected pm_cfg_obj.c file.

So the question is which is better.  If the user configures an invalid file format, should there be a build failure or a run-time boot failure?

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

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

* Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-09 19:40 [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj Brandon Maier via buildroot
  2022-12-10  9:55 ` Frager, Neal via buildroot
@ 2022-12-12  9:31 ` Luca Ceresoli via buildroot
  2022-12-12 12:52   ` Frager, Neal via buildroot
  2022-12-12 15:41   ` [Buildroot] [External] " Maier, Brandon L Collins via buildroot
  1 sibling, 2 replies; 11+ messages in thread
From: Luca Ceresoli via buildroot @ 2022-12-12  9:31 UTC (permalink / raw)
  To: Brandon Maier; +Cc: Neal Frager, buildroot

Hello Brandon,

thanks for your patch.

On Fri,  9 Dec 2022 13:40:43 -0600
Brandon Maier <brandon.maier@collins.com> wrote:

> BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always
> tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. Rework
> the logic so if the pm_cfg_obj isn't a C file, it is provided directly
> to U-Boot.

Can you describe a use case where this would be useful? While I
intuitively understand there _might_ be some, I cannot find any good
example based on my experience.

Building the pm_cfg_obj from source code is very easily done by
Buildroot, unlike building the PMUFW. Additionally one PMUFW binary
can be used on multiple zynqmp hardware, while the cfg obj is very
likely to change across different projects, and a somewhat readable
text file is way more understandable than a binary blob.

> This mimics changes done to the ZYNQMP_PMUFW which had a similar issue.

That is different: we now support two different binary formats, not
binary-vs-source, and detection is based on the file extension.

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

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

* Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-10  9:55 ` Frager, Neal via buildroot
@ 2022-12-12  9:35   ` Luca Ceresoli via buildroot
  2022-12-12  9:43     ` Frager, Neal via buildroot
  0 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli via buildroot @ 2022-12-12  9:35 UTC (permalink / raw)
  To: Frager, Neal; +Cc: Brandon Maier, buildroot@buildroot.org

Hello Brandon, Neal,

On Sat, 10 Dec 2022 09:55:01 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> Hi Brandon,
> 
> > BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. Rework the logic so if the pm_cfg_obj isn't a C file, it is provided directly to U-Boot.  
> 
> Good idea to add this support.  It is true that since developers cannot build the pmufw with buildroot, it makes sense that they may also
> build the pm_cfg_obj.bin binary outside of buildroot as well.
> 
> 
> > +	$(if $(filter %.c,$(UBOOT_ZYNQMP_PM_CFG_PATH)),
> > +		$(UBOOT_DIR)/tools/zynqmp_pm_cfg_obj_convert.py $(UBOOT_ZYNQMP_PM_CFG_BASENAME).c $(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin
> > +		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin"),
> > +		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_PATH)"))
> >  endef  
> 
> Just a question to the community.  Should we handle the error when a user configures the BR2_TARGET_UBOOT_ZYNQMP_PM_CFG to be a file
> that is not a c file nor a binary file?  With the above if statement, the file would get fed directly into u-boot and the build process will succeed.
> However, the user will get a run-time error at boot time when the spl attempts to load an invalid pmu configuration blob.
> 
> In the previous version, when we only accepted c files, the zynqmp_pm_cfg_obj_convert.py would error out causing a build failure if the input
> file was not an expected pm_cfg_obj.c file.
> 
> So the question is which is better.  If the user configures an invalid file format, should there be a build failure or a run-time boot failure?

How would you detect which binary is "valid"? It's a pretty fuzzy
definition.

We definitely don't check the validity of other binaries the user wants
to use (the pmufw to name one), which makes sense as Buildroot users
need to understand what they are doing after all.

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

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

* Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-12  9:35   ` Luca Ceresoli via buildroot
@ 2022-12-12  9:43     ` Frager, Neal via buildroot
  0 siblings, 0 replies; 11+ messages in thread
From: Frager, Neal via buildroot @ 2022-12-12  9:43 UTC (permalink / raw)
  To: Luca Ceresoli; +Cc: Brandon Maier, buildroot@buildroot.org

Hi Luca,

> Hi Brandon,
> 
> > BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. Rework the logic so if the pm_cfg_obj isn't a C file, it is provided directly to U-Boot.  
> 
> Good idea to add this support.  It is true that since developers 
> cannot build the pmufw with buildroot, it makes sense that they may also build the pm_cfg_obj.bin binary outside of buildroot as well.
> 
> 
> > +	$(if $(filter %.c,$(UBOOT_ZYNQMP_PM_CFG_PATH)),
> > +		$(UBOOT_DIR)/tools/zynqmp_pm_cfg_obj_convert.py $(UBOOT_ZYNQMP_PM_CFG_BASENAME).c $(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin
> > +		$(call KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_PM_CFG_BASENAME).bin"),
> > +		$(call 
> > +KCONFIG_SET_OPT,CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE,"$(UBOOT_ZYNQMP_P
> > +M_CFG_PATH)"))
> >  endef
> 
> Just a question to the community.  Should we handle the error when a 
> user configures the BR2_TARGET_UBOOT_ZYNQMP_PM_CFG to be a file that is not a c file nor a binary file?  With the above if statement, the file would get fed directly into u-boot and the build process will succeed.
> However, the user will get a run-time error at boot time when the spl attempts to load an invalid pmu configuration blob.
> 
> In the previous version, when we only accepted c files, the 
> zynqmp_pm_cfg_obj_convert.py would error out causing a build failure if the input file was not an expected pm_cfg_obj.c file.
> 
> So the question is which is better.  If the user configures an invalid file format, should there be a build failure or a run-time boot failure?

> How would you detect which binary is "valid"? It's a pretty fuzzy definition.

> We definitely don't check the validity of other binaries the user wants to use (the pmufw to name one), which makes sense as Buildroot users need to understand what they are doing after all.

Very good points.  We cannot verify the validity of a pm_cfg_obj.bin from the file extension.  Plus, if the pmufw itself is invalid, we will also fail at run-time, since we cannot verify it.

I believe you have convinced me that if a user passes an invalid pm_cfg_obj, it should fail at run-time, so no reason to try to verify the validity of a binary built outside buildroot.

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

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

* Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-12  9:31 ` Luca Ceresoli via buildroot
@ 2022-12-12 12:52   ` Frager, Neal via buildroot
  2022-12-13  8:23     ` Luca Ceresoli via buildroot
  2022-12-12 15:41   ` [Buildroot] [External] " Maier, Brandon L Collins via buildroot
  1 sibling, 1 reply; 11+ messages in thread
From: Frager, Neal via buildroot @ 2022-12-12 12:52 UTC (permalink / raw)
  To: Luca Ceresoli, Brandon Maier; +Cc: buildroot@buildroot.org

Hi Luca,

> BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always 
> tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. 
> Rework the logic so if the pm_cfg_obj isn't a C file, it is provided 
> directly to U-Boot.

> Can you describe a use case where this would be useful? While I intuitively understand there _might_ be some, I cannot find any good example based on my experience.

> Building the pm_cfg_obj from source code is very easily done by Buildroot, unlike building the PMUFW. Additionally one PMUFW binary can be used on multiple zynqmp hardware, while the cfg obj is very likely to change across different projects, and a somewhat readable text file is way more understandable than a binary blob.

I believe Brandon should weigh in on this with his opinion as well.  As I thought about it this morning, I just wanted to share my two cents for what they are worth.

I agree with you that most users would want to build everything with buildroot, if they could.  Right now, we support using pmufw binaries only because we do not have a solution for building the pmufw itself with buildroot.
If users could build the pmufw itself with buildroot, I am sure most of them would.

However, at the same time, does Brandon's patch really hurt anything?  By allowing users to provide a pre-built pm_cfg_obj binary blob, it does not prevent users from still providing a c file.
Basically, it allows for the capability of building the pm_cfg_obj.c file with Xilinx tools and just providing the binary blob for being included in the spl/boot.bin by buildroot without removing any capability.

So from my view, this patch may not be critical, but it does not break anything either.  I am ok with whatever you and Brandon decide about committing it.

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

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

* Re: [Buildroot] [External] Re: [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-12  9:31 ` Luca Ceresoli via buildroot
  2022-12-12 12:52   ` Frager, Neal via buildroot
@ 2022-12-12 15:41   ` Maier, Brandon L Collins via buildroot
  2022-12-13  8:21     ` Luca Ceresoli via buildroot
  2022-12-22 14:37     ` Frager, Neal via buildroot
  1 sibling, 2 replies; 11+ messages in thread
From: Maier, Brandon L Collins via buildroot @ 2022-12-12 15:41 UTC (permalink / raw)
  To: Luca Ceresoli; +Cc: Neal Frager, buildroot@buildroot.org

Hello Luca,

> Can you describe a use case where this would be useful? While I
> intuitively understand there _might_ be some, I cannot find any good
> example based on my experience.
> 
> Building the pm_cfg_obj from source code is very easily done by
> Buildroot, unlike building the PMUFW. Additionally one PMUFW binary
> can be used on multiple zynqmp hardware, while the cfg obj is very
> likely to change across different projects, and a somewhat readable
> text file is way more understandable than a binary blob.
 
One of our developers was having issues getting their pm_cfg_obj.c to compile with zynqmp_pm_cfg_obj_convert.py, so they decided to compile it manually instead. This patch set was the result when they asked why Buildroot wouldn't accept their bin file. However for our team this patch is now moot, as I ported your U-Boot fix for building Vivado 2021.x to their U-Boot and that fixed the issue ;)

I decided to send this patch anyway, as I already tested it and it seems like a reasonable thing to support anyway.

> 
> > This mimics changes done to the ZYNQMP_PMUFW which had a similar
> issue.
> 
> That is different: we now support two different binary formats, not
> binary-vs-source, and detection is based on the file extension.
 
I meant they are similar in the sense, they are both detecting based on file extension and then either preprocessing the file or passing it directly to U-Boot.

> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering

Thanks,
Brandon Maier
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

* Re: [Buildroot] [External] Re: [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-12 15:41   ` [Buildroot] [External] " Maier, Brandon L Collins via buildroot
@ 2022-12-13  8:21     ` Luca Ceresoli via buildroot
  2022-12-22 14:37     ` Frager, Neal via buildroot
  1 sibling, 0 replies; 11+ messages in thread
From: Luca Ceresoli via buildroot @ 2022-12-13  8:21 UTC (permalink / raw)
  To: Maier, Brandon L                            Collins
  Cc: Neal Frager, buildroot@buildroot.org

Hello Brandon,

On Mon, 12 Dec 2022 15:41:27 +0000
"Maier, Brandon L                            Collins"
<Brandon.Maier@collins.com> wrote:

> Hello Luca,
> 
> > Can you describe a use case where this would be useful? While I
> > intuitively understand there _might_ be some, I cannot find any good
> > example based on my experience.
> > 
> > Building the pm_cfg_obj from source code is very easily done by
> > Buildroot, unlike building the PMUFW. Additionally one PMUFW binary
> > can be used on multiple zynqmp hardware, while the cfg obj is very
> > likely to change across different projects, and a somewhat readable
> > text file is way more understandable than a binary blob.  
>  
> One of our developers was having issues getting their pm_cfg_obj.c to compile with zynqmp_pm_cfg_obj_convert.py, so they decided to compile it manually instead. This patch set was the result when they asked why Buildroot wouldn't accept their bin file. However for our team this patch is now moot, as I ported your U-Boot fix for building Vivado 2021.x to their U-Boot and that fixed the issue ;)

Ah, good that you found the fix! :-)

> I decided to send this patch anyway, as I already tested it and it seems like a reasonable thing to support anyway.

Sure, it's worth having it out, in case a real use case will emerge in
the future we can revive it.

> > > This mimics changes done to the ZYNQMP_PMUFW which had a similar  
> > issue.
> > 
> > That is different: we now support two different binary formats, not
> > binary-vs-source, and detection is based on the file extension.  
>  
> I meant they are similar in the sense, they are both detecting based on file extension and then either preprocessing the file or passing it directly to U-Boot.

Ah, sure, indeed my point here was bogus. I replied to your email only
after reading the whole thread and I probably mixed yours and Neal's
arguments in my mind. Apologies for the noise.

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

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

* Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-12 12:52   ` Frager, Neal via buildroot
@ 2022-12-13  8:23     ` Luca Ceresoli via buildroot
  0 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli via buildroot @ 2022-12-13  8:23 UTC (permalink / raw)
  To: Frager, Neal; +Cc: Brandon Maier, buildroot@buildroot.org

Hi Neal,

On Mon, 12 Dec 2022 12:52:37 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> Hi Luca,
> 
> > BR2_TARGET_UBOOT_ZYNQMP_PM_CFG only works with C files, as it always 
> > tries to process them through tools/zynqmp_pm_cfg_obj_convert.py. 
> > Rework the logic so if the pm_cfg_obj isn't a C file, it is provided 
> > directly to U-Boot.  
> 
> > Can you describe a use case where this would be useful? While I intuitively understand there _might_ be some, I cannot find any good example based on my experience.  
> 
> > Building the pm_cfg_obj from source code is very easily done by Buildroot, unlike building the PMUFW. Additionally one PMUFW binary can be used on multiple zynqmp hardware, while the cfg obj is very likely to change across different projects, and a somewhat readable text file is way more understandable than a binary blob.  
> 
> I believe Brandon should weigh in on this with his opinion as well.  As I thought about it this morning, I just wanted to share my two cents for what they are worth.
> 
> I agree with you that most users would want to build everything with buildroot, if they could.  Right now, we support using pmufw binaries only because we do not have a solution for building the pmufw itself with buildroot.
> If users could build the pmufw itself with buildroot, I am sure most of them would.
> 
> However, at the same time, does Brandon's patch really hurt anything?  By allowing users to provide a pre-built pm_cfg_obj binary blob, it does not prevent users from still providing a c file.
> Basically, it allows for the capability of building the pm_cfg_obj.c file with Xilinx tools and just providing the binary blob for being included in the spl/boot.bin by buildroot without removing any capability.
> 
> So from my view, this patch may not be critical, but it does not break anything either.  I am ok with whatever you and Brandon decide about committing it.

I think any line of code that is not very useful (or not useful at all)
does hurt. People have to maintain it, code is less readable, etc.

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

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

* Re: [Buildroot] [External] Re: [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-12 15:41   ` [Buildroot] [External] " Maier, Brandon L Collins via buildroot
  2022-12-13  8:21     ` Luca Ceresoli via buildroot
@ 2022-12-22 14:37     ` Frager, Neal via buildroot
  2022-12-29 22:42       ` Luca Ceresoli via buildroot
  1 sibling, 1 reply; 11+ messages in thread
From: Frager, Neal via buildroot @ 2022-12-22 14:37 UTC (permalink / raw)
  To: Maier, Brandon L                            Collins,
	Luca Ceresoli
  Cc: buildroot@buildroot.org

Hi Brandon, Luca,

> Can you describe a use case where this would be useful? While I 
> intuitively understand there _might_ be some, I cannot find any good 
> example based on my experience.
> 
> Building the pm_cfg_obj from source code is very easily done by 
> Buildroot, unlike building the PMUFW. Additionally one PMUFW binary 
> can be used on multiple zynqmp hardware, while the cfg obj is very 
> likely to change across different projects, and a somewhat readable 
> text file is way more understandable than a binary blob.
 
> One of our developers was having issues getting their pm_cfg_obj.c to compile with zynqmp_pm_cfg_obj_convert.py, so they decided to compile it manually instead. This patch set was the result when they asked why Buildroot wouldn't accept their bin file. However for our team this patch is now moot, as I ported your U-Boot fix for building Vivado 2021.x to their U-Boot and that fixed the issue ;)

> I decided to send this patch anyway, as I already tested it and it seems like a reasonable thing to support anyway.

As this patch is still pending, I thought I would change my neutral position on it.

At the moment, the defines and format of the pm_cfg_obj.c file could change from one Xilinx release to the next.  zynqmp should
be mature enough that these changes are minor, but even a single new define being added to the pm_cfg_obj.c could potentially 
break the zynqmp_pm_cfg_obj_convert.py.  Since users might use any version of the Xilinx tools with buildroot, there always could
be an issue as Brandon has experienced.

For this reason, I believe it is worth having the support for users to supply a pre-compiled pm_cfg_obj.bin, and I would like to see
Brandon's patch included in buildroot.

> 
> > This mimics changes done to the ZYNQMP_PMUFW which had a similar
> issue.
> 
> That is different: we now support two different binary formats, not 
> binary-vs-source, and detection is based on the file extension.
 
> I meant they are similar in the sense, they are both detecting based on file extension and then either preprocessing the file or passing it directly to U-Boot.

> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering

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

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

* Re: [Buildroot] [External] Re: [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj
  2022-12-22 14:37     ` Frager, Neal via buildroot
@ 2022-12-29 22:42       ` Luca Ceresoli via buildroot
  0 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli via buildroot @ 2022-12-29 22:42 UTC (permalink / raw)
  To: Frager, Neal
  Cc: Maier, Brandon L                            Collins,
	buildroot@buildroot.org

Hello,

On Thu, 22 Dec 2022 14:37:52 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> Hi Brandon, Luca,
> 
> > Can you describe a use case where this would be useful? While I 
> > intuitively understand there _might_ be some, I cannot find any good 
> > example based on my experience.
> > 
> > Building the pm_cfg_obj from source code is very easily done by 
> > Buildroot, unlike building the PMUFW. Additionally one PMUFW binary 
> > can be used on multiple zynqmp hardware, while the cfg obj is very 
> > likely to change across different projects, and a somewhat readable 
> > text file is way more understandable than a binary blob.  
>  
> > One of our developers was having issues getting their pm_cfg_obj.c to compile with zynqmp_pm_cfg_obj_convert.py, so they decided to compile it manually instead. This patch set was the result when they asked why Buildroot wouldn't accept their bin file. However for our team this patch is now moot, as I ported your U-Boot fix for building Vivado 2021.x to their U-Boot and that fixed the issue ;)  
> 
> > I decided to send this patch anyway, as I already tested it and it seems like a reasonable thing to support anyway.  
> 
> As this patch is still pending, I thought I would change my neutral position on it.
> 
> At the moment, the defines and format of the pm_cfg_obj.c file could change from one Xilinx release to the next.  zynqmp should
> be mature enough that these changes are minor, but even a single new define being added to the pm_cfg_obj.c could potentially 
> break the zynqmp_pm_cfg_obj_convert.py.  Since users might use any version of the Xilinx tools with buildroot, there always could
> be an issue as Brandon has experienced.
> 
> For this reason, I believe it is worth having the support for users to supply a pre-compiled pm_cfg_obj.bin, and I would like to see
> Brandon's patch included in buildroot.

As you wrote, "zynqmp should be mature enough that these changes are
minor", so I think we should optimistically predict that no breaking
changes are going to happen. We should reconsider this only in case
such issues will start happening in the future creating a major
annoyance.

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

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

end of thread, other threads:[~2022-12-29 22:43 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-09 19:40 [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: support binary pm_cfg_obj Brandon Maier via buildroot
2022-12-10  9:55 ` Frager, Neal via buildroot
2022-12-12  9:35   ` Luca Ceresoli via buildroot
2022-12-12  9:43     ` Frager, Neal via buildroot
2022-12-12  9:31 ` Luca Ceresoli via buildroot
2022-12-12 12:52   ` Frager, Neal via buildroot
2022-12-13  8:23     ` Luca Ceresoli via buildroot
2022-12-12 15:41   ` [Buildroot] [External] " Maier, Brandon L Collins via buildroot
2022-12-13  8:21     ` Luca Ceresoli via buildroot
2022-12-22 14:37     ` Frager, Neal via buildroot
2022-12-29 22:42       ` Luca Ceresoli via buildroot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox