2021-07-23 10:44:26

by Oleksij Rempel

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

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 <[email protected]>
Signed-off-by: Dan Murphy <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
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 <[email protected]>
+ */
+
+#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 <[email protected]");
+MODULE_LICENSE("GPL v2");
--
2.30.2


2021-07-23 13:23:57

by Andrew Lunn

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

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

2021-07-23 17:10:57

by Oleksij Rempel

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

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 |

2021-07-23 18:14:38

by Andrew Lunn

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

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

2021-07-24 01:00:20

by kernel test robot

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

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 <[email protected]>

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/[email protected]


Attachments:
(No filename) (3.62 kB)
.config.gz (53.73 kB)
Download all attachments

2021-07-26 12:21:30

by Oleksij Rempel

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

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 |

2021-07-26 13:24:18

by Andrew Lunn

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

> 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

2021-07-27 06:53:58

by Oleksij Rempel

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

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 |