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
