Add a driver for the motorcomm yt8531 gigabit ethernet phy. We have verified
the patch on AM335x platform which has one YT8531 interface
card and passed all test cases.
The tested cases indluding: YT8531 UTP function with support of 10M/100M/1000M
and wol(based on magic packet).
Signed-off-by: Frank <[email protected]>
---
drivers/net/phy/Kconfig | 2 +-
drivers/net/phy/motorcomm.c | 162 ++++++++++++++++++++++++++++++++++--
2 files changed, 158 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index af00cf44cd97..4291fdfbb600 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -260,7 +260,7 @@ config MOTORCOMM_PHY
tristate "Motorcomm PHYs"
help
Enables support for Motorcomm network PHYs.
- Currently supports the YT8511, YT8521, YT8531S Gigabit Ethernet PHYs.
+ Currently supports the YT8511, YT8521, YT8531, YT8531S Gigabit Ethernet PHYs.
config NATIONAL_PHY
tristate "National Semiconductor PHYs"
diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 685190db72de..e38d7a0b7beb 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0+
/*
- * Motorcomm 8511/8521/8531S PHY driver.
+ * Motorcomm 8511/8521/8531/8531S PHY driver.
*
* Author: Peter Geis <[email protected]>
* Author: Frank <[email protected]>
@@ -12,8 +12,9 @@
#include <linux/phy.h>
#define PHY_ID_YT8511 0x0000010a
-#define PHY_ID_YT8521 0x0000011A
-#define PHY_ID_YT8531S 0x4F51E91A
+#define PHY_ID_YT8521 0x0000011a
+#define PHY_ID_YT8531 0x4f51e91b
+#define PHY_ID_YT8531S 0x4f51e91a
/* YT8521/YT8531S Register Overview
* UTP Register space | FIBER Register space
@@ -225,6 +226,9 @@
#define YT8531S_SYNCE_CFG_REG 0xA012
#define YT8531S_SCR_SYNCE_ENABLE BIT(6)
+#define YT8531_SYNCE_CFG_REG 0xA012
+#define YT8531_SCR_SYNCE_ENABLE BIT(6)
+
/* Extended Register end */
struct yt8521_priv {
@@ -479,6 +483,77 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
return phy_restore_page(phydev, old_page, ret);
}
+/**
+ * yt8531_set_wol() - turn wake-on-lan on or off
+ * @phydev: a pointer to a &struct phy_device
+ * @wol: a pointer to a &struct ethtool_wolinfo
+ *
+ * NOTE: YTPHY_WOL_CONFIG_REG, YTPHY_WOL_MACADDR2_REG, YTPHY_WOL_MACADDR1_REG
+ * and YTPHY_WOL_MACADDR0_REG are common ext reg.
+ *
+ * returns 0 or negative errno code
+ */
+static int yt8531_set_wol(struct phy_device *phydev,
+ struct ethtool_wolinfo *wol)
+{
+ struct net_device *p_attached_dev;
+ const u16 mac_addr_reg[] = {
+ YTPHY_WOL_MACADDR2_REG,
+ YTPHY_WOL_MACADDR1_REG,
+ YTPHY_WOL_MACADDR0_REG,
+ };
+ const u8 *mac_addr;
+ u16 mask;
+ u16 val;
+ int ret;
+ u8 i;
+
+ if (wol->wolopts & WAKE_MAGIC) {
+ p_attached_dev = phydev->attached_dev;
+ if (!p_attached_dev)
+ return -ENODEV;
+
+ mac_addr = (const u8 *)p_attached_dev->dev_addr;
+ if (!is_valid_ether_addr(mac_addr))
+ return -EINVAL;
+
+ /* Store the device address for the magic packet */
+ for (i = 0; i < 3; i++) {
+ ret = ytphy_write_ext(phydev, mac_addr_reg[i],
+ ((mac_addr[i * 2] << 8)) |
+ (mac_addr[i * 2 + 1]));
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Enable WOL feature */
+ mask = YTPHY_WCR_PULSE_WIDTH_MASK | YTPHY_WCR_INTR_SEL;
+ val = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
+ val |= YTPHY_WCR_TYPE_PULSE | YTPHY_WCR_PULSE_WIDTH_672MS;
+ ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, val);
+ if (ret < 0)
+ return ret;
+
+ /* Enable WOL interrupt */
+ ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG, 0,
+ YTPHY_IER_WOL);
+ if (ret < 0)
+ return ret;
+ } else {
+ /* Disable WOL feature */
+ mask = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
+ ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, 0);
+
+ /* Disable WOL interrupt */
+ ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG,
+ YTPHY_IER_WOL, 0);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
static int yt8511_read_page(struct phy_device *phydev)
{
return __phy_read(phydev, YT8511_PAGE_SELECT);
@@ -651,6 +726,19 @@ static int yt8521_probe(struct phy_device *phydev)
return 0;
}
+/**
+ * yt8531_probe() - Now only disable SyncE clock output
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * returns 0 or negative errno code
+ */
+static int yt8531_probe(struct phy_device *phydev)
+{
+ /* Disable SyncE clock output by default */
+ return ytphy_modify_ext_with_lock(phydev, YT8531_SYNCE_CFG_REG,
+ YT8531_SCR_SYNCE_ENABLE, 0);
+}
+
/**
* yt8531s_probe() - read chip config then set suitable polling_mode
* @phydev: a pointer to a &struct phy_device
@@ -1192,6 +1280,59 @@ static int yt8521_config_init(struct phy_device *phydev)
return phy_restore_page(phydev, old_page, ret);
}
+/**
+ * yt8531_config_init() - called to initialize the PHY
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * returns 0 or negative errno code
+ */
+static int yt8531_config_init(struct phy_device *phydev)
+{
+ int ret;
+ u16 val;
+
+ switch (phydev->interface) {
+ case PHY_INTERFACE_MODE_RGMII:
+ val = YT8521_RC1R_GE_TX_DELAY_DIS | YT8521_RC1R_FE_TX_DELAY_DIS;
+ val |= YT8521_RC1R_RX_DELAY_DIS;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ val = YT8521_RC1R_GE_TX_DELAY_DIS | YT8521_RC1R_FE_TX_DELAY_DIS;
+ val |= YT8521_RC1R_RX_DELAY_EN;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ val = YT8521_RC1R_GE_TX_DELAY_EN | YT8521_RC1R_FE_TX_DELAY_EN;
+ val |= YT8521_RC1R_RX_DELAY_DIS;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ val = YT8521_RC1R_GE_TX_DELAY_EN | YT8521_RC1R_FE_TX_DELAY_EN;
+ val |= YT8521_RC1R_RX_DELAY_EN;
+ break;
+ default: /* do not support other modes */
+ return -EOPNOTSUPP;
+ }
+
+ /* set rgmii delay mode */
+ ret = ytphy_modify_ext_with_lock(phydev, YT8521_RGMII_CONFIG1_REG,
+ (YT8521_RC1R_RX_DELAY_MASK |
+ YT8521_RC1R_FE_TX_DELAY_MASK |
+ YT8521_RC1R_GE_TX_DELAY_MASK),
+ val);
+ if (ret < 0)
+ return ret;
+
+ /* disable auto sleep */
+ ret = ytphy_modify_ext_with_lock(phydev,
+ YT8521_EXTREG_SLEEP_CONTROL1_REG,
+ YT8521_ESC1R_SLEEP_SW, 0);
+ if (ret < 0)
+ return ret;
+
+ /* enable RXC clock when no wire plug */
+ return ytphy_modify_ext_with_lock(phydev, YT8521_CLOCK_GATING_REG,
+ YT8521_CGR_RX_CLK_EN, 0);
+}
+
/**
* yt8521_prepare_fiber_features() - A small helper function that setup
* fiber's features.
@@ -1774,6 +1915,16 @@ static struct phy_driver motorcomm_phy_drvs[] = {
.suspend = yt8521_suspend,
.resume = yt8521_resume,
},
+ {
+ PHY_ID_MATCH_EXACT(PHY_ID_YT8531),
+ .name = "YT8531 Gigabit Ethernet",
+ .probe = yt8531_probe,
+ .config_init = yt8531_config_init,
+ .suspend = genphy_suspend,
+ .resume = genphy_resume,
+ .get_wol = ytphy_get_wol,
+ .set_wol = yt8531_set_wol,
+ },
{
PHY_ID_MATCH_EXACT(PHY_ID_YT8531S),
.name = "YT8531S Gigabit Ethernet",
@@ -1795,7 +1946,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
module_phy_driver(motorcomm_phy_drvs);
-MODULE_DESCRIPTION("Motorcomm 8511/8521/8531S PHY driver");
+MODULE_DESCRIPTION("Motorcomm 8511/8521/8531/8531S PHY driver");
MODULE_AUTHOR("Peter Geis");
MODULE_AUTHOR("Frank");
MODULE_LICENSE("GPL");
@@ -1803,8 +1954,9 @@ MODULE_LICENSE("GPL");
static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
{ PHY_ID_MATCH_EXACT(PHY_ID_YT8521) },
+ { PHY_ID_MATCH_EXACT(PHY_ID_YT8531) },
{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
- { /* sentinal */ }
+ { /* sentinel */ }
};
MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);
--
2.34.1
On Wed, Nov 30, 2022 at 05:49:28PM +0800, Frank wrote:
> +/**
> + * yt8531_set_wol() - turn wake-on-lan on or off
> + * @phydev: a pointer to a &struct phy_device
> + * @wol: a pointer to a &struct ethtool_wolinfo
> + *
> + * NOTE: YTPHY_WOL_CONFIG_REG, YTPHY_WOL_MACADDR2_REG, YTPHY_WOL_MACADDR1_REG
> + * and YTPHY_WOL_MACADDR0_REG are common ext reg.
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8531_set_wol(struct phy_device *phydev,
> + struct ethtool_wolinfo *wol)
> +{
So this is called from the .set_wol method directly, and won't have the
MDIO bus lock taken...
> + struct net_device *p_attached_dev;
> + const u16 mac_addr_reg[] = {
> + YTPHY_WOL_MACADDR2_REG,
> + YTPHY_WOL_MACADDR1_REG,
> + YTPHY_WOL_MACADDR0_REG,
> + };
> + const u8 *mac_addr;
> + u16 mask;
> + u16 val;
> + int ret;
> + u8 i;
> +
> + if (wol->wolopts & WAKE_MAGIC) {
> + p_attached_dev = phydev->attached_dev;
> + if (!p_attached_dev)
> + return -ENODEV;
> +
> + mac_addr = (const u8 *)p_attached_dev->dev_addr;
> + if (!is_valid_ether_addr(mac_addr))
> + return -EINVAL;
> +
> + /* Store the device address for the magic packet */
> + for (i = 0; i < 3; i++) {
> + ret = ytphy_write_ext(phydev, mac_addr_reg[i],
> + ((mac_addr[i * 2] << 8)) |
> + (mac_addr[i * 2 + 1]));
This accesses the MDIO bus without taking the lock.
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* Enable WOL feature */
> + mask = YTPHY_WCR_PULSE_WIDTH_MASK | YTPHY_WCR_INTR_SEL;
> + val = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
> + val |= YTPHY_WCR_TYPE_PULSE | YTPHY_WCR_PULSE_WIDTH_672MS;
> + ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, val);
This accesses the MDIO bus without taking the lock.
> + if (ret < 0)
> + return ret;
> +
> + /* Enable WOL interrupt */
> + ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG, 0,
> + YTPHY_IER_WOL);
This accesses the MDIO bus without taking the lock.
> + if (ret < 0)
> + return ret;
> + } else {
> + /* Disable WOL feature */
> + mask = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
> + ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, 0);
This accesses the MDIO bus without taking the lock.
> +
> + /* Disable WOL interrupt */
> + ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG,
> + YTPHY_IER_WOL, 0);
This accesses the MDIO bus without taking the lock.
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
Which makes this function entirely unsafe as another thread can change
the YTPHY_PAGE_SELECT register between writing that register and
accessing the YTPHY_PAGE_DATA register.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 30, 2022 at 09:55:50AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 30, 2022 at 05:49:28PM +0800, Frank wrote:
> > +/**
> > + * yt8531_set_wol() - turn wake-on-lan on or off
> > + * @phydev: a pointer to a &struct phy_device
> > + * @wol: a pointer to a &struct ethtool_wolinfo
> > + *
> > + * NOTE: YTPHY_WOL_CONFIG_REG, YTPHY_WOL_MACADDR2_REG, YTPHY_WOL_MACADDR1_REG
> > + * and YTPHY_WOL_MACADDR0_REG are common ext reg.
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int yt8531_set_wol(struct phy_device *phydev,
> > + struct ethtool_wolinfo *wol)
> > +{
>
> So this is called from the .set_wol method directly, and won't have the
> MDIO bus lock taken...
Hi Frank
This is not the first time Russell has pointed out your locking is
wrong.
How about adding a check in functions which should be called with the
lock taken really do have the lock taken?
ASSERT_RTNL() but for an MDIO bus.
Andrew
Hi Andrew,
On Wed, Nov 30, 2022 at 06:10:42PM +0100, Andrew Lunn wrote:
> This is not the first time Russell has pointed out your locking is
> wrong.
>
> How about adding a check in functions which should be called with the
> lock taken really do have the lock taken?
They already do:
lockdep_assert_held_once(&bus->mdio_lock);
but I guess people just aren't testing their code with lockdep enabled.
The only other thing I can think of trying is to use mutex_trylock():
if (WARN_ON_ONCE(mutex_trylock(&bus->mdio_lock)))
mutex_unlock(&bus->mdio_lock);
scattered throughout.
However, if the author does have lockdep enabled but ignores the
warnings, that isn't going to help.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 30, 2022 at 05:33:15PM +0000, Russell King (Oracle) wrote:
> Hi Andrew,
>
> On Wed, Nov 30, 2022 at 06:10:42PM +0100, Andrew Lunn wrote:
> > This is not the first time Russell has pointed out your locking is
> > wrong.
> >
> > How about adding a check in functions which should be called with the
> > lock taken really do have the lock taken?
>
> They already do:
>
> lockdep_assert_held_once(&bus->mdio_lock);
>
> but I guess people just aren't testing their code with lockdep enabled.
>
> The only other thing I can think of trying is to use mutex_trylock():
>
> if (WARN_ON_ONCE(mutex_trylock(&bus->mdio_lock)))
> mutex_unlock(&bus->mdio_lock);
>
> scattered throughout.
The ASSERT_RTNL() macro does this, it does not depend on lockdep.
And given the persistent sort of problems we have seen, you are
probably correct, lockdep is not being enabled by some developers. I
guess they don't even know what it is.
Andrew