Re: [PATCH net-next v3 4/4] net: dp83869: Add RGMII internal delay configuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux