Re: [patch 2.6.24-git] net/enc28j60: oops fix, low power mode
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 -- 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: oops fix, low power mode
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
--
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: oops fix, low power mode
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.
--
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: oops fix, low power mode
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.) -- 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: oops fix, low power mode
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. -- 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: oops fix, low power mode
David Brownell wrote:
From: David Brownell [EMAIL PROTECTED]
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
--
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: oops fix, low power mode
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 -- 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
[patch 2.6.24-git] net/enc28j60: oops fix, low power mode
From: David Brownell [EMAIL PROTECTED]
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 [EMAIL PROTECTED]
---
drivers/net/enc28j60.c | 54 +
1 files changed, 50 insertions(+), 4 deletions(-)
--- avr.orig/drivers/net/enc28j60.c 2008-02-05 10:04:22.0 -0800
+++ avr/drivers/net/enc28j60.c 2008-02-05 10:50:50.0 -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))
--
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
