Hi Codrin,
On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu codrin.ciubotariu@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@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot