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

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@freescale.com>
Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@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(-)

Hi Codrin,

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

Hi Joe,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
> Sent: Friday, June 26, 2015 1:39 AM
> To: Ciubotariu Codrin Constantin-B43658
> Cc: u-boot; Joe Hershberger; Sun York-R58495
> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
> manipulate the FDB for VSC9953
>
> > +       return !!timeout;
> 
> Maybe return -EBUSY like suggested in previous patch.

Ok.

> > +       /* 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.

Ok.

> > +               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.

Yes, something like #define field_set(val, mask)           (((val) * ((mask) & ~((mask) << 1))) & mask) and
#define field_get(val, mask)           ((val & mask) / ((mask) & ~((mask) << 1))). 

> > +       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()?

To assure that the shifted vid value is not higher than its mask. Adding the mask to the macro/inline function as described above should assure this.

> > +       /* 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()?

Same here.

> > +       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()?

Same here.

> > +       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()?

Same here.

> > +       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()?

Same here.

> > +       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).

Ok.

> 
> > +       }
> > +
> > +       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.

Ok.

> > +       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.

Ok.

> > +#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"

Ok.

> > +       /* check if MAC address is present */
> > +       if (!parsed_cmd->mac_addr) {
> 
> Use this:
> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {

is_valid_ethaddr() returns false if the mac address is 00:00:00:00:00:00, but for the L2 Switch, this mac address is valid. Maybe is_broadcast_ethaddr()?

> > +       /* check if MAC address is present */
> > +       if (!parsed_cmd->mac_addr) {
> 
> Use this:
> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {

Same as above.

> > +/* 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.

Yes, I noticed that this function is already implemented, but with no access to it. I will see how I can use the one available.

> > +       parsed_cmd->mac_addr = malloc(6);
> 
> Why malloc this? It is small and known size.

I will declare the array statically.

> > +       if (is_broadcast_ethaddr(parsed_cmd->mac_addr)) {
> > +               free(parsed_cmd->mac_addr);
> > +               parsed_cmd->mac_addr = NULL;
> 
> Drop these two lines.

Ok.
> > -       /* 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.

Ok.

> >  #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.

Ok.

Thanks and best regards,
Codrin

Hi Codrin,

On Tue, Jun 30, 2015 at 8:31 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> Hi Joe,
>
>> -----Original Message-----
>> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
>> Sent: Friday, June 26, 2015 1:39 AM
>> To: Ciubotariu Codrin Constantin-B43658
>> Cc: u-boot; Joe Hershberger; Sun York-R58495
>> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
>> manipulate the FDB for VSC9953
>>
>> > +       return !!timeout;
>>
>> Maybe return -EBUSY like suggested in previous patch.
>
> Ok.
>
>> > +       /* 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.
>
> Ok.
>
>> > +               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.
>
> Yes, something like #define field_set(val, mask)           (((val) * ((mask) & ~((mask) << 1))) & mask) and
> #define field_get(val, mask)           ((val & mask) / ((mask) & ~((mask) << 1))).

The set seems the same as replace, just without any other bitfields to
preserve. You could just use replace and pass in 0 as the reg_val.

>> > +       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()?
>
> To assure that the shifted vid value is not higher than its mask. Adding the mask to the macro/inline function as described above should assure this.

OK. Maybe the existing bitfield_replace() should be updated to also
mask off the bitfield_val.

>> > +       /* check if MAC address is present */
>> > +       if (!parsed_cmd->mac_addr) {
>>
>> Use this:
>> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
>
> is_valid_ethaddr() returns false if the mac address is 00:00:00:00:00:00, but for the L2 Switch, this mac address is valid. Maybe is_broadcast_ethaddr()?

I'm not sure what criteria you intend to check for here, so I'm not
sure if that is appropriate.

>> > +       /* check if MAC address is present */
>> > +       if (!parsed_cmd->mac_addr) {
>>
>> Use this:
>> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
>
> Same as above.
>
>> > +/* 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.
>
> Yes, I noticed that this function is already implemented, but with no access to it. I will see how I can use the one available.

Which path do you plan to take?

Thanks,
-Joe

Hi Joe,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
> Sent: Wednesday, July 01, 2015 1:50 AM
> To: Ciubotariu Codrin Constantin-B43658
> Cc: u-boot; Joe Hershberger; Sun York-R58495
> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
> manipulate the FDB for VSC9953
> 
> Hi Codrin,
> 
> On Tue, Jun 30, 2015 at 8:31 AM, Codrin Constantin Ciubotariu
> <codrin.ciubotariu@freescale.com> wrote:
> > Hi Joe,
> >
> >> -----Original Message-----
> >> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
> >> Sent: Friday, June 26, 2015 1:39 AM
> >> To: Ciubotariu Codrin Constantin-B43658
> >> Cc: u-boot; Joe Hershberger; Sun York-R58495
> >> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
> >> manipulate the FDB for VSC9953
> >>
> >> > +       return !!timeout;
> >>
> >> Maybe return -EBUSY like suggested in previous patch.
> >
> > Ok.
> >
> >> > +       /* 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.
> >
> > Ok.
> >
> >> > +               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.
> >
> > Yes, something like #define field_set(val, mask)           (((val) * ((mask) &
> ~((mask) << 1))) & mask) and
> > #define field_get(val, mask)           ((val & mask) / ((mask) & ~((mask) <<
> 1))).
> 
> The set seems the same as replace, just without any other bitfields to
> preserve. You could just use replace and pass in 0 as the reg_val.

Yes, although I still have to pass the "shift" parameter.

> 
> >> > +       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()?
> >
> > To assure that the shifted vid value is not higher than its mask. Adding the
> mask to the macro/inline function as described above should assure this.
> 
> OK. Maybe the existing bitfield_replace() should be updated to also
> mask off the bitfield_val.

Yes, I think it should be updated. Should I make a separate patch for it? It should be out of this patch set since it has nothing to do with VSC9953.

> 
> >> > +       /* check if MAC address is present */
> >> > +       if (!parsed_cmd->mac_addr) {
> >>
> >> Use this:
> >> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
> >
> > is_valid_ethaddr() returns false if the mac address is 00:00:00:00:00:00, but
> for the L2 Switch, this mac address is valid. Maybe is_broadcast_ethaddr()?
> 
> I'm not sure what criteria you intend to check for here, so I'm not
> sure if that is appropriate.

When a command for adding/removing a mac address is issued, we should check if there is a valid mac address in parsed_cmd->mac_addr. I guess that this could be checked only in keyword_match_mac_addr(). Also, since mac_addr will become a static array, I should initialize this array with an invalid mac address. Since 0 mac address is a valid address for the L2 Switch, I was thinking to initialize it with L2 Broadcast address. What do you think?

> 
> >> > +       /* check if MAC address is present */
> >> > +       if (!parsed_cmd->mac_addr) {
> >>
> >> Use this:
> >> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
> >
> > Same as above.
> >
> >> > +/* 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.
> >
> > Yes, I noticed that this function is already implemented, but with no access
> to it. I will see how I can use the one available.
> 
> Which path do you plan to take?

I looked through both eth_parse_enetaddr() and _env_flags_validate_type() and I prefer the implementation from the latter function, since eth_parse_enetaddr() doesn't check for ':' or if the value returned by simple_strtoul() is <= 0xFF. I could move the code from "case env_flags_vartype_macaddr:" to a separate function (something like eth_check_enetaddr()? ) and add its prototype somewhere in include/net.h. What do you think?

Best regards,
Codrin

Hi Codrin,

On Thu, Jul 2, 2015 at 3:44 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> Hi Joe,
>
>> -----Original Message-----
>> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
>> Sent: Wednesday, July 01, 2015 1:50 AM
>> To: Ciubotariu Codrin Constantin-B43658
>> Cc: u-boot; Joe Hershberger; Sun York-R58495
>> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
>> manipulate the FDB for VSC9953
>>
>> Hi Codrin,
>>
>> On Tue, Jun 30, 2015 at 8:31 AM, Codrin Constantin Ciubotariu
>> <codrin.ciubotariu@freescale.com> wrote:
>> > Hi Joe,
>> >
>> >> -----Original Message-----
>> >> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
>> >> Sent: Friday, June 26, 2015 1:39 AM
>> >> To: Ciubotariu Codrin Constantin-B43658
>> >> Cc: u-boot; Joe Hershberger; Sun York-R58495
>> >> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
>> >> manipulate the FDB for VSC9953
>> >>
>> >> > +       return !!timeout;
>> >>
>> >> Maybe return -EBUSY like suggested in previous patch.
>> >
>> > Ok.
>> >
>> >> > +       /* 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.
>> >
>> > Ok.
>> >
>> >> > +               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.
>> >
>> > Yes, something like #define field_set(val, mask)           (((val) * ((mask) &
>> ~((mask) << 1))) & mask) and
>> > #define field_get(val, mask)           ((val & mask) / ((mask) & ~((mask) <<
>> 1))).
>>
>> The set seems the same as replace, just without any other bitfields to
>> preserve. You could just use replace and pass in 0 as the reg_val.
>
> Yes, although I still have to pass the "shift" parameter.

Not if you use the new bitfield_replace_by_mask().

>>
>> >> > +       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()?
>> >
>> > To assure that the shifted vid value is not higher than its mask. Adding the
>> mask to the macro/inline function as described above should assure this.
>>
>> OK. Maybe the existing bitfield_replace() should be updated to also
>> mask off the bitfield_val.
>
> Yes, I think it should be updated. Should I make a separate patch for it? It should be out of this patch set since it has nothing to do with VSC9953.

Definitely a separate patch.

>> >> > +       /* check if MAC address is present */
>> >> > +       if (!parsed_cmd->mac_addr) {
>> >>
>> >> Use this:
>> >> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
>> >
>> > is_valid_ethaddr() returns false if the mac address is 00:00:00:00:00:00, but
>> for the L2 Switch, this mac address is valid. Maybe is_broadcast_ethaddr()?
>>
>> I'm not sure what criteria you intend to check for here, so I'm not
>> sure if that is appropriate.
>
> When a command for adding/removing a mac address is issued, we should check if there is a valid mac address in parsed_cmd->mac_addr. I guess that this could be checked only in keyword_match_mac_addr(). Also, since mac_addr will become a static array, I should initialize this array with an invalid mac address. Since 0 mac address is a valid address for the L2 Switch, I was thinking to initialize it with L2 Broadcast address. What do you think?

Sounds good.

>> >> > +       /* check if MAC address is present */
>> >> > +       if (!parsed_cmd->mac_addr) {
>> >>
>> >> Use this:
>> >> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
>> >
>> > Same as above.
>> >
>> >> > +/* 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.
>> >
>> > Yes, I noticed that this function is already implemented, but with no access
>> to it. I will see how I can use the one available.
>>
>> Which path do you plan to take?
>
> I looked through both eth_parse_enetaddr() and _env_flags_validate_type() and I prefer the implementation from the latter function, since eth_parse_enetaddr() doesn't check for ':' or if the value returned by simple_strtoul() is <= 0xFF. I could move the code from "case env_flags_vartype_macaddr:" to a separate function (something like eth_check_enetaddr()? ) and add its prototype somewhere in include/net.h. What do you think?

I think it should be called eth_validate_ethaddr_str(). You can add it
to include/net.h and the top of net/eth.c

Cheers,
-Joe