[PATCH net-next 0/2] net: phy: Add WoL and Auto Mdix drivers for Microsemi PHYs.

[PATCH net-next 0/2] net: phy: Add WoL and Auto Mdix drivers for Microsemi PHYs.

[Raju Lakkaraju]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

This series adds support to the Wake-on-LAN and Auto/Mdi-x set drivers
for Microsemi PHYs.

Patch 1/2:
        Wake-on-LAN (WoL) is an Ethernet networking standard that allows
        a computer/device to be turned on or awakened by a network message.
        VSC8531 PHY can support this feature configure by driver set function.
        WoL status get by driver get function.

Patch 2/2:
        To connect two ports of the same configuration (MDI to MDI or
        MDI-X to MDI-X) with a 10/100/1000 Mbit/s connection, an
        Ethernet crossover cable is needed to cross over the transmit
        and receive signals in the cable, so that they are matched at
        the connector level.
        When connecting an MDI port to an MDI-X port a straight through
        cable is used while to connect two MDI ports or two MDI-X ports
        a crossover cable must be used. Conventionally MDI is used on end
        devices while MDI-X is used on hubs and switches

        Auto MDI-X automatically detects the required cable connection
        type and configures the connection appropriately, removing the
        need for crossover cables to interconnect switches or connecting
        PCs peer-to-peer.

        VSC8531 PHY supports Auto MDI-x, MDI and MDI-X configuraion by
        driver set loopback function.

Tested on Beaglebone Black with VSC 8531 PHY.

Raju Lakkaraju (2):
  net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
  net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

 drivers/net/phy/mscc.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 202 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.

[Raju Lakkaraju]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

Wake-on-LAN (WoL) is an Ethernet networking standard that allows
a computer/device to be turned on or awakened by a network message.
VSC8531 PHY can support this feature configure by driver set function.
WoL status get by driver get function.

Tested on Beaglebone Black with VSC 8531 PHY.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
---
 drivers/net/phy/mscc.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index d350deb..ca6ea23 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -11,6 +11,7 @@
 #include <linux/mdio.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/netdevice.h>
 
 enum rgmii_rx_clock_delay {
 	RGMII_RX_CLK_DELAY_0_2_NS = 0,
@@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
 
 #define MII_VSC85XX_INT_MASK		  25
 #define MII_VSC85XX_INT_MASK_MASK	  0xa000
+#define MII_VSC85XX_INT_MASK_WOL	  0x0040
 #define MII_VSC85XX_INT_STATUS		  26
 
 #define MSCC_EXT_PAGE_ACCESS		  31
@@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
 #define RGMII_RX_CLK_DELAY_MASK		  0x0070
 #define RGMII_RX_CLK_DELAY_POS		  4
 
+#define MSCC_PHY_WOL_LOWER_MAC_ADDR	  21
+#define MSCC_PHY_WOL_MID_MAC_ADDR	  22
+#define MSCC_PHY_WOL_UPPER_MAC_ADDR	  23
+#define MSCC_PHY_WOL_LOWER_PASSWD	  24
+#define MSCC_PHY_WOL_MID_PASSWD		  25
+#define MSCC_PHY_WOL_UPPER_PASSWD	  26
+
+#define MSCC_PHY_WOL_MAC_CONTROL	  27
+#define EDGE_RATE_CNTL_POS		  5
+#define EDGE_RATE_CNTL_MASK		  0x00E0
+#define SECURE_ON_ENABLE		  0x8000
+#define SECURE_ON_PASSWD_LEN_4		  0x4000
+
 /* Microsemi PHY ID's */
 #define PHY_ID_VSC8531			  0x00070570
 #define PHY_ID_VSC8541			  0x00070770
@@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 	return rc;
 }
 
+static int vsc85xx_wol_set(struct phy_device *phydev,
+			   struct ethtool_wolinfo *wol)
+{
+	int rc;
+	u16 reg_val;
+	struct ethtool_wolinfo *wol_conf = wol;
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc != 0)
+		goto out_unlock;
+
+	if (wol->wolopts & WAKE_MAGIC) {
+		/* Store the device address for the magic packet */
+		reg_val = phydev->attached_dev->dev_addr[4] << 8;
+		reg_val |= phydev->attached_dev->dev_addr[5];
+		phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
+		reg_val = phydev->attached_dev->dev_addr[2] << 8;
+		reg_val |= phydev->attached_dev->dev_addr[3];
+		phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
+		reg_val = phydev->attached_dev->dev_addr[0] << 8;
+		reg_val |= phydev->attached_dev->dev_addr[1];
+		phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
+	} else {
+		phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
+		phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
+		phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
+	}
+
+	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+	if (wol_conf->wolopts & WAKE_MAGICSECURE)
+		reg_val |= SECURE_ON_ENABLE;
+	else
+		reg_val &= ~SECURE_ON_ENABLE;
+	phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+
+	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
+		reg_val = wol_conf->sopass[4] << 8;
+		reg_val |= wol_conf->sopass[5];
+		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
+		reg_val = wol_conf->sopass[2] << 8;
+		reg_val |= wol_conf->sopass[3];
+		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
+		reg_val = wol_conf->sopass[0] << 8;
+		reg_val |= wol_conf->sopass[1];
+		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
+	} else {
+		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
+		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
+		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
+	}
+
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+	if (rc != 0)
+		goto out_unlock;
+
+	if (wol->wolopts & WAKE_MAGIC) {
+		/* Enable the WOL interrupt */
+		reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
+		reg_val |= MII_VSC85XX_INT_MASK_WOL;
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
+		if (rc != 0)
+			goto out_unlock;
+	} else {
+		/* Disable the WOL interrupt */
+		reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
+		reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
+		if (rc != 0)
+			goto out_unlock;
+	}
+	/* Clear WOL iterrupt status */
+	reg_val = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
+static void vsc85xx_wol_get(struct phy_device *phydev,
+			    struct ethtool_wolinfo *wol)
+{
+	int rc;
+	u16 reg_val;
+	struct ethtool_wolinfo *wol_conf = wol;
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc != 0)
+		goto out_unlock;
+
+	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+	if (reg_val & SECURE_ON_ENABLE)
+		wol_conf->wolopts |= WAKE_MAGICSECURE;
+	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
+		reg_val = phy_read(phydev, MSCC_PHY_WOL_LOWER_PASSWD);
+		wol_conf->sopass[5] = reg_val & 0x00ff;
+		wol_conf->sopass[4] = (reg_val & 0xff00) >> 8;
+		reg_val = phy_read(phydev, MSCC_PHY_WOL_MID_PASSWD);
+		wol_conf->sopass[3] = reg_val & 0x00ff;
+		wol_conf->sopass[2] = (reg_val & 0xff00) >> 8;
+		reg_val = phy_read(phydev, MSCC_PHY_WOL_UPPER_PASSWD);
+		wol_conf->sopass[1] = reg_val & 0x00ff;
+		wol_conf->sopass[0] = (reg_val & 0xff00) >> 8;
+	}
+
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+}
+
 static int vsc85xx_mac_if_set(struct phy_device *phydev,
 			      phy_interface_t interface)
 {
@@ -177,6 +305,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
+	.set_wol        = &vsc85xx_wol_set,
+	.get_wol        = &vsc85xx_wol_get,
 },
 {
 	.phy_id		= PHY_ID_VSC8541,
@@ -193,6 +323,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
+	.set_wol        = &vsc85xx_wol_set,
+	.get_wol        = &vsc85xx_wol_get,
 }
 
 };
-- 
2.7.4

[PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Raju Lakkaraju]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

To connect two ports of the same configuration (MDI to MDI or
MDI-X to MDI-X) with a 10/100/1000 Mbit/s connection, an
Ethernet crossover cable is needed to cross over the transmit
and receive signals in the cable, so that they are matched at
the connector level.
When connecting an MDI port to an MDI-X port a straight through
cable is used while to connect two MDI ports or two MDI-X ports
a crossover cable must be used. Conventionally MDI is used on end
devices while MDI-X is used on hubs and switches

Auto MDI-X automatically detects the required cable connection
type and configures the connection appropriately, removing the
need for crossover cables to interconnect switches or connecting
PCs peer-to-peer.

VSC8531 PHY supports Auto MDI-x, MDI and MDI-X configuraion by
driver set mdix function.

Tested on Beaglebone Black with VSC 8531 PHY.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
---
 drivers/net/phy/mscc.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index ca6ea23..dbf8434 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -26,6 +26,11 @@ enum rgmii_rx_clock_delay {
 
 /* Microsemi VSC85xx PHY registers */
 /* IEEE 802. Std Registers */
+#define MSCC_PHY_BYPASS_CONTROL		  18
+#define DISABLE_HP_AUTO_MDIX_MASK	  0x0080
+#define DISABLE_PAIR_SWAP_CORR_MASK	  0x0020
+#define DISABLE_POLARITY_CORR_MASK	  0x0010
+
 #define MSCC_PHY_EXT_PHY_CNTL_1           23
 #define MAC_IF_SELECTION_MASK             0x1800
 #define MAC_IF_SELECTION_GMII             0
@@ -41,8 +46,16 @@ enum rgmii_rx_clock_delay {
 
 #define MSCC_EXT_PAGE_ACCESS		  31
 #define MSCC_PHY_PAGE_STANDARD		  0x0000 /* Standard registers */
+#define MSCC_PHY_PAGE_EXTENDED		  0x0001 /* Extended registers */
 #define MSCC_PHY_PAGE_EXTENDED_2	  0x0002 /* Extended reg - page 2 */
 
+/* Extended Page 1 Registers */
+#define MSCC_PHY_EXT_MODE_CNTL		  19
+#define FORCE_MDI_CROSSOVER_MASK	  0x000C
+#define FORCE_MDI_CROSSOVER_MDIX	  0x000C
+#define FORCE_MDI_CROSSOVER_MDI		  0x0008
+#define FORCE_MDI_CROSSOVER_NORMAL	  0x0000
+
 /* Extended Page 2 Registers */
 #define MSCC_PHY_RGMII_CNTL		  20
 #define RGMII_RX_CLK_DELAY_MASK		  0x0070
@@ -73,6 +86,47 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 	return rc;
 }
 
+static int vsc85xx_mdix_set(struct phy_device *phydev,
+			    u8 mdix)
+{
+	int rc;
+	u16 reg_val;
+
+	reg_val = phy_read(phydev, MSCC_PHY_BYPASS_CONTROL);
+	if ((mdix == ETH_TP_MDI) || (mdix == ETH_TP_MDI_X)) {
+		reg_val |= (DISABLE_PAIR_SWAP_CORR_MASK |
+			    DISABLE_POLARITY_CORR_MASK  |
+			    DISABLE_HP_AUTO_MDIX_MASK);
+	} else {
+		reg_val &= ~(DISABLE_PAIR_SWAP_CORR_MASK |
+			     DISABLE_POLARITY_CORR_MASK  |
+			     DISABLE_HP_AUTO_MDIX_MASK);
+	}
+	rc = phy_write(phydev, MSCC_PHY_BYPASS_CONTROL, reg_val);
+	if (rc != 0)
+		goto out_unlock;
+
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
+	if (rc != 0)
+		goto out_unlock;
+
+	reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
+	reg_val &= ~(FORCE_MDI_CROSSOVER_MASK);
+	if (mdix == ETH_TP_MDI)
+		reg_val |= FORCE_MDI_CROSSOVER_MDI;
+	else if (mdix == ETH_TP_MDI_X)
+		reg_val |= FORCE_MDI_CROSSOVER_MDIX;
+	rc = phy_write(phydev, MSCC_PHY_EXT_MODE_CNTL, reg_val);
+	if (rc != 0)
+		goto out_unlock;
+
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+
+	return rc;
+}
+
 static int vsc85xx_wol_set(struct phy_device *phydev,
 			   struct ethtool_wolinfo *wol)
 {
@@ -227,6 +281,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
 	int rc;
 	u16 reg_val;
 
+	phydev->mdix = ETH_TP_MDI_AUTO;
 	mutex_lock(&phydev->lock);
 	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
 	if (rc != 0)
@@ -288,6 +343,19 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
 	return rc;
 }
 
+static int vsc85xx_config_aneg(struct phy_device *phydev)
+{
+	int rc;
+
+	rc = vsc85xx_mdix_set(phydev, phydev->mdix);
+	if (rc < 0)
+		return rc;
+
+	rc = genphy_config_aneg(phydev);
+
+	return rc;
+}
+
 /* Microsemi VSC85xx PHYs */
 static struct phy_driver vsc85xx_driver[] = {
 {
@@ -298,7 +366,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.flags		= PHY_HAS_INTERRUPT,
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc85xx_config_init,
-	.config_aneg    = &genphy_config_aneg,
+	.config_aneg    = &vsc85xx_config_aneg,
 	.aneg_done	= &genphy_aneg_done,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
@@ -316,7 +384,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.flags		= PHY_HAS_INTERRUPT,
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc85xx_config_init,
-	.config_aneg    = &genphy_config_aneg,
+	.config_aneg    = &vsc85xx_config_aneg,
 	.aneg_done	= &genphy_aneg_done,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
-- 
2.7.4

Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.

[Andrew Lunn]

> +#define MSCC_PHY_WOL_MAC_CONTROL	  27
> +#define EDGE_RATE_CNTL_POS		  5
> +#define EDGE_RATE_CNTL_MASK		  0x00E0

This patch does not require these two #defines.

Please indicate in the cover note if the patches depends on other
patches in order to cleanly apply. Or if these patches are going to
conflict with some other patches.

> +	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> +	if (wol_conf->wolopts & WAKE_MAGICSECURE)
> +		reg_val |= SECURE_ON_ENABLE;
> +	else
> +		reg_val &= ~SECURE_ON_ENABLE;
> +	phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> +
> +	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> +		reg_val = wol_conf->sopass[4] << 8;
> +		reg_val |= wol_conf->sopass[5];
> +		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> +		reg_val = wol_conf->sopass[2] << 8;
> +		reg_val |= wol_conf->sopass[3];
> +		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> +		reg_val = wol_conf->sopass[0] << 8;
> +		reg_val |= wol_conf->sopass[1];
> +		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> +	} else {
> +		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> +		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> +		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> +	}

Wouldn't it be better to set the password, and then enable the
password feature?

I don't know much about WOL. Hopefully Florian will add further
comments.

	 Andrew

Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.

[Florian Fainelli]

On 09/28/2016 05:01 AM, Raju Lakkaraju wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> 
> Wake-on-LAN (WoL) is an Ethernet networking standard that allows
> a computer/device to be turned on or awakened by a network message.
> VSC8531 PHY can support this feature configure by driver set function.
> WoL status get by driver get function.
> 
> Tested on Beaglebone Black with VSC 8531 PHY.
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> ---
>  drivers/net/phy/mscc.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 132 insertions(+)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index d350deb..ca6ea23 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -11,6 +11,7 @@
>  #include <linux/mdio.h>
>  #include <linux/mii.h>
>  #include <linux/phy.h>
> +#include <linux/netdevice.h>
>  
>  enum rgmii_rx_clock_delay {
>  	RGMII_RX_CLK_DELAY_0_2_NS = 0,
> @@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
>  
>  #define MII_VSC85XX_INT_MASK		  25
>  #define MII_VSC85XX_INT_MASK_MASK	  0xa000
> +#define MII_VSC85XX_INT_MASK_WOL	  0x0040
>  #define MII_VSC85XX_INT_STATUS		  26
>  
>  #define MSCC_EXT_PAGE_ACCESS		  31
> @@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
>  #define RGMII_RX_CLK_DELAY_MASK		  0x0070
>  #define RGMII_RX_CLK_DELAY_POS		  4
>  
> +#define MSCC_PHY_WOL_LOWER_MAC_ADDR	  21
> +#define MSCC_PHY_WOL_MID_MAC_ADDR	  22
> +#define MSCC_PHY_WOL_UPPER_MAC_ADDR	  23
> +#define MSCC_PHY_WOL_LOWER_PASSWD	  24
> +#define MSCC_PHY_WOL_MID_PASSWD		  25
> +#define MSCC_PHY_WOL_UPPER_PASSWD	  26
> +
> +#define MSCC_PHY_WOL_MAC_CONTROL	  27
> +#define EDGE_RATE_CNTL_POS		  5
> +#define EDGE_RATE_CNTL_MASK		  0x00E0
> +#define SECURE_ON_ENABLE		  0x8000
> +#define SECURE_ON_PASSWD_LEN_4		  0x4000
> +
>  /* Microsemi PHY ID's */
>  #define PHY_ID_VSC8531			  0x00070570
>  #define PHY_ID_VSC8541			  0x00070770
> @@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
>  	return rc;
>  }
>  
> +static int vsc85xx_wol_set(struct phy_device *phydev,
> +			   struct ethtool_wolinfo *wol)
> +{
> +	int rc;
> +	u16 reg_val;
> +	struct ethtool_wolinfo *wol_conf = wol;
> +
> +	mutex_lock(&phydev->lock);

This mutex is used here because you are using an indirect page access,
right? This is not to protect against multiple calls of wol_set from
different executing threads?

> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	if (wol->wolopts & WAKE_MAGIC) {
> +		/* Store the device address for the magic packet */
> +		reg_val = phydev->attached_dev->dev_addr[4] << 8;
> +		reg_val |= phydev->attached_dev->dev_addr[5];
> +		phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
> +		reg_val = phydev->attached_dev->dev_addr[2] << 8;
> +		reg_val |= phydev->attached_dev->dev_addr[3];
> +		phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
> +		reg_val = phydev->attached_dev->dev_addr[0] << 8;
> +		reg_val |= phydev->attached_dev->dev_addr[1];
> +		phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
> +	} else {
> +		phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
> +		phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
> +		phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
> +	}
> +
> +	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> +	if (wol_conf->wolopts & WAKE_MAGICSECURE)
> +		reg_val |= SECURE_ON_ENABLE;
> +	else
> +		reg_val &= ~SECURE_ON_ENABLE;
> +	phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> +
> +	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> +		reg_val = wol_conf->sopass[4] << 8;
> +		reg_val |= wol_conf->sopass[5];
> +		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> +		reg_val = wol_conf->sopass[2] << 8;
> +		reg_val |= wol_conf->sopass[3];
> +		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> +		reg_val = wol_conf->sopass[0] << 8;
> +		reg_val |= wol_conf->sopass[1];
> +		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> +	} else {
> +		phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> +		phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> +		phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> +	}

How about making the code a little simpler in both cases with something
like this the following:

	u16 pwd = { };
	unsigned int i;

	if (wol_conf->wolopts & WAKE_MAGICECURE)
		for (i = 0; i < ARRAY_SIZE(pwd); i++)
			pwd[i] = wol_conf->so_pass[5 - (i * 2 + 1)] << 8|
				 wol_conf->so_pass[5 - i * 2 ]

	phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, pwd[0]);
	phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, pwd[1]);
	phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, pwd[2]);

> +
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	if (wol->wolopts & WAKE_MAGIC) {

Don't you also need to check WAKE_MAGICSECURE here as well? Or is the
interrupt going to be generated only if there is no password defined?

> +		/* Enable the WOL interrupt */
> +		reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
> +		reg_val |= MII_VSC85XX_INT_MASK_WOL;
> +		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
> +		if (rc != 0)
> +			goto out_unlock;
> +	} else {
> +		/* Disable the WOL interrupt */
> +		reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
> +		reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
> +		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
> +		if (rc != 0)
> +			goto out_unlock;
> +	}
> +	/* Clear WOL iterrupt status */
> +	reg_val = phy_read(phydev, MII_VSC85XX_INT_STATUS);
> +
> +out_unlock:
> +	mutex_unlock(&phydev->lock);
> +
> +	return rc;
> +}
> +
> +static void vsc85xx_wol_get(struct phy_device *phydev,
> +			    struct ethtool_wolinfo *wol)
> +{
> +	int rc;
> +	u16 reg_val;
> +	struct ethtool_wolinfo *wol_conf = wol;
> +
> +	mutex_lock(&phydev->lock);
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> +	if (reg_val & SECURE_ON_ENABLE)
> +		wol_conf->wolopts |= WAKE_MAGICSECURE;
> +	if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> +		reg_val = phy_read(phydev, MSCC_PHY_WOL_LOWER_PASSWD);
> +		wol_conf->sopass[5] = reg_val & 0x00ff;
> +		wol_conf->sopass[4] = (reg_val & 0xff00) >> 8;
> +		reg_val = phy_read(phydev, MSCC_PHY_WOL_MID_PASSWD);
> +		wol_conf->sopass[3] = reg_val & 0x00ff;
> +		wol_conf->sopass[2] = (reg_val & 0xff00) >> 8;
> +		reg_val = phy_read(phydev, MSCC_PHY_WOL_UPPER_PASSWD);
> +		wol_conf->sopass[1] = reg_val & 0x00ff;
> +		wol_conf->sopass[0] = (reg_val & 0xff00) >> 8;
> +	}
> +
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:
> +	mutex_unlock(&phydev->lock);
> +}
> +
>  static int vsc85xx_mac_if_set(struct phy_device *phydev,
>  			      phy_interface_t interface)
>  {
> @@ -177,6 +305,8 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.config_intr    = &vsc85xx_config_intr,
>  	.suspend	= &genphy_suspend,
>  	.resume		= &genphy_resume,
> +	.set_wol        = &vsc85xx_wol_set,
> +	.get_wol        = &vsc85xx_wol_get,
>  },
>  {
>  	.phy_id		= PHY_ID_VSC8541,
> @@ -193,6 +323,8 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.config_intr    = &vsc85xx_config_intr,
>  	.suspend	= &genphy_suspend,
>  	.resume		= &genphy_resume,
> +	.set_wol        = &vsc85xx_wol_set,
> +	.get_wol        = &vsc85xx_wol_get,
>  }
>  
>  };
> 


-- 
Florian

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Andrew Lunn]

> +	reg_val = phy_read(phydev, MSCC_PHY_BYPASS_CONTROL);
> +	if ((mdix == ETH_TP_MDI) || (mdix == ETH_TP_MDI_X)) {
> +		reg_val |= (DISABLE_PAIR_SWAP_CORR_MASK |
> +			    DISABLE_POLARITY_CORR_MASK  |
> +			    DISABLE_HP_AUTO_MDIX_MASK);
> +	} else {
> +		reg_val &= ~(DISABLE_PAIR_SWAP_CORR_MASK |
> +			     DISABLE_POLARITY_CORR_MASK  |
> +			     DISABLE_HP_AUTO_MDIX_MASK);
> +	}
> +	rc = phy_write(phydev, MSCC_PHY_BYPASS_CONTROL, reg_val);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
> +	reg_val &= ~(FORCE_MDI_CROSSOVER_MASK);
> +	if (mdix == ETH_TP_MDI)
> +		reg_val |= FORCE_MDI_CROSSOVER_MDI;
> +	else if (mdix == ETH_TP_MDI_X)
> +		reg_val |= FORCE_MDI_CROSSOVER_MDIX;
> +	rc = phy_write(phydev, MSCC_PHY_EXT_MODE_CNTL, reg_val);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:

out_unlock seems a bit of an odd name, since you are not unlocking
anything.

I also wonder if you should try to reset to MSCC_PHY_PAGE_STANDARD in
the error condition?

> +
> +	return rc;
> +}
> +
>  static int vsc85xx_wol_set(struct phy_device *phydev,
>  			   struct ethtool_wolinfo *wol)
>  {
> @@ -227,6 +281,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
>  	int rc;
>  	u16 reg_val;
>  
> +	phydev->mdix = ETH_TP_MDI_AUTO;

Humm, interesting. The only other driver supporting mdix is the
Marvell one. It does not do this, it leaves it to its default value of
ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
meaning as ETH_TP_MDI_AUTO.

There needs to be consistency here. You either need to do the same as
the Marvell driver, or you need to modify the Marvell driver to also
set phydev->mdix to ETH_TP_MDI_AUTO.

I don't yet know which of these two is the right thing to do.

Florian?

	Andrew

Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Andrew,

Thank you for code review and valuable comments.

On Wed, Sep 28, 2016 at 06:27:05PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
> 
> 
> > +#define MSCC_PHY_WOL_MAC_CONTROL       27
> > +#define EDGE_RATE_CNTL_POS             5
> > +#define EDGE_RATE_CNTL_MASK            0x00E0
> 
> This patch does not require these two #defines.
> 
> Please indicate in the cover note if the patches depends on other
> patches in order to cleanly apply. Or if these patches are going to
> conflict with some other patches.
> 

Accepted. I will remove those 2 defines.

> > +     reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> > +     if (wol_conf->wolopts & WAKE_MAGICSECURE)
> > +             reg_val |= SECURE_ON_ENABLE;
> > +     else
> > +             reg_val &= ~SECURE_ON_ENABLE;
> > +     phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> > +
> > +     if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> > +             reg_val = wol_conf->sopass[4] << 8;
> > +             reg_val |= wol_conf->sopass[5];
> > +             phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> > +             reg_val = wol_conf->sopass[2] << 8;
> > +             reg_val |= wol_conf->sopass[3];
> > +             phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> > +             reg_val = wol_conf->sopass[0] << 8;
> > +             reg_val |= wol_conf->sopass[1];
> > +             phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> > +     } else {
> > +             phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> > +             phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> > +             phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> > +     }
> 
> Wouldn't it be better to set the password, and then enable the
> password feature?
> 

Accepted. I will change.

> I don't know much about WOL. Hopefully Florian will add further
> comments.
> 
>          Andrew

---
Thanks,
Raju.

Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Florian,

Thank you for code review and valuable comments.


On Wed, Sep 28, 2016 at 10:37:07AM -0700, Florian Fainelli wrote:
> EXTERNAL EMAIL
> 
> 
> On 09/28/2016 05:01 AM, Raju Lakkaraju wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Wake-on-LAN (WoL) is an Ethernet networking standard that allows
> > a computer/device to be turned on or awakened by a network message.
> > VSC8531 PHY can support this feature configure by driver set function.
> > WoL status get by driver get function.
> >
> > Tested on Beaglebone Black with VSC 8531 PHY.
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> > ---
> >  drivers/net/phy/mscc.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 132 insertions(+)
> >
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index d350deb..ca6ea23 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/mdio.h>
> >  #include <linux/mii.h>
> >  #include <linux/phy.h>
> > +#include <linux/netdevice.h>
> >
> >  enum rgmii_rx_clock_delay {
> >       RGMII_RX_CLK_DELAY_0_2_NS = 0,
> > @@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
> >
> >  #define MII_VSC85XX_INT_MASK           25
> >  #define MII_VSC85XX_INT_MASK_MASK      0xa000
> > +#define MII_VSC85XX_INT_MASK_WOL       0x0040
> >  #define MII_VSC85XX_INT_STATUS                 26
> >
> >  #define MSCC_EXT_PAGE_ACCESS           31
> > @@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
> >  #define RGMII_RX_CLK_DELAY_MASK                0x0070
> >  #define RGMII_RX_CLK_DELAY_POS                 4
> >
> > +#define MSCC_PHY_WOL_LOWER_MAC_ADDR    21
> > +#define MSCC_PHY_WOL_MID_MAC_ADDR      22
> > +#define MSCC_PHY_WOL_UPPER_MAC_ADDR    23
> > +#define MSCC_PHY_WOL_LOWER_PASSWD      24
> > +#define MSCC_PHY_WOL_MID_PASSWD                25
> > +#define MSCC_PHY_WOL_UPPER_PASSWD      26
> > +
> > +#define MSCC_PHY_WOL_MAC_CONTROL       27
> > +#define EDGE_RATE_CNTL_POS             5
> > +#define EDGE_RATE_CNTL_MASK            0x00E0
> > +#define SECURE_ON_ENABLE               0x8000
> > +#define SECURE_ON_PASSWD_LEN_4                 0x4000
> > +
> >  /* Microsemi PHY ID's */
> >  #define PHY_ID_VSC8531                         0x00070570
> >  #define PHY_ID_VSC8541                         0x00070770
> > @@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
> >       return rc;
> >  }
> >
> > +static int vsc85xx_wol_set(struct phy_device *phydev,
> > +                        struct ethtool_wolinfo *wol)
> > +{
> > +     int rc;
> > +     u16 reg_val;
> > +     struct ethtool_wolinfo *wol_conf = wol;
> > +
> > +     mutex_lock(&phydev->lock);
> 
> This mutex is used here because you are using an indirect page access,
> right? This is not to protect against multiple calls of wol_set from
> different executing threads?
> 

Correct. mutex is used for indirect page access.

I did find any protect against multiple calls of wol_set in other vendors.
Do you have any suggestions?

> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     if (wol->wolopts & WAKE_MAGIC) {
> > +             /* Store the device address for the magic packet */
> > +             reg_val = phydev->attached_dev->dev_addr[4] << 8;
> > +             reg_val |= phydev->attached_dev->dev_addr[5];
> > +             phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
> > +             reg_val = phydev->attached_dev->dev_addr[2] << 8;
> > +             reg_val |= phydev->attached_dev->dev_addr[3];
> > +             phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
> > +             reg_val = phydev->attached_dev->dev_addr[0] << 8;
> > +             reg_val |= phydev->attached_dev->dev_addr[1];
> > +             phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
> > +     } else {
> > +             phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
> > +             phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
> > +             phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
> > +     }
> > +
> > +     reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> > +     if (wol_conf->wolopts & WAKE_MAGICSECURE)
> > +             reg_val |= SECURE_ON_ENABLE;
> > +     else
> > +             reg_val &= ~SECURE_ON_ENABLE;
> > +     phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> > +
> > +     if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> > +             reg_val = wol_conf->sopass[4] << 8;
> > +             reg_val |= wol_conf->sopass[5];
> > +             phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> > +             reg_val = wol_conf->sopass[2] << 8;
> > +             reg_val |= wol_conf->sopass[3];
> > +             phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> > +             reg_val = wol_conf->sopass[0] << 8;
> > +             reg_val |= wol_conf->sopass[1];
> > +             phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> > +     } else {
> > +             phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> > +             phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> > +             phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> > +     }
> 
> How about making the code a little simpler in both cases with something
> like this the following:
> 
>         u16 pwd = { };
>         unsigned int i;
> 
>         if (wol_conf->wolopts & WAKE_MAGICECURE)
>                 for (i = 0; i < ARRAY_SIZE(pwd); i++)
>                         pwd[i] = wol_conf->so_pass[5 - (i * 2 + 1)] << 8|
>                                  wol_conf->so_pass[5 - i * 2 ]
> 
>         phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, pwd[0]);
>         phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, pwd[1]);
>         phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, pwd[2]);
> 

Accepted. I will change.

> > +
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     if (wol->wolopts & WAKE_MAGIC) {
> 
> Don't you also need to check WAKE_MAGICSECURE here as well? Or is the
> interrupt going to be generated only if there is no password defined?
> 

Interrupt is going to be generated when WAKE_MAGIC.
WAKE_MAGICSECURE is extra protection.

> > +             /* Enable the WOL interrupt */
> > +             reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
> > +             reg_val |= MII_VSC85XX_INT_MASK_WOL;
> > +             rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
> > +             if (rc != 0)
> > +                     goto out_unlock;
> > +     } else {
> > +             /* Disable the WOL interrupt */
> > +             reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
> > +             reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
> > +             rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
> > +             if (rc != 0)
> > +                     goto out_unlock;
> > +     }
> > +     /* Clear WOL iterrupt status */
> > +     reg_val = phy_read(phydev, MII_VSC85XX_INT_STATUS);
> > +
> > +out_unlock:
> > +     mutex_unlock(&phydev->lock);
> > +
> > +     return rc;
> > +}
> > +
> > +static void vsc85xx_wol_get(struct phy_device *phydev,
> > +                         struct ethtool_wolinfo *wol)
> > +{
> > +     int rc;
> > +     u16 reg_val;
> > +     struct ethtool_wolinfo *wol_conf = wol;
> > +
> > +     mutex_lock(&phydev->lock);
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> > +     if (reg_val & SECURE_ON_ENABLE)
> > +             wol_conf->wolopts |= WAKE_MAGICSECURE;
> > +     if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> > +             reg_val = phy_read(phydev, MSCC_PHY_WOL_LOWER_PASSWD);
> > +             wol_conf->sopass[5] = reg_val & 0x00ff;
> > +             wol_conf->sopass[4] = (reg_val & 0xff00) >> 8;
> > +             reg_val = phy_read(phydev, MSCC_PHY_WOL_MID_PASSWD);
> > +             wol_conf->sopass[3] = reg_val & 0x00ff;
> > +             wol_conf->sopass[2] = (reg_val & 0xff00) >> 8;
> > +             reg_val = phy_read(phydev, MSCC_PHY_WOL_UPPER_PASSWD);
> > +             wol_conf->sopass[1] = reg_val & 0x00ff;
> > +             wol_conf->sopass[0] = (reg_val & 0xff00) >> 8;
> > +     }
> > +
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> > +
> > +out_unlock:
> > +     mutex_unlock(&phydev->lock);
> > +}
> > +
> >  static int vsc85xx_mac_if_set(struct phy_device *phydev,
> >                             phy_interface_t interface)
> >  {
> > @@ -177,6 +305,8 @@ static struct phy_driver vsc85xx_driver[] = {
> >       .config_intr    = &vsc85xx_config_intr,
> >       .suspend        = &genphy_suspend,
> >       .resume         = &genphy_resume,
> > +     .set_wol        = &vsc85xx_wol_set,
> > +     .get_wol        = &vsc85xx_wol_get,
> >  },
> >  {
> >       .phy_id         = PHY_ID_VSC8541,
> > @@ -193,6 +323,8 @@ static struct phy_driver vsc85xx_driver[] = {
> >       .config_intr    = &vsc85xx_config_intr,
> >       .suspend        = &genphy_suspend,
> >       .resume         = &genphy_resume,
> > +     .set_wol        = &vsc85xx_wol_set,
> > +     .get_wol        = &vsc85xx_wol_get,
> >  }
> >
> >  };
> >
> 
> 
> --
> Florian


---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Andrew,

Thank you for code review and valuable comments.

On Wed, Sep 28, 2016 at 10:24:51PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
> 
> 
> > +     reg_val = phy_read(phydev, MSCC_PHY_BYPASS_CONTROL);
> > +     if ((mdix == ETH_TP_MDI) || (mdix == ETH_TP_MDI_X)) {
> > +             reg_val |= (DISABLE_PAIR_SWAP_CORR_MASK |
> > +                         DISABLE_POLARITY_CORR_MASK  |
> > +                         DISABLE_HP_AUTO_MDIX_MASK);
> > +     } else {
> > +             reg_val &= ~(DISABLE_PAIR_SWAP_CORR_MASK |
> > +                          DISABLE_POLARITY_CORR_MASK  |
> > +                          DISABLE_HP_AUTO_MDIX_MASK);
> > +     }
> > +     rc = phy_write(phydev, MSCC_PHY_BYPASS_CONTROL, reg_val);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
> > +     reg_val &= ~(FORCE_MDI_CROSSOVER_MASK);
> > +     if (mdix == ETH_TP_MDI)
> > +             reg_val |= FORCE_MDI_CROSSOVER_MDI;
> > +     else if (mdix == ETH_TP_MDI_X)
> > +             reg_val |= FORCE_MDI_CROSSOVER_MDIX;
> > +     rc = phy_write(phydev, MSCC_PHY_EXT_MODE_CNTL, reg_val);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> > +
> > +out_unlock:
> 
> out_unlock seems a bit of an odd name, since you are not unlocking
> anything.
> 

It's my mistake. Mutex should be there. I will add mutex.

> I also wonder if you should try to reset to MSCC_PHY_PAGE_STANDARD in
> the error condition?
> 

> > +
> > +     return rc;
> > +}
> > +
> >  static int vsc85xx_wol_set(struct phy_device *phydev,
> >                          struct ethtool_wolinfo *wol)
> >  {
> > @@ -227,6 +281,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
> >       int rc;
> >       u16 reg_val;
> >
> > +     phydev->mdix = ETH_TP_MDI_AUTO;
> 
> Humm, interesting. The only other driver supporting mdix is the
> Marvell one. It does not do this, it leaves it to its default value of
> ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
> meaning as ETH_TP_MDI_AUTO.
> 
> There needs to be consistency here. You either need to do the same as
> the Marvell driver, or you need to modify the Marvell driver to also
> set phydev->mdix to ETH_TP_MDI_AUTO.
> 
In Ethtool two variable i.e. eth_tp_mdix_ctrl, eth_tp_mdix use to update
the status. But, driver header is having one variable i.e. mdix.
Driver header should also have another variabl like mdix_ctrl.
Then, Ethtool can get/set the Auto MDIX/MDI.
In case, mdix is not configure with ETH_TP_MDI_AUTO, Ethtool shows error as
"setting MDI not supported"

Please suggest me if you have any better method to fix this issue.

> I don't yet know which of these two is the right thing to do.
> 
> Florian?
> 
>         Andrew

---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Andrew Lunn]

> > > +     phydev->mdix = ETH_TP_MDI_AUTO;
> > 
> > Humm, interesting. The only other driver supporting mdix is the
> > Marvell one. It does not do this, it leaves it to its default value of
> > ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
> > meaning as ETH_TP_MDI_AUTO.
> > 
> > There needs to be consistency here. You either need to do the same as
> > the Marvell driver, or you need to modify the Marvell driver to also
> > set phydev->mdix to ETH_TP_MDI_AUTO.
> > 
> In Ethtool two variable i.e. eth_tp_mdix_ctrl, eth_tp_mdix use to update
> the status. But, driver header is having one variable i.e. mdix.
> Driver header should also have another variabl like mdix_ctrl.
> Then, Ethtool can get/set the Auto MDIX/MDI.
> In case, mdix is not configure with ETH_TP_MDI_AUTO, Ethtool shows error as
> "setting MDI not supported"
> 
> Please suggest me if you have any better method to fix this issue.

Maybe we should add a new flag for the .flags member of the
phy_driver. If PHY_HAS_MDIX is set, the phy core will set phydev->mdix
to ETH_TP_MDI_AUTO?

Florian, what do you think?

	 Andrew

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Florian Fainelli]

On 09/28/2016 01:24 PM, Andrew Lunn wrote:
>>  static int vsc85xx_wol_set(struct phy_device *phydev,
>>  			   struct ethtool_wolinfo *wol)
>>  {
>> @@ -227,6 +281,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
>>  	int rc;
>>  	u16 reg_val;
>>  
>> +	phydev->mdix = ETH_TP_MDI_AUTO;
> 
> Humm, interesting. The only other driver supporting mdix is the
> Marvell one. It does not do this, it leaves it to its default value of
> ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
> meaning as ETH_TP_MDI_AUTO.
> 
> There needs to be consistency here. You either need to do the same as
> the Marvell driver, or you need to modify the Marvell driver to also
> set phydev->mdix to ETH_TP_MDI_AUTO.
> 
> I don't yet know which of these two is the right thing to do.
> 
> Florian?

It's really hard to tell because the other drivers I looked at do not
necessarily seem to be consistent either. Here, if the MDI status is
really auto, then this is the correct value to return, if it is unknown,
it should be ETH_TP_MDI_INVALID.

For the Marvell PHY, it sounds like we should be able to determine what
was configured and return the correct MDI status value
-- 
Florian

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Florian Fainelli]

On 10/05/2016 12:18 AM, Andrew Lunn wrote:
>>>> +     phydev->mdix = ETH_TP_MDI_AUTO;
>>>
>>> Humm, interesting. The only other driver supporting mdix is the
>>> Marvell one. It does not do this, it leaves it to its default value of
>>> ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
>>> meaning as ETH_TP_MDI_AUTO.
>>>
>>> There needs to be consistency here. You either need to do the same as
>>> the Marvell driver, or you need to modify the Marvell driver to also
>>> set phydev->mdix to ETH_TP_MDI_AUTO.
>>>
>> In Ethtool two variable i.e. eth_tp_mdix_ctrl, eth_tp_mdix use to update
>> the status. But, driver header is having one variable i.e. mdix.
>> Driver header should also have another variabl like mdix_ctrl.
>> Then, Ethtool can get/set the Auto MDIX/MDI.
>> In case, mdix is not configure with ETH_TP_MDI_AUTO, Ethtool shows error as
>> "setting MDI not supported"

Agreed, we currently report eth_tp_mdi and eth_tp_mdi_ctrl using
phydev->mdix, but this is too limiting.

>>
>> Please suggest me if you have any better method to fix this issue.
> 
> Maybe we should add a new flag for the .flags member of the
> phy_driver. If PHY_HAS_MDIX is set, the phy core will set phydev->mdix
> to ETH_TP_MDI_AUTO?

I agree with Raju here, like most other Ethernet drivers, we should
allow PHY drivers to have an eth_tp_mdix_ctrl to indicate what is the
configured MDI setting, and read eth_tp_mdi to indicate what is the
current status, then ethtool can properly differentiate what is going on.

Raju, Andrew, does that work for you?
-- 
Florian

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Florian/Andrew,

Thank you for review comments.

On Thu, Oct 06, 2016 at 03:57:32AM -0700, Florian Fainelli wrote:
> EXTERNAL EMAIL
> 
> 
> On 09/28/2016 01:24 PM, Andrew Lunn wrote:
> >>  static int vsc85xx_wol_set(struct phy_device *phydev,
> >>                         struct ethtool_wolinfo *wol)
> >>  {
> >> @@ -227,6 +281,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
> >>      int rc;
> >>      u16 reg_val;
> >>
> >> +    phydev->mdix = ETH_TP_MDI_AUTO;
> >
> > Humm, interesting. The only other driver supporting mdix is the
> > Marvell one. It does not do this, it leaves it to its default value of
> > ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
> > meaning as ETH_TP_MDI_AUTO.
> >
> > There needs to be consistency here. You either need to do the same as
> > the Marvell driver, or you need to modify the Marvell driver to also
> > set phydev->mdix to ETH_TP_MDI_AUTO.
> >
> > I don't yet know which of these two is the right thing to do.
> >
> > Florian?
> 
> It's really hard to tell because the other drivers I looked at do not
> necessarily seem to be consistent either. Here, if the MDI status is
> really auto, then this is the correct value to return, if it is unknown,
> it should be ETH_TP_MDI_INVALID.
> 

In mdix get status function, This value will be update as per 
PHY mdix current status.

Shall i configure "phydev->mdix = ETH_TP_MDI_AUTO" as default ?

Andrew, do you have any comments?

> For the Marvell PHY, it sounds like we should be able to determine what
> was configured and return the correct MDI status value
> --
> Florian

---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Florian/Andrew,

Thank you for review comments.

On Thu, Oct 06, 2016 at 04:09:56AM -0700, Florian Fainelli wrote:
> EXTERNAL EMAIL
> 
> 
> On 10/05/2016 12:18 AM, Andrew Lunn wrote:
> >>>> +     phydev->mdix = ETH_TP_MDI_AUTO;
> >>>
> >>> Humm, interesting. The only other driver supporting mdix is the
> >>> Marvell one. It does not do this, it leaves it to its default value of
> >>> ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
> >>> meaning as ETH_TP_MDI_AUTO.
> >>>
> >>> There needs to be consistency here. You either need to do the same as
> >>> the Marvell driver, or you need to modify the Marvell driver to also
> >>> set phydev->mdix to ETH_TP_MDI_AUTO.
> >>>
> >> In Ethtool two variable i.e. eth_tp_mdix_ctrl, eth_tp_mdix use to update
> >> the status. But, driver header is having one variable i.e. mdix.
> >> Driver header should also have another variabl like mdix_ctrl.
> >> Then, Ethtool can get/set the Auto MDIX/MDI.
> >> In case, mdix is not configure with ETH_TP_MDI_AUTO, Ethtool shows error as
> >> "setting MDI not supported"
> 
> Agreed, we currently report eth_tp_mdi and eth_tp_mdi_ctrl using
> phydev->mdix, but this is too limiting.
> 
> >>
> >> Please suggest me if you have any better method to fix this issue.
> >
> > Maybe we should add a new flag for the .flags member of the
> > phy_driver. If PHY_HAS_MDIX is set, the phy core will set phydev->mdix
> > to ETH_TP_MDI_AUTO?
> 
> I agree with Raju here, like most other Ethernet drivers, we should
> allow PHY drivers to have an eth_tp_mdix_ctrl to indicate what is the
> configured MDI setting, and read eth_tp_mdi to indicate what is the
> current status, then ethtool can properly differentiate what is going on.
> 

Andrew, Do you want me to add new flag (mdix_ctrl) or keep it as it is?
These changes are more relevant for mdix get status function.
Do you want to me implement along with mdix get status function? 

> Raju, Andrew, does that work for you?
> --
> Florian

---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Andrew,

Thank you for code review and valuable comments.

On Wed, Sep 28, 2016 at 10:24:51PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
> 
> 
> > +     reg_val = phy_read(phydev, MSCC_PHY_BYPASS_CONTROL);
> > +     if ((mdix == ETH_TP_MDI) || (mdix == ETH_TP_MDI_X)) {
> > +             reg_val |= (DISABLE_PAIR_SWAP_CORR_MASK |
> > +                         DISABLE_POLARITY_CORR_MASK  |
> > +                         DISABLE_HP_AUTO_MDIX_MASK);
> > +     } else {
> > +             reg_val &= ~(DISABLE_PAIR_SWAP_CORR_MASK |
> > +                          DISABLE_POLARITY_CORR_MASK  |
> > +                          DISABLE_HP_AUTO_MDIX_MASK);
> > +     }
> > +     rc = phy_write(phydev, MSCC_PHY_BYPASS_CONTROL, reg_val);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
> > +     reg_val &= ~(FORCE_MDI_CROSSOVER_MASK);
> > +     if (mdix == ETH_TP_MDI)
> > +             reg_val |= FORCE_MDI_CROSSOVER_MDI;
> > +     else if (mdix == ETH_TP_MDI_X)
> > +             reg_val |= FORCE_MDI_CROSSOVER_MDIX;
> > +     rc = phy_write(phydev, MSCC_PHY_EXT_MODE_CNTL, reg_val);
> > +     if (rc != 0)
> > +             goto out_unlock;
> > +
> > +     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> > +
> > +out_unlock:
> 
> out_unlock seems a bit of an odd name, since you are not unlocking
> anything.
> 

Accepted. I will change.

> I also wonder if you should try to reset to MSCC_PHY_PAGE_STANDARD in
> the error condition?
> 

In case of PHY write error, then PHY page set will also return error.
I would like to return error condition, so return after write failure.

> > +
> > +     return rc;
> > +}
> > +
> >  static int vsc85xx_wol_set(struct phy_device *phydev,
> >                          struct ethtool_wolinfo *wol)
> >  {
> > @@ -227,6 +281,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
> >       int rc;
> >       u16 reg_val;
> >
> > +     phydev->mdix = ETH_TP_MDI_AUTO;
> 
> Humm, interesting. The only other driver supporting mdix is the
> Marvell one. It does not do this, it leaves it to its default value of
> ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
> meaning as ETH_TP_MDI_AUTO.
> 
> There needs to be consistency here. You either need to do the same as
> the Marvell driver, or you need to modify the Marvell driver to also
> set phydev->mdix to ETH_TP_MDI_AUTO.
> 
> I don't yet know which of these two is the right thing to do.
> 
> Florian?
> 
>         Andrew

For this comment, I responded in another thread.

---
Thanks,
Raju.