Small improvements for the at803x PHY driver

Small improvements for the at803x PHY driver

[Martin Blumenstingl]

Hello,

while trying to debug a problem on a board with an AR8030 PHY (which turned
out to be an incorrectly configured MDC clock) I made a few changes to the
at803x driver.
Due to lack of other hardware I could only test these changes with an
AR8030 chip.


Martin

[PATCH 1/4] net: phy: at803x: Don't set gbit features for the AR8030 phy

[Martin Blumenstingl]

The 8030 is only a "RMII Fast Ethernet PHY", thus it must not have the
SUPPORTED_1000* bits set.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/at803x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 2d020a3..f566b6e 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -325,7 +325,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_wol		= at803x_get_wol,
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
-	.features		= PHY_GBIT_FEATURES,
+	.features		= PHY_BASIC_FEATURES,
 	.flags			= PHY_HAS_INTERRUPT,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
-- 
2.6.4

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

[Martin Blumenstingl]

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).

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
+#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);
+
+	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);
+}
+
+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);
+}
+
 /* 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;
 	}
 
-- 
2.6.4

[PATCH 3/4] net: phy: at803x: Clean up duplicate register definitions

[Martin Blumenstingl]

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/at803x.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 0b262a2..6e8aafd 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -20,9 +20,12 @@
 #include <linux/gpio/consumer.h>
 
 #define AT803X_INTR_ENABLE			0x12
+#define AT803X_INTR_ENABLE_INIT			0xec00
 #define AT803X_INTR_STATUS			0x13
+
 #define AT803X_SMART_SPEED			0x14
 #define AT803X_LED_CONTROL			0x18
+
 #define AT803X_WOL_ENABLE			0x01
 #define AT803X_DEVICE_ADDR			0x03
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
@@ -31,13 +34,13 @@
 #define AT803X_MMD_ACCESS_CONTROL		0x0D
 #define AT803X_MMD_ACCESS_CONTROL_DATA		0x0E
 #define AT803X_FUNC_DATA			0x4003
-#define AT803X_INER				0x0012
-#define AT803X_INER_INIT			0xec00
-#define AT803X_INSR				0x0013
+
 #define AT803X_DEBUG_ADDR			0x1D
 #define AT803X_DEBUG_DATA			0x1E
+
 #define AT803X_DEBUG_REG_0			0x00
 #define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
+
 #define AT803X_DEBUG_REG_5			0x05
 #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
 
@@ -295,7 +298,7 @@ static int at803x_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
 
-	err = phy_read(phydev, AT803X_INSR);
+	err = phy_read(phydev, AT803X_INTR_STATUS);
 
 	return (err < 0) ? err : 0;
 }
@@ -305,13 +308,13 @@ static int at803x_config_intr(struct phy_device *phydev)
 	int err;
 	int value;
 
-	value = phy_read(phydev, AT803X_INER);
+	value = phy_read(phydev, AT803X_INTR_ENABLE);
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, AT803X_INER,
-				value | AT803X_INER_INIT);
+		err = phy_write(phydev, AT803X_INTR_ENABLE,
+				value | AT803X_INTR_ENABLE_INIT);
 	else
-		err = phy_write(phydev, AT803X_INER, 0);
+		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
 
 	return err;
 }
-- 
2.6.4

[PATCH 4/4] net: phy: at803x: Add the interrupt register bit definitions

[Martin Blumenstingl]

Also use them instead of a magic value when enabling the interrupts.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/at803x.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 6e8aafd..edb0a5f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -20,13 +20,21 @@
 #include <linux/gpio/consumer.h>
 
 #define AT803X_INTR_ENABLE			0x12
-#define AT803X_INTR_ENABLE_INIT			0xec00
+#define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
+#define AT803X_INTR_ENABLE_SPEED_CHANGED	BIT(14)
+#define AT803X_INTR_ENABLE_DUPLEX_CHANGED	BIT(13)
+#define AT803X_INTR_ENABLE_PAGE_RECEIVED	BIT(12)
+#define AT803X_INTR_ENABLE_LINK_FAIL		BIT(11)
+#define AT803X_INTR_ENABLE_LINK_SUCCESS		BIT(10)
+#define AT803X_INTR_ENABLE_WIRESPEED_DOWNGRADE	BIT(5)
+#define AT803X_INTR_ENABLE_POLARITY_CHANGED	BIT(1)
+#define AT803X_INTR_ENABLE_WOL			BIT(0)
+
 #define AT803X_INTR_STATUS			0x13
 
 #define AT803X_SMART_SPEED			0x14
 #define AT803X_LED_CONTROL			0x18
 
-#define AT803X_WOL_ENABLE			0x01
 #define AT803X_DEVICE_ADDR			0x03
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
 #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
@@ -179,14 +187,14 @@ static int at803x_set_wol(struct phy_device *phydev,
 		}
 
 		value = phy_read(phydev, AT803X_INTR_ENABLE);
-		value |= AT803X_WOL_ENABLE;
+		value |= AT803X_INTR_ENABLE_WOL;
 		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
 		if (ret)
 			return ret;
 		value = phy_read(phydev, AT803X_INTR_STATUS);
 	} else {
 		value = phy_read(phydev, AT803X_INTR_ENABLE);
-		value &= (~AT803X_WOL_ENABLE);
+		value &= (~AT803X_INTR_ENABLE_WOL);
 		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
 		if (ret)
 			return ret;
@@ -205,7 +213,7 @@ static void at803x_get_wol(struct phy_device *phydev,
 	wol->wolopts = 0;
 
 	value = phy_read(phydev, AT803X_INTR_ENABLE);
-	if (value & AT803X_WOL_ENABLE)
+	if (value & AT803X_INTR_ENABLE_WOL)
 		wol->wolopts |= WAKE_MAGIC;
 }
 
@@ -217,7 +225,7 @@ static int at803x_suspend(struct phy_device *phydev)
 	mutex_lock(&phydev->lock);
 
 	value = phy_read(phydev, AT803X_INTR_ENABLE);
-	wol_enabled = value & AT803X_WOL_ENABLE;
+	wol_enabled = value & AT803X_INTR_ENABLE_WOL;
 
 	value = phy_read(phydev, MII_BMCR);
 
@@ -310,9 +318,15 @@ static int at803x_config_intr(struct phy_device *phydev)
 
 	value = phy_read(phydev, AT803X_INTR_ENABLE);
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, AT803X_INTR_ENABLE,
-				value | AT803X_INTR_ENABLE_INIT);
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
+		value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
+		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
+		value |= AT803X_INTR_ENABLE_LINK_FAIL;
+		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
+
+		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
+	}
 	else
 		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
 
-- 
2.6.4

Re: Small improvements for the at803x PHY driver

[Mason]

[ CCing people who might be interested in this patch series ]

On 26/12/2015 01:26, Martin Blumenstingl wrote:

> while trying to debug a problem on a board with an AR8030 PHY (which turned
> out to be an incorrectly configured MDC clock) I made a few changes to the
> at803x driver.
> Due to lack of other hardware I could only test these changes with an
> AR8030 chip.

Re: [PATCH 1/4] net: phy: at803x: Don't set gbit features for the AR8030 phy

[Florian Fainelli]

Le 25/12/2015 16:26, Martin Blumenstingl a écrit :
> The 8030 is only a "RMII Fast Ethernet PHY", thus it must not have the
> SUPPORTED_1000* bits set.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/net/phy/at803x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 2d020a3..f566b6e 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -325,7 +325,7 @@ static struct phy_driver at803x_driver[] = {
>  	.get_wol		= at803x_get_wol,
>  	.suspend		= at803x_suspend,
>  	.resume			= at803x_resume,
> -	.features		= PHY_GBIT_FEATURES,
> +	.features		= PHY_BASIC_FEATURES,
>  	.flags			= PHY_HAS_INTERRUPT,
>  	.config_aneg		= genphy_config_aneg,
>  	.read_status		= genphy_read_status,
> 


-- 
Florian

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

[Florian Fainelli]

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

Re: [PATCH 3/4] net: phy: at803x: Clean up duplicate register definitions

[Florian Fainelli]

Le 25/12/2015 16:27, Martin Blumenstingl a écrit :
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/net/phy/at803x.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 0b262a2..6e8aafd 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -20,9 +20,12 @@
>  #include <linux/gpio/consumer.h>
>  
>  #define AT803X_INTR_ENABLE			0x12
> +#define AT803X_INTR_ENABLE_INIT			0xec00
>  #define AT803X_INTR_STATUS			0x13
> +
>  #define AT803X_SMART_SPEED			0x14
>  #define AT803X_LED_CONTROL			0x18
> +
>  #define AT803X_WOL_ENABLE			0x01
>  #define AT803X_DEVICE_ADDR			0x03
>  #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
> @@ -31,13 +34,13 @@
>  #define AT803X_MMD_ACCESS_CONTROL		0x0D
>  #define AT803X_MMD_ACCESS_CONTROL_DATA		0x0E
>  #define AT803X_FUNC_DATA			0x4003
> -#define AT803X_INER				0x0012
> -#define AT803X_INER_INIT			0xec00
> -#define AT803X_INSR				0x0013
> +
>  #define AT803X_DEBUG_ADDR			0x1D
>  #define AT803X_DEBUG_DATA			0x1E
> +
>  #define AT803X_DEBUG_REG_0			0x00
>  #define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
> +
>  #define AT803X_DEBUG_REG_5			0x05
>  #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
>  
> @@ -295,7 +298,7 @@ static int at803x_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
>  
> -	err = phy_read(phydev, AT803X_INSR);
> +	err = phy_read(phydev, AT803X_INTR_STATUS);
>  
>  	return (err < 0) ? err : 0;
>  }
> @@ -305,13 +308,13 @@ static int at803x_config_intr(struct phy_device *phydev)
>  	int err;
>  	int value;
>  
> -	value = phy_read(phydev, AT803X_INER);
> +	value = phy_read(phydev, AT803X_INTR_ENABLE);
>  
>  	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> -		err = phy_write(phydev, AT803X_INER,
> -				value | AT803X_INER_INIT);
> +		err = phy_write(phydev, AT803X_INTR_ENABLE,
> +				value | AT803X_INTR_ENABLE_INIT);
>  	else
> -		err = phy_write(phydev, AT803X_INER, 0);
> +		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
>  
>  	return err;
>  }
> 


-- 
Florian

Re: [PATCH 4/4] net: phy: at803x: Add the interrupt register bit definitions

[Florian Fainelli]

Le 25/12/2015 16:27, Martin Blumenstingl a écrit :
> Also use them instead of a magic value when enabling the interrupts.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/net/phy/at803x.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 6e8aafd..edb0a5f 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -20,13 +20,21 @@
>  #include <linux/gpio/consumer.h>
>  
>  #define AT803X_INTR_ENABLE			0x12
> -#define AT803X_INTR_ENABLE_INIT			0xec00
> +#define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
> +#define AT803X_INTR_ENABLE_SPEED_CHANGED	BIT(14)
> +#define AT803X_INTR_ENABLE_DUPLEX_CHANGED	BIT(13)
> +#define AT803X_INTR_ENABLE_PAGE_RECEIVED	BIT(12)
> +#define AT803X_INTR_ENABLE_LINK_FAIL		BIT(11)
> +#define AT803X_INTR_ENABLE_LINK_SUCCESS		BIT(10)
> +#define AT803X_INTR_ENABLE_WIRESPEED_DOWNGRADE	BIT(5)
> +#define AT803X_INTR_ENABLE_POLARITY_CHANGED	BIT(1)
> +#define AT803X_INTR_ENABLE_WOL			BIT(0)
> +
>  #define AT803X_INTR_STATUS			0x13
>  
>  #define AT803X_SMART_SPEED			0x14
>  #define AT803X_LED_CONTROL			0x18
>  
> -#define AT803X_WOL_ENABLE			0x01
>  #define AT803X_DEVICE_ADDR			0x03
>  #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
>  #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
> @@ -179,14 +187,14 @@ static int at803x_set_wol(struct phy_device *phydev,
>  		}
>  
>  		value = phy_read(phydev, AT803X_INTR_ENABLE);
> -		value |= AT803X_WOL_ENABLE;
> +		value |= AT803X_INTR_ENABLE_WOL;
>  		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
>  		if (ret)
>  			return ret;
>  		value = phy_read(phydev, AT803X_INTR_STATUS);
>  	} else {
>  		value = phy_read(phydev, AT803X_INTR_ENABLE);
> -		value &= (~AT803X_WOL_ENABLE);
> +		value &= (~AT803X_INTR_ENABLE_WOL);
>  		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
>  		if (ret)
>  			return ret;
> @@ -205,7 +213,7 @@ static void at803x_get_wol(struct phy_device *phydev,
>  	wol->wolopts = 0;
>  
>  	value = phy_read(phydev, AT803X_INTR_ENABLE);
> -	if (value & AT803X_WOL_ENABLE)
> +	if (value & AT803X_INTR_ENABLE_WOL)
>  		wol->wolopts |= WAKE_MAGIC;
>  }
>  
> @@ -217,7 +225,7 @@ static int at803x_suspend(struct phy_device *phydev)
>  	mutex_lock(&phydev->lock);
>  
>  	value = phy_read(phydev, AT803X_INTR_ENABLE);
> -	wol_enabled = value & AT803X_WOL_ENABLE;
> +	wol_enabled = value & AT803X_INTR_ENABLE_WOL;
>  
>  	value = phy_read(phydev, MII_BMCR);
>  
> @@ -310,9 +318,15 @@ static int at803x_config_intr(struct phy_device *phydev)
>  
>  	value = phy_read(phydev, AT803X_INTR_ENABLE);
>  
> -	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> -		err = phy_write(phydev, AT803X_INTR_ENABLE,
> -				value | AT803X_INTR_ENABLE_INIT);
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
> +		value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
> +		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
> +		value |= AT803X_INTR_ENABLE_LINK_FAIL;
> +		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
> +
> +		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
> +	}
>  	else
>  		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
>  
> 


-- 
Florian

Re: Small improvements for the at803x PHY driver

[Florian Fainelli]

Le 26/12/2015 03:57, Mason a écrit :
> [ CCing people who might be interested in this patch series ]
> 
> On 26/12/2015 01:26, Martin Blumenstingl wrote:
> 
>> while trying to debug a problem on a board with an AR8030 PHY (which turned
>> out to be an incorrectly configured MDC clock) I made a few changes to the
>> at803x driver.
>> Due to lack of other hardware I could only test these changes with an
>> AR8030 chip.
> 

Overall, this looks pretty nice to me, few comments in individual patches.
-- 
Florian

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

[Martin Blumenstingl]

On Sun, Dec 27, 2015 at 4:28 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> 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?
I could not find any tarball calling the 0x05 register "SYSTEM_MODE_CTRL".
Thus I also have no better name for DEBUG_REG_0.
If you want I can change DEBUG_REG_5 back (or rename both
DEBUG_REG_* to DEBUG_SYSTEM_MODE_CTRL_*).

>> +#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?
Indeed, it seems that I misunderstood the other driver that I had looked at
(which uses a mutex, but now that I'm looking at it again: for a
totally different
reason). I will remove this once all other open points are clear.

>> +
>> +     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)?
My intention was to keep the bootloader/chip defaults, because I don't want
to break existing "users". I just had a look at other drivers and they're doing
exactly what you are proposing: turn the RX/TX delays on *and* off based on
the phy mode. If you want I can change the behavior, but I can't test it due to
lack of (AR8031/AR8033/AR8035) hardware.

>> +}
>> +
>> +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
Same as above

Otherwise: thanks for reviewing these so quick!


Martin

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

[Mason]

On 27/12/2015 04:28, Florian Fainelli wrote:

> Le 25/12/2015 16:27, Martin Blumenstingl wrote:
>
>> 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?

http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf

According to the 8035 data sheet, the debug register at offset 0
is just "Debug register 0". In fact, the only non-reserved bit is
"rgmii rx clock delay enable/disable"

So the SYSTEM_MODE_CTRL name is misleading. Unless the register
has different semantics on the other PHYs?

Regards.

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

[Martin Blumenstingl]

On Sun, Dec 27, 2015 at 9:22 PM, Mason <slash.tmp@free.fr> wrote:
> On 27/12/2015 04:28, Florian Fainelli wrote:
>
>> Le 25/12/2015 16:27, Martin Blumenstingl wrote:
>>
>>> 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?
>
> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>
> According to the 8035 data sheet, the debug register at offset 0
> is just "Debug register 0". In fact, the only non-reserved bit is
> "rgmii rx clock delay enable/disable"
Thanks for verifying this.

@Florian: Did you already have time to look at my comments? I know
it's vacation season, but I'd like to fix all remaining issues soon.

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

[Florian Fainelli]

On 04/01/16 13:17, Martin Blumenstingl wrote:
> On Sun, Dec 27, 2015 at 9:22 PM, Mason <slash.tmp@free.fr> wrote:
>> On 27/12/2015 04:28, Florian Fainelli wrote:
>>
>>> Le 25/12/2015 16:27, Martin Blumenstingl wrote:
>>>
>>>> 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?
>>
>> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>>
>> According to the 8035 data sheet, the debug register at offset 0
>> is just "Debug register 0". In fact, the only non-reserved bit is
>> "rgmii rx clock delay enable/disable"
> Thanks for verifying this.
> 
> @Florian: Did you already have time to look at my comments? I know
> it's vacation season, but I'd like to fix all remaining issues soon.

That answers my question, there were still two other things that needed
to be addressed:

- the bool parameter to the enable RX/TX delay is currently only set to
"true"
- the mutex does not need to be acquired

Could you respin this patch series with this addressed? Thanks!
-- 
Florian

Re: Small improvements for the at803x PHY driver

[Martin Blumenstingl]

On Sun, Dec 27, 2015 at 4:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Overall, this looks pretty nice to me, few comments in individual patches.
I have just sent refreshed versions of all patches and fixed the parts
we've discussed (see the comment section in the corresponding
patches).


Martin