[PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Peter Geis]

Add a driver for the Motorcomm yt8511 phy that will be used in the
production Pine64 rk3566-quartz64 development board.
It supports gigabit transfer speeds, rgmii, and 125mhz clk output.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
Changes v3:
- Add rgmii mode selection support

Changes v2:
- Change to __phy_modify
- Handle return errors
- Remove unnecessary &

 MAINTAINERS                 |   6 ++
 drivers/net/phy/Kconfig     |   6 ++
 drivers/net/phy/Makefile    |   1 +
 drivers/net/phy/motorcomm.c | 121 ++++++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+)
 create mode 100644 drivers/net/phy/motorcomm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 601b5ae0368a..2a2e406238fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12388,6 +12388,12 @@ F:	Documentation/userspace-api/media/drivers/meye*
 F:	drivers/media/pci/meye/
 F:	include/uapi/linux/meye.h
 
+MOTORCOMM PHY DRIVER
+M:	Peter Geis <pgwipeout@gmail.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/motorcomm.c
+
 MOXA SMARTIO/INDUSTIO/INTELLIO SERIAL CARD
 S:	Orphan
 F:	Documentation/driver-api/serial/moxa-smartio.rst
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 288bf405ebdb..16db9f8037b5 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -229,6 +229,12 @@ config MICROSEMI_PHY
 	help
 	  Currently supports VSC8514, VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
 
+config MOTORCOMM_PHY
+	tristate "Motorcomm PHYs"
+	help
+	  Enables support for Motorcomm network PHYs.
+	  Currently supports the YT8511 gigabit PHY.
+
 config NATIONAL_PHY
 	tristate "National Semiconductor PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index bcda7ed2455d..37ffbc6e3c87 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
+obj-$(CONFIG_MOTORCOMM_PHY)	+= motorcomm.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
 obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
new file mode 100644
index 000000000000..b85f10efa28e
--- /dev/null
+++ b/drivers/net/phy/motorcomm.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Motorcomm PHYs
+ *
+ * Author: Peter Geis <pgwipeout@gmail.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#define PHY_ID_YT8511		0x0000010a
+
+#define YT8511_PAGE_SELECT	0x1e
+#define YT8511_PAGE		0x1f
+#define YT8511_EXT_CLK_GATE	0x0c
+#define YT8511_EXT_SLEEP_CTRL	0x27
+
+/* 2b00 25m from pll
+ * 2b01 25m from xtl *default*
+ * 2b10 62.m from pll
+ * 2b11 125m from pll
+ */
+#define YT8511_CLK_125M		(BIT(2) | BIT(1))
+
+/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
+#define YT8511_DELAY_RX		BIT(0)
+
+/* TX Delay is bits 7:4, default 0x5
+ * Delay = 150ps * N - 250ps, Default = 500ps
+ */
+#define YT8511_DELAY_TX		(0x5 << 4)
+
+static int yt8511_read_page(struct phy_device *phydev)
+{
+	return __phy_read(phydev, YT8511_PAGE_SELECT);
+};
+
+static int yt8511_write_page(struct phy_device *phydev, int page)
+{
+	return __phy_write(phydev, YT8511_PAGE_SELECT, page);
+};
+
+static int yt8511_config_init(struct phy_device *phydev)
+{
+	int ret, oldpage, val;
+
+	/* set clock mode to 125mhz */
+	oldpage = phy_select_page(phydev, YT8511_EXT_CLK_GATE);
+	if (oldpage < 0)
+		goto err_restore_page;
+
+	ret = __phy_modify(phydev, YT8511_PAGE, 0, YT8511_CLK_125M);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* set rgmii delay mode */
+	val = __phy_read(phydev, YT8511_PAGE);
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val &= ~(YT8511_DELAY_TX);
+		val |= YT8511_DELAY_RX;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val &= ~(YT8511_DELAY_RX);
+		val |= YT8511_DELAY_TX;
+		break;
+	default: /* leave everything alone in other modes */
+		break;
+	}
+
+	ret = __phy_write(phydev, YT8511_PAGE, val);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* disable auto sleep */
+	ret = __phy_write(phydev, YT8511_PAGE_SELECT, YT8511_EXT_SLEEP_CTRL);
+	if (ret < 0)
+		goto err_restore_page;
+	ret = __phy_modify(phydev, YT8511_PAGE, BIT(15), 0);
+	if (ret < 0)
+		goto err_restore_page;
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
+static struct phy_driver motorcomm_phy_drvs[] = {
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
+		.name		= "YT8511 Gigabit Ethernet",
+		.config_init	= yt8511_config_init,
+		.get_features	= genphy_read_abilities,
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.read_page	= yt8511_read_page,
+		.write_page	= yt8511_write_page,
+	},
+};
+
+module_phy_driver(motorcomm_phy_drvs);
+
+MODULE_DESCRIPTION("Motorcomm PHY driver");
+MODULE_AUTHOR("Peter Geis");
+MODULE_LICENSE("GPL");
+
+static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
+	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
+	{ /* sentinal */ }
+};
+
+MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);
-- 
2.25.1

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Heiner Kallweit]

On 14.05.2021 13:58, Peter Geis wrote:
> Add a driver for the Motorcomm yt8511 phy that will be used in the
> production Pine64 rk3566-quartz64 development board.
> It supports gigabit transfer speeds, rgmii, and 125mhz clk output.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> Changes v3:
> - Add rgmii mode selection support
> 
> Changes v2:
> - Change to __phy_modify
> - Handle return errors
> - Remove unnecessary &
> 
>  MAINTAINERS                 |   6 ++
>  drivers/net/phy/Kconfig     |   6 ++
>  drivers/net/phy/Makefile    |   1 +
>  drivers/net/phy/motorcomm.c | 121 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 134 insertions(+)
>  create mode 100644 drivers/net/phy/motorcomm.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 601b5ae0368a..2a2e406238fc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12388,6 +12388,12 @@ F:	Documentation/userspace-api/media/drivers/meye*
>  F:	drivers/media/pci/meye/
>  F:	include/uapi/linux/meye.h
>  
> +MOTORCOMM PHY DRIVER
> +M:	Peter Geis <pgwipeout@gmail.com>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	drivers/net/phy/motorcomm.c
> +
>  MOXA SMARTIO/INDUSTIO/INTELLIO SERIAL CARD
>  S:	Orphan
>  F:	Documentation/driver-api/serial/moxa-smartio.rst
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 288bf405ebdb..16db9f8037b5 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -229,6 +229,12 @@ config MICROSEMI_PHY
>  	help
>  	  Currently supports VSC8514, VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
>  
> +config MOTORCOMM_PHY
> +	tristate "Motorcomm PHYs"
> +	help
> +	  Enables support for Motorcomm network PHYs.
> +	  Currently supports the YT8511 gigabit PHY.
> +
>  config NATIONAL_PHY
>  	tristate "National Semiconductor PHYs"
>  	help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index bcda7ed2455d..37ffbc6e3c87 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
>  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>  obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
>  obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
> +obj-$(CONFIG_MOTORCOMM_PHY)	+= motorcomm.o
>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
>  obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
>  obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> new file mode 100644
> index 000000000000..b85f10efa28e
> --- /dev/null
> +++ b/drivers/net/phy/motorcomm.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Motorcomm PHYs
> + *
> + * Author: Peter Geis <pgwipeout@gmail.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +
> +#define PHY_ID_YT8511		0x0000010a

This PHY ID looks weird, the OUI part is empty.
Looking here http://standards-oui.ieee.org/cid/cid.txt
it seems Motorcomm has been assigned at least a CID.
An invalid OUI leaves a good chance for a PHY ID conflict.

> +
> +#define YT8511_PAGE_SELECT	0x1e
> +#define YT8511_PAGE		0x1f
> +#define YT8511_EXT_CLK_GATE	0x0c
> +#define YT8511_EXT_SLEEP_CTRL	0x27
> +
> +/* 2b00 25m from pll
> + * 2b01 25m from xtl *default*
> + * 2b10 62.m from pll
> + * 2b11 125m from pll
> + */
> +#define YT8511_CLK_125M		(BIT(2) | BIT(1))
> +
> +/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
> +#define YT8511_DELAY_RX		BIT(0)
> +
> +/* TX Delay is bits 7:4, default 0x5
> + * Delay = 150ps * N - 250ps, Default = 500ps
> + */
> +#define YT8511_DELAY_TX		(0x5 << 4)
> +
> +static int yt8511_read_page(struct phy_device *phydev)
> +{
> +	return __phy_read(phydev, YT8511_PAGE_SELECT);
> +};
> +
> +static int yt8511_write_page(struct phy_device *phydev, int page)
> +{
> +	return __phy_write(phydev, YT8511_PAGE_SELECT, page);
> +};
> +
> +static int yt8511_config_init(struct phy_device *phydev)
> +{
> +	int ret, oldpage, val;
> +
> +	/* set clock mode to 125mhz */
> +	oldpage = phy_select_page(phydev, YT8511_EXT_CLK_GATE);
> +	if (oldpage < 0)
> +		goto err_restore_page;
> +
> +	ret = __phy_modify(phydev, YT8511_PAGE, 0, YT8511_CLK_125M);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* set rgmii delay mode */
> +	val = __phy_read(phydev, YT8511_PAGE);
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		val &= ~(YT8511_DELAY_TX);
> +		val |= YT8511_DELAY_RX;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val &= ~(YT8511_DELAY_RX);
> +		val |= YT8511_DELAY_TX;
> +		break;
> +	default: /* leave everything alone in other modes */
> +		break;
> +	}
> +
> +	ret = __phy_write(phydev, YT8511_PAGE, val);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* disable auto sleep */
> +	ret = __phy_write(phydev, YT8511_PAGE_SELECT, YT8511_EXT_SLEEP_CTRL);
> +	if (ret < 0)
> +		goto err_restore_page;
> +	ret = __phy_modify(phydev, YT8511_PAGE, BIT(15), 0);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +err_restore_page:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> +static struct phy_driver motorcomm_phy_drvs[] = {
> +	{
> +		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
> +		.name		= "YT8511 Gigabit Ethernet",
> +		.config_init	= yt8511_config_init,
> +		.get_features	= genphy_read_abilities,
> +		.config_aneg	= genphy_config_aneg,
> +		.read_status	= genphy_read_status,

These three genphy callbacks are fallbacks anyway.
So you don't have to set them.

> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= yt8511_read_page,
> +		.write_page	= yt8511_write_page,
> +	},
> +};
> +
> +module_phy_driver(motorcomm_phy_drvs);
> +
> +MODULE_DESCRIPTION("Motorcomm PHY driver");
> +MODULE_AUTHOR("Peter Geis");
> +MODULE_LICENSE("GPL");
> +
> +static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
> +	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
> +	{ /* sentinal */ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);
> 

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Russell King (Oracle)]

Hi!

On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:
> +	/* set rgmii delay mode */
> +	val = __phy_read(phydev, YT8511_PAGE);
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		val &= ~(YT8511_DELAY_TX);
> +		val |= YT8511_DELAY_RX;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val &= ~(YT8511_DELAY_RX);
> +		val |= YT8511_DELAY_TX;
> +		break;
> +	default: /* leave everything alone in other modes */
> +		break;
> +	}
> +
> +	ret = __phy_write(phydev, YT8511_PAGE, val);
> +	if (ret < 0)
> +		goto err_restore_page;

Another way of writing the above is to set "val" to be the value of the
YT8511_DELAY_RX and YT8511_DELAY_TX bits, and then do:

	ret = __phy_modify(phydev, YT8511_PAGE,
			   (YT8511_DELAY_RX | YT8511_DELAY_TX), val);
	if (ret < 0)
		goto err_restore_page;

which moves the read-modify-write out of the driver into core code and
makes the driver code smaller. It also handles your missing error check
on __phy_read() above - would you want the above code to attempt to
write a -ve error number back to this register? I suspect not!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Andrew Lunn]

On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:
> Add a driver for the Motorcomm yt8511 phy that will be used in the
> production Pine64 rk3566-quartz64 development board.
> It supports gigabit transfer speeds, rgmii, and 125mhz clk output.

Thanks for adding RGMII support.

> +#define PHY_ID_YT8511		0x0000010a

No OUI in the PHY ID?

Humm, the datasheet says it defaults to zero. That is not very
good. This could be a source of problems in the future, if some other
manufacture also does not use an OUI.

> +/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
> +#define YT8511_DELAY_RX		BIT(0)
> +
> +/* TX Delay is bits 7:4, default 0x5
> + * Delay = 150ps * N - 250ps, Default = 500ps
> + */
> +#define YT8511_DELAY_TX		(0x5 << 4)

> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> +		break;

This is not correct. YT8511_DELAY_TX will only mask the 2 bits in 0x5,
not all the bits in 7:4. And since the formula is

Delay = 150ps * N - 250ps

setting N to 0 is not what you want. You probably want N=2, so you end up with 50ps

> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		val &= ~(YT8511_DELAY_TX);
> +		val |= YT8511_DELAY_RX;

The delay should be around 2ns. For RX you only have 1.8ns, which is
probably good enough. But for TX you have more flexibility. You are
setting it to the default of 500ps which is too small. I would suggest
1.85ns, N=14, so it is the same as RX.

I also wonder about bits 15:12 of PHY EXT ODH: Delay and driver
strength CFG register.

	 Andrew

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Peter Geis]

On Fri, May 14, 2021 at 9:09 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 14.05.2021 13:58, Peter Geis wrote:
> > Add a driver for the Motorcomm yt8511 phy that will be used in the
> > production Pine64 rk3566-quartz64 development board.
> > It supports gigabit transfer speeds, rgmii, and 125mhz clk output.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> > Changes v3:
> > - Add rgmii mode selection support
> >
> > Changes v2:
> > - Change to __phy_modify
> > - Handle return errors
> > - Remove unnecessary &
> >
> >  MAINTAINERS                 |   6 ++
> >  drivers/net/phy/Kconfig     |   6 ++
> >  drivers/net/phy/Makefile    |   1 +
> >  drivers/net/phy/motorcomm.c | 121 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 134 insertions(+)
> >  create mode 100644 drivers/net/phy/motorcomm.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 601b5ae0368a..2a2e406238fc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12388,6 +12388,12 @@ F:   Documentation/userspace-api/media/drivers/meye*
> >  F:   drivers/media/pci/meye/
> >  F:   include/uapi/linux/meye.h
> >
> > +MOTORCOMM PHY DRIVER
> > +M:   Peter Geis <pgwipeout@gmail.com>
> > +L:   netdev@vger.kernel.org
> > +S:   Maintained
> > +F:   drivers/net/phy/motorcomm.c
> > +
> >  MOXA SMARTIO/INDUSTIO/INTELLIO SERIAL CARD
> >  S:   Orphan
> >  F:   Documentation/driver-api/serial/moxa-smartio.rst
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 288bf405ebdb..16db9f8037b5 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -229,6 +229,12 @@ config MICROSEMI_PHY
> >       help
> >         Currently supports VSC8514, VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
> >
> > +config MOTORCOMM_PHY
> > +     tristate "Motorcomm PHYs"
> > +     help
> > +       Enables support for Motorcomm network PHYs.
> > +       Currently supports the YT8511 gigabit PHY.
> > +
> >  config NATIONAL_PHY
> >       tristate "National Semiconductor PHYs"
> >       help
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index bcda7ed2455d..37ffbc6e3c87 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_MICREL_PHY)    += micrel.o
> >  obj-$(CONFIG_MICROCHIP_PHY)  += microchip.o
> >  obj-$(CONFIG_MICROCHIP_T1_PHY)       += microchip_t1.o
> >  obj-$(CONFIG_MICROSEMI_PHY)  += mscc/
> > +obj-$(CONFIG_MOTORCOMM_PHY)  += motorcomm.o
> >  obj-$(CONFIG_NATIONAL_PHY)   += national.o
> >  obj-$(CONFIG_NXP_C45_TJA11XX_PHY)    += nxp-c45-tja11xx.o
> >  obj-$(CONFIG_NXP_TJA11XX_PHY)        += nxp-tja11xx.o
> > diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> > new file mode 100644
> > index 000000000000..b85f10efa28e
> > --- /dev/null
> > +++ b/drivers/net/phy/motorcomm.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Motorcomm PHYs
> > + *
> > + * Author: Peter Geis <pgwipeout@gmail.com>
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/phy.h>
> > +
> > +#define PHY_ID_YT8511                0x0000010a
>
> This PHY ID looks weird, the OUI part is empty.
> Looking here http://standards-oui.ieee.org/cid/cid.txt
> it seems Motorcomm has been assigned at least a CID.
> An invalid OUI leaves a good chance for a PHY ID conflict.

Delightfully, that's what the OUI reports....
The only bits that aren't all 0s are the Type and Revision blocks.
I also haven't found a way to update the eeprom so I don't know how
this could get fixed.

>
> > +
> > +#define YT8511_PAGE_SELECT   0x1e
> > +#define YT8511_PAGE          0x1f
> > +#define YT8511_EXT_CLK_GATE  0x0c
> > +#define YT8511_EXT_SLEEP_CTRL        0x27
> > +
> > +/* 2b00 25m from pll
> > + * 2b01 25m from xtl *default*
> > + * 2b10 62.m from pll
> > + * 2b11 125m from pll
> > + */
> > +#define YT8511_CLK_125M              (BIT(2) | BIT(1))
> > +
> > +/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
> > +#define YT8511_DELAY_RX              BIT(0)
> > +
> > +/* TX Delay is bits 7:4, default 0x5
> > + * Delay = 150ps * N - 250ps, Default = 500ps
> > + */
> > +#define YT8511_DELAY_TX              (0x5 << 4)
> > +
> > +static int yt8511_read_page(struct phy_device *phydev)
> > +{
> > +     return __phy_read(phydev, YT8511_PAGE_SELECT);
> > +};
> > +
> > +static int yt8511_write_page(struct phy_device *phydev, int page)
> > +{
> > +     return __phy_write(phydev, YT8511_PAGE_SELECT, page);
> > +};
> > +
> > +static int yt8511_config_init(struct phy_device *phydev)
> > +{
> > +     int ret, oldpage, val;
> > +
> > +     /* set clock mode to 125mhz */
> > +     oldpage = phy_select_page(phydev, YT8511_EXT_CLK_GATE);
> > +     if (oldpage < 0)
> > +             goto err_restore_page;
> > +
> > +     ret = __phy_modify(phydev, YT8511_PAGE, 0, YT8511_CLK_125M);
> > +     if (ret < 0)
> > +             goto err_restore_page;
> > +
> > +     /* set rgmii delay mode */
> > +     val = __phy_read(phydev, YT8511_PAGE);
> > +
> > +     switch (phydev->interface) {
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +             val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +             val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             val &= ~(YT8511_DELAY_TX);
> > +             val |= YT8511_DELAY_RX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_TXID:
> > +             val &= ~(YT8511_DELAY_RX);
> > +             val |= YT8511_DELAY_TX;
> > +             break;
> > +     default: /* leave everything alone in other modes */
> > +             break;
> > +     }
> > +
> > +     ret = __phy_write(phydev, YT8511_PAGE, val);
> > +     if (ret < 0)
> > +             goto err_restore_page;
> > +
> > +     /* disable auto sleep */
> > +     ret = __phy_write(phydev, YT8511_PAGE_SELECT, YT8511_EXT_SLEEP_CTRL);
> > +     if (ret < 0)
> > +             goto err_restore_page;
> > +     ret = __phy_modify(phydev, YT8511_PAGE, BIT(15), 0);
> > +     if (ret < 0)
> > +             goto err_restore_page;
> > +
> > +err_restore_page:
> > +     return phy_restore_page(phydev, oldpage, ret);
> > +}
> > +
> > +static struct phy_driver motorcomm_phy_drvs[] = {
> > +     {
> > +             PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
> > +             .name           = "YT8511 Gigabit Ethernet",
> > +             .config_init    = yt8511_config_init,
> > +             .get_features   = genphy_read_abilities,
> > +             .config_aneg    = genphy_config_aneg,
> > +             .read_status    = genphy_read_status,
>
> These three genphy callbacks are fallbacks anyway.
> So you don't have to set them.

Okay, thanks!

>
> > +             .suspend        = genphy_suspend,
> > +             .resume         = genphy_resume,
> > +             .read_page      = yt8511_read_page,
> > +             .write_page     = yt8511_write_page,
> > +     },
> > +};
> > +
> > +module_phy_driver(motorcomm_phy_drvs);
> > +
> > +MODULE_DESCRIPTION("Motorcomm PHY driver");
> > +MODULE_AUTHOR("Peter Geis");
> > +MODULE_LICENSE("GPL");
> > +
> > +static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
> > +     { PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
> > +     { /* sentinal */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);
> >
>

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Peter Geis]

On Fri, May 14, 2021 at 9:09 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> Hi!
>
> On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:
> > +     /* set rgmii delay mode */
> > +     val = __phy_read(phydev, YT8511_PAGE);
> > +
> > +     switch (phydev->interface) {
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +             val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +             val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             val &= ~(YT8511_DELAY_TX);
> > +             val |= YT8511_DELAY_RX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_TXID:
> > +             val &= ~(YT8511_DELAY_RX);
> > +             val |= YT8511_DELAY_TX;
> > +             break;
> > +     default: /* leave everything alone in other modes */
> > +             break;
> > +     }
> > +
> > +     ret = __phy_write(phydev, YT8511_PAGE, val);
> > +     if (ret < 0)
> > +             goto err_restore_page;
>
> Another way of writing the above is to set "val" to be the value of the
> YT8511_DELAY_RX and YT8511_DELAY_TX bits, and then do:
>
>         ret = __phy_modify(phydev, YT8511_PAGE,
>                            (YT8511_DELAY_RX | YT8511_DELAY_TX), val);
>         if (ret < 0)
>                 goto err_restore_page;
>
> which moves the read-modify-write out of the driver into core code and
> makes the driver code smaller. It also handles your missing error check
> on __phy_read() above - would you want the above code to attempt to
> write a -ve error number back to this register? I suspect not!

That makes sense, thanks!
I was thinking about how to use __phy_modify with a functional mask,
but it didn't click until I had already sent it in.
Also thanks for catching handling that ret on the read!

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Peter Geis]

On Fri, May 14, 2021 at 9:24 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:
> > Add a driver for the Motorcomm yt8511 phy that will be used in the
> > production Pine64 rk3566-quartz64 development board.
> > It supports gigabit transfer speeds, rgmii, and 125mhz clk output.
>
> Thanks for adding RGMII support.
>
> > +#define PHY_ID_YT8511                0x0000010a
>
> No OUI in the PHY ID?
>
> Humm, the datasheet says it defaults to zero. That is not very
> good. This could be a source of problems in the future, if some other
> manufacture also does not use an OUI.
>
> > +/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
> > +#define YT8511_DELAY_RX              BIT(0)
> > +
> > +/* TX Delay is bits 7:4, default 0x5
> > + * Delay = 150ps * N - 250ps, Default = 500ps
> > + */
> > +#define YT8511_DELAY_TX              (0x5 << 4)
>
> > +
> > +     switch (phydev->interface) {
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +             val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> > +             break;
>
> This is not correct. YT8511_DELAY_TX will only mask the 2 bits in 0x5,
> not all the bits in 7:4. And since the formula is
>
> Delay = 150ps * N - 250ps
>
> setting N to 0 is not what you want. You probably want N=2, so you end up with 50ps

The manufacturer's driver set this to <0> to disable, but I agree the
datasheet disagrees with this.

>
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +             val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             val &= ~(YT8511_DELAY_TX);
> > +             val |= YT8511_DELAY_RX;
>
> The delay should be around 2ns. For RX you only have 1.8ns, which is
> probably good enough. But for TX you have more flexibility. You are
> setting it to the default of 500ps which is too small. I would suggest
> 1.85ns, N=14, so it is the same as RX.

Makes sense, these are the insights I was hoping for!

>
> I also wonder about bits 15:12 of PHY EXT ODH: Delay and driver
> strength CFG register.

The default value *works*, and from an emi perspective we want the
lowest strength single that is reliable.
Unfortunately I don't have the equipment to *test* the actual output
strengths and the datasheet isn't explicitly clear about them either.
This is one of the values I was considering having defined in the devicetree.

>
>          Andrew

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Andrew Lunn]

> > I also wonder about bits 15:12 of PHY EXT ODH: Delay and driver
> > strength CFG register.
> 
> The default value *works*, and from an emi perspective we want the
> lowest strength single that is reliable.

I was not meaning signal strength, but Txc_delay_sel_fe,

  selecte tx_clk_rgmii delay in chip which is used to latch txd_rgmii
  in 100BT/10BTe mode. 150ps step. Default value 15 means about 2ns
  clock delay compared to txd_rgmii in typical cornor.

[Typos courtesy of the datasheet, not me!]

This sounds like more RGMII delays. It seems like PHY EXT 0CH is about
1G mode, and PHY EXT 0DH is about 10/100 mode. I think you probably
need to set this bits as well. Have you tested against a link peer at
10 Half? 100 Full?

   Andrew

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Peter Geis]

On Fri, May 14, 2021 at 10:52 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > I also wonder about bits 15:12 of PHY EXT ODH: Delay and driver
> > > strength CFG register.
> >
> > The default value *works*, and from an emi perspective we want the
> > lowest strength single that is reliable.
>
> I was not meaning signal strength, but Txc_delay_sel_fe,
>
>   selecte tx_clk_rgmii delay in chip which is used to latch txd_rgmii
>   in 100BT/10BTe mode. 150ps step. Default value 15 means about 2ns
>   clock delay compared to txd_rgmii in typical cornor.
>
> [Typos courtesy of the datasheet, not me!]
>
> This sounds like more RGMII delays. It seems like PHY EXT 0CH is about
> 1G mode, and PHY EXT 0DH is about 10/100 mode. I think you probably
> need to set this bits as well. Have you tested against a link peer at
> 10 Half? 100 Full?

Good Catch!

Guess I'll have to set that too, anything else you'd recommend looking into?

>
>    Andrew

Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Andrew Lunn]

> Good Catch!
> 
> Guess I'll have to set that too, anything else you'd recommend looking into?

I think for a first submission, you have the basics. I'm just pushing
RGMII delays because we have had backwards compatibility problems in
that area when added later. Experience suggests adding features in
other areas is much less of a problem. So as you suggested, you can
add cable test, downshift control, interrupts etc later.

    Andrew