| Message ID | 20210629105554.1443676-1-pei.lee.ling@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,V2] net: phy: marvell10g: enable WoL for mv2110 | expand |
On Tue, Jun 29, 2021 at 06:55:54PM +0800, Ling Pei Lee wrote: > From: Voon Weifeng <weifeng.voon@intel.com> > > Basically it is just to enable to WoL interrupt and enable WoL detection. > Then, configure the MAC address into address detection register. > > Change Log: > V2: > (1) Reviewer Marek request to rorganize code to readable way. > (2) Reviewer Rusell request to put phy_clear_bits_mmd() outside of if(){}else{} > and modify return ret to return phy_clear_bits_mmd(). > (3) Reviewer Rusell request to add return on phy_read_mmd() in set_wol(). > (4) Reorganize register layout to be put before MV_V2_TEMP_CTRL. I actually said: "Please put these new register definitions in address order. This list is first sorted by MMD and then by address. So these should be before the definition of MV_V2_TEMP_CTRL." Which you have partially done. > @@ -99,6 +100,17 @@ enum { > MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN = 0x5, > MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH = 0x6, > MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII = 0x7, > + MV_V2_MAGIC_PKT_WORD0 = 0xf06b, > + MV_V2_MAGIC_PKT_WORD1 = 0xf06c, > + MV_V2_MAGIC_PKT_WORD2 = 0xf06d, > + /* Wake on LAN registers */ > + MV_V2_WOL_CTRL = 0xf06e, > + MV_V2_WOL_STS = 0xf06f, > + MV_V2_WOL_CLEAR_STS = BIT(15), > + MV_V2_WOL_MAGIC_PKT_EN = BIT(0), > + MV_V2_PORT_INTR_STS = 0xf040, > + MV_V2_PORT_INTR_MASK = 0xf043, > + MV_V2_WOL_INTR_EN = BIT(8), Clearly MV_V2_PORT_INTR_STS is at a lower address than MV_V2_MAGIC_PKT_WORD0, so the list is not sorted as it originally was. Apart from that, the rest of the patch looks good, thanks!
Hi Ling, some more things, please look at these. First, the subject line should be net: phy: marvell10g: enable WoL for 88X3310 and 88E2110 since you are also implementing it for 3310. > From: Voon Weifeng <weifeng.voon@intel.com> > > Basically it is just to enable to WoL interrupt and enable WoL > detection. Then, configure the MAC address into address detection > register. "Implement Wake-on-LAN feature for 88X3310 and 88E2110. This is done by enabling WoL interrupt and WoL detection and configuring MAC address into WoL magic packet registers." > Change Log: > V2: > (1) Reviewer Marek request to rorganize code to readable way. > (2) Reviewer Rusell request to put phy_clear_bits_mmd() outside of > if(){}else{} and modify return ret to return phy_clear_bits_mmd(). > (3) Reviewer Rusell request to add return on phy_read_mmd() in > set_wol(). (4) Reorganize register layout to be put before > MV_V2_TEMP_CTRL. (5) Add the .{get|set}_wol for 88E3110 too as per > feedback from Russell. We do not put changelogs into the commit message normally. Mostly we put it into cover letters or after the "--" line so that it is not included in the commit message in git history (merge commits are one of the exceptions). As Russell wrote, you have only partially reorganized register constants. The constants should be defined in this order + MV_V2_PORT_INTR_STS = 0xf040, + MV_V2_PORT_INTR_MASK = 0xf043, + MV_V2_WOL_INTR_EN = BIT(8), + MV_V2_MAGIC_PKT_WORD0 = 0xf06b, + MV_V2_MAGIC_PKT_WORD1 = 0xf06c, + MV_V2_MAGIC_PKT_WORD2 = 0xf06d, + /* Wake on LAN registers */ + MV_V2_WOL_CTRL = 0xf06e, + MV_V2_WOL_CLEAR_STS = BIT(15), + MV_V2_WOL_MAGIC_PKT_EN = BIT(0), + MV_V2_WOL_STS = 0xf06f, Also: - MV_V2_WOL_STS is not used, you should read the register before cleaning or don't define the constant at all. (IMO you should read it). - register value constants should be prefixed with whole register names, so: MV_V2_WOL_INTR_EN rename to MV_V2_PORT_INTR_STS_WOL_EN MV_V2_WOL_CLEAR_STS rename to MV_V2_WOL_CTRL_CLEAR_STS MV_V2_WOL_MAGIC_PKT_EN rename to MV_V2_WOL_CTRL_MAGIC_PKT_EN + /* Reset the clear WOL status bit as it does not self-clear */ + return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_WOL_CTRL, + MV_V2_WOL_CLEAR_STS); This has wrong indentation, the arguments in new lines should start at the position of first argument. Marek
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index bbbc6ac8fa82..5bddf4682127 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -28,6 +28,7 @@ #include <linux/marvell_phy.h> #include <linux/phy.h> #include <linux/sfp.h> +#include <linux/netdevice.h> #define MV_PHY_ALASKA_NBT_QUIRK_MASK 0xfffffffe #define MV_PHY_ALASKA_NBT_QUIRK_REV (MARVELL_PHY_ID_88X3310 | 0xa) @@ -99,6 +100,17 @@ enum { MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN = 0x5, MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH = 0x6, MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII = 0x7, + MV_V2_MAGIC_PKT_WORD0 = 0xf06b, + MV_V2_MAGIC_PKT_WORD1 = 0xf06c, + MV_V2_MAGIC_PKT_WORD2 = 0xf06d, + /* Wake on LAN registers */ + MV_V2_WOL_CTRL = 0xf06e, + MV_V2_WOL_STS = 0xf06f, + MV_V2_WOL_CLEAR_STS = BIT(15), + MV_V2_WOL_MAGIC_PKT_EN = BIT(0), + MV_V2_PORT_INTR_STS = 0xf040, + MV_V2_PORT_INTR_MASK = 0xf043, + MV_V2_WOL_INTR_EN = BIT(8), /* Temperature control/read registers (88X3310 only) */ MV_V2_TEMP_CTRL = 0xf08a, MV_V2_TEMP_CTRL_MASK = 0xc000, @@ -991,6 +1003,78 @@ static int mv2111_match_phy_device(struct phy_device *phydev) return mv211x_match_phy_device(phydev, false); } +static void mv2110_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) +{ + int ret; + + wol->supported = WAKE_MAGIC; + wol->wolopts = 0; + + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL); + if (ret < 0) + return; + + if (ret & MV_V2_WOL_MAGIC_PKT_EN) + wol->wolopts |= WAKE_MAGIC; +} + +static int mv2110_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) +{ + int ret; + + if (wol->wolopts & WAKE_MAGIC) { + /* Enable the WOL interrupt */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_PORT_INTR_MASK, + MV_V2_WOL_INTR_EN); + if (ret < 0) + return ret; + + /* Store the device address for the magic packet */ + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_MAGIC_PKT_WORD2, + ((phydev->attached_dev->dev_addr[5] << 8) | + phydev->attached_dev->dev_addr[4])); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_MAGIC_PKT_WORD1, + ((phydev->attached_dev->dev_addr[3] << 8) | + phydev->attached_dev->dev_addr[2])); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_MAGIC_PKT_WORD0, + ((phydev->attached_dev->dev_addr[1] << 8) | + phydev->attached_dev->dev_addr[0])); + if (ret < 0) + return ret; + + /* Clear WOL status and enable magic packet matching */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_WOL_CTRL, + MV_V2_WOL_MAGIC_PKT_EN | + MV_V2_WOL_CLEAR_STS); + if (ret < 0) + return ret; + } else { + /* Disable magic packet matching & reset WOL status bit */ + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_WOL_CTRL, + MV_V2_WOL_MAGIC_PKT_EN, + MV_V2_WOL_CLEAR_STS); + if (ret < 0) + return ret; + } + + /* Reset the clear WOL status bit as it does not self-clear */ + return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, + MV_V2_WOL_CTRL, + MV_V2_WOL_CLEAR_STS); +} + static struct phy_driver mv3310_drivers[] = { { .phy_id = MARVELL_PHY_ID_88X3310, @@ -1009,6 +1093,8 @@ static struct phy_driver mv3310_drivers[] = { .set_tunable = mv3310_set_tunable, .remove = mv3310_remove, .set_loopback = genphy_c45_loopback, + .get_wol = mv2110_get_wol, + .set_wol = mv2110_set_wol, }, { .phy_id = MARVELL_PHY_ID_88X3340, @@ -1045,6 +1131,8 @@ static struct phy_driver mv3310_drivers[] = { .set_tunable = mv3310_set_tunable, .remove = mv3310_remove, .set_loopback = genphy_c45_loopback, + .get_wol = mv2110_get_wol, + .set_wol = mv2110_set_wol, }, { .phy_id = MARVELL_PHY_ID_88E2110,