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

[Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>]

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