Hi Joe,

...

    /* alloc eth device */
    dev = (struct eth_device *)calloc(1, sizeof(struct eth_device));
    if (!dev)

•           return 1;
  

•           return -1;
  

Is it reasonable to use values from asm/errno.h here and elsewhere in the driver? This seems like it should return -ENODEV instead of -EPERM.

Yes, I should use values from errno.h . -ENOMEM seems more appropriate to me. What do you think?

...

•   enabled = !!(vsc9953_l2sw.port[port_no].enabled &
  

•                val & CONFIG_VSC9953_PORT_ENA);
  

This is incorrect... Should be:

•   enabled = vsc9953_l2sw.port[port_no].enabled &&
  

•                (val & CONFIG_VSC9953_PORT_ENA);
  

Ok.

...

diff --git a/include/vsc9953.h b/include/vsc9953.h index 3d11b87..920402f 100644 --- a/include/vsc9953.h +++ b/include/vsc9953.h @@ -33,29 +33,60 @@ #define T1040_SWITCH_GMII_DEV_OFFSET 0x010000 #define VSC9953_PHY_REGS_OFFST 0x0000AC

+/* Macros for vsc9953_chip_regs.soft_rst register */ #define CONFIG_VSC9953_SOFT_SWC_RST_ENA 0x00000001

All of there that are register constants should not have "CONFIG_" prepended to them. That should only be for constants that configure something, eventually only from Kconfig. Please add another patch before this one that removes that.

Ok, I will add another patch before this one.

...

+/* Macros for vsc9953_sys_pause_cfg.pause_cfg register */ #define CONFIG_VSC9953_PAUSE_CFG 0x001ffffe

+/* Macros for vsc9953_sys_pause_cfg.pause_cfg register */ +#define CONFIG_VSC9953_PAUSE_CFG 0x001ffffe

This adds a duplicate of the define above it.

I will remove one of them.

...

+/* Macros for vsc9953_vcap_core_cfg.vcap_mv_cfg register */ #define CONFIG_VSC9953_VCAP_MV_CFG 0x0000ffff #define CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004

May as well get rid of the tabs here after the defines.

Ok.

Thank you for your review. I will make v3.

Best regards, Codrin

Hi Codrin,

On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu codrin.ciubotariu@freescale.com wrote:

At startup, the default configuration should be:

• enable HW learning on all ports (HW default);
• all ports are VLAN aware;
• all ports are members of VLAN 1;
• all ports have Port-based VLAN 1;
• on all ports, the switch is allowed to remove maximum one VLAN tag,
• on egress, the switch should add a VLAN tag if the frame is classified to a different VLAN than the port's Port-based VLAN;

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 | 266 +++++++++++++++++++++++++++++++++++++++++++++++++- include/vsc9953.h | 61 +++++++++++- 2 files changed, 325 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c index 720ae47..9dec683 100644 --- a/drivers/net/vsc9953.c +++ b/drivers/net/vsc9953.c @@ -1,5 +1,5 @@ /*

• • Copyright 2014 Freescale Semiconductor, Inc.

• • Copyright 2014-2015 Freescale Semiconductor, Inc.
  • SPDX-License-Identifier: GPL-2.0+

@@ -176,6 +176,268 @@ static int vsc9953_port_init(int port_no) return 0; }

+static int vsc9953_vlan_table_poll_idle(void) +{

•   struct vsc9953_analyzer         *l2ana_reg;
  

•   int                             timeout;
  

•   l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
  

•                   VSC9953_ANA_OFFSET);
  

•   timeout = 50000;
  

•   while (((in_le32(&l2ana_reg->ana_tables.vlan_access) &
  

•                   CONFIG_VSC9953_VLAN_CMD_MASK) !=
  

•           CONFIG_VSC9953_VLAN_CMD_IDLE) && --timeout)
  

•           udelay(1);
  

•   return !!timeout;
  

Maybe this:

+ return timeout ? 0 : -EBUSY;

+}

+/* vlan table set/clear all membership of vid */ +static void vsc9953_vlan_table_membership_all_set(int vid, int set) +{

•   struct vsc9953_analyzer         *l2ana_reg;
  

•   l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
  

•                   VSC9953_ANA_OFFSET);
  

•   if (!vsc9953_vlan_table_poll_idle()) {
  

If you accept the above, you need to change these to: + if (vsc9953_vlan_table_poll_idle() < 0) {

or

+ if (vsc9953_vlan_table_poll_idle() == -EBUSY) {

•           debug("VLAN table timeout\n");
  

•           return;
  

•   }
  

•   /* read current vlan configuration */
  

•   clrsetbits_le32(&l2ana_reg->ana_tables.vlan_tidx,
  

•                   CONFIG_VSC9953_ANA_TBL_VID_MASK,
  

•                   field_set(vid, CONFIG_VSC9953_ANA_TBL_VID_MASK));
  

•   clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access,
  

•                   CONFIG_VSC9953_VLAN_CMD_MASK,
  

•                   field_set(CONFIG_VSC9953_VLAN_CMD_READ,
  

•                             CONFIG_VSC9953_VLAN_CMD_MASK));
  

•   if (!vsc9953_vlan_table_poll_idle()) {
  

Here too.

•           debug("VLAN table timeout\n");
  

•           return;
  

•   }
  

•   clrsetbits_le32(&l2ana_reg->ana_tables.vlan_tidx,
  

•                   CONFIG_VSC9953_ANA_TBL_VID_MASK,
  

•                   field_set(vid, CONFIG_VSC9953_ANA_TBL_VID_MASK));
  

•   if (!set)
  

•           clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access,
  

•                           CONFIG_VSC9953_VLAN_PORT_MASK |
  

•                           CONFIG_VSC9953_VLAN_CMD_MASK,
  

•                           field_set(CONFIG_VSC9953_VLAN_CMD_WRITE,
  

•                                     CONFIG_VSC9953_VLAN_CMD_MASK));
  

•   else
  

•           clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access,
  

•                           CONFIG_VSC9953_VLAN_PORT_MASK |
  

•                           CONFIG_VSC9953_VLAN_CMD_MASK,
  

•                           field_set(CONFIG_VSC9953_VLAN_CMD_WRITE,
  

•                                     CONFIG_VSC9953_VLAN_CMD_MASK) |
  

•                           CONFIG_VSC9953_VLAN_PORT_MASK);
  

It seems this could if statement could all be simplified as: + clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access, + CONFIG_VSC9953_VLAN_PORT_MASK | + CONFIG_VSC9953_VLAN_CMD_MASK, + field_set(CONFIG_VSC9953_VLAN_CMD_WRITE, + CONFIG_VSC9953_VLAN_CMD_MASK) | + (set ? CONFIG_VSC9953_VLAN_PORT_MASK : 0));

It may also help to rename the parameter from "set" to something like "set_member".

+}

+/* Set PVID for a VSC9953 port */ +static void vsc9953_port_vlan_pvid_set(int port_no, int pvid) +{

•   struct vsc9953_analyzer *l2ana_reg;
  

•   struct vsc9953_rew_reg  *l2rew_reg;
  

•   /* Administrative down */
  

•   if ((!vsc9953_l2sw.port[port_no].enabled)) {
  

Why do you have double "((" and "))"?

•           printf("Port %d is administrative down\n", port_no);
  

•           return;
  

•   }
  

•   l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
  

•                   VSC9953_ANA_OFFSET);
  

•   l2rew_reg = (struct vsc9953_rew_reg *)(VSC9953_OFFSET +
  

•                   VSC9953_REW_OFFSET);
  

•   /* Set PVID on ingress */
  

•   clrsetbits_le32(&l2ana_reg->port[port_no].vlan_cfg,
  

•                   CONFIG_VSC9953_VLAN_CFG_VID_MASK,
  

•                   field_set(pvid, CONFIG_VSC9953_VLAN_CFG_VID_MASK));
  

•   /* Set PVID on egress */
  

•   clrsetbits_le32(&l2rew_reg->port[port_no].port_vlan_cfg,
  

•                   CONFIG_VSC9953_PORT_VLAN_CFG_VID_MASK,
  

•                   field_set(pvid, CONFIG_VSC9953_PORT_VLAN_CFG_VID_MASK));
  

+}

+static void vsc9953_port_all_vlan_pvid_set(int pvid) +{

•   int             i;
  

Use a single space.

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_port_vlan_pvid_set(i, pvid);
  

+}

+/* Enable/disable vlan aware of a VSC9953 port */ +static void vsc9953_port_vlan_aware_set(int port_no, int enabled) +{

•   struct vsc9953_analyzer         *l2ana_reg;
  

•   /* Administrative down */
  

•   if (!vsc9953_l2sw.port[port_no].enabled) {
  

•           printf("Port %d is administrative down\n", port_no);
  

•           return;
  

•   }
  

•   l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
  

•                   VSC9953_ANA_OFFSET);
  

•   if (enabled)
  

•           setbits_le32(&l2ana_reg->port[port_no].vlan_cfg,
  

•                        CONFIG_VSC9953_VLAN_CFG_AWARE_ENA);
  

•   else
  

•           clrbits_le32(&l2ana_reg->port[port_no].vlan_cfg,
  

•                        CONFIG_VSC9953_VLAN_CFG_AWARE_ENA);
  

+}

+/* Set all VSC9953 ports' vlan aware */ +static void vsc9953_port_all_vlan_aware_set(int enabled) +{

•   int             i;
  

Use a single space.

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_port_vlan_aware_set(i, enabled);
  

+}

+/* Enable/disable vlan pop count of a VSC9953 port */ +static void vsc9953_port_vlan_popcnt_set(int port_no, int popcnt) +{

•   struct vsc9953_analyzer         *l2ana_reg;
  

Use a single space.

•   /* Administrative down */
  

•   if (!vsc9953_l2sw.port[port_no].enabled) {
  

•           printf("Port %d is administrative down\n", port_no);
  

•           return;
  

•   }
  

•   if (popcnt > 3 || popcnt < 0) {
  

•           printf("Invalid pop count value: %d\n", port_no);
  

•           return;
  

•   }
  

•   l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
  

•                   VSC9953_ANA_OFFSET);
  

•   clrsetbits_le32(&l2ana_reg->port[port_no].vlan_cfg,
  

•                   CONFIG_VSC9953_VLAN_CFG_POP_CNT_MASK,
  

•                   field_set(popcnt,
  

•                             CONFIG_VSC9953_VLAN_CFG_POP_CNT_MASK));
  

+}

+/* Set all VSC9953 ports' pop count */ +static void vsc9953_port_all_vlan_poncnt_set(int popcnt) +{

•   int             i;
  

Use a single space.

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_port_vlan_popcnt_set(i, popcnt);
  

+}

+/* Enable/disable learning for frames dropped due to ingress filtering */ +static void vsc9953_vlan_ingr_fltr_learn_drop(int enable) +{

•   struct vsc9953_analyzer         *l2ana_reg;
  

Use a single space.

•   l2ana_reg = (struct vsc9953_analyzer *)(VSC9953_OFFSET +
  

•                   VSC9953_ANA_OFFSET);
  

•   if (enable)
  

•           setbits_le32(&l2ana_reg->ana.adv_learn,
  

•                        CONFIG_VSC9953_VLAN_CHK);
  

•   else
  

•           clrbits_le32(&l2ana_reg->ana.adv_learn,
  

•                        CONFIG_VSC9953_VLAN_CHK);
  

+}

+/* Egress untag modes of a VSC9953 port */ +enum egress_untag_mode {

•   EGRESS_UNTAG_ALL = 0,
  

•   EGRESS_UNTAG_PVID_AND_ZERO,
  

•   EGRESS_UNTAG_ZERO,
  

•   EGRESS_UNTAG_NONE,
  

+};

+static void vsc9953_port_vlan_egr_untag_set(int port_no,

•                                       enum egress_untag_mode mode)
  

+{

•   struct vsc9953_rew_reg  *l2rew_reg;
  

Use a single space.

•   /* Administrative down */
  

•   if (!vsc9953_l2sw.port[port_no].enabled) {
  

•           printf("Port %d is administrative down\n", port_no);
  

•           return;
  

•   }
  

•   l2rew_reg = (struct vsc9953_rew_reg *)(VSC9953_OFFSET +
  

•                   VSC9953_REW_OFFSET);
  

•   switch (mode) {
  

•   case EGRESS_UNTAG_ALL:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_NONE);
  

•           break;
  

•   case EGRESS_UNTAG_PVID_AND_ZERO:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO);
  

This seems like the naming is inverted. The enum value is called "untag" pvid and zero, but the config is called "tag" all pvid and zero. Is this a bug or just poorly named constants / enum values?

•           break;
  

•   case EGRESS_UNTAG_ZERO:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_ALL_ZERO);
  

Also here.

•           break;
  

•   case EGRESS_UNTAG_NONE:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_ALL);
  

•           break;
  

•   default:
  

•           printf("Unknown untag mode for port %d\n", port_no);
  

•   }
  

+}

+static void vsc9953_port_all_vlan_egress_untagged_set(

•           enum egress_untag_mode mode)
  

+{

•   int             i;
  

Use a single space.

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_port_vlan_egr_untag_set(i, mode);
  

+}

+/***************************************************************************** +At startup, the default configuration would be:

•   - HW learning enabled on all ports; (HW default)
  

•   - All ports are in VLAN 1;
  

•   - All ports are VLAN aware;
  

•   - All ports have POP_COUNT 1;
  

•   - All ports have PVID 1;
  

•   - All ports have TPID 0x8100; (HW default)
  

•   - All ports tag frames classified to all VLANs that are not PVID;
  

+*****************************************************************************/ +void vsc9953_default_configuration(void) +{

•   int i;
  

•   for (i = 0; i < VSC9953_MAX_VLAN; i++)
  

•           vsc9953_vlan_table_membership_all_set(i, 0);
  

•   vsc9953_port_all_vlan_aware_set(1);
  

•   vsc9953_port_all_vlan_pvid_set(1);
  

•   vsc9953_port_all_vlan_poncnt_set(1);
  

•   vsc9953_vlan_table_membership_all_set(1, 1);
  

•   vsc9953_vlan_ingr_fltr_learn_drop(1);
  

•   vsc9953_port_all_vlan_egress_untagged_set(EGRESS_UNTAG_PVID_AND_ZERO);
  

+}

void vsc9953_init(bd_t *bis) { u32 i, hdx_cfg = 0, phy_addr = 0; @@ -306,6 +568,8 @@ void vsc9953_init(bd_t *bis) } }

•   vsc9953_default_configuration();
  

•   printf("VSC9953 L2 switch initialized\n");
    return;
  

} diff --git a/include/vsc9953.h b/include/vsc9953.h index 2b88c5c..bf81623 100644 --- a/include/vsc9953.h +++ b/include/vsc9953.h @@ -7,7 +7,7 @@

• terms of the GNU Public License, Version 2, incorporated
• herein by reference.

• • Copyright 2013 Freescale Semiconductor, Inc.

• • Copyright 2013, 2015 Freescale Semiconductor, Inc.
  */

@@ -19,9 +19,13 @@ #include <asm/types.h> #include <malloc.h>

+#define field_set(val, mask) ((val) * ((mask) & ~((mask) << 1))) +#define field_get(val, mask) ((val) / ((mask) & ~((mask) << 1)))

I don't follow why this is unique to this chip? Also, get is never used. Is it just for completeness, I assume.

I think you should either be using the functions in include/bitfield.h or you should be adding these there instead of here. If you decide to add them there, then naturally do it as a separate patch and with good comments and naming consistent with that file and as functions not macros. This method is nice in that you use the mask to define the shift instead of requiring it as a separate constant.

#define VSC9953_OFFSET (CONFIG_SYS_CCSRBAR_DEFAULT + 0x800000)

#define VSC9953_SYS_OFFSET 0x010000 +#define VSC9953_REW_OFFSET 0x030000 #define VSC9953_DEV_GMII_OFFSET 0x100000 #define VSC9953_QSYS_OFFSET 0x200000 #define VSC9953_ANA_OFFSET 0x280000 @@ -84,9 +88,38 @@ #define CONFIG_VSC9953_VCAP_MV_CFG 0x0000ffff #define CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004

+/* 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 +#define CONFIG_VSC9953_VLAN_CFG_VID_MASK 0x00000fff

+/* Macros for vsc9953_rew_port.port_vlan_cfg register */ +#define CONFIG_VSC9953_PORT_VLAN_CFG_VID_MASK 0x00000fff

+/* Macros for vsc9953_ana_ana_tables.vlan_tidx register */ +#define CONFIG_VSC9953_ANA_TBL_VID_MASK 0x00000fff

+/* Macros for vsc9953_ana_ana_tables.vlan_access register */ +#define CONFIG_VSC9953_VLAN_PORT_MASK 0x00001ffc +#define CONFIG_VSC9953_VLAN_CMD_MASK 0x00000003 +#define CONFIG_VSC9953_VLAN_CMD_IDLE 0x00000000 +#define CONFIG_VSC9953_VLAN_CMD_READ 0x00000001 +#define CONFIG_VSC9953_VLAN_CMD_WRITE 0x00000002 +#define CONFIG_VSC9953_VLAN_CMD_INIT 0x00000003

/* Macros for vsc9953_qsys_sys.switch_port_mode register */ #define CONFIG_VSC9953_PORT_ENA 0x00002000

+/* Macros for vsc9953_ana_ana.adv_learn register */ +#define CONFIG_VSC9953_VLAN_CHK 0x00000400

+/* Macros for vsc9953_rew_port.port_tag_cfg register */ +#define CONFIG_VSC9953_TAG_CFG_MASK 0x00000180 +#define CONFIG_VSC9953_TAG_CFG_NONE 0x00000000 +#define CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO 0x00000080 +#define CONFIG_VSC9953_TAG_CFG_ALL_ZERO 0x00000100 +#define CONFIG_VSC9953_TAG_CFG_ALL 0x00000180

#define VSC9953_MAX_PORTS 10 #define VSC9953_PORT_CHECK(port) \ (((port) < 0 || (port) >= VSC9953_MAX_PORTS) ? 0 : 1) @@ -95,6 +128,9 @@ (port) < VSC9953_MAX_PORTS - 2 || (port) >= VSC9953_MAX_PORTS \ ) ? 0 : 1 \ ) +#define VSC9953_MAX_VLAN 4096 +#define VSC9953_VLAN_CHECK(vid) \

•   (((vid) < 0 || (vid) >= VSC9953_MAX_VLAN) ? 0 : 1)
  
  

#define DEFAULT_VSC9953_MDIO_NAME "VSC9953_MDIO0"

@@ -342,6 +378,29 @@ struct vsc9953_system_reg {

/* END VSC9953 SYS structure for T1040 U-boot*/

+/* VSC9953 REW structure for T1040 U-boot*/

+struct vsc9953_rew_port {

•   u32     port_vlan_cfg;
  

•   u32     port_tag_cfg;
  

•   u32     port_port_cfg;
  

•   u32     port_dscp_cfg;
  

•   u32     port_pcp_dei_qos_map_cfg[16];
  

Seems like you should drop the "port_" from all of these member names.

•   u32     reserved[12];
  

+};

+struct vsc9953_rew_common {

•   u32     reserve[4];
  

•   u32     dscp_remap_dp1_cfg[64];
  

•   u32     dscp_remap_cfg[64];
  

+};

+struct vsc9953_rew_reg {

•   struct vsc9953_rew_port port[12];
  

•   struct vsc9953_rew_common common;
  

+};

+/* END VSC9953 REW structure for T1040 U-boot*/

These comments seem gratuitous and not particularly relevant (begin and end). Perhaps either remove them throughout the file or at least don't add more. At the very least, drop the " structure for T1040 U-boot" which isn't helpful or accurate.

/* VSC9953 DEVCPU_GCB structure for T1040 U-boot*/

-- 1.9.3

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Hi Codrin,

On Tue, Jun 30, 2015 at 2:51 AM, Codrin Constantin Ciubotariu codrin.ciubotariu@freescale.com wrote:

Hi Joe,

I removed the lines on which we agreed on...

...

...

...

...

•   switch (mode) {
  

•   case EGRESS_UNTAG_ALL:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_NONE);
  

•           break;
  

•   case EGRESS_UNTAG_PVID_AND_ZERO:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

• CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO);

This seems like the naming is inverted. The enum value is called "untag" pvid and zero, but the config is called "tag" all pvid and zero. Is this a bug or just poorly named constants / enum values?

...

•           break;
  

•   case EGRESS_UNTAG_ZERO:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_ALL_ZERO);
  

Also here.

...

•           break;
  

•   case EGRESS_UNTAG_NONE:
  

•           clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
  

•                           CONFIG_VSC9953_TAG_CFG_MASK,
  

•                           CONFIG_VSC9953_TAG_CFG_ALL);
  

•           break;
  

•   default:
  

•           printf("Unknown untag mode for port %d\n", port_no);
  

•   }
  

Yes, the naming is inverted. The main reason for this is that I couldn't find a short and easy to use command to configure a port's egress to send all frames VLAN tagged except when the VLAN ID equals the Port

VLAN ID.

...

I decided to make a command to tell the switch for which VLAN ID's not to tag a frame (untag) instead of making a command to tell the switch for which VLAN IDs to tag the frame (tag). So, for example, the command "ethsw [port <port_no>] tag all except pvid" or "ethsw [port <port_no>] tag !pvid" became "ethsw [port <port_no>] untagged pvid". If you think this is not intuitive for both users and developers, I will try to find something more appropriate.

I don't have a problem with using the inverted logic if that's what typical use- cases call for, what I was referring to was those two specific examples. The "all" and "none" seem correctly inverted.

In the other 2 cases, the "tag" vs "untag" is inverted, but the subject is not "PVID_AND_ZERO" vs "ALL_PVID_ZERO"

"EGRESS_UNTAG_PVID_AND_ZERO" -> "CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO", for example. That's the discrepancy I'm concerned about.

Ok, should I rename the constants to something like VSC9953_TAG_CFG_ALL_BUT_PRIV_ZERO instead of CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO and VSC9953_TAG_CFG_ALL_BUT_ZERO instead of CONFIG_VSC9953_TAG_CFG_ALL_ZERO?

I assume you meant to say VSC9953_TAG_CFG_ALL_BUT_*PVID*_ZERO here.

If so, I think that's clear enough.

...

...

...

...

+#define field_set(val, mask) ((val) * ((mask) & ~((mask) << 1))) +#define field_get(val, mask) ((val) / ((mask) & ~((mask) << 1)))

I don't follow why this is unique to this chip? Also, get is never used. Is it just for completeness, I assume.

I think you should either be using the functions in include/bitfield.h or you should be adding these there instead of here. If you decide to add them there, then naturally do it as a separate patch and with good comments and naming consistent with that file and as functions not macros. This method is nice in that you use the mask to define the shift instead of requiring it as a separate constant.

These are not unique to this chip. If you consider them useful, I will make a separate patch and add them (or something similar) to include/bitfield.h .

I think this would be the best approach.

Ok, I will make another patch and add bitfield_set/get() inline functions in include/bitfield.h .

I would recommend you structure it as 3 new functions.

diff --git a/include/bitfield.h b/include/bitfield.h index ec4815c..b685804 100644 --- a/include/bitfield.h +++ b/include/bitfield.h @@ -39,6 +39,12 @@ static inline uint bitfield_mask(uint shift, uint width) return ((1 << width) - 1) << shift; }

+/* Produces a shift of the bitfield given a mask */ +static inline uint bitfield_shift(uint mask) +{ + return ffs(mask) - 1; +} + /* Extract the value of a bitfield found within a given register value */ static inline uint bitfield_extract(uint reg_val, uint shift, uint width) { @@ -56,3 +62,23 @@ static inline uint bitfield_replace(uint reg_val, uint shift, uint width,

return (reg_val & ~mask) | (bitfield_val << shift); } + +/* Extract the value of a bitfield found within a given register value */ +static inline uint bitfield_extract_by_mask(uint reg_val, uint mask) +{ + uint shift = bitfield_shift(mask); + + return (reg_val & mask) >> shift; +} + +/* + * Replace the value of a bitfield found within a given register value + * Returns the newly modified uint value with the replaced field. + */ +static inline uint bitfield_replace_by_mask(uint reg_val, uint mask, + uint bitfield_val) +{ + uint shift = bitfield_shift(mask); + + return (reg_val & ~mask) | ((bitfield_val << shift) & mask); +}

...

...

...

...

+struct vsc9953_rew_common {

•   u32     reserve[4];
  

•   u32     dscp_remap_dp1_cfg[64];
  

•   u32     dscp_remap_cfg[64];
  

+};

+struct vsc9953_rew_reg {

•   struct vsc9953_rew_port port[12];
  

•   struct vsc9953_rew_common common; };
  

+/* END VSC9953 REW structure for T1040 U-boot*/

These comments seem gratuitous and not particularly relevant (begin and end). Perhaps either remove them throughout the file or at least don't add more. At the very least, drop the " structure for T1040 U-boot" which isn't helpful or accurate.

Yes, the " structure for T1040 U-boot" seems irrelevant indeed. I will also remove the other comments if you consider them unnecessary. To me it looks like it groups the structures a bit and might help developers look for a specific register. I will remove them in the patch with the clean-up.

If you think the bracketing of these structs adds clarity, then only remove the trailing text. Otherwise remove all of them completely. Up to you; I'm fine with either way.

Ok, I will remove the trailing text and I will see if the remaining comments make sense.

...

Thanks, -Joe

Thanks and best regards, Codrin

Cheers, -Joe

Hi Codrin,

On Wed, Jul 1, 2015 at 9:31 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:26 AM To: Ciubotariu Codrin Constantin-B43658 Cc: u-boot; Joe Hershberger; Sun York-R58495 Subject: Re: [U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch

Hi Codrin,

On Tue, Jun 30, 2015 at 2:51 AM, Codrin Constantin Ciubotariu codrin.ciubotariu@freescale.com wrote:

...

Hi Joe,

I removed the lines on which we agreed on...

...

...

...

> + switch (mode) { > + case EGRESS_UNTAG_ALL: > + clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg, > + CONFIG_VSC9953_TAG_CFG_MASK, > + CONFIG_VSC9953_TAG_CFG_NONE); > + break; > + case EGRESS_UNTAG_PVID_AND_ZERO: > + clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg, > + CONFIG_VSC9953_TAG_CFG_MASK, > + > + CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO);

This seems like the naming is inverted. The enum value is called "untag" pvid and zero, but the config is called "tag" all pvid and zero. Is this a bug or just poorly named constants / enum values?

> + break; > + case EGRESS_UNTAG_ZERO: > + clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg, > + CONFIG_VSC9953_TAG_CFG_MASK, > + CONFIG_VSC9953_TAG_CFG_ALL_ZERO);

Also here.

> + break; > + case EGRESS_UNTAG_NONE: > + clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg, > + CONFIG_VSC9953_TAG_CFG_MASK, > + CONFIG_VSC9953_TAG_CFG_ALL); > + break; > + default: > + printf("Unknown untag mode for port %d\n", port_no); > + }

Yes, the naming is inverted. The main reason for this is that I couldn't find a short and easy to use command to configure a port's egress to send all frames VLAN tagged except when the VLAN ID equals the

Port

...

...

VLAN ID.

...

I decided to make a command to tell the switch for which VLAN ID's not to tag a frame (untag) instead of making a command to tell the switch for which VLAN IDs to tag the frame (tag). So, for example, the command "ethsw [port <port_no>] tag all except pvid" or "ethsw [port <port_no>] tag !pvid" became "ethsw [port <port_no>] untagged pvid". If you think this is not intuitive for both users and developers, I will try to find something more appropriate.

I don't have a problem with using the inverted logic if that's what typical

use-

...

...

cases call for, what I was referring to was those two specific examples. The "all" and "none" seem correctly inverted.

In the other 2 cases, the "tag" vs "untag" is inverted, but the subject is

not

...

...

"PVID_AND_ZERO" vs "ALL_PVID_ZERO"

"EGRESS_UNTAG_PVID_AND_ZERO" -> "CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO", for example. That's the discrepancy

I'm

...

...

concerned about.

Ok, should I rename the constants to something like VSC9953_TAG_CFG_ALL_BUT_PRIV_ZERO instead of CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO and VSC9953_TAG_CFG_ALL_BUT_ZERO instead of CONFIG_VSC9953_TAG_CFG_ALL_ZERO?

I assume you meant to say VSC9953_TAG_CFG_ALL_BUT_*PVID*_ZERO here.

If so, I think that's clear enough.

Yes, VSC9953_TAG_CFG_ALL_BUT_PVID_ZERO and VSC9953_TAG_CFG_ALL_BUT_ZERO.

Sounds good.

...

...

...

...

...

> +#define field_set(val, mask) ((val) * ((mask) & ~((mask) <<

1)))

...

...

...

...

> +#define field_get(val, mask) ((val) / ((mask) & ~((mask) <<

1)))

...

...

...

...

I don't follow why this is unique to this chip? Also, get is never used. Is it just for completeness, I assume.

I think you should either be using the functions in include/bitfield.h or you should be adding these there instead of here. If you decide to add them there, then naturally do it as a separate patch and with good comments and naming consistent with that file and as functions not macros. This method is nice in that you use the mask to define the shift instead of requiring it as a separate

constant.

...

...

...

These are not unique to this chip. If you consider them useful, I will make a separate patch and add them (or something similar) to include/bitfield.h .

I think this would be the best approach.

Ok, I will make another patch and add bitfield_set/get() inline functions in

include/bitfield.h .

I would recommend you structure it as 3 new functions.

diff --git a/include/bitfield.h b/include/bitfield.h index ec4815c..b685804 100644 --- a/include/bitfield.h +++ b/include/bitfield.h @@ -39,6 +39,12 @@ static inline uint bitfield_mask(uint shift, uint width) return ((1 << width) - 1) << shift; }

+/* Produces a shift of the bitfield given a mask */ +static inline uint bitfield_shift(uint mask) +{

•   return ffs(mask) - 1;
  

+}

Ok, should we assure we return 0 if mask is 0? Something like return mask : ffs(mask) - 1 ? 0;

Sounds like a good idea.

...

/* Extract the value of a bitfield found within a given register value */ static inline uint bitfield_extract(uint reg_val, uint shift, uint width) { @@ -56,3 +62,23 @@ static inline uint bitfield_replace(uint reg_val, uint shift, uint width,

    return (reg_val & ~mask) | (bitfield_val << shift);

}

+/* Extract the value of a bitfield found within a given register value */ +static inline uint bitfield_extract_by_mask(uint reg_val, uint mask) +{

•   uint shift = bitfield_shift(mask);
  

•   return (reg_val & mask) >> shift;
  

+}

+/*

• • Replace the value of a bitfield found within a given register value
• • Returns the newly modified uint value with the replaced field.
• */

+static inline uint bitfield_replace_by_mask(uint reg_val, uint mask,

•                                       uint bitfield_val)
  

+{

•   uint shift = bitfield_shift(mask);
  

•   return (reg_val & ~mask) | ((bitfield_val << shift) & mask);
  

+}

Ok.

Best regards, Codrin

Cheers, -Joe

Hi Joe,

-----Original Message----- From: Joe Hershberger [mailto:joe.hershberger@gmail.com] Sent: Friday, June 26, 2015 1:26 AM To: Ciubotariu Codrin Constantin-B43658 Cc: u-boot; Joe Hershberger; Sun York-R58495 Subject: Re: [U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch

...

diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c index 720ae47..9dec683 100644 --- a/drivers/net/vsc9953.c +++ b/drivers/net/vsc9953.c @@ -1,5 +1,5 @@ /*

• • Copyright 2014 Freescale Semiconductor, Inc.

• • Copyright 2014-2015 Freescale Semiconductor, Inc.

This change should be moved to the last patch.

...

• SPDX-License-Identifier: GPL-2.0+

<snip>

...

diff --git a/include/vsc9953.h b/include/vsc9953.h index 2b88c5c..bf81623 100644 --- a/include/vsc9953.h +++ b/include/vsc9953.h @@ -7,7 +7,7 @@

• terms of the GNU Public License, Version 2, incorporated
• herein by reference.

• • Copyright 2013 Freescale Semiconductor, Inc.

• • Copyright 2013, 2015 Freescale Semiconductor, Inc.

This change should be moved to the last patch.

...

*/

<snip>

Ok, I will move these two changes in the last patch.

Best regards, Codrin

Hi Codrin,

On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu codrin.ciubotariu@freescale.com wrote:

In order to support multiple commands to configure the VSC9953 L2 Switch, the parser needs to be changed to be more flexible and to support more complex commands. This patch adds a parser that searches for defined keywords in the command and calls the proper function when a match is found. Also, the parser allows for optional keywords, such as "port", to apply the command on a port or on all ports. The already defined commands are also changed a bit to: ethsw [port <port_no>] { enable | disable | show }

Signed-off-by: Codrin Ciubotariu codrin.ciubotariu@freescale.com

Changes for v2: - removed Change-id field;

drivers/net/vsc9953.c | 381 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 310 insertions(+), 71 deletions(-)

diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c index 9dec683..4df751a 100644 --- a/drivers/net/vsc9953.c +++ b/drivers/net/vsc9953.c @@ -10,6 +10,7 @@ #include <asm/fsl_serdes.h> #include <fm_eth.h> #include <fsl_memac.h> +#include <errno.h> #include <vsc9953.h>

static struct vsc9953_info vsc9953_l2sw = { @@ -575,6 +576,10 @@ void vsc9953_init(bd_t *bis) }

#ifdef CONFIG_VSC9953_CMD

I'd like to see this moved to its own file in common... maybe "common/cmd_ethsw.c". I'd also like to see this #define change to something like "CONFIG_CMD_ETHSW".

These changes don't necessarily need to be part of this series, since it already got in as is, but if you feel motivated, I would recommend you add a patch before this one that moves it.

+#define VSC9953_MAX_CMD_PARAMS 20 +#define VSC9953_CMD_PORT_ALL -1

/* Enable/disable status of a VSC9953 port */ static void vsc9953_port_status_set(int port_no, u8 enabled) { @@ -595,15 +600,6 @@ static void vsc9953_port_status_set(int port_no, u8 enabled) CONFIG_VSC9953_PORT_ENA); }

-/* Set all VSC9953 ports' status */ -static void vsc9953_port_all_status_set(u8 enabled) -{

•   int             i;
  

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_port_status_set(i, enabled);
  

-}

/* Start autonegotiation for a VSC9953 PHY */ static void vsc9953_phy_autoneg(int port_no) { @@ -615,15 +611,6 @@ static void vsc9953_phy_autoneg(int port_no) printf("Failed to start PHY for port %d\n", port_no); }

-/* Start autonegotiation for all VSC9953 PHYs */ -static void vsc9953_phy_all_autoneg(void) -{

•   int             i;
  

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_phy_autoneg(i);
  

-}

/* Print a VSC9953 port's configuration */ static void vsc9953_port_config_show(int port_no) { @@ -685,75 +672,327 @@ static void vsc9953_port_config_show(int port_no) printf("%8s\n", duplex == DUPLEX_FULL ? "full" : "half"); }

-/* Print VSC9953 ports' configuration */ -static void vsc9953_port_all_config_show(void) -{

•   int             i;
  

+/* IDs used to track keywords in a command */ +enum keyword_id {

•   id_key_end = -1,
  

•   id_help,
  

•   id_show,
  

•   id_port,
  

•   id_enable,
  

•   id_disable,
  

•   id_count,       /* keep last */
  

+};

•   for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•           vsc9953_port_config_show(i);
  

-} +enum keyword_opt_id {

•   id_port_no = id_count + 1,
  

•   id_count_all,   /* keep last */
  

+};

-/* function to interpret commands starting with "ethsw " */ -static int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +struct command_def {

•   int cmd_to_keywords[VSC9953_MAX_CMD_PARAMS];
  

•   int cmd_keywords_nr;
  

•   int port;
  

•   int err;
  

Remove this. Just use a return value.

•   int (*cmd_function)(struct command_def *parsed_cmd);
  

+};

+#define VSC9953_PORT_CONF_HELP "[port <port_no>] { enable | disable | show } " \ +"- enable/disable a port; show shows a port's configuration"

Probably better to define this down by the use.

+static int vsc9953_port_status_key_func(struct command_def *parsed_cmd) {

•   u8 enable;
  

•   u32 port;
  

•   int                     i;
  

•   u8                      enabled;
  

Use a single space.

•   if (argc < 4)
  

•   /* Last keyword should tell us if we should enable/disable the port */
  

•   if (parsed_cmd->cmd_to_keywords[parsed_cmd->cmd_keywords_nr - 1] ==
  

•       id_enable)
  

•           enabled = 1;
  

•   else if (parsed_cmd->cmd_to_keywords[parsed_cmd->cmd_keywords_nr - 1] ==
  

•            id_disable)
  

•           enabled = 0;
  

•   else {
  

•           parsed_cmd->err = 1;
  

Remove this.

            return -1;

Please use "return CMD_RET_USAGE;" from include/command.h.

•   }
  
  

•   if (strcmp(argv[1], "port"))
  

•           return -1;
  

•   if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
  

•           vsc9953_port_status_set(parsed_cmd->port, enabled);
  

•   } else {
  

•           for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•                   vsc9953_port_status_set(i, enabled);
  

•   }
  

•   return 0;
  

Please use "return CMD_RET_SUCCESS;"

+}

+static int vsc9953_port_config_key_func(struct command_def *parsed_cmd) +{

•   int                     i;
  

Use a single space.

•   if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
  

•           vsc9953_phy_autoneg(parsed_cmd->port);
  

•           printf("%8s %8s %8s %8s %8s\n",
  

•                  "Port", "Status", "Link", "Speed",
  

•                  "Duplex");
  

•           vsc9953_port_config_show(parsed_cmd->port);
  
  

•   if (!strcmp(argv[3], "show")) {
  

•           if (!strcmp(argv[2], "all")) {
  

•                   vsc9953_phy_all_autoneg();
  

•                   printf("%8s %8s %8s %8s %8s\n",
  

•                          "Port", "Status", "Link", "Speed",
  

•                          "Duplex");
  

•                   vsc9953_port_all_config_show();
  

•                   return 0;
  

•           } else {
  

•                   port = simple_strtoul(argv[2], NULL, 10);
  

•                   if (!VSC9953_PORT_CHECK(port))
  

•                           return -1;
  

•                   vsc9953_phy_autoneg(port);
  

•                   printf("%8s %8s %8s %8s %8s\n",
  

•                          "Port", "Status", "Link", "Speed",
  

•                          "Duplex");
  

•                   vsc9953_port_config_show(port);
  

•                   return 0;
  

•           }
  

•   } else if (!strcmp(argv[3], "enable")) {
  

•           enable = 1;
  

•   } else if (!strcmp(argv[3], "disable")) {
  

•           enable = 0;
    } else {
  

•           return -1;
  

•           for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•                   vsc9953_phy_autoneg(i);
  

•           printf("%8s %8s %8s %8s %8s\n",
  

•                  "Port", "Status", "Link", "Speed", "Duplex");
  

•           for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•                   vsc9953_port_config_show(i);
    }
  
  

•   if (!strcmp(argv[2], "all")) {
  

•           vsc9953_port_all_status_set(enable);
  

•   return 0;
  

Please use "return CMD_RET_SUCCESS;"

+}

+struct keywords_to_function {

•   enum keyword_id cmd_keyword[VSC9953_MAX_CMD_PARAMS];
  

•   int (*keyword_function)(struct command_def *parsed_cmd);
  

+} cmd_def[] = {

•           {
  

•                   .cmd_keyword = {
  

•                                   id_enable,
  

•                                   id_key_end,
  

•                   },
  

•                   .keyword_function = &vsc9953_port_status_key_func,
  

•           }, {
  

•                   .cmd_keyword = {
  

•                                   id_disable,
  

•                                   id_key_end,
  

•                   },
  

•                   .keyword_function = &vsc9953_port_status_key_func,
  

•           }, {
  

•                   .cmd_keyword = {
  

•                                   id_show,
  

•                                   id_key_end,
  

•                   },
  

•                   .keyword_function = &vsc9953_port_config_key_func,
  

•           },
  

+};

+struct keywords_optional {

•   enum keyword_id cmd_keyword[VSC9953_MAX_CMD_PARAMS];
  

+} cmd_opt_def[] = {

•           {
  

•                           .cmd_keyword = {
  

•                                           id_port,
  

•                                           id_port_no,
  

•                                           id_key_end,
  

•                           },
  

•           },
  

+};

+static int keyword_match_gen(enum keyword_id key_id, int argc,

•                        char *const argv[], int *argc_nr,
  

•                        struct command_def *parsed_cmd);
  

+static int keyword_match_port(enum keyword_id key_id, int argc,

•                         char *const argv[], int *argc_nr,
  

•                         struct command_def *parsed_cmd);
  

+/* Define properties for each keyword;

• • keep the order synced with enum keyword_id
• */

+struct keyword_def {

•   const char *keyword_name;
  

•   int (*match)(enum keyword_id key_id, int argc, char *const argv[],
  

•                int *argc_nr, struct command_def *parsed_cmd);
  

+} keyword[] = {

•           {
  

•                           .keyword_name = "help",
  

•                           .match = &keyword_match_gen,
  

•           }, {
  

•                           .keyword_name = "show",
  

•                           .match = &keyword_match_gen,
  

•           }, {
  

•                           .keyword_name = "port",
  

•                           .match = &keyword_match_port
  

•           },  {
  

•                           .keyword_name = "enable",
  

•                           .match = &keyword_match_gen,
  

•           }, {
  

•                           .keyword_name = "disable",
  

•                           .match = &keyword_match_gen,
  

•           },
  

+};

+/* Generic function used to match a keyword only by a string */ +static int keyword_match_gen(enum keyword_id key_id, int argc,

•                        char *const argv[], int *argc_nr,
  

•                        struct command_def *parsed_cmd)
  

+{

•   if (strcmp(argv[*argc_nr], keyword[key_id].keyword_name) == 0) {
  

•           parsed_cmd->cmd_to_keywords[*argc_nr] = key_id;
  

•           return 1;
  

•   }
  

•   return 0;
  

+}

+/* Function used to match the command's port */ +static int keyword_match_port(enum keyword_id key_id, int argc,

•                         char *const argv[], int *argc_nr,
  

•                         struct command_def *parsed_cmd)
  

+{

•   unsigned long                   val;
  

Use a single space.

•   if (!keyword_match_gen(key_id, argc, argv, argc_nr, parsed_cmd))
            return 0;
  

•   } else {
  

•           port = simple_strtoul(argv[2], NULL, 10);
  

•           if (!VSC9953_PORT_CHECK(port))
  

•                   return -1;
  

•           vsc9953_port_status_set(port, enable);
  

•   if (*argc_nr + 1 >= argc)
            return 0;
  

•   if (strict_strtoul(argv[*argc_nr + 1], 10, &val) != -EINVAL) {
  

•           if (!VSC9953_PORT_CHECK(val)) {
  

•                   printf("Invalid port number: %lu\n", val);
  

•                   return 0;
  

•           }
  

•           parsed_cmd->port = val;
  

•           (*argc_nr)++;
  

•           parsed_cmd->cmd_to_keywords[*argc_nr] = id_port_no;
  

•           return 1;
  

•   }
  

•   return 0;
  

+}

+/* match optional keywords */ +static void cmd_keywords_opt_check(struct command_def *parsed_cmd,

•                              int *argc_val)
  

+{

•   int                     i, keyw_opt_id, argc_val_max;
  

Use a single space. Put each var on a separate line.

•   /* remember the best match */
  

•   argc_val_max = *argc_val;
  

•   for (i = 0; i < ARRAY_SIZE(cmd_opt_def); i++) {
  

•           keyw_opt_id = 0;
  

•           while (keyw_opt_id + *argc_val <
  

•                           parsed_cmd->cmd_keywords_nr &&
  

•                  cmd_opt_def[i].cmd_keyword[keyw_opt_id] != id_key_end &&
  

•                  parsed_cmd->cmd_to_keywords[keyw_opt_id + *argc_val] ==
  

•                  cmd_opt_def[i].cmd_keyword[keyw_opt_id])
  

•                   keyw_opt_id++;
  

It might help to break this up a bit and use some intermediate variables to make the code more readable.

•           if (keyw_opt_id && keyw_opt_id + *argc_val <=
  

•                              parsed_cmd->cmd_keywords_nr &&
  

•               cmd_opt_def[i].cmd_keyword[keyw_opt_id] == id_key_end &&
  

•               (*argc_val + keyw_opt_id > argc_val_max))
  

This could benefit from a comment describing what you expect to be verified.

•                           argc_val_max = *argc_val + keyw_opt_id;
    }
  
  

•   return -1;
  

•   *argc_val = argc_val_max;
  

}

-U_BOOT_CMD(ethsw, 5, 0, do_ethsw, +/* get the function to call based on keywords */ +static void cmd_keywords_check(struct command_def *parsed_cmd, int *argc_val) +{

•   int                     i, keyword_id;
  

Use a single space. Put each var on a separate line.

•   for (i = 0; i < ARRAY_SIZE(cmd_def); i++) {
  

•           keyword_id = 0;
  

•           while (keyword_id + *argc_val < parsed_cmd->cmd_keywords_nr &&
  

•                  cmd_def[i].cmd_keyword[keyword_id] != id_key_end &&
  

•                  parsed_cmd->cmd_to_keywords[keyword_id + *argc_val] ==
  

•                                 cmd_def[i].cmd_keyword[keyword_id])
  

•                   keyword_id++;
  

It might help to break this up a bit and use some intermediate variables to make the code more readable.

•           if (keyword_id && keyword_id + *argc_val ==
  

•                             parsed_cmd->cmd_keywords_nr &&
  

•               cmd_def[i].cmd_keyword[keyword_id] == id_key_end) {
  

This could benefit from a comment describing what you expect to be verified.

•                   *argc_val += keyword_id;
  

•                   parsed_cmd->cmd_function = cmd_def[i].keyword_function;
  

•                   return;
  

•           }
  

•   }
  

+}

+/* find all the keywords in the command */ +static void keywords_find(int argc, char * const argv[],

Make this function return an int.

•                     struct command_def *parsed_cmd)
  

+{

•   int                     i, j, argc_val;
  

Use a single space. Put each var on a separate line.

•   for (i = 1; i < argc; i++) {
  

•           for (j = 0; j < id_count; j++) {
  

•                   if (keyword[j].match(j, argc, argv, &i,
  

•                                        parsed_cmd))
  

•                           break;
  

•           }
  

•   }
  

•   for (i = 1; i < argc; i++)
  

•           if (parsed_cmd->cmd_to_keywords[i] == -1)
  

•                   parsed_cmd->err = 1;
  

Instead return CMD_RET_USAGE;

•   parsed_cmd->cmd_keywords_nr = argc;
  

•   argc_val = 1;
  

•   /* get optional parameters first */
  

•   cmd_keywords_opt_check(parsed_cmd, &argc_val);
  

•   cmd_keywords_check(parsed_cmd, &argc_val);
  

•   /* error if not all commands' parameters were matched */
  

•   if (argc_val != parsed_cmd->cmd_keywords_nr ||
  

•       !parsed_cmd->cmd_function)
  

•           parsed_cmd->err = 1;
  

Instead return CMD_RET_USAGE;

+}

+static void command_def_init(struct command_def *parsed_cmd) +{

•   int                     i;
  

•   for (i = 0; i < VSC9953_MAX_CMD_PARAMS; i++)
  

•           parsed_cmd->cmd_to_keywords[i] = -1;
  

•   parsed_cmd->port = VSC9953_CMD_PORT_ALL;
  

•   parsed_cmd->err = 0;
  

Remove this.

•   parsed_cmd->cmd_function = NULL;
  

+}

+static void command_def_cleanup(struct command_def *parsed_cmd) +{

•   /* Nothing to do for now */
  

Then why define it?

+}

+/* function to interpret commands starting with "ethsw " */ +static int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{

•   struct command_def              parsed_cmd;
  

•   int                             rc = 0;
  

Use a single space.

•   if (argc >= VSC9953_MAX_CMD_PARAMS) {
  

•           rc = -1;
  

Use this: + rc = CMD_RET_USAGE;

•           goto __ret;
  

•   }
  

•   command_def_init(&parsed_cmd);
  

•   keywords_find(argc, argv, &parsed_cmd);
  

Use this: + rc = keywords_find(argc, argv, &parsed_cmd); + if (rc) + goto __ret_cmd_cleanup;

•   if (!parsed_cmd.cmd_function) {
  

•           rc = -1;
  

•           goto __ret_cmd_cleanup;
  

•   }
  

I think this whole if statement is unneeded since this same test already exists in keywords_find().

•   rc = parsed_cmd.cmd_function(&parsed_cmd);
  

•   if (parsed_cmd.err) {
  

•           rc = -1;
  

•           goto __ret_cmd_cleanup;
  

•   }
  

Remove this if statement along with parsed_cmd.err.

+__ret_cmd_cleanup:

•   command_def_cleanup(&parsed_cmd);
  

+__ret:

•   return rc;
  

+}

+U_BOOT_CMD(ethsw, VSC9953_MAX_CMD_PARAMS, 0, do_ethsw, "vsc9953 l2 switch commands",

•      "port <port_no> enable|disable\n"
  

•      "    - enable/disable an l2 switch port\n"
  

•      "      port_no=0..9; use \"all\" for all ports\n"
  

•      "ethsw port <port_no> show\n"
  

•      "    - show an l2 switch port's configuration\n"
  

•      "      port_no=0..9; use \"all\" for all ports\n"
  

•      VSC9953_PORT_CONF_HELP"\n"
  

);

#endif /* CONFIG_VSC9953_CMD */

1.9.3

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Hi Codrin,

On Tue, Jun 30, 2015 at 3:57 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:31 AM To: Ciubotariu Codrin Constantin-B43658 Cc: u-boot; Joe Hershberger; Sun York-R58495 Subject: Re: [U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the parser for VSC9953 commands

...

static struct vsc9953_info vsc9953_l2sw = { @@ -575,6 +576,10 @@ void vsc9953_init(bd_t *bis) }

#ifdef CONFIG_VSC9953_CMD

I'd like to see this moved to its own file in common... maybe "common/cmd_ethsw.c". I'd also like to see this #define change to something like "CONFIG_CMD_ETHSW".

These changes don't necessarily need to be part of this series, since it already got in as is, but if you feel motivated, I would recommend you add a patch before this one that moves it.

I could move this parser in common/do_ethsw.c and rename the define. I guess this would imply that upcoming drivers for Ethernet L2 Switches could use the same commands while calling their specific functions.

That's the idea. It would be great to keep it semi-generic.

Thanks, -Joe

Hi Codrin,

On Tue, Jul 7, 2015 at 8:08 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:31 AM To: Ciubotariu Codrin Constantin-B43658 Cc: u-boot; Joe Hershberger; Sun York-R58495 Subject: Re: [U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the parser for VSC9953 commands

Hi Codrin,

On Tue, Jun 30, 2015 at 3:57 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:31 AM To: Ciubotariu Codrin Constantin-B43658 Cc: u-boot; Joe Hershberger; Sun York-R58495 Subject: Re: [U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the parser for VSC9953 commands

...

static struct vsc9953_info vsc9953_l2sw = { @@ -575,6 +576,10 @@ void vsc9953_init(bd_t *bis) }

#ifdef CONFIG_VSC9953_CMD

I'd like to see this moved to its own file in common... maybe "common/cmd_ethsw.c". I'd also like to see this #define change to something like "CONFIG_CMD_ETHSW".

These changes don't necessarily need to be part of this series, since it already got in as is, but if you feel motivated, I would recommend you add a patch before this one that moves it.

I could move this parser in common/do_ethsw.c and rename the define. I guess

this would imply that upcoming drivers for Ethernet L2 Switches could use the same commands while calling their specific functions.

That's the idea. It would be great to keep it semi-generic.

I also need to create a do_ethsw.h that should contain at least struct keywords_to_function. I need this so that any driver could set its implementation in .keyword_function. Where should I put this header?

I think you should add a file simply called "include/ethsw.h"

Cheers, -Joe

Hi Codrin,

On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu codrin.ciubotariu@freescale.com wrote:

The new added command: ethsw [port <port_no>] statistics { [help] | [clear] }

will print counters like the number of Rx/Tx frames, number of Rx/Tx bytes, number of Rx/Tx unicast frames, etc.

Signed-off-by: Codrin Ciubotariu codrin.ciubotariu@freescale.com

Changes for v2: - removed Change-id field;

drivers/net/vsc9953.c | 280 ++++++++++++++++++++++++++++++++++++++++++++++++++ include/vsc9953.h | 116 ++++++++++++++++++++- 2 files changed, 393 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c index 4df751a..62ab0eb 100644 --- a/drivers/net/vsc9953.c +++ b/drivers/net/vsc9953.c @@ -672,6 +672,219 @@ static void vsc9953_port_config_show(int port_no) printf("%8s\n", duplex == DUPLEX_FULL ? "full" : "half"); }

+/* Show VSC9953 ports' statistics */ +static void vsc9953_port_statistics_show(int port_no) +{

•   u32                             rx_val, tx_val;
  

•   struct vsc9953_system_reg       *l2sys_reg;
  

Use a single space. Place each variable on its own line.

•   /* Administrative down */
  

•   if (!vsc9953_l2sw.port[port_no].enabled) {
  

•           printf("Port %d is administrative down\n", port_no);
  

•           return;
  

•   }
  

•   l2sys_reg = (struct vsc9953_system_reg *)(VSC9953_OFFSET +
  

•                   VSC9953_SYS_OFFSET);
  

•   printf("Statistics for L2 Switch port %d:\n", port_no);
  

•   /* Set counter view for our port */
  

•   out_le32(&l2sys_reg->sys.stat_cfg, port_no);
  

+#define VSC9953_STATS_PRINTF "%-15s %10u"

•   /* Get number of Rx and Tx frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_short) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_frag) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_jabber) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_long) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_64) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_65_127) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_128_255) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_256_511) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_512_1023) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_1024_1526) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_jumbo);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_64) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_65_127) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_128_255) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_256_511) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_512_1023) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_1024_1526) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_jumbo);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx frames:", rx_val, "Tx frames:", tx_val);
  

•   /* Get number of Rx and Tx bytes */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_oct);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_oct);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx bytes:", rx_val, "Tx bytes:", tx_val);
  

•   /* Get number of Rx frames received ok and Tx frames sent ok */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_0) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_1) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_2) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_3) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_4) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_5) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_6) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_yellow_prio_7) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_0) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_1) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_2) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_3) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_4) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_5) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_6) +
  

•            in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_green_prio_7);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_64) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_65_127) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_128_255) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_256_511) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_512_1023) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_1024_1526) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_jumbo);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx frames ok:", rx_val, "Tx frames ok:", tx_val);
  

•   /* Get number of Rx and Tx unicast frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_uc);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_uc);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx unicast:", rx_val, "Tx unicast:", tx_val);
  

•   /* Get number of Rx and Tx broadcast frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_bc);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_bc);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx broadcast:", rx_val, "Tx broadcast:", tx_val);
  

•   /* Get number of Rx and Tx frames of 64B */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_64);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_64);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx 64B:", rx_val, "Tx 64B:", tx_val);
  

•   /* Get number of Rx and Tx frames with sizes between 65B and 127B */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_65_127);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_65_127);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx 65B-127B:", rx_val, "Tx 65B-127B:", tx_val);
  

•   /* Get number of Rx and Tx frames with sizes between 128B and 255B */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_128_255);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_128_255);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx 128B-255B:", rx_val, "Tx 128B-255B:", tx_val);
  

•   /* Get number of Rx and Tx frames with sizes between 256B and 511B */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_256_511);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_256_511);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx 256B-511B:", rx_val, "Tx 256B-511B:", tx_val);
  

•   /* Get number of Rx and Tx frames with sizes between 512B and 1023B */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_512_1023);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_512_1023);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx 512B-1023B:", rx_val, "Tx 512B-1023B:", tx_val);
  

•   /* Get number of Rx and Tx frames with sizes between 1024B and 1526B */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_1024_1526);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_1024_1526);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx 1024B-1526B:", rx_val, "Tx 1024B-1526B:", tx_val);
  

•   /* Get number of Rx and Tx jumbo frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_sz_jumbo);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_sz_jumbo);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx jumbo:", rx_val, "Tx jumbo:", tx_val);
  

•   /* Get number of Rx and Tx dropped frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_cat_drop) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_tail) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_0) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_1) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_2) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_3) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_4) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_5) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_6) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_yellow_prio_7) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_0) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_1) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_2) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_3) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_4) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_5) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_6) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_green_prio_7);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_drop) +
  

•            in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_aged);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx drops:", rx_val, "Tx drops:", tx_val);
  

•   /* Get number of Rx frames with CRC or alignment errors
  

•    * and number of detected Tx collisions
  

•    */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_crc);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_col);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx CRC&align:", rx_val, "Tx coll:", tx_val);
  

•   /* Get number of Rx undersized frames and
  

•    * number of Tx aged frames
  

•    */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_short);
  

•   tx_val = in_le32(&l2sys_reg->stat.tx_cntrs.c_tx_aged);
  

•   printf(VSC9953_STATS_PRINTF"\t\t"VSC9953_STATS_PRINTF"\n",
  

•          "Rx undersize:", rx_val, "Tx aged:", tx_val);
  

•   /* Get number of Rx oversized frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_long);
  

•   printf(VSC9953_STATS_PRINTF"\n", "Rx oversized:", rx_val);
  

•   /* Get number of Rx fragmented frames */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_frag);
  

•   printf(VSC9953_STATS_PRINTF"\n", "Rx fragments:", rx_val);
  

•   /* Get number of Rx jabber errors */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_jabber);
  

•   printf(VSC9953_STATS_PRINTF"\n", "Rx jabbers:", rx_val);
  

•   /* Get number of Rx frames filtered due to classification rules or
  

•    * no destination ports
  

•    */
  

•   rx_val = in_le32(&l2sys_reg->stat.rx_cntrs.c_rx_cat_drop) +
  

•            in_le32(&l2sys_reg->stat.drop_cntrs.c_dr_local);
  

•   printf(VSC9953_STATS_PRINTF"\n", "Rx filtered:", rx_val);
  

•   printf("\n");
  

+}

+/* Clear statistics for a VSC9953 port */ +static void vsc9953_port_statistics_clear(int port_no) +{

•   struct vsc9953_system_reg       *l2sys_reg;
  

Use a single space.

•   /* Administrative down */
  

•   if (!vsc9953_l2sw.port[port_no].enabled) {
  

•           printf("Port %d is administrative down\n", port_no);
  

•           return;
  

•   }
  

•   l2sys_reg = (struct vsc9953_system_reg *)(VSC9953_OFFSET +
  

•                           VSC9953_SYS_OFFSET);
  

•   /* Clear all counter groups for our ports */
  

•   out_le32(&l2sys_reg->sys.stat_cfg, port_no |
  

•            CONFIG_VSC9953_STAT_CLEAR_RX | CONFIG_VSC9953_STAT_CLEAR_TX |
  

•            CONFIG_VSC9953_STAT_CLEAR_DR);
  

+}

/* IDs used to track keywords in a command */ enum keyword_id { id_key_end = -1, @@ -680,6 +893,8 @@ enum keyword_id { id_port, id_enable, id_disable,

•   id_statistics,
  

•   id_clear,
    id_count,       /* keep last */
  

};

@@ -749,6 +964,44 @@ static int vsc9953_port_config_key_func(struct command_def *parsed_cmd) return 0; }

+#define VSC9953_PORT_STATS_HELP "ethsw [port <port_no>] statistics " \ +"{ [help] | [clear] } - show an l2 switch port's statistics"

+static int vsc9953_port_stats_help_key_func(struct command_def *parsed_cmd) +{

•   printf(VSC9953_PORT_STATS_HELP"\n");
  

•   return 0;
  

Please use: + return CMD_RET_SUCCESS;

+}

+static int vsc9953_port_stats_key_func(struct command_def *parsed_cmd) +{

•   int i;
  

•   if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
  

•           vsc9953_port_statistics_show(parsed_cmd->port);
  

•   } else {
  

•           for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•                   vsc9953_port_statistics_show(i);
  

•   }
  

•   return 0;
  

Please use: + return CMD_RET_SUCCESS;

+}

+static int vsc9953_port_stats_clear_key_func(struct command_def *parsed_cmd) +{

•   int i;
  

•   if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
  

•           vsc9953_port_statistics_clear(parsed_cmd->port);
  

•   } else {
  

•           for (i = 0; i < VSC9953_MAX_PORTS; i++)
  

•                   vsc9953_port_statistics_clear(i);
  

•   }
  

•   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); @@ -771,6 +1024,26 @@ struct keywords_to_function { id_key_end, }, .keyword_function = &vsc9953_port_config_key_func,

•           }, {
  

•                   .cmd_keyword = {
  

•                                   id_statistics,
  

•                                   id_key_end,
  

•                   },
  

•                   .keyword_function = &vsc9953_port_stats_key_func,
  

•           }, {
  

•                   .cmd_keyword = {
  

•                                   id_statistics,
  

•                                   id_help,
  

•                                   id_key_end,
  

•                   },
  

•                   .keyword_function = &vsc9953_port_stats_help_key_func,
  

If you're going to add this, shouldn't you also add one that prints "VSC9953_PORT_CONF_HELP" individually?

•           }, {
  

•                   .cmd_keyword = {
  

•                                   id_statistics,
  

•                                   id_clear,
  

•                                   id_key_end,
  

•                   },
  

•                   .keyword_function = &vsc9953_port_stats_clear_key_func,
            },
  

};

@@ -816,6 +1089,12 @@ struct keyword_def { }, { .keyword_name = "disable", .match = &keyword_match_gen,

•           }, {
  

•                           .keyword_name = "statistics",
  

•                           .match = &keyword_match_gen,
  

•           }, {
  

•                           .keyword_name = "clear",
  

•                           .match = &keyword_match_gen,
            },
  

};

@@ -993,6 +1272,7 @@ __ret: U_BOOT_CMD(ethsw, VSC9953_MAX_CMD_PARAMS, 0, do_ethsw, "vsc9953 l2 switch commands", VSC9953_PORT_CONF_HELP"\n"

•      VSC9953_PORT_STATS_HELP"\n"
  

);

#endif /* CONFIG_VSC9953_CMD */ diff --git a/include/vsc9953.h b/include/vsc9953.h index bf81623..482acac 100644 --- a/include/vsc9953.h +++ b/include/vsc9953.h @@ -84,6 +84,11 @@ /* Macros for vsc9953_sys_pause_cfgtot_tail_drop_lvl register */ #define CONFIG_VSC9953_TOT_TAIL_DROP_LVL 0x000003ff

+/* Macros for vsc9953_sys_sys.stat_cfg register */ +#define CONFIG_VSC9953_STAT_CLEAR_RX 0x00000400 +#define CONFIG_VSC9953_STAT_CLEAR_TX 0x00000800 +#define CONFIG_VSC9953_STAT_CLEAR_DR 0x00001000

/* Macros for vsc9953_vcap_core_cfg.vcap_mv_cfg register */ #define CONFIG_VSC9953_VCAP_MV_CFG 0x0000ffff #define CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004 @@ -339,10 +344,115 @@ struct vsc9953_qsys_reg {

/* VSC9953 SYS structure for T1040 U-boot*/

+struct vsc9953_rx_cntrs {

•   u32     c_rx_oct;
  

•   u32     c_rx_uc;
  

•   u32     c_rx_mc;
  

•   u32     c_rx_bc;
  

•   u32     c_rx_short;
  

•   u32     c_rx_frag;
  

•   u32     c_rx_jabber;
  

•   u32     c_rx_crc;
  

•   u32     c_rx_symbol_err;
  

•   u32     c_rx_sz_64;
  

•   u32     c_rx_sz_65_127;
  

•   u32     c_rx_sz_128_255;
  

•   u32     c_rx_sz_256_511;
  

•   u32     c_rx_sz_512_1023;
  

•   u32     c_rx_sz_1024_1526;
  

•   u32     c_rx_sz_jumbo;
  

•   u32     c_rx_pause;
  

•   u32     c_rx_control;
  

•   u32     c_rx_long;
  

•   u32     c_rx_cat_drop;
  

•   u32     c_rx_red_prio_0;
  

•   u32     c_rx_red_prio_1;
  

•   u32     c_rx_red_prio_2;
  

•   u32     c_rx_red_prio_3;
  

•   u32     c_rx_red_prio_4;
  

•   u32     c_rx_red_prio_5;
  

•   u32     c_rx_red_prio_6;
  

•   u32     c_rx_red_prio_7;
  

•   u32     c_rx_yellow_prio_0;
  

•   u32     c_rx_yellow_prio_1;
  

•   u32     c_rx_yellow_prio_2;
  

•   u32     c_rx_yellow_prio_3;
  

•   u32     c_rx_yellow_prio_4;
  

•   u32     c_rx_yellow_prio_5;
  

•   u32     c_rx_yellow_prio_6;
  

•   u32     c_rx_yellow_prio_7;
  

•   u32     c_rx_green_prio_0;
  

•   u32     c_rx_green_prio_1;
  

•   u32     c_rx_green_prio_2;
  

•   u32     c_rx_green_prio_3;
  

•   u32     c_rx_green_prio_4;
  

•   u32     c_rx_green_prio_5;
  

•   u32     c_rx_green_prio_6;
  

•   u32     c_rx_green_prio_7;
  

•   u32     reserved[20];
  

+};

+struct vsc9953_tx_cntrs {

•   u32     c_tx_oct;
  

•   u32     c_tx_uc;
  

•   u32     c_tx_mc;
  

•   u32     c_tx_bc;
  

•   u32     c_tx_col;
  

•   u32     c_tx_drop;
  

•   u32     c_tx_pause;
  

•   u32     c_tx_sz_64;
  

•   u32     c_tx_sz_65_127;
  

•   u32     c_tx_sz_128_255;
  

•   u32     c_tx_sz_256_511;
  

•   u32     c_tx_sz_512_1023;
  

•   u32     c_tx_sz_1024_1526;
  

•   u32     c_tx_sz_jumbo;
  

•   u32     c_tx_yellow_prio_0;
  

•   u32     c_tx_yellow_prio_1;
  

•   u32     c_tx_yellow_prio_2;
  

•   u32     c_tx_yellow_prio_3;
  

•   u32     c_tx_yellow_prio_4;
  

•   u32     c_tx_yellow_prio_5;
  

•   u32     c_tx_yellow_prio_6;
  

•   u32     c_tx_yellow_prio_7;
  

•   u32     c_tx_green_prio_0;
  

•   u32     c_tx_green_prio_1;
  

•   u32     c_tx_green_prio_2;
  

•   u32     c_tx_green_prio_3;
  

•   u32     c_tx_green_prio_4;
  

•   u32     c_tx_green_prio_5;
  

•   u32     c_tx_green_prio_6;
  

•   u32     c_tx_green_prio_7;
  

•   u32     c_tx_aged;
  

•   u32     reserved[33];
  

+};

+struct vsc9953_drop_cntrs {

•   u32     c_dr_local;
  

•   u32     c_dr_tail;
  

•   u32     c_dr_yellow_prio_0;
  

•   u32     c_dr_yellow_prio_1;
  

•   u32     c_dr_yellow_prio_2;
  

•   u32     c_dr_yellow_prio_3;
  

•   u32     c_dr_yellow_prio_4;
  

•   u32     c_dr_yellow_prio_5;
  

•   u32     c_dr_yellow_prio_6;
  

•   u32     c_dr_yellow_prio_7;
  

•   u32     c_dr_green_prio_0;
  

•   u32     c_dr_green_prio_1;
  

•   u32     c_dr_green_prio_2;
  

•   u32     c_dr_green_prio_3;
  

•   u32     c_dr_green_prio_4;
  

•   u32     c_dr_green_prio_5;
  

•   u32     c_dr_green_prio_6;
  

•   u32     c_dr_green_prio_7;
  

•   u32     reserved[46];
  

+};

struct vsc9953_sys_stat {

•   u32     rx_cntrs[64];
  

•   u32     tx_cntrs[64];
  

•   u32     drop_cntrs[64];
  

•   struct vsc9953_rx_cntrs         rx_cntrs;
  

•   struct vsc9953_tx_cntrs         tx_cntrs;
  

•   struct vsc9953_drop_cntrs       drop_cntrs;
    u32     reserved1[6];
  

};

-- 1.9.3

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

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));