Re: [PATCH net-next v2 3/3] net: phy: mscc: RGMII skew delay configuration

[Andrew Lunn <andrew@lunn.ch>]

> +static void vsc8584_rgmii_set_skews(struct phy_device *phydev)
> +{
> +	u32 skew_rx, skew_tx;
> +	struct device *dev = &phydev->mdio.dev;
> +
> +	/* We first set the Rx and Tx skews to their default value in h/w
> +	 * (0.2 ns).
> +	 */
> +	skew_rx = VSC8584_RGMII_SKEW_0_2;
> +	skew_tx = VSC8584_RGMII_SKEW_0_2;
> +
> +	/* Based on the interface mode, we then retrieve (if available) Rx
> +	 * and/or Tx skews from the device tree. We do not fail if the
> +	 * properties do not exist, the default skew configuration is a valid
> +	 * one.
> +	 */
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx",
> +				     &skew_rx);
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx",
> +				     &skew_tx);

Hi Antoine

That is not the correct meaning of PHY_INTERFACE_MODE_RGMII_ID etc.
PHY_INTERFACE_MODE_RGMII_ID means add a 2ns delay on both RX and TX.
PHY_INTERFACE_MODE_RGMII_RXID means add a 2ns delay on the RX.
PHY_INTERFACE_MODE_RGMII means 0 delay, with the assumption that
either the MAC is adding the delay, or the PCB is designed with extra
copper tracks to add the delay.

You only need "vsc8584,rgmii-skew-rx" when 2ns is not correct for you
board, and you need more fine grain control.

What is not clearly defined, is how you combine
PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
in DT are absolute values.

There is also some discussion about what should go in DT. #defines
like you have, or time in pS, and the driver converts to register
values. I generally push towards pS.

	 Andrew