[PATCH 00/15] net: sun8i-emac fixes and cleanups

[PATCH 00/15] net: sun8i-emac fixes and cleanups

[Andre Przywara]

Hi,

while looking at several U-Boot network drivers in the past year, I
typically compared them to the sun8i-emac driver, as a kind of personal
reference. While doing so, I figured that there are quite some things
broken in here, and other things are not so nice.
This series attempts the fix those shortcomings.
Fix-wise we get proper handling of PHY failures (try without a
cable connected), support for external RMII PHYs (as seen on the
Pine64-non-plus model), and more future-proof internal PHY handling.
The rest of the patches are cleanups, which fix things that are wrong,
but we get away with so far, for one or another reason.
This also cleans up a good part of the cache maintenance. There is more
to be done (and I have patches for that), but that requires to drop
the overzealous alignment checks in cache_v7.c first, which is part of
another, upcoming series.

A git repo with those patches can be found here:
https://github.com/apritzel/u-boot/commits/sun8i-emac-cleanup

Please have a look and send comments!

Cheers,
Andre

Andre Przywara (15):
  net: sun8i-emac: Bail out on PHY error
  net: sun8i_emac: Don't hand out TX descriptor too early
  net: sun8i_emac: Simplify mdio_read/mdio_write functions
  net: sun8i_emac: Remove pointless wrapper functions
  net: sun8i_emac: Name magic bits and simplify read-modify-write calls
  net: sun8i_emac: Improve cache maintenance on RX descriptor init
  net: sun8i_emac: Reduce cache maintenance on TX descriptor init
  net: sun8i_emac: Drop unneeded cache invalidation before sending
  net: sun8i_emac: Wrap and simplify cache maintenance operations
  net: sun8i_emac: Fix overlong lines
  net: sun8i_emac: Fix MAC soft reset
  net: sun8i_emac: Simplify and fix error handling for RX
  net: sun8i-emac: Make internal PHY handling more robust
  net: sun8i-emac: Lower MDIO frequency
  sunxi: Pine-H64: Explicitly enable PHY regulator

 configs/pine_h64_defconfig |   1 +
 drivers/net/sun8i_emac.c   | 458 +++++++++++++++++--------------------
 2 files changed, 208 insertions(+), 251 deletions(-)

-- 
2.17.5

[PATCH 01/15] net: sun8i-emac: Bail out on PHY error

[Andre Przywara]

When phy_startup() returns with an error, because there is no link or
the user interrupted the process, we shall stop the _start() routine
and return with an error, instead of proceeding anyway.

This fixes pointless operations when there is no Ethernet cable
connected, for instance.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index e2b05ace8f..d6685e00dc 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -435,6 +435,7 @@ static int _sun8i_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr)
 {
 	u32 reg, v;
 	int timeout = 100;
+	int ret;
 
 	reg = readl((priv->mac_reg + EMAC_CTL1));
 
@@ -473,7 +474,9 @@ static int _sun8i_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr)
 	tx_descs_init(priv);
 
 	/* PHY Start Up */
-	phy_startup(priv->phydev);
+	ret = phy_startup(priv->phydev);
+	if (ret)
+		return ret;
 
 	sun8i_adjust_link(priv, priv->phydev);
 
-- 
2.17.5

[PATCH 02/15] net: sun8i_emac: Don't hand out TX descriptor too early

[Andre Przywara]

When initialising the TX DMA descriptors, we mostly chain them up,
but of course don't know about any data or its length yet.
That means they are still invalid, and the OWN bit should NOT be set
yet.

In fact when we later tell the MAC about the beginning of the chain,
and enable TX DMA in the start() routine, the MAC will start fetching
TX descriptors prematurely, as it can be seen by dumping the TX_DMA_STA
and TX_DMA_CUR_DESC registers.

Clear the owner bit, to not give the MAC the wrong illusion that it
owns the descriptors already.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index d6685e00dc..22163f9405 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -415,7 +415,7 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 		desc_p->buf_addr = (uintptr_t)&txbuffs[idx * CONFIG_ETH_BUFSIZE]
 			;
 		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
-		desc_p->status = (1 << 31);
+		desc_p->status = 0;
 		desc_p->st = 0;
 	}
 
-- 
2.17.5

[PATCH 03/15] net: sun8i_emac: Simplify mdio_read/mdio_write functions

[Andre Przywara]

When preparing the register value for the MDIO command register, we
start with a zeroed register, so there is no need to mask off certain
bits before setting them.
Simplify the sequence, and rename the variable to a more matching
mii_cmd on the way.

Also the open-coded time-out routine can be replaced with a much safer
and easier-to-read call to wait_for_bit_le32().

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 63 ++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 22163f9405..832f71fb60 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -29,6 +29,7 @@
 #include <net.h>
 #include <reset.h>
 #include <dt-bindings/pinctrl/sun4i-a10.h>
+#include <wait_bit.h>
 #if CONFIG_IS_ENABLED(DM_GPIO)
 #include <asm-generic/gpio.h>
 #endif
@@ -166,32 +167,25 @@ static int sun8i_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
 {
 	struct udevice *dev = bus->priv;
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-	ulong start;
-	u32 miiaddr = 0;
-	int timeout = CONFIG_MDIO_TIMEOUT;
+	u32 mii_cmd;
+	int ret;
 
-	miiaddr &= ~MDIO_CMD_MII_WRITE;
-	miiaddr &= ~MDIO_CMD_MII_PHY_REG_ADDR_MASK;
-	miiaddr |= (reg << MDIO_CMD_MII_PHY_REG_ADDR_SHIFT) &
+	mii_cmd = (reg << MDIO_CMD_MII_PHY_REG_ADDR_SHIFT) &
 		MDIO_CMD_MII_PHY_REG_ADDR_MASK;
-
-	miiaddr &= ~MDIO_CMD_MII_PHY_ADDR_MASK;
-
-	miiaddr |= (addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) &
+	mii_cmd |= (addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) &
 		MDIO_CMD_MII_PHY_ADDR_MASK;
 
-	miiaddr |= MDIO_CMD_MII_BUSY;
+	mii_cmd |= MDIO_CMD_MII_BUSY;
 
-	writel(miiaddr, priv->mac_reg + EMAC_MII_CMD);
+	writel(mii_cmd, priv->mac_reg + EMAC_MII_CMD);
 
-	start = get_timer(0);
-	while (get_timer(start) < timeout) {
-		if (!(readl(priv->mac_reg + EMAC_MII_CMD) & MDIO_CMD_MII_BUSY))
-			return readl(priv->mac_reg + EMAC_MII_DATA);
-		udelay(10);
-	};
+	ret = wait_for_bit_le32(priv->mac_reg + EMAC_MII_CMD,
+				MDIO_CMD_MII_BUSY, false,
+				CONFIG_MDIO_TIMEOUT, true);
+	if (ret < 0)
+		return ret;
 
-	return -1;
+	return readl(priv->mac_reg + EMAC_MII_DATA);
 }
 
 static int sun8i_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
@@ -199,35 +193,22 @@ static int sun8i_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
 {
 	struct udevice *dev = bus->priv;
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-	ulong start;
-	u32 miiaddr = 0;
-	int ret = -1, timeout = CONFIG_MDIO_TIMEOUT;
+	u32 mii_cmd;
 
-	miiaddr &= ~MDIO_CMD_MII_PHY_REG_ADDR_MASK;
-	miiaddr |= (reg << MDIO_CMD_MII_PHY_REG_ADDR_SHIFT) &
+	mii_cmd = (reg << MDIO_CMD_MII_PHY_REG_ADDR_SHIFT) &
 		MDIO_CMD_MII_PHY_REG_ADDR_MASK;
-
-	miiaddr &= ~MDIO_CMD_MII_PHY_ADDR_MASK;
-	miiaddr |= (addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) &
+	mii_cmd |= (addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) &
 		MDIO_CMD_MII_PHY_ADDR_MASK;
 
-	miiaddr |= MDIO_CMD_MII_WRITE;
-	miiaddr |= MDIO_CMD_MII_BUSY;
+	mii_cmd |= MDIO_CMD_MII_WRITE;
+	mii_cmd |= MDIO_CMD_MII_BUSY;
 
 	writel(val, priv->mac_reg + EMAC_MII_DATA);
-	writel(miiaddr, priv->mac_reg + EMAC_MII_CMD);
+	writel(mii_cmd, priv->mac_reg + EMAC_MII_CMD);
 
-	start = get_timer(0);
-	while (get_timer(start) < timeout) {
-		if (!(readl(priv->mac_reg + EMAC_MII_CMD) &
-					MDIO_CMD_MII_BUSY)) {
-			ret = 0;
-			break;
-		}
-		udelay(10);
-	};
-
-	return ret;
+	return wait_for_bit_le32(priv->mac_reg + EMAC_MII_CMD,
+				 MDIO_CMD_MII_BUSY, false,
+				 CONFIG_MDIO_TIMEOUT, true);
 }
 
 static int _sun8i_write_hwaddr(struct emac_eth_dev *priv, u8 *mac_id)
-- 
2.17.5

[PATCH 04/15] net: sun8i_emac: Remove pointless wrapper functions

[Andre Przywara]

Apparently due to copying from some older or converted driver, the
sun8i_emac driver contains pointless wrapper functions to bridge
between a legacy driver and the driver model.

Since sun8i_emac is (and always was) driver model only, there is no
reason to have those confusing wrappers. Just remove them, and use
the driver model prototypes directly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 62 +++++++++++-----------------------------
 1 file changed, 16 insertions(+), 46 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 832f71fb60..8413cf50c3 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -211,8 +211,11 @@ static int sun8i_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
 				 CONFIG_MDIO_TIMEOUT, true);
 }
 
-static int _sun8i_write_hwaddr(struct emac_eth_dev *priv, u8 *mac_id)
+static int sun8i_eth_write_hwaddr(struct udevice *dev)
 {
+	struct emac_eth_dev *priv = dev_get_priv(dev);
+	struct eth_pdata *pdata = dev_get_platdata(dev);
+	uchar *mac_id = pdata->enetaddr;
 	u32 macid_lo, macid_hi;
 
 	macid_lo = mac_id[0] + (mac_id[1] << 8) + (mac_id[2] << 16) +
@@ -412,8 +415,9 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 	priv->tx_currdescnum = 0;
 }
 
-static int _sun8i_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr)
+static int sun8i_emac_eth_start(struct udevice *dev)
 {
+	struct emac_eth_dev *priv = dev_get_priv(dev);
 	u32 reg, v;
 	int timeout = 100;
 	int ret;
@@ -433,7 +437,7 @@ static int _sun8i_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr)
 	}
 
 	/* Rewrite mac address after reset */
-	_sun8i_write_hwaddr(priv, enetaddr);
+	sun8i_eth_write_hwaddr(dev);
 
 	v = readl(priv->mac_reg + EMAC_TX_CTL1);
 	/* TX_MD Transmission starts after a full frame located in TX DMA FIFO*/
@@ -542,8 +546,9 @@ static int parse_phy_pins(struct udevice *dev)
 	return 0;
 }
 
-static int _sun8i_eth_recv(struct emac_eth_dev *priv, uchar **packetp)
+static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 {
+	struct emac_eth_dev *priv = dev_get_priv(dev);
 	u32 status, desc_num = priv->rx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->rx_chain[desc_num];
 	int length = -EAGAIN;
@@ -589,9 +594,9 @@ static int _sun8i_eth_recv(struct emac_eth_dev *priv, uchar **packetp)
 	return length;
 }
 
-static int _sun8i_emac_eth_send(struct emac_eth_dev *priv, void *packet,
-				int len)
+static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 {
+	struct emac_eth_dev *priv = dev_get_priv(dev);
 	u32 v, desc_num = priv->tx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->tx_chain[desc_num];
 	uintptr_t desc_start = (uintptr_t)desc_p;
@@ -600,16 +605,16 @@ static int _sun8i_emac_eth_send(struct emac_eth_dev *priv, void *packet,
 
 	uintptr_t data_start = (uintptr_t)desc_p->buf_addr;
 	uintptr_t data_end = data_start +
-		roundup(len, ARCH_DMA_MINALIGN);
+		roundup(length, ARCH_DMA_MINALIGN);
 
 	/* Invalidate entire buffer descriptor */
 	invalidate_dcache_range(desc_start, desc_end);
 
-	desc_p->st = len;
+	desc_p->st = length;
 	/* Mandatory undocumented bit */
 	desc_p->st |= BIT(24);
 
-	memcpy((void *)data_start, packet, len);
+	memcpy((void *)data_start, packet, length);
 
 	/* Flush data to be sent */
 	flush_dcache_range(data_start, data_end);
@@ -639,14 +644,6 @@ static int _sun8i_emac_eth_send(struct emac_eth_dev *priv, void *packet,
 	return 0;
 }
 
-static int sun8i_eth_write_hwaddr(struct udevice *dev)
-{
-	struct eth_pdata *pdata = dev_get_platdata(dev);
-	struct emac_eth_dev *priv = dev_get_priv(dev);
-
-	return _sun8i_write_hwaddr(priv, pdata->enetaddr);
-}
-
 static int sun8i_emac_board_setup(struct emac_eth_dev *priv)
 {
 	int ret;
@@ -743,29 +740,10 @@ static int sun8i_mdio_init(const char *name, struct udevice *priv)
 	return  mdio_register(bus);
 }
 
-static int sun8i_emac_eth_start(struct udevice *dev)
-{
-	struct eth_pdata *pdata = dev_get_platdata(dev);
-
-	return _sun8i_emac_eth_init(dev->priv, pdata->enetaddr);
-}
-
-static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
-{
-	struct emac_eth_dev *priv = dev_get_priv(dev);
-
-	return _sun8i_emac_eth_send(priv, packet, length);
-}
-
-static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
+static int sun8i_eth_free_pkt(struct udevice *dev, uchar *packet,
+			      int length)
 {
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-
-	return _sun8i_eth_recv(priv, packetp);
-}
-
-static int _sun8i_free_pkt(struct emac_eth_dev *priv)
-{
 	u32 desc_num = priv->rx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->rx_chain[desc_num];
 	uintptr_t desc_start = (uintptr_t)desc_p;
@@ -786,14 +764,6 @@ static int _sun8i_free_pkt(struct emac_eth_dev *priv)
 	return 0;
 }
 
-static int sun8i_eth_free_pkt(struct udevice *dev, uchar *packet,
-			      int length)
-{
-	struct emac_eth_dev *priv = dev_get_priv(dev);
-
-	return _sun8i_free_pkt(priv);
-}
-
 static void sun8i_emac_eth_stop(struct udevice *dev)
 {
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-- 
2.17.5

[PATCH 05/15] net: sun8i_emac: Name magic bits and simplify read-modify-write calls

[Andre Przywara]

The EMAC driver contains a lot of magic bits, although the manuals
and the Linux driver have all names for them.

Define those names and use them when programming the registers.
Also this replaces a lot of readl/mask/writel operations with the much
easier-to-read setbits_le32() macro.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 113 +++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 52 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 8413cf50c3..1eafb66698 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -85,15 +85,30 @@
 
 /* H3/A64 EMAC Register's offset */
 #define EMAC_CTL0		0x00
+#define EMAC_CTL0_FULL_DUPLEX		BIT(0)
+#define EMAC_CTL0_SPEED_MASK		GENMASK(3, 2)
+#define EMAC_CTL0_SPEED_10		(0x2 << 2)
+#define EMAC_CTL0_SPEED_100		(0x3 << 2)
+#define EMAC_CTL0_SPEED_1000		(0x0 << 2)
 #define EMAC_CTL1		0x04
+#define EMAC_CTL1_SOFT_RST		BIT(0)
+#define EMAC_CTL1_BURST_LEN_SHIFT	24
 #define EMAC_INT_STA		0x08
 #define EMAC_INT_EN		0x0c
 #define EMAC_TX_CTL0		0x10
+#define	EMAC_TX_CTL0_TX_EN		BIT(31)
 #define EMAC_TX_CTL1		0x14
+#define	EMAC_TX_CTL1_TX_MD		BIT(1)
+#define	EMAC_TX_CTL1_TX_DMA_EN		BIT(30)
+#define	EMAC_TX_CTL1_TX_DMA_START	BIT(31)
 #define EMAC_TX_FLOW_CTL	0x1c
 #define EMAC_TX_DMA_DESC	0x20
 #define EMAC_RX_CTL0		0x24
+#define	EMAC_RX_CTL0_RX_EN		BIT(31)
 #define EMAC_RX_CTL1		0x28
+#define	EMAC_RX_CTL1_RX_MD		BIT(1)
+#define	EMAC_RX_CTL1_RX_DMA_EN		BIT(30)
+#define	EMAC_RX_CTL1_RX_DMA_START	BIT(31)
 #define EMAC_RX_DMA_DESC	0x34
 #define EMAC_MII_CMD		0x48
 #define EMAC_MII_DATA		0x4c
@@ -105,6 +120,11 @@
 #define EMAC_RX_DMA_STA		0xc0
 #define EMAC_RX_CUR_DESC	0xc4
 
+#define EMAC_DESC_OWN_DMA	BIT(31)
+#define EMAC_DESC_LAST_DESC	BIT(30)
+#define EMAC_DESC_FIRST_DESC	BIT(29)
+#define EMAC_DESC_CHAIN_SECOND	BIT(24)
+
 DECLARE_GLOBAL_DATA_PTR;
 
 enum emac_variant {
@@ -117,7 +137,7 @@ enum emac_variant {
 
 struct emac_dma_desc {
 	u32 status;
-	u32 st;
+	u32 ctl_size;
 	u32 buf_addr;
 	u32 next;
 } __aligned(ARCH_DMA_MINALIGN);
@@ -236,21 +256,21 @@ static void sun8i_adjust_link(struct emac_eth_dev *priv,
 	v = readl(priv->mac_reg + EMAC_CTL0);
 
 	if (phydev->duplex)
-		v |= BIT(0);
+		v |= EMAC_CTL0_FULL_DUPLEX;
 	else
-		v &= ~BIT(0);
+		v &= ~EMAC_CTL0_FULL_DUPLEX;
 
-	v &= ~0x0C;
+	v &= ~EMAC_CTL0_SPEED_MASK;
 
 	switch (phydev->speed) {
 	case 1000:
+		v |= EMAC_CTL0_SPEED_1000;
 		break;
 	case 100:
-		v |= BIT(2);
-		v |= BIT(3);
+		v |= EMAC_CTL0_SPEED_100;
 		break;
 	case 10:
-		v |= BIT(3);
+		v |= EMAC_CTL0_SPEED_10;
 		break;
 	}
 	writel(v, priv->mac_reg + EMAC_CTL0);
@@ -372,8 +392,8 @@ static void rx_descs_init(struct emac_eth_dev *priv)
 		desc_p->buf_addr = (uintptr_t)&rxbuffs[idx * CONFIG_ETH_BUFSIZE]
 			;
 		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
-		desc_p->st |= CONFIG_ETH_RXSIZE;
-		desc_p->status = BIT(31);
+		desc_p->ctl_size |= CONFIG_ETH_RXSIZE;
+		desc_p->status = EMAC_DESC_OWN_DMA;
 	}
 
 	/* Correcting the last pointer of the chain */
@@ -399,8 +419,8 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 		desc_p->buf_addr = (uintptr_t)&txbuffs[idx * CONFIG_ETH_BUFSIZE]
 			;
 		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
+		desc_p->ctl_size = 0;
 		desc_p->status = 0;
-		desc_p->st = 0;
 	}
 
 	/* Correcting the last pointer of the chain */
@@ -418,7 +438,7 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 static int sun8i_emac_eth_start(struct udevice *dev)
 {
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-	u32 reg, v;
+	u32 reg;
 	int timeout = 100;
 	int ret;
 
@@ -439,20 +459,17 @@ static int sun8i_emac_eth_start(struct udevice *dev)
 	/* Rewrite mac address after reset */
 	sun8i_eth_write_hwaddr(dev);
 
-	v = readl(priv->mac_reg + EMAC_TX_CTL1);
-	/* TX_MD Transmission starts after a full frame located in TX DMA FIFO*/
-	v |= BIT(1);
-	writel(v, priv->mac_reg + EMAC_TX_CTL1);
+	/* transmission starts after the full frame arrived in TX DMA FIFO */
+	setbits_le32(priv->mac_reg + EMAC_TX_CTL1, EMAC_TX_CTL1_TX_MD);
 
-	v = readl(priv->mac_reg + EMAC_RX_CTL1);
-	/* RX_MD RX DMA reads data from RX DMA FIFO to host memory after a
+	/*
+	 * RX DMA reads data from RX DMA FIFO to host memory after a
 	 * complete frame has been written to RX DMA FIFO
 	 */
-	v |= BIT(1);
-	writel(v, priv->mac_reg + EMAC_RX_CTL1);
+	setbits_le32(priv->mac_reg + EMAC_RX_CTL1, EMAC_RX_CTL1_RX_MD);
 
-	/* DMA */
-	writel(8 << 24, priv->mac_reg + EMAC_CTL1);
+	/* DMA burst length */
+	writel(8 << EMAC_CTL1_BURST_LEN_SHIFT, priv->mac_reg + EMAC_CTL1);
 
 	/* Initialize rx/tx descriptors */
 	rx_descs_init(priv);
@@ -465,18 +482,13 @@ static int sun8i_emac_eth_start(struct udevice *dev)
 
 	sun8i_adjust_link(priv, priv->phydev);
 
-	/* Start RX DMA */
-	v = readl(priv->mac_reg + EMAC_RX_CTL1);
-	v |= BIT(30);
-	writel(v, priv->mac_reg + EMAC_RX_CTL1);
-	/* Start TX DMA */
-	v = readl(priv->mac_reg + EMAC_TX_CTL1);
-	v |= BIT(30);
-	writel(v, priv->mac_reg + EMAC_TX_CTL1);
+	/* Start RX/TX DMA */
+	setbits_le32(priv->mac_reg + EMAC_RX_CTL1, EMAC_RX_CTL1_RX_DMA_EN);
+	setbits_le32(priv->mac_reg + EMAC_TX_CTL1, EMAC_TX_CTL1_TX_DMA_EN);
 
 	/* Enable RX/TX */
-	setbits_le32(priv->mac_reg + EMAC_RX_CTL0, BIT(31));
-	setbits_le32(priv->mac_reg + EMAC_TX_CTL0, BIT(31));
+	setbits_le32(priv->mac_reg + EMAC_RX_CTL0, EMAC_RX_CTL0_RX_EN);
+	setbits_le32(priv->mac_reg + EMAC_TX_CTL0, EMAC_TX_CTL0_TX_EN);
 
 	return 0;
 }
@@ -566,7 +578,7 @@ static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 	status = desc_p->status;
 
 	/* Check for DMA own bit */
-	if (!(status & BIT(31))) {
+	if (!(status & EMAC_DESC_OWN_DMA)) {
 		length = (desc_p->status >> 16) & 0x3FFF;
 
 		if (length < 0x40) {
@@ -597,7 +609,7 @@ static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 {
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-	u32 v, desc_num = priv->tx_currdescnum;
+	u32 desc_num = priv->tx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->tx_chain[desc_num];
 	uintptr_t desc_start = (uintptr_t)desc_p;
 	uintptr_t desc_end = desc_start +
@@ -610,22 +622,16 @@ static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 	/* Invalidate entire buffer descriptor */
 	invalidate_dcache_range(desc_start, desc_end);
 
-	desc_p->st = length;
-	/* Mandatory undocumented bit */
-	desc_p->st |= BIT(24);
+	desc_p->ctl_size = length | EMAC_DESC_CHAIN_SECOND;
 
 	memcpy((void *)data_start, packet, length);
 
 	/* Flush data to be sent */
 	flush_dcache_range(data_start, data_end);
 
-	/* frame end */
-	desc_p->st |= BIT(30);
-	desc_p->st |= BIT(31);
-
-	/*frame begin */
-	desc_p->st |= BIT(29);
-	desc_p->status = BIT(31);
+	/* frame begin and end */
+	desc_p->ctl_size |= EMAC_DESC_LAST_DESC | EMAC_DESC_FIRST_DESC;
+	desc_p->status = EMAC_DESC_OWN_DMA;
 
 	/*Descriptors st and status field has changed, so FLUSH it */
 	flush_dcache_range(desc_start, desc_end);
@@ -636,10 +642,12 @@ static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 	priv->tx_currdescnum = desc_num;
 
 	/* Start the DMA */
-	v = readl(priv->mac_reg + EMAC_TX_CTL1);
-	v |= BIT(31);/* mandatory */
-	v |= BIT(30);/* mandatory */
-	writel(v, priv->mac_reg + EMAC_TX_CTL1);
+	setbits_le32(priv->mac_reg + EMAC_TX_CTL1, EMAC_TX_CTL1_TX_DMA_START);
+
+	/*
+	 * Since we copied the data above, we return here without waiting
+	 * for the packet to be actually send out.
+	 */
 
 	return 0;
 }
@@ -751,7 +759,7 @@ static int sun8i_eth_free_pkt(struct udevice *dev, uchar *packet,
 		roundup(sizeof(u32), ARCH_DMA_MINALIGN);
 
 	/* Make the current descriptor valid again */
-	desc_p->status |= BIT(31);
+	desc_p->status |= EMAC_DESC_OWN_DMA;
 
 	/* Flush Status field of descriptor */
 	flush_dcache_range(desc_start, desc_end);
@@ -769,11 +777,12 @@ static void sun8i_emac_eth_stop(struct udevice *dev)
 	struct emac_eth_dev *priv = dev_get_priv(dev);
 
 	/* Stop Rx/Tx transmitter */
-	clrbits_le32(priv->mac_reg + EMAC_RX_CTL0, BIT(31));
-	clrbits_le32(priv->mac_reg + EMAC_TX_CTL0, BIT(31));
+	clrbits_le32(priv->mac_reg + EMAC_RX_CTL0, EMAC_RX_CTL0_RX_EN);
+	clrbits_le32(priv->mac_reg + EMAC_TX_CTL0, EMAC_TX_CTL0_TX_EN);
 
-	/* Stop TX DMA */
-	clrbits_le32(priv->mac_reg + EMAC_TX_CTL1, BIT(30));
+	/* Stop RX/TX DMA */
+	clrbits_le32(priv->mac_reg + EMAC_TX_CTL1, EMAC_TX_CTL1_TX_DMA_EN);
+	clrbits_le32(priv->mac_reg + EMAC_RX_CTL1, EMAC_RX_CTL1_RX_DMA_EN);
 
 	phy_shutdown(priv->phydev);
 }
-- 
2.17.5

[PATCH 06/15] net: sun8i_emac: Improve cache maintenance on RX descriptor init

[Andre Przywara]

Before we initialise the RX descriptors, there is no need to *clean*
them from the cache, as we touch them for the first time.
However we should cover the case that those buffers contain dirty cache
lines, which could be evicted and written back to DRAM any time later,
in the worst case *after* the MAC has transferred a packet into them.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 1eafb66698..44ec8f3a63 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -383,16 +383,21 @@ static void rx_descs_init(struct emac_eth_dev *priv)
 	struct emac_dma_desc *desc_p;
 	u32 idx;
 
-	/* flush Rx buffers */
-	flush_dcache_range((uintptr_t)rxbuffs, (ulong)rxbuffs +
-			RX_TOTAL_BUFSIZE);
+	/*
+	 * Make sure we don't have dirty cache lines around, which could
+	 * be cleaned to DRAM *after* the MAC has already written data to it.
+	 */
+	invalidate_dcache_range((uintptr_t)desc_table_p,
+			      (uintptr_t)desc_table_p + sizeof(priv->rx_chain));
+	invalidate_dcache_range((uintptr_t)rxbuffs,
+				(uintptr_t)rxbuffs + sizeof(priv->rxbuffer));
 
 	for (idx = 0; idx < CONFIG_RX_DESCR_NUM; idx++) {
 		desc_p = &desc_table_p[idx];
 		desc_p->buf_addr = (uintptr_t)&rxbuffs[idx * CONFIG_ETH_BUFSIZE]
 			;
 		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
-		desc_p->ctl_size |= CONFIG_ETH_RXSIZE;
+		desc_p->ctl_size = CONFIG_ETH_RXSIZE;
 		desc_p->status = EMAC_DESC_OWN_DMA;
 	}
 
-- 
2.17.5

[PATCH 07/15] net: sun8i_emac: Reduce cache maintenance on TX descriptor init

[Andre Przywara]

When we initialise the TX descriptors, there is no need yet to clean
them all to memory, as they don't contain any data yet. Later we will
touch and clean each descriptor anyway.
However we tell the MAC about the beginning of the chain, so we have to
clean at least the first descriptor, to make it clear that this is empty
and there are no packets to transfer yet.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 44ec8f3a63..38c56bde70 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -431,10 +431,10 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 	/* Correcting the last pointer of the chain */
 	desc_p->next =  (uintptr_t)&desc_table_p[0];
 
-	/* Flush all Tx buffer descriptors */
+	/* Flush the first TX buffer descriptor we will tell the MAC about. */
 	flush_dcache_range((uintptr_t)priv->tx_chain,
 			   (uintptr_t)priv->tx_chain +
-			sizeof(priv->tx_chain));
+			   sizeof(priv->tx_chain[0]));
 
 	writel((uintptr_t)&desc_table_p[0], priv->mac_reg + EMAC_TX_DMA_DESC);
 	priv->tx_currdescnum = 0;
-- 
2.17.5

[PATCH 08/15] net: sun8i_emac: Drop unneeded cache invalidation before sending

[Andre Przywara]

There is no reason to invalidate a TX descriptor before we are setting
it up, as we will only write to a field.

Remove the not needed invalidate_dcache_range() call.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 38c56bde70..b704ec47a3 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -624,9 +624,6 @@ static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 	uintptr_t data_end = data_start +
 		roundup(length, ARCH_DMA_MINALIGN);
 
-	/* Invalidate entire buffer descriptor */
-	invalidate_dcache_range(desc_start, desc_end);
-
 	desc_p->ctl_size = length | EMAC_DESC_CHAIN_SECOND;
 
 	memcpy((void *)data_start, packet, length);
-- 
2.17.5

[PATCH 09/15] net: sun8i_emac: Wrap and simplify cache maintenance operations

[Andre Przywara]

To meet the current alignment requirements for our cache maintenance
functions, we were explicitly aligning the *arguments* to those calls.
This is not only ugly to read, but also wrong, as we need to make sure
we are not accidentally stepping on other data.

Provide wrapper functions for the common case of cleaning or
invalidating a descriptor, to make the cache maintenance calls more
readable. This fixes a good deal of the problematic calls.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index b704ec47a3..939dfbade1 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -376,6 +376,14 @@ static int sun8i_phy_init(struct emac_eth_dev *priv, void *dev)
 	return 0;
 }
 
+#define cache_clean_descriptor(desc)					\
+	flush_dcache_range((uintptr_t)(desc), 				\
+			   (uintptr_t)(desc) + sizeof(struct emac_dma_desc))
+
+#define cache_inv_descriptor(desc)					\
+	invalidate_dcache_range((uintptr_t)(desc),			\
+			       (uintptr_t)(desc) + sizeof(struct emac_dma_desc))
+
 static void rx_descs_init(struct emac_eth_dev *priv)
 {
 	struct emac_dma_desc *desc_table_p = &priv->rx_chain[0];
@@ -432,9 +440,7 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 	desc_p->next =  (uintptr_t)&desc_table_p[0];
 
 	/* Flush the first TX buffer descriptor we will tell the MAC about. */
-	flush_dcache_range((uintptr_t)priv->tx_chain,
-			   (uintptr_t)priv->tx_chain +
-			   sizeof(priv->tx_chain[0]));
+	cache_clean_descriptor(desc_table_p);
 
 	writel((uintptr_t)&desc_table_p[0], priv->mac_reg + EMAC_TX_DMA_DESC);
 	priv->tx_currdescnum = 0;
@@ -570,15 +576,11 @@ static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 	struct emac_dma_desc *desc_p = &priv->rx_chain[desc_num];
 	int length = -EAGAIN;
 	int good_packet = 1;
-	uintptr_t desc_start = (uintptr_t)desc_p;
-	uintptr_t desc_end = desc_start +
-		roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN);
-
 	ulong data_start = (uintptr_t)desc_p->buf_addr;
 	ulong data_end;
 
 	/* Invalidate entire buffer descriptor */
-	invalidate_dcache_range(desc_start, desc_end);
+	cache_inv_descriptor(desc_p);
 
 	status = desc_p->status;
 
@@ -616,10 +618,6 @@ static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 	struct emac_eth_dev *priv = dev_get_priv(dev);
 	u32 desc_num = priv->tx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->tx_chain[desc_num];
-	uintptr_t desc_start = (uintptr_t)desc_p;
-	uintptr_t desc_end = desc_start +
-		roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN);
-
 	uintptr_t data_start = (uintptr_t)desc_p->buf_addr;
 	uintptr_t data_end = data_start +
 		roundup(length, ARCH_DMA_MINALIGN);
@@ -635,8 +633,8 @@ static int sun8i_emac_eth_send(struct udevice *dev, void *packet, int length)
 	desc_p->ctl_size |= EMAC_DESC_LAST_DESC | EMAC_DESC_FIRST_DESC;
 	desc_p->status = EMAC_DESC_OWN_DMA;
 
-	/*Descriptors st and status field has changed, so FLUSH it */
-	flush_dcache_range(desc_start, desc_end);
+	/* make sure the MAC reads the actual data from DRAM */
+	cache_clean_descriptor(desc_p);
 
 	/* Move to next Descriptor and wrap around */
 	if (++desc_num >= CONFIG_TX_DESCR_NUM)
@@ -756,15 +754,12 @@ static int sun8i_eth_free_pkt(struct udevice *dev, uchar *packet,
 	struct emac_eth_dev *priv = dev_get_priv(dev);
 	u32 desc_num = priv->rx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->rx_chain[desc_num];
-	uintptr_t desc_start = (uintptr_t)desc_p;
-	uintptr_t desc_end = desc_start +
-		roundup(sizeof(u32), ARCH_DMA_MINALIGN);
 
-	/* Make the current descriptor valid again */
+	/* give the current descriptor back to the MAC */
 	desc_p->status |= EMAC_DESC_OWN_DMA;
 
 	/* Flush Status field of descriptor */
-	flush_dcache_range(desc_start, desc_end);
+	cache_clean_descriptor(desc_p);
 
 	/* Move to next desc and wrap-around condition. */
 	if (++desc_num >= CONFIG_RX_DESCR_NUM)
-- 
2.17.5

[PATCH 10/15] net: sun8i_emac: Fix overlong lines

[Andre Przywara]

When iterating over all RX/TX buffers, we were using a rather long "idx"
control variable, which lead to a nasty overlong line.

Replace "idx" with "i" to avoid this.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 939dfbade1..93b746161c 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -389,7 +389,7 @@ static void rx_descs_init(struct emac_eth_dev *priv)
 	struct emac_dma_desc *desc_table_p = &priv->rx_chain[0];
 	char *rxbuffs = &priv->rxbuffer[0];
 	struct emac_dma_desc *desc_p;
-	u32 idx;
+	int i;
 
 	/*
 	 * Make sure we don't have dirty cache lines around, which could
@@ -400,11 +400,10 @@ static void rx_descs_init(struct emac_eth_dev *priv)
 	invalidate_dcache_range((uintptr_t)rxbuffs,
 				(uintptr_t)rxbuffs + sizeof(priv->rxbuffer));
 
-	for (idx = 0; idx < CONFIG_RX_DESCR_NUM; idx++) {
-		desc_p = &desc_table_p[idx];
-		desc_p->buf_addr = (uintptr_t)&rxbuffs[idx * CONFIG_ETH_BUFSIZE]
-			;
-		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
+	for (i = 0; i < CONFIG_RX_DESCR_NUM; i++) {
+		desc_p = &desc_table_p[i];
+		desc_p->buf_addr = (uintptr_t)&rxbuffs[i * CONFIG_ETH_BUFSIZE];
+		desc_p->next = (uintptr_t)&desc_table_p[i + 1];
 		desc_p->ctl_size = CONFIG_ETH_RXSIZE;
 		desc_p->status = EMAC_DESC_OWN_DMA;
 	}
@@ -425,13 +424,12 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 	struct emac_dma_desc *desc_table_p = &priv->tx_chain[0];
 	char *txbuffs = &priv->txbuffer[0];
 	struct emac_dma_desc *desc_p;
-	u32 idx;
+	int i;
 
-	for (idx = 0; idx < CONFIG_TX_DESCR_NUM; idx++) {
-		desc_p = &desc_table_p[idx];
-		desc_p->buf_addr = (uintptr_t)&txbuffs[idx * CONFIG_ETH_BUFSIZE]
-			;
-		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
+	for (i = 0; i < CONFIG_TX_DESCR_NUM; i++) {
+		desc_p = &desc_table_p[i];
+		desc_p->buf_addr = (uintptr_t)&txbuffs[i * CONFIG_ETH_BUFSIZE];
+		desc_p->next = (uintptr_t)&desc_table_p[i + 1];
 		desc_p->ctl_size = 0;
 		desc_p->status = 0;
 	}
-- 
2.17.5

[PATCH 11/15] net: sun8i_emac: Fix MAC soft reset

[Andre Przywara]

The EMAC soft reset routine was subtly broken, using an open coded
timeout routine without any actual delay.
Remove the unneeded initial reset bit read, and call wait_for_bit_le32()
to handle the timeout correctly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 93b746161c..e2694d3619 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -447,22 +447,15 @@ static void tx_descs_init(struct emac_eth_dev *priv)
 static int sun8i_emac_eth_start(struct udevice *dev)
 {
 	struct emac_eth_dev *priv = dev_get_priv(dev);
-	u32 reg;
-	int timeout = 100;
 	int ret;
 
-	reg = readl((priv->mac_reg + EMAC_CTL1));
-
-	if (!(reg & 0x1)) {
-		/* Soft reset MAC */
-		setbits_le32((priv->mac_reg + EMAC_CTL1), 0x1);
-		do {
-			reg = readl(priv->mac_reg + EMAC_CTL1);
-		} while ((reg & 0x01) != 0 &&  (--timeout));
-		if (!timeout) {
-			printf("%s: Timeout\n", __func__);
-			return -1;
-		}
+	/* Soft reset MAC */
+	writel(EMAC_CTL1_SOFT_RST, priv->mac_reg + EMAC_CTL1);
+	ret = wait_for_bit_le32(priv->mac_reg + EMAC_CTL1,
+				EMAC_CTL1_SOFT_RST, false, 10, true);
+	if (ret) {
+		printf("%s: Timeout\n", __func__);
+		return ret;
 	}
 
 	/* Rewrite mac address after reset */
-- 
2.17.5

[PATCH 12/15] net: sun8i_emac: Simplify and fix error handling for RX

[Andre Przywara]

The error handling in recv() is somewhat broken, for instance
good_packet isn't really used, and it's hardly readable. Also we try
to check for short or too big packets, but those are actually filtered
out by the hardware.

Simplify the whole routine and improve the error handling:
- Bail out early if the current RX descriptor is not ready.
- Enable propagation of runt, huge and broken packets.
- Check for runt and huge packets, and return 0 to indicate this.
  This will force the framework to call free_pkt for cleanup.
- Avoid aligning the packet buffer for invalidation again.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 56 +++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index e2694d3619..48ba244f21 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -107,6 +107,8 @@
 #define	EMAC_RX_CTL0_RX_EN		BIT(31)
 #define EMAC_RX_CTL1		0x28
 #define	EMAC_RX_CTL1_RX_MD		BIT(1)
+#define	EMAC_RX_CTL1_RX_RUNT_FRM	BIT(2)
+#define	EMAC_RX_CTL1_RX_ERR_FRM		BIT(3)
 #define	EMAC_RX_CTL1_RX_DMA_EN		BIT(30)
 #define	EMAC_RX_CTL1_RX_DMA_START	BIT(31)
 #define EMAC_RX_DMA_DESC	0x34
@@ -125,6 +127,8 @@
 #define EMAC_DESC_FIRST_DESC	BIT(29)
 #define EMAC_DESC_CHAIN_SECOND	BIT(24)
 
+#define EMAC_DESC_RX_ERROR_MASK	0x400068db
+
 DECLARE_GLOBAL_DATA_PTR;
 
 enum emac_variant {
@@ -485,7 +489,8 @@ static int sun8i_emac_eth_start(struct udevice *dev)
 	sun8i_adjust_link(priv, priv->phydev);
 
 	/* Start RX/TX DMA */
-	setbits_le32(priv->mac_reg + EMAC_RX_CTL1, EMAC_RX_CTL1_RX_DMA_EN);
+	setbits_le32(priv->mac_reg + EMAC_RX_CTL1, EMAC_RX_CTL1_RX_DMA_EN |
+		     EMAC_RX_CTL1_RX_ERR_FRM | EMAC_RX_CTL1_RX_RUNT_FRM);
 	setbits_le32(priv->mac_reg + EMAC_TX_CTL1, EMAC_TX_CTL1_TX_DMA_EN);
 
 	/* Enable RX/TX */
@@ -565,10 +570,8 @@ static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 	struct emac_eth_dev *priv = dev_get_priv(dev);
 	u32 status, desc_num = priv->rx_currdescnum;
 	struct emac_dma_desc *desc_p = &priv->rx_chain[desc_num];
-	int length = -EAGAIN;
-	int good_packet = 1;
-	ulong data_start = (uintptr_t)desc_p->buf_addr;
-	ulong data_end;
+	uintptr_t data_start = (uintptr_t)desc_p->buf_addr;
+	int length;
 
 	/* Invalidate entire buffer descriptor */
 	cache_inv_descriptor(desc_p);
@@ -576,30 +579,31 @@ static int sun8i_emac_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 	status = desc_p->status;
 
 	/* Check for DMA own bit */
-	if (!(status & EMAC_DESC_OWN_DMA)) {
-		length = (desc_p->status >> 16) & 0x3FFF;
+	if (status & EMAC_DESC_OWN_DMA)
+		return -EAGAIN;
 
-		if (length < 0x40) {
-			good_packet = 0;
-			debug("RX: Bad Packet (runt)\n");
-		}
+	length = (status >> 16) & 0x3fff;
 
-		data_end = data_start + length;
-		/* Invalidate received data */
-		invalidate_dcache_range(rounddown(data_start,
-						  ARCH_DMA_MINALIGN),
-					roundup(data_end,
-						ARCH_DMA_MINALIGN));
-		if (good_packet) {
-			if (length > CONFIG_ETH_RXSIZE) {
-				printf("Received packet is too big (len=%d)\n",
-				       length);
-				return -EMSGSIZE;
-			}
-			*packetp = (uchar *)(ulong)desc_p->buf_addr;
-			return length;
-		}
+	/* make sure we read from DRAM, not our cache */
+	invalidate_dcache_range(data_start,
+				data_start + roundup(length, ARCH_DMA_MINALIGN));
+
+	if (status & EMAC_DESC_RX_ERROR_MASK) {
+		debug("RX: packet error: 0x%x\n",
+		      status & EMAC_DESC_RX_ERROR_MASK);
+		return 0;
 	}
+	if (length < 0x40) {
+		debug("RX: Bad Packet (runt)\n");
+		return 0;
+	}
+
+	if (length > CONFIG_ETH_RXSIZE) {
+		debug("RX: Too large packet (%d bytes)\n", length);
+		return 0;
+	}
+
+	*packetp = (uchar *)(ulong)desc_p->buf_addr;
 
 	return length;
 }
-- 
2.17.5

[PATCH 13/15] net: sun8i-emac: Make internal PHY handling more robust

[Andre Przywara]

The current implementation of sun8i_get_ephy_nodes() makes quite some
assumptions, in general relying on DT path names is a bad idea.
I think the idea of the code was to determine if we are using the
internal PHY, for which there are simpler and more robust methods:

Rewrite (and rename) the existing function to simply lookup the DT node
that "phy-handle" points to, using the device's DT node.
Then check whether the parent of that PHY node is using an "H3 internal
MDIO" compatible string. If we ever get another internal MDIO bus
implementation, we will probably need code adjustments anyway, so this
is good enough for now.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 48 ++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 48ba244f21..dc716f94f3 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -809,47 +809,31 @@ static const struct eth_ops sun8i_emac_eth_ops = {
 	.stop                   = sun8i_emac_eth_stop,
 };
 
-static int sun8i_get_ephy_nodes(struct emac_eth_dev *priv)
+static int sun8i_handle_internal_phy(struct udevice *dev)
 {
-	int emac_node, ephy_node, ret, ephy_handle;
-
-	emac_node = fdt_path_offset(gd->fdt_blob,
-				    "/soc/ethernet at 1c30000");
-	if (emac_node < 0) {
-		debug("failed to get emac node\n");
-		return emac_node;
-	}
-	ephy_handle = fdtdec_lookup_phandle(gd->fdt_blob,
-					    emac_node, "phy-handle");
+	struct emac_eth_dev *priv = dev_get_priv(dev);
+	struct ofnode_phandle_args phandle;
+	int ret;
 
-	/* look for mdio-mux node for internal PHY node */
-	ephy_node = fdt_path_offset(gd->fdt_blob,
-				    "/soc/ethernet at 1c30000/mdio-mux/mdio at 1/ethernet-phy at 1");
-	if (ephy_node < 0) {
-		debug("failed to get mdio-mux with internal PHY\n");
-		return ephy_node;
-	}
+	ret = ofnode_parse_phandle_with_args(dev_ofnode(dev), "phy-handle",
+					     NULL, 0, 0, &phandle);
+	if (ret)
+		return ret;
 
-	/* This is not the phy we are looking for */
-	if (ephy_node != ephy_handle)
+	/* If the PHY node is not a child of the internal MDIO bus, we are
+	 * using some external PHY.
+	 */
+	if (!ofnode_device_is_compatible(ofnode_get_parent(phandle.node),
+					 "allwinner,sun8i-h3-mdio-internal"))
 		return 0;
 
-	ret = fdt_node_check_compatible(gd->fdt_blob, ephy_node,
-					"allwinner,sun8i-h3-mdio-internal");
-	if (ret < 0) {
-		debug("failed to find mdio-internal node\n");
-		return ret;
-	}
-
-	ret = clk_get_by_index_nodev(offset_to_ofnode(ephy_node), 0,
-				     &priv->ephy_clk);
+	ret = clk_get_by_index_nodev(phandle.node, 0, &priv->ephy_clk);
 	if (ret) {
 		dev_err(dev, "failed to get EPHY TX clock\n");
 		return ret;
 	}
 
-	ret = reset_get_by_index_nodev(offset_to_ofnode(ephy_node), 0,
-				       &priv->ephy_rst);
+	ret = reset_get_by_index_nodev(phandle.node, 0, &priv->ephy_rst);
 	if (ret) {
 		dev_err(dev, "failed to get EPHY TX reset\n");
 		return ret;
@@ -941,7 +925,7 @@ static int sun8i_emac_eth_ofdata_to_platdata(struct udevice *dev)
 	}
 
 	if (priv->variant == H3_EMAC) {
-		ret = sun8i_get_ephy_nodes(priv);
+		ret = sun8i_handle_internal_phy(dev);
 		if (ret)
 			return ret;
 	}
-- 
2.17.5

[PATCH 14/15] net: sun8i-emac: Lower MDIO frequency

[Andre Przywara]

When sending a command via the MDIO bus, the Designware MAC expects some
bits in the CMD register to describe the clock divider value between
the main clock and the MDIO clock.
So far we were omitting these bits, resulting in setting "00", which
means "/ 16", so ending up with an MDIO frequency of either 18.75 or
12.5 MHz.
All the internal PHYs in the H3/H5/H6 SoCs as well as the Gbit Realtek
PHYs seem to be fine with that - although it looks like to be severly
overclocked (the MDIO spec limits the frequency to 2.5 MHz).
However the external 100Mbit PHY on the Pine64 (non-plus) board is
not happy with that, Ethernet was actually never working there, as the
PHY didn't probe.

As we set the EMAC clock (via AHB2) to 300 MHz in ATF (on the 64-bit
SoCs), and use 200 MHz on the H3, we need the highest divider of 128
to let the MDIO clock end up below the required 2.5 MHz.

This enables Ethernet on the Pine64(non-plus).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index dc716f94f3..b2007b4c1d 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -41,6 +41,11 @@
 #define MDIO_CMD_MII_PHY_REG_ADDR_SHIFT	4
 #define MDIO_CMD_MII_PHY_ADDR_MASK	0x0001f000
 #define MDIO_CMD_MII_PHY_ADDR_SHIFT	12
+#define MDIO_CMD_MII_CLK_CSR_DIV_16	0x0
+#define MDIO_CMD_MII_CLK_CSR_DIV_32	0x1
+#define MDIO_CMD_MII_CLK_CSR_DIV_64	0x2
+#define MDIO_CMD_MII_CLK_CSR_DIV_128	0x3
+#define MDIO_CMD_MII_CLK_CSR_SHIFT	20
 
 #define CONFIG_TX_DESCR_NUM	32
 #define CONFIG_RX_DESCR_NUM	32
@@ -199,6 +204,12 @@ static int sun8i_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
 	mii_cmd |= (addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) &
 		MDIO_CMD_MII_PHY_ADDR_MASK;
 
+	/*
+	 * The EMAC clock is either 200 or 300 MHz, so we need a divider
+	 * of 128 to get the MDIO frequency below the required 2.5 MHz.
+	 */
+	mii_cmd |= MDIO_CMD_MII_CLK_CSR_DIV_128 << MDIO_CMD_MII_CLK_CSR_SHIFT;
+
 	mii_cmd |= MDIO_CMD_MII_BUSY;
 
 	writel(mii_cmd, priv->mac_reg + EMAC_MII_CMD);
@@ -224,6 +235,12 @@ static int sun8i_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
 	mii_cmd |= (addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) &
 		MDIO_CMD_MII_PHY_ADDR_MASK;
 
+	/*
+	 * The EMAC clock is either 200 or 300 MHz, so we need a divider
+	 * of 128 to get the MDIO frequency below the required 2.5 MHz.
+	 */
+	mii_cmd |= MDIO_CMD_MII_CLK_CSR_DIV_128 << MDIO_CMD_MII_CLK_CSR_SHIFT;
+
 	mii_cmd |= MDIO_CMD_MII_WRITE;
 	mii_cmd |= MDIO_CMD_MII_BUSY;
 
-- 
2.17.5

[PATCH 14/15] net: sun8i-emac: Lower MDIO frequency

[Jagan Teki]

On Mon, Jul 6, 2020 at 6:12 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> When sending a command via the MDIO bus, the Designware MAC expects some
> bits in the CMD register to describe the clock divider value between
> the main clock and the MDIO clock.
> So far we were omitting these bits, resulting in setting "00", which
> means "/ 16", so ending up with an MDIO frequency of either 18.75 or
> 12.5 MHz.
> All the internal PHYs in the H3/H5/H6 SoCs as well as the Gbit Realtek
> PHYs seem to be fine with that - although it looks like to be severly
> overclocked (the MDIO spec limits the frequency to 2.5 MHz).
> However the external 100Mbit PHY on the Pine64 (non-plus) board is
> not happy with that, Ethernet was actually never working there, as the
> PHY didn't probe.

How come the existing divider cannot work with 100Mbit external
PHY(assuming external regulator pin as properly enabled) since it
works with 1Gbit already?

Jagan.

[PATCH 14/15] net: sun8i-emac: Lower MDIO frequency

[André Przywara]

On 11/07/2020 10:27, Jagan Teki wrote:

Hi,

> On Mon, Jul 6, 2020 at 6:12 AM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> When sending a command via the MDIO bus, the Designware MAC expects some
>> bits in the CMD register to describe the clock divider value between
>> the main clock and the MDIO clock.
>> So far we were omitting these bits, resulting in setting "00", which
>> means "/ 16", so ending up with an MDIO frequency of either 18.75 or
>> 12.5 MHz.
>> All the internal PHYs in the H3/H5/H6 SoCs as well as the Gbit Realtek
>> PHYs seem to be fine with that - although it looks like to be severly
>> overclocked (the MDIO spec limits the frequency to 2.5 MHz).
>> However the external 100Mbit PHY on the Pine64 (non-plus) board is
>> not happy with that, Ethernet was actually never working there, as the
>> PHY didn't probe.
> 
> How come the existing divider cannot work with 100Mbit external
> PHY(assuming external regulator pin as properly enabled) since it
> works with 1Gbit already?

Because it's far too high to be MDIO spec compliant. My guess is that
some PHYs (for instance the RTL8211 used on most boards with GBit
Ethernet) can cope with that, but apparently that's too much for the
RTL8201 on the Pine64. I would say that most boards have either an
external GBit PHY or use the internal PHY for 100MBit, so there are not
many boards using the sun8i EMAC with an 8201 PHY, that's why nobody
complained so far.

At least this is my best guess, based on the observation that this patch
makes the difference between Ethernet working or not on the Pine64
(non-plus).

Cheers,
Andre.

[PATCH 15/15] sunxi: Pine-H64: Explicitly enable PHY regulator

[Andre Przywara]

According to the devicetree and the schematic, the 3.3V power rail for
the PHY is enabled by GPIO PC16. It's wired as active-high, with a
pull-up resistor, so actually works already when the GPIO is in
High-Z state.

However we should not take any chances and explicitly set the GPIO pin
to high, to avoid accidentally losing the PHY power.
The existing MACPWR Kconfig allows to do this easily.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 configs/pine_h64_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/pine_h64_defconfig b/configs/pine_h64_defconfig
index 87871fd19f..fb18bb7df7 100644
--- a/configs/pine_h64_defconfig
+++ b/configs/pine_h64_defconfig
@@ -11,5 +11,6 @@ CONFIG_SPL_SPI_SUNXI=y
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_DEFAULT_DEVICE_TREE="sun50i-h6-pine-h64"
 CONFIG_SUN8I_EMAC=y
+CONFIG_MACPWR="PC16"
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_OHCI_HCD=y
-- 
2.17.5

[PATCH 00/15] net: sun8i-emac fixes and cleanups

[Maxime Ripard]

On Mon, Jul 06, 2020 at 01:40:31AM +0100, Andre Przywara wrote:
> Hi,
> 
> while looking at several U-Boot network drivers in the past year, I
> typically compared them to the sun8i-emac driver, as a kind of personal
> reference. While doing so, I figured that there are quite some things
> broken in here, and other things are not so nice.
> This series attempts the fix those shortcomings.
> Fix-wise we get proper handling of PHY failures (try without a
> cable connected), support for external RMII PHYs (as seen on the
> Pine64-non-plus model), and more future-proof internal PHY handling.
> The rest of the patches are cleanups, which fix things that are wrong,
> but we get away with so far, for one or another reason.
> This also cleans up a good part of the cache maintenance. There is more
> to be done (and I have patches for that), but that requires to drop
> the overzealous alignment checks in cache_v7.c first, which is part of
> another, upcoming series.
> 
> A git repo with those patches can be found here:
> https://github.com/apritzel/u-boot/commits/sun8i-emac-cleanup
> 
> Please have a look and send comments!

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH 00/15] net: sun8i-emac fixes and cleanups

[Amit Tomer]

Hi,

On Mon, Jul 6, 2020 at 6:12 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> Hi,
>
> while looking at several U-Boot network drivers in the past year, I
> typically compared them to the sun8i-emac driver, as a kind of personal
> reference. While doing so, I figured that there are quite some things
> broken in here, and other things are not so nice.
> This series attempts the fix those shortcomings.
> Fix-wise we get proper handling of PHY failures (try without a
> cable connected), support for external RMII PHYs (as seen on the
> Pine64-non-plus model), and more future-proof internal PHY handling.
> The rest of the patches are cleanups, which fix things that are wrong,
> but we get away with so far, for one or another reason.
> This also cleans up a good part of the cache maintenance. There is more
> to be done (and I have patches for that), but that requires to drop
> the overzealous alignment checks in cache_v7.c first, which is part of
> another, upcoming series.
>
> A git repo with those patches can be found here:
> https://github.com/apritzel/u-boot/commits/sun8i-emac-cleanup

Tested this on Pine64+, and it worked without any issue.

Tested-by: Amit Singh Tomar <amittomer25@gmail.com>

Thanks
-Amit