Hello
Adding features to the DP83869 PHY. These features are also supported in other
TI PHYs like the DP83867 and DP83822.
Fiber Advertisement:
The DP83869 supports a 100Base-FX connection. When this mode is selected the
driver needs to advertise that this PHY supports fiber.
WoL:
The PHY also supports Wake on Lan feature with SecureOn password.
Downshift:
Speed optimization or AKA downshift is also supported by this PHY.
Dan
Dan Murphy (3):
net: dp83869: Add ability to advertise Fiber connection
net: phy: dp83869: support Wake on LAN
net: dp83869: Add speed optimization feature
drivers/net/phy/dp83869.c | 280 ++++++++++++++++++++++++++++++++++++++
1 file changed, 280 insertions(+)
--
2.28.0
This adds WoL support on TI DP83869 for magic, magic secure, unicast and
broadcast.
Signed-off-by: Dan Murphy <[email protected]>
---
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);
+
+ 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]));
+ 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]));
+
+ 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;
+}
+
static int dp83869_config_port_mirroring(struct phy_device *phydev)
{
struct dp83869_private *dp83869 = phydev->priv;
@@ -531,6 +656,9 @@ static struct phy_driver dp83869_driver[] = {
.ack_interrupt = dp83869_ack_interrupt,
.config_intr = dp83869_config_intr,
+ .get_wol = dp83869_get_wol,
+ .set_wol = dp83869_set_wol,
+
.suspend = genphy_suspend,
.resume = genphy_resume,
},
--
2.28.0
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 <[email protected]>
> ---
> 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?
Jakub
Thanks for the review
On 9/5/20 1:34 PM, Jakub Kicinski wrote:
> 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 <[email protected]>
>> ---
>> 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.
I will add it back
>
>> + 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
OK
>
>> + 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?
Unicast and broadcast are the ways to send the magic packet.
Magic secure is programmed below into the SOP (secure on pass) registers
>
>> +
>> + 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?
Enhanced MAC - Enables enhanced RX features. This bit should be set when
using
wakeup abilities, CRC check or RX 1588 indication
>
> 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.
A helper is a bit overkill see below.
>
>> +}
>> +
>> 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?
Factor what out? Yes the DP83867 and DP83869 are very similar in
registers and bitmaps. They just differ in their feature sets.
The WoL code was copied and pasted to the 869 and I would like to keep
the two files as similar as I can as it will be easier to fix and find bugs.
> > > 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?
>
> Factor what out?? Yes the DP83867 and DP83869 are very similar in registers
> and bitmaps.? They just differ in their feature sets.
>
> The WoL code was copied and pasted to the 869 and I would like to keep the
> two files as similar as I can as it will be easier to fix and find bugs.
It will be even easier if they shared the same code. You could create
a library of functions, like bcm-phy-lib.c.
Andrew
Andrew
On 9/10/20 1:02 PM, Andrew Lunn wrote:
>>>> 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?
>> Factor what out? Yes the DP83867 and DP83869 are very similar in registers
>> and bitmaps. They just differ in their feature sets.
>>
>> The WoL code was copied and pasted to the 869 and I would like to keep the
>> two files as similar as I can as it will be easier to fix and find bugs.
> It will be even easier if they shared the same code. You could create
> a library of functions, like bcm-phy-lib.c.
If I do that I would want to add in the DP83822 and the DP83811 as well
even though the SOP and Data registers are different the code is the same.
I can just pass in the register numbers in.
That will have to be something I refactor later as it will rip up at
least 4 TI drivers if not more.
Dan