On Tue, 2010-08-10 at 05:33 -0700, Lennert Buytenhek wrote:

On Tue, Aug 10, 2010 at 02:00:04PM +0530, Sachin Sanap wrote:

quoted

* Headroom in SKB for 802.11 not included in the patch since that
  varies based on 802.11 a/b/g/n.

I don't think this is true?

(The 11a/b/n on-the-air preambles are of different lengths (and are
sent at different rates), but that isn't visible to software.)

I might be wrong in the above reasoning, but if skb headroom of 64 is
enough then in 2.6.35 NET_SKB_PAD is already pulled up to 64 so still
not including the headroom patch. Also if wireless driver needs more
headroom than 64 then we might think of doing it the way David has
suggested.

quoted

+#define ETH_HW_IP_ALIGN		2	/* hw aligns IP header */
+#define ETH_DATA_LEN		1500
+#define MAX_PKT_SIZE		1518

How about (stacked) VLANs?

Is the hardware entirely unable to receive larger packets than this, or
is it capable of receiving such packets but e.g. with loss of hardware
receive checksum offloading?

Changed the code to receive larger packet sizes with configurable MTU
upto 9500 bytes.

(I guess the hardware can't do RX checksum offload at all since I see
no references to skb->ip_summed and CHECKSUM_*?)

Hardware can not do any checksum offloading.

quoted

+#define MAX_DESCS_PER_HIGH	(60)
+#define TX_DESC_COUNT_LOW	(10)

These don't seem used.

Removed.

quoted

+struct pxa168_eth_private {

[...]

+	/* Size of Tx Ring per queue */
+	int tx_ring_size;

[...]

+	/* Size of Rx Ring per queue */
+	int rx_ring_size;

If you're not going to let the tx/rx ring size be runtime configurable
(like they are in mv643xx_eth), you might as well leave these as defines.

Added the code for runtime configuration of tx/rx ring size.

quoted

+static void ethernet_phy_set_addr(struct pxa168_eth_private *pep, int phy_addr)
+{
+	u32 reg_data;
+
+	reg_data = rdl(pep, PHY_ADDRESS);
+	reg_data &= ~(0x1f);

No need for the parentheses.

Hardware does support 3ports so got that code back in.

quoted

+static inline u8 flip_8_bits(u8 x)
+{
+	return (((x) & 0x01) << 3) | (((x) & 0x002) << 1)
+	    | (((x) & 0x04) >> 1) | (((x) & 0x008) >> 3)

0x02, 0x08

quoted

+	addr0 = (mac_addr[5] >> 2) & 0x03f;
+	addr1 = (mac_addr[5] & 0x003) | (((mac_addr[4] & 0x7f)) << 2);
+	addr2 = ((mac_addr[4] & 0x80) >> 7) | mac_addr[3] << 1;
+	addr3 = (mac_addr[2] & 0x0ff) | ((mac_addr[1] & 1) << 8);

0x34, 0x03, 0xff

Changed.

quoted

+	if (i == HOP_NUMBER) {
+		if (!del) {
+			printk(KERN_INFO "%s: table section is full\n",
+			       __FILE__);
+			return -ENOSPC;
+		} else

What does it mean in practice if this happens?  (The error message
could be a bit more descriptive.)

might need the 8kB implementation of hash table. Added the comment.

quoted

+static void pxa168_eth_set_rx_mode(struct net_device *dev)
+{
+	struct pxa168_eth_private *pep = netdev_priv(dev);
+	struct netdev_hw_addr *ha;
+	u32 val;
+
+	val = rdl(pep, PORT_CONFIG);
+	if (dev->flags & IFF_PROMISC)
+		val |= PCR_PM;
+	else
+		val &= ~PCR_PM;
+	wrl(pep, PORT_CONFIG, val);
+	netdev_for_each_mc_addr(ha, dev)
+	    update_hash_table_mac_address(pep, NULL, ha->addr);
+}

1. Don't indent with spaces.
2. This will never remove old multicast MAC addresses?

Added code to flush old MAC addresses.

quoted

+	pep->work_todo &= ~(WORK_TX_DONE);

Doesn't need parentheses.

removed.

quoted

+static int rxq_process(struct net_device *dev, int budget)
+{
+	struct pxa168_eth_private *pep = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	unsigned int received_packets = 0;
+	struct sk_buff *skb;
+
+	while (budget-- > 0) {
+
+		int rx_next_curr_desc, rx_curr_desc, rx_used_desc;

No need for an empty line.

removed.

quoted

+static int pxa168_eth_collect_events(struct pxa168_eth_private *pep,
+				     struct net_device *dev)
+{
+	u32 icr;
+	int ret = 0;
+
+	icr = rdl(pep, INT_CAUSE);
+	if (0x00 == icr)

	icr == 0

done.

quoted

+	wrl(pep, INT_CAUSE, icr ^ 0xffffffff);

~icr ?

done.

quoted

+	/* Extended Port Configuration */
+	wrl(pep,
+	    PORT_CONFIG_EXT, PCXR_2BSM | /* Two byte suffix aligns IP hdr */

Prefix?

changed.

quoted

+		dma_free_coherent(NULL, pep->tx_desc_area_size,
+				  pep->p_tx_desc_area, pep->tx_desc_dma);

BTW, you should pass in a struct device * to the DMA allocation
functions.

done.

quoted

+	err = request_irq(dev->irq, pxa168_eth_int_handler,
+			  IRQF_DISABLED , dev->name, dev);

Superfluous space before the comma.

removed.

quoted

+static void eth_tx_submit_descs_for_skb(struct pxa168_eth_private *pep,
+					struct sk_buff *skb)
+{
+	int tx_index;
+	struct tx_desc *desc;
+	int length;
+
+	tx_index = eth_alloc_tx_desc_index(pep);
+	desc = &pep->p_tx_desc_area[tx_index];
+	length = skb->len;
+	pep->tx_skb[tx_index] = skb;
+	desc->byte_cnt = length;
+	desc->buf_ptr = dma_map_single(NULL, skb->data, length, DMA_TO_DEVICE);
+	wmb();
+	desc->cmd_sts = BUF_OWNED_BY_DMA | TX_GEN_CRC | TX_FIRST_DESC |
+			TX_ZERO_PADDING | TX_LAST_DESC;
+	if (unlikely(!(pep->tx_desc_count % (pep->tx_ring_size / 4))))
+		desc->cmd_sts |= TX_EN_INT;

Is this intended to only generate transmit completion interrupts for
every N packets?

If so, you cannot delay kfree_skb()ing a transmitted skb indefinitely.
If you want to batch TX completion interrupts, you at least have to put
in a timeout.

Modified the code to generate interrupt per packet.

Also, the descriptor is in device-visible memory, and BUF_OWNED_BY_DMA
becomes visible to the device as soon as you do the preceding store to
desc->cmd_sts -- you cannot then go back and alter that field, as you
could race with the device clearing BUF_OWNED_BY_DMA.

quoted

+static int pxa168_eth_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct pxa168_eth_private *pep = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+
+	eth_tx_submit_descs_for_skb(pep, skb);

In mv643xx_eth, txq_submit_*() are much larger because they have to deal
with scatter-gather transmit.  Since you don't support that, you might
as well just inline this function here.

done.

quoted

+static int pxa168_smi_read(struct mii_bus *bus, int phy_addr, int regnum)
+{
+	int val;
+	struct pxa168_eth_private *pep = bus->priv;
+	int i = 0;
+
+	/* wait for the SMI register to become available */
+	for (i = 0; (val = rdl(pep, SMI)) & SMI_BUSY; i++) {
+		if (i == PHY_WAIT_ITERATIONS) {
+			printk(KERN_ERR
+			       "pxa168 PHY timeout, val=0x%x\n", val);
+			return -ETIMEDOUT;
+		}
+		udelay(1);
+	}
+	wrl(pep, SMI, (phy_addr << 16) | (regnum << 21) | SMI_OP_R);
+	/* now wait for the data to be valid */
+	for (i = 0; !((val = rdl(pep, SMI)) & SMI_R_VALID); i++) {
+		if (i == PHY_WAIT_ITERATIONS) {
+			printk(KERN_ERR
+			       "pxa168 PHY RD timeout, val=0x%x\n", val);
+			return -ETIMEDOUT;
+		}
+		udelay(1);
+	}
+	return val & 0xffff;
+}

This can end up busy-waiting (i.e. hogging the CPU) for twice 500 us,
i.e. 1 msec.  Isn't there a SMI completion interrupt you can use, or
at least yield the cpu by sleeping for a bit?

Added code to yield the CPU.

quoted

+static int pxa168_smi_write(struct mii_bus *bus, int phy_addr, int regnum,
+			    u16 value)
+{
+	struct pxa168_eth_private *pep = bus->priv;
+	int i;
+
+	/* wait for the SMI register to become available */
+	for (i = 0; rdl(pep, SMI) & SMI_BUSY; i++) {
+		if (i == PHY_WAIT_ITERATIONS) {
+			printk(KERN_ERR "pxa168 PHY busy timeout.\n");
+			return -ETIMEDOUT;
+		}
+		udelay(1);
+	}
+	wrl(pep, SMI, (phy_addr << 16) | (regnum << 21) |
+	    SMI_OP_W | (value & 0xffff));

I would wait here for the write to complete, otherwise you can't report
errors due to the slave address not responding.

done.

quoted

+	clk = clk_get(&pdev->dev, "MFUCLK");
+	if (IS_ERR(clk)) {
+		printk(KERN_ERR "fast Ethernet failed to get clock\n");

At least stick the name of the driver in here.

quoted

+	/* init callback is used for board specific initialization
+	 * e.g on Aspenite its used to initialize the PHY transceiver.
+	 */
+	int (*init)(void);

Is resetting the PHY not enough?

On Aspenite the PHY transceiver needs to be enabled by sending high
signals on two GPIO expander pins which are under I2C control.I could
not do this in the board specific init function since at that time the
I2C is not up. If we dont do this the phy is not detected so we cant
even reset it.