[U-Boot] [PATCH] fix always succesful memory test

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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 Majewski 

Thanks.

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

[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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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)

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

[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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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"

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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

[Rasmus Villemoes]

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