| Message ID | 20200622204531.11301-1-brandon.maier@rockwellcollins.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | arm64: zynqmp: Support converting pm_cfg_obj.c | expand |
Hi Brandon, On 22/06/20 22:45, Brandon Maier wrote: > To use CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, a developer must pull down the > U-Boot source and run ./tools/zynqmp_pm_cfg_obj_convert.py to convert > their pm_cfg_obj.c into U-Boot's PMU loader format, then feed that file > back to U-Boot during build. > > Instead, by doing the conversion in U-Boot during the build, we can > simplify the developer's build system. And it ensures that if > zynqmp_pm_cfg_obj_convert.py is updated, the pm_cfg_obj will stay in > sync with U-Boot. In the workflows I have used so far U-Boot tools are extracted in a different step w.r.t. U-Boot cross-compilation, so I never felt the need for this feature. But I understand this feature can be handy. The code looks OK too. Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
On 22. 06. 20 22:45, Brandon Maier wrote: > To use CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, a developer must pull down the > U-Boot source and run ./tools/zynqmp_pm_cfg_obj_convert.py to convert > their pm_cfg_obj.c into U-Boot's PMU loader format, then feed that file > back to U-Boot during build. > > Instead, by doing the conversion in U-Boot during the build, we can > simplify the developer's build system. And it ensures that if > zynqmp_pm_cfg_obj_convert.py is updated, the pm_cfg_obj will stay in > sync with U-Boot. > > Add a config to set the file format, but leave the default as binary > type for backwards compatibility. Based on description I can't see the reason for it. pmu config object should be aligned with pmu not with u-boot itself. Also using this script is just one way how to get pmufw config object. I personally don't use it but I can't see any issue to be in u-boot project if others want to use it. Thanks, Michal
Forwarding as I forgot to CC ---------- Forwarded message --------- From: Brandon Maier <brandon.maier@rockwellcollins.com> Date: Tue, Jun 23, 2020 at 12:08 PM Subject: Re: [PATCH] arm64: zynqmp: Support converting pm_cfg_obj.c To: Luca Ceresoli <luca@lucaceresoli.net> On Tue, Jun 23, 2020 at 10:18 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Brandon, > > On 22/06/20 22:45, Brandon Maier wrote: > > To use CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, a developer must pull down the > > U-Boot source and run ./tools/zynqmp_pm_cfg_obj_convert.py to convert > > their pm_cfg_obj.c into U-Boot's PMU loader format, then feed that file > > back to U-Boot during build. > > > > Instead, by doing the conversion in U-Boot during the build, we can > > simplify the developer's build system. And it ensures that if > > zynqmp_pm_cfg_obj_convert.py is updated, the pm_cfg_obj will stay in > > sync with U-Boot. > > In the workflows I have used so far U-Boot tools are extracted in a > different step w.r.t. U-Boot cross-compilation, so I never felt the need > for this feature. But I understand this feature can be handy. I'm trying to get it to work with Buildroot. Buildroot does have a package for uboot-tools, so I could use that to run it. I'd have to move the config and build commands into Buildroot's uboot package. Since the script belongs to U-Boot anyway, it seemed cleaner to integrate it directly into U-Boot so it could be used by other build systems too. > > The code looks OK too. > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > -- > Luca
Forgot to CC ---------- Forwarded message --------- From: Brandon Maier <brandon.maier@rockwellcollins.com> Date: Tue, Jun 23, 2020 at 12:16 PM Subject: Re: [PATCH] arm64: zynqmp: Support converting pm_cfg_obj.c To: Michal Simek <michal.simek@xilinx.com> On Tue, Jun 23, 2020 at 11:11 AM Michal Simek <michal.simek@xilinx.com> wrote: > On 22. 06. 20 22:45, Brandon Maier wrote: > > Add a config to set the file format, but leave the default as binary > > type for backwards compatibility. > > > Based on description I can't see the reason for it. > pmu config object should be aligned with pmu not with u-boot itself. What I mean is, for example if there is a bug in zynqmp_pm_cfg_obj_convert then the pm_cfg binary will automatically be updated the next time U-Boot is built. Compared to if the pm_cfg_obj.bin is stored separately and has to be independently updated for a bug. But it's a minor issue and not that important. > > Also using this script is just one way how to get pmufw config object. > I personally don't use it but I can't see any issue to be in u-boot > project if others want to use it. > We've been trying to piece together the scripts to build pmu config from an HDF using Luca's blog posts and other source code. Out of curiosity, what are these other methods? > > Thanks, > Michal
Hi Brandon, On 23/06/20 19:08, Brandon Maier wrote: > On Tue, Jun 23, 2020 at 10:18 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> Hi Brandon, >> >> On 22/06/20 22:45, Brandon Maier wrote: >>> To use CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, a developer must pull down the >>> U-Boot source and run ./tools/zynqmp_pm_cfg_obj_convert.py to convert >>> their pm_cfg_obj.c into U-Boot's PMU loader format, then feed that file >>> back to U-Boot during build. >>> >>> Instead, by doing the conversion in U-Boot during the build, we can >>> simplify the developer's build system. And it ensures that if >>> zynqmp_pm_cfg_obj_convert.py is updated, the pm_cfg_obj will stay in >>> sync with U-Boot. >> >> In the workflows I have used so far U-Boot tools are extracted in a >> different step w.r.t. U-Boot cross-compilation, so I never felt the need >> for this feature. But I understand this feature can be handy. > > I'm trying to get it to work with Buildroot. Buildroot does have a > package for uboot-tools, so I could use that to run it. I'd have to > move the config and build commands into Buildroot's uboot package. > Since the script belongs to U-Boot anyway, it seemed cleaner to > integrate it directly into U-Boot so it could be used by other build > systems too. I understand. But I also understand Michal's concern: the U-Boot makefiles are quite complex already, and this feature is a relatively minor improvement. Probably having the conversion in Buildroot could be even simpler as you can support _only_ the C format there and do the conversion unconditionally. Only one workflow, no ifs, simpler code. Don't forget to Cc: me if you send a patch for that, I'll be glad to review it.
Hi, On 23/06/20 23:23, Luca Ceresoli wrote: > Hi Brandon, > > On 23/06/20 19:08, Brandon Maier wrote: >> On Tue, Jun 23, 2020 at 10:18 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>> >>> Hi Brandon, >>> >>> On 22/06/20 22:45, Brandon Maier wrote: >>>> To use CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, a developer must pull down the >>>> U-Boot source and run ./tools/zynqmp_pm_cfg_obj_convert.py to convert >>>> their pm_cfg_obj.c into U-Boot's PMU loader format, then feed that file >>>> back to U-Boot during build. >>>> >>>> Instead, by doing the conversion in U-Boot during the build, we can >>>> simplify the developer's build system. And it ensures that if >>>> zynqmp_pm_cfg_obj_convert.py is updated, the pm_cfg_obj will stay in >>>> sync with U-Boot. >>> >>> In the workflows I have used so far U-Boot tools are extracted in a >>> different step w.r.t. U-Boot cross-compilation, so I never felt the need >>> for this feature. But I understand this feature can be handy. >> >> I'm trying to get it to work with Buildroot. Buildroot does have a >> package for uboot-tools, so I could use that to run it. I'd have to >> move the config and build commands into Buildroot's uboot package. >> Since the script belongs to U-Boot anyway, it seemed cleaner to >> integrate it directly into U-Boot so it could be used by other build >> systems too. > > I understand. But I also understand Michal's concern: the U-Boot > makefiles are quite complex already, and this feature is a relatively > minor improvement. > > Probably having the conversion in Buildroot could be even simpler as you > can support _only_ the C format there and do the conversion > unconditionally. Only one workflow, no ifs, simpler code. Don't forget > to Cc: me if you send a patch for that, I'll be glad to review it. I almost forgot: I wrote some notes on how this could be implemented in yocto a while back. The principles are pretty much the same so you might be interested in reading them: https://www.yoctoproject.org/pipermail/meta-xilinx/2019-November/004578.html
On Tue, Jun 23, 2020 at 4:33 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi, > > On 23/06/20 23:23, Luca Ceresoli wrote: > > Hi Brandon, > > > > On 23/06/20 19:08, Brandon Maier wrote: > >> On Tue, Jun 23, 2020 at 10:18 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >>> > >> I'm trying to get it to work with Buildroot. Buildroot does have a > >> package for uboot-tools, so I could use that to run it. I'd have to > >> move the config and build commands into Buildroot's uboot package. > >> Since the script belongs to U-Boot anyway, it seemed cleaner to > >> integrate it directly into U-Boot so it could be used by other build > >> systems too. > > > > I understand. But I also understand Michal's concern: the U-Boot > > makefiles are quite complex already, and this feature is a relatively > > minor improvement. > > > > Probably having the conversion in Buildroot could be even simpler as you > > can support _only_ the C format there and do the conversion > > unconditionally. Only one workflow, no ifs, simpler code. Don't forget > > to Cc: me if you send a patch for that, I'll be glad to review it. Either U-Boot or Buildroot is fine by me. If Yocto alreads does it in their U-Boot package then I can see that being a precedent for patching Buildroot. I'll drop this patch to U-Boot then. > > I almost forgot: I wrote some notes on how this could be implemented in > yocto a while back. The principles are pretty much the same so you might > be interested in reading them: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.yoctoproject.org_pipermail_meta-2Dxilinx_2019-2DNovember_004578.html&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=bIwUnEkCqKFQQ0RVQaaY0gBWY7SIAhmiWLyMS82_mSU&m=RYp4WeqFQ03XpIUkriybKoxAFXTNGqdNLAgzp4S2S8k&s=MswLT0A5A80afFFIfBit21JLH37rUpN4gBC_ACxPw6k&e= > Thanks! > > -- > Luca
On 23. 06. 20 23:46, Brandon Maier wrote: > On Tue, Jun 23, 2020 at 4:33 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> Hi, >> >> On 23/06/20 23:23, Luca Ceresoli wrote: >>> Hi Brandon, >>> >>> On 23/06/20 19:08, Brandon Maier wrote: >>>> On Tue, Jun 23, 2020 at 10:18 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>>> >>>> I'm trying to get it to work with Buildroot. Buildroot does have a >>>> package for uboot-tools, so I could use that to run it. I'd have to >>>> move the config and build commands into Buildroot's uboot package. >>>> Since the script belongs to U-Boot anyway, it seemed cleaner to >>>> integrate it directly into U-Boot so it could be used by other build >>>> systems too. >>> >>> I understand. But I also understand Michal's concern: the U-Boot >>> makefiles are quite complex already, and this feature is a relatively >>> minor improvement. >>> >>> Probably having the conversion in Buildroot could be even simpler as you >>> can support _only_ the C format there and do the conversion >>> unconditionally. Only one workflow, no ifs, simpler code. Don't forget >>> to Cc: me if you send a patch for that, I'll be glad to review it. > > Either U-Boot or Buildroot is fine by me. If Yocto alreads does it in > their U-Boot package then I can see that being a precedent for > patching Buildroot. I'll drop this patch to U-Boot then. I have never started to use Luca's python script because I use different methods how to get this fragment. The first one was taking fsbl and simply extract pmu cfg fragment from it with using binutils. I have sent that script some time ago. And the second method is simply take what Xilinx C file and link it with embeddedSW repository (just for headers) and build it by gcc. A month ago I was experiment with this fragment and was trying to find out if we can have generic one which works on all platforms. As is visible this is fine for boot but not so good for live system. The fragment itself by spec has feature for exchanging it at run time. I sent a patch to utilize it based on current code. https://lists.denx.de/pipermail/u-boot/2020-June/416310.html It means I would more prefer to go in a way that pmufw will be able to accept board/platform/configuration specific fragments and have just one in u-boot which in general enable everything by default. But I don't have time to go to pmufw and fix it. Thanks, Michal
diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig index d82a737a69..d9116870ee 100644 --- a/arch/arm/mach-zynqmp/Kconfig +++ b/arch/arm/mach-zynqmp/Kconfig @@ -65,6 +65,26 @@ config PMUFW_INIT_FILE Include external PMUFW (Platform Management Unit FirmWare) to a Xilinx bootable image (boot.bin). +choice + prompt "PMU firmware configuration format" + default ZYNQMP_SPL_PM_CFG_BIN + help + Format of the file given to ZYNQMP_SPL_PM_CFG_OBJ_FILE. + +config ZYNQMP_SPL_PM_CFG_BIN + bool "pm_cfg_obj.bin" + help + The pm_cfg_obj.c after being preprocessed to a binary blob. + +config ZYNQMP_SPL_PM_CFG_SRC + bool "pm_cfg_obj.c" + help + The source file output from the Xilinx tools. This file is + generated when building the Xilinx SDK BSP. For example at + ./psu_cortexa53_0/libsrc/xilpm_v2_4/src/pm_cfg_obj.c + +endchoice + config ZYNQMP_SPL_PM_CFG_OBJ_FILE string "PMU firmware configuration object to load at runtime by SPL" depends on SPL diff --git a/board/xilinx/zynqmp/Makefile b/board/xilinx/zynqmp/Makefile index 398c6aaa45..05067c43be 100644 --- a/board/xilinx/zynqmp/Makefile +++ b/board/xilinx/zynqmp/Makefile @@ -40,8 +40,16 @@ endif ifdef CONFIG_SPL_BUILD ifneq ($(CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE),"") +ZYNQMP_SPL_CFG_OBJ_FILE = $(shell cd $(srctree); readlink -f $(CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE)) FORCE +ifdef CONFIG_ZYNQMP_SPL_PM_CFG_BIN +board/xilinx/zynqmp/pm_cfg_obj.bin: $(ZYNQMP_SPL_CFG_OBJ_FILE) + cp $< $@ +else +board/xilinx/zynqmp/pm_cfg_obj.bin: $(ZYNQMP_SPL_CFG_OBJ_FILE) + $(srctree)/tools/zynqmp_pm_cfg_obj_convert.py $< $@ +endif obj-$(CONFIG_SPL_BUILD) += pm_cfg_obj.o -$(obj)/pm_cfg_obj.o: $(shell cd $(srctree); readlink -f $(CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE)) FORCE +$(obj)/pm_cfg_obj.o: board/xilinx/zynqmp/pm_cfg_obj.bin endif endif diff --git a/board/xilinx/zynqmp/pm_cfg_obj.S b/board/xilinx/zynqmp/pm_cfg_obj.S index c4ca77e396..77e39017f8 100644 --- a/board/xilinx/zynqmp/pm_cfg_obj.S +++ b/board/xilinx/zynqmp/pm_cfg_obj.S @@ -9,7 +9,7 @@ zynqmp_pm_cfg_obj: .align 4 -.incbin CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE +.incbin "board/xilinx/zynqmp/pm_cfg_obj.bin" zynqmp_pm_cfg_obj_end:
To use CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, a developer must pull down the U-Boot source and run ./tools/zynqmp_pm_cfg_obj_convert.py to convert their pm_cfg_obj.c into U-Boot's PMU loader format, then feed that file back to U-Boot during build. Instead, by doing the conversion in U-Boot during the build, we can simplify the developer's build system. And it ensures that if zynqmp_pm_cfg_obj_convert.py is updated, the pm_cfg_obj will stay in sync with U-Boot. Add a config to set the file format, but leave the default as binary type for backwards compatibility. Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> --- arch/arm/mach-zynqmp/Kconfig | 20 ++++++++++++++++++++ board/xilinx/zynqmp/Makefile | 10 +++++++++- board/xilinx/zynqmp/pm_cfg_obj.S | 2 +- 3 files changed, 30 insertions(+), 2 deletions(-)