Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700435 > unrolled thread

[PATCH net-next 00/11] net: dsa: rework EEE support

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 00/11] net: dsa: rework EEE support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 03/11] net: dsa: qca8k: enable EEE once Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 02/11] net: dsa: qca8k: fix EEE init Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 11/11] net: dsa: rename switch EEE ops Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 09/11] net: dsa: remove PHY device argument from .set_eee Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 04/11] net: dsa: qca8k: do not cache unneeded EEE fields Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 01/11] net: dsa: make EEE ops optional Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
      Re: [PATCH net-next 01/11] net: dsa: make EEE ops optional Andrew Lunn <andrew@lunn.ch> - 2017-08-01 16:10 +0200
    [PATCH net-next 06/11] net: dsa: bcm_sf2: remove unneeded supported flags Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
    [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 00:30 +0200
      Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Andrew Lunn <andrew@lunn.ch> - 2017-08-01 16:30 +0200
        Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 17:40 +0200
          Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Andrew Lunn <andrew@lunn.ch> - 2017-08-01 18:10 +0200
            Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 18:40 +0200
              Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-01 22:20 +0200
            Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Florian Fainelli <f.fainelli@gmail.com> - 2017-08-01 18:40 +0200
              Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Andrew Lunn <andrew@lunn.ch> - 2017-08-01 19:30 +0200
                Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support Florian Fainelli <f.fainelli@gmail.com> - 2017-08-01 21:00 +0200
    Re: [PATCH net-next 00/11] net: dsa: rework EEE support Andrew Lunn <andrew@lunn.ch> - 2017-08-01 16:40 +0200

#1700435 — [PATCH net-next 00/11] net: dsa: rework EEE support

EEE implies configuring the port's PHY and MAC of both ends of the wire.

The current EEE support in DSA mixes PHY and MAC configuration, which is
bad because PHYs must be configured through a proper PHY driver. The DSA
switch operations for EEE are only meant for configuring the port's MAC,
which are integrated in the Ethernet switch device.

This patchset fixes the EEE support in qca8k driver, makes the DSA layer
call phy_init_eee for all drivers, and remove the EEE support from the
mv88e6xxx driver since the Marvell PHY driver should be enough for it.

Vivien Didelot (11):
  net: dsa: make EEE ops optional
  net: dsa: qca8k: fix EEE init
  net: dsa: qca8k: enable EEE once
  net: dsa: qca8k: do not cache unneeded EEE fields
  net: dsa: qca8k: remove qca8k_get_eee
  net: dsa: bcm_sf2: remove unneeded supported flags
  net: dsa: mv88e6xxx: call phy_init_eee
  net: dsa: call phy_init_eee in DSA layer
  net: dsa: remove PHY device argument from .set_eee
  net: dsa: mv88e6xxx: remove EEE support
  net: dsa: rename switch EEE ops

 drivers/net/dsa/bcm_sf2.c        | 26 +++--------
 drivers/net/dsa/mv88e6xxx/chip.c | 82 ----------------------------------
 drivers/net/dsa/mv88e6xxx/chip.h |  6 ---
 drivers/net/dsa/mv88e6xxx/phy.c  | 96 ----------------------------------------
 drivers/net/dsa/mv88e6xxx/phy.h  | 22 ---------
 drivers/net/dsa/mv88e6xxx/port.c | 17 -------
 drivers/net/dsa/mv88e6xxx/port.h |  3 --
 drivers/net/dsa/qca8k.c          | 68 ++--------------------------
 drivers/net/dsa/qca8k.h          |  1 -
 include/net/dsa.h                | 11 +++--
 net/dsa/slave.c                  | 48 ++++++++++++--------
 11 files changed, 45 insertions(+), 335 deletions(-)

-- 
2.13.3

[toc] | [next] | [standalone]

#1700437 — [PATCH net-next 03/11] net: dsa: qca8k: enable EEE once

If EEE is queried enabled, qca8k_set_eee calls qca8k_eee_enable_set
twice (because it is already called in qca8k_eee_init). Fix that.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/qca8k.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index e076ab23d4df..9d6b5d2f7a4a 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -684,12 +684,13 @@ qca8k_set_eee(struct dsa_switch *ds, int port,
 
 	p->eee_enabled = e->eee_enabled;
 
-	if (e->eee_enabled) {
+	if (!p->eee_enabled) {
+		qca8k_eee_enable_set(ds, port, false);
+	} else {
 		p->eee_enabled = qca8k_eee_init(ds, port, phydev);
 		if (!p->eee_enabled)
 			ret = -EOPNOTSUPP;
 	}
-	qca8k_eee_enable_set(ds, port, p->eee_enabled);
 
 	return ret;
 }
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1700439 — [PATCH net-next 02/11] net: dsa: qca8k: fix EEE init

The qca8k obviously copied code from the sf2 driver as how to set EEE:

    if (e->eee_enabled) {
        p->eee_enabled = qca8k_eee_init(ds, port, phydev);
        if (!p->eee_enabled)
            ret = -EOPNOTSUPP;
    }

But it did not use the same logic for the EEE init routine, which is
"Returns 0 if EEE was not enabled, or 1 otherwise". This results in
returning -EOPNOTSUPP on success and caching EEE enabled on failure.

This patch fixes the returned value of qca8k_eee_init.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/qca8k.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b3bee7eab45f..e076ab23d4df 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -666,11 +666,11 @@ qca8k_eee_init(struct dsa_switch *ds, int port,
 
 	ret = phy_init_eee(phy, 0);
 	if (ret)
-		return ret;
+		return 0;
 
 	qca8k_eee_enable_set(ds, port, true);
 
-	return 0;
+	return 1;
 }
 
 static int
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1700440 — [PATCH net-next 11/11] net: dsa: rename switch EEE ops

To avoid confusion with the PHY EEE settings, rename the .set_eee and
.get_eee ops to respectively .set_mac_eee and .get_mac_eee.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/bcm_sf2.c | 12 ++++++------
 drivers/net/dsa/qca8k.c   |  4 ++--
 include/net/dsa.h         | 10 +++++-----
 net/dsa/slave.c           |  8 ++++----
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ce886345d8d2..6bbfa6ea1efb 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -338,8 +338,8 @@ static int bcm_sf2_eee_init(struct dsa_switch *ds, int port,
 	return 1;
 }
 
-static int bcm_sf2_sw_get_eee(struct dsa_switch *ds, int port,
-			      struct ethtool_eee *e)
+static int bcm_sf2_sw_get_mac_eee(struct dsa_switch *ds, int port,
+				  struct ethtool_eee *e)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	struct ethtool_eee *p = &priv->port_sts[port].eee;
@@ -352,8 +352,8 @@ static int bcm_sf2_sw_get_eee(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int bcm_sf2_sw_set_eee(struct dsa_switch *ds, int port,
-			      struct ethtool_eee *e)
+static int bcm_sf2_sw_set_mac_eee(struct dsa_switch *ds, int port,
+				  struct ethtool_eee *e)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	struct ethtool_eee *p = &priv->port_sts[port].eee;
@@ -1011,8 +1011,8 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.set_wol		= bcm_sf2_sw_set_wol,
 	.port_enable		= bcm_sf2_port_setup,
 	.port_disable		= bcm_sf2_port_disable,
-	.get_eee		= bcm_sf2_sw_get_eee,
-	.set_eee		= bcm_sf2_sw_set_eee,
+	.get_mac_eee		= bcm_sf2_sw_get_mac_eee,
+	.set_mac_eee		= bcm_sf2_sw_set_mac_eee,
 	.port_bridge_join	= b53_br_join,
 	.port_bridge_leave	= b53_br_leave,
 	.port_stp_state_set	= b53_br_set_stp_state,
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 400333077a9f..1f6cb107bc63 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -638,7 +638,7 @@ qca8k_get_sset_count(struct dsa_switch *ds)
 }
 
 static int
-qca8k_set_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
+qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
@@ -855,7 +855,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.phy_write		= qca8k_phy_write,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
-	.set_eee		= qca8k_set_eee,
+	.set_mac_eee		= qca8k_set_mac_eee,
 	.port_enable		= qca8k_port_enable,
 	.port_disable		= qca8k_port_disable,
 	.port_stp_state_set	= qca8k_port_stp_state_set,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index ce46db323394..0b1a0622b33c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -332,12 +332,12 @@ struct dsa_switch_ops {
 				struct phy_device *phy);
 
 	/*
-	 * EEE setttings
+	 * Port's MAC EEE settings
 	 */
-	int	(*set_eee)(struct dsa_switch *ds, int port,
-			   struct ethtool_eee *e);
-	int	(*get_eee)(struct dsa_switch *ds, int port,
-			   struct ethtool_eee *e);
+	int	(*set_mac_eee)(struct dsa_switch *ds, int port,
+			       struct ethtool_eee *e);
+	int	(*get_mac_eee)(struct dsa_switch *ds, int port,
+			       struct ethtool_eee *e);
 
 	/* EEPROM access */
 	int	(*get_eeprom_len)(struct dsa_switch *ds);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6bc75ab438e8..832a54c94652 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -648,8 +648,8 @@ static int dsa_slave_set_eee(struct net_device *dev, struct ethtool_eee *e)
 	struct dsa_switch *ds = p->dp->ds;
 	int err = -ENODEV;
 
-	if (ds->ops->set_eee) {
-		err = ds->ops->set_eee(ds, p->dp->index, e);
+	if (ds->ops->set_mac_eee) {
+		err = ds->ops->set_mac_eee(ds, p->dp->index, e);
 		if (err)
 			return err;
 	}
@@ -675,8 +675,8 @@ static int dsa_slave_get_eee(struct net_device *dev, struct ethtool_eee *e)
 	struct dsa_switch *ds = p->dp->ds;
 	int err = -ENODEV;
 
-	if (ds->ops->get_eee) {
-		err = ds->ops->get_eee(ds, p->dp->index, e);
+	if (ds->ops->get_mac_eee) {
+		err = ds->ops->get_mac_eee(ds, p->dp->index, e);
 		if (err)
 			return err;
 	}
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1700442 — [PATCH net-next 09/11] net: dsa: remove PHY device argument from .set_eee

The DSA switch operations for EEE are only meant to configure a port's
MAC EEE settings. The port's PHY EEE settings are accessed by the DSA
layer and must be made available via a proper PHY driver.

In order to reduce this confusion, remove the phy_device argument from
the .set_eee operation.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/bcm_sf2.c        |  1 -
 drivers/net/dsa/mv88e6xxx/chip.c |  2 +-
 drivers/net/dsa/qca8k.c          | 14 +++-----------
 include/net/dsa.h                |  1 -
 net/dsa/slave.c                  |  2 +-
 5 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 9d10aac8f241..ce886345d8d2 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -353,7 +353,6 @@ static int bcm_sf2_sw_get_eee(struct dsa_switch *ds, int port,
 }
 
 static int bcm_sf2_sw_set_eee(struct dsa_switch *ds, int port,
-			      struct phy_device *phydev,
 			      struct ethtool_eee *e)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 647d5d45c1d6..aaa96487f21f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -850,7 +850,7 @@ static int mv88e6xxx_get_eee(struct dsa_switch *ds, int port,
 }
 
 static int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
-			     struct phy_device *phydev, struct ethtool_eee *e)
+			     struct ethtool_eee *e)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 038a895d9a96..400333077a9f 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -637,8 +637,8 @@ qca8k_get_sset_count(struct dsa_switch *ds)
 	return ARRAY_SIZE(ar8327_mib);
 }
 
-static void
-qca8k_eee_enable_set(struct dsa_switch *ds, int port, bool enable)
+static int
+qca8k_set_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
@@ -646,20 +646,12 @@ qca8k_eee_enable_set(struct dsa_switch *ds, int port, bool enable)
 
 	mutex_lock(&priv->reg_mutex);
 	reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
-	if (enable)
+	if (eee->eee_enabled)
 		reg |= lpi_en;
 	else
 		reg &= ~lpi_en;
 	qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
 	mutex_unlock(&priv->reg_mutex);
-}
-
-static int
-qca8k_set_eee(struct dsa_switch *ds, int port,
-	      struct phy_device *phydev,
-	      struct ethtool_eee *e)
-{
-	qca8k_eee_enable_set(ds, port, e->eee_enabled);
 
 	return 0;
 }
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 88da272d20d0..ce46db323394 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -335,7 +335,6 @@ struct dsa_switch_ops {
 	 * EEE setttings
 	 */
 	int	(*set_eee)(struct dsa_switch *ds, int port,
-			   struct phy_device *phydev,
 			   struct ethtool_eee *e);
 	int	(*get_eee)(struct dsa_switch *ds, int port,
 			   struct ethtool_eee *e);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index bf71c206fe8f..6bc75ab438e8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -649,7 +649,7 @@ static int dsa_slave_set_eee(struct net_device *dev, struct ethtool_eee *e)
 	int err = -ENODEV;
 
 	if (ds->ops->set_eee) {
-		err = ds->ops->set_eee(ds, p->dp->index, p->phy, e);
+		err = ds->ops->set_eee(ds, p->dp->index, e);
 		if (err)
 			return err;
 	}
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1700443 — [PATCH net-next 04/11] net: dsa: qca8k: do not cache unneeded EEE fields

The qca8k driver is currently caching a bitfield of the supported member
of a ethtool_eee private structure, which is unused.

Only the eee_enabled field of the private ethtool_eee copy is updated,
thus using p->advertised and p->lp_advertised is also erroneous.

Remove the usage of these private ethtool_eee members and only rely on
phy_ethtool_get_eee to assign the eee_active member.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/qca8k.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9d6b5d2f7a4a..c316c55aabc6 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -658,12 +658,8 @@ static int
 qca8k_eee_init(struct dsa_switch *ds, int port,
 	       struct phy_device *phy)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	struct ethtool_eee *p = &priv->port_sts[port].eee;
 	int ret;
 
-	p->supported = (SUPPORTED_1000baseT_Full | SUPPORTED_100baseT_Full);
-
 	ret = phy_init_eee(phy, 0);
 	if (ret)
 		return 0;
@@ -705,12 +701,7 @@ qca8k_get_eee(struct dsa_switch *ds, int port,
 	int ret;
 
 	ret = phy_ethtool_get_eee(netdev->phydev, p);
-	if (!ret)
-		e->eee_active =
-			!!(p->supported & p->advertised & p->lp_advertised);
-	else
-		e->eee_active = 0;
-
+	e->eee_active = p->eee_active;
 	e->eee_enabled = p->eee_enabled;
 
 	return ret;
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1700446 — [PATCH net-next 01/11] net: dsa: make EEE ops optional

Even though EEE implies the port's PHY and MAC of both ends, a switch
may not need to do anything to configure the port's MAC.

This makes it impossible for the DSA layer to distinguish e.g. this case
from a disabled EEE when a driver returns 0 from the get EEE operation.

For this reason, make the EEE ops optional and call them only when
provided. Calling it first allows a switch driver to stop the whole
operation at runtime if a given switch does not support the EEE setting.

If both the MAC operation and PHY are not present, -ENODEV is returned.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9507bd38cf04..518145ced434 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -646,38 +646,42 @@ static int dsa_slave_set_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->dp->ds;
-	int ret;
+	int err = -ENODEV;
 
-	if (!ds->ops->set_eee)
-		return -EOPNOTSUPP;
+	if (ds->ops->set_eee) {
+		err = ds->ops->set_eee(ds, p->dp->index, p->phy, e);
+		if (err)
+			return err;
+	}
 
-	ret = ds->ops->set_eee(ds, p->dp->index, p->phy, e);
-	if (ret)
-		return ret;
+	if (p->phy) {
+		err = phy_ethtool_set_eee(p->phy, e);
+		if (err)
+			return err;
+	}
 
-	if (p->phy)
-		ret = phy_ethtool_set_eee(p->phy, e);
-
-	return ret;
+	return err;
 }
 
 static int dsa_slave_get_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->dp->ds;
-	int ret;
+	int err = -ENODEV;
 
-	if (!ds->ops->get_eee)
-		return -EOPNOTSUPP;
+	if (ds->ops->get_eee) {
+		err = ds->ops->get_eee(ds, p->dp->index, e);
+		if (err)
+			return err;
+	}
 
-	ret = ds->ops->get_eee(ds, p->dp->index, e);
-	if (ret)
-		return ret;
+	if (p->phy) {
+		err = phy_ethtool_get_eee(p->phy, e);
+		if (err)
+			return err;
+	}
 
-	if (p->phy)
-		ret = phy_ethtool_get_eee(p->phy, e);
-
-	return ret;
+	return err;
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1701084 — Re: [PATCH net-next 01/11] net: dsa: make EEE ops optional

Hi Vivien

> @@ -646,38 +646,42 @@ static int dsa_slave_set_eee(struct net_device *dev, struct ethtool_eee *e)
>  {
>  	struct dsa_slave_priv *p = netdev_priv(dev);
>  	struct dsa_switch *ds = p->dp->ds;
> -	int ret;
> +	int err = -ENODEV;
>  
> -	if (!ds->ops->set_eee)
> -		return -EOPNOTSUPP;
> +	if (ds->ops->set_eee) {
> +		err = ds->ops->set_eee(ds, p->dp->index, p->phy, e);
> +		if (err)
> +			return err;
> +	}
>  
> -	ret = ds->ops->set_eee(ds, p->dp->index, p->phy, e);
> -	if (ret)
> -		return ret;
> +	if (p->phy) {
> +		err = phy_ethtool_set_eee(p->phy, e);
> +		if (err)
> +			return err;

I don't think you need this if (err). You unconditionally return err
as you exit the function.

> +	}
>  
> -	if (p->phy)
> -		ret = phy_ethtool_set_eee(p->phy, e);
> -
> -	return ret;
> +	return err;




>  }
>  
>  static int dsa_slave_get_eee(struct net_device *dev, struct ethtool_eee *e)
>  {
>  	struct dsa_slave_priv *p = netdev_priv(dev);
>  	struct dsa_switch *ds = p->dp->ds;
> -	int ret;
> +	int err = -ENODEV;
>  
> -	if (!ds->ops->get_eee)
> -		return -EOPNOTSUPP;
> +	if (ds->ops->get_eee) {
> +		err = ds->ops->get_eee(ds, p->dp->index, e);
> +		if (err)
> +			return err;
> +	}
>  
> -	ret = ds->ops->get_eee(ds, p->dp->index, e);
> -	if (ret)
> -		return ret;
> +	if (p->phy) {
> +		err = phy_ethtool_get_eee(p->phy, e);
> +		if (err)
> +			return err;

Same here.

> +	}
>  
> -	if (p->phy)
> -		ret = phy_ethtool_get_eee(p->phy, e);
> -
> -	return ret;
> +	return err;
>  }
>  
>  #ifdef CONFIG_NET_POLL_CONTROLLER
> -- 
> 2.13.3
> 

[toc] | [prev] | [next] | [standalone]

#1700447 — [PATCH net-next 06/11] net: dsa: bcm_sf2: remove unneeded supported flags

The SF2 driver is masking the supported bitfield of its private copy of
the ports' ethtool_eee structures. It is used nowhere, thus remove it.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/bcm_sf2.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 648f91b58d1e..aef475f1ce06 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -327,12 +327,8 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
 static int bcm_sf2_eee_init(struct dsa_switch *ds, int port,
 			    struct phy_device *phy)
 {
-	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	struct ethtool_eee *p = &priv->port_sts[port].eee;
 	int ret;
 
-	p->supported = (SUPPORTED_1000baseT_Full | SUPPORTED_100baseT_Full);
-
 	ret = phy_init_eee(phy, 0);
 	if (ret)
 		return 0;
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1700448 — [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

The PHY's EEE settings are already accessed by the DSA layer through the
Marvell PHY driver and there is nothing to be done for switch's MACs.

Remove all EEE support from the mv88e6xxx driver.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 82 ----------------------------------
 drivers/net/dsa/mv88e6xxx/chip.h |  6 ---
 drivers/net/dsa/mv88e6xxx/phy.c  | 96 ----------------------------------------
 drivers/net/dsa/mv88e6xxx/phy.h  | 22 ---------
 drivers/net/dsa/mv88e6xxx/port.c | 17 -------
 drivers/net/dsa/mv88e6xxx/port.h |  3 --
 6 files changed, 226 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index aaa96487f21f..746ebf2fed80 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -810,58 +810,6 @@ static void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
 	mutex_unlock(&chip->reg_lock);
 }
 
-static int mv88e6xxx_energy_detect_read(struct mv88e6xxx_chip *chip, int port,
-					struct ethtool_eee *eee)
-{
-	int err;
-
-	if (!chip->info->ops->phy_energy_detect_read)
-		return -EOPNOTSUPP;
-
-	/* assign eee->eee_enabled and eee->tx_lpi_enabled */
-	err = chip->info->ops->phy_energy_detect_read(chip, port, eee);
-	if (err)
-		return err;
-
-	/* assign eee->eee_active */
-	return mv88e6xxx_port_status_eee(chip, port, eee);
-}
-
-static int mv88e6xxx_energy_detect_write(struct mv88e6xxx_chip *chip, int port,
-					 struct ethtool_eee *eee)
-{
-	if (!chip->info->ops->phy_energy_detect_write)
-		return -EOPNOTSUPP;
-
-	return chip->info->ops->phy_energy_detect_write(chip, port, eee);
-}
-
-static int mv88e6xxx_get_eee(struct dsa_switch *ds, int port,
-			     struct ethtool_eee *e)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-	int err;
-
-	mutex_lock(&chip->reg_lock);
-	err = mv88e6xxx_energy_detect_read(chip, port, e);
-	mutex_unlock(&chip->reg_lock);
-
-	return err;
-}
-
-static int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
-			     struct ethtool_eee *e)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-	int err;
-
-	mutex_lock(&chip->reg_lock);
-	err = mv88e6xxx_energy_detect_write(chip, port, e);
-	mutex_unlock(&chip->reg_lock);
-
-	return err;
-}
-
 static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 {
 	struct dsa_switch *ds = NULL;
@@ -2521,8 +2469,6 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2648,8 +2594,6 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
@@ -2719,8 +2663,6 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
@@ -2784,8 +2726,6 @@ static const struct mv88e6xxx_ops mv88e6190_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6390_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6390_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2821,8 +2761,6 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6390_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6390_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2858,8 +2796,6 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6390_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6390_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2895,8 +2831,6 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
@@ -2933,8 +2867,6 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6390_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6390_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2971,8 +2903,6 @@ static const struct mv88e6xxx_ops mv88e6320_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_speed = mv88e6185_port_set_speed,
@@ -3006,8 +2936,6 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_speed = mv88e6185_port_set_speed,
@@ -3039,8 +2967,6 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -3142,8 +3068,6 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
@@ -3180,8 +3104,6 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6390_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6390_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -3220,8 +3142,6 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
-	.phy_energy_detect_read = mv88e6390_phy_energy_detect_read,
-	.phy_energy_detect_write = mv88e6390_phy_energy_detect_write,
 	.port_set_link = mv88e6xxx_port_set_link,
 	.port_set_duplex = mv88e6xxx_port_set_duplex,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -3956,8 +3876,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_sset_count		= mv88e6xxx_get_sset_count,
 	.port_enable		= mv88e6xxx_port_enable,
 	.port_disable		= mv88e6xxx_port_disable,
-	.set_eee		= mv88e6xxx_set_eee,
-	.get_eee		= mv88e6xxx_get_eee,
 	.get_eeprom_len		= mv88e6xxx_get_eeprom_len,
 	.get_eeprom		= mv88e6xxx_get_eeprom,
 	.set_eeprom		= mv88e6xxx_set_eeprom,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 9111e1316250..334f6f7544ba 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -239,12 +239,6 @@ struct mv88e6xxx_ops {
 			 struct mii_bus *bus,
 			 int addr, int reg, u16 val);
 
-	/* Copper Energy Detect operations */
-	int (*phy_energy_detect_read)(struct mv88e6xxx_chip *chip, int phy,
-				      struct ethtool_eee *eee);
-	int (*phy_energy_detect_write)(struct mv88e6xxx_chip *chip, int phy,
-				       struct ethtool_eee *eee);
-
 	/* Priority Override Table operations */
 	int (*pot_clear)(struct mv88e6xxx_chip *chip);
 
diff --git a/drivers/net/dsa/mv88e6xxx/phy.c b/drivers/net/dsa/mv88e6xxx/phy.c
index 317ae89cfa68..436668bd50dc 100644
--- a/drivers/net/dsa/mv88e6xxx/phy.c
+++ b/drivers/net/dsa/mv88e6xxx/phy.c
@@ -246,99 +246,3 @@ int mv88e6xxx_phy_setup(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_phy_ppu_enable(chip);
 }
-
-/* Page 0, Register 16: Copper Specific Control Register 1 */
-
-int mv88e6352_phy_energy_detect_read(struct mv88e6xxx_chip *chip, int phy,
-				     struct ethtool_eee *eee)
-{
-	u16 val;
-	int err;
-
-	err = mv88e6xxx_phy_read(chip, phy, MV88E6XXX_PHY_CSCTL1, &val);
-	if (err)
-		return err;
-
-	val &= MV88E6352_PHY_CSCTL1_ENERGY_DETECT_MASK;
-
-	eee->eee_enabled = false;
-	eee->tx_lpi_enabled = false;
-
-	switch (val) {
-	case MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP:
-		eee->tx_lpi_enabled = true;
-		/* fall through... */
-	case MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV:
-		eee->eee_enabled = true;
-	}
-
-	return 0;
-}
-
-int mv88e6352_phy_energy_detect_write(struct mv88e6xxx_chip *chip, int phy,
-				      struct ethtool_eee *eee)
-{
-	u16 val;
-	int err;
-
-	err = mv88e6xxx_phy_read(chip, phy, MV88E6XXX_PHY_CSCTL1, &val);
-	if (err)
-		return err;
-
-	val &= ~MV88E6352_PHY_CSCTL1_ENERGY_DETECT_MASK;
-
-	if (eee->eee_enabled)
-		val |= MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV;
-	if (eee->tx_lpi_enabled)
-		val |= MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP;
-
-	return mv88e6xxx_phy_write(chip, phy, MV88E6XXX_PHY_CSCTL1, val);
-}
-
-int mv88e6390_phy_energy_detect_read(struct mv88e6xxx_chip *chip, int phy,
-				     struct ethtool_eee *eee)
-{
-	u16 val;
-	int err;
-
-	err = mv88e6xxx_phy_read(chip, phy, MV88E6XXX_PHY_CSCTL1, &val);
-	if (err)
-		return err;
-
-	val &= MV88E6390_PHY_CSCTL1_ENERGY_DETECT_MASK;
-
-	eee->eee_enabled = false;
-	eee->tx_lpi_enabled = false;
-
-	switch (val) {
-	case MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP_AUTO:
-	case MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP_SW:
-		eee->tx_lpi_enabled = true;
-		/* fall through... */
-	case MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV_AUTO:
-	case MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV_SW:
-		eee->eee_enabled = true;
-	}
-
-	return 0;
-}
-
-int mv88e6390_phy_energy_detect_write(struct mv88e6xxx_chip *chip, int phy,
-				      struct ethtool_eee *eee)
-{
-	u16 val;
-	int err;
-
-	err = mv88e6xxx_phy_read(chip, phy, MV88E6XXX_PHY_CSCTL1, &val);
-	if (err)
-		return err;
-
-	val &= ~MV88E6390_PHY_CSCTL1_ENERGY_DETECT_MASK;
-
-	if (eee->eee_enabled)
-		val |= MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV_AUTO;
-	if (eee->tx_lpi_enabled)
-		val |= MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP_AUTO;
-
-	return mv88e6xxx_phy_write(chip, phy, MV88E6XXX_PHY_CSCTL1, val);
-}
diff --git a/drivers/net/dsa/mv88e6xxx/phy.h b/drivers/net/dsa/mv88e6xxx/phy.h
index 988802799ad6..556b74a0502a 100644
--- a/drivers/net/dsa/mv88e6xxx/phy.h
+++ b/drivers/net/dsa/mv88e6xxx/phy.h
@@ -17,19 +17,6 @@
 #define MV88E6XXX_PHY_PAGE		0x16
 #define MV88E6XXX_PHY_PAGE_COPPER	0x00
 
-/* Page 0, Register 16: Copper Specific Control Register 1 */
-#define MV88E6XXX_PHY_CSCTL1					16
-#define MV88E6352_PHY_CSCTL1_ENERGY_DETECT_MASK			0x0300
-#define MV88E6352_PHY_CSCTL1_ENERGY_DETECT_OFF_MASK		0x0100 /* 0x */
-#define MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV		0x0200
-#define MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP		0x0300
-#define MV88E6390_PHY_CSCTL1_ENERGY_DETECT_MASK			0x0380
-#define MV88E6390_PHY_CSCTL1_ENERGY_DETECT_OFF_MASK		0x0180 /* 0xx */
-#define MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV_AUTO	0x0200
-#define MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV_SW		0x0280
-#define MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP_AUTO	0x0300
-#define MV88E6390_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP_SW		0x0380
-
 /* PHY Registers accesses implementations */
 int mv88e6165_phy_read(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
 		       int addr, int reg, u16 *val);
@@ -53,13 +40,4 @@ void mv88e6xxx_phy_init(struct mv88e6xxx_chip *chip);
 void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip);
 int mv88e6xxx_phy_setup(struct mv88e6xxx_chip *chip);
 
-int mv88e6352_phy_energy_detect_read(struct mv88e6xxx_chip *chip, int phy,
-				     struct ethtool_eee *eee);
-int mv88e6352_phy_energy_detect_write(struct mv88e6xxx_chip *chip, int phy,
-				      struct ethtool_eee *eee);
-int mv88e6390_phy_energy_detect_read(struct mv88e6xxx_chip *chip, int phy,
-				     struct ethtool_eee *eee);
-int mv88e6390_phy_energy_detect_write(struct mv88e6xxx_chip *chip, int phy,
-				      struct ethtool_eee *eee);
-
 #endif /*_MV88E6XXX_PHY_H */
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 2837a9128557..a7801f6668a5 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -35,23 +35,6 @@ int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
 	return mv88e6xxx_write(chip, addr, reg, val);
 }
 
-/* Offset 0x00: Port Status Register */
-
-int mv88e6xxx_port_status_eee(struct mv88e6xxx_chip *chip, int port,
-			      struct ethtool_eee *eee)
-{
-	u16 val;
-	int err;
-
-	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, &val);
-	if (err)
-		return err;
-
-	eee->eee_active = !!(val & MV88E6352_PORT_STS_EEE);
-
-	return 0;
-}
-
 /* Offset 0x01: MAC (or PCS or Physical) Control Register
  *
  * Link, Duplex and Flow Control have one force bit, one value bit.
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 6fcab309cd85..b16d5f0e6e9c 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -241,9 +241,6 @@ int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
 int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
 			 u16 val);
 
-int mv88e6xxx_port_status_eee(struct mv88e6xxx_chip *chip, int port,
-			      struct ethtool_eee *eee);
-
 int mv88e6352_port_set_rgmii_delay(struct mv88e6xxx_chip *chip, int port,
 				   phy_interface_t mode);
 int mv88e6390_port_set_rgmii_delay(struct mv88e6xxx_chip *chip, int port,
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1701112 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

On Mon, Jul 31, 2017 at 06:17:18PM -0400, Vivien Didelot wrote:
> The PHY's EEE settings are already accessed by the DSA layer through the
> Marvell PHY driver and there is nothing to be done for switch's MACs.

I'm confused, or missing something. Does not patch #1 mean that if the
DSA driver does not have a set_eee function, we always return -ENODEV
in slave.c?

There might be nothing to configure here, but some of the switches do
support EEE. So we need at least a NOP set_eee. Better still it should
return -ENODEV for those switches which don't actually support EEE,
and 0 for those that do?

    Andrew

[toc] | [prev] | [next] | [standalone]

#1701162 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Mon, Jul 31, 2017 at 06:17:18PM -0400, Vivien Didelot wrote:
>> The PHY's EEE settings are already accessed by the DSA layer through the
>> Marvell PHY driver and there is nothing to be done for switch's MACs.
>
> I'm confused, or missing something. Does not patch #1 mean that if the
> DSA driver does not have a set_eee function, we always return -ENODEV
> in slave.c?

If there is a PHY, phy_init_eee (if eee_enabled is true) and
phy_ethtool_set_eee is called. If there is a .set_eee op, it is
called. If both are absent, -ENODEV is returned.

> There might be nothing to configure here, but some of the switches do
> support EEE. So we need at least a NOP set_eee. Better still it should
> return -ENODEV for those switches which don't actually support EEE,
> and 0 for those that do?

As I explain in a commit message, I didn't want to make the EEE ops
mandatory, because it makes it impossible for the DSA layer to
distinguish whether the driver did not update the ethtool_eee structure
because there is nothing to do on the port's MAC side (e.g. mv88e6xxx or
qca8k) or if it returned EEE disabled. To avoid confusion, I prefered to
make the ops optional, making the phy_* calls enough in the first case.

That being said, if you don't share this point of view and prefer to
define an inline dsa_set_eee_noop() function, I don't mind, since this
allows the DSA layer to make the distinction.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1701195 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

On Tue, Aug 01, 2017 at 11:36:13AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Mon, Jul 31, 2017 at 06:17:18PM -0400, Vivien Didelot wrote:
> >> The PHY's EEE settings are already accessed by the DSA layer through the
> >> Marvell PHY driver and there is nothing to be done for switch's MACs.
> >
> > I'm confused, or missing something. Does not patch #1 mean that if the
> > DSA driver does not have a set_eee function, we always return -ENODEV
> > in slave.c?
> 
> If there is a PHY, phy_init_eee (if eee_enabled is true) and
> phy_ethtool_set_eee is called. If there is a .set_eee op, it is
> called. If both are absent, -ENODEV is returned.

O.K, i don't think that is correct. EEE should only be enabled if both
the MAC and the PHY supports it. We need some way for the MAC to
indicate it does not support EEE.

If set_eee is optional the meaning of a NULL pointer is that the MAC
does support EEE. So for mv88e6060, lan9303, microchip and mt7530
which currently don't support EEE, you need to add a set_eee which
returns -ENODEV.

Having to implement the op to say you don't implement the feature just
seems wrong.

      Andrew

[toc] | [prev] | [next] | [standalone]

#1701223 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> >> The PHY's EEE settings are already accessed by the DSA layer through the
>> >> Marvell PHY driver and there is nothing to be done for switch's MACs.
>> >
>> > I'm confused, or missing something. Does not patch #1 mean that if the
>> > DSA driver does not have a set_eee function, we always return -ENODEV
>> > in slave.c?
>> 
>> If there is a PHY, phy_init_eee (if eee_enabled is true) and
>> phy_ethtool_set_eee is called. If there is a .set_eee op, it is
>> called. If both are absent, -ENODEV is returned.
>
> O.K, i don't think that is correct. EEE should only be enabled if both
> the MAC and the PHY supports it. We need some way for the MAC to
> indicate it does not support EEE.
>
> If set_eee is optional the meaning of a NULL pointer is that the MAC
> does support EEE. So for mv88e6060, lan9303, microchip and mt7530
> which currently don't support EEE, you need to add a set_eee which
> returns -ENODEV.
>
> Having to implement the op to say you don't implement the feature just
> seems wrong.

Agreed, above I simply described how this patchset currently behaves.

I suggested in the previous mail to define a DSA noop so that the driver
can indicate that its MACs supports EEE, even though there's nothing to
do (and the DSA layer can learn about that):

    static inline int dsa_set_mac_eee_noop(struct dsa_switch *ds,
                                           int port,
                                           struct ethtool_eee *e)
    {
        dev_dbg(ds->dev, "nothing to do for port %d's MAC\n", port);
        return 0;
    }

(and the respective dsa_get_mac_eee_noop() for sure.)

Second option is: we keep it KISS and let the driver define its noop,
but as I explain, it is confusion, especially for the get operation.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1701429 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Second option is: we keep it KISS and let the driver define its noop,
> but as I explain, it is confusing, especially for the get operation.

In fact we should be good because the DSA layer will call
ds->ops->{g,s}et_mac_eee before phy_ethtool_{g,s}et_eee, so if the DSA
driver didn't touch the ethtool_eee structure, the PHY ops will anyway.

I'm sending a v2 right away.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1701225 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

On 08/01/2017 09:06 AM, Andrew Lunn wrote:
> On Tue, Aug 01, 2017 at 11:36:13AM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>>
>> Andrew Lunn <andrew@lunn.ch> writes:
>>
>>> On Mon, Jul 31, 2017 at 06:17:18PM -0400, Vivien Didelot wrote:
>>>> The PHY's EEE settings are already accessed by the DSA layer through the
>>>> Marvell PHY driver and there is nothing to be done for switch's MACs.
>>>
>>> I'm confused, or missing something. Does not patch #1 mean that if the
>>> DSA driver does not have a set_eee function, we always return -ENODEV
>>> in slave.c?
>>
>> If there is a PHY, phy_init_eee (if eee_enabled is true) and
>> phy_ethtool_set_eee is called. If there is a .set_eee op, it is
>> called. If both are absent, -ENODEV is returned.
> 
> O.K, i don't think that is correct. EEE should only be enabled if both
> the MAC and the PHY supports it. We need some way for the MAC to
> indicate it does not support EEE.

If the MAC does not support EEE but the PHY does I think you can still
allow EEE to be advertised and enabled, you just won't have the MAC be
able to leverage the power savings that EEE brings. AFAICT this is still
a valid mode whereby the PHY is put in a lower power mode, just not the
whole transmit path (MAC + PHY).

> 
> If set_eee is optional the meaning of a NULL pointer is that the MAC
> does support EEE. So for mv88e6060, lan9303, microchip and mt7530
> which currently don't support EEE, you need to add a set_eee which
> returns -ENODEV.
> 
> Having to implement the op to say you don't implement the feature just
> seems wrong.

If it is truly optional then we can either request drivers to implement
it and return something agreed upon, or just not implementing it means
we might be able to do EEE only at the PHY level.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1701266 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

> If the MAC does not support EEE but the PHY does I think you can still
> allow EEE to be advertised and enabled, you just won't have the MAC be
> able to leverage the power savings that EEE brings. AFAICT this is still
> a valid mode whereby the PHY is put in a lower power mode, just not the
> whole transmit path (MAC + PHY).

Hi Florian

I read a couple of datasheets for a few phys doing EEE. Both said the
same, the MAC has to indicate to the PHY when low power should be
entered and existed. The PHY itself does not appear to do anything on
its own.

It would be good to read the standards about this. But i don't think
we should tell userspace EEE is enabled, if the PHY has it enabled,
but the MAC is not capable and hence EEE is not actually being used at
all.

	Andrew

[toc] | [prev] | [next] | [standalone]

#1701346 — Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

On 08/01/2017 10:27 AM, Andrew Lunn wrote:
>> If the MAC does not support EEE but the PHY does I think you can still
>> allow EEE to be advertised and enabled, you just won't have the MAC be
>> able to leverage the power savings that EEE brings. AFAICT this is still
>> a valid mode whereby the PHY is put in a lower power mode, just not the
>> whole transmit path (MAC + PHY).
> 
> Hi Florian
> 
> I read a couple of datasheets for a few phys doing EEE. Both said the
> same, the MAC has to indicate to the PHY when low power should be
> entered and existed. The PHY itself does not appear to do anything on
> its own.

Oh you are right, the LPI signal has to come from the MAC for the PHY to
decide how to do the idle signaling, my bad.

> 
> It would be good to read the standards about this. But i don't think
> we should tell userspace EEE is enabled, if the PHY has it enabled,
> but the MAC is not capable and hence EEE is not actually being used at
> all.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1701118

On Mon, Jul 31, 2017 at 06:17:08PM -0400, Vivien Didelot wrote:
> EEE implies configuring the port's PHY and MAC of both ends of the wire.
> 
> The current EEE support in DSA mixes PHY and MAC configuration, which is
> bad because PHYs must be configured through a proper PHY driver. The DSA
> switch operations for EEE are only meant for configuring the port's MAC,
> which are integrated in the Ethernet switch device.
> 
> This patchset fixes the EEE support in qca8k driver, makes the DSA layer
> call phy_init_eee for all drivers, and remove the EEE support from the
> mv88e6xxx driver since the Marvell PHY driver should be enough for it.

Hi Vivien

Thanks for working on this. I like the general direction this takes,
moving the repeated code into slave.c

       Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web