[PATCH net-next v3 0/3] DP83869 Feature additions

[PATCH net-next v3 0/3] DP83869 Feature additions

[Dan Murphy]

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

[PATCH net-next v3 1/3] net: dp83869: Add ability to advertise Fiber connection

[Dan Murphy]

Add the ability to advertise the Fiber connection if the strap or the
op-mode is configured for 100Base-FX.

Auto negotiation is not supported on this PHY when in fiber mode.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 58103152c601..48a68474f89c 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -52,6 +52,11 @@
 					 BMCR_FULLDPLX | \
 					 BMCR_SPEED1000)
 
+#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
+					ADVERTISED_FIBRE | ADVERTISED_BNC |  \
+					ADVERTISED_Pause | ADVERTISED_Asym_Pause | \
+					ADVERTISED_100baseT_Full)
+
 /* This is the same bit mask as the BMCR so re-use the BMCR default */
 #define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
 
@@ -300,6 +305,7 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 {
 	int phy_ctrl_val;
 	int ret;
+	int bmcr;
 
 	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
 	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
@@ -383,7 +389,37 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 
 		break;
 	case DP83869_RGMII_1000_BASE:
+		break;
 	case DP83869_RGMII_100_BASE:
+		/* Only allow advertising what this PHY supports */
+		linkmode_and(phydev->advertising, phydev->advertising,
+			     phydev->supported);
+
+		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
+				 phydev->supported);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
+				 phydev->advertising);
+
+		/* Auto neg is not supported in fiber mode */
+		bmcr = phy_read(phydev, MII_BMCR);
+		if (bmcr < 0)
+			return bmcr;
+
+		phydev->autoneg = AUTONEG_DISABLE;
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				   phydev->advertising);
+
+		if (bmcr & BMCR_ANENABLE) {
+			ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
+			if (ret < 0)
+				return ret;
+		}
+
+		phy_modify_changed(phydev, MII_ADVERTISE,
+				   MII_DP83869_FIBER_ADVERTISE,
+				   MII_DP83869_FIBER_ADVERTISE);
 		break;
 	default:
 		return -EINVAL;
-- 
2.28.0

[PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN

[Dan Murphy]

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);
+
+	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

[PATCH net-next v3 3/3] net: dp83869: Add speed optimization feature

[Dan Murphy]

Set the speed optimization bit on the DP83869 PHY.

Speed optimization, also known as link downshift, enables fallback to 100M
operation after multiple consecutive failed attempts at Gigabit link
establishment. Such a case could occur if cabling with only four wires
(two twisted pairs) were connected instead of the standard cabling with
eight wires (four twisted pairs).

The number of failed link attempts before falling back to 100M operation is
configurable. By default, four failed link attempts are required before
falling back to 100M.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---

v3 - Fixed checkpatch format issues

 drivers/net/phy/dp83869.c | 116 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 5045df9515a5..5d0130cf5a44 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/delay.h>
+#include <linux/bitfield.h>
 
 #include <dt-bindings/net/ti-dp83869.h>
 
@@ -20,6 +21,7 @@
 #define MII_DP83869_PHYCTRL	0x10
 #define MII_DP83869_MICR	0x12
 #define MII_DP83869_ISR		0x13
+#define DP83869_CFG2		0x14
 #define DP83869_CTRL		0x1f
 #define DP83869_CFG4		0x1e
 
@@ -121,6 +123,18 @@
 #define DP83869_WOL_SEC_EN		BIT(5)
 #define DP83869_WOL_ENH_MAC		BIT(7)
 
+/* CFG2 bits */
+#define DP83869_DOWNSHIFT_EN		(BIT(8) | BIT(9))
+#define DP83869_DOWNSHIFT_ATTEMPT_MASK	(BIT(10) | BIT(11))
+#define DP83869_DOWNSHIFT_1_COUNT_VAL	0
+#define DP83869_DOWNSHIFT_2_COUNT_VAL	1
+#define DP83869_DOWNSHIFT_4_COUNT_VAL	2
+#define DP83869_DOWNSHIFT_8_COUNT_VAL	3
+#define DP83869_DOWNSHIFT_1_COUNT	1
+#define DP83869_DOWNSHIFT_2_COUNT	2
+#define DP83869_DOWNSHIFT_4_COUNT	4
+#define DP83869_DOWNSHIFT_8_COUNT	8
+
 enum {
 	DP83869_PORT_MIRRORING_KEEP,
 	DP83869_PORT_MIRRORING_EN,
@@ -281,6 +295,99 @@ static void dp83869_get_wol(struct phy_device *phydev,
 		wol->wolopts = 0;
 }
 
+static int dp83869_get_downshift(struct phy_device *phydev, u8 *data)
+{
+	int val, cnt, enable, count;
+
+	val = phy_read(phydev, DP83869_CFG2);
+	if (val < 0)
+		return val;
+
+	enable = FIELD_GET(DP83869_DOWNSHIFT_EN, val);
+	cnt = FIELD_GET(DP83869_DOWNSHIFT_ATTEMPT_MASK, val);
+
+	switch (cnt) {
+	case DP83869_DOWNSHIFT_1_COUNT_VAL:
+		count = DP83869_DOWNSHIFT_1_COUNT;
+		break;
+	case DP83869_DOWNSHIFT_2_COUNT_VAL:
+		count = DP83869_DOWNSHIFT_2_COUNT;
+		break;
+	case DP83869_DOWNSHIFT_4_COUNT_VAL:
+		count = DP83869_DOWNSHIFT_4_COUNT;
+		break;
+	case DP83869_DOWNSHIFT_8_COUNT_VAL:
+		count = DP83869_DOWNSHIFT_8_COUNT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*data = enable ? count : DOWNSHIFT_DEV_DISABLE;
+
+	return 0;
+}
+
+static int dp83869_set_downshift(struct phy_device *phydev, u8 cnt)
+{
+	int val, count;
+
+	if (cnt > DP83869_DOWNSHIFT_8_COUNT)
+		return -E2BIG;
+
+	if (!cnt)
+		return phy_clear_bits(phydev, DP83869_CFG2,
+				      DP83869_DOWNSHIFT_EN);
+
+	switch (cnt) {
+	case DP83869_DOWNSHIFT_1_COUNT:
+		count = DP83869_DOWNSHIFT_1_COUNT_VAL;
+		break;
+	case DP83869_DOWNSHIFT_2_COUNT:
+		count = DP83869_DOWNSHIFT_2_COUNT_VAL;
+		break;
+	case DP83869_DOWNSHIFT_4_COUNT:
+		count = DP83869_DOWNSHIFT_4_COUNT_VAL;
+		break;
+	case DP83869_DOWNSHIFT_8_COUNT:
+		count = DP83869_DOWNSHIFT_8_COUNT_VAL;
+		break;
+	default:
+		phydev_err(phydev,
+			   "Downshift count must be 1, 2, 4 or 8\n");
+		return -EINVAL;
+	}
+
+	val = DP83869_DOWNSHIFT_EN;
+	val |= FIELD_PREP(DP83869_DOWNSHIFT_ATTEMPT_MASK, count);
+
+	return phy_modify(phydev, DP83869_CFG2,
+			  DP83869_DOWNSHIFT_EN | DP83869_DOWNSHIFT_ATTEMPT_MASK,
+			  val);
+}
+
+static int dp83869_get_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return dp83869_get_downshift(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int dp83869_set_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return dp83869_set_downshift(phydev, *(const u8 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int dp83869_config_port_mirroring(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
@@ -558,6 +665,12 @@ static int dp83869_config_init(struct phy_device *phydev)
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret, val;
 
+	/* Force speed optimization for the PHY even if it strapped */
+	ret = phy_modify(phydev, DP83869_CFG2, DP83869_DOWNSHIFT_EN,
+			 DP83869_DOWNSHIFT_EN);
+	if (ret)
+		return ret;
+
 	ret = dp83869_configure_mode(phydev, dp83869);
 	if (ret)
 		return ret;
@@ -656,6 +769,9 @@ static struct phy_driver dp83869_driver[] = {
 		.ack_interrupt	= dp83869_ack_interrupt,
 		.config_intr	= dp83869_config_intr,
 
+		.get_tunable	= dp83869_get_tunable,
+		.set_tunable	= dp83869_set_tunable,
+
 		.get_wol	= dp83869_get_wol,
 		.set_wol	= dp83869_set_wol,
 
-- 
2.28.0

Re: [PATCH net-next v3 1/3] net: dp83869: Add ability to advertise Fiber connection

[Jakub Kicinski]

On Thu, 3 Sep 2020 06:42:57 -0500 Dan Murphy wrote:
> Add the ability to advertise the Fiber connection if the strap or the
> op-mode is configured for 100Base-FX.
> 
> Auto negotiation is not supported on this PHY when in fiber mode.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Some comments, I'm not very phy-knowledgeable so bear with me
(hopefully PHY maintainers can correct me, too).

> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> index 58103152c601..48a68474f89c 100644
> --- a/drivers/net/phy/dp83869.c
> +++ b/drivers/net/phy/dp83869.c
> @@ -52,6 +52,11 @@
>  					 BMCR_FULLDPLX | \
>  					 BMCR_SPEED1000)
>  
> +#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
> +					ADVERTISED_FIBRE | ADVERTISED_BNC |  \

I'm not actually sure myself what the semantics of port type advertise
bits are, but if this is fiber why advertise TP and do you really have
BNC connectors? :S

> +					ADVERTISED_Pause | ADVERTISED_Asym_Pause | \
> +					ADVERTISED_100baseT_Full)

You say 100Base-FX, yet you advertise 100Base-T?

>  /* This is the same bit mask as the BMCR so re-use the BMCR default */
>  #define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
>  
> @@ -300,6 +305,7 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>  {
>  	int phy_ctrl_val;
>  	int ret;
> +	int bmcr;

Please keep reverse xmas tree ordering.

>  	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
>  	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
> @@ -383,7 +389,37 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>  
>  		break;
>  	case DP83869_RGMII_1000_BASE:
> +		break;
>  	case DP83869_RGMII_100_BASE:
> +		/* Only allow advertising what this PHY supports */
> +		linkmode_and(phydev->advertising, phydev->advertising,
> +			     phydev->supported);
> +
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> +				 phydev->supported);
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> +				 phydev->advertising);
> +
> +		/* Auto neg is not supported in fiber mode */
> +		bmcr = phy_read(phydev, MII_BMCR);
> +		if (bmcr < 0)
> +			return bmcr;
> +
> +		phydev->autoneg = AUTONEG_DISABLE;
> +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				   phydev->supported);
> +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				   phydev->advertising);
> +
> +		if (bmcr & BMCR_ANENABLE) {
> +			ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		phy_modify_changed(phydev, MII_ADVERTISE,
> +				   MII_DP83869_FIBER_ADVERTISE,
> +				   MII_DP83869_FIBER_ADVERTISE);

This only accesses standard registers, should it perhaps be a helper in
the kernel's phy code?

>  		break;
>  	default:
>  		return -EINVAL;

Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN

[Jakub Kicinski]

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?

Re: [PATCH net-next v3 3/3] net: dp83869: Add speed optimization feature

[Jakub Kicinski]

On Thu, 3 Sep 2020 06:42:59 -0500 Dan Murphy wrote:
> +static int dp83869_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> +	int val, count;
> +
> +	if (cnt > DP83869_DOWNSHIFT_8_COUNT)
> +		return -E2BIG;

ERANGE

> +	if (!cnt)
> +		return phy_clear_bits(phydev, DP83869_CFG2,
> +				      DP83869_DOWNSHIFT_EN);
> +
> +	switch (cnt) {
> +	case DP83869_DOWNSHIFT_1_COUNT:
> +		count = DP83869_DOWNSHIFT_1_COUNT_VAL;
> +		break;
> +	case DP83869_DOWNSHIFT_2_COUNT:
> +		count = DP83869_DOWNSHIFT_2_COUNT_VAL;
> +		break;
> +	case DP83869_DOWNSHIFT_4_COUNT:
> +		count = DP83869_DOWNSHIFT_4_COUNT_VAL;
> +		break;
> +	case DP83869_DOWNSHIFT_8_COUNT:
> +		count = DP83869_DOWNSHIFT_8_COUNT_VAL;
> +		break;
> +	default:
> +		phydev_err(phydev,
> +			   "Downshift count must be 1, 2, 4 or 8\n");
> +		return -EINVAL;
> +	}
> +
> +	val = DP83869_DOWNSHIFT_EN;
> +	val |= FIELD_PREP(DP83869_DOWNSHIFT_ATTEMPT_MASK, count);
> +
> +	return phy_modify(phydev, DP83869_CFG2,
> +			  DP83869_DOWNSHIFT_EN | DP83869_DOWNSHIFT_ATTEMPT_MASK,
> +			  val);
> +}

Re: [PATCH net-next v3 1/3] net: dp83869: Add ability to advertise Fiber connection

[Andrew Lunn]

On Sat, Sep 05, 2020 at 11:17:55AM -0700, Jakub Kicinski wrote:
> On Thu, 3 Sep 2020 06:42:57 -0500 Dan Murphy wrote:
> > Add the ability to advertise the Fiber connection if the strap or the
> > op-mode is configured for 100Base-FX.
> > 
> > Auto negotiation is not supported on this PHY when in fiber mode.
> > 
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
> Some comments, I'm not very phy-knowledgeable so bear with me
> (hopefully PHY maintainers can correct me, too).
> 
> > diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> > index 58103152c601..48a68474f89c 100644
> > --- a/drivers/net/phy/dp83869.c
> > +++ b/drivers/net/phy/dp83869.c
> > @@ -52,6 +52,11 @@
> >  					 BMCR_FULLDPLX | \
> >  					 BMCR_SPEED1000)
> >  
> > +#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
> > +					ADVERTISED_FIBRE | ADVERTISED_BNC |  \
> 
> I'm not actually sure myself what the semantics of port type advertise
> bits are, but if this is fiber why advertise TP and do you really have
> BNC connectors? :S

Hi Jakub

Normally, we start with a base of ETHTOOL_LINK_MODE_TP_BIT,
ETHTOOL_LINK_MODE_MII_BIT and then use genphy_read_abilities() to read
the standard registers in the PHY to determine what the PHY
supports. The PHY driver has the ability of provide its own function
to get the supported features, which is happening here. As far as i
remember, there is no standard way to indicate a PHY is doing Fibre,
not copper.

I agree that TP and BMC make no sense here, since my understanding is
that the device only supports Fibre when strapped for Fibre. It cannot
swap to TP, and it has been at least 20 years since i last had a BNC
cable in my hands.

In this context, i've no idea what MII means.

> > +					ADVERTISED_Pause | ADVERTISED_Asym_Pause | \
> > +					ADVERTISED_100baseT_Full)
> 
> You say 100Base-FX, yet you advertise 100Base-T?

100Base-FX does not actually exist in ADVERTISED_X form. I guess this
is historical. It was not widely supported, the broadcom PHYs appear
to support it, but not much else. We were also running out of bits to
represent these ADVERTISED_X values. Now that we have changed to linux
bitmaps and have unlimited number of bits, it makes sense to add it.

> > @@ -383,7 +389,37 @@ static int dp83869_configure_mode(struct phy_device *phydev,
> >  
> >  		break;
> >  	case DP83869_RGMII_1000_BASE:
> > +		break;
> >  	case DP83869_RGMII_100_BASE:
> > +		/* Only allow advertising what this PHY supports */
> > +		linkmode_and(phydev->advertising, phydev->advertising,
> > +			     phydev->supported);
> > +
> > +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> > +				 phydev->supported);
> > +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> > +				 phydev->advertising);
> > +
> > +		/* Auto neg is not supported in fiber mode */
> > +		bmcr = phy_read(phydev, MII_BMCR);
> > +		if (bmcr < 0)
> > +			return bmcr;
> > +
> > +		phydev->autoneg = AUTONEG_DISABLE;
> > +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +				   phydev->supported);
> > +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +				   phydev->advertising);
> > +
> > +		if (bmcr & BMCR_ANENABLE) {
> > +			ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		phy_modify_changed(phydev, MII_ADVERTISE,
> > +				   MII_DP83869_FIBER_ADVERTISE,
> > +				   MII_DP83869_FIBER_ADVERTISE);
> 
> This only accesses standard registers, should it perhaps be a helper in
> the kernel's phy code?

I suspect the PHY is not following the standard when strapped to
fibre.

	Andrew

Re: [PATCH net-next v3 3/3] net: dp83869: Add speed optimization feature

[Dan Murphy]

Jakub

On 9/5/20 1:38 PM, Jakub Kicinski wrote:
> On Thu, 3 Sep 2020 06:42:59 -0500 Dan Murphy wrote:
>> +static int dp83869_set_downshift(struct phy_device *phydev, u8 cnt)
>> +{
>> +	int val, count;
>> +
>> +	if (cnt > DP83869_DOWNSHIFT_8_COUNT)
>> +		return -E2BIG;
> ERANGE

This is not checking a range but making sure it is not bigger then 8.

IMO I would use ERANGE if the check was a boundary check for upper and 
lower bounds.

Dan

Re: [PATCH net-next v3 3/3] net: dp83869: Add speed optimization feature

[Jakub Kicinski]

On Tue, 8 Sep 2020 09:07:22 -0500 Dan Murphy wrote:
> On 9/5/20 1:38 PM, Jakub Kicinski wrote:
> > On Thu, 3 Sep 2020 06:42:59 -0500 Dan Murphy wrote:  
> >> +static int dp83869_set_downshift(struct phy_device *phydev, u8 cnt)
> >> +{
> >> +	int val, count;
> >> +
> >> +	if (cnt > DP83869_DOWNSHIFT_8_COUNT)
> >> +		return -E2BIG;  
> > ERANGE  
> 
> This is not checking a range but making sure it is not bigger then 8.
> 
> IMO I would use ERANGE if the check was a boundary check for upper and 
> lower bounds.

Yeah, ERANGE is not perfect, but the strerror for E2BIG is
"Argument list too long" - IDK if users seeing that will know that it
means the value is too large. Perhaps we should stick to EINVAL?

Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN

[Dan Murphy]

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 <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.
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.

Re: [PATCH net-next v3 3/3] net: dp83869: Add speed optimization feature

[Dan Murphy]

Jakub

On 9/8/20 12:47 PM, Jakub Kicinski wrote:
> On Tue, 8 Sep 2020 09:07:22 -0500 Dan Murphy wrote:
>> On 9/5/20 1:38 PM, Jakub Kicinski wrote:
>>> On Thu, 3 Sep 2020 06:42:59 -0500 Dan Murphy wrote:
>>>> +static int dp83869_set_downshift(struct phy_device *phydev, u8 cnt)
>>>> +{
>>>> +	int val, count;
>>>> +
>>>> +	if (cnt > DP83869_DOWNSHIFT_8_COUNT)
>>>> +		return -E2BIG;
>>> ERANGE
>> This is not checking a range but making sure it is not bigger then 8.
>>
>> IMO I would use ERANGE if the check was a boundary check for upper and
>> lower bounds.
> Yeah, ERANGE is not perfect, but the strerror for E2BIG is
> "Argument list too long" - IDK if users seeing that will know that it
> means the value is too large. Perhaps we should stick to EINVAL?

EINVAL works for me to.

Dan

Re: [PATCH net-next v3 1/3] net: dp83869: Add ability to advertise Fiber connection

[Dan Murphy]

Hello

On 9/7/20 9:29 AM, Andrew Lunn wrote:
> On Sat, Sep 05, 2020 at 11:17:55AM -0700, Jakub Kicinski wrote:
>> On Thu, 3 Sep 2020 06:42:57 -0500 Dan Murphy wrote:
>>> Add the ability to advertise the Fiber connection if the strap or the
>>> op-mode is configured for 100Base-FX.
>>>
>>> Auto negotiation is not supported on this PHY when in fiber mode.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> Some comments, I'm not very phy-knowledgeable so bear with me
>> (hopefully PHY maintainers can correct me, too).
>>
>>> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
>>> index 58103152c601..48a68474f89c 100644
>>> --- a/drivers/net/phy/dp83869.c
>>> +++ b/drivers/net/phy/dp83869.c
>>> @@ -52,6 +52,11 @@
>>>   					 BMCR_FULLDPLX | \
>>>   					 BMCR_SPEED1000)
>>>   
>>> +#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
>>> +					ADVERTISED_FIBRE | ADVERTISED_BNC |  \
>> I'm not actually sure myself what the semantics of port type advertise
>> bits are, but if this is fiber why advertise TP and do you really have
>> BNC connectors? :S
> Hi Jakub
>
> Normally, we start with a base of ETHTOOL_LINK_MODE_TP_BIT,
> ETHTOOL_LINK_MODE_MII_BIT and then use genphy_read_abilities() to read
> the standard registers in the PHY to determine what the PHY
> supports. The PHY driver has the ability of provide its own function
> to get the supported features, which is happening here. As far as i
> remember, there is no standard way to indicate a PHY is doing Fibre,
> not copper.
>
> I agree that TP and BMC make no sense here, since my understanding is
> that the device only supports Fibre when strapped for Fibre. It cannot
> swap to TP, and it has been at least 20 years since i last had a BNC
> cable in my hands.
>
> In this context, i've no idea what MII means.

I will remove the TP and BNC.


>
>>> +					ADVERTISED_Pause | ADVERTISED_Asym_Pause | \
>>> +					ADVERTISED_100baseT_Full)
>> You say 100Base-FX, yet you advertise 100Base-T?
> 100Base-FX does not actually exist in ADVERTISED_X form. I guess this
> is historical. It was not widely supported, the broadcom PHYs appear
> to support it, but not much else. We were also running out of bits to
> represent these ADVERTISED_X values. Now that we have changed to linux
> bitmaps and have unlimited number of bits, it makes sense to add it.

The note in the ethtool.h says

     /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
      * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
      * macro for bits > 31. The only way to use indices > 31 is to
      * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API.
      */

Which was added by Heiner

I guess I would prefer to add this in a separate patchset once I figure 
out how the ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API works

>
>>> @@ -383,7 +389,37 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>>   
>>>   		break;
>>>   	case DP83869_RGMII_1000_BASE:
>>> +		break;
>>>   	case DP83869_RGMII_100_BASE:
>>> +		/* Only allow advertising what this PHY supports */
>>> +		linkmode_and(phydev->advertising, phydev->advertising,
>>> +			     phydev->supported);
>>> +
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
>>> +				 phydev->supported);
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
>>> +				 phydev->advertising);
>>> +
>>> +		/* Auto neg is not supported in fiber mode */
>>> +		bmcr = phy_read(phydev, MII_BMCR);
>>> +		if (bmcr < 0)
>>> +			return bmcr;
>>> +
>>> +		phydev->autoneg = AUTONEG_DISABLE;
>>> +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>>> +				   phydev->supported);
>>> +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>>> +				   phydev->advertising);
>>> +
>>> +		if (bmcr & BMCR_ANENABLE) {
>>> +			ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
>>> +			if (ret < 0)
>>> +				return ret;
>>> +		}
>>> +
>>> +		phy_modify_changed(phydev, MII_ADVERTISE,
>>> +				   MII_DP83869_FIBER_ADVERTISE,
>>> +				   MII_DP83869_FIBER_ADVERTISE);
>> This only accesses standard registers, should it perhaps be a helper in
>> the kernel's phy code?
> I suspect the PHY is not following the standard when strapped to
> fibre.

No its a bit wonky in that respect.

Dan

Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN

[Andrew Lunn]

> > >   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

Re: [PATCH net-next v3 1/3] net: dp83869: Add ability to advertise Fiber connection

[Andrew Lunn]

> The note in the ethtool.h says
> 
>     /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
>      * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
>      * macro for bits > 31. The only way to use indices > 31 is to
>      * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API.
>      */
> 
> Which was added by Heiner
> 
> I guess I would prefer to add this in a separate patchset once I figure out
> how the ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API works

The phydev supported value is no longer a u32, it is now a bitmap. So
you can do something like

linkmode_set_bit(ETHTOOL_LINK_MODE_100BaseFX_Full_BIT, &supported);

       Andrew

Re: [PATCH net-next v3 2/3] net: phy: dp83869: support Wake on LAN

[Dan Murphy]

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