[U-Boot,03/11,v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch

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

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

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>

Thanks,
-Joe

Hi Joe,

> > +       return !!timeout;
> 
> Maybe this:
> 
> +       return timeout ? 0 : -EBUSY;

Ok.

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

Ok, I think I will go with the first choice. 

> > +       if (!vsc9953_vlan_table_poll_idle()) {
> 
> Here too.

Ok.

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

Ok.

> > +       /* Administrative down */
> > +       if ((!vsc9953_l2sw.port[port_no].enabled)) {
> 
> Why do you have double "((" and "))"?

By mistake. I will remove the extra pair.

> > +       int             i;
> 
> Use a single space.

Ok, I will make sure there are no tabs after a variable's type, in all these patches.

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


> > +#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 could also use those from bitfield.h, but then I
should create new macros or use some sort of magic numbers for the
"shift" parameter. Changing the already defined bitfiled_*() functions
doesn't look like an option since it would require changes all over
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.

Yes, I will remove the "port_". 

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

Thanks and best regards,
Codrin

Hi Codrin,

On Mon, Jun 29, 2015 at 11:06 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> Hi Joe,
>
>> > +       return !!timeout;
>>
>> Maybe this:
>>
>> +       return timeout ? 0 : -EBUSY;
>
> Ok.
>
>> > +
>> > +       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) {
>>
>
> Ok, I think I will go with the first choice.
>
>> > +       if (!vsc9953_vlan_table_poll_idle()) {
>>
>> Here too.
>
> Ok.
>
>> > +       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".
>
> Ok.
>
>> > +       /* Administrative down */
>> > +       if ((!vsc9953_l2sw.port[port_no].enabled)) {
>>
>> Why do you have double "((" and "))"?
>
> By mistake. I will remove the extra pair.
>
>> > +       int             i;
>>
>> Use a single space.
>
> Ok, I will make sure there are no tabs after a variable's type, in all these patches.
>
>> > +       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.

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

> I could also use those from bitfield.h, but then I
> should create new macros or use some sort of magic numbers for the
> "shift" parameter.

That could be done, but if you do create macros for the shift
parameter, they should go in bitfield.h.

Please do not add magic numbers.

> Changing the already defined bitfiled_*() functions
> doesn't look like an option since it would require changes all over
> u-boot.

Yes, certainly do not do this. Use them as is or add new versions. Do
no change the existing functions.

>> > +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.
>
> Yes, I will remove the "port_".
>
>> > +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.

Thanks,
-Joe

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?

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

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

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