Messages in this thread | | | Date | Sat, 5 Sep 2020 11:34:28 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN |
| |
On Thu, 3 Sep 2020 06:42:58 -0500 Dan Murphy wrote: > This adds WoL support on TI DP83869 for magic, magic secure, unicast and > broadcast. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/net/phy/dp83869.c | 128 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 128 insertions(+) > > diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c > index 48a68474f89c..5045df9515a5 100644 > --- a/drivers/net/phy/dp83869.c > +++ b/drivers/net/phy/dp83869.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/ethtool.h> > +#include <linux/etherdevice.h> > #include <linux/kernel.h> > #include <linux/mii.h> > #include <linux/module.h> > @@ -27,6 +28,13 @@ > #define DP83869_RGMIICTL 0x0032 > #define DP83869_STRAP_STS1 0x006e > #define DP83869_RGMIIDCTL 0x0086 > +#define DP83869_RXFCFG 0x0134 > +#define DP83869_RXFPMD1 0x0136 > +#define DP83869_RXFPMD2 0x0137 > +#define DP83869_RXFPMD3 0x0138 > +#define DP83869_RXFSOP1 0x0139 > +#define DP83869_RXFSOP2 0x013A > +#define DP83869_RXFSOP3 0x013B > #define DP83869_IO_MUX_CFG 0x0170 > #define DP83869_OP_MODE 0x01df > #define DP83869_FX_CTRL 0x0c00 > @@ -105,6 +113,14 @@ > #define DP83869_OP_MODE_MII BIT(5) > #define DP83869_SGMII_RGMII_BRIDGE BIT(6) > > +/* RXFCFG bits*/ > +#define DP83869_WOL_MAGIC_EN BIT(0) > +#define DP83869_WOL_PATTERN_EN BIT(1) > +#define DP83869_WOL_BCAST_EN BIT(2) > +#define DP83869_WOL_UCAST_EN BIT(4) > +#define DP83869_WOL_SEC_EN BIT(5) > +#define DP83869_WOL_ENH_MAC BIT(7) > + > enum { > DP83869_PORT_MIRRORING_KEEP, > DP83869_PORT_MIRRORING_EN, > @@ -156,6 +172,115 @@ static int dp83869_config_intr(struct phy_device *phydev) > return phy_write(phydev, MII_DP83869_MICR, micr_status); > } > > +static int dp83869_set_wol(struct phy_device *phydev, > + struct ethtool_wolinfo *wol) > +{ > + struct net_device *ndev = phydev->attached_dev; > + u16 val_rxcfg, val_micr; > + u8 *mac; > + > + val_rxcfg = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG); > + val_micr = phy_read(phydev, MII_DP83869_MICR);
In the previous patch you checked if phy_read() failed, here you don't.
> + if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_UCAST | > + WAKE_BCAST)) { > + val_rxcfg |= DP83869_WOL_ENH_MAC; > + val_micr |= MII_DP83869_MICR_WOL_INT_EN; > + > + if (wol->wolopts & WAKE_MAGIC) { > + mac = (u8 *)ndev->dev_addr; > + > + if (!is_valid_ether_addr(mac)) > + return -EINVAL; > + > + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD1, > + (mac[1] << 8 | mac[0]));
parenthesis unnecessary
> + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD2, > + (mac[3] << 8 | mac[2])); > + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFPMD3, > + (mac[5] << 8 | mac[4]));
Why only program mac addr for wake_magic, does magic_secure or unicast not require it?
> + > + val_rxcfg |= DP83869_WOL_MAGIC_EN; > + } else { > + val_rxcfg &= ~DP83869_WOL_MAGIC_EN; > + } > + > + if (wol->wolopts & WAKE_MAGICSECURE) { > + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP1, > + (wol->sopass[1] << 8) | wol->sopass[0]); > + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP2, > + (wol->sopass[3] << 8) | wol->sopass[2]); > + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFSOP3, > + (wol->sopass[5] << 8) | wol->sopass[4]); > + > + val_rxcfg |= DP83869_WOL_SEC_EN; > + } else { > + val_rxcfg &= ~DP83869_WOL_SEC_EN; > + } > + > + if (wol->wolopts & WAKE_UCAST) > + val_rxcfg |= DP83869_WOL_UCAST_EN; > + else > + val_rxcfg &= ~DP83869_WOL_UCAST_EN; > + > + if (wol->wolopts & WAKE_BCAST) > + val_rxcfg |= DP83869_WOL_BCAST_EN; > + else > + val_rxcfg &= ~DP83869_WOL_BCAST_EN; > + } else { > + val_rxcfg &= ~DP83869_WOL_ENH_MAC; > + val_micr &= ~MII_DP83869_MICR_WOL_INT_EN; > + } > + > + phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG, val_rxcfg); > + phy_write(phydev, MII_DP83869_MICR, val_micr); > + > + return 0; > +} > + > +static void dp83869_get_wol(struct phy_device *phydev, > + struct ethtool_wolinfo *wol) > +{ > + u16 value, sopass_val; > + > + wol->supported = (WAKE_UCAST | WAKE_BCAST | WAKE_MAGIC | > + WAKE_MAGICSECURE); > + wol->wolopts = 0; > + > + value = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RXFCFG); > + > + if (value & DP83869_WOL_UCAST_EN) > + wol->wolopts |= WAKE_UCAST; > + > + if (value & DP83869_WOL_BCAST_EN) > + wol->wolopts |= WAKE_BCAST; > + > + if (value & DP83869_WOL_MAGIC_EN) > + wol->wolopts |= WAKE_MAGIC; > + > + if (value & DP83869_WOL_SEC_EN) { > + sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR, > + DP83869_RXFSOP1); > + wol->sopass[0] = (sopass_val & 0xff); > + wol->sopass[1] = (sopass_val >> 8); > + > + sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR, > + DP83869_RXFSOP2); > + wol->sopass[2] = (sopass_val & 0xff); > + wol->sopass[3] = (sopass_val >> 8); > + > + sopass_val = phy_read_mmd(phydev, DP83869_DEVADDR, > + DP83869_RXFSOP3); > + wol->sopass[4] = (sopass_val & 0xff); > + wol->sopass[5] = (sopass_val >> 8); > + > + wol->wolopts |= WAKE_MAGICSECURE; > + } > + > + if (!(value & DP83869_WOL_ENH_MAC)) > + wol->wolopts = 0;
What does ENH stand for?
Perhaps it would be cleaner to make a helper like this:
u32 helper(u16 rxfsop1) { u32 wolopts;
if (!(value & DP83869_WOL_ENH_MAC)) return 0;
if (value & DP83869_WOL_UCAST_EN) wolopts |= WAKE_UCAST; if (value & DP83869_WOL_BCAST_EN) wolopts |= WAKE_BCAST; if (value & DP83869_WOL_MAGIC_EN) wolopts |= WAKE_MAGIC; if (value & DP83869_WOL_SEC_EN) wolopts |= WAKE_MAGICSECURE;
return wolopts; }
wol->wolopts = helper(value);
setting the bits and then clearing the value looks strange.
> +} > + > static int dp83869_config_port_mirroring(struct phy_device *phydev) > { > struct dp83869_private *dp83869 = phydev->priv;
Overall this code looks quite similar to dp83867, is there no way to factor this out?
| |