[PATCH net-next v9 0/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs

[PATCH net-next v9 0/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs

[Allan W. Nielsen]

Hi Andrew (and other),

Raju and I have been going through all the comments received on the edge-rate
feature, and tried to address them (once again...).

The patch is rebased to fit on top of net-next (it depends on a4cc96d1f).

Following initiatives is covered:

- Updated device tree bindings documentation for edge-rate
- The edge-rate is now specified as a "slowdown", meaning that it is now
  being specified as positive values instead of negative (both
  documentation and implementation wise).
- Only explicitly documented values for "vsc8531,vddmac" and
  "vsc8531,edge-slowdown" are accepted by the device driver.
- Deleting include/dt-bindings/net/mscc-phy-vsc8531.h as it was not needed.
- Initialize to default values when CONFIG_OF_MDIO is not defined
- Using ARRAY_SIZE when iterating through an array.

Please review.

Best regards
Allan W. Nielsen

[PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs.

[Allan W. Nielsen]

Edge-Rate cleanup include the following:
- Updated device tree bindings documentation for edge-rate
- The edge-rate is now specified as a "slowdown", meaning that it is now
  being specified as positive values instead of negative (both
  documentation and implementation wise).
- Only explicitly documented values for "vsc8531,vddmac" and
  "vsc8531,edge-slowdown" are accepted by the device driver.
- Deleting include/dt-bindings/net/mscc-phy-vsc8531.h as it was not needed.

Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
Signed-off-by: Raju Lakkaraju <raju.lakkaraju@microsemi.com>
---
 .../devicetree/bindings/net/mscc-phy-vsc8531.txt   | 49 ++++++++------
 drivers/net/phy/mscc.c                             | 79 +++++++++++++---------
 include/dt-bindings/net/mscc-phy-vsc8531.h         | 21 ------
 3 files changed, 75 insertions(+), 74 deletions(-)
 delete mode 100644 include/dt-bindings/net/mscc-phy-vsc8531.h

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 99c7eb0..f552033 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -6,20 +6,27 @@ Required properties:
 		  Documentation/devicetree/bindings/net/phy.txt
 
 Optional properties:
-- vsc8531,vddmac	: The vddmac in mV.
+- vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
+			  in the first row of Table 1 (below).
+			  This property is only used in combination
+			  with the 'edge-slowdown' property.
+			  Default value is 3300.
 - vsc8531,edge-slowdown	: % the edge should be slowed down relative to
-			  the fastest possible edge time. Native sign
-			  need not enter.
+			  the fastest possible edge time.
 			  Edge rate sets the drive strength of the MAC
-			  interface output signals.  Changing the drive
-			  strength will affect the edge rate of the output
-			  signal.  The goal of this setting is to help
-			  reduce electrical emission (EMI) by being able
-			  to reprogram drive strength and in effect slow
-			  down the edge rate if desired.  Table 1 shows the
-			  impact to the edge rate per VDDMAC supply for each
-			  drive strength setting.
-			  Ref: Table:1 - Edge rate change below.
+			  interface output signals.  Changing the
+			  drive strength will affect the edge rate of
+			  the output signal.  The goal of this setting
+			  is to help reduce electrical emission (EMI)
+			  by being able to reprogram drive strength
+			  and in effect slow down the edge rate if
+			  desired.
+			  To adjust the edge-slowdown, the 'vddmac'
+			  must be specified. Table 1 lists the
+			  supported edge-slowdown values for a given
+			  'vddmac'.
+			  Default value is 0%.
+			  Ref: Table:1 - Edge rate change (below).
 
 Note: see dt-bindings/net/mscc-phy-vsc8531.h for applicable values
 
@@ -29,23 +36,23 @@ Table: 1 - Edge rate change
 |								|
 | 3300 mV	2500 mV		1800 mV		1500 mV		|
 |---------------------------------------------------------------|
-| Default	Deafult		Default		Default		|
+| 0%		0%		0%		0%		|
 | (Fastest)			(recommended)	(recommended)	|
 |---------------------------------------------------------------|
-| -2%		-3%		-5%		-6%		|
+| 2%		3%		5%		6%		|
 |---------------------------------------------------------------|
-| -4%		-6%		-9%		-14%		|
+| 4%		6%		9%		14%		|
 |---------------------------------------------------------------|
-| -7%		-10%		-16%		-21%		|
+| 7%		10%		16%		21%		|
 |(recommended)	(recommended)					|
 |---------------------------------------------------------------|
-| -10%		-14%		-23%		-29%		|
+| 10%		14%		23%		29%		|
 |---------------------------------------------------------------|
-| -17%		-23%		-35%		-42%		|
+| 17%		23%		35%		42%		|
 |---------------------------------------------------------------|
-| -29%		-37%		-52%		-58%		|
+| 29%		37%		52%		58%		|
 |---------------------------------------------------------------|
-| -53%		-63%		-76%		-77%		|
+| 53%		63%		76%		77%		|
 | (slowest)							|
 |---------------------------------------------------------------|
 
@@ -54,5 +61,5 @@ Example:
         vsc8531_0: ethernet-phy@0 {
                 compatible = "ethernet-phy-id0007.0570";
                 vsc8531,vddmac		= <3300>;
-                vsc8531,edge-slowdown	= <21>;
+                vsc8531,edge-slowdown	= <7>;
         };
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index a17573e..2bd00b6 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -12,7 +12,6 @@
 #include <linux/mii.h>
 #include <linux/phy.h>
 #include <linux/of.h>
-#include <dt-bindings/net/mscc-phy-vsc8531.h>
 
 enum rgmii_rx_clock_delay {
 	RGMII_RX_CLK_DELAY_0_2_NS = 0,
@@ -56,21 +55,27 @@ enum rgmii_rx_clock_delay {
 #define PHY_ID_VSC8531			  0x00070570
 #define PHY_ID_VSC8541			  0x00070770
 
-struct edge_rate_table {
+#define MSCC_VDDMAC_1500		  1500
+#define MSCC_VDDMAC_1800		  1800
+#define MSCC_VDDMAC_2500		  2500
+#define MSCC_VDDMAC_3300		  3300
+
+struct vsc8531_edge_rate_table {
 	u16 vddmac;
-	int slowdown[MSCC_SLOWDOWN_MAX];
+	u8 slowdown[8];
 };
 
-struct edge_rate_table edge_table[MSCC_VDDMAC_MAX] = {
-	{3300, { 0, -2, -4,  -7,  -10, -17, -29, -53} },
-	{2500, { 0, -3, -6,  -10, -14, -23, -37, -63} },
-	{1800, { 0, -5, -9,  -16, -23, -35, -52, -76} },
-	{1500, { 0, -6, -14, -21, -29, -42, -58, -77} },
+static const struct vsc8531_edge_rate_table edge_table[] = {
+	{MSCC_VDDMAC_3300, { 0, 2,  4,  7, 10, 17, 29, 53} },
+	{MSCC_VDDMAC_2500, { 0, 3,  6, 10, 14, 23, 37, 63} },
+	{MSCC_VDDMAC_1800, { 0, 5,  9, 16, 23, 35, 52, 76} },
+	{MSCC_VDDMAC_1500, { 0, 6, 14, 21, 29, 42, 58, 77} },
 };
 
 struct vsc8531_private {
 	u8 edge_slowdown;
 	u16 vddmac;
+	int rate_magic;
 };
 
 static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
@@ -81,29 +86,21 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 	return rc;
 }
 
-static u8 edge_rate_magic_get(u16 vddmac,
-			      int slowdown)
+static int vsc85xx_edge_rate_magic_get(u16 vddmac, u8 slowdown)
 {
-	int rc = (MSCC_SLOWDOWN_MAX - 1);
 	u8 vdd;
 	u8 sd;
-
-	for (vdd = 0; vdd < MSCC_VDDMAC_MAX; vdd++) {
-		if (edge_table[vdd].vddmac == vddmac) {
-			for (sd = 0; sd < MSCC_SLOWDOWN_MAX; sd++) {
-				if (edge_table[vdd].slowdown[sd] <= slowdown) {
-					rc = (MSCC_SLOWDOWN_MAX - sd - 1);
-					break;
-				}
-			}
-		}
-	}
-
-	return rc;
+	u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);
+
+	for (vdd = 0; vdd < ARRAY_SIZE(edge_table); vdd++)
+		if (edge_table[vdd].vddmac == vddmac)
+			for (sd = 0; sd < sd_array_size; sd++)
+				if (edge_table[vdd].slowdown[sd] == slowdown)
+					return (sd_array_size - sd - 1);
+	return -EINVAL;
 }
 
-static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
-				      u8 edge_rate)
+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
 {
 	int rc;
 	u16 reg_val;
@@ -199,17 +196,38 @@ static int vsc8531_of_init(struct phy_device *phydev)
 				  &vsc8531->vddmac);
 	if (rc == -EINVAL)
 		vsc8531->vddmac = MSCC_VDDMAC_3300;
+
 	rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown",
 				 &vsc8531->edge_slowdown);
 	if (rc == -EINVAL)
 		vsc8531->edge_slowdown = 0;
 
-	rc = 0;
-	return rc;
+	vsc8531->rate_magic = vsc85xx_edge_rate_magic_get(
+		vsc8531->vddmac, vsc8531->edge_slowdown);
+	if (vsc8531->rate_magic < 0) {
+		phydev_err(phydev,
+			   "Invalid vsc8531,vddmac or vsc8531,edge-slowdown");
+		return vsc8531->rate_magic;
+	}
+
+	return 0;
 }
 #else
 static int vsc8531_of_init(struct phy_device *phydev)
 {
+	struct vsc8531_private *vsc8531 = phydev->priv;
+
+	vsc8531->vddmac = MSCC_VDDMAC_3300;
+	vsc8531->edge_slowdown = 0;
+
+	vsc8531->rate_magic = vsc85xx_edge_rate_magic_get(
+		vsc8531->vddmac, vsc8531->edge_slowdown);
+	if (vsc8531->rate_magic < 0) {
+		phydev_err(phydev,
+			   "Invalid vsc8531,vddmac or vsc8531,edge-slowdown");
+		return vsc8531->rate_magic;
+	}
+
 	return 0;
 }
 #endif /* CONFIG_OF_MDIO */
@@ -218,7 +236,6 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc;
 	struct vsc8531_private *vsc8531 = phydev->priv;
-	u8 edge_rate;
 
 	rc = vsc8531_of_init(phydev);
 	if (rc)
@@ -232,9 +249,7 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	if (rc)
 		return rc;
 
-	edge_rate = edge_rate_magic_get(vsc8531->vddmac,
-					-(int)vsc8531->edge_slowdown);
-	rc = vsc85xx_edge_rate_cntl_set(phydev, edge_rate);
+	rc = vsc85xx_edge_rate_cntl_set(phydev, vsc8531->rate_magic);
 	if (rc)
 		return rc;
 
diff --git a/include/dt-bindings/net/mscc-phy-vsc8531.h b/include/dt-bindings/net/mscc-phy-vsc8531.h
deleted file mode 100644
index 2383dd2..0000000
--- a/include/dt-bindings/net/mscc-phy-vsc8531.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Device Tree constants for Microsemi VSC8531 PHY
- *
- * Author: Nagaraju Lakkaraju
- *
- * License: Dual MIT/GPL
- * Copyright (c) 2016 Microsemi Corporation
- */
-
-#ifndef _DT_BINDINGS_MSCC_VSC8531_H
-#define _DT_BINDINGS_MSCC_VSC8531_H
-
-/* MAC interface Edge rate control VDDMAC in milli Volts */
-#define MSCC_VDDMAC_3300		 3300
-#define MSCC_VDDMAC_2500		 2500
-#define MSCC_VDDMAC_1800		 1800
-#define MSCC_VDDMAC_1500		 1500
-#define MSCC_VDDMAC_MAX			 4
-#define MSCC_SLOWDOWN_MAX		 8
-
-#endif
-- 
2.7.3

Re: [PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs.

[Andrew Lunn]

On Fri, Oct 07, 2016 at 10:28:24AM +0200, Allan W. Nielsen wrote:
> Edge-Rate cleanup include the following:
> - Updated device tree bindings documentation for edge-rate
> - The edge-rate is now specified as a "slowdown", meaning that it is now
>   being specified as positive values instead of negative (both
>   documentation and implementation wise).
> - Only explicitly documented values for "vsc8531,vddmac" and
>   "vsc8531,edge-slowdown" are accepted by the device driver.
> - Deleting include/dt-bindings/net/mscc-phy-vsc8531.h as it was not needed.
> 
> Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
> Signed-off-by: Raju Lakkaraju <raju.lakkaraju@microsemi.com>
> ---
>  .../devicetree/bindings/net/mscc-phy-vsc8531.txt   | 49 ++++++++------
>  drivers/net/phy/mscc.c                             | 79 +++++++++++++---------
>  include/dt-bindings/net/mscc-phy-vsc8531.h         | 21 ------
>  3 files changed, 75 insertions(+), 74 deletions(-)
>  delete mode 100644 include/dt-bindings/net/mscc-phy-vsc8531.h
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 99c7eb0..f552033 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -6,20 +6,27 @@ Required properties:
>  		  Documentation/devicetree/bindings/net/phy.txt
>  
>  Optional properties:
> -- vsc8531,vddmac	: The vddmac in mV.
> +- vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
> +			  in the first row of Table 1 (below).
> +			  This property is only used in combination
> +			  with the 'edge-slowdown' property.
> +			  Default value is 3300.
>  - vsc8531,edge-slowdown	: % the edge should be slowed down relative to
> -			  the fastest possible edge time. Native sign
> -			  need not enter.
> +			  the fastest possible edge time.
>  			  Edge rate sets the drive strength of the MAC
> -			  interface output signals.  Changing the drive
> -			  strength will affect the edge rate of the output
> -			  signal.  The goal of this setting is to help
> -			  reduce electrical emission (EMI) by being able
> -			  to reprogram drive strength and in effect slow
> -			  down the edge rate if desired.  Table 1 shows the
> -			  impact to the edge rate per VDDMAC supply for each
> -			  drive strength setting.
> -			  Ref: Table:1 - Edge rate change below.
> +			  interface output signals.  Changing the
> +			  drive strength will affect the edge rate of
> +			  the output signal.  The goal of this setting
> +			  is to help reduce electrical emission (EMI)
> +			  by being able to reprogram drive strength
> +			  and in effect slow down the edge rate if
> +			  desired.
> +			  To adjust the edge-slowdown, the 'vddmac'
> +			  must be specified. Table 1 lists the
> +			  supported edge-slowdown values for a given
> +			  'vddmac'.
> +			  Default value is 0%.
> +			  Ref: Table:1 - Edge rate change (below).
>  
>  Note: see dt-bindings/net/mscc-phy-vsc8531.h for applicable values

Hi Allen

Overall, this is much better. I just have a few nitpicks.

dt-bindings/net/mscc-phy-vsc8531.h is removed by this patch. It would
be good to also remove the reference.

>  
> @@ -29,23 +36,23 @@ Table: 1 - Edge rate change
>  |								|
>  | 3300 mV	2500 mV		1800 mV		1500 mV		|
>  |---------------------------------------------------------------|
> -| Default	Deafult		Default		Default		|
> +| 0%		0%		0%		0%		|
>  | (Fastest)			(recommended)	(recommended)	|
>  |---------------------------------------------------------------|
> -| -2%		-3%		-5%		-6%		|
> +| 2%		3%		5%		6%		|
>  |---------------------------------------------------------------|
> -| -4%		-6%		-9%		-14%		|
> +| 4%		6%		9%		14%		|
>  |---------------------------------------------------------------|
> -| -7%		-10%		-16%		-21%		|
> +| 7%		10%		16%		21%		|
>  |(recommended)	(recommended)					|
>  |---------------------------------------------------------------|
> -| -10%		-14%		-23%		-29%		|
> +| 10%		14%		23%		29%		|
>  |---------------------------------------------------------------|
> -| -17%		-23%		-35%		-42%		|
> +| 17%		23%		35%		42%		|
>  |---------------------------------------------------------------|
> -| -29%		-37%		-52%		-58%		|
> +| 29%		37%		52%		58%		|
>  |---------------------------------------------------------------|
> -| -53%		-63%		-76%		-77%		|
> +| 53%		63%		76%		77%		|
>  | (slowest)							|
>  |---------------------------------------------------------------|
>  
> @@ -54,5 +61,5 @@ Example:
>          vsc8531_0: ethernet-phy@0 {
>                  compatible = "ethernet-phy-id0007.0570";
>                  vsc8531,vddmac		= <3300>;
> -                vsc8531,edge-slowdown	= <21>;
> +                vsc8531,edge-slowdown	= <7>;
>          };
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index a17573e..2bd00b6 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -12,7 +12,6 @@
>  #include <linux/mii.h>
>  #include <linux/phy.h>
>  #include <linux/of.h>
> -#include <dt-bindings/net/mscc-phy-vsc8531.h>
>  
>  enum rgmii_rx_clock_delay {
>  	RGMII_RX_CLK_DELAY_0_2_NS = 0,
> @@ -56,21 +55,27 @@ enum rgmii_rx_clock_delay {
>  #define PHY_ID_VSC8531			  0x00070570
>  #define PHY_ID_VSC8541			  0x00070770
>  
> -struct edge_rate_table {
> +#define MSCC_VDDMAC_1500		  1500
> +#define MSCC_VDDMAC_1800		  1800
> +#define MSCC_VDDMAC_2500		  2500
> +#define MSCC_VDDMAC_3300		  3300
> +
> +struct vsc8531_edge_rate_table {
>  	u16 vddmac;
> -	int slowdown[MSCC_SLOWDOWN_MAX];
> +	u8 slowdown[8];
>  };
>  
> -struct edge_rate_table edge_table[MSCC_VDDMAC_MAX] = {
> -	{3300, { 0, -2, -4,  -7,  -10, -17, -29, -53} },
> -	{2500, { 0, -3, -6,  -10, -14, -23, -37, -63} },
> -	{1800, { 0, -5, -9,  -16, -23, -35, -52, -76} },
> -	{1500, { 0, -6, -14, -21, -29, -42, -58, -77} },
> +static const struct vsc8531_edge_rate_table edge_table[] = {
> +	{MSCC_VDDMAC_3300, { 0, 2,  4,  7, 10, 17, 29, 53} },
> +	{MSCC_VDDMAC_2500, { 0, 3,  6, 10, 14, 23, 37, 63} },
> +	{MSCC_VDDMAC_1800, { 0, 5,  9, 16, 23, 35, 52, 76} },
> +	{MSCC_VDDMAC_1500, { 0, 6, 14, 21, 29, 42, 58, 77} },
>  };
>  
>  struct vsc8531_private {
>  	u8 edge_slowdown;
>  	u16 vddmac;
> +	int rate_magic;
>  };

You don't need edge_slowdown and vddmac in the private structure,
since they are never used after determining what rate_magic is.

>  
>  static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
> @@ -81,29 +86,21 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
>  	return rc;
>  }
>  
> -static u8 edge_rate_magic_get(u16 vddmac,
> -			      int slowdown)
> +static int vsc85xx_edge_rate_magic_get(u16 vddmac, u8 slowdown)
>  {
> -	int rc = (MSCC_SLOWDOWN_MAX - 1);
>  	u8 vdd;
>  	u8 sd;
> -
> -	for (vdd = 0; vdd < MSCC_VDDMAC_MAX; vdd++) {
> -		if (edge_table[vdd].vddmac == vddmac) {
> -			for (sd = 0; sd < MSCC_SLOWDOWN_MAX; sd++) {
> -				if (edge_table[vdd].slowdown[sd] <= slowdown) {
> -					rc = (MSCC_SLOWDOWN_MAX - sd - 1);
> -					break;
> -				}
> -			}
> -		}
> -	}
> -
> -	return rc;
> +	u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);
> +
> +	for (vdd = 0; vdd < ARRAY_SIZE(edge_table); vdd++)
> +		if (edge_table[vdd].vddmac == vddmac)
> +			for (sd = 0; sd < sd_array_size; sd++)
> +				if (edge_table[vdd].slowdown[sd] == slowdown)
> +					return (sd_array_size - sd - 1);
> +	return -EINVAL;
>  }
>  
> -static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
> -				      u8 edge_rate)
> +static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
>  {
>  	int rc;
>  	u16 reg_val;
> @@ -199,17 +196,38 @@ static int vsc8531_of_init(struct phy_device *phydev)
>  				  &vsc8531->vddmac);
>  	if (rc == -EINVAL)
>  		vsc8531->vddmac = MSCC_VDDMAC_3300;
> +
>  	rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown",
>  				 &vsc8531->edge_slowdown);
>  	if (rc == -EINVAL)
>  		vsc8531->edge_slowdown = 0;
>  
> -	rc = 0;
> -	return rc;
> +	vsc8531->rate_magic = vsc85xx_edge_rate_magic_get(
> +		vsc8531->vddmac, vsc8531->edge_slowdown);
> +	if (vsc8531->rate_magic < 0) {
> +		phydev_err(phydev,
> +			   "Invalid vsc8531,vddmac or vsc8531,edge-slowdown");
> +		return vsc8531->rate_magic;
> +	}
> +
> +	return 0;
>  }
>  #else
>  static int vsc8531_of_init(struct phy_device *phydev)
>  {
> +	struct vsc8531_private *vsc8531 = phydev->priv;
> +
> +	vsc8531->vddmac = MSCC_VDDMAC_3300;
> +	vsc8531->edge_slowdown = 0;
> +
> +	vsc8531->rate_magic = vsc85xx_edge_rate_magic_get(
> +		vsc8531->vddmac, vsc8531->edge_slowdown);
> +	if (vsc8531->rate_magic < 0) {
> +		phydev_err(phydev,
> +			   "Invalid vsc8531,vddmac or vsc8531,edge-slowdown");
> +		return vsc8531->rate_magic;
> +	}

You could skip all this and just have:

    vsc8531->rate_magic = 0;

vsc8531->vddmac and vsc8531->edge_slowdown are not used anywhere else.

	Andrew

Re: [PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs.

[Allan W. Nielsen]

Hi,

On 07/10/16 22:22, Andrew Lunn wrote:
> Overall, this is much better. I just have a few nitpicks.
It is good to hear that we are getting closer ;-)

> dt-bindings/net/mscc-phy-vsc8531.h is removed by this patch. It would
> be good to also remove the reference.
My bad.

> You don't need edge_slowdown and vddmac in the private structure,
> since they are never used after determining what rate_magic is.
Year... I was actually a bit confused about this... But assumed that you had
some conventions about saving "input" configuration.

Well, clearly not - I will clean this up.

Actually, there is no need to store rate_magic either... It is only used in the
init function. This means that there is no need for private date...

The code could look somthing like this (it is not tested, so just look at see if
you like the idea):

#ifdef CONFIG_OF_MDIO
static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
{
	int rc;
	u8 sd;
	u16 vdd;
	struct device *dev = &phydev->mdio.dev;
	struct device_node *of_node = dev->of_node;
	u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);

	if (!of_node)
		return -ENODEV;

	rc = of_property_read_u16(of_node, "vsc8531,vddmac", &vdd);
	if (rc != 0)
		vdd = MSCC_VDDMAC_3300;

	rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown", &sd);
	if (rc != 0)
		sd = 0;

	for (vdd = 0; vdd < ARRAY_SIZE(edge_table); vdd++)
		if (edge_table[vdd].vddmac == vddmac)
			for (sd = 0; sd < sd_array_size; sd++)
				if (edge_table[vdd].slowdown[sd] == slowdown)
					return (sd_array_size - sd - 1);

	return -EINVAL;
}
#else
static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
{
	return 0;
}
#endif /* CONFIG_OF_MDIO */


static int vsc85xx_config_init(struct phy_device *phydev)
{
	int rc, rate_magic;
...
	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
	if (rate_magic < 0)
		return rate_magic;

	rc = vsc85xx_edge_rate_cntl_set(phydev, rate_magic);
	if (rc)
		return rc;
...
}

This is clearly how I would prefere it, as it would simplify the implementation.

It should be no problem to have this tested, and have a new patch avialable
tomorrow.

/Allan

Re: [PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs.

[Andrew Lunn]

> Year... I was actually a bit confused about this... But assumed that you had
> some conventions about saving "input" configuration.

Nope.

Humm, actually, i messed up here and missed something. Sorry.

You should really do all the DT parsing in the probe function, or a
function it calls. We want the probe to return -EINVAL if the device
tree is invalid. Either you can program the magic value immediately,
if it will survive a reset, or save it in the private structure until
the init is called.

   Andrew