diff mbox series

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

Message ID 20190102091729.18582-7-vkoul@kernel.org
State New
Headers show
Series None | expand

Commit Message

Vinod Koul Jan. 2, 2019, 9:17 a.m. UTC 
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(+)

-- 
2.20.1

Comments

Andrew Lunn Jan. 2, 2019, 1:40 p.m. UTC | #1 
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
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.

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.

	 Andrew
Vinod Koul Jan. 2, 2019, 2:36 p.m. UTC | #2 
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
diff mbox series

Patch

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");
+
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
 			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		rx_delay = true;
@@ -263,6 +280,12 @@  static int at803x_config_init(struct phy_device *phydev)
 			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		tx_delay = true;
 
+	/* if DT specified disable delay then don't enable it */
+	if (rx_disable_prop)
+		rx_delay = false;
+	if (tx_disable_prop)
+		tx_delay = false;
+
 	if (rx_delay) {
 		ret = at803x_enable_rx_delay(phydev);
 		if (ret < 0)
@@ -275,6 +298,19 @@  static int at803x_config_init(struct phy_device *phydev)
 			return ret;
 	}
 
+	/* if DT specified disable delay then disable it explicitly */
+	if (rx_disable_prop) {
+		ret = at803x_disable_rx_delay(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (tx_disable_prop) {
+		ret = at803x_disable_tx_delay(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }