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 <vk...@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
