From c11669a2757e285958e3d7647bad48807ae8e283 Mon Sep 17 00:00:00 2001 From: Trent Piepho Date: Wed, 22 May 2019 18:43:23 +0000 Subject: [PATCH] net: phy: dp83867: Rework delay rgmii delay handling The code was assuming the reset default of the delay control register was to have delay disabled. This is what the datasheet shows as the register's initial value. However, that's not actually true: the default is controlled by the PHY's pin strapping. If the interface mode is selected as RX or TX delay only, insure the other direction's delay is disabled. If the interface mode is just "rgmii", with neither TX or RX internal delay, one might expect that the driver should disable both delays. But this is not what the driver does. It leaves the setting at the PHY's strapping's default. And that default, for no pins with strapping resistors, is to have delay enabled and 2.00 ns. Rather than change this behavior, I've kept it the same and documented it. No delay will most likely not work and will break ethernet on any board using "rgmii" mode. If the board is strapped to have a delay and is configured to use "rgmii" mode a warning is generated that "rgmii-id" should have been used. Also validate the delay values and fail if they are not in range. Cc: Andrew Lunn Cc: Florian Fainelli Cc: Heiner Kallweit Signed-off-by: Trent Piepho Reviewed-by: Andrew Lunn Signed-off-by: David S. Miller --- drivers/net/phy/dp83867.c | 78 +++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 12 deletions(-) diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 54fbc833bf5d..fc5baa5d14d0 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -28,6 +28,7 @@ #define DP83867_CFG4 0x0031 #define DP83867_RGMIICTL 0x0032 #define DP83867_STRAP_STS1 0x006E +#define DP83867_STRAP_STS2 0x006f #define DP83867_RGMIIDCTL 0x0086 #define DP83867_IO_MUX_CFG 0x0170 @@ -55,13 +56,23 @@ /* STRAP_STS1 bits */ #define DP83867_STRAP_STS1_RESERVED BIT(11) +/* STRAP_STS2 bits */ +#define DP83867_STRAP_STS2_CLK_SKEW_TX_MASK GENMASK(6, 4) +#define DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT 4 +#define DP83867_STRAP_STS2_CLK_SKEW_RX_MASK GENMASK(2, 0) +#define DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT 0 +#define DP83867_STRAP_STS2_CLK_SKEW_NONE BIT(2) + /* PHY CTRL bits */ #define DP83867_PHYCR_FIFO_DEPTH_SHIFT 14 #define DP83867_PHYCR_FIFO_DEPTH_MASK (3 << 14) #define DP83867_PHYCR_RESERVED_MASK BIT(11) /* RGMIIDCTL bits */ +#define DP83867_RGMII_TX_CLK_DELAY_MAX 0xf #define DP83867_RGMII_TX_CLK_DELAY_SHIFT 4 +#define DP83867_RGMII_RX_CLK_DELAY_MAX 0xf +#define DP83867_RGMII_RX_CLK_DELAY_SHIFT 0 /* IO_MUX_CFG bits */ #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL 0x1f @@ -178,19 +189,56 @@ static int dp83867_of_init(struct phy_device *phydev) dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node, "ti,dp83867-rxctrl-strap-quirk"); - ret = of_property_read_u32(of_node, "ti,rx-internal-delay", - &dp83867->rx_id_delay); - if (ret && - (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || - phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)) - return ret; + /* Existing behavior was to use default pin strapping delay in rgmii + * mode, but rgmii should have meant no delay. Warn existing users. + */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { + const u16 val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_STRAP_STS2); + const u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >> + DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT; + const u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >> + DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT; + + if (txskew != DP83867_STRAP_STS2_CLK_SKEW_NONE || + rxskew != DP83867_STRAP_STS2_CLK_SKEW_NONE) + phydev_warn(phydev, + "PHY has delays via pin strapping, but phy-mode = 'rgmii'\n" + "Should be 'rgmii-id' to use internal delays\n"); + } - ret = of_property_read_u32(of_node, "ti,tx-internal-delay", - &dp83867->tx_id_delay); - if (ret && - (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || - phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) - return ret; + /* RX delay *must* be specified if internal delay of RX is used. */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { + ret = of_property_read_u32(of_node, "ti,rx-internal-delay", + &dp83867->rx_id_delay); + if (ret) { + phydev_err(phydev, "ti,rx-internal-delay must be specified\n"); + return ret; + } + if (dp83867->rx_id_delay > DP83867_RGMII_RX_CLK_DELAY_MAX) { + phydev_err(phydev, + "ti,rx-internal-delay value of %u out of range\n", + dp83867->rx_id_delay); + return -EINVAL; + } + } + + /* TX delay *must* be specified if internal delay of RX is used. */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { + ret = of_property_read_u32(of_node, "ti,tx-internal-delay", + &dp83867->tx_id_delay); + if (ret) { + phydev_err(phydev, "ti,tx-internal-delay must be specified\n"); + return ret; + } + if (dp83867->tx_id_delay > DP83867_RGMII_TX_CLK_DELAY_MAX) { + phydev_err(phydev, + "ti,tx-internal-delay value of %u out of range\n", + dp83867->tx_id_delay); + return -EINVAL; + } + } if (of_property_read_bool(of_node, "enet-phy-lane-swap")) dp83867->port_mirroring = DP83867_PORT_MIRROING_EN; @@ -259,10 +307,16 @@ static int dp83867_config_init(struct phy_device *phydev) return ret; } + /* If rgmii mode with no internal delay is selected, we do NOT use + * aligned mode as one might expect. Instead we use the PHY's default + * based on pin strapping. And the "mode 0" default is to *use* + * internal delay with a value of 7 (2.00 ns). + */ if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) && (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) { val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL); + val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN); if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) val |= (DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN); -- 2.30.2