[PATCH] b44: power down PHY when interface down

[PATCH] b44: power down PHY when interface down

[Török Edvin]

When the interface is down (or driver removed), the BroadCom 44xx card remains
powered on, and both its MAC and PHY is using up power.
This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
is halted, and does a partial chip reset turns off the activity LEDs too.

Applies to 2.6.22-rc6, or current git head.

Tested on a Broadcom BCM4401-B0 card, it saves ~0.5W (measured using powertop).

Signed-off-by: Torok Edwin <edwintorok@gmail.com>
---
 b44.c |   25 ++++++++++++++++++++-----
  1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index 879a2ff..00d0f57 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -113,6 +113,8 @@ static void b44_init_rings(struct b44 *);
 #define B44_FULL_RESET		1
 #define B44_FULL_RESET_SKIP_PHY	2
 #define B44_PARTIAL_RESET	3
+#define B44_CHIP_RESET_FULL     4
+#define B44_CHIP_RESET_PARTIAL  5

 static void b44_init_hw(struct b44 *, int);

@@ -1283,7 +1285,7 @@ static void b44_clear_stats(struct b44 *bp)
 }

 /* bp->lock is held. */
-static void b44_chip_reset(struct b44 *bp)
+static void b44_chip_reset(struct b44 *bp, int reset_kind)
 {
 	if (ssb_is_core_up(bp)) {
 		bw32(bp, B44_RCV_LAZY, 0);
@@ -1307,6 +1309,10 @@ static void b44_chip_reset(struct b44 *bp)

 	b44_clear_stats(bp);

+	if (reset_kind == B44_CHIP_RESET_PARTIAL)
+		return;/* don't enable PHY if we are doing a partial reset
+		, we are probably going to power down */
+
 	/* Make PHY accessible. */
 	bw32(bp, B44_MDIO_CTRL, (MDIO_CTRL_PREAMBLE |
 			     (0x0d & MDIO_CTRL_MAXF_MASK)));
@@ -1332,7 +1338,14 @@ static void b44_chip_reset(struct b44 *bp)
 static void b44_halt(struct b44 *bp)
 {
 	b44_disable_ints(bp);
-	b44_chip_reset(bp);
+	/* reset PHY */
+	b44_phy_reset(bp);
+	/* power down PHY */
+	printk(KERN_INFO PFX "%s: powering down PHY\n", bp->dev->name);
+	bw32(bp, B44_MAC_CTRL, MAC_CTRL_PHY_PDOWN);
+	/* now reset the chip, but without enabling the MAC&PHY
+	 * part of it. This has to be done _after_ we shut down the PHY */
+	b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
 }

 /* bp->lock is held. */
@@ -1376,7 +1389,7 @@ static void b44_init_hw(struct b44 *bp, int reset_kind)
 {
 	u32 val;

-	b44_chip_reset(bp);
+	b44_chip_reset(bp, B44_CHIP_RESET_FULL);
 	if (reset_kind == B44_FULL_RESET) {
 		b44_phy_reset(bp);
 		b44_setup_phy(bp);
@@ -1430,7 +1443,7 @@ static int b44_open(struct net_device *dev)

 	err = request_irq(dev->irq, b44_interrupt, IRQF_SHARED, dev->name, dev);
 	if (unlikely(err < 0)) {
-		b44_chip_reset(bp);
+		b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
 		b44_free_rings(bp);
 		b44_free_consistent(bp);
 		goto out;
@@ -2250,7 +2263,7 @@ static int __devinit b44_init_one(struct pci_dev *pdev,
 	/* Chip reset provides power to the b44 MAC & PCI cores, which
 	 * is necessary for MAC register access.
 	 */
-	b44_chip_reset(bp);
+	b44_chip_reset(bp, B44_CHIP_RESET_FULL);

 	printk(KERN_INFO "%s: Broadcom 4400 10/100BaseT Ethernet ", dev->name);
 	for (i = 0; i < 6; i++)
@@ -2284,6 +2297,7 @@ static void __devexit b44_remove_one(struct pci_dev *pdev)
 	free_netdev(dev);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
 	pci_set_drvdata(pdev, NULL);
 }

@@ -2312,6 +2326,7 @@ static int b44_suspend(struct pci_dev *pdev,
pm_message_t state)
 		b44_setup_wol(bp);
 	}
 	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
 	return 0;
 }

Re: [PATCH] b44: power down PHY when interface down

[Matthew Garrett]

On Sat, Jun 30, 2007 at 02:47:35PM +0300, Török Edvin wrote:
> When the interface is down (or driver removed), the BroadCom 44xx card 
> remains
> powered on, and both its MAC and PHY is using up power.
> This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
> is halted, and does a partial chip reset turns off the activity LEDs too.

Do you still get link beat detection when the phy is powered down?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] b44: power down PHY when interface down

[Arjan van de Ven]

Matthew Garrett wrote:
> On Sat, Jun 30, 2007 at 02:47:35PM +0300, Török Edvin wrote:
>> When the interface is down (or driver removed), the BroadCom 44xx card 
>> remains
>> powered on, and both its MAC and PHY is using up power.
>> This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
>> is halted, and does a partial chip reset turns off the activity LEDs too.
> 
> Do you still get link beat detection when the phy is powered down?
> 
does that matter?
If the interface is down, nic drivers aren't expected to detect 
link... if userspace wants to find link status it should have the 
interface up.

Re: [PATCH] b44: power down PHY when interface down

[Matthew Garrett]

On Sat, Jun 30, 2007 at 07:44:59AM -0700, Arjan van de Ven wrote:
> Matthew Garrett wrote:
> >Do you still get link beat detection when the phy is powered down?
> >
> does that matter?
> If the interface is down, nic drivers aren't expected to detect 
> link... if userspace wants to find link status it should have the 
> interface up.

If that's the policy, why do we leave this up to individual drivers? 
Right now most of them let you make the query regardless of interface 
state.

I'd agree that there's a need for a state where we power down as much as 
possible (even at the cost of functionality), but where possible it 
would also be nice to offer a state where the mac is powered down and 
the phy left up. If the existing semantics are confused then it would be 
nice to fix them, too.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] b44: power down PHY when interface down

[Lennert Buytenhek]

On Sat, Jun 30, 2007 at 04:19:23PM +0100, Matthew Garrett wrote:

> I'd agree that there's a need for a state where we power down as much as 
> possible (even at the cost of functionality), but where possible it 
> would also be nice to offer a state where the mac is powered down and 
> the phy left up.

There are PHYs which can detect that someone's on the other end even
when powered down..

Re: [PATCH] b44: power down PHY when interface down

[Török Edvin]

On 6/30/07, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sat, Jun 30, 2007 at 07:44:59AM -0700, Arjan van de Ven wrote:
> > Matthew Garrett wrote:
> > >Do you still get link beat detection when the phy is powered down?

No.
As for link detection on the switch connected to this card:
If I have WoL turned off, then I don't get link beat detection.
If I have WoL turned on, then PHY won't be powered down, and I get
link detection on the switch.

> > >
> > does that matter?
> > If the interface is down, nic drivers aren't expected to detect
> > link... if userspace wants to find link status it should have the
> > interface up.

I don't get link beat detection with the original driver either, when
interface is down
(unless I pass it -a, but then it will bring the interface up).


--Edwin

PM policy, hotplug, power saving (was Re: [PATCH] b44: power down PHY when interface down)

[Jeff Garzik]

Arjan van de Ven wrote:
> Matthew Garrett wrote:
>> Do you still get link beat detection when the phy is powered down?

> does that matter?
> If the interface is down, nic drivers aren't expected to detect link... 
> if userspace wants to find link status it should have the interface up.


Definitely matters.  Switch renegotiation can take a while, and you must 
take into account the common case of interface bouncing (immediate down, 
then up).

Hoards actively complained the few times we experimented with this, 
because of e.g. DHCP's habit of bouncing the interface, which resulted 
in PHY power bouncing, which resulted in negotiation, which resulted in 
an excrutiating wait on various broken or stupid switches.

Overall, this may be classed with other problems of a similar sort:  we 
can power down a PHY, but that removes hotplug capability and extends 
partner/link negotiation time.

Like SATA, we actually want to support BOTH -- active hotplug and PHY 
power-down -- and so this wanders into power management policy.

Give me a knob, and we can program plenty of ethernet|SATA|USB|... 
drivers to power down the PHY and save power.

	Jeff

Re: PM policy, hotplug, power saving (was Re: [PATCH] b44: power down PHY when interface down)

[Stephen Hemminger]

On Sat, 30 Jun 2007 12:42:06 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Arjan van de Ven wrote:
> > Matthew Garrett wrote:
> >> Do you still get link beat detection when the phy is powered down?
> 
> > does that matter?
> > If the interface is down, nic drivers aren't expected to detect link... 
> > if userspace wants to find link status it should have the interface up.
> 
> 
> Definitely matters.  Switch renegotiation can take a while, and you must 
> take into account the common case of interface bouncing (immediate down, 
> then up).
> 
> Hoards actively complained the few times we experimented with this, 
> because of e.g. DHCP's habit of bouncing the interface, which resulted 
> in PHY power bouncing, which resulted in negotiation, which resulted in 
> an excrutiating wait on various broken or stupid switches.
> 
> Overall, this may be classed with other problems of a similar sort:  we 
> can power down a PHY, but that removes hotplug capability and extends 
> partner/link negotiation time.
> 
> Like SATA, we actually want to support BOTH -- active hotplug and PHY 
> power-down -- and so this wanders into power management policy.
> 
> Give me a knob, and we can program plenty of ethernet|SATA|USB|... 
> drivers to power down the PHY and save power.
> 
> 	Jeff

We do have IFF_DORMANT, but almost no driver uses it. And most
certainly, the common applications wouldn't know how to use it.

PM policy, hotplug, power saving and WoL

[Henrique de Moraes Holschuh]

On Sat, 30 Jun 2007, Jeff Garzik wrote:
> Like SATA, we actually want to support BOTH -- active hotplug and PHY 
> power-down -- and so this wanders into power management policy.
>
> Give me a knob, and we can program plenty of ethernet|SATA|USB|... drivers 
> to power down the PHY and save power.

While at it, could we please fix our borked WOL interface requirements, so
that the PHY is *never* to be powered down when WOL is active?  This is
another deficiency that userspace has to work around in halt(8)...

If WOL is enabled and supported in a device, the kernel must never do
anything that would cause that device to stop responding to WOL packets.

OTOH, if WOL is disabled, it would be very very nice indeed to be able to
tell the kernel that yes, it is to power down the PHY if it is not in use.
Laptops appreciate it a lot, and so do the switches (which will know they
don't have to forward any traffic over that port).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: PM policy, hotplug, power saving (was Re: [PATCH] b44: power down PHY when interface down)

[Kyle Moffett]

On Jun 30, 2007, at 12:42:06, Jeff Garzik wrote:
> Definitely matters.  Switch renegotiation can take a while, and you  
> must take into account the common case of interface bouncing  
> (immediate down, then up).
>
> Hoards actively complained the few times we experimented with this,  
> because of e.g. DHCP's habit of bouncing the interface, which  
> resulted in PHY power bouncing, which resulted in negotiation,  
> which resulted in an excrutiating wait on various broken or stupid  
> switches.
>
> Overall, this may be classed with other problems of a similar  
> sort:  we can power down a PHY, but that removes hotplug capability  
> and extends partner/link negotiation time.
>
> Like SATA, we actually want to support BOTH -- active hotplug and  
> PHY power-down -- and so this wanders into power management policy.
>
> Give me a knob, and we can program plenty of ethernet|SATA|USB|...  
> drivers to power down the PHY and save power.

With some buggy switches and other hardware you actually *want* to  
bounce the link to get them to properly renegotiate.  I can also see  
wanting to power off and on a single-PoE-port NIC to restart whatever  
device is at the other end, although I don't know if any such devices  
exist.  Currently the tg3 driver turns the PHY off and on during down/ 
up on a few of my systems, which I use to make a buggy no-name switch  
recognize STP changes properly.

Cheers,
Kyle Moffett

Re: [PATCH] b44: power down PHY when interface down

[Stephen Hemminger]

On Sat, 30 Jun 2007 14:47:35 +0300
"Török Edvin" <edwintorok-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> When the interface is down (or driver removed), the BroadCom 44xx card remains
> powered on, and both its MAC and PHY is using up power.
> This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
> is halted, and does a partial chip reset turns off the activity LEDs too.
> 
> Applies to 2.6.22-rc6, or current git head.
> 
> Tested on a Broadcom BCM4401-B0 card, it saves ~0.5W (measured using powertop).
> 
> Signed-off-by: Torok Edwin <edwintorok-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  b44.c |   25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)
> diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> index 879a2ff..00d0f57 100644
> --- a/drivers/net/b44.c
> +++ b/drivers/net/b44.c
> @@ -113,6 +113,8 @@ static void b44_init_rings(struct b44 *);
>  #define B44_FULL_RESET		1
>  #define B44_FULL_RESET_SKIP_PHY	2
>  #define B44_PARTIAL_RESET	3
> +#define B44_CHIP_RESET_FULL     4
> +#define B44_CHIP_RESET_PARTIAL  5
> 
>  static void b44_init_hw(struct b44 *, int);
> 
> @@ -1283,7 +1285,7 @@ static void b44_clear_stats(struct b44 *bp)
>  }
> 
>  /* bp->lock is held. */
> -static void b44_chip_reset(struct b44 *bp)
> +static void b44_chip_reset(struct b44 *bp, int reset_kind)
>  {
>  	if (ssb_is_core_up(bp)) {
>  		bw32(bp, B44_RCV_LAZY, 0);
> @@ -1307,6 +1309,10 @@ static void b44_chip_reset(struct b44 *bp)
> 
>  	b44_clear_stats(bp);
> 
> +	if (reset_kind == B44_CHIP_RESET_PARTIAL)
> +		return;/* don't enable PHY if we are doing a partial reset
> +		, we are probably going to power down */
> +

This is a non-standard formatting for comments, please follow
Coding style:


Linux style for comments is the C89 "/* ... */" style.
Don't use C99-style "// ..." comments.

The preferred style for long (multi-line) comments is:

	/*
	 * This is the preferred style for multi-line
	 * comments in the Linux kernel source code.
	 * Please use it consistently.
	 *
	 * Description:  A column of asterisks on the left side,
	 * with beginning and ending almost-blank lines.
	 */

Re: [PATCH] b44: power down PHY when interface down

[Török Edvin]

On 6/30/07, Stephen Hemminger <shemminger@linux-foundation.org> wrote:
>
> This is a non-standard formatting for comments, please follow
> Coding style:

Thanks, I have updated the patch.

When the interface is down (or driver removed), the BroadCom 44xx card
remains powered on,
and both its MAC and PHY is using up power.
This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the
interface is halted.
Also doing a partial chip reset turns off the activity LEDs too.

Signed-off-by: Torok Edwin <edwintorok@gmail.com>
---
 b44.c |   28 +++++++++++++++++++++++-----
  1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index 879a2ff..43926fd 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -113,6 +113,8 @@ static void b44_init_rings(struct b44 *);
 #define B44_FULL_RESET		1
 #define B44_FULL_RESET_SKIP_PHY	2
 #define B44_PARTIAL_RESET	3
+#define B44_CHIP_RESET_FULL     4
+#define B44_CHIP_RESET_PARTIAL  5

 static void b44_init_hw(struct b44 *, int);

@@ -1283,7 +1285,7 @@ static void b44_clear_stats(struct b44 *bp)
 }

 /* bp->lock is held. */
-static void b44_chip_reset(struct b44 *bp)
+static void b44_chip_reset(struct b44 *bp, int reset_kind)
 {
 	if (ssb_is_core_up(bp)) {
 		bw32(bp, B44_RCV_LAZY, 0);
@@ -1307,6 +1309,13 @@ static void b44_chip_reset(struct b44 *bp)

 	b44_clear_stats(bp);

+	/*
+	 * Don't enable PHY if we are doing a partial reset
+	 * we are probably going to power down
+	 */
+	if (reset_kind == B44_CHIP_RESET_PARTIAL)
+		return;
+
 	/* Make PHY accessible. */
 	bw32(bp, B44_MDIO_CTRL, (MDIO_CTRL_PREAMBLE |
 			     (0x0d & MDIO_CTRL_MAXF_MASK)));
@@ -1332,7 +1341,14 @@ static void b44_chip_reset(struct b44 *bp)
 static void b44_halt(struct b44 *bp)
 {
 	b44_disable_ints(bp);
-	b44_chip_reset(bp);
+	/* reset PHY */
+	b44_phy_reset(bp);
+	/* power down PHY */
+	printk(KERN_INFO PFX "%s: powering down PHY\n", bp->dev->name);
+	bw32(bp, B44_MAC_CTRL, MAC_CTRL_PHY_PDOWN);
+	/* now reset the chip, but without enabling the MAC&PHY
+	 * part of it. This has to be done _after_ we shut down the PHY */
+	b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
 }

 /* bp->lock is held. */
@@ -1376,7 +1392,7 @@ static void b44_init_hw(struct b44 *bp, int reset_kind)
 {
 	u32 val;

-	b44_chip_reset(bp);
+	b44_chip_reset(bp, B44_CHIP_RESET_FULL);
 	if (reset_kind == B44_FULL_RESET) {
 		b44_phy_reset(bp);
 		b44_setup_phy(bp);
@@ -1430,7 +1446,7 @@ static int b44_open(struct net_device *dev)

 	err = request_irq(dev->irq, b44_interrupt, IRQF_SHARED, dev->name, dev);
 	if (unlikely(err < 0)) {
-		b44_chip_reset(bp);
+		b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
 		b44_free_rings(bp);
 		b44_free_consistent(bp);
 		goto out;
@@ -2250,7 +2266,7 @@ static int __devinit b44_init_one(struct pci_dev *pdev,
 	/* Chip reset provides power to the b44 MAC & PCI cores, which
 	 * is necessary for MAC register access.
 	 */
-	b44_chip_reset(bp);
+	b44_chip_reset(bp, B44_CHIP_RESET_FULL);

 	printk(KERN_INFO "%s: Broadcom 4400 10/100BaseT Ethernet ", dev->name);
 	for (i = 0; i < 6; i++)
@@ -2284,6 +2300,7 @@ static void __devexit b44_remove_one(struct pci_dev *pdev)
 	free_netdev(dev);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
 	pci_set_drvdata(pdev, NULL);
 }

@@ -2312,6 +2329,7 @@ static int b44_suspend(struct pci_dev *pdev,
pm_message_t state)
 		b44_setup_wol(bp);
 	}
 	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
 	return 0;
 }

Re: [PATCH] b44: power down PHY when interface down

[Michael Buesch]

On Sunday 01 July 2007 14:49:23 Török Edvin wrote:
> On 6/30/07, Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> >
> > This is a non-standard formatting for comments, please follow
> > Coding style:
> 
> Thanks, I have updated the patch.
> 
> When the interface is down (or driver removed), the BroadCom 44xx card
> remains powered on,
> and both its MAC and PHY is using up power.
> This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the
> interface is halted.
> Also doing a partial chip reset turns off the activity LEDs too.
> 
> Signed-off-by: Torok Edwin <edwintorok@gmail.com>
> ---
>  b44.c |   28 +++++++++++++++++++++++-----
>   1 file changed, 23 insertions(+), 5 deletions(-)
> diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> index 879a2ff..43926fd 100644
> --- a/drivers/net/b44.c
> +++ b/drivers/net/b44.c
> @@ -113,6 +113,8 @@ static void b44_init_rings(struct b44 *);
>  #define B44_FULL_RESET		1
>  #define B44_FULL_RESET_SKIP_PHY	2
>  #define B44_PARTIAL_RESET	3
> +#define B44_CHIP_RESET_FULL     4
> +#define B44_CHIP_RESET_PARTIAL  5
> 
>  static void b44_init_hw(struct b44 *, int);
> 
> @@ -1283,7 +1285,7 @@ static void b44_clear_stats(struct b44 *bp)
>  }
> 
>  /* bp->lock is held. */
> -static void b44_chip_reset(struct b44 *bp)
> +static void b44_chip_reset(struct b44 *bp, int reset_kind)
>  {
>  	if (ssb_is_core_up(bp)) {
>  		bw32(bp, B44_RCV_LAZY, 0);
> @@ -1307,6 +1309,13 @@ static void b44_chip_reset(struct b44 *bp)
> 
>  	b44_clear_stats(bp);
> 
> +	/*
> +	 * Don't enable PHY if we are doing a partial reset
> +	 * we are probably going to power down
> +	 */
> +	if (reset_kind == B44_CHIP_RESET_PARTIAL)
> +		return;
> +
>  	/* Make PHY accessible. */
>  	bw32(bp, B44_MDIO_CTRL, (MDIO_CTRL_PREAMBLE |
>  			     (0x0d & MDIO_CTRL_MAXF_MASK)));
> @@ -1332,7 +1341,14 @@ static void b44_chip_reset(struct b44 *bp)
>  static void b44_halt(struct b44 *bp)
>  {
>  	b44_disable_ints(bp);
> -	b44_chip_reset(bp);
> +	/* reset PHY */
> +	b44_phy_reset(bp);
> +	/* power down PHY */
> +	printk(KERN_INFO PFX "%s: powering down PHY\n", bp->dev->name);
> +	bw32(bp, B44_MAC_CTRL, MAC_CTRL_PHY_PDOWN);
> +	/* now reset the chip, but without enabling the MAC&PHY
> +	 * part of it. This has to be done _after_ we shut down the PHY */
> +	b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
>  }
> 
>  /* bp->lock is held. */
> @@ -1376,7 +1392,7 @@ static void b44_init_hw(struct b44 *bp, int reset_kind)
>  {
>  	u32 val;
> 
> -	b44_chip_reset(bp);
> +	b44_chip_reset(bp, B44_CHIP_RESET_FULL);
>  	if (reset_kind == B44_FULL_RESET) {
>  		b44_phy_reset(bp);
>  		b44_setup_phy(bp);
> @@ -1430,7 +1446,7 @@ static int b44_open(struct net_device *dev)
> 
>  	err = request_irq(dev->irq, b44_interrupt, IRQF_SHARED, dev->name, dev);
>  	if (unlikely(err < 0)) {
> -		b44_chip_reset(bp);
> +		b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
>  		b44_free_rings(bp);
>  		b44_free_consistent(bp);
>  		goto out;
> @@ -2250,7 +2266,7 @@ static int __devinit b44_init_one(struct pci_dev *pdev,
>  	/* Chip reset provides power to the b44 MAC & PCI cores, which
>  	 * is necessary for MAC register access.
>  	 */
> -	b44_chip_reset(bp);
> +	b44_chip_reset(bp, B44_CHIP_RESET_FULL);
> 
>  	printk(KERN_INFO "%s: Broadcom 4400 10/100BaseT Ethernet ", dev->name);
>  	for (i = 0; i < 6; i++)
> @@ -2284,6 +2300,7 @@ static void __devexit b44_remove_one(struct pci_dev *pdev)
>  	free_netdev(dev);
>  	pci_release_regions(pdev);
>  	pci_disable_device(pdev);
> +	pci_set_power_state(pdev, PCI_D3hot);
>  	pci_set_drvdata(pdev, NULL);
>  }
> 
> @@ -2312,6 +2329,7 @@ static int b44_suspend(struct pci_dev *pdev,
> pm_message_t state)

You patch is also damaged here.
Check your MUA settings.

-- 
Greetings Michael.

Re: [PATCH] b44: power down PHY when interface down

[Michael Buesch]

On Saturday 30 June 2007 13:47:35 Török Edvin wrote:
> When the interface is down (or driver removed), the BroadCom 44xx card remains
> powered on, and both its MAC and PHY is using up power.
> This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
> is halted, and does a partial chip reset turns off the activity LEDs too.
> 
> Applies to 2.6.22-rc6, or current git head.
> 
> Tested on a Broadcom BCM4401-B0 card, it saves ~0.5W (measured using powertop).

Hm, I was going to measure the real power advantage with a
PCI-extender card. But my B44B0 card doesn't seem to work in
that extender card. It works perfectly fine sticked directly into
the motherboard, though, and other cards like a BCM4318 work in
the extender, too.
Not sure what this is.
The extender has an application note about nonworking cards in the
extender and a too big resistor on the board IDSEL pin being the
cause of this.
Maybe I can try with another machine tomorrow.

-- 
Greetings Michael.

Re: [PATCH] b44: power down PHY when interface down

[Lennert Buytenhek]

On Sat, Jun 30, 2007 at 11:53:25PM +0200, Michael Buesch wrote:

> > When the interface is down (or driver removed), the BroadCom 44xx card remains
> > powered on, and both its MAC and PHY is using up power.
> > This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
> > is halted, and does a partial chip reset turns off the activity LEDs too.
> > 
> > Applies to 2.6.22-rc6, or current git head.
> > 
> > Tested on a Broadcom BCM4401-B0 card, it saves ~0.5W (measured using powertop).
> 
> Hm, I was going to measure the real power advantage with a
> PCI-extender card. But my B44B0 card doesn't seem to work in
> that extender card. It works perfectly fine sticked directly into
> the motherboard, though, and other cards like a BCM4318 work in
> the extender, too.
> Not sure what this is.
> The extender has an application note about nonworking cards in the
> extender and a too big resistor on the board IDSEL pin being the
> cause of this.

Does the card show up in lspci at all?  IDSEL drive strength
issues should only affect config space accesses.

Does the extender board have a PCI-PCI bridge on it?  (If not,
there's not really any reason to resistively couple the IDSEL
line to the host, since the host should take care of that.)


> Maybe I can try with another machine tomorrow.

That would only make a difference if there is no PCI-PCI bridge on
the extender board.

If the extender resistively couples the host's IDSEL line, you might
see different results on a different host bridge, since different
host bridges can use different numbers of IDSEL stepping cycles.

Re: [PATCH] b44: power down PHY when interface down

[Michael Buesch]

On Sunday 01 July 2007 00:03:01 Lennert Buytenhek wrote:
> On Sat, Jun 30, 2007 at 11:53:25PM +0200, Michael Buesch wrote:
> 
> > > When the interface is down (or driver removed), the BroadCom 44xx card remains
> > > powered on, and both its MAC and PHY is using up power.
> > > This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface
> > > is halted, and does a partial chip reset turns off the activity LEDs too.
> > > 
> > > Applies to 2.6.22-rc6, or current git head.
> > > 
> > > Tested on a Broadcom BCM4401-B0 card, it saves ~0.5W (measured using powertop).
> > 
> > Hm, I was going to measure the real power advantage with a
> > PCI-extender card. But my B44B0 card doesn't seem to work in
> > that extender card. It works perfectly fine sticked directly into
> > the motherboard, though, and other cards like a BCM4318 work in
> > the extender, too.
> > Not sure what this is.
> > The extender has an application note about nonworking cards in the
> > extender and a too big resistor on the board IDSEL pin being the
> > cause of this.
> 
> Does the card show up in lspci at all?

No it doesn't.

> IDSEL drive strength 
> issues should only affect config space accesses.

Yeah.

> Does the extender board have a PCI-PCI bridge on it?  (If not,
> there's not really any reason to resistively couple the IDSEL
> line to the host, since the host should take care of that.)

There's no bridge. It just decouples all voltage lines, so you can
drive it from external supply and/or measure voltages and current.
On the PCB it looks like the the IDSEL line is rather directly
routed to the host IDSEL. It just goes through one of the bus
isolation chips. So I guess (just my guess) that this chip has some
resistance and if the total resistance of the chip + the IDSEL
resistor on the mainboard goes above some threshold it doesn't work
anymore for some cards. In the application note they write
about trouble for IDSEL resistors >51ohms.

> > Maybe I can try with another machine tomorrow.
> 
> That would only make a difference if there is no PCI-PCI bridge on
> the extender board.

Well, they suggest it in the application note as a possible fix. ;)


-- 
Greetings Michael.

Re: [PATCH] b44: power down PHY when interface down

[Lennert Buytenhek]

On Sun, Jul 01, 2007 at 12:24:40AM +0200, Michael Buesch wrote:

> > > Hm, I was going to measure the real power advantage with a
> > > PCI-extender card. But my B44B0 card doesn't seem to work in
> > > that extender card. It works perfectly fine sticked directly into
> > > the motherboard, though, and other cards like a BCM4318 work in
> > > the extender, too.
> > > Not sure what this is.
> > > The extender has an application note about nonworking cards in the
> > > extender and a too big resistor on the board IDSEL pin being the
> > > cause of this.
> > 
> > Does the card show up in lspci at all?
> 
> No it doesn't.

Right, so it sounds like it might be this issue.


> > Does the extender board have a PCI-PCI bridge on it?  (If not,
> > there's not really any reason to resistively couple the IDSEL
> > line to the host, since the host should take care of that.)
> 
> There's no bridge. It just decouples all voltage lines, so you can
> drive it from external supply and/or measure voltages and current.
> On the PCB it looks like the the IDSEL line is rather directly
> routed to the host IDSEL. It just goes through one of the bus
> isolation chips. So I guess (just my guess) that this chip has some
> resistance and if the total resistance of the chip + the IDSEL
> resistor on the mainboard goes above some threshold it doesn't work
> anymore for some cards. In the application note they write
> about trouble for IDSEL resistors >51ohms.

More or less.  You can't add the resistances like that, since the
bus isolation chip buffers the IDSEL signal, but it is correct that
if the host's IDSEL resistor is larger than a certain value, the
combination of the resistive coupling of IDSEL plus the extra buffer
in the isolator might be causing the IDSEL input on the 'guest' PCI
board to assert too late (or not assert at all), causing config
accesses to fail.

(This also depends on the specific 'guest' PCI board used, as you
noted, due to differing IDSEL trace lengths/capacitances and input
pin capacitances on different PCI boards.  Also, it might work at
33 MHz but not work at 66 MHz, etc.)

If you feel adventurous, you could try to hack around this by
figuring out which AD[31:16] line this PCI slot's IDSEL line is
resistively coupled to (depends on the slot), and then adding
another parallel resistor on the board itself to make the bus
isolator's input buffer charge faster.  Note that this does
increase the load on that specific AD[] line, which might cause
other funny effects.


> > > Maybe I can try with another machine tomorrow.
> > 
> > That would only make a difference if there is no PCI-PCI bridge on
> > the extender board.
> 
> Well, they suggest it in the application note as a possible fix. ;)

The bus isolation chip doesn't count as a PCI-PCI bridge. :)  I'm just
saying that you wouldn't see the issue you are seeing now if the
extender board had a real PCI-PCI bridge on it, since in that case the
type 0 config access to the guest PCI board would be generated by the
bridge instead of by the host.

Re: [PATCH] b44: power down PHY when interface down

[Michael Buesch]

On Sunday 01 July 2007 01:17:34 Lennert Buytenhek wrote:
> More or less.  You can't add the resistances like that, since the
> bus isolation chip buffers the IDSEL signal, but it is correct that
> if the host's IDSEL resistor is larger than a certain value, the
> combination of the resistive coupling of IDSEL plus the extra buffer
> in the isolator might be causing the IDSEL input on the 'guest' PCI
> board to assert too late (or not assert at all), causing config
> accesses to fail.
> 
> (This also depends on the specific 'guest' PCI board used, as you
> noted, due to differing IDSEL trace lengths/capacitances and input
> pin capacitances on different PCI boards.  Also, it might work at
> 33 MHz but not work at 66 MHz, etc.)

It doesn't work on any of my boards :(

> If you feel adventurous, you could try to hack around this by
> figuring out which AD[31:16] line this PCI slot's IDSEL line is
> resistively coupled to (depends on the slot), and then adding
> another parallel resistor on the board itself to make the bus
> isolator's input buffer charge faster.  Note that this does
> increase the load on that specific AD[] line, which might cause
> other funny effects.

Well, but how to find out to which address line it's connected to?
Pretty hard to follow the PCB traces, especially since it's
multilayered.

-- 
Greetings Michael.

Re: [PATCH] b44: power down PHY when interface down

[Lennert Buytenhek]

On Sun, Jul 01, 2007 at 12:23:16PM +0200, Michael Buesch wrote:

> > More or less.  You can't add the resistances like that, since the
> > bus isolation chip buffers the IDSEL signal, but it is correct that
> > if the host's IDSEL resistor is larger than a certain value, the
> > combination of the resistive coupling of IDSEL plus the extra buffer
> > in the isolator might be causing the IDSEL input on the 'guest' PCI
> > board to assert too late (or not assert at all), causing config
> > accesses to fail.
> > 
> > (This also depends on the specific 'guest' PCI board used, as you
> > noted, due to differing IDSEL trace lengths/capacitances and input
> > pin capacitances on different PCI boards.  Also, it might work at
> > 33 MHz but not work at 66 MHz, etc.)
> 
> It doesn't work on any of my boards :(

What extender board is this?  Do you have docs/schematics?
And what motherboard brand/type?


> > If you feel adventurous, you could try to hack around this by
> > figuring out which AD[31:16] line this PCI slot's IDSEL line is
> > resistively coupled to (depends on the slot), and then adding
> > another parallel resistor on the board itself to make the bus
> > isolator's input buffer charge faster.  Note that this does
> > increase the load on that specific AD[] line, which might cause
> > other funny effects.
> 
> Well, but how to find out to which address line it's connected to?
> Pretty hard to follow the PCB traces, especially since it's
> multilayered.

Actually, the IDSEL resistor would be on the computer's
motherboard, not on the PCI board.  And to which address line
the IDSEL line is connected depends on which PCI slot on the
motherboard you're looking at.

A multimeter should do the trick, but I would advise against this
if you're not totally comfortable with hacking hardware.

Re: [PATCH] b44: power down PHY when interface down

[Michael Buesch]

On Sunday 01 July 2007 17:00:06 Lennert Buytenhek wrote:
> On Sun, Jul 01, 2007 at 12:23:16PM +0200, Michael Buesch wrote:
> 
> > > More or less.  You can't add the resistances like that, since the
> > > bus isolation chip buffers the IDSEL signal, but it is correct that
> > > if the host's IDSEL resistor is larger than a certain value, the
> > > combination of the resistive coupling of IDSEL plus the extra buffer
> > > in the isolator might be causing the IDSEL input on the 'guest' PCI
> > > board to assert too late (or not assert at all), causing config
> > > accesses to fail.
> > > 
> > > (This also depends on the specific 'guest' PCI board used, as you
> > > noted, due to differing IDSEL trace lengths/capacitances and input
> > > pin capacitances on different PCI boards.  Also, it might work at
> > > 33 MHz but not work at 66 MHz, etc.)
> > 
> > It doesn't work on any of my boards :(
> 
> What extender board is this?  Do you have docs/schematics?

catalyst pcibx32
http://bu3sch.de/pcibx.php
Docs yes, schematics no.

> And what motherboard brand/type?

ABit AI7
The other was some MSI and some very old random board. dunno.
It works perfectly fine with other cards, like a linksys
wlan card with a broadcom 4318 chip. It's just the b44
that doesn't work in the extender.

> Actually, the IDSEL resistor would be on the computer's
> motherboard, not on the PCI board.  And to which address line

Yeah, I know.

> the IDSEL line is connected depends on which PCI slot on the
> motherboard you're looking at.

Sure.

> A multimeter should do the trick, but I would advise against this
> if you're not totally comfortable with hacking hardware.

Well, you mean to measure the idsel against each possible AD line?
It's difficult, because the motherboard is inside of a standard
computer case and a watercooling system is mounted. So I would
have to disassemble all that stuff. :/
Probably I can measure it with very thin probes on the slots
without unmounting the board, hm...

-- 
Greetings Michael.

Re: [PATCH] b44: power down PHY when interface down

[Michael Buesch]

On Sunday 01 July 2007 17:00:06 Lennert Buytenhek wrote:
> A multimeter should do the trick, but I would advise against this
> if you're not totally comfortable with hacking hardware.

Ok, the resistor on the board is 100ohm, which is too big
according to the docs of the extender.
So what I tried is to connect a 10ohm resistor between
IDSEL and the AD pin, on the extender itself. But it didn't
do the trick. It still doesn't work.
I also connected a 10ohm resistor between IDSEL and the AD
pin directly on the mainboard. Also doesn't work. So I guess
it's some other issue.
But I have no idea what could be wrong besides that.

-- 
Greetings Michael.

Re: [PATCH] b44: power down PHY when interface down

[Török Edvin]

On 7/1/07, Michael Buesch <mb@bu3sch.de> wrote:
> 
> You patch is also damaged here.
> Check your MUA settings.
> 

[Sorry about that, resending again]



When the interface is down (or driver removed), the BroadCom 44xx card remains powered on,
and both its MAC and PHY is using up power.
This patch makes the driver issue a MAC_CTRL_PHY_PDOWN when the interface is halted.
Also doing a partial chip reset turns off the activity LEDs too.

Signed-off-by: Torok Edwin <edwintorok@gmail.com>
---
 b44.c |   28 +++++++++++++++++++++++-----
  1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index 879a2ff..43926fd 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -113,6 +113,8 @@ static void b44_init_rings(struct b44 *);
 #define B44_FULL_RESET		1
 #define B44_FULL_RESET_SKIP_PHY	2
 #define B44_PARTIAL_RESET	3
+#define B44_CHIP_RESET_FULL     4
+#define B44_CHIP_RESET_PARTIAL  5
 
 static void b44_init_hw(struct b44 *, int);
 
@@ -1283,7 +1285,7 @@ static void b44_clear_stats(struct b44 *bp)
 }
 
 /* bp->lock is held. */
-static void b44_chip_reset(struct b44 *bp)
+static void b44_chip_reset(struct b44 *bp, int reset_kind)
 {
 	if (ssb_is_core_up(bp)) {
 		bw32(bp, B44_RCV_LAZY, 0);
@@ -1307,6 +1309,13 @@ static void b44_chip_reset(struct b44 *bp)
 
 	b44_clear_stats(bp);
 
+	/*
+	 * Don't enable PHY if we are doing a partial reset
+	 * we are probably going to power down
+	 */
+	if (reset_kind == B44_CHIP_RESET_PARTIAL)
+		return;
+
 	/* Make PHY accessible. */
 	bw32(bp, B44_MDIO_CTRL, (MDIO_CTRL_PREAMBLE |
 			     (0x0d & MDIO_CTRL_MAXF_MASK)));
@@ -1332,7 +1341,14 @@ static void b44_chip_reset(struct b44 *bp)
 static void b44_halt(struct b44 *bp)
 {
 	b44_disable_ints(bp);
-	b44_chip_reset(bp);
+	/* reset PHY */
+	b44_phy_reset(bp);
+	/* power down PHY */
+	printk(KERN_INFO PFX "%s: powering down PHY\n", bp->dev->name);
+	bw32(bp, B44_MAC_CTRL, MAC_CTRL_PHY_PDOWN);
+	/* now reset the chip, but without enabling the MAC&PHY
+	 * part of it. This has to be done _after_ we shut down the PHY */
+	b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
 }
 
 /* bp->lock is held. */
@@ -1376,7 +1392,7 @@ static void b44_init_hw(struct b44 *bp, int reset_kind)
 {
 	u32 val;
 
-	b44_chip_reset(bp);
+	b44_chip_reset(bp, B44_CHIP_RESET_FULL);
 	if (reset_kind == B44_FULL_RESET) {
 		b44_phy_reset(bp);
 		b44_setup_phy(bp);
@@ -1430,7 +1446,7 @@ static int b44_open(struct net_device *dev)
 
 	err = request_irq(dev->irq, b44_interrupt, IRQF_SHARED, dev->name, dev);
 	if (unlikely(err < 0)) {
-		b44_chip_reset(bp);
+		b44_chip_reset(bp, B44_CHIP_RESET_PARTIAL);
 		b44_free_rings(bp);
 		b44_free_consistent(bp);
 		goto out;
@@ -2250,7 +2266,7 @@ static int __devinit b44_init_one(struct pci_dev *pdev,
 	/* Chip reset provides power to the b44 MAC & PCI cores, which
 	 * is necessary for MAC register access.
 	 */
-	b44_chip_reset(bp);
+	b44_chip_reset(bp, B44_CHIP_RESET_FULL);
 
 	printk(KERN_INFO "%s: Broadcom 4400 10/100BaseT Ethernet ", dev->name);
 	for (i = 0; i < 6; i++)
@@ -2284,6 +2300,7 @@ static void __devexit b44_remove_one(struct pci_dev *pdev)
 	free_netdev(dev);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
 	pci_set_drvdata(pdev, NULL);
 }
 
@@ -2312,6 +2329,7 @@ static int b44_suspend(struct pci_dev *pdev, pm_message_t state)
 		b44_setup_wol(bp);
 	}
 	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
 	return 0;
 }