[U-Boot] [RFC PATCH] net: ag7xxx: Clean up some issues with phy access
Don't wait forever, Pass errors back, etc.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
--- This is a pass at improving the code quality. This has not been tested in any way.
drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -26,6 +26,7 @@ enum ag7xxx_model { AG7XXX_MODEL_AG934X, };
+/* MAC Configuration 1 */ #define AG7XXX_ETH_CFG1 0x00 #define AG7XXX_ETH_CFG1_SOFT_RST BIT(31) #define AG7XXX_ETH_CFG1_RX_RST BIT(19) @@ -34,6 +35,7 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG1_RX_EN BIT(2) #define AG7XXX_ETH_CFG1_TX_EN BIT(0)
+/* MAC Configuration 2 */ #define AG7XXX_ETH_CFG2 0x04 #define AG7XXX_ETH_CFG2_IF_1000 BIT(9) #define AG7XXX_ETH_CFG2_IF_10_100 BIT(8) @@ -43,26 +45,34 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG2_PAD_CRC_EN BIT(2) #define AG7XXX_ETH_CFG2_FDX BIT(0)
+/* MII Configuration */ #define AG7XXX_ETH_MII_MGMT_CFG 0x20 #define AG7XXX_ETH_MII_MGMT_CFG_RESET BIT(31)
+/* MII Command */ #define AG7XXX_ETH_MII_MGMT_CMD 0x24 #define AG7XXX_ETH_MII_MGMT_CMD_READ 0x1
+/* MII Address */ #define AG7XXX_ETH_MII_MGMT_ADDRESS 0x28 #define AG7XXX_ETH_MII_MGMT_ADDRESS_SHIFT 8
+/* MII Control */ #define AG7XXX_ETH_MII_MGMT_CTRL 0x2c
+/* MII Status */ #define AG7XXX_ETH_MII_MGMT_STATUS 0x30
+/* MII Indicators */ #define AG7XXX_ETH_MII_MGMT_IND 0x34 #define AG7XXX_ETH_MII_MGMT_IND_INVALID BIT(2) #define AG7XXX_ETH_MII_MGMT_IND_BUSY BIT(0)
+/* STA Address 1 & 2 */ #define AG7XXX_ETH_ADDR1 0x40 #define AG7XXX_ETH_ADDR2 0x44
+/* ETH Configuration 0 - 5 */ #define AG7XXX_ETH_FIFO_CFG_0 0x48 #define AG7XXX_ETH_FIFO_CFG_1 0x4c #define AG7XXX_ETH_FIFO_CFG_2 0x50 @@ -70,20 +80,32 @@ enum ag7xxx_model { #define AG7XXX_ETH_FIFO_CFG_4 0x58 #define AG7XXX_ETH_FIFO_CFG_5 0x5c
+/* DMA Transfer Control for Queue 0 */ #define AG7XXX_ETH_DMA_TX_CTRL 0x180 #define AG7XXX_ETH_DMA_TX_CTRL_TXE BIT(0)
+/* Descriptor Address for Queue 0 Tx */ #define AG7XXX_ETH_DMA_TX_DESC 0x184
+/* DMA Tx Status */ #define AG7XXX_ETH_DMA_TX_STATUS 0x188
+/* Rx Control */ #define AG7XXX_ETH_DMA_RX_CTRL 0x18c #define AG7XXX_ETH_DMA_RX_CTRL_RXE BIT(0)
+/* Pointer to Rx Descriptor */ #define AG7XXX_ETH_DMA_RX_DESC 0x190
+/* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194
+/* PHY Control Registers */ + +/* PHY Specific Status Register */ +#define AG7XXX_PHY_PSSR 0x11 +#define AG7XXX_PHY_PSSR_LINK_UP BIT(10) + /* Custom register at 0x18070000 */ #define AG7XXX_GMAC_ETH_CFG 0x00 #define AG7XXX_ETH_CFG_SW_PHY_ADDR_SWAP BIT(8) @@ -269,18 +291,33 @@ static int ag7xxx_switch_reg_write(struct mii_dev *bus, int reg, u32 val) return 0; }
-static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) +static int ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) { u32 data; + unsigned long start; + int ret; + /* No idea if this is long enough or too long */ + int timeout_ms = 1000;
/* Dummy read followed by PHY read/write command. */ - ag7xxx_switch_reg_read(bus, 0x98, &data); + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); + if (ret < 0) + return ret; data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31); - ag7xxx_switch_reg_write(bus, 0x98, data); + ret = ag7xxx_switch_reg_write(bus, 0x98, data); + if (ret < 0) + return ret; + + start = get_timer(0);
/* Wait for operation to finish */ do { - ag7xxx_switch_reg_read(bus, 0x98, &data); + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); + if (ret < 0) + return ret; + + if (get_timer(start) > timeout_ms) + return -ETIMEDOUT; } while (data & BIT(31));
return data & 0xffff; @@ -294,7 +331,11 @@ static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg, u16 val) { - ag7xxx_mdio_rw(bus, addr, reg, val); + int ret; + + ret = ag7xxx_mdio_rw(bus, addr, reg, val); + if (ret < 0) + return ret; return 0; }
@@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */ - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret;
+ if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) + return -ENOLINK; + return 0; }
@@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
- for (i = 0; i < phymax; i++) { - /* Read out link status */ - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); - if (ret < 0) - return ret; - } - return 0; }
On 06/12/2017 10:20 PM, Joe Hershberger wrote:
Don't wait forever, Pass errors back, etc.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
This is a pass at improving the code quality. This has not been tested in any way.
drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -26,6 +26,7 @@ enum ag7xxx_model { AG7XXX_MODEL_AG934X, };
+/* MAC Configuration 1 */ #define AG7XXX_ETH_CFG1 0x00 #define AG7XXX_ETH_CFG1_SOFT_RST BIT(31) #define AG7XXX_ETH_CFG1_RX_RST BIT(19) @@ -34,6 +35,7 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG1_RX_EN BIT(2) #define AG7XXX_ETH_CFG1_TX_EN BIT(0)
+/* MAC Configuration 2 */ #define AG7XXX_ETH_CFG2 0x04 #define AG7XXX_ETH_CFG2_IF_1000 BIT(9) #define AG7XXX_ETH_CFG2_IF_10_100 BIT(8) @@ -43,26 +45,34 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG2_PAD_CRC_EN BIT(2) #define AG7XXX_ETH_CFG2_FDX BIT(0)
+/* MII Configuration */ #define AG7XXX_ETH_MII_MGMT_CFG 0x20 #define AG7XXX_ETH_MII_MGMT_CFG_RESET BIT(31)
+/* MII Command */ #define AG7XXX_ETH_MII_MGMT_CMD 0x24 #define AG7XXX_ETH_MII_MGMT_CMD_READ 0x1
+/* MII Address */ #define AG7XXX_ETH_MII_MGMT_ADDRESS 0x28 #define AG7XXX_ETH_MII_MGMT_ADDRESS_SHIFT 8
+/* MII Control */ #define AG7XXX_ETH_MII_MGMT_CTRL 0x2c
+/* MII Status */ #define AG7XXX_ETH_MII_MGMT_STATUS 0x30
+/* MII Indicators */ #define AG7XXX_ETH_MII_MGMT_IND 0x34 #define AG7XXX_ETH_MII_MGMT_IND_INVALID BIT(2) #define AG7XXX_ETH_MII_MGMT_IND_BUSY BIT(0)
+/* STA Address 1 & 2 */ #define AG7XXX_ETH_ADDR1 0x40 #define AG7XXX_ETH_ADDR2 0x44
+/* ETH Configuration 0 - 5 */ #define AG7XXX_ETH_FIFO_CFG_0 0x48 #define AG7XXX_ETH_FIFO_CFG_1 0x4c #define AG7XXX_ETH_FIFO_CFG_2 0x50 @@ -70,20 +80,32 @@ enum ag7xxx_model { #define AG7XXX_ETH_FIFO_CFG_4 0x58 #define AG7XXX_ETH_FIFO_CFG_5 0x5c
+/* DMA Transfer Control for Queue 0 */ #define AG7XXX_ETH_DMA_TX_CTRL 0x180 #define AG7XXX_ETH_DMA_TX_CTRL_TXE BIT(0)
+/* Descriptor Address for Queue 0 Tx */ #define AG7XXX_ETH_DMA_TX_DESC 0x184
+/* DMA Tx Status */ #define AG7XXX_ETH_DMA_TX_STATUS 0x188
+/* Rx Control */ #define AG7XXX_ETH_DMA_RX_CTRL 0x18c #define AG7XXX_ETH_DMA_RX_CTRL_RXE BIT(0)
+/* Pointer to Rx Descriptor */ #define AG7XXX_ETH_DMA_RX_DESC 0x190
+/* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194
+/* PHY Control Registers */
+/* PHY Specific Status Register */ +#define AG7XXX_PHY_PSSR 0x11 +#define AG7XXX_PHY_PSSR_LINK_UP BIT(10)
/* Custom register at 0x18070000 */ #define AG7XXX_GMAC_ETH_CFG 0x00 #define AG7XXX_ETH_CFG_SW_PHY_ADDR_SWAP BIT(8) @@ -269,18 +291,33 @@ static int ag7xxx_switch_reg_write(struct mii_dev *bus, int reg, u32 val) return 0; }
-static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) +static int ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) { u32 data;
unsigned long start;
int ret;
/* No idea if this is long enough or too long */
int timeout_ms = 1000;
/* Dummy read followed by PHY read/write command. */
- ag7xxx_switch_reg_read(bus, 0x98, &data);
- ret = ag7xxx_switch_reg_read(bus, 0x98, &data);
- if (ret < 0)
return ret;
- ag7xxx_switch_reg_write(bus, 0x98, data);
ret = ag7xxx_switch_reg_write(bus, 0x98, data);
if (ret < 0)
return ret;start = get_timer(0);
/* Wait for operation to finish */ do {
ag7xxx_switch_reg_read(bus, 0x98, &data);
ret = ag7xxx_switch_reg_read(bus, 0x98, &data);if (ret < 0)return ret;if (get_timer(start) > timeout_ms)return -ETIMEDOUT;} while (data & BIT(31));
return data & 0xffff;
@@ -294,7 +331,11 @@ static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg, u16 val) {
- ag7xxx_mdio_rw(bus, addr, reg, val);
- int ret;
- ret = ag7xxx_mdio_rw(bus, addr, reg, val);
- if (ret < 0)
return ret;}
@@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR);if (ret < 0) return ret;
if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))return -ENOLINK;
Are you sure about this ?
return 0;}
@@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
- for (i = 0; i < phymax; i++) {
/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);if (ret < 0)return ret;- }
And this ?
return 0; }
On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut marex@denx.de wrote:
On 06/12/2017 10:20 PM, Joe Hershberger wrote:
Don't wait forever, Pass errors back, etc.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
This is a pass at improving the code quality. This has not been tested in any way.
drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c
[...] SNIP
@@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret;if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))return -ENOLINK;Are you sure about this ?
It seems reasonable to me, but I don't have the HW to test against as noted above.
return 0; }@@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
for (i = 0; i < phymax; i++) {/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);if (ret < 0)return ret;}And this ?
This was based on your comment: "Actually, I think this is only for the switch ports, so we don't care about the link status."
return 0;}
-- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 06/13/2017 06:28 PM, Joe Hershberger wrote:
On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut marex@denx.de wrote:
On 06/12/2017 10:20 PM, Joe Hershberger wrote:
Don't wait forever, Pass errors back, etc.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
This is a pass at improving the code quality. This has not been tested in any way.
drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c
[...] SNIP
@@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret;if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))return -ENOLINK;Are you sure about this ?
It seems reasonable to me, but I don't have the HW to test against as noted above.
CCing Wills . I wouldn't be surprised if the hardware was somehow magically screwed up, so I'd prefer to stick with equivalent changes and maybe changes of the logic in a separate patch.
return 0; }@@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
for (i = 0; i < phymax; i++) {/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);if (ret < 0)return ret;}And this ?
This was based on your comment: "Actually, I think this is only for the switch ports, so we don't care about the link status."
Just so I understand it correctly, the code reads link status and does nothing with it ?
On Mon, Jun 19, 2017 at 4:37 AM, Marek Vasut marex@denx.de wrote:
On 06/13/2017 06:28 PM, Joe Hershberger wrote:
On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut marex@denx.de wrote:
On 06/12/2017 10:20 PM, Joe Hershberger wrote:
Don't wait forever, Pass errors back, etc.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
This is a pass at improving the code quality. This has not been tested in any way.
drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c
[...] SNIP
@@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret;if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))return -ENOLINK;Are you sure about this ?
It seems reasonable to me, but I don't have the HW to test against as noted above.
CCing Wills . I wouldn't be surprised if the hardware was somehow magically screwed up, so I'd prefer to stick with equivalent changes and maybe changes of the logic in a separate patch.
OK.
return 0; }@@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
for (i = 0; i < phymax; i++) {/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);if (ret < 0)return ret;}And this ?
This was based on your comment: "Actually, I think this is only for the switch ports, so we don't care about the link status."
Just so I understand it correctly, the code reads link status and does nothing with it ?
That's how I read it.
-- Best regards, Marek Vasut
On 06/19/2017 05:55 PM, Joe Hershberger wrote:
On Mon, Jun 19, 2017 at 4:37 AM, Marek Vasut marex@denx.de wrote:
On 06/13/2017 06:28 PM, Joe Hershberger wrote:
On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut marex@denx.de wrote:
On 06/12/2017 10:20 PM, Joe Hershberger wrote:
Don't wait forever, Pass errors back, etc.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
This is a pass at improving the code quality. This has not been tested in any way.
drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c
[...] SNIP
@@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret;if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))return -ENOLINK;Are you sure about this ?
It seems reasonable to me, but I don't have the HW to test against as noted above.
CCing Wills . I wouldn't be surprised if the hardware was somehow magically screwed up, so I'd prefer to stick with equivalent changes and maybe changes of the logic in a separate patch.
OK.
return 0; }@@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
for (i = 0; i < phymax; i++) {/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);if (ret < 0)return ret;}And this ?
This was based on your comment: "Actually, I think this is only for the switch ports, so we don't care about the link status."
Just so I understand it correctly, the code reads link status and does nothing with it ?
That's how I read it.
Sigh, well ... if you could split the patch in two, that'd be nice. I will try to test it as soon as I have a chance. If it's broken, I'll try to bisect it then.
The register constants don't use the exact names that are used in the TRM, so add comments that use the exact names so that it is clear what register is being referred to.
https://www.atheros-drivers.com/qualcomm-atheros-datasheets-for-AR9331.html
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
---
Changes in v2: - New - Comments split into a separate change
drivers/net/ag7xxx.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..30771b9 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -26,6 +26,7 @@ enum ag7xxx_model { AG7XXX_MODEL_AG934X, };
+/* MAC Configuration 1 */ #define AG7XXX_ETH_CFG1 0x00 #define AG7XXX_ETH_CFG1_SOFT_RST BIT(31) #define AG7XXX_ETH_CFG1_RX_RST BIT(19) @@ -34,6 +35,7 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG1_RX_EN BIT(2) #define AG7XXX_ETH_CFG1_TX_EN BIT(0)
+/* MAC Configuration 2 */ #define AG7XXX_ETH_CFG2 0x04 #define AG7XXX_ETH_CFG2_IF_1000 BIT(9) #define AG7XXX_ETH_CFG2_IF_10_100 BIT(8) @@ -43,26 +45,34 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG2_PAD_CRC_EN BIT(2) #define AG7XXX_ETH_CFG2_FDX BIT(0)
+/* MII Configuration */ #define AG7XXX_ETH_MII_MGMT_CFG 0x20 #define AG7XXX_ETH_MII_MGMT_CFG_RESET BIT(31)
+/* MII Command */ #define AG7XXX_ETH_MII_MGMT_CMD 0x24 #define AG7XXX_ETH_MII_MGMT_CMD_READ 0x1
+/* MII Address */ #define AG7XXX_ETH_MII_MGMT_ADDRESS 0x28 #define AG7XXX_ETH_MII_MGMT_ADDRESS_SHIFT 8
+/* MII Control */ #define AG7XXX_ETH_MII_MGMT_CTRL 0x2c
+/* MII Status */ #define AG7XXX_ETH_MII_MGMT_STATUS 0x30
+/* MII Indicators */ #define AG7XXX_ETH_MII_MGMT_IND 0x34 #define AG7XXX_ETH_MII_MGMT_IND_INVALID BIT(2) #define AG7XXX_ETH_MII_MGMT_IND_BUSY BIT(0)
+/* STA Address 1 & 2 */ #define AG7XXX_ETH_ADDR1 0x40 #define AG7XXX_ETH_ADDR2 0x44
+/* ETH Configuration 0 - 5 */ #define AG7XXX_ETH_FIFO_CFG_0 0x48 #define AG7XXX_ETH_FIFO_CFG_1 0x4c #define AG7XXX_ETH_FIFO_CFG_2 0x50 @@ -70,18 +80,24 @@ enum ag7xxx_model { #define AG7XXX_ETH_FIFO_CFG_4 0x58 #define AG7XXX_ETH_FIFO_CFG_5 0x5c
+/* DMA Transfer Control for Queue 0 */ #define AG7XXX_ETH_DMA_TX_CTRL 0x180 #define AG7XXX_ETH_DMA_TX_CTRL_TXE BIT(0)
+/* Descriptor Address for Queue 0 Tx */ #define AG7XXX_ETH_DMA_TX_DESC 0x184
+/* DMA Tx Status */ #define AG7XXX_ETH_DMA_TX_STATUS 0x188
+/* Rx Control */ #define AG7XXX_ETH_DMA_RX_CTRL 0x18c #define AG7XXX_ETH_DMA_RX_CTRL_RXE BIT(0)
+/* Pointer to Rx Descriptor */ #define AG7XXX_ETH_DMA_RX_DESC 0x190
+/* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194
/* Custom register at 0x18070000 */
Don't wait forever. Pass errors back to the caller.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
---
Changes in v2: - Isolate error propagation changes
drivers/net/ag7xxx.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index 30771b9..00e6806 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -285,18 +285,33 @@ static int ag7xxx_switch_reg_write(struct mii_dev *bus, int reg, u32 val) return 0; }
-static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) +static int ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) { u32 data; + unsigned long start; + int ret; + /* No idea if this is long enough or too long */ + int timeout_ms = 1000;
/* Dummy read followed by PHY read/write command. */ - ag7xxx_switch_reg_read(bus, 0x98, &data); + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); + if (ret < 0) + return ret; data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31); - ag7xxx_switch_reg_write(bus, 0x98, data); + ret = ag7xxx_switch_reg_write(bus, 0x98, data); + if (ret < 0) + return ret; + + start = get_timer(0);
/* Wait for operation to finish */ do { - ag7xxx_switch_reg_read(bus, 0x98, &data); + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); + if (ret < 0) + return ret; + + if (get_timer(start) > timeout_ms) + return -ETIMEDOUT; } while (data & BIT(31));
return data & 0xffff; @@ -310,7 +325,11 @@ static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg, u16 val) { - ag7xxx_mdio_rw(bus, addr, reg, val); + int ret; + + ret = ag7xxx_mdio_rw(bus, addr, reg, val); + if (ret < 0) + return ret; return 0; }
On 06/26/2017 09:40 PM, Joe Hershberger wrote:
Don't wait forever. Pass errors back to the caller.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Acked-by: Marek Vasut marex@denx.de
Changes in v2:
- Isolate error propagation changes
drivers/net/ag7xxx.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index 30771b9..00e6806 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -285,18 +285,33 @@ static int ag7xxx_switch_reg_write(struct mii_dev *bus, int reg, u32 val) return 0; }
-static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) +static int ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) { u32 data;
unsigned long start;
int ret;
/* No idea if this is long enough or too long */
int timeout_ms = 1000;
/* Dummy read followed by PHY read/write command. */
- ag7xxx_switch_reg_read(bus, 0x98, &data);
- ret = ag7xxx_switch_reg_read(bus, 0x98, &data);
- if (ret < 0)
return ret;
- ag7xxx_switch_reg_write(bus, 0x98, data);
ret = ag7xxx_switch_reg_write(bus, 0x98, data);
if (ret < 0)
return ret;start = get_timer(0);
/* Wait for operation to finish */ do {
ag7xxx_switch_reg_read(bus, 0x98, &data);
ret = ag7xxx_switch_reg_read(bus, 0x98, &data);if (ret < 0)return ret;if (get_timer(start) > timeout_ms)return -ETIMEDOUT;} while (data & BIT(31));
return data & 0xffff;
@@ -310,7 +325,11 @@ static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg, u16 val) {
- ag7xxx_mdio_rw(bus, addr, reg, val);
- int ret;
- ret = ag7xxx_mdio_rw(bus, addr, reg, val);
- if (ret < 0)
return ret;}
In the case of the WAN port, pay attention to the link status. In the case of LAN ports, stop reading the link status since we don't care.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
--- This is a pass at improving the code quality. This has not been tested in any way.
Changes in v2: - New - Split link status change into its own patch (so it can be dropped if it affects behavior)
drivers/net/ag7xxx.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index 00e6806..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -100,6 +100,12 @@ enum ag7xxx_model { /* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194
+/* PHY Control Registers */ + +/* PHY Specific Status Register */ +#define AG7XXX_PHY_PSSR 0x11 +#define AG7XXX_PHY_PSSR_LINK_UP BIT(10) + /* Custom register at 0x18070000 */ #define AG7XXX_GMAC_ETH_CFG 0x00 #define AG7XXX_ETH_CFG_SW_PHY_ADDR_SWAP BIT(8) @@ -758,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */ - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret;
+ if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) + return -ENOLINK; + return 0; }
@@ -778,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
- for (i = 0; i < phymax; i++) { - /* Read out link status */ - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); - if (ret < 0) - return ret; - } - return 0; }
On 06/26/2017 09:40 PM, Joe Hershberger wrote:
In the case of the WAN port, pay attention to the link status. In the case of LAN ports, stop reading the link status since we don't care.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Well, let's see how that works, ev. I'll have to bisect. btw I hope all this is post-2017.07 material.
This is a pass at improving the code quality. This has not been tested in any way.
Changes in v2:
- New - Split link status change into its own patch (so it can be dropped if it affects behavior)
drivers/net/ag7xxx.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index 00e6806..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -100,6 +100,12 @@ enum ag7xxx_model { /* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194
+/* PHY Control Registers */
+/* PHY Specific Status Register */ +#define AG7XXX_PHY_PSSR 0x11 +#define AG7XXX_PHY_PSSR_LINK_UP BIT(10)
/* Custom register at 0x18070000 */ #define AG7XXX_GMAC_ETH_CFG 0x00 #define AG7XXX_ETH_CFG_SW_PHY_ADDR_SWAP BIT(8) @@ -758,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret;
/* Read out link status */
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR);if (ret < 0) return ret;
if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))return -ENOLINK;return 0; }
@@ -778,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; }
- for (i = 0; i < phymax; i++) {
/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);if (ret < 0)return ret;- }
- return 0;
}
On Tue, Jun 27, 2017 at 4:32 AM, Marek Vasut marex@denx.de wrote:
On 06/26/2017 09:40 PM, Joe Hershberger wrote:
In the case of the WAN port, pay attention to the link status. In the case of LAN ports, stop reading the link status since we don't care.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Well, let's see how that works, ev. I'll have to bisect. btw I hope all this is post-2017.07 material.
I'm not intending to apply any of these until you sign off it's functionality.
-Joe
On 06/27/2017 05:46 PM, Joe Hershberger wrote:
On Tue, Jun 27, 2017 at 4:32 AM, Marek Vasut marex@denx.de wrote:
On 06/26/2017 09:40 PM, Joe Hershberger wrote:
In the case of the WAN port, pay attention to the link status. In the case of LAN ports, stop reading the link status since we don't care.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Well, let's see how that works, ev. I'll have to bisect. btw I hope all this is post-2017.07 material.
I'm not intending to apply any of these until you sign off it's functionality.
Hm, maybe Wills can help with that ... CCed
On Tue, Jun 27, 2017 at 11:10 AM, Marek Vasut marex@denx.de wrote:
On 06/27/2017 05:46 PM, Joe Hershberger wrote:
On Tue, Jun 27, 2017 at 4:32 AM, Marek Vasut marex@denx.de wrote:
On 06/26/2017 09:40 PM, Joe Hershberger wrote:
In the case of the WAN port, pay attention to the link status. In the case of LAN ports, stop reading the link status since we don't care.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Well, let's see how that works, ev. I'll have to bisect. btw I hope all this is post-2017.07 material.
I'm not intending to apply any of these until you sign off it's functionality.
Hm, maybe Wills can help with that ... CCed
So... still functional?
Thanks, -Joe
On 06/26/2017 09:40 PM, Joe Hershberger wrote:
The register constants don't use the exact names that are used in the TRM, so add comments that use the exact names so that it is clear what register is being referred to.
https://www.atheros-drivers.com/qualcomm-atheros-datasheets-for-AR9331.html
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Acked-by: Marek Vasut marex@denx.de
Changes in v2:
- New - Comments split into a separate change
drivers/net/ag7xxx.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..30771b9 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -26,6 +26,7 @@ enum ag7xxx_model { AG7XXX_MODEL_AG934X, };
+/* MAC Configuration 1 */ #define AG7XXX_ETH_CFG1 0x00 #define AG7XXX_ETH_CFG1_SOFT_RST BIT(31) #define AG7XXX_ETH_CFG1_RX_RST BIT(19) @@ -34,6 +35,7 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG1_RX_EN BIT(2) #define AG7XXX_ETH_CFG1_TX_EN BIT(0)
+/* MAC Configuration 2 */ #define AG7XXX_ETH_CFG2 0x04 #define AG7XXX_ETH_CFG2_IF_1000 BIT(9) #define AG7XXX_ETH_CFG2_IF_10_100 BIT(8) @@ -43,26 +45,34 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG2_PAD_CRC_EN BIT(2) #define AG7XXX_ETH_CFG2_FDX BIT(0)
+/* MII Configuration */ #define AG7XXX_ETH_MII_MGMT_CFG 0x20 #define AG7XXX_ETH_MII_MGMT_CFG_RESET BIT(31)
+/* MII Command */ #define AG7XXX_ETH_MII_MGMT_CMD 0x24 #define AG7XXX_ETH_MII_MGMT_CMD_READ 0x1
+/* MII Address */ #define AG7XXX_ETH_MII_MGMT_ADDRESS 0x28 #define AG7XXX_ETH_MII_MGMT_ADDRESS_SHIFT 8
+/* MII Control */ #define AG7XXX_ETH_MII_MGMT_CTRL 0x2c
+/* MII Status */ #define AG7XXX_ETH_MII_MGMT_STATUS 0x30
+/* MII Indicators */ #define AG7XXX_ETH_MII_MGMT_IND 0x34 #define AG7XXX_ETH_MII_MGMT_IND_INVALID BIT(2) #define AG7XXX_ETH_MII_MGMT_IND_BUSY BIT(0)
+/* STA Address 1 & 2 */ #define AG7XXX_ETH_ADDR1 0x40 #define AG7XXX_ETH_ADDR2 0x44
+/* ETH Configuration 0 - 5 */ #define AG7XXX_ETH_FIFO_CFG_0 0x48 #define AG7XXX_ETH_FIFO_CFG_1 0x4c #define AG7XXX_ETH_FIFO_CFG_2 0x50 @@ -70,18 +80,24 @@ enum ag7xxx_model { #define AG7XXX_ETH_FIFO_CFG_4 0x58 #define AG7XXX_ETH_FIFO_CFG_5 0x5c
+/* DMA Transfer Control for Queue 0 */ #define AG7XXX_ETH_DMA_TX_CTRL 0x180 #define AG7XXX_ETH_DMA_TX_CTRL_TXE BIT(0)
+/* Descriptor Address for Queue 0 Tx */ #define AG7XXX_ETH_DMA_TX_DESC 0x184
+/* DMA Tx Status */ #define AG7XXX_ETH_DMA_TX_STATUS 0x188
+/* Rx Control */ #define AG7XXX_ETH_DMA_RX_CTRL 0x18c #define AG7XXX_ETH_DMA_RX_CTRL_RXE BIT(0)
+/* Pointer to Rx Descriptor */ #define AG7XXX_ETH_DMA_RX_DESC 0x190
+/* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194
/* Custom register at 0x18070000 */
participants (3)
-
Joe Hershberger -
Joe Hershberger -
Marek Vasut