linux-mips
[Top] [All Lists]

RE: [Repost PATCH 6/6] PMC MSP85x0 gigabit ethernet driver

To: "'Francois Romieu'" <romieu@fr.zoreil.com>
Subject: RE: [Repost PATCH 6/6] PMC MSP85x0 gigabit ethernet driver
From: Kiran Thota <Kiran_Thota@pmc-sierra.com>
Date: Tue, 27 Jun 2006 00:15:46 -0700
Cc: "'Yoichi Yuasa'" <yoichi_yuasa@tripeaks.co.jp>, linux-mips@linux-mips.org, netdev@vger.kernel.org, Raj Palani <Rajesh_Palani@pmc-sierra.com>, ralf@linux-mips.org
Original-recipient: rfc822;linux-mips@linux-mips.org
Sender: linux-mips-bounce@linux-mips.org
Francois,
 Thanks for your comments. I have absorbed almost all of your suggestions. You 
skimmed through 33% of the file before you dozed off... Waiting for the rest!

Kiran 

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On 
Behalf Of Francois Romieu
Sent: Monday, June 26, 2006 3:31 PM
To: Kiran Thota
Cc: 'Yoichi Yuasa'; linux-mips@linux-mips.org; netdev@vger.kernel.org; Raj 
Palani; ralf@linux-mips.org
Subject: Re: [Repost PATCH 6/6] PMC MSP85x0 gigabit ethernet driver

Kiran Thota <Kiran_Thota@pmc-sierra.com> :
[...]
>  - Based on linux-2.6.12 from
>  
> http://www.linux-mips.org/pub/linux/mips/kernel/v2.6/linux-2.6.12.tar.
> gz

Is there a reason why the patch is not diffed against a more recent version of 
the mips tree ?

The patch includes ~130 lines ending with a tab or space delimiter.
They could be removed.

[...]
> diff -Naur a/drivers/net/msp85x0_ge.c b/drivers/net/msp85x0_ge.c
> --- a/drivers/net/msp85x0_ge.c        1969-12-31 16:00:00.000000000 -0800
> +++ b/drivers/net/msp85x0_ge.c        2006-06-26 12:12:39.000000000 -0700
[...]
> +/* Static Function Declarations       */
> +static int msp85x0_ge_eth_open(struct net_device *); static int 
> +msp85x0_ge_eth_reopen(struct net_device *netdev); static void 
> +msp85x0_ge_eth_stop(struct net_device *); static struct 
> +net_device_stats *msp85x0_ge_get_stats(struct net_device *); static 
> +int msp85x0_ge_init_rx_desc_ring(msp85x0_ge_port_info *, int, int,
> +                                   unsigned long, unsigned long,
> +                                   unsigned long);
> +static int msp85x0_ge_init_tx_desc_ring(msp85x0_ge_port_info *, int,
> +                                   unsigned long, unsigned long);
> +
> +static int msp85x0_ge_open(struct net_device *); static int 
> +msp85x0_ge_start_xmit(struct sk_buff *, struct net_device *); static 
> +int msp85x0_ge_stop(struct net_device *);
> +
> +static unsigned int msp85x0_ge_tx_coal(int);
> +
> +static void msp85x0_ge_port_reset(unsigned int); static int 
> +msp85x0_ge_free_tx_queue(struct net_device *); static int 
> +msp85x0_ge_rx_task(struct net_device *, msp85x0_ge_port_info *); 
> +static int msp85x0_ge_port_start(struct net_device *); static int 
> +msp85x0_eth_setup_tx_rx_fifo(struct net_device *dev); static int 
> +msp85x0_ge_xdma_reset(void);

You should be able to remove the forward declarations by correctly reordering 
the code.

[...]
> +static struct platform_device *msp85x0_ge_device[NO_PORTS]; static 
> +struct net_device *msp85x0_eth[NO_PORTS]; static unsigned int 
> +msp85x0_ge_sram; static unsigned int port_number;

The way it is used, port_number should be a local variable.

> +
> +/*
> + * The MSP85x0 GE has two alignment requirements:
> + * -> skb->data to be cacheline aligned (32 byte)
> + * -> IP header alignment to 16 bytes
> + *
> + * The latter is not implemented. So, that results in an extra copy 
> +on
> + * the Rx. This is a big performance hog. For the former case, the
> + * dev_alloc_skb() has been replaced with msp85x0_ge_alloc_skb(). The 
> +size
> + * requested is calculated:
> + *
> + * Ethernet Frame Size : 1518
> + * Ethernet Header     : 14
> + * Future MSP85x0 change for IP header alignment : 2
> + *
> + * Hence, we allocate (1518 + 14 + 2+ 64) = 1580 bytes.  For IP 
> +header
> + * alignment, we use skb_reserve().
> + */
> +#define ALIGNED_RX_SKB_ADDR(addr) \
> +     ((((unsigned long)(addr) + (32UL - 1UL)) \
> +     & ~(32UL - 1UL)) - (unsigned long)(addr)) #define 
> +msp85x0_ge_alloc_skb(__length, __gfp_flags) \
> +({      struct sk_buff *__skb; \
> +     __skb = alloc_skb((__length),(__gfp_flags)); \
> +     if(__skb){ \
> +             int __offset = (int) ALIGNED_RX_SKB_ADDR(__skb->data); \
> +             if(__offset) \
> +                     skb_reserve(__skb, __offset); \
> +     } \
> +     __skb; \
> +})

Please use a function for msp85x0_ge_alloc_skb.

> +
> +int increment_tx_pkt_count(unsigned int port)

static

> +{
> +     unsigned int flags;
> +     spin_lock_irqsave(&msp85x0_lock,flags);
> +     if(port==0){

CodingStyle: if (port == 0) {
(or 'if (!port)')

> +             port0_pkt_count++;
> +     }
> +     else{

        } else {

> +             port1_pkt_count++;      
> +     }
> +     spin_unlock_irqrestore(&msp85x0_lock,flags);

increment_tx_pkt_count() is only issued from msp85x0_ge_tx_queue(), itself only 
issued from msp85x0_ge_start_xmit and xmit handlers run with irq enabled. The 
save/restore is not needed.

> +     return 0;

Useless return.

> +}
> +
> +unsigned int get_tx_pkt_count(unsigned int port)

static

> +{
> +     unsigned int flags,pktCount,tx_pending_pkt;

Mixed case can be avoided here.

> +     spin_lock_irqsave(&msp85x0_lock,flags);

See above.

> +     tx_pending_pkt=MSP85x0_GE_READ(MSP85x0_GE_CHANNEL0_TX_DMA_STS + (port 
> << XDMA_PORT_OFFSET));
> +     if(port==0){
> +             pktCount=port0_pkt_count - tx_pending_pkt;
> +     }
> +     else{

Sic.

> +             pktCount=port1_pkt_count - tx_pending_pkt;      
> +     }
> +     spin_unlock_irqrestore(&msp85x0_lock,flags);
> +     return pktCount;        
> +}
> +
> +unsigned int decrement_tx_pkt_count(unsigned int port)

static

[...]
> +void msp85x0_bringup_sequence(int port)

static

> +{
> +     unsigned int reg_data;
> +
> +     /* SDQPF */
> +     reg_data=MSP85x0_GE_READ(MSP85x0_GE_SDQPF_RXFIFO_CTL + (port << 8));

reg_data = MSP85x0_GE_READ(MSP85x0_GE_...


[...]
> +static void msp85x0_ge_gmii_config(int port_num) {
[...]
> +     if (phy_reg & 0x8000) {
> +             if (phy_reg & 0x2000) {
> +                     /* Full Duplex and 1000 Mbps */
> +                     MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
> +                                     (port_num << MII_PORT_OFFSET)), 0x201);
> +             }  else {
> +                     /* Half Duplex and 1000 Mbps */
> +                     MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
> +                                     (port_num << MII_PORT_OFFSET)), 0x2201);
> +                     }
> +     }

:o(
                mode = (phy_reg & 0x2000) ? 0x0201 : 0x2201;
                MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
                                 (port_num << MII_PORT_OFFSET)), mode);
> +     if (phy_reg & 0x4000) {
> +             if (phy_reg & 0x2000) {
> +                     /* Full Duplex and 100 Mbps */
> +                     MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
> +                                     (port_num << MII_PORT_OFFSET)), 0x100);
> +             } else {
> +                     /* Half Duplex and 100 Mbps */
> +                     MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
> +                                     (port_num << MII_PORT_OFFSET)), 0x2100);
> +             }
> +     }

I'd bet you could make a single MSP85x0_GE_WRITE for this one and the two above.

[...]
> +static void msp85x0_ge_update_afx(msp85x0_ge_port_info * 
> +msp85x0_ge_eth) {
> +     int port = msp85x0_ge_eth->port_num;
> +     unsigned int i;
> +     volatile unsigned long reg_data = 0;

Mantra: volatile is not the answer.

Now, what's the question ? :o)


[...]
> +static void msp85x0_ge_tx_timeout_task(struct net_device *netdev) {
> +     msp85x0_ge_port_info *msp85x0_ge_eth = netdev_priv(netdev);
> +     int port = msp85x0_ge_eth->port_num;
> +
> +     printk("MSP85x0 GE: Transmit timed out. Resetting ... \n");

Missing KERN_xyz

[...]
> +static int msp85x0_ge_change_mtu(struct net_device *netdev, int 
> +new_mtu) {
[...]
> +     if (netif_running(netdev)) {
> +             msp85x0_ge_eth_stop(netdev);
> +             MSP85x0_GE_WRITE((MSP85x0_GE_RMAC_MAX_FRAME_LEN + 
> +(msp85x0_ge_eth->port_num << MAC_PORT_OFFSET)), new_mtu + 2); // for 
> +the padded bytes
> +
> +             if (msp85x0_ge_eth_reopen(netdev) != 0) {
> +                     printk(KERN_ERR
> +                            "%s: Fatal error on opening device\n",
> +                            netdev->name);
> +                     spin_unlock_irqrestore(&msp85x0_ge_eth->lock, flags);
> +                     return -1;

Please use -Esomething.

You may consider goto to balance spin_{lock/unlock}.

[...]
> +static irqreturn_t msp85x0_ge_sequoia_int_handler(int irq, void *dev_id,
> +     struct pt_regs *regs)
> +{
> +     struct net_device *netdev = (struct net_device *) dev_id;

Useless cast from void *.

[...]
> +        /* Handle the Rx next */
> +        if (eth_int_cause1 & (RX_INT << (port_num * 16))) {
> +           clear_xdma_interrupts(port_num,RX_INT);
> +              if (netif_rx_schedule_prep(netdev)) {
> +                msp85x0_ge_disable_rx_int(port_num); 
> +                   __netif_rx_schedule(netdev);                              
>         

The indentation went badly wrong.

[...]
> +static int msp85x0_ge_open(struct net_device *netdev) {
> +     msp85x0_ge_port_info *msp85x0_ge_eth = netdev_priv(netdev);
> +     unsigned int port_num = msp85x0_ge_eth->port_num;
> +     unsigned int irq;
> +     int retval;       
> +
> +     spin_lock_irq(&(msp85x0_ge_eth->lock));
> +
> +     if (msp85x0_ge_eth_open(netdev) != MSP85x0_OK) {
> +             spin_unlock_irq(&(msp85x0_ge_eth->lock));
> +             printk("%s: Error opening interface \n", netdev->name);

Missing KERN_xyz

> +             free_irq(netdev->irq, netdev);

request_irq() comes later.

> +             return -EBUSY;

Why not propagate an usual return status code from msp85x0_ge_eth_open ?

> +     }
> +
> +     spin_unlock_irq(&(msp85x0_ge_eth->lock));
> +
> +        irq = MSP85x0_ETH_PORT_IRQ;
> +
> +     retval = request_irq(irq, INTERRUPT_HANDLER,
> +                  SA_SAMPLE_RANDOM | SA_SHIRQ, netdev->name, netdev);
> +
> +     if (retval != 0) {
> +             printk(KERN_ERR "Cannot assign IRQ number to MSP85x0 GE \n");
> +             return -1;

- msp85x0_ge_eth_open() should be balanced a bit (stop queuing for instance) ;
- no need to throw away retval.

(some sleep needed here, sorry)

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

<Prev in Thread] Current Thread [Next in Thread>