Patchwork [1/1] ethernet driver: dm9000: davicom: Upgrade the driver to suit for all DM9000 series chips

login
register
mail settings
Submitter Joseph CHANG
Date March 25, 2013, 8:04 a.m.
Message ID <1364198682-1589-1-git-send-email-josright123@gmail.com>
Download mbox | patch
Permalink /patch/230585/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Joseph CHANG - March 25, 2013, 8:04 a.m.
From: Joseph CHANG <joseph_chang@davicom.com.tw>

Some necessary modification for DM9000 series chips to be better in operation!

Had tested to all of DM9000 series chips, include DM9000E, DM9000A, DM9000B,
and DM9000C in X86 and ARM embedded Linux system these years.

Signed-off-by: Joseph CHANG <joseph_chang@davicom.com.tw>
---
 drivers/net/ethernet/davicom/dm9000.c |   28 +++++++++++++++++++++++-----
 drivers/net/ethernet/davicom/dm9000.h |    4 +++-
 2 files changed, 26 insertions(+), 6 deletions(-)
David Miller - March 25, 2013, 5:09 p.m.
From: Joseph CHANG <josright123@gmail.com>
Date: Mon, 25 Mar 2013 16:04:42 +0800

>  #define CARDNAME	"dm9000"
> -#define DRV_VERSION	"1.31"
> +/* [KERN-ADD-1](upgrade) */
> +#define DRV_VERSION	"1.39"
 ...
> +			/* [KERN-ADD-2] */

These kinds of comments are absolutely inapproprate.
--
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
Jonathan Corbet - March 25, 2013, 8:08 p.m.
On Mon, 25 Mar 2013 16:04:42 +0800
Joseph CHANG <josright123@gmail.com> wrote:

> From: Joseph CHANG <joseph_chang@davicom.com.tw>
> 
> Some necessary modification for DM9000 series chips to be better in operation!

Interesting, this had been sent to me privately at about the same time.
For the record, here's what I sent back:

From: Jonathan Corbet <corbet@lwn.net>
To: Allen Huang (黃偉格) <allen_huang@davicom.com.tw>
Subject: Re: Davicom Linux driver
Date: Mon, 25 Mar 2013 09:55:48 -0600
Organization: LWN.net

On Mon, 25 Mar 2013 18:31:35 +0800
Allen Huang (黃偉格) <allen_huang@davicom.com.tw> wrote:

> Please see our Linux Patch in the attachment and we would appreciate any
> comments that you have on our driver and/or whether it is ready to go into
> the kernel. Thanks.   

I'll take a quick look at it.  But the only true way to know whether it's
ready, of course, is to post it to the public lists.

> Some necessary modification for DM9000 series chips to be better in operation!  

The patch will be better received if you say what "better in operation"
actually means.  Are you supporting new hardware, adding new features,
fixing bugs?  Hopefully not more than one of those in a single patch.

> Had tested to all of DM9000 series chips, include DM9000E, DM9000A, DM9000B,
> and DM9000C in X86 and ARM embedded Linux system these years.
> 
> Signed-off-by: Joseph CHANG <joseph_chang@davicom.com.tw>
> ---
>  drivers/net/ethernet/davicom/dm9000.c |   28 +++++++++++++++++++++++-----
>  drivers/net/ethernet/davicom/dm9000.h |    4 +++-
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index 8cdf025..54bdc92 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -47,7 +47,8 @@
>  #define DM9000_PHY		0x40	/* PHY address 0x01 */
>  
>  #define CARDNAME	"dm9000"
> -#define DRV_VERSION	"1.31"
> +/* [KERN-ADD-1](upgrade) */  

I would lose the [KERN-ADD*] comments, those will certainly draw some
grumbling when the patch is posted publicly.  They don't add any value; the
repository will show which changes were made when.

> +#define DRV_VERSION	"1.39"
>  
>  /*
>   * Transmit timeout, default 5 seconds.
> @@ -671,10 +672,16 @@ dm9000_poll_work(struct work_struct *w)
>  			if (netif_msg_link(db))
>  				dm9000_show_carrier(db, new_carrier, nsr);
>  
> -			if (!new_carrier)
> +			/* [KERN-ADD-2] */
> +			if (!new_carrier) {
>  				netif_carrier_off(ndev);
> -			else
> +				netdev_info(ndev, "[%s Ethernet Driver, V%s]: Link-Down!!\n",
> +					CARDNAME, DRV_VERSION);
> +			} else {
>  				netif_carrier_on(ndev);
> +				netdev_info(ndev, "[%s Ethernet Driver, V%s]: Link-Up!!\n",
> +					CARDNAME, DRV_VERSION);
> +			}  

So "better in operation" means more log messages?  This, too, will likely
draw complaints.  That could be a lot of log messages!  The additional
information is also probably unnecessary, netdev_info() should print the
relevant device information.

>  		}
>  	} else
>  		mii_check_media(&db->mii, netif_msg_link(db), 0);
> @@ -794,6 +801,9 @@ dm9000_init_dm9000(struct net_device *dev)
>  			(dev->features & NETIF_F_RXCSUM) ? RCSR_CSUM : 0);
>  
>  	iow(db, DM9000_GPCR, GPCR_GEP_CNTL);	/* Let GPIO0 output */
> +    /* [KERN-ADD-3] */
> +	dm9000_phy_write(dev, 0, 0, 0x8000); /* reset PHY */
> +	mdelay(2);  

Magic constants aren't good, you can't define a symbol for that?

dm900_poll_work() is a workqueue function, right?  So it can sleep.  So you
should almost certainly use msleep() rather than mdelay().

>  	ncr = (db->flags & DM9000_PLATF_EXT_PHY) ? NCR_EXT_PHY : 0;
>  
> @@ -830,6 +840,8 @@ dm9000_init_dm9000(struct net_device *dev)
>  	db->tx_pkt_cnt = 0;
>  	db->queue_pkt_len = 0;
>  	dev->trans_start = jiffies;
> +    /* [KERN-ADD-4]	*/
> +	dm9000_phy_write(dev, 0, 27, 0xE100);  

Again, can we avoid the magic constants?

>  }
>  
>  /* Our watchdog timed out. Called by the networking layer */
> @@ -1181,7 +1193,8 @@ dm9000_open(struct net_device *dev)
>  
>  	/* GPIO0 on pre-activate PHY, Reg 1F is not set by reset */
>  	iow(db, DM9000_GPR, 0);	/* REG_1F bit0 activate phyxcer */
> -	mdelay(1); /* delay needs by DM9000B */
> +	/* [KERN-ADD-5](Enable PHY) */
> +	mdelay(2); /* delay needs by DM9000B */  

You say "Enable PHY" but I see no change that does that?
Again, msleep() would be better here - an open function can certainly
sleep! 

>  	/* Initialize DM9000 board */
>  	dm9000_reset(db);
> @@ -1311,6 +1324,8 @@ dm9000_shutdown(struct net_device *dev)
>  
>  	/* RESET device */
>  	dm9000_phy_write(dev, 0, MII_BMCR, BMCR_RESET);	/* PHY RESET */
> +    /* [KERN-ADD-6](NOTICE)(by)("if (!db->wake_state) ..") */
> +    /* iow(db, DM9000_GPR, 0x01); */
>  	iow(db, DM9000_GPR, 0x01);	/* Power-Down PHY */  

What is the purpose of this change?

>  	iow(db, DM9000_IMR, IMR_PAR);	/* Disable all interrupt */
>  	iow(db, DM9000_RCR, 0x00);	/* Disable RX */
> @@ -1465,6 +1480,8 @@ dm9000_probe(struct platform_device *pdev)
>  	/* fill in parameters for net-dev structure */
>  	ndev->base_addr = (unsigned long)db->io_addr;
>  	ndev->irq	= db->irq_res->start;
> +    /* [KERN-ADD-7] */
> +	netdev_info(ndev, "[dm9] %s ndev->irq=%x\n", __func__, ndev->irq);  

That should be unnecessary; that information is available in
/proc/interrupts, and probably via ethtool too.

>  	/* ensure at least we have a default set of IO routines */
>  	dm9000_set_io(db, iosize);
> @@ -1502,7 +1519,8 @@ dm9000_probe(struct platform_device *pdev)
>  	db->flags |= DM9000_PLATF_SIMPLE_PHY;
>  #endif
>  
> -	dm9000_reset(db);
> +    /* [KERN-ADD-8](SPECIAL WHEN INIT_OF_PROBE)[takeover"dm9000_reset(db)"] */
> +	iow(db, DM9000_NCR, NCR_MAC_LBK | NCR_RST); /* 0x03 */  

What is accomplished by this change?  Should it maybe be a parameter to
dm9000_reset()?

>  	/* try multiple times, DM9000 sometimes gets the read wrong */
>  	for (i = 0; i < 8; i++) {
> diff --git a/drivers/net/ethernet/davicom/dm9000.h b/drivers/net/ethernet/davicom/dm9000.h
> index 55688bd..d644506 100644
> --- a/drivers/net/ethernet/davicom/dm9000.h
> +++ b/drivers/net/ethernet/davicom/dm9000.h
> @@ -69,7 +69,9 @@
>  #define NCR_WAKEEN          (1<<6)
>  #define NCR_FCOL            (1<<4)
>  #define NCR_FDX             (1<<3)
> -#define NCR_LBK             (3<<1)
> +
> +#define NCR_RESERVED        (3<<1)
> +#define NCR_MAC_LBK         (1<<1) /* [kern-add-0] */
>  #define NCR_RST	            (1<<0)
>  
>  #define NSR_SPEED           (1<<7)
> -- 
> 1.7.1
>   
Hope that helps,

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

Patch

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 8cdf025..54bdc92 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -47,7 +47,8 @@ 
 #define DM9000_PHY		0x40	/* PHY address 0x01 */
 
 #define CARDNAME	"dm9000"
-#define DRV_VERSION	"1.31"
+/* [KERN-ADD-1](upgrade) */
+#define DRV_VERSION	"1.39"
 
 /*
  * Transmit timeout, default 5 seconds.
@@ -671,10 +672,16 @@  dm9000_poll_work(struct work_struct *w)
 			if (netif_msg_link(db))
 				dm9000_show_carrier(db, new_carrier, nsr);
 
-			if (!new_carrier)
+			/* [KERN-ADD-2] */
+			if (!new_carrier) {
 				netif_carrier_off(ndev);
-			else
+				netdev_info(ndev, "[%s Ethernet Driver, V%s]: Link-Down!!\n",
+					CARDNAME, DRV_VERSION);
+			} else {
 				netif_carrier_on(ndev);
+				netdev_info(ndev, "[%s Ethernet Driver, V%s]: Link-Up!!\n",
+					CARDNAME, DRV_VERSION);
+			}
 		}
 	} else
 		mii_check_media(&db->mii, netif_msg_link(db), 0);
@@ -794,6 +801,9 @@  dm9000_init_dm9000(struct net_device *dev)
 			(dev->features & NETIF_F_RXCSUM) ? RCSR_CSUM : 0);
 
 	iow(db, DM9000_GPCR, GPCR_GEP_CNTL);	/* Let GPIO0 output */
+    /* [KERN-ADD-3] */
+	dm9000_phy_write(dev, 0, 0, 0x8000); /* reset PHY */
+	mdelay(2);
 
 	ncr = (db->flags & DM9000_PLATF_EXT_PHY) ? NCR_EXT_PHY : 0;
 
@@ -830,6 +840,8 @@  dm9000_init_dm9000(struct net_device *dev)
 	db->tx_pkt_cnt = 0;
 	db->queue_pkt_len = 0;
 	dev->trans_start = jiffies;
+    /* [KERN-ADD-4]	*/
+	dm9000_phy_write(dev, 0, 27, 0xE100);
 }
 
 /* Our watchdog timed out. Called by the networking layer */
@@ -1181,7 +1193,8 @@  dm9000_open(struct net_device *dev)
 
 	/* GPIO0 on pre-activate PHY, Reg 1F is not set by reset */
 	iow(db, DM9000_GPR, 0);	/* REG_1F bit0 activate phyxcer */
-	mdelay(1); /* delay needs by DM9000B */
+	/* [KERN-ADD-5](Enable PHY) */
+	mdelay(2); /* delay needs by DM9000B */
 
 	/* Initialize DM9000 board */
 	dm9000_reset(db);
@@ -1311,6 +1324,8 @@  dm9000_shutdown(struct net_device *dev)
 
 	/* RESET device */
 	dm9000_phy_write(dev, 0, MII_BMCR, BMCR_RESET);	/* PHY RESET */
+    /* [KERN-ADD-6](NOTICE)(by)("if (!db->wake_state) ..") */
+    /* iow(db, DM9000_GPR, 0x01); */
 	iow(db, DM9000_GPR, 0x01);	/* Power-Down PHY */
 	iow(db, DM9000_IMR, IMR_PAR);	/* Disable all interrupt */
 	iow(db, DM9000_RCR, 0x00);	/* Disable RX */
@@ -1465,6 +1480,8 @@  dm9000_probe(struct platform_device *pdev)
 	/* fill in parameters for net-dev structure */
 	ndev->base_addr = (unsigned long)db->io_addr;
 	ndev->irq	= db->irq_res->start;
+    /* [KERN-ADD-7] */
+	netdev_info(ndev, "[dm9] %s ndev->irq=%x\n", __func__, ndev->irq);
 
 	/* ensure at least we have a default set of IO routines */
 	dm9000_set_io(db, iosize);
@@ -1502,7 +1519,8 @@  dm9000_probe(struct platform_device *pdev)
 	db->flags |= DM9000_PLATF_SIMPLE_PHY;
 #endif
 
-	dm9000_reset(db);
+    /* [KERN-ADD-8](SPECIAL WHEN INIT_OF_PROBE)[takeover"dm9000_reset(db)"] */
+	iow(db, DM9000_NCR, NCR_MAC_LBK | NCR_RST); /* 0x03 */
 
 	/* try multiple times, DM9000 sometimes gets the read wrong */
 	for (i = 0; i < 8; i++) {
diff --git a/drivers/net/ethernet/davicom/dm9000.h b/drivers/net/ethernet/davicom/dm9000.h
index 55688bd..d644506 100644
--- a/drivers/net/ethernet/davicom/dm9000.h
+++ b/drivers/net/ethernet/davicom/dm9000.h
@@ -69,7 +69,9 @@ 
 #define NCR_WAKEEN          (1<<6)
 #define NCR_FCOL            (1<<4)
 #define NCR_FDX             (1<<3)
-#define NCR_LBK             (3<<1)
+
+#define NCR_RESERVED        (3<<1)
+#define NCR_MAC_LBK         (1<<1) /* [kern-add-0] */
 #define NCR_RST	            (1<<0)
 
 #define NSR_SPEED           (1<<7)