[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[Vladimir Olovyannikov]

cmd: adding malloc, math, and strcmp u-boot commands.
- malloc  supports allocation of heap memory and free allocated memory
          via u-boot command line.
- math    provides math commands such as "add", "sub", "mul", "div",
          "shift", ability to convert dec->hex.
- strcmp  provides string compare command feature for a script.

All these commands are introduced to be used in u-boot scripts.

Signed-off-by: Suji Velupiallai <suji.velupillai@broadcom.com>
Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
---
 cmd/Kconfig  | 21 ++++++++++++++
 cmd/Makefile |  4 +++
 cmd/malloc.c | 54 ++++++++++++++++++++++++++++++++++++
 cmd/math.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 cmd/strcmp.c | 28 +++++++++++++++++++
 5 files changed, 185 insertions(+)
 create mode 100644 cmd/malloc.c
 create mode 100644 cmd/math.c
 create mode 100644 cmd/strcmp.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index cf982ff65e..f11903fe3d 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1286,6 +1286,20 @@ config CMD_ITEST
 	help
 	  Return true/false on integer compare.
 
+config CMD_MALLOC
+	bool "malloc"
+	default y
+	help
+	  Supports allocation of heap memory and free allocated memory commands.
+	  These commands are used by u-boot scripts.
+
+config CMD_MATH
+	bool "math"
+	default y
+	help
+	  Provides math commands such as add, sub, mul, div, shift,
+	  convert decimal to hex functionalities to be available in the script.
+
 config CMD_SOURCE
 	bool "source"
 	default y
@@ -1301,6 +1315,13 @@ config CMD_SETEXPR
 	  Also supports loading the value at a memory location into a variable.
 	  If CONFIG_REGEX is enabled, setexpr also supports a gsub function.
 
+config CMD_STRCMP
+	bool "strcmp"
+	default y
+	help
+	  Provides string compare command feature to u-boot scripts.
+
+
 endmenu
 
 menu "Android support commands"
diff --git a/cmd/Makefile b/cmd/Makefile
index 2d723ea0f0..942d60a0a2 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -164,6 +164,10 @@ obj-$(CONFIG_CMD_GPT) += gpt.o
 obj-$(CONFIG_CMD_ETHSW) += ethsw.o
 obj-$(CONFIG_CMD_AXI) += axi.o
 
+obj-$(CONFIG_CMD_MALLOC) += malloc.o
+obj-$(CONFIG_CMD_MATH) += math.o
+obj-$(CONFIG_CMD_STRCMP) += strcmp.o
+
 # Power
 obj-$(CONFIG_CMD_PMIC) += pmic.o
 obj-$(CONFIG_CMD_REGULATOR) += regulator.o
diff --git a/cmd/malloc.c b/cmd/malloc.c
new file mode 100644
index 0000000000..e11e030a59
--- /dev/null
+++ b/cmd/malloc.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 Broadcom
+ */
+
+#include <common.h>
+#include <command.h>
+#include <environment.h>
+#include <errno.h>
+#include <malloc.h>
+
+static unsigned long get_value(const char *val)
+{
+	char *env = env_get((char *)val);
+
+	if (env)
+		return simple_strtoul(env, NULL, 16);
+	else
+		return simple_strtoul(val, NULL, 16);
+}
+
+static int do_malloc(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
+{
+	char numberbuf[32];
+	void *mem;
+
+	if (argc < 3)
+		return cmd_usage(cmdtp);
+
+	mem = memalign(ARCH_DMA_MINALIGN, get_value(argv[2]));
+	if (mem) {
+		sprintf(numberbuf, "%08x", (unsigned int)mem);
+		env_set(argv[1], numberbuf);
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int do_free(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
+{
+	if (argc < 2)
+		return cmd_usage(cmdtp);
+
+	free((void *)get_value(argv[1]));
+	env_set(argv[1], "");
+	return 0;
+}
+
+U_BOOT_CMD(malloc, 3, 0, do_malloc,
+	   "Allocate memory from u-boot heap and store pointer in environment variable.",
+	   "target size\n");
+
+U_BOOT_CMD(free, 2, 0, do_free,
+	   "Release memory from u-boot heap@target.", "target\n");
diff --git a/cmd/math.c b/cmd/math.c
new file mode 100644
index 0000000000..17de5ef70b
--- /dev/null
+++ b/cmd/math.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2010-2017 Broadcom
+ */
+
+#include <common.h>
+#include <command.h>
+#include <environment.h>
+
+unsigned long long simple_strtoull(const char *cp, char **endp,
+				   unsigned int base);
+
+static unsigned long long get_value(const char *val)
+{
+	char *env = env_get((char *)val);
+
+	if (env)
+		return simple_strtoull(env, NULL, 16);
+	else
+		return simple_strtoull(val, NULL, 16);
+}
+
+static unsigned long long get_value_base10(const char *val)
+{
+	char *env = env_get((char *)val);
+
+	if (env)
+		return simple_strtoull(env, NULL, 10);
+	else
+		return simple_strtoull(val, NULL, 10);
+}
+
+/*
+ * Top level addenv command
+ */
+#define IS_MATH_CMD(cmd, args) ((strcmp(argv[1], cmd) == 0) && \
+				  (argc == (args)))
+
+static int do_math(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	char numberbuf[32];
+	unsigned long long i = 0;
+
+	if (IS_MATH_CMD("add", 5))
+		i = get_value(argv[3]) + get_value(argv[4]);
+	else if (IS_MATH_CMD("sub", 5))
+		i = get_value(argv[3]) - get_value(argv[4]);
+	else if (IS_MATH_CMD("mul", 5))
+		i = get_value(argv[3]) * get_value(argv[4]);
+	else if (IS_MATH_CMD("div", 5))
+		i = get_value(argv[3]) / get_value(argv[4]);
+	else if (IS_MATH_CMD("shl", 5))
+		i = get_value(argv[3]) << get_value(argv[4]);
+	else if (IS_MATH_CMD("d2h", 4))
+		i = get_value_base10(argv[3]);
+	else
+		return cmd_usage(cmdtp);
+
+	sprintf(numberbuf, "%llx", i);
+	env_set(argv[2], numberbuf);
+	return 0;
+}
+
+U_BOOT_CMD(math, 5, 0, do_math,
+	   "Environment variable math.",
+	   "add a b c\n"
+	   "    - add b to c and store in a.\n"
+	   "math sub a b c\n"
+	   "    - subtract b from c and store in a.\n"
+	   "math mul a b c\n"
+	   "    - multiply b by c and store in a.\n"
+	   "math div a b c\n"
+	   "    - divide b by c and store in a.\n"
+	   "math shl a b c\n"
+	   "    - shift left b by c and store in a.\n"
+	   "math d2h a b\n"
+	   "    - Convert b from decimal to hex and store in a.\n"
+);
diff --git a/cmd/strcmp.c b/cmd/strcmp.c
new file mode 100644
index 0000000000..3abd270d40
--- /dev/null
+++ b/cmd/strcmp.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2010-2017 Broadcom
+ */
+
+#include <common.h>
+#include <command.h>
+
+static int do_strcmp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	if (argc != 3)
+		return cmd_usage(cmdtp);
+
+	if (strlen(argv[1]) != strlen(argv[2]))
+		return CMD_RET_FAILURE;
+
+	/* Compare the temporary string to the given parameter */
+	if (!strncmp(argv[1], argv[2], strlen(argv[2])))
+		return CMD_RET_SUCCESS;
+
+	return CMD_RET_FAILURE;
+}
+
+U_BOOT_CMD(strcmp, 3, 0, do_strcmp,
+	   "String to string comparison.",
+	   "[str] [str]\n"
+	   "    - Returns true if str are same, else returns false.\n"
+);
-- 
2.17.1

[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[Simon Glass]

Hi Vladimir,

On Mon, 18 Nov 2019 at 16:27, Vladimir Olovyannikov
<vladimir.olovyannikov@broadcom.com> wrote:
>
> cmd: adding malloc, math, and strcmp u-boot commands.

U-Boot

> - malloc  supports allocation of heap memory and free allocated memory
>           via u-boot command line.

U-Boot (please fix globally)

> - math    provides math commands such as "add", "sub", "mul", "div",
>           "shift", ability to convert dec->hex.
> - strcmp  provides string compare command feature for a script.
>
> All these commands are introduced to be used in u-boot scripts.
>
> Signed-off-by: Suji Velupiallai <suji.velupillai@broadcom.com>
> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> ---
>  cmd/Kconfig  | 21 ++++++++++++++
>  cmd/Makefile |  4 +++
>  cmd/malloc.c | 54 ++++++++++++++++++++++++++++++++++++
>  cmd/math.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  cmd/strcmp.c | 28 +++++++++++++++++++
>  5 files changed, 185 insertions(+)
>  create mode 100644 cmd/malloc.c
>  create mode 100644 cmd/math.c
>  create mode 100644 cmd/strcmp.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index cf982ff65e..f11903fe3d 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1286,6 +1286,20 @@ config CMD_ITEST
>         help
>           Return true/false on integer compare.
>
> +config CMD_MALLOC
> +       bool "malloc"
> +       default y
> +       help
> +         Supports allocation of heap memory and free allocated memory commands.
> +         These commands are used by u-boot scripts.
> +
> +config CMD_MATH
> +       bool "math"
> +       default y
> +       help
> +         Provides math commands such as add, sub, mul, div, shift,
> +         convert decimal to hex functionalities to be available in the script.
> +
>  config CMD_SOURCE
>         bool "source"
>         default y
> @@ -1301,6 +1315,13 @@ config CMD_SETEXPR
>           Also supports loading the value at a memory location into a variable.
>           If CONFIG_REGEX is enabled, setexpr also supports a gsub function.

I think this would be better as three patches.

>
> +config CMD_STRCMP
> +       bool "strcmp"
> +       default y
> +       help
> +         Provides string compare command feature to u-boot scripts.

U-Boot

> +
> +
>  endmenu
>
>  menu "Android support commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 2d723ea0f0..942d60a0a2 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -164,6 +164,10 @@ obj-$(CONFIG_CMD_GPT) += gpt.o
>  obj-$(CONFIG_CMD_ETHSW) += ethsw.o
>  obj-$(CONFIG_CMD_AXI) += axi.o
>
> +obj-$(CONFIG_CMD_MALLOC) += malloc.o
> +obj-$(CONFIG_CMD_MATH) += math.o
> +obj-$(CONFIG_CMD_STRCMP) += strcmp.o
> +
>  # Power
>  obj-$(CONFIG_CMD_PMIC) += pmic.o
>  obj-$(CONFIG_CMD_REGULATOR) += regulator.o
> diff --git a/cmd/malloc.c b/cmd/malloc.c
> new file mode 100644
> index 0000000000..e11e030a59
> --- /dev/null
> +++ b/cmd/malloc.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019 Broadcom
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <environment.h>
> +#include <errno.h>
> +#include <malloc.h>
> +
> +static unsigned long get_value(const char *val)

Needs a comment

> +{
> +       char *env = env_get((char *)val);
> +
> +       if (env)
> +               return simple_strtoul(env, NULL, 16);
> +       else
> +               return simple_strtoul(val, NULL, 16);
> +}
> +
> +static int do_malloc(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +       char numberbuf[32];
> +       void *mem;

const?

> +
> +       if (argc < 3)
> +               return cmd_usage(cmdtp);
> +
> +       mem = memalign(ARCH_DMA_MINALIGN, get_value(argv[2]));
> +       if (mem) {
> +               sprintf(numberbuf, "%08x", (unsigned int)mem);

I think (ulong) would be better

> +               env_set(argv[1], numberbuf);

blank line before return (please fix globally)

> +               return 0;
> +       }
> +       return -EINVAL;

You can't return errors from command functions. Try printing an error
and return CMD_RET_FAILURE instead.

> +}
> +
> +static int do_free(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +       if (argc < 2)
> +               return cmd_usage(cmdtp);
> +
> +       free((void *)get_value(argv[1]));
> +       env_set(argv[1], "");
> +       return 0;
> +}
> +
> +U_BOOT_CMD(malloc, 3, 0, do_malloc,
> +          "Allocate memory from u-boot heap and store pointer in environment variable.",
> +          "target size\n");

Needs better help - e.g. list arguments

> +
> +U_BOOT_CMD(free, 2, 0, do_free,
> +          "Release memory from u-boot heap at target.", "target\n");

I wonder if it would be better to have a 'mem' command, then have 'mem
alloc', 'mem free', etc.?

> diff --git a/cmd/math.c b/cmd/math.c
> new file mode 100644
> index 0000000000..17de5ef70b
> --- /dev/null
> +++ b/cmd/math.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2010-2017 Broadcom
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <environment.h>
> +
> +unsigned long long simple_strtoull(const char *cp, char **endp,
> +                                  unsigned int base);

Use #include <vsprintf.h> instead

> +
> +static unsigned long long get_value(const char *val)

Function comment

> +{
> +       char *env = env_get((char *)val);
> +
> +       if (env)
> +               return simple_strtoull(env, NULL, 16);
> +       else
> +               return simple_strtoull(val, NULL, 16);
> +}
> +
> +static unsigned long long get_value_base10(const char *val)
> +{
> +       char *env = env_get((char *)val);
> +
> +       if (env)
> +               return simple_strtoull(env, NULL, 10);
> +       else
> +               return simple_strtoull(val, NULL, 10);
> +}
> +
> +/*
> + * Top level addenv command
> + */
> +#define IS_MATH_CMD(cmd, args) ((strcmp(argv[1], cmd) == 0) && \
> +                                 (argc == (args)))

Use a function instead of a macro.

> +
> +static int do_math(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       char numberbuf[32];
> +       unsigned long long i = 0;
> +
> +       if (IS_MATH_CMD("add", 5))
> +               i = get_value(argv[3]) + get_value(argv[4]);

It might be better to do something like:

const char *left = NULL, *right = NULL;

if (argc > 3)
   left = argv[3];
...

Then you can use 'left' and 'right' below.

> +       else if (IS_MATH_CMD("sub", 5))
> +               i = get_value(argv[3]) - get_value(argv[4]);
> +       else if (IS_MATH_CMD("mul", 5))
> +               i = get_value(argv[3]) * get_value(argv[4]);
> +       else if (IS_MATH_CMD("div", 5))
> +               i = get_value(argv[3]) / get_value(argv[4]);
> +       else if (IS_MATH_CMD("shl", 5))
> +               i = get_value(argv[3]) << get_value(argv[4]);
> +       else if (IS_MATH_CMD("d2h", 4))
> +               i = get_value_base10(argv[3]);
> +       else
> +               return cmd_usage(cmdtp);
> +
> +       sprintf(numberbuf, "%llx", i);
> +       env_set(argv[2], numberbuf);
> +       return 0;
> +}
> +
> +U_BOOT_CMD(math, 5, 0, do_math,
> +          "Environment variable math.",
> +          "add a b c\n"
> +          "    - add b to c and store in a.\n"

But also b and c can be numbers, right? You should mention that.

> +          "math sub a b c\n"
> +          "    - subtract b from c and store in a.\n"
> +          "math mul a b c\n"
> +          "    - multiply b by c and store in a.\n"
> +          "math div a b c\n"
> +          "    - divide b by c and store in a.\n"
> +          "math shl a b c\n"
> +          "    - shift left b by c and store in a.\n"
> +          "math d2h a b\n"
> +          "    - Convert b from decimal to hex and store in a.\n"
> +);
> diff --git a/cmd/strcmp.c b/cmd/strcmp.c
> new file mode 100644
> index 0000000000..3abd270d40
> --- /dev/null
> +++ b/cmd/strcmp.c
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2010-2017 Broadcom
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +
> +static int do_strcmp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       if (argc != 3)
> +               return cmd_usage(cmdtp);
> +
> +       if (strlen(argv[1]) != strlen(argv[2]))
> +               return CMD_RET_FAILURE;
> +
> +       /* Compare the temporary string to the given parameter */
> +       if (!strncmp(argv[1], argv[2], strlen(argv[2])))
> +               return CMD_RET_SUCCESS;
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +U_BOOT_CMD(strcmp, 3, 0, do_strcmp,
> +          "String to string comparison.",
> +          "[str] [str]\n"

str1 str2, I think

> +          "    - Returns true if str are same, else returns false.\n"
> +);
> --
> 2.17.1
>

Regards,
Simon

[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[Vladimir Olovyannikov]

Hi Simon,
Thank you for reviewing. I will split the patch as you suggested, and
resubmit.

Vladimir

> -----Original Message-----
> From: Simon Glass [mailto:sjg at chromium.org]
> Sent: Monday, November 18, 2019 5:21 PM
> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Heinrich Schuchardt
> <xypron.glpk@gmx.de>; Suji Velupiallai <suji.velupillai@broadcom.com>
> Subject: Re: [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands
> to u-boot
>
> Hi Vladimir,
>
> On Mon, 18 Nov 2019 at 16:27, Vladimir Olovyannikov
> <vladimir.olovyannikov@broadcom.com> wrote:
> >
> > cmd: adding malloc, math, and strcmp u-boot commands.
>
> U-Boot
>
> > - malloc  supports allocation of heap memory and free allocated memory
> >           via u-boot command line.
>
> U-Boot (please fix globally)
>
> > - math    provides math commands such as "add", "sub", "mul", "div",
> >           "shift", ability to convert dec->hex.
> > - strcmp  provides string compare command feature for a script.
> >
> > All these commands are introduced to be used in u-boot scripts.
> >
> > Signed-off-by: Suji Velupiallai <suji.velupillai@broadcom.com>
> > Signed-off-by: Vladimir Olovyannikov
> > <vladimir.olovyannikov@broadcom.com>
> > ---
> >  cmd/Kconfig  | 21 ++++++++++++++
> >  cmd/Makefile |  4 +++
> >  cmd/malloc.c | 54 ++++++++++++++++++++++++++++++++++++
> >  cmd/math.c   | 78
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  cmd/strcmp.c | 28 +++++++++++++++++++
> >  5 files changed, 185 insertions(+)
> >  create mode 100644 cmd/malloc.c
> >  create mode 100644 cmd/math.c
> >  create mode 100644 cmd/strcmp.c
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig index cf982ff65e..f11903fe3d
> > 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1286,6 +1286,20 @@ config CMD_ITEST
> >         help
> >           Return true/false on integer compare.
> >
> > +config CMD_MALLOC
> > +       bool "malloc"
> > +       default y
> > +       help
> > +         Supports allocation of heap memory and free allocated memory
> commands.
> > +         These commands are used by u-boot scripts.
> > +
> > +config CMD_MATH
> > +       bool "math"
> > +       default y
> > +       help
> > +         Provides math commands such as add, sub, mul, div, shift,
> > +         convert decimal to hex functionalities to be available in the
> > script.
> > +
> >  config CMD_SOURCE
> >         bool "source"
> >         default y
> > @@ -1301,6 +1315,13 @@ config CMD_SETEXPR
> >           Also supports loading the value at a memory location into a
> > variable.
> >           If CONFIG_REGEX is enabled, setexpr also supports a gsub
> > function.
>
> I think this would be better as three patches.
>
> >
> > +config CMD_STRCMP
> > +       bool "strcmp"
> > +       default y
> > +       help
> > +         Provides string compare command feature to u-boot scripts.
>
> U-Boot
>
> > +
> > +
> >  endmenu
> >
> >  menu "Android support commands"
> > diff --git a/cmd/Makefile b/cmd/Makefile index 2d723ea0f0..942d60a0a2
> > 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -164,6 +164,10 @@ obj-$(CONFIG_CMD_GPT) += gpt.o
> >  obj-$(CONFIG_CMD_ETHSW) += ethsw.o
> >  obj-$(CONFIG_CMD_AXI) += axi.o
> >
> > +obj-$(CONFIG_CMD_MALLOC) += malloc.o
> > +obj-$(CONFIG_CMD_MATH) += math.o
> > +obj-$(CONFIG_CMD_STRCMP) += strcmp.o
> > +
> >  # Power
> >  obj-$(CONFIG_CMD_PMIC) += pmic.o
> >  obj-$(CONFIG_CMD_REGULATOR) += regulator.o diff --git a/cmd/malloc.c
> > b/cmd/malloc.c new file mode 100644 index 0000000000..e11e030a59
> > --- /dev/null
> > +++ b/cmd/malloc.c
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019 Broadcom
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <environment.h>
> > +#include <errno.h>
> > +#include <malloc.h>
> > +
> > +static unsigned long get_value(const char *val)
>
> Needs a comment
>
> > +{
> > +       char *env = env_get((char *)val);
> > +
> > +       if (env)
> > +               return simple_strtoul(env, NULL, 16);
> > +       else
> > +               return simple_strtoul(val, NULL, 16); }
> > +
> > +static int do_malloc(cmd_tbl_t *cmdtp, int flag, int argc, char
> > +*const argv[]) {
> > +       char numberbuf[32];
> > +       void *mem;
>
> const?
>
> > +
> > +       if (argc < 3)
> > +               return cmd_usage(cmdtp);
> > +
> > +       mem = memalign(ARCH_DMA_MINALIGN, get_value(argv[2]));
> > +       if (mem) {
> > +               sprintf(numberbuf, "%08x", (unsigned int)mem);
>
> I think (ulong) would be better
>
> > +               env_set(argv[1], numberbuf);
>
> blank line before return (please fix globally)
>
> > +               return 0;
> > +       }
> > +       return -EINVAL;
>
> You can't return errors from command functions. Try printing an error and
> return CMD_RET_FAILURE instead.
>
> > +}
> > +
> > +static int do_free(cmd_tbl_t *cmdtp, int flag, int argc, char *const
> > +argv[]) {
> > +       if (argc < 2)
> > +               return cmd_usage(cmdtp);
> > +
> > +       free((void *)get_value(argv[1]));
> > +       env_set(argv[1], "");
> > +       return 0;
> > +}
> > +
> > +U_BOOT_CMD(malloc, 3, 0, do_malloc,
> > +          "Allocate memory from u-boot heap and store pointer in
> environment variable.",
> > +          "target size\n");
>
> Needs better help - e.g. list arguments
>
> > +
> > +U_BOOT_CMD(free, 2, 0, do_free,
> > +          "Release memory from u-boot heap at target.", "target\n");
>
> I wonder if it would be better to have a 'mem' command, then have 'mem
> alloc', 'mem free', etc.?
>
> > diff --git a/cmd/math.c b/cmd/math.c
> > new file mode 100644
> > index 0000000000..17de5ef70b
> > --- /dev/null
> > +++ b/cmd/math.c
> > @@ -0,0 +1,78 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2010-2017 Broadcom
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <environment.h>
> > +
> > +unsigned long long simple_strtoull(const char *cp, char **endp,
> > +                                  unsigned int base);
>
> Use #include <vsprintf.h> instead
>
> > +
> > +static unsigned long long get_value(const char *val)
>
> Function comment
>
> > +{
> > +       char *env = env_get((char *)val);
> > +
> > +       if (env)
> > +               return simple_strtoull(env, NULL, 16);
> > +       else
> > +               return simple_strtoull(val, NULL, 16); }
> > +
> > +static unsigned long long get_value_base10(const char *val) {
> > +       char *env = env_get((char *)val);
> > +
> > +       if (env)
> > +               return simple_strtoull(env, NULL, 10);
> > +       else
> > +               return simple_strtoull(val, NULL, 10); }
> > +
> > +/*
> > + * Top level addenv command
> > + */
> > +#define IS_MATH_CMD(cmd, args) ((strcmp(argv[1], cmd) == 0) && \
> > +                                 (argc == (args)))
>
> Use a function instead of a macro.
>
> > +
> > +static int do_math(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > +argv[]) {
> > +       char numberbuf[32];
> > +       unsigned long long i = 0;
> > +
> > +       if (IS_MATH_CMD("add", 5))
> > +               i = get_value(argv[3]) + get_value(argv[4]);
>
> It might be better to do something like:
>
> const char *left = NULL, *right = NULL;
>
> if (argc > 3)
>    left = argv[3];
> ...
>
> Then you can use 'left' and 'right' below.
>
> > +       else if (IS_MATH_CMD("sub", 5))
> > +               i = get_value(argv[3]) - get_value(argv[4]);
> > +       else if (IS_MATH_CMD("mul", 5))
> > +               i = get_value(argv[3]) * get_value(argv[4]);
> > +       else if (IS_MATH_CMD("div", 5))
> > +               i = get_value(argv[3]) / get_value(argv[4]);
> > +       else if (IS_MATH_CMD("shl", 5))
> > +               i = get_value(argv[3]) << get_value(argv[4]);
> > +       else if (IS_MATH_CMD("d2h", 4))
> > +               i = get_value_base10(argv[3]);
> > +       else
> > +               return cmd_usage(cmdtp);
> > +
> > +       sprintf(numberbuf, "%llx", i);
> > +       env_set(argv[2], numberbuf);
> > +       return 0;
> > +}
> > +
> > +U_BOOT_CMD(math, 5, 0, do_math,
> > +          "Environment variable math.",
> > +          "add a b c\n"
> > +          "    - add b to c and store in a.\n"
>
> But also b and c can be numbers, right? You should mention that.
>
> > +          "math sub a b c\n"
> > +          "    - subtract b from c and store in a.\n"
> > +          "math mul a b c\n"
> > +          "    - multiply b by c and store in a.\n"
> > +          "math div a b c\n"
> > +          "    - divide b by c and store in a.\n"
> > +          "math shl a b c\n"
> > +          "    - shift left b by c and store in a.\n"
> > +          "math d2h a b\n"
> > +          "    - Convert b from decimal to hex and store in a.\n"
> > +);
> > diff --git a/cmd/strcmp.c b/cmd/strcmp.c new file mode 100644 index
> > 0000000000..3abd270d40
> > --- /dev/null
> > +++ b/cmd/strcmp.c
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2010-2017 Broadcom
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +
> > +static int do_strcmp(cmd_tbl_t *cmdtp, int flag, int argc, char *
> > +const argv[]) {
> > +       if (argc != 3)
> > +               return cmd_usage(cmdtp);
> > +
> > +       if (strlen(argv[1]) != strlen(argv[2]))
> > +               return CMD_RET_FAILURE;
> > +
> > +       /* Compare the temporary string to the given parameter */
> > +       if (!strncmp(argv[1], argv[2], strlen(argv[2])))
> > +               return CMD_RET_SUCCESS;
> > +
> > +       return CMD_RET_FAILURE;
> > +}
> > +
> > +U_BOOT_CMD(strcmp, 3, 0, do_strcmp,
> > +          "String to string comparison.",
> > +          "[str] [str]\n"
>
> str1 str2, I think
>
> > +          "    - Returns true if str are same, else returns false.\n"
> > +);
> > --
> > 2.17.1
> >
>
> Regards,
> Simon

[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[Tom Rini]

On Mon, Nov 18, 2019 at 04:26:30PM -0800, Vladimir Olovyannikov wrote:

> cmd: adding malloc, math, and strcmp u-boot commands.
> - malloc  supports allocation of heap memory and free allocated memory
>           via u-boot command line.

Can you expand on how this is used in a script?  I'm not sure I see that
exactly.  Also:

> +config CMD_MALLOC
> +	bool "malloc"
> +	default y
> +	help
> +	  Supports allocation of heap memory and free allocated memory commands.
> +	  These commands are used by u-boot scripts.
> +
> +config CMD_MATH
> +	bool "math"
> +	default y
> +	help
> +	  Provides math commands such as add, sub, mul, div, shift,
> +	  convert decimal to hex functionalities to be available in the script.

First, why do we need this, rather than using setexpr ?

> +
>  config CMD_SOURCE
>  	bool "source"
>  	default y
> @@ -1301,6 +1315,13 @@ config CMD_SETEXPR
>  	  Also supports loading the value at a memory location into a variable.
>  	  If CONFIG_REGEX is enabled, setexpr also supports a gsub function.
>  
> +config CMD_STRCMP
> +	bool "strcmp"
> +	default y
> +	help
> +	  Provides string compare command feature to u-boot scripts.

Second, new commands must not default to y, but they should be enabled
on sandbox and new test.py tests added for them.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191119/a7f5af39/attachment.sig>

[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[Vladimir Olovyannikov]

Hi Tom,

> -----Original Message-----
> From: Tom Rini [mailto:trini at konsulko.com]
> Sent: Tuesday, November 19, 2019 12:38 PM
> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Cc: u-boot at lists.denx.de; Suji Velupiallai
<suji.velupillai@broadcom.com>;
> Heinrich Schuchardt <xypron.glpk@gmx.de>
> Subject: Re: [U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and
strcmp
> commands to u-boot
>
> On Mon, Nov 18, 2019 at 04:26:30PM -0800, Vladimir Olovyannikov wrote:
>
> > cmd: adding malloc, math, and strcmp u-boot commands.
> > - malloc  supports allocation of heap memory and free allocated memory
> >           via u-boot command line.
>
> Can you expand on how this is used in a script?  I'm not sure I see that
> exactly.
I am upstreaming the new bcm platform. Here is an excerpt from the script
which uses
these:
#define SD_UPDATE \
	"sd_update="\
	"if malloc tmp 2000; then "\
	"else "\
		"echo [sd_update] malloc 2000 bytes ** FAILED **;"\
		"exit;"\
	"fi;"\
	"if fatload mmc ${sd_device_number} ${tmp} "\
	"${sd_update_prefix}.sd-update; then "\
	"else "\
		"echo [sd_update] fatload ${sd_update_prefix}.sd-update "\
		"** FAILED **;"\
		"exit;"\
	"fi;"\
	"if source ${tmp}; then "\
	"else "\
		"echo [sd_update] Executing script ** FAILED **;"\
		"exit;"\
	"fi;"\
	"if free tmp; then "\
	"else " \
		"echo [sd_update] free 2000 bytes ** FAILED **;"\
		"exit;"\
	"fi \0"
;
	"if math add filesize filesize 1FF; then "\
	"else "\
		"echo [mmc_flash_image_rsa] math add command ** FAILED
**;"\
		"exit;"\
	"fi;"\
	"if math div fileblocks filesize 200; then "\
	"else "\
		"echo [mmc_flash_image_rsa] math div command ** FAILED
**;"\
		"exit;"\
	"fi;"\
> Also:
> > +config CMD_MALLOC
> > +	bool "malloc"
> > +	default y
> > +	help
> > +	  Supports allocation of heap memory and free allocated memory
> commands.
> > +	  These commands are used by u-boot scripts.
> > +
> > +config CMD_MATH
> > +	bool "math"
> > +	default y
> > +	help
> > +	  Provides math commands such as add, sub, mul, div, shift,
> > +	  convert decimal to hex functionalities to be available in the
script.
>
> First, why do we need this, rather than using setexpr ?
I agree, the platform needs to use setexpr as it contains all math
operations.
>
> > +
> >  config CMD_SOURCE
> >  	bool "source"
> >  	default y
> > @@ -1301,6 +1315,13 @@ config CMD_SETEXPR
> >  	  Also supports loading the value at a memory location into a
variable.
> >  	  If CONFIG_REGEX is enabled, setexpr also supports a gsub
function.
> >
> > +config CMD_STRCMP
> > +	bool "strcmp"
> > +	default y
> > +	help
> > +	  Provides string compare command feature to u-boot scripts.
>
> Second, new commands must not default to y, but they should be enabled
> on sandbox and new test.py tests added for them.  Thanks!
OK, thank you.
Is there an example/doc on how to enable these on sandbox and to provide
tests?

Vladimir
>
> --
> Tom

[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot

[Tom Rini]

On Wed, Nov 20, 2019 at 02:50:53PM -0800, Vladimir Olovyannikov wrote:
> Hi Tom,
> 
> > -----Original Message-----
> > From: Tom Rini [mailto:trini at konsulko.com]
> > Sent: Tuesday, November 19, 2019 12:38 PM
> > To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > Cc: u-boot at lists.denx.de; Suji Velupiallai
> <suji.velupillai@broadcom.com>;
> > Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Subject: Re: [U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and
> strcmp
> > commands to u-boot
> >
> > On Mon, Nov 18, 2019 at 04:26:30PM -0800, Vladimir Olovyannikov wrote:
> >
> > > cmd: adding malloc, math, and strcmp u-boot commands.
> > > - malloc  supports allocation of heap memory and free allocated memory
> > >           via u-boot command line.
> >
> > Can you expand on how this is used in a script?  I'm not sure I see that
> > exactly.
> I am upstreaming the new bcm platform. Here is an excerpt from the script
> which uses
> these:
> #define SD_UPDATE \
> 	"sd_update="\
> 	"if malloc tmp 2000; then "\
> 	"else "\
> 		"echo [sd_update] malloc 2000 bytes ** FAILED **;"\
> 		"exit;"\
> 	"fi;"\
> 	"if fatload mmc ${sd_device_number} ${tmp} "\
> 	"${sd_update_prefix}.sd-update; then "\
> 	"else "\
> 		"echo [sd_update] fatload ${sd_update_prefix}.sd-update "\
> 		"** FAILED **;"\
> 		"exit;"\
> 	"fi;"\
> 	"if source ${tmp}; then "\
> 	"else "\
> 		"echo [sd_update] Executing script ** FAILED **;"\
> 		"exit;"\
> 	"fi;"\
> 	"if free tmp; then "\
> 	"else " \
> 		"echo [sd_update] free 2000 bytes ** FAILED **;"\
> 		"exit;"\
> 	"fi \0"
> ;
> 	"if math add filesize filesize 1FF; then "\
> 	"else "\
> 		"echo [mmc_flash_image_rsa] math add command ** FAILED
> **;"\
> 		"exit;"\
> 	"fi;"\
> 	"if math div fileblocks filesize 200; then "\
> 	"else "\
> 		"echo [mmc_flash_image_rsa] math div command ** FAILED
> **;"\
> 		"exit;"\
> 	"fi;"\

Ah, interesting.  So you're malloc'ing the memory location that you load
in to rather than the usual method of using known variables and
locations for the SoC.  I would discourage this as we generally do not
have a large malloc pool available.

> > Also:
> > > +config CMD_MALLOC
> > > +	bool "malloc"
> > > +	default y
> > > +	help
> > > +	  Supports allocation of heap memory and free allocated memory
> > commands.
> > > +	  These commands are used by u-boot scripts.
> > > +
> > > +config CMD_MATH
> > > +	bool "math"
> > > +	default y
> > > +	help
> > > +	  Provides math commands such as add, sub, mul, div, shift,
> > > +	  convert decimal to hex functionalities to be available in the
> script.
> >
> > First, why do we need this, rather than using setexpr ?
> I agree, the platform needs to use setexpr as it contains all math
> operations.
> >
> > > +
> > >  config CMD_SOURCE
> > >  	bool "source"
> > >  	default y
> > > @@ -1301,6 +1315,13 @@ config CMD_SETEXPR
> > >  	  Also supports loading the value at a memory location into a
> variable.
> > >  	  If CONFIG_REGEX is enabled, setexpr also supports a gsub
> function.
> > >
> > > +config CMD_STRCMP
> > > +	bool "strcmp"
> > > +	default y
> > > +	help
> > > +	  Provides string compare command feature to u-boot scripts.
> >
> > Second, new commands must not default to y, but they should be enabled
> > on sandbox and new test.py tests added for them.  Thanks!
> OK, thank you.
> Is there an example/doc on how to enable these on sandbox and to provide
> tests?

Unfortunately there is not (but would be greatly appreciated).  There
are a lot of examples under test/py/ however.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191120/c4bfcfbb/attachment.sig>