* [PATCH net-next 0/3] marvell10g tunable and power saving support
@ 2020-03-03 14:42 Russell King - ARM Linux admin
2020-03-03 14:43 ` [PATCH net-next 1/3] net: phy: marvell10g: add mdix control Russell King
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-03 14:42 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Antoine Tenart
Cc: David S. Miller, netdev
Hi,
This patch series adds support for:
- mdix configuration (auto, mdi, mdix)
- energy detect power down (edpd)
- placing in edpd mode at probe
for both the 88x3310 and 88x2110 PHYs.
Antione, could you test this for the 88x2110 PHY please?
drivers/net/phy/marvell10g.c | 181 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 173 insertions(+), 8 deletions(-)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 1/3] net: phy: marvell10g: add mdix control
2020-03-03 14:42 [PATCH net-next 0/3] marvell10g tunable and power saving support Russell King - ARM Linux admin
@ 2020-03-03 14:43 ` Russell King
2020-03-03 15:09 ` Antoine Tenart
2020-03-03 14:44 ` [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable Russell King
2020-03-03 14:44 ` [PATCH net-next 3/3] net: phy: marvell10g: place in powersave mode at probe Russell King
2 siblings, 1 reply; 11+ messages in thread
From: Russell King @ 2020-03-03 14:43 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Antoine Tenart
Cc: David S. Miller, netdev
Add support for controlling the MDI-X state of the PHY.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell10g.c | 40 ++++++++++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 9a4e12a2af07..20b24b1971a3 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -39,6 +39,12 @@ enum {
MV_PCS_BASE_R = 0x1000,
MV_PCS_1000BASEX = 0x2000,
+ MV_PCS_CSCR1 = 0x8000,
+ MV_PCS_CSCR1_MDIX_MASK = 0x0060,
+ MV_PCS_CSCR1_MDIX_MDI = 0x0000,
+ MV_PCS_CSCR1_MDIX_MDIX = 0x0020,
+ MV_PCS_CSCR1_MDIX_AUTO = 0x0060,
+
MV_PCS_CSSR1 = 0x8008,
MV_PCS_CSSR1_SPD1_MASK = 0xc000,
MV_PCS_CSSR1_SPD1_SPD2 = 0xc000,
@@ -316,6 +322,8 @@ static int mv3310_config_init(struct phy_device *phydev)
phydev->interface != PHY_INTERFACE_MODE_10GBASER)
return -ENODEV;
+ phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+
return 0;
}
@@ -345,14 +353,42 @@ static int mv3310_get_features(struct phy_device *phydev)
return 0;
}
+static int mv3310_config_mdix(struct phy_device *phydev)
+{
+ u16 val;
+ int err;
+
+ switch (phydev->mdix_ctrl) {
+ case ETH_TP_MDI_AUTO:
+ val = MV_PCS_CSCR1_MDIX_AUTO;
+ break;
+ case ETH_TP_MDI_X:
+ val = MV_PCS_CSCR1_MDIX_MDIX;
+ break;
+ case ETH_TP_MDI:
+ val = MV_PCS_CSCR1_MDIX_MDI;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
+ MV_PCS_CSCR1_MDIX_MASK, val);
+ if (err < 0)
+ return err;
+
+ return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+}
+
static int mv3310_config_aneg(struct phy_device *phydev)
{
bool changed = false;
u16 reg;
int ret;
- /* We don't support manual MDI control */
- phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+ ret = mv3310_config_mdix(phydev);
+ if (ret < 0)
+ return ret;
if (phydev->autoneg == AUTONEG_DISABLE)
return genphy_c45_pma_setup_forced(phydev);
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable
2020-03-03 14:42 [PATCH net-next 0/3] marvell10g tunable and power saving support Russell King - ARM Linux admin
2020-03-03 14:43 ` [PATCH net-next 1/3] net: phy: marvell10g: add mdix control Russell King
@ 2020-03-03 14:44 ` Russell King
2020-03-03 15:07 ` Antoine Tenart
2020-03-03 14:44 ` [PATCH net-next 3/3] net: phy: marvell10g: place in powersave mode at probe Russell King
2 siblings, 1 reply; 11+ messages in thread
From: Russell King @ 2020-03-03 14:44 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Antoine Tenart
Cc: David S. Miller, netdev
Add support for the energy detect power down tunable, which saves
around 600mW when the link is down. The 88x3310 supports off, rx-only
and NLP every second. Enable EDPD by default for 88x3310.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
1 file changed, 109 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 20b24b1971a3..e1116d091d84 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -23,6 +23,7 @@
* link takes priority and the other port is completely locked out.
*/
#include <linux/ctype.h>
+#include <linux/delay.h>
#include <linux/hwmon.h>
#include <linux/marvell_phy.h>
#include <linux/phy.h>
@@ -40,6 +41,10 @@ enum {
MV_PCS_1000BASEX = 0x2000,
MV_PCS_CSCR1 = 0x8000,
+ MV_PCS_CSCR1_ED_MASK = 0x0300,
+ MV_PCS_CSCR1_ED_OFF = 0x0000,
+ MV_PCS_CSCR1_ED_RX = 0x0200,
+ MV_PCS_CSCR1_ED_NLP = 0x0300,
MV_PCS_CSCR1_MDIX_MASK = 0x0060,
MV_PCS_CSCR1_MDIX_MDI = 0x0000,
MV_PCS_CSCR1_MDIX_MDIX = 0x0020,
@@ -222,6 +227,82 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
}
#endif
+static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
+{
+ int retries, val, err;
+
+ if (!reset)
+ return 0;
+
+ err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
+ MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
+ if (err < 0)
+ return err;
+
+ retries = 20;
+ do {
+ msleep(5);
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1);
+ if (val < 0)
+ return val;
+ } while (val & MDIO_CTRL1_RESET && --retries);
+
+ return val & MDIO_CTRL1_RESET ? -ETIMEDOUT : 0;
+}
+
+static int mv3310_get_edpd(struct phy_device *phydev, u16 *edpd)
+{
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1);
+ if (val < 0)
+ return val;
+
+ switch (val & MV_PCS_CSCR1_ED_MASK) {
+ case MV_PCS_CSCR1_ED_NLP:
+ *edpd = 1000;
+ break;
+ case MV_PCS_CSCR1_ED_RX:
+ *edpd = ETHTOOL_PHY_EDPD_NO_TX;
+ break;
+ default:
+ *edpd = ETHTOOL_PHY_EDPD_DISABLE;
+ break;
+ }
+ return 0;
+}
+
+static int mv3310_set_edpd(struct phy_device *phydev, u16 edpd)
+{
+ u16 val;
+ int err;
+
+ switch (edpd) {
+ case 1000:
+ case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
+ val = MV_PCS_CSCR1_ED_NLP;
+ break;
+
+ case ETHTOOL_PHY_EDPD_NO_TX:
+ val = MV_PCS_CSCR1_ED_RX;
+ break;
+
+ case ETHTOOL_PHY_EDPD_DISABLE:
+ val = MV_PCS_CSCR1_ED_OFF;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
+ MV_PCS_CSCR1_ED_MASK, val);
+ if (err < 0)
+ return err;
+
+ return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+}
+
static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
{
struct phy_device *phydev = upstream;
@@ -324,7 +405,8 @@ static int mv3310_config_init(struct phy_device *phydev)
phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
- return 0;
+ /* Enable EDPD mode - saving 600mW */
+ return mv3310_set_edpd(phydev, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS);
}
static int mv3310_get_features(struct phy_device *phydev)
@@ -573,6 +655,28 @@ static int mv3310_read_status(struct phy_device *phydev)
return 0;
}
+static int mv3310_get_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna, void *data)
+{
+ switch (tuna->id) {
+ case ETHTOOL_PHY_EDPD:
+ return mv3310_get_edpd(phydev, data);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mv3310_set_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna, const void *data)
+{
+ switch (tuna->id) {
+ case ETHTOOL_PHY_EDPD:
+ return mv3310_set_edpd(phydev, *(u16 *)data);
+ default:
+ return -EINVAL;
+ }
+}
+
static struct phy_driver mv3310_drivers[] = {
{
.phy_id = MARVELL_PHY_ID_88X3310,
@@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
.name = "mv88x3310",
.get_features = mv3310_get_features,
.soft_reset = genphy_no_soft_reset,
- .config_init = mv3310_config_init,
.probe = mv3310_probe,
.suspend = mv3310_suspend,
.resume = mv3310_resume,
.config_aneg = mv3310_config_aneg,
.aneg_done = mv3310_aneg_done,
.read_status = mv3310_read_status,
+ .get_tunable = mv3310_get_tunable,
+ .set_tunable = mv3310_set_tunable,
},
{
.phy_id = MARVELL_PHY_ID_88E2110,
@@ -600,6 +705,8 @@ static struct phy_driver mv3310_drivers[] = {
.config_aneg = mv3310_config_aneg,
.aneg_done = mv3310_aneg_done,
.read_status = mv3310_read_status,
+ .get_tunable = mv3310_get_tunable,
+ .set_tunable = mv3310_set_tunable,
},
};
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 3/3] net: phy: marvell10g: place in powersave mode at probe
2020-03-03 14:42 [PATCH net-next 0/3] marvell10g tunable and power saving support Russell King - ARM Linux admin
2020-03-03 14:43 ` [PATCH net-next 1/3] net: phy: marvell10g: add mdix control Russell King
2020-03-03 14:44 ` [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable Russell King
@ 2020-03-03 14:44 ` Russell King
2 siblings, 0 replies; 11+ messages in thread
From: Russell King @ 2020-03-03 14:44 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Antoine Tenart
Cc: David S. Miller, netdev
Place the 88x3310 into powersaving mode when probing, which saves 600mW
per PHY. For both PHYs on the Macchiatobin double-shot, this saves
about 10% of the board idle power.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell10g.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index e1116d091d84..ec699fb6f2ea 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -227,6 +227,18 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
}
#endif
+static int mv3310_power_down(struct phy_device *phydev)
+{
+ return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+ MV_V2_PORT_CTRL_PWRDOWN);
+}
+
+static int mv3310_power_up(struct phy_device *phydev)
+{
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+ MV_V2_PORT_CTRL_PWRDOWN);
+}
+
static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
{
int retries, val, err;
@@ -351,6 +363,11 @@ static int mv3310_probe(struct phy_device *phydev)
dev_set_drvdata(&phydev->mdio.dev, priv);
+ /* Powering down the port when not in use saves about 600mW */
+ ret = mv3310_power_down(phydev);
+ if (ret)
+ return ret;
+
ret = mv3310_hwmon_probe(phydev);
if (ret)
return ret;
@@ -360,16 +377,14 @@ static int mv3310_probe(struct phy_device *phydev)
static int mv3310_suspend(struct phy_device *phydev)
{
- return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
- MV_V2_PORT_CTRL_PWRDOWN);
+ return mv3310_power_down(phydev);
}
static int mv3310_resume(struct phy_device *phydev)
{
int ret;
- ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
- MV_V2_PORT_CTRL_PWRDOWN);
+ ret = mv3310_power_up(phydev);
if (ret)
return ret;
@@ -395,6 +410,8 @@ static bool mv3310_has_pma_ngbaset_quirk(struct phy_device *phydev)
static int mv3310_config_init(struct phy_device *phydev)
{
+ int err;
+
/* Check that the PHY interface type is compatible */
if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
@@ -405,6 +422,11 @@ static int mv3310_config_init(struct phy_device *phydev)
phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+ /* Power up so reset works */
+ err = mv3310_power_up(phydev);
+ if (err)
+ return err;
+
/* Enable EDPD mode - saving 600mW */
return mv3310_set_edpd(phydev, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable
2020-03-03 14:44 ` [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable Russell King
@ 2020-03-03 15:07 ` Antoine Tenart
2020-03-03 15:12 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 11+ messages in thread
From: Antoine Tenart @ 2020-03-03 15:07 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Antoine Tenart,
David S. Miller, netdev
Hi Russell,
On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
>
> +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> +{
> + int retries, val, err;
> +
> + if (!reset)
> + return 0;
You could also call mv3310_maybe_reset after testing the 'reset'
condition, that would make it easier to read the code.
> static struct phy_driver mv3310_drivers[] = {
> {
> .phy_id = MARVELL_PHY_ID_88X3310,
> @@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
> .name = "mv88x3310",
> .get_features = mv3310_get_features,
> .soft_reset = genphy_no_soft_reset,
> - .config_init = mv3310_config_init,
Having a quick look at the code, it seems this is a leftover and you
don't actually want to remove config_init for the 3310.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: marvell10g: add mdix control
2020-03-03 14:43 ` [PATCH net-next 1/3] net: phy: marvell10g: add mdix control Russell King
@ 2020-03-03 15:09 ` Antoine Tenart
2020-03-03 15:20 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 11+ messages in thread
From: Antoine Tenart @ 2020-03-03 15:09 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Antoine Tenart,
David S. Miller, netdev
Hello Russell,
On Tue, Mar 03, 2020 at 02:43:57PM +0000, Russell King wrote:
>
> +static int mv3310_config_mdix(struct phy_device *phydev)
> +{
> + u16 val;
> + int err;
> +
> + switch (phydev->mdix_ctrl) {
> + case ETH_TP_MDI_AUTO:
> + val = MV_PCS_CSCR1_MDIX_AUTO;
> + break;
> + case ETH_TP_MDI_X:
> + val = MV_PCS_CSCR1_MDIX_MDIX;
> + break;
> + case ETH_TP_MDI:
> + val = MV_PCS_CSCR1_MDIX_MDI;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> + MV_PCS_CSCR1_MDIX_MASK, val);
> + if (err < 0)
> + return err;
> +
> + return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
This helper is only introduced in patch 2.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable
2020-03-03 15:07 ` Antoine Tenart
@ 2020-03-03 15:12 ` Russell King - ARM Linux admin
2020-03-03 15:19 ` Antoine Tenart
0 siblings, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-03 15:12 UTC (permalink / raw)
To: Antoine Tenart
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> Hi Russell,
>
> On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> >
> > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > +{
> > + int retries, val, err;
> > +
> > + if (!reset)
> > + return 0;
>
> You could also call mv3310_maybe_reset after testing the 'reset'
> condition, that would make it easier to read the code.
I'm not too convinced:
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ef1ed9415d9f..3daf73e61dff 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
MV_V2_PORT_CTRL_PWRDOWN);
}
-static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
+static int mv3310_reset(struct phy_device *phydev, u32 unit)
{
int retries, val, err;
- if (!reset)
- return 0;
-
err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
if (err < 0)
@@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
MV_PCS_CSCR1_MDIX_MASK, val);
- if (err < 0)
+ if (err <= 0)
return err;
- return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+ return mv3310_reset(phydev, MV_PCS_BASE_T);
}
static int mv3310_config_aneg(struct phy_device *phydev)
The change from:
if (err < 0)
to:
if (err <= 0)
could easily be mistaken as a bug, and someone may decide to try to
"fix" that back to being the former instead. The way I have the code
makes the intention explicit.
>
> > static struct phy_driver mv3310_drivers[] = {
> > {
> > .phy_id = MARVELL_PHY_ID_88X3310,
> > @@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
> > .name = "mv88x3310",
> > .get_features = mv3310_get_features,
> > .soft_reset = genphy_no_soft_reset,
> > - .config_init = mv3310_config_init,
>
> Having a quick look at the code, it seems this is a leftover and you
> don't actually want to remove config_init for the 3310.
Hmm, I wonder how that crept in... it shouldn't be there!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable
2020-03-03 15:12 ` Russell King - ARM Linux admin
@ 2020-03-03 15:19 ` Antoine Tenart
2020-03-03 15:30 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 11+ messages in thread
From: Antoine Tenart @ 2020-03-03 15:19 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Antoine Tenart, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
David S. Miller, netdev
On Tue, Mar 03, 2020 at 03:12:32PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> > On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > > drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> > >
> > > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > +{
> > > + int retries, val, err;
> > > +
> > > + if (!reset)
> > > + return 0;
> >
> > You could also call mv3310_maybe_reset after testing the 'reset'
> > condition, that would make it easier to read the code.
>
> I'm not too convinced:
>
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index ef1ed9415d9f..3daf73e61dff 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
> MV_V2_PORT_CTRL_PWRDOWN);
> }
>
> -static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> +static int mv3310_reset(struct phy_device *phydev, u32 unit)
> {
> int retries, val, err;
>
> - if (!reset)
> - return 0;
> -
> err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
> MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
> if (err < 0)
> @@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
>
> err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> MV_PCS_CSCR1_MDIX_MASK, val);
> - if (err < 0)
> + if (err <= 0)
> return err;
>
> - return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> + return mv3310_reset(phydev, MV_PCS_BASE_T);
> }
>
> static int mv3310_config_aneg(struct phy_device *phydev)
>
> The change from:
>
> if (err < 0)
>
> to:
>
> if (err <= 0)
>
> could easily be mistaken as a bug, and someone may decide to try to
> "fix" that back to being the former instead. The way I have the code
> makes the intention explicit.
Using a single line to test both the error and the 'return 0'
conditions, yes, I agree. Another solution would be to do something of
the like:
phy_modify_mmd_changed()
if (err < 0)
return err;
if (err)
mv3310_reset();
return 0;
I find it more readable, but this kind of thing is also a matter of
personal taste.
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/3] net: phy: marvell10g: add mdix control
2020-03-03 15:09 ` Antoine Tenart
@ 2020-03-03 15:20 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-03 15:20 UTC (permalink / raw)
To: Antoine Tenart
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Tue, Mar 03, 2020 at 04:09:58PM +0100, Antoine Tenart wrote:
> Hello Russell,
>
> On Tue, Mar 03, 2020 at 02:43:57PM +0000, Russell King wrote:
> >
> > +static int mv3310_config_mdix(struct phy_device *phydev)
> > +{
> > + u16 val;
> > + int err;
> > +
> > + switch (phydev->mdix_ctrl) {
> > + case ETH_TP_MDI_AUTO:
> > + val = MV_PCS_CSCR1_MDIX_AUTO;
> > + break;
> > + case ETH_TP_MDI_X:
> > + val = MV_PCS_CSCR1_MDIX_MDIX;
> > + break;
> > + case ETH_TP_MDI:
> > + val = MV_PCS_CSCR1_MDIX_MDI;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> > + MV_PCS_CSCR1_MDIX_MASK, val);
> > + if (err < 0)
> > + return err;
> > +
> > + return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
>
> This helper is only introduced in patch 2.
Hmm, must have happened when I reordered the patches, and I hadn't
noticed. Thanks, v2 coming soon.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable
2020-03-03 15:19 ` Antoine Tenart
@ 2020-03-03 15:30 ` Russell King - ARM Linux admin
2020-03-03 15:33 ` Antoine Tenart
0 siblings, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-03 15:30 UTC (permalink / raw)
To: Antoine Tenart
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Tue, Mar 03, 2020 at 04:19:58PM +0100, Antoine Tenart wrote:
> On Tue, Mar 03, 2020 at 03:12:32PM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> > > On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > > > drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> > > >
> > > > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > > +{
> > > > + int retries, val, err;
> > > > +
> > > > + if (!reset)
> > > > + return 0;
> > >
> > > You could also call mv3310_maybe_reset after testing the 'reset'
> > > condition, that would make it easier to read the code.
> >
> > I'm not too convinced:
> >
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index ef1ed9415d9f..3daf73e61dff 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
> > MV_V2_PORT_CTRL_PWRDOWN);
> > }
> >
> > -static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > +static int mv3310_reset(struct phy_device *phydev, u32 unit)
> > {
> > int retries, val, err;
> >
> > - if (!reset)
> > - return 0;
> > -
> > err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
> > MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
> > if (err < 0)
> > @@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
> >
> > err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> > MV_PCS_CSCR1_MDIX_MASK, val);
> > - if (err < 0)
> > + if (err <= 0)
> > return err;
> >
> > - return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> > + return mv3310_reset(phydev, MV_PCS_BASE_T);
> > }
> >
> > static int mv3310_config_aneg(struct phy_device *phydev)
> >
> > The change from:
> >
> > if (err < 0)
> >
> > to:
> >
> > if (err <= 0)
> >
> > could easily be mistaken as a bug, and someone may decide to try to
> > "fix" that back to being the former instead. The way I have the code
> > makes the intention explicit.
>
> Using a single line to test both the error and the 'return 0'
> conditions, yes, I agree. Another solution would be to do something of
> the like:
>
> phy_modify_mmd_changed()
> if (err < 0)
> return err;
>
> if (err)
> mv3310_reset();
>
> return 0;
>
> I find it more readable, but this kind of thing is also a matter of
> personal taste.
Well, it either becomes:
err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
MV_PCS_CSCR1_MDIX_MASK, val);
if (err < 0)
return err;
if (err > 0)
return mv3310_reset(phydev, MV_PCS_BASE_T);
return 0;
or:
err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
MV_PCS_CSCR1_MDIX_MASK, val);
if (err > 0)
err = mv3310_reset(phydev, MV_PCS_BASE_T);
return err;
In the former case, we have two success-exit paths - one via a successful
mv3310_reset() and one by dropping through to the final return statement.
The latter case looks a bit better, at least to me.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable
2020-03-03 15:30 ` Russell King - ARM Linux admin
@ 2020-03-03 15:33 ` Antoine Tenart
0 siblings, 0 replies; 11+ messages in thread
From: Antoine Tenart @ 2020-03-03 15:33 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Antoine Tenart, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
David S. Miller, netdev
On Tue, Mar 03, 2020 at 03:30:13PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Mar 03, 2020 at 04:19:58PM +0100, Antoine Tenart wrote:
> > On Tue, Mar 03, 2020 at 03:12:32PM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> > > > On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > > > > drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> > > > >
> > > > > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > > > +{
> > > > > + int retries, val, err;
> > > > > +
> > > > > + if (!reset)
> > > > > + return 0;
> > > >
> > > > You could also call mv3310_maybe_reset after testing the 'reset'
> > > > condition, that would make it easier to read the code.
> > >
> > > I'm not too convinced:
> > >
> > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > > index ef1ed9415d9f..3daf73e61dff 100644
> > > --- a/drivers/net/phy/marvell10g.c
> > > +++ b/drivers/net/phy/marvell10g.c
> > > @@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
> > > MV_V2_PORT_CTRL_PWRDOWN);
> > > }
> > >
> > > -static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > +static int mv3310_reset(struct phy_device *phydev, u32 unit)
> > > {
> > > int retries, val, err;
> > >
> > > - if (!reset)
> > > - return 0;
> > > -
> > > err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
> > > MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
> > > if (err < 0)
> > > @@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
> > >
> > > err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> > > MV_PCS_CSCR1_MDIX_MASK, val);
> > > - if (err < 0)
> > > + if (err <= 0)
> > > return err;
> > >
> > > - return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> > > + return mv3310_reset(phydev, MV_PCS_BASE_T);
> > > }
> > >
> > > static int mv3310_config_aneg(struct phy_device *phydev)
> > >
> > > The change from:
> > >
> > > if (err < 0)
> > >
> > > to:
> > >
> > > if (err <= 0)
> > >
> > > could easily be mistaken as a bug, and someone may decide to try to
> > > "fix" that back to being the former instead. The way I have the code
> > > makes the intention explicit.
> >
> > Using a single line to test both the error and the 'return 0'
> > conditions, yes, I agree. Another solution would be to do something of
> > the like:
> >
> > phy_modify_mmd_changed()
> > if (err < 0)
> > return err;
> >
> > if (err)
> > mv3310_reset();
> >
> > return 0;
> >
> > I find it more readable, but this kind of thing is also a matter of
> > personal taste.
>
> Well, it either becomes:
>
> err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> MV_PCS_CSCR1_MDIX_MASK, val);
> if (err < 0)
> return err;
>
> if (err > 0)
> return mv3310_reset(phydev, MV_PCS_BASE_T);
>
> return 0;
>
> or:
>
> err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> MV_PCS_CSCR1_MDIX_MASK, val);
> if (err > 0)
> err = mv3310_reset(phydev, MV_PCS_BASE_T);
>
> return err;
>
> In the former case, we have two success-exit paths - one via a successful
> mv3310_reset() and one by dropping through to the final return statement.
>
> The latter case looks a bit better, at least to me.
I do agree, the latter looks good.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-03-03 15:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 14:42 [PATCH net-next 0/3] marvell10g tunable and power saving support Russell King - ARM Linux admin
2020-03-03 14:43 ` [PATCH net-next 1/3] net: phy: marvell10g: add mdix control Russell King
2020-03-03 15:09 ` Antoine Tenart
2020-03-03 15:20 ` Russell King - ARM Linux admin
2020-03-03 14:44 ` [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable Russell King
2020-03-03 15:07 ` Antoine Tenart
2020-03-03 15:12 ` Russell King - ARM Linux admin
2020-03-03 15:19 ` Antoine Tenart
2020-03-03 15:30 ` Russell King - ARM Linux admin
2020-03-03 15:33 ` Antoine Tenart
2020-03-03 14:44 ` [PATCH net-next 3/3] net: phy: marvell10g: place in powersave mode at probe Russell King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).