[U-Boot] [U-BOOT] [RFC PATCH] EMAC driver for H3/A64

[LABBE Corentin <clabbe.montjoie@gmail.com>]

On Mon, Apr 25, 2016 at 12:34:06AM +0530, Amit Singh Tomar wrote:
> Hello
> 
> I wanted to contribute and in the process trying to add support for the driver for EMAC controller found on H3/A64 based SoC.
> 
> Sorry, I don't want to post it at this moment but I am kind of stuck from last few days.
> 
> What is working:
>                 - Able to establish the link between host and Organepi pc(H3).
>                 - Tx on host side getting incremented.
>                 - When ping from Orangepi PC to host machine , I see ONE
>                   packet is trasnmitted over wire but its bad packet with following output
>                   after running tcpdump on host
> 
> 20:41:49.148111 00:00:00:00:00:00 (oui Ethernet) > 00:00:00:00:00:00 (oui Ethernet) Null Information, send seq 0, rcv seq 0, Flags [Command], length 46
> 	0x0000:  0000 0000 0000 0000 0000 0000 0000 0000  ................
> 	0x0010:  0000 0000 0000 0000 0000 0000 0000 0000  ................
> 	0x0020:  0000 0000 0000 0000 0000 0000 0000 0000  ................
> 	0x0030:  0000 0000 0000 0000 0000 0000            ............
> 
> What needs to be done:
>                 - Why bad packet is transmitted over the wire from Orangepi pc to Host ?
> 		- Make it compatible with external PHY.
> 
> This Patch is based on LABBE Corentin's and Chen-Yu Tsai's work done for Linux Kernel(Thanks to both of them).
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>  arch/arm/dts/sun8i-h3-orangepi-pc.dts |   10 +
>  arch/arm/dts/sun8i-h3.dtsi            |    7 +
>  configs/orangepi_pc_defconfig         |    2 +
>  drivers/net/Makefile                  |    1 +
>  drivers/net/sun8i_emac.c              |  619 +++++++++++++++++++++++++++++++++
>  5 files changed, 639 insertions(+)
>  create mode 100644 drivers/net/sun8i_emac.c
> 

Hello

I have some comments below

> +
> +#define CONFIG_TX_DESCR_NUM     128
> +#define CONFIG_RX_DESCR_NUM     64
> +#define CONFIG_ETH_BUFSIZE      2048
I do not think you will doing Jumbo frame, so 2048 is too much
Anyway, I have got strange reaction from hw with this value (Even BSP use 2047)

> +#define TX_TOTAL_BUFSIZE        (CONFIG_ETH_BUFSIZE * CONFIG_TX_DESCR_NUM)
> +#define RX_TOTAL_BUFSIZE        (CONFIG_ETH_BUFSIZE * CONFIG_RX_DESCR_NUM)
> +
> +#define REG_DEFAULT_VALUE	0x58000
> +#define REG_DEFAULT_MASK	GENMASK(31, 15)
> +
> +#define REG_PHY_ADDR_SHIFT	20
> +#define REG_PHY_ADDR_MASK	GENMASK(4, 0)
> +#define REG_LED_POL		BIT(17)	/* 1: active low, 0: active high */
> +#define REG_SHUTDOWN		BIT(16)	/* 1: shutdown, 0: power up */
> +#define REG_PHY_SELECT		BIT(15) /* 1: internal PHY, 0: external PHY */
> +
> +
> +#define RX_BUF_SZ (2048*64)
Unused, and if used, use the proper define (CONFIG_RX_DESCR_NUM/CONFIG_ETH_BUFSIZE)

> +
> +#define CONFIG_MDIO_TIMEOUT     (3 * CONFIG_SYS_HZ)
> +
> +#define  AHB_GATE_OFFSET_GMAC 17
> +#define  AHB_GATE_OFFSET_EPHY 0
> +
> +#define SUN8I_GPD8_GMAC 4
> +
> +/* H3/A64 EMAC Register Base*/
> +#define SUN8I_EMAC_BASE 0x01c30000
> +
> +/* H3/A64 EMAC Register's offset */
> +#define	CTL0		0x00
> +#define CTL1		0x04
> +#define INT_STA		0x08
> +#define INT_EN		0x0c
> +#define TX_CTL0		0x10
> +#define TX_CTL1		0x14
> +#define	TX_FLOW_CTL	0x1c
> +#define	TX_DMA_DESC	0x20
> +#define RX_CTL0		0x24
> +#define RX_CTL1		0x28
> +#define RX_DMA_DESC	0x34
> +#define	MII_CMD		0x48
> +#define	MII_DATA	0x4c
> +#define	ADDR0_HIGH	0x50
> +#define	ADDR0_LOW	0x54
> +#define TX_DMA_STA	0xb0
> +#define	TX_CUR_DESC	0xb4
> +#define TX_CUR_BUF      0xb8
> +#define RX_DMA_STA	0xc0
> +#define RX_CUR_DESC	0xc4
> +#define TX_CUR_BUF	0xc8
> +
> +/* CCM Register base */
> +#define SUN8I_CCM_BASE 0x01c20000
> +
> +/* CCM Regsiter's offset */
typo
your patch have some typos, run aspell on it

> +#define	CLK_GATING_REG0		0x60
> +#define CLK_GATING_REG1		0x64
> +#define CLK_GATING_REG2		0x68
> +#define CLK_GATING_REG3		0x6c
> +#define CLK_GATING_REG4		0x70
> +
> +/* System Control Register base */
> +#define SUN8I_SCTL_BASE 0x01c00000
> +
> +/* System Control Register's offset */
> +#define VER_REG 0x24
> +#define EMAC_CLK_REG 0x30
> +
> +struct emac_dma_desc {
> +	u32 status;
> +	u32 st;
> +	void *buf_addr;
> +	struct emac_dma_desc *next;
> +} __aligned(4);
> +
> +struct emac_eth_dev {
> +	struct emac_dma_desc rx_chain[CONFIG_TX_DESCR_NUM];
> +	struct emac_dma_desc tx_chain[CONFIG_RX_DESCR_NUM];
> +	char rxbuffer[RX_TOTAL_BUFSIZE] __aligned(ARCH_DMA_MINALIGN);
> +	char txbuffer[TX_TOTAL_BUFSIZE] __aligned(ARCH_DMA_MINALIGN);
> +
> +	u32 interface;
> +	u32 link;
> +	u32 speed;
> +	u32 duplex;
> +	u32 phy_configured;
> +	u32 tx_currdescnum;
> +	u32 rx_currdescnum;
> +	u32 addr;
> +	u32 tx_slot;
> +	void *mac_reg;
> +	void *sysctl_reg;
> +	void *ccu_reg;
> +	struct phy_device *phydev;
> +	struct mii_dev *bus;
> +};
> +
> +static int sun8i_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
> +{
> +	struct emac_eth_dev *priv = bus->priv;
> +	ulong start;
> +	u32 miiaddr;
> +	int timeout = CONFIG_MDIO_TIMEOUT;
> +
> +	/*miiaddr = ((addr << MDIO_CMD_MII_PHY_ADDR_SHIFT) & MDIO_CMD_MII_PHY_ADDR_MASK ) |
> +	  ((reg << MDIO_CMD_MII_PHY_REG_ADDR_SHIFT) & MDIO_CMD_MII_PHY_REG_ADDR_MASK);
> +
> +	  writel(miiaddr | MDIO_CMD_MII_BUSY, &mac_p->miicmd);*/
> +	miiaddr &= ~MDIO_CMD_MII_WRITE;
> +	miiaddr &= ~MDIO_CMD_MII_PHY_REG_ADDR_MASK;
> +	miiaddr |= (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) &
> +		MDIO_CMD_MII_PHY_ADDR_MASK;
> +
> +	miiaddr |= MDIO_CMD_MII_BUSY;
> +
> +	writel(miiaddr, priv->mac_reg + MII_CMD);
> +
> +	start = get_timer(0);
> +	while (get_timer(start) < timeout) {
> +		if (!(readl(priv->mac_reg + MII_CMD) & MDIO_CMD_MII_BUSY))
> +			return readl(priv->mac_reg + MII_DATA);
> +		udelay(10);
> +	};
> +
> +	return -1;
> +}
> +
> +static int sun8i_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
> +		u16 val)
> +{
> +	struct emac_eth_dev *priv = bus->priv;
> +	ulong start;
> +	u32 miiaddr;
> +	int ret = -1, timeout = CONFIG_MDIO_TIMEOUT;
> +
> +	/*writel(val, &mac_p->miidata);
> +	  miiaddr = ((addr << MIIADDRSHIFT) & MII_ADDRMSK) |
> +	  ((reg << MIIREGSHIFT) & MII_REGMSK) | MII_WRITE;
> +
> +	  writel(miiaddr | MII_BUSY, &mac_p->miiaddr);*/
> +
> +	miiaddr &= ~MDIO_CMD_MII_PHY_REG_ADDR_MASK;
> +	miiaddr |= (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) &
> +		MDIO_CMD_MII_PHY_ADDR_MASK;
> +
> +	miiaddr |= MDIO_CMD_MII_WRITE;
> +	miiaddr |= MDIO_CMD_MII_BUSY;
> +
> +	writel(miiaddr, priv->mac_reg + MII_CMD);
> +	writel(val, priv->mac_reg + MII_DATA);
> +
> +	start = get_timer(0);
> +	while (get_timer(start) < timeout) {
> +		if (!(readl(priv->mac_reg + MII_CMD) & MDIO_CMD_MII_BUSY)) {
> +			ret = 0;
> +			break;
> +		}
> +		udelay(10);
> +	};
> +
> +	return ret;
> +}
> +
> +static int sun8i_phy_init(struct emac_eth_dev *priv, void *dev)
> +{
> +	u32 val;
> +	int mask = 0xffffffff;
> +
> +#ifdef CONFIG_PHY_ADDR
> +	mask =  1 << CONFIG_PHY_ADDR;
> +#endif
> +	struct phy_device *phydev;
> +
> +	/* H3 based SoC has an Internal PHY that needs
> +	   to be configured and powered up befoe use
> +	*/
> +
> +	val = readl(priv->sysctl_reg + EMAC_CLK_REG);
> +	val = (val & ~REG_DEFAULT_MASK) | REG_DEFAULT_VALUE;
> +
> +	val |= CONFIG_PHY_ADDR << REG_PHY_ADDR_SHIFT;
> +	val &= ~REG_SHUTDOWN;
> +	val |= REG_PHY_SELECT;
> +
> +	setbits_le32((priv->sysctl_reg + EMAC_CLK_REG), (1 << 13));
> +	writel(val, (priv->sysctl_reg + EMAC_CLK_REG));
> +
> +	phydev = phy_find_by_mask(priv->bus, mask,
> +			PHY_INTERFACE_MODE_MII);
> +	if (!phydev)
> +		return -ENODEV;
> +
> +	phy_connect_dev(phydev, dev);
> +
> +	priv->phydev = phydev;
> +	phy_config(phydev);
> +
> +	return 0;
> +}
> +
> +static void sun8i_adjust_link(struct emac_eth_dev *priv,
> +		struct phy_device *phydev)
> +{
> +	u32 v;
> +	v = readl(priv->mac_reg + CTL0);
> +
> +	if (phydev->duplex)
> +		v |= BIT(0);
> +	else
> +		v &= ~BIT(0);
> +
> +	v &= ~0x0C;
> +
> +	switch (phydev->speed) {
> +	case 1000:
> +		break;
> +	case 100:
> +		v |= BIT(2);
> +		v |= BIT(3);
> +		break;
> +	case 10:
> +		v |= BIT(3);
> +		break;
> +	}
> +	writel(v, priv->mac_reg + CTL0);
> +}
> +
> +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;
> +
> +	flush_dcache_range((unsigned int)rxbuffs, (unsigned int)rxbuffs +
> +			RX_TOTAL_BUFSIZE);
> +
> +	for (idx = 0; idx < CONFIG_RX_DESCR_NUM; idx++) {
> +		desc_p = &desc_table_p[idx];
> +		desc_p->buf_addr = &rxbuffs[idx * CONFIG_ETH_BUFSIZE];
> +		desc_p->next = &desc_table_p[idx + 1];
> +	}
> +
> +	/* Correcting the last pointer of the chain */
> +	desc_p->next = &desc_table_p[0];
> +	flush_dcache_range((unsigned int)priv->rx_chain,
> +			    (unsigned int)priv->rx_chain +
> +			sizeof(priv->rx_chain));
> +
> +	writel((ulong)&desc_table_p[0], (priv->mac_reg + RX_DMA_DESC));
> +	priv->rx_currdescnum = 0;
> +}
> +
> +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;
> +
> +	for (idx = 0; idx < CONFIG_TX_DESCR_NUM; idx++) {
> +		desc_p = &desc_table_p[idx];
> +		desc_p->buf_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE];
> +		desc_p->next = &desc_table_p[idx + 1];
> +		desc_p->status = 0;
> +		desc_p->st = 0;
> +	}
> +
> +	/* Correcting the last pointer of the chain */
> +	desc_p->next = &desc_table_p[0];
> +	flush_dcache_range((unsigned int)priv->tx_chain,
> +			   (unsigned int)priv->tx_chain +
> +			sizeof(priv->tx_chain));
> +
> +	writel((ulong)&desc_table_p[0], priv->mac_reg + TX_DMA_DESC);
> +	priv->tx_currdescnum = 0;
> +}
> +
> +static int _sun8i_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr)
> +{
> +	u32 reg, v;
> +	priv->tx_slot = 0;
> +
> +	reg = readl((priv->mac_reg + CTL1));
> +
> +	if (!(reg & 0x1)) {
> +		/* Soft reset MAC */
> +		setbits_le32((priv->mac_reg + CTL1), 0x1 << 0);
> +		do {
> +			reg = readl(priv->mac_reg + CTL1);
> +		} while ((reg & 0x01) != 0);
> +	}
You need to setup a timeout

> +
> +	/* Rewrite mac addres after reset */
> +	writel((enetaddr[3] << 24 | enetaddr[2] << 16 | enetaddr[1] << 8
> +		| enetaddr[0]), priv->mac_reg + ADDR0_LOW);
> +	writel((enetaddr[5] << 8 | enetaddr[4]), priv->mac_reg + ADDR0_HIGH);
Why not using sun8i_write_hwaddr ?

> +
> +	v = readl(priv->mac_reg + TX_CTL1);
> +	/* TX_MD Transmission starts after a full frame located in TX DMA FIFO*/
> +	v |= BIT(1);
> +	writel(v, priv->mac_reg + TX_CTL1);
> +
> +	v = readl(priv->mac_reg + RX_CTL1);
> +	/* RX_MD 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 + RX_CTL1);
> +
> +	/* DMA */
> +	writel(8 << 24 , priv->mac_reg + CTL1);
> +
> +	/* Enable recieve and trasnmit interupt */
Double typo and I do not see any interrupt handling in this file

> +	writel(RX_INT | TX_INT | TX_UNF_INT, (priv->mac_reg + INT_EN));
> +
> +	/* Intialize rx/tx descriptors */
> +	rx_descs_init(priv);
> +	tx_descs_init(priv);
> +
> +	/*Start up the PHY */
> +	if (phy_startup(priv->phydev)) {
> +		printf("Could not initialize PHY %s\n",
> +		       priv->phydev->dev->name);
> +		return -1;
> +	}
> +
> +	sun8i_adjust_link(priv, priv->phydev);
> +
> +	/* start RX DMA */
> +	v = readl(priv->mac_reg + RX_CTL1);
> +	v |= BIT(30);
> +	writel(v, priv->mac_reg + RX_CTL1);
> +
> +	v = readl(priv->mac_reg + TX_CTL1);
> +	v |= BIT(30);
> +	writel(v, priv->mac_reg + TX_CTL1);
> +
> +	/* Enable RX/TX */
> +	setbits_le32(priv->mac_reg + RX_CTL0, 0x1 << 31);
> +	setbits_le32(priv->mac_reg + TX_CTL0, 0x1 << 31);
> +
> +	writel(0x3FFF, priv->mac_reg + INT_STA);
> +
> +	return 0;
> +}
> +
> +static void sun8i_emac_board_setup(struct emac_eth_dev *priv)
> +{
> +	priv->ccu_reg = (void *)SUN8I_CCU_BASE;
> +	priv->sysctl_reg = (void *)SUN8I_SCTL_BASE;
> +	int pin;
> +
> +	/* Set clock gating for emac */
> +	setbits_le32((priv->ccu_reg + CLK_GATING_REG0),
> +		      0x1 << AHB_GATE_OFFSET_GMAC);
> +
> +	/* Set clock gating for ephy */
> +	setbits_le32((priv->ccu_reg + CLK_GATING_REG4),
> +		     0x1 << AHB_GATE_OFFSET_EPHY);
> +
> +	/* Set Tx clock source as MII with rate 25MZ */
> +	clrbits_le32((priv->sysctl_reg + EMAC_CLK_REG),
> +		     SCTL_EMAC_TX_CLK_SRC_MII |
> +		     SCTL_EMAC_EPIT_MII | SCTL_EMAC_CLK_SEL);
> +
> +	/* Set EMAC clock */
> +	setbits_le32((priv->ccu_reg + AHB2_CFG_REG), (0x1 << 1) | (0x1 << 0));
> +
> +	/* Deassert EPHY */
> +	setbits_le32((priv->ccu_reg + BUS_SOFT_RST_REG2), EPHY_RST);
> +
> +	/* Deassert EMAC */
> +	setbits_le32((priv->ccu_reg + BUS_SOFT_RST_REG0), EMAC_RST);
> +
> +	/* Pin mux setting for emac */
> +	for (pin = SUNXI_GPD(8); pin <= SUNXI_GPD(22); pin++)
> +		sunxi_gpio_set_cfgpin(pin, SUN8I_GPD8_GMAC);
> +}
> +
> +static int _sun8i_eth_recv(struct emac_eth_dev *priv, uchar **packetp)
> +{
> +	u32 status, desc_num = priv->rx_currdescnum;
> +	struct emac_dma_desc *desc_p = &priv->rx_chain[desc_num];
> +	int length = -EAGAIN;
> +	uint32_t desc_start = (uint32_t)desc_p;
> +	uint32_t desc_end = desc_start +
> +		roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN);
> +
> +	uint32_t data_start = (uint32_t)desc_p->buf_addr;
> +	uint32_t data_end;
> +
> +	/* Invalidate entire buffer descriptor */
> +	invalidate_dcache_range(desc_start, desc_end);
> +
> +	status = desc_p->status;
> +
> +	length = (desc_p->status >> 16) & 0x3FFF;
> +
> +	data_end = data_start + roundup(length, ARCH_DMA_MINALIGN);
> +	invalidate_dcache_range(data_start, data_end);
> +	*packetp = desc_p->buf_addr;
> +
> +	return length;
> +}

I do not understand why you do not check for BIT(31)(DMA own)

> +
> +static int _sun8i_emac_eth_send(struct emac_eth_dev *priv, void *packet,
> +		int len)
> +{
> +	u32 v, status, desc_num = priv->tx_currdescnum;
> +	struct emac_dma_desc *desc_p = &priv->tx_chain[desc_num];
> +
> +	int length = -EAGAIN;
not used

> +	uint32_t desc_start = (uint32_t)desc_p;
> +	uint32_t desc_end = desc_start +
> +		roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN);
> +
> +	uint32_t data_start = (uint32_t)desc_p->buf_addr;
> +	uint32_t data_end;
> +
> +	/* Invalidate entire buffer descriptor */
> +	invalidate_dcache_range(desc_start, desc_end);
> +
> +	desc_p->st = len;
> +	desc_p->st |= 1<<24;
> +	desc_p->st |= 1<<29;
Use BIT()

> +
> +	memcpy((unsigned *)desc_p->buf_addr, packet, len);
> +
> +	flush_dcache_range(data_start, data_end);
> +
> +	desc_p->st |= BIT(31);
> +	desc_p->st |= BIT(30);
> +
> +	desc_p->st |= 1<<29;
Why setting this twice ?

> +	desc_p->status = 1<<31;
Use BIT() and put a comment of meaning of thoses bits.

For your send error, try using the same order than my driver (bit 29)
Problem with sending I got was about write order and cache.

Regards