[U-Boot] [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands to u-boot
Vladimir Olovyannikov
vladimir.olovyannikov at broadcom.com
Tue Nov 19 20:02:15 UTC 2019
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 at broadcom.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Heinrich Schuchardt
> <xypron.glpk at gmx.de>; Suji Velupiallai <suji.velupillai at 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 at 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 at broadcom.com>
> > Signed-off-by: Vladimir Olovyannikov
> > <vladimir.olovyannikov at 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
More information about the U-Boot
mailing list