[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

Reply via email to