[PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Oleksij Rempel]

The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
that supports 10M single pair cable.

This driver provides basic support for this chip:
- link status
- autoneg can be turned off
- master/slave can be configured to be able to work without autoneg

This driver and PHY was tested with ASIX AX88772B USB Ethernet controller.

Co-developed-by: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/Kconfig     |   6 +
 drivers/net/phy/Makefile    |   1 +
 drivers/net/phy/dp83td510.c | 303 ++++++++++++++++++++++++++++++++++++
 3 files changed, 310 insertions(+)
 create mode 100644 drivers/net/phy/dp83td510.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index c56f703ae998..9bdc88deb5e1 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -326,6 +326,12 @@ config DP83869_PHY
 	  Currently supports the DP83869 PHY.  This PHY supports copper and
 	  fiber connections.
 
+config DP83TD510_PHY
+	tristate "Texas Instruments DP83TD510 Ethernet 10Base-T1L PHY"
+	help
+	  Support for the DP83TD510 Ethernet 10Base-T1L PHY. This PHY supports
+	  a 10M single pair Ethernet connection for up to 1000 meter cable.
+
 config VITESSE_PHY
 	tristate "Vitesse PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 172bb193ae6a..e4927c59bfdb 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_DP83848_PHY)	+= dp83848.o
 obj-$(CONFIG_DP83867_PHY)	+= dp83867.o
 obj-$(CONFIG_DP83869_PHY)	+= dp83869.o
 obj-$(CONFIG_DP83TC811_PHY)	+= dp83tc811.o
+obj-$(CONFIG_DP83TD510_PHY)	+= dp83td510.o
 obj-$(CONFIG_FIXED_PHY)		+= fixed_phy.o
 obj-$(CONFIG_ICPLUS_PHY)	+= icplus.o
 obj-$(CONFIG_INTEL_XWAY_PHY)	+= intel-xway.o
diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
new file mode 100644
index 000000000000..860e2b516dda
--- /dev/null
+++ b/drivers/net/phy/dp83td510.c
@@ -0,0 +1,303 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for the Texas Instruments DP83TD510 PHY
+ * Copyright (C) 2020 Texas Instruments Incorporated - https://www.ti.com/
+ * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#define DP83TD510E_PHY_ID			0x20000181
+
+#define DP83TD510_PHY_STS			0x10
+#define DP83TD510_PHY_STS_LINK_STATUS		BIT(0)
+
+#define DP83TD510_AN_CONTROL			0x200
+#define DP83TD510_AN_ENABLE			BIT(12)
+
+#define DP83TD510_AN_STAT_1			0x60c
+/* Master/Slave resolution failed */
+#define DP83TD510_AN_STAT_1_MS_FAIL		BIT(15)
+
+#define DP83TD510_PMA_PMD_CTRL			0x1834
+#define DP83TD510_PMD_CTRL_MASTER_MODE		BIT(14)
+
+#define DP83TD510_SOR_0				0x420
+#define DP83TD510_SOR_0_GPIO2			BIT(6)
+
+#define DP83TD510_SOR_1				0x467
+#define DP83TD510_SOR_1_GPIO1			BIT(9)
+#define DP83TD510_SOR_1_LED_0			BIT(8)
+#define DP83TD510_SOR_1_LED_2			BIT(7)
+#define DP83TD510_SOR_1_RX_ER			BIT(6)
+#define DP83TD510_SOR_1_RX_CTRL			BIT(5)
+#define DP83TD510_SOR_1_CLK_OUT			BIT(4)
+#define DP83TD510_SOR_1_RX_D0			BIT(3)
+#define DP83TD510_SOR_1_RX_D1			BIT(2)
+#define DP83TD510_SOR_1_RX_D2			BIT(1)
+#define DP83TD510_SOR_1_RX_D3			BIT(0)
+
+enum dp83td510_xmii_mode {
+	DP83TD510_MII = 0,
+	DP83TD510_RMII_MASTER,
+	DP83TD510_RGMII,
+	DP83TD510_RMII_SLAVE,
+};
+
+static const char *dp83td510_get_xmii_mode_str(enum dp83td510_xmii_mode mode)
+{
+	switch (mode) {
+	case DP83TD510_MII:
+		return "MII";
+	case DP83TD510_RMII_MASTER:
+		return "RMII master";
+	case DP83TD510_RGMII:
+		return "RGMII";
+	case DP83TD510_RMII_SLAVE:
+		return "RMII slave";
+	}
+
+	return "<unknown>";
+}
+
+static int dp83td510_get_mmd(struct phy_device *phydev, u16 *reg)
+{
+	switch (*reg) {
+	case 0x1000 ... 0x18f8:
+		/* According to the datasheet:
+		 * Prefixed 0x1 in [15:12] of address to differentiate. Please
+		 * remove 0x1 from [15:12] while using the address.
+		 */
+		*reg &= 0xfff;
+		return 0x1;
+	case 0x3000 ... 0x38e7:
+		/* According to the datasheet:
+		 * Prefixed 0x3 in [15:12] of address to differentiate. Please
+		 * remove 0x3 from [15:12] while using the address.
+		 */
+		*reg &= 0xfff;
+		return 0x3;
+	case 0x0200 ... 0x020f:
+		return 0x7;
+	case 0x0000 ... 0x0130:
+	case 0x0300 ... 0x0e01:
+		return 0x1f;
+	default:
+		phydev_err(phydev, "Unknown register 0x%04x\n", *reg);
+		return -EOPNOTSUPP;
+	}
+}
+
+static int dp83td510_read(struct phy_device *phydev, u16 reg)
+{
+	int mmd;
+
+	mmd = dp83td510_get_mmd(phydev, &reg);
+	if (mmd < 0)
+		return mmd;
+
+	return phy_read_mmd(phydev, mmd, reg);
+}
+
+static int dp83td510_write(struct phy_device *phydev, u16 reg, u16 val)
+{
+	int mmd;
+
+	mmd = dp83td510_get_mmd(phydev, &reg);
+	if (mmd < 0)
+		return mmd;
+
+	return phy_write_mmd(phydev, mmd, reg, val);
+}
+
+static int dp83td510_modify(struct phy_device *phydev, u16 reg, u16 mask,
+			    u16 set)
+{
+	int mmd;
+
+	mmd = dp83td510_get_mmd(phydev, &reg);
+	if (mmd < 0)
+		return mmd;
+
+	return phy_modify_mmd(phydev, mmd, reg, mask, set);
+}
+
+static int dp83td510_modify_changed(struct phy_device *phydev, u16 reg,
+				    u16 mask, u16 set)
+{
+	int mmd;
+
+	mmd = dp83td510_get_mmd(phydev, &reg);
+	if (mmd < 0)
+		return mmd;
+
+	return phy_modify_mmd_changed(phydev, mmd, reg, mask, set);
+}
+
+static int dp83td510_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+	ret = dp83td510_read(phydev, DP83TD510_PHY_STS);
+	if (ret < 0)
+		return ret;
+
+	phydev->link = ret & DP83TD510_PHY_STS_LINK_STATUS;
+	if (phydev->link) {
+		phydev->duplex = DUPLEX_FULL;
+		phydev->speed = SPEED_10;
+	} else {
+		phydev->speed = SPEED_UNKNOWN;
+		phydev->duplex = DUPLEX_UNKNOWN;
+	}
+
+	ret = dp83td510_read(phydev, DP83TD510_AN_STAT_1);
+	if (ret < 0)
+		return ret;
+
+	if (ret & DP83TD510_AN_STAT_1_MS_FAIL)
+		phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
+
+	ret = dp83td510_read(phydev, DP83TD510_PMA_PMD_CTRL);
+	if (ret < 0)
+		return ret;
+
+	if (!phydev->autoneg) {
+		if (ret & DP83TD510_PMD_CTRL_MASTER_MODE)
+			phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+		else
+			phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+	}
+
+	return 0;
+}
+
+static int dp83td510_config_aneg(struct phy_device *phydev)
+{
+	u16 ctrl = 0, pmd_ctrl = 0;
+	int ret;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+		if (phydev->autoneg) {
+			phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
+			phydev_warn(phydev, "Can't force master mode if autoneg is enabled\n");
+			goto do_aneg;
+		}
+		pmd_ctrl |= DP83TD510_PMD_CTRL_MASTER_MODE;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		if (phydev->autoneg) {
+			phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
+			phydev_warn(phydev, "Can't force slave mode if autoneg is enabled\n");
+			goto do_aneg;
+		}
+		break;
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+		phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
+		phydev_warn(phydev, "Preferred master/slave modes are not supported\n");
+		goto do_aneg;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+		goto do_aneg;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = dp83td510_modify(phydev, DP83TD510_PMA_PMD_CTRL,
+			       DP83TD510_PMD_CTRL_MASTER_MODE, pmd_ctrl);
+	if (ret)
+		return ret;
+
+do_aneg:
+	if (phydev->autoneg)
+		ctrl |= DP83TD510_AN_ENABLE;
+
+	ret = dp83td510_modify_changed(phydev, DP83TD510_AN_CONTROL,
+				       DP83TD510_AN_ENABLE, ctrl);
+	if (ret < 0)
+		return ret;
+
+	/* Reset link if settings are changed */
+	if (ret)
+		ret = dp83td510_write(phydev, MII_BMCR, BMCR_RESET);
+
+	return ret;
+}
+
+static int dp83td510_strap(struct phy_device *phydev)
+{
+	int tx_vpp, pin18, rx_trap, pin30, rx_ctrl;
+	enum dp83td510_xmii_mode xmii_mode;
+	int sor0, sor1;
+	u8 addr;
+
+	sor0 = dp83td510_read(phydev, DP83TD510_SOR_0);
+	if (sor0 < 0)
+		return sor0;
+
+	rx_trap = FIELD_GET(DP83TD510_SOR_0_GPIO2, sor0);
+
+	sor1 = dp83td510_read(phydev, DP83TD510_SOR_1);
+	if (sor1 < 0)
+		return sor0;
+
+	addr = FIELD_GET(DP83TD510_SOR_1_RX_D3, sor1) << 3 |
+		FIELD_GET(DP83TD510_SOR_1_RX_D0, sor1) << 2 |
+		FIELD_GET(DP83TD510_SOR_1_RX_ER, sor1) << 1 |
+		FIELD_GET(DP83TD510_SOR_1_GPIO1, sor1) << 0;
+
+	tx_vpp = FIELD_GET(DP83TD510_SOR_1_LED_2, sor1);
+	xmii_mode = FIELD_GET(DP83TD510_SOR_1_LED_0, sor1) << 1 |
+		FIELD_GET(DP83TD510_SOR_1_RX_D1, sor1) << 0;
+	pin18 = FIELD_GET(DP83TD510_SOR_1_RX_D2, sor1);
+	pin30 = FIELD_GET(DP83TD510_SOR_1_CLK_OUT, sor1);
+	rx_ctrl = FIELD_GET(DP83TD510_SOR_1_RX_CTRL, sor1);
+
+	phydev_info(phydev,
+		    "bootstrap cfg: Pin 18: %s, Pin 30: %s, TX Vpp: %s, RX trap: %s, xMII mode: %s, PHY addr: 0x%x\n",
+		    pin18 ? "RX_DV" : "CRS_DV",
+		    pin30 ? "LED_1" : "CLKOUT",
+		    tx_vpp ? "1.0V p2p" : "2.4V & 1.0V p2p",
+		    rx_trap ? "< 40Ω" : "50Ω",
+		    dp83td510_get_xmii_mode_str(xmii_mode),
+		    addr);
+
+	return 0;
+}
+
+static int dp83td510_probe(struct phy_device *phydev)
+{
+	return dp83td510_strap(phydev);
+}
+
+static struct phy_driver dp83td510_driver[] = {
+{
+	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
+	.name		= "TI DP83TD510E",
+	.probe          = dp83td510_probe,
+
+	.config_aneg	= dp83td510_config_aneg,
+	.read_status	= dp83td510_read_status,
+
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
+} };
+module_phy_driver(dp83td510_driver);
+
+static struct mdio_device_id __maybe_unused dp83td510_tbl[] = {
+	{ PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID) },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, dp83td510_tbl);
+
+MODULE_DESCRIPTION("Texas Instruments DP83TD510E PHY driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Andrew Lunn]

On Fri, Jul 23, 2021 at 12:42:18PM +0200, Oleksij Rempel wrote:
> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> that supports 10M single pair cable.
> 
> This driver provides basic support for this chip:
> - link status
> - autoneg can be turned off
> - master/slave can be configured to be able to work without autoneg
> 
> This driver and PHY was tested with ASIX AX88772B USB Ethernet controller.

Hi Oleksij

There were patches flying around recently for another T1L PHY which
added new link modes. Please could you work together with that patch
to set the phydev features correctly to indicate this PHY is also a
T1L, and if it support 2.4v etc.

> +static int dp83td510_config_aneg(struct phy_device *phydev)
> +{
> +	u16 ctrl = 0, pmd_ctrl = 0;
> +	int ret;
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		if (phydev->autoneg) {
> +			phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
> +			phydev_warn(phydev, "Can't force master mode if autoneg is enabled\n");
> +			goto do_aneg;
> +		}
> +		pmd_ctrl |= DP83TD510_PMD_CTRL_MASTER_MODE;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		if (phydev->autoneg) {
> +			phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
> +			phydev_warn(phydev, "Can't force slave mode if autoneg is enabled\n");
> +			goto do_aneg;
> +		}
> +		break;
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
> +		phydev_warn(phydev, "Preferred master/slave modes are not supported\n");
> +		goto do_aneg;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		goto do_aneg;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = dp83td510_modify(phydev, DP83TD510_PMA_PMD_CTRL,
> +			       DP83TD510_PMD_CTRL_MASTER_MODE, pmd_ctrl);
> +	if (ret)
> +		return ret;
> +
> +do_aneg:
> +	if (phydev->autoneg)
> +		ctrl |= DP83TD510_AN_ENABLE;
> +
> +	ret = dp83td510_modify_changed(phydev, DP83TD510_AN_CONTROL,
> +				       DP83TD510_AN_ENABLE, ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Reset link if settings are changed */
> +	if (ret)
> +		ret = dp83td510_write(phydev, MII_BMCR, BMCR_RESET);
> +
> +	return ret;
> +}
> +
> +static int dp83td510_strap(struct phy_device *phydev)
> +{

> +	phydev_info(phydev,
> +		    "bootstrap cfg: Pin 18: %s, Pin 30: %s, TX Vpp: %s, RX trap: %s, xMII mode: %s, PHY addr: 0x%x\n",
> +		    pin18 ? "RX_DV" : "CRS_DV",
> +		    pin30 ? "LED_1" : "CLKOUT",
> +		    tx_vpp ? "1.0V p2p" : "2.4V & 1.0V p2p",
> +		    rx_trap ? "< 40Ω" : "50Ω",
> +		    dp83td510_get_xmii_mode_str(xmii_mode),
> +		    addr);

What i learned reviewing the other T1L driver is that 2.4v operation
seems to be something you negotiate. Yet i don't see anything about it
in dp83td510_config_aneg() ?

   Andrew

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Oleksij Rempel]

Hi Andrew,

On Fri, Jul 23, 2021 at 03:22:16PM +0200, Andrew Lunn wrote:
> On Fri, Jul 23, 2021 at 12:42:18PM +0200, Oleksij Rempel wrote:
> > The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> > that supports 10M single pair cable.
> > 
> > This driver provides basic support for this chip:
> > - link status
> > - autoneg can be turned off
> > - master/slave can be configured to be able to work without autoneg
> > 
> > This driver and PHY was tested with ASIX AX88772B USB Ethernet controller.
> 
> Hi Oleksij
> 
> There were patches flying around recently for another T1L PHY which
> added new link modes. Please could you work together with that patch
> to set the phydev features correctly to indicate this PHY is also a
> T1L, and if it support 2.4v etc.

ACK, thx. I was not able to spend enough time to investigate all needed
caps, so I decided to go mainline with limited functionality first.

> > +static int dp83td510_config_aneg(struct phy_device *phydev)
> > +{
> > +	u16 ctrl = 0, pmd_ctrl = 0;
> > +	int ret;
> > +
> > +	switch (phydev->master_slave_set) {
> > +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> > +		if (phydev->autoneg) {
> > +			phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
> > +			phydev_warn(phydev, "Can't force master mode if autoneg is enabled\n");
> > +			goto do_aneg;
> > +		}
> > +		pmd_ctrl |= DP83TD510_PMD_CTRL_MASTER_MODE;
> > +		break;
> > +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> > +		if (phydev->autoneg) {
> > +			phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
> > +			phydev_warn(phydev, "Can't force slave mode if autoneg is enabled\n");
> > +			goto do_aneg;
> > +		}
> > +		break;
> > +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> > +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> > +		phydev->master_slave_set = MASTER_SLAVE_CFG_UNSUPPORTED;
> > +		phydev_warn(phydev, "Preferred master/slave modes are not supported\n");
> > +		goto do_aneg;
> > +	case MASTER_SLAVE_CFG_UNKNOWN:
> > +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> > +		goto do_aneg;
> > +	default:
> > +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	ret = dp83td510_modify(phydev, DP83TD510_PMA_PMD_CTRL,
> > +			       DP83TD510_PMD_CTRL_MASTER_MODE, pmd_ctrl);
> > +	if (ret)
> > +		return ret;
> > +
> > +do_aneg:
> > +	if (phydev->autoneg)
> > +		ctrl |= DP83TD510_AN_ENABLE;
> > +
> > +	ret = dp83td510_modify_changed(phydev, DP83TD510_AN_CONTROL,
> > +				       DP83TD510_AN_ENABLE, ctrl);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Reset link if settings are changed */
> > +	if (ret)
> > +		ret = dp83td510_write(phydev, MII_BMCR, BMCR_RESET);
> > +
> > +	return ret;
> > +}
> > +
> > +static int dp83td510_strap(struct phy_device *phydev)
> > +{
> 
> > +	phydev_info(phydev,
> > +		    "bootstrap cfg: Pin 18: %s, Pin 30: %s, TX Vpp: %s, RX trap: %s, xMII mode: %s, PHY addr: 0x%x\n",
> > +		    pin18 ? "RX_DV" : "CRS_DV",
> > +		    pin30 ? "LED_1" : "CLKOUT",
> > +		    tx_vpp ? "1.0V p2p" : "2.4V & 1.0V p2p",
> > +		    rx_trap ? "< 40Ω" : "50Ω",
> > +		    dp83td510_get_xmii_mode_str(xmii_mode),
> > +		    addr);
> 
> What i learned reviewing the other T1L driver is that 2.4v operation
> seems to be something you negotiate. Yet i don't see anything about it
> in dp83td510_config_aneg() ?

voltage depends on the end application: cable length, safety requirements. I do
not see how this can be chosen only on auto negotiation. We would need proper
user space interface to let user/integrator set the limits.

May be IEEE 802.3cg (802.3-2019?) provides more information on how this should be
done. Do any one has access to it? I'll be happy to have it.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Andrew Lunn]

On Fri, Jul 23, 2021 at 07:08:49PM +0200, Oleksij Rempel wrote:
> Hi Andrew,
> 
> On Fri, Jul 23, 2021 at 03:22:16PM +0200, Andrew Lunn wrote:
> > On Fri, Jul 23, 2021 at 12:42:18PM +0200, Oleksij Rempel wrote:
> > > The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> > > that supports 10M single pair cable.
> > > 
> > > This driver provides basic support for this chip:
> > > - link status
> > > - autoneg can be turned off
> > > - master/slave can be configured to be able to work without autoneg
> > > 
> > > This driver and PHY was tested with ASIX AX88772B USB Ethernet controller.
> > 
> > Hi Oleksij
> > 
> > There were patches flying around recently for another T1L PHY which
> > added new link modes. Please could you work together with that patch
> > to set the phydev features correctly to indicate this PHY is also a
> > T1L, and if it support 2.4v etc.
> 
> ACK, thx. I was not able to spend enough time to investigate all needed
> caps, so I decided to go mainline with limited functionality first.

Limited functionality is fine, but what is implemented should be
correct. And from what i see in the patch, it is hard to know if the
PHYs basic features are correctly determined. What does ethtool show?
Is 100BaseT being offered? Half duplex?

> voltage depends on the end application: cable length, safety requirements. I do
> not see how this can be chosen only on auto negotiation. We would need proper
> user space interface to let user/integrator set the limits.

I think we are talking at cross purposes here. As far as i understand
T1L supports the data signals to be 2.4Vpp as well as the usual
1Vpp. This is something which can be negotiated in the same way as
master/slave, duplex etc.

I suspect you are talking about the PoE aspects. That is outside the
scope for phylib. PoE in general is not really supported in the Linux
kernel, and probably needs a subsystem of its own.

   Andrew

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3624 bytes --]

Hi Oleksij,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Oleksij-Rempel/net-phy-dp83td510-Add-basic-support-for-the-DP83TD510-Ethernet-PHY/20210723-184426
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4431531c482a2c05126caaa9fcc5053a4a5c495b
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b58ac37ec2e1ecf8d19fd6f7da3d8d23ddc92d61
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Oleksij-Rempel/net-phy-dp83td510-Add-basic-support-for-the-DP83TD510-Ethernet-PHY/20210723-184426
        git checkout b58ac37ec2e1ecf8d19fd6f7da3d8d23ddc92d61
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/phy/dp83td510.c: In function 'dp83td510_strap':
>> drivers/net/phy/dp83td510.c:237:37: warning: variable 'rx_ctrl' set but not used [-Wunused-but-set-variable]
     237 |  int tx_vpp, pin18, rx_trap, pin30, rx_ctrl;
         |                                     ^~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
   Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
   Selected by
   - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
   - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC


vim +/rx_ctrl +237 drivers/net/phy/dp83td510.c

   234	
   235	static int dp83td510_strap(struct phy_device *phydev)
   236	{
 > 237		int tx_vpp, pin18, rx_trap, pin30, rx_ctrl;
   238		enum dp83td510_xmii_mode xmii_mode;
   239		int sor0, sor1;
   240		u8 addr;
   241	
   242		sor0 = dp83td510_read(phydev, DP83TD510_SOR_0);
   243		if (sor0 < 0)
   244			return sor0;
   245	
   246		rx_trap = FIELD_GET(DP83TD510_SOR_0_GPIO2, sor0);
   247	
   248		sor1 = dp83td510_read(phydev, DP83TD510_SOR_1);
   249		if (sor1 < 0)
   250			return sor0;
   251	
   252		addr = FIELD_GET(DP83TD510_SOR_1_RX_D3, sor1) << 3 |
   253			FIELD_GET(DP83TD510_SOR_1_RX_D0, sor1) << 2 |
   254			FIELD_GET(DP83TD510_SOR_1_RX_ER, sor1) << 1 |
   255			FIELD_GET(DP83TD510_SOR_1_GPIO1, sor1) << 0;
   256	
   257		tx_vpp = FIELD_GET(DP83TD510_SOR_1_LED_2, sor1);
   258		xmii_mode = FIELD_GET(DP83TD510_SOR_1_LED_0, sor1) << 1 |
   259			FIELD_GET(DP83TD510_SOR_1_RX_D1, sor1) << 0;
   260		pin18 = FIELD_GET(DP83TD510_SOR_1_RX_D2, sor1);
   261		pin30 = FIELD_GET(DP83TD510_SOR_1_CLK_OUT, sor1);
   262		rx_ctrl = FIELD_GET(DP83TD510_SOR_1_RX_CTRL, sor1);
   263	
   264		phydev_info(phydev,
   265			    "bootstrap cfg: Pin 18: %s, Pin 30: %s, TX Vpp: %s, RX trap: %s, xMII mode: %s, PHY addr: 0x%x\n",
   266			    pin18 ? "RX_DV" : "CRS_DV",
   267			    pin30 ? "LED_1" : "CLKOUT",
   268			    tx_vpp ? "1.0V p2p" : "2.4V & 1.0V p2p",
   269			    rx_trap ? "< 40Ω" : "50Ω",
   270			    dp83td510_get_xmii_mode_str(xmii_mode),
   271			    addr);
   272	
   273		return 0;
   274	}
   275	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55015 bytes --]

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Oleksij Rempel]

Hi Andrew,

On Fri, Jul 23, 2021 at 08:12:05PM +0200, Andrew Lunn wrote:
> On Fri, Jul 23, 2021 at 07:08:49PM +0200, Oleksij Rempel wrote:
> > Hi Andrew,
> > 
> > On Fri, Jul 23, 2021 at 03:22:16PM +0200, Andrew Lunn wrote:
> > > On Fri, Jul 23, 2021 at 12:42:18PM +0200, Oleksij Rempel wrote:
> > > > The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> > > > that supports 10M single pair cable.
> > > > 
> > > > This driver provides basic support for this chip:
> > > > - link status
> > > > - autoneg can be turned off
> > > > - master/slave can be configured to be able to work without autoneg
> > > > 
> > > > This driver and PHY was tested with ASIX AX88772B USB Ethernet controller.
> > > 
> > > Hi Oleksij
> > > 
> > > There were patches flying around recently for another T1L PHY which
> > > added new link modes. Please could you work together with that patch
> > > to set the phydev features correctly to indicate this PHY is also a
> > > T1L, and if it support 2.4v etc.
> > 
> > ACK, thx. I was not able to spend enough time to investigate all needed
> > caps, so I decided to go mainline with limited functionality first.
> 
> Limited functionality is fine, but what is implemented should be
> correct. And from what i see in the patch, it is hard to know if the
> PHYs basic features are correctly determined. What does ethtool show?
> Is 100BaseT being offered? Half duplex?

With current driver ethtool with show this information:
Settings for eth1:
	Supported ports: [ TP	 MII ]
	Supported link modes:   Not reported
	Supported pause frame use: Symmetric Receive-only
	Supports auto-negotiation: Yes
	Supported FEC modes: Not reported
	Advertised link modes:  Not reported
	Advertised pause frame use: No
	Advertised auto-negotiation: Yes
	Advertised FEC modes: Not reported
	Speed: 10Mb/s
	Duplex: Full
	Auto-negotiation: on
	master-slave cfg: unknown
	master-slave status: unknown
	Port: Twisted Pair
	PHYAD: 1
	Transceiver: external
	MDI-X: Unknown
	Supports Wake-on: pg
	Wake-on: p
        Current message level: 0x00000007 (7)
                               drv probe link
	Link detected: yes

> > voltage depends on the end application: cable length, safety requirements. I do
> > not see how this can be chosen only on auto negotiation. We would need proper
> > user space interface to let user/integrator set the limits.
> 
> I think we are talking at cross purposes here. As far as i understand
> T1L supports the data signals to be 2.4Vpp as well as the usual
> 1Vpp. This is something which can be negotiated in the same way as
> master/slave, duplex etc.
> 
> I suspect you are talking about the PoE aspects. That is outside the
> scope for phylib. PoE in general is not really supported in the Linux
> kernel, and probably needs a subsystem of its own.

No, no. I'm talking about data signals configuration (2.4Vpp/1Vpp), which
depends on application and cable length. 1Vpp should not be used with
cable over 200 meter and 2.4Vpp should not be used on safety critical
applications. 

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Andrew Lunn]

> With current driver ethtool with show this information:
> Settings for eth1:
> 	Supported ports: [ TP	 MII ]
> 	Supported link modes:   Not reported

Interesting. The default function for getting the PHYs abilities is
doing better than i expected. I was guessing you would see 10BaseT
here.

Given that, what you have is O.K. for the moment. 

> > I suspect you are talking about the PoE aspects. That is outside the
> > scope for phylib. PoE in general is not really supported in the Linux
> > kernel, and probably needs a subsystem of its own.
> 
> No, no. I'm talking about data signals configuration (2.4Vpp/1Vpp), which
> depends on application and cable length. 1Vpp should not be used with
> cable over 200 meter

Should not be used, or is not expected to work very well?

> and 2.4Vpp should not be used on safety critical applications. 

Please work with the other T1L driver writer to propose a suitable way
to configure this. We want all T1L drivers to use the same
configuration method, DT properties etc.

	      Andrew

Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add basic support for the DP83TD510 Ethernet PHY

[Oleksij Rempel]

On Mon, Jul 26, 2021 at 03:23:00PM +0200, Andrew Lunn wrote:
> > With current driver ethtool with show this information:
> > Settings for eth1:
> > 	Supported ports: [ TP	 MII ]
> > 	Supported link modes:   Not reported
> 
> Interesting. The default function for getting the PHYs abilities is
> doing better than i expected. I was guessing you would see 10BaseT
> here.
> 
> Given that, what you have is O.K. for the moment. 
> 
> > > I suspect you are talking about the PoE aspects. That is outside the
> > > scope for phylib. PoE in general is not really supported in the Linux
> > > kernel, and probably needs a subsystem of its own.
> > 
> > No, no. I'm talking about data signals configuration (2.4Vpp/1Vpp), which
> > depends on application and cable length. 1Vpp should not be used with
> > cable over 200 meter
> 
> Should not be used, or is not expected to work very well?

ack, it will not work very well :)

> > and 2.4Vpp should not be used on safety critical applications. 
> 
> Please work with the other T1L driver writer to propose a suitable way
> to configure this. We want all T1L drivers to use the same
> configuration method, DT properties etc.

ack.

After getting 802.3cg-2019 i give NACK to my patch. At least part
of all needed functionality can be implemented as common code.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |