net: phy: DP83822 initial driver submission

Message ID 20171003155316.12312-1-dmurphy@ti.com
State New
Headers show
Series
  • net: phy: DP83822 initial driver submission
Related show

Commit Message

Dan Murphy Oct. 3, 2017, 3:53 p.m.
Add support for the TI  DP83822 10/100Mbit ethernet phy.

The DP83822 provides flexibility to connect to a MAC through a
standard MII, RMII or RGMII interface.

Datasheet:
http://www.ti.com/product/DP83822I/datasheet

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

---
 drivers/net/phy/Kconfig   |   5 +
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/dp83822.c | 313 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 319 insertions(+)
 create mode 100644 drivers/net/phy/dp83822.c

-- 
1.9.1

Comments

Dan Murphy Oct. 3, 2017, 4:11 p.m. | #1
All

On 10/03/2017 10:53 AM, Dan Murphy wrote:
> Add support for the TI  DP83822 10/100Mbit ethernet phy.

> 

> The DP83822 provides flexibility to connect to a MAC through a

> standard MII, RMII or RGMII interface.

> 


I need to submit an additional patch to remove DP83822 from the DP83848.
The main difference in the driver is that this driver supports WoL and the DP83848
does not.

So please kindly review this code and I can submit v2 with the DP83848 change

Dan

> Datasheet:

> http://www.ti.com/product/DP83822I/datasheet

> 

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

> ---

>  drivers/net/phy/Kconfig   |   5 +

>  drivers/net/phy/Makefile  |   1 +

>  drivers/net/phy/dp83822.c | 313 ++++++++++++++++++++++++++++++++++++++++++++++

>  3 files changed, 319 insertions(+)

>  create mode 100644 drivers/net/phy/dp83822.c

> 

> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig

> index cd931cf..8e78a48 100644

> --- a/drivers/net/phy/Kconfig

> +++ b/drivers/net/phy/Kconfig

> @@ -277,6 +277,11 @@ config DAVICOM_PHY

>  	---help---

>  	  Currently supports dm9161e and dm9131

>  

> +config DP83822_PHY

> +	tristate "Texas Instruments DP83822 PHY"

> +	---help---

> +	  Supports the DP83822 PHY.

> +

>  config DP83848_PHY

>  	tristate "Texas Instruments DP83848 PHY"

>  	---help---

> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile

> index 416df92..df3b82b 100644

> --- a/drivers/net/phy/Makefile

> +++ b/drivers/net/phy/Makefile

> @@ -55,6 +55,7 @@ obj-$(CONFIG_CICADA_PHY)	+= cicada.o

>  obj-$(CONFIG_CORTINA_PHY)	+= cortina.o

>  obj-$(CONFIG_DAVICOM_PHY)	+= davicom.o

>  obj-$(CONFIG_DP83640_PHY)	+= dp83640.o

> +obj-$(CONFIG_DP83822_PHY)	+= dp83822.o

>  obj-$(CONFIG_DP83848_PHY)	+= dp83848.o

>  obj-$(CONFIG_DP83867_PHY)	+= dp83867.o

>  obj-$(CONFIG_FIXED_PHY)		+= fixed_phy.o

> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c

> new file mode 100644

> index 0000000..1d77515

> --- /dev/null

> +++ b/drivers/net/phy/dp83822.c

> @@ -0,0 +1,313 @@

> +/*

> + * Driver for the Texas Instruments DP83822 PHY

> + *

> + * Copyright (C) 2017 Texas Instruments Inc.

> + *

> + * This program is free software; you can redistribute it and/or modify

> + * it under the terms of the GNU General Public License as published by

> + * the Free Software Foundation; either version 2 of the License.

> + *

> + * This program is distributed in the hope that it will be useful,

> + * but WITHOUT ANY WARRANTY; without even the implied warranty of

> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> + * GNU General Public License for more details.

> + */

> +

> +#include <linux/ethtool.h>

> +#include <linux/etherdevice.h>

> +#include <linux/kernel.h>

> +#include <linux/mii.h>

> +#include <linux/module.h>

> +#include <linux/of.h>

> +#include <linux/phy.h>

> +#include <linux/netdevice.h>

> +

> +#define DP83822_PHY_ID	        0x2000a240

> +#define DP83822_DEVADDR		0x1f

> +

> +#define MII_DP83822_MISR1	0x12

> +#define MII_DP83822_MISR2	0x13

> +#define MII_DP83822_RESET_CTRL	0x1f

> +

> +#define DP83822_HW_RESET	BIT(15)

> +#define DP83822_SW_RESET	BIT(14)

> +

> +/* MISR1 bits */

> +#define DP83822_RX_ERR_HF_INT_EN	BIT(0)

> +#define DP83822_FALSE_CARRIER_HF_INT_EN	BIT(1)

> +#define DP83822_ANEG_COMPLETE_INT_EN	BIT(2)

> +#define DP83822_DUP_MODE_CHANGE_INT_EN	BIT(3)

> +#define DP83822_SPEED_CHANGED_INT_EN	BIT(4)

> +#define DP83822_LINK_STAT_INT_EN	BIT(5)

> +#define DP83822_ENERGY_DET_INT_EN	BIT(6)

> +#define DP83822_LINK_QUAL_INT_EN	BIT(7)

> +

> +/* MISR2 bits */

> +#define DP83822_JABBER_DET_INT_EN	BIT(0)

> +#define DP83822_WOL_PKT_INT_EN		BIT(1)

> +#define DP83822_SLEEP_MODE_INT_EN	BIT(2)

> +#define DP83822_MDI_XOVER_INT_EN	BIT(3)

> +#define DP83822_LB_FIFO_INT_EN		BIT(4)

> +#define DP83822_PAGE_RX_INT_EN		BIT(5)

> +#define DP83822_ANEG_ERR_INT_EN		BIT(6)

> +#define DP83822_EEE_ERROR_CHANGE_INT_EN	BIT(7)

> +

> +/* INT_STAT1 bits */

> +#define DP83822_WOL_INT_EN	BIT(4)

> +#define DP83822_WOL_INT_STAT	BIT(12)

> +

> +#define MII_DP83822_RXSOP1	0x04A5

> +#define	MII_DP83822_RXSOP2	0x04A6

> +#define	MII_DP83822_RXSOP3	0x04A7

> +

> +/* WoL Registers */

> +#define	MII_DP83822_WOL_CFG	0x04A0

> +#define	MII_DP83822_WOL_STAT	0x04A1

> +#define	MII_DP83822_WOL_DA1	0x04A2

> +#define	MII_DP83822_WOL_DA2	0x04A3

> +#define	MII_DP83822_WOL_DA3	0x04A4

> +

> +/* WoL bits */

> +#define DP83822_WOL_MAGIC_EN	BIT(1)

> +#define DP83822_WOL_SECURE_ON	BIT(5)

> +#define DP83822_WOL_EN		BIT(7)

> +#define DP83822_WOL_INDICATION_SEL BIT(8)

> +#define DP83822_WOL_CLR_INDICATION BIT(11)

> +

> +static int dp83822_ack_interrupt(struct phy_device *phydev)

> +{

> +	int err = phy_read(phydev, MII_DP83822_MISR1);

> +

> +	if (err < 0)

> +		return err;

> +

> +	err = phy_read(phydev, MII_DP83822_MISR2);

> +	if (err < 0)

> +		return err;

> +

> +	return 0;

> +}

> +

> +static int dp83822_set_wol(struct phy_device *phydev,

> +			   struct ethtool_wolinfo *wol)

> +{

> +	struct net_device *ndev = phydev->attached_dev;

> +	u16 value;

> +	const u8 *mac;

> +

> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) {

> +		mac = (const u8 *)ndev->dev_addr;

> +

> +		if (!is_valid_ether_addr(mac))

> +			return -EFAULT;

> +

> +		/* MAC addresses start with byte 5, but stored in mac[0].

> +		 * 822 PHYs store bytes 4|5, 2|3, 0|1

> +		 */

> +		phy_write_mmd(phydev, DP83822_DEVADDR,

> +			      MII_DP83822_WOL_DA1, (mac[1] << 8) | mac[0]);

> +		phy_write_mmd(phydev, DP83822_DEVADDR,

> +			      MII_DP83822_WOL_DA2, (mac[3] << 8) | mac[2]);

> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_DA3,

> +			      (mac[5] << 8) | mac[4]);

> +

> +		value = phy_read_mmd(phydev, DP83822_DEVADDR,

> +				     MII_DP83822_WOL_CFG);

> +		if (wol->wolopts & WAKE_MAGIC)

> +			value |= DP83822_WOL_MAGIC_EN;

> +		else

> +			value &= ~DP83822_WOL_MAGIC_EN;

> +

> +		if (wol->wolopts & WAKE_MAGICSECURE) {

> +			value |= DP83822_WOL_SECURE_ON;

> +			phy_write_mmd(phydev, DP83822_DEVADDR,

> +				      MII_DP83822_RXSOP1,

> +				      (wol->sopass[1] << 8) | wol->sopass[0]);

> +			phy_write_mmd(phydev, DP83822_DEVADDR,

> +				      MII_DP83822_RXSOP2,

> +				      (wol->sopass[3] << 8) | wol->sopass[2]);

> +			phy_write_mmd(phydev, DP83822_DEVADDR,

> +				      MII_DP83822_RXSOP3,

> +				      (wol->sopass[5] << 8) | wol->sopass[4]);

> +		} else {

> +			value &= ~DP83822_WOL_SECURE_ON;

> +		}

> +

> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

> +			  DP83822_WOL_CLR_INDICATION);

> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

> +			      value);

> +	} else {

> +		value =

> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +		value &= (~DP83822_WOL_EN);

> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

> +			      value);

> +	}

> +

> +	return 0;

> +}

> +

> +static void dp83822_get_wol(struct phy_device *phydev,

> +			    struct ethtool_wolinfo *wol)

> +{

> +	int value;

> +

> +	wol->supported = (WAKE_MAGIC | WAKE_MAGICSECURE);

> +	wol->wolopts = 0;

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +	if (value & DP83822_WOL_MAGIC_EN)

> +		wol->wolopts |= WAKE_MAGIC;

> +

> +	if (value & DP83822_WOL_SECURE_ON)

> +		wol->wolopts |= WAKE_MAGICSECURE;

> +

> +	if (~value & DP83822_WOL_CLR_INDICATION)

> +		wol->wolopts = 0;

> +

> +	wol->sopass[0] = (phy_read_mmd(phydev,

> +				       DP83822_DEVADDR,

> +				       MII_DP83822_RXSOP1) & 0xFF);

> +	wol->sopass[1] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP1) >> 8);

> +	wol->sopass[2] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) & 0xFF);

> +	wol->sopass[3] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) >> 8);

> +	wol->sopass[4] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) & 0xFF);

> +	wol->sopass[5] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) >> 8);

> +}

> +

> +static int dp83822_config_intr(struct phy_device *phydev)

> +{

> +	int misr_status;

> +	int err;

> +

> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {

> +		misr_status = phy_read(phydev, MII_DP83822_MISR1);

> +		if (misr_status < 0)

> +			return misr_status;

> +

> +		misr_status |= (DP83822_RX_ERR_HF_INT_EN |

> +				DP83822_FALSE_CARRIER_HF_INT_EN |

> +				DP83822_ANEG_COMPLETE_INT_EN |

> +				DP83822_DUP_MODE_CHANGE_INT_EN |

> +				DP83822_SPEED_CHANGED_INT_EN |

> +				DP83822_LINK_STAT_INT_EN |

> +				DP83822_ENERGY_DET_INT_EN |

> +				DP83822_LINK_QUAL_INT_EN);

> +

> +		err = phy_write(phydev, MII_DP83822_MISR1, misr_status);

> +		if (err < 0)

> +			return err;

> +

> +		misr_status = phy_read(phydev, MII_DP83822_MISR2);

> +		if (misr_status < 0)

> +			return misr_status;

> +

> +		misr_status |= (DP83822_JABBER_DET_INT_EN |

> +				DP83822_WOL_PKT_INT_EN |

> +				DP83822_SLEEP_MODE_INT_EN |

> +				DP83822_MDI_XOVER_INT_EN |

> +				DP83822_LB_FIFO_INT_EN |

> +				DP83822_PAGE_RX_INT_EN |

> +				DP83822_ANEG_ERR_INT_EN |

> +				DP83822_EEE_ERROR_CHANGE_INT_EN);

> +

> +		err = phy_write(phydev, MII_DP83822_MISR2, misr_status);

> +	} else {

> +		err = phy_write(phydev, MII_DP83822_MISR1, 0);

> +		if (err < 0)

> +			return err;

> +

> +		err = phy_write(phydev, MII_DP83822_MISR1, 0);

> +	}

> +

> +	return err;

> +}

> +

> +static int dp83822_phy_reset(struct phy_device *phydev)

> +{

> +	int err;

> +

> +	err = phy_write(phydev, MII_DP83822_RESET_CTRL, DP83822_HW_RESET);

> +	if (err < 0)

> +		return err;

> +

> +	return 0;

> +}

> +

> +static int dp83822_suspend(struct phy_device *phydev)

> +{

> +	int value;

> +

> +	mutex_lock(&phydev->lock);

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +	if (~value & DP83822_WOL_EN) {

> +		value = phy_read(phydev, MII_BMCR);

> +		phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);

> +	}

> +

> +	mutex_unlock(&phydev->lock);

> +

> +	return 0;

> +}

> +

> +static int dp83822_resume(struct phy_device *phydev)

> +{

> +	int value;

> +

> +	mutex_lock(&phydev->lock);

> +

> +	value = phy_read(phydev, MII_BMCR);

> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +

> +	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |

> +		      DP83822_WOL_CLR_INDICATION);

> +

> +	mutex_unlock(&phydev->lock);

> +

> +	return 0;

> +}

> +

> +static struct phy_driver dp83822_driver[] = {

> +	{

> +	 .phy_id = DP83822_PHY_ID,

> +	 .phy_id_mask = 0xfffffff0,

> +	 .name = "TI DP83822",

> +	 .features = PHY_BASIC_FEATURES,

> +	 .flags = PHY_HAS_INTERRUPT,

> +

> +	 .config_init = genphy_config_init,

> +	 .soft_reset = dp83822_phy_reset,

> +

> +	 .get_wol = dp83822_get_wol,

> +	 .set_wol = dp83822_set_wol,

> +

> +	 /* IRQ related */

> +	 .ack_interrupt = dp83822_ack_interrupt,

> +	 .config_intr = dp83822_config_intr,

> +

> +	 .config_aneg = genphy_config_aneg,

> +	 .read_status = genphy_read_status,

> +	 .suspend = dp83822_suspend,

> +	 .resume = dp83822_resume,

> +	 },

> +};

> +module_phy_driver(dp83822_driver);

> +

> +static struct mdio_device_id __maybe_unused dp83822_tbl[] = {

> +	{ DP83822_PHY_ID, 0xfffffff0 },

> +	{ }

> +};

> +MODULE_DEVICE_TABLE(mdio, dp83822_tbl);

> +

> +MODULE_DESCRIPTION("Texas Instruments DP83822 PHY driver");

> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com");

> +MODULE_LICENSE("GPL");

> 



-- 
------------------
Dan Murphy
Florian Fainelli Oct. 3, 2017, 5:15 p.m. | #2
On 10/03/2017 08:53 AM, Dan Murphy wrote:
> Add support for the TI  DP83822 10/100Mbit ethernet phy.

> 

> The DP83822 provides flexibility to connect to a MAC through a

> standard MII, RMII or RGMII interface.

> 

> Datasheet:

> http://www.ti.com/product/DP83822I/datasheet


This looks pretty good, just a few nits below.

[snip]

> +static int dp83822_set_wol(struct phy_device *phydev,

> +			   struct ethtool_wolinfo *wol)

> +{

> +	struct net_device *ndev = phydev->attached_dev;

> +	u16 value;

> +	const u8 *mac;

> +

> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) {

> +		mac = (const u8 *)ndev->dev_addr;

> +

> +		if (!is_valid_ether_addr(mac))

> +			return -EFAULT;


-EINVAL maybe?

> +

> +		/* MAC addresses start with byte 5, but stored in mac[0].

> +		 * 822 PHYs store bytes 4|5, 2|3, 0|1

> +		 */

> +		phy_write_mmd(phydev, DP83822_DEVADDR,

> +			      MII_DP83822_WOL_DA1, (mac[1] << 8) | mac[0]);

> +		phy_write_mmd(phydev, DP83822_DEVADDR,

> +			      MII_DP83822_WOL_DA2, (mac[3] << 8) | mac[2]);

> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_DA3,

> +			      (mac[5] << 8) | mac[4]);

> +

> +		value = phy_read_mmd(phydev, DP83822_DEVADDR,

> +				     MII_DP83822_WOL_CFG);

> +		if (wol->wolopts & WAKE_MAGIC)

> +			value |= DP83822_WOL_MAGIC_EN;

> +		else

> +			value &= ~DP83822_WOL_MAGIC_EN;

> +

> +		if (wol->wolopts & WAKE_MAGICSECURE) {

> +			value |= DP83822_WOL_SECURE_ON;


Just in case any of the writes below fail, you would probably want to
set this bit last, thus indicating that the password was successfully set.

> +			phy_write_mmd(phydev, DP83822_DEVADDR,

> +				      MII_DP83822_RXSOP1,

> +				      (wol->sopass[1] << 8) | wol->sopass[0]);

> +			phy_write_mmd(phydev, DP83822_DEVADDR,

> +				      MII_DP83822_RXSOP2,

> +				      (wol->sopass[3] << 8) | wol->sopass[2]);

> +			phy_write_mmd(phydev, DP83822_DEVADDR,

> +				      MII_DP83822_RXSOP3,

> +				      (wol->sopass[5] << 8) | wol->sopass[4]);


In the else clause, you don't appear to be clearing the MagicPacket
SecureOn password, but your get_wol function does not check for
DP83822_WOL_SECURE_ON before returning the secure password, so either
one of these two should be fixed. I would go with fixing the get_wol
function see below.

> +		} else {

> +			value &= ~DP83822_WOL_SECURE_ON;

> +		}

> +

> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

> +			  DP83822_WOL_CLR_INDICATION);


The extra parenthesis should not be required here.

> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

> +			      value);

> +	} else {

> +		value =

> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +		value &= (~DP83822_WOL_EN);


Same here, parenthesis should not be needed.

> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

> +			      value);

> +	}

> +

> +	return 0;

> +}

> +

> +static void dp83822_get_wol(struct phy_device *phydev,

> +			    struct ethtool_wolinfo *wol)

> +{

> +	int value;

> +

> +	wol->supported = (WAKE_MAGIC | WAKE_MAGICSECURE);

> +	wol->wolopts = 0;

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +	if (value & DP83822_WOL_MAGIC_EN)

> +		wol->wolopts |= WAKE_MAGIC;

> +

> +	if (value & DP83822_WOL_SECURE_ON)

> +		wol->wolopts |= WAKE_MAGICSECURE;

> +

> +	if (~value & DP83822_WOL_CLR_INDICATION)

> +		wol->wolopts = 0;

> +

> +	wol->sopass[0] = (phy_read_mmd(phydev,

> +				       DP83822_DEVADDR,

> +				       MII_DP83822_RXSOP1) & 0xFF);

> +	wol->sopass[1] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP1) >> 8);


You can save about twice the amount of reads by using a temporary
variable to hold the 16-bit register ;)

> +	wol->sopass[2] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) & 0xFF);

> +	wol->sopass[3] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) >> 8);

> +	wol->sopass[4] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) & 0xFF);

> +	wol->sopass[5] =

> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) >> 8);


Unless DP83822_WOL_SECURE_ON is set, you probably should not try reading
the password at all, because there is no guarantee it has been correctly
set.

> +}


> +static int dp83822_phy_reset(struct phy_device *phydev)

> +{

> +	int err;

> +

> +	err = phy_write(phydev, MII_DP83822_RESET_CTRL, DP83822_HW_RESET);

> +	if (err < 0)

> +		return err;

> +

> +	return 0;

> +}

> +

> +static int dp83822_suspend(struct phy_device *phydev)

> +{

> +	int value;

> +

> +	mutex_lock(&phydev->lock);

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +	if (~value & DP83822_WOL_EN) {

> +		value = phy_read(phydev, MII_BMCR);

> +		phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);

> +	}


Can you use genphy_suspend() here along with careful locking of course?

> +

> +	mutex_unlock(&phydev->lock);

> +

> +	return 0;

> +}

> +

> +static int dp83822_resume(struct phy_device *phydev)

> +{

> +	int value;

> +

> +	mutex_lock(&phydev->lock);

> +

> +	value = phy_read(phydev, MII_BMCR);

> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);


And genphy_resume() here as well?

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> +

> +	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |

> +		      DP83822_WOL_CLR_INDICATION);

> +

> +	mutex_unlock(&phydev->lock);

> +

> +	return 0;

> +}

> +

> +static struct phy_driver dp83822_driver[] = {

> +	{

> +	 .phy_id = DP83822_PHY_ID,

> +	 .phy_id_mask = 0xfffffff0,

> +	 .name = "TI DP83822",

> +	 .features = PHY_BASIC_FEATURES,

> +	 .flags = PHY_HAS_INTERRUPT,

> +

> +	 .config_init = genphy_config_init,

> +	 .soft_reset = dp83822_phy_reset,

> +

> +	 .get_wol = dp83822_get_wol,

> +	 .set_wol = dp83822_set_wol,

> +

> +	 /* IRQ related */

> +	 .ack_interrupt = dp83822_ack_interrupt,

> +	 .config_intr = dp83822_config_intr,

> +

> +	 .config_aneg = genphy_config_aneg,

> +	 .read_status = genphy_read_status,

> +	 .suspend = dp83822_suspend,

> +	 .resume = dp83822_resume,

> +	 },


I would omit newlines between definitions of callbacks, but this is
really a personal preference. Unless you are planning on adding new IDs,
you could also avoid using an array of 1 element and just a plain
phy_driver structure, but that's not a big deal either.
-- 
Florian
woojung.huh@microchip.com Oct. 3, 2017, 5:43 p.m. | #3
> +static int dp83822_suspend(struct phy_device *phydev)

> +{

> +	int value;

> +

> +	mutex_lock(&phydev->lock);

> +

> +	value = phy_read_mmd(phydev, DP83822_DEVADDR,

> MII_DP83822_WOL_CFG);

> +	if (~value & DP83822_WOL_EN) {

Same result, but how about " if (!(value & DP83822_WOL_EN))" ?

> +		value = phy_read(phydev, MII_BMCR);

> +		phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);

> +	}

> +

> +	mutex_unlock(&phydev->lock);

> +

> +	return 0;

> +}
Dan Murphy Oct. 3, 2017, 6:03 p.m. | #4
Florian

Thanks for the review

On 10/03/2017 12:15 PM, Florian Fainelli wrote:
> On 10/03/2017 08:53 AM, Dan Murphy wrote:

>> Add support for the TI  DP83822 10/100Mbit ethernet phy.

>>

>> The DP83822 provides flexibility to connect to a MAC through a

>> standard MII, RMII or RGMII interface.

>>

>> Datasheet:

>> http://www.ti.com/product/DP83822I/datasheet

> 

> This looks pretty good, just a few nits below.

> 

> [snip]

> 

>> +static int dp83822_set_wol(struct phy_device *phydev,

>> +			   struct ethtool_wolinfo *wol)

>> +{

>> +	struct net_device *ndev = phydev->attached_dev;

>> +	u16 value;

>> +	const u8 *mac;

>> +

>> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) {

>> +		mac = (const u8 *)ndev->dev_addr;

>> +

>> +		if (!is_valid_ether_addr(mac))

>> +			return -EFAULT;

> 

> -EINVAL maybe?


I referenced the at803x driver for the error code.  I can change it if you want it to be -EINVAL.
I can submit a patch to do the same for the other driver.

-EIVAL does make more sense.


> 

>> +

>> +		/* MAC addresses start with byte 5, but stored in mac[0].

>> +		 * 822 PHYs store bytes 4|5, 2|3, 0|1

>> +		 */

>> +		phy_write_mmd(phydev, DP83822_DEVADDR,

>> +			      MII_DP83822_WOL_DA1, (mac[1] << 8) | mac[0]);

>> +		phy_write_mmd(phydev, DP83822_DEVADDR,

>> +			      MII_DP83822_WOL_DA2, (mac[3] << 8) | mac[2]);

>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_DA3,

>> +			      (mac[5] << 8) | mac[4]);

>> +

>> +		value = phy_read_mmd(phydev, DP83822_DEVADDR,

>> +				     MII_DP83822_WOL_CFG);

>> +		if (wol->wolopts & WAKE_MAGIC)

>> +			value |= DP83822_WOL_MAGIC_EN;

>> +		else

>> +			value &= ~DP83822_WOL_MAGIC_EN;

>> +

>> +		if (wol->wolopts & WAKE_MAGICSECURE) {

>> +			value |= DP83822_WOL_SECURE_ON;

> 

> Just in case any of the writes below fail, you would probably want to

> set this bit last, thus indicating that the password was successfully set.


Good point

> 

>> +			phy_write_mmd(phydev, DP83822_DEVADDR,

>> +				      MII_DP83822_RXSOP1,

>> +				      (wol->sopass[1] << 8) | wol->sopass[0]);

>> +			phy_write_mmd(phydev, DP83822_DEVADDR,

>> +				      MII_DP83822_RXSOP2,

>> +				      (wol->sopass[3] << 8) | wol->sopass[2]);

>> +			phy_write_mmd(phydev, DP83822_DEVADDR,

>> +				      MII_DP83822_RXSOP3,

>> +				      (wol->sopass[5] << 8) | wol->sopass[4]);

> 

> In the else clause, you don't appear to be clearing the MagicPacket

> SecureOn password, but your get_wol function does not check for

> DP83822_WOL_SECURE_ON before returning the secure password, so either

> one of these two should be fixed. I would go with fixing the get_wol

> function see below.


OK

> 

>> +		} else {

>> +			value &= ~DP83822_WOL_SECURE_ON;

>> +		}

>> +

>> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

>> +			  DP83822_WOL_CLR_INDICATION);

> 

> The extra parenthesis should not be required here.


I did not code that in.  I had to add it after Checkpatch cribbed about it.
Let me know if you want me to remove it.

> 

>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

>> +			      value);

>> +	} else {

>> +		value =

>> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>> +		value &= (~DP83822_WOL_EN);

> 

> Same here, parenthesis should not be needed.


There are three lines of code in the else.  This code all needs to be excuted in the else case.
I might reformat it to read better.  Lindent messed that one up.

> 

>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

>> +			      value);

>> +	}

>> +

>> +	return 0;

>> +}

>> +

>> +static void dp83822_get_wol(struct phy_device *phydev,

>> +			    struct ethtool_wolinfo *wol)

>> +{

>> +	int value;

>> +

>> +	wol->supported = (WAKE_MAGIC | WAKE_MAGICSECURE);

>> +	wol->wolopts = 0;

>> +

>> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>> +	if (value & DP83822_WOL_MAGIC_EN)

>> +		wol->wolopts |= WAKE_MAGIC;

>> +

>> +	if (value & DP83822_WOL_SECURE_ON)

>> +		wol->wolopts |= WAKE_MAGICSECURE;

>> +

>> +	if (~value & DP83822_WOL_CLR_INDICATION)

>> +		wol->wolopts = 0;

>> +

>> +	wol->sopass[0] = (phy_read_mmd(phydev,

>> +				       DP83822_DEVADDR,

>> +				       MII_DP83822_RXSOP1) & 0xFF);

>> +	wol->sopass[1] =

>> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP1) >> 8);

> 

> You can save about twice the amount of reads by using a temporary

> variable to hold the 16-bit register ;)


You are right I can clean that up.

> 

>> +	wol->sopass[2] =

>> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) & 0xFF);

>> +	wol->sopass[3] =

>> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) >> 8);

>> +	wol->sopass[4] =

>> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) & 0xFF);

>> +	wol->sopass[5] =

>> +	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) >> 8);

> 

> Unless DP83822_WOL_SECURE_ON is set, you probably should not try reading

> the password at all, because there is no guarantee it has been correctly

> set.

> 


OK

>> +}

> 

>> +static int dp83822_phy_reset(struct phy_device *phydev)

>> +{

>> +	int err;

>> +

>> +	err = phy_write(phydev, MII_DP83822_RESET_CTRL, DP83822_HW_RESET);

>> +	if (err < 0)

>> +		return err;

>> +

>> +	return 0;

>> +}

>> +

>> +static int dp83822_suspend(struct phy_device *phydev)

>> +{

>> +	int value;

>> +

>> +	mutex_lock(&phydev->lock);

>> +

>> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>> +	if (~value & DP83822_WOL_EN) {

>> +		value = phy_read(phydev, MII_BMCR);

>> +		phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);

>> +	}

> 

> Can you use genphy_suspend() here along with careful locking of course?

> 


genphy_suspend does not have WoL.

>> +

>> +	mutex_unlock(&phydev->lock);

>> +

>> +	return 0;

>> +}

>> +

>> +static int dp83822_resume(struct phy_device *phydev)

>> +{

>> +	int value;

>> +

>> +	mutex_lock(&phydev->lock);

>> +

>> +	value = phy_read(phydev, MII_BMCR);

>> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);

> 

> And genphy_resume() here as well?


genphy_resume does not have WoL.

> 

>> +

>> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>> +

>> +	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |

>> +		      DP83822_WOL_CLR_INDICATION);

>> +

>> +	mutex_unlock(&phydev->lock);

>> +

>> +	return 0;

>> +}

>> +

>> +static struct phy_driver dp83822_driver[] = {

>> +	{

>> +	 .phy_id = DP83822_PHY_ID,

>> +	 .phy_id_mask = 0xfffffff0,

>> +	 .name = "TI DP83822",

>> +	 .features = PHY_BASIC_FEATURES,

>> +	 .flags = PHY_HAS_INTERRUPT,

>> +

>> +	 .config_init = genphy_config_init,

>> +	 .soft_reset = dp83822_phy_reset,

>> +

>> +	 .get_wol = dp83822_get_wol,

>> +	 .set_wol = dp83822_set_wol,

>> +

>> +	 /* IRQ related */

>> +	 .ack_interrupt = dp83822_ack_interrupt,

>> +	 .config_intr = dp83822_config_intr,

>> +

>> +	 .config_aneg = genphy_config_aneg,

>> +	 .read_status = genphy_read_status,

>> +	 .suspend = dp83822_suspend,

>> +	 .resume = dp83822_resume,

>> +	 },

> 

> I would omit newlines between definitions of callbacks, but this is

> really a personal preference. Unless you are planning on adding new IDs,

> you could also avoid using an array of 1 element and just a plain

> phy_driver structure, but that's not a big deal either.


Yes there is a plan to add another phy id in early 2018 to this driver.

> 



-- 
------------------
Dan Murphy
Florian Fainelli Oct. 3, 2017, 6:31 p.m. | #5
On 10/03/2017 11:03 AM, Dan Murphy wrote:
> Florian

> 

> Thanks for the review

> 

> On 10/03/2017 12:15 PM, Florian Fainelli wrote:

>>> +		} else {

>>> +			value &= ~DP83822_WOL_SECURE_ON;

>>> +		}

>>> +

>>> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

>>> +			  DP83822_WOL_CLR_INDICATION);

>>

>> The extra parenthesis should not be required here.

> 

> I did not code that in.  I had to add it after Checkpatch cribbed about it.

> Let me know if you want me to remove it.


Let's keep those, that does not change much.

> 

>>

>>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

>>> +			      value);

>>> +	} else {

>>> +		value =

>>> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>>> +		value &= (~DP83822_WOL_EN);

>>

>> Same here, parenthesis should not be needed.

> 

> There are three lines of code in the else.  This code all needs to be excuted in the else case.

> I might reformat it to read better.  Lindent messed that one up.


sorry, I meant to write that you don't need the parenthesis around
DP83822_WOL_EN since that is just a single bit here.

[snip]

>>> +

>>> +	mutex_unlock(&phydev->lock);

>>> +

>>> +	return 0;

>>> +}

>>> +

>>> +static int dp83822_resume(struct phy_device *phydev)

>>> +{

>>> +	int value;

>>> +

>>> +	mutex_lock(&phydev->lock);

>>> +

>>> +	value = phy_read(phydev, MII_BMCR);

>>> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);

>>

>> And genphy_resume() here as well?

> 

> genphy_resume does not have WoL.


I should have been cleared, I meant using genphy_{suspend,resume} to
avoid open coding the setting of the BMCR_PDOWN bit, conversely clearing
of that bit. Because of the locking, maybe you could introduce unlocked
versions of these two routines, or you acquire and release the lock
outside of genphy_{suspend,resume}?

> 

>>

>>> +

>>> +	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>>> +

>>> +	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |

>>> +		      DP83822_WOL_CLR_INDICATION);

>>> +

>>> +	mutex_unlock(&phydev->lock);

>>> +

>>> +	return 0;

>>> +}

>>> +

>>> +static struct phy_driver dp83822_driver[] = {

>>> +	{

>>> +	 .phy_id = DP83822_PHY_ID,

>>> +	 .phy_id_mask = 0xfffffff0,

>>> +	 .name = "TI DP83822",

>>> +	 .features = PHY_BASIC_FEATURES,

>>> +	 .flags = PHY_HAS_INTERRUPT,

>>> +

>>> +	 .config_init = genphy_config_init,

>>> +	 .soft_reset = dp83822_phy_reset,

>>> +

>>> +	 .get_wol = dp83822_get_wol,

>>> +	 .set_wol = dp83822_set_wol,

>>> +

>>> +	 /* IRQ related */

>>> +	 .ack_interrupt = dp83822_ack_interrupt,

>>> +	 .config_intr = dp83822_config_intr,

>>> +

>>> +	 .config_aneg = genphy_config_aneg,

>>> +	 .read_status = genphy_read_status,

>>> +	 .suspend = dp83822_suspend,

>>> +	 .resume = dp83822_resume,

>>> +	 },

>>

>> I would omit newlines between definitions of callbacks, but this is

>> really a personal preference. Unless you are planning on adding new IDs,

>> you could also avoid using an array of 1 element and just a plain

>> phy_driver structure, but that's not a big deal either.

> 

> Yes there is a plan to add another phy id in early 2018 to this driver.


Alright then!
-- 
Florian
Dan Murphy Oct. 4, 2017, 3:41 p.m. | #6
Florian

On 10/03/2017 01:31 PM, Florian Fainelli wrote:
> On 10/03/2017 11:03 AM, Dan Murphy wrote:

>> Florian

>>

>> Thanks for the review

>>

>> On 10/03/2017 12:15 PM, Florian Fainelli wrote:

>>>> +		} else {

>>>> +			value &= ~DP83822_WOL_SECURE_ON;

>>>> +		}

>>>> +

>>>> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

>>>> +			  DP83822_WOL_CLR_INDICATION);

>>>

>>> The extra parenthesis should not be required here.

>>

>> I did not code that in.  I had to add it after Checkpatch cribbed about it.

>> Let me know if you want me to remove it.

> 

> Let's keep those, that does not change much.

> 

>>

>>>

>>>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

>>>> +			      value);

>>>> +	} else {

>>>> +		value =

>>>> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>>>> +		value &= (~DP83822_WOL_EN);

>>>

>>> Same here, parenthesis should not be needed.

>>

>> There are three lines of code in the else.  This code all needs to be excuted in the else case.

>> I might reformat it to read better.  Lindent messed that one up.

> 

> sorry, I meant to write that you don't need the parenthesis around

> DP83822_WOL_EN since that is just a single bit here.

> 

> [snip]

> 

>>>> +

>>>> +	mutex_unlock(&phydev->lock);

>>>> +

>>>> +	return 0;

>>>> +}

>>>> +

>>>> +static int dp83822_resume(struct phy_device *phydev)

>>>> +{

>>>> +	int value;

>>>> +

>>>> +	mutex_lock(&phydev->lock);

>>>> +

>>>> +	value = phy_read(phydev, MII_BMCR);

>>>> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);

>>>

>>> And genphy_resume() here as well?

>>

>> genphy_resume does not have WoL.

> 

> I should have been cleared, I meant using genphy_{suspend,resume} to

> avoid open coding the setting of the BMCR_PDOWN bit, conversely clearing

> of that bit. Because of the locking, maybe you could introduce unlocked

> versions of these two routines, or you acquire and release the lock

> outside of genphy_{suspend,resume}?


OK I have addressed all of the open comments and will be posting v2 shortly.

I do have a question on this request.
Are you indicating to exclusively call genphy_(suspend/resume) from within the over ridden
routine and then take care of the WoL bits in the phy specific code?

Since the genphy code is duplicated here for the BMCR value that would make the most sense
to me.  The genphy code is exported so I can call it explicitly then do the WoL 

Dan

[snip]-- 
------------------
Dan Murphy
Florian Fainelli Oct. 4, 2017, 4:18 p.m. | #7
On 10/04/2017 08:41 AM, Dan Murphy wrote:
> Florian

> 

> On 10/03/2017 01:31 PM, Florian Fainelli wrote:

>> On 10/03/2017 11:03 AM, Dan Murphy wrote:

>>> Florian

>>>

>>> Thanks for the review

>>>

>>> On 10/03/2017 12:15 PM, Florian Fainelli wrote:

>>>>> +		} else {

>>>>> +			value &= ~DP83822_WOL_SECURE_ON;

>>>>> +		}

>>>>> +

>>>>> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

>>>>> +			  DP83822_WOL_CLR_INDICATION);

>>>>

>>>> The extra parenthesis should not be required here.

>>>

>>> I did not code that in.  I had to add it after Checkpatch cribbed about it.

>>> Let me know if you want me to remove it.

>>

>> Let's keep those, that does not change much.

>>

>>>

>>>>

>>>>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

>>>>> +			      value);

>>>>> +	} else {

>>>>> +		value =

>>>>> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>>>>> +		value &= (~DP83822_WOL_EN);

>>>>

>>>> Same here, parenthesis should not be needed.

>>>

>>> There are three lines of code in the else.  This code all needs to be excuted in the else case.

>>> I might reformat it to read better.  Lindent messed that one up.

>>

>> sorry, I meant to write that you don't need the parenthesis around

>> DP83822_WOL_EN since that is just a single bit here.

>>

>> [snip]

>>

>>>>> +

>>>>> +	mutex_unlock(&phydev->lock);

>>>>> +

>>>>> +	return 0;

>>>>> +}

>>>>> +

>>>>> +static int dp83822_resume(struct phy_device *phydev)

>>>>> +{

>>>>> +	int value;

>>>>> +

>>>>> +	mutex_lock(&phydev->lock);

>>>>> +

>>>>> +	value = phy_read(phydev, MII_BMCR);

>>>>> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);

>>>>

>>>> And genphy_resume() here as well?

>>>

>>> genphy_resume does not have WoL.

>>

>> I should have been cleared, I meant using genphy_{suspend,resume} to

>> avoid open coding the setting of the BMCR_PDOWN bit, conversely clearing

>> of that bit. Because of the locking, maybe you could introduce unlocked

>> versions of these two routines, or you acquire and release the lock

>> outside of genphy_{suspend,resume}?

> 

> OK I have addressed all of the open comments and will be posting v2 shortly.

> 

> I do have a question on this request.

> Are you indicating to exclusively call genphy_(suspend/resume) from within the over ridden

> routine and then take care of the WoL bits in the phy specific code?

> 

> Since the genphy code is duplicated here for the BMCR value that would make the most sense

> to me.  The genphy code is exported so I can call it explicitly then do the WoL 


I would expect you to wrap your own calls around genphy_suspend and
genphy_resume, such your functions become:

static int dp83822_suspend(struct phy_device *phydev)
{
	int value;

	mutex_lock(&phydev->lock);
	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
	mutex_unlock(&phydev->lock);

	if (~value & DP83822_WOL_EN)
		genphy_suspend(phydev);
}

static int dp83822_resume(struct phy_device *phydev)
{
	int value;

	genphy_resume(phydev);

	mutex_lock(&phydev->lock);
	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |
		      DP83822_WOL_CLR_INDICATION);

	mutex_unlock(&phydev->lock);

	return 0;
}

> 

> Dan

> 

> [snip]-- 

> ------------------

> Dan Murphy

> 



-- 
Florian
Dan Murphy Oct. 4, 2017, 4:22 p.m. | #8
Florian

On 10/04/2017 11:18 AM, Florian Fainelli wrote:
> On 10/04/2017 08:41 AM, Dan Murphy wrote:

>> Florian

>>

>> On 10/03/2017 01:31 PM, Florian Fainelli wrote:

>>> On 10/03/2017 11:03 AM, Dan Murphy wrote:

>>>> Florian

>>>>

>>>> Thanks for the review

>>>>

>>>> On 10/03/2017 12:15 PM, Florian Fainelli wrote:

>>>>>> +		} else {

>>>>>> +			value &= ~DP83822_WOL_SECURE_ON;

>>>>>> +		}

>>>>>> +

>>>>>> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |

>>>>>> +			  DP83822_WOL_CLR_INDICATION);

>>>>>

>>>>> The extra parenthesis should not be required here.

>>>>

>>>> I did not code that in.  I had to add it after Checkpatch cribbed about it.

>>>> Let me know if you want me to remove it.

>>>

>>> Let's keep those, that does not change much.

>>>

>>>>

>>>>>

>>>>>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,

>>>>>> +			      value);

>>>>>> +	} else {

>>>>>> +		value =

>>>>>> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

>>>>>> +		value &= (~DP83822_WOL_EN);

>>>>>

>>>>> Same here, parenthesis should not be needed.

>>>>

>>>> There are three lines of code in the else.  This code all needs to be excuted in the else case.

>>>> I might reformat it to read better.  Lindent messed that one up.

>>>

>>> sorry, I meant to write that you don't need the parenthesis around

>>> DP83822_WOL_EN since that is just a single bit here.

>>>

>>> [snip]

>>>

>>>>>> +

>>>>>> +	mutex_unlock(&phydev->lock);

>>>>>> +

>>>>>> +	return 0;

>>>>>> +}

>>>>>> +

>>>>>> +static int dp83822_resume(struct phy_device *phydev)

>>>>>> +{

>>>>>> +	int value;

>>>>>> +

>>>>>> +	mutex_lock(&phydev->lock);

>>>>>> +

>>>>>> +	value = phy_read(phydev, MII_BMCR);

>>>>>> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);

>>>>>

>>>>> And genphy_resume() here as well?

>>>>

>>>> genphy_resume does not have WoL.

>>>

>>> I should have been cleared, I meant using genphy_{suspend,resume} to

>>> avoid open coding the setting of the BMCR_PDOWN bit, conversely clearing

>>> of that bit. Because of the locking, maybe you could introduce unlocked

>>> versions of these two routines, or you acquire and release the lock

>>> outside of genphy_{suspend,resume}?

>>

>> OK I have addressed all of the open comments and will be posting v2 shortly.

>>

>> I do have a question on this request.

>> Are you indicating to exclusively call genphy_(suspend/resume) from within the over ridden

>> routine and then take care of the WoL bits in the phy specific code?

>>

>> Since the genphy code is duplicated here for the BMCR value that would make the most sense

>> to me.  The genphy code is exported so I can call it explicitly then do the WoL 

> 

> I would expect you to wrap your own calls around genphy_suspend and

> genphy_resume, such your functions become:

> 

> static int dp83822_suspend(struct phy_device *phydev)

> {

> 	int value;

> 

> 	mutex_lock(&phydev->lock);

> 	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> 	mutex_unlock(&phydev->lock);

> 

> 	if (~value & DP83822_WOL_EN)

> 		genphy_suspend(phydev);

> }

> 

> static int dp83822_resume(struct phy_device *phydev)

> {

> 	int value;

> 

> 	genphy_resume(phydev);

> 

> 	mutex_lock(&phydev->lock);

> 	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);

> 

> 	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |

> 		      DP83822_WOL_CLR_INDICATION);

> 

> 	mutex_unlock(&phydev->lock);

> 

> 	return 0;

> }

> 


Great thats exactly what I did.

Dan

>>

>> Dan

>>

>> [snip]-- 

>> ------------------

>> Dan Murphy

>>

> 

> 



-- 
------------------
Dan Murphy

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cd931cf..8e78a48 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -277,6 +277,11 @@  config DAVICOM_PHY
 	---help---
 	  Currently supports dm9161e and dm9131
 
+config DP83822_PHY
+	tristate "Texas Instruments DP83822 PHY"
+	---help---
+	  Supports the DP83822 PHY.
+
 config DP83848_PHY
 	tristate "Texas Instruments DP83848 PHY"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 416df92..df3b82b 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -55,6 +55,7 @@  obj-$(CONFIG_CICADA_PHY)	+= cicada.o
 obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
 obj-$(CONFIG_DAVICOM_PHY)	+= davicom.o
 obj-$(CONFIG_DP83640_PHY)	+= dp83640.o
+obj-$(CONFIG_DP83822_PHY)	+= dp83822.o
 obj-$(CONFIG_DP83848_PHY)	+= dp83848.o
 obj-$(CONFIG_DP83867_PHY)	+= dp83867.o
 obj-$(CONFIG_FIXED_PHY)		+= fixed_phy.o
diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
new file mode 100644
index 0000000..1d77515
--- /dev/null
+++ b/drivers/net/phy/dp83822.c
@@ -0,0 +1,313 @@ 
+/*
+ * Driver for the Texas Instruments DP83822 PHY
+ *
+ * Copyright (C) 2017 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/ethtool.h>
+#include <linux/etherdevice.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/netdevice.h>
+
+#define DP83822_PHY_ID	        0x2000a240
+#define DP83822_DEVADDR		0x1f
+
+#define MII_DP83822_MISR1	0x12
+#define MII_DP83822_MISR2	0x13
+#define MII_DP83822_RESET_CTRL	0x1f
+
+#define DP83822_HW_RESET	BIT(15)
+#define DP83822_SW_RESET	BIT(14)
+
+/* MISR1 bits */
+#define DP83822_RX_ERR_HF_INT_EN	BIT(0)
+#define DP83822_FALSE_CARRIER_HF_INT_EN	BIT(1)
+#define DP83822_ANEG_COMPLETE_INT_EN	BIT(2)
+#define DP83822_DUP_MODE_CHANGE_INT_EN	BIT(3)
+#define DP83822_SPEED_CHANGED_INT_EN	BIT(4)
+#define DP83822_LINK_STAT_INT_EN	BIT(5)
+#define DP83822_ENERGY_DET_INT_EN	BIT(6)
+#define DP83822_LINK_QUAL_INT_EN	BIT(7)
+
+/* MISR2 bits */
+#define DP83822_JABBER_DET_INT_EN	BIT(0)
+#define DP83822_WOL_PKT_INT_EN		BIT(1)
+#define DP83822_SLEEP_MODE_INT_EN	BIT(2)
+#define DP83822_MDI_XOVER_INT_EN	BIT(3)
+#define DP83822_LB_FIFO_INT_EN		BIT(4)
+#define DP83822_PAGE_RX_INT_EN		BIT(5)
+#define DP83822_ANEG_ERR_INT_EN		BIT(6)
+#define DP83822_EEE_ERROR_CHANGE_INT_EN	BIT(7)
+
+/* INT_STAT1 bits */
+#define DP83822_WOL_INT_EN	BIT(4)
+#define DP83822_WOL_INT_STAT	BIT(12)
+
+#define MII_DP83822_RXSOP1	0x04A5
+#define	MII_DP83822_RXSOP2	0x04A6
+#define	MII_DP83822_RXSOP3	0x04A7
+
+/* WoL Registers */
+#define	MII_DP83822_WOL_CFG	0x04A0
+#define	MII_DP83822_WOL_STAT	0x04A1
+#define	MII_DP83822_WOL_DA1	0x04A2
+#define	MII_DP83822_WOL_DA2	0x04A3
+#define	MII_DP83822_WOL_DA3	0x04A4
+
+/* WoL bits */
+#define DP83822_WOL_MAGIC_EN	BIT(1)
+#define DP83822_WOL_SECURE_ON	BIT(5)
+#define DP83822_WOL_EN		BIT(7)
+#define DP83822_WOL_INDICATION_SEL BIT(8)
+#define DP83822_WOL_CLR_INDICATION BIT(11)
+
+static int dp83822_ack_interrupt(struct phy_device *phydev)
+{
+	int err = phy_read(phydev, MII_DP83822_MISR1);
+
+	if (err < 0)
+		return err;
+
+	err = phy_read(phydev, MII_DP83822_MISR2);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int dp83822_set_wol(struct phy_device *phydev,
+			   struct ethtool_wolinfo *wol)
+{
+	struct net_device *ndev = phydev->attached_dev;
+	u16 value;
+	const u8 *mac;
+
+	if (wol->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) {
+		mac = (const u8 *)ndev->dev_addr;
+
+		if (!is_valid_ether_addr(mac))
+			return -EFAULT;
+
+		/* MAC addresses start with byte 5, but stored in mac[0].
+		 * 822 PHYs store bytes 4|5, 2|3, 0|1
+		 */
+		phy_write_mmd(phydev, DP83822_DEVADDR,
+			      MII_DP83822_WOL_DA1, (mac[1] << 8) | mac[0]);
+		phy_write_mmd(phydev, DP83822_DEVADDR,
+			      MII_DP83822_WOL_DA2, (mac[3] << 8) | mac[2]);
+		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_DA3,
+			      (mac[5] << 8) | mac[4]);
+
+		value = phy_read_mmd(phydev, DP83822_DEVADDR,
+				     MII_DP83822_WOL_CFG);
+		if (wol->wolopts & WAKE_MAGIC)
+			value |= DP83822_WOL_MAGIC_EN;
+		else
+			value &= ~DP83822_WOL_MAGIC_EN;
+
+		if (wol->wolopts & WAKE_MAGICSECURE) {
+			value |= DP83822_WOL_SECURE_ON;
+			phy_write_mmd(phydev, DP83822_DEVADDR,
+				      MII_DP83822_RXSOP1,
+				      (wol->sopass[1] << 8) | wol->sopass[0]);
+			phy_write_mmd(phydev, DP83822_DEVADDR,
+				      MII_DP83822_RXSOP2,
+				      (wol->sopass[3] << 8) | wol->sopass[2]);
+			phy_write_mmd(phydev, DP83822_DEVADDR,
+				      MII_DP83822_RXSOP3,
+				      (wol->sopass[5] << 8) | wol->sopass[4]);
+		} else {
+			value &= ~DP83822_WOL_SECURE_ON;
+		}
+
+		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |
+			  DP83822_WOL_CLR_INDICATION);
+		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
+			      value);
+	} else {
+		value =
+		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
+		value &= (~DP83822_WOL_EN);
+		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
+			      value);
+	}
+
+	return 0;
+}
+
+static void dp83822_get_wol(struct phy_device *phydev,
+			    struct ethtool_wolinfo *wol)
+{
+	int value;
+
+	wol->supported = (WAKE_MAGIC | WAKE_MAGICSECURE);
+	wol->wolopts = 0;
+
+	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
+	if (value & DP83822_WOL_MAGIC_EN)
+		wol->wolopts |= WAKE_MAGIC;
+
+	if (value & DP83822_WOL_SECURE_ON)
+		wol->wolopts |= WAKE_MAGICSECURE;
+
+	if (~value & DP83822_WOL_CLR_INDICATION)
+		wol->wolopts = 0;
+
+	wol->sopass[0] = (phy_read_mmd(phydev,
+				       DP83822_DEVADDR,
+				       MII_DP83822_RXSOP1) & 0xFF);
+	wol->sopass[1] =
+	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP1) >> 8);
+	wol->sopass[2] =
+	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) & 0xFF);
+	wol->sopass[3] =
+	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2) >> 8);
+	wol->sopass[4] =
+	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) & 0xFF);
+	wol->sopass[5] =
+	    (phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3) >> 8);
+}
+
+static int dp83822_config_intr(struct phy_device *phydev)
+{
+	int misr_status;
+	int err;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		misr_status = phy_read(phydev, MII_DP83822_MISR1);
+		if (misr_status < 0)
+			return misr_status;
+
+		misr_status |= (DP83822_RX_ERR_HF_INT_EN |
+				DP83822_FALSE_CARRIER_HF_INT_EN |
+				DP83822_ANEG_COMPLETE_INT_EN |
+				DP83822_DUP_MODE_CHANGE_INT_EN |
+				DP83822_SPEED_CHANGED_INT_EN |
+				DP83822_LINK_STAT_INT_EN |
+				DP83822_ENERGY_DET_INT_EN |
+				DP83822_LINK_QUAL_INT_EN);
+
+		err = phy_write(phydev, MII_DP83822_MISR1, misr_status);
+		if (err < 0)
+			return err;
+
+		misr_status = phy_read(phydev, MII_DP83822_MISR2);
+		if (misr_status < 0)
+			return misr_status;
+
+		misr_status |= (DP83822_JABBER_DET_INT_EN |
+				DP83822_WOL_PKT_INT_EN |
+				DP83822_SLEEP_MODE_INT_EN |
+				DP83822_MDI_XOVER_INT_EN |
+				DP83822_LB_FIFO_INT_EN |
+				DP83822_PAGE_RX_INT_EN |
+				DP83822_ANEG_ERR_INT_EN |
+				DP83822_EEE_ERROR_CHANGE_INT_EN);
+
+		err = phy_write(phydev, MII_DP83822_MISR2, misr_status);
+	} else {
+		err = phy_write(phydev, MII_DP83822_MISR1, 0);
+		if (err < 0)
+			return err;
+
+		err = phy_write(phydev, MII_DP83822_MISR1, 0);
+	}
+
+	return err;
+}
+
+static int dp83822_phy_reset(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_write(phydev, MII_DP83822_RESET_CTRL, DP83822_HW_RESET);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int dp83822_suspend(struct phy_device *phydev)
+{
+	int value;
+
+	mutex_lock(&phydev->lock);
+
+	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
+	if (~value & DP83822_WOL_EN) {
+		value = phy_read(phydev, MII_BMCR);
+		phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);
+	}
+
+	mutex_unlock(&phydev->lock);
+
+	return 0;
+}
+
+static int dp83822_resume(struct phy_device *phydev)
+{
+	int value;
+
+	mutex_lock(&phydev->lock);
+
+	value = phy_read(phydev, MII_BMCR);
+	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);
+
+	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
+
+	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |
+		      DP83822_WOL_CLR_INDICATION);
+
+	mutex_unlock(&phydev->lock);
+
+	return 0;
+}
+
+static struct phy_driver dp83822_driver[] = {
+	{
+	 .phy_id = DP83822_PHY_ID,
+	 .phy_id_mask = 0xfffffff0,
+	 .name = "TI DP83822",
+	 .features = PHY_BASIC_FEATURES,
+	 .flags = PHY_HAS_INTERRUPT,
+
+	 .config_init = genphy_config_init,
+	 .soft_reset = dp83822_phy_reset,
+
+	 .get_wol = dp83822_get_wol,
+	 .set_wol = dp83822_set_wol,
+
+	 /* IRQ related */
+	 .ack_interrupt = dp83822_ack_interrupt,
+	 .config_intr = dp83822_config_intr,
+
+	 .config_aneg = genphy_config_aneg,
+	 .read_status = genphy_read_status,
+	 .suspend = dp83822_suspend,
+	 .resume = dp83822_resume,
+	 },
+};
+module_phy_driver(dp83822_driver);
+
+static struct mdio_device_id __maybe_unused dp83822_tbl[] = {
+	{ DP83822_PHY_ID, 0xfffffff0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, dp83822_tbl);
+
+MODULE_DESCRIPTION("Texas Instruments DP83822 PHY driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com");
+MODULE_LICENSE("GPL");