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@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
[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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
