Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Sean Wang]

On Tue, 2017-12-12 at 09:24 +0100, Felix Fietkau wrote:
> On 2017-12-07 07:06, sean.w...@mediatek.com wrote:
> > From: Sean Wang 
> > 
> > MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
> Shouldn't that be VLAN-unaware/VLAN-aware (in the code as well)?
> 
> - Felix
> 

Hi, Felix

Thank you. It is my misspelling problem. I will fix them all with this
including in the code.

Sean

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Sean Wang]

On Tue, 2017-12-12 at 09:24 +0100, Felix Fietkau wrote:
> On 2017-12-07 07:06, sean.w...@mediatek.com wrote:
> > From: Sean Wang 
> > 
> > MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
> Shouldn't that be VLAN-unaware/VLAN-aware (in the code as well)?
> 
> - Felix
> 

Hi, Felix

Thank you. It is my misspelling problem. I will fix them all with this
including in the code.

Sean

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Felix Fietkau]

On 2017-12-07 07:06, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
Shouldn't that be VLAN-unaware/VLAN-aware (in the code as well)?

- Felix

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Felix Fietkau]

On 2017-12-07 07:06, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
Shouldn't that be VLAN-unaware/VLAN-aware (in the code as well)?

- Felix

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Sean Wang]

Hi, Andrew

All sounds reasonable. All will be fixed in the next version.

Sean

On Thu, 2017-12-07 at 16:24 +0100, Andrew Lunn wrote:
> >  static void
> > +mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
> > +{
> > +   struct mt7530_priv *priv = ds->priv;
> > +   int i;
> > +   bool all_user_ports_removed = true;
> 
> Hi Sean
> 
> Reverse Christmas tree please.
> 

will be fixed 

> > +static int
> > +mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 
> > vid)
> > +{
> > +   u32 val;
> > +   int ret;
> > +   struct mt7530_dummy_poll p;
> 
> Here too.
> 

will be fixed

> > +static int
> > +mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
> > +const struct switchdev_obj_port_vlan *vlan,
> > +struct switchdev_trans *trans)
> > +{
> > +   struct mt7530_priv *priv = ds->priv;
> > +
> > +   /* The port is being kept as VLAN-unware port when bridge is set up
> > +* with vlan_filtering not being set, Otherwise, the port and the
> > +* corresponding CPU port is required the setup for becoming a
> > +* VLAN-ware port.
> > +*/
> > +   if (!priv->ports[port].vlan_filtering)
> > +   return 0;
> > +
> > +   mt7530_port_set_vlan_ware(ds, port);
> > +   mt7530_port_set_vlan_ware(ds, MT7530_CPU_PORT);
> 
> A prepare function should just validate that it is possible to carry
> out the operation. It should not change any state. These two last
> lines probably don't belong here.
> 

okay, it will be moved into the proper place such as
mt7530_port_vlan_filtering

> > +
> > +   return 0;
> > +}
> > +
> > +static void
> > +mt7530_hw_vlan_add(struct mt7530_priv *priv,
> > +  struct mt7530_hw_vlan_entry *entry)
> > +{
> > +   u32 val;
> > +   u8 new_members;
> 
> Reverse Christmas tree. Please check the whole patch.
> 
 will be fixed

> > +static inline void INIT_MT7530_HW_ENTRY(struct mt7530_hw_vlan_entry *e,
> > +   int port, bool untagged)
> > +{
> > +   e->port = port;
> > +   e->untagged = untagged;
> > +}
> 
> All CAPITAL letters is for #defines. This is just a normal
> function. Please use lower case.
> 

will be fixed
> Andrew
> 

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Sean Wang]

Hi, Andrew

All sounds reasonable. All will be fixed in the next version.

Sean

On Thu, 2017-12-07 at 16:24 +0100, Andrew Lunn wrote:
> >  static void
> > +mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
> > +{
> > +   struct mt7530_priv *priv = ds->priv;
> > +   int i;
> > +   bool all_user_ports_removed = true;
> 
> Hi Sean
> 
> Reverse Christmas tree please.
> 

will be fixed 

> > +static int
> > +mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 
> > vid)
> > +{
> > +   u32 val;
> > +   int ret;
> > +   struct mt7530_dummy_poll p;
> 
> Here too.
> 

will be fixed

> > +static int
> > +mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
> > +const struct switchdev_obj_port_vlan *vlan,
> > +struct switchdev_trans *trans)
> > +{
> > +   struct mt7530_priv *priv = ds->priv;
> > +
> > +   /* The port is being kept as VLAN-unware port when bridge is set up
> > +* with vlan_filtering not being set, Otherwise, the port and the
> > +* corresponding CPU port is required the setup for becoming a
> > +* VLAN-ware port.
> > +*/
> > +   if (!priv->ports[port].vlan_filtering)
> > +   return 0;
> > +
> > +   mt7530_port_set_vlan_ware(ds, port);
> > +   mt7530_port_set_vlan_ware(ds, MT7530_CPU_PORT);
> 
> A prepare function should just validate that it is possible to carry
> out the operation. It should not change any state. These two last
> lines probably don't belong here.
> 

okay, it will be moved into the proper place such as
mt7530_port_vlan_filtering

> > +
> > +   return 0;
> > +}
> > +
> > +static void
> > +mt7530_hw_vlan_add(struct mt7530_priv *priv,
> > +  struct mt7530_hw_vlan_entry *entry)
> > +{
> > +   u32 val;
> > +   u8 new_members;
> 
> Reverse Christmas tree. Please check the whole patch.
> 
 will be fixed

> > +static inline void INIT_MT7530_HW_ENTRY(struct mt7530_hw_vlan_entry *e,
> > +   int port, bool untagged)
> > +{
> > +   e->port = port;
> > +   e->untagged = untagged;
> > +}
> 
> All CAPITAL letters is for #defines. This is just a normal
> function. Please use lower case.
> 

will be fixed
> Andrew
> 

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Andrew Lunn]

>  static void
> +mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + int i;
> + bool all_user_ports_removed = true;

Hi Sean

Reverse Christmas tree please.

> +static int
> +mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 vid)
> +{
> + u32 val;
> + int ret;
> + struct mt7530_dummy_poll p;

Here too.

> +static int
> +mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
> +  const struct switchdev_obj_port_vlan *vlan,
> +  struct switchdev_trans *trans)
> +{
> + struct mt7530_priv *priv = ds->priv;
> +
> + /* The port is being kept as VLAN-unware port when bridge is set up
> +  * with vlan_filtering not being set, Otherwise, the port and the
> +  * corresponding CPU port is required the setup for becoming a
> +  * VLAN-ware port.
> +  */
> + if (!priv->ports[port].vlan_filtering)
> + return 0;
> +
> + mt7530_port_set_vlan_ware(ds, port);
> + mt7530_port_set_vlan_ware(ds, MT7530_CPU_PORT);

A prepare function should just validate that it is possible to carry
out the operation. It should not change any state. These two last
lines probably don't belong here.

> +
> + return 0;
> +}
> +
> +static void
> +mt7530_hw_vlan_add(struct mt7530_priv *priv,
> +struct mt7530_hw_vlan_entry *entry)
> +{
> + u32 val;
> + u8 new_members;

Reverse Christmas tree. Please check the whole patch.

> +static inline void INIT_MT7530_HW_ENTRY(struct mt7530_hw_vlan_entry *e,
> + int port, bool untagged)
> +{
> + e->port = port;
> + e->untagged = untagged;
> +}

All CAPITAL letters is for #defines. This is just a normal
function. Please use lower case.

  Andrew

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Andrew Lunn]

>  static void
> +mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + int i;
> + bool all_user_ports_removed = true;

Hi Sean

Reverse Christmas tree please.

> +static int
> +mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 vid)
> +{
> + u32 val;
> + int ret;
> + struct mt7530_dummy_poll p;

Here too.

> +static int
> +mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
> +  const struct switchdev_obj_port_vlan *vlan,
> +  struct switchdev_trans *trans)
> +{
> + struct mt7530_priv *priv = ds->priv;
> +
> + /* The port is being kept as VLAN-unware port when bridge is set up
> +  * with vlan_filtering not being set, Otherwise, the port and the
> +  * corresponding CPU port is required the setup for becoming a
> +  * VLAN-ware port.
> +  */
> + if (!priv->ports[port].vlan_filtering)
> + return 0;
> +
> + mt7530_port_set_vlan_ware(ds, port);
> + mt7530_port_set_vlan_ware(ds, MT7530_CPU_PORT);

A prepare function should just validate that it is possible to carry
out the operation. It should not change any state. These two last
lines probably don't belong here.

> +
> + return 0;
> +}
> +
> +static void
> +mt7530_hw_vlan_add(struct mt7530_priv *priv,
> +struct mt7530_hw_vlan_entry *entry)
> +{
> + u32 val;
> + u8 new_members;

Reverse Christmas tree. Please check the whole patch.

> +static inline void INIT_MT7530_HW_ENTRY(struct mt7530_hw_vlan_entry *e,
> + int port, bool untagged)
> +{
> + e->port = port;
> + e->untagged = untagged;
> +}

All CAPITAL letters is for #defines. This is just a normal
function. Please use lower case.

  Andrew

[PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[sean.wang]

From: Sean Wang 

MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
through the implementation of port matrix mode or port security mode on
the ingress port, respectively. On one hand, Each port has been acting as
the VLAN-unware one whenever the device is created in the initial or
certain port joins or leaves into/from the bridge at the runtime. On the
other hand, the patch just filling the required callbacks for VLAN
operations is achieved via extending the port to be into port security
mode when the port is configured as VLAN-ware port. Which mode can make
the port be able to recognize VID from incoming packets and look up VLAN
table to validate and judge which port it should be going to. And the
range for VID from 1 to 4094 is valid for the hardware.

Signed-off-by: Sean Wang 
---
 drivers/net/dsa/mt7530.c | 292 ++-
 drivers/net/dsa/mt7530.h |  83 +-
 2 files changed, 368 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2820d69..a7c5370 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -805,6 +805,69 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 }
 
 static void
+mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
+{
+   struct mt7530_priv *priv = ds->priv;
+   int i;
+   bool all_user_ports_removed = true;
+
+   /* When a port is removed from the bridge, the port would be set up
+* back to the default as is at initial boot which is a VLAN-unware
+* port.
+*/
+   mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
+  MT7530_PORT_MATRIX_MODE);
+   mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+  VLAN_ATTR(MT7530_VLAN_TRANSPARENT));
+
+   priv->ports[port].vlan_filtering = false;
+
+   for (i = 0; i < MT7530_NUM_PORTS; i++) {
+   if (dsa_is_user_port(ds, i) &&
+   priv->ports[i].vlan_filtering) {
+   all_user_ports_removed = false;
+   break;
+   }
+   }
+
+   /* CPU port also does the same thing until all user ports belonging to
+* the CPU port get out of VLAN filtering mode.
+*/
+   if (all_user_ports_removed) {
+   mt7530_write(priv, MT7530_PCR_P(MT7530_CPU_PORT),
+PCR_MATRIX(dsa_user_ports(priv->ds)));
+   mt7530_write(priv, MT7530_PVC_P(MT7530_CPU_PORT),
+PORT_SPEC_TAG);
+   }
+}
+
+static void
+mt7530_port_set_vlan_ware(struct dsa_switch *ds, int port)
+{
+   struct mt7530_priv *priv = ds->priv;
+
+   /* The real fabric path would be decided on the membership in the
+* entry of VLAN table. PCR_MATRIX set up here with ALL_MEMBERS
+* means potential VLAN can be consisting of certain subset of all
+* ports.
+*/
+   mt7530_rmw(priv, MT7530_PCR_P(port),
+  PCR_MATRIX_MASK, PCR_MATRIX(MT7530_ALL_MEMBERS));
+
+   /* Trapped into security mode allows packet forwarding through VLAN
+* table lookup.
+*/
+   mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
+  MT7530_PORT_SECURITY_MODE);
+
+   /* Set the port as a user port which is to be able to recognize VID
+* from incoming packets before fetching entry within the VLAN table.
+*/
+   mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+  VLAN_ATTR(MT7530_VLAN_USER));
+}
+
+static void
 mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 struct net_device *bridge)
 {
@@ -817,8 +880,11 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
/* Remove this port from the port matrix of the other ports
 * in the same bridge. If the port is disabled, port matrix
 * is kept and not being setup until the port becomes enabled.
+* And the other port's port matrix cannot be broken when the
+* other port is still a VLAN-ware port.
 */
-   if (dsa_is_user_port(ds, i) && i != port) {
+   if (!priv->ports[i].vlan_filtering &&
+   dsa_is_user_port(ds, i) && i != port) {
if (dsa_to_port(ds, i)->bridge_dev != bridge)
continue;
if (priv->ports[i].enable)
@@ -836,6 +902,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
 
+   mt7530_port_set_vlan_unware(ds, port);
+
mutex_unlock(>reg_mutex);
 }
 
@@ -906,6 +974,224 @@ mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
return 0;
 }
 
+static int

[PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[sean.wang]

From: Sean Wang 

MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
through the implementation of port matrix mode or port security mode on
the ingress port, respectively. On one hand, Each port has been acting as
the VLAN-unware one whenever the device is created in the initial or
certain port joins or leaves into/from the bridge at the runtime. On the
other hand, the patch just filling the required callbacks for VLAN
operations is achieved via extending the port to be into port security
mode when the port is configured as VLAN-ware port. Which mode can make
the port be able to recognize VID from incoming packets and look up VLAN
table to validate and judge which port it should be going to. And the
range for VID from 1 to 4094 is valid for the hardware.

Signed-off-by: Sean Wang 
---
 drivers/net/dsa/mt7530.c | 292 ++-
 drivers/net/dsa/mt7530.h |  83 +-
 2 files changed, 368 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2820d69..a7c5370 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -805,6 +805,69 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 }
 
 static void
+mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
+{
+   struct mt7530_priv *priv = ds->priv;
+   int i;
+   bool all_user_ports_removed = true;
+
+   /* When a port is removed from the bridge, the port would be set up
+* back to the default as is at initial boot which is a VLAN-unware
+* port.
+*/
+   mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
+  MT7530_PORT_MATRIX_MODE);
+   mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+  VLAN_ATTR(MT7530_VLAN_TRANSPARENT));
+
+   priv->ports[port].vlan_filtering = false;
+
+   for (i = 0; i < MT7530_NUM_PORTS; i++) {
+   if (dsa_is_user_port(ds, i) &&
+   priv->ports[i].vlan_filtering) {
+   all_user_ports_removed = false;
+   break;
+   }
+   }
+
+   /* CPU port also does the same thing until all user ports belonging to
+* the CPU port get out of VLAN filtering mode.
+*/
+   if (all_user_ports_removed) {
+   mt7530_write(priv, MT7530_PCR_P(MT7530_CPU_PORT),
+PCR_MATRIX(dsa_user_ports(priv->ds)));
+   mt7530_write(priv, MT7530_PVC_P(MT7530_CPU_PORT),
+PORT_SPEC_TAG);
+   }
+}
+
+static void
+mt7530_port_set_vlan_ware(struct dsa_switch *ds, int port)
+{
+   struct mt7530_priv *priv = ds->priv;
+
+   /* The real fabric path would be decided on the membership in the
+* entry of VLAN table. PCR_MATRIX set up here with ALL_MEMBERS
+* means potential VLAN can be consisting of certain subset of all
+* ports.
+*/
+   mt7530_rmw(priv, MT7530_PCR_P(port),
+  PCR_MATRIX_MASK, PCR_MATRIX(MT7530_ALL_MEMBERS));
+
+   /* Trapped into security mode allows packet forwarding through VLAN
+* table lookup.
+*/
+   mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
+  MT7530_PORT_SECURITY_MODE);
+
+   /* Set the port as a user port which is to be able to recognize VID
+* from incoming packets before fetching entry within the VLAN table.
+*/
+   mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+  VLAN_ATTR(MT7530_VLAN_USER));
+}
+
+static void
 mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 struct net_device *bridge)
 {
@@ -817,8 +880,11 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
/* Remove this port from the port matrix of the other ports
 * in the same bridge. If the port is disabled, port matrix
 * is kept and not being setup until the port becomes enabled.
+* And the other port's port matrix cannot be broken when the
+* other port is still a VLAN-ware port.
 */
-   if (dsa_is_user_port(ds, i) && i != port) {
+   if (!priv->ports[i].vlan_filtering &&
+   dsa_is_user_port(ds, i) && i != port) {
if (dsa_to_port(ds, i)->bridge_dev != bridge)
continue;
if (priv->ports[i].enable)
@@ -836,6 +902,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
 
+   mt7530_port_set_vlan_unware(ds, port);
+
mutex_unlock(>reg_mutex);
 }
 
@@ -906,6 +974,224 @@ mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
return 0;
 }
 
+static int
+mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd,