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
