[U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to manipulate the FDB for VSC9953

Joe Hershberger joe.hershberger at gmail.com
Fri Jun 26 00:39:27 CEST 2015


Hi Codrin,

On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu
<codrin.ciubotariu at freescale.com> wrote:
> The new command:
> ethsw [port <port_no>] [vlan <vid>] fdb
>         { [help] | show | flush | { add | del } <mac> }
>
> Can be used to add and delete FDB entries. Also, the command can be used
> to show entries from the FDB tables. When used with [port <port_no>]
> and [vlan <vid>], only the matching the FDB entries can be seen or
> flushed.
>
> Signed-off-by: Johnson Leung <johnson.leung at freescale.com>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu at freescale.com>
> ---
> Changes for v2:
>         - removed Change-id field;
>
>  drivers/net/vsc9953.c | 635 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/vsc9953.h     |  28 +++
>  2 files changed, 662 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c
> index 1936c4a..ef7b50c 100644
> --- a/drivers/net/vsc9953.c
> +++ b/drivers/net/vsc9953.c
> @@ -12,6 +12,7 @@
>  #include <fsl_memac.h>
>  #include <errno.h>
>  #include <vsc9953.h>
> +#include <linux/ctype.h>
>
>  static struct vsc9953_info vsc9953_l2sw = {
>                 .port[0] = VSC9953_PORT_INFO_INITIALIZER(0),
> @@ -579,6 +580,7 @@ void vsc9953_init(bd_t *bis)
>
>  #define VSC9953_MAX_CMD_PARAMS 20
>  #define VSC9953_CMD_PORT_ALL   -1
> +#define VSC9953_CMD_VLAN_ALL   -1
>
>  /* Enable/disable status of a VSC9953 port */
>  static void vsc9953_port_status_set(int port_no, u8 enabled)
> @@ -952,6 +954,365 @@ static void vsc9953_port_statistics_clear(int port_no)
>                  CONFIG_VSC9953_STAT_CLEAR_DR);
>  }
>
> +/* wait for FDB to become available */
> +static int vsc9953_mac_table_poll_idle(void)
> +{
> +       struct vsc9953_analyzer         *l2ana_reg;
> +       u32                             timeout;

Use a single space.

> +
> +       l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
> +                       VSC9953_ANA_OFFSET);
> +
> +       timeout = 50000;
> +       while (((in_le32(&l2ana_reg->ana_tables.mac_access) &
> +                       CONFIG_VSC9953_MAC_CMD_MASK) !=
> +                CONFIG_VSC9953_MAC_CMD_IDLE) && --timeout)
> +               udelay(1);
> +
> +       return !!timeout;

Maybe return -EBUSY like suggested in previous patch.

> +}
> +
> +/* enum describing available commands for the MAC table */
> +enum mac_table_cmd {
> +       MAC_TABLE_READ_DIRECT,
> +       MAC_TABLE_READ_INDIRECT,
> +       MAC_TABLE_WRITE,
> +       MAC_TABLE_LEARN,
> +       MAC_TABLE_FORGET,
> +       MAC_TABLE_GET_NEXT,
> +       MAC_TABLE_AGE,
> +};
> +
> +/* Issues a command to the FDB table */
> +static int vsc9953_mac_table_cmd(enum mac_table_cmd cmd)
> +{
> +       struct vsc9953_analyzer         *l2ana_reg;

Use a single space.

> +
> +       l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
> +                       VSC9953_ANA_OFFSET);
> +
> +       switch (cmd) {
> +       case MAC_TABLE_READ_DIRECT:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK |
> +                               CONFIG_VSC9953_MAC_CMD_VALID,
> +                               CONFIG_VSC9953_MAC_CMD_READ);
> +               break;
> +       case MAC_TABLE_READ_INDIRECT:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK,
> +                               CONFIG_VSC9953_MAC_CMD_READ |
> +                               CONFIG_VSC9953_MAC_CMD_VALID);
> +               break;
> +       case MAC_TABLE_WRITE:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_MASK,
> +                               CONFIG_VSC9953_MAC_CMD_WRITE |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_LOCKED);
> +               break;
> +       case MAC_TABLE_LEARN:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_MASK,
> +                               CONFIG_VSC9953_MAC_CMD_LEARN |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_LOCKED |
> +                               CONFIG_VSC9953_MAC_CMD_VALID);
> +               break;
> +       case MAC_TABLE_FORGET:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_MASK,
> +                               CONFIG_VSC9953_MAC_CMD_FORGET);
> +               break;
> +       case MAC_TABLE_GET_NEXT:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_MASK,
> +                               CONFIG_VSC9953_MAC_CMD_NEXT);
> +               break;
> +       case MAC_TABLE_AGE:
> +               clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                               CONFIG_VSC9953_MAC_CMD_MASK |
> +                               CONFIG_VSC9953_MAC_ENTRYTYPE_MASK,
> +                               CONFIG_VSC9953_MAC_CMD_AGE);
> +               break;
> +       default:
> +               printf("Unknown MAC table command\n");
> +       }
> +
> +       if (!vsc9953_mac_table_poll_idle()) {
> +               debug("MAC table timeout\n");
> +               return -1;
> +       }
> +
> +       /* For some commands we might have a way to
> +        * detect if the command succeeded
> +        */
> +       if ((cmd == MAC_TABLE_GET_NEXT || cmd == MAC_TABLE_READ_DIRECT ||
> +            MAC_TABLE_READ_INDIRECT) &&
> +           !(in_le32(&l2ana_reg->ana_tables.mac_access) &
> +            CONFIG_VSC9953_MAC_CMD_VALID))
> +               return -1;
> +
> +       return 0;
> +}
> +
> +/* show the FDB entries that correspond to a port and a VLAN */
> +static void vsc9953_mac_table_show(int port_no, int vid)
> +{
> +       int                             i, rc[VSC9953_MAX_PORTS];
> +       u32                             val, vlan, mach, macl, dest_indx;

Use a separate line for each variable.

> +       enum port_learn_mode            mode[VSC9953_MAX_PORTS];
> +       struct vsc9953_analyzer         *l2ana_reg;
> +
> +       l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
> +                       VSC9953_ANA_OFFSET);
> +
> +       /* disable auto learning */
> +       if (port_no == VSC9953_CMD_PORT_ALL) {
> +               for (i = 0; i < VSC9953_MAX_PORTS; i++) {
> +                       rc[i] = vsc9953_port_learn_mode_get(i, &mode[i]);
> +                       if (!rc[i] && mode[i] != PORT_LEARN_NONE)
> +                               vsc9953_port_learn_mode_set(i,
> +                                                           PORT_LEARN_NONE);
> +               }
> +       } else {
> +               rc[port_no] = vsc9953_port_learn_mode_get(port_no,
> +                                                         &mode[port_no]);
> +               if (!rc[port_no] && mode[port_no] != PORT_LEARN_NONE)
> +                       vsc9953_port_learn_mode_set(port_no, PORT_LEARN_NONE);
> +       }
> +
> +       /* write port and vid to get selected FDB entries */
> +       val = in_le32(&l2ana_reg->ana.anag_efil);
> +       if (port_no != VSC9953_CMD_PORT_ALL) {
> +               val = (val & ~CONFIG_VSC9953_AGE_PORT_MASK) |
> +                               CONFIG_VSC9953_AGE_PORT_EN |
> +                               field_set(port_no,
> +                                         CONFIG_VSC9953_AGE_PORT_MASK);

Seems like a good place to use bitfield_replace() from
include/bitfield.h (or a new one that you add that uses the mask for
the shift).

> +       }
> +       if (vid != VSC9953_CMD_VLAN_ALL) {
> +               val = (val & ~CONFIG_VSC9953_AGE_VID_MASK) |
> +                               CONFIG_VSC9953_AGE_VID_EN |
> +                               field_set(vid, CONFIG_VSC9953_AGE_VID_MASK);

Same here.

> +       }
> +       out_le32(&l2ana_reg->ana.anag_efil, val);
> +
> +       /* set MAC and VLAN to 0 to look from beginning */
> +       clrbits_le32(&l2ana_reg->ana_tables.mach_data,
> +                    CONFIG_VSC9953_MAC_VID_MASK |
> +                    CONFIG_VSC9953_MAC_MACH_MASK);
> +       out_le32(&l2ana_reg->ana_tables.macl_data, 0);
> +
> +       /* get entries */
> +       printf("%10s %17s %5s %4s\n", "EntryType", "MAC", "PORT", "VID");
> +       do {
> +               /* get out when an invalid entry is found */
> +               if (vsc9953_mac_table_cmd(MAC_TABLE_GET_NEXT))
> +                       break;
> +
> +               val = in_le32(&l2ana_reg->ana_tables.mac_access);
> +
> +               switch (val & CONFIG_VSC9953_MAC_ENTRYTYPE_MASK) {
> +               case CONFIG_VSC9953_MAC_ENTRYTYPE_NORMAL:
> +                       printf("%10s ", "Dynamic");
> +                       break;
> +               case CONFIG_VSC9953_MAC_ENTRYTYPE_LOCKED:
> +                       printf("%10s ", "Static");
> +                       break;
> +               case CONFIG_VSC9953_MAC_ENTRYTYPE_IPV4MCAST:
> +                       printf("%10s ", "IPv4 Mcast");
> +                       break;
> +               case CONFIG_VSC9953_MAC_ENTRYTYPE_IPV6MCAST:
> +                       printf("%10s ", "IPv6 Mcast");
> +                       break;
> +               default:
> +                       printf("%10s ", "Unknown");
> +               }
> +
> +               dest_indx = field_get(val & CONFIG_VSC9953_MAC_DESTIDX_MASK,
> +                                     CONFIG_VSC9953_MAC_DESTIDX_MASK);
> +
> +               val = in_le32(&l2ana_reg->ana_tables.mach_data);
> +               vlan = field_get(val & CONFIG_VSC9953_MAC_VID_MASK,
> +                                CONFIG_VSC9953_MAC_VID_MASK);

It seems like masking off the val before shifting it would be better
implemented inside the field_get function (renamed and moved to
include/bitfield.h) instead of on each use.

> +               mach = field_get(val & CONFIG_VSC9953_MAC_MACH_MASK,
> +                                CONFIG_VSC9953_MAC_MACH_MASK);
> +               macl = in_le32(&l2ana_reg->ana_tables.macl_data);
> +
> +               printf("%02x:%02x:%02x:%02x:%02x:%02x ", (mach >> 8) & 0xff,
> +                      mach & 0xff, (macl >> 24) & 0xff, (macl >> 16) & 0xff,
> +                      (macl >> 8) & 0xff, macl & 0xff);
> +               printf("%5d ", dest_indx);
> +               printf("%4d\n", vlan);
> +       } while (1);
> +
> +       /* set learning mode to previous value */
> +       if (port_no == VSC9953_CMD_PORT_ALL) {
> +               for (i = 0; i < VSC9953_MAX_PORTS; i++) {
> +                       if (!rc[i] && mode[i] != PORT_LEARN_NONE)
> +                               vsc9953_port_learn_mode_set(i, mode[i]);
> +               }
> +       } else {
> +               /* If administrative down, skip */
> +               if (!rc[port_no] && mode[port_no] != PORT_LEARN_NONE)
> +                       vsc9953_port_learn_mode_set(port_no, mode[port_no]);
> +       }
> +
> +       /* reset FDB port and VLAN FDB selection */
> +       clrbits_le32(&l2ana_reg->ana.anag_efil, CONFIG_VSC9953_AGE_PORT_EN |
> +                    CONFIG_VSC9953_AGE_PORT_MASK | CONFIG_VSC9953_AGE_VID_EN |
> +                    CONFIG_VSC9953_AGE_VID_MASK);
> +}
> +
> +/* Add a static FDB entry */
> +static int vsc9953_mac_table_add(u8 port_no, uchar mac[6], int vid)
> +{
> +       u32                             val;
> +       struct vsc9953_analyzer         *l2ana_reg;
> +
> +       l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
> +                       VSC9953_ANA_OFFSET);
> +
> +       out_le32(&l2ana_reg->ana_tables.mach_data,
> +                (mac[0] << 8) | (mac[1] << 0) |
> +                (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) &
> +                                CONFIG_VSC9953_MACHDATA_VID_MASK));

Why do you need to & with the mask again after field_set()?

> +       out_le32(&l2ana_reg->ana_tables.macl_data,
> +                (mac[2] << 24) | (mac[3] << 16) | (mac[4] << 8) |
> +                (mac[5] << 0));
> +
> +       /* set on which port is the MAC address added */
> +       clrsetbits_le32(&l2ana_reg->ana_tables.mac_access,
> +                       CONFIG_VSC9953_MAC_DESTIDX_MASK,
> +                       field_set(port_no, CONFIG_VSC9953_MAC_DESTIDX_MASK));
> +       if (vsc9953_mac_table_cmd(MAC_TABLE_LEARN))
> +               return -1;
> +
> +       /* check if the MAC address was indeed added */
> +       out_le32(&l2ana_reg->ana_tables.mach_data,
> +                (mac[0] << 8) | (mac[1] << 0) |
> +                (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) &
> +                               CONFIG_VSC9953_MACHDATA_VID_MASK));

Why do you need to & with the mask again after field_set()?

> +       out_le32(&l2ana_reg->ana_tables.macl_data,
> +                (mac[2] << 24) | (mac[3] << 16) | (mac[4] << 8) |
> +                (mac[5] << 0));
> +
> +       if (vsc9953_mac_table_cmd(MAC_TABLE_READ_DIRECT))
> +               return -1;
> +
> +       val = in_le32(&l2ana_reg->ana_tables.mac_access);
> +
> +       if ((port_no != field_get(val & CONFIG_VSC9953_MAC_DESTIDX_MASK,
> +                                 CONFIG_VSC9953_MAC_DESTIDX_MASK))) {
> +               printf("Failed to add MAC address\n");
> +               return -1;
> +       }
> +       return 0;
> +}
> +
> +/* Delete a FDB entry */
> +static int vsc9953_mac_table_del(uchar mac[6], u16 vid)
> +{
> +       struct vsc9953_analyzer         *l2ana_reg;

Use a single space.

> +
> +       l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
> +                       VSC9953_ANA_OFFSET);
> +
> +       /* check first if MAC entry is present */
> +       out_le32(&l2ana_reg->ana_tables.mach_data,
> +                (mac[0] << 8) | (mac[1] << 0) |
> +                (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) &
> +                 CONFIG_VSC9953_MACHDATA_VID_MASK));

Why do you need to & with the mask again after field_set()?

> +       out_le32(&l2ana_reg->ana_tables.macl_data,
> +                (mac[2] << 24) | (mac[3] << 16) | (mac[4] << 8) |
> +                (mac[5] << 0));
> +
> +       if (vsc9953_mac_table_cmd(MAC_TABLE_READ_INDIRECT)) {
> +               printf("The MAC address: %02x:%02x:%02x:%02x:%02x:%02x ",
> +                      mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +                       printf("VLAN: %d does not exist.\n", vid);
> +               return -1;
> +       }
> +
> +       /* FDB entry found, proceed to delete */
> +       out_le32(&l2ana_reg->ana_tables.mach_data, (mac[0] << 8) |
> +                (mac[1] << 0) |
> +                (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) &
> +                 CONFIG_VSC9953_MACHDATA_VID_MASK));

Why do you need to & with the mask again after field_set()?

> +       out_le32(&l2ana_reg->ana_tables.macl_data, (mac[2] << 24) |
> +                (mac[3] << 16) | (mac[4] << 8) | (mac[5] << 0));
> +
> +       if (vsc9953_mac_table_cmd(MAC_TABLE_FORGET))
> +               return -1;
> +
> +       /* check if the MAC entry is still in FDB */
> +       out_le32(&l2ana_reg->ana_tables.mach_data, (mac[0] << 8) |
> +                (mac[1] << 0) |
> +                (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) &
> +                 CONFIG_VSC9953_MACHDATA_VID_MASK));

Why do you need to & with the mask again after field_set()?

> +       out_le32(&l2ana_reg->ana_tables.macl_data, (mac[2] << 24) |
> +                (mac[3] << 16) | (mac[4] << 8) | (mac[5] << 0));
> +
> +       if (vsc9953_mac_table_cmd(MAC_TABLE_READ_INDIRECT)) {
> +               printf("Failed to delete MAC address\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +/* age the unlocked entries in FDB */
> +static void vsc9953_mac_table_age(int port_no, int vid)
> +{
> +       int                             rc;
> +       u32                             val;
> +       struct vsc9953_analyzer         *l2ana_reg;
> +       enum port_learn_mode            mode;
> +
> +       l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
> +                       VSC9953_ANA_OFFSET);
> +
> +       /* disable auto learning */
> +       rc = vsc9953_port_learn_mode_get(port_no, &mode);
> +       if (!rc && mode != PORT_LEARN_NONE)
> +               vsc9953_port_learn_mode_set(port_no, PORT_LEARN_NONE);
> +
> +       /* set port and VID for selective aging */
> +       val = in_le32(&l2ana_reg->ana.anag_efil);
> +       if (port_no != VSC9953_CMD_PORT_ALL) {
> +               val = (val & ~CONFIG_VSC9953_AGE_PORT_MASK) |
> +                     CONFIG_VSC9953_AGE_PORT_EN |
> +                     field_set(port_no, CONFIG_VSC9953_AGE_PORT_MASK);

Seems like a good place to use bitfield_replace() from
include/bitfield.h (or a new one that you add that uses the mask for
the shift).

> +       }
> +
> +       if (vid != VSC9953_CMD_VLAN_ALL) {
> +               val = (val & ~CONFIG_VSC9953_AGE_VID_MASK) |
> +                     CONFIG_VSC9953_AGE_VID_EN |
> +                     field_set(vid, CONFIG_VSC9953_AGE_VID_MASK);

Same here.

> +       }
> +       out_le32(&l2ana_reg->ana.anag_efil, val);
> +
> +       /* age the dynamic FDB entries */
> +       vsc9953_mac_table_cmd(MAC_TABLE_AGE);
> +
> +       /* clear previously set port and VID */
> +       clrbits_le32(&l2ana_reg->ana.anag_efil, CONFIG_VSC9953_AGE_PORT_EN |
> +                    CONFIG_VSC9953_AGE_PORT_MASK | CONFIG_VSC9953_AGE_VID_EN |
> +                    CONFIG_VSC9953_AGE_VID_MASK);
> +
> +       if (!rc && mode != PORT_LEARN_NONE)
> +               vsc9953_port_learn_mode_set(port_no, mode);
> +}
> +
> +/* Delete all the dynamic FDB entries */
> +static void vsc9953_mac_table_flush(int port, int vid)
> +{
> +       vsc9953_mac_table_age(port, vid);
> +       vsc9953_mac_table_age(port, vid);
> +}
> +
>  /* IDs used to track keywords in a command */
>  enum keyword_id {
>         id_key_end = -1,
> @@ -964,11 +1325,19 @@ enum keyword_id {
>         id_clear,
>         id_learning,
>         id_auto,
> +       id_vlan,
> +       id_fdb,
> +       id_add,
> +       id_del,
> +       id_flush,
>         id_count,       /* keep last */
>  };
>
>  enum keyword_opt_id {
>         id_port_no = id_count + 1,
> +       id_vlan_no,
> +       id_add_del_no,
> +       id_add_del_mac,
>         id_count_all,   /* keep last */
>  };
>
> @@ -977,6 +1346,8 @@ struct command_def {
>         int cmd_keywords_nr;
>         int port;
>         int err;
> +       int vid;
> +       uchar *mac_addr;

Use this:
+       uchar ethaddr[6];
I recently made a pass through U-Boot trying to standardize on that
naming. Also, don't make it a pointer that has to be allocated. It is
small and of known size.

>         int (*cmd_function)(struct command_def *parsed_cmd);
>  };
>
> @@ -1149,6 +1520,77 @@ static int vsc9953_learn_set_key_func(struct command_def *parsed_cmd)
>         return 0;
>  }
>
> +#define VSC9953_FDB_HELP "ethsw [port <port_no>] [vlan <vid>] fdb " \
> +"{ [help] | show | flush | { add | del } <mac> } " \
> +"- Add/delete a mac entry in FDB; use show to see FDB entries; " \
> +"if vlan <vid> is missing, will be used VID 1"

Please use this:
+"if vlan <vid> is missing, VID 1 will be used"

> +
> +static int vsc9953_fdb_help_key_func(struct command_def *parsed_cmd)
> +{
> +       printf(VSC9953_FDB_HELP"\n");
> +
> +       return 0;

Please use:
+       return CMD_RET_SUCCESS;

> +}
> +
> +static int vsc9953_fdb_show_key_func(struct command_def *parsed_cmd)
> +{
> +       vsc9953_mac_table_show(parsed_cmd->port, parsed_cmd->vid);
> +
> +       return 0;

Please use:
+       return CMD_RET_SUCCESS;

> +}
> +
> +static int vsc9953_fdb_flush_key_func(struct command_def *parsed_cmd)
> +{
> +       vsc9953_mac_table_flush(parsed_cmd->port, parsed_cmd->vid);
> +
> +       return 0;

Please use:
+       return CMD_RET_SUCCESS;

> +}
> +
> +static int vsc9953_fdb_entry_add_key_func(struct command_def *parsed_cmd)
> +{
> +       int                     vid;
> +
> +       /* check if MAC address is present */
> +       if (!parsed_cmd->mac_addr) {

Use this:
+       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {

> +               printf("Please use a valid MAC address\n");
> +               return -EINVAL;

Please use:
+               return CMD_RET_USAGE;

> +       }
> +
> +       /* a port number must be present */
> +       if (parsed_cmd->port == VSC9953_CMD_PORT_ALL) {
> +               printf("Please specify a port\n");
> +               return -EINVAL;

Please use:
+               return CMD_RET_USAGE;

> +       }
> +
> +       /* Use VLAN 1 if VID is not set */
> +       vid = (parsed_cmd->vid == VSC9953_CMD_VLAN_ALL ? 1 : parsed_cmd->vid);
> +
> +       if (vsc9953_mac_table_add(parsed_cmd->port, parsed_cmd->mac_addr, vid))
> +               return -1;

Please use:
+               return CMD_RET_FAILURE;

> +
> +       return 0;

Please use:
+       return CMD_RET_SUCCESS;

> +}
> +
> +static int vsc9953_fdb_entry_del_key_func(struct command_def *parsed_cmd)
> +{
> +       int                     vid;
> +
> +       /* check if MAC address is present */
> +       if (!parsed_cmd->mac_addr) {

Use this:
+       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {

> +               printf("Please use a valid MAC address\n");
> +               return -EINVAL;

Please use:
+               return CMD_RET_USAGE;

> +       }
> +
> +       /* Use VLAN 1 if VID is not set */
> +       vid = (parsed_cmd->vid == VSC9953_CMD_VLAN_ALL ?
> +              1 : parsed_cmd->vid);
> +
> +       if (vsc9953_mac_table_del(parsed_cmd->mac_addr, vid))
> +               return -1;

Please use:
+               return CMD_RET_FAILURE;

> +
> +       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);
> @@ -1225,6 +1667,49 @@ struct keywords_to_function {
>                                         id_key_end,
>                         },
>                         .keyword_function = &vsc9953_learn_set_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_fdb,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_fdb_help_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_fdb,
> +                                       id_help,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_fdb_help_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_fdb,
> +                                       id_show,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_fdb_show_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_fdb,
> +                                       id_flush,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_fdb_flush_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_fdb,
> +                                       id_add,
> +                                       id_add_del_mac,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_fdb_entry_add_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_fdb,
> +                                       id_del,
> +                                       id_add_del_mac,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_fdb_entry_del_key_func,
>                 },
>  };
>
> @@ -1237,6 +1722,20 @@ struct keywords_optional {
>                                                 id_port_no,
>                                                 id_key_end,
>                                 },
> +               }, {
> +                               .cmd_keyword = {
> +                                               id_vlan,
> +                                               id_vlan_no,
> +                                               id_key_end,
> +                               },
> +               }, {
> +                               .cmd_keyword = {
> +                                               id_port,
> +                                               id_port_no,
> +                                               id_vlan,
> +                                               id_vlan_no,
> +                                               id_key_end,
> +                               },
>                 },
>  };
>
> @@ -1246,6 +1745,12 @@ static int keyword_match_gen(enum keyword_id key_id, int argc,
>  static int keyword_match_port(enum keyword_id key_id, int argc,
>                               char *const argv[], int *argc_nr,
>                               struct command_def *parsed_cmd);
> +static int keyword_match_vlan(enum keyword_id key_id, int argc,
> +                             char *const argv[], int *argc_nr,
> +                             struct command_def *parsed_cmd);
> +static int keyword_match_mac_addr(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
> @@ -1282,6 +1787,21 @@ struct keyword_def {
>                 }, {
>                                 .keyword_name = "auto",
>                                 .match = &keyword_match_gen,
> +               }, {
> +                               .keyword_name = "vlan",
> +                                .match = &keyword_match_vlan,
> +               }, {
> +                               .keyword_name = "fdb",
> +                               .match = &keyword_match_gen,
> +               }, {
> +                               .keyword_name = "add",
> +                               .match = &keyword_match_mac_addr,
> +               }, {
> +                               .keyword_name = "del",
> +                               .match = &keyword_match_mac_addr,
> +               }, {
> +                               .keyword_name = "flush",
> +                               .match = &keyword_match_gen,
>                 },
>  };
>
> @@ -1325,6 +1845,112 @@ static int keyword_match_port(enum keyword_id key_id, int argc,
>         return 0;
>  }
>
> +/* Function used to match the command's vlan */
> +static int keyword_match_vlan(enum keyword_id key_id, int argc,
> +                             char *const argv[], int *argc_nr,
> +                             struct command_def *parsed_cmd)
> +{
> +       unsigned long                   val;
> +       int                             aux;

Use a single space.

> +
> +       if (!keyword_match_gen(key_id, argc, argv, argc_nr, parsed_cmd))
> +               return 0;
> +
> +       if (*argc_nr + 1 >= argc)
> +               return 0;
> +
> +       if (strict_strtoul(argv[*argc_nr + 1], 10, &val) != -EINVAL) {
> +               if (!VSC9953_VLAN_CHECK(val)) {
> +                       printf("Invalid vlan number: %lu\n", val);
> +                       return 0;
> +               }
> +               parsed_cmd->vid = val;
> +               (*argc_nr)++;
> +               parsed_cmd->cmd_to_keywords[*argc_nr] = id_vlan_no;
> +               return 1;
> +       }
> +
> +       aux = *argc_nr + 1;
> +
> +       if (keyword_match_gen(id_add, argc, argv, &aux, parsed_cmd))
> +               parsed_cmd->cmd_to_keywords[*argc_nr + 1] = id_add;
> +       else if (keyword_match_gen(id_del, argc, argv, &aux, parsed_cmd))
> +               parsed_cmd->cmd_to_keywords[*argc_nr + 1] = id_del;
> +       else
> +               return 0;
> +
> +       if (*argc_nr + 2 >= argc)
> +               return 0;
> +
> +       if (strict_strtoul(argv[*argc_nr + 2], 10, &val) != -EINVAL) {
> +               if (!VSC9953_VLAN_CHECK(val)) {
> +                       printf("Invalid vlan number: %lu\n", val);
> +                       return 0;
> +               }
> +               parsed_cmd->vid = val;
> +               (*argc_nr) += 2;
> +               parsed_cmd->cmd_to_keywords[*argc_nr] = id_add_del_no;
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +/* check if the string has the format for a MAC address */
> +static int string_is_mac_addr(const char *mac)
> +{
> +       int                     i;
> +
> +       if (!mac)
> +               return 0;
> +
> +       for (i = 0; i < 6; i++) {
> +               if (!isxdigit(*mac) || !isxdigit(*(mac + 1)))
> +                       return 0;
> +               mac += 2;
> +               if (i != 5) {
> +                       if (*mac != ':' && *mac != '-')
> +                               return 0;
> +                       mac++;
> +               }
> +       }
> +
> +       if (*mac != '\0')
> +               return 0;
> +
> +       return 1;

This functionality is already implemented in common/env_flags.c in the
_env_flags_validate_type() function. Maybe that implementation should
be extracted.  Another option is to make the eth_parse_enetaddr() in
net/eth.c return a status and then it can be used. Either way I don't
think it should be re-implemented here.

> +}
> +
> +/* Function used to match the command's MAC address */
> +static int keyword_match_mac_addr(enum keyword_id key_id, int argc,
> +                                 char *const argv[], int *argc_nr,
> +                                 struct command_def *parsed_cmd)
> +{
> +       if (!keyword_match_gen(key_id, argc, argv, argc_nr, parsed_cmd))
> +               return 0;
> +
> +       if ((*argc_nr + 1 >= argc) || parsed_cmd->mac_addr)
> +               return 1;
> +
> +       if (!string_is_mac_addr(argv[*argc_nr + 1])) {
> +               printf("Invalid mac address: %s\n", argv[*argc_nr + 1]);
> +               return 0;
> +       }
> +
> +       parsed_cmd->mac_addr = malloc(6);

Why malloc this? It is small and known size.

> +       eth_parse_enetaddr(argv[*argc_nr + 1], parsed_cmd->mac_addr);
> +
> +       if (is_broadcast_ethaddr(parsed_cmd->mac_addr)) {
> +               free(parsed_cmd->mac_addr);
> +               parsed_cmd->mac_addr = NULL;

Drop these two lines.

> +               return 0;
> +       }
> +
> +       parsed_cmd->cmd_to_keywords[*argc_nr + 1] = id_add_del_mac;
> +
> +       return 1;
> +}
> +
>  /* match optional keywords */
>  static void cmd_keywords_opt_check(struct command_def *parsed_cmd,
>                                    int *argc_val)
> @@ -1414,13 +2040,19 @@ static void command_def_init(struct command_def *parsed_cmd)
>                 parsed_cmd->cmd_to_keywords[i] = -1;
>
>         parsed_cmd->port = VSC9953_CMD_PORT_ALL;
> +       parsed_cmd->vid = VSC9953_CMD_VLAN_ALL;
>         parsed_cmd->err = 0;
> +       parsed_cmd->mac_addr = NULL;

Drop this.

>         parsed_cmd->cmd_function = NULL;
>  }
>
>  static void command_def_cleanup(struct command_def *parsed_cmd)
>  {
> -       /* Nothing to do for now */
> +       /* free MAC address */
> +       if (parsed_cmd->mac_addr) {
> +               free(parsed_cmd->mac_addr);
> +               parsed_cmd->mac_addr = NULL;
> +       }

Don't malloc, and you don't need free.

>  }
>
>  /* function to interpret commands starting with "ethsw " */
> @@ -1461,6 +2093,7 @@ U_BOOT_CMD(ethsw, VSC9953_MAX_CMD_PARAMS, 0, do_ethsw,
>            VSC9953_PORT_CONF_HELP"\n"
>            VSC9953_PORT_STATS_HELP"\n"
>            VSC9953_LEARN_HELP"\n"
> +          VSC9953_FDB_HELP"\n"
>  );
>
>  #endif /* CONFIG_VSC9953_CMD */
> diff --git a/include/vsc9953.h b/include/vsc9953.h
> index 59c85c3..051c24e 100644
> --- a/include/vsc9953.h
> +++ b/include/vsc9953.h
> @@ -93,6 +93,25 @@
>  #define        CONFIG_VSC9953_VCAP_MV_CFG      0x0000ffff
>  #define        CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004
>
> +/* Macros for register vsc9953_ana_ana_tables.mac_access register */
> +#define CONFIG_VSC9953_MAC_CMD_IDLE    0x00000000
> +#define CONFIG_VSC9953_MAC_CMD_LEARN   0x00000001
> +#define CONFIG_VSC9953_MAC_CMD_FORGET  0x00000002
> +#define CONFIG_VSC9953_MAC_CMD_AGE     0x00000003
> +#define CONFIG_VSC9953_MAC_CMD_NEXT    0x00000004
> +#define CONFIG_VSC9953_MAC_CMD_READ    0x00000006
> +#define CONFIG_VSC9953_MAC_CMD_WRITE   0x00000007
> +#define CONFIG_VSC9953_MAC_CMD_MASK    0x00000007
> +#define CONFIG_VSC9953_MAC_CMD_VALID   0x00000800
> +#define CONFIG_VSC9953_MAC_ENTRYTYPE_NORMAL    0x00000000
> +#define CONFIG_VSC9953_MAC_ENTRYTYPE_LOCKED    0x00000200
> +#define CONFIG_VSC9953_MAC_ENTRYTYPE_IPV4MCAST 0x00000400
> +#define CONFIG_VSC9953_MAC_ENTRYTYPE_IPV6MCAST 0x00000600
> +#define CONFIG_VSC9953_MAC_ENTRYTYPE_MASK      0x00000600
> +#define CONFIG_VSC9953_MAC_DESTIDX_MASK        0x000001f8
> +#define CONFIG_VSC9953_MAC_VID_MASK    0x1fff0000
> +#define CONFIG_VSC9953_MAC_MACH_MASK   0x0000ffff
> +
>  /* Macros for vsc9953_ana_port.vlan_cfg register */
>  #define CONFIG_VSC9953_VLAN_CFG_AWARE_ENA              0x00100000
>  #define CONFIG_VSC9953_VLAN_CFG_POP_CNT_MASK           0x000c0000
> @@ -131,6 +150,15 @@
>  #define CONFIG_VSC9953_TAG_CFG_ALL_ZERO                0x00000100
>  #define CONFIG_VSC9953_TAG_CFG_ALL     0x00000180
>
> +/* Macros for vsc9953_ana_ana.anag_efil register */
> +#define CONFIG_VSC9953_AGE_PORT_EN     0x00080000
> +#define CONFIG_VSC9953_AGE_PORT_MASK   0x0007c000
> +#define CONFIG_VSC9953_AGE_VID_EN      0x00002000
> +#define CONFIG_VSC9953_AGE_VID_MASK    0x00001fff
> +
> +/* Macros for vsc9953_ana_ana_tables.mach_data register */
> +#define CONFIG_VSC9953_MACHDATA_VID_MASK       0x1fff0000

Drop "CONFIG_" from all these.

> +
>  #define VSC9953_MAX_PORTS              10
>  #define VSC9953_PORT_CHECK(port)       \
>         (((port) < 0 || (port) >= VSC9953_MAX_PORTS) ? 0 : 1)
> --
> 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