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

Andre Przywara

6 Jul 2020 6 Jul '20

2:40 a.m.

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

Show replies by date

Andre Przywara

6 Jul 6 Jul

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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

Andre Przywara

2:40 a.m.

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

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@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@1c30000/mdio-mux/mdio@1/ethernet-phy@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

Andre Przywara

2:40 a.m.

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

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;

-- 2.17.5

Jagan Teki

11 Jul 11 Jul

11:27 a.m.

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

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.

André Przywara

12 Jul 12 Jul

1:53 a.m.

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

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.

Andre Przywara

6 Jul 6 Jul

2:40 a.m.

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

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

Maxime Ripard

2:10 p.m.

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

Amit Tomer

7 Jul 7 Jul

3:59 p.m.

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

1764

Age (days ago)

1769

Last active (days ago)

List overview

Download

19 comments

5 participants

tags (0)

participants (5)

Amit Tomer
Andre Przywara
André Przywara
Jagan Teki
Maxime Ripard