Re: [PATCH 1/5] net: Add davicom wemac ethernet driver found on Allwinner A10 SoC's

[Florian Fainelli <florian@openwrt.org>]

Hello Maxime, Stefan,

Please find below some comments regarding your PHY implementation in the driver 
as well as the transmit and transmit completion routines.

Le vendredi 15 mars 2013 21:50:00, Maxime Ripard a écrit :
> From: Stefan Roese <sr@denx.de>
> 
> The Allwinner A10 has an ethernet controller that is advertised as
> coming from Davicom.
> 
> The exact feature set of this controller is unknown, since there is no
> public documentation for this IP, and this driver is mostly the one
> published by Allwinner that has been heavily cleaned up.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

[snip]

> +
> +wemac: ethernet@01c0b000 {
> +       compatible = "davicom,wemac";
> +       reg = <0x01c0b000 0x1000>;
> +       interrupts = <55>;
> +       allwinner,power-gpios = <&pio 7 19 0>;
> +};

You do not define any handle for the PHY chip, but I think you should make this 
configurable and use the standard Device Tree helpers for finding and connecting 
a PHY driver to your EMAC adapter driver.
[snip]

> +#define WEMAC_PHY		0x100	/* PHY address 0x01 */

This should be made configurable if possible.

[snip]

> + */
> +
> +struct wemac_board_info {
> +	struct clk		*clk;
> +	struct device		*dev;
> +	spinlock_t		lock;
> +	void __iomem		*membase;
> +	struct mii_if_info	mii;
> +	u32			msg_enable;
> +	struct net_device	*ndev;
> +	struct mutex		phy_lock;
> +	struct delayed_work	phy_poll;

Consider using phylib which should allow you to remove struct mii_if_info and 
the mutex and poll delayed workqueue.

> +	unsigned		power_gpio;

you could make this an int, so that you use gpio_is_valid() on this one.

> +	struct sk_buff		*skb_last;
> +	u16			tx_fifo_stat;
> +};
> +
> +static inline struct wemac_board_info *to_wemac_board(struct net_device
> *dev) +{
> +	return netdev_priv(dev);
> +}
> +
> +static int wemac_phy_read(struct net_device *dev, int phyaddr, int reg)
> +{
> +	struct wemac_board_info *db = netdev_priv(dev);
> +	unsigned long flags;
> +	int ret;
> +
> +	mutex_lock(&db->phy_lock);
> +
> +	spin_lock_irqsave(&db->lock, flags);
> +	/* issue the phy address and reg */
> +	writel(phyaddr | reg, db->membase + EMAC_MAC_MADR_REG);
> +	/* pull up the phy io line */
> +	writel(0x1, db->membase + EMAC_MAC_MCMD_REG);
> +	spin_unlock_irqrestore(&db->lock, flags);
> +
> +	/* Wait read complete */
> +	mdelay(1);
> +
> +	/* push down the phy io line and read data */
> +	spin_lock_irqsave(&db->lock, flags);
> +	/* push down the phy io line */
> +	writel(0x0, db->membase + EMAC_MAC_MCMD_REG);
> +	/* and read data */
> +	ret = readl(db->membase + EMAC_MAC_MRDD_REG);
> +	spin_unlock_irqrestore(&db->lock, flags);
> +
> +	mutex_unlock(&db->phy_lock);

This sounds pretty complicated as a locking scheme, using phylib should make 
this simpler anyway.

> +
> +	return ret;
> +}
> +
> +/* Write a word to phyxcer */
> +static void wemac_phy_write(struct net_device *dev,
> +			    int phyaddr, int reg, int value)
> +{
> +	struct wemac_board_info *db = netdev_priv(dev);
> +	unsigned long flags;
> +
> +	mutex_lock(&db->phy_lock);
> +
> +	spin_lock_irqsave(&db->lock, flags);
> +	/* issue the phy address and reg */
> +	writel(phyaddr | reg, db->membase + EMAC_MAC_MADR_REG);
> +	/* pull up the phy io line */
> +	writel(0x1, db->membase + EMAC_MAC_MCMD_REG);
> +	spin_unlock_irqrestore(&db->lock, flags);
> +
> +	/* Wait write complete */
> +	mdelay(1);
> +
> +	spin_lock_irqsave(&db->lock, flags);
> +	/* push down the phy io line */
> +	writel(0x0, db->membase + EMAC_MAC_MCMD_REG);
> +	/* and write data */
> +	writel(value, db->membase + EMAC_MAC_MWTD_REG);
> +	spin_unlock_irqrestore(&db->lock, flags);
> +
> +	mutex_unlock(&db->phy_lock);

Ditto

> +}
> +
> +static int emacrx_completed_flag = 1;

This should be moved to your interface specific private structure, as it won't 
work when there are two instances of the same driver.

[snip]

> +
> +static void wemac_dumpblk_32bit(void __iomem *reg, int count)
> +{
> +	int i;
> +	int tmp;
> +
> +	for (i = 0; i < (round_up(count, 4) / 4); i++)
> +		tmp = readl(reg);
> +}

This could probably be removed, tmp is a write only location, did you remove 
the variable printing that came along this?

> +
> +static void wemac_schedule_poll(struct wemac_board_info *db)
> +{
> +	schedule_delayed_work(&db->phy_poll, HZ * 2);
> +}

phylib takes care of the polling for you.

> +
> +static int wemac_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
> +{
> +	struct wemac_board_info *dm = to_wemac_board(dev);
> +
> +	if (!netif_running(dev))
> +		return -EINVAL;
> +
> +	return generic_mii_ioctl(&dm->mii, if_mii(req), cmd, NULL);
> +}
> +
> +/* ethtool ops */
> +static void wemac_get_drvinfo(struct net_device *dev,
> +			      struct ethtool_drvinfo *info)
> +{
> +	strcpy(info->driver, DRV_NAME);
> +	strcpy(info->version, DRV_VERSION);

You should use strlcpy() here to ensure null-termination, and you should also 
fill in the bus_info member of struct ethtool_drvinfo.

> +}
> +

[snip]

> +unsigned int phy_link_check(struct net_device *dev)
> +{
> +	unsigned int reg_val;
> +
> +	reg_val = wemac_phy_read(dev, WEMAC_PHY, 1);
> +
> +	if (reg_val & 0x4) {
> +		netdev_info(dev, "EMAC PHY Linked...\n");
> +		return 1;
> +	} else {
> +		netdev_info(dev, "EMAC PHY Link waiting......\n");
> +		return 0;
> +	}
> +}

Please remove this and migrate to phylib.

> +
> +unsigned int emac_setup(struct net_device *ndev)
> +{
> +	unsigned int reg_val;
> +	unsigned int phy_val;
> +	unsigned int duplex_flag;
> +	struct wemac_board_info *db = netdev_priv(ndev);
> +
> +	/* set up TX */
> +	reg_val = readl(db->membase + EMAC_TX_MODE_REG);
> +
> +	writel(reg_val | EMAC_TX_MODE_ABORTED_FRAME_EN,
> +		db->membase + EMAC_TX_MODE_REG);
> +
> +	/* set up RX */
> +	reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> +
> +	writel(reg_val | EMAC_RX_CTL_PASS_LEN_OOR_EN |
> +		EMAC_RX_CTL_ACCEPT_UNICAST_EN | EMAC_RX_CTL_DA_FILTER_EN |
> +		EMAC_RX_CTL_ACCEPT_MULTICAST_EN |
> +		EMAC_RX_CTL_ACCEPT_BROADCAST_EN,
> +		db->membase + EMAC_RX_CTL_REG);
> +
> +	/* set MAC */
> +	/* set MAC CTL0 */
> +	reg_val = readl(db->membase + EMAC_MAC_CTL0_REG);
> +	writel(reg_val | EMAC_MAC_CTL0_RX_FLOW_CTL_EN |
> +		EMAC_MAC_CTL0_TX_FLOW_CTL_EN,
> +		db->membase + EMAC_MAC_CTL0_REG);
> +
> +	/* set MAC CTL1 */
> +	reg_val = readl(db->membase + EMAC_MAC_CTL1_REG);
> +	phy_val = wemac_phy_read(ndev, WEMAC_PHY, 0);
> +	dev_dbg(db->dev, "PHY SETUP, reg 0 value: %x\n", phy_val);
> +	duplex_flag = !!(phy_val & EMAC_PHY_DUPLEX);
> +
> +	if (duplex_flag)
> +		reg_val |= EMAC_MAC_CTL1_DUPLEX_EN;
> +
> +	reg_val |= EMAC_MAC_CTL1_LEN_CHECK_EN;
> +	reg_val |= EMAC_MAC_CTL1_CRC_EN;
> +	reg_val |= EMAC_MAC_CTL1_PAD_EN;
> +	writel(reg_val, db->membase + EMAC_MAC_CTL1_REG);
> +
> +	/* set up IPGT */
> +	writel(EMAC_MAC_IPGT_FULL_DUPLEX, db->membase + EMAC_MAC_IPGT_REG);
> +
> +	/* set up IPGR */
> +	writel((EMAC_MAC_IPGR_IPG1 << 8) | EMAC_MAC_IPGR_IPG2,
> +		db->membase + EMAC_MAC_IPGR_REG);
> +
> +	/* set up Collison window */
> +	writel((EMAC_MAC_CLRT_COLLISION_WINDOW << 8) | EMAC_MAC_CLRT_RM,
> +		db->membase + EMAC_MAC_CLRT_REG);
> +
> +	/* set up Max Frame Length */
> +	writel(WEMAC_MAX_FRAME_LEN,
> +		db->membase + EMAC_MAC_MAXF_REG);
> +
> +	return 0;
> +}
> +
> +unsigned int wemac_powerup(struct net_device *ndev)
> +{
> +	struct wemac_board_info *db = netdev_priv(ndev);
> +	unsigned int reg_val;
> +
> +	/* initial EMAC */
> +	/* flush RX FIFO */
> +	reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> +	reg_val |= 0x8;
> +	writel(reg_val, db->membase + EMAC_RX_CTL_REG);
> +	udelay(1);
> +
> +	/* initial MAC */
> +	/* soft reset MAC */
> +	reg_val = readl(db->membase + EMAC_MAC_CTL0_REG);
> +	reg_val &= ~EMAC_MAC_CTL0_SOFT_RESET;
> +	writel(reg_val, db->membase + EMAC_MAC_CTL0_REG);
> +
> +	/* set MII clock */
> +	reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
> +	reg_val &= (~(0xf << 2));
> +	reg_val |= (0xD << 2);
> +	writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
> +
> +	/* clear RX counter */
> +	writel(0x0, db->membase + EMAC_RX_FBC_REG);
> +
> +	/* disable all interrupt and clear interrupt status */
> +	writel(0, db->membase + EMAC_INT_CTL_REG);
> +	reg_val = readl(db->membase + EMAC_INT_STA_REG);
> +	writel(reg_val, db->membase + EMAC_INT_STA_REG);
> +
> +	udelay(1);
> +
> +	/* set up EMAC */
> +	emac_setup(ndev);
> +
> +	/* set mac_address to chip */
> +	writel(ndev->dev_addr[0] << 16 | ndev->dev_addr[1] << 8 | ndev->
> +	       dev_addr[2], db->membase + EMAC_MAC_A1_REG);
> +	writel(ndev->dev_addr[3] << 16 | ndev->dev_addr[4] << 8 | ndev->
> +	       dev_addr[5], db->membase + EMAC_MAC_A0_REG);
> +
> +	mdelay(1);
> +
> +	return 1;
> +}
> +
> +static void wemac_poll_work(struct work_struct *w)
> +{
> +	struct delayed_work *dw = container_of(w, struct delayed_work, work);
> +	struct wemac_board_info *db = container_of(dw,
> +						struct wemac_board_info,
> +						phy_poll);
> +	struct net_device *ndev = db->ndev;
> +
> +	mii_check_media(&db->mii, netif_msg_link(db), 0);
> +
> +	if (netif_running(ndev))
> +		wemac_schedule_poll(db);
> +}
> +
> +static int wemac_set_mac_address(struct net_device *dev, void *p)
> +{
> +	struct sockaddr *addr = p;
> +	struct wemac_board_info *db = netdev_priv(dev);
> +
> +	if (netif_running(dev))
> +		return -EBUSY;
> +
> +	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
> +
> +	writel(dev->dev_addr[0] << 16 | dev->dev_addr[1] << 8 | dev->
> +	       dev_addr[2], db->membase + EMAC_MAC_A1_REG);
> +	writel(dev->dev_addr[3] << 16 | dev->dev_addr[4] << 8 | dev->
> +	       dev_addr[5], db->membase + EMAC_MAC_A0_REG);
> +
> +	return 0;
> +}
> +
> +/* Initialize wemac board */
> +static void wemac_init_wemac(struct net_device *dev)
> +{
> +	struct wemac_board_info *db = netdev_priv(dev);
> +	unsigned int phy_reg;
> +	unsigned int reg_val;
> +
> +	if (gpio_is_valid(db->power_gpio))
> +		gpio_set_value(db->power_gpio, 1);
> +
> +	/* PHY POWER UP */
> +	phy_reg = wemac_phy_read(dev, WEMAC_PHY, 0);
> +	wemac_phy_write(dev, WEMAC_PHY, 0, phy_reg & (~(1 << 11)));
> +	mdelay(1);
> +
> +	phy_reg = wemac_phy_read(dev, WEMAC_PHY, 0);
> +
> +	/* set EMAC SPEED, depend on PHY  */
> +	reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
> +	reg_val &= (~(0x1 << 8));
> +	reg_val |= (((phy_reg & (1 << 13)) >> 13) << 8);
> +	writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
> +
> +	/* set duplex depend on phy */
> +	reg_val = readl(db->membase + EMAC_MAC_CTL1_REG);
> +	reg_val &= (~(0x1 << 0));
> +	reg_val |= (((phy_reg & (1 << 8)) >> 8) << 0);
> +	writel(reg_val, db->membase + EMAC_MAC_CTL1_REG);
> +
> +	/* enable RX/TX */
> +	reg_val = readl(db->membase + EMAC_CTL_REG);
> +	writel(reg_val | EMAC_CTL_RESET | EMAC_CTL_TX_EN | EMAC_CTL_RX_EN,
> +		db->membase + EMAC_CTL_REG);
> +
> +	/* enable RX/TX0/RX Hlevel interrup */
> +	reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> +	reg_val |= (0xf << 0) | (0x01 << 8);
> +	writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> +
> +	/* Init Driver variable */
> +	db->tx_fifo_stat = 0;
> +	dev->trans_start = 0;

I do not think this is required you are supposed to update this field only in 
the receive path.

> +}
> +
> +/* Our watchdog timed out. Called by the networking layer */
> +static void wemac_timeout(struct net_device *dev)
> +{
> +	struct wemac_board_info *db = netdev_priv(dev);
> +	unsigned long flags;
> +
> +	if (netif_msg_timer(db))
> +		dev_err(db->dev, "tx time out.\n");
> +
> +	/* Save previous register address */
> +	spin_lock_irqsave(&db->lock, flags);
> +
> +	netif_stop_queue(dev);
> +	wemac_reset(db);
> +	wemac_init_wemac(dev);
> +	/* We can accept TX packets again */
> +	dev->trans_start = jiffies;
> +	netif_wake_queue(dev);
> +
> +	/* Restore previous register address */
> +	spin_unlock_irqrestore(&db->lock, flags);
> +}
> +
> +/* Hardware start transmission.
> + * Send a packet to media from the upper layer.
> + */
> +static int wemac_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct wemac_board_info *db = netdev_priv(dev);
> +	unsigned long channel;
> +	unsigned long flags;
> +
> +	channel = db->tx_fifo_stat & 3;
> +	if (channel == 3)
> +		return 1;

start_xmit expects standardized return values such as NETDEV_TX_BUSY and 
NETDEV_TX_COMPLETE, please use them.

> +
> +	channel = (channel == 1 ? 1 : 0);
> +
> +	spin_lock_irqsave(&db->lock, flags);
> +
> +	writel(channel, db->membase + EMAC_TX_INS_REG);
> +
> +	wemac_outblk_32bit(db->membase + EMAC_TX_IO_DATA_REG,
> +			skb->data, skb->len);
> +	dev->stats.tx_bytes += skb->len;
> +
> +	db->tx_fifo_stat |= 1 << channel;
> +	/* TX control: First packet immediately send, second packet queue */
> +	if (channel == 0) {
> +		/* set TX len */
> +		writel(skb->len, db->membase + EMAC_TX_PL0_REG);
> +		/* start translate from fifo to phy */
> +		writel(readl(db->membase + EMAC_TX_CTL0_REG) | 1,
> +		       db->membase + EMAC_TX_CTL0_REG);

Do not you need some write barrier here to ensure your descriptor address and 
control are properly written before the EMAC will see these?

> +
> +		/* save the time stamp */
> +		dev->trans_start = jiffies;
> +	} else if (channel == 1) {
> +		/* set TX len */
> +		writel(skb->len, db->membase + EMAC_TX_PL1_REG);
> +		/* start translate from fifo to phy */
> +		writel(readl(db->membase + EMAC_TX_CTL1_REG) | 1,
> +		       db->membase + EMAC_TX_CTL1_REG);
> +
> +		/* save the time stamp */
> +		dev->trans_start = jiffies;
> +	}
> +
> +	if ((db->tx_fifo_stat & 3) == 3) {
> +		/* Second packet */
> +		netif_stop_queue(dev);

Why is that required? Does that mean that your EMAC can only transmit one 
packet at a time?

> +	}
> +
> +	spin_unlock_irqrestore(&db->lock, flags);
> +
> +	/* free this SKB */
> +	dev_kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +/* WEMAC interrupt handler
> + * receive the packet to upper layer, free the transmitted packet
> + */
> +static void wemac_tx_done(struct net_device *dev, struct wemac_board_info
> *db, +			  unsigned int tx_status)
> +{
> +	/* One packet sent complete */
> +	db->tx_fifo_stat &= ~(tx_status & 3);
> +	if (3 == (tx_status & 3))
> +		dev->stats.tx_packets += 2;
> +	else
> +		dev->stats.tx_packets++;
> +
> +	if (netif_msg_tx_done(db))
> +		dev_dbg(db->dev, "tx done, NSR %02x\n", tx_status);
> +
> +	netif_wake_queue(dev);

Why is that also required? According to your start_xmit function you should do 
this only when you have transmitted 2 packets no?

[snip]

> +
> +		reg_val = readl(db->membase + EMAC_RX_IO_DATA_REG);
> +		if (netif_msg_rx_status(db))
> +			dev_dbg(db->dev, "receive header: %x\n", reg_val);
> +		if (reg_val != 0x0143414d) {

Where is that magic value coming from?
-- 
Florian