| Message ID | 1525364617-23633-4-git-send-email-luca@lucaceresoli.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xilinx ZynqMP and ZCU106 board support | expand |
Hello, On Thu, 3 May 2018 18:23:35 +0200, Luca Ceresoli wrote: > +ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y) > + > +ifneq ($(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR)),) It'd be nice to have an intermediate UBOOT_ZYNQMP_PSU_INIT_DIR variable instead of calculating its qstripped value twice. > +define UBOOT_ZYNQMP_COPY_PSU_INIT > +# In U-Boot's board/xilinx/zynqmp/Makefile the bundled psu_init_gpl.c > +# has precedence over ours if placed in a subdir whose name matches > +# CONFIG_DEFAULT_DEVICE_TREE. Delete them all to be sure we use ours. > + rm -f $(@D)/board/xilinx/zynqmp/*/psu_init*.[ch] > + cp $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR))/psu_init_gpl.[ch] \ > + $(@D)/board/xilinx/zynqmp/ However, the bigger question is why do we need this in the first place ? This is in fact just patching the U-Boot source code. Why not use a patch instead ? Aren't people in general anyway going to have their own custom U-Boot Git tree, in which they will version control their psu_init files ? Since this really looks like a specialized patching step, I'm not sure it really makes sense to add this. Unless you can convince me otherwise :-) Best regards, Thomas
Hi Thomas, On 28/05/2018 22:49, Thomas Petazzoni wrote: > Hello, > > On Thu, 3 May 2018 18:23:35 +0200, Luca Ceresoli wrote: > >> +ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y) >> + >> +ifneq ($(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR)),) > > It'd be nice to have an intermediate UBOOT_ZYNQMP_PSU_INIT_DIR variable > instead of calculating its qstripped value twice. > >> +define UBOOT_ZYNQMP_COPY_PSU_INIT >> +# In U-Boot's board/xilinx/zynqmp/Makefile the bundled psu_init_gpl.c >> +# has precedence over ours if placed in a subdir whose name matches >> +# CONFIG_DEFAULT_DEVICE_TREE. Delete them all to be sure we use ours. >> + rm -f $(@D)/board/xilinx/zynqmp/*/psu_init*.[ch] >> + cp $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR))/psu_init_gpl.[ch] \ >> + $(@D)/board/xilinx/zynqmp/ > > However, the bigger question is why do we need this in the first > place ? This is in fact just patching the U-Boot source code. Why not > use a patch instead ? Aren't people in general anyway going to have > their own custom U-Boot Git tree, in which they will version control > their psu_init files ? Of course course having a custom U-Boot tree with these files versioned is a valid workflow, which does not need the present patch. But there are other valid use cases. > Since this really looks like a specialized patching step, I'm not sure > it really makes sense to add this. Unless you can convince me > otherwise :-) The psu_init files are special because they contain automatically-generated code to configure the SoC peripherals: enable devices, set pinmuxes etc, which is not otherwise done at runtime. Have a look at a few examples: [0] [1]. This file will change whenever the hardware *or* the configuration changes (e.g. to enable peripherals). It is perfectly fine if one wants to use a mainline U-Boot release or a Xilinx release, unmodified, that works fine... *except* for the psu_init file. With your proposed workflow all users will be forced to use a custom git tree, just because they have a slightly different psu_init file. With my patch these users can just use a "standard" U-Boot (downloaded straight from the Internet), throw a psu_init file in their Buildroot boards/ dir and change their Buildroot config to point to it. Even though it's technically code, you can consider the psu_init somewhat like a configuration file. Did I convince you? :-) [0] http://git.denx.de/?p=u-boot.git;a=blob;f=board/xilinx/zynqmp/zynqmp-zcu106-revA/psu_init_gpl.c;h=fcd6a46ad9ffa0da06c4f0a67c2397aa4128ca29;hb=HEAD [1] http://git.denx.de/?p=u-boot.git;a=blob;f=board/xilinx/zynqmp/zynqmp-zcu102-revA/psu_init_gpl.c;h=5f21c4747584815ba86e50a36906b6c1fe8af1ca;hb=HEAD Regards,
On 29-05-18 22:45, Luca Ceresoli wrote: > Hi Thomas, > > On 28/05/2018 22:49, Thomas Petazzoni wrote: [snip] >> Since this really looks like a specialized patching step, I'm not sure >> it really makes sense to add this. Unless you can convince me >> otherwise :-) > > The psu_init files are special because they contain > automatically-generated code to configure the SoC peripherals: enable > devices, set pinmuxes etc, which is not otherwise done at runtime. Have > a look at a few examples: [0] [1]. Regards, Arnout > > This file will change whenever the hardware *or* the configuration > changes (e.g. to enable peripherals). > > It is perfectly fine if one wants to use a mainline U-Boot release or a > Xilinx release, unmodified, that works fine... *except* for the psu_init > file. With your proposed workflow all users will be forced to use a > custom git tree, just because they have a slightly different psu_init > file. With my patch these users can just use a "standard" U-Boot > (downloaded straight from the Internet), throw a psu_init file in their > Buildroot boards/ dir and change their Buildroot config to point to it. > > Even though it's technically code, you can consider the psu_init > somewhat like a configuration file. Yeah, I guess you could say it's a kind of dts but as a .c file instead of a .dts file. Right? > > Did I convince you? :-) You convinced me :-) Regards, Arnout > > [0] > http://git.denx.de/?p=u-boot.git;a=blob;f=board/xilinx/zynqmp/zynqmp-zcu106-revA/psu_init_gpl.c;h=fcd6a46ad9ffa0da06c4f0a67c2397aa4128ca29;hb=HEAD > [1] > http://git.denx.de/?p=u-boot.git;a=blob;f=board/xilinx/zynqmp/zynqmp-zcu102-revA/psu_init_gpl.c;h=5f21c4747584815ba86e50a36906b6c1fe8af1ca;hb=HEAD > > Regards, >
Hi Arnout, On 30/05/2018 22:43, Arnout Vandecappelle wrote: > > > On 29-05-18 22:45, Luca Ceresoli wrote: >> Hi Thomas, >> >> On 28/05/2018 22:49, Thomas Petazzoni wrote: > [snip] >>> Since this really looks like a specialized patching step, I'm not sure >>> it really makes sense to add this. Unless you can convince me >>> otherwise :-) >> >> The psu_init files are special because they contain >> automatically-generated code to configure the SoC peripherals: enable >> devices, set pinmuxes etc, which is not otherwise done at runtime. Have >> a look at a few examples: [0] [1]. > Regards, > Arnout >> >> This file will change whenever the hardware *or* the configuration >> changes (e.g. to enable peripherals). >> >> It is perfectly fine if one wants to use a mainline U-Boot release or a >> Xilinx release, unmodified, that works fine... *except* for the psu_init >> file. With your proposed workflow all users will be forced to use a >> custom git tree, just because they have a slightly different psu_init >> file. With my patch these users can just use a "standard" U-Boot >> (downloaded straight from the Internet), throw a psu_init file in their >> Buildroot boards/ dir and change their Buildroot config to point to it. >> >> Even though it's technically code, you can consider the psu_init >> somewhat like a configuration file. > > Yeah, I guess you could say it's a kind of dts but as a .c file instead of a > .dts file. Right? Right, in that they both contain some configuration data. But with the difference that a dtb is usually a separate file from the kernel, while the psu_init is compiled and linked in the bootloader binary. psu_init is more similar to an appended DTB.
diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in index 95c17e39865e..adde4040e55b 100644 --- a/boot/uboot/Config.in +++ b/boot/uboot/Config.in @@ -365,6 +365,36 @@ config BR2_TARGET_UBOOT_ZYNQ_IMAGE for u-boot-dtb.img file so this U-Boot format is required to be set. +config BR2_TARGET_UBOOT_ZYNQMP + bool "Boot on the Xilinx ZynqMP SoCs" + depends on BR2_aarch64 + help + Enable options specific to the Xilinx ZynqMP family of SoCs. + +if BR2_TARGET_UBOOT_ZYNQMP + +config BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR + string "Custom psu_init_gpl files" + help + On ZynqMP the booloader is responsible for some basic + initializations, such as enabling peripherals and + configuring pinmuxes. The psu_init_gpl.c file (and, + optionally, psu_init_gpl.h) contains the code for such + initializations. + + Although U-Boot contains psu_init_gpl.c files for some + boards, each of them describes only one specific + configuration. Users of a different board, or needing a + different configuration, can generate custom files using the + Xilinx development tools. + + Set this variable to the path where the psu_init_gpl.c file + (and psu_init_gpl.h if needed) is located. U-Boot will build + and link the user-provided file instead of the built-in + one. Leave empty to use the files provided by U-Boot. + +endif + config BR2_TARGET_UBOOT_ALTERA_SOCFPGA_IMAGE_CRC bool "CRC image for Altera SoC FPGA (mkpimage)" depends on BR2_arm diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index 03bd7ea743ed..cbb899515181 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -274,6 +274,22 @@ define UBOOT_INSTALL_IMAGES_CMDS $(BINARIES_DIR)/boot.scr) endef +ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y) + +ifneq ($(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR)),) +define UBOOT_ZYNQMP_COPY_PSU_INIT +# In U-Boot's board/xilinx/zynqmp/Makefile the bundled psu_init_gpl.c +# has precedence over ours if placed in a subdir whose name matches +# CONFIG_DEFAULT_DEVICE_TREE. Delete them all to be sure we use ours. + rm -f $(@D)/board/xilinx/zynqmp/*/psu_init*.[ch] + cp $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR))/psu_init_gpl.[ch] \ + $(@D)/board/xilinx/zynqmp/ +endef +UBOOT_PRE_CONFIGURE_HOOKS += UBOOT_ZYNQMP_COPY_PSU_INIT +endif # BR2_TARGET_UBOOT_ZYNQMP_PSU_INIT_DIR + +endif # BR2_TARGET_UBOOT_ZYNQMP + define UBOOT_INSTALL_OMAP_IFT_IMAGE cp -dpf $(@D)/$(UBOOT_BIN_IFT) $(BINARIES_DIR)/ endef
U-Boot SPL configures pinmuxes, clocks and other low-level devices. On the Xilinx ZynqMP SoCs the code to do this resides in a file called psu_init_gpl.c which is initially generated by the Xilinx development tools. Add an option to pass these files from the outside (e.g. in the board files). Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- Changes v2 -> v3: - Add a bool option to show/hidw all ZynqMP-specific config knobs - Move this patch before "uboot: zynqmp: generate SPL image with PMUFW binary" - Reword Config.in text Changes v1 -> v2: - split from a larger patch doing 2 things - document the option of having psu_init_gpl.c without .h --- boot/uboot/Config.in | 30 ++++++++++++++++++++++++++++++ boot/uboot/uboot.mk | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)