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

[Dan Murphy]

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