Date:	Sat, 14 Aug 2010 12:38:59 +0530
From:	Sachin Sanap <ssanap@...vell.com>
To:	Lennert Buytenhek <buytenh@...tstofly.org>
CC:	Philip Rakity <prakity@...vell.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Ashish Karkare <akarkare@...vell.com>,
	"Prabhanjan Sarnaik" <sarnaik@...vell.com>,
	"eric.y.miao@...il.com" <eric.y.miao@...il.com>,
	Mark Brown <markb@...vell.com>
Subject: Re: [PATCH v2]  net: add Fast Ethernet driver for PXA168.

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:
> 
> > * 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.

> 
> > +#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.
> 
> 
> > +#define MAX_DESCS_PER_HIGH	(60)
> > +#define TX_DESC_COUNT_LOW	(10)
> 
> These don't seem used.
Removed.
> 
> 
> > +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.
> 
> > +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.
> 
> 
> > +static inline u8 flip_8_bits(u8 x)
> > +{
> > +	return (((x) & 0x01) << 3) | (((x) & 0x002) << 1)
> > +	    | (((x) & 0x04) >> 1) | (((x) & 0x008) >> 3)
> 
> 0x02, 0x08
> 
> 
> > +	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.
> 
> 
> > +	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.
> 
> > +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.
> 
> 
> > +	pep->work_todo &= ~(WORK_TX_DONE);
> 
> Doesn't need parentheses.
removed.
> 
> 
> > +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.
> 
> 
> > +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.
> 
> 
> > +	wrl(pep, INT_CAUSE, icr ^ 0xffffffff);
> 
> ~icr ?
done.
> 
> 
> > +	/* Extended Port Configuration */
> > +	wrl(pep,
> > +	    PORT_CONFIG_EXT, PCXR_2BSM | /* Two byte suffix aligns IP hdr */
> 
> Prefix?
changed.
> 
> 
> > +		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.
> 
> 
> > +	err = request_irq(dev->irq, pxa168_eth_int_handler,
> > +			  IRQF_DISABLED , dev->name, dev);
> 
> Superfluous space before the comma.
removed.
> 
> 
> > +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.
> 
> 
> > +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.
> 
> 
> > +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.
> 
> > +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.
> 
> > +	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.
> 
> 
> > +	/* 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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists