| Message ID | 20201030172950.12767-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | DP83TD510 Single Pair 10Mbps Ethernet PHY | expand |
On Fri, Oct 30, 2020 at 12:29:47PM -0500, Dan Murphy wrote: > Add entries for the 10base-T1L full and half duplex supported modes. > > $ ethtool eth0 > Supported ports: [ TP ] > Supported link modes: 10baseT1L/Half 10baseT1L/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Supported FEC modes: Not reported > Advertised link modes: 10baseT1L/Half 10baseT1L/Full > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Speed: 10Mb/s > Duplex: Full > Auto-negotiation: on > Port: MII > PHYAD: 1 > Transceiver: external > Supports Wake-on: gs > Wake-on: d > SecureOn password: 00:00:00:00:00:00 > Current message level: 0x00000000 (0) > > Link detected: yes > > Signed-off-by: Dan Murphy <dmurphy@ti.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> +static int dp83td510_config_init(struct phy_device *phydev) > +{ > + struct dp83td510_private *dp83td510 = phydev->priv; > + int mst_slave_cfg; > + int ret = 0; > + > + if (phy_interface_is_rgmii(phydev)) { > + if (dp83td510->rgmii_delay) { > + ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR, > + DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay); > + if (ret) > + return ret; > + } > + } Hi Dan I'm getting a bit paranoid about RGMII delays... > +static int dp83td510_read_straps(struct phy_device *phydev) > +{ > + struct dp83td510_private *dp83td510 = phydev->priv; > + int strap; > + > + strap = phy_read_mmd(phydev, DP83TD510_DEVADDR, DP83TD510_SOR_1); > + if (strap < 0) > + return strap; > + > + if (strap & DP83TD510_RGMII) > + dp83td510->is_rgmii = true; > + > + return 0; > +}; So dp83td510->is_rgmii is the strapping configuration. So if one of the four RGMII modes is selected, your appear to ignore which of the four is selected, and program the hardware with the strapping? That seems like a bad idea. > +#if IS_ENABLED(CONFIG_OF_MDIO) > +static int dp83td510_of_init(struct phy_device *phydev) > +{ > + struct dp83td510_private *dp83td510 = phydev->priv; > + struct device *dev = &phydev->mdio.dev; > + struct device_node *of_node = dev->of_node; > + s32 rx_int_delay; > + s32 tx_int_delay; > + int ret; > + > + if (!of_node) > + return -ENODEV; > + > + ret = dp83td510_read_straps(phydev); > + if (ret) > + return ret; > + > + dp83td510->hi_diff_output = device_property_read_bool(&phydev->mdio.dev, > + "tx-rx-output-high"); > + > + if (device_property_read_u32(&phydev->mdio.dev, "tx-fifo-depth", > + &dp83td510->tx_fifo_depth)) > + dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB; Please don't use device_property_read_foo API, we don't want to give the impression it is O.K. to stuff DT properties in ACPI tables. Please use of_ API calls. Andrew
On Fri, 30 Oct 2020 12:29:50 -0500 Dan Murphy wrote: > The DP83TD510E is an ultra-low power Ethernet physical layer transceiver > that supports 10M single pair cable. > > The device supports both 2.4-V p2p and 1-V p2p output voltage as defined > by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via > the device tree or the device is defaulted to auto negotiation to > determine the proper p2p voltage. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> drivers/net/phy/dp83td510.c:70:11: warning: symbol 'dp83td510_feature_array' was not declared. Should it be static? Also this: WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #429: FILE: drivers/net/phy/dp83td510.c:371: + return -ENOTSUPP; WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #524: FILE: drivers/net/phy/dp83td510.c:466: + return -ENOTSUPP; ERROR: space required before the open parenthesis '(' #580: FILE: drivers/net/phy/dp83td510.c:522: + if(phydev->autoneg) { ERROR: space required before the open parenthesis '(' #588: FILE: drivers/net/phy/dp83td510.c:530: + if(phydev->autoneg) { And please try to wrap the code on 80 chars on the non trivial lines: WARNING: line length of 88 exceeds 80 columns #458: FILE: drivers/net/phy/dp83td510.c:400: + mst_slave_cfg = phy_read_mmd(phydev, DP83TD510_PMD_DEVADDR, DP83TD510_PMD_CTRL); WARNING: line length of 91 exceeds 80 columns #505: FILE: drivers/net/phy/dp83td510.c:447: + linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported); WARNING: line length of 93 exceeds 80 columns #507: FILE: drivers/net/phy/dp83td510.c:449: + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported); WARNING: line length of 91 exceeds 80 columns #514: FILE: drivers/net/phy/dp83td510.c:456: + linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported); WARNING: line length of 93 exceeds 80 columns #516: FILE: drivers/net/phy/dp83td510.c:458: + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported); WARNING: line length of 92 exceeds 80 columns #560: FILE: drivers/net/phy/dp83td510.c:502: + DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay); WARNING: line length of 88 exceeds 80 columns #574: FILE: drivers/net/phy/dp83td510.c:516: + mst_slave_cfg = phy_read_mmd(phydev, DP83TD510_PMD_DEVADDR, DP83TD510_PMD_CTRL); WARNING: line length of 84 exceeds 80 columns #695: FILE: drivers/net/phy/dp83td510.c:637: + dp83td510 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83td510), GFP_KERNEL);
On Fri, Oct 30, 2020 at 12:29:50PM -0500, Dan Murphy wrote: > The DP83TD510E is an ultra-low power Ethernet physical layer transceiver > that supports 10M single pair cable. > > The device supports both 2.4-V p2p and 1-V p2p output voltage as defined > by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via > the device tree or the device is defaulted to auto negotiation to > determine the proper p2p voltage. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/net/phy/Kconfig | 6 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/dp83td510.c | 681 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 688 insertions(+) > create mode 100644 drivers/net/phy/dp83td510.c > (...) > +static int dp83td510_ack_interrupt(struct phy_device *phydev) > +{ > + int ret; > + > + ret = phy_read(phydev, DP83TD510_INT_REG1); > + if (ret < 0) > + return ret; > + > + ret = phy_read(phydev, DP83TD510_INT_REG2); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int dp83td510_config_intr(struct phy_device *phydev) > +{ > + int int_status; > + int gen_cfg_val; > + int ret; > + > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + int_status = phy_read(phydev, DP83TD510_INT_REG1); > + if (int_status < 0) > + return int_status; > + > + int_status = (DP83TD510_INT1_ESD_EN | DP83TD510_INT1_LINK_EN | > + DP83TD510_INT1_RHF_EN); > + > + ret = phy_write(phydev, DP83TD510_INT_REG1, int_status); > + if (ret) > + return ret; > + > + int_status = phy_read(phydev, DP83TD510_INT_REG2); > + if (int_status < 0) > + return int_status; > + > + int_status = (DP83TD510_INT2_POR | DP83TD510_INT2_POL | > + DP83TD510_INT2_PAGE); > + > + ret = phy_write(phydev, DP83TD510_INT_REG2, int_status); > + if (ret) > + return ret; > + > + gen_cfg_val = phy_read(phydev, DP83TD510_GEN_CFG); > + if (gen_cfg_val < 0) > + return gen_cfg_val; > + > + gen_cfg_val |= DP83TD510_INT_OE | DP83TD510_INT_EN; > + > + } else { > + ret = phy_write(phydev, DP83TD510_INT_REG1, 0); > + if (ret) > + return ret; > + > + ret = phy_write(phydev, DP83TD510_INT_REG2, 0); > + if (ret) > + return ret; > + > + gen_cfg_val = phy_read(phydev, DP83TD510_GEN_CFG); > + if (gen_cfg_val < 0) > + return gen_cfg_val; > + > + gen_cfg_val &= ~DP83TD510_INT_EN; > + } > + > + return phy_write(phydev, DP83TD510_GEN_CFG, gen_cfg_val); > +} > + I am not really sure if the shared-IRQ work in the below linked patch set will go through, but I think it would be cleaner just to ack any pending interrupts after you disable them. https://lore.kernel.org/netdev/20201029100741.462818-1-ciorneiioana@gmail.com/ I see that you are reading the INT_REG1 and INT_REG2 registers (basically servicing any pending interrupts) before enabling the IRQ. The same reads should be done after the IRQ has been disabled. > +static struct phy_driver dp83td510_driver[] = { > + { > + PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID), > + .name = "TI DP83TD510E", > + .probe = dp83td510_probe, > + .config_init = dp83td510_config_init, > + .soft_reset = dp83td510_phy_reset, > + > + /* IRQ related */ > + .ack_interrupt = dp83td510_ack_interrupt, > + .config_intr = dp83td510_config_intr, I think the PHY maintainers could comment on this more, but maybe it would help if the driver implements the .handle_interrupt() callback just so that I wouldn't have to touch a driver that was just added to rework it for the shared-IRQ transition. Ioana
Andrew On 10/30/20 3:15 PM, Andrew Lunn wrote: >> +static int dp83td510_config_init(struct phy_device *phydev) >> +{ >> + struct dp83td510_private *dp83td510 = phydev->priv; >> + int mst_slave_cfg; >> + int ret = 0; >> + >> + if (phy_interface_is_rgmii(phydev)) { >> + if (dp83td510->rgmii_delay) { >> + ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR, >> + DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay); >> + if (ret) >> + return ret; >> + } >> + } > Hi Dan > > I'm getting a bit paranoid about RGMII delays... Not sure what this means. > >> +static int dp83td510_read_straps(struct phy_device *phydev) >> +{ >> + struct dp83td510_private *dp83td510 = phydev->priv; >> + int strap; >> + >> + strap = phy_read_mmd(phydev, DP83TD510_DEVADDR, DP83TD510_SOR_1); >> + if (strap < 0) >> + return strap; >> + >> + if (strap & DP83TD510_RGMII) >> + dp83td510->is_rgmii = true; >> + >> + return 0; >> +}; > So dp83td510->is_rgmii is the strapping configuration. So if one of > the four RGMII modes is selected, your appear to ignore which of the > four is selected, and program the hardware with the strapping? > > That seems like a bad idea. I will re-look at this code. > >> +#if IS_ENABLED(CONFIG_OF_MDIO) >> +static int dp83td510_of_init(struct phy_device *phydev) >> +{ >> + struct dp83td510_private *dp83td510 = phydev->priv; >> + struct device *dev = &phydev->mdio.dev; >> + struct device_node *of_node = dev->of_node; >> + s32 rx_int_delay; >> + s32 tx_int_delay; >> + int ret; >> + >> + if (!of_node) >> + return -ENODEV; >> + >> + ret = dp83td510_read_straps(phydev); >> + if (ret) >> + return ret; >> + >> + dp83td510->hi_diff_output = device_property_read_bool(&phydev->mdio.dev, >> + "tx-rx-output-high"); >> + >> + if (device_property_read_u32(&phydev->mdio.dev, "tx-fifo-depth", >> + &dp83td510->tx_fifo_depth)) >> + dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB; > Please don't use device_property_read_foo API, we don't want to give > the impression it is O.K. to stuff DT properties in ACPI > tables. Please use of_ API calls. Hmm. Is this a new stance in DT handling for the networking tree? If it is should I go back and rework some of my other drivers that use device_property APIs Dan
Hello On 10/30/20 6:03 PM, Jakub Kicinski wrote: > On Fri, 30 Oct 2020 12:29:50 -0500 Dan Murphy wrote: >> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver >> that supports 10M single pair cable. >> >> The device supports both 2.4-V p2p and 1-V p2p output voltage as defined >> by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via >> the device tree or the device is defaulted to auto negotiation to >> determine the proper p2p voltage. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > drivers/net/phy/dp83td510.c:70:11: warning: symbol 'dp83td510_feature_array' was not declared. Should it be static? I did not see this warning. Did you use W=1? > > > Also this: > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > #429: FILE: drivers/net/phy/dp83td510.c:371: > + return -ENOTSUPP; > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > #524: FILE: drivers/net/phy/dp83td510.c:466: > + return -ENOTSUPP; Same with these warnings how where they reproduced? > > ERROR: space required before the open parenthesis '(' > #580: FILE: drivers/net/phy/dp83td510.c:522: > + if(phydev->autoneg) { > > ERROR: space required before the open parenthesis '(' > #588: FILE: drivers/net/phy/dp83td510.c:530: > + if(phydev->autoneg) { > > > And please try to wrap the code on 80 chars on the non trivial lines: What is the LoC limit for networking just for my clarification and I will align with that. I know some maintainers like to keep the 80 LoC and some allow a longer line. Dan
On Tue, Nov 03, 2020 at 11:07:00AM -0600, Dan Murphy wrote: > Andrew > > On 10/30/20 3:15 PM, Andrew Lunn wrote: > > > +static int dp83td510_config_init(struct phy_device *phydev) > > > +{ > > > + struct dp83td510_private *dp83td510 = phydev->priv; > > > + int mst_slave_cfg; > > > + int ret = 0; > > > + > > > + if (phy_interface_is_rgmii(phydev)) { > > > + if (dp83td510->rgmii_delay) { > > > + ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR, > > > + DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay); > > > + if (ret) > > > + return ret; > > > + } > > > + } > > Hi Dan > > > > I'm getting a bit paranoid about RGMII delays... > Not sure what this means. See the discussion and breakage around the realtek PHY. It wrongly implemented RGMII delays. When it was fixed, lots of board broke because the bug in the PHY driver hid bugs in the DT. > > Please don't use device_property_read_foo API, we don't want to give > > the impression it is O.K. to stuff DT properties in ACPI > > tables. Please use of_ API calls. > > Hmm. Is this a new stance in DT handling for the networking tree? > > If it is should I go back and rework some of my other drivers that use > device_property APIs There is a slowly growing understanding what ACPI support in this area means. It seems to mean that the firmware should actually do all the setup, and the kernel should not touch the hardware configuration. But some developers are ignoring this, and just stuffing DT properties into ACPI tables and letting the kernel configure the hardware, if it happens to use the device_property_read API. So i want to make it clear that these properties are for device tree, and if you want to use ACPI, you should do things the ACPI way. For new code, i will be pushing for OF only calls. Older code is a bit more tricky. There might be boards out there using ACPI, but doing it wrongly, and stuffing OF properties into ACPI tables. We should try to avoid breaking them. Andrew
On Tue, Nov 03, 2020 at 11:09:44AM -0600, Dan Murphy wrote: > Hello > > On 10/30/20 6:03 PM, Jakub Kicinski wrote: > > On Fri, 30 Oct 2020 12:29:50 -0500 Dan Murphy wrote: > > > The DP83TD510E is an ultra-low power Ethernet physical layer transceiver > > > that supports 10M single pair cable. > > > > > > The device supports both 2.4-V p2p and 1-V p2p output voltage as defined > > > by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via > > > the device tree or the device is defaulted to auto negotiation to > > > determine the proper p2p voltage. > > > > > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > > drivers/net/phy/dp83td510.c:70:11: warning: symbol 'dp83td510_feature_array' was not declared. Should it be static? > I did not see this warning. Did you use W=1? I _think_ that one is W=1. All the PHY drivers are W=1 clean, and i want to keep it that way. And i hope to make it the default in a lot of the network code soon. > > Also this: > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > #429: FILE: drivers/net/phy/dp83td510.c:371: > > + return -ENOTSUPP; > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > #524: FILE: drivers/net/phy/dp83td510.c:466: > > + return -ENOTSUPP; > Same with these warnings how where they reproduced? > > > > ERROR: space required before the open parenthesis '(' > > #580: FILE: drivers/net/phy/dp83td510.c:522: > > + if(phydev->autoneg) { > > > > ERROR: space required before the open parenthesis '(' > > #588: FILE: drivers/net/phy/dp83td510.c:530: > > + if(phydev->autoneg) { > > These look like checkpatch. > > > > And please try to wrap the code on 80 chars on the non trivial lines: > > What is the LoC limit for networking just for my clarification and I will > align with that. 80. I would not be too surprised to see checkpatch getting a patch to set it to 80 for networking code. Andrew
Andrew On 11/3/20 11:21 AM, Andrew Lunn wrote: > On Tue, Nov 03, 2020 at 11:09:44AM -0600, Dan Murphy wrote: >> Hello >> >> On 10/30/20 6:03 PM, Jakub Kicinski wrote: >>> On Fri, 30 Oct 2020 12:29:50 -0500 Dan Murphy wrote: >>>> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver >>>> that supports 10M single pair cable. >>>> >>>> The device supports both 2.4-V p2p and 1-V p2p output voltage as defined >>>> by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via >>>> the device tree or the device is defaulted to auto negotiation to >>>> determine the proper p2p voltage. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> drivers/net/phy/dp83td510.c:70:11: warning: symbol 'dp83td510_feature_array' was not declared. Should it be static? >> I did not see this warning. Did you use W=1? > I _think_ that one is W=1. All the PHY drivers are W=1 clean, and i > want to keep it that way. And i hope to make it the default in a lot > of the network code soon. OK I built with the W=1 before submission I did not see this but I will try some other things. >>> Also this: >>> >>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP >>> #429: FILE: drivers/net/phy/dp83td510.c:371: >>> + return -ENOTSUPP; >>> >>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP >>> #524: FILE: drivers/net/phy/dp83td510.c:466: >>> + return -ENOTSUPP; >> Same with these warnings how where they reproduced? Same as above >>> ERROR: space required before the open parenthesis '(' >>> #580: FILE: drivers/net/phy/dp83td510.c:522: >>> + if(phydev->autoneg) { >>> >>> ERROR: space required before the open parenthesis '(' >>> #588: FILE: drivers/net/phy/dp83td510.c:530: >>> + if(phydev->autoneg) { >>> > These look like checkpatch. These I missed >>> And please try to wrap the code on 80 chars on the non trivial lines: >> What is the LoC limit for networking just for my clarification and I will >> align with that. > 80. I would not be too surprised to see checkpatch getting a patch to > set it to 80 for networking code. OK I will align the lines to 80 then. Dan > Andrew
Andrew On 11/3/20 11:18 AM, Andrew Lunn wrote: > On Tue, Nov 03, 2020 at 11:07:00AM -0600, Dan Murphy wrote: >> Andrew >> >> On 10/30/20 3:15 PM, Andrew Lunn wrote: >>>> +static int dp83td510_config_init(struct phy_device *phydev) >>>> +{ >>>> + struct dp83td510_private *dp83td510 = phydev->priv; >>>> + int mst_slave_cfg; >>>> + int ret = 0; >>>> + >>>> + if (phy_interface_is_rgmii(phydev)) { >>>> + if (dp83td510->rgmii_delay) { >>>> + ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR, >>>> + DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + } >>> Hi Dan >>> >>> I'm getting a bit paranoid about RGMII delays... >> Not sure what this means. > See the discussion and breakage around the realtek PHY. It wrongly > implemented RGMII delays. When it was fixed, lots of board broke > because the bug in the PHY driver hid bugs in the DT. > I will have to go find that thread. Do you have a link? >>> Please don't use device_property_read_foo API, we don't want to give >>> the impression it is O.K. to stuff DT properties in ACPI >>> tables. Please use of_ API calls. >> Hmm. Is this a new stance in DT handling for the networking tree? >> >> If it is should I go back and rework some of my other drivers that use >> device_property APIs > There is a slowly growing understanding what ACPI support in this area > means. It seems to mean that the firmware should actually do all the > setup, and the kernel should not touch the hardware configuration. But > some developers are ignoring this, and just stuffing DT properties > into ACPI tables and letting the kernel configure the hardware, if it > happens to use the device_property_read API. So i want to make it > clear that these properties are for device tree, and if you want to > use ACPI, you should do things the ACPI way. > > For new code, i will be pushing for OF only calls. Older code is a bit > more tricky. There might be boards out there using ACPI, but doing it > wrongly, and stuffing OF properties into ACPI tables. We should try to > avoid breaking them. Got it. I will move back to of_* calls Dan > Andrew
> > > > drivers/net/phy/dp83td510.c:70:11: warning: symbol 'dp83td510_feature_array' was not declared. Should it be static? > > > I did not see this warning. Did you use W=1? > > I _think_ that one is W=1. All the PHY drivers are W=1 clean, and i > > want to keep it that way. And i hope to make it the default in a lot > > of the network code soon. > OK I built with the W=1 before submission I did not see this but I will try > some other things. Then it might be sparse. Try C=1. Andrew
On 10/30/2020 10:29 AM, Dan Murphy wrote: > Add entries for the 10base-T1L full and half duplex supported modes. > > $ ethtool eth0 > Supported ports: [ TP ] > Supported link modes: 10baseT1L/Half 10baseT1L/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Supported FEC modes: Not reported > Advertised link modes: 10baseT1L/Half 10baseT1L/Full > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Speed: 10Mb/s > Duplex: Full > Auto-negotiation: on > Port: MII > PHYAD: 1 > Transceiver: external > Supports Wake-on: gs > Wake-on: d > SecureOn password: 00:00:00:00:00:00 > Current message level: 0x00000000 (0) > > Link detected: yes > > Signed-off-by: Dan Murphy <dmurphy@ti.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On 11/3/2020 9:35 AM, Dan Murphy wrote: > Andrew > > On 11/3/20 11:18 AM, Andrew Lunn wrote: >> On Tue, Nov 03, 2020 at 11:07:00AM -0600, Dan Murphy wrote: >>> Andrew >>> >>> On 10/30/20 3:15 PM, Andrew Lunn wrote: >>>>> +static int dp83td510_config_init(struct phy_device *phydev) >>>>> +{ >>>>> + struct dp83td510_private *dp83td510 = phydev->priv; >>>>> + int mst_slave_cfg; >>>>> + int ret = 0; >>>>> + >>>>> + if (phy_interface_is_rgmii(phydev)) { >>>>> + if (dp83td510->rgmii_delay) { >>>>> + ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR, >>>>> + DP83TD510_MAC_CFG_1, >>>>> dp83td510->rgmii_delay); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + } >>>> Hi Dan >>>> >>>> I'm getting a bit paranoid about RGMII delays... >>> Not sure what this means. >> See the discussion and breakage around the realtek PHY. It wrongly >> implemented RGMII delays. When it was fixed, lots of board broke >> because the bug in the PHY driver hid bugs in the DT. >> > I will have to go find that thread. Do you have a link? That would be the thread: https://lore.kernel.org/netdev/CAMj1kXEEF_Un-4NTaD5iUN0NoZYaJQn-rPediX0S6oRiuVuW-A@mail.gmail.com/