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

From: Andrew Lunn <andrew@lunn.ch>
To: Vinod Koul <vkoul@kernel.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	netdev@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Niklas Cassel <niklas.cassel@linaro.org>,
	"David S . Miller" <davem@davemloft.net>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 6/7] net: phy: at803x: Add support to disable tx/rx delays
Date: Wed, 2 Jan 2019 14:40:54 +0100	[thread overview]
Message-ID: <20190102134054.GF22737@lunn.ch> (raw)
In-Reply-To: <20190102091729.18582-7-vkoul@kernel.org>

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

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