Re: [PATCH v2 2/2] drivers: net:ethernet: cpsw: add support for VLAN

2013-01-31 Thread Mugunthan V N 

On 1/31/2013 3:32 AM, Francois Romieu wrote:

Mugunthan V N mugunthan...@ti.com :
[...]

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 6ddd028..99696bf 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -24,6 +24,8 @@ Required properties:
  Optional properties:
  - ti,hwmods   : Must be cpgmac0
  - no_bd_ram   : Must be 0 or 1
+- default_vlan : Specifies Default VLAN for non tagged packets
+ ALE processing

Isn't it a device-tree hack for what should belong to a common API ?
Its a hardware feature which stack will not be aware of. It is used in 
the ALE filtering

process with a non-tagged packet arrives.


[...]

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index a40750e..6c66f01 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c

[...]

@@ -607,14 +611,41 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
}
  }
  
+static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)

+{
+   writel(priv-data.default_vlan, priv-host_port_regs-port_vlan);
+   if (priv-version == CPSW_VERSION_1) {
+   slave_write(priv-slaves[0], priv-data.default_vlan,
+   CPSW1_PORT_VLAN);
+   slave_write(priv-slaves[1], priv-data.default_vlan,
+   CPSW1_PORT_VLAN);
+   } else {
+   slave_write(priv-slaves[0], priv-data.default_vlan,
+   CPSW2_PORT_VLAN);
+   slave_write(priv-slaves[1], priv-data.default_vlan,
+   CPSW2_PORT_VLAN);
+   }
+   cpsw_ale_add_vlan(priv-ale, priv-data.default_vlan,
+   ALE_ALL_PORTS  priv-host_port,
+   ALE_ALL_PORTS  priv-host_port,
+   ALE_ALL_PORTS  priv-host_port,
+   (BIT(1) | BIT(2))  priv-host_port);
+}

static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
{
const int vlan = priv-data.default_vlan;
const int port = priv-host_port;
u32 reg;
int i;

reg = (priv-version == CPSW_VERSION_1) ? CPSW1_PORT_VLAN :
  CPSW2_PORT_VLAN;

writel(vlan, priv-host_port_regs-port_vlan);

for (int i = 0; i  2; i++)
slave_write(priv-slaves + i, vlan, reg);

cpsw_ale_add_vlan(priv-ale, vlan, ALE_ALL_PORTS  port,
  ALE_ALL_PORTS  port, ALE_ALL_PORTS  port,
  (BIT(1) | BIT(2))  port);
}

... or somewhere between both. Your call.

Will modify the code as this looks simpler


[...]

@@ -933,6 +967,55 @@ static void cpsw_ndo_poll_controller(struct net_device 
*ndev)
  }
  #endif
  
+static inline void cpsw_add_vlan_ale_entry(struct cpsw_priv *priv,

+   unsigned short vid)
+{
+   cpsw_ale_add_vlan(priv-ale, vid, ALE_ALL_PORTS  priv-host_port,
+   0, ALE_ALL_PORTS  priv-host_port,
+   (BIT(1) | BIT(2))  priv-host_port);

(BIT(1) | BIT(2)) is repeated a couple of times.

Will replace with port number defines.


[...]

+static int cpsw_ndo_vlan_rx_add_vid(struct net_device *ndev,
+   unsigned short vid)
+{
+   struct cpsw_priv *priv = netdev_priv(ndev);
+
+   if (vid == priv-data.default_vlan)
+   return 0;
+
+   spin_lock(priv-lock);
+
+   dev_info(priv-dev, Adding vlanid %d to vlan filter\n, vid);
+   cpsw_add_vlan_ale_entry(priv, vid);
+
+   spin_unlock(priv-lock);
+   return 0;
+}
+
+static int cpsw_ndo_vlan_rx_kill_vid(struct net_device *ndev,
+   unsigned short vid)
+{
+   struct cpsw_priv *priv = netdev_priv(ndev);
+
+   if (vid == priv-data.default_vlan)
+   return 0;
+
+   spin_lock(priv-lock);
+
+   dev_info(priv-dev, removing vlanid %d from vlan filter\n, vid);
+   cpsw_ale_del_vlan(priv-ale, vid, 0);
+   cpsw_ale_del_ucast(priv-ale, priv-mac_addr,
+  priv-host_port, ALE_VLAN, vid);
+   cpsw_ale_del_mcast(priv-ale, priv-ndev-broadcast, 0, ALE_VLAN, vid);
+
+   spin_unlock(priv-lock);

What are you trying to achieve with the lock ?

It is not used anywhere else and both cpsw_ndo_vlan_rx_{add, kill}_vid are
called under RTNL.

Will remove the lock from both apis

Regards
Mugunthan V N
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] drivers: net:ethernet: cpsw: add support for VLAN

2013-01-30 Thread Francois Romieu 
Mugunthan V N mugunthan...@ti.com :
[...]
 diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
 b/Documentation/devicetree/bindings/net/cpsw.txt
 index 6ddd028..99696bf 100644
 --- a/Documentation/devicetree/bindings/net/cpsw.txt
 +++ b/Documentation/devicetree/bindings/net/cpsw.txt
 @@ -24,6 +24,8 @@ Required properties:
  Optional properties:
  - ti,hwmods  : Must be cpgmac0
  - no_bd_ram  : Must be 0 or 1
 +- default_vlan   : Specifies Default VLAN for non tagged packets
 +   ALE processing

Isn't it a device-tree hack for what should belong to a common API ?

[...]
 diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
 index a40750e..6c66f01 100644
 --- a/drivers/net/ethernet/ti/cpsw.c
 +++ b/drivers/net/ethernet/ti/cpsw.c
[...]
 @@ -607,14 +611,41 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
 struct cpsw_priv *priv)
   }
  }
  
 +static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
 +{
 + writel(priv-data.default_vlan, priv-host_port_regs-port_vlan);
 + if (priv-version == CPSW_VERSION_1) {
 + slave_write(priv-slaves[0], priv-data.default_vlan,
 + CPSW1_PORT_VLAN);
 + slave_write(priv-slaves[1], priv-data.default_vlan,
 + CPSW1_PORT_VLAN);
 + } else {
 + slave_write(priv-slaves[0], priv-data.default_vlan,
 + CPSW2_PORT_VLAN);
 + slave_write(priv-slaves[1], priv-data.default_vlan,
 + CPSW2_PORT_VLAN);
 + }
 + cpsw_ale_add_vlan(priv-ale, priv-data.default_vlan,
 + ALE_ALL_PORTS  priv-host_port,
 + ALE_ALL_PORTS  priv-host_port,
 + ALE_ALL_PORTS  priv-host_port,
 + (BIT(1) | BIT(2))  priv-host_port);
 +}

static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
{
const int vlan = priv-data.default_vlan;
const int port = priv-host_port;
u32 reg;
int i;

reg = (priv-version == CPSW_VERSION_1) ? CPSW1_PORT_VLAN :
  CPSW2_PORT_VLAN;

writel(vlan, priv-host_port_regs-port_vlan);

for (int i = 0; i  2; i++)
slave_write(priv-slaves + i, vlan, reg);

cpsw_ale_add_vlan(priv-ale, vlan, ALE_ALL_PORTS  port,
  ALE_ALL_PORTS  port, ALE_ALL_PORTS  port,
  (BIT(1) | BIT(2))  port);
}

... or somewhere between both. Your call.

[...]
 @@ -933,6 +967,55 @@ static void cpsw_ndo_poll_controller(struct net_device 
 *ndev)
  }
  #endif
  
 +static inline void cpsw_add_vlan_ale_entry(struct cpsw_priv *priv,
 + unsigned short vid)
 +{
 + cpsw_ale_add_vlan(priv-ale, vid, ALE_ALL_PORTS  priv-host_port,
 + 0, ALE_ALL_PORTS  priv-host_port,
 + (BIT(1) | BIT(2))  priv-host_port);

(BIT(1) | BIT(2)) is repeated a couple of times.

[...]
 +static int cpsw_ndo_vlan_rx_add_vid(struct net_device *ndev,
 + unsigned short vid)
 +{
 + struct cpsw_priv *priv = netdev_priv(ndev);
 +
 + if (vid == priv-data.default_vlan)
 + return 0;
 +
 + spin_lock(priv-lock);
 +
 + dev_info(priv-dev, Adding vlanid %d to vlan filter\n, vid);
 + cpsw_add_vlan_ale_entry(priv, vid);
 +
 + spin_unlock(priv-lock);
 + return 0;
 +}
 +
 +static int cpsw_ndo_vlan_rx_kill_vid(struct net_device *ndev,
 + unsigned short vid)
 +{
 + struct cpsw_priv *priv = netdev_priv(ndev);
 +
 + if (vid == priv-data.default_vlan)
 + return 0;
 +
 + spin_lock(priv-lock);
 +
 + dev_info(priv-dev, removing vlanid %d from vlan filter\n, vid);
 + cpsw_ale_del_vlan(priv-ale, vid, 0);
 + cpsw_ale_del_ucast(priv-ale, priv-mac_addr,
 +priv-host_port, ALE_VLAN, vid);
 + cpsw_ale_del_mcast(priv-ale, priv-ndev-broadcast, 0, ALE_VLAN, vid);
 +
 + spin_unlock(priv-lock);

What are you trying to achieve with the lock ?

It is not used anywhere else and both cpsw_ndo_vlan_rx_{add, kill}_vid are
called under RTNL.

-- 
Ueimor
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html