Date:	Fri, 6 Aug 2010 08:44:02 -0700
From:	Philip Rakity <prakity@...vell.com>
To:	Sachin Sanap <ssanap@...vell.com>
CC:	Lennert Buytenhek <buytenh@...tstofly.org>,
	"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] net: add Fast Ethernet driver for PXA168.


Sachin,

You should change

>>> +		skb = dev_alloc_skb(MAX_PKT_SIZE + ETH_HW_IP_ALIGN);

>> 

to allocatie ADDITIONAL prepend space in the header.  If you are doing ethernet to 802.11 routing or bridging then the wireless driver will not have enough space in front of the header to prepend its headers without having to allocate additional space.   You should check what 802.11n requires.  64 is okay for 11b/g. I would imagine doing something like this.

#define EXTRA_PREPEND_SPACE  64   /* extra header space for wireless headers */

>>> skb = dev_alloc_skb(MAX_PKT_SIZE + ETH_HW_IP_ALIGN + EXTRA_PREPEND_SPACE);

> +             skb_reserve(skb, ETH_HW_IP_ALIGN + EXTRA_PREPEND_SPACE);

etc

Philip


On Aug 6, 2010, at 8:14 AM, Sachin Sanap wrote:

> Thanks Lennert,Eric for your review. Comments inline. I will resend the modified patch as a reply to this email.
>> 
>> On Wed, Aug 04, 2010 at 04:17:50PM +0530, Sachin Sanap wrote:
>> 
>>> This patch adds support for PXA168 Fast Ethernet on Aspenite
>>> board.
>> 
>> There's nothing Aspenite-specific in this patch (nor should there be),
>> so you can leave that part out.
> 
> Removed.
>> 
>> 
>>> Patch generated against Linux 2.6.35-rc5
>>> commit cd5b8f8755a89a57fc8c408d284b8b613f090345
>> 
>> This might be interesting to know but shouldn't be part of the commit
>> message.
> 
> Removed.
>> 
>> 
>>> diff --git a/arch/arm/mach-mmp/include/mach/pxa168_eth.h
>> b/arch/arm/mach-mmp/include/mach/pxa168_eth.h
>>> new file mode 100644
>>> index 0000000..abfd335
>>> --- /dev/null
>>> +++ b/arch/arm/mach-mmp/include/mach/pxa168_eth.h
>>> @@ -0,0 +1,18 @@
>>> +/*
>>> + *pxa168 ethernet platform device data definition file.
>>> + */
>>> +#ifndef __LINUX_PXA168_ETH_H
>>> +#define __LINUX_PXA168_ETH_H
>> 
>> This should just be in include/linux, IMHO, as this unit isn't only used
>> in the PXA168, for one.
> I have moved it to include/linux but the PXA168 name is still there.
> 
>> 
>> 
>>> +struct pxa168_eth_platform_data {
>>> +	int	port_number;
>>> +	u16	phy_addr;
>>> +
>>> +	/* If speed is 0, then speed and duplex are autonegotiated. */
>>> +	u32	speed;		/* 0, SPEED_10, SPEED_100 */
>>> +	u32	duplex;		/* DUPLEX_HALF or DUPLEX_FULL */
>> 
>> phylib treats these three (phy address, speed and duplex) as ints, is
>> there any reason you need these to be of different types?
> 
> 
> Used int at all three places.
> 
>> 
>> 
>>> +	int (*init)(void);
>> 
>> What's this needed for?  The name of this callback is entirely
>> nondescriptive, there's no comment as to when it's called, etc.
> 
> Added a comment for it.
>> 
>> 
>>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>> index ce2fcdd..5ebf287 100644
>>> --- a/drivers/net/Kconfig
>>> +++ b/drivers/net/Kconfig
>>> @@ -927,6 +927,16 @@ config SMC91X
>>> 	  The module will be called smc91x.  If you want to compile it as a
>>> 	  module, say M here and read
>> <file:Documentation/kbuild/modules.txt>.
>>> 
>>> +config PXA168_ETH
>>> +	tristate "Marvell pxa168 ethernet support"
>>> +	depends on MACH_ASPENITE
>> 
>> You can have it depend on ARM or PXA168, but having it depend on support
>> for one specific board support file is almost certainly wrong.
> 
> Depends on CPU_PXA168.
>> 
>> 
>>> diff --git a/drivers/net/pxa168_eth.c b/drivers/net/pxa168_eth.c
>>> new file mode 100644
>>> index 0000000..618e558
>>> --- /dev/null
>>> +++ b/drivers/net/pxa168_eth.c
>>> @@ -0,0 +1,1723 @@
>>> +/*
>>> + * Driver for PXA168 based boards.
>> 
>> Why not call this "PXA168 ethernet driver"?
> Change done.
>> 
>> 
>>> + * Based on MV643XX driver.
>> 
>> The mv643xx ethernet driver, that is.
> Change done.
>> 
>> 
>>> +#define DRIVER_NAME	"pxa168-mfu"
>> 
>> mfu?
> Its was for multi function unit on Aspenite, changed it to pxa168-eth 
>> 
>> 
>>> +/* smi register */
>>> +#define SMI_BUSY		(1<<28)	/* 0 - Write, 1 - Read  */
>>> +#define SMI_R_VALID		(1<<27)	/* 0 - Write, 1 - Read  */
>>> +#define SMI_OP_W		(0<<26)	/* Write operation      */
>>> +#define SMI_OP_R		(1<<26)	/* Read operation */
>> 
>> (1 << 28)
>> (1 << 27)
>> (0 << 26)
>> 
>> etc (thoughout the file)
> Changed.
> 
>> 
>> 
>>> +struct pxa168_private {
>> 
>> ITYM pxa168_eth_private
> Yes, changed.
>> 
>> 
>>> +	/*
>>> +	 * Used in case RX Ring is empty, which can be caused when
>>> +	 * system does not have resources (skb's)
>>> +	 */
>> 
>> can be caused by
>> can occur when
> Changed.
>> 
>> 
>>> +	struct timer_list timeout;
>>> +	struct mii_bus *smi_bus;
>>> +	struct phy_device *phy;
>>> +
>>> +	/* clock */
>>> +	struct clk *clk;
>>> +	struct pxa168_eth_platform_data *pd;
>>> +	/*
>>> +	 * Ethernet controller base address.
>>> +	 */
>>> +	void __iomem *base;
>>> +
>>> +	u8 *htpr;		/* hash pointer */
>> 
>> IMHO it'd look better if you'd either always put the comment on the same
>> line or put the comment above the member (within the same struct).
>> 
>> Also, 'hash pointer' as a comment isn't very helpful.  Making it
>> 'hardware address filter table' or so would be more descriptive.
> Changed.
>> 
>> 
>>> +struct addr_table_entry {
>>> +	u32 lo;
>>> +	u32 hi;
>>> +};
>> 
>> If the address filter table consists of these structures, it's
>> confusing to make htpr a u8 * and then cast things around.  Better make
>> htpr a void * then.
> Changed.
>> 
>> Also, what about endian-cleanness?  If you run the CPU core in big
>> endian, will the address filter table handling logic in this driver
>> still work?
> 
> 
> Added cpu_to_le32 for the mac_high and mac_low variables before passing them to the add_del_hash_entry. I have not tested it on the Big endian but I think it should now work.
> 
>> 
>> 
>>> +static char marvell_OUI[3] = { 0x02, 0x50, 0x43 };
>> 
>> This isn't even a marvell OUI -- 00:50:43 is a marvell OUI, and
>> anything with 02: is a locally generated address.
>> 
>> Why don't you just use random_ether_addr()?
> Changed to use random_ether_addr.
>> 
>> 
>>> +static inline u32 rdl(struct pxa168_private *mp, int offset)
>>> +{
>>> +	return readl(mp->base + offset);
>>> +}
>> 
>> In mv643xx_eth, 'mp' refers to mv643xx_eth_private, it's funny that
>> you've changed the name of the struct but not that of the variable.
>> 
>> As the rest of the driver looks very very similar to mv643xx_eth, if
>> not pretty much entirely identical in a lot of places, you should give
>> more credit to mv643xx_eth than 'based on', in my opinion.
> Changed the credit line for the same.
>> 
>> 
>>> +static int ethernet_phy_get(struct pxa168_private *mp)
>>> +{
>>> +	unsigned int reg_data;
>>> +
>>> +	/* only support 3 ports */
>>> +	BUG_ON(mp->port_num > 2);
>> 
>> Hm, does it actually support 3 ports?
> No. changed the code.
>> 
>> 
>>> +static void ethernet_phy_set_addr(struct pxa168_private *mp, int
>> phy_addr)
>>> +{
>>> +	u32 reg_data;
>>> +	int addr_shift = 5 * mp->port_num;
>>> +
>>> +	/* only support 3 ports */
>>> +	BUG_ON(mp->port_num > 2);
>>> +
>>> +	reg_data = rdl(mp, PHY_ADDRESS);
>>> +	reg_data &= ~(0x1f << addr_shift);
>>> +	reg_data |= (phy_addr & 0x1f) << addr_shift;
>>> +	wrl(mp, PHY_ADDRESS, reg_data);
>>> +}
>>> +static void ethernet_phy_reset(struct pxa168_private *mp)
>>> +{
>> 
>> Blank line between functions.
>> 
>> 
>>> +	do {
>>> +		data = phy_read(mp->phy, MII_BMCR);
>>> +	} while (data >= 0 && data & BMCR_RESET);
>>> +
>>> +}
>> 
>> No blank line here.
>> 
>> 
>>> +static void rxq_refill(struct net_device *dev)
>>> +{
>>> +	struct pxa168_private *mp = netdev_priv(dev);
>>> +	struct sk_buff *skb;
>>> +	struct rx_desc *p_used_rx_desc;
>>> +	int used_rx_desc;
>>> +
>>> +	while (mp->rx_desc_count < mp->rx_ring_size) {
>>> +
>>> +		skb = dev_alloc_skb(MAX_PKT_SIZE + ETH_HW_IP_ALIGN);
>> 
>> No blank line here.
>> 
>> 
>>> +	if (mp->rx_desc_count == 0) {
>>> +		printk(KERN_INFO "%s: Rx ring is empty\n", dev->name);
>> 
>> I wouldn't printk for this (and certainly not without a rate limit) --
>> going OOM will then just mean that you'll get endless console spam as
>> well, which isn't helpful.
> Hm, removed the printk.
> 
>> 
>> 
>>> +/*
>>> + * --------------------------------------------------------------------
>> --------
>>> + * This function will calculate the hash function of the address.
>>> + * depends on the hash mode and hash size.
>> 
>> Where are the hash mode and size configured?
>> 
>> 
>>> +static u32 hash_function(u32 mac_high, u32 mac_low)
>>> +{
>>> +	u32 hash_result;
>>> +	u32 addr_high;
>>> +	u32 addr_low;
>>> +	u32 addr0;
>>> +	u32 addr1;
>>> +	u32 addr2;
>>> +	u32 addr3;
>>> +	u32 addr_high_swapped;
>>> +	u32 addr_low_swapped;
>>> +
>>> +	addr_high = nibble_swapping_16_bit(mac_high);
>>> +	addr_low = nibble_swapping_32_bit(mac_low);
>>> +
>>> +	addr_high_swapped = flip_4_bits(addr_high & 0xf)
>>> +	    + ((flip_4_bits((addr_high >> 4) & 0xf)) << 4)
>>> +	    + ((flip_4_bits((addr_high >> 8) & 0xf)) << 8)
>>> +	    + ((flip_4_bits((addr_high >> 12) & 0xf)) << 12);
>>> +
>>> +	addr_low_swapped = flip_4_bits(addr_low & 0xf)
>>> +	    + ((flip_4_bits((addr_low >> 4) & 0xf)) << 4)
>>> +	    + ((flip_4_bits((addr_low >> 8) & 0xf)) << 8)
>>> +	    + ((flip_4_bits((addr_low >> 12) & 0xf)) << 12)
>>> +	    + ((flip_4_bits((addr_low >> 16) & 0xf)) << 16)
>>> +	    + ((flip_4_bits((addr_low >> 20) & 0xf)) << 20)
>>> +	    + ((flip_4_bits((addr_low >> 24) & 0xf)) << 24)
>>> +	    + ((flip_4_bits((addr_low >> 28) & 0xf)) << 28);
>>> +
>>> +	addr_high = addr_high_swapped;
>>> +	addr_low = addr_low_swapped;
>>> +
>>> +	addr0 = (addr_low >> 2) & 0x03f;
>>> +	addr1 = (addr_low & 0x003) | ((addr_low >> 8) & 0x7f) << 2;
>>> +	addr2 = (addr_low >> 15) & 0x1ff;
>>> +	addr3 = ((addr_low >> 24) & 0x0ff) | ((addr_high & 1) << 8);
>>> +
>>> +	hash_result = (addr0 << 9) | (addr1 ^ addr2 ^ addr3);
>>> +	hash_result = hash_result & 0x07ff;
>>> +	return hash_result;
>> 
>> As it's only a bunch of XORs, can you not calculate the hash function
>> first and only then do the bit reversal?
>> 
>> 
>>> + * --------------------------------------------------------------------
>> --------
>>> + * This function will add an entry to the address table.
>>> + * depends on the hash mode and hash size that was initialized.
>>> + * Inputs
>>> + * mp - ETHERNET .
>>> + * mac_high - the 2 most significant bytes of the MAC address.
>>> + * mac_low - the 4 least significant bytes of the MAC address.
>>> + * skip - if 1, skip this address.
>> 
>> What does 'skip' do?
>> 
>> 
>>> + * rd   - the RD field in the address table.
>> 
>> And what does that do?
> 
> Added more comments regarding the address filtering and the hash function.
> 
>> 
>> If one has hardware docs, this comment doesn't really add anything,
>> while if one doesn't have hardware docs, this comment doesn't really
>> explain anything -- so the comment isn't very useful.
>> 
>> 
>>> +static void update_hash_table_mac_address(struct pxa168_private *mp,
>>> +					  u8 *oaddr, u8 *addr)
>>> [...]
>>> +static int init_hashtable(struct pxa168_private *mp)
>> 
>> hash_table?
>> 
>> 
>>> +{
>>> +	u8 *addr;
>>> +	dma_addr_t reg_dma;
>>> +
>>> +	if (mp->htpr == NULL) {
>>> +		mp->htpr = dma_alloc_coherent(NULL,
>>> +					      HASH_ADDR_TABLE_SIZE + 7,
>>> +					      &mp->htpr_dma, GFP_KERNEL);
>>> +		if (mp->htpr == NULL)
>>> +			return -ENOMEM;
>>> +	}
>>> +
>>> +	/* align to 8 byte boundary */
>>> +	addr = (u8 *) (((u32) mp->htpr + 7) & ~0x7);
>>> +	reg_dma = (dma_addr_t) (((u32) mp->htpr_dma + 7) & ~0x7);
>> 
>> DMA-API-HOWTO.txt says about dma_alloc_coherent():
>> 
>> 	The cpu return address and the DMA bus master address are both
>> 	guaranteed to be aligned to the smallest PAGE_SIZE order which
>> 	is greater than or equal to the requested size.
>> 
>> So this shuffling isn't needed.
> 
> Hm, removed the shuffling code.
>> 
>> 
>> etc etc.

--
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