| Message ID | 20210514115826.3025223-1-pgwipeout@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net: phy: add driver for Motorcomm yt8511 phy | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: please write a paragraph that describes the config symbol fully |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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); >
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!
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
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); > > >
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!
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
> > 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
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
> 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
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);
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