[2/3,v2] net: phy: DP83822 initial driver submission

Message ID 20171004182031.13794-2-dmurphy@ti.com
State Superseded
Headers show
Series
  • [1/3,v2] net: phy: Remove TI DP83822 from DP83848 driver
Related show

Commit Message

Dan Murphy Oct. 4, 2017, 6:20 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>

---

v2 - Updated per comments.  Removed unnessary parantheis, called genphy_suspend/
resume routines and then performing WoL changes, reworked sopass storage and reduced
the number of phy reads, and moved WOL_SECURE_ON - 
https://www.mail-archive.com/netdev@vger.kernel.org/msg191392.html

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

-- 
2.14.0

Comments

woojung.huh@microchip.com Oct. 4, 2017, 10:44 p.m. | #1
> +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);

Would we need mutex to access phy_read_mmd()?
phy_read_mmd() has mdio_lock for indirect access.

> +	if (!(value & DP83822_WOL_EN))

> +		genphy_suspend(phydev);

> +

> +	return 0;

> +}

> +

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

Same here.

Woojung
Andrew Lunn Oct. 4, 2017, 11:53 p.m. | #2
On Wed, Oct 04, 2017 at 10:44:36PM +0000, Woojung.Huh@microchip.com wrote:
> > +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);


> Would we need mutex to access phy_read_mmd()?

> phy_read_mmd() has mdio_lock for indirect access.


Hi Woojung

The mdio lock is not sufficient. It protects against two mdio
accesses. But here we need to protect against two phy operations.
There is a danger something else tries to access the phy during
suspend.

> > +	if (!(value & DP83822_WOL_EN))

> > +		genphy_suspend(phydev);


Releasing the lock before calling genphy_suspend() is not so nice.
Maybe add a version which assumes the lock has already been taken?

      Andrew
Dan Murphy Oct. 5, 2017, 8:06 p.m. | #3
Andrew

On 10/04/2017 06:53 PM, Andrew Lunn wrote:
> On Wed, Oct 04, 2017 at 10:44:36PM +0000, Woojung.Huh@microchip.com wrote:

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

> 

>> Would we need mutex to access phy_read_mmd()?

>> phy_read_mmd() has mdio_lock for indirect access.

> 

> Hi Woojung

> 

> The mdio lock is not sufficient. It protects against two mdio

> accesses. But here we need to protect against two phy operations.

> There is a danger something else tries to access the phy during

> suspend.

> 

>>> +	if (!(value & DP83822_WOL_EN))

>>> +		genphy_suspend(phydev);

> 

> Releasing the lock before calling genphy_suspend() is not so nice.

> Maybe add a version which assumes the lock has already been taken?

> 


The marvell driver does not take a lock and calls genphy_suspend/resume
so I am wondering if this driver needs to take a lock.

The at803x needs to take the lock because it does not call into the genphy
functions.

Dan

>       Andrew

> 



-- 
------------------
Dan Murphy
woojung.huh@microchip.com Oct. 5, 2017, 10:16 p.m. | #4
> > > +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);

> 

> > Would we need mutex to access phy_read_mmd()?

> > phy_read_mmd() has mdio_lock for indirect access.

> 

> Hi Woojung

> 

> The mdio lock is not sufficient. It protects against two mdio

> accesses. But here we need to protect against two phy operations.

> There is a danger something else tries to access the phy during

> suspend.


Hi Andrew,

Thanks for comment. I have question then.
phy_read_mmd() is protected by bus->mdio_lock around mmd_indirect & mdio_read.
While these operation, other phy_read() & phy_write() will be blocked inside
mdiobus_read() & mdiobus_write().

Because this phy_read_mmd(.., DP83822_DEVADDR, MII_DP83822_WOL_CFG) is not 
read-modify-write operation, I think phydev->lock may not be necessary.

Am I missing something?

> > > +	if (!(value & DP83822_WOL_EN))

> > > +		genphy_suspend(phydev);

> 

> Releasing the lock before calling genphy_suspend() is not so nice.

> Maybe add a version which assumes the lock has already been taken?

> 

We can expand genphy_suspend() per setting by phy_driver->set_wol.
When genphy_suspend() acts per wol setting, not many phy driver needs to extra work
When WOL is enabled.
How do you think?

- Woojung
Dan Murphy Oct. 6, 2017, noon | #5
Andrew

On 10/04/2017 06:53 PM, Andrew Lunn wrote:
> On Wed, Oct 04, 2017 at 10:44:36PM +0000, Woojung.Huh@microchip.com wrote:

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

> 

>> Would we need mutex to access phy_read_mmd()?

>> phy_read_mmd() has mdio_lock for indirect access.

> 

> Hi Woojung

> 

> The mdio lock is not sufficient. It protects against two mdio

> accesses. But here we need to protect against two phy operations.

> There is a danger something else tries to access the phy during

> suspend.

> 

>>> +	if (!(value & DP83822_WOL_EN))

>>> +		genphy_suspend(phydev);

> 

> Releasing the lock before calling genphy_suspend() is not so nice.

> Maybe add a version which assumes the lock has already been taken?

> 


The marvell driver does not take a lock and calls genphy_suspend/resume
so I am wondering if this driver needs to take a lock.

The at803x needs to take the lock because it does not call into the genphy
functions.

I will submit a new version with the lock removed.

Dan

>       Andrew

> 



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

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cd931cf9dcc2..8e78a482e09e 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 416df92fbf4f..df3b82ba8550 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 000000000000..200a0e39756e
--- /dev/null
+++ b/drivers/net/phy/dp83822.c
@@ -0,0 +1,306 @@ 
+/*
+ * 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 -EINVAL;
+
+		/* 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) {
+			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]);
+			value |= DP83822_WOL_SECURE_ON;
+		} 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;
+	u16 sopass_val;
+
+	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_CLR_INDICATION)
+		wol->wolopts = 0;
+
+	if (value & DP83822_WOL_SECURE_ON) {
+		wol->wolopts |= WAKE_MAGICSECURE;
+	} else {
+		wol->wolopts &= ~WAKE_MAGICSECURE;
+		return;
+	}
+
+	sopass_val = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP1);
+	wol->sopass[0] = (sopass_val & 0xff);
+	wol->sopass[1] = (sopass_val >> 8);
+
+	sopass_val = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP2);
+	wol->sopass[2] = (sopass_val & 0xff);
+	wol->sopass[3] = (sopass_val >> 8);
+
+	sopass_val = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RXSOP3);
+	wol->sopass[4] = (sopass_val & 0xff);
+	wol->sopass[5] = (sopass_val >> 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);
+	mutex_unlock(&phydev->lock);
+
+	if (!(value & DP83822_WOL_EN))
+		genphy_suspend(phydev);
+
+	return 0;
+}
+
+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;
+}
+
+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,
+	 .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");