[U-Boot] [RFC PATCH] net: ag7xxx: Clean up some issues with phy access

[Marek Vasut]

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 at 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;

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;
>  }
>  
> 


-- 
Best regards,
Marek Vasut