[U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the parser for VSC9953 commands
Joe Hershberger
joe.hershberger at gmail.com
Fri Jun 26 00:30:37 CEST 2015
Hi Codrin,
On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu
<codrin.ciubotariu at freescale.com> wrote:
> In order to support multiple commands to configure the VSC9953
> L2 Switch, the parser needs to be changed to be more flexible and
> to support more complex commands. This patch adds a parser that
> searches for defined keywords in the command and calls the proper
> function when a match is found. Also, the parser allows for
> optional keywords, such as "port", to apply the command on a port
> or on all ports. The already defined commands are also changed a
> bit to:
> ethsw [port <port_no>] { enable | disable | show }
>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu at freescale.com>
> ---
> Changes for v2:
> - removed Change-id field;
>
> drivers/net/vsc9953.c | 381 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 310 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c
> index 9dec683..4df751a 100644
> --- a/drivers/net/vsc9953.c
> +++ b/drivers/net/vsc9953.c
> @@ -10,6 +10,7 @@
> #include <asm/fsl_serdes.h>
> #include <fm_eth.h>
> #include <fsl_memac.h>
> +#include <errno.h>
> #include <vsc9953.h>
>
> static struct vsc9953_info vsc9953_l2sw = {
> @@ -575,6 +576,10 @@ void vsc9953_init(bd_t *bis)
> }
>
> #ifdef CONFIG_VSC9953_CMD
I'd like to see this moved to its own file in common... maybe
"common/cmd_ethsw.c". I'd also like to see this #define change to
something like "CONFIG_CMD_ETHSW".
These changes don't necessarily need to be part of this series, since
it already got in as is, but if you feel motivated, I would recommend
you add a patch before this one that moves it.
> +
> +#define VSC9953_MAX_CMD_PARAMS 20
> +#define VSC9953_CMD_PORT_ALL -1
> +
> /* Enable/disable status of a VSC9953 port */
> static void vsc9953_port_status_set(int port_no, u8 enabled)
> {
> @@ -595,15 +600,6 @@ static void vsc9953_port_status_set(int port_no, u8 enabled)
> CONFIG_VSC9953_PORT_ENA);
> }
>
> -/* Set all VSC9953 ports' status */
> -static void vsc9953_port_all_status_set(u8 enabled)
> -{
> - int i;
> -
> - for (i = 0; i < VSC9953_MAX_PORTS; i++)
> - vsc9953_port_status_set(i, enabled);
> -}
> -
> /* Start autonegotiation for a VSC9953 PHY */
> static void vsc9953_phy_autoneg(int port_no)
> {
> @@ -615,15 +611,6 @@ static void vsc9953_phy_autoneg(int port_no)
> printf("Failed to start PHY for port %d\n", port_no);
> }
>
> -/* Start autonegotiation for all VSC9953 PHYs */
> -static void vsc9953_phy_all_autoneg(void)
> -{
> - int i;
> -
> - for (i = 0; i < VSC9953_MAX_PORTS; i++)
> - vsc9953_phy_autoneg(i);
> -}
> -
> /* Print a VSC9953 port's configuration */
> static void vsc9953_port_config_show(int port_no)
> {
> @@ -685,75 +672,327 @@ static void vsc9953_port_config_show(int port_no)
> printf("%8s\n", duplex == DUPLEX_FULL ? "full" : "half");
> }
>
> -/* Print VSC9953 ports' configuration */
> -static void vsc9953_port_all_config_show(void)
> -{
> - int i;
> +/* IDs used to track keywords in a command */
> +enum keyword_id {
> + id_key_end = -1,
> + id_help,
> + id_show,
> + id_port,
> + id_enable,
> + id_disable,
> + id_count, /* keep last */
> +};
>
> - for (i = 0; i < VSC9953_MAX_PORTS; i++)
> - vsc9953_port_config_show(i);
> -}
> +enum keyword_opt_id {
> + id_port_no = id_count + 1,
> + id_count_all, /* keep last */
> +};
>
> -/* function to interpret commands starting with "ethsw " */
> -static int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +struct command_def {
> + int cmd_to_keywords[VSC9953_MAX_CMD_PARAMS];
> + int cmd_keywords_nr;
> + int port;
> + int err;
Remove this. Just use a return value.
> + int (*cmd_function)(struct command_def *parsed_cmd);
> +};
> +
> +#define VSC9953_PORT_CONF_HELP "[port <port_no>] { enable | disable | show } " \
> +"- enable/disable a port; show shows a port's configuration"
Probably better to define this down by the use.
> +
> +static int vsc9953_port_status_key_func(struct command_def *parsed_cmd)
> {
> - u8 enable;
> - u32 port;
> + int i;
> + u8 enabled;
Use a single space.
>
> - if (argc < 4)
> + /* Last keyword should tell us if we should enable/disable the port */
> + if (parsed_cmd->cmd_to_keywords[parsed_cmd->cmd_keywords_nr - 1] ==
> + id_enable)
> + enabled = 1;
> + else if (parsed_cmd->cmd_to_keywords[parsed_cmd->cmd_keywords_nr - 1] ==
> + id_disable)
> + enabled = 0;
> + else {
> + parsed_cmd->err = 1;
Remove this.
> return -1;
Please use "return CMD_RET_USAGE;" from include/command.h.
> + }
>
> - if (strcmp(argv[1], "port"))
> - return -1;
> + if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
> + vsc9953_port_status_set(parsed_cmd->port, enabled);
> + } else {
> + for (i = 0; i < VSC9953_MAX_PORTS; i++)
> + vsc9953_port_status_set(i, enabled);
> + }
> +
> + return 0;
Please use "return CMD_RET_SUCCESS;"
> +}
> +
> +static int vsc9953_port_config_key_func(struct command_def *parsed_cmd)
> +{
> + int i;
Use a single space.
> +
> + if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
> + vsc9953_phy_autoneg(parsed_cmd->port);
> + printf("%8s %8s %8s %8s %8s\n",
> + "Port", "Status", "Link", "Speed",
> + "Duplex");
> + vsc9953_port_config_show(parsed_cmd->port);
>
> - if (!strcmp(argv[3], "show")) {
> - if (!strcmp(argv[2], "all")) {
> - vsc9953_phy_all_autoneg();
> - printf("%8s %8s %8s %8s %8s\n",
> - "Port", "Status", "Link", "Speed",
> - "Duplex");
> - vsc9953_port_all_config_show();
> - return 0;
> - } else {
> - port = simple_strtoul(argv[2], NULL, 10);
> - if (!VSC9953_PORT_CHECK(port))
> - return -1;
> - vsc9953_phy_autoneg(port);
> - printf("%8s %8s %8s %8s %8s\n",
> - "Port", "Status", "Link", "Speed",
> - "Duplex");
> - vsc9953_port_config_show(port);
> - return 0;
> - }
> - } else if (!strcmp(argv[3], "enable")) {
> - enable = 1;
> - } else if (!strcmp(argv[3], "disable")) {
> - enable = 0;
> } else {
> - return -1;
> + for (i = 0; i < VSC9953_MAX_PORTS; i++)
> + vsc9953_phy_autoneg(i);
> + printf("%8s %8s %8s %8s %8s\n",
> + "Port", "Status", "Link", "Speed", "Duplex");
> + for (i = 0; i < VSC9953_MAX_PORTS; i++)
> + vsc9953_port_config_show(i);
> }
>
> - if (!strcmp(argv[2], "all")) {
> - vsc9953_port_all_status_set(enable);
> + return 0;
Please use "return CMD_RET_SUCCESS;"
> +}
> +
> +struct keywords_to_function {
> + enum keyword_id cmd_keyword[VSC9953_MAX_CMD_PARAMS];
> + int (*keyword_function)(struct command_def *parsed_cmd);
> +} cmd_def[] = {
> + {
> + .cmd_keyword = {
> + id_enable,
> + id_key_end,
> + },
> + .keyword_function = &vsc9953_port_status_key_func,
> + }, {
> + .cmd_keyword = {
> + id_disable,
> + id_key_end,
> + },
> + .keyword_function = &vsc9953_port_status_key_func,
> + }, {
> + .cmd_keyword = {
> + id_show,
> + id_key_end,
> + },
> + .keyword_function = &vsc9953_port_config_key_func,
> + },
> +};
> +
> +struct keywords_optional {
> + enum keyword_id cmd_keyword[VSC9953_MAX_CMD_PARAMS];
> +} cmd_opt_def[] = {
> + {
> + .cmd_keyword = {
> + id_port,
> + id_port_no,
> + id_key_end,
> + },
> + },
> +};
> +
> +static int keyword_match_gen(enum keyword_id key_id, int argc,
> + char *const argv[], int *argc_nr,
> + struct command_def *parsed_cmd);
> +static int keyword_match_port(enum keyword_id key_id, int argc,
> + char *const argv[], int *argc_nr,
> + struct command_def *parsed_cmd);
> +
> +/* Define properties for each keyword;
> + * keep the order synced with enum keyword_id
> + */
> +struct keyword_def {
> + const char *keyword_name;
> + int (*match)(enum keyword_id key_id, int argc, char *const argv[],
> + int *argc_nr, struct command_def *parsed_cmd);
> +} keyword[] = {
> + {
> + .keyword_name = "help",
> + .match = &keyword_match_gen,
> + }, {
> + .keyword_name = "show",
> + .match = &keyword_match_gen,
> + }, {
> + .keyword_name = "port",
> + .match = &keyword_match_port
> + }, {
> + .keyword_name = "enable",
> + .match = &keyword_match_gen,
> + }, {
> + .keyword_name = "disable",
> + .match = &keyword_match_gen,
> + },
> +};
> +
> +/* Generic function used to match a keyword only by a string */
> +static int keyword_match_gen(enum keyword_id key_id, int argc,
> + char *const argv[], int *argc_nr,
> + struct command_def *parsed_cmd)
> +{
> + if (strcmp(argv[*argc_nr], keyword[key_id].keyword_name) == 0) {
> + parsed_cmd->cmd_to_keywords[*argc_nr] = key_id;
> +
> + return 1;
> + }
> + return 0;
> +}
> +
> +/* Function used to match the command's port */
> +static int keyword_match_port(enum keyword_id key_id, int argc,
> + char *const argv[], int *argc_nr,
> + struct command_def *parsed_cmd)
> +{
> + unsigned long val;
Use a single space.
> +
> + if (!keyword_match_gen(key_id, argc, argv, argc_nr, parsed_cmd))
> return 0;
> - } else {
> - port = simple_strtoul(argv[2], NULL, 10);
> - if (!VSC9953_PORT_CHECK(port))
> - return -1;
> - vsc9953_port_status_set(port, enable);
> +
> + if (*argc_nr + 1 >= argc)
> return 0;
> +
> + if (strict_strtoul(argv[*argc_nr + 1], 10, &val) != -EINVAL) {
> + if (!VSC9953_PORT_CHECK(val)) {
> + printf("Invalid port number: %lu\n", val);
> + return 0;
> + }
> + parsed_cmd->port = val;
> + (*argc_nr)++;
> + parsed_cmd->cmd_to_keywords[*argc_nr] = id_port_no;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +/* match optional keywords */
> +static void cmd_keywords_opt_check(struct command_def *parsed_cmd,
> + int *argc_val)
> +{
> + int i, keyw_opt_id, argc_val_max;
Use a single space. Put each var on a separate line.
> +
> + /* remember the best match */
> + argc_val_max = *argc_val;
> +
> + for (i = 0; i < ARRAY_SIZE(cmd_opt_def); i++) {
> + keyw_opt_id = 0;
> + while (keyw_opt_id + *argc_val <
> + parsed_cmd->cmd_keywords_nr &&
> + cmd_opt_def[i].cmd_keyword[keyw_opt_id] != id_key_end &&
> + parsed_cmd->cmd_to_keywords[keyw_opt_id + *argc_val] ==
> + cmd_opt_def[i].cmd_keyword[keyw_opt_id])
> + keyw_opt_id++;
It might help to break this up a bit and use some intermediate
variables to make the code more readable.
> + if (keyw_opt_id && keyw_opt_id + *argc_val <=
> + parsed_cmd->cmd_keywords_nr &&
> + cmd_opt_def[i].cmd_keyword[keyw_opt_id] == id_key_end &&
> + (*argc_val + keyw_opt_id > argc_val_max))
This could benefit from a comment describing what you expect to be verified.
> + argc_val_max = *argc_val + keyw_opt_id;
> }
>
> - return -1;
> + *argc_val = argc_val_max;
> }
>
> -U_BOOT_CMD(ethsw, 5, 0, do_ethsw,
> +/* get the function to call based on keywords */
> +static void cmd_keywords_check(struct command_def *parsed_cmd, int *argc_val)
> +{
> + int i, keyword_id;
Use a single space. Put each var on a separate line.
> +
> + for (i = 0; i < ARRAY_SIZE(cmd_def); i++) {
> + keyword_id = 0;
> + while (keyword_id + *argc_val < parsed_cmd->cmd_keywords_nr &&
> + cmd_def[i].cmd_keyword[keyword_id] != id_key_end &&
> + parsed_cmd->cmd_to_keywords[keyword_id + *argc_val] ==
> + cmd_def[i].cmd_keyword[keyword_id])
> + keyword_id++;
It might help to break this up a bit and use some intermediate
variables to make the code more readable.
> + if (keyword_id && keyword_id + *argc_val ==
> + parsed_cmd->cmd_keywords_nr &&
> + cmd_def[i].cmd_keyword[keyword_id] == id_key_end) {
This could benefit from a comment describing what you expect to be verified.
> + *argc_val += keyword_id;
> + parsed_cmd->cmd_function = cmd_def[i].keyword_function;
> + return;
> + }
> + }
> +}
> +
> +/* find all the keywords in the command */
> +static void keywords_find(int argc, char * const argv[],
Make this function return an int.
> + struct command_def *parsed_cmd)
> +{
> + int i, j, argc_val;
Use a single space. Put each var on a separate line.
> +
> + for (i = 1; i < argc; i++) {
> + for (j = 0; j < id_count; j++) {
> + if (keyword[j].match(j, argc, argv, &i,
> + parsed_cmd))
> + break;
> + }
> + }
> +
> + for (i = 1; i < argc; i++)
> + if (parsed_cmd->cmd_to_keywords[i] == -1)
> + parsed_cmd->err = 1;
Instead return CMD_RET_USAGE;
> +
> + parsed_cmd->cmd_keywords_nr = argc;
> + argc_val = 1;
> +
> + /* get optional parameters first */
> + cmd_keywords_opt_check(parsed_cmd, &argc_val);
> +
> + cmd_keywords_check(parsed_cmd, &argc_val);
> +
> + /* error if not all commands' parameters were matched */
> + if (argc_val != parsed_cmd->cmd_keywords_nr ||
> + !parsed_cmd->cmd_function)
> + parsed_cmd->err = 1;
Instead return CMD_RET_USAGE;
> +}
> +
> +static void command_def_init(struct command_def *parsed_cmd)
> +{
> + int i;
> +
> + for (i = 0; i < VSC9953_MAX_CMD_PARAMS; i++)
> + parsed_cmd->cmd_to_keywords[i] = -1;
> +
> + parsed_cmd->port = VSC9953_CMD_PORT_ALL;
> + parsed_cmd->err = 0;
Remove this.
> + parsed_cmd->cmd_function = NULL;
> +}
> +
> +static void command_def_cleanup(struct command_def *parsed_cmd)
> +{
> + /* Nothing to do for now */
Then why define it?
> +}
> +
> +/* function to interpret commands starting with "ethsw " */
> +static int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> + struct command_def parsed_cmd;
> + int rc = 0;
Use a single space.
> +
> + if (argc >= VSC9953_MAX_CMD_PARAMS) {
> + rc = -1;
Use this:
+ rc = CMD_RET_USAGE;
> + goto __ret;
> + }
> +
> + command_def_init(&parsed_cmd);
> +
> + keywords_find(argc, argv, &parsed_cmd);
Use this:
+ rc = keywords_find(argc, argv, &parsed_cmd);
+ if (rc)
+ goto __ret_cmd_cleanup;
> +
> + if (!parsed_cmd.cmd_function) {
> + rc = -1;
> + goto __ret_cmd_cleanup;
> + }
I think this whole if statement is unneeded since this same test
already exists in keywords_find().
> +
> + rc = parsed_cmd.cmd_function(&parsed_cmd);
> +
> + if (parsed_cmd.err) {
> + rc = -1;
> + goto __ret_cmd_cleanup;
> + }
Remove this if statement along with parsed_cmd.err.
> +
> +__ret_cmd_cleanup:
> + command_def_cleanup(&parsed_cmd);
> +__ret:
> + return rc;
> +}
> +
> +U_BOOT_CMD(ethsw, VSC9953_MAX_CMD_PARAMS, 0, do_ethsw,
> "vsc9953 l2 switch commands",
> - "port <port_no> enable|disable\n"
> - " - enable/disable an l2 switch port\n"
> - " port_no=0..9; use \"all\" for all ports\n"
> - "ethsw port <port_no> show\n"
> - " - show an l2 switch port's configuration\n"
> - " port_no=0..9; use \"all\" for all ports\n"
> + VSC9953_PORT_CONF_HELP"\n"
> );
> +
> #endif /* CONFIG_VSC9953_CMD */
> --
> 1.9.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list