Re: [PATCH 2/4] net: phy: at803x: Allow specifying the RGMII RX clock delay via phy mode

[Florian Fainelli <f.fainelli@gmail.com>]

Le 25/12/2015 16:27, Martin Blumenstingl a écrit :
> at803x currently automatically enables the RGMII TX clock delay when the
> phy interface mode is PHY_INTERFACE_MODE_RGMII_TXID. The same should be
> done when PHY_INTERFACE_MODE_RGMII_ID is specified.
> Use a similar logic to enable the RGMII RX clock delay as well.
> at803x_context_{save,restore} were not touched because these are only
> used on AR8030 which is a RMII phy (RGMII clock delays are irrelevant).

Few nits below

> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/net/phy/at803x.c | 78 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index f566b6e..0b262a2 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -36,8 +36,10 @@
>  #define AT803X_INSR				0x0013
>  #define AT803X_DEBUG_ADDR			0x1D
>  #define AT803X_DEBUG_DATA			0x1E
> -#define AT803X_DEBUG_SYSTEM_MODE_CTRL		0x05
> -#define AT803X_DEBUG_RGMII_TX_CLK_DLY		BIT(8)
> +#define AT803X_DEBUG_REG_0			0x00

Seems like the previous register name might have been clearer that the
new name you suggest here, did that come from a different GPL tarball or
documentation?

> +#define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
> +#define AT803X_DEBUG_REG_5			0x05
> +#define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
>  
>  #define ATH8030_PHY_ID 0x004dd076
>  #define ATH8031_PHY_ID 0x004dd074
> @@ -61,6 +63,61 @@ struct at803x_context {
>  	u16 led_control;
>  };
>  
> +static int _at803x_debug_reg_read(struct phy_device *phydev, u16 reg)
> +{
> +	int ret;
> +
> +	ret = phy_write(phydev, AT803X_DEBUG_ADDR, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_read(phydev, AT803X_DEBUG_DATA);
> +}
> +
> +static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
> +				 u16 clear, u16 set)
> +{
> +	u16 val;
> +	int ret;
> +
> +	mutex_lock(&phydev->lock);

I do not think the mutex is required here, did you encounter a case
where the PHY state machine was improperly calling into these?

> +
> +	ret = _at803x_debug_reg_read(phydev, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	val = ret & 0xffff;
> +	val &= ~clear;
> +	val |= set;
> +
> +	ret = phy_write(phydev, AT803X_DEBUG_DATA, val);
> +
> +out:
> +	mutex_unlock(&phydev->lock);
> +
> +	return ret;
> +}
> +
> +static int at803x_set_rx_delay(struct phy_device *phydev, bool enable)
> +{
> +	if (enable)
> +		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
> +					     AT803X_DEBUG_RX_CLK_DLY_EN);
> +	else
> +		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> +					     AT803X_DEBUG_RX_CLK_DLY_EN, 0);

The enable argument is always true right now, are we potentially missing
an initialization to false for the non-delay case (RGMII and RGMII_TXID
cases)?

> +}
> +
> +static int at803x_set_tx_delay(struct phy_device *phydev, bool enable)
> +{
> +	if (enable)
> +		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
> +					     AT803X_DEBUG_TX_CLK_DLY_EN);
> +	else
> +		return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
> +					     AT803X_DEBUG_TX_CLK_DLY_EN, 0);
> +}

Same here

> +
>  /* save relevant PHY registers to private copy */
>  static void at803x_context_save(struct phy_device *phydev,
>  				struct at803x_context *context)
> @@ -217,14 +274,17 @@ static int at803x_config_init(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> -		ret = phy_write(phydev, AT803X_DEBUG_ADDR,
> -				AT803X_DEBUG_SYSTEM_MODE_CTRL);
> -		if (ret)
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> +			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> +		ret = at803x_set_rx_delay(phydev, true);
> +		if (ret < 0)
>  			return ret;
> -		ret = phy_write(phydev, AT803X_DEBUG_DATA,
> -				AT803X_DEBUG_RGMII_TX_CLK_DLY);
> -		if (ret)
> +	}
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> +			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> +		ret = at803x_set_tx_delay(phydev, true);
> +		if (ret < 0)
>  			return ret;
>  	}
>  
> 


-- 
Florian