Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only

[Serge Semin]

On Fri, Apr 26, 2019 at 11:46:31PM +0200, Andrew Lunn wrote:
> On Sat, Apr 27, 2019 at 12:21:12AM +0300, Serge Semin wrote:
> > It's prone to problems if delay is cleared out for other than RGMII
> > modes. So lets set/clear the TX-delay in the config register only
> > if actually RGMII-like interface mode is requested.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > 
> > ---
> >  drivers/net/phy/realtek.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index ab567a1923ad..a18cb01158f9 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -163,16 +163,24 @@ static int rtl8211c_config_init(struct phy_device *phydev)
> >  static int rtl8211f_config_init(struct phy_device *phydev)
> >  {
> >  	int ret;
> > -	u16 val = 0;
> > +	u16 val;
> >  
> >  	ret = genphy_config_init(phydev);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	/* enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it */
> > -	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > -	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +	/* enable TX-delay for rgmii-id/rgmii-txid, and disable it for rgmii */
> > +	switch (phydev->interface) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +		val = 0;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> >  		val = RTL8211F_TX_DELAY;
> > +		break;
> > +	default: /* the rest of the modes imply leaving delay as is. */
> > +		return 0;
> > +	}
> 
> So there is no control of the RX delay?
> 

As you can see it hasn't been there even before this change. So I suppose
either the hardware just doesn't support it (although the openly available
datasheet states that there is an RXD pin) or the original driver developer
decided to set TX-delay only.

Just to make sure you understand. I am not working for realtek and don't
posses any inside info regarding these PHYs. I was working on a project,
which happened to utilize a rtl8211e PHY. We needed to find a way to
programmatically change the delays setting. So I searched the Internet
and found the U-boot rtl8211f driver and freebsd-folks discussion. This
info has been used to write the config_init method for Linux version of the
PHY' driver. That's it.

> That means PHY_INTERFACE_MODE_RGMII_ID and
> PHY_INTERFACE_MODE_RGMII_RXID are not supported, and you should return
> -EINVAL.
> 

Apparently the current config_init method doesn't support RXID setting.
The patch introduced current function code was submitted by
Martin Blumenstingl in 2016:
https://patchwork.kernel.org/patch/9447581/
and was reviewed by Florian. So we'd better ask him why it was ok to mark
the RGMII_ID as supported while only TX-delay could be set.
I also failed to find anything regarding programmatic rtl8211f delays setting
in the Internet. So at this point we can set TX-delay only for f-model of the PHY.

Anyway lets clarify the situation before to proceed further. You are suggesting
to return an error in case if either RGMII_ID or RGMII_RXID interface mode is
requested to be enabled for the PHY. It's fair seeing the driver can't fully
support either of them. But what about the rest of the modes like GMII, MII
and others? Shouldn't we also return an error instead of leaving a default
delay value?

The same question can be actually asked regarding the config_init method of
rtl8211e PHY, which BTW you already tagged as Reviewed-by.

> This is where we get into interesting backwards compatibility
> issues. Are there any broken DT blobs with rgmii-id or rgmii-rxid,
> which will break with such a change?
> 

Not that I am aware of and which simple grep rtl8211 could find. Do you
know about one?

-Sergey

> 	Andrew