[PATCH] net: phy: TI DP83869 fix invalid clock delay configuration
Setting the clock delay from the device tree settings rx-internal-delay-ps and tx-internal-delay-ps was broken:
- The expected value in the device tree is suppose to be a delay in picoseconds, but the driver only allowed an array index. - Driver converted this array index to the actual delay in picoseconds and tried to apply this in the device register. This however is not a valid register value. The actual logic here was reversed, it converted an register representation of the delay to the device tree delay in picoseconds.
Only when the internal delays were NOT configured in the device tree and they default value of 7 (=2000ps) was used, a valid value was loaded in the register.
Signed-off-by: Frank de Brabander debrabander@gmail.com --- drivers/net/phy/dp83869.c | 53 +++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index 8d32d73b07..f9d4782580 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -81,7 +81,10 @@
/* RGMIIDCTL bits */ #define DP83869_RGMII_TX_CLK_DELAY_SHIFT 4 -#define DP83869_CLK_DELAY_DEF 7 +#define DP83869_CLK_DELAY_STEP 250 +#define DP83869_CLK_DELAY_MIN 250 +#define DP83869_CLK_DELAY_MAX 4000 +#define DP83869_CLK_DELAY_DEFAULT 2000
/* CFG2 bits */ #define MII_DP83869_CFG2_SPEEDOPT_10EN 0x0040 @@ -157,10 +160,6 @@ static int dp83869_config_port_mirroring(struct phy_device *phydev) return 0; }
-static const int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, - 1750, 2000, 2250, 2500, 2750, 3000, - 3250, 3500, 3750, 4000}; - static int dp83869_set_strapped_mode(struct phy_device *phydev) { struct dp83869_private *dp83869 = phydev->priv; @@ -183,7 +182,6 @@ static int dp83869_set_strapped_mode(struct phy_device *phydev) static int dp83869_of_init(struct phy_device *phydev) { struct dp83869_private * const dp83869 = phydev->priv; - const int delay_entries = ARRAY_SIZE(dp83869_internal_delay); int ret; ofnode node;
@@ -238,32 +236,45 @@ static int dp83869_of_init(struct phy_device *phydev) dp83869->tx_fifo_depth = ofnode_read_s32_default(node, "tx-fifo-depth", DP83869_PHYCR_FIFO_DEPTH_4_B_NIB);
+ /* Internal clock delay values can be configured in steps of + * 250ps (0.25ns). The register field for clock delay is 4-bits wide, + * the values range from 0b0000 for 0.25ns to 0b1111 for 4ns. + */ + /* 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) { - dp83869->rx_int_delay = ofnode_read_u32_default(node, "rx-internal-delay-ps", - DP83869_CLK_DELAY_DEF); - if (dp83869->rx_int_delay > delay_entries) { - dp83869->rx_int_delay = DP83869_CLK_DELAY_DEF; - pr_debug("rx-internal-delay-ps not set/invalid, default to %ups\n", - dp83869_internal_delay[dp83869->rx_int_delay]); + dp83869->rx_int_delay = ofnode_read_u32_default(node, + "rx-internal-delay-ps", DP83869_CLK_DELAY_DEFAULT); + if (dp83869->rx_int_delay > DP83869_CLK_DELAY_MAX || + dp83869->rx_int_delay < DP83869_CLK_DELAY_MIN || + dp83869->rx_int_delay % DP83869_CLK_DELAY_STEP) { + dp83869->rx_int_delay = DP83869_CLK_DELAY_DEFAULT; + pr_warn("rx-internal-delay-ps not set/invalid, default" + " to %ups\n", DP83869_CLK_DELAY_DEFAULT); }
- dp83869->rx_int_delay = dp83869_internal_delay[dp83869->rx_int_delay]; + dp83869->rx_int_delay = + (dp83869->rx_int_delay - DP83869_CLK_DELAY_STEP) + / DP83869_CLK_DELAY_STEP; }
- /* TX delay *must* be specified if internal delay of RX is used. */ + /* TX delay *must* be specified if internal delay of TX is used. */ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { - dp83869->tx_int_delay = ofnode_read_u32_default(node, "tx-internal-delay-ps", - DP83869_CLK_DELAY_DEF); - if (dp83869->tx_int_delay > delay_entries) { - dp83869->tx_int_delay = DP83869_CLK_DELAY_DEF; - pr_debug("tx-internal-delay-ps not set/invalid, default to %ups\n", - dp83869_internal_delay[dp83869->tx_int_delay]); + dp83869->tx_int_delay = ofnode_read_u32_default(node, + "tx-internal-delay-ps", DP83869_CLK_DELAY_DEFAULT); + if (dp83869->tx_int_delay > DP83869_CLK_DELAY_MAX || + dp83869->tx_int_delay < DP83869_CLK_DELAY_MIN || + dp83869->tx_int_delay % DP83869_CLK_DELAY_STEP) { + dp83869->tx_int_delay = DP83869_CLK_DELAY_DEFAULT; + pr_warn("tx-internal-delay-ps not set/invalid, default" + " to %ups\n", DP83869_CLK_DELAY_DEFAULT); }
- dp83869->tx_int_delay = dp83869_internal_delay[dp83869->tx_int_delay]; + dp83869->tx_int_delay = + (dp83869->tx_int_delay - DP83869_CLK_DELAY_STEP) + / DP83869_CLK_DELAY_STEP; }
return 0;
On Fri, Oct 6, 2023 at 3:24 PM Frank de Brabander debrabander@gmail.com wrote:
Setting the clock delay from the device tree settings rx-internal-delay-ps and tx-internal-delay-ps was broken:
- The expected value in the device tree is suppose to be a delay in picoseconds, but the driver only allowed an array index.
- Driver converted this array index to the actual delay in picoseconds and tried to apply this in the device register. This however is not a valid register value. The actual logic here was reversed, it converted an register representation of the delay to the device tree delay in picoseconds.
Only when the internal delays were NOT configured in the device tree and they default value of 7 (=2000ps) was used, a valid value was loaded in the register.
Signed-off-by: Frank de Brabander debrabander@gmail.com
drivers/net/phy/dp83869.c | 53 +++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index 8d32d73b07..f9d4782580 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -81,7 +81,10 @@
/* RGMIIDCTL bits */ #define DP83869_RGMII_TX_CLK_DELAY_SHIFT 4 -#define DP83869_CLK_DELAY_DEF 7 +#define DP83869_CLK_DELAY_STEP 250 +#define DP83869_CLK_DELAY_MIN 250 +#define DP83869_CLK_DELAY_MAX 4000 +#define DP83869_CLK_DELAY_DEFAULT 2000
/* CFG2 bits */ #define MII_DP83869_CFG2_SPEEDOPT_10EN 0x0040 @@ -157,10 +160,6 @@ static int dp83869_config_port_mirroring(struct phy_device *phydev) return 0; }
-static const int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500,
1750, 2000, 2250, 2500, 2750, 3000,3250, 3500, 3750, 4000};static int dp83869_set_strapped_mode(struct phy_device *phydev) { struct dp83869_private *dp83869 = phydev->priv; @@ -183,7 +182,6 @@ static int dp83869_set_strapped_mode(struct phy_device *phydev) static int dp83869_of_init(struct phy_device *phydev) { struct dp83869_private * const dp83869 = phydev->priv;
const int delay_entries = ARRAY_SIZE(dp83869_internal_delay); int ret; ofnode node;@@ -238,32 +236,45 @@ static int dp83869_of_init(struct phy_device *phydev) dp83869->tx_fifo_depth = ofnode_read_s32_default(node, "tx-fifo-depth", DP83869_PHYCR_FIFO_DEPTH_4_B_NIB);
/* Internal clock delay values can be configured in steps of* 250ps (0.25ns). The register field for clock delay is 4-bits wide,* the values range from 0b0000 for 0.25ns to 0b1111 for 4ns.*//* 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) {
dp83869->rx_int_delay = ofnode_read_u32_default(node, "rx-internal-delay-ps",DP83869_CLK_DELAY_DEF);if (dp83869->rx_int_delay > delay_entries) {dp83869->rx_int_delay = DP83869_CLK_DELAY_DEF;pr_debug("rx-internal-delay-ps not set/invalid, default to %ups\n",dp83869_internal_delay[dp83869->rx_int_delay]);
dp83869->rx_int_delay = ofnode_read_u32_default(node,"rx-internal-delay-ps", DP83869_CLK_DELAY_DEFAULT);if (dp83869->rx_int_delay > DP83869_CLK_DELAY_MAX ||dp83869->rx_int_delay < DP83869_CLK_DELAY_MIN ||dp83869->rx_int_delay % DP83869_CLK_DELAY_STEP) {dp83869->rx_int_delay = DP83869_CLK_DELAY_DEFAULT;pr_warn("rx-internal-delay-ps not set/invalid, default"" to %ups\n", DP83869_CLK_DELAY_DEFAULT); }
dp83869->rx_int_delay = dp83869_internal_delay[dp83869->rx_int_delay];
dp83869->rx_int_delay =(dp83869->rx_int_delay - DP83869_CLK_DELAY_STEP)/ DP83869_CLK_DELAY_STEP; }
/* TX delay *must* be specified if internal delay of RX is used. */
/* TX delay *must* be specified if internal delay of TX is used. */ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
dp83869->tx_int_delay = ofnode_read_u32_default(node, "tx-internal-delay-ps",DP83869_CLK_DELAY_DEF);if (dp83869->tx_int_delay > delay_entries) {dp83869->tx_int_delay = DP83869_CLK_DELAY_DEF;pr_debug("tx-internal-delay-ps not set/invalid, default to %ups\n",dp83869_internal_delay[dp83869->tx_int_delay]);
dp83869->tx_int_delay = ofnode_read_u32_default(node,"tx-internal-delay-ps", DP83869_CLK_DELAY_DEFAULT);if (dp83869->tx_int_delay > DP83869_CLK_DELAY_MAX ||dp83869->tx_int_delay < DP83869_CLK_DELAY_MIN ||dp83869->tx_int_delay % DP83869_CLK_DELAY_STEP) {dp83869->tx_int_delay = DP83869_CLK_DELAY_DEFAULT;pr_warn("tx-internal-delay-ps not set/invalid, default"" to %ups\n", DP83869_CLK_DELAY_DEFAULT); }
dp83869->tx_int_delay = dp83869_internal_delay[dp83869->tx_int_delay];
dp83869->tx_int_delay =(dp83869->tx_int_delay - DP83869_CLK_DELAY_STEP)/ DP83869_CLK_DELAY_STEP; } return 0;-- 2.30.2
Reviewed-by: Ramon Fried rfried.dev@gmail.com
On Fri, Oct 06, 2023 at 02:24:39PM +0200, Frank de Brabander wrote:
Setting the clock delay from the device tree settings rx-internal-delay-ps and tx-internal-delay-ps was broken:
- The expected value in the device tree is suppose to be a delay in picoseconds, but the driver only allowed an array index.
- Driver converted this array index to the actual delay in picoseconds and tried to apply this in the device register. This however is not a valid register value. The actual logic here was reversed, it converted an register representation of the delay to the device tree delay in picoseconds.
Only when the internal delays were NOT configured in the device tree and they default value of 7 (=2000ps) was used, a valid value was loaded in the register.
Signed-off-by: Frank de Brabander debrabander@gmail.com
Applied to u-boot/master, thanks!
participants (3)
-
Frank de Brabander -
Ramon Fried -
Tom Rini