diff mbox series

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

Message ID 20200903114259.14013-4-dmurphy@ti.com
State Superseded
Headers show
Series None | expand

Commit Message

Dan Murphy Sept. 3, 2020, 11:42 a.m. UTC 
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(+)

Comments

Jakub Kicinski Sept. 5, 2020, 6:38 p.m. UTC | #1 
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);
> +}
Dan Murphy Sept. 8, 2020, 2:07 p.m. UTC | #2 
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
Jakub Kicinski Sept. 8, 2020, 5:47 p.m. UTC | #3 
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?
Dan Murphy Sept. 10, 2020, 5:34 p.m. UTC | #4 
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
diff mbox series

Patch

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,