Re: [PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
From: Guenter Roeck Date: Fri, 6 Mar 2015 22:23:51 -0800 > EEE configuration is similar for the various MV88E6xxx chips. > Add generic support for it. > > Signed-off-by: Guenter Roeck > Reviewed-by: Florian Fainelli Applied. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
From: Guenter Roeck li...@roeck-us.net Date: Fri, 6 Mar 2015 22:23:51 -0800 EEE configuration is similar for the various MV88E6xxx chips. Add generic support for it. Signed-off-by: Guenter Roeck li...@roeck-us.net Reviewed-by: Florian Fainelli f.faine...@gmail.com Applied. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
EEE configuration is similar for the various MV88E6xxx chips.
Add generic support for it.
Signed-off-by: Guenter Roeck
Reviewed-by: Florian Fainelli
---
Changes since RFT:
- Additional testing; no code changes
- Dropped comment about phy_init_eee
drivers/net/dsa/mv88e6xxx.c | 51 +
drivers/net/dsa/mv88e6xxx.h | 3 +++
2 files changed, 54 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a83ace0..c18ffc9 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -649,6 +649,57 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds,
int addr, int regnum,
return mv88e6xxx_phy_wait(ds);
}
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
+{
+ int reg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg < 0)
+ return -EOPNOTSUPP;
+
+ e->eee_enabled = !!(reg & 0x0200);
+ e->tx_lpi_enabled = !!(reg & 0x0100);
+
+ reg = REG_READ(REG_PORT(port), 0);
+ e->eee_active = !!(reg & 0x0040);
+
+ return 0;
+}
+
+static int mv88e6xxx_eee_enable_set(struct dsa_switch *ds, int port,
+ bool eee_enabled, bool tx_lpi_enabled)
+{
+ int reg, nreg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg < 0)
+ return reg;
+
+ nreg = reg & ~0x0300;
+ if (eee_enabled)
+ nreg |= 0x0200;
+ if (tx_lpi_enabled)
+ nreg |= 0x0100;
+
+ if (nreg != reg)
+ return mv88e6xxx_phy_write_indirect(ds, port, 16, nreg);
+
+ return 0;
+}
+
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e)
+{
+ int ret;
+
+ ret = mv88e6xxx_eee_enable_set(ds, port, e->eee_enabled,
+ e->tx_lpi_enabled);
+ if (ret)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
static int __init mv88e6xxx_init(void)
{
#if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 7294227..5fd42ce 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -88,6 +88,9 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
u16 val);
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e);
extern struct dsa_switch_driver mv88e6131_switch_driver;
extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
EEE configuration is similar for the various MV88E6xxx chips.
Add generic support for it.
Signed-off-by: Guenter Roeck li...@roeck-us.net
Reviewed-by: Florian Fainelli f.faine...@gmail.com
---
Changes since RFT:
- Additional testing; no code changes
- Dropped comment about phy_init_eee
drivers/net/dsa/mv88e6xxx.c | 51 +
drivers/net/dsa/mv88e6xxx.h | 3 +++
2 files changed, 54 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a83ace0..c18ffc9 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -649,6 +649,57 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds,
int addr, int regnum,
return mv88e6xxx_phy_wait(ds);
}
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
+{
+ int reg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg 0)
+ return -EOPNOTSUPP;
+
+ e-eee_enabled = !!(reg 0x0200);
+ e-tx_lpi_enabled = !!(reg 0x0100);
+
+ reg = REG_READ(REG_PORT(port), 0);
+ e-eee_active = !!(reg 0x0040);
+
+ return 0;
+}
+
+static int mv88e6xxx_eee_enable_set(struct dsa_switch *ds, int port,
+ bool eee_enabled, bool tx_lpi_enabled)
+{
+ int reg, nreg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg 0)
+ return reg;
+
+ nreg = reg ~0x0300;
+ if (eee_enabled)
+ nreg |= 0x0200;
+ if (tx_lpi_enabled)
+ nreg |= 0x0100;
+
+ if (nreg != reg)
+ return mv88e6xxx_phy_write_indirect(ds, port, 16, nreg);
+
+ return 0;
+}
+
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e)
+{
+ int ret;
+
+ ret = mv88e6xxx_eee_enable_set(ds, port, e-eee_enabled,
+ e-tx_lpi_enabled);
+ if (ret)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
static int __init mv88e6xxx_init(void)
{
#if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 7294227..5fd42ce 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -88,6 +88,9 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
u16 val);
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e);
extern struct dsa_switch_driver mv88e6131_switch_driver;
extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 23/02/15 18:29, Guenter Roeck wrote: > On 02/23/2015 02:19 PM, David Miller wrote: >> From: Guenter Roeck >> Date: Mon, 23 Feb 2015 08:26:09 -0800 >> >>> +/* Don't call phy_init_eee for now. It fails if the link is down, >>> + * but that should not really be a reason to fail configuration. >>> + */ >> >> I think there is some confusion about phy_init_eee(). >> >> You invoke it after a link has been established. Because programming >> the MDIO registers that turn on EEE can only be done if the link >> is configured in a certain way. >> >> If you look at stmmac, it invokes phy_init_eee() via it's adjust_link >> callback passed to phy_connect(). This is basically how I would >> expect it to be used, in that any time a link parameter changes we >> rerun phy_init_eee() to check the link partner registers, duplex >> state, etc. >> >> SXGBE on the other hand seems to not be using phy_init_eee() properly, >> it only invokes it once per device open and that makes no sense at all >> because then you're stuck with the eee state resulting from the link >> state at open time. >> > That explains a lot. I suspect the use in bcm_sf2 and bcmgenet is also > not as intended. Yes, it certainly needs fixing. -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 02/23/2015 02:19 PM, David Miller wrote: From: Guenter Roeck Date: Mon, 23 Feb 2015 08:26:09 -0800 + /* Don't call phy_init_eee for now. It fails if the link is down, +* but that should not really be a reason to fail configuration. +*/ I think there is some confusion about phy_init_eee(). You invoke it after a link has been established. Because programming the MDIO registers that turn on EEE can only be done if the link is configured in a certain way. If you look at stmmac, it invokes phy_init_eee() via it's adjust_link callback passed to phy_connect(). This is basically how I would expect it to be used, in that any time a link parameter changes we rerun phy_init_eee() to check the link partner registers, duplex state, etc. SXGBE on the other hand seems to not be using phy_init_eee() properly, it only invokes it once per device open and that makes no sense at all because then you're stuck with the eee state resulting from the link state at open time. That explains a lot. I suspect the use in bcm_sf2 and bcmgenet is also not as intended. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
From: Guenter Roeck Date: Mon, 23 Feb 2015 08:26:09 -0800 > + /* Don't call phy_init_eee for now. It fails if the link is down, > + * but that should not really be a reason to fail configuration. > + */ I think there is some confusion about phy_init_eee(). You invoke it after a link has been established. Because programming the MDIO registers that turn on EEE can only be done if the link is configured in a certain way. If you look at stmmac, it invokes phy_init_eee() via it's adjust_link callback passed to phy_connect(). This is basically how I would expect it to be used, in that any time a link parameter changes we rerun phy_init_eee() to check the link partner registers, duplex state, etc. SXGBE on the other hand seems to not be using phy_init_eee() properly, it only invokes it once per device open and that makes no sense at all because then you're stuck with the eee state resulting from the link state at open time. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 23/02/15 10:52, Guenter Roeck wrote: > On Mon, Feb 23, 2015 at 09:45:01AM -0800, Florian Fainelli wrote: >> On 23/02/15 08:26, Guenter Roeck wrote: >>> EEE configuration is similar for the various MV88E6xxx chips. >>> Add generic support for it. >>> >>> Signed-off-by: Guenter Roeck >> >> Reviewed-by: Florian Fainelli >> >>> --- >>> Applies to net-next. >>> >>> The code seems to be working, at least according to ethtool, but some >>> more testing with other chip types would be useful. Also, I am not sure >>> what to do with phy_init_eee. >> >> phy_init_eee() is to be used in case you have a PHY which is not managed >> by the switch indirect or direct accesses, it looks like you are just >> fine with the current code. >> >> One possible improvement could be ironing out the EEE >> enabling/resolution by ensuring that the link partner also supports EEE? >> Not sure if there is an existing register returning that from the >> switch, or if you need to do a direct read to the PHY? >> > > EEE configuration on Marvell switches is independent from link partner > capabilities. In the hardware available to me, EEE is enabled by > default with a strapping pin on the chip. Making it dependent on link > partner capabilities would be odd because it would mean that, if the > link is down or if the link partner doesn't support it, it could be > disabled, but it could no longer be re-enabled. This is what > phy_init_eee enforces today. I dropped calling it because I thought > that this behavior would be odd and inconsistent. > > Question for me is if it makes sense to have phy_init_eee depend on > the link status or on link partner capabilities in the first place. > Personally I think it should only depend on local PHY capabilities, > and that it should be possible to configure EEE even if the link > is down or if the (current) link partner doesn't support it. Completely agree with that. BTW, the clock stop thing is also poorly handled imho because you would want the PHY library to tell you whether your PHY supports TX clock stopping, and not having to discover that by doing a first call with phy_init_eee(phydev, 1), see that it fails, retry with phy_init_eee(phydev, 0), but that's for another series of patches: https://github.com/ffainelli/linux/tree/phy-eee-tx-clk > > Consider the following: Assume both ends are configured to have EEE > disabled, even if the PHYs support it. Both ends run linux and call > phy_init_eee() to check for EEE capabilities. I have not tested it, > but I suspect that it is not currently possible to enable EEE on either > end because both ends believe that the link partner doesn't support it. > I'll test that theory once I get a system where I can control both ends. Well, it will work the first time you enable EEE and all of these conditions are met: link is negotiated, your PHY reports EEE capability, your link partner advertises EEE, and subsequent link up/down events should maintain EEE operation. I agree that this needs revisiting because you would certainly want to advertise EEE all the time if you support it and have configured it, and enable it as soon as you find a link partner that can also support EEE. -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On Mon, Feb 23, 2015 at 09:45:01AM -0800, Florian Fainelli wrote: > On 23/02/15 08:26, Guenter Roeck wrote: > > EEE configuration is similar for the various MV88E6xxx chips. > > Add generic support for it. > > > > Signed-off-by: Guenter Roeck > > Reviewed-by: Florian Fainelli > > > --- > > Applies to net-next. > > > > The code seems to be working, at least according to ethtool, but some > > more testing with other chip types would be useful. Also, I am not sure > > what to do with phy_init_eee. > > phy_init_eee() is to be used in case you have a PHY which is not managed > by the switch indirect or direct accesses, it looks like you are just > fine with the current code. > > One possible improvement could be ironing out the EEE > enabling/resolution by ensuring that the link partner also supports EEE? > Not sure if there is an existing register returning that from the > switch, or if you need to do a direct read to the PHY? > EEE configuration on Marvell switches is independent from link partner capabilities. In the hardware available to me, EEE is enabled by default with a strapping pin on the chip. Making it dependent on link partner capabilities would be odd because it would mean that, if the link is down or if the link partner doesn't support it, it could be disabled, but it could no longer be re-enabled. This is what phy_init_eee enforces today. I dropped calling it because I thought that this behavior would be odd and inconsistent. Question for me is if it makes sense to have phy_init_eee depend on the link status or on link partner capabilities in the first place. Personally I think it should only depend on local PHY capabilities, and that it should be possible to configure EEE even if the link is down or if the (current) link partner doesn't support it. Consider the following: Assume both ends are configured to have EEE disabled, even if the PHYs support it. Both ends run linux and call phy_init_eee() to check for EEE capabilities. I have not tested it, but I suspect that it is not currently possible to enable EEE on either end because both ends believe that the link partner doesn't support it. I'll test that theory once I get a system where I can control both ends. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 23/02/15 08:26, Guenter Roeck wrote:
> EEE configuration is similar for the various MV88E6xxx chips.
> Add generic support for it.
>
> Signed-off-by: Guenter Roeck
Reviewed-by: Florian Fainelli
> ---
> Applies to net-next.
>
> The code seems to be working, at least according to ethtool, but some
> more testing with other chip types would be useful. Also, I am not sure
> what to do with phy_init_eee.
phy_init_eee() is to be used in case you have a PHY which is not managed
by the switch indirect or direct accesses, it looks like you are just
fine with the current code.
One possible improvement could be ironing out the EEE
enabling/resolution by ensuring that the link partner also supports EEE?
Not sure if there is an existing register returning that from the
switch, or if you need to do a direct read to the PHY?
>
> drivers/net/dsa/mv88e6xxx.c | 55
> +
> drivers/net/dsa/mv88e6xxx.h | 3 +++
> 2 files changed, 58 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index a83ace0..2d5306a 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -649,6 +649,61 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds,
> int addr, int regnum,
> return mv88e6xxx_phy_wait(ds);
> }
>
> +int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
> +{
> + int reg;
> +
> + reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
> + if (reg < 0)
> + return -EOPNOTSUPP;
> +
> + e->eee_enabled = !!(reg & 0x0200);
> + e->tx_lpi_enabled = !!(reg & 0x0100);
> +
> + reg = REG_READ(REG_PORT(port), 0);
> + e->eee_active = !!(reg & 0x0040);
> +
> + return 0;
> +}
> +
> +static int mv88e6xxx_eee_enable_set(struct dsa_switch *ds, int port,
> + bool eee_enabled, bool tx_lpi_enabled)
> +{
> + int reg, nreg;
> +
> + /* Don't call phy_init_eee for now. It fails if the link is down,
> + * but that should not really be a reason to fail configuration.
> + */
> +
> + reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
> + if (reg < 0)
> + return reg;
> +
> + nreg = reg & ~0x0300;
> + if (eee_enabled)
> + nreg |= 0x0200;
> + if (tx_lpi_enabled)
> + nreg |= 0x0100;
> +
> + if (nreg != reg)
> + return mv88e6xxx_phy_write_indirect(ds, port, 16, nreg);
> +
> + return 0;
> +}
> +
> +int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
> + struct phy_device *phydev, struct ethtool_eee *e)
> +{
> + int ret;
> +
> + ret = mv88e6xxx_eee_enable_set(ds, port, e->eee_enabled,
> +e->tx_lpi_enabled);
> + if (ret)
> + return -EOPNOTSUPP;
> +
> + return 0;
> +}
> +
> static int __init mv88e6xxx_init(void)
> {
> #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
> diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
> index 7294227..5fd42ce 100644
> --- a/drivers/net/dsa/mv88e6xxx.h
> +++ b/drivers/net/dsa/mv88e6xxx.h
> @@ -88,6 +88,9 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
> int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
> int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
>u16 val);
> +int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee
> *e);
> +int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
> + struct phy_device *phydev, struct ethtool_eee *e);
>
> extern struct dsa_switch_driver mv88e6131_switch_driver;
> extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
>
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
EEE configuration is similar for the various MV88E6xxx chips.
Add generic support for it.
Signed-off-by: Guenter Roeck
---
Applies to net-next.
The code seems to be working, at least according to ethtool, but some
more testing with other chip types would be useful. Also, I am not sure
what to do with phy_init_eee.
drivers/net/dsa/mv88e6xxx.c | 55 +
drivers/net/dsa/mv88e6xxx.h | 3 +++
2 files changed, 58 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a83ace0..2d5306a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -649,6 +649,61 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds,
int addr, int regnum,
return mv88e6xxx_phy_wait(ds);
}
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
+{
+ int reg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg < 0)
+ return -EOPNOTSUPP;
+
+ e->eee_enabled = !!(reg & 0x0200);
+ e->tx_lpi_enabled = !!(reg & 0x0100);
+
+ reg = REG_READ(REG_PORT(port), 0);
+ e->eee_active = !!(reg & 0x0040);
+
+ return 0;
+}
+
+static int mv88e6xxx_eee_enable_set(struct dsa_switch *ds, int port,
+ bool eee_enabled, bool tx_lpi_enabled)
+{
+ int reg, nreg;
+
+ /* Don't call phy_init_eee for now. It fails if the link is down,
+* but that should not really be a reason to fail configuration.
+*/
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg < 0)
+ return reg;
+
+ nreg = reg & ~0x0300;
+ if (eee_enabled)
+ nreg |= 0x0200;
+ if (tx_lpi_enabled)
+ nreg |= 0x0100;
+
+ if (nreg != reg)
+ return mv88e6xxx_phy_write_indirect(ds, port, 16, nreg);
+
+ return 0;
+}
+
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e)
+{
+ int ret;
+
+ ret = mv88e6xxx_eee_enable_set(ds, port, e->eee_enabled,
+ e->tx_lpi_enabled);
+ if (ret)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
static int __init mv88e6xxx_init(void)
{
#if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 7294227..5fd42ce 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -88,6 +88,9 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
u16 val);
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e);
extern struct dsa_switch_driver mv88e6131_switch_driver;
extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 23/02/15 08:26, Guenter Roeck wrote:
EEE configuration is similar for the various MV88E6xxx chips.
Add generic support for it.
Signed-off-by: Guenter Roeck li...@roeck-us.net
Reviewed-by: Florian Fainelli f.faine...@gmail.com
---
Applies to net-next.
The code seems to be working, at least according to ethtool, but some
more testing with other chip types would be useful. Also, I am not sure
what to do with phy_init_eee.
phy_init_eee() is to be used in case you have a PHY which is not managed
by the switch indirect or direct accesses, it looks like you are just
fine with the current code.
One possible improvement could be ironing out the EEE
enabling/resolution by ensuring that the link partner also supports EEE?
Not sure if there is an existing register returning that from the
switch, or if you need to do a direct read to the PHY?
drivers/net/dsa/mv88e6xxx.c | 55
+
drivers/net/dsa/mv88e6xxx.h | 3 +++
2 files changed, 58 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a83ace0..2d5306a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -649,6 +649,61 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds,
int addr, int regnum,
return mv88e6xxx_phy_wait(ds);
}
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
+{
+ int reg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg 0)
+ return -EOPNOTSUPP;
+
+ e-eee_enabled = !!(reg 0x0200);
+ e-tx_lpi_enabled = !!(reg 0x0100);
+
+ reg = REG_READ(REG_PORT(port), 0);
+ e-eee_active = !!(reg 0x0040);
+
+ return 0;
+}
+
+static int mv88e6xxx_eee_enable_set(struct dsa_switch *ds, int port,
+ bool eee_enabled, bool tx_lpi_enabled)
+{
+ int reg, nreg;
+
+ /* Don't call phy_init_eee for now. It fails if the link is down,
+ * but that should not really be a reason to fail configuration.
+ */
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg 0)
+ return reg;
+
+ nreg = reg ~0x0300;
+ if (eee_enabled)
+ nreg |= 0x0200;
+ if (tx_lpi_enabled)
+ nreg |= 0x0100;
+
+ if (nreg != reg)
+ return mv88e6xxx_phy_write_indirect(ds, port, 16, nreg);
+
+ return 0;
+}
+
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e)
+{
+ int ret;
+
+ ret = mv88e6xxx_eee_enable_set(ds, port, e-eee_enabled,
+e-tx_lpi_enabled);
+ if (ret)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
static int __init mv88e6xxx_init(void)
{
#if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 7294227..5fd42ce 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -88,6 +88,9 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
u16 val);
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee
*e);
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e);
extern struct dsa_switch_driver mv88e6131_switch_driver;
extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
EEE configuration is similar for the various MV88E6xxx chips.
Add generic support for it.
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
Applies to net-next.
The code seems to be working, at least according to ethtool, but some
more testing with other chip types would be useful. Also, I am not sure
what to do with phy_init_eee.
drivers/net/dsa/mv88e6xxx.c | 55 +
drivers/net/dsa/mv88e6xxx.h | 3 +++
2 files changed, 58 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a83ace0..2d5306a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -649,6 +649,61 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds,
int addr, int regnum,
return mv88e6xxx_phy_wait(ds);
}
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
+{
+ int reg;
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg 0)
+ return -EOPNOTSUPP;
+
+ e-eee_enabled = !!(reg 0x0200);
+ e-tx_lpi_enabled = !!(reg 0x0100);
+
+ reg = REG_READ(REG_PORT(port), 0);
+ e-eee_active = !!(reg 0x0040);
+
+ return 0;
+}
+
+static int mv88e6xxx_eee_enable_set(struct dsa_switch *ds, int port,
+ bool eee_enabled, bool tx_lpi_enabled)
+{
+ int reg, nreg;
+
+ /* Don't call phy_init_eee for now. It fails if the link is down,
+* but that should not really be a reason to fail configuration.
+*/
+
+ reg = mv88e6xxx_phy_read_indirect(ds, port, 16);
+ if (reg 0)
+ return reg;
+
+ nreg = reg ~0x0300;
+ if (eee_enabled)
+ nreg |= 0x0200;
+ if (tx_lpi_enabled)
+ nreg |= 0x0100;
+
+ if (nreg != reg)
+ return mv88e6xxx_phy_write_indirect(ds, port, 16, nreg);
+
+ return 0;
+}
+
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e)
+{
+ int ret;
+
+ ret = mv88e6xxx_eee_enable_set(ds, port, e-eee_enabled,
+ e-tx_lpi_enabled);
+ if (ret)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
static int __init mv88e6xxx_init(void)
{
#if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 7294227..5fd42ce 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -88,6 +88,9 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds);
int mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum);
int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
u16 val);
+int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
+int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
+ struct phy_device *phydev, struct ethtool_eee *e);
extern struct dsa_switch_driver mv88e6131_switch_driver;
extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 23/02/15 10:52, Guenter Roeck wrote: On Mon, Feb 23, 2015 at 09:45:01AM -0800, Florian Fainelli wrote: On 23/02/15 08:26, Guenter Roeck wrote: EEE configuration is similar for the various MV88E6xxx chips. Add generic support for it. Signed-off-by: Guenter Roeck li...@roeck-us.net Reviewed-by: Florian Fainelli f.faine...@gmail.com --- Applies to net-next. The code seems to be working, at least according to ethtool, but some more testing with other chip types would be useful. Also, I am not sure what to do with phy_init_eee. phy_init_eee() is to be used in case you have a PHY which is not managed by the switch indirect or direct accesses, it looks like you are just fine with the current code. One possible improvement could be ironing out the EEE enabling/resolution by ensuring that the link partner also supports EEE? Not sure if there is an existing register returning that from the switch, or if you need to do a direct read to the PHY? EEE configuration on Marvell switches is independent from link partner capabilities. In the hardware available to me, EEE is enabled by default with a strapping pin on the chip. Making it dependent on link partner capabilities would be odd because it would mean that, if the link is down or if the link partner doesn't support it, it could be disabled, but it could no longer be re-enabled. This is what phy_init_eee enforces today. I dropped calling it because I thought that this behavior would be odd and inconsistent. Question for me is if it makes sense to have phy_init_eee depend on the link status or on link partner capabilities in the first place. Personally I think it should only depend on local PHY capabilities, and that it should be possible to configure EEE even if the link is down or if the (current) link partner doesn't support it. Completely agree with that. BTW, the clock stop thing is also poorly handled imho because you would want the PHY library to tell you whether your PHY supports TX clock stopping, and not having to discover that by doing a first call with phy_init_eee(phydev, 1), see that it fails, retry with phy_init_eee(phydev, 0), but that's for another series of patches: https://github.com/ffainelli/linux/tree/phy-eee-tx-clk Consider the following: Assume both ends are configured to have EEE disabled, even if the PHYs support it. Both ends run linux and call phy_init_eee() to check for EEE capabilities. I have not tested it, but I suspect that it is not currently possible to enable EEE on either end because both ends believe that the link partner doesn't support it. I'll test that theory once I get a system where I can control both ends. Well, it will work the first time you enable EEE and all of these conditions are met: link is negotiated, your PHY reports EEE capability, your link partner advertises EEE, and subsequent link up/down events should maintain EEE operation. I agree that this needs revisiting because you would certainly want to advertise EEE all the time if you support it and have configured it, and enable it as soon as you find a link partner that can also support EEE. -- Florian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On Mon, Feb 23, 2015 at 09:45:01AM -0800, Florian Fainelli wrote: On 23/02/15 08:26, Guenter Roeck wrote: EEE configuration is similar for the various MV88E6xxx chips. Add generic support for it. Signed-off-by: Guenter Roeck li...@roeck-us.net Reviewed-by: Florian Fainelli f.faine...@gmail.com --- Applies to net-next. The code seems to be working, at least according to ethtool, but some more testing with other chip types would be useful. Also, I am not sure what to do with phy_init_eee. phy_init_eee() is to be used in case you have a PHY which is not managed by the switch indirect or direct accesses, it looks like you are just fine with the current code. One possible improvement could be ironing out the EEE enabling/resolution by ensuring that the link partner also supports EEE? Not sure if there is an existing register returning that from the switch, or if you need to do a direct read to the PHY? EEE configuration on Marvell switches is independent from link partner capabilities. In the hardware available to me, EEE is enabled by default with a strapping pin on the chip. Making it dependent on link partner capabilities would be odd because it would mean that, if the link is down or if the link partner doesn't support it, it could be disabled, but it could no longer be re-enabled. This is what phy_init_eee enforces today. I dropped calling it because I thought that this behavior would be odd and inconsistent. Question for me is if it makes sense to have phy_init_eee depend on the link status or on link partner capabilities in the first place. Personally I think it should only depend on local PHY capabilities, and that it should be possible to configure EEE even if the link is down or if the (current) link partner doesn't support it. Consider the following: Assume both ends are configured to have EEE disabled, even if the PHYs support it. Both ends run linux and call phy_init_eee() to check for EEE capabilities. I have not tested it, but I suspect that it is not currently possible to enable EEE on either end because both ends believe that the link partner doesn't support it. I'll test that theory once I get a system where I can control both ends. Thanks, Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
From: Guenter Roeck li...@roeck-us.net Date: Mon, 23 Feb 2015 08:26:09 -0800 + /* Don't call phy_init_eee for now. It fails if the link is down, + * but that should not really be a reason to fail configuration. + */ I think there is some confusion about phy_init_eee(). You invoke it after a link has been established. Because programming the MDIO registers that turn on EEE can only be done if the link is configured in a certain way. If you look at stmmac, it invokes phy_init_eee() via it's adjust_link callback passed to phy_connect(). This is basically how I would expect it to be used, in that any time a link parameter changes we rerun phy_init_eee() to check the link partner registers, duplex state, etc. SXGBE on the other hand seems to not be using phy_init_eee() properly, it only invokes it once per device open and that makes no sense at all because then you're stuck with the eee state resulting from the link state at open time. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 23/02/15 18:29, Guenter Roeck wrote: On 02/23/2015 02:19 PM, David Miller wrote: From: Guenter Roeck li...@roeck-us.net Date: Mon, 23 Feb 2015 08:26:09 -0800 +/* Don't call phy_init_eee for now. It fails if the link is down, + * but that should not really be a reason to fail configuration. + */ I think there is some confusion about phy_init_eee(). You invoke it after a link has been established. Because programming the MDIO registers that turn on EEE can only be done if the link is configured in a certain way. If you look at stmmac, it invokes phy_init_eee() via it's adjust_link callback passed to phy_connect(). This is basically how I would expect it to be used, in that any time a link parameter changes we rerun phy_init_eee() to check the link partner registers, duplex state, etc. SXGBE on the other hand seems to not be using phy_init_eee() properly, it only invokes it once per device open and that makes no sense at all because then you're stuck with the eee state resulting from the link state at open time. That explains a lot. I suspect the use in bcm_sf2 and bcmgenet is also not as intended. Yes, it certainly needs fixing. -- Florian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT PATCH 1/2] net: dsa: mv88e6xxx: Add EEE support
On 02/23/2015 02:19 PM, David Miller wrote: From: Guenter Roeck li...@roeck-us.net Date: Mon, 23 Feb 2015 08:26:09 -0800 + /* Don't call phy_init_eee for now. It fails if the link is down, +* but that should not really be a reason to fail configuration. +*/ I think there is some confusion about phy_init_eee(). You invoke it after a link has been established. Because programming the MDIO registers that turn on EEE can only be done if the link is configured in a certain way. If you look at stmmac, it invokes phy_init_eee() via it's adjust_link callback passed to phy_connect(). This is basically how I would expect it to be used, in that any time a link parameter changes we rerun phy_init_eee() to check the link partner registers, duplex state, etc. SXGBE on the other hand seems to not be using phy_init_eee() properly, it only invokes it once per device open and that makes no sense at all because then you're stuck with the eee state resulting from the link state at open time. That explains a lot. I suspect the use in bcm_sf2 and bcmgenet is also not as intended. Thanks, Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
