[PATCH 0/9] rtl8187: start cleanup/revisiting code

[PATCH 0/9] rtl8187: start cleanup/revisiting code

[Herton Ronaldo Krzesinski]

Hi,

While looking at rtl8187 driver I noticed possible cleanups and found 2
bugs (last 2 patches in the series). This series presents the changes.
I didn't finished revisiting everything, so there are more cleanups
possible, but for now the 2 bug fixes are worth the initial submit.

--
[]'s
Herton

[PATCH 1/9] rtl8187: remove redundant initialization of ARFR

[Herton Ronaldo Krzesinski]

This removes redundant write to Auto Rate Fallback Register on RTL8187B.
The same value was being written twice in the same function. Avoid this
removing the duplicate initialization on rtl8187b_reg_table, and also
add comment for this write (information from Realtek source).

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 6e26149..3dbf305 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -712,10 +712,9 @@ static const u8 rtl8187b_reg_table[][3] = {
 
 	{0x58, 0x4B, 1}, {0x59, 0x00, 1}, {0x5A, 0x4B, 1}, {0x5B, 0x00, 1},
 	{0x60, 0x4B, 1}, {0x61, 0x09, 1}, {0x62, 0x4B, 1}, {0x63, 0x09, 1},
-	{0xCE, 0x0F, 1}, {0xCF, 0x00, 1}, {0xE0, 0xFF, 1}, {0xE1, 0x0F, 1},
-	{0xE2, 0x00, 1}, {0xF0, 0x4E, 1}, {0xF1, 0x01, 1}, {0xF2, 0x02, 1},
-	{0xF3, 0x03, 1}, {0xF4, 0x04, 1}, {0xF5, 0x05, 1}, {0xF6, 0x06, 1},
-	{0xF7, 0x07, 1}, {0xF8, 0x08, 1},
+	{0xCE, 0x0F, 1}, {0xCF, 0x00, 1}, {0xF0, 0x4E, 1}, {0xF1, 0x01, 1},
+	{0xF2, 0x02, 1}, {0xF3, 0x03, 1}, {0xF4, 0x04, 1}, {0xF5, 0x05, 1},
+	{0xF6, 0x06, 1}, {0xF7, 0x07, 1}, {0xF8, 0x08, 1},
 
 	{0x4E, 0x00, 2}, {0x0C, 0x04, 2}, {0x21, 0x61, 2}, {0x22, 0x68, 2},
 	{0x23, 0x6F, 2}, {0x24, 0x76, 2}, {0x25, 0x7D, 2}, {0x26, 0x84, 2},
@@ -776,7 +775,9 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	reg |= RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT;
 	rtl818x_iowrite8(priv, &priv->map->CW_CONF, reg);
 
+	/* Auto Rate Fallback Register (ARFR): 1M-54M setting */
 	rtl818x_iowrite16_idx(priv, (__le16 *)0xFFE0, 0x0FFF, 1);
+	rtl818x_iowrite8_idx(priv, (u8 *)0xFFE2, 0x00, 1);
 
 	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL, 100);
 	rtl818x_iowrite16(priv, &priv->map->ATIM_WND, 2);
-- 
1.7.3.2

[PATCH 2/9] rtl8187: remove setting of beacon/atim registers from initialization

[Herton Ronaldo Krzesinski]

On 8187B path, we set a initial value for beacon interval and atim
window on initialization. But this isn't needed, since same setup is
done on rtl8187_config.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 3dbf305..30c2120 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -779,8 +779,6 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	rtl818x_iowrite16_idx(priv, (__le16 *)0xFFE0, 0x0FFF, 1);
 	rtl818x_iowrite8_idx(priv, (u8 *)0xFFE2, 0x00, 1);
 
-	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL, 100);
-	rtl818x_iowrite16(priv, &priv->map->ATIM_WND, 2);
 	rtl818x_iowrite16_idx(priv, (__le16 *)0xFFD4, 0xFFFF, 1);
 
 	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-- 
1.7.3.2

[PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Herton Ronaldo Krzesinski]

We were using wrong address for BRSR (Basic Rate Set Register) while
initializing its value, comparing with Realtek sources, for 8187B case.

Also, the same register is initialized in rtl8187b_reg_table, so remove
the duplicate initialization from the table.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 30c2120..b9ce2a8 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -725,11 +725,11 @@ static const u8 rtl8187b_reg_table[][3] = {
 	{0x62, 0x08, 2}, {0x63, 0x08, 2}, {0x64, 0xCF, 2}, {0x72, 0x56, 2},
 	{0x73, 0x9A, 2},
 
-	{0x34, 0xF0, 0}, {0x35, 0x0F, 0}, {0x5B, 0x40, 0}, {0x84, 0x88, 0},
-	{0x85, 0x24, 0}, {0x88, 0x54, 0}, {0x8B, 0xB8, 0}, {0x8C, 0x07, 0},
-	{0x8D, 0x00, 0}, {0x94, 0x1B, 0}, {0x95, 0x12, 0}, {0x96, 0x00, 0},
-	{0x97, 0x06, 0}, {0x9D, 0x1A, 0}, {0x9F, 0x10, 0}, {0xB4, 0x22, 0},
-	{0xBE, 0x80, 0}, {0xDB, 0x00, 0}, {0xEE, 0x00, 0}, {0x4C, 0x00, 2},
+	{0x5B, 0x40, 0}, {0x84, 0x88, 0}, {0x85, 0x24, 0}, {0x88, 0x54, 0},
+	{0x8B, 0xB8, 0}, {0x8C, 0x07, 0}, {0x8D, 0x00, 0}, {0x94, 0x1B, 0},
+	{0x95, 0x12, 0}, {0x96, 0x00, 0}, {0x97, 0x06, 0}, {0x9D, 0x1A, 0},
+	{0x9F, 0x10, 0}, {0xB4, 0x22, 0}, {0xBE, 0x80, 0}, {0xDB, 0x00, 0},
+	{0xEE, 0x00, 0}, {0x4C, 0x00, 2},
 
 	{0x9F, 0x00, 3}, {0x8C, 0x01, 0}, {0x8D, 0x10, 0}, {0x8E, 0x08, 0},
 	{0x8F, 0x00, 0}
@@ -770,7 +770,11 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	if (res)
 		return res;
 
-	rtl818x_iowrite16(priv, (__le16 *)0xFF2D, 0x0FFF);
+	/* BRSR (Basic Rate Set Register) on 8187B looks to be the same as
+	 * RESP_RATE on 8187L in Realtek sources: each bit should be each
+	 * one of the 12 rates, all are enabled */
+	rtl818x_iowrite16(priv, (__le16 *)0xFF34, 0x0FFF);
+
 	reg = rtl818x_ioread8(priv, &priv->map->CW_CONF);
 	reg |= RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT;
 	rtl818x_iowrite8(priv, &priv->map->CW_CONF, reg);
-- 
1.7.3.2

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Hin-Tak Leung]

--- On Tue, 2/11/10, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:

> We were using wrong address for BRSR
> (Basic Rate Set Register) while
> initializing its value, comparing with Realtek sources, for
> 8187B case.
> 
> Also, the same register is initialized in
> rtl8187b_reg_table, so remove
> the duplicate initialization from the table.
> 
> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

Acked-by: Hin-Tak Leung <htl10@users.sourceforge.net>

ANAPARAM* stands for "anonymous parameters", right?  One of these days we should give them some meaningful names. 



      

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Larry Finger]

On 11/01/2010 09:42 PM, Hin-Tak Leung wrote:
> 
> --- On Tue, 2/11/10, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
> 
>> We were using wrong address for BRSR
>> (Basic Rate Set Register) while
>> initializing its value, comparing with Realtek sources, for
>> 8187B case.
>>
>> Also, the same register is initialized in
>> rtl8187b_reg_table, so remove
>> the duplicate initialization from the table.
>>
>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>> Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
> 
> Acked-by: Hin-Tak Leung <htl10@users.sourceforge.net>
> 
> ANAPARAM* stands for "anonymous parameters", right?  One of these days we should give them some meaningful names. 

I have always thought it was for "analog parameter". Am I wrong?

Larry

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Herton Ronaldo Krzesinski]

On 02-11-2010 00:46, Larry Finger wrote:
> On 11/01/2010 09:42 PM, Hin-Tak Leung wrote:
>>
>> --- On Tue, 2/11/10, Herton Ronaldo Krzesinski<herton@mandriva.com.br>  wrote:
>>
>>> We were using wrong address for BRSR
>>> (Basic Rate Set Register) while
>>> initializing its value, comparing with Realtek sources, for
>>> 8187B case.
>>>
>>> Also, the same register is initialized in
>>> rtl8187b_reg_table, so remove
>>> the duplicate initialization from the table.
>>>
>>> Signed-off-by: Herton Ronaldo Krzesinski<herton@mandriva.com.br>
>>> Acked-by: Larry Finger<Larry.Finger@lwfinger.net>
>>
>> Acked-by: Hin-Tak Leung<htl10@users.sourceforge.net>
>>
>> ANAPARAM* stands for "anonymous parameters", right?  One of these days we should give them some meaningful names.
>
> I have always thought it was for "analog parameter". Am I wrong?

Nope, you're right, its listed description is "analog parameter" in 8180 
spec.

>
> Larry

--
[]'s
Herton

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Rogerio Luz Coelho]

Ok, I see the 8187b is in development.

But PLEASE ask Thadeu Cascardo for his patch as it seems to fix the
issue with these cards when connected to Laptop (Positivo Laptops in
Brasil for instance), this would even let the Mandriva folks show
Positivo that they don't need to ship the Mandriva 2008.0 woith their
Linux Laptops anymore ...

the patch he sent me FIXED the problem where the chip did not connect
or when it did it would lose the connection right away...

see
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/215802?comments=all


I haven´t aplied any other patches and the one he sent me JUST WORKS,
tested in a Ubuntu 10.04 64bit (64bit being important since the
Realtek support told me they did NOT support 64bit with their linux
drivers)

Thanks

Rogerio

2010/11/2 Herton Ronaldo Krzesinski <herton@mandriva.com.br>:
> On 02-11-2010 00:46, Larry Finger wrote:
>>
>> On 11/01/2010 09:42 PM, Hin-Tak Leung wrote:
>>>
>>> --- On Tue, 2/11/10, Herton Ronaldo Krzesinski<herton@mandriva.com.br>
>>>  wrote:
>>>
>>>> We were using wrong address for BRSR
>>>> (Basic Rate Set Register) while
>>>> initializing its value, comparing with Realtek sources, for
>>>> 8187B case.
>>>>
>>>> Also, the same register is initialized in
>>>> rtl8187b_reg_table, so remove
>>>> the duplicate initialization from the table.
>>>>
>>>> Signed-off-by: Herton Ronaldo Krzesinski<herton@mandriva.com.br>
>>>> Acked-by: Larry Finger<Larry.Finger@lwfinger.net>
>>>
>>> Acked-by: Hin-Tak Leung<htl10@users.sourceforge.net>
>>>
>>> ANAPARAM* stands for "anonymous parameters", right?  One of these days we
>>> should give them some meaningful names.
>>
>> I have always thought it was for "analog parameter". Am I wrong?
>
> Nope, you're right, its listed description is "analog parameter" in 8180
> spec.
>
>>
>> Larry
>
> --
> []'s
> Herton
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Larry Finger]

On 11/02/2010 06:48 PM, Rogerio Luz Coelho wrote:
> Ok, I see the 8187b is in development.
> 
> But PLEASE ask Thadeu Cascardo for his patch as it seems to fix the
> issue with these cards when connected to Laptop (Positivo Laptops in
> Brasil for instance), this would even let the Mandriva folks show
> Positivo that they don't need to ship the Mandriva 2008.0 woith their
> Linux Laptops anymore ...
> 
> the patch he sent me FIXED the problem where the chip did not connect
> or when it did it would lose the connection right away...
> 
> see
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/215802?comments=all
> 
> 
> I haven´t aplied any other patches and the one he sent me JUST WORKS,
> tested in a Ubuntu 10.04 64bit (64bit being important since the
> Realtek support told me they did NOT support 64bit with their linux
> drivers)

Realtek does not support 64-bit systems with most of their drivers, but we do.
In fact, I only do development on x86_64 and then test on i386.

Obviously, there is something special about the Toshiba/Brazil devices. The
patch that you need to make a cdonnection had absolutely no effect on mine.

Larry

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Rogerio Luz Coelho]

2010/11/2 Larry Finger <Larry.Finger@lwfinger.net>:
> On 11/02/2010 06:48 PM, Rogerio Luz Coelho wrote:
>> Ok, I see the 8187b is in development.
>>
>> But PLEASE ask Thadeu Cascardo for his patch as it seems to fix the
>> issue with these cards when connected to Laptop (Positivo Laptops in
>> Brasil for instance), this would even let the Mandriva folks show
>> Positivo that they don't need to ship the Mandriva 2008.0 woith their
>> Linux Laptops anymore ...
>>
>> the patch he sent me FIXED the problem where the chip did not connect
>> or when it did it would lose the connection right away...
>>
>> see
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/215802?comments=all
>>
>>
>> I haven´t aplied any other patches and the one he sent me JUST WORKS,
>> tested in a Ubuntu 10.04 64bit (64bit being important since the
>> Realtek support told me they did NOT support 64bit with their linux
>> drivers)
>
> Realtek does not support 64-bit systems with most of their drivers, but we do.
> In fact, I only do development on x86_64 and then test on i386.
>
> Obviously, there is something special about the Toshiba/Brazil devices. The
> patch that you need to make a cdonnection had absolutely no effect on mine.
>
> Larry
>

No effect is better than "regression" right?

I am not saying the chip got magically better, it still seems a cheap
POS, but with Cascardo's patch is as good as the Win7 driver here.

Rogerio

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Larry Finger]

On 11/02/2010 07:01 PM, Rogerio Luz Coelho wrote:
 No effect is better than "regression" right?
> 
> I am not saying the chip got magically better, it still seems a cheap
> POS, but with Cascardo's patch is as good as the Win7 driver here.

If you apply Herton's series of 9 patches, you should see a throughput
improvement. On my card, I get full performance from the 54M rate (27 Mb/s),
whereas the best I could do before those patches was 11 Mb/s.

Incidentally, making the Realtek drivers work on x86_64 is not that difficult.
In any place that they cast a pointer as (u32), that gets changed to
(__kernel_size_t). The other place that is affected is due to changes in the skb
buffer parameters tail and data, which are pointers in 32-bit, and offsets in
64-bit systems. The main point is that you cannot ignore any gcc warnings.

Larry

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Rogerio Luz Coelho]

2010/11/2 Larry Finger <Larry.Finger@lwfinger.net>:
> On 11/02/2010 07:01 PM, Rogerio Luz Coelho wrote:
>  No effect is better than "regression" right?
>>
>> I am not saying the chip got magically better, it still seems a cheap
>> POS, but with Cascardo's patch is as good as the Win7 driver here.
>
> If you apply Herton's series of 9 patches, you should see a throughput
> improvement. On my card, I get full performance from the 54M rate (27 Mb/s),
> whereas the best I could do before those patches was 11 Mb/s.
>
> Incidentally, making the Realtek drivers work on x86_64 is not that difficult.
> In any place that they cast a pointer as (u32), that gets changed to
> (__kernel_size_t). The other place that is affected is due to changes in the skb
> buffer parameters tail and data, which are pointers in 32-bit, and offsets in
> 64-bit systems. The main point is that you cannot ignore any gcc warnings.
>
> Larry

:) My biggest issue wasn't actually doing the patch, it was actually
compiling the damn thing on Ubuntu, that's what you get when you
install a distro that tells you in their kernel help page that they
don't WANT you to be able to compile your own kernel.

But as I said the connect - disconnect is gone.

I will try the new patches alone and with Cascardo's ... will post result here.

Rogerio

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Larry Finger]

On 11/02/2010 08:06 PM, Rogerio Luz Coelho wrote:
> 
> :) My biggest issue wasn't actually doing the patch, it was actually
> compiling the damn thing on Ubuntu, that's what you get when you
> install a distro that tells you in their kernel help page that they
> don't WANT you to be able to compile your own kernel.
> 
> But as I said the connect - disconnect is gone.
> 
> I will try the new patches alone and with Cascardo's ... will post result here.

As I did something like 20 full kernel builds today, you know I do not use a
distro that actively discourages kernel compilation. In fact I did run Ubuntu
once to help a user debug a problem. After experiencing the hoops they make you
jump through, I blew it away as soon as I could.

Larry

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Hin-Tak Leung]

On Wed, Nov 3, 2010 at 1:44 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
<snipped>
> As I did something like 20 full kernel builds today, you know I do not use a
> distro that actively discourages kernel compilation. In fact I did run Ubuntu
> once to help a user debug a problem. After experiencing the hoops they make you
> jump through, I blew it away as soon as I could.

What hoops do they make you do? compiling a new kernel isn't really
much more than the normal development with GNU toolchain?

I only did full kernel compilation with my old 32-bit x86 slackware
box (and it died last year), but I found compat-wireless quite alright
on top of x64_64 fedora, even for developing/tryinng patches. (that's
thanks to Luis and John, of course).

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Hin-Tak Leung]

On Wed, Nov 3, 2010 at 12:01 AM, Rogerio Luz Coelho
<rogluz.news@gmail.com> wrote:

>
> No effect is better than "regression" right?
>
> I am not saying the chip got magically better, it still seems a cheap
> POS, but with Cascardo's patch is as good as the Win7 driver here.

i'll have to read the Ubuntu launchpad thread a bit, but "no effect"
patches need to be clearly documented, etc because
(1) code stays in the kernel for a long time (years) and get modified,
re-factored, etc over time and one day somebody might need to
continue/modify/change a "no effect" patch and need to know why it was
included and what impact further changes.
(2) "no effect" really meant "no known effect" (yet).

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Rogerio Luz Coelho]

2010/11/4 Hin-Tak Leung <hintak.leung@gmail.com>:
> On Wed, Nov 3, 2010 at 12:01 AM, Rogerio Luz Coelho
> <rogluz.news@gmail.com> wrote:
>
>>
>> No effect is better than "regression" right?
>>
>> I am not saying the chip got magically better, it still seems a cheap
>> POS, but with Cascardo's patch is as good as the Win7 driver here.
>
> i'll have to read the Ubuntu launchpad thread a bit, but "no effect"
> patches need to be clearly documented, etc because
> (1) code stays in the kernel for a long time (years) and get modified,
> re-factored, etc over time and one day somebody might need to
> continue/modify/change a "no effect" patch and need to know why it was
> included and what impact further changes.
> (2) "no effect" really meant "no known effect" (yet).
>
But that's the point ... no effect to Larry that has an external card
is ok, but for my internal card the patch made everything better (more
so, now the box is useable again), so even if it is just 1 person (or
vendor) if the patch is "no effect" for the rest of the functionality
it should be added to mainline (al least IMHO)

Rogerio

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Herton Ronaldo Krzesinski]

On Tue, 2 Nov 2010 21:48:20 -0200
Rogerio Luz Coelho <rogluz.news@gmail.com> wrote:

> Ok, I see the 8187b is in development.
> 
> But PLEASE ask Thadeu Cascardo for his patch as it seems to fix the
> issue with these cards when connected to Laptop (Positivo Laptops in
> Brasil for instance), this would even let the Mandriva folks show
> Positivo that they don't need to ship the Mandriva 2008.0 woith their
> Linux Laptops anymore ...

He already reposted the patch and this series is based on top of it, so
soon we should see it applied I guess.

> 
> the patch he sent me FIXED the problem where the chip did not connect
> or when it did it would lose the connection right away...
> 
> see
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/215802?comments=all
> 
> 
> I haven´t aplied any other patches and the one he sent me JUST WORKS,
> tested in a Ubuntu 10.04 64bit (64bit being important since the
> Realtek support told me they did NOT support 64bit with their linux
> drivers)

Note that this series don't replace patch from Thadeu, it's just more cleanups
and fixes.

> 
> Thanks
> 
> Rogerio
> 
> 2010/11/2 Herton Ronaldo Krzesinski <herton@mandriva.com.br>:
> > On 02-11-2010 00:46, Larry Finger wrote:
> >>
> >> On 11/01/2010 09:42 PM, Hin-Tak Leung wrote:
> >>>
> >>> --- On Tue, 2/11/10, Herton Ronaldo Krzesinski<herton@mandriva.com.br>
> >>>  wrote:
> >>>
> >>>> We were using wrong address for BRSR
> >>>> (Basic Rate Set Register) while
> >>>> initializing its value, comparing with Realtek sources, for
> >>>> 8187B case.
> >>>>
> >>>> Also, the same register is initialized in
> >>>> rtl8187b_reg_table, so remove
> >>>> the duplicate initialization from the table.
> >>>>
> >>>> Signed-off-by: Herton Ronaldo Krzesinski<herton@mandriva.com.br>
> >>>> Acked-by: Larry Finger<Larry.Finger@lwfinger.net>
> >>>
> >>> Acked-by: Hin-Tak Leung<htl10@users.sourceforge.net>
> >>>
> >>> ANAPARAM* stands for "anonymous parameters", right?  One of these days we
> >>> should give them some meaningful names.
> >>
> >> I have always thought it was for "analog parameter". Am I wrong?
> >
> > Nope, you're right, its listed description is "analog parameter" in 8180
> > spec.
> >
> >>
> >> Larry
> >
> > --
> > []'s
> > Herton
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >


-- 
[]'s
Herton

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Rogerio Luz Coelho]

Could I apply these patches on top of Cascardo's?

Or should I tru individually for better testing ?

Rogerio

2010/11/3 Herton Ronaldo Krzesinski <herton@mandriva.com.br>:
> On Tue, 2 Nov 2010 21:48:20 -0200
> Rogerio Luz Coelho <rogluz.news@gmail.com> wrote:
>
>> Ok, I see the 8187b is in development.
>>
>> But PLEASE ask Thadeu Cascardo for his patch as it seems to fix the
>> issue with these cards when connected to Laptop (Positivo Laptops in
>> Brasil for instance), this would even let the Mandriva folks show
>> Positivo that they don't need to ship the Mandriva 2008.0 woith their
>> Linux Laptops anymore ...
>
> He already reposted the patch and this series is based on top of it, so
> soon we should see it applied I guess.
>
>>
>> the patch he sent me FIXED the problem where the chip did not connect
>> or when it did it would lose the connection right away...
>>
>> see
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/215802?comments=all
>>
>>
>> I haven´t aplied any other patches and the one he sent me JUST WORKS,
>> tested in a Ubuntu 10.04 64bit (64bit being important since the
>> Realtek support told me they did NOT support 64bit with their linux
>> drivers)
>
> Note that this series don't replace patch from Thadeu, it's just more cleanups
> and fixes.
>
>>
>> Thanks
>>
>> Rogerio
>>
>> 2010/11/2 Herton Ronaldo Krzesinski <herton@mandriva.com.br>:
>> > On 02-11-2010 00:46, Larry Finger wrote:
>> >>
>> >> On 11/01/2010 09:42 PM, Hin-Tak Leung wrote:
>> >>>
>> >>> --- On Tue, 2/11/10, Herton Ronaldo Krzesinski<herton@mandriva.com.br>
>> >>>  wrote:
>> >>>
>> >>>> We were using wrong address for BRSR
>> >>>> (Basic Rate Set Register) while
>> >>>> initializing its value, comparing with Realtek sources, for
>> >>>> 8187B case.
>> >>>>
>> >>>> Also, the same register is initialized in
>> >>>> rtl8187b_reg_table, so remove
>> >>>> the duplicate initialization from the table.
>> >>>>
>> >>>> Signed-off-by: Herton Ronaldo Krzesinski<herton@mandriva.com.br>
>> >>>> Acked-by: Larry Finger<Larry.Finger@lwfinger.net>
>> >>>
>> >>> Acked-by: Hin-Tak Leung<htl10@users.sourceforge.net>
>> >>>
>> >>> ANAPARAM* stands for "anonymous parameters", right?  One of these days we
>> >>> should give them some meaningful names.
>> >>
>> >> I have always thought it was for "analog parameter". Am I wrong?
>> >
>> > Nope, you're right, its listed description is "analog parameter" in 8180
>> > spec.
>> >
>> >>
>> >> Larry
>> >
>> > --
>> > []'s
>> > Herton
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>
>
> --
> []'s
> Herton
>

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Larry Finger]

On 11/03/2010 09:41 PM, Rogerio Luz Coelho wrote:
> Could I apply these patches on top of Cascardo's?
> 
> Or should I tru individually for better testing ?

Herton said to apply after Cascardo's changes. I tested them individually to
test whether the system would build at each step of the way. Only the last two
made any difference in the throughput. You can just test before, and after all
of them are applied.

Larry

Re: [PATCH 3/9] rtl8187: fix wrong register initialization in 8187B

[Hin-Tak Leung]

--- On Tue, 2/11/10, Larry Finger <Larry.Finger@lwfinger.net> wrote:

> > ANAPARAM* stands for "anonymous parameters",
> right?  One of these days we should give them some
> meaningful names. 
> 
> I have always thought it was for "analog parameter". Am I
> wrong?

Argh, you are right :-). Sorry for the noise. It is one of those big-eyeful tokens among the source where the meaningful part (the "analog" part) of it is much smaller than the not-so-meaningful part.


      

[PATCH 4/9] rtl8187: avoid redundant write to register FF72 (RFSW_CTRL)

[Herton Ronaldo Krzesinski]

The table with misc register initialization was setting it, and later
on we would set it again with a explicity call to rtl818x_iowrite16_idx.

Remove duplicate initialization from the register table.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index b9ce2a8..063374a 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -722,8 +722,7 @@ static const u8 rtl8187b_reg_table[][3] = {
 	{0x52, 0x04, 2}, {0x53, 0xA0, 2}, {0x54, 0x1F, 2}, {0x55, 0x23, 2},
 	{0x56, 0x45, 2}, {0x57, 0x67, 2}, {0x58, 0x08, 2}, {0x59, 0x08, 2},
 	{0x5A, 0x08, 2}, {0x5B, 0x08, 2}, {0x60, 0x08, 2}, {0x61, 0x08, 2},
-	{0x62, 0x08, 2}, {0x63, 0x08, 2}, {0x64, 0xCF, 2}, {0x72, 0x56, 2},
-	{0x73, 0x9A, 2},
+	{0x62, 0x08, 2}, {0x63, 0x08, 2}, {0x64, 0xCF, 2},
 
 	{0x5B, 0x40, 0}, {0x84, 0x88, 0}, {0x85, 0x24, 0}, {0x88, 0x54, 0},
 	{0x8B, 0xB8, 0}, {0x8C, 0x07, 0}, {0x8D, 0x00, 0}, {0x94, 0x1B, 0},
@@ -810,6 +809,7 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 
 	rtl818x_iowrite32(priv, &priv->map->RF_TIMING, 0x00004001);
 
+	/* RFSW_CTRL register */
 	rtl818x_iowrite16_idx(priv, (__le16 *)0xFF72, 0x569A, 2);
 
 	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-- 
1.7.3.2

[PATCH 5/9] rtl8187: move pll reset at start out of ANAPARAM write

[Herton Ronaldo Krzesinski]

On 8187B start, comment about pll reset, and move it out of ANAPARAM
write sequence, so that code is more readable.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 063374a..2b4ee26 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -742,7 +742,6 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 
 	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
 			 RTL818X_EEPROM_CMD_CONFIG);
-
 	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
 	reg |= RTL818X_CONFIG3_ANAPARAM_WRITE | RTL818X_CONFIG3_GNT_SELECT;
 	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
@@ -752,19 +751,19 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 			  RTL8187B_RTL8225_ANAPARAM_ON);
 	rtl818x_iowrite8(priv, &priv->map->ANAPARAM3,
 			 RTL8187B_RTL8225_ANAPARAM3_ON);
-
-	rtl818x_iowrite8(priv, (u8 *)0xFF61, 0x10);
-	reg = rtl818x_ioread8(priv, (u8 *)0xFF62);
-	rtl818x_iowrite8(priv, (u8 *)0xFF62, reg & ~(1 << 5));
-	rtl818x_iowrite8(priv, (u8 *)0xFF62, reg | (1 << 5));
-
 	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
 	reg &= ~RTL818X_CONFIG3_ANAPARAM_WRITE;
 	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
-
 	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
 			 RTL818X_EEPROM_CMD_NORMAL);
 
+	/* Reset PLL sequence on 8187B. Realtek note: reduces power
+	 * consumption about 30 mA */
+	rtl818x_iowrite8(priv, (u8 *)0xFF61, 0x10);
+	reg = rtl818x_ioread8(priv, (u8 *)0xFF62);
+	rtl818x_iowrite8(priv, (u8 *)0xFF62, reg & ~(1 << 5));
+	rtl818x_iowrite8(priv, (u8 *)0xFF62, reg | (1 << 5));
+
 	res = rtl8187_cmd_reset(dev);
 	if (res)
 		return res;
-- 
1.7.3.2

[PATCH 6/9] rtl8187: don't set RTL818X_CONFIG3_GNT_SELECT flag on 8187B

[Herton Ronaldo Krzesinski]

The GNTSel bit should only concern pci devices by looking at RTL8180
spec, which is not the case of 8187B. Also testing shows that trying to
set this bit fails, a subsequent read from the register after trying to
set it shows that the bit isn't set, seems the hardware ignores it,
which makes sense. This setting was a left over from Realtek sources.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 2b4ee26..d7ea5d1 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -743,7 +743,7 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
 			 RTL818X_EEPROM_CMD_CONFIG);
 	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	reg |= RTL818X_CONFIG3_ANAPARAM_WRITE | RTL818X_CONFIG3_GNT_SELECT;
+	reg |= RTL818X_CONFIG3_ANAPARAM_WRITE;
 	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
 	rtl818x_iowrite32(priv, &priv->map->ANAPARAM2,
 			  RTL8187B_RTL8225_ANAPARAM2_ON);
-- 
1.7.3.2

[PATCH 7/9] rtl8187: consolidate anaparam on/off write sequences

[Herton Ronaldo Krzesinski]

There are repeated calls for anaparam on/off sequence in the code.
Consolidate the common code in rtl8187_set_anaparam and use it where
needed.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c     |   84 ++++++++++++-----------
 drivers/net/wireless/rtl818x/rtl8187_rtl8225.c |   22 ------
 2 files changed, 44 insertions(+), 62 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index d7ea5d1..c0c75aa 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -553,6 +553,46 @@ static int rtl8187b_init_status_urb(struct ieee80211_hw *dev)
 	return ret;
 }
 
+static void rtl8187_set_anaparam(struct rtl8187_priv *priv, bool rfon)
+{
+	u32 anaparam, anaparam2;
+	u8 anaparam3, reg;
+
+	if (!priv->is_rtl8187b) {
+		if (rfon) {
+			anaparam = RTL8187_RTL8225_ANAPARAM_ON;
+			anaparam2 = RTL8187_RTL8225_ANAPARAM2_ON;
+		} else {
+			anaparam = RTL8187_RTL8225_ANAPARAM_OFF;
+			anaparam2 = RTL8187_RTL8225_ANAPARAM2_OFF;
+		}
+	} else {
+		if (rfon) {
+			anaparam = RTL8187B_RTL8225_ANAPARAM_ON;
+			anaparam2 = RTL8187B_RTL8225_ANAPARAM2_ON;
+			anaparam3 = RTL8187B_RTL8225_ANAPARAM3_ON;
+		} else {
+			anaparam = RTL8187B_RTL8225_ANAPARAM_OFF;
+			anaparam2 = RTL8187B_RTL8225_ANAPARAM2_OFF;
+			anaparam3 = RTL8187B_RTL8225_ANAPARAM3_OFF;
+		}
+	}
+
+	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
+			 RTL818X_EEPROM_CMD_CONFIG);
+	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
+	reg |= RTL818X_CONFIG3_ANAPARAM_WRITE;
+	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
+	rtl818x_iowrite32(priv, &priv->map->ANAPARAM, anaparam);
+	rtl818x_iowrite32(priv, &priv->map->ANAPARAM2, anaparam2);
+	if (priv->is_rtl8187b)
+		rtl818x_iowrite8(priv, &priv->map->ANAPARAM3, anaparam3);
+	reg &= ~RTL818X_CONFIG3_ANAPARAM_WRITE;
+	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
+	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
+			 RTL818X_EEPROM_CMD_NORMAL);
+}
+
 static int rtl8187_cmd_reset(struct ieee80211_hw *dev)
 {
 	struct rtl8187_priv *priv = dev->priv;
@@ -603,19 +643,7 @@ static int rtl8187_init_hw(struct ieee80211_hw *dev)
 	int res;
 
 	/* reset */
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-			 RTL818X_EEPROM_CMD_CONFIG);
-	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg |
-			 RTL818X_CONFIG3_ANAPARAM_WRITE);
-	rtl818x_iowrite32(priv, &priv->map->ANAPARAM,
-			  RTL8187_RTL8225_ANAPARAM_ON);
-	rtl818x_iowrite32(priv, &priv->map->ANAPARAM2,
-			  RTL8187_RTL8225_ANAPARAM2_ON);
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg &
-			 ~RTL818X_CONFIG3_ANAPARAM_WRITE);
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-			 RTL818X_EEPROM_CMD_NORMAL);
+	rtl8187_set_anaparam(priv, true);
 
 	rtl818x_iowrite16(priv, &priv->map->INT_MASK, 0);
 
@@ -629,17 +657,7 @@ static int rtl8187_init_hw(struct ieee80211_hw *dev)
 	if (res)
 		return res;
 
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, RTL818X_EEPROM_CMD_CONFIG);
-	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3,
-			reg | RTL818X_CONFIG3_ANAPARAM_WRITE);
-	rtl818x_iowrite32(priv, &priv->map->ANAPARAM,
-			  RTL8187_RTL8225_ANAPARAM_ON);
-	rtl818x_iowrite32(priv, &priv->map->ANAPARAM2,
-			  RTL8187_RTL8225_ANAPARAM2_ON);
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3,
-			reg & ~RTL818X_CONFIG3_ANAPARAM_WRITE);
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, RTL818X_EEPROM_CMD_NORMAL);
+	rtl8187_set_anaparam(priv, true);
 
 	/* setup card */
 	rtl818x_iowrite16(priv, &priv->map->RFPinsSelect, 0);
@@ -740,22 +758,7 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	int res, i;
 	u8 reg;
 
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-			 RTL818X_EEPROM_CMD_CONFIG);
-	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	reg |= RTL818X_CONFIG3_ANAPARAM_WRITE;
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
-	rtl818x_iowrite32(priv, &priv->map->ANAPARAM2,
-			  RTL8187B_RTL8225_ANAPARAM2_ON);
-	rtl818x_iowrite32(priv, &priv->map->ANAPARAM,
-			  RTL8187B_RTL8225_ANAPARAM_ON);
-	rtl818x_iowrite8(priv, &priv->map->ANAPARAM3,
-			 RTL8187B_RTL8225_ANAPARAM3_ON);
-	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	reg &= ~RTL818X_CONFIG3_ANAPARAM_WRITE;
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-			 RTL818X_EEPROM_CMD_NORMAL);
+	rtl8187_set_anaparam(priv, true);
 
 	/* Reset PLL sequence on 8187B. Realtek note: reduces power
 	 * consumption about 30 mA */
@@ -1006,6 +1009,7 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
 	rtl818x_iowrite8(priv, &priv->map->CMD, reg);
 
 	priv->rf->stop(dev);
+	rtl8187_set_anaparam(priv, false);
 
 	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, RTL818X_EEPROM_CMD_CONFIG);
 	reg = rtl818x_ioread8(priv, &priv->map->CONFIG4);
diff --git a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
index 97eebdc..5c6666f 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
@@ -898,29 +898,7 @@ static void rtl8225z2_b_rf_init(struct ieee80211_hw *dev)
 
 static void rtl8225_rf_stop(struct ieee80211_hw *dev)
 {
-	u8 reg;
-	struct rtl8187_priv *priv = dev->priv;
-
 	rtl8225_write(dev, 0x4, 0x1f);
-
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, RTL818X_EEPROM_CMD_CONFIG);
-	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg | RTL818X_CONFIG3_ANAPARAM_WRITE);
-	if (!priv->is_rtl8187b) {
-		rtl818x_iowrite32(priv, &priv->map->ANAPARAM2,
-				  RTL8187_RTL8225_ANAPARAM2_OFF);
-		rtl818x_iowrite32(priv, &priv->map->ANAPARAM,
-				  RTL8187_RTL8225_ANAPARAM_OFF);
-	} else {
-		rtl818x_iowrite32(priv, &priv->map->ANAPARAM2,
-				  RTL8187B_RTL8225_ANAPARAM2_OFF);
-		rtl818x_iowrite32(priv, &priv->map->ANAPARAM,
-				  RTL8187B_RTL8225_ANAPARAM_OFF);
-		rtl818x_iowrite8(priv, &priv->map->ANAPARAM3,
-				  RTL8187B_RTL8225_ANAPARAM3_OFF);
-	}
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg & ~RTL818X_CONFIG3_ANAPARAM_WRITE);
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, RTL818X_EEPROM_CMD_NORMAL);
 }
 
 static void rtl8225_rf_set_channel(struct ieee80211_hw *dev,
-- 
1.7.3.2

[PATCH 8/9] rtl8187: remove uneeded setting of anaparam write

[Herton Ronaldo Krzesinski]

Usually you set RTL818X_CONFIG3_ANAPARAM_WRITE when you are going to
change/write ANAPARAM registers. But in current initialization of
RTL8187B there is a place where ANAPARAM_WRITE bit is set without any
ANAPARAM register being written, without reason, so remove it.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: seno <senada@t-online.de>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index c0c75aa..4448647 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -814,14 +814,6 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	/* RFSW_CTRL register */
 	rtl818x_iowrite16_idx(priv, (__le16 *)0xFF72, 0x569A, 2);
 
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-			 RTL818X_EEPROM_CMD_CONFIG);
-	reg = rtl818x_ioread8(priv, &priv->map->CONFIG3);
-	reg |= RTL818X_CONFIG3_ANAPARAM_WRITE;
-	rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
-	rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
-			 RTL818X_EEPROM_CMD_NORMAL);
-
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, 0x0480);
 	rtl818x_iowrite16(priv, &priv->map->RFPinsSelect, 0x2488);
 	rtl818x_iowrite16(priv, &priv->map->RFPinsEnable, 0x1FFF);
-- 
1.7.3.2

[PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Herton Ronaldo Krzesinski]

Current 8187B initialization misses anaparam registers restore after
8187 reset. This causes ANAPARAM register to stay zeroed out (ANAPARAM2
kept its value on my tests). To avoid this, call rtl8187_set_anaparam
right after chip reset (to be on the safe side, as it makes sure we
restore all ANAPARAM registers).

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: seno <senada@t-online.de>
---
 drivers/net/wireless/rtl818x/rtl8187_dev.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 4448647..eeee244 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -771,6 +771,8 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 	if (res)
 		return res;
 
+	rtl8187_set_anaparam(priv, true);
+
 	/* BRSR (Basic Rate Set Register) on 8187B looks to be the same as
 	 * RESP_RATE on 8187L in Realtek sources: each bit should be each
 	 * one of the 12 rates, all are enabled */
-- 
1.7.3.2

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On Mon, Nov 01, 2010 at 10:59:39PM -0200, Herton Ronaldo Krzesinski wrote:
> Current 8187B initialization misses anaparam registers restore after
> 8187 reset. This causes ANAPARAM register to stay zeroed out (ANAPARAM2
> kept its value on my tests). To avoid this, call rtl8187_set_anaparam
> right after chip reset (to be on the safe side, as it makes sure we
> restore all ANAPARAM registers).
> 

Hello, Herton.

Thank you very much for these patches. I am in the process of testing
them right now. The first thing I've noticed is a drop in the signal
level by 10dBm when using this last patch. Is this something we should
be concerned with?

I will do some tests with the distance to the Access Point and send my
results later.

Regards,
Cascardo.

> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: seno <senada@t-online.de>
> ---
>  drivers/net/wireless/rtl818x/rtl8187_dev.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> index 4448647..eeee244 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> @@ -771,6 +771,8 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
>  	if (res)
>  		return res;
>  
> +	rtl8187_set_anaparam(priv, true);
> +
>  	/* BRSR (Basic Rate Set Register) on 8187B looks to be the same as
>  	 * RESP_RATE on 8187L in Realtek sources: each bit should be each
>  	 * one of the 12 rates, all are enabled */
> -- 
> 1.7.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Herton Ronaldo Krzesinski]

On Thu, 4 Nov 2010 13:30:57 -0200
Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com> wrote:

> On Mon, Nov 01, 2010 at 10:59:39PM -0200, Herton Ronaldo Krzesinski wrote:
> > Current 8187B initialization misses anaparam registers restore after
> > 8187 reset. This causes ANAPARAM register to stay zeroed out (ANAPARAM2
> > kept its value on my tests). To avoid this, call rtl8187_set_anaparam
> > right after chip reset (to be on the safe side, as it makes sure we
> > restore all ANAPARAM registers).
> > 
> 
> Hello, Herton.
> 
> Thank you very much for these patches. I am in the process of testing
> them right now. The first thing I've noticed is a drop in the signal
> level by 10dBm when using this last patch. Is this something we should
> be concerned with?

I think not, may be the signal is a bit weaker with the anaparam now being
what Realtek uses/recomends (unfortunately it's a magic number and not
disclosed what each bit/parameter is doing..., probably the buggy previous
behaviour of the setting being zero after reset was making the signal too
high when not needed), but as with it we can get higher rate and bandwidth
on same distance, it shouldn't be a concern (my iperf test results showed
improvements too with the last two changes).

> 
> I will do some tests with the distance to the Access Point and send my
> results later.
> 
> Regards,
> Cascardo.
> 
> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
> > Cc: seno <senada@t-online.de>
> > ---
> >  drivers/net/wireless/rtl818x/rtl8187_dev.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > index 4448647..eeee244 100644
> > --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > @@ -771,6 +771,8 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
> >  	if (res)
> >  		return res;
> >  
> > +	rtl8187_set_anaparam(priv, true);
> > +
> >  	/* BRSR (Basic Rate Set Register) on 8187B looks to be the same as
> >  	 * RESP_RATE on 8187L in Realtek sources: each bit should be each
> >  	 * one of the 12 rates, all are enabled */
> > -- 
> > 1.7.3.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
[]'s
Herton

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 4340 bytes --]

On Thu, Nov 04, 2010 at 01:50:37PM -0200, Herton Ronaldo Krzesinski wrote:
> On Thu, 4 Nov 2010 13:30:57 -0200
> Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com> wrote:
> 
> > On Mon, Nov 01, 2010 at 10:59:39PM -0200, Herton Ronaldo Krzesinski wrote:
> > > Current 8187B initialization misses anaparam registers restore after
> > > 8187 reset. This causes ANAPARAM register to stay zeroed out (ANAPARAM2
> > > kept its value on my tests). To avoid this, call rtl8187_set_anaparam
> > > right after chip reset (to be on the safe side, as it makes sure we
> > > restore all ANAPARAM registers).
> > > 
> > 
> > Hello, Herton.
> > 
> > Thank you very much for these patches. I am in the process of testing
> > them right now. The first thing I've noticed is a drop in the signal
> > level by 10dBm when using this last patch. Is this something we should
> > be concerned with?
> 
> I think not, may be the signal is a bit weaker with the anaparam now being
> what Realtek uses/recomends (unfortunately it's a magic number and not
> disclosed what each bit/parameter is doing..., probably the buggy previous
> behaviour of the setting being zero after reset was making the signal too
> high when not needed), but as with it we can get higher rate and bandwidth
> on same distance, it shouldn't be a concern (my iperf test results showed
> improvements too with the last two changes).
> 

Hello, again.

I've done some more tests around here and I also get throughput
improvements with the last patch when my AP is in my desk. It does
happen that I have what seems to be a badly assembled card in my
notebook. I can try to get one I can plug outside in the external USB
port.

However, the tests I've done with my card with the AP a little further
away (about 2.7 meters), I cannot ping the machine for more than the
first seconds. Without the last patch, I get 50% loss, and, sometimes,
iperf works and gives me more than 2Kbits/s (that's right, that little).

I have also tried with the AP whithin about 1 meter of distance.
Without the patch, I get no loss and iperf gives me something between
1Mbit/s and 2Mbit/s. With the patch, sometimes I get 4Mbit/s, sometimes
less than 10Kbit/s. In any occasion, with lots of losses using ping.

For reference, my "AP" is, in fact, a netbook using hostapd and a rt73
USB dongle. I am going to send the patches for experimentation in an
environment with many devices far away from the AP. As soon as I get the
results (it may take a while), I'll send them to the list.

Anyway, although I am not very confident about this last patch, perhaps
it should go forward and we get can revert it later if people does
complain about it. Or we should add a comment stating in the code that
it does improve throughput, but may cause packet losses when the devices
are distant. Any thoughts on how to turn this on or off depending on the
case too?

Regards,
Cascardo.

> > 
> > I will do some tests with the distance to the Access Point and send my
> > results later.
> > 
> > Regards,
> > Cascardo.
> > 
> > > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > > Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
> > > Cc: seno <senada@t-online.de>
> > > ---
> > >  drivers/net/wireless/rtl818x/rtl8187_dev.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > > index 4448647..eeee244 100644
> > > --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > > +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > > @@ -771,6 +771,8 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
> > >  	if (res)
> > >  		return res;
> > >  
> > > +	rtl8187_set_anaparam(priv, true);
> > > +
> > >  	/* BRSR (Basic Rate Set Register) on 8187B looks to be the same as
> > >  	 * RESP_RATE on 8187L in Realtek sources: each bit should be each
> > >  	 * one of the 12 rates, all are enabled */
> > > -- 
> > > 1.7.3.2
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> []'s
> Herton

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Herton Ronaldo Krzesinski]

Em Fri, 5 Nov 2010 17:45:50 -0200
Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com> escreveu:
[snip]
> 
> Hello, again.
> 
> I've done some more tests around here and I also get throughput
> improvements with the last patch when my AP is in my desk. It does
> happen that I have what seems to be a badly assembled card in my
> notebook. I can try to get one I can plug outside in the external USB
> port.
> 
> However, the tests I've done with my card with the AP a little further
> away (about 2.7 meters), I cannot ping the machine for more than the
> first seconds. Without the last patch, I get 50% loss, and, sometimes,
> iperf works and gives me more than 2Kbits/s (that's right, that
> little).
> 
> I have also tried with the AP whithin about 1 meter of distance.
> Without the patch, I get no loss and iperf gives me something between
> 1Mbit/s and 2Mbit/s. With the patch, sometimes I get 4Mbit/s,
> sometimes less than 10Kbit/s. In any occasion, with lots of losses
> using ping.

It's strange that with or without the patch you get too low transfer
values. In this case we should have another bug with your device. I don't
know now what could be happening, but first thing that I thought is that
this could be related to antenna selection (and I'm curious about analog
parameters on the eeprom of device too). Please try following debug patch
and post results (also post what type of RTL8187 the driver detects/prints
on kernel log too, may help). And if is something related to antena selection,
try change the if condition on the patch and see if that helps.

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index eeee244..d8271f0 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1538,6 +1538,18 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
 #endif
 	rtl8187_rfkill_init(dev);
 
+	eeprom_93cx6_read(&eeprom, 0x3F, &reg);
+	printk("EEPROM_SW_REVD == 0x%04x\n", reg);
+
+	eeprom_93cx6_read(&eeprom, 0xe, &reg);
+	printk("ANA_PARM eeprom value: 0x%04x", reg);
+	eeprom_93cx6_read(&eeprom, 0xd, &reg);
+	printk("%04x\n", reg);
+	eeprom_93cx6_read(&eeprom, 0x19, &reg);
+	printk("ANA_PARM2 eeprom value: 0x%04x", reg);
+	eeprom_93cx6_read(&eeprom, 0x1a, &reg);
+	printk("%04x\n", reg);
+
 	return 0;
 
  err_free_dmabuf:
diff --git a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
index 5c6666f..15c0165 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
@@ -877,7 +877,6 @@ static void rtl8225z2_b_rf_init(struct ieee80211_hw *dev)
 
 	rtl818x_iowrite8(priv, &priv->map->TX_GAIN_CCK, 0x03);
 	rtl818x_iowrite8(priv, &priv->map->TX_GAIN_OFDM, 0x07);
-	rtl818x_iowrite8(priv, &priv->map->TX_ANTENNA, 0x03);
 
 	rtl8225_write_phy_ofdm(dev, 0x80, 0x12);
 	for (i = 0; i < ARRAY_SIZE(rtl8225z2_agc); i++) {
@@ -894,6 +893,17 @@ static void rtl8225z2_b_rf_init(struct ieee80211_hw *dev)
 	rtl8225_write_phy_ofdm(dev, 0xa4, 0xb6);
 	rtl8225_write_phy_ofdm(dev, 0x85, 0xfc);
 	rtl8225_write_phy_cck(dev, 0xc1, 0x88);
+
+	if (0) {
+		rtl8225_write_phy_cck(dev, 0x10, 0xdb);
+		rtl8225_write_phy_ofdm(dev, 0x26, 0x10);
+		rtl818x_iowrite8(priv, &priv->map->TX_ANTENNA, 0x00);
+	} else {
+		rtl8225_write_phy_cck(dev, 0x10, 0x9b);
+		rtl8225_write_phy_ofdm(dev, 0x26, 0x90);
+		rtl818x_iowrite8(priv, &priv->map->TX_ANTENNA, 0x03);
+	}
+	msleep(1);
 }
 
 static void rtl8225_rf_stop(struct ieee80211_hw *dev)


> 
> For reference, my "AP" is, in fact, a netbook using hostapd and a rt73
> USB dongle. I am going to send the patches for experimentation in an
> environment with many devices far away from the AP. As soon as I get
> the results (it may take a while), I'll send them to the list.
> 
> Anyway, although I am not very confident about this last patch,
> perhaps it should go forward and we get can revert it later if people
> does complain about it. Or we should add a comment stating in the
> code that it does improve throughput, but may cause packet losses
> when the devices are distant. Any thoughts on how to turn this on or
> off depending on the case too?
> 
> Regards,
> Cascardo.

--
[]'s
Herton

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Hin-Tak Leung]

--- On Sun, 7/11/10, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:

<snipped>
> It's strange that with or without the patch you get too low
> transfer
> values. In this case we should have another bug with your
> device. I don't
> know now what could be happening, but first thing that I
> thought is that
> this could be related to antenna selection (and I'm curious
> about analog
> parameters on the eeprom of device too). Please try
> following debug patch
> and post results (also post what type of RTL8187 the driver
> detects/prints
> on kernel log too, may help). And if is something related
> to antena selection,
> try change the if condition on the patch and see if that
> helps.
<snipped>

That reminds me - I have an "antenna diversity" patch dated "April 9 2009" on my desktop which I think I got off linux-wireless from somebody with a dual-antenna device, derived from scavenging from the vendor driver, I think, that we are supposed to do something about but I certainly haven't done anything about it. Did either/any of you look at that and/or remember having done anything about that patch? 



      

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Herton Ronaldo Krzesinski]

Em Sun, 7 Nov 2010 01:29:30 +0000 (GMT)
Hin-Tak Leung <htl10@users.sourceforge.net> escreveu:

> --- On Sun, 7/11/10, Herton Ronaldo Krzesinski
> <herton@mandriva.com.br> wrote:
> 
> <snipped>
> > It's strange that with or without the patch you get too low
> > transfer
> > values. In this case we should have another bug with your
> > device. I don't
> > know now what could be happening, but first thing that I
> > thought is that
> > this could be related to antenna selection (and I'm curious
> > about analog
> > parameters on the eeprom of device too). Please try
> > following debug patch
> > and post results (also post what type of RTL8187 the driver
> > detects/prints
> > on kernel log too, may help). And if is something related
> > to antena selection,
> > try change the if condition on the patch and see if that
> > helps.
> <snipped>
> 
> That reminds me - I have an "antenna diversity" patch dated "April 9
> 2009" on my desktop which I think I got off linux-wireless from
> somebody with a dual-antenna device, derived from scavenging from the
> vendor driver, I think, that we are supposed to do something about
> but I certainly haven't done anything about it. Did either/any of you
> look at that and/or remember having done anything about that patch? 

Do you have any pointer to it? (I can't find in the archives in April
9 2009). Never saw it, if was posted in the mailing list certainly
missed it (and probably I was not in CC).

--
[]'s
Herton

Re: [PATCH 9/9] rtl8187: restore anaparam registers after reset with 8187B

[Larry Finger]

On 11/06/2010 10:28 PM, Herton Ronaldo Krzesinski wrote:
> Em Sun, 7 Nov 2010 01:29:30 +0000 (GMT)
> Hin-Tak Leung <htl10@users.sourceforge.net> escreveu:
> 
>> --- On Sun, 7/11/10, Herton Ronaldo Krzesinski
>> <herton@mandriva.com.br> wrote:
>>
>> <snipped>
>>> It's strange that with or without the patch you get too low
>>> transfer
>>> values. In this case we should have another bug with your
>>> device. I don't
>>> know now what could be happening, but first thing that I
>>> thought is that
>>> this could be related to antenna selection (and I'm curious
>>> about analog
>>> parameters on the eeprom of device too). Please try
>>> following debug patch
>>> and post results (also post what type of RTL8187 the driver
>>> detects/prints
>>> on kernel log too, may help). And if is something related
>>> to antena selection,
>>> try change the if condition on the patch and see if that
>>> helps.
>> <snipped>
>>
>> That reminds me - I have an "antenna diversity" patch dated "April 9
>> 2009" on my desktop which I think I got off linux-wireless from
>> somebody with a dual-antenna device, derived from scavenging from the
>> vendor driver, I think, that we are supposed to do something about
>> but I certainly haven't done anything about it. Did either/any of you
>> look at that and/or remember having done anything about that patch? 
> 
> Do you have any pointer to it? (I can't find in the archives in April
> 9 2009). Never saw it, if was posted in the mailing list certainly
> missed it (and probably I was not in CC).

There is a reference to it at
http://www.spinics.net/lists/linux-wireless/msg31345.html. The patches were
never done quite right, and I do not remember testing. As I have only 1 antenna
(I think), it likely would not have affected the results.

Larry

Re: [PATCH 0/9] rtl8187: start cleanup/revisiting code

[Herton Ronaldo Krzesinski]

On 01-11-2010 22:59, Herton Ronaldo Krzesinski wrote:
> Hi,
>
> While looking at rtl8187 driver I noticed possible cleanups and found 2
> bugs (last 2 patches in the series). This series presents the changes.
> I didn't finished revisiting everything, so there are more cleanups
> possible, but for now the 2 bug fixes are worth the initial submit.

Sorry, forgot to add that this series should be applied after "rtl8187b: 
do not do per packet TX AGC" posted last week on linux-wireless (first 
patch probably will conflict/not apply without it).

--
[]'s
Herton