[EMAIL PROTECTED] wrote:
+/* pointers to maintain transmit list */ +static struct net_dma_desc_tx *tx_list_head; +static struct net_dma_desc_tx *tx_list_tail; +static struct net_dma_desc_rx *rx_list_head; +static struct net_dma_desc_rx *rx_list_tail; +static struct net_dma_desc_rx *current_rx_ptr; +static struct net_dma_desc_tx *current_tx_ptr; +static struct net_dma_desc_tx *tx_desc; +static struct net_dma_desc_rx *rx_desc;
these should not be global variables. At a minimum, they should (a) be in a per-interface (or per-device) private structure or (b) encapsulated in a structure, then exported as a single global variable.
+static int desc_list_init(void) +{ + struct net_dma_desc_tx *tmp_desc_tx; + struct net_dma_desc_rx *tmp_desc_rx; + int i; + struct sk_buff *new_skb; +#if !defined(CONFIG_BFIN_MAC_USE_L1) + dma_addr_t dma_handle; +#endif + + tx_desc = + bfin_mac_alloc(&dma_handle, + sizeof(struct net_dma_desc_tx) * + CONFIG_BFIN_TX_DESC_NUM); + if (tx_desc == NULL) + goto init_error; + + rx_desc = + bfin_mac_alloc(&dma_handle, + sizeof(struct net_dma_desc_rx) * + CONFIG_BFIN_RX_DESC_NUM); + if (rx_desc == NULL) + goto init_error; + + /* init tx_list */ + for (i = 0; i < CONFIG_BFIN_TX_DESC_NUM; i++) { + + tmp_desc_tx = tx_desc + i; + + if (i == 0) { + tx_list_head = tmp_desc_tx; + tx_list_tail = tmp_desc_tx; + } + + tmp_desc_tx->desc_a.start_addr = + (unsigned long)tmp_desc_tx->packet; + tmp_desc_tx->desc_a.x_count = 0; + tmp_desc_tx->desc_a.config.b_DMA_EN = 0; /* disabled */ + tmp_desc_tx->desc_a.config.b_WNR = 0; /* read from memory */ + tmp_desc_tx->desc_a.config.b_WDSIZE = 2; /* wordsize is 32 bits */ + tmp_desc_tx->desc_a.config.b_NDSIZE = 6; /* 6 half words is desc size. */ + tmp_desc_tx->desc_a.config.b_FLOW = 7; /* large desc flow */ + tmp_desc_tx->desc_a.next_dma_desc = &(tmp_desc_tx->desc_b); + + tmp_desc_tx->desc_b.start_addr = + (unsigned long)(&(tmp_desc_tx->status)); + tmp_desc_tx->desc_b.x_count = 0; + tmp_desc_tx->desc_b.config.b_DMA_EN = 1; /* enabled */ + tmp_desc_tx->desc_b.config.b_WNR = 1; /* write to memory */ + tmp_desc_tx->desc_b.config.b_WDSIZE = 2; /* wordsize is 32 bits */ + tmp_desc_tx->desc_b.config.b_DI_EN = 0; /* disable interrupt */ + tmp_desc_tx->desc_b.config.b_NDSIZE = 6; + tmp_desc_tx->desc_b.config.b_FLOW = 7; /* stop mode */ + tmp_desc_tx->skb = NULL; + tx_list_tail->desc_b.next_dma_desc = &(tmp_desc_tx->desc_a); + tx_list_tail->next = tmp_desc_tx;
should be using dma mapping
+ tx_list_tail = tmp_desc_tx; + } + tx_list_tail->next = tx_list_head; /* tx_list is a circle */ + tx_list_tail->desc_b.next_dma_desc = &(tx_list_head->desc_a); + current_tx_ptr = tx_list_head;
why do you use a list rather than an array like other drivers?
+ /* init rx_list */ + for (i = 0; i < CONFIG_BFIN_RX_DESC_NUM; i++) { + + tmp_desc_rx = rx_desc + i; + + if (i == 0) { + rx_list_head = tmp_desc_rx; + rx_list_tail = tmp_desc_rx; + } + + /* allocat a new skb for next time receive */ + new_skb = dev_alloc_skb(PKT_BUF_SZ + 2);
use NET_IP_ALIGN
+ if (!new_skb) { + printk(KERN_NOTICE CARDNAME + ": init: low on mem - packet dropped\n"); + goto init_error; + } + skb_reserve(new_skb, 2);
ditto
+ tmp_desc_rx->skb = new_skb; + /* since RXDWA is enabled */ + tmp_desc_rx->desc_a.start_addr = + (unsigned long)new_skb->data - 2; + tmp_desc_rx->desc_a.x_count = 0; + tmp_desc_rx->desc_a.config.b_DMA_EN = 1; /* enabled */ + tmp_desc_rx->desc_a.config.b_WNR = 1; /* Write to memory */ + tmp_desc_rx->desc_a.config.b_WDSIZE = 2; /* wordsize is 32 bits */ + tmp_desc_rx->desc_a.config.b_NDSIZE = 6; /* 6 half words is desc size. */ + tmp_desc_rx->desc_a.config.b_FLOW = 7; /* large desc flow */ + tmp_desc_rx->desc_a.next_dma_desc = &(tmp_desc_rx->desc_b); + + tmp_desc_rx->desc_b.start_addr = + (unsigned long)(&(tmp_desc_rx->status)); + tmp_desc_rx->desc_b.x_count = 0; + tmp_desc_rx->desc_b.config.b_DMA_EN = 1; /* enabled */ + tmp_desc_rx->desc_b.config.b_WNR = 1; /* Write to memory */ + tmp_desc_rx->desc_b.config.b_WDSIZE = 2; /* wordsize is 32 bits */ + tmp_desc_rx->desc_b.config.b_NDSIZE = 6; + tmp_desc_rx->desc_b.config.b_DI_EN = 1; /* enable interrupt */ + tmp_desc_rx->desc_b.config.b_FLOW = 7; /* large mode */ + rx_list_tail->desc_b.next_dma_desc = &(tmp_desc_rx->desc_a);
should be using dma mapping
+ rx_list_tail->next = tmp_desc_rx; + rx_list_tail = tmp_desc_rx; + } + rx_list_tail->next = rx_list_head; /* rx_list is a circle */ + rx_list_tail->desc_b.next_dma_desc = &(rx_list_head->desc_a); + current_rx_ptr = rx_list_head; + + return 0; + + init_error: + desc_list_free(); + printk(KERN_ERR CARDNAME ": kmalloc failed\n"); + return -ENOMEM;
fix indentation
+/* Wait until the previous MDC/MDIO transaction has completed */ +static void poll_mdc_done(void) +{ + /* poll the STABUSY bit */ + while ((bfin_read_EMAC_STAADD()) & STABUSY) { + }; +}
no infinite loops no empty loops (at a minimum, must use cpu_relax())
+/* Read an off-chip register in a PHY through the MDC/MDIO port */ +static u16 read_phy_reg(u16 PHYAddr, u16 RegAddr) +{ + poll_mdc_done(); + bfin_write_EMAC_STAADD(SET_PHYAD(PHYAddr) | SET_REGAD(RegAddr) | STABUSY); /* read mode */
remove the ALL CAPS from function names
+ poll_mdc_done(); + + return (u16) bfin_read_EMAC_STADAT();
unneeded cast
+/* Write an off-chip register in a PHY through the MDC/MDIO port */ +static void raw_write_phy_reg(u16 PHYAddr, u16 RegAddr, u32 Data) +{ + bfin_write_EMAC_STADAT(Data); + + bfin_write_EMAC_STAADD(SET_PHYAD(PHYAddr) | SET_REGAD(RegAddr) | STAOP | STABUSY); /* write mode */ + + poll_mdc_done(); +} + +static void write_phy_reg(u16 PHYAddr, u16 RegAddr, u32 Data) +{ + poll_mdc_done(); + raw_write_phy_reg(PHYAddr, RegAddr, Data); +} + +/* set up the phy */ +static void bf537mac_setphy(struct net_device *dev) +{ + u16 phydat; + u32 sysctl; + struct bf537mac_local *lp = netdev_priv(dev); + + pr_debug("start settting up phy\n"); + + /* Program PHY registers */ + phydat = 0; + + /* issue a reset */ + raw_write_phy_reg(lp->PhyAddr, PHYREG_MODECTL, 0x8000); + + /* wait half a second */ + udelay(500);
must read after write, to ensure propagation to bus before delay otherwise, delay can happen /in parallel/ with write posting
+ phydat = read_phy_reg(lp->PhyAddr, PHYREG_MODECTL); + + /* advertise flow control supported */ + phydat = read_phy_reg(lp->PhyAddr, PHYREG_ANAR); + phydat |= (1 << 10); + write_phy_reg(lp->PhyAddr, PHYREG_ANAR, phydat); + + phydat = 0; + if (lp->Negotiate) { + phydat |= 0x1000; /* enable auto negotiation */ + } else { + if (lp->FullDuplex) { + phydat |= (1 << 8); /* full duplex */ + } else { + phydat &= (~(1 << 8)); /* half duplex */ + } + if (!lp->Port10) { + phydat |= (1 << 13); /* 100 Mbps */ + } else { + phydat &= (~(1 << 13)); /* 10 Mbps */ + } + }
remove braces around single-line C statements use defines from linux/mii.h where appropriate create named constants rather than using magic numbers like "1 << 14", i.e.: enum { phy_dat_10mbps = (1 << 13), };
+ if (lp->Loopback) { + phydat |= (1 << 14); /* enable TX->RX loopback */ +#if 0 + write_phy_reg(lp->PhyAddr, PHYREG_MODECTL, phydat); +#endif + } + + write_phy_reg(lp->PhyAddr, PHYREG_MODECTL, phydat); + udelay(500);
udelay after write
+ phydat = read_phy_reg(lp->PhyAddr, PHYREG_MODECTL); + /* check for SMSC PHY */ + if ((read_phy_reg(lp->PhyAddr, PHYREG_PHYID1) == 0x7) + && ((read_phy_reg(lp->PhyAddr, PHYREG_PHYID2) & 0xfff0) == 0xC0A0)) { + /* we have SMSC PHY so reqest interrupt on link down condition */ + write_phy_reg(lp->PhyAddr, 30, 0x0ff); /* enable interrupts */ + /* enable PHY_INT */ + sysctl = bfin_read_EMAC_SYSCTL(); + sysctl |= 0x1; +#if 0 + bfin_write_EMAC_SYSCTL(sysctl); +#endif + } +} + +/**************************************************************************/ +void setup_system_regs(struct net_device *dev) +{ + int PHYADDR; + unsigned short sysctl, phydat; + u32 opmode; + struct bf537mac_local *lp = netdev_priv(dev); + int count = 0; + + PHYADDR = lp->PhyAddr; + + /* Enable PHY output */ + if (!(bfin_read_VR_CTL() & PHYCLKOE)) + bfin_write_VR_CTL(bfin_read_VR_CTL() | PHYCLKOE); + + /* MDC = 2.5 MHz */ + sysctl = SET_MDCDIV(24); + /* Odd word alignment for Receive Frame DMA word */ + /* Configure checksum support and rcve frame word alignment */ +#if defined(BFIN_MAC_CSUM_OFFLOAD) + sysctl |= RXDWA | RXCKS; +#else + sysctl |= RXDWA; +#endif + bfin_write_EMAC_SYSCTL(sysctl); + /* auto negotiation on */ + /* full duplex */ + /* 100 Mbps */ + phydat = PHY_ANEG_EN | PHY_DUPLEX | PHY_SPD_SET; + write_phy_reg(PHYADDR, PHYREG_MODECTL, phydat); + + /* test if full duplex supported */ + do { + msleep(100); + phydat = read_phy_reg(PHYADDR, PHYREG_MODESTAT); + if (count > 30) { + printk(KERN_NOTICE CARDNAME + ": Link is down, please check your network connection\n"); + break; + } + count++; + } while (!(phydat & 0x0004));
kill magic numbers
+ phydat = read_phy_reg(PHYADDR, PHYREG_ANLPAR); + + if ((phydat & 0x0100) || (phydat & 0x0040)) {
ditto
+ opmode = FDMODE; + } else { + opmode = 0; + printk(KERN_INFO CARDNAME ": Network is set to half duplex\n"); + } + +#if defined(CONFIG_BFIN_MAC_RMII) + opmode |= RMII; /* For Now only 100MBit are supported */ +#endif + + bfin_write_EMAC_OPMODE(opmode); + +#if 0 + bfin_write_EMAC_MMC_CTL(RSTC | CROLL | MMCE); +#endif + bfin_write_EMAC_MMC_CTL(RSTC | CROLL); + + /* Initialize the TX DMA channel registers */ + bfin_write_DMA2_X_COUNT(0); + bfin_write_DMA2_X_MODIFY(4); + bfin_write_DMA2_Y_COUNT(0); + bfin_write_DMA2_Y_MODIFY(0); + + /* Initialize the RX DMA channel registers */ + bfin_write_DMA1_X_COUNT(0); + bfin_write_DMA1_X_MODIFY(4); + bfin_write_DMA1_Y_COUNT(0); + bfin_write_DMA1_Y_MODIFY(0); +} + +void setup_mac_addr(u8 * mac_addr) +{ + /* this depends on a little-endian machine */ + bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]); + bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
please make endian-neutral
+static void adjust_tx_list(void) +{ + if (tx_list_head->status.status_word != 0 + && current_tx_ptr != tx_list_head) { + goto adjust_head; /* released something, just return; */ + } + + /* if nothing released, check wait condition */ + /* current's next can not be the head, otherwise the dma will not stop as we want */ + if (current_tx_ptr->next->next == tx_list_head) { + while (tx_list_head->status.status_word == 0) { + udelay(10); + if (tx_list_head->status.status_word != 0 + || !(bfin_read_DMA2_IRQ_STATUS() & 0x08)) { + goto adjust_head;
infinite loop
+ } + } + if (tx_list_head->status.status_word != 0) { + goto adjust_head; + }
remove braces
+ } + + return; + + adjust_head: + do { + tx_list_head->desc_a.config.b_DMA_EN = 0; + tx_list_head->status.status_word = 0; + if (tx_list_head->skb) { + dev_kfree_skb(tx_list_head->skb); + tx_list_head->skb = NULL; + } else { + printk(KERN_ERR CARDNAME + ": no sk_buff in a transmitted frame!\n"); + } + tx_list_head = tx_list_head->next; + } while (tx_list_head->status.status_word != 0 + && current_tx_ptr != tx_list_head); + return; + +} + +static int bf537mac_hard_start_xmit(struct sk_buff *skb, struct net_device *dev) +{ + struct bf537mac_local *lp = netdev_priv(dev); + unsigned int data; + + current_tx_ptr->skb = skb; + /* Is skb->data always 16-bit aligned? Do we need to memcpy((char *)(tail->packet + 2),skb->data,len)? */ + if ((((unsigned int)(skb->data)) & 0x02) == 2) { + /* move skb->data to current_tx_ptr payload */ + data = (unsigned int)(skb->data) - 2; + *((unsigned short *)data) = (unsigned short)(skb->len); + current_tx_ptr->desc_a.start_addr = (unsigned long)data; + blackfin_dcache_flush_range(data, (data + (skb->len)) + 2); /* this is important! */ + + } else { + *((unsigned short *)(current_tx_ptr->packet)) = + (unsigned short)(skb->len); + memcpy((char *)(current_tx_ptr->packet + 2), skb->data, + (skb->len)); + current_tx_ptr->desc_a.start_addr = + (unsigned long)current_tx_ptr->packet; + if (current_tx_ptr->status.status_word != 0) + current_tx_ptr->status.status_word = 0; + blackfin_dcache_flush_range((unsigned int)current_tx_ptr-> + packet, + (unsigned int)(current_tx_ptr-> + packet + skb->len) + + 2); + }
maybe you need skb_copy_and_csum_dev() ? the above code is a mess
+ current_tx_ptr->desc_a.config.b_DMA_EN = 1; /* enable this packet's dma */ + + if (bfin_read_DMA2_IRQ_STATUS() & 0x08) { /* tx dma is running, just return */ + goto out; + } else { + /* tx dma is not running */ + bfin_write_DMA2_NEXT_DESC_PTR(&(current_tx_ptr->desc_a)); + /* dma enabled, read from memory, size is 6 */ + bfin_write_DMA2_CONFIG(*((unsigned short *)(&(current_tx_ptr->desc_a.config)))); + /* Turn on the EMAC tx */ + bfin_write_EMAC_OPMODE(bfin_read_EMAC_OPMODE() | TE); + }
this is blatantly racy
+ out: + adjust_tx_list(); + current_tx_ptr = current_tx_ptr->next; + dev->trans_start = jiffies; + lp->stats.tx_packets++; + lp->stats.tx_bytes += (skb->len); + return 0; +} + +static void bf537mac_rx(struct net_device *dev) +{ + struct sk_buff *skb, *new_skb; + struct bf537mac_local *lp = netdev_priv(dev); + unsigned short len; + + /* allocat a new skb for next time receive */ + skb = current_rx_ptr->skb; + new_skb = dev_alloc_skb(PKT_BUF_SZ + 2);
NET_IP_ALIGN
+ if (!new_skb) { + printk(KERN_NOTICE CARDNAME + ": rx: low on mem - packet dropped\n"); + lp->stats.rx_dropped++; + goto out; + } + /* reserve 2 bytes for RXDWA padding */ + skb_reserve(new_skb, 2);
ditto
+ current_rx_ptr->skb = new_skb; + current_rx_ptr->desc_a.start_addr = (unsigned long)new_skb->data - 2; + +#if 0 + int i; + if (len >= 64) { + for (i=0;i<len;i++) { + printk("%.2x-",((unsigned char *)pkt)[i]); + if (((i%8)==0) && (i!=0)) printk("\n"); + } + printk("\n"); + } +#endif
remove all this #if 0 code
+ len = (unsigned short)((current_rx_ptr->status.status_word) & RX_FRLEN); + skb_put(skb, len); + blackfin_dcache_invalidate_range((unsigned long)skb->head, + (unsigned long)skb->tail); + + dev->last_rx = jiffies; + skb->dev = dev;
not needed due to next line of code:
+ skb->protocol = eth_type_trans(skb, dev);
+#if defined(BFIN_MAC_CSUM_OFFLOAD) + skb->csum = current_rx_ptr->status.ip_payload_csum; + skb->ip_summed = CHECKSUM_PARTIAL; +#endif + + netif_rx(skb); + lp->stats.rx_packets++; + lp->stats.rx_bytes += len; + current_rx_ptr->status.status_word = 0x00000000; + current_rx_ptr = current_rx_ptr->next;
look into NAPI
+ out:
fix indentation
+static void bf537mac_set_multicast_list(struct net_device *dev) +{ + u32 sysctl; + + if (dev->flags & IFF_PROMISC) { + printk(KERN_INFO "%s: set to promisc mode\n", dev->name); + sysctl = bfin_read_EMAC_OPMODE(); + sysctl |= RAF; + bfin_write_EMAC_OPMODE(sysctl); + } else if (dev->flags & IFF_ALLMULTI || dev->mc_count > 16) { + /* accept all multicast */ + sysctl = bfin_read_EMAC_OPMODE(); + sysctl |= PAM; + bfin_write_EMAC_OPMODE(sysctl); + } else if (dev->mc_count) { + /* set multicast */
obvious bug: INSERT CODE HERE
+#if 0 + dev->ethtool_ops = &bf537mac_ethtool_ops; +#endif
implement this even the most basic GDRVINFO ioctl, which takes all of two seconds to do
+#ifdef CONFIG_NET_POLL_CONTROLLER + dev->poll_controller = bf537mac_poll; +#endif + + /* fill in some of the fields */ + lp->version = 1; + lp->PhyAddr = 0x01; + lp->CLKIN = 25; + lp->FullDuplex = 0; + lp->Negotiate = 1; + lp->FlowControl = 0; + spin_lock_init(&lp->lock); + + /* set the GPIO pins to Ethernet mode */ + setup_pin_mux(); + + /* now, enable interrupts */ + /* register irq handler */ + if (request_irq + (IRQ_MAC_RX, bf537mac_interrupt, IRQF_DISABLED | IRQF_SHARED, + "BFIN537_MAC_RX", dev)) { + printk(KERN_WARNING CARDNAME + ": Unable to attach BlackFin MAC RX interrupt\n"); + return -EBUSY; + } + + /* Enable PHY output early */ + if (!(bfin_read_VR_CTL() & PHYCLKOE)) + bfin_write_VR_CTL(bfin_read_VR_CTL() | PHYCLKOE); + + retval = register_netdev(dev); + if (retval == 0) { + /* now, print out the card info, in a short format.. */ + printk(KERN_INFO "Blackfin mac net device registered\n"); + } + + err_out: + return retval;
if register_netdev() fails, you must unregister irq
+static int bfin_mac_probe(struct platform_device *pdev) +{ + struct net_device *ndev; + + ndev = alloc_etherdev(sizeof(struct bf537mac_local)); + + if (!ndev) { + printk(KERN_WARNING CARDNAME ": could not allocate device\n"); + return -ENOMEM; + } + + if (bf537mac_probe(ndev) != 0) { + platform_set_drvdata(pdev, NULL);
pointless, you haven't set drvdata yet
+ free_netdev(ndev); + printk(KERN_WARNING CARDNAME ": not found\n"); + return -ENODEV; + } + + SET_MODULE_OWNER(ndev); + SET_NETDEV_DEV(ndev, &pdev->dev);
move this code to with the rest of the net device init code
+ platform_set_drvdata(pdev, ndev); + + return 0; +} + +static int bfin_mac_remove(struct platform_device *pdev) +{ + struct net_device *ndev = platform_get_drvdata(pdev); + + platform_set_drvdata(pdev, NULL);
do this after you unregister netdev and irq
+ unregister_netdev(ndev); + + free_irq(IRQ_MAC_RX, ndev); + + free_netdev(ndev); + + return 0; +} + +static int bfin_mac_suspend(struct platform_device *pdev, pm_message_t state) +{ + return 0; +} + +static int bfin_mac_resume(struct platform_device *pdev) +{ + return 0; +}
NAK obviously broken suspend/resume
diff -puN /dev/null drivers/net/bfin_mac.h --- /dev/null +++ a/drivers/net/bfin_mac.h
delete this header, move it to the top of the C source file - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html