[U-Boot] [PATCH] fix always succesful memory test
Since 51209b1f42cb ("Use common mtest iteration counting"), do_mem_mtest has always reported 0 errors and hence returned 0, even if errors were detected. Fix the helpers mem_test_alt() and mem_test_quick() to return the number of errors found. Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> --- common/cmd_mem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/cmd_mem.c b/common/cmd_mem.c index 9fb2584..efa3929 100644 --- a/common/cmd_mem.c +++ b/common/cmd_mem.c @@ -931,7 +931,7 @@ static ulong mem_test_alt(vu_long *buf, ulong start_addr, ulong end_addr, addr[offset] = 0; } - return 0; + return errs; } static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr, @@ -990,7 +990,7 @@ static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr, val += incr; } - return 0; + return errs; } /* -- 2.5.0 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC PATCH] Allow providing default environment from file
On 2018-01-25 10:30, Lukasz Majewski wrote: > Hi Rasmus, > >> It is sometimes useful to be able to define the entire default >> environment in an external file. > > There is already available script for extracting the environment. > > Please look into: > ./scripts/get_default_envs.sh > > Maybe you can reuse it in this patch? I'm sorry, but I don't see what I could use that for. It seems to do the opposite of what I want, namely extract the default environment and store it in a plain-text file. I want to provide a plain-text file to define the default environment. It's quite likely that that script can be useful for generating a sketch for the external file (i.e., build a U-boot as "usual" with default environment built from various config options etc. etc., then hand-edit that file to remove redundant stuff and add the things one needs). The thing is, having the default environment in an external file makes it much easier to put it under version control than having to maintain a branch inside the U-boot repo just to tweak CONFIG_EXTRA_ENV_SETTINGS. Rasmus ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC PATCH] Allow providing default environment from file
On 2018-01-24 10:55, Rasmus Villemoes wrote: > It is sometimes useful to be able to define the entire default > environment in an external file. This implements a Kconfig option for > allowing that. ping ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] fw_setenv: avoid writing environment when nothing has changed
In the case where one deletes an already-non-existing variable, or sets a variable to the value it already has, there is no point in writing the environment back, thus reducing wear on the underlying storage device. Signed-off-by: Rasmus Villemoes --- tools/env/fw_env.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index a5d75958e1..87aaa15198 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -110,6 +110,7 @@ struct environment { unsigned char *flags; char *data; enum flag_scheme flag_scheme; + int dirty; }; static struct environment environment = { @@ -508,6 +509,9 @@ int fw_env_flush(struct env_opts *opts) if (!opts) opts = _opts; + if (!environment.dirty) + return 0; + /* * Update CRC */ @@ -553,7 +557,8 @@ int fw_env_write(char *name, char *value) deleting = (oldval && !(value && strlen(value))); creating = (!oldval && (value && strlen(value))); - overwriting = (oldval && (value && strlen(value))); + overwriting = (oldval && (value && strlen(value) && + strcmp(oldval, value))); /* check for permission */ if (deleting) { @@ -593,6 +598,7 @@ int fw_env_write(char *name, char *value) /* Nothing to do */ return 0; + environment.dirty = 1; if (deleting || overwriting) { if (*++nxt == '\0') { *env = '\0'; @@ -1441,6 +1447,7 @@ int fw_env_open(struct env_opts *opts) "Warning: Bad CRC, using default environment\n"); memcpy(environment.data, default_environment, sizeof(default_environment)); + environment.dirty = 1; } } else { flag0 = *environment.flags; @@ -1503,6 +1510,7 @@ int fw_env_open(struct env_opts *opts) "Warning: Bad CRC, using default environment\n"); memcpy(environment.data, default_environment, sizeof(default_environment)); + environment.dirty = 1; dev_current = 0; } else { switch (environment.flag_scheme) { -- 2.16.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RFC PATCH] Allow providing default environment from file
It is sometimes useful to be able to define the entire default environment in an external file. This implements a Kconfig option for allowing that. It is somewhat annoying to have two visible Kconfig options; it would probably be more user-friendly to just have the string option (with empty string obviously meaning not to use this feature). But then we'd also need a hidden CONFIG that we can use in the #ifdef in env_default.h, and I don't think one can set a def_bool based on whether a string-valued config is empty or not. I've tried to make the accepted format the same as the one the mkenvimage tool accepts. I have no idea how portable the sed script implementing the "allow embedded newlines in values" is. Nor do I know if one can expect xxd to be available. Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> --- Makefile | 16 env/Kconfig | 18 ++ include/env_default.h | 4 3 files changed, 38 insertions(+) diff --git a/Makefile b/Makefile index 4981a2ed6f..e5ba5213fd 100644 --- a/Makefile +++ b/Makefile @@ -423,6 +423,7 @@ endif version_h := include/generated/version_autogenerated.h timestamp_h := include/generated/timestamp_autogenerated.h +defaultenv_h := include/generated/defaultenv_autogenerated.h no-dot-config-targets := clean clobber mrproper distclean \ help %docs check% coccicheck \ @@ -1366,6 +1367,10 @@ ifeq ($(wildcard $(LDSCRIPT)),) @/bin/false endif +ifeq ($(CONFIG_DEFAULT_ENV_FROM_FILE),y) +prepare1: $(defaultenv_h) +endif + archprepare: prepare1 scripts_basic prepare0: archprepare FORCE @@ -1413,12 +1418,23 @@ define filechk_timestamp.h fi) endef +define filechk_defaultenv.h + (grep -v '^#' | \ +grep -v '^$$' | \ +tr '\n' '\0' | \ +sed -e 's/\\\x0/\n/' | \ +xxd -i ; echo ", 0x00" ; ) +endef + $(version_h): include/config/uboot.release FORCE $(call filechk,version.h) $(timestamp_h): $(srctree)/Makefile FORCE $(call filechk,timestamp.h) +$(defaultenv_h): $(CONFIG_DEFAULT_ENV_FILE:"%"=%) FORCE + $(call filechk,defaultenv.h) + # --- quiet_cmd_cpp_lds = LDS $@ cmd_cpp_lds = $(CPP) -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) \ diff --git a/env/Kconfig b/env/Kconfig index a24370786b..1baebd743b 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -482,4 +482,22 @@ config ENV_SIZE endif +config DEFAULT_ENV_FROM_FILE + bool "Create default environment from file" + help + Normally, the default environment is automatically generated + based on the settings of various CONFIG_* options, as well + as the CONFIG_EXTRA_ENV_SETTINGS. By selecting this option, + you can instead define the entire default environment in an + external file. + +config DEFAULT_ENV_FILE + string "Path to default environment file" + depends on DEFAULT_ENV_FROM_FILE + help + The path containing the default environment. The format is + the same as accepted by the mkenvimage tool: lines + containing key=value pairs, blank lines and lines beginning + with # are ignored. + endmenu diff --git a/include/env_default.h b/include/env_default.h index b574345af2..656d202cc7 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -22,6 +22,7 @@ static char default_environment[] = { #else const uchar default_environment[] = { #endif +#ifndef CONFIG_DEFAULT_ENV_FROM_FILE #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" #endif @@ -108,6 +109,9 @@ const uchar default_environment[] = { CONFIG_EXTRA_ENV_SETTINGS #endif "\0" +#else /* CONFIG_DEFAULT_ENV_FROM_FILE */ +#include "generated/defaultenv_autogenerated.h" +#endif #ifdef DEFAULT_ENV_INSTANCE_EMBEDDED } #endif -- 2.15.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] Allow providing default environment from file
On 2018-03-20 15:47, Rasmus Villemoes wrote: > On 2018-03-20 15:20, Lukasz Majewski wrote: >> Hi Rasmus, >> >>> Modifying the default environment via CONFIG_EXTRA_ENV_SETTINGS is >>> somewhat inflexible, partly because the cpp language does not allow >>> appending to an existing macro. This prevents reuse of "environment >>> fragments" for different boards, which in turn makes maintaining that >>> environment consistently tedious and error-prone. [...] >> >> The patch looks ok. >> >> Reviewed-by: Lukasz Majewski <lu...@denx.de> Any chance this could get picked up, or is there anything more needed on my end? Thanks, Rasmus ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2] Allow providing default environment from file
Modifying the default environment via CONFIG_EXTRA_ENV_SETTINGS is somewhat inflexible, partly because the cpp language does not allow appending to an existing macro. This prevents reuse of "environment fragments" for different boards, which in turn makes maintaining that environment consistently tedious and error-prone. This implements a Kconfig option for allowing one to define the entire default environment in an external file, which can then, for example, be generated programmatically as part of a Yocto recipe, or simply be kept in version control separately from the U-boot repository. Tested-by: Sean Nyekjaer <sean.nyekj...@prevas.dk> Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> --- v2: * rename CONFIG_DEFAULT_ENV_FROM_FILE -> CONFIG_USE_DEFAULT_ENV_FILE * add Tested-by * provide a little more rationale (example use case instead of just "sometimes be useful") * rebase to current master (v2018.03-189-gda773532cd) This adds xxd as a build-time requirement. Not sure whether that needs mentioning in the Kconfig help text. On my Ubuntu 16.04, it is provided by the vim-common package, while more recent Ubuntu and Debian seem to have split it to a separate package. Makefile | 16 env/Kconfig | 18 ++ include/env_default.h | 4 3 files changed, 38 insertions(+) diff --git a/Makefile b/Makefile index 5fa14789d9..867b189c41 100644 --- a/Makefile +++ b/Makefile @@ -423,6 +423,7 @@ endif version_h := include/generated/version_autogenerated.h timestamp_h := include/generated/timestamp_autogenerated.h +defaultenv_h := include/generated/defaultenv_autogenerated.h no-dot-config-targets := clean clobber mrproper distclean \ help %docs check% coccicheck \ @@ -1387,6 +1388,10 @@ ifeq ($(wildcard $(LDSCRIPT)),) @/bin/false endif +ifeq ($(CONFIG_USE_DEFAULT_ENV_FILE),y) +prepare1: $(defaultenv_h) +endif + archprepare: prepare1 scripts_basic prepare0: archprepare FORCE @@ -1434,12 +1439,23 @@ define filechk_timestamp.h fi) endef +define filechk_defaultenv.h + (grep -v '^#' | \ +grep -v '^$$' | \ +tr '\n' '\0' | \ +sed -e 's/\\\x0/\n/' | \ +xxd -i ; echo ", 0x00" ; ) +endef + $(version_h): include/config/uboot.release FORCE $(call filechk,version.h) $(timestamp_h): $(srctree)/Makefile FORCE $(call filechk,timestamp.h) +$(defaultenv_h): $(CONFIG_DEFAULT_ENV_FILE:"%"=%) FORCE + $(call filechk,defaultenv.h) + # --- quiet_cmd_cpp_lds = LDS $@ cmd_cpp_lds = $(CPP) -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) \ diff --git a/env/Kconfig b/env/Kconfig index a3c6298273..e8e21dcfc6 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -486,4 +486,22 @@ config ENV_SIZE endif +config USE_DEFAULT_ENV_FILE + bool "Create default environment from file" + help + Normally, the default environment is automatically generated + based on the settings of various CONFIG_* options, as well + as the CONFIG_EXTRA_ENV_SETTINGS. By selecting this option, + you can instead define the entire default environment in an + external file. + +config DEFAULT_ENV_FILE + string "Path to default environment file" + depends on USE_DEFAULT_ENV_FILE + help + The path containing the default environment. The format is + the same as accepted by the mkenvimage tool: lines + containing key=value pairs, blank lines and lines beginning + with # are ignored. + endmenu diff --git a/include/env_default.h b/include/env_default.h index b574345af2..1fbeb92f38 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -22,6 +22,7 @@ static char default_environment[] = { #else const uchar default_environment[] = { #endif +#ifndef CONFIG_USE_DEFAULT_ENV_FILE #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" #endif @@ -108,6 +109,9 @@ const uchar default_environment[] = { CONFIG_EXTRA_ENV_SETTINGS #endif "\0" +#else /* CONFIG_USE_DEFAULT_ENV_FILE */ +#include "generated/defaultenv_autogenerated.h" +#endif #ifdef DEFAULT_ENV_INSTANCE_EMBEDDED } #endif -- 2.15.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] Allow providing default environment from file
On 2018-03-20 15:20, Lukasz Majewski wrote: > Hi Rasmus, > >> Modifying the default environment via CONFIG_EXTRA_ENV_SETTINGS is >> somewhat inflexible, partly because the cpp language does not allow >> appending to an existing macro. This prevents reuse of "environment >> fragments" for different boards, which in turn makes maintaining that >> environment consistently tedious and error-prone. > > It is also possible to build boot.scr image, which is afterwards read > from e.g. vfat, from text file. > > As a reference and example please look > into ./boards/samsung/common/bootscripts/*.cmd > >> >> This implements a Kconfig option for allowing one to define the entire >> default environment in an external file, which can then, for example, >> be generated programmatically as part of a Yocto recipe, > > Is this yocto generation upstreamed? Or this is some kind of internal > patch? Neither, for now, we're just using the "entire environment in file maintained elsewhere" model. But I can easily see us using the ability to amend the environment with a simple "echo ... >> foo.env" in some pre/postfunc, or having foo.env contain some __PLACEHOLDER__ which we fix up with sed using values from .conf files before the U-boot build. Stuff like tftp server ip address is nice to be able to override easily to point at one's own laptop while doing development. > > The patch looks ok. > > Reviewed-by: Lukasz MajewskiThanks. Rasmus ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/2] Makefile: always preserve output for images that can contain HAB Blocks
The current makefile logic disables creation of the SPL.log/u-boot-ivt.img.log etc. files when V=1 is given on the command line, the rationale presumably being that the user wants and gets the information on the console. However, from general principles, I don't think a higher V= level should affect which build artifacts get generated (and certainly shouldn't produce fewer). Concretely, it's also a problem that when doing a V=1 build in a terminal, the relevant HAB blocks lines easily drown in all the other V=1 output. Moreover, build systems such as Yocto by default pass V=1, so in that case the information gets hidden away in the do_compile log file, making it nigh impossible to create a recipe for creating signed U-boot images - I don't want to disable V=1, because having verbose output in the log file is valuable when things go wrong, but OTOH trying to go digging in the do_compile log file (and getting exactly the right lines) is not pleasant to even think about. So change the logic so that for V=0, the mkimage output is redirected to MKIMAGEOUTPUT (which is also the current behaviour), while for any other value of V, we _additionally_ write the information to make's stdout, whatever that might be. Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> --- Makefile | 4 ++-- scripts/Makefile.lib | 2 +- scripts/Makefile.spl | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 5fa14789d9..a63dc96e57 100644 --- a/Makefile +++ b/Makefile @@ -845,11 +845,11 @@ MKIMAGEOUTPUT ?= /dev/null quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) quiet_cmd_mkfitimage = MKIMAGE $@ cmd_mkfitimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -f $(U_BOOT_ITS) -E $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) quiet_cmd_cat = CAT $@ cmd_cat = cat $(filter-out $(PHONY), $^) > $@ diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 8f21653136..c6b3f69064 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -521,7 +521,7 @@ cmd_xzmisc = (cat $(filter-out FORCE,$^) | \ MKIMAGEOUTPUT ?= /dev/null quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) # fdtgrep # --- diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 2993ade41e..7f2908d799 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -144,7 +144,7 @@ MKIMAGEOUTPUT ?= /dev/null quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) MKIMAGEFLAGS_MLO = -T omapimage -a $(CONFIG_SPL_TEXT_BASE) -- 2.15.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 2/2] tools/imximage: use 0x prefix in HAB Blocks line
The u-boot-ivt.img.log file contains 0x prefixes in the HAB Blocks line, while the SPL.log does not. For consistency, and to make it easier to extract and put into a .csf file for use with NXP's code signing tool, add 0x prefixes here. Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> --- doc/README.mxc_hab | 14 +++--- tools/imximage.c | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/README.mxc_hab b/doc/README.mxc_hab index 75390a505e..a40ebf3e84 100644 --- a/doc/README.mxc_hab +++ b/doc/README.mxc_hab @@ -33,12 +33,12 @@ Image Ver:2 (i.MX53/6 compatible) Data Size:327680 Bytes = 320.00 kB = 0.31 MB Load Address: 177ff420 Entry Point: 1780 -HAB Blocks: 177ff400 0004dc00 - - | | | - | | (1) - | | - | --- (2) +HAB Blocks: 0x177ff400 0x 0x0004dc00 + ^^ ^^ ^^ + | | | + | | - (1) + | | + | (2) | --- (3) @@ -78,7 +78,7 @@ Example Output of the SPL (imximage) creation: Data Size:61440 Bytes = 60.00 kB = 0.06 MB Load Address: 00907420 Entry Point: 00908000 - HAB Blocks: 00907400 cc00 + HAB Blocks: 0x00907400 0x 0xcc00 Example Output of the u-boot-ivt.img (firmware_ivt) creation: Image Name: U-Boot 2016.11-rc1-31589-g2a4411 diff --git a/tools/imximage.c b/tools/imximage.c index ed9d935903..6dabb13520 100644 --- a/tools/imximage.c +++ b/tools/imximage.c @@ -516,7 +516,7 @@ static void print_hdr_v2(struct imx_header *imx_hdr) offs = (char *)_v2->data.dcd_table - (char *)hdr_v2; - printf("HAB Blocks: %08x %08x %08x\n", + printf("HAB Blocks: 0x%08x 0x%08x 0x%08x\n", (uint32_t)fhdr_v2->self, 0, hdr_v2->boot_data.size - imximage_ivt_offset - imximage_csf_size); -- 2.15.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] Makefile: always preserve output for images that can contain HAB Blocks
The current makefile logic disables creation of the SPL.log/u-boot-ivt.img.log etc. files when V=1 is given on the command line, the rationale presumably being that the user wants and gets the information on the console. However, from general principles, I don't think a higher V= level should affect which build artifacts get generated (and certainly shouldn't produce fewer). Concretely, it's also a problem that when doing a V=1 build in a terminal, the relevant HAB blocks lines easily drown in all the other V=1 output. And build systems such as Yocto by default pass V=1, so in that case the information gets hidden away in the do_compile log file, making it nigh impossible to create a recipe for creating signed U-boot images. So change the logic so that for V=0, the mkimage output is redirected to MKIMAGEOUTPUT (which is also the current behaviour), while for any other value of V, we _additionally_ write the information to make's stdout, whatever that might be. Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> --- My first attempt did $(if $(KBUILD_VERBOSE:0=), | tee , >) $(MKIMAGEOUTPUT) but we can't use a pipeline, since tee is always succesful, so we would not propagate errors from mkimage. Makefile | 4 ++-- scripts/Makefile.lib | 2 +- scripts/Makefile.spl | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 50abc979ad..2eb1b1a22f 100644 --- a/Makefile +++ b/Makefile @@ -840,11 +840,11 @@ MKIMAGEOUTPUT ?= /dev/null quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) quiet_cmd_mkfitimage = MKIMAGE $@ cmd_mkfitimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -f $(U_BOOT_ITS) -E $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) quiet_cmd_cat = CAT $@ cmd_cat = cat $(filter-out $(PHONY), $^) > $@ diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 8f21653136..c6b3f69064 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -521,7 +521,7 @@ cmd_xzmisc = (cat $(filter-out FORCE,$^) | \ MKIMAGEOUTPUT ?= /dev/null quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) # fdtgrep # --- diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 2993ade41e..7f2908d799 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -144,7 +144,7 @@ MKIMAGEOUTPUT ?= /dev/null quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ - $(if $(KBUILD_VERBOSE:1=), >$(MKIMAGEOUTPUT)) + >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat $(MKIMAGEOUTPUT)) MKIMAGEFLAGS_MLO = -T omapimage -a $(CONFIG_SPL_TEXT_BASE) -- 2.15.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] fw_setenv: avoid writing environment when nothing has changed
On 2018-09-27 22:45, Rasmus Villemoes wrote: > In the case where one deletes an already-non-existing variable, or sets > a variable to the value it already has, there is no point in writing the > environment back, thus reducing wear on the underlying storage > device. > > In the case of redundant environments, if the two environments > differ (e.g. because one is corrupt), make sure that any call of > fw_setenv causes the two to become synchronized, even if the fw_setenv > call does not change anything in the good copy. > > Signed-off-by: Rasmus Villemoes > --- > Add logic to ensure a corrupt copy gets replaced, even if fw_setenv > wouldn't change anything in the good copy. ping. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fw_setenv: avoid writing environment when nothing has changed
On 2018-09-05 21:22, Rasmus Villemoes wrote: > In the case where one deletes an already-non-existing variable, or sets > a variable to the value it already has, there is no point in writing the > environment back, thus reducing wear on the underlying storage > device. ping ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fw_setenv: avoid writing environment when nothing has changed
On 2018-09-24 09:42, Alex Kiernan wrote: > On Wed, Sep 5, 2018 at 8:23 PM Rasmus Villemoes > wrote: >> >> In the case where one deletes an already-non-existing variable, or sets >> a variable to the value it already has, there is no point in writing the >> environment back, thus reducing wear on the underlying storage >> device. >> >> Signed-off-by: Rasmus Villemoes > > If you were running with a redundant env, and you were trying to sync > both copies, wouldn't you want a non-changing write to happen? Hm, probably, yes. But I'd still like to avoid it if they are already in sync, so perhaps I could just add if (memcmp(environment.data, redundant->data, ENV_SIZE)) environment.dirty = 1; after reading in the second copy [using that environment.data at that point is still ((struct env_image_redundant *)addr0)->data ]. Rasmus ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2] fw_setenv: avoid writing environment when nothing has changed
In the case where one deletes an already-non-existing variable, or sets a variable to the value it already has, there is no point in writing the environment back, thus reducing wear on the underlying storage device. In the case of redundant environments, if the two environments differ (e.g. because one is corrupt), make sure that any call of fw_setenv causes the two to become synchronized, even if the fw_setenv call does not change anything in the good copy. Signed-off-by: Rasmus Villemoes --- Add logic to ensure a corrupt copy gets replaced, even if fw_setenv wouldn't change anything in the good copy. tools/env/fw_env.c | 20 +++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index a5d75958e1..66372dad55 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -110,6 +110,7 @@ struct environment { unsigned char *flags; char *data; enum flag_scheme flag_scheme; + int dirty; }; static struct environment environment = { @@ -508,6 +509,9 @@ int fw_env_flush(struct env_opts *opts) if (!opts) opts = _opts; + if (!environment.dirty) + return 0; + /* * Update CRC */ @@ -553,7 +557,8 @@ int fw_env_write(char *name, char *value) deleting = (oldval && !(value && strlen(value))); creating = (!oldval && (value && strlen(value))); - overwriting = (oldval && (value && strlen(value))); + overwriting = (oldval && (value && strlen(value) && + strcmp(oldval, value))); /* check for permission */ if (deleting) { @@ -593,6 +598,7 @@ int fw_env_write(char *name, char *value) /* Nothing to do */ return 0; + environment.dirty = 1; if (deleting || overwriting) { if (*++nxt == '\0') { *env = '\0'; @@ -1441,6 +1447,7 @@ int fw_env_open(struct env_opts *opts) "Warning: Bad CRC, using default environment\n"); memcpy(environment.data, default_environment, sizeof(default_environment)); + environment.dirty = 1; } } else { flag0 = *environment.flags; @@ -1494,6 +1501,16 @@ int fw_env_open(struct env_opts *opts) crc1_ok = (crc1 == redundant->crc); flag1 = redundant->flags; + /* +* environment.data still points to ((struct +* env_image_redundant *)addr0)->data. If the two +* environments differ, or one has bad crc, force a +* write-out by marking the environment dirty. +*/ + if (memcmp(environment.data, redundant->data, ENV_SIZE) || + !crc0_ok || !crc1_ok) + environment.dirty = 1; + if (crc0_ok && !crc1_ok) { dev_current = 0; } else if (!crc0_ok && crc1_ok) { @@ -1503,6 +1520,7 @@ int fw_env_open(struct env_opts *opts) "Warning: Bad CRC, using default environment\n"); memcpy(environment.data, default_environment, sizeof(default_environment)); + environment.dirty = 1; dev_current = 0; } else { switch (environment.flag_scheme) { -- 2.16.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Default environment file
On 12/06/2019 10.43, Stefano Babic wrote: > Hi Pascal, > > On 12/06/19 10:20, Linder Pascal wrote: >> Hi everyone, >> >> >> I am currently moving the configurations of the KM boards from header files >> to Kconfig. But for the customly defined environment variables I did not >> found a decent solution until I have come across the default environment >> file, which seems very interesting to me. >> >> >> To this day, nevertheless, it appears that noone made use of the >> CONFIG_USE_DEFAULT_ENV_FILE configuration defined in env/Kconfig. Does >> anyone still have an example for this kind of environment definition or >> knows how to create it? >> >> >> In my opinion, this could be highly relevant for the transition away from >> the header files in include/configs. >> > > Fully agree. Rather, I do not think there is a relevant example. But the > environment is something like data and should not be part of the header > file as it is for histoical reason. I added some times ago a way to > extract the environment from the header and make the transition easy > (see make u-boot-initial-env). And if the environment is split from the > header as CONFIG_USE_DEFAULT_ENV_FILE allows, it is also easier to set > an own environment via OE BSP layer without pushing for each small > change to U-Boot. Not only, environments often conflict, and what is > good for a project becomes evil for another one. Yup. FWIW, here's a small snippet from our Yocto recipe that creates the default env file: ENV_INPUT = "common.txt board.txt" ENV_INPUT_append_basic-test = " testctrl.txt" python do_configure() { import fileinput with open("uboot.txt", "w") as out: for line in fileinput.input(files=d.getVar("ENV_INPUT").split()): line = d.expand(line) # Strip backslash-newline if line.endswith("\\\n"): line = line[:-2] out.write(line) } The line = d.expand(line) allows us to use ${SOME_BITBAKE_VARIABLE} in the input files and have them replaced appropriately - this can be quite useful to e.g. configure a tftp server address differently. Rasmus ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] booting multi-image with several embedded dtbs
Hi, I have a (legacy) multi-image uImage with several embedded dtbs, but no initrd. I.e. one created by something like "mkimage -T multi -d zImage:dtb1:dtb2:dtb3". I thought I could boot that using the bootm command like this bootm $loadaddr:0 - $loadaddr:3 # choose dtb3 but that fails with ERROR: uImage is not a fdt Just doing "bootm $loadaddr" kind of works (in the sense that the kernel starts, and I can see that dtb2 got passed as dtb) fails quite miserably once dtb1 gets interpreted as an initrd - and of course what I want is to be able to choose between the dtbs at runtime. Yes, we will switch to proper FIT image at some point, but is there any way to use the existing multi-image uImage as described? Thanks, Rasmus ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] Makefile: fix newline escaping for CONFIG_DEFAULT_ENV_FILE
I wanted this to be compatible with mkenvimage, including the ability to embed newlines in variables by escaping them. But I failed to check that it works more than once. Fixes: f3d8f7dd73a (Allow providing default environment from file) Signed-off-by: Rasmus Villemoes --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index aedc2ea4d9..8ae59ef3f0 100644 --- a/Makefile +++ b/Makefile @@ -1633,7 +1633,7 @@ define filechk_defaultenv.h (grep -v '^#' | \ grep -v '^$$' | \ tr '\n' '\0' | \ -sed -e 's/\\\x0/\n/' | \ +sed -e 's/\\\x0/\n/g' | \ xxd -i ; echo ", 0x00" ; ) endef -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 2/4] arm: mxs: fix comments in arch_cpu_init to match the code
The comment says to clear the bypass bit, but in fact it sets it, thus selecting ref_xtal. And the next line of code does not set the divider to 12, but to (the reset value of) 1. Signed-off-by: Rasmus Villemoes --- arch/arm/cpu/arm926ejs/mxs/mxs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm/cpu/arm926ejs/mxs/mxs.c b/arch/arm/cpu/arm926ejs/mxs/mxs.c index 85c65dcb44..585c53baf6 100644 --- a/arch/arm/cpu/arm926ejs/mxs/mxs.c +++ b/arch/arm/cpu/arm926ejs/mxs/mxs.c @@ -98,11 +98,11 @@ int arch_cpu_init(void) /* * Enable NAND clock */ - /* Clear bypass bit */ + /* Set bypass bit */ writel(CLKCTRL_CLKSEQ_BYPASS_GPMI, _regs->hw_clkctrl_clkseq_set); - /* Set GPMI clock to ref_gpmi / 12 */ + /* Set GPMI clock to ref_xtal / 1 */ clrsetbits_le32(_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_CLKGATE | CLKCTRL_GPMI_DIV_MASK, 1); -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 3/4] arm: mxs: be more careful when enabling gpmi_clk
The data sheet says that the DIV field cannot change while the CLKGATE bit is set or modified. So do it a little more carefully, by first clearing the bit, waiting for that to appear, then setting the DIV field. Signed-off-by: Rasmus Villemoes --- arch/arm/cpu/arm926ejs/mxs/mxs.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/arm/cpu/arm926ejs/mxs/mxs.c b/arch/arm/cpu/arm926ejs/mxs/mxs.c index 585c53baf6..183aa40b6d 100644 --- a/arch/arm/cpu/arm926ejs/mxs/mxs.c +++ b/arch/arm/cpu/arm926ejs/mxs/mxs.c @@ -103,8 +103,11 @@ int arch_cpu_init(void) _regs->hw_clkctrl_clkseq_set); /* Set GPMI clock to ref_xtal / 1 */ + clrbits_le32(_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_CLKGATE); + while (readl(_regs->hw_clkctrl_gpmi) & CLKCTRL_GPMI_CLKGATE) + ; clrsetbits_le32(_regs->hw_clkctrl_gpmi, - CLKCTRL_GPMI_CLKGATE | CLKCTRL_GPMI_DIV_MASK, 1); + CLKCTRL_GPMI_DIV_MASK, 1); udelay(1000); -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 0/4] arm: mxs: mxs_set_gpmiclk
While trying to implement an mxs_set_gpmiclk() I stumbled on a few minor things. Rasmus Villemoes (4): arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX arm: mxs: fix comments in arch_cpu_init to match the code arm: mxs: be more careful when enabling gpmi_clk arm: mxs: implement mxs_set_gpmiclk arch/arm/cpu/arm926ejs/mxs/clock.c| 41 +++ arch/arm/cpu/arm926ejs/mxs/mxs.c | 9 ++-- arch/arm/include/asm/arch-mxs/clock.h | 1 + .../include/asm/arch-mxs/regs-clkctrl-mx23.h | 6 ++- .../include/asm/arch-mxs/regs-clkctrl-mx28.h | 15 --- 5 files changed, 62 insertions(+), 10 deletions(-) -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 4/4] arm: mxs: implement mxs_set_gpmiclk
This allows a board file to set the gpmiclk to something other than the default 24 MHz based on ref_xtal. I don't have an mx23-based board, but I believe there's a bug in the current mxs_get_gpmiclk: According to the data sheet, the gpmiclk can be derived from ref_io, not ref_cpu. Since other clocks are also derived from ref_io, it seems most sensible to require the board file to set that appropriately first, then derive the divider from its current setting. For mx28 boards, OTOH, there's a separate ref_gpmi only used for clk_gpmi. For simplicity, if !xtal, simply enable that at its maximum frequency. Signed-off-by: Rasmus Villemoes --- arch/arm/cpu/arm926ejs/mxs/clock.c| 41 +++ arch/arm/include/asm/arch-mxs/clock.h | 1 + 2 files changed, 42 insertions(+) diff --git a/arch/arm/cpu/arm926ejs/mxs/clock.c b/arch/arm/cpu/arm926ejs/mxs/clock.c index 43d044d917..d247da56fe 100644 --- a/arch/arm/cpu/arm926ejs/mxs/clock.c +++ b/arch/arm/cpu/arm926ejs/mxs/clock.c @@ -138,6 +138,47 @@ static uint32_t mxs_get_gpmiclk(void) return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div; } +void mxs_set_gpmiclk(uint32_t freq, int xtal) +{ + struct mxs_clkctrl_regs *clkctrl_regs = + (struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE; + uint32_t clk, div; + + if (xtal) { + clk = XTAL_FREQ_KHZ; + } else { +#if defined(CONFIG_MX23) + clk = mxs_get_ioclk(MXC_IOCLK0); +#elif defined(CONFIG_MX28) + /* enable ref_gpmi at 480 MHz. */ + writeb(CLKCTRL_FRAC_CLKGATE, + _regs->hw_clkctrl_frac1_set[CLKCTRL_FRAC1_GPMI]); + writeb(CLKCTRL_FRAC_CLKGATE | PLL_FREQ_COEF, + _regs->hw_clkctrl_frac1[CLKCTRL_FRAC1_GPMI]); + writeb(CLKCTRL_FRAC_CLKGATE, + _regs->hw_clkctrl_frac1_clr[CLKCTRL_FRAC1_GPMI]); + clk = PLL_FREQ_KHZ; +#endif + } + if (freq > clk) + return; + div = clk / freq; + if (div > CLKCTRL_GPMI_DIV_MASK) + div = CLKCTRL_GPMI_DIV_MASK; + + clrbits_le32(_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_CLKGATE); + while (readl(_regs->hw_clkctrl_gpmi) & CLKCTRL_GPMI_CLKGATE) + ; + clrsetbits_le32(_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_DIV_MASK, div); + while (readl(_regs->hw_clkctrl_gpmi) & CLKCTRL_GPMI_BUSY) + ; + + if (xtal) + writel(CLKCTRL_CLKSEQ_BYPASS_GPMI, _regs->hw_clkctrl_clkseq_set); + else + writel(CLKCTRL_CLKSEQ_BYPASS_GPMI, _regs->hw_clkctrl_clkseq_clr); +} + /* * Set IO clock frequency, in kHz */ diff --git a/arch/arm/include/asm/arch-mxs/clock.h b/arch/arm/include/asm/arch-mxs/clock.h index ee56d10fec..0a6625eb90 100644 --- a/arch/arm/include/asm/arch-mxs/clock.h +++ b/arch/arm/include/asm/arch-mxs/clock.h @@ -44,6 +44,7 @@ uint32_t mxc_get_clock(enum mxc_clock clk); void mxs_set_ioclk(enum mxs_ioclock io, uint32_t freq); void mxs_set_sspclk(enum mxs_sspclock ssp, uint32_t freq, int xtal); +void mxs_set_gpmiclk(uint32_t freq, int xtal); void mxs_set_ssp_busclock(unsigned int bus, uint32_t freq); void mxs_set_lcdclk(uint32_t __maybe_unused lcd_base, uint32_t freq); -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/4] arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX
I tried clearing a bit by writing to hw_clkctrl_gpmi_clr, then busy-waiting for it to actually clear. My board hung. The data sheet agrees, these registers do not have _set, _clr, _tog, so fix up the definitions. git grep -E 'clkctrl_(gpmi|ssp[0-9])_' says that nobody uses those non-existing ops registers. Signed-off-by: Rasmus Villemoes --- arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h | 6 -- arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h | 15 ++- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h index 6e9ffeb6d5..50fdc9cd03 100644 --- a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h +++ b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h @@ -24,8 +24,10 @@ struct mxs_clkctrl_regs { mxs_reg_32(hw_clkctrl_xbus) /* 0x40 */ mxs_reg_32(hw_clkctrl_xtal) /* 0x50 */ mxs_reg_32(hw_clkctrl_pix) /* 0x60 */ - mxs_reg_32(hw_clkctrl_ssp0) /* 0x70 */ - mxs_reg_32(hw_clkctrl_gpmi) /* 0x80 */ + uint32_thw_clkctrl_ssp0;/* 0x70 */ + uint32_treserved_ssp0[3]; /* 0x74-0x7c */ + uint32_thw_clkctrl_gpmi;/* 0x80 */ + uint32_treserved_gpmi[3]; /* 0x84-0x8c */ mxs_reg_32(hw_clkctrl_spdif)/* 0x90 */ mxs_reg_32(hw_clkctrl_emi) /* 0xa0 */ diff --git a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h index 01e0a7a053..caef9e4b1f 100644 --- a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h +++ b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h @@ -27,11 +27,16 @@ struct mxs_clkctrl_regs { mxs_reg_32(hw_clkctrl_hbus) /* 0x60 */ mxs_reg_32(hw_clkctrl_xbus) /* 0x70 */ mxs_reg_32(hw_clkctrl_xtal) /* 0x80 */ - mxs_reg_32(hw_clkctrl_ssp0) /* 0x90 */ - mxs_reg_32(hw_clkctrl_ssp1) /* 0xa0 */ - mxs_reg_32(hw_clkctrl_ssp2) /* 0xb0 */ - mxs_reg_32(hw_clkctrl_ssp3) /* 0xc0 */ - mxs_reg_32(hw_clkctrl_gpmi) /* 0xd0 */ + uint32_thw_clkctrl_ssp0;/* 0x90 */ + uint32_treserved_ssp0[3]; /* 0x94-0x9c */ + uint32_thw_clkctrl_ssp1;/* 0xa0 */ + uint32_treserved_ssp1[3]; /* 0xa4-0xac */ + uint32_thw_clkctrl_ssp2;/* 0xb0 */ + uint32_treserved_ssp2[3]; /* 0xb4-0xbc */ + uint32_thw_clkctrl_ssp3;/* 0xc0 */ + uint32_treserved_ssp3[3]; /* 0xc4-0xcc */ + uint32_thw_clkctrl_gpmi;/* 0xd0 */ + uint32_treserved_gpmi[3]; /* 0xd4-0xdc */ mxs_reg_32(hw_clkctrl_spdif)/* 0xe0 */ mxs_reg_32(hw_clkctrl_emi) /* 0xf0 */ mxs_reg_32(hw_clkctrl_saif0)/* 0x100 */ -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 2/2] ARM: asm/io.h: remove redundant #if !defined(readb) block
readb is unconditionally defined earlier in io.h, so there's no point checking whether it's undefined. Signed-off-by: Rasmus Villemoes --- arch/arm/include/asm/io.h | 15 --- 1 file changed, 15 deletions(-) diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 78b183d42f..723f3cf497 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -319,21 +319,6 @@ extern void __readwrite_bug(const char *fn); #define memcpy_fromio(a, b, c) memcpy((a), (void *)(b), (c)) #define memcpy_toio(a, b, c) memcpy((void *)(a), (b), (c)) -#if !defined(readb) - -#define readb(addr)(__readwrite_bug("readb"),0) -#define readw(addr)(__readwrite_bug("readw"),0) -#define readl(addr)(__readwrite_bug("readl"),0) -#define writeb(v,addr) __readwrite_bug("writeb") -#define writew(v,addr) __readwrite_bug("writew") -#define writel(v,addr) __readwrite_bug("writel") - -#define eth_io_copy_and_sum(a,b,c,d) __readwrite_bug("eth_io_copy_and_sum") - -#define check_signature(io,sig,len)(0) - -#endif - /* * If this architecture has ISA IO, then define the isa_read/isa_write * macros. -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/2] ARM: asm/io.h: kill off confusing #ifdef __mem_pci block
No ARM board seems to define __mem_pci - and if it did, one would get tons of ./arch/arm/include/asm/io.h:307:0: warning: "readl" redefined warnings, because readl and friends are unconditionally defined earlier in io.h. Moreover, the redefinitions lack the memory barriers that the first definitions have. So I'm guessing this is practically dead code. Signed-off-by: Rasmus Villemoes --- arch/arm/include/asm/io.h | 41 --- 1 file changed, 41 deletions(-) diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index e6d27b69f9..78b183d42f 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -315,46 +315,6 @@ extern void _memset_io(unsigned long, int, size_t); extern void __readwrite_bug(const char *fn); -/* - * If this architecture has PCI memory IO, then define the read/write - * macros. These should only be used with the cookie passed from - * ioremap. - */ -#ifdef __mem_pci - -#define readb(c) ({ unsigned int __v = __raw_readb(__mem_pci(c)); __v; }) -#define readw(c) ({ unsigned int __v = le16_to_cpu(__raw_readw(__mem_pci(c))); __v; }) -#define readl(c) ({ unsigned int __v = le32_to_cpu(__raw_readl(__mem_pci(c))); __v; }) - -#define writeb(v,c)__raw_writeb(v,__mem_pci(c)) -#define writew(v,c)__raw_writew(cpu_to_le16(v),__mem_pci(c)) -#define writel(v,c)__raw_writel(cpu_to_le32(v),__mem_pci(c)) - -#define memset_io(c,v,l) _memset_io(__mem_pci(c),(v),(l)) -#define memcpy_fromio(a,c,l) _memcpy_fromio((a),__mem_pci(c),(l)) -#define memcpy_toio(c,a,l) _memcpy_toio(__mem_pci(c),(a),(l)) - -#define eth_io_copy_and_sum(s,c,l,b) \ - eth_copy_and_sum((s),__mem_pci(c),(l),(b)) - -static inline int -check_signature(unsigned long io_addr, const unsigned char *signature, - int length) -{ - int retval = 0; - do { - if (readb(io_addr) != *signature) - goto out; - io_addr++; - signature++; - length--; - } while (length); - retval = 1; -out: - return retval; -} - -#else #define memset_io(a, b, c) memset((void *)(a), (b), (c)) #define memcpy_fromio(a, b, c) memcpy((a), (void *)(b), (c)) #define memcpy_toio(a, b, c) memcpy((void *)(a), (b), (c)) @@ -373,7 +333,6 @@ out: #define check_signature(io,sig,len)(0) #endif -#endif /* __mem_pci */ /* * If this architecture has ISA IO, then define the isa_read/isa_write -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] ARM: mxs: spl_boot.c: make early_delay more robust
It's true that booting normally doesn't take long enough for the register to roll (which actually happens in a little over an hour, not just a few seconds). However, the counter starts at power-on, and if the board is held in reset to be booted over USB, one actually risks hitting wrap-around during boot, which can both result in too short delays (if the "st += delay" calculation makes st small) and theoretically also unbound delays (if st ends up being UINT_MAX and one just misses sampling digctl_microseconds at that point). It doesn't take more code to DTRT, and once bitten, twice shy. Signed-off-by: Rasmus Villemoes --- arch/arm/cpu/arm926ejs/mxs/spl_boot.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_boot.c b/arch/arm/cpu/arm926ejs/mxs/spl_boot.c index cb361ac65c..336266fe82 100644 --- a/arch/arm/cpu/arm926ejs/mxs/spl_boot.c +++ b/arch/arm/cpu/arm926ejs/mxs/spl_boot.c @@ -24,9 +24,7 @@ static bd_t bdata __section(".data"); /* * This delay function is intended to be used only in early stage of boot, where - * clock are not set up yet. The timer used here is reset on every boot and - * takes a few seconds to roll. The boot doesn't take that long, so to keep the - * code simple, it doesn't take rolling into consideration. + * clock are not set up yet. */ void early_delay(int delay) { @@ -34,8 +32,7 @@ void early_delay(int delay) (struct mxs_digctl_regs *)MXS_DIGCTL_BASE; uint32_t st = readl(_regs->hw_digctl_microseconds); - st += delay; - while (st > readl(_regs->hw_digctl_microseconds)) + while (readl(_regs->hw_digctl_microseconds) - st <= delay) ; } -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RFC PATCH 0/3] collect entropy, populate /chosen/rng-seed
[This is very much an early Request For Comments - it builds and shows a sketch of what I have in mind, but has not really been tested. The rest of the cover letter is copied from patch 3.] A recurring theme on LKML is the boot process deadlocking due to some process blocking waiting for random numbers, while the kernel's Cryptographic Random Number Generator (crng) is not initalized yet, but that very blocking means no activity happens that would generate the entropy necessary to finalize seeding the crng. This is not a problem on boards that have a good hwrng (when the kernel is configured to trust it), whether in the CPU or in a TPM or elsewhere. However, that's far from all boards out there. Moreover, when booting with an initrd, all the "disk activity" that would otherwise generate some timing variances has already been done by U-boot. Hence it makes sense to try to collect that entropy in U-boot and pass it on to the kernel. On the kernel side, support for that has just landed in master (commit 428826f5358c "fdt: add support for rng-seed"). By itself, this does not help with the initialization of the crng, since the kernel only considers the rng-seed "trustworthy" if CONFIG_RANDOM_TRUST_BOOTLOADER is set (it is always fed into the crng, but entropy is only accounted when that config option is set). This adds some basic infrastructure for collecting entropy in U-boot, and then it's up to the BSP developer to decide if CONFIG_RANDOM_TRUST_BOOTLOADER should be enabled in the kernel. If this is accepted, I think we should add entropy() calls before and after most disk and network activities. Moreover, at least some boards seem to have a rather reliable source of randomness in the contents of RAM after a cold boot [*], so I imagine exposing the entropy_mix() either via a command "entropy " that can be run during a boot script, or perhaps to be done automatically (and as early as possible to reduce risk of "tainting") via some CONFIG_ENTROPY_RAM_{ADDR,LEN}. There's probably good sources of entropy to be had from the SPL phase, but I couldn't find a good way of passing that on other than by putting the sha256_context inside global_data, which would bloat that by over 100 bytes. [*] Looking at a slightly arbitrary place in the middle of physical memory I got these 40d07670: 34081000 400a8020 2040 20024400...4 ..@@ ...D. 40d07670: 343c1000 640a9120 00036040 6006e400..<4 ..d@`.` 40d07670: 353c1040 600a9120 00026041 6246e400@.<5 ..`A`Fb 40d07670: 34281000 600a9100 00026040 2046e400..(4...`@`F So some bits are always the same, but there's quite a few that flip randomly between boots - so mixing in a MB or two seems that it should provide plenty of real entropy. Rasmus Villemoes (3): u-boot/sha256.h: add SHA256_INIT macro u-boot/sha256.h: include linux/types.h add infrastructure for collecting entropy common/fdt_support.c| 12 include/common.h| 3 ++ include/entropy.h | 42 +++ include/u-boot/sha256.h | 13 + lib/Kconfig | 10 +++ lib/Makefile| 1 + lib/entropy.c | 63 + 7 files changed, 144 insertions(+) create mode 100644 include/entropy.h create mode 100644 lib/entropy.c -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RFC PATCH 2/3] u-boot/sha256.h: include linux/types.h
One cannot use sha256.h by itself - the includer must already have made sure that uint32_t and friends are defined; i.e., having included linux/types.h either directly or indirectly. That's a little annoying, so just make the header self-contained. Signed-off-by: Rasmus Villemoes --- include/u-boot/sha256.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/u-boot/sha256.h b/include/u-boot/sha256.h index 10f42091ee..9e16b9715e 100644 --- a/include/u-boot/sha256.h +++ b/include/u-boot/sha256.h @@ -1,6 +1,8 @@ #ifndef _SHA256_H #define _SHA256_H +#include + #define SHA256_SUM_LEN 32 #define SHA256_DER_LEN 19 -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RFC PATCH 1/3] u-boot/sha256.h: add SHA256_INIT macro
To be used for statically initializing a sha256 context. Signed-off-by: Rasmus Villemoes --- include/u-boot/sha256.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/include/u-boot/sha256.h b/include/u-boot/sha256.h index 9aa1251789..10f42091ee 100644 --- a/include/u-boot/sha256.h +++ b/include/u-boot/sha256.h @@ -22,4 +22,15 @@ void sha256_finish(sha256_context * ctx, uint8_t digest[SHA256_SUM_LEN]); void sha256_csum_wd(const unsigned char *input, unsigned int ilen, unsigned char *output, unsigned int chunk_sz); +#define SHA256_INIT { \ + .state[0] = 0x6A09E667, \ + .state[1] = 0xBB67AE85, \ + .state[2] = 0x3C6EF372, \ + .state[3] = 0xA54FF53A, \ + .state[4] = 0x510E527F, \ + .state[5] = 0x9B05688C, \ + .state[6] = 0x1F83D9AB, \ + .state[7] = 0x5BE0CD19, \ + } + #endif /* _SHA256_H */ -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RFC PATCH 3/3] add infrastructure for collecting entropy
A recurring theme on LKML is the boot process deadlocking due to some process blocking waiting for random numbers, while the kernel's Cryptographic Random Number Generator (crng) is not initalized yet, but that very blocking means no activity happens that would generate the entropy necessary to finalize seeding the crng. This is not a problem on boards that have a good hwrng (when the kernel is configured to trust it), whether in the CPU or in a TPM or elsewhere. However, that's far from all boards out there. Moreover, when booting with an initrd, all the "disk activity" that would otherwise generate some timing variances has already been done by U-boot. Hence it makes sense to try to collect that entropy in U-boot and pass it on to the kernel. On the kernel side, support for that has just landed in master (commit 428826f5358c "fdt: add support for rng-seed"). By itself, this does not help with the initialization of the crng, since the kernel only considers the rng-seed "trustworthy" if CONFIG_RANDOM_TRUST_BOOTLOADER is set (it is always fed into the crng, but entropy is only accounted when that config option is set). This adds some basic infrastructure for collecting entropy in U-boot, and then it's up to the BSP developer to decide if CONFIG_RANDOM_TRUST_BOOTLOADER should be enabled in the kernel. If this is accepted, I think we should add entropy() calls before and after most disk and network activities. Moreover, at least some boards seem to have a rather reliable source of randomness in the contents of RAM after a cold boot [*], so I imagine exposing the entropy_mix() either via a command "entropy " that can be run during a boot script, or perhaps to be done automatically (and as early as possible to reduce risk of "tainting") via some CONFIG_ENTROPY_RAM_{ADDR,LEN}. There's probably good sources of entropy to be had from the SPL phase, but I couldn't find a good way of passing that on other than by putting the sha256_context inside global_data, which would bloat that by over 100 bytes. [*] Looking at a slightly arbitrary place in the middle of physical memory I got these 40d07670: 34081000 400a8020 2040 20024400...4 ..@@ ...D. 40d07670: 343c1000 640a9120 00036040 6006e400..<4 ..d@`.` 40d07670: 353c1040 600a9120 00026041 6246e400@.<5 ..`A`Fb 40d07670: 34281000 600a9100 00026040 2046e400..(4...`@`F So some bits are always the same, but there's quite a few that flip randomly between boots - so mixing in a MB or two seems that it should provide plenty of real entropy. Signed-off-by: Rasmus Villemoes --- common/fdt_support.c | 12 + include/common.h | 3 +++ include/entropy.h| 42 + lib/Kconfig | 10 +++ lib/Makefile | 1 + lib/entropy.c| 63 6 files changed, 131 insertions(+) create mode 100644 include/entropy.h create mode 100644 lib/entropy.c diff --git a/common/fdt_support.c b/common/fdt_support.c index baf7924ff6..e071abaabb 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -274,6 +274,7 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end) int fdt_chosen(void *fdt) { + uint8_t digest[SHA256_SUM_LEN]; int nodeoffset; int err; char *str; /* used to set string properties */ @@ -299,6 +300,17 @@ int fdt_chosen(void *fdt) return err; } } + if (IS_ENABLED(CONFIG_ENTROPY)) { + entropy_digest(digest); + + err = fdt_setprop(fdt, nodeoffset, "rng-seed", digest, + sizeof(digest)); + if (err < 0) { + printf("WARNING: could not set rng-seed %s.\n", + fdt_strerror(err)); + return err; + } + } return fdt_fixup_stdout(fdt, nodeoffset); } diff --git a/include/common.h b/include/common.h index d8f302ea92..55df86b6e7 100644 --- a/include/common.h +++ b/include/common.h @@ -331,6 +331,9 @@ void srand(unsigned int seed); unsigned int rand(void); unsigned int rand_r(unsigned int *seedp); +/* lib/entropy.c */ +#include + /* * STDIO based functions (can always be used) */ diff --git a/include/entropy.h b/include/entropy.h new file mode 100644 index 00..47fa73a3a3 --- /dev/null +++ b/include/entropy.h @@ -0,0 +1,42 @@ +#ifndef __ENTROPY_H +#define __ENTROPY_H + +#include + +#if defined(CONFIG_ENTROPY) && !defined(CONFIG_SPL_BUILD) + +/* Overridable, should preferably include some timer with microsecond-or-better resolution. */ +uint64_t entropy_token(void); + +/* + * Call entropy_token() and mix in the return value - this is the main + * function for adding entropy from timing variances. Call it before + * and after any operation that may have so
Re: [U-Boot] [PATCH 0/4] arm: mxs: mxs_set_gpmiclk
On 12/09/2019 11.17, Rasmus Villemoes wrote: > While trying to implement an mxs_set_gpmiclk() I stumbled on a few minor > things. > > Rasmus Villemoes (4): > arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX > arm: mxs: fix comments in arch_cpu_init to match the code > arm: mxs: be more careful when enabling gpmi_clk > arm: mxs: implement mxs_set_gpmiclk ping ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[PATCH] mpc83xx: make ARCH_MPC8309 select SYS_FSL_ERRATUM_ESDHC111
The mpc8309 is also affected by the "Manual Asynchronous CMD12 abort operation causes protocol violations" erratum, though it is enumerated as eSDHC16 in the errata sheet for mpc8309. Signed-off-by: Rasmus Villemoes --- arch/powerpc/cpu/mpc83xx/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/cpu/mpc83xx/Kconfig b/arch/powerpc/cpu/mpc83xx/Kconfig index 72053ceab4..e9f6e93c21 100644 --- a/arch/powerpc/cpu/mpc83xx/Kconfig +++ b/arch/powerpc/cpu/mpc83xx/Kconfig @@ -220,6 +220,7 @@ config ARCH_MPC8309 select MPC83XX_QUICC_ENGINE select MPC83XX_PCI_SUPPORT select MPC83XX_SECOND_I2C_SUPPORT + select SYS_FSL_ERRATUM_ESDHC111 config ARCH_MPC831X bool -- 2.23.0
[PATCH] sysreset_mpc83xx: fix mcp83xx -> mpc83xx typo
Signed-off-by: Rasmus Villemoes --- configs/gazerbeam_defconfig | 2 +- drivers/sysreset/Kconfig| 2 +- drivers/sysreset/Makefile | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/configs/gazerbeam_defconfig b/configs/gazerbeam_defconfig index 3a29bb1277..2d690245f1 100644 --- a/configs/gazerbeam_defconfig +++ b/configs/gazerbeam_defconfig @@ -181,7 +181,7 @@ CONFIG_DM_RESET=y CONFIG_DM_SERIAL=y CONFIG_SYS_NS16550=y CONFIG_SYSRESET=y -CONFIG_SYSRESET_MCP83XX=y +CONFIG_SYSRESET_MPC83XX=y CONFIG_TIMER=y CONFIG_MPC83XX_TIMER=y CONFIG_TPM_ATMEL_TWI=y diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index f565ae0310..44ced450f1 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -107,7 +107,7 @@ config SYSRESET_X86 help Reboot support for generic x86 processor reset. -config SYSRESET_MCP83XX +config SYSRESET_MPC83XX bool "Enable support MPC83xx SoC family reboot driver" help Reboot support for NXP MPC83xx SoCs. diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile index cf01492295..1c5d38f43a 100644 --- a/drivers/sysreset/Makefile +++ b/drivers/sysreset/Makefile @@ -8,7 +8,7 @@ obj-$(CONFIG_ARCH_ROCKCHIP) += sysreset_rockchip.o obj-$(CONFIG_ARCH_STI) += sysreset_sti.o obj-$(CONFIG_SANDBOX) += sysreset_sandbox.o obj-$(CONFIG_SYSRESET_GPIO) += sysreset_gpio.o -obj-$(CONFIG_SYSRESET_MCP83XX) += sysreset_mpc83xx.o +obj-$(CONFIG_SYSRESET_MPC83XX) += sysreset_mpc83xx.o obj-$(CONFIG_SYSRESET_MICROBLAZE) += sysreset_microblaze.o obj-$(CONFIG_SYSRESET_PSCI) += sysreset_psci.o obj-$(CONFIG_SYSRESET_SOCFPGA) += sysreset_socfpga.o -- 2.23.0
commit "dm: spi: Avoid setting the speed with every transfer"
Hi I'm wondering how commit 60e2809a84 (dm: spi: Avoid setting the speed with every transfer) works. AFAIU, the currently selected speed is a property of the SPI master, so suppose we have two slaves, A which has max_hz = 10MHz and B which has max_hz = 20MHz. Now suppose we do transfers to A, then B, then A again. The third time we still compute speed = 10MHz, but since that matches what we cached in slave->speed, we don't call spi_set_speed_mode(), hence we proceed to do the xfer with a too large speed? I'm probably missing something obvious here since the commit is more than 4 years old. Thanks, Rasmus
[PATCH] env: another attempt at fixing SPL build failures
I'm also seeing the build failure that commit 7d4776545b env: solve compilation error in SPL tried to fix, namely that the reference to env_flags_validate from env_htab cannot be satisfied when flags.o is not built in. However, that commit got reverted by d90fc9c3de Revert "env: solve compilation error in SPL" Necessary, but not sufficient conditions to see this are CONFIG_SPL=y (obviously) CONFIG_SPL_ENV_SUPPORT=n (so flags.o does not get compiled) CONFIG_SPL_LIBCOMMON_SUPPORT=y (so env/built-in.o is part of the SPL link) Now, these are satisfied for e.g. imx6q_logic_defconfig. But that builds just fine, and spl/u-boot-spl.map lists .data.env_htab among the discarded (garbage collected) sections. Yet, on our mpc8309-derived board, we do see the build failure, so perhaps the linker works a bit differently on ppc than on ARM, or there's yet some other configuration option needed to observe the break. This is another attempt at solving it, which also cleans up env/Makefile a bit: Introduce a def_bool y symbol CONFIG_ENV_SUPPORT which complements CONFIG_(SPL/TPL)_SUPPORT. Then use CONFIG_$(SPL_TPL_)ENV_SUPPORT to decide whether to include the five basic env/*.o files. For attr.o, flags.o and callback.o, this shouldn't change anything. Also, common.o and env.o still get unconditionally built for U-boot proper. But for TPL/SPL, those two are only included if CONFIG_(SPL/TPL)_SUPPORT is set. Having that symbol should also allow simplifying conditionals such as #if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT) found in drivers/reset/reset-socfpga.c to just CONFIG_IS_ENABLED(ENV_SUPPORT). Signed-off-by: Rasmus Villemoes --- env/Kconfig | 3 +++ env/Makefile | 13 + 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/env/Kconfig b/env/Kconfig index ed12609f6a..4661082f0e 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -1,5 +1,8 @@ menu "Environment" +config ENV_SUPPORT + def_bool y + config ENV_IS_NOWHERE bool "Environment is not stored" default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \ diff --git a/env/Makefile b/env/Makefile index 90144d6caf..e2a165b8f1 100644 --- a/env/Makefile +++ b/env/Makefile @@ -3,12 +3,13 @@ # (C) Copyright 2004-2006 # Wolfgang Denk, DENX Software Engineering, w...@denx.de. -obj-y += common.o env.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += common.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o ifndef CONFIG_SPL_BUILD -obj-y += attr.o -obj-y += callback.o -obj-y += flags.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o @@ -19,10 +20,6 @@ obj-$(CONFIG_ENV_IS_IN_ONENAND) += onenand.o obj-$(CONFIG_ENV_IS_IN_SATA) += sata.o obj-$(CONFIG_ENV_IS_IN_REMOTE) += remote.o obj-$(CONFIG_ENV_IS_IN_UBI) += ubi.o -else -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o endif obj-$(CONFIG_$(SPL_TPL_)ENV_IS_NOWHERE) += nowhere.o -- 2.23.0
[PATCH] spl_fit.c: enable loading compressed u-boot from fit image
From: "Klaus H. Sorensen" Allow reading compressed content from fit image, even if CONFIG_SPL_OS_BOOT is not set. This allow booting compressed 2nd stage u-boot from fit image. Additionally, do not print warning message if compression node is not found, since it simply implies the content is uncompressed. Signed-off-by: Klaus H. Sorensen Signed-off-by: Rasmus Villemoes --- common/spl/spl_fit.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index b3e3ccd5a2..98271eb878 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -195,11 +195,9 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, debug("%s ", genimg_get_type_name(type)); } - if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_SPL_GZIP)) { - if (fit_image_get_comp(fit, node, _comp)) - puts("Cannot get image compression format.\n"); - else - debug("%s ", genimg_get_comp_name(image_comp)); + if (IS_ENABLED(CONFIG_SPL_GZIP)) { + fit_image_get_comp(fit, node, _comp); + debug("%s ", genimg_get_comp_name(image_comp)); } if (fit_image_get_load(fit, node, _addr)) -- 2.23.0
[PATCH] cmd/eeprom.c: prepend 0x to hex numbers in output message format
From: "Klaus H. Sorensen" If the numbers do not happen to contain any digits from [a-f], it's not clear that they are base 16. Signed-off-by: Klaus H. Sorensen Signed-off-by: Rasmus Villemoes --- cmd/eeprom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/eeprom.c b/cmd/eeprom.c index 19953df082..23d9a21ab6 100644 --- a/cmd/eeprom.c +++ b/cmd/eeprom.c @@ -307,7 +307,7 @@ static int eeprom_execute_command(enum eeprom_action action, int i2c_bus, { int rcode = 0; const char *const fmt = - "\nEEPROM @0x%lX %s: addr %08lx off %04lx count %ld ... "; + "\nEEPROM @0x%lX %s: addr 0x%08lx off 0x%04lx count %ld ... "; #ifdef CONFIG_CMD_EEPROM_LAYOUT struct eeprom_layout layout; #endif -- 2.23.0
[PATCH] mpc83xx: set MPC83XX_GPIO_CTRLRS to 2 for MPC8309
The MPC8309 has two gpio controllers (which is already correctly reflected in its struct immap definition). Signed-off-by: Rasmus Villemoes --- arch/powerpc/include/asm/arch-mpc83xx/gpio.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/include/asm/arch-mpc83xx/gpio.h b/arch/powerpc/include/asm/arch-mpc83xx/gpio.h index 385d651d20..8a6896e622 100644 --- a/arch/powerpc/include/asm/arch-mpc83xx/gpio.h +++ b/arch/powerpc/include/asm/arch-mpc83xx/gpio.h @@ -9,7 +9,8 @@ #if defined(CONFIG_ARCH_MPC8313) || defined(CONFIG_ARCH_MPC8308) || \ defined(CONFIG_ARCH_MPC8315) #define MPC83XX_GPIO_CTRLRS 1 -#elif defined(CONFIG_ARCH_MPC834X) || defined(CONFIG_ARCH_MPC837X) +#elif defined(CONFIG_ARCH_MPC834X) || defined(CONFIG_ARCH_MPC837X) || \ + defined(CONFIG_ARCH_MPC8309) #define MPC83XX_GPIO_CTRLRS 2 #else #define MPC83XX_GPIO_CTRLRS 0 -- 2.23.0
[PATCH] mpc83xx: immap_83xx: add spi8xxx_t in immap for mpc8309
Allow drivers/spi/mpc8xxx_spi.c to be built for an mpc8309 target. Signed-off-by: Rasmus Villemoes --- arch/powerpc/include/asm/immap_83xx.h | 3 +-- arch/powerpc/include/asm/mpc8xxx_spi.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/include/asm/immap_83xx.h b/arch/powerpc/include/asm/immap_83xx.h index d02da6495c..609869c715 100644 --- a/arch/powerpc/include/asm/immap_83xx.h +++ b/arch/powerpc/include/asm/immap_83xx.h @@ -941,8 +941,7 @@ typedef struct immap { u8 res4[0x500]; fsl_lbc_t im_lbc; /* Local Bus Controller Regs */ u8 res5[0x1000]; - u8 spi[0x100]; - u8 res6[0xf00]; + spi8xxx_t spi;/* Serial Peripheral Interface */ dma83xx_t dma;/* DMA */ pciconf83xx_t pci_conf[1];/* PCI Configuration Registers */ u8 res7[0x80]; diff --git a/arch/powerpc/include/asm/mpc8xxx_spi.h b/arch/powerpc/include/asm/mpc8xxx_spi.h index b583a3269d..470ee955f3 100644 --- a/arch/powerpc/include/asm/mpc8xxx_spi.h +++ b/arch/powerpc/include/asm/mpc8xxx_spi.h @@ -11,6 +11,7 @@ #include #if defined(CONFIG_ARCH_MPC8308) || \ + defined(CONFIG_ARCH_MPC8309) || \ defined(CONFIG_ARCH_MPC8313) || \ defined(CONFIG_ARCH_MPC8315) || \ defined(CONFIG_ARCH_MPC834X) || \ -- 2.23.0
[PATCH] powerpc: mpc83xx: convert CONFIG_FSL_ELBC to Kconfig
This complements commit 068789773d0 which did the conversion for mpc85xx. Signed-off-by: Rasmus Villemoes --- arch/powerpc/cpu/mpc83xx/Kconfig | 7 +++ include/configs/MPC8313ERDB_NAND.h | 1 - include/configs/MPC8313ERDB_NOR.h | 1 - include/configs/MPC8315ERDB.h | 2 -- include/configs/MPC837XEMDS.h | 2 -- include/configs/MPC837XERDB.h | 2 -- include/configs/ids8313.h | 2 -- include/configs/ve8313.h | 1 - 8 files changed, 7 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/cpu/mpc83xx/Kconfig b/arch/powerpc/cpu/mpc83xx/Kconfig index e9f6e93c21..5d7650294d 100644 --- a/arch/powerpc/cpu/mpc83xx/Kconfig +++ b/arch/powerpc/cpu/mpc83xx/Kconfig @@ -221,6 +221,7 @@ config ARCH_MPC8309 select MPC83XX_PCI_SUPPORT select MPC83XX_SECOND_I2C_SUPPORT select SYS_FSL_ERRATUM_ESDHC111 + select FSL_ELBC config ARCH_MPC831X bool @@ -232,6 +233,7 @@ config ARCH_MPC8313 bool select ARCH_MPC831X select MPC83XX_SECOND_I2C_SUPPORT + select FSL_ELBC config ARCH_MPC8315 bool @@ -239,6 +241,7 @@ config ARCH_MPC8315 select MPC83XX_PCIE1_SUPPORT select MPC83XX_PCIE2_SUPPORT select MPC83XX_SATA_SUPPORT + select FSL_ELBC config ARCH_MPC832X bool @@ -275,6 +278,7 @@ config ARCH_MPC837X select MPC83XX_SATA_SUPPORT select MPC83XX_LDP_PIN select MPC83XX_SECOND_I2C_SUPPORT + select FSL_ELBC config SYS_IMMR hex "Value for IMMR" @@ -318,6 +322,9 @@ endif endmenu +config FSL_ELBC + bool + source "board/esd/vme8349/Kconfig" source "board/freescale/mpc8308rdb/Kconfig" source "board/freescale/mpc8313erdb/Kconfig" diff --git a/include/configs/MPC8313ERDB_NAND.h b/include/configs/MPC8313ERDB_NAND.h index 4153d609be..569b59008c 100644 --- a/include/configs/MPC8313ERDB_NAND.h +++ b/include/configs/MPC8313ERDB_NAND.h @@ -43,7 +43,6 @@ #endif #define CONFIG_PCI_INDIRECT_BRIDGE -#define CONFIG_FSL_ELBC 1 /* * On-board devices diff --git a/include/configs/MPC8313ERDB_NOR.h b/include/configs/MPC8313ERDB_NOR.h index ff8dedf03e..274653df29 100644 --- a/include/configs/MPC8313ERDB_NOR.h +++ b/include/configs/MPC8313ERDB_NOR.h @@ -19,7 +19,6 @@ #endif #define CONFIG_PCI_INDIRECT_BRIDGE -#define CONFIG_FSL_ELBC 1 /* * On-board devices diff --git a/include/configs/MPC8315ERDB.h b/include/configs/MPC8315ERDB.h index 521c5ca6ee..e60a6a7d7a 100644 --- a/include/configs/MPC8315ERDB.h +++ b/include/configs/MPC8315ERDB.h @@ -116,8 +116,6 @@ #define CONFIG_SYS_GBL_DATA_OFFSET \ (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) -#define CONFIG_FSL_ELBC - /* * FLASH on the Local Bus */ diff --git a/include/configs/MPC837XEMDS.h b/include/configs/MPC837XEMDS.h index 724f8afb76..36bf4b18f2 100644 --- a/include/configs/MPC837XEMDS.h +++ b/include/configs/MPC837XEMDS.h @@ -134,8 +134,6 @@ #define CONFIG_SYS_GBL_DATA_OFFSET \ (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) -#define CONFIG_FSL_ELBC1 - /* * FLASH on the Local Bus */ diff --git a/include/configs/MPC837XERDB.h b/include/configs/MPC837XERDB.h index 37f51ba743..31ecc2ff3f 100644 --- a/include/configs/MPC837XERDB.h +++ b/include/configs/MPC837XERDB.h @@ -158,8 +158,6 @@ #define CONFIG_SYS_GBL_DATA_OFFSET \ (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) -#define CONFIG_FSL_ELBC1 - /* * FLASH on the Local Bus */ diff --git a/include/configs/ids8313.h b/include/configs/ids8313.h index 43cb14c14e..e1d271cc33 100644 --- a/include/configs/ids8313.h +++ b/include/configs/ids8313.h @@ -14,8 +14,6 @@ /* * High Level Configuration Options */ -#define CONFIG_FSL_ELBC - #define CONFIG_BOOT_RETRY_TIME 900 #define CONFIG_BOOT_RETRY_MIN 30 #define CONFIG_RESET_TO_RETRY diff --git a/include/configs/ve8313.h b/include/configs/ve8313.h index 66f771d818..67793c17ae 100644 --- a/include/configs/ve8313.h +++ b/include/configs/ve8313.h @@ -18,7 +18,6 @@ #define CONFIG_E3001 #define CONFIG_PCI_INDIRECT_BRIDGE 1 -#define CONFIG_FSL_ELBC1 /* * On-board devices -- 2.23.0
[PATCH] doc: really get rid of Documentation/ directory
Commit 656d8da9d2 (doc: Remove duplicated documentation directory) got rid of most of Documentation/. But there's still an obviously useless .gitignore left behind. Also, there's a copy of the linux kernel's net/ethernet.txt binding imported from v5.0, while the existing one in doc/ is from 4.0-rc1. So replace the latter by the former, and making Documentation/ finally empty. Signed-off-by: Rasmus Villemoes --- Documentation/.gitignore | 1 - .../devicetree/bindings/net/ethernet.txt | 66 --- doc/device-tree-bindings/net/ethernet.txt | 55 ++-- 3 files changed, 48 insertions(+), 74 deletions(-) delete mode 100644 Documentation/.gitignore delete mode 100644 Documentation/devicetree/bindings/net/ethernet.txt diff --git a/Documentation/.gitignore b/Documentation/.gitignore deleted file mode 100644 index 0d20b6487c..00 --- a/Documentation/.gitignore +++ /dev/null @@ -1 +0,0 @@ -*.pyc diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt deleted file mode 100644 index cfc376bc97..00 --- a/Documentation/devicetree/bindings/net/ethernet.txt +++ /dev/null @@ -1,66 +0,0 @@ -The following properties are common to the Ethernet controllers: - -NOTE: All 'phy*' properties documented below are Ethernet specific. For the -generic PHY 'phys' property, see -Documentation/devicetree/bindings/phy/phy-bindings.txt. - -- local-mac-address: array of 6 bytes, specifies the MAC address that was - assigned to the network device; -- mac-address: array of 6 bytes, specifies the MAC address that was last used by - the boot program; should be used in cases where the MAC address assigned to - the device by the boot program is different from the "local-mac-address" - property; -- nvmem-cells: phandle, reference to an nvmem node for the MAC address; -- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used; -- max-speed: number, specifies maximum speed in Mbit/s supported by the device; -- max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than - the maximum frame size (there's contradiction in the Devicetree - Specification). -- phy-mode: string, operation mode of the PHY interface. This is now a de-facto - standard property; supported values are: - * "internal" - * "mii" - * "gmii" - * "sgmii" - * "qsgmii" - * "tbi" - * "rev-mii" - * "rmii" - * "rgmii" (RX and TX delays are added by the MAC when required) - * "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the - MAC should not add the RX or TX delays in this case) - * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC - should not add an RX delay in this case) - * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC - should not add an TX delay in this case) - * "rtbi" - * "smii" - * "xgmii" - * "trgmii" - * "2000base-x", - * "2500base-x", - * "rxaui" - * "xaui" - * "10gbase-kr" (10GBASE-KR, XFI, SFI) -- phy-connection-type: the same as "phy-mode" property but described in the - Devicetree Specification; -- phy-handle: phandle, specifies a reference to a node representing a PHY - device; this property is described in the Devicetree Specification and so - preferred; -- phy: the same as "phy-handle" property, not recommended for new bindings. -- phy-device: the same as "phy-handle" property, not recommended for new - bindings. -- rx-fifo-depth: the size of the controller's receive fifo in bytes. This - is used for components that can have configurable receive fifo sizes, - and is useful for determining certain configuration settings such as - flow control thresholds. -- tx-fifo-depth: the size of the controller's transmit fifo in bytes. This - is used for components that can have configurable fifo sizes. -- managed: string, specifies the PHY management type. Supported values are: - "auto", "in-band-status". "auto" is the default, it usess MDIO for - management if fixed-link is not specified. - -Child nodes of the Ethernet controller are typically the individual PHY devices -connected via the MDIO bus (sometimes the MDIO bus controller is separate). -They are described in the phy.txt file in this same directory. -For non-MDIO PHY management see fixed-link.txt. diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt index 3fc360523b..cfc376bc97 100644 --- a/doc/device-tree-bindings/net/ethernet.txt +++ b/doc/device-tree-bindings/net/ethernet.txt @@ -1,25 +1,66 @@ The following properties are common to the Ethernet controllers: +NOTE: All 'phy*' properties docu
[PATCH] mpc83xx_clk: always treat MPC83XX_CLK_PCI as invalid
The current mpc83xx_clk driver is broken for any board for which mpc83xx_has_pci() is true, i.e. anything not MPC8308: When is_clk_valid() reports that MPC83XX_CLK_PCI is valid, init_all_clks() proceeds to call init_single_clk(), but that doesn't know about either MPC83XX_CLK_PCI or has any handling of the TYPE_SCCR_ONOFF mode correctly returned by retrieve_mode(). Hence init_single_clk() ends up returning -EINVAL, and the whole board hangs in serial_init(). The quickest fix is to simply pretend that clock is invalid for all, since nobody can have been relying on it. Adding proper support seems to be a bit more involved than just handling TYPE_SCCR_ONOFF: - The power-on-reset value of SCCR[PCICM] is 0, so mpc83xx_clk_enable() would probably need to be tought to enable the clock. - The frequency of PCI_SYNC_OUT is either SYS_CLK_IN or SYS_CLK_IN/2 depending on the CFG_CLKIN_DIV configuration input, but that can't be read from software, so to properly fill out ->speed[MPC83XX_CLK_PCI] I think one would need guidance from Kconfig or dtb. Partially fixes: 07d538d281 clk: Add MPC83xx clock driver Signed-off-by: Rasmus Villemoes --- drivers/clk/mpc83xx_clk.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/clk/mpc83xx_clk.c b/drivers/clk/mpc83xx_clk.c index 32d2db9eda..1b2b216596 100644 --- a/drivers/clk/mpc83xx_clk.c +++ b/drivers/clk/mpc83xx_clk.c @@ -65,7 +65,10 @@ static inline bool is_clk_valid(struct udevice *clk, int id) case MPC83XX_CLK_DMAC: return (type == SOC_MPC8308) || (type == SOC_MPC8309); case MPC83XX_CLK_PCI: - return mpc83xx_has_pci(type); + /* +* FIXME: implement proper support for this. +*/ + return 0 && mpc83xx_has_pci(type); case MPC83XX_CLK_CSB: return true; case MPC83XX_CLK_I2C2: -- 2.23.0
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 12/02/2020 12.38, Wolfgang Denk wrote: > Dear Rasmus, > > In message you wrote: >> >>> HUSH does not support arrays anyway... >> >> Of course not, but they can be emulated by having variables foo0, foo1, >> foo2 and programmatically accessing the variable foo$index, if only >> there's a way to do that... In a sense, my BOOT_A_LEFT/BOOT_B_LEFT is >> also just an array with keys "A" and "B". > > Actually the port to U-Boot cripples HUSH in many more aspects. > I've always hoped someone would some day volunteer and (1)( update > HUSH to a more recent (and less buggy) version and address a few of > the missing parts, like Command Substitution, which would be really > handy in many cases - here as well. I'm certainly also missing break (and to a lesser extent continue) in loops. >>> Well, there _are_ other ways... >> >> Please do tell. How can I avoid code duplication and access a variable >> whose name I generate by string concatenation/variable interpolation? >> I.e., I don't want anything like "if test $slot = "A" ; then setenv >> result BOOT_A_LEFT ; elif test $slot = "B" ; then setenv result >> BOOT_B_LEFT ; fi", because that doesn't scale. > > => slot=A > => setenv result BOOT_${slot}_LEFT > => printenv result > result=BOOT_A_LEFT > => setenv foo 'setenv result BOOT_${slot}_LEFT; printenv result' > => slot=B > => run foo > result=BOOT_B_LEFT > => slot=X > => run foo > result=BOOT_X_LEFT > > What exactly is your question? I'm sorry, I see I mistyped in my example above, it should have been if test $slot = "A" ; setenv result $BOOT_A_LEFT ... as should hopefully be clear from the original post and the eval examples. So to reiterate, the problem is to get the contents (or value, if you will) of the BOOT_A_LEFT variable into the result variable, not setting result to the string "BOOT_A_LEFT" - but with the wrinkle that BOOT_A_LEFT is generated programmatically, so the code cannot literally mention BOOT_A_LEFT anywhere. >> env set -E result "\${BOOT_${x}_LEFT}" >> >> corresponds to >> >> eval "result=\${BOOT_${x}_LEFT}" > > ...and things become already very tricky here, as you can see from > the need of having "\$" and "$" mixed. Now assume you have more of > this in the embedded variables... Eh, yes, exactly as one needs to do in ordinary shells when using the eval construct. I don't see how one can on the one hand trust programmers to have arbitrary read and write access to all of physical memory but not trust them to get such rather basic escaping right (and testing should immediately show if one got it wrong). I also proposed an escape-less solution, namely "env get". That would be slightly less powerful, since env get dest whatever could be implemented in terms of "env set -E" as env set -E dest "\${whatever}" but as I said, I think that would be sufficient for my purposes. So just as print[env] takes the name of a variable and shows the name=value string, and one can thus say "printenv BOOT_${slot}_LEFT" as you did in your extended example, I could do env get result BOOT_${slot}_LEFT and get the value of the BOOT_${slot}_LEFT variable into result. Would you be ok with adding such an "env get" with less foot-gun potential? Rasmus
caching BLOBLISTT_SPL_HANDOFF (was Re: [PATCH] common/board_f.c: use #ifdefs a little more consistently)
On 28/02/2020 18.35, Tom Rini wrote: > On Fri, Feb 28, 2020 at 05:24:58PM +0000, Rasmus Villemoes wrote: >> eliminated, and there's not an #ifdef in sight. > > That sounds pretty nice actually. If you're so inclined I'd like to see > it. > So I started looking at that, and while it's mostly mechanical, one quickly hits the case I was worried about, some of the functions referring to symbols or struct members that are conditionally defined/declared, so there's no way around guarding such accesses at the preprocessor level. Case in point: I'd like to do --- a/common/board_f.c +++ b/common/board_f.c @@ -287,11 +287,9 @@ static int setup_mon_len(void) static int setup_spl_handoff(void) { -#if CONFIG_IS_ENABLED(HANDOFF) gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF, sizeof(struct spl_handoff)); debug("Found SPL hand-off info %p\n", gd->spl_handoff); -#endif return 0; } @@ -886,7 +884,7 @@ static const init_fnc_t init_sequence_f[] = { #ifdef CONFIG_BLOBLIST bloblist_init, #endif - setup_spl_handoff, + CONFIG_IS_ENABLED(HANDOFF) ? setup_spl_handoff : NULL, initf_console_record, #if defined(CONFIG_HAVE_FSP) arch_fsp_init, but that doesn't work because gd->spl_handoff only exists when CONFIG_IS_ENABLED(BLOBLIST) && defined(CONFIG_SPL). Now that particular one seems a bit fishy: Why is it ok to cache the location of the BLOBLISTT_SPL_HANDOFF blob in gd->spl_handoff? Later in the init sequence there's a call to reserve_bloblist, and later again reloc_bloblist. Doesn't that leave gd->spl_handoff stale? I'd expect that users would always have to look it up via bloblist_find(). Simon? Rasmus
[PATCH] common/board_f.c: use #ifdefs a little more consistently
Some init functions, e.g. print_resetinfo(), are conditionally defined depending on some config options, and are correspondingly conditionally included in the init_sequence_f[] array. Others are unconditionally defined and included in init_sequence_f[], but have their entire body, sans a mandatory "return 0", conditionally compiled. For the simple cases, switch to the former model, making it a bit more consistent. This also makes the U-Boot image very slightly smaller and avoids a few useless calls to no-op functions during board_init_f. Signed-off-by: Rasmus Villemoes --- common/board_f.c | 54 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/common/board_f.c b/common/board_f.c index 82a164752a..ed63fbcea2 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -285,16 +285,16 @@ static int setup_mon_len(void) return 0; } +#if CONFIG_IS_ENABLED(HANDOFF) static int setup_spl_handoff(void) { -#if CONFIG_IS_ENABLED(HANDOFF) gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF, sizeof(struct spl_handoff)); debug("Found SPL hand-off info %p\n", gd->spl_handoff); -#endif return 0; } +#endif __weak int arch_cpu_init(void) { @@ -437,17 +437,17 @@ static int reserve_video(void) return 0; } +#ifdef CONFIG_TRACE static int reserve_trace(void) { -#ifdef CONFIG_TRACE gd->relocaddr -= CONFIG_TRACE_BUFFER_SIZE; gd->trace_buff = map_sysmem(gd->relocaddr, CONFIG_TRACE_BUFFER_SIZE); debug("Reserving %luk for trace data at: %08lx\n", (unsigned long)CONFIG_TRACE_BUFFER_SIZE >> 10, gd->relocaddr); -#endif return 0; } +#endif static int reserve_uboot(void) { @@ -520,13 +520,13 @@ static int reserve_board(void) return 0; } +#ifdef CONFIG_MACH_TYPE static int setup_machine(void) { -#ifdef CONFIG_MACH_TYPE gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ -#endif return 0; } +#endif static int reserve_global_data(void) { @@ -537,9 +537,9 @@ static int reserve_global_data(void) return 0; } +#ifndef CONFIG_OF_EMBED static int reserve_fdt(void) { -#ifndef CONFIG_OF_EMBED /* * If the device tree is sitting immediately above our image then we * must relocate it. If it is embedded in the data section, then it @@ -553,24 +553,24 @@ static int reserve_fdt(void) debug("Reserving %lu Bytes for FDT at: %08lx\n", gd->fdt_size, gd->start_addr_sp); } -#endif return 0; } +#endif +#ifdef CONFIG_BOOTSTAGE static int reserve_bootstage(void) { -#ifdef CONFIG_BOOTSTAGE int size = bootstage_get_size(); gd->start_addr_sp -= size; gd->new_bootstage = map_sysmem(gd->start_addr_sp, size); debug("Reserving %#x Bytes for bootstage at: %08lx\n", size, gd->start_addr_sp); -#endif return 0; } +#endif __weak int arch_reserve_stacks(void) { @@ -590,16 +590,16 @@ static int reserve_stacks(void) return arch_reserve_stacks(); } +#ifdef CONFIG_BLOBLIST static int reserve_bloblist(void) { -#ifdef CONFIG_BLOBLIST gd->start_addr_sp &= ~0xf; gd->start_addr_sp -= CONFIG_BLOBLIST_SIZE; gd->new_bloblist = map_sysmem(gd->start_addr_sp, CONFIG_BLOBLIST_SIZE); -#endif return 0; } +#endif static int display_new_sp(void) { @@ -675,23 +675,23 @@ static int init_post(void) } #endif +#ifndef CONFIG_OF_EMBED static int reloc_fdt(void) { -#ifndef CONFIG_OF_EMBED if (gd->flags & GD_FLG_SKIP_RELOC) return 0; if (gd->new_fdt) { memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size); gd->fdt_blob = gd->new_fdt; } -#endif return 0; } +#endif +#ifdef CONFIG_BOOTSTAGE static int reloc_bootstage(void) { -#ifdef CONFIG_BOOTSTAGE if (gd->flags & GD_FLG_SKIP_RELOC) return 0; if (gd->new_bootstage) { @@ -703,14 +703,14 @@ static int reloc_bootstage(void) gd->bootstage = gd->new_bootstage; bootstage_relocate(); } -#endif return 0; } +#endif +#ifdef CONFIG_BLOBLIST static int reloc_bloblist(void) { -#ifdef CONFIG_BLOBLIST if (gd->flags & GD_FLG_SKIP_RELOC) return 0; if (gd->new_bloblist) { @@ -721,10 +721,10 @@ static int reloc_bloblist(void) memcpy(gd->new_bloblist, gd->bloblist, size); gd->bloblist = gd->new_bloblist; } -#endif return 0; } +#endif static int setup_reloc(void) { @@ -886,7 +886,9 @@ static const init_fnc_t init_sequence_f[] = { #ifdef CONFIG_BLOBLIST blobli
Re: [PATCH] env: introduce CONFIG_ENV_DOTVARS_TEMPORARY
On 20/01/2020 17.44, Wolfgang Denk wrote: > Dear Rasmus Villemoes, > > In message <20200108134247.31443-1-rasmus.villem...@prevas.dk> you wrote: >> The printenv command already by default hides variables beginning with >> a dot. It can be useful to take that convention even further, and >> prevent such variables from ever being stored persistently (and >> ignored if they happen to exist in stable storage). >> >> This way, one can freely use such variable names in script logic, >> without worrying about random temporary variables leaking to >> persistent storage and/or to/from another U-boot "session". > > This is not a good idea. The decision whether a variable shall be > stored permanently or not, or wheter it is readonly or writable, and > other such properties should never based on their name. Sorry, but what other property of the variable could possibly determine those things, then? There may > be many good reasons that some .name variable _shall_ be persistent. Sure, absolutely. Which is why this is entirely opt-in for those who know they won't need that, but do have some semi-complicated script that interacts with various commands that return their output via an environment variable. > Naked-by: Wolfgang Denk Ah, now I see how env_flags_varaccess is actually implemented, involving a .flags special variable. OK, then I can certainly see why one would not want that to be excluded from the environment - I just thought the idea behind "printenv" hiding dot-variables by default was that those were considered temporary, and not special in this way. So, would you accept introducing env_flags_varaccess_temporary, for which I could then add tmp_.*:st ? Thanks, Rasmus
Re: [PATCH resend 0/2] gpio: mpc8xxx: honour shadow register when writing gpdat
On 28/01/2020 13.04, Rasmus Villemoes wrote: > [resending with Mario's correct address, sorry for the double post] > > The driver correctly uses the shadow register when asked for the > current value of an output gpio. Unfortunately, it does RMW on the > gpdat register both when setting a gpio as input and output. These two > patches fix that. Ping. Any chance these fixes can make it to 2020.04 ? Rasmus
[PATCH 4/4] make env_entry::callback conditional on !CONFIG_SPL_BUILD
The callback member of struct env_entry is always NULL for an SPL build. Removing it thus saves a bit of run-time memory in the SPL (when CONFIG_SPL_ENV_SUPPORT=y) since struct env_entry is embedded in struct env_entry_node - i.e. about 2KB for the normal case of 512+change hash table entries. Two small fixups are needed for this, all other references to the callback member are already under !CONFIG_SPL_BUILD: Don't initialize .callback in set_flags() - hsearch_r doesn't use that value anyway. And make env_callback_init() initialize ->callback to NULL for a new entry instead of relying on an unused or deleted entry having NULL in ->callback. Signed-off-by: Rasmus Villemoes --- env/callback.c | 2 ++ env/flags.c | 1 - include/search.h | 2 ++ lib/hashtable.c | 1 - 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/env/callback.c b/env/callback.c index f0904cfdc5..4054b9ef58 100644 --- a/env/callback.c +++ b/env/callback.c @@ -55,6 +55,8 @@ void env_callback_init(struct env_entry *var_entry) first_call = 0; } + var_entry->callback = NULL; + /* look in the ".callbacks" var for a reference to this variable */ if (callback_list != NULL) ret = env_attr_lookup(callback_list, var_name, callback_name); diff --git a/env/flags.c b/env/flags.c index 418d6cc742..b88fe7ba9c 100644 --- a/env/flags.c +++ b/env/flags.c @@ -457,7 +457,6 @@ static int set_flags(const char *name, const char *value, void *priv) e.key = name; e.data = NULL; - e.callback = NULL; hsearch_r(e, ENV_FIND, , _htab, 0); /* does the env variable actually exist? */ diff --git a/include/search.h b/include/search.h index 0469a852e0..8f87dc72ce 100644 --- a/include/search.h +++ b/include/search.h @@ -29,8 +29,10 @@ enum env_action { struct env_entry { const char *key; char *data; +#ifndef CONFIG_SPL_BUILD int (*callback)(const char *name, const char *value, enum env_op op, int flags); +#endif int flags; }; diff --git a/lib/hashtable.c b/lib/hashtable.c index c4e1e2bd45..f82f2463cf 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -450,7 +450,6 @@ static void _hdelete(const char *key, struct hsearch_data *htab, debug("hdelete: DELETING key \"%s\"\n", key); free((void *)ep->key); free(ep->data); - ep->callback = NULL; ep->flags = 0; htab->table[idx].used = USED_DELETED; -- 2.23.0
[PATCH 3/4] lib/hashtable.c: don't test ->callback in SPL
In SPL, environment callbacks are not supported, so e->callback is always NULL. Removing this makes the SPL a little smaller (about 400 bytes in my ppc build) with no functional change. Signed-off-by: Rasmus Villemoes --- lib/hashtable.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/hashtable.c b/lib/hashtable.c index 574ec6af86..c4e1e2bd45 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -226,8 +226,10 @@ static int do_callback(const struct env_entry *e, const char *name, const char *value, enum env_op op, int flags) { +#ifndef CONFIG_SPL_BUILD if (e->callback) return e->callback(name, value, op, flags); +#endif return 0; } -- 2.23.0
[PATCH 2/4] lib/hashtable.c: create helper for calling env_entry::callback
This is preparation for compiling out the "call the callback" code and associated error handling for SPL, where ->callback is always NULL. Signed-off-by: Rasmus Villemoes --- lib/hashtable.c | 23 +++ 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/hashtable.c b/lib/hashtable.c index 907e8a642f..574ec6af86 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -222,6 +222,15 @@ int hmatch_r(const char *match, int last_idx, struct env_entry **retval, return 0; } +static int +do_callback(const struct env_entry *e, const char *name, const char *value, + enum env_op op, int flags) +{ + if (e->callback) + return e->callback(name, value, op, flags); + return 0; +} + /* * Compare an existing entry with the desired key, and overwrite if the action * is ENV_ENTER. This is simply a helper function for hsearch_r(). @@ -247,9 +256,8 @@ static inline int _compare_and_overwrite_entry(struct env_entry item, } /* If there is a callback, call it */ - if (htab->table[idx].entry.callback && - htab->table[idx].entry.callback(item.key, - item.data, env_op_overwrite, flag)) { + if (do_callback(>table[idx].entry, item.key, + item.data, env_op_overwrite, flag)) { debug("callback() rejected setting variable " "%s, skipping it!\n", item.key); __set_errno(EINVAL); @@ -402,9 +410,8 @@ int hsearch_r(struct env_entry item, enum env_action action, } /* If there is a callback, call it */ - if (htab->table[idx].entry.callback && - htab->table[idx].entry.callback(item.key, item.data, - env_op_create, flag)) { + if (do_callback(>table[idx].entry, item.key, item.data, + env_op_create, flag)) { debug("callback() rejected setting variable " "%s, skipping it!\n", item.key); _hdelete(item.key, htab, >table[idx].entry, idx); @@ -473,8 +480,8 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag) } /* If there is a callback, call it */ - if (htab->table[idx].entry.callback && - htab->table[idx].entry.callback(key, NULL, env_op_delete, flag)) { + if (do_callback(>table[idx].entry, key, NULL, + env_op_delete, flag)) { debug("callback() rejected deleting variable " "%s, skipping it!\n", key); __set_errno(EINVAL); -- 2.23.0
[PATCH 0/4] remove (more) env callback code for SPL
The actual environment callbacks are automatically dropped from an SPL build, but the support code (env/callback.o) for associating a callback to an environment variable is still there. These patches reduce a CONFIG_SPL_ENV_SUPPORT=y SPL image by about 1K (at least for my ppc build), and another ~2K of run-time memory. === Aside: While poking around in this code, I noticed a few somewhat related bugs in callback.o: (1) The caching of env_get(".callbacks") in env_callback_init() is dubious at best: It gets called during the initial import, so env_get ends up calling env_get_f. So either we cache a value of NULL (no .callbacks variable in initial environment), or we cache a value of gd->env_buf. Now, of course env_get_f will soon no longer be used, so there's not a big risk that gd->env_buf will be overwritten, but it's rather fragile. In either case (2), the cache is not invalidated or updated by the on_callbacks() callback associated to .callbacks itself. Finally, (3) with CONFIG_REGEX=y, that on_callbacks() function has the unfortunate effect of removing itself as the callback associated to .callbacks: When .callbacks is initially added to the environment, env_callback_init() correctly associates on_callbacks, because it uses env_attr_lookup() (which is regex-aware) on the ENV_CALLBACK_STATIC_LIST. Now, when .callbacks is set the next time, on_callbacks() is called, starts by clearing all callbacks, including the one for .callbacks. It then tries to initialize them again, but it uses env_attr_walk() (which roughly speaking just splits the given string on , and : and calls back to the user's handler) on ENV_CALLBACK_STATIC_LIST, which means that set_callback() gets called with the string "\.callbacks" - and such a variable does not exist. set_callback() is never called with ".callbacks" as name, so .callbacks (and e.g. eth5addr - anything else which is supposed to be matched by a regex) now no longer has a callback. Perhaps this can all be fixed by just having on_callbacks update the cached static callback_list with value, then do a hwalk_r(_htab, env_callback_init) - no need for either set_callback or clear_callback. Rasmus Villemoes (4): env: remove callback.o for an SPL build lib/hashtable.c: create helper for calling env_entry::callback lib/hashtable.c: don't test ->callback in SPL make env_entry::callback conditional on !CONFIG_SPL_BUILD env/Makefile | 2 +- env/callback.c | 2 ++ env/flags.c| 1 - include/env_callback.h | 6 ++ include/search.h | 2 ++ lib/hashtable.c| 26 +- 6 files changed, 28 insertions(+), 11 deletions(-) -- 2.23.0
[PATCH 1/4] env: remove callback.o for an SPL build
env.h says this about about callback declarations (U_BOOT_ENV_CALLBACK): * For SPL these are silently dropped to reduce code size, since environment * callbacks are not supported with SPL. So env_callback_init() does a lot of work to not find anything in the guaranteed empty env_clbk list. Drop callback.o entirely from the link and stub out the only public function defined in callback.o. This cuts about 600 bytes from the SPL on my ppc build. Signed-off-by: Rasmus Villemoes --- env/Makefile | 2 +- include/env_callback.h | 6 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/env/Makefile b/env/Makefile index e2a165b8f1..c4ad654328 100644 --- a/env/Makefile +++ b/env/Makefile @@ -7,9 +7,9 @@ obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += common.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o ifndef CONFIG_SPL_BUILD +obj-y += callback.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o diff --git a/include/env_callback.h b/include/env_callback.h index 74da20eec3..05e9516a0f 100644 --- a/include/env_callback.h +++ b/include/env_callback.h @@ -72,6 +72,12 @@ "serial#:serialno," \ CONFIG_ENV_CALLBACK_LIST_STATIC +#ifndef CONFIG_SPL_BUILD void env_callback_init(struct env_entry *var_entry); +#else +static inline void env_callback_init(struct env_entry *var_entry) +{ +} +#endif #endif /* __ENV_CALLBACK_H__ */ -- 2.23.0
Re: [PATCH] common/board_f.c: use #ifdefs a little more consistently
On 28/02/2020 00.40, Simon Glass wrote: > Hi Rasmus, > > On Thu, 27 Feb 2020 at 00:18, Rasmus Villemoes > wrote: >> >> Some init functions, e.g. print_resetinfo(), are conditionally defined >> depending on some config options, and are correspondingly >> conditionally included in the init_sequence_f[] array. >> >> Others are unconditionally defined and included in init_sequence_f[], >> but have their entire body, sans a mandatory "return 0", conditionally >> compiled. >> >> For the simple cases, switch to the former model, making it a bit more >> consistent. This also makes the U-Boot image very slightly smaller and >> avoids a few useless calls to no-op functions during board_init_f. > > Can you check if it definitely does change the size? It does, I did build to see how much. On ppc, it's 8 bytes per no-op function (one "put 0 in r3", one blr instruction), plus 4 bytes for the array entry, plus 4 bytes for a .fixup entry - in my case ending up with 7*16=112, because all but the #ifndef CONFIG_OF_EMBED applied. The reason I ask > is that it inlines those function calls in the normal case, at least > from my inspection. The compiler can't inline and thus eliminate these as they are not called directly, but their addresses are used to populate the init_sequence_f[] array and called through that. > Using if() is preferable to #if if there is no cost. Completely agree, and I also prefer to have the linker eliminate unused functions rather than cluttering the C code with #ifdefs. But that can't be used in this case. Anyway, this wasn't primarily to save 112 bytes or whatnot from the U-Boot image, just to use one style a little more consistently. Rasmus
Re: [PATCH] common/board_f.c: use #ifdefs a little more consistently
On 28/02/2020 16.46, Tom Rini wrote: > On Fri, Feb 28, 2020 at 08:42:21AM +0000, Rasmus Villemoes wrote: >> On 28/02/2020 00.40, Simon Glass wrote: >>> Using if() is preferable to #if if there is no cost. >> >> Completely agree, and I also prefer to have the linker eliminate unused >> functions rather than cluttering the C code with #ifdefs. But that can't >> be used in this case. >> >> Anyway, this wasn't primarily to save 112 bytes or whatnot from the >> U-Boot image, just to use one style a little more consistently. > > Perhaps we could come up with a little more macro-magic? In > psuedo-code: > #define RESERVE_INIT_SEQ_F_ENTRY(fn) \ > #if CONFIG_IS_ENABLED(toupper(fn)) > reserve_##fn > #endif > #endif I'm afraid that's rather far from something that can be implemented; macros cannot expand to other preprocessor directives, and toupper is also pretty hard to do. What we could do if we want to reduce #ifdefs while still eliminating the no-op functions is to replace #ifdef CONFIG_FOO static int foo_init(void) { blabla; return 0; } #endif ... init_sequence_f[] = { ... #ifdef CONFIG_FOO foo_init, #endif ... } by static int foo_init(void) { /* always defined */ blabla; return 0; } init_sequence_f[] = { ... IS_ENABLED(CONFIG_FOO) ? foo_init : NULL, ... } and teach the iterator to ignore NULL entries (I don't remember if we use a NULL terminator or use ARRAY_SIZE; if the former one should switch to the latter). It will still cost sizeof(void*) for the NULL entries, but the function bodies (and on powerpc the .fixups) should be eliminated, and there's not an #ifdef in sight. If one also wants to get rid of those NULL entries, I did https://lore.kernel.org/lkml/1444165543-2209-1-git-send-email-li...@rasmusvillemoes.dk/ a long time ago (see the COND_INITIALIZER()), but whether that can be tweaked to still automatically avoid a "defined but not used" warning I don't know. > And then a little harder thinking about negative-case (OF_EMBED) ? Or > just have those be the few ifndef's? That can be done in the same manner by just switching second and third operand. The problem is the function bodies which rely on symbols (for example CONFIG_* values) that only exist if CONFIG_FOO. Rasmus
Re: [PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic
On 21/02/2020 17.19, Tom Rini wrote: > On Fri, Feb 21, 2020 at 05:14:14PM +0100, Wolfgang Denk wrote: >> Dear Rasmus, >> >> In message <5265fdd5-3992-4e5f-3235-5586b3b77...@prevas.dk> you wrote: >>> >>> So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. >> >> OK, but what about bords that don't store the envionment in a file >> system, but instead for example in (parallel or SPI) NOR flash or in >> a UBI volume? > > I think the intent is that there is no change today but the door is now > open for someone that can test / confirm changes there to do so. Yes, exactly. I could have just fixed sf.c which is the one I need for my current project, but it turns out that without the ability to say CONFIG_IS_ENABLED(SAVEENV) the changes to sf.c would be significantly uglier, so it seemed better to provide the infrastructure that will also be useful for converting other storage drivers to honour CONFIG_SPL_SAVEENV. Rasmus
Re: caching BLOBLISTT_SPL_HANDOFF
On 02/03/2020 20.47, Simon Glass wrote: > Hi Rasmus, > > On Fri, 28 Feb 2020 at 16:09, Rasmus Villemoes > wrote: >> >> Now that particular one seems a bit fishy: Why is it ok to cache the >> location of the BLOBLISTT_SPL_HANDOFF blob in gd->spl_handoff? Later in >> the init sequence there's a call to reserve_bloblist, and later again >> reloc_bloblist. Doesn't that leave gd->spl_handoff stale? > > Yes it does. It is only supposed to be used in the early stages of > U-Boot (proper) init. Yes, that's what I thought - and if it's only actually used once or twice during the early stages, there's not much point in caching it. > Actually I think that member could be dropped and we could search for > it each time: > > ./arch/x86/cpu/broadwell/cpu_from_spl.c Yes, there didn't seem to be many users, so it should not be that hard to get rid of. I also think that sets a better precedent for future bloblist users. Rasmus
[PATCH] mmc: fsl_esdhc: actually enable cache snooping on mpc830x
The reference manuals for MPC8308 and MPC8309 both say that the esdhcctl aka DMA Control Register "is implemented as SDHCCR" in the System configuration registers. Unfortunately, that doesn't mean that the registers are just mirrors of each other - any write to esdhcctl is simply ignored. So to actually enable cache snooping, we unfortunately have to add a little ifdeffery. There is, naturally, no description of the bit fields of esdhcctl in the MPC8309 manual, but comparing the description of esdhcctl from the LS1021A reference manual to the description of the sdhccr in MPC8309, one also finds that the fields are bit-reversed, so the bit to set is 0x0200 rather than 0x0040 - this is also what board_mmc_init() uses in the two gdsys/mpc8308/ boards. Signed-off-by: Rasmus Villemoes --- drivers/mmc/fsl_esdhc.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index 1e7d606cd8..34e5bd270f 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -577,6 +577,18 @@ static int esdhc_set_ios_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) return 0; } +static void esdhc_enable_cache_snooping(struct fsl_esdhc *regs) +{ +#ifdef CONFIG_ARCH_MPC830X + immap_t *immr = (immap_t *)CONFIG_SYS_IMMR; + sysconf83xx_t *sysconf = >sysconf; + + setbits_be32(>sdhccr, 0x0200); +#else + esdhc_write32(>esdhcctl, 0x0040); +#endif +} + static int esdhc_init_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) { struct fsl_esdhc *regs = priv->esdhc_regs; @@ -592,8 +604,7 @@ static int esdhc_init_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) return -ETIMEDOUT; } - /* Enable cache snooping */ - esdhc_write32(>esdhcctl, 0x0040); + esdhc_enable_cache_snooping(regs); esdhc_setbits32(>sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN); -- 2.23.0
Re: [U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images
On 08/08/2019 05.16, Tom Rini wrote: > On Fri, Aug 02, 2019 at 03:52:28PM -0700, Julius Werner wrote: > >> The Linux ramdisk should always be decompressed by the kernel itself, >> not by U-Boot. Therefore, the 'compression' node in the FIT image should >> always be set to "none" for ramdisk images, since the only point of >> using that node is if you want U-Boot to do the decompression itself. >> >> Yet some systems populate the node to the compression algorithm used by >> the kernel instead. This used to be ignored, but now that we support >> decompression of all image types it becomes a problem. Since ramdisks >> should never be decompressed by U-Boot anyway, this patch adds a special >> exception for them to avoid these issues. Still, setting the >> 'compression' node like that is wrong in the first place, so we still >> want to print out a warning so that third-party distributions doing this >> can notice and fix it. >> >> Signed-off-by: Julius Werner >> Reviewed-by: Heiko Schocher >> Tested-by: Heiko Schocher >> Reviewed-by: Simon Goldschmidt This part + if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE) + puts("WARNING: 'compression' nodes for ramdisks are deprecated," +" please fix your .its file!\n"); + ends up being a little confusing, because when one dutifully removes the compression = "foo" property, the warning is still there (because comp ends up being (u8)-1) - the only way to silence it is by actually _having_ a 'compression = "none"' property. (It also says node instead of property). So, what is the intention? Should ramdisk images not have a compression property at all, or must it be present but set to "none", or are either acceptable? Rasmus
enabling CONFIG_FIT_SIGNATURE breaks U-Boot
I'm trying to enable verified boot on an mpc8309, but the first step of simply enabling CONFIG_FIT_SIGNATURE causes this: [2020-02-07 17:23:34.342] U-Boot 2020.01-00037-g6e82ca3d2e (Feb 07 2020 - 17:23:16 +0100) [2020-02-07 17:23:34.379] [2020-02-07 17:23:34.379] Error binding driver 'mpc83xx_cpu': -12 [2020-02-07 17:23:34.379] Some drivers failed to bind [2020-02-07 17:23:34.379] Reset Status: External/Internal Hard [2020-02-07 17:23:34.380] [2020-02-07 17:23:34.380] initcall sequence 000975a4 failed at call 00016d24 (err=-19) [2020-02-07 17:23:34.380] ### ERROR ### Please RESET the board ### 00016d24 is , so I'm guessing that just fails due to the earlier error of mpc83xx_cpu failing to bind with -ENOMEM. But I can't figure out why just enabling CONFIG_FIT_SIGNATURE would have that dramatic an effect. I probably need to adjust some other config settings, but which? Thanks, Rasmus
[PATCH] net: convert NET_MAXDEFRAG to Kconfig
Signed-off-by: Rasmus Villemoes --- net/Kconfig | 10 ++ net/net.c| 3 --- scripts/config_whitelist.txt | 1 - 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/net/Kconfig b/net/Kconfig index a07f6746c5..96bbce1778 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -29,6 +29,16 @@ config IP_DEFRAG Selecting this will enable IP datagram reassembly according to the algorithm in RFC815. +config NET_MAXDEFRAG + int "Size of buffer used for IP datagram reassembly" + depends on IP_DEFRAG + default 16384 + range 1024 65536 + help + This defines the size of the statically allocated buffer + used for reassembly, and thus an upper bound for the size of + IP datagrams that can be received. + config TFTP_BLOCKSIZE int "TFTP block size" default 1468 diff --git a/net/net.c b/net/net.c index 5199d679a1..322b1f81bf 100644 --- a/net/net.c +++ b/net/net.c @@ -882,9 +882,6 @@ int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int sport, * to the algorithm in RFC815. It returns NULL or the pointer to * a complete packet, in static storage */ -#ifndef CONFIG_NET_MAXDEFRAG -#define CONFIG_NET_MAXDEFRAG 16384 -#endif #define IP_PKTSIZE (CONFIG_NET_MAXDEFRAG) #define IP_MAXUDP (IP_PKTSIZE - IP_HDR_SIZE) diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt index cf1808e051..669d0bf65d 100644 --- a/scripts/config_whitelist.txt +++ b/scripts/config_whitelist.txt @@ -1226,7 +1226,6 @@ CONFIG_NETSPACE_LITE_V2 CONFIG_NETSPACE_MAX_V2 CONFIG_NETSPACE_MINI_V2 CONFIG_NETSPACE_V2 -CONFIG_NET_MAXDEFRAG CONFIG_NET_MULTI CONFIG_NET_RETRY_COUNT CONFIG_NEVER_ASSERT_ODT_TO_CPU -- 2.23.0
Re: enabling CONFIG_FIT_SIGNATURE breaks U-Boot
On 07/02/2020 17.30, Rasmus Villemoes wrote: > I'm trying to enable verified boot on an mpc8309, but the first step of > simply enabling CONFIG_FIT_SIGNATURE causes this: Well, more precisely, CONFIG_RSA causes that - I tried disabling CONFIG_FIT_SIGNATURE but kept CONFIG_RSA, and the same still happens. Removing CONFIG_RSA makes the board boot. Still digging, but pointers are much appreciated. Thanks, Rasmus
Re: enabling CONFIG_FIT_SIGNATURE breaks U-Boot
On 07/02/2020 17.50, Rasmus Villemoes wrote: > On 07/02/2020 17.30, Rasmus Villemoes wrote: >> I'm trying to enable verified boot on an mpc8309, but the first step of >> simply enabling CONFIG_FIT_SIGNATURE causes this: > > Well, more precisely, CONFIG_RSA causes that - I tried disabling > CONFIG_FIT_SIGNATURE but kept CONFIG_RSA, and the same still happens. > Removing CONFIG_RSA makes the board boot. Still digging, but pointers > are much appreciated. OK, it wasn't that hard, sorry for the noise. Turns out I've been living on the edge, with the default value of CONFIG_SYS_MALLOC_F_LEN of 0x400 being just exactly enough, but then when the mod_exp_sw driver is also probed I run out. Rasmus
[PATCH resend 1/2] gpio: mpc8xxx: don't modify gpdat when setting gpio as input
Since some chips don't support reading back the value of output gpios from the gpdat register, we should not do a RMW cycle (i.e., the clrbits_be32) on the gpdat register when setting a gpio as input, as that might accidentally change the value of some other (still configured as output) gpio. The extra indirection through mpc8xxx_gpio_set_in() does not help readability, so just fold the gpdir update into mpc8xxx_gpio_direction_input(). Signed-off-by: Rasmus Villemoes --- drivers/gpio/mpc8xxx_gpio.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c index 9964d69035..5438196fe0 100644 --- a/drivers/gpio/mpc8xxx_gpio.c +++ b/drivers/gpio/mpc8xxx_gpio.c @@ -57,13 +57,6 @@ static inline u32 mpc8xxx_gpio_get_dir(struct ccsr_gpio *base, u32 mask) return in_be32(>gpdir) & mask; } -static inline void mpc8xxx_gpio_set_in(struct ccsr_gpio *base, u32 gpios) -{ - clrbits_be32(>gpdat, gpios); - /* GPDIR register 0 -> input */ - clrbits_be32(>gpdir, gpios); -} - static inline void mpc8xxx_gpio_set_low(struct ccsr_gpio *base, u32 gpios) { clrbits_be32(>gpdat, gpios); @@ -100,8 +93,11 @@ static inline void mpc8xxx_gpio_open_drain_off(struct ccsr_gpio *base, static int mpc8xxx_gpio_direction_input(struct udevice *dev, uint gpio) { struct mpc8xxx_gpio_data *data = dev_get_priv(dev); + u32 mask = gpio_mask(gpio); + + /* GPDIR register 0 -> input */ + clrbits_be32(>base->gpdir, mask); - mpc8xxx_gpio_set_in(data->base, gpio_mask(gpio)); return 0; } -- 2.23.0
[PATCH resend 0/2] gpio: mpc8xxx: honour shadow register when writing gpdat
[resending with Mario's correct address, sorry for the double post] The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that. Aside: Apparently, the mpc8309 also partially suffers from the errata preventing outputs from being read back - the bits corresponding to gpios 0-7 are always read as 0, while at least the value of gpio10 is correctly reflected when reading gpdat. Which is how I noticed these bugs - I couldn't understand why turning one LED on would turn off another. Rasmus Villemoes (2): gpio: mpc8xxx: don't modify gpdat when setting gpio as input gpio: mpc8xxx: don't do RMW on gpdat register when setting value drivers/gpio/mpc8xxx_gpio.c | 41 ++--- 1 file changed, 15 insertions(+), 26 deletions(-) -- 2.23.0
[PATCH 2/2] gpio: mpc8xxx: don't do RMW on gpdat register when setting value
The driver correctly handles reading back the value of an output gpio by reading from the shadow register for output, and from gpdat for inputs. Unfortunately, when setting the value of some gpio, we do a RMW cycle on the gpdat register without taking the shadow register into account, thus accidentally setting other output gpios (at least those whose value cannot be read back) to 0 at the same time. When changing a gpio from input to output, we still need to make sure it initially has the requested value. So, the procedure is - update the shadow register - compute the new gpdir register - write the bitwise and of the shadow and new gpdir register to gpdat - write the new gpdir register Signed-off-by: Rasmus Villemoes --- drivers/gpio/mpc8xxx_gpio.c | 29 +++-- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c index 5438196fe0..c5be8e673b 100644 --- a/drivers/gpio/mpc8xxx_gpio.c +++ b/drivers/gpio/mpc8xxx_gpio.c @@ -57,20 +57,6 @@ static inline u32 mpc8xxx_gpio_get_dir(struct ccsr_gpio *base, u32 mask) return in_be32(>gpdir) & mask; } -static inline void mpc8xxx_gpio_set_low(struct ccsr_gpio *base, u32 gpios) -{ - clrbits_be32(>gpdat, gpios); - /* GPDIR register 1 -> output */ - setbits_be32(>gpdir, gpios); -} - -static inline void mpc8xxx_gpio_set_high(struct ccsr_gpio *base, u32 gpios) -{ - setbits_be32(>gpdat, gpios); - /* GPDIR register 1 -> output */ - setbits_be32(>gpdir, gpios); -} - static inline int mpc8xxx_gpio_open_drain_val(struct ccsr_gpio *base, u32 mask) { return in_be32(>gpodr) & mask; @@ -104,14 +90,21 @@ static int mpc8xxx_gpio_direction_input(struct udevice *dev, uint gpio) static int mpc8xxx_gpio_set_value(struct udevice *dev, uint gpio, int value) { struct mpc8xxx_gpio_data *data = dev_get_priv(dev); + struct ccsr_gpio *base = data->base; + u32 mask = gpio_mask(gpio); + u32 gpdir; if (value) { - data->dat_shadow |= gpio_mask(gpio); - mpc8xxx_gpio_set_high(data->base, gpio_mask(gpio)); + data->dat_shadow |= mask; } else { - data->dat_shadow &= ~gpio_mask(gpio); - mpc8xxx_gpio_set_low(data->base, gpio_mask(gpio)); + data->dat_shadow &= ~mask; } + + gpdir = in_be32(>gpdir); + gpdir |= gpio_mask(gpio); + out_be32(>gpdat, gpdir & data->dat_shadow); + out_be32(>gpdir, gpdir); + return 0; } -- 2.23.0
[PATCH 0/2] gpio: mpc8xxx: honour shadow register when writing gpdat
The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that. Aside: Apparently, the mpc8309 also partially suffers from the errata preventing outputs from being read back - the bits corresponding to gpios 0-7 are always read as 0, while at least the value of gpio10 is correctly reflected when reading gpdat. Which is how I noticed these bugs - I couldn't understand why turning one LED on would turn off another. Rasmus Villemoes (2): gpio: mpc8xxx: don't modify gpdat when setting gpio as input gpio: mpc8xxx: don't do RMW on gpdat register when setting value drivers/gpio/mpc8xxx_gpio.c | 41 ++--- 1 file changed, 15 insertions(+), 26 deletions(-) -- 2.23.0
[PATCH 1/2] gpio: mpc8xxx: don't modify gpdat when setting gpio as input
Since some chips don't support reading back the value of output gpios from the gpdat register, we should not do a RMW cycle (i.e., the clrbits_be32) on the gpdat register when setting a gpio as input, as that might accidentally change the value of some other (still configured as output) gpio. The extra indirection through mpc8xxx_gpio_set_in() does not help readability, so just fold the gpdir update into mpc8xxx_gpio_direction_input(). Signed-off-by: Rasmus Villemoes --- drivers/gpio/mpc8xxx_gpio.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c index 9964d69035..5438196fe0 100644 --- a/drivers/gpio/mpc8xxx_gpio.c +++ b/drivers/gpio/mpc8xxx_gpio.c @@ -57,13 +57,6 @@ static inline u32 mpc8xxx_gpio_get_dir(struct ccsr_gpio *base, u32 mask) return in_be32(>gpdir) & mask; } -static inline void mpc8xxx_gpio_set_in(struct ccsr_gpio *base, u32 gpios) -{ - clrbits_be32(>gpdat, gpios); - /* GPDIR register 0 -> input */ - clrbits_be32(>gpdir, gpios); -} - static inline void mpc8xxx_gpio_set_low(struct ccsr_gpio *base, u32 gpios) { clrbits_be32(>gpdat, gpios); @@ -100,8 +93,11 @@ static inline void mpc8xxx_gpio_open_drain_off(struct ccsr_gpio *base, static int mpc8xxx_gpio_direction_input(struct udevice *dev, uint gpio) { struct mpc8xxx_gpio_data *data = dev_get_priv(dev); + u32 mask = gpio_mask(gpio); + + /* GPDIR register 0 -> input */ + clrbits_be32(>base->gpdir, mask); - mpc8xxx_gpio_set_in(data->base, gpio_mask(gpio)); return 0; } -- 2.23.0
[PATCH resend 2/2] gpio: mpc8xxx: don't do RMW on gpdat register when setting value
The driver correctly handles reading back the value of an output gpio by reading from the shadow register for output, and from gpdat for inputs. Unfortunately, when setting the value of some gpio, we do a RMW cycle on the gpdat register without taking the shadow register into account, thus accidentally setting other output gpios (at least those whose value cannot be read back) to 0 at the same time. When changing a gpio from input to output, we still need to make sure it initially has the requested value. So, the procedure is - update the shadow register - compute the new gpdir register - write the bitwise and of the shadow and new gpdir register to gpdat - write the new gpdir register Signed-off-by: Rasmus Villemoes --- drivers/gpio/mpc8xxx_gpio.c | 29 +++-- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c index 5438196fe0..c5be8e673b 100644 --- a/drivers/gpio/mpc8xxx_gpio.c +++ b/drivers/gpio/mpc8xxx_gpio.c @@ -57,20 +57,6 @@ static inline u32 mpc8xxx_gpio_get_dir(struct ccsr_gpio *base, u32 mask) return in_be32(>gpdir) & mask; } -static inline void mpc8xxx_gpio_set_low(struct ccsr_gpio *base, u32 gpios) -{ - clrbits_be32(>gpdat, gpios); - /* GPDIR register 1 -> output */ - setbits_be32(>gpdir, gpios); -} - -static inline void mpc8xxx_gpio_set_high(struct ccsr_gpio *base, u32 gpios) -{ - setbits_be32(>gpdat, gpios); - /* GPDIR register 1 -> output */ - setbits_be32(>gpdir, gpios); -} - static inline int mpc8xxx_gpio_open_drain_val(struct ccsr_gpio *base, u32 mask) { return in_be32(>gpodr) & mask; @@ -104,14 +90,21 @@ static int mpc8xxx_gpio_direction_input(struct udevice *dev, uint gpio) static int mpc8xxx_gpio_set_value(struct udevice *dev, uint gpio, int value) { struct mpc8xxx_gpio_data *data = dev_get_priv(dev); + struct ccsr_gpio *base = data->base; + u32 mask = gpio_mask(gpio); + u32 gpdir; if (value) { - data->dat_shadow |= gpio_mask(gpio); - mpc8xxx_gpio_set_high(data->base, gpio_mask(gpio)); + data->dat_shadow |= mask; } else { - data->dat_shadow &= ~gpio_mask(gpio); - mpc8xxx_gpio_set_low(data->base, gpio_mask(gpio)); + data->dat_shadow &= ~mask; } + + gpdir = in_be32(>gpdir); + gpdir |= gpio_mask(gpio); + out_be32(>gpdat, gpdir & data->dat_shadow); + out_be32(>gpdir, gpdir); + return 0; } -- 2.23.0
[PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
Currently, there's no way to fetch the value of an environment variable whose name is stored in some other variable, or generated from such - in non-working pseudo-code, ${${varname}} ${array${index}} This forces some scripts to needlessly duplicate logic and hardcode assumptions. For example, in an A/B scheme with three variables BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated BOOT_A_LEFT # 0..3 BOOT_B_LEFT # 0..3 when one needs to determine the slot to boot from, one does something like setenv found for slot in $BOOT_ORDER ; do if test "x$found" != "x" ; then # work around lack of break elif test "x$slot" = "xA" ; then if test $BOOT_A_LEFT -gt 0 ; then setexpr BOOT_A_LEFT $BOOT_A_LEFT - 1 setenv found A setenv bootargs ${bootargs_A} setenv ubivol ${ubivol_A} # more setup based on A fi elif test "x$slot" = "xB" ; then if test $BOOT_B_LEFT -gt 0 ; then # the same ... fi fi done This is already bad enough, but extending that to A/B/C is tedious and prone to copy-pastos. So this is an attempt at allowing one to do "env set -E var value1 value2" with the effect that, of course, normal variable expansion happens on the command line, the valueX are joined with spaces as usual, and then one more pass is done over that string replacing occurrences of ${foo}. The above would become setenv found for slot in $BOOT_ORDER ; do if test "x$found" != "x" ; then # work around lack of break else env set -E boot_left "\${BOOT_${slot}_LEFT}" if test $boot_left -gt 0 ; then setexpr BOOT_${slot}_LEFT $boot_left - 1 env set found $slot env set -E bootargs "\${bootargs_${slot}}" env set -E ubivol "\${ubivol_${slot}}" fi fi done I'm pleasantly surprised it was that easy to implement, but of course I'm cheating a bit (cli_simple_process_macros is only available if CONFIG_CMDLINE, though I think cli_simple.o could be unconditionally built and then link-time GC should get rid of the excess functions). This has been lightly tested in the sandbox. I'll add some proper unit tests, update the help texts and try to handle the Kconfig issue if this is something that might be accepted. Signed-off-by: Rasmus Villemoes --- cmd/nvedit.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 81d94cd193..ff6ffcb674 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -224,7 +224,7 @@ DONE: */ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) { - int i, len; + int i, len, expand = 0; char *name, *value, *s; struct env_entry e, *ep; @@ -244,6 +244,9 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) case 'f': /* force */ env_flag |= H_FORCE; break; + case 'E': + expand = 1; + break; default: return CMD_RET_USAGE; } @@ -287,6 +290,18 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) if (s != value) *--s = '\0'; + if (expand) { + char *expanded = malloc(CONFIG_SYS_CBSIZE); + + if (expanded == NULL) { + printf("## Can't malloc %d bytes\n", CONFIG_SYS_CBSIZE); + free(value); + return 1; + } + cli_simple_process_macros(value, expanded); + free(value); + value = expanded; + } e.key = name; e.data = value; hsearch_r(e, ENV_ENTER, , _htab, env_flag); -- 2.23.0
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 13/02/2020 16.55, Wolfgang Denk wrote: > Dear Rasmus, > > In message you wrote: >> >> I'm sorry, I see I mistyped in my example above, it should have been >> >> if test $slot = "A" ; setenv result $BOOT_A_LEFT ... >> >> as should hopefully be clear from the original post and the eval >> examples. So to reiterate, the problem is to get the contents (or value, >> if you will) of the BOOT_A_LEFT variable into the result variable, not >> setting result to the string "BOOT_A_LEFT" - but with the wrinkle that >> BOOT_A_LEFT is generated programmatically, so the code cannot literally >> mention BOOT_A_LEFT anywhere. > > Didn't I show this in my second, expanded example? I'm sorry, but no, you did not. You used the capability of the print (or rather printenv) command to take the name of a variable and print "name=value" to the terminal. In your example, result had the value BOOT_A_LEFT, so doing "print $result" meant executing the command "print BOOT_A_LEFT", and of course the output of that was then "BOOT_A_LEFT=boot a left". However, what I need is to get that "boot a left" value stored in some other variable so I can actually do further logic based on that value. > I suggest you provide a working example of shell code (say, bash, if > you like) to demonstrate what you really have in mind. It seems > I have problems understanding your verbal description. Assume BOOT_ORDER contains some permutation of "A B C", and for each letter x, there's a BOOT_x_LEFT counter telling how many boot attempts that slot has left. Now I want to find the first x in $BOOT_ORDER for which $BOOT_x_LEFT is positive. If all BOOT_x_LEFT are 0, say I want the sentinel value 'none'. So in bash, that might be written slot=none for x in $BOOT_ORDER ; do eval "left=\${BOOT_${x}_LEFT}" if [ $left -gt 0 ] ; then slot=$x break fi done Now we can work around the lack of break in the busybox shell by writing the loop so that the main body is skipped if we've found a valid slot: slot=none for x in $BOOT_ORDER ; do if [ $slot != 'none' ] ; then true else eval "left=\${BOOT_${x}_LEFT}" if [ $left -gt 0 ] ; then slot=$x fi fi done But we still can't translate this to busybox shell, because there's no eval. Now, I can do this with a hypothetical "env get" command which I just implemented to test that it works, and then the above becomes env set slot none; for x in $BOOT_ORDER ; do if test $slot != 'none' ; then true ; else env get left BOOT_${x}_LEFT ; # magic if test $left -gt 0 ; then env set slot $x ; fi; fi; done; Now, if you can implement the line marked #magic with existing functions, I'm all ears. Or if you can implement the semantics of this snippet in some other way, which does not open-code explicit references to BOOT_A_LEFT, BOOT_B_LEFT etc. This is what I meant when I said I'd prefer not to write the loop like this: env set slot none; for x in $BOOT_ORDER ; do if test $slot != 'none' ; then true ; else if test $x = A && test $BOOT_A_LEFT -gt 0 ; then env set slot A env set left $BOOT_A_LEFT elif test $x = B && test $BOOT_B_LEFT -gt 0 ; then env set slot B env set left $BOOT_B_LEFT elif test $x = C && test $BOOT_C_LEFT -gt 0 ; then env set slot C env set left $BOOT_C_LEFT fi fi; done; [yes, I also want left set as a side effect to the current value of the appropriate BOOT_x_LEFT]. >> So just as print[env] takes the name of a variable and shows the >> name=value string, and one can thus say "printenv BOOT_${slot}_LEFT" as >> you did in your extended example, I could do >> >> env get result BOOT_${slot}_LEFT >> >> and get the value of the BOOT_${slot}_LEFT variable into result. > > I still fail to see why you think this cannot be done with just the > already existing code. Just use setenv instead of printenv in my > example? > >> Would you be ok with adding such an "env get" with less foot-gun potential? > > I'm not OK with adding any special-purpose code which can easily > be implemented with existing scripting capabilites. Of course not. But _I'm_ not seeing how one can actually fetch the value of one variable into another, just given the first variable's name - when that name is programmatically generated, e.g. as BOOT_${x}_LEFT or whatnot. PS: Implementation of "env get" is just: --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -386,6 +386,20 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return _do_env_set(flag, argc, argv, H_INTERACTIVE); } +static int do_env_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + char *local_args[4]; + + if (argc != 3) + return CMD_RET_USAGE; + local_args[0] = argv[0]; + local_args[1] = argv[1]; + local_args[2] = env_get(argv[2]); + local_args[3] = NULL; + + return _do_env_set(flag, argc, local_args, H_INTERACTIVE); +} + /* * Prompt
[PATCH] env: make file-scope env_ptr variables static
The combination ENV_IS_IN_NVRAM=y, ENV_IS_IN_REMOTE=y fails to build: env/remote.o:/mnt/ext4/devel/u-boot/env/remote.c:17: multiple definition of `env_ptr' env/nvram.o:/mnt/ext4/devel/u-boot/env/nvram.c:41: first defined here It's not necessarily a meaningful combination, but for build-testing it's nice to be able to enable most ENV_IS_IN_* at the same time, and since these env_ptr are not declared anywhere, they really have no reason to have external linkage. nand.c and flash.c similarly already define file-scope static env_ptr variables. Signed-off-by: Rasmus Villemoes --- env/nvram.c | 2 +- env/remote.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/env/nvram.c b/env/nvram.c index a78db21623..1a9fcf1c06 100644 --- a/env/nvram.c +++ b/env/nvram.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR; extern void *nvram_read(void *dest, const long src, size_t count); extern void nvram_write(long dest, const void *src, size_t count); #else -env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; +static env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; #endif #ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE diff --git a/env/remote.c b/env/remote.c index 50d77b8dfe..e3f0608b16 100644 --- a/env/remote.c +++ b/env/remote.c @@ -12,9 +12,9 @@ #include #ifdef ENV_IS_EMBEDDED -env_t *env_ptr = +static env_t *env_ptr = #else /* ! ENV_IS_EMBEDDED */ -env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; +static env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; #endif /* ENV_IS_EMBEDDED */ DECLARE_GLOBAL_DATA_PTR; -- 2.23.0
[PATCH] include/eeprom.h: fix build errors
CMD_EEPROM and ENV_IS_IN_EEPROM can be selected independently, and cmd/eeprom.o gets built in either case, so whether to declare the real prototypes needs to follow the same logic as whether cmd/eeprom.c is built. Otherwise a ENV_IS_IN_EEPROM=y, CMD_EEPROM=n build fails cmd/eeprom.c:73:1: error: expected identifier or ‘(’ before ‘{’ token { While at it, fix the dummy replacements (at least assuming they are meant to allow the code to compile) - they need to have the same type as the expression they replace, or one gets errors such as env/eeprom.c: In function ‘eeprom_bus_read’: env/eeprom.c:37:8: error: void value not ignored as it ought to be rcode = eeprom_read(dev_addr, offset, buffer, cnt); Signed-off-by: Rasmus Villemoes --- include/eeprom.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/eeprom.h b/include/eeprom.h index 79118eb83d..6820844cea 100644 --- a/include/eeprom.h +++ b/include/eeprom.h @@ -7,7 +7,7 @@ #ifndef __EEPROM_LEGACY_H #define __EEPROM_LEGACY_H -#ifdef CONFIG_CMD_EEPROM +#if defined(CONFIG_CMD_EEPROM) || defined(CONFIG_ENV_IS_IN_EEPROM) void eeprom_init(int bus); int eeprom_read(uint dev_addr, uint offset, uchar *buffer, uint cnt); int eeprom_write(uint dev_addr, uint offset, uchar *buffer, uint cnt); @@ -17,8 +17,8 @@ int eeprom_write(uint dev_addr, uint offset, uchar *buffer, uint cnt); * some macros here so we don't have to touch every one of those uses */ #define eeprom_init(bus) -#define eeprom_read(dev_addr, offset, buffer, cnt) ((void)-ENOSYS) -#define eeprom_write(dev_addr, offset, buffer, cnt) ((void)-ENOSYS) +#define eeprom_read(dev_addr, offset, buffer, cnt) (-ENOSYS) +#define eeprom_write(dev_addr, offset, buffer, cnt) (-ENOSYS) #endif #if !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) && defined(CONFIG_SYS_I2C_EEPROM_ADDR) -- 2.23.0
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 16/02/2020 18.25, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200216152427.e80c7240...@gemini.denx.de> I wrote: >> >> So lets change my little script to add setting "left": >> >> slot=none >> for i in $BOOT_ORDER ; do >> setenv tmp_cmd 'setexpr tmp_val sub '^' "" $'BOOT_${i}_LEFT >> run tmp_cmd >> test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val >> done >> echo "## Chosen Slot = $slot ; Left = $left" >> >> Result: >> >> ## Chosen Slot = C ; Left = 2 > > Actually I'm stupid. No, but I did notice the above seemed needlessly obfuscated :) It's much easier this way, and without the > ugly printed messages: > > setenv BOOT_ORDER A B C D > setenv BOOT_A_LEFT 0 > setenv BOOT_B_LEFT 0 > setenv BOOT_C_LEFT 2 > setenv BOOT_D_LEFT 5 > > slot=none > for i in $BOOT_ORDER ; do > setenv tmp_cmd 'setenv tmp_val $'BOOT_${i}_LEFT > run tmp_cmd Nice. So the trick I was missing was to get a literal $, followed by the ("computed") name of the env var I wanted, all embedded in a command to be run (to invoke the second expansion). It's a bit tricky, but it does get the job done. There should be some catalogue of things like this, mentioning "U-Boot shell doesn't directly have $that feature, but you can often emulate it with something like $this". > test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val If performance matters, one can move the tmp_cmd handling after the slot=none test - then one can also use left directly instead of tmp_val, so the line only grows by one clause: test $slot = none && setenv tmp_cmd 'setenv left $'BOOT_${i}_LEFT && run tmp_cmd && test $left -gt 0 && slot=$i Or as a more readable alternative that still avoids the "run" overhead and saves one line (and the tmp_var) setenv tmp_cmd 'setenv left $'BOOT_${i}_LEFT test $slot = none && run tmp_cmd && test $left -gt 0 && slot=$i Thanks, Wolfgang. Consider both "env set -E" and the alternative "env get" withdrawn. Rasmus
Re: [PATCH 0/5] CMD_SAVEENV ifdef cleanup
On 19/02/2020 14.25, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200219094726.26798-1-rasmus.villem...@prevas.dk> you wrote: >> >> [1] Here's the current conditions for which these three drivers >> provide .save: >> >> SPL U-Boot >> ext4.cCONFIG_CMD_SAVEENV=yCONFIG_CMD_SAVEENV=y >> fat.c never CONFIG_CMD_SAVEENV=y >> sf.c never CONFIG_CMD_SAVEENV=y [2] > > Some questions: > > > 1) I'm not sure if your changes cover the situation that you want to >have "saveenv" available in U-Boot proper, but do NOT want to >have it in SPL. They do, that's the whole point of introducing the simple CONFIG_IS_ENABLED(SAVEENV) - for answering the question "do we want to enable saving the environment in this context". Then the .save method gets built and linked in precisely if that's the case, so one gets a consistent matrix that says SPL U-Boot ext4.cCONFIG_SPL_SAVEENV=yCONFIG_CMD_SAVEENV=y fat.c CONFIG_SPL_SAVEENV=yCONFIG_CMD_SAVEENV=y sf.c CONFIG_SPL_SAVEENV=yCONFIG_CMD_SAVEENV=y But I can't fix the whole world in one go, so only the above three get fixed to that state for now. >It is mandatory that this possibility is kept. Of course, and _nothing_ changes in that regard. [It is of course possible that I messed up with the implementation, but it is certainly the intention to keep it that way.] > > 2) It seems wrong to me to make such cleanup in any way dependent on >file system type or a mix of arbitrary storage driver types. >this should be handled in two independent, orthogonal steps: >a) clean up any drivers or file system accessors that do not fit > into the general model >b) adapt the general model to such changes > >Maybe it makes sense to change the order of these steps, if this >results in less intrusive patches - I have no ides. > >In any case testing must _also_ include all the other many ways >of storing the environment, including parallel or SPI NOR flash, >NAND flash, UBI, UNIFS, etc. See above, I can't fix, much less test, all those backend drivers. Nothing changes for those, they provide a .save method under exactly the same (probably mutually inconsistent...) conditions as they used to. So I expect that when someone else runs into wanting CONFIG_SPL_SAVEENV honoured with, say, mmc backend, they probably quickly discover that doesn't work at all, and then they can fix mmc.c to make it work. But it's not in all cases as simply as just removing the custom/arbitrary ifdef logic, sometimes those ifdefs cover code that depends on certain macros or whatnot that are not available for an SPL_BUILD. Rasmus
Re: [PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic
On 19/02/2020 14.27, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200219094726.26798-4-rasmus.villem...@prevas.dk> you wrote: >> Always compile the env_fat_save() function, and let >> CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether >> it actually ends up being compiled in. > > Have you tested that this works? How do the sizes of the > images differe before and after applying your changes? With or without these patches, I get $ size u-boot spl/u-boot-spl textdata bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 524033360 276 56039dae7 spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load $ nm u-boot | grep env_fat 17826cb4 t env_fat_load 17826c10 t env_fat_save for a wandboard_defconfig modified by -CONFIG_SPL_FS_EXT4=y +CONFIG_SPL_FS_FAT=y +CONFIG_SPL_ENV_SUPPORT=y +CONFIG_ENV_IS_IN_FAT=y So in the "read-only environment access in SPL" case, everything is the same before and after. Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my patches we get $ size u-boot spl/u-boot-spl textdata bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 582983360 65860 127518 1f21e spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c6e0 t env_fat_load 0090c63c t env_fat_save but without, $ size u-boot spl/u-boot-spl textdata bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 526593360 280 56299dbeb spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. > [Same question for ext4] Actually, the situation for ext4 is even worse than indicated. Just from reading code in ext4.c, env_ext4_save gets built into the SPL if CONFIG_CMD_SAVEENV, whether or not CONFIG_SPL_SAVEENV is set. So I expected my patch to simply reduce the spl image size in the CONFIG_SPL_SAVEENV=n case. But one cannot compare - currently building with CONFIG_CMD_SAVEENV=y CONFIG_SPL_ENV_IS_IN_EXT4=y CONFIG_SPL_SAVEENV=n simply fails the SPL build because env_ext4_save refers to hexport_r, which is only compiled if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV)) - which took me a while to read, it's a little easier if spelled !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_SAVEENV) Anyway, a side-effect of my ext4 patch is that the above combination actually builds, because env_ext4_save is not linked in, so hexport_r isn't needed. And turning on SPL_SAVEENV also works as expected. Rasmus
Re: [PATCH 2/3] tools: add fdt_add_pubkey
On 11/02/2020 10.54, Alex Kiernan wrote: > On Tue, Feb 11, 2020 at 9:49 AM Rasmus Villemoes > wrote: >> >> Having to use the -K option to mkimage to populate U-Boot's .dtb with the >> public key while signing the kernel FIT image is often a little >> awkward. In particular, when using a meta-build system such as >> bitbake/Yocto, having the tasks of the kernel and U-Boot recipes >> intertwined, modifying deployed artifacts and rebuilding U-Boot with >> an updated .dtb is quite cumbersome. Also, in some scenarios one may >> wish to build U-Boot complete with the public key(s) embedded in the >> .dtb without the corresponding private keys being present on the same >> build host. >> > > Have you started looking at the required bitbake pieces? You're > definitely dealing with a piece of pain that I'd like resolved! Not really, but I know that something like this is a necessary first part - and I'm glad to know I'm not the only one struggling with this. For now I've come to the conclusion that kernel-fitimage.bbclass is not worth the trouble (for example, I need to create two different fit images with different initramfs, but a fit image without initramfs is pointless in my case, and there's no way to use kernel-fitimage.bbclass for that), so I just set KERNEL_IMAGETYPE to vmlinux, then have my own extra tasks doing the objcopy -O binary, gzip, and mkimage the different fit images I need. [I'm also thinking that adding a companion tool for doing the signing part might make sense at some point - it's somewhat counter-intuituve that the .its contains some of the information (base name of key and algorithm - mkimage currently just segfaults if key-name-hint is accidentally omitted from the .its), while mkimage needs to be fed with another part (directory holding the keys).] Rasmus
[PATCH 2/3] tools: add fdt_add_pubkey
Having to use the -K option to mkimage to populate U-Boot's .dtb with the public key while signing the kernel FIT image is often a little awkward. In particular, when using a meta-build system such as bitbake/Yocto, having the tasks of the kernel and U-Boot recipes intertwined, modifying deployed artifacts and rebuilding U-Boot with an updated .dtb is quite cumbersome. Also, in some scenarios one may wish to build U-Boot complete with the public key(s) embedded in the .dtb without the corresponding private keys being present on the same build host. So this adds a simple tool that allows one to disentangle the kernel and U-Boot builds, by simply copy-pasting just enough of the mkimage code to allow one to add a public key to a .dtb. When using mkimage, some of the information is taken from the .its used to build the kernel (algorithm and key name), so that of course needs to be supplied on the command line. Signed-off-by: Rasmus Villemoes --- tools/.gitignore | 1 + tools/Makefile | 3 ++ tools/fdt_add_pubkey.c | 96 ++ 3 files changed, 100 insertions(+) create mode 100644 tools/fdt_add_pubkey.c diff --git a/tools/.gitignore b/tools/.gitignore index 82bdce2782..a9894db853 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -6,6 +6,7 @@ /dumpimage /easylogo/easylogo /envcrc +/fdt_add_pubkey /fdtgrep /file2include /fit_check_sign diff --git a/tools/Makefile b/tools/Makefile index 345bc84e48..d91edeaddc 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -54,6 +54,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o hostprogs-y += dumpimage mkimage hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign +hostprogs-$(CONFIG_FIT_SIGNATURE) += fdt_add_pubkey hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include @@ -122,6 +123,7 @@ dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o mkimage-objs := $(dumpimage-mkimage-objs) mkimage.o fit_info-objs := $(dumpimage-mkimage-objs) fit_info.o fit_check_sign-objs := $(dumpimage-mkimage-objs) fit_check_sign.o +fdt_add_pubkey-objs := $(dumpimage-mkimage-objs) fdt_add_pubkey.o file2include-objs := file2include.o ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_FIT_SIGNATURE),) @@ -166,6 +168,7 @@ HOSTCFLAGS_fit_image.o += -DMKIMAGE_DTC=\"$(CONFIG_MKIMAGE_DTC_PATH)\" HOSTLOADLIBES_dumpimage := $(HOSTLOADLIBES_mkimage) HOSTLOADLIBES_fit_info := $(HOSTLOADLIBES_mkimage) HOSTLOADLIBES_fit_check_sign := $(HOSTLOADLIBES_mkimage) +HOSTLOADLIBES_fdt_add_pubkey := $(HOSTLOADLIBES_mkimage) hostprogs-$(CONFIG_EXYNOS5250) += mkexynosspl hostprogs-$(CONFIG_EXYNOS5420) += mkexynosspl diff --git a/tools/fdt_add_pubkey.c b/tools/fdt_add_pubkey.c new file mode 100644 index 00..45a2ce9ad2 --- /dev/null +++ b/tools/fdt_add_pubkey.c @@ -0,0 +1,96 @@ +#include +#include "fit_common.h" + +static const char *cmdname; + +static const char *algo_name = "sha1,rsa2048"; /* -a */ +static const char *keydir = "."; /* -k */ +static const char *keyname = "key"; /* -n */ +static const char *require_keys; /* -r */ +static const char *keydest; /* argv[n] */ + +static void usage(const char *msg) +{ + fprintf(stderr, "Error: %s\n", msg); + fprintf(stderr, "Usage: %s [-a ] [-k ] [-n ] [-r ] \n", + cmdname); + exit(EXIT_FAILURE); +} + +static void process_args(int argc, char *argv[]) +{ + int opt; + + while((opt = getopt(argc, argv, "a:k:n:r:")) != -1) { + switch (opt) { + case 'k': + keydir = optarg; + break; + case 'a': + algo_name = optarg; + break; + case 'n': + keyname = optarg; + break; + case 'r': + require_keys = optarg; + break; + default: + usage("Invalid option"); + } + } + /* The last parameter is expected to be the .dtb to add the public key to */ + if (optind < argc) + keydest = argv[optind]; + + if (!keydest) + usage("Missing dtb file to update"); +} + +int main(int argc, char *argv[]) +{ + struct image_sign_info info; + int destfd, ret; + void *dest_blob = NULL; + struct stat dest_sbuf; + size_t size_inc = 0; + + cmdname = argv[0]; + + process_args(argc, argv); + + memset(, 0, sizeof(info)); + + info.keydir = keydir; + info.keyname = keyname; + info.name = algo_name; + info.require_keys = require_keys; + info.crypto = image_get_crypto_algo(algo_name); + if (!info.crypto) { +fprintf(stderr, "Unsupported signature algorithm '%s'\n", algo_name); + exit(E
[PATCH 0/3] RFC: add fdt_add_pubkey tool
In order to reduce the coupling between building the kernel and U-Boot, I'd like a tool that can add a public key to U-Boot's dtb without simultaneously signing a FIT image. That tool doesn't seem to exist, so I stole the necessary pieces from mkimage et al and put it in a single .c file. I'm still working on the details of my proposed "require just k out these n required keys" and how it should be implemented, but it will probably involve teaching this tool a bunch of new options. These patches are not necessarily ready for inclusion (unless someone else finds fdt_add_pubkey useful as is), but I thought I might as well send it out for early comments. Rasmus Villemoes (3): test_vboot.py: remove extraneous -k option to fit_check_sign tools: add fdt_add_pubkey test_vboot.py: include test of fdt_add_pubkey tool test/py/tests/test_vboot.py | 11 - tools/.gitignore| 1 + tools/Makefile | 3 ++ tools/fdt_add_pubkey.c | 96 + 4 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 tools/fdt_add_pubkey.c -- 2.23.0
[PATCH 1/3] test_vboot.py: remove extraneous -k option to fit_check_sign
Signed-off-by: Rasmus Villemoes --- test/py/tests/test_vboot.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py index 9c41ee56b1..3dd8e3cb66 100644 --- a/test/py/tests/test_vboot.py +++ b/test/py/tests/test_vboot.py @@ -180,8 +180,7 @@ def test_vboot(u_boot_console): cons.log.action('%s: Check signed config on the host' % sha_algo) -util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', tmpdir, -'-k', dtb]) +util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb]) # Replace header bytes bcfg = u_boot_console.config.buildconfig -- 2.23.0
[PATCH 3/3] test_vboot.py: include test of fdt_add_pubkey tool
Signed-off-by: Rasmus Villemoes --- test/py/tests/test_vboot.py | 8 1 file changed, 8 insertions(+) diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py index 3dd8e3cb66..799c28cc2c 100644 --- a/test/py/tests/test_vboot.py +++ b/test/py/tests/test_vboot.py @@ -182,6 +182,13 @@ def test_vboot(u_boot_console): util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb]) +# Create a fresh .dtb without the public keys +dtc('sandbox-u-boot.dts') +# Then add the dev key via the fdt_add_pubkey tool +util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,rsa2048' % sha_algo, +'-k', tmpdir, '-n', 'dev', '-r', 'conf', dtb]) +util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb]) + # Replace header bytes bcfg = u_boot_console.config.buildconfig max_size = int(bcfg.get('config_fit_signature_max_size', 0x1000), 0) @@ -246,6 +253,7 @@ def test_vboot(u_boot_console): fit = '%stest.fit' % tmpdir mkimage = cons.config.build_dir + '/tools/mkimage' fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign' +fdt_add_pubkey = cons.config.build_dir + '/tools/fdt_add_pubkey' dtc_args = '-I dts -O dtb -i %s' % tmpdir dtb = '%ssandbox-u-boot.dtb' % tmpdir sig_node = '/configurations/conf-1/signature' -- 2.23.0
Re: [PATCH resend 0/2] gpio: mpc8xxx: honour shadow register when writing gpdat
On 28/01/2020 13.04, Rasmus Villemoes wrote: > Rasmus Villemoes (2): > gpio: mpc8xxx: don't modify gpdat when setting gpio as input > gpio: mpc8xxx: don't do RMW on gpdat register when setting value > > drivers/gpio/mpc8xxx_gpio.c | 41 ++--- > 1 file changed, 15 insertions(+), 26 deletions(-) ping
Re: [PATCH] mmc: fsl_esdhc: actually enable cache snooping on mpc830x
On 06/02/2020 05.14, Y.b. Lu wrote: >> -Original Message- >> From: Peng Fan >> Sent: Wednesday, February 5, 2020 3:08 PM >> To: Rasmus Villemoes ; u-boot@lists.denx.de; >> Y.b. Lu >> Cc: Mario Six >> Subject: RE: [PATCH] mmc: fsl_esdhc: actually enable cache snooping on >> mpc830x >> >>> Subject: [PATCH] mmc: fsl_esdhc: actually enable cache snooping on >> mpc830x >> >> + Y.b >> >> Are you ok with this patch? > > The mpc830x is old Freescale PowerPC platforms I don't have. > But I agree the fix-up. > > Reviewed-by: Yangbo Lu Thanks. Any chance this could make it to master before the 2020.04 release? Rasmus
[PATCH resend 1/5] gpio/mpc83xx_spisel_boot.c: gpio driver for SPISEL_BOOT signal
From: "Klaus H. Sorensen" Some SoCs in the mpc83xx family, e.g. mpc8309, have a dedicated spi chip select, SPISEL_BOOT, that is used by the boot code to boot from flash. This chip select will typically be used to select a SPI boot flash. The SPISEL_BOOT signal is controlled by a single bit in the SPI_CS register. Implement a gpio driver for the spi chip select register. This allows a spi driver capable of using gpios as chip select, to bind a chip select to SPISEL_BOOT. It may be a little odd to do this as a GPIO driver, since the signal is neither GP or I, but it is quite convenient to present it to the spi driver that way. The alternative it to teach mpc8xxx_spi to handle the SPISEL_BOOT signal itself (that is how it's done in the linux kernel, see commit 69b921acae8a) Signed-off-by: Klaus H. Sorensen Signed-off-by: Rasmus Villemoes --- .../gpio/fsl,mpc83xx-spisel-boot.txt | 22 +++ drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 1 + drivers/gpio/mpc83xx_spisel_boot.c| 148 ++ 4 files changed, 179 insertions(+) create mode 100644 doc/device-tree-bindings/gpio/fsl,mpc83xx-spisel-boot.txt create mode 100644 drivers/gpio/mpc83xx_spisel_boot.c diff --git a/doc/device-tree-bindings/gpio/fsl,mpc83xx-spisel-boot.txt b/doc/device-tree-bindings/gpio/fsl,mpc83xx-spisel-boot.txt new file mode 100644 index 00..52d8bb0a5c --- /dev/null +++ b/doc/device-tree-bindings/gpio/fsl,mpc83xx-spisel-boot.txt @@ -0,0 +1,22 @@ +MPC83xx SPISEL_BOOT gpio controller + +Provide access to MPC83xx SPISEL_BOOT signal as a gpio to allow it to be +easily bound as a SPI controller chip select. + +The SPISEL_BOOT signal is always an output. + +Required properties: + +- compatible: must be "fsl,mpc83xx-spisel-boot" or "fsl,mpc8309-spisel-boot". +- reg: must point to the SPI_CS register in the SoC register map. +- ngpios: number of gpios provided by driver, normally 1. + +Example: + + spisel_boot: spisel_boot@14c { + compatible = "fsl,mpc8309-spisel-boot"; + reg = <0x14c 0x04>; + #gpio-cells = <2>; + device_type = "gpio"; + ngpios = <1>; + }; diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index c1ad5d64a3..73fdb8cb3b 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -383,6 +383,14 @@ config MPC8XXX_GPIO value setting, the open-drain feature, which can configure individual GPIOs to work as open-drain outputs, is supported. +config MPC83XX_SPISEL_BOOT + bool "Freescale MPC83XX SPISEL_BOOT driver" + depends on DM_GPIO && ARCH_MPC830X + help + GPIO driver to set/clear dedicated SPISEL_BOOT output on MPC83XX. + + This pin is typically used as spi chip select to a spi nor flash. + config MT7621_GPIO bool "MediaTek MT7621 GPIO driver" depends on DM_GPIO && SOC_MT7628 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index ccc49e2eb0..bbeec30431 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_DM644X_GPIO) += da8xx_gpio.o obj-$(CONFIG_ALTERA_PIO) += altera_pio.o obj-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o obj-$(CONFIG_MPC8XXX_GPIO) += mpc8xxx_gpio.o +obj-$(CONFIG_MPC83XX_SPISEL_BOOT) += mpc83xx_spisel_boot.o obj-$(CONFIG_SH_GPIO_PFC) += sh_pfc.o obj-$(CONFIG_OMAP_GPIO)+= omap_gpio.o obj-$(CONFIG_DB8500_GPIO) += db8500_gpio.o diff --git a/drivers/gpio/mpc83xx_spisel_boot.c b/drivers/gpio/mpc83xx_spisel_boot.c new file mode 100644 index 00..c7b08404d9 --- /dev/null +++ b/drivers/gpio/mpc83xx_spisel_boot.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2019 DEIF A/S + * + * GPIO driver to set/clear SPISEL_BOOT pin on mpc83xx. + */ + +#include +#include +#include +#include + +struct mpc83xx_spisel_boot { + u32 __iomem *spi_cs; + ulong addr; + uint gpio_count; + ulong type; +}; + +static u32 gpio_mask(uint gpio) +{ + return (1U << (31 - (gpio))); +} + +static int mpc83xx_spisel_boot_direction_input(struct udevice *dev, uint gpio) +{ + return -EINVAL; +} + +static int mpc83xx_spisel_boot_set_value(struct udevice *dev, uint gpio, int value) +{ + struct mpc83xx_spisel_boot *data = dev_get_priv(dev); + + debug("%s: gpio=%d, value=%u, gpio_mask=0x%08x\n", __func__, + gpio, value, gpio_mask(gpio)); + + if (value) + setbits_be32(data->spi_cs, gpio_mask(gpio)); + else + clrbits_be32(data->spi_cs, gpio_mask(gpio)); + + return 0; +} + +static int mpc83xx_spisel_boot_direction_output(struct udevice *dev, uint gpio, int value) +{ + return 0; +} + +static int mpc83xx_spisel_boot_get_value(str
[PATCH resend 5/5] mpc8xxx_spi: implement real ->set_speed
Not all boards have the same CSB frequency, nor do every SPI slave necessarily support running at 16.7 MHz. So implement ->set_speed; that also allows using a smaller PM (i.e., 0) for slaves that do support a higher speed. Based on work by Klaus H. Sørensen. Cc: Klaus H. Sorensen Signed-off-by: Rasmus Villemoes --- drivers/spi/mpc8xxx_spi.c | 64 --- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c index 8ef2451411..1bde31ad34 100644 --- a/drivers/spi/mpc8xxx_spi.c +++ b/drivers/spi/mpc8xxx_spi.c @@ -5,6 +5,7 @@ */ #include +#include #include #include #include @@ -28,6 +29,7 @@ enum { SPI_MODE_LEN_MASK = 0xf0, SPI_MODE_LEN_SHIFT = 20, + SPI_MODE_PM_SHIFT = 16, SPI_MODE_PM_MASK = 0xf, SPI_COM_LST = BIT(31 - 9), @@ -37,24 +39,19 @@ struct mpc8xxx_priv { spi8xxx_t *spi; struct gpio_desc gpios[16]; int cs_count; + ulong clk_rate; }; -static inline u32 to_prescale_mod(u32 val) -{ - return (min(val, (u32)15) << 16); -} - #define SPI_TIMEOUT1000 static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev) { struct mpc8xxx_priv *priv = dev_get_priv(dev); + struct clk clk; int ret; priv->spi = (spi8xxx_t *)dev_read_addr(dev); - /* TODO(mario@gdsys.cc): Read clock and save the value */ - ret = gpio_request_list_by_name(dev, "gpios", priv->gpios, ARRAY_SIZE(priv->gpios), GPIOD_IS_OUT | GPIOD_ACTIVE_LOW); if (ret < 0) @@ -62,6 +59,18 @@ static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev) priv->cs_count = ret; + ret = clk_get_by_index(dev, 0, ); + if (ret) { + dev_err(dev, "%s: clock not defined\n", __func__); + return ret; + } + + priv->clk_rate = clk_get_rate(); + if (!priv->clk_rate) { + dev_err(dev, "%s: failed to get clock rate\n", __func__); + return -EINVAL; + } + return 0; } @@ -79,10 +88,6 @@ static int mpc8xxx_spi_probe(struct udevice *dev) /* set len to 8 bits */ setbits_be32(>mode, (8 - 1) << SPI_MODE_LEN_SHIFT); - /* TODO(mario@gdsys.cc): This only ever sets one fixed speed */ - /* Use SYSCLK / 8 (16.67MHz typ.) */ - clrsetbits_be32(>mode, SPI_MODE_PM_MASK, to_prescale_mod(1)); - setbits_be32(>mode, SPI_MODE_EN); /* Clear all SPI events */ @@ -204,6 +209,43 @@ static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen, static int mpc8xxx_spi_set_speed(struct udevice *dev, uint speed) { + struct mpc8xxx_priv *priv = dev_get_priv(dev); + spi8xxx_t *spi = priv->spi; + u32 bits, mask, div16, pm; + u32 mode; + ulong clk; + + clk = priv->clk_rate; + if (clk / 64 > speed) { + div16 = SPI_MODE_DIV16; + clk /= 16; + } else { + div16 = 0; + } + pm = (clk - 1)/(4*speed) + 1; + if (pm > 16) { + dev_err(dev, "requested speed %u too small\n", speed); + return -EINVAL; + } + pm--; + + bits = div16 | (pm << SPI_MODE_PM_SHIFT); + mask = SPI_MODE_DIV16 | SPI_MODE_PM_MASK; + mode = in_be32(>mode); + if ((mode & mask) != bits) { + /* Must clear mode[EN] while changing speed. */ + mode &= ~(mask | SPI_MODE_EN); + out_be32(>mode, mode); + mode |= bits; + out_be32(>mode, mode); + mode |= SPI_MODE_EN; + out_be32(>mode, mode); + } + + debug("requested speed %u, set speed to %lu/(%s4*%u) == %lu\n", + speed, priv->clk_rate, div16 ? "16*" : "", pm + 1, + clk/(4*(pm + 1))); + return 0; } -- 2.23.0
[PATCH resend 2/5] gazerbeam: add clocks property to SPI node
Prepare for supporting setting different speeds in mpc8xxx_spi.c. Signed-off-by: Rasmus Villemoes --- arch/powerpc/dts/gdsys/mpc8308.dtsi | 7 +++ 1 file changed, 7 insertions(+) diff --git a/arch/powerpc/dts/gdsys/mpc8308.dtsi b/arch/powerpc/dts/gdsys/mpc8308.dtsi index 23e7403d91..1a319e2328 100644 --- a/arch/powerpc/dts/gdsys/mpc8308.dtsi +++ b/arch/powerpc/dts/gdsys/mpc8308.dtsi @@ -17,6 +17,7 @@ /dts-v1/; #include +#include / { compatible = "fsl,mpc8308rdb"; @@ -50,6 +51,11 @@ }; }; + socclocks: clocks { + compatible = "fsl,mpc8308-clk"; + #clock-cells = <1>; + }; + board_lbc: localbus@e0005000 { #address-cells = <2>; #size-cells = <1>; @@ -173,6 +179,7 @@ reg = <0x7000 0x1000>; interrupts = <16 0x8>; interrupt-parent = <>; + clocks = < MPC83XX_CLK_CSB>; mode = "cpu"; }; -- 2.23.0
[PATCH resend 3/5] mpc8xxx_spi: put max_cs to use
Currently, max_cs is write-only; it's just set in mpc8xxx_spi_ofdata_to_platdata and not otherwise used. My mpc8309 was always resetting during an "sf probe 0". It turns out dm_gpio_set_dir_flags() was being called with garbage, since nothing had initialized priv->gpios[0] - our device tree used "cs-gpios" rather than "gpios", so gpio_request_list_by_name() had returned 0. That would have been a lot easier to figure out if the chip select index was sanity checked, so rename max_cs to cs_count, and reject a xfer with a too large cs index. Signed-off-by: Rasmus Villemoes --- drivers/spi/mpc8xxx_spi.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c index 1c7bf10f91..ac4d0a9bae 100644 --- a/drivers/spi/mpc8xxx_spi.c +++ b/drivers/spi/mpc8xxx_spi.c @@ -35,7 +35,7 @@ enum { struct mpc8xxx_priv { spi8xxx_t *spi; struct gpio_desc gpios[16]; - int max_cs; + int cs_count; }; static inline u32 to_prescale_mod(u32 val) @@ -74,7 +74,7 @@ static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev) if (ret < 0) return -EINVAL; - priv->max_cs = ret; + priv->cs_count = ret; return 0; } @@ -131,6 +131,11 @@ static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen, debug("%s: slave %s:%u dout %08X din %08X bitlen %u\n", __func__, bus->name, platdata->cs, *(uint *)dout, *(uint *)din, bitlen); + if (platdata->cs >= priv->cs_count) { + dev_err(dev, "chip select index %d too large (cs_count=%d)\n", + platdata->cs, priv->cs_count); + return -EINVAL; + } if (flags & SPI_XFER_BEGIN) mpc8xxx_spi_cs_activate(dev); -- 2.23.0
[PATCH resend 4/5] mpc8xxx_spi: always use 8-bit characters, don't read or write garbage
There are a few problems with the current driver. First, it unconditionally reads from dout/writes to din whether or not those pointers are NULL. So for example a simple "sf probe" ends up writing four bytes at address 0: => md.l 0x0 8 : 45454545 45454545 05050505 05050505 0010: 07070707 07070707 => sf probe 0 mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB53618 din bitlen 8 mpc8xxx_spi_xfer: slave spi@7000:0 dout din 0FB536B8 bitlen 48 SF: Detected s25sl032p with page size 256 Bytes, erase size 64 KiB, total 4 MiB => md.l 0x0 8 : ff00 45454545 05050505 05050505 0010: 07070707 07070707 (here I've change the first debug statement to a printf, and made it print the din/dout pointers rather than the uints they point at). Second, as we can also see above, it always writes a full 32 bits, even if a smaller amount was requested. So for example => mw.l $loadaddr 0xaabbccdd 8 => md.l $loadaddr 8 0200: aabbccdd aabbccdd aabbccdd aabbccdd 0210: aabbccdd aabbccdd aabbccdd aabbccdd => sf read $loadaddr 0x400 6 device 0 offset 0x400, size 0x6 mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din bitlen 40 mpc8xxx_spi_xfer: slave spi@7000:0 dout din 0200 bitlen 48 SF: 6 bytes @ 0x400 Read: OK => sf read 0x0210 0x400 8 device 0 offset 0x400, size 0x8 mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB53848 din bitlen 40 mpc8xxx_spi_xfer: slave spi@7000:0 dout din 0210 bitlen 64 SF: 8 bytes @ 0x400 Read: OK => md.l $loadaddr 8 0200: 45454545 4545 aabbccdd aabbccddEE.. 0210: 45454545 45454545 aabbccdd aabbccdd Finally, when the bitlen is 24 mod 32 (e.g. requesting to read 3 or 7 bytes), the last three bytes and up being the wrong ones, since the driver does a full 32 bit read and then shifts the wrong byte out: => mw.l $loadaddr 0xaabbccdd 4 => md.l $loadaddr 4 0200: aabbccdd aabbccdd aabbccdd aabbccdd => sf read $loadaddr 0x444 10 device 0 offset 0x444, size 0x10 mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din bitlen 40 mpc8xxx_spi_xfer: slave spi@7000:0 dout din 0200 bitlen 128 SF: 16 bytes @ 0x444 Read: OK => md.l $loadaddr 4 0200: 552d426f 6f742032 3031392e 30342d30U-Boot 2019.04-0 => mw.l $loadaddr 0xaabbccdd 4 => sf read $loadaddr 0x444 0xb device 0 offset 0x444, size 0xb mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din bitlen 40 mpc8xxx_spi_xfer: slave spi@7000:0 dout din 0200 bitlen 88 SF: 11 bytes @ 0x444 Read: OK => md.l $loadaddr 4 0200: 552d426f 6f742032 31392e00 aabbccddU-Boot 219.. Fix all of that by always using a character size of 8, and reject transfers that are not a whole number of bytes. While it ends being more work for the CPU, we're mostly bounded by the speed of the SPI bus, and we avoid writing to the mode register in every loop. Based on work by Klaus H. Sørensen. Cc: Klaus H. Sorensen Signed-off-by: Rasmus Villemoes --- drivers/spi/mpc8xxx_spi.c | 80 +-- 1 file changed, 27 insertions(+), 53 deletions(-) diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c index ac4d0a9bae..8ef2451411 100644 --- a/drivers/spi/mpc8xxx_spi.c +++ b/drivers/spi/mpc8xxx_spi.c @@ -27,6 +27,7 @@ enum { SPI_MODE_EN= BIT(31 - 7), /* Enable interface */ SPI_MODE_LEN_MASK = 0xf0, + SPI_MODE_LEN_SHIFT = 20, SPI_MODE_PM_MASK = 0xf, SPI_COM_LST = BIT(31 - 9), @@ -43,23 +44,8 @@ static inline u32 to_prescale_mod(u32 val) return (min(val, (u32)15) << 16); } -static void set_char_len(spi8xxx_t *spi, u32 val) -{ - clrsetbits_be32(>mode, SPI_MODE_LEN_MASK, (val << 20)); -} - #define SPI_TIMEOUT1000 -static int __spi_set_speed(spi8xxx_t *spi, uint speed) -{ - /* TODO(mario@gdsys.cc): This only ever sets one fixed speed */ - - /* Use SYSCLK / 8 (16.67MHz typ.) */ - clrsetbits_be32(>mode, SPI_MODE_PM_MASK, to_prescale_mod(1)); - - return 0; -} - static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev) { struct mpc8xxx_priv *priv = dev_get_priv(dev); @@ -82,14 +68,22 @@ static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev) static int mpc8xxx_spi_probe(struct udevice *dev) { struct mpc8xxx_priv *priv = dev_get_priv(dev); + spi8xxx_t *spi = priv->spi; /* * SPI pins on the MPC83xx are not muxed, so all we do is initialize * some registers */ - out_be32(>spi->mode, SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN); + out_be32(>spi->mode, SPI_MODE_REV | SPI_MODE_MS); + + /* set len to 8 bits */ + setbits_b
[PATCH resend 0/5] spi: mpc8xxx_spi: bug fixes, real ->set_speed and a pseudo-gpio driver
This is a combination of a single patch and a 4-part series sent previously [1,2], this time with Jagan on Cc. Patch 1 is a convenient pseudo-gpio driver for controlling a single output signal on mpc830x. Since it's (usually) used as a chip select, representing it as a gpio (without the gp or i) makes it simple to use in device tree. The remaining four fix bugs in the mpc8xxx_spi driver, most importantly patch 4. Without it, reads and writes of certain lengths from spi-nor fails, and stuff at physical address 0x0 gets overwritten even if no input buffer is supplied (e.g. when sending a command). Tested on an mpc8309-derived board. It would be nice if someone with access to the gazerbeam board can test that this doesn't break that - in particular, the "only do transfers that are multiple of 8 bits" part. [1] https://patchwork.ozlabs.org/patch/1219513/ [2] https://patchwork.ozlabs.org/cover/1218170/ Klaus H. Sorensen (1): gpio/mpc83xx_spisel_boot.c: gpio driver for SPISEL_BOOT signal Rasmus Villemoes (4): gazerbeam: add clocks property to SPI node mpc8xxx_spi: put max_cs to use mpc8xxx_spi: always use 8-bit characters, don't read or write garbage mpc8xxx_spi: implement real ->set_speed arch/powerpc/dts/gdsys/mpc8308.dtsi | 7 + .../gpio/fsl,mpc83xx-spisel-boot.txt | 22 +++ drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 1 + drivers/gpio/mpc83xx_spisel_boot.c| 148 ++ drivers/spi/mpc8xxx_spi.c | 141 ++--- 6 files changed, 267 insertions(+), 60 deletions(-) create mode 100644 doc/device-tree-bindings/gpio/fsl,mpc83xx-spisel-boot.txt create mode 100644 drivers/gpio/mpc83xx_spisel_boot.c -- 2.23.0
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 05/02/2020 18.59, Simon Glass wrote: > Hi Rasmus, > >> This has been lightly tested in the sandbox. I'll add some proper unit >> tests, update the help texts and try to handle the Kconfig issue if >> this is something that might be accepted. >> >> Signed-off-by: Rasmus Villemoes >> --- >> cmd/nvedit.c | 17 - >> 1 file changed, 16 insertions(+), 1 deletion(-) > > Seems OK to me. Thanks. I'll go ahead and write some tests. > I suppose we don't want to implement bash's nested > expansion? ${var${suffix}} It's not that easy to implement inside-out expansion, one needs to juggle a lot of temporary buffers. So I went with the rather simple one-extra-pass, which can mostly achieve the same things (although perhaps the script needs to do a few extra steps). Out of curiosity, what bash version supports the above? $ echo $BASH_VERSION 4.4.20(1)-release $ foo_a=3 $ foo_b=7 $ x=a $ echo ${foo_${x}} bash: ${foo_${x}}: bad substitution however, this variant is supported: $ var=foo_$x $ echo ${var} foo_a $ echo ${!var} 3 > Regards, > Simon > -- Rasmus Villemoes Software Developer Prevas A/S Hedeager 3 DK-8200 Aarhus N +45 51210274 rasmus.villem...@prevas.dk www.prevas.dk
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 11/02/2020 17.30, Wolfgang Denk wrote: > Dear Rasmus Villemoes, > > In message <20200205010812.20373-1-rasmus.villem...@prevas.dk> you wrote: >> Currently, there's no way to fetch the value of an environment >> variable whose name is stored in some other variable, or generated from >> such - in non-working pseudo-code, >> >> ${${varname}} >> ${array${index}} > > HUSH does not support arrays anyway... Of course not, but they can be emulated by having variables foo0, foo1, foo2 and programmatically accessing the variable foo$index, if only there's a way to do that... In a sense, my BOOT_A_LEFT/BOOT_B_LEFT is also just an array with keys "A" and "B". >> This forces some scripts to needlessly duplicate logic and hardcode >> assumptions. For example, in an A/B scheme with three variables >> >> BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated >> BOOT_A_LEFT # 0..3 >> BOOT_B_LEFT # 0..3 >> >> when one needs to determine the slot to boot from, one does something >> like > > Well, there _are_ other ways... Please do tell. How can I avoid code duplication and access a variable whose name I generate by string concatenation/variable interpolation? I.e., I don't want anything like "if test $slot = "A" ; then setenv result BOOT_A_LEFT ; elif test $slot = "B" ; then setenv result BOOT_B_LEFT ; fi", because that doesn't scale. >> This has been lightly tested in the sandbox. I'll add some proper unit >> tests, update the help texts and try to handle the Kconfig issue if >> this is something that might be accepted. > > I'm pretty sure this will blow up in your face in real life, because > if side effects on existing shell scripts even if you don't > expect this. How so? The behaviour is completely opt-in per "env set", so nothing at all changes for existing scripts, and it's only supposed to be used where one would otherwise use some eval-like construct in a "normal" shell. So env set -E result "\${BOOT_${x}_LEFT}" corresponds to eval "result=\${BOOT_${x}_LEFT}" And just as "eval" is used sparingly in shell scripts, I expect "env set -E" to be used only when necessary. > This needs _lots_ of testing with existing code / > scripts. I'm not proposing changing semantics of existing scripts at all. Thanks, Rasmus
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 18/02/2020 09.11, Rasmus Villemoes wrote: > Thanks, Wolfgang. Consider both "env set -E" and the alternative "env > get" withdrawn. So, if I wanted to change the status of such a patch in patchwork, what would be the appropriate status? There's no "Withdrawn" or "Retracted". So "Not applicable" or "Rejected"? Rasmus
Re: [PATCH] RFC: nvedit: support doing one (extra) expansion of the value in "env set"
On 14/02/2020 12.54, Rasmus Villemoes wrote: > Now we can work around the lack of break in the busybox shell by writing ^ > But we still can't translate this to busybox shell, because there's no ^ I of course meant "U-Boot shell", sorry about that. Rasmus
[PATCH] sandbox: also restore terminal settings when killed by SIGINT
Hitting Ctrl-C is a documented way to exit the sandbox, but it is not actually equivalent to the reset command. The latter, since it follows normal process exit, takes care to reset terminal settings and restoring the O_NONBLOCK behaviour of stdin (and, in a terminal, that is usually the same file description as stdout and stderr, i.e. some /dev/pts/NN). Failure to restore (remove) O_NONBLOCK from stdout/stderr can cause very surprising and hard to debug problems back in the terminal. For example, I had "make -j8" consistently failing without much information about just exactly what went wrong, but sometimes I did get a "echo: write error". I was at first afraid my disk was getting bad, but then a simple "dmesg" _also_ failed with write error - so it was writing to the terminal that was buggered. And both "make -j8" and dmesg in another terminal window worked just fine. So install a SIGINT handler so that if the chosen terminal mode (cooked or raw-with-sigs) means Ctrl-C sends a SIGINT, we will still call os_fd_restore(), then reraise the signal and die as usual from SIGINT. Before: $ grep flags /proc/$$/fdinfo/1 flags: 0102002 $ ./u-boot # hit Ctrl-C $ grep flags /proc/$$/fdinfo/1 flags: 0106002 After: $ grep flags /proc/$$/fdinfo/1 flags: 0102002 $ ./u-boot # hit Ctrl-C $ grep flags /proc/$$/fdinfo/1 flags: 0102002 Signed-off-by: Rasmus Villemoes --- arch/sandbox/cpu/os.c | 9 + 1 file changed, 9 insertions(+) diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index f7c73e3a0b..e7ec892bdf 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -175,6 +176,13 @@ void os_fd_restore(void) } } +static void os_sigint_handler(int sig) +{ + os_fd_restore(); + signal(SIGINT, SIG_DFL); + raise(SIGINT); +} + /* Put tty into raw mode so and work */ void os_tty_raw(int fd, bool allow_sigs) { @@ -205,6 +213,7 @@ void os_tty_raw(int fd, bool allow_sigs) term_setup = true; atexit(os_fd_restore); + signal(SIGINT, os_sigint_handler); } void *os_malloc(size_t length) -- 2.23.0
Re: [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
On 20/02/2020 07.43, Stefan Roese wrote: > On 20.02.20 07:38, Christophe Leroy wrote: > > > > +void watchdog_reset(void) > +{ > + static ulong next_reset; > + ulong now; > + > + /* Exit if GD is not ready or watchdog is not initialized yet */ > + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) > + return; > + > + /* Do not reset the watchdog too often */ > + now = get_timer(0); > + if (now > next_reset) { > + next_reset = now + 1000; /* reset every 1000ms */ > + wdt_reset(gd->watchdog_dev); > + } This is a problem for the MPC8xx. When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable. >>> >>> So you need a shorted delay between the wdt_reset() calls? Is this >>> correct? We could introduce a new Kconfig option which defaults to >>> 1000 (ms) and you can "select" a shorter value for MPC8xx. >> >> Exactly. However, why is this limitation needed at all ? Why is it a >> problem to refresh more often ? > > Very likely its not. What is a reasonable value for your platform? 100 > or 500ms? I think we could change it to default to a shorter value, but > such a change should go in early in the merge window, so that other > platforms have a bit of time to test it. > > Please feel free to send a patch for this and please add a comment to > explain, why the delay is this "short". IMO, this should come from DT. For a gpio watchdog (which for some reason U-Boot doesn't have a generic driver for) the linux kernel uses a hw_margin_ms property that tells the core how often the watchdog must be pinged - that could be generalized to apply for all, with 1000ms as a default if not set. And I've seen boards with a gpio watchdog with a timeout of 200 ms. Also, I'm wondering why that generic _reset only handles one watchdog device? I can easily imagine needing to reset both, say, an external gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop over all DM watchdogs, and have the next_reset/hw_margin etc. metadata live with the watchdog device instead of in static variable/build-time constants? Rasmus
[PATCH 5/5] env/sf.c: drop private CMD_SAVEENV logic
Deciding whether to compile the env_sf_save() function based solely on CONFIG_SPL_BUILD is wrong: For U-Boot proper, it leads to a build warning in case CONFIG_CMD_SAVEENV=n (because the env_save_ptr() macro causes the function to indeed not be referenced anywhere). And for SPL, when one selects CONFIG_SPL_SAVEENV, one obviously expects to actually be able to save the environment. Signed-off-by: Rasmus Villemoes --- env/sf.c | 12 +--- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/env/sf.c b/env/sf.c index 5ef4055219..22b70ad319 100644 --- a/env/sf.c +++ b/env/sf.c @@ -21,16 +21,12 @@ #include #ifndef CONFIG_SPL_BUILD -#define CMD_SAVEENV #define INITENV #endif #ifdef CONFIG_ENV_OFFSET_REDUND -#ifdef CMD_SAVEENV static ulong env_offset= CONFIG_ENV_OFFSET; static ulong env_new_offset= CONFIG_ENV_OFFSET_REDUND; -#endif - #endif /* CONFIG_ENV_OFFSET_REDUND */ DECLARE_GLOBAL_DATA_PTR; @@ -69,7 +65,6 @@ static int setup_flash_device(void) } #if defined(CONFIG_ENV_OFFSET_REDUND) -#ifdef CMD_SAVEENV static int env_sf_save(void) { env_t env_new; @@ -148,7 +143,6 @@ static int env_sf_save(void) return ret; } -#endif /* CMD_SAVEENV */ static int env_sf_load(void) { @@ -187,7 +181,6 @@ out: return ret; } #else -#ifdef CMD_SAVEENV static int env_sf_save(void) { u32 saved_size, saved_offset, sector; @@ -247,7 +240,6 @@ static int env_sf_save(void) return ret; } -#endif /* CMD_SAVEENV */ static int env_sf_load(void) { @@ -313,9 +305,7 @@ U_BOOT_ENV_LOCATION(sf) = { .location = ENVL_SPI_FLASH, ENV_NAME("SPI Flash") .load = env_sf_load, -#ifdef CMD_SAVEENV - .save = env_save_ptr(env_sf_save), -#endif + .save = ENV_SAVE_PTR(env_sf_save), #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) .init = env_sf_init, #endif -- 2.23.0
[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic
Always compile the env_fat_save() function, and let CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether it actually ends up being compiled in. Signed-off-by: Rasmus Villemoes --- env/fat.c | 9 + 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/env/fat.c b/env/fat.c index 1836556f36..cf2e5e2b26 100644 --- a/env/fat.c +++ b/env/fat.c @@ -26,12 +26,8 @@ # endif #else # define LOADENV -# if defined(CONFIG_CMD_SAVEENV) -# define CMD_SAVEENV -# endif #endif -#ifdef CMD_SAVEENV static int env_fat_save(void) { env_t __aligned(ARCH_DMA_MINALIGN) env_new; @@ -76,7 +72,6 @@ static int env_fat_save(void) return 0; } -#endif /* CMD_SAVEENV */ #ifdef LOADENV static int env_fat_load(void) @@ -135,7 +130,5 @@ U_BOOT_ENV_LOCATION(fat) = { #ifdef LOADENV .load = env_fat_load, #endif -#ifdef CMD_SAVEENV - .save = env_save_ptr(env_fat_save), -#endif + .save = ENV_SAVE_PTR(env_fat_save), }; -- 2.23.0
[PATCH 1/5] env: add SAVEENV as an alias of the CMD_SAVEENV symbol
Currently, testing whether to compile in support for saving the environment is a bit awkward when one needs to take SPL_SAVEENV into account, and quite a few storage drivers currently do not honour SPL_SAVEENV. To make it a bit easier to decide whether environment saving should be enabled, introduce SAVEENV as an alias for the CMD_SAVEENV symbol. Then one can simply use CONFIG_IS_ENABLED(SAVEENV) Signed-off-by: Rasmus Villemoes --- env/Kconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/env/Kconfig b/env/Kconfig index 0d6f559b39..969308fe6c 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -3,6 +3,9 @@ menu "Environment" config ENV_SUPPORT def_bool y +config SAVEENV + def_bool y if CMD_SAVEENV + config ENV_IS_NOWHERE bool "Environment is not stored" default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \ -- 2.23.0
[PATCH 4/5] env/ext4.c: remove CONFIG_CMD_SAVEENV ifdef
Removing this ifdef/endif pair yields a "defined but unused warning" for CONFIG_CMD_SAVEENV=n, but that vanishes if we use the ENV_SAVE_PTR macro instead. This gives slightly better compile testing, and moreover, it's possible to have CONFIG_CMD_SAVEENV=n CONFIG_SPL_SAVEENV=y SPL_ENV_IS_IN_EXT4=y in which case env_ext4_save would erroneously not be compiled in. Signed-off-by: Rasmus Villemoes --- env/ext4.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/env/ext4.c b/env/ext4.c index 1f6b1b5bd8..911e19c6d3 100644 --- a/env/ext4.c +++ b/env/ext4.c @@ -41,7 +41,6 @@ __weak const char *env_ext4_get_dev_part(void) return (const char *)CONFIG_ENV_EXT4_DEVICE_AND_PART; } -#ifdef CONFIG_CMD_SAVEENV static int env_ext4_save(void) { env_t env_new; @@ -83,7 +82,6 @@ static int env_ext4_save(void) puts("done\n"); return 0; } -#endif /* CONFIG_CMD_SAVEENV */ static int env_ext4_load(void) { @@ -137,5 +135,5 @@ U_BOOT_ENV_LOCATION(ext4) = { .location = ENVL_EXT4, ENV_NAME("EXT4") .load = env_ext4_load, - .save = env_save_ptr(env_ext4_save), + .save = ENV_SAVE_PTR(env_ext4_save), }; -- 2.23.0
[PATCH 2/5] env_internal.h: add alternative ENV_SAVE_PTR macro
The current definition of the env_save_ptr does not take SPL_SAVEENV into account. Moreover, the way it is implemented means that drivers need to guard the definitions of their _save methods with ifdefs to avoid "defined but unused" warnings in case CMD_SAVEENV=n. The ifdeffery can be avoided by using a "something ? x : NULL" construction instead and still have the compiler elide the _save method when it is not referenced. Unfortunately we can't just switch the existing env_save_ptr macro, since that would give a lot of build errors unless all the ifdeffery is removed at the same time. Conversely, removing that ifdeffery first would merely lead to the "defined but unused" warnings temporarily, but for some storage drivers it requires a bit more work than just removing their private CMD_SAVEENV logic. So introduce an alternative to env_save_ptr, which for lack of a better name is simply uppercased, allowing one to update storage drivers piecemeal to both reduce their ifdeffery and honour CONFIG_SPL_SAVEENV. Signed-off-by: Rasmus Villemoes --- include/env_internal.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/env_internal.h b/include/env_internal.h index 90a4df8a72..e89fbdb1b7 100644 --- a/include/env_internal.h +++ b/include/env_internal.h @@ -207,6 +207,8 @@ struct env_driver { #define env_save_ptr(x) NULL #endif +#define ENV_SAVE_PTR(x) (CONFIG_IS_ENABLED(SAVEENV) ? (x) : NULL) + extern struct hsearch_data env_htab; #endif /* DO_DEPS_ONLY */ -- 2.23.0
[PATCH 0/5] CMD_SAVEENV ifdef cleanup
The various env storage drivers almost all have their own logic [1] for deciding whether to compile and provide the .save method, many of which fail to honour CONFIG_SPL_SAVEENV. For example, fat.c and sf.c define a CMD_SAVEENV macro only for !CONFIG_SPL_BUILD, while ext4.c "only" depends on CONFIG_CMD_SAVEENV - but CONFIG_SPL_SAVEENV=y, CONFIG_CMD_SAVEENV=n is a valid combination. A lot of that ifdeffery can be removed while at the same time providing the .save method if either CONFIG_SPL_SAVEENV (for an SPL build) or CONFIG_CMD_SAVEENV (for U-Boot proper) is set. The first two patches introduce infrastructure for that, while the last three are example conversions for the above-mentioned three storage drivers. The sf.c is the one I need to use in the SPL and have actually tested, ext4.c and fat.c are included mostly as low-hanging fruit. [1] Here's the current conditions for which these three drivers provide .save: SPL U-Boot ext4.cCONFIG_CMD_SAVEENV=yCONFIG_CMD_SAVEENV=y fat.c never CONFIG_CMD_SAVEENV=y sf.c never CONFIG_CMD_SAVEENV=y [2] [2] It always compiles env_sf_save for U-Boot proper, but then the use of env_save_ptr() ends up with a build warning in case CONFIG_CMD_SAVEENV=n - fat.c doesn't have that proplem. Rasmus Villemoes (5): env: add SAVEENV as an alias of the CMD_SAVEENV symbol env_internal.h: add alternative ENV_SAVE_PTR macro env/fat.c: remove private CMD_SAVEENV logic env/ext4.c: remove CONFIG_CMD_SAVEENV ifdef env/sf.c: drop private CMD_SAVEENV logic env/Kconfig| 3 +++ env/ext4.c | 4 +--- env/fat.c | 9 + env/sf.c | 12 +--- include/env_internal.h | 2 ++ 5 files changed, 8 insertions(+), 22 deletions(-) -- 2.23.0