Re: [ptxdist] [PATCH v3] u-boot: generate environment image

[Michael Olbrich <m.olbrich@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