Re: [PATCH 6/7] net: phy: at803x: Add support to disable tx/rx delays

[Vinod Koul <vkoul@kernel.org>]

Hi Andrew,

Thanks for the comments,

On 02-01-19, 14:40, Andrew Lunn wrote:
> On Wed, Jan 02, 2019 at 02:47:28PM +0530, Vinod Koul wrote:
> > Some controllers require the tx and rx delays to be disabled. So check
> > the property and if present do not enable the delay and disable the
> > delay explicitly.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/net/phy/at803x.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index 63e3d3d774d1..9bfc0d381159 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -122,6 +122,17 @@ static inline int at803x_enable_tx_delay(struct phy_device *phydev)
> >  					AT803X_DEBUG_TX_CLK_DLY_EN);
> >  }
> >  
> > +static inline int at803x_disable_rx_delay(struct phy_device *phydev)
> > +{
> > +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> > +					AT803X_DEBUG_RX_CLK_DLY_EN, 0);
> > +}
> > +
> > +static inline int at803x_disable_tx_delay(struct phy_device *phydev)
> > +{
> > +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
> > +					AT803X_DEBUG_TX_CLK_DLY_EN, 0);
> > +}
> >  /* save relevant PHY registers to private copy */
> >  static void at803x_context_save(struct phy_device *phydev,
> >  				struct at803x_context *context)
> > @@ -250,12 +261,18 @@ static int at803x_probe(struct phy_device *phydev)
> >  static int at803x_config_init(struct phy_device *phydev)
> >  {
> >  	bool rx_delay = false, tx_delay = false;
> > +	bool rx_disable_prop, tx_disable_prop;
> >  	int ret;
> >  
> >  	ret = genphy_config_init(phydev);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	rx_disable_prop = device_property_read_bool(&phydev->mdio.dev,
> > +						    "rx-delay-disable");
> > +	tx_disable_prop = device_property_read_bool(&phydev->mdio.dev,
> > +						    "rx-delay-disable");
> > +
> 
> Hi Vinod
> 
> I understand why you are doing this, to not break backwards
> compatibility, but it is ugly. Lets see if we can avoid it. Thinking

Agreed this is ugly and the reason to do this is not to break existing
users

> allowed here. Here are the use cases i can think of:
> 
> 1) The DT does not specify any phy-mode. The board works because delays
> are enable by the bootloader
> 
> 2) The DT correctly specifies RXID/ID/TXID and the driver does the
> right thing.
> 
> 3) The DT incorrectly specifies no delay, the bootloader however sets
> delays, and the driver does not disable the delay.
> 
> 4) The DT correctly specifies no delay, but the driver does not
> disable delays, and it does not work.
> 
> You are interested in 4) if i understand this patch correct.

that is correct reading of the patch :-)

> 1) should not be a problem. If phy-mode is not one of the RGMII
> values, don't touch the delays.
> 
> 2) works
> 
> 3) is the tricky one. But i would also say that is a bug in the DT.
> The question is, do we want to keep bug compatible?
> 
> I say don't add these new properties. If we have a phy-mode which
> explicitly specifies no delay, clear the delay. And we then fixup
> anything which breaks because of DT bugs.

I do not mind fixing this and doing disable delays for rgmii mode, if we
agree that it would break devices and those should be fixed and not
treated as a regression due to this fix. As long as Dave agree to this,
I can spin a v2 and post :)

~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel