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

2008-02-10 Thread 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 [EMAIL PROTECTED]
---
 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

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

2008-02-10 Thread 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 - 1.22
 +++ enc28j60.c 7 Feb 2008 11:07:20 -
 @@ -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.

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

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

2008-02-07 Thread 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

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

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

2008-02-07 Thread 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 - 1.22
+++ enc28j60.c 7 Feb 2008 11:07:20 -
@@ -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?

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

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

2008-02-06 Thread 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

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

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

2008-02-06 Thread 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 [EMAIL PROTECTED]
---
 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;
}
+