[patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Prevent unaligned packet oops on enc28j60 packet RX.

Keep enc28j60 chips in low-power mode when they're not in use.
At typically 120 mA, these chips run hot even when idle.  Low
power mode cuts that power usage by a factor of around 100.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/net/enc28j60.c |   54 +++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 50 insertions(+), 4 deletions(-)

--- avr.orig/drivers/net/enc28j60.c	2008-02-05 10:04:22.000000000 -0800
+++ avr/drivers/net/enc28j60.c	2008-02-05 10:50:50.000000000 -0800
@@ -594,6 +594,43 @@ static void nolock_txfifo_init(struct en
 	nolock_regw_write(priv, ETXNDL, end);
 }
 
+/*
+ * Low power mode shrinks power consumption about 100x, so we'd like
+ * the chip to be in that mode whenever it's inactive.
+ */
+static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
+{
+	int	tmp;
+
+	dev_dbg(&priv->spi->dev, "%s power...\n", is_low ? "low" : "high");
+
+	mutex_lock(&priv->lock);
+	if (is_low) {
+		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
+		for (;;) {
+			tmp = nolock_regb_read(priv, ESTAT);
+			if (!(tmp & ESTAT_RXBUSY))
+				break;
+		}
+		for (;;) {
+			tmp = nolock_regb_read(priv, ECON1);
+			if (!(tmp & ECON1_TXRTS))
+				break;
+		}
+		/* ECON2_VRPS was set during initialization */
+		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
+	} else {
+		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
+		for (;;) {
+			tmp = nolock_regb_read(priv, ESTAT);
+			if (tmp & ESTAT_CLKRDY)
+				break;
+		}
+		/* caller sets ECON1_RXEN */
+	}
+	mutex_unlock(&priv->lock);
+}
+
 static int enc28j60_hw_init(struct enc28j60_net *priv)
 {
 	u8 reg;
@@ -612,8 +649,8 @@ static int enc28j60_hw_init(struct enc28
 	priv->tx_retry_count = 0;
 	priv->max_pk_counter = 0;
 	priv->rxfilter = RXFILTER_NORMAL;
-	/* enable address auto increment */
-	nolock_regb_write(priv, ECON2, ECON2_AUTOINC);
+	/* enable address auto increment and voltage regulator powersave */
+	nolock_regb_write(priv, ECON2, ECON2_AUTOINC | ECON2_VRPS);
 
 	nolock_rxfifo_init(priv, RXSTART_INIT, RXEND_INIT);
 	nolock_txfifo_init(priv, TXSTART_INIT, TXEND_INIT);
@@ -690,7 +727,9 @@ static int enc28j60_hw_init(struct enc28
 
 static void enc28j60_hw_enable(struct enc28j60_net *priv)
 {
-	/* enable interrutps */
+	enc28j60_lowpower(priv, false);
+
+	/* enable interrupts */
 	if (netif_msg_hw(priv))
 		printk(KERN_DEBUG DRV_NAME ": %s() enabling interrupts.\n",
 			__FUNCTION__);
@@ -717,6 +756,8 @@ static void enc28j60_hw_disable(struct e
 	nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
 	priv->hw_enable = false;
 	mutex_unlock(&priv->lock);
+
+	enc28j60_lowpower(priv, true);
 }
 
 static int
@@ -734,6 +775,8 @@ enc28j60_setlink(struct net_device *ndev
 						"hw_reset() failed\n");
 				ret = -EINVAL;
 			}
+			enc28j60_lowpower(priv, true);
+
 		} else {
 			if (netif_msg_link(priv))
 				dev_warn(&ndev->dev,
@@ -900,7 +943,7 @@ static void enc28j60_hw_rx(struct net_de
 		if (RSV_GETBIT(rxstat, RSV_LENCHECKERR))
 			ndev->stats.rx_frame_errors++;
 	} else {
-		skb = dev_alloc_skb(len);
+		skb = dev_alloc_skb(len + NET_IP_ALIGN);
 		if (!skb) {
 			if (netif_msg_rx_err(priv))
 				dev_err(&ndev->dev,
@@ -908,6 +951,7 @@ static void enc28j60_hw_rx(struct net_de
 			ndev->stats.rx_dropped++;
 		} else {
 			skb->dev = ndev;
+			skb_reserve(skb, NET_IP_ALIGN);
 			/* copy the packet from the receive buffer */
 			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
 					len, skb_put(skb, len));
@@ -1536,6 +1580,8 @@ static int __devinit enc28j60_probe(stru
 	dev->watchdog_timeo = TX_TIMEOUT;
 	SET_ETHTOOL_OPS(dev, &enc28j60_ethtool_ops);
 
+	enc28j60_lowpower(priv, true);
+
 	ret = register_netdev(dev);
 	if (ret) {
 		if (netif_msg_probe(priv))

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[Claudio Lanconelli]

David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
>
> Prevent unaligned packet oops on enc28j60 packet RX.
>   
How can I reproduce the unaligned packet oops? Did you use any utilities 
to force this condition?
> Keep enc28j60 chips in low-power mode when they're not in use.
> At typically 120 mA, these chips run hot even when idle.  Low
> power mode cuts that power usage by a factor of around 100.
>   
Good idea, but with your patch applied, after some ifconfig down - 
ifconfig up cycle, the
enc28j60 is left in an unknown state and it doesn' Rx/Tx anything.
In such cases If I dump the counters with ifconfig I got rx error 
counter > 0 and the RX and TX packets counters are freezed.
Actually I don't know what causes the freeze, it needs investigation. 
The cause can be the rx error condition or the power down/up commands.
May be receiving packets while it's going to wakeup causes problems.

Can you split the patch in 2 parts, unaligned rx and power save?

> +/*
> + * Low power mode shrinks power consumption about 100x, so we'd like
> + * the chip to be in that mode whenever it's inactive.
> + */
> +static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
> +{
> +	int	tmp;
> +
> +	dev_dbg(&priv->spi->dev, "%s power...\n", is_low ? "low" : "high");
>   
use
if(netif_msg_drv(priv)) printk(...
instead of dev_dbg(), please.
Doing so we can switch on/off messages runtime using ethtool.

> +
> +	mutex_lock(&priv->lock);
> +	if (is_low) {
> +		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
> +		for (;;) {
> +			tmp = nolock_regb_read(priv, ESTAT);
> +			if (!(tmp & ESTAT_RXBUSY))
> +				break;
> +		}
>   
Avoid infinite waiting loops, please.
Look at enc28j60_phy_read() for example.
> +		for (;;) {
> +			tmp = nolock_regb_read(priv, ECON1);
> +			if (!(tmp & ECON1_TXRTS))
> +				break;
> +		}
>   
idem
> +		/* ECON2_VRPS was set during initialization */
> +		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
> +	} else {
> +		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
> +		for (;;) {
> +			tmp = nolock_regb_read(priv, ESTAT);
> +			if (tmp & ESTAT_CLKRDY)
> +				break;
> +		}
>   
idem

Re: [patch 2.6.24-git] net/enc28j60: oops fix

[David Brownell]

> > Prevent unaligned packet oops on enc28j60 packet RX.
>
> How can I reproduce the unaligned packet oops?

Use any architecture that doesn't support unaligned accesses
in kernel space.  I used AVR32 for this (it's got a pretty
solid SPI master controller driver). ARM isn't immune either,
and there are others.  That's why NET_IP_ALIGN exists.


> Did you use any utilities to force this condition?

"ping" ... the first RX packet oopsed.


> Can you split the patch in 2 parts, unaligned rx and power save?

Below is: unaligned access.

- Dave


=========== CUT HERE
Prevent oops on enc28j60 packet RX:  make sure buffers are aligned.
Not all architectures support unaligned accesses in kernel space.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/net/enc28j60.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/net/enc28j60.c	2008-02-06 09:29:00.000000000 -0800
+++ b/drivers/net/enc28j60.c	2008-02-06 09:30:03.000000000 -0800
@@ -900,7 +900,7 @@ static void enc28j60_hw_rx(struct net_de
 		if (RSV_GETBIT(rxstat, RSV_LENCHECKERR))
 			ndev->stats.rx_frame_errors++;
 	} else {
-		skb = dev_alloc_skb(len);
+		skb = dev_alloc_skb(len + NET_IP_ALIGN);
 		if (!skb) {
 			if (netif_msg_rx_err(priv))
 				dev_err(&ndev->dev,
@@ -908,6 +908,7 @@ static void enc28j60_hw_rx(struct net_de
 			ndev->stats.rx_dropped++;
 		} else {
 			skb->dev = ndev;
+			skb_reserve(skb, NET_IP_ALIGN);
 			/* copy the packet from the receive buffer */
 			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
 					len, skb_put(skb, len));

Re: [patch 2.6.24-git] net/enc28j60: oops fix

[Claudio Lanconelli]

David Brownell wrote:
> Prevent oops on enc28j60 packet RX:  make sure buffers are aligned.
> Not all architectures support unaligned accesses in kernel space.
>
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>   

Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>

> ---
>  drivers/net/enc28j60.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletion(-)
>
> --- a/drivers/net/enc28j60.c	2008-02-06 09:29:00.000000000 -0800
> +++ b/drivers/net/enc28j60.c	2008-02-06 09:30:03.000000000 -0800
> @@ -900,7 +900,7 @@ static void enc28j60_hw_rx(struct net_de
>  		if (RSV_GETBIT(rxstat, RSV_LENCHECKERR))
>  			ndev->stats.rx_frame_errors++;
>  	} else {
> -		skb = dev_alloc_skb(len);
> +		skb = dev_alloc_skb(len + NET_IP_ALIGN);
>  		if (!skb) {
>  			if (netif_msg_rx_err(priv))
>  				dev_err(&ndev->dev,
> @@ -908,6 +908,7 @@ static void enc28j60_hw_rx(struct net_de
>  			ndev->stats.rx_dropped++;
>  		} else {
>  			skb->dev = ndev;
> +			skb_reserve(skb, NET_IP_ALIGN);
>  			/* copy the packet from the receive buffer */
>  			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
>  					len, skb_put(skb, len));
>
>
>   

[RESEND/patch 2.6.25-rc2-git] net/enc28j60: oops fix

[David Brownell]

Prevent oops on enc28j60 packet RX:  make sure buffers are aligned.
Not all architectures support unaligned accesses in kernel space.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
---
Seems the enc28j60 patches didn't get picked up, ergo resend...
they're not in net-2.6.git, even this oops fix.

 drivers/net/enc28j60.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/net/enc28j60.c	2008-02-06 09:29:00.000000000 -0800
+++ b/drivers/net/enc28j60.c	2008-02-06 09:30:03.000000000 -0800
@@ -900,7 +900,7 @@ static void enc28j60_hw_rx(struct net_de
 		if (RSV_GETBIT(rxstat, RSV_LENCHECKERR))
 			ndev->stats.rx_frame_errors++;
 	} else {
-		skb = dev_alloc_skb(len);
+		skb = dev_alloc_skb(len + NET_IP_ALIGN);
 		if (!skb) {
 			if (netif_msg_rx_err(priv))
 				dev_err(&ndev->dev,
@@ -908,6 +908,7 @@ static void enc28j60_hw_rx(struct net_de
 			ndev->stats.rx_dropped++;
 		} else {
 			skb->dev = ndev;
+			skb_reserve(skb, NET_IP_ALIGN);
 			/* copy the packet from the receive buffer */
 			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
 					len, skb_put(skb, len));

[RE(*2)SEND/patch 2.6.25-rc2-git] net/enc28j60: oops fix

[David Brownell]

Prevent oops on enc28j60 packet RX:  make sure buffers are aligned.
Not all architectures support unaligned accesses in kernel space.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
---
Seems the enc28j60 patches didn't get picked up, ergo (2x) resend...
they're not in net-2.6.git, even this oops fix (which I'd expect
would be prioritized for 2.6.25)

 drivers/net/enc28j60.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/net/enc28j60.c	2008-02-06 09:29:00.000000000 -0800
+++ b/drivers/net/enc28j60.c	2008-02-06 09:30:03.000000000 -0800
@@ -900,7 +900,7 @@ static void enc28j60_hw_rx(struct net_de
 		if (RSV_GETBIT(rxstat, RSV_LENCHECKERR))
 			ndev->stats.rx_frame_errors++;
 	} else {
-		skb = dev_alloc_skb(len);
+		skb = dev_alloc_skb(len + NET_IP_ALIGN);
 		if (!skb) {
 			if (netif_msg_rx_err(priv))
 				dev_err(&ndev->dev,
@@ -908,6 +908,7 @@ static void enc28j60_hw_rx(struct net_de
 			ndev->stats.rx_dropped++;
 		} else {
 			skb->dev = ndev;
+			skb_reserve(skb, NET_IP_ALIGN);
 			/* copy the packet from the receive buffer */
 			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
 					len, skb_put(skb, len));

Re: [RE(*2)SEND/patch 2.6.25-rc2-git] net/enc28j60: oops fix

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Tue, 4 Mar 2008 17:17:32 -0800

> Seems the enc28j60 patches didn't get picked up, ergo (2x) resend...
> they're not in net-2.6.git, even this oops fix (which I'd expect
> would be prioritized for 2.6.25)

We prioritize them about the same as bootup hard hangs with EHCI.

> Subject: [RE(*2)SEND/patch 2.6.25-rc2-git] net/enc28j60: oops fix
> 
> Prevent oops on enc28j60 packet RX:  make sure buffers are aligned.
> Not all architectures support unaligned accesses in kernel space.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>

Applied.

Re: [RE(*2)SEND/patch 2.6.25-rc2-git] net/enc28j60: oops fix

[David Brownell]

On Wednesday 05 March 2008, David Miller wrote:
> From: David Brownell <david-b@pacbell.net>
> Date: Tue, 4 Mar 2008 17:17:32 -0800
> 
> > Seems the enc28j60 patches didn't get picked up, ergo (2x) resend...
> > they're not in net-2.6.git, even this oops fix (which I'd expect
> > would be prioritized for 2.6.25)
> 
> We prioritize them about the same as bootup hard hangs with EHCI.

I don't know about any of those that are unfixed ...


> > Subject: [RE(*2)SEND/patch 2.6.25-rc2-git] net/enc28j60: oops fix
> > 
> > Prevent oops on enc28j60 packet RX:  make sure buffers are aligned.
> > Not all architectures support unaligned accesses in kernel space.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> > Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
> 
> Applied.

Thanks.

Re: [patch 2.6.24-git] net/enc28j60: low power mode

[David Brownell]

> > Keep enc28j60 chips in low-power mode when they're not in use.
> > At typically 120 mA, these chips run hot even when idle.  Low
> > power mode cuts that power usage by a factor of around 100.
>
> Good idea, but with your patch applied, after some ifconfig down - 
> ifconfig up cycle, the
> enc28j60 is left in an unknown state and it doesn' Rx/Tx anything.

The driver seemed to already have some goofage there:

  # ifconfig eth1 up
  net eth1: link down
  net eth1: link down
  net eth1: normal mode
  net eth1: multicast mode
  net eth1: multicast mode
  #

I'd normally expect it not to go down when told to go up, and
then only to do the multicast thing once ...


> In such cases If I dump the counters with ifconfig I got rx error 
> counter > 0 and the RX and TX packets counters are freezed.
> Actually I don't know what causes the freeze, it needs investigation. 
> The cause can be the rx error condition or the power down/up commands.
> May be receiving packets while it's going to wakeup causes problems.

The enc28j60_setlink() was odd too.  It insists the link be down
before changing duplex, then brings the link up ... so I had to
put it down again to maintain the "lowpower if not up" invariant.

But the way it brings the link up is to enc28j60_hw_init(), which
it also does when bringing the link up.  So there's no need to
bring the link up when changing duplex ...


> > +/*
> > + * Low power mode shrinks power consumption about 100x, so we'd like
> > + * the chip to be in that mode whenever it's inactive.
> > + */
> > +static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
> > +{
> > +	int	tmp;
> > +
> > +	dev_dbg(&priv->spi->dev, "%s power...\n", is_low ? "low" : "high");
>
> use
> if(netif_msg_drv(priv)) ...
> Doing so we can switch on/off messages runtime using ethtool.

OK, although there's still a role for "-DDEBUG" compile-time
mesage removal.


> printk(...  instead of dev_dbg(), please.

Actually, I had to resist sending a patch converting this driver
to stop abusing printk in all those locations it should have been
using dev_*() messaging instead.

And to use the dev_*() messaging correctly ... the above shouldn't
have said "net eth1", but either "eth1:" or "enc28j60 spi1.0".

The general policy in the kernel is to avoid using printk for driver
messaging.  Couple messages to the hardware, so that when there are
two instances it's easy to tell which chip is affected.  That happens
for free with the "driver and device" prefix of the dev_*() messages.
	
In absense of interface renaming, network devices sometimes use "eth1"
style message prefixes ... after a previous message coupling that to
the particular hardware.

This driver also abuses __FUNCTION__ (general policy:  don't use it)
and underuses the "-DDEBUG" compile-time string removal.


> > +
> > +	mutex_lock(&priv->lock);
> > +	if (is_low) {
> > +		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
> > +		for (;;) {
> > +			tmp = nolock_regb_read(priv, ESTAT);
> > +			if (!(tmp & ESTAT_RXBUSY))
> > +				break;
> > +		}
> >   
> Avoid infinite waiting loops, please.
> Look at enc28j60_phy_read() for example.

Then there should be a single routine for all such busy-wait loops,
so each such usage doesn't need to be open-coded.  Less space, and
more obviously correct.  I'll add one and make phy_read() use it too.


> > +		for (;;) {
> > +			tmp = nolock_regb_read(priv, ECON1);
> > +			if (!(tmp & ECON1_TXRTS))
> > +				break;
> > +		}
> >   
> idem
> > +		/* ECON2_VRPS was set during initialization */
> > +		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
> > +	} else {
> > +		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
> > +		for (;;) {
> > +			tmp = nolock_regb_read(priv, ESTAT);
> > +			if (tmp & ESTAT_CLKRDY)
> > +				break;
> > +		}
> >   
> idem
>

Re: [patch 2.6.24-git] net/enc28j60: low power mode

[Claudio Lanconelli]

David Brownell wrote:
> The driver seemed to already have some goofage there:
>
>   # ifconfig eth1 up
>   net eth1: link down
>   net eth1: link down
>   net eth1: normal mode
>   net eth1: multicast mode
>   net eth1: multicast mode
>   #
>
>   
Without low power patch I have:

# ifconfig eth0 up
net eth0: link down
net eth0: normal mode
net eth0: multicast mode
net eth0: multicast mode
# net eth0: link up - Half duplex

> I'd normally expect it not to go down when told to go up, and
> then only to do the multicast thing once ...
>   
multicast is called by network stacks, no control by the driver. The driver
just print message.
I don't know why enc28j60_set_multicast_list() are called more than once.

When you do an ifconfig up the driver reset the chip, so you see link down
before link up message.

>
>   
>> In such cases If I dump the counters with ifconfig I got rx error 
>> counter > 0 and the RX and TX packets counters are freezed.
>> Actually I don't know what causes the freeze, it needs investigation. 
>> The cause can be the rx error condition or the power down/up commands.
>> May be receiving packets while it's going to wakeup causes problems.
>>     
>
> The enc28j60_setlink() was odd too.  It insists the link be down
> before changing duplex, then brings the link up ... so I had to
> put it down again to maintain the "lowpower if not up" invariant.
>
> But the way it brings the link up is to enc28j60_hw_init(), which
> it also does when bringing the link up.  So there's no need to
> bring the link up when changing duplex ...
>
>
>   
I don't follow you anymore.
To change duplex mode the link must be down.
enc28j60_setlink() reinitialize the chip with new duplex mode,
enc28j60_hw_init() never brings link up.

I propose you to add set_lowpower(true) in the enc28j60_probe() and in 
the enc28j60_net_close() after enc28j60_hw_disable().
Probably we don't need to set_lowpower(false) in enc28j60_net_open() since
it performs a soft reset with enc28j60_hw_init().
Do you agree?


>>
>> use
>> if(netif_msg_drv(priv)) ...
>> Doing so we can switch on/off messages runtime using ethtool.
>>     
>
> OK, although there's still a role for "-DDEBUG" compile-time
> mesage removal.
>
>   
It's ok to use

if(netif_msg_drv(priv)) dev_dbv(...


> This driver also abuses __FUNCTION__ (general policy:  don't use it)
>   
Why?
> Then there should be a single routine for all such busy-wait loops,
> so each such usage doesn't need to be open-coded.  Less space, and
> more obviously correct.  I'll add one and make phy_read() use it too.
>
>   
Ok

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[David Brownell]

On Wednesday 06 February 2008, Claudio Lanconelli wrote:
> Good idea, but with your patch applied, after some ifconfig down - 
> ifconfig up cycle, the
> enc28j60 is left in an unknown state and it doesn' Rx/Tx anything.

How long did that take?  I did about four dozen

	ifconfig eth1 up
	sleep 3
	ifconfig eth1 down

cycles ... it worked fine.  The "sleep" was to let the link
negotiation complete.

- Dave

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[Claudio Lanconelli]

David Brownell wrote:
> How long did that take?  I did about four dozen
>
> 	ifconfig eth1 up
> 	sleep 3
> 	ifconfig eth1 down
>
> cycles ... it worked fine.  The "sleep" was to let the link
> negotiation complete.
>
>   
After a couple of :

ifconfig eth0 down
(wait just 1 second)
ifconfig eth0 up

the network is frozen.

If I do another
ifconfig eth0 down
(wait just 1 second)
ifconfig eth0 up

restarts.
It's random, no rule.

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[David Brownell]

On Thursday 07 February 2008, Claudio Lanconelli wrote:
> David Brownell wrote:
> > How long did that take?  I did about four dozen
> >
> > 	ifconfig eth1 up
> > 	sleep 3
> > 	ifconfig eth1 down
> >
> > cycles ... it worked fine.  The "sleep" was to let the link
> > negotiation complete.
> >
> >   
> After a couple of :
> 
> ifconfig eth0 down
> (wait just 1 second)
> ifconfig eth0 up
> 
> the network is frozen.
> 
> If I do another
> ifconfig eth0 down
> (wait just 1 second)
> ifconfig eth0 up
> 
> restarts.
> It's random, no rule.

I write a shell loop to do that, and added a "ping -c2" too.
If that was done before the "sleep 1" no packets flowed.
Afterwards, no problem -- ever. 

(And outside the loop, "ethool -s eth1 duplex full".)

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[Claudio Lanconelli]

David Brownell wrote:

>> > and in  the enc28j60_net_close() after enc28j60_hw_disable().
>> > Probably we don't need to set_lowpower(false) in enc28j60_net_open() since
>> > it performs a soft reset with enc28j60_hw_init() (not sure).
>>     
>
> The current patch sets the device in low power mode in hw_disable(),
> and takes it out of that mode in hw_enable().  I can move them; and
> the only "soft" thing about this chip's reset is when it starts from
> a protocol command not the reset command.
>
>   
I want to mean the reset software command. It's functionally
equivalent to hardware system reset, but it seems to need exit low
power mode to work flawlessly.
I have tried your latest patch. Only after the following change it
works fine (no more rx errors during ifconfig up).
I added enc28j60_lowpower(false) just before enc28j60_hw_init()

@@ -1318,8 +1347,9 @@
         }
         return -EADDRNOTAVAIL;
     }
-    /* Reset the hardware here */
+    /* Reset the hardware here (and take it out of low power mode) */
     enc28j60_hw_disable(priv);
+    enc28j60_lowpower(priv, false);
     if (!enc28j60_hw_init(priv)) {
         if (netif_msg_ifup(priv))
             dev_err(&dev->dev, "hw_reset() failed\n");

With this addition you can add Acked-by line.
Thank you.
>> After a couple of :
>>
>> ifconfig eth0 down
>> (wait just 1 second)
>> ifconfig eth0 up
>>
>> the network is frozen.
>>
>> If I do another
>> ifconfig eth0 down
>> (wait just 1 second)
>> ifconfig eth0 up
>>
>> restarts.
>> It's random, no rule.
>>     
>
> I write a shell loop to do that, and added a "ping -c2" too.
> If that was done before the "sleep 1" no packets flowed.
> Afterwards, no problem -- ever. 
>
> (And outside the loop, "ethool -s eth1 duplex full".)
>
>
>   
I forgot to tell that during my test I have a web server running on the 
board and
a client continuously requesting a page.

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[David Brownell]

On Monday 11 February 2008, Claudio Lanconelli wrote:
> I have tried your latest patch. Only after the following change it
> works fine (no more rx errors during ifconfig up).

Hmm, what chip rev do you have?  Different errata and all.
ISTR mine is rev4; so, not the most current, but not the
oldest version either.


> I added enc28j60_lowpower(false) just before enc28j60_hw_init()

Hmm, I'd have expected it would go best *before* that, but
what you include below shows it going *after* ...

If there's some problem where reset doesn't work correctly
in low power mode, who knows what else would need manual
resetting.

 
> @@ -1318,8 +1347,9 @@
>          }
>          return -EADDRNOTAVAIL;
>      }
> -    /* Reset the hardware here */
> +    /* Reset the hardware here (and take it out of low power mode) */
>      enc28j60_hw_disable(priv);
> +    enc28j60_lowpower(priv, false);
>      if (!enc28j60_hw_init(priv)) {
>          if (netif_msg_ifup(priv))
>              dev_err(&dev->dev, "hw_reset() failed\n");
>
> With this addition you can add Acked-by line.

Better yet, since I can't reproduce the problem, why don't
you just update my latest patch with the relevant version
of this tweak, and then forward it as "From: " me and with
both our signoffs.  That's the usual way to cope with this
type of tweaking.  (Not all updates to your driver should
need your signoff, but then most patches shouldn't need
very many iterations either.)

- Dave

Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode

[Claudio Lanconelli]

David Brownell wrote:
> On Monday 11 February 2008, Claudio Lanconelli wrote:
>   
>> I have tried your latest patch. Only after the following change it
>> works fine (no more rx errors during ifconfig up).
>>     
>
> Hmm, what chip rev do you have?  Different errata and all.
> ISTR mine is rev4; so, not the most current, but not the
> oldest version either.
>   
I use the same revision.

>> I added enc28j60_lowpower(false) just before enc28j60_hw_init()
>>     
>
> Hmm, I'd have expected it would go best *before* that, but
> what you include below shows it going *after* ...
>
> If there's some problem where reset doesn't work correctly
> in low power mode, who knows what else would need manual
> resetting.
>
>   
I don't know why it needs low power resume before reset.
I read in the errata tath clkready bit after reset doesn't work reliably.
May be something related to this, but undocumented.

> Better yet, since I can't reproduce the problem, why don't
> you just update my latest patch with the relevant version
> of this tweak, and then forward it as "From: " me and with
> both our signoffs.  That's the usual way to cope with this
> type of tweaking.  (Not all updates to your driver should
> need your signoff, but then most patches shouldn't need
> very many iterations either.)
>   
Done

Re: [patch 2.6.24-git] net/enc28j60: low power mode

[David Brownell]

On Wednesday 06 February 2008, Claudio Lanconelli wrote:
> > +		for (;;) {
> > +			tmp = nolock_regb_read(priv, ESTAT);
> > +			if (!(tmp & ESTAT_RXBUSY))
> > +				break;
> > +		}
> >   
> Avoid infinite waiting loops, please.
> Look at enc28j60_phy_read() for example.

Updated patch is appended.

==========	CUT HERE
Keep enc28j60 chips in low-power mode when they're not in use.
At typically 120 mA, these chips run hot even when idle; this
low power mode cuts that power usage by a factor of around 100.

This version provides a generic routine to poll a register until
its masked value equals some value ... e.g. bit set or cleared.
It's basically what the previous wait_phy_ready() did, but this
version is generalized to support the handshaking needed to
enter and exit low power mode.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/net/enc28j60.c |   70 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 55 insertions(+), 15 deletions(-)

--- a/drivers/net/enc28j60.c
+++ b/drivers/net/enc28j60.c
@@ -400,26 +400,31 @@ enc28j60_packet_write(struct enc28j60_ne
 	mutex_unlock(&priv->lock);
 }
 
-/*
- * Wait until the PHY operation is complete.
- */
-static int wait_phy_ready(struct enc28j60_net *priv)
+static unsigned long msec20_to_jiffies;
+
+static int poll_ready(struct enc28j60_net *priv, u8 reg, u8 mask, u8 val)
 {
-	unsigned long timeout = jiffies + 20 * HZ / 1000;
-	int ret = 1;
+	unsigned long timeout = jiffies + msec20_to_jiffies;
 
 	/* 20 msec timeout read */
-	while (nolock_regb_read(priv, MISTAT) & MISTAT_BUSY) {
+	while ((nolock_regb_read(priv, reg) & mask) != val) {
 		if (time_after(jiffies, timeout)) {
 			if (netif_msg_drv(priv))
-				printk(KERN_DEBUG DRV_NAME
-					": PHY ready timeout!\n");
-			ret = 0;
-			break;
+				dev_dbg(&priv->spi->dev,
+					"reg %02x ready timeout!\n", reg);
+			return -ETIMEDOUT;
 		}
 		cpu_relax();
 	}
-	return ret;
+	return 0;
+}
+
+/*
+ * Wait until the PHY operation is complete.
+ */
+static int wait_phy_ready(struct enc28j60_net *priv)
+{
+	return poll_ready(priv, MISTAT, MISTAT_BUSY, 0) ? 0 : 1;
 }
 
 /*
@@ -594,6 +599,31 @@ static void nolock_txfifo_init(struct en
 	nolock_regw_write(priv, ETXNDL, end);
 }
 
+/*
+ * Low power mode shrinks power consumption about 100x, so we'd like
+ * the chip to be in that mode whenever it's inactive.
+ */
+static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
+{
+	if (netif_msg_drv(priv))
+		dev_dbg(&priv->spi->dev, "%s power...\n",
+				is_low ? "low" : "high");
+
+	mutex_lock(&priv->lock);
+	if (is_low) {
+		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
+		poll_ready(priv, ESTAT, ESTAT_RXBUSY, 0);
+		poll_ready(priv, ECON1, ECON1_TXRTS, 0);
+		/* ECON2_VRPS was set during initialization */
+		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
+	} else {
+		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
+		poll_ready(priv, ESTAT, ESTAT_CLKRDY, ESTAT_CLKRDY);
+		/* caller sets ECON1_RXEN */
+	}
+	mutex_unlock(&priv->lock);
+}
+
 static int enc28j60_hw_init(struct enc28j60_net *priv)
 {
 	u8 reg;
@@ -612,8 +642,8 @@ static int enc28j60_hw_init(struct enc28
 	priv->tx_retry_count = 0;
 	priv->max_pk_counter = 0;
 	priv->rxfilter = RXFILTER_NORMAL;
-	/* enable address auto increment */
-	nolock_regb_write(priv, ECON2, ECON2_AUTOINC);
+	/* enable address auto increment and voltage regulator powersave */
+	nolock_regb_write(priv, ECON2, ECON2_AUTOINC | ECON2_VRPS);
 
 	nolock_rxfifo_init(priv, RXSTART_INIT, RXEND_INIT);
 	nolock_txfifo_init(priv, TXSTART_INIT, TXEND_INIT);
@@ -690,7 +720,9 @@ static int enc28j60_hw_init(struct enc28
 
 static void enc28j60_hw_enable(struct enc28j60_net *priv)
 {
-	/* enable interrutps */
+	enc28j60_lowpower(priv, false);
+
+	/* enable interrupts */
 	if (netif_msg_hw(priv))
 		printk(KERN_DEBUG DRV_NAME ": %s() enabling interrupts.\n",
 			__FUNCTION__);
@@ -717,6 +749,8 @@ static void enc28j60_hw_disable(struct e
 	nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
 	priv->hw_enable = false;
 	mutex_unlock(&priv->lock);
+
+	enc28j60_lowpower(priv, true);
 }
 
 static int
@@ -734,6 +768,8 @@ enc28j60_setlink(struct net_device *ndev
 						"hw_reset() failed\n");
 				ret = -EINVAL;
 			}
+			enc28j60_lowpower(priv, true);
+
 		} else {
 			if (netif_msg_link(priv))
 				dev_warn(&ndev->dev,
@@ -1537,6 +1573,8 @@ static int __devinit enc28j60_probe(stru
 	dev->watchdog_timeo = TX_TIMEOUT;
 	SET_ETHTOOL_OPS(dev, &enc28j60_ethtool_ops);
 
+	enc28j60_lowpower(priv, true);
+
 	ret = register_netdev(dev);
 	if (ret) {
 		if (netif_msg_probe(priv))
@@ -1582,6 +1620,8 @@ static struct spi_driver enc28j60_driver
 
 static int __init enc28j60_init(void)
 {
+	msec20_to_jiffies = msecs_to_jiffies(20);
+
 	return spi_register_driver(&enc28j60_driver);
 }
 

Re: [patch 2.6.24-git] net/enc28j60: low power mode

[Claudio Lanconelli]

Sorry,
let me repeat what I said in previous mail.
I propose you to add set_lowpower(true) in the enc28j60_probe() and in 
the enc28j60_net_close() after enc28j60_hw_disable().
Probably we don't need to set_lowpower(false) in enc28j60_net_open() since
it performs a soft reset with enc28j60_hw_init() (not sure).

Furthermore, as you suggested, we also need to remove hw_init() from the 
setlink()
because hw_init() is called when we bring link up.

--- enc28j60.c 20 Dec 2007 10:47:01 -0000 1.22
+++ enc28j60.c 7 Feb 2008 11:07:20 -0000
@@ -740,12 +740,6 @@
if (!priv->hw_enable) {
if (autoneg == AUTONEG_DISABLE && speed == SPEED_10) {
priv->full_duplex = (duplex == DUPLEX_FULL);
- if (!enc28j60_hw_init(priv)) {
- if (netif_msg_drv(priv))
- dev_err(&ndev->dev,
- "hw_reset() failed\n");
- ret = -EINVAL;
- }
} else {
if (netif_msg_link(priv))
dev_warn(&ndev->dev,

Can you update your low power patch with these modifications?

Re: [patch 2.6.24-git] net/enc28j60: low power mode

[David Brownell]

On Thursday 07 February 2008, Claudio Lanconelli wrote:
> Sorry,
> let me repeat what I said in previous mail.
> I propose you to add set_lowpower(true) in the enc28j60_probe() 

As the current patch does...


> and in  the enc28j60_net_close() after enc28j60_hw_disable().
> Probably we don't need to set_lowpower(false) in enc28j60_net_open() since
> it performs a soft reset with enc28j60_hw_init() (not sure).

The current patch sets the device in low power mode in hw_disable(),
and takes it out of that mode in hw_enable().  I can move them; and
the only "soft" thing about this chip's reset is when it starts from
a protocol command not the reset command.


> Furthermore, as you suggested, we also need to remove hw_init() from the 
> setlink()
> because hw_init() is called when we bring link up.
> 
> --- enc28j60.c 20 Dec 2007 10:47:01 -0000 1.22
> +++ enc28j60.c 7 Feb 2008 11:07:20 -0000
> @@ -740,12 +740,6 @@
> if (!priv->hw_enable) {
> if (autoneg == AUTONEG_DISABLE && speed == SPEED_10) {
> priv->full_duplex = (duplex == DUPLEX_FULL);
> - if (!enc28j60_hw_init(priv)) {
> - if (netif_msg_drv(priv))
> - dev_err(&ndev->dev,
> - "hw_reset() failed\n");
> - ret = -EINVAL;
> - }

Right.  Without the patch mangling presumably done by your mailer.  ;)


> } else {
> if (netif_msg_link(priv))
> dev_warn(&ndev->dev,
> 
> Can you update your low power patch with these modifications?
> 

Done -- see my next patch.

Re: [patch 2.6.24-git] net/enc28j60: low power mode

[David Brownell]

Keep enc28j60 chips in low-power mode when they're not in use.
At typically 120 mA, these chips run hot even when idle; this
low power mode cuts that power usage by a factor of around 100.

This version provides a generic routine to poll a register until
its masked value equals some value ... e.g. bit set or cleared.
It's basically what the previous wait_phy_ready() did, but this
version is generalized to support the handshaking needed to
enter and exit low power mode.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/net/enc28j60.c |   81 ++++++++++++++++++++++++++++++++++---------------
 1 files changed, 57 insertions(+), 24 deletions(-)

--- a/drivers/net/enc28j60.c
+++ b/drivers/net/enc28j60.c
@@ -400,26 +400,31 @@ enc28j60_packet_write(struct enc28j60_ne
 	mutex_unlock(&priv->lock);
 }
 
-/*
- * Wait until the PHY operation is complete.
- */
-static int wait_phy_ready(struct enc28j60_net *priv)
+static unsigned long msec20_to_jiffies;
+
+static int poll_ready(struct enc28j60_net *priv, u8 reg, u8 mask, u8 val)
 {
-	unsigned long timeout = jiffies + 20 * HZ / 1000;
-	int ret = 1;
+	unsigned long timeout = jiffies + msec20_to_jiffies;
 
 	/* 20 msec timeout read */
-	while (nolock_regb_read(priv, MISTAT) & MISTAT_BUSY) {
+	while ((nolock_regb_read(priv, reg) & mask) != val) {
 		if (time_after(jiffies, timeout)) {
 			if (netif_msg_drv(priv))
-				printk(KERN_DEBUG DRV_NAME
-					": PHY ready timeout!\n");
-			ret = 0;
-			break;
+				dev_dbg(&priv->spi->dev,
+					"reg %02x ready timeout!\n", reg);
+			return -ETIMEDOUT;
 		}
 		cpu_relax();
 	}
-	return ret;
+	return 0;
+}
+
+/*
+ * Wait until the PHY operation is complete.
+ */
+static int wait_phy_ready(struct enc28j60_net *priv)
+{
+	return poll_ready(priv, MISTAT, MISTAT_BUSY, 0) ? 0 : 1;
 }
 
 /*
@@ -594,6 +599,32 @@ static void nolock_txfifo_init(struct en
 	nolock_regw_write(priv, ETXNDL, end);
 }
 
+/*
+ * Low power mode shrinks power consumption about 100x, so we'd like
+ * the chip to be in that mode whenever it's inactive.  (However, we
+ * can't stay in lowpower mode during suspend with WOL active.)
+ */
+static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
+{
+	if (netif_msg_drv(priv))
+		dev_dbg(&priv->spi->dev, "%s power...\n",
+				is_low ? "low" : "high");
+
+	mutex_lock(&priv->lock);
+	if (is_low) {
+		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
+		poll_ready(priv, ESTAT, ESTAT_RXBUSY, 0);
+		poll_ready(priv, ECON1, ECON1_TXRTS, 0);
+		/* ECON2_VRPS was set during initialization */
+		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
+	} else {
+		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
+		poll_ready(priv, ESTAT, ESTAT_CLKRDY, ESTAT_CLKRDY);
+		/* caller sets ECON1_RXEN */
+	}
+	mutex_unlock(&priv->lock);
+}
+
 static int enc28j60_hw_init(struct enc28j60_net *priv)
 {
 	u8 reg;
@@ -612,8 +643,8 @@ static int enc28j60_hw_init(struct enc28
 	priv->tx_retry_count = 0;
 	priv->max_pk_counter = 0;
 	priv->rxfilter = RXFILTER_NORMAL;
-	/* enable address auto increment */
-	nolock_regb_write(priv, ECON2, ECON2_AUTOINC);
+	/* enable address auto increment and voltage regulator powersave */
+	nolock_regb_write(priv, ECON2, ECON2_AUTOINC | ECON2_VRPS);
 
 	nolock_rxfifo_init(priv, RXSTART_INIT, RXEND_INIT);
 	nolock_txfifo_init(priv, TXSTART_INIT, TXEND_INIT);
@@ -690,7 +721,7 @@ static int enc28j60_hw_init(struct enc28
 
 static void enc28j60_hw_enable(struct enc28j60_net *priv)
 {
-	/* enable interrutps */
+	/* enable interrupts */
 	if (netif_msg_hw(priv))
 		printk(KERN_DEBUG DRV_NAME ": %s() enabling interrupts.\n",
 			__FUNCTION__);
@@ -726,15 +757,12 @@ enc28j60_setlink(struct net_device *ndev
 	int ret = 0;
 
 	if (!priv->hw_enable) {
-		if (autoneg == AUTONEG_DISABLE && speed == SPEED_10) {
+		/* link is in low power mode now; duplex setting
+		 * will take effect on next enc28j60_hw_init().
+		 */
+		if (autoneg == AUTONEG_DISABLE && speed == SPEED_10)
 			priv->full_duplex = (duplex == DUPLEX_FULL);
-			if (!enc28j60_hw_init(priv)) {
-				if (netif_msg_drv(priv))
-					dev_err(&ndev->dev,
-						"hw_reset() failed\n");
-				ret = -EINVAL;
-			}
-		} else {
+		else {
 			if (netif_msg_link(priv))
 				dev_warn(&ndev->dev,
 					"unsupported link setting\n");
@@ -1307,7 +1335,7 @@ static int enc28j60_net_open(struct net_
 		}
 		return -EADDRNOTAVAIL;
 	}
-	/* Reset the hardware here */
+	/* Reset the hardware here (and take it out of low power mode) */
 	enc28j60_hw_disable(priv);
 	if (!enc28j60_hw_init(priv)) {
 		if (netif_msg_ifup(priv))
@@ -1337,6 +1365,7 @@ static int enc28j60_net_close(struct net
 		printk(KERN_DEBUG DRV_NAME ": %s() enter\n", __FUNCTION__);
 
 	enc28j60_hw_disable(priv);
+	enc28j60_lowpower(priv, true);
 	netif_stop_queue(dev);
 
 	return 0;
@@ -1537,6 +1566,8 @@ static int __devinit enc28j60_probe(stru
 	dev->watchdog_timeo = TX_TIMEOUT;
 	SET_ETHTOOL_OPS(dev, &enc28j60_ethtool_ops);
 
+	enc28j60_lowpower(priv, true);
+
 	ret = register_netdev(dev);
 	if (ret) {
 		if (netif_msg_probe(priv))
@@ -1582,6 +1613,8 @@ static struct spi_driver enc28j60_driver
 
 static int __init enc28j60_init(void)
 {
+	msec20_to_jiffies = msecs_to_jiffies(20);
+
 	return spi_register_driver(&enc28j60_driver);
 }
 

Re: [patch 2.6.24-git] net/enc28j60: section fix

[David Brownell]

Minor bugfixes to the enc28j60 driver ... wrong section marking and
indentation, and bogus use of spi_bus_type.  (Setting the bus type
of a driver is a SPI framework responsibility.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Bonus patch.

 drivers/net/enc28j60.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

--- a/drivers/net/enc28j60.c
+++ b/drivers/net/enc28j60.c
@@ -1555,7 +1555,7 @@ error_alloc:
 	return ret;
 }
 
-static int enc28j60_remove(struct spi_device *spi)
+static int __devexit enc28j60_remove(struct spi_device *spi)
 {
 	struct enc28j60_net *priv = dev_get_drvdata(&spi->dev);
 
@@ -1572,9 +1572,8 @@ static int enc28j60_remove(struct spi_de
 static struct spi_driver enc28j60_driver = {
 	.driver = {
 		   .name = DRV_NAME,
-		   .bus = &spi_bus_type,
 		   .owner = THIS_MODULE,
-		   },
+	 },
 	.probe = enc28j60_probe,
 	.remove = __devexit_p(enc28j60_remove),
 };

Re: [patch 2.6.24-git] net/enc28j60: section fix

[Claudio Lanconelli]

David Brownell wrote:
> Minor bugfixes to the enc28j60 driver ... wrong section marking and
> indentation, and bogus use of spi_bus_type.  (Setting the bus type
> of a driver is a SPI framework responsibility.)
>
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>   

Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
> ---
> Bonus patch.
>
>  drivers/net/enc28j60.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> --- a/drivers/net/enc28j60.c
> +++ b/drivers/net/enc28j60.c
> @@ -1555,7 +1555,7 @@ error_alloc:
>  	return ret;
>  }
>  
> -static int enc28j60_remove(struct spi_device *spi)
> +static int __devexit enc28j60_remove(struct spi_device *spi)
>  {
>  	struct enc28j60_net *priv = dev_get_drvdata(&spi->dev);
>  
> @@ -1572,9 +1572,8 @@ static int enc28j60_remove(struct spi_de
>  static struct spi_driver enc28j60_driver = {
>  	.driver = {
>  		   .name = DRV_NAME,
> -		   .bus = &spi_bus_type,
>  		   .owner = THIS_MODULE,
> -		   },
> +	 },
>  	.probe = enc28j60_probe,
>  	.remove = __devexit_p(enc28j60_remove),
>  };
>
>
>   

[RESEND/patch 2.6.25-rc2-git] net/enc28j60: low power mode

[David Brownell]

Keep enc28j60 chips in low-power mode when they're not in use.
At typically 120 mA, these chips run hot even when idle; this
low power mode cuts that power usage by a factor of around 100.

This version provides a generic routine to poll a register until
its masked value equals some value ... e.g. bit set or cleared.
It's basically what the previous wait_phy_ready() did, but this
version is generalized to support the handshaking needed to
enter and exit low power mode.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
---
 drivers/net/enc28j60.c |   82 ++++++++++++++++++++++++++++++++++---------------
 1 files changed, 58 insertions(+), 24 deletions(-)

--- a/drivers/net/enc28j60.c
+++ b/drivers/net/enc28j60.c
@@ -400,26 +400,31 @@ enc28j60_packet_write(struct enc28j60_ne
 	mutex_unlock(&priv->lock);
 }
 
-/*
- * Wait until the PHY operation is complete.
- */
-static int wait_phy_ready(struct enc28j60_net *priv)
+static unsigned long msec20_to_jiffies;
+
+static int poll_ready(struct enc28j60_net *priv, u8 reg, u8 mask, u8 val)
 {
-	unsigned long timeout = jiffies + 20 * HZ / 1000;
-	int ret = 1;
+	unsigned long timeout = jiffies + msec20_to_jiffies;
 
 	/* 20 msec timeout read */
-	while (nolock_regb_read(priv, MISTAT) & MISTAT_BUSY) {
+	while ((nolock_regb_read(priv, reg) & mask) != val) {
 		if (time_after(jiffies, timeout)) {
 			if (netif_msg_drv(priv))
-				printk(KERN_DEBUG DRV_NAME
-					": PHY ready timeout!\n");
-			ret = 0;
-			break;
+				dev_dbg(&priv->spi->dev,
+					"reg %02x ready timeout!\n", reg);
+			return -ETIMEDOUT;
 		}
 		cpu_relax();
 	}
-	return ret;
+	return 0;
+}
+
+/*
+ * Wait until the PHY operation is complete.
+ */
+static int wait_phy_ready(struct enc28j60_net *priv)
+{
+	return poll_ready(priv, MISTAT, MISTAT_BUSY, 0) ? 0 : 1;
 }
 
 /*
@@ -594,6 +599,32 @@ static void nolock_txfifo_init(struct en
 	nolock_regw_write(priv, ETXNDL, end);
 }
 
+/*
+ * Low power mode shrinks power consumption about 100x, so we'd like
+ * the chip to be in that mode whenever it's inactive.  (However, we
+ * can't stay in lowpower mode during suspend with WOL active.)
+ */
+static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
+{
+	if (netif_msg_drv(priv))
+		dev_dbg(&priv->spi->dev, "%s power...\n",
+				is_low ? "low" : "high");
+
+	mutex_lock(&priv->lock);
+	if (is_low) {
+		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
+		poll_ready(priv, ESTAT, ESTAT_RXBUSY, 0);
+		poll_ready(priv, ECON1, ECON1_TXRTS, 0);
+		/* ECON2_VRPS was set during initialization */
+		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
+	} else {
+		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
+		poll_ready(priv, ESTAT, ESTAT_CLKRDY, ESTAT_CLKRDY);
+		/* caller sets ECON1_RXEN */
+	}
+	mutex_unlock(&priv->lock);
+}
+
 static int enc28j60_hw_init(struct enc28j60_net *priv)
 {
 	u8 reg;
@@ -612,8 +643,8 @@ static int enc28j60_hw_init(struct enc28
 	priv->tx_retry_count = 0;
 	priv->max_pk_counter = 0;
 	priv->rxfilter = RXFILTER_NORMAL;
-	/* enable address auto increment */
-	nolock_regb_write(priv, ECON2, ECON2_AUTOINC);
+	/* enable address auto increment and voltage regulator powersave */
+	nolock_regb_write(priv, ECON2, ECON2_AUTOINC | ECON2_VRPS);
 
 	nolock_rxfifo_init(priv, RXSTART_INIT, RXEND_INIT);
 	nolock_txfifo_init(priv, TXSTART_INIT, TXEND_INIT);
@@ -690,7 +721,7 @@ static int enc28j60_hw_init(struct enc28
 
 static void enc28j60_hw_enable(struct enc28j60_net *priv)
 {
-	/* enable interrutps */
+	/* enable interrupts */
 	if (netif_msg_hw(priv))
 		printk(KERN_DEBUG DRV_NAME ": %s() enabling interrupts.\n",
 			__FUNCTION__);
@@ -726,15 +757,12 @@ enc28j60_setlink(struct net_device *ndev
 	int ret = 0;
 
 	if (!priv->hw_enable) {
-		if (autoneg == AUTONEG_DISABLE && speed == SPEED_10) {
+		/* link is in low power mode now; duplex setting
+		 * will take effect on next enc28j60_hw_init().
+		 */
+		if (autoneg == AUTONEG_DISABLE && speed == SPEED_10)
 			priv->full_duplex = (duplex == DUPLEX_FULL);
-			if (!enc28j60_hw_init(priv)) {
-				if (netif_msg_drv(priv))
-					dev_err(&ndev->dev,
-						"hw_reset() failed\n");
-				ret = -EINVAL;
-			}
-		} else {
+		else {
 			if (netif_msg_link(priv))
 				dev_warn(&ndev->dev,
 					"unsupported link setting\n");
@@ -1307,7 +1335,8 @@ static int enc28j60_net_open(struct net_
 		}
 		return -EADDRNOTAVAIL;
 	}
-	/* Reset the hardware here */
+	/* Reset the hardware here (and take it out of low power mode) */
+	enc28j60_lowpower(priv, false);
 	enc28j60_hw_disable(priv);
 	if (!enc28j60_hw_init(priv)) {
 		if (netif_msg_ifup(priv))
@@ -1337,6 +1366,7 @@ static int enc28j60_net_close(struct net
 		printk(KERN_DEBUG DRV_NAME ": %s() enter\n", __FUNCTION__);
 
 	enc28j60_hw_disable(priv);
+	enc28j60_lowpower(priv, true);
 	netif_stop_queue(dev);
 
 	return 0;
@@ -1537,6 +1567,8 @@ static int __devinit enc28j60_probe(stru
 	dev->watchdog_timeo = TX_TIMEOUT;
 	SET_ETHTOOL_OPS(dev, &enc28j60_ethtool_ops);
 
+	enc28j60_lowpower(priv, true);
+
 	ret = register_netdev(dev);
 	if (ret) {
 		if (netif_msg_probe(priv))
@@ -1582,6 +1614,8 @@ static struct spi_driver enc28j60_driver
 
 static int __init enc28j60_init(void)
 {
+	msec20_to_jiffies = msecs_to_jiffies(20);
+
 	return spi_register_driver(&enc28j60_driver);
 }
 

[RESEND/patch 2.6.25-rc2-git] net/enc28j60: section fix

[David Brownell]

Minor bugfixes to the enc28j60 driver ... wrong section marking,
indentation, and bogus use of spi_bus_type.  There's still major
overuse of printk; essentially every place it's used should switch
to dev_*() messaging primitives.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
---
 drivers/net/enc28j60.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

--- a/drivers/net/enc28j60.c	2008-01-29 11:07:14.000000000 -0800
+++ b/drivers/net/enc28j60.c	2008-01-29 12:43:18.000000000 -0800
@@ -1555,7 +1555,7 @@ error_alloc:
 	return ret;
 }
 
-static int enc28j60_remove(struct spi_device *spi)
+static int __devexit enc28j60_remove(struct spi_device *spi)
 {
 	struct enc28j60_net *priv = dev_get_drvdata(&spi->dev);
 
@@ -1572,9 +1572,8 @@ static int enc28j60_remove(struct spi_de
 static struct spi_driver enc28j60_driver = {
 	.driver = {
 		   .name = DRV_NAME,
-		   .bus = &spi_bus_type,
 		   .owner = THIS_MODULE,
-		   },
+	 },
 	.probe = enc28j60_probe,
 	.remove = __devexit_p(enc28j60_remove),
 };

[RESEND/patch 2.6.25] net/enc28j60: section fix

[David Brownell]

Minor bugfixes to the enc28j60 driver ... wrong section marking,
indentation, and bogus use of spi_bus_type. 

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
---
 drivers/net/enc28j60.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

--- g26.orig/drivers/net/enc28j60.c	2008-02-22 19:48:13.000000000 -0800
+++ g26/drivers/net/enc28j60.c	2008-02-22 19:48:14.000000000 -0800
@@ -1556,7 +1556,7 @@ error_alloc:
 	return ret;
 }
 
-static int enc28j60_remove(struct spi_device *spi)
+static int __devexit enc28j60_remove(struct spi_device *spi)
 {
 	struct enc28j60_net *priv = dev_get_drvdata(&spi->dev);
 
@@ -1573,9 +1573,8 @@ static int enc28j60_remove(struct spi_de
 static struct spi_driver enc28j60_driver = {
 	.driver = {
 		   .name = DRV_NAME,
-		   .bus = &spi_bus_type,
 		   .owner = THIS_MODULE,
-		   },
+	 },
 	.probe = enc28j60_probe,
 	.remove = __devexit_p(enc28j60_remove),
 };

[RESEND/patch 2.6.25] net/enc28j60: low power mode

[David Brownell]

Keep enc28j60 chips in low-power mode when they're not in use.
At typically 120 mA, these chips run hot even when idle; this
low power mode cuts that power usage by a factor of around 100.

This version provides a generic routine to poll a register until
its masked value equals some value ... e.g. bit set or cleared.
It's basically what the previous wait_phy_ready() did, but this
version is generalized to support the handshaking needed to
enter and exit low power mode.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Claudio Lanconelli <lanconelli.claudio@eptar.com>
---
 drivers/net/enc28j60.c |   82 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 24 deletions(-)

--- g26.orig/drivers/net/enc28j60.c	2008-02-22 19:48:14.000000000 -0800
+++ g26/drivers/net/enc28j60.c	2008-02-22 19:48:21.000000000 -0800
@@ -400,26 +400,31 @@ enc28j60_packet_write(struct enc28j60_ne
 	mutex_unlock(&priv->lock);
 }
 
-/*
- * Wait until the PHY operation is complete.
- */
-static int wait_phy_ready(struct enc28j60_net *priv)
+static unsigned long msec20_to_jiffies;
+
+static int poll_ready(struct enc28j60_net *priv, u8 reg, u8 mask, u8 val)
 {
-	unsigned long timeout = jiffies + 20 * HZ / 1000;
-	int ret = 1;
+	unsigned long timeout = jiffies + msec20_to_jiffies;
 
 	/* 20 msec timeout read */
-	while (nolock_regb_read(priv, MISTAT) & MISTAT_BUSY) {
+	while ((nolock_regb_read(priv, reg) & mask) != val) {
 		if (time_after(jiffies, timeout)) {
 			if (netif_msg_drv(priv))
-				printk(KERN_DEBUG DRV_NAME
-					": PHY ready timeout!\n");
-			ret = 0;
-			break;
+				dev_dbg(&priv->spi->dev,
+					"reg %02x ready timeout!\n", reg);
+			return -ETIMEDOUT;
 		}
 		cpu_relax();
 	}
-	return ret;
+	return 0;
+}
+
+/*
+ * Wait until the PHY operation is complete.
+ */
+static int wait_phy_ready(struct enc28j60_net *priv)
+{
+	return poll_ready(priv, MISTAT, MISTAT_BUSY, 0) ? 0 : 1;
 }
 
 /*
@@ -594,6 +599,32 @@ static void nolock_txfifo_init(struct en
 	nolock_regw_write(priv, ETXNDL, end);
 }
 
+/*
+ * Low power mode shrinks power consumption about 100x, so we'd like
+ * the chip to be in that mode whenever it's inactive.  (However, we
+ * can't stay in lowpower mode during suspend with WOL active.)
+ */
+static void enc28j60_lowpower(struct enc28j60_net *priv, bool is_low)
+{
+	if (netif_msg_drv(priv))
+		dev_dbg(&priv->spi->dev, "%s power...\n",
+				is_low ? "low" : "high");
+
+	mutex_lock(&priv->lock);
+	if (is_low) {
+		nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
+		poll_ready(priv, ESTAT, ESTAT_RXBUSY, 0);
+		poll_ready(priv, ECON1, ECON1_TXRTS, 0);
+		/* ECON2_VRPS was set during initialization */
+		nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
+	} else {
+		nolock_reg_bfclr(priv, ECON2, ECON2_PWRSV);
+		poll_ready(priv, ESTAT, ESTAT_CLKRDY, ESTAT_CLKRDY);
+		/* caller sets ECON1_RXEN */
+	}
+	mutex_unlock(&priv->lock);
+}
+
 static int enc28j60_hw_init(struct enc28j60_net *priv)
 {
 	u8 reg;
@@ -612,8 +643,8 @@ static int enc28j60_hw_init(struct enc28
 	priv->tx_retry_count = 0;
 	priv->max_pk_counter = 0;
 	priv->rxfilter = RXFILTER_NORMAL;
-	/* enable address auto increment */
-	nolock_regb_write(priv, ECON2, ECON2_AUTOINC);
+	/* enable address auto increment and voltage regulator powersave */
+	nolock_regb_write(priv, ECON2, ECON2_AUTOINC | ECON2_VRPS);
 
 	nolock_rxfifo_init(priv, RXSTART_INIT, RXEND_INIT);
 	nolock_txfifo_init(priv, TXSTART_INIT, TXEND_INIT);
@@ -690,7 +721,7 @@ static int enc28j60_hw_init(struct enc28
 
 static void enc28j60_hw_enable(struct enc28j60_net *priv)
 {
-	/* enable interrutps */
+	/* enable interrupts */
 	if (netif_msg_hw(priv))
 		printk(KERN_DEBUG DRV_NAME ": %s() enabling interrupts.\n",
 			__FUNCTION__);
@@ -726,15 +757,12 @@ enc28j60_setlink(struct net_device *ndev
 	int ret = 0;
 
 	if (!priv->hw_enable) {
-		if (autoneg == AUTONEG_DISABLE && speed == SPEED_10) {
+		/* link is in low power mode now; duplex setting
+		 * will take effect on next enc28j60_hw_init().
+		 */
+		if (autoneg == AUTONEG_DISABLE && speed == SPEED_10)
 			priv->full_duplex = (duplex == DUPLEX_FULL);
-			if (!enc28j60_hw_init(priv)) {
-				if (netif_msg_drv(priv))
-					dev_err(&ndev->dev,
-						"hw_reset() failed\n");
-				ret = -EINVAL;
-			}
-		} else {
+		else {
 			if (netif_msg_link(priv))
 				dev_warn(&ndev->dev,
 					"unsupported link setting\n");
@@ -1307,7 +1335,8 @@ static int enc28j60_net_open(struct net_
 		}
 		return -EADDRNOTAVAIL;
 	}
-	/* Reset the hardware here */
+	/* Reset the hardware here (and take it out of low power mode) */
+	enc28j60_lowpower(priv, false);
 	enc28j60_hw_disable(priv);
 	if (!enc28j60_hw_init(priv)) {
 		if (netif_msg_ifup(priv))
@@ -1337,6 +1366,7 @@ static int enc28j60_net_close(struct net
 		printk(KERN_DEBUG DRV_NAME ": %s() enter\n", __FUNCTION__);
 
 	enc28j60_hw_disable(priv);
+	enc28j60_lowpower(priv, true);
 	netif_stop_queue(dev);
 
 	return 0;
@@ -1537,6 +1567,8 @@ static int __devinit enc28j60_probe(stru
 	dev->watchdog_timeo = TX_TIMEOUT;
 	SET_ETHTOOL_OPS(dev, &enc28j60_ethtool_ops);
 
+	enc28j60_lowpower(priv, true);
+
 	ret = register_netdev(dev);
 	if (ret) {
 		if (netif_msg_probe(priv))
@@ -1581,6 +1613,8 @@ static struct spi_driver enc28j60_driver
 
 static int __init enc28j60_init(void)
 {
+	msec20_to_jiffies = msecs_to_jiffies(20);
+
 	return spi_register_driver(&enc28j60_driver);
 }