| Message ID | 20200526174716.14116-5-dmurphy@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | RGMII Internal delay common property | expand |
> @@ -218,6 +224,7 @@ static int dp83869_of_init(struct phy_device *phydev) > ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1); > if (ret < 0) > return ret; > + > if (ret & DP83869_STRAP_MIRROR_ENABLED) > dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN; > else This random white space change does not belong in this patch. > @@ -232,6 +239,20 @@ static int dp83869_of_init(struct phy_device *phydev) > &dp83869->tx_fifo_depth)) > dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; > > + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", > + &dp83869->rx_id_delay); > + if (ret) { > + dp83869->rx_id_delay = ret; > + ret = 0; > + } This looks odd. If this optional property is not found, -EINVAL will be returned. It could also return -ENODATA. You then assign this error value to dp83869->rx_id_delay? I would of expected you to assign 2000, the default value? > + > + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", > + &dp83869->tx_id_delay); > + if (ret) { > + dp83869->tx_id_delay = ret; > + ret = 0; > + } > + > return ret; > } > #else > @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev, > return ret; > } > > +static int dp83869_get_delay(struct phy_device *phydev) > +{ > + struct dp83869_private *dp83869 = phydev->priv; > + int delay_size = ARRAY_SIZE(dp83869_internal_delay); > + int tx_delay = 0; > + int rx_delay = 0; > + > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { > + tx_delay = phy_get_delay_index(phydev, > + &dp83869_internal_delay[0], > + delay_size, dp83869->tx_id_delay, > + false); > + if (tx_delay < 0) { > + phydev_err(phydev, "Tx internal delay is invalid\n"); > + return tx_delay; > + } > + } > + > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { > + rx_delay = phy_get_delay_index(phydev, > + &dp83869_internal_delay[0], > + delay_size, dp83869->rx_id_delay, > + false); > + if (rx_delay < 0) { > + phydev_err(phydev, "Rx internal delay is invalid\n"); > + return rx_delay; > + } > + } So any PHY using these properties is going to pretty much reproduce this code. Meaning is should all be in a helper. Andrew
> +static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750, > + 2000, 2250, 2500, 2750, 3000, 3250, > + 3500, 3750, 4000}; > + You should make this const. Otherwise it takes up twice the space. Andrew
Andrew On 5/26/20 7:52 PM, Andrew Lunn wrote: >> @@ -218,6 +224,7 @@ static int dp83869_of_init(struct phy_device *phydev) >> ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1); >> if (ret < 0) >> return ret; >> + >> if (ret & DP83869_STRAP_MIRROR_ENABLED) >> dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN; >> else > This random white space change does not belong in this patch. OK >> @@ -232,6 +239,20 @@ static int dp83869_of_init(struct phy_device *phydev) >> &dp83869->tx_fifo_depth)) >> dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; >> >> + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", >> + &dp83869->rx_id_delay); >> + if (ret) { >> + dp83869->rx_id_delay = ret; >> + ret = 0; >> + } > This looks odd. > > If this optional property is not found, -EINVAL will be returned. It > could also return -ENODATA. You then assign this error value to > dp83869->rx_id_delay? I would of expected you to assign 2000, the > default value? Well the driver cannot assume this is the intended value. If the dt defines rgmii-rx/tx-id then these values are required not optional. That was the discussion on the binding. I set these to errno because when config_init is called the driver verifies that the values are valid and present and if they are not then the PHY will fail to init. If we set the delay to default then the PHY may be programmed with the wrong delay. >> + >> + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", >> + &dp83869->tx_id_delay); >> + if (ret) { >> + dp83869->tx_id_delay = ret; >> + ret = 0; >> + } >> + >> return ret; >> } >> #else >> @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev, >> return ret; >> } >> >> +static int dp83869_get_delay(struct phy_device *phydev) >> +{ >> + struct dp83869_private *dp83869 = phydev->priv; >> + int delay_size = ARRAY_SIZE(dp83869_internal_delay); >> + int tx_delay = 0; >> + int rx_delay = 0; >> + >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { >> + tx_delay = phy_get_delay_index(phydev, >> + &dp83869_internal_delay[0], >> + delay_size, dp83869->tx_id_delay, >> + false); >> + if (tx_delay < 0) { >> + phydev_err(phydev, "Tx internal delay is invalid\n"); >> + return tx_delay; >> + } >> + } >> + >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { >> + rx_delay = phy_get_delay_index(phydev, >> + &dp83869_internal_delay[0], >> + delay_size, dp83869->rx_id_delay, >> + false); >> + if (rx_delay < 0) { >> + phydev_err(phydev, "Rx internal delay is invalid\n"); >> + return rx_delay; >> + } >> + } > So any PHY using these properties is going to pretty much reproduce > this code. Meaning is should all be in a helper. The issue here is that the phy_mode may only be rgmii-txid so you only want to find the tx_delay and return. Same with the RXID. How is the helper supposed to know what delay to return and look for? The PHY also only needs to use the helper if the PHY is in certain modes. And the decision to use the checks is really based on the PHY driver. Not sure if other PHYs delays require both delays to be set or if the delays are independent. The helper cannot assume this. Dan > > Andrew
> If the dt defines rgmii-rx/tx-id then these values are required not > optional. That was the discussion on the binding. How many times do i need to say it. They are optional. If not specified, default to 2ns. > > > + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", > > > + &dp83869->tx_id_delay); > > > + if (ret) { > > > + dp83869->tx_id_delay = ret; > > > + ret = 0; > > > + } > > > + > > > return ret; > > > } > > > #else > > > @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev, > > > return ret; > > > } > > > +static int dp83869_get_delay(struct phy_device *phydev) > > > +{ > > > + struct dp83869_private *dp83869 = phydev->priv; > > > + int delay_size = ARRAY_SIZE(dp83869_internal_delay); > > > + int tx_delay = 0; > > > + int rx_delay = 0; > > > + > > > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || > > > + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { > > > + tx_delay = phy_get_delay_index(phydev, > > > + &dp83869_internal_delay[0], > > > + delay_size, dp83869->tx_id_delay, > > > + false); > > > + if (tx_delay < 0) { > > > + phydev_err(phydev, "Tx internal delay is invalid\n"); > > > + return tx_delay; > > > + } > > > + } > > > + > > > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || > > > + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { > > > + rx_delay = phy_get_delay_index(phydev, > > > + &dp83869_internal_delay[0], > > > + delay_size, dp83869->rx_id_delay, > > > + false); > > > + if (rx_delay < 0) { > > > + phydev_err(phydev, "Rx internal delay is invalid\n"); > > > + return rx_delay; > > > + } > > > + } > > So any PHY using these properties is going to pretty much reproduce > > this code. Meaning is should all be in a helper. > > The issue here is that the phy_mode may only be rgmii-txid so you only want > to find the tx_delay and return. > > Same with the RXID. How is the helper supposed to know what delay to return > and look for? How does this code do it? It looks at the value of interface. Andrew
Andrew On 5/27/20 8:12 AM, Andrew Lunn wrote: >> If the dt defines rgmii-rx/tx-id then these values are required not >> optional. That was the discussion on the binding. > How many times do i need to say it. They are optional. If not > specified, default to 2ns. OK. I guess then the DP83867 driver is wrong because it specifically states in bold /* RX delay *must* be specified if internal delay of RX is used. */ It was signed off in commit fafc5db28a2ff >>>> + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", >>>> + &dp83869->tx_id_delay); >>>> + if (ret) { >>>> + dp83869->tx_id_delay = ret; >>>> + ret = 0; >>>> + } >>>> + >>>> return ret; >>>> } >>>> #else >>>> @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev, >>>> return ret; >>>> } >>>> +static int dp83869_get_delay(struct phy_device *phydev) >>>> +{ >>>> + struct dp83869_private *dp83869 = phydev->priv; >>>> + int delay_size = ARRAY_SIZE(dp83869_internal_delay); >>>> + int tx_delay = 0; >>>> + int rx_delay = 0; >>>> + >>>> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || >>>> + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { >>>> + tx_delay = phy_get_delay_index(phydev, >>>> + &dp83869_internal_delay[0], >>>> + delay_size, dp83869->tx_id_delay, >>>> + false); >>>> + if (tx_delay < 0) { >>>> + phydev_err(phydev, "Tx internal delay is invalid\n"); >>>> + return tx_delay; >>>> + } >>>> + } >>>> + >>>> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || >>>> + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { >>>> + rx_delay = phy_get_delay_index(phydev, >>>> + &dp83869_internal_delay[0], >>>> + delay_size, dp83869->rx_id_delay, >>>> + false); >>>> + if (rx_delay < 0) { >>>> + phydev_err(phydev, "Rx internal delay is invalid\n"); >>>> + return rx_delay; >>>> + } >>>> + } >>> So any PHY using these properties is going to pretty much reproduce >>> this code. Meaning is should all be in a helper. >> The issue here is that the phy_mode may only be rgmii-txid so you only want >> to find the tx_delay and return. >> >> Same with the RXID. How is the helper supposed to know what delay to return >> and look for? > How does this code do it? It looks at the value of interface. Actually I will be removing this check with setting the delays to default. Dan > Andrew
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index cfb22a21a2e6..1c440e0c0d64 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -64,6 +64,9 @@ #define DP83869_RGMII_TX_CLK_DELAY_EN BIT(1) #define DP83869_RGMII_RX_CLK_DELAY_EN BIT(0) +/* RGMIIDCTL */ +#define DP83869_RGMII_CLK_DELAY_SHIFT 4 + /* STRAP_STS1 bits */ #define DP83869_STRAP_OP_MODE_MASK GENMASK(2, 0) #define DP83869_STRAP_STS1_RESERVED BIT(11) @@ -78,9 +81,6 @@ #define DP83869_PHYCR_FIFO_DEPTH_MASK GENMASK(15, 12) #define DP83869_PHYCR_RESERVED_MASK BIT(11) -/* RGMIIDCTL bits */ -#define DP83869_RGMII_TX_CLK_DELAY_SHIFT 4 - /* IO_MUX_CFG bits */ #define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL 0x1f @@ -99,6 +99,10 @@ #define DP83869_OP_MODE_MII BIT(5) #define DP83869_SGMII_RGMII_BRIDGE BIT(6) +static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750, + 2000, 2250, 2500, 2750, 3000, 3250, + 3500, 3750, 4000}; + enum { DP83869_PORT_MIRRORING_KEEP, DP83869_PORT_MIRRORING_EN, @@ -108,6 +112,8 @@ enum { struct dp83869_private { int tx_fifo_depth; int rx_fifo_depth; + s32 rx_id_delay; + s32 tx_id_delay; int io_impedance; int port_mirroring; bool rxctrl_strap_quirk; @@ -218,6 +224,7 @@ static int dp83869_of_init(struct phy_device *phydev) ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1); if (ret < 0) return ret; + if (ret & DP83869_STRAP_MIRROR_ENABLED) dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN; else @@ -232,6 +239,20 @@ static int dp83869_of_init(struct phy_device *phydev) &dp83869->tx_fifo_depth)) dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", + &dp83869->rx_id_delay); + if (ret) { + dp83869->rx_id_delay = ret; + ret = 0; + } + + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", + &dp83869->tx_id_delay); + if (ret) { + dp83869->tx_id_delay = ret; + ret = 0; + } + return ret; } #else @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev, return ret; } +static int dp83869_get_delay(struct phy_device *phydev) +{ + struct dp83869_private *dp83869 = phydev->priv; + int delay_size = ARRAY_SIZE(dp83869_internal_delay); + int tx_delay = 0; + int rx_delay = 0; + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { + tx_delay = phy_get_delay_index(phydev, + &dp83869_internal_delay[0], + delay_size, dp83869->tx_id_delay, + false); + if (tx_delay < 0) { + phydev_err(phydev, "Tx internal delay is invalid\n"); + return tx_delay; + } + } + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { + rx_delay = phy_get_delay_index(phydev, + &dp83869_internal_delay[0], + delay_size, dp83869->rx_id_delay, + false); + if (rx_delay < 0) { + phydev_err(phydev, "Rx internal delay is invalid\n"); + return rx_delay; + } + } + + return rx_delay | tx_delay << DP83869_RGMII_CLK_DELAY_SHIFT; +} + static int dp83869_config_init(struct phy_device *phydev) { struct dp83869_private *dp83869 = phydev->priv; int ret, val; + int delay; ret = dp83869_configure_mode(phydev, dp83869); if (ret) @@ -394,6 +450,35 @@ static int dp83869_config_init(struct phy_device *phydev) dp83869->clk_output_sel << DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT); + if (phy_interface_is_rgmii(phydev)) { + ret = dp83869_get_delay(phydev); + if (ret < 0) + return ret; + + delay = ret; + ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL, + delay); + if (ret) + return ret; + + val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL); + val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | + DP83869_RGMII_RX_CLK_DELAY_EN); + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + val |= (DP83869_RGMII_TX_CLK_DELAY_EN | + DP83869_RGMII_RX_CLK_DELAY_EN); + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) + val |= DP83869_RGMII_TX_CLK_DELAY_EN; + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) + val |= DP83869_RGMII_RX_CLK_DELAY_EN; + + ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, + val); + } + return ret; }
Add RGMII internal delay configuration for Rx and Tx. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- drivers/net/phy/dp83869.c | 91 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 3 deletions(-)