This patchset removes the unnecessary PHY device argument in port
enable/disable switch operations, makes slave open and close symmetrical
and finally provides helpers for enabling or disabling a DSA port.
Vivien Didelot (4):
net: dsa: move up phy enabling in core
net: dsa: remove phy arg from port enable/disable
net: dsa: make slave close symmetrical to open
net: dsa: add port enable and disable helpers
drivers/net/dsa/b53/b53_common.c | 6 +++---
drivers/net/dsa/b53/b53_priv.h | 4 ++--
drivers/net/dsa/bcm_sf2.c | 32 ++++++++----------------------
drivers/net/dsa/lan9303-core.c | 6 ++----
drivers/net/dsa/microchip/ksz_common.c | 6 ++----
drivers/net/dsa/mt7530.c | 8 +++-----
drivers/net/dsa/mv88e6xxx/chip.c | 6 ++----
drivers/net/dsa/qca8k.c | 6 ++----
include/net/dsa.h | 6 ++----
net/dsa/dsa_priv.h | 3 ++-
net/dsa/port.c | 31 ++++++++++++++++++++++++++++-
net/dsa/slave.c | 36 ++++++++++++++++++----------------
12 files changed, 77 insertions(+), 73 deletions(-)
--
2.14.1
Provide dsa_port_enable and dsa_port_disable helpers to respectively
enable and disable a switch port. This makes the dsa_port_set_state_now
helper static.
Signed-off-by: Vivien Didelot <[email protected]>
---
net/dsa/dsa_priv.h | 3 ++-
net/dsa/port.c | 31 ++++++++++++++++++++++++++++++-
net/dsa/slave.c | 19 +++++--------------
3 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 9803952a5b40..6bfff19d1615 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -117,7 +117,8 @@ void dsa_master_ethtool_restore(struct net_device *dev);
/* port.c */
int dsa_port_set_state(struct dsa_port *dp, u8 state,
struct switchdev_trans *trans);
-void dsa_port_set_state_now(struct dsa_port *dp, u8 state);
+int dsa_port_enable(struct dsa_port *dp);
+void dsa_port_disable(struct dsa_port *dp);
int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br);
void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br);
int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 76d43a82d397..50749339e252 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -56,7 +56,7 @@ int dsa_port_set_state(struct dsa_port *dp, u8 state,
return 0;
}
-void dsa_port_set_state_now(struct dsa_port *dp, u8 state)
+static void dsa_port_set_state_now(struct dsa_port *dp, u8 state)
{
int err;
@@ -65,6 +65,35 @@ void dsa_port_set_state_now(struct dsa_port *dp, u8 state)
pr_err("DSA: failed to set STP state %u (%d)\n", state, err);
}
+int dsa_port_enable(struct dsa_port *dp)
+{
+ u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
+ struct dsa_switch *ds = dp->ds;
+ int port = dp->index;
+ int err;
+
+ if (ds->ops->port_enable) {
+ err = ds->ops->port_enable(ds, port);
+ if (err)
+ return err;
+ }
+
+ dsa_port_set_state_now(dp, stp_state);
+
+ return 0;
+}
+
+void dsa_port_disable(struct dsa_port *dp)
+{
+ struct dsa_switch *ds = dp->ds;
+ int port = dp->index;
+
+ dsa_port_set_state_now(dp, BR_STATE_DISABLED);
+
+ if (ds->ops->port_disable)
+ ds->ops->port_disable(ds, port);
+}
+
int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
{
struct dsa_notifier_bridge_info info = {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 235a5c95dfcc..e40623939323 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -74,9 +74,7 @@ static int dsa_slave_open(struct net_device *dev)
struct dsa_slave_priv *p = netdev_priv(dev);
struct phy_device *phy = p->phy;
struct dsa_port *dp = p->dp;
- struct dsa_switch *ds = dp->ds;
struct net_device *master = dsa_master_netdev(p);
- u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
int err;
if (!(master->flags & IFF_UP))
@@ -99,13 +97,9 @@ static int dsa_slave_open(struct net_device *dev)
goto clear_allmulti;
}
- if (ds->ops->port_enable) {
- err = ds->ops->port_enable(ds, p->dp->index);
- if (err)
- goto clear_promisc;
- }
-
- dsa_port_set_state_now(p->dp, stp_state);
+ err = dsa_port_enable(dp);
+ if (err)
+ goto clear_promisc;
if (phy) {
/* If phy_stop() has been called before, phy will be in
@@ -139,15 +133,12 @@ static int dsa_slave_close(struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
struct net_device *master = dsa_master_netdev(p);
- struct dsa_switch *ds = p->dp->ds;
+ struct dsa_port *dp = p->dp;
if (p->phy)
phy_stop(p->phy);
- dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
-
- if (ds->ops->port_disable)
- ds->ops->port_disable(ds, p->dp->index);
+ dsa_port_disable(dp);
dev_mc_unsync(master, dev);
dev_uc_unsync(master, dev);
--
2.14.1
The DSA slave open function configures the unicast MAC addresses on the
master device, enable the switch port, change its STP state, then start
the PHY device.
Make the close function symmetric, by first stopping the PHY device,
then changing the STP state, disabling the switch port and restore the
master device.
Signed-off-by: Vivien Didelot <[email protected]>
---
net/dsa/slave.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6290741e496a..235a5c95dfcc 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -144,6 +144,11 @@ static int dsa_slave_close(struct net_device *dev)
if (p->phy)
phy_stop(p->phy);
+ dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
+
+ if (ds->ops->port_disable)
+ ds->ops->port_disable(ds, p->dp->index);
+
dev_mc_unsync(master, dev);
dev_uc_unsync(master, dev);
if (dev->flags & IFF_ALLMULTI)
@@ -154,11 +159,6 @@ static int dsa_slave_close(struct net_device *dev)
if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
dev_uc_del(master, dev->dev_addr);
- if (ds->ops->port_disable)
- ds->ops->port_disable(ds, p->dp->index);
-
- dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
-
return 0;
}
--
2.14.1
The .port_enable and .port_disable functions are meant to deal with the
switch ports only, and no driver is using the phy argument anyway.
Remove it.
Signed-off-by: Vivien Didelot <[email protected]>
---
drivers/net/dsa/b53/b53_common.c | 6 +++---
drivers/net/dsa/b53/b53_priv.h | 4 ++--
drivers/net/dsa/bcm_sf2.c | 16 +++++++---------
drivers/net/dsa/lan9303-core.c | 6 ++----
drivers/net/dsa/microchip/ksz_common.c | 6 ++----
drivers/net/dsa/mt7530.c | 8 +++-----
drivers/net/dsa/mv88e6xxx/chip.c | 6 ++----
drivers/net/dsa/qca8k.c | 6 ++----
include/net/dsa.h | 6 ++----
net/dsa/slave.c | 4 ++--
10 files changed, 27 insertions(+), 41 deletions(-)
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index d4ce092def83..e46eb29d29f0 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
}
EXPORT_SYMBOL(b53_imp_vlan_setup);
-int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
+int b53_enable_port(struct dsa_switch *ds, int port)
{
struct b53_device *dev = ds->priv;
unsigned int cpu_port = dev->cpu_port;
@@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
}
EXPORT_SYMBOL(b53_enable_port);
-void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
+void b53_disable_port(struct dsa_switch *ds, int port)
{
struct b53_device *dev = ds->priv;
u8 reg;
@@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds)
if (dsa_is_cpu_port(ds, port))
b53_enable_cpu_port(dev, port);
else if (!(BIT(port) & ds->enabled_port_mask))
- b53_disable_port(ds, port, NULL);
+ b53_disable_port(ds, port);
}
return ret;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 603c66d240d8..688d02ee6155 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
void b53_mirror_del(struct dsa_switch *ds, int port,
struct dsa_mall_mirror_tc_entry *mirror);
-int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
-void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
+int b53_enable_port(struct dsa_switch *ds, int port);
+void b53_disable_port(struct dsa_switch *ds, int port);
void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ad96b9725a2c..77e0c43f973b 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct bcm_sf2_priv *priv,
intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR);
}
-static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static int bcm_sf2_port_setup(struct dsa_switch *ds, int port)
{
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
unsigned int i;
@@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
if (port == priv->moca_port)
bcm_sf2_port_intr_enable(priv, port);
- return b53_enable_port(ds, port, phy);
+ return b53_enable_port(ds, port);
}
-static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
{
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
u32 off, reg;
@@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
else
off = CORE_G_PCTL_PORT(port);
- b53_disable_port(ds, port, phy);
+ b53_disable_port(ds, port);
/* Power down the port memory */
reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
@@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
for (port = 0; port < DSA_MAX_PORTS; port++) {
if ((1 << port) & ds->enabled_port_mask ||
dsa_is_cpu_port(ds, port))
- bcm_sf2_port_disable(ds, port, NULL);
+ bcm_sf2_port_disable(ds, port);
}
return 0;
@@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
for (port = 0; port < DSA_MAX_PORTS; port++) {
if ((1 << port) & ds->enabled_port_mask)
- bcm_sf2_port_setup(ds, port, NULL);
+ bcm_sf2_port_setup(ds, port);
else if (dsa_is_cpu_port(ds, port))
bcm_sf2_imp_setup(ds, port);
}
@@ -745,7 +743,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
if (dsa_is_cpu_port(ds, port))
bcm_sf2_imp_setup(ds, port);
else if (!((1 << port) & ds->enabled_port_mask))
- bcm_sf2_port_disable(ds, port, NULL);
+ bcm_sf2_port_disable(ds, port);
}
bcm_sf2_sw_configure_vlan(ds);
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 07355db2ad81..0c33b02562dc 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -799,8 +799,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
}
}
-static int lan9303_port_enable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static int lan9303_port_enable(struct dsa_switch *ds, int port)
{
struct lan9303 *chip = ds->priv;
@@ -817,8 +816,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
return -ENODEV;
}
-static void lan9303_port_disable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static void lan9303_port_disable(struct dsa_switch *ds, int port)
{
struct lan9303 *chip = ds->priv;
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 56cd6d365352..4095c50ae111 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -418,8 +418,7 @@ static int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
return 0;
}
-static int ksz_enable_port(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static int ksz_enable_port(struct dsa_switch *ds, int port)
{
struct ksz_device *dev = ds->priv;
@@ -429,8 +428,7 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
return 0;
}
-static void ksz_disable_port(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static void ksz_disable_port(struct dsa_switch *ds, int port)
{
struct ksz_device *dev = ds->priv;
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index faa3b88d2206..0a7f6209767f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -693,8 +693,7 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
}
static int
-mt7530_port_enable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+mt7530_port_enable(struct dsa_switch *ds, int port)
{
struct mt7530_priv *priv = ds->priv;
@@ -719,8 +718,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
}
static void
-mt7530_port_disable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+mt7530_port_disable(struct dsa_switch *ds, int port)
{
struct mt7530_priv *priv = ds->priv;
@@ -1006,7 +1004,7 @@ mt7530_setup(struct dsa_switch *ds)
if (dsa_is_cpu_port(ds, i))
mt7530_cpu_port_enable(priv, i);
else
- mt7530_port_disable(ds, i, NULL);
+ mt7530_port_disable(ds, i);
}
/* Flush the FDB table */
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c6678aa9b4ef..e47898fb7dbc 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1862,8 +1862,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_DEFAULT_VLAN, 0);
}
-static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
+static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port)
{
struct mv88e6xxx_chip *chip = ds->priv;
int err;
@@ -1875,8 +1874,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
return err;
}
-static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
+static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
{
struct mv88e6xxx_chip *chip = ds->priv;
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 82f09711ac1a..622ee9b8e72b 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -743,8 +743,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
}
static int
-qca8k_port_enable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+qca8k_port_enable(struct dsa_switch *ds, int port)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
@@ -755,8 +754,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
}
static void
-qca8k_port_disable(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+qca8k_port_disable(struct dsa_switch *ds, int port)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8dee216a5a9b..65b031a69c19 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -337,10 +337,8 @@ struct dsa_switch_ops {
/*
* Port enable/disable
*/
- int (*port_enable)(struct dsa_switch *ds, int port,
- struct phy_device *phy);
- void (*port_disable)(struct dsa_switch *ds, int port,
- struct phy_device *phy);
+ int (*port_enable)(struct dsa_switch *ds, int port);
+ void (*port_disable)(struct dsa_switch *ds, int port);
/*
* Port's MAC EEE settings
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 606812160fd5..6290741e496a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -100,7 +100,7 @@ static int dsa_slave_open(struct net_device *dev)
}
if (ds->ops->port_enable) {
- err = ds->ops->port_enable(ds, p->dp->index, p->phy);
+ err = ds->ops->port_enable(ds, p->dp->index);
if (err)
goto clear_promisc;
}
@@ -155,7 +155,7 @@ static int dsa_slave_close(struct net_device *dev)
dev_uc_del(master, dev->dev_addr);
if (ds->ops->port_disable)
- ds->ops->port_disable(ds, p->dp->index, p->phy);
+ ds->ops->port_disable(ds, p->dp->index);
dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
--
2.14.1
bcm_sf2 is currently the only driver using the phy argument passed to
.port_enable. It resets the state machine if the phy has been hard
reset. This check is generic and can be moved to DSA core.
Signed-off-by: Vivien Didelot <[email protected]>
---
drivers/net/dsa/bcm_sf2.c | 16 +---------------
net/dsa/slave.c | 15 +++++++++++++--
2 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 898d5642b516..ad96b9725a2c 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -184,22 +184,8 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
core_writel(priv, reg, CORE_PORT_TC2_QOS_MAP_PORT(port));
/* Re-enable the GPHY and re-apply workarounds */
- if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) {
+ if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1)
bcm_sf2_gphy_enable_set(ds, true);
- if (phy) {
- /* if phy_stop() has been called before, phy
- * will be in halted state, and phy_start()
- * will call resume.
- *
- * the resume path does not configure back
- * autoneg settings, and since we hard reset
- * the phy manually here, we need to reset the
- * state machine also.
- */
- phy->state = PHY_READY;
- phy_init_hw(phy);
- }
- }
/* Enable MoCA port interrupts to get notified */
if (port == priv->moca_port)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 02ace7d462c4..606812160fd5 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -72,6 +72,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
static int dsa_slave_open(struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
+ struct phy_device *phy = p->phy;
struct dsa_port *dp = p->dp;
struct dsa_switch *ds = dp->ds;
struct net_device *master = dsa_master_netdev(p);
@@ -106,8 +107,18 @@ static int dsa_slave_open(struct net_device *dev)
dsa_port_set_state_now(p->dp, stp_state);
- if (p->phy)
- phy_start(p->phy);
+ if (phy) {
+ /* If phy_stop() has been called before, phy will be in
+ * halted state, and phy_start() will call resume.
+ *
+ * The resume path does not configure back autoneg
+ * settings, and since the internal phy may have been
+ * hard reset, we need to reset the state machine also.
+ */
+ phy->state = PHY_READY;
+ phy_init_hw(phy);
+ phy_start(phy);
+ }
return 0;
--
2.14.1
On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote:
> bcm_sf2 is currently the only driver using the phy argument passed to
> .port_enable. It resets the state machine if the phy has been hard
> reset. This check is generic and can be moved to DSA core.
>
> dsa_port_set_state_now(p->dp, stp_state);
>
> - if (p->phy)
> - phy_start(p->phy);
> + if (phy) {
> + /* If phy_stop() has been called before, phy will be in
> + * halted state, and phy_start() will call resume.
> + *
> + * The resume path does not configure back autoneg
> + * settings, and since the internal phy may have been
> + * hard reset, we need to reset the state machine also.
> + */
> + phy->state = PHY_READY;
> + phy_init_hw(phy);
> + phy_start(phy);
> + }
Hi Vivien
If this is generic, why is it needed at all here? Shouldn't this
actually by in phylib?
Florian ?
Andrew
On Fri, Sep 22, 2017 at 12:17:52PM -0400, Vivien Didelot wrote:
> The DSA slave open function configures the unicast MAC addresses on the
> master device, enable the switch port, change its STP state, then start
> the PHY device.
>
> Make the close function symmetric, by first stopping the PHY device,
> then changing the STP state, disabling the switch port and restore the
> master device.
>
> Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Fri, Sep 22, 2017 at 12:17:53PM -0400, Vivien Didelot wrote:
> Provide dsa_port_enable and dsa_port_disable helpers to respectively
> enable and disable a switch port. This makes the dsa_port_set_state_now
> helper static.
>
> Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> bcm_sf2 is currently the only driver using the phy argument passed to
> .port_enable. It resets the state machine if the phy has been hard
> reset. This check is generic and can be moved to DSA core.
This is completely specific to bcm_sf2 because it does call
bcm_sf2_gphy_enable_set() which performs a HW reset of the PHY, you
can't move this to the generic portion of net/dsa/slave.c. NACK.
>
> Signed-off-by: Vivien Didelot <[email protected]>
> ---
> drivers/net/dsa/bcm_sf2.c | 16 +---------------
> net/dsa/slave.c | 15 +++++++++++++--
> 2 files changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 898d5642b516..ad96b9725a2c 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -184,22 +184,8 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
> core_writel(priv, reg, CORE_PORT_TC2_QOS_MAP_PORT(port));
>
> /* Re-enable the GPHY and re-apply workarounds */
> - if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) {
> + if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1)
> bcm_sf2_gphy_enable_set(ds, true);
> - if (phy) {
> - /* if phy_stop() has been called before, phy
> - * will be in halted state, and phy_start()
> - * will call resume.
> - *
> - * the resume path does not configure back
> - * autoneg settings, and since we hard reset
> - * the phy manually here, we need to reset the
> - * state machine also.
> - */
> - phy->state = PHY_READY;
> - phy_init_hw(phy);
> - }
> - }
>
> /* Enable MoCA port interrupts to get notified */
> if (port == priv->moca_port)
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 02ace7d462c4..606812160fd5 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -72,6 +72,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
> static int dsa_slave_open(struct net_device *dev)
> {
> struct dsa_slave_priv *p = netdev_priv(dev);
> + struct phy_device *phy = p->phy;
> struct dsa_port *dp = p->dp;
> struct dsa_switch *ds = dp->ds;
> struct net_device *master = dsa_master_netdev(p);
> @@ -106,8 +107,18 @@ static int dsa_slave_open(struct net_device *dev)
>
> dsa_port_set_state_now(p->dp, stp_state);
>
> - if (p->phy)
> - phy_start(p->phy);
> + if (phy) {
> + /* If phy_stop() has been called before, phy will be in
> + * halted state, and phy_start() will call resume.
> + *
> + * The resume path does not configure back autoneg
> + * settings, and since the internal phy may have been
> + * hard reset, we need to reset the state machine also.
> + */
> + phy->state = PHY_READY;
> + phy_init_hw(phy);
> + phy_start(phy);
> + }
>
> return 0;
>
>
--
Florian
On 09/22/2017 09:32 AM, Andrew Lunn wrote:
> On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote:
>> bcm_sf2 is currently the only driver using the phy argument passed to
>> .port_enable. It resets the state machine if the phy has been hard
>> reset. This check is generic and can be moved to DSA core.
>>
>> dsa_port_set_state_now(p->dp, stp_state);
>>
>> - if (p->phy)
>> - phy_start(p->phy);
>> + if (phy) {
>> + /* If phy_stop() has been called before, phy will be in
>> + * halted state, and phy_start() will call resume.
>> + *
>> + * The resume path does not configure back autoneg
>> + * settings, and since the internal phy may have been
>> + * hard reset, we need to reset the state machine also.
>> + */
>> + phy->state = PHY_READY;
>> + phy_init_hw(phy);
>> + phy_start(phy);
>> + }
>
> Hi Vivien
>
> If this is generic, why is it needed at all here? Shouldn't this
> actually by in phylib?
This does not belong in the core logic within net/dsa/slave.c. The
reason why this is necessary here is because we are doing a HW-based
reset of the PHY, as the comment explains this is specific to how the HW
works. There may be a cleaner solution to this problem, but in any case,
I don't think other drivers should inherit that logic.
--
Florian
On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> The .port_enable and .port_disable functions are meant to deal with the
> switch ports only, and no driver is using the phy argument anyway.
> Remove it.
I don't think this makes sense, there are perfectly legit reasons why a
switch driver may have something to do with the PHY device attached to
its per-port network interface, we should definitively keep that around,
unless you think we should be accessing the PHY within the switch
drivers by doing:
struct phy_device *phydev = ds->ports[port].netdev->phydev?
>
> Signed-off-by: Vivien Didelot <[email protected]>
> ---
> drivers/net/dsa/b53/b53_common.c | 6 +++---
> drivers/net/dsa/b53/b53_priv.h | 4 ++--
> drivers/net/dsa/bcm_sf2.c | 16 +++++++---------
> drivers/net/dsa/lan9303-core.c | 6 ++----
> drivers/net/dsa/microchip/ksz_common.c | 6 ++----
> drivers/net/dsa/mt7530.c | 8 +++-----
> drivers/net/dsa/mv88e6xxx/chip.c | 6 ++----
> drivers/net/dsa/qca8k.c | 6 ++----
> include/net/dsa.h | 6 ++----
> net/dsa/slave.c | 4 ++--
> 10 files changed, 27 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index d4ce092def83..e46eb29d29f0 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
> }
> EXPORT_SYMBOL(b53_imp_vlan_setup);
>
> -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
> +int b53_enable_port(struct dsa_switch *ds, int port)
> {
> struct b53_device *dev = ds->priv;
> unsigned int cpu_port = dev->cpu_port;
> @@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
> }
> EXPORT_SYMBOL(b53_enable_port);
>
> -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
> +void b53_disable_port(struct dsa_switch *ds, int port)
> {
> struct b53_device *dev = ds->priv;
> u8 reg;
> @@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds)
> if (dsa_is_cpu_port(ds, port))
> b53_enable_cpu_port(dev, port);
> else if (!(BIT(port) & ds->enabled_port_mask))
> - b53_disable_port(ds, port, NULL);
> + b53_disable_port(ds, port);
> }
>
> return ret;
> diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
> index 603c66d240d8..688d02ee6155 100644
> --- a/drivers/net/dsa/b53/b53_priv.h
> +++ b/drivers/net/dsa/b53/b53_priv.h
> @@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
> struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
> void b53_mirror_del(struct dsa_switch *ds, int port,
> struct dsa_mall_mirror_tc_entry *mirror);
> -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
> -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
> +int b53_enable_port(struct dsa_switch *ds, int port);
> +void b53_disable_port(struct dsa_switch *ds, int port);
> void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
> void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
> int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ad96b9725a2c..77e0c43f973b 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct bcm_sf2_priv *priv,
> intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR);
> }
>
> -static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +static int bcm_sf2_port_setup(struct dsa_switch *ds, int port)
> {
> struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> unsigned int i;
> @@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
> if (port == priv->moca_port)
> bcm_sf2_port_intr_enable(priv, port);
>
> - return b53_enable_port(ds, port, phy);
> + return b53_enable_port(ds, port);
> }
>
> -static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
> {
> struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> u32 off, reg;
> @@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
> else
> off = CORE_G_PCTL_PORT(port);
>
> - b53_disable_port(ds, port, phy);
> + b53_disable_port(ds, port);
>
> /* Power down the port memory */
> reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
> @@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
> for (port = 0; port < DSA_MAX_PORTS; port++) {
> if ((1 << port) & ds->enabled_port_mask ||
> dsa_is_cpu_port(ds, port))
> - bcm_sf2_port_disable(ds, port, NULL);
> + bcm_sf2_port_disable(ds, port);
> }
>
> return 0;
> @@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
>
> for (port = 0; port < DSA_MAX_PORTS; port++) {
> if ((1 << port) & ds->enabled_port_mask)
> - bcm_sf2_port_setup(ds, port, NULL);
> + bcm_sf2_port_setup(ds, port);
> else if (dsa_is_cpu_port(ds, port))
> bcm_sf2_imp_setup(ds, port);
> }
> @@ -745,7 +743,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
> if (dsa_is_cpu_port(ds, port))
> bcm_sf2_imp_setup(ds, port);
> else if (!((1 << port) & ds->enabled_port_mask))
> - bcm_sf2_port_disable(ds, port, NULL);
> + bcm_sf2_port_disable(ds, port);
> }
>
> bcm_sf2_sw_configure_vlan(ds);
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 07355db2ad81..0c33b02562dc 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -799,8 +799,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> }
> }
>
> -static int lan9303_port_enable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +static int lan9303_port_enable(struct dsa_switch *ds, int port)
> {
> struct lan9303 *chip = ds->priv;
>
> @@ -817,8 +816,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
> return -ENODEV;
> }
>
> -static void lan9303_port_disable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +static void lan9303_port_disable(struct dsa_switch *ds, int port)
> {
> struct lan9303 *chip = ds->priv;
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 56cd6d365352..4095c50ae111 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -418,8 +418,7 @@ static int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
> return 0;
> }
>
> -static int ksz_enable_port(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +static int ksz_enable_port(struct dsa_switch *ds, int port)
> {
> struct ksz_device *dev = ds->priv;
>
> @@ -429,8 +428,7 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
> return 0;
> }
>
> -static void ksz_disable_port(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +static void ksz_disable_port(struct dsa_switch *ds, int port)
> {
> struct ksz_device *dev = ds->priv;
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index faa3b88d2206..0a7f6209767f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -693,8 +693,7 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
> }
>
> static int
> -mt7530_port_enable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +mt7530_port_enable(struct dsa_switch *ds, int port)
> {
> struct mt7530_priv *priv = ds->priv;
>
> @@ -719,8 +718,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
> }
>
> static void
> -mt7530_port_disable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +mt7530_port_disable(struct dsa_switch *ds, int port)
> {
> struct mt7530_priv *priv = ds->priv;
>
> @@ -1006,7 +1004,7 @@ mt7530_setup(struct dsa_switch *ds)
> if (dsa_is_cpu_port(ds, i))
> mt7530_cpu_port_enable(priv, i);
> else
> - mt7530_port_disable(ds, i, NULL);
> + mt7530_port_disable(ds, i);
> }
>
> /* Flush the FDB table */
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c6678aa9b4ef..e47898fb7dbc 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1862,8 +1862,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_DEFAULT_VLAN, 0);
> }
>
> -static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
> - struct phy_device *phydev)
> +static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port)
> {
> struct mv88e6xxx_chip *chip = ds->priv;
> int err;
> @@ -1875,8 +1874,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
> return err;
> }
>
> -static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port,
> - struct phy_device *phydev)
> +static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
> {
> struct mv88e6xxx_chip *chip = ds->priv;
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 82f09711ac1a..622ee9b8e72b 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -743,8 +743,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
> }
>
> static int
> -qca8k_port_enable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +qca8k_port_enable(struct dsa_switch *ds, int port)
> {
> struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>
> @@ -755,8 +754,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
> }
>
> static void
> -qca8k_port_disable(struct dsa_switch *ds, int port,
> - struct phy_device *phy)
> +qca8k_port_disable(struct dsa_switch *ds, int port)
> {
> struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 8dee216a5a9b..65b031a69c19 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -337,10 +337,8 @@ struct dsa_switch_ops {
> /*
> * Port enable/disable
> */
> - int (*port_enable)(struct dsa_switch *ds, int port,
> - struct phy_device *phy);
> - void (*port_disable)(struct dsa_switch *ds, int port,
> - struct phy_device *phy);
> + int (*port_enable)(struct dsa_switch *ds, int port);
> + void (*port_disable)(struct dsa_switch *ds, int port);
>
> /*
> * Port's MAC EEE settings
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 606812160fd5..6290741e496a 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -100,7 +100,7 @@ static int dsa_slave_open(struct net_device *dev)
> }
>
> if (ds->ops->port_enable) {
> - err = ds->ops->port_enable(ds, p->dp->index, p->phy);
> + err = ds->ops->port_enable(ds, p->dp->index);
> if (err)
> goto clear_promisc;
> }
> @@ -155,7 +155,7 @@ static int dsa_slave_close(struct net_device *dev)
> dev_uc_del(master, dev->dev_addr);
>
> if (ds->ops->port_disable)
> - ds->ops->port_disable(ds, p->dp->index, p->phy);
> + ds->ops->port_disable(ds, p->dp->index);
>
> dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
>
>
--
Florian
On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> The DSA slave open function configures the unicast MAC addresses on the
> master device, enable the switch port, change its STP state, then start
> the PHY device.
>
> Make the close function symmetric, by first stopping the PHY device,
> then changing the STP state, disabling the switch port and restore the
> master device.
>
> Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> Provide dsa_port_enable and dsa_port_disable helpers to respectively
> enable and disable a switch port. This makes the dsa_port_set_state_now
> helper static.
>
> Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hi Florian,
Florian Fainelli <[email protected]> writes:
> On 09/22/2017 09:17 AM, Vivien Didelot wrote:
>> The .port_enable and .port_disable functions are meant to deal with the
>> switch ports only, and no driver is using the phy argument anyway.
>> Remove it.
>
> I don't think this makes sense, there are perfectly legit reasons why a
> switch driver may have something to do with the PHY device attached to
> its per-port network interface, we should definitively keep that around,
> unless you think we should be accessing the PHY within the switch
> drivers by doing:
>
> struct phy_device *phydev = ds->ports[port].netdev->phydev?
bcm_sf2 is the only user for this phy argument right now. The reason I'm
doing this is because I prefer to discourage switch drivers to dig into
the phy device themselves while as you said there must be a cleaner
solution. This must be handled somehow elsewhere in the stack.
In the meantime, moving the PHY device up to the dsa_port structure is a
good solution, in order not to expose it in switch ops, but still make
it available to more complex drivers.
Do you know if netdev->phydev is usable? Why do DSA has its own copy in
dsa_slave_priv then?
I'll respin, thanks.
Vivien
On 09/22/2017 11:12 AM, Vivien Didelot wrote:
> Hi Florian,
>
> Florian Fainelli <[email protected]> writes:
>
>> On 09/22/2017 09:17 AM, Vivien Didelot wrote:
>>> The .port_enable and .port_disable functions are meant to deal with the
>>> switch ports only, and no driver is using the phy argument anyway.
>>> Remove it.
>>
>> I don't think this makes sense, there are perfectly legit reasons why a
>> switch driver may have something to do with the PHY device attached to
>> its per-port network interface, we should definitively keep that around,
>> unless you think we should be accessing the PHY within the switch
>> drivers by doing:
>>
>> struct phy_device *phydev = ds->ports[port].netdev->phydev?
>
> bcm_sf2 is the only user for this phy argument right now. The reason I'm
> doing this is because I prefer to discourage switch drivers to dig into
> the phy device themselves while as you said there must be a cleaner
> solution. This must be handled somehow elsewhere in the stack.
The current approach of passing the phy_device reference as an argument
is certainly a cleaner way then. The port_enable caller can provide the
correct phy_device and that lifts the switch driver from having to dig
it itself from its per-port netdev.
>
> In the meantime, moving the PHY device up to the dsa_port structure is a
> good solution, in order not to expose it in switch ops, but still make
> it available to more complex drivers.
>
> Do you know if netdev->phydev is usable? Why do DSA has its own copy in
> dsa_slave_priv then?
Historical reasons mostly. Considering the complexity of
dsa_slave_phy_setup(), I would certainly be extremely careful in
changing any of this, the potential for breakage is pretty big. At first
glance, I would say that this is a safe conversion to do, and I can test
this on the HW I have here anyway.
--
Florian
> Historical reasons mostly. Considering the complexity of
> dsa_slave_phy_setup(), I would certainly be extremely careful in
> changing any of this, the potential for breakage is pretty big.
Yes, i took a look at this, wondering how to convert to phylink. I
went away and got a stiff drink :-)
Andrew