[PATCH 1/2] Added information about which firmware file is being requested.

[PATCH 1/2] Added information about which firmware file is being requested.

[Jarl Friis]

This is informative information to provide about which actual firmware
file is being used.

Signed-off-by: Jarl Friis <jarl@softace.dk>
---
 drivers/net/wireless/b43/main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index a140165..202a0eb 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -2131,6 +2131,7 @@ int b43_do_request_fw(struct b43_request_fw_context *ctx,
 		B43_WARN_ON(1);
 		return -ENOSYS;
 	}
+	b43info(ctx->dev->wl, "Requesting firmware file '%s'\n", ctx->fwname);
 	err = request_firmware(&blob, ctx->fwname, ctx->dev->dev->dev);
 	if (err == -ENOENT) {
 		snprintf(ctx->errors[ctx->req_type],
-- 
1.7.9.5

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Jarl Friis]

This is using the LP specific firmware to better take advantage of the
Low-Power capabilities.

Signed-off-by: Jarl Friis <jarl@softace.dk>
---
 drivers/net/wireless/b43/main.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 202a0eb..9ee6030 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -8,6 +8,7 @@
   Copyright (c) 2005 Danny van Dyk <kugelfang@gentoo.org>
   Copyright (c) 2005 Andreas Jaggi <andreas.jaggi@waterwave.ch>
   Copyright (c) 2010-2011 Rafa? Mi?ecki <zajec5@gmail.com>
+  Copyright (c) 2012 Jarl Friis <jarl@softace.dk>
 
   SDIO support
   Copyright (c) 2009 Albert Herranz <albert_herranz@yahoo.es>
@@ -72,6 +73,7 @@ MODULE_FIRMWARE("b43/ucode11.fw");
 MODULE_FIRMWARE("b43/ucode13.fw");
 MODULE_FIRMWARE("b43/ucode14.fw");
 MODULE_FIRMWARE("b43/ucode15.fw");
+MODULE_FIRMWARE("b43/ucode16_lp.fw");
 MODULE_FIRMWARE("b43/ucode16_mimo.fw");
 MODULE_FIRMWARE("b43/ucode5.fw");
 MODULE_FIRMWARE("b43/ucode9.fw");
@@ -2208,6 +2210,12 @@ static int b43_try_request_fw(struct b43_request_fw_context *ctx)
 			else
 				goto err_no_ucode;
 			break;
+		case B43_PHYTYPE_LP:
+			if (rev >= 16)
+				filename = "ucode16_lp";
+			else
+				goto err_no_ucode;
+			break;
 		case B43_PHYTYPE_HT:
 			if (rev == 29)
 				filename = "ucode29_mimo";
@@ -2277,8 +2285,10 @@ static int b43_try_request_fw(struct b43_request_fw_context *ctx)
 			filename = "lp0initvals13";
 		else if (rev == 14)
 			filename = "lp0initvals14";
-		else if (rev >= 15)
+		else if (rev == 15)
 			filename = "lp0initvals15";
+		else if (rev >= 16)
+			filename = "lp0initvals16";
 		else
 			goto err_no_initvals;
 		break;
@@ -2336,8 +2346,10 @@ static int b43_try_request_fw(struct b43_request_fw_context *ctx)
 			filename = "lp0bsinitvals13";
 		else if (rev == 14)
 			filename = "lp0bsinitvals14";
-		else if (rev >= 15)
+		else if (rev == 15)
 			filename = "lp0bsinitvals15";
+		else if (rev >= 16)
+			filename = "lp0bsinitvals16";
 		else
 			goto err_no_initvals;
 		break;
-- 
1.7.9.5

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Larry Finger]

On 09/19/2012 06:18 AM, Jarl Friis wrote:
> This is using the LP specific firmware to better take advantage of the
> Low-Power capabilities.
>
> Signed-off-by: Jarl Friis <jarl@softace.dk>
> ---
>   drivers/net/wireless/b43/main.c |   16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 202a0eb..9ee6030 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -8,6 +8,7 @@
>     Copyright (c) 2005 Danny van Dyk <kugelfang@gentoo.org>
>     Copyright (c) 2005 Andreas Jaggi <andreas.jaggi@waterwave.ch>
>     Copyright (c) 2010-2011 Rafa? Mi?ecki <zajec5@gmail.com>
> +  Copyright (c) 2012 Jarl Friis <jarl@softace.dk>
>
>     SDIO support
>     Copyright (c) 2009 Albert Herranz <albert_herranz@yahoo.es>
> @@ -72,6 +73,7 @@ MODULE_FIRMWARE("b43/ucode11.fw");
>   MODULE_FIRMWARE("b43/ucode13.fw");
>   MODULE_FIRMWARE("b43/ucode14.fw");
>   MODULE_FIRMWARE("b43/ucode15.fw");
> +MODULE_FIRMWARE("b43/ucode16_lp.fw");
>   MODULE_FIRMWARE("b43/ucode16_mimo.fw");
>   MODULE_FIRMWARE("b43/ucode5.fw");
>   MODULE_FIRMWARE("b43/ucode9.fw");
> @@ -2208,6 +2210,12 @@ static int b43_try_request_fw(struct b43_request_fw_context *ctx)
>   			else
>   				goto err_no_ucode;
>   			break;
> +		case B43_PHYTYPE_LP:
> +			if (rev >= 16)
> +				filename = "ucode16_lp";
> +			else
> +				goto err_no_ucode;
> +			break;
>   		case B43_PHYTYPE_HT:
>   			if (rev == 29)
>   				filename = "ucode29_mimo";
> @@ -2277,8 +2285,10 @@ static int b43_try_request_fw(struct b43_request_fw_context *ctx)
>   			filename = "lp0initvals13";
>   		else if (rev == 14)
>   			filename = "lp0initvals14";
> -		else if (rev >= 15)
> +		else if (rev == 15)
>   			filename = "lp0initvals15";
> +		else if (rev >= 16)
> +			filename = "lp0initvals16";
>   		else
>   			goto err_no_initvals;
>   		break;
> @@ -2336,8 +2346,10 @@ static int b43_try_request_fw(struct b43_request_fw_context *ctx)
>   			filename = "lp0bsinitvals13";
>   		else if (rev == 14)
>   			filename = "lp0bsinitvals14";
> -		else if (rev >= 15)
> +		else if (rev == 15)
>   			filename = "lp0bsinitvals15";
> +		else if (rev >= 16)
> +			filename = "lp0bsinitvals16";
>   		else
>   			goto err_no_initvals;
>   		break;

I have some questions about this patch. Where did you get the information needed 
to make these changes? Did it come from reverse engineering some Broadcom code, 
or did you look at their actual code? There is a great deal of difference 
relative to our "clean-room" status. Anyone that has seen non-GPL Broadcom 
material cannot contribute code to b43.

Have you tested this code on devices with rev>=16?

Now for some comments: This patch also needs the "b43:" added to the subject. In 
addition, you appear to have at least one white-space error in the 
MODULE_FIRMWARE line. Is the addition of your copyright to the driver warranted 
by this change? For example, I have made much larger contributions to b43 over 
the years before I started doing reverse-engineering on this driver, but I never 
added my copyright. Your "Signed-off-by" implies copyright for the patch.

Larry

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Jarl Friis]

Thanks for feed back.

2012/9/19 Larry Finger <Larry.Finger@lwfinger.net>:
>
>
> I have some questions about this patch. Where did you get the information
> needed to make these changes?

To be completely honest: I didn't get any information, it is based
purely on practical experiments: I experienced some inconsistency wrt.
performance on my hp6735b. I had some time to look at the driver (it's
15 years since I patched the kernel last time). Anyway I saw the
filename pattern for firmware and saw that `b43-fwcutter
broadcom-wl-5.100.138/linux/wl_apsta.o -w` contained similar files
ending on "16". So I simple tried it out (for version 16), and so far
it seems to work better. It also seems to wake up faster after a
sleep.

> Did it come from reverse engineering some
> Broadcom code

No!

> or did you look at their actual code?

No!

> There is a great deal
> of difference relative to our "clean-room" status. Anyone that has seen
> non-GPL Broadcom material cannot contribute code to b43.

I have not seen any Broadcom code at all (apart from the stuff that is
already in the linux source tree)

>
> Have you tested this code on devices with rev>=16?

Yes on my HP6735b having this chip integrated:
[ 1577.549270] b43-phy1: Broadcom 4322 WLAN found (core revision 16)
[ 1577.592117] b43-phy1 debug: Found PHY: Analog 8, Type 4, Revision 4
[ 1577.592158] b43-phy1 debug: Found Radio: Manuf 0x17F, Version
0x2056, Revision 3

I guess the part of the patch for PHY_LP has not been reached. I will
submit a new series of patches that separates things

>
> Now for some comments: This patch also needs the "b43:" added to the
> subject.

Sorry. It's long ago I have submitted patches to the kernel.

> In addition, you appear to have at least one white-space error in
> the MODULE_FIRMWARE line.

I am not sure what you mean here. Is this a mail issue... (I wrote it
just like the other ones around it)

> Is the addition of your copyright to the driver
> warranted by this change?

As far as I understand the copyright law: Yes, but I'm not an expert.
Neither am I 100% sure what you mean.

> For example, I have made much larger contributions
> to b43 over the years before I started doing reverse-engineering on this
> driver, but I never added my copyright.

I suggest you do.

> Your "Signed-off-by" implies
> copyright for the patch.

The fact that I authored the patch implies copyright (even without
Signed-off-by)


Jarl

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Arend van Spriel]

On 09/19/2012 08:54 PM, Jarl Friis wrote:
>> Is the addition of your copyright to the driver
>> >warranted by this change?
> As far as I understand the copyright law: Yes, but I'm not an expert.
> Neither am I 100% sure what you mean.

You add your name to the copy notice of the file so you claim it for the 
entire file.

>> >For example, I have made much larger contributions
>> >to b43 over the years before I started doing reverse-engineering on this
>> >driver, but I never added my copyright.
> I suggest you do.

Typically, the names you see there are and/or were b43 maintainers.

>> >Your "Signed-off-by" implies
>> >copyright for the patch.
> The fact that I authored the patch implies copyright (even without
> Signed-off-by)
>
> Jarl

You can indeed claim it for your patch, but I think not beyond that. But 
I am no lawyer either so .....pffff.

Gr. AvS

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Jarl Friis]

Never mind the copyright notice I was just trying to follow procedures...

And apparently that was not one of them... :-)

Jarl

2012/9/19 Arend van Spriel <arend@broadcom.com>:
> On 09/19/2012 08:54 PM, Jarl Friis wrote:
>>>
>>> Is the addition of your copyright to the driver
>>> >warranted by this change?
>>
>> As far as I understand the copyright law: Yes, but I'm not an expert.
>> Neither am I 100% sure what you mean.
>
>
> You add your name to the copy notice of the file so you claim it for the
> entire file.
>
>
>>> >For example, I have made much larger contributions
>>> >to b43 over the years before I started doing reverse-engineering on this
>>> >driver, but I never added my copyright.
>>
>> I suggest you do.
>
>
> Typically, the names you see there are and/or were b43 maintainers.
>
>
>>> >Your "Signed-off-by" implies
>>> >copyright for the patch.
>>
>> The fact that I authored the patch implies copyright (even without
>> Signed-off-by)
>>
>> Jarl
>
>
> You can indeed claim it for your patch, but I think not beyond that. But I
> am no lawyer either so .....pffff.
>
> Gr. AvS
>



-- 
Jarl Friis
Softace ApS
R?dhustorvet 7, 2.
3520 Farum
LinkedIn: http://dk.linkedin.com/in/jarlfriis

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Jarl Friis]

2012/9/19 Jarl Friis <jarl@softace.dk>:
> This is using the LP specific firmware to better take advantage of the
> Low-Power capabilities.

Gosh... I just realized that the code I introduced is completely
untested. My hardware does not reach these pieces of code...

Sorry...

However the code seems natural (due to the firmware file name pattern)
for a PHY-LP hardware, that is a bcm4313 chip (pciid 14e4:4315).

So if there is anybody with such hardware... you might want to try it... Sorry.

Jarl

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Rafał Miłecki]

2012/9/19 Jarl Friis <jarl@softace.dk>:
> 2012/9/19 Jarl Friis <jarl@softace.dk>:
>> This is using the LP specific firmware to better take advantage of the
>> Low-Power capabilities.
>
> Gosh... I just realized that the code I introduced is completely
> untested. My hardware does not reach these pieces of code...
>
> Sorry...
>
> However the code seems natural (due to the firmware file name pattern)
> for a PHY-LP hardware, that is a bcm4313 chip (pciid 14e4:4315).

Well, your BCM4322 is not LP-PHY card... does it really work better
after that change, and wakes up faster? ;)

BCM4313 is LCN-PHY, so your change won't affect it, not to mention it
is not supported by b43.

14e4:4315 is really BCM4312 (LP-PHY) and maybe could be affected by
this patch... but just in case of core 16. AFAIK our 14e4:4315 are
usually devices with core rev 15.

-- 
Rafa?

[PATCH 2/2] Using LP firmware for taking advantage of the low-power capabilities.

[Jarl Friis]

2012/9/20 Rafa? Mi?ecki <zajec5@gmail.com>:
> Well, your BCM4322 is not LP-PHY card... does it really work better
> after that change, and wakes up faster? ;)

That was my un-scientific impression, yes (probably biased by
expectations, right? :-) ). But it could also be that I was using a
more upstream kernel than the stock ubuntu kernel. I don't know :-)

> BCM4313 is LCN-PHY, so your change won't affect it, not to mention it
> is not supported by b43.
>
> 14e4:4315 is really BCM4312 (LP-PHY) and maybe could be affected by
> this patch... but just in case of core 16. AFAIK our 14e4:4315 are
> usually devices with core rev 15.

Thanks for clearing that out...

Jarl

[PATCH 1/2] Added information about which firmware file is being requested.

[Julian Calaby]

Hi Jarl,

You should really mention which driver these are for in the subject line, say:

[PATCH] b43: Added information about which firmware file is being requested

On Wed, Sep 19, 2012 at 9:18 PM, Jarl Friis <jarl@softace.dk> wrote:
> This is informative information to provide about which actual firmware
> file is being used.

Also, this patch is so small and obvious that you could arguably get
away with something like:

Subject: [PATCH] b43: Log firmware filename

Log the name of the firmware file requested.


Or something along those lines.

Thanks,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

[PATCH 1/2] Added information about which firmware file is being requested.

[Michael Tokarev]

On 19.09.2012 15:18, Jarl Friis wrote:

> +	b43info(ctx->dev->wl, "Requesting firmware file '%s'\n", ctx->fwname);
>  	err = request_firmware(&blob, ctx->fwname, ctx->dev->dev->dev);

Hmm.  I wonder if this should be printed in request_firmware()
itself instead of in all callers?

/mjt

[PATCH 1/2] Added information about which firmware file is being requested.

[Jarl Friis]

2012/9/19 Michael Tokarev <mjt@tls.msk.ru>:
> On 19.09.2012 15:18, Jarl Friis wrote:
>
>> +     b43info(ctx->dev->wl, "Requesting firmware file '%s'\n", ctx->fwname);
>>       err = request_firmware(&blob, ctx->fwname, ctx->dev->dev->dev);
>
> Hmm.  I wonder if this should be printed in request_firmware()
> itself instead of in all callers?

Now that you mention it, I also think that is a much better idea.
However that would be a much more central place to do the change, so I
would gladly see somebody else do that patch (in replacement of mine)

Jarl

[PATCH 1/2] Added information about which firmware file is being requested.

[Rafał Miłecki]

2012/9/19 Jarl Friis <jarl@softace.dk>:
> 2012/9/19 Michael Tokarev <mjt@tls.msk.ru>:
>> On 19.09.2012 15:18, Jarl Friis wrote:
>>
>>> +     b43info(ctx->dev->wl, "Requesting firmware file '%s'\n", ctx->fwname);
>>>       err = request_firmware(&blob, ctx->fwname, ctx->dev->dev->dev);
>>
>> Hmm.  I wonder if this should be printed in request_firmware()
>> itself instead of in all callers?
>
> Now that you mention it, I also think that is a much better idea.
> However that would be a much more central place to do the change, so I
> would gladly see somebody else do that patch (in replacement of mine)

I agree, please submit patch modifying request_firmware if you believe
it's important.

-- 
Rafa?