From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from dude02.hi.pengutronix.de ([2001:67c:670:100:1d::28] helo=dude02.lab.pengutronix.de) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1iY9hj-00034u-R1 for ptxdist@pengutronix.de; Fri, 22 Nov 2019 15:14:35 +0100 Received: from mol by dude02.lab.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1iY9hj-0007D8-Jl for ptxdist@pengutronix.de; Fri, 22 Nov 2019 15:14:35 +0100 Date: Fri, 22 Nov 2019 15:14:35 +0100 From: Michael Olbrich Message-ID: <20191122141435.GF1287@pengutronix.de> References: <20191119164002.5288-1-bruno.thomsen@gmail.com> <9844490.fM4Dk9IfLu@ada> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Subject: Re: [ptxdist] [PATCH v3] u-boot: generate environment image List-Id: PTXdist Development Mailing List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: ptxdist@pengutronix.de Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ptxdist-bounces@pengutronix.de Sender: "ptxdist" To: ptxdist@pengutronix.de Hi, On Fri, Nov 22, 2019 at 02:31:30PM +0100, Bruno Thomsen wrote: > > thanks for your effort. I guess some folks have custom rules for generating > > env images, so do we. It's good to have a common approach for ordinary use > > cases to prevent reinventing the wheel. I have some remarks however, see > > below. ;-) > > Thanks for review. > > We just realized that around 75% of our rules was custom but not > project specific > in any way. So we might as well upstream them if possible :) My Thanks to both of you for working on this. I don't use u-boot, so I can only comment on the PTXdist stuff. So having someone else to review this is great. > > > +config U_BOOT_ENV_IMAGE_SIZE > > > + prompt "Environment image size" > > > + string > > > + default "0x2000" > > > + help > > > + Enter the U-Boot environment size for generation of image. > > > + Size can be prefixed with 0x for hexadecimal values. > > > + Must match size defined in target config and "/etc/fw_env.config". > > > > Kconfig knows the type "hex" which you could use here. It would prevent > > invalid user input. > > Ok, will fix in next version. > > > > +ifdef PTXCONF_U_BOOT_DEFAULT_ENV_IMAGE > > > + $(U_BOOT_MAKE_ENV) $(U_BOOT_DIR)/scripts/get_default_envs.sh > > > $(U_BOOT_BUILD_DIR) | \ + $(U_BOOT_BUILD_DIR)/tools/mkenvimage -p 0x0 \ > > > + $(call ptx/ifdef,PTXCONF_U_BOOT_ENV_IMAGE_REDUNDANT,-r,) \ > > > + -s $(PTXCONF_U_BOOT_ENV_IMAGE_SIZE) \ > > > + -o $(U_BOOT_BUILD_DIR)/u-boot-env.img - > > > > This way of calling scripts/get_default_envs.sh is possible since U-Boot > > v2018.03-rc3, it will probably fail with older U-Boot. We allow arbitrary U- > > Boot versions with this package and can not easily test on that. Nevertheless > > I would keep it in, but add a hint in the menu on this requirement. Users with > > older U-Boot can just deactivate it. > > > > With U-Boot v2018.01 this gives: > > > > File 'env_common.o' not found! > > > > The resulting platform-v7a/images/u-boot-env.img is just the checksum and all > > zeros then. > > > > The script itself exists since U-Boot v2016.11-rc2, but without support for > > separate build dir, and it might be broken somewhere in between. Before > > v2016.11 we would get such an error: > > > > /bin/bash: /home/adahl/Work/bsp/tt-generic/platform-v7a/build-target/u- > > boot-2016.09.01/scripts/get_default_envs.sh: No such file or directory > > > > The resulting platform-v7a/images/u-boot-env.img is also just the checksum and > > all zeros then. > > > > I'm not sure if we can bail out here or just add something to the help text? > > Maybe add it to the prompt? > > I will update help to reflect this limitation using final version as shipping > with release candidates is just bad practice(tm) ;) > > Running an old version of U-Boot has many limitation like poor support of new > gcc versions and boards not using Kconfig. So don't enable this option if you > use an ancient U-Boot version. Evolution has to continue. > > > > > > +endif > > > +ifdef PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE > > > + $(U_BOOT_BUILD_DIR)/tools/mkenvimage -p 0x0 \ > > > > Why -p 0x0? What's the benefit over stuffing with 0xFF by default? > > U-boot run-time will by default pad with 0x00 and mkenvimage pad with > 0xFF by default. U-boot does not care what pad is used. > So it was just to mimic what normal U-boot run-time does, but we can use > default. > > > > > > + $(call ptx/ifdef,PTXCONF_U_BOOT_ENV_IMAGE_REDUNDANT,-r,) \ > > > + -s $(PTXCONF_U_BOOT_ENV_IMAGE_SIZE) \ > > > + -o $(U_BOOT_BUILD_DIR)/u-boot-custom-env.img \ > > > + $(PTXDIST_WORKSPACE)/$(PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE_SOURCE) > > > > I'm not sure about that path. I would put my input file to e.g. [workspace]/ > > configs/platform-v7a/env_source.config and use of PTXDIST_PLATFORMCONFIGDIR is > > not possible this way. I would have to set > > PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE_SOURCE it like this: > > > > configs/platform-${PTXCONF_PLATFORM}/my-env-source.config > > > > besides: I had a similar problem with dts files and layers. You can not use > > the macros from the make files in those config variables for automatically > > locating that in lower layers. :-/ > > > > Anyways, if I set it to a working path, it works. > > > > You might add a make dependency on the input file however, so the stage gets > > rebuild automatically if the custom env input changes? > > > > Okay, rebuild in case of source file changes and move it to platformconfigdir, > so it's stored next to the kernel configuration. > > Have not tested the below section with ptxdist layers, but seems to work > in my setup without layers. According to docs[1] it should search from top > layer and down. > > ifdef PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE > U_BOOT_CUSTOM_ENV_SRC := $(call ptx/in-platformconfigdir, \ > $(PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE_SOURCE)) You need the remove_quotes here for PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE_SOURCE. Other than that, this should work correctly with layers. > $(STATEDIR)/u-boot.compile: $(call remove_quotes, $(U_BOOT_CUSTOM_ENV_SRC)) No need for remove_quotes here. Michael > endif > > > > +ifdef PTXCONF_U_BOOT_DEFAULT_ENV_IMAGE > > > + @install -v -D -m644 $(U_BOOT_BUILD_DIR)/u-boot-env.img \ > > > + $(IMAGEDIR)/u-boot-env.img > > > +endif > > > +ifdef PTXCONF_U_BOOT_CUSTOM_ENV_IMAGE > > > + @install -v -D -m644 $(U_BOOT_BUILD_DIR)/u-boot-custom-env.img \ > > > + $(IMAGEDIR)/u-boot-custom-env.img > > > +endif > > > > > > ifdef PTXCONF_U_BOOT_BOOT_SCRIPT > > > @$(call install_init, u-boot) > > > > Removing those images in clean stage is missing. > > Yes, I will add them to cleanup including a missing cleanup of imx dtb image. > > /Bruno > > [1] https://www.ptxdist.org/doc/dev_manual.html#writing-layer-aware-rules > > _______________________________________________ > ptxdist mailing list > ptxdist@pengutronix.de > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ ptxdist mailing list ptxdist@pengutronix.de