Message ID | 20201117201555.26723-1-dmurphy@ti.com |
---|---|
Headers | show |
Series | DP83TD510 Single Pair 10Mbps Ethernet PHY | expand |
On Tue, Nov 17, 2020 at 02:15:55PM -0600, 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> > --- > > v4 - Considerable rework of the code after secondary test setup was created. > This version also uses the handle_interrupt call back and reduces the > configuration arrays as it was determined that 80% of the array was the same. > > drivers/net/phy/Kconfig | 6 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/dp83td510.c | 505 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 512 insertions(+) > create mode 100644 drivers/net/phy/dp83td510.c > [snip] > +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; > + > + phy_trigger_machine(phydev); > + > + return 0; > +} > + > +static irqreturn_t dp83td510_handle_interrupt(struct phy_device *phydev) > +{ > + int ret; > + > + ret = dp83td510_ack_interrupt(phydev); > + if (ret) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} From what I can see in the datasheet, the INT_REG1 and INT_REG2 are used for both interrupt configuration and interrupt status. If this is the case, the state machine should only be triggered if the interrupt was triggered (eg DP83TD510_INT1_LINK is set), not if _any_ bit from the register is set. This is broken since anytime you have interrupts enabled, the lower half of the register will be non-zero since that contains you interrupt enabled bits. The .handle_interrupt() should look something like: ret = phy_read(phydev, DP83TD510_INT_REG1); if (ret < 0) return ret; if (!(ret & (DP83TD510_INT1_ESD | DP83TD510_INT1_LINK | DP83TD510_INT1_RHF))) return IRQ_NONE; ret = phy_read(phydev, DP83TD510_INT_REG2); if (ret < 0) return ret; if (!(ret & (DP83TD510_INT2_POR | DP83TD510_INT2_POL | DP83TD510_INT2_PAGE))) return IRQ_NONE; phy_trigger_machine(phydev); return IRQ_HANDLED; > + > +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); > + Shouldn't you use DP83TD510_INT2_POR_EN, DP83TD510_INT2_POL_EN etc here? It seems that you are setting up the bits corresponding with the interrupt status and not the interrupt enable. Ioana
> +static int dp83td510_config_init(struct phy_device *phydev) > +{ > + struct dp83td510_private *dp83td510 = phydev->priv; > + 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); Just to be safe, you should always write rgmii_delay, even if it is zero. We have had too many bugs with RGMII delays which cause bad backwards compatibility problems, so i would prefer to do a write which might be unneeded, that find a bug here in a few years time. > + if (ret) > + return ret; > + } > + } > + > + if (phydev->interface == PHY_INTERFACE_MODE_RMII) { > + ret = phy_modify(phydev, DP83TD510_GEN_CFG, > + DP83TD510_FIFO_DEPTH_MASK, > + dp83td510->tx_fifo_depth); So there is no need to set the FIFO depth for the other three RGMII modes? Or should this also be phy_interface_is_rgmii(phydev)? > +#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; You need to move this assignment to later in order to keep with reverse christmas tree. > +#else > +static int dp83869_of_init(struct phy_device *phydev) > +{ > + dp83td510->hi_diff_output = DP83TD510_2_4V_P2P > + dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB You don't have DT, so there is no fine control, but you still need to do the basic 2ns delay as indicated by the phydev->interface value. So i think you still need to set dp83td510->rgmii_delay depending on which RGMII mode is requested. Andrew
Andrew On 11/19/20 7:49 PM, Andrew Lunn wrote: >> +static int dp83td510_config_init(struct phy_device *phydev) >> +{ >> + struct dp83td510_private *dp83td510 = phydev->priv; >> + 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); > Just to be safe, you should always write rgmii_delay, even if it is > zero. We have had too many bugs with RGMII delays which cause bad > backwards compatibility problems, so i would prefer to do a write > which might be unneeded, that find a bug here in a few years time. OK. > >> + if (ret) >> + return ret; >> + } >> + } >> + >> + if (phydev->interface == PHY_INTERFACE_MODE_RMII) { >> + ret = phy_modify(phydev, DP83TD510_GEN_CFG, >> + DP83TD510_FIFO_DEPTH_MASK, >> + dp83td510->tx_fifo_depth); > So there is no need to set the FIFO depth for the other three RGMII > modes? Or should this also be phy_interface_is_rgmii(phydev)? According to the data sheet the FIFO depth is for RMII. "Fifo depth for RMII Tx fifo" But I will ask the HW team for clarification. > >> +#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; > You need to move this assignment to later in order to keep with > reverse christmas tree. Well this is only used once so I will just remove the of_node declaration > >> +#else >> +static int dp83869_of_init(struct phy_device *phydev) >> +{ >> + dp83td510->hi_diff_output = DP83TD510_2_4V_P2P >> + dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB > You don't have DT, so there is no fine control, but you still need to > do the basic 2ns delay as indicated by the phydev->interface value. So > i think you still need to set dp83td510->rgmii_delay depending on > which RGMII mode is requested. The RGMII delay is fixed in the PHY. The user can either turn it on or off. The default is 'off' which is 0. I can explicitly set the rgmii_delay to 0 in non-OF cases. Dan