I/Q calibration was completely broken, resulting in a high number of CRC errors
on received packets. before i could see around 10% to 20% CRC errors, with this
patch they are between 0% and 3%.
1.) the removal of the mask in commit "ath5k: Fix I/Q calibration
(f1cf2dbd0f798b71b1590e7aca6647f2caef1649)" resulted in no mask beeing used
when writing the I/Q values into the register. additional errors in the
calculation of the values (see 2.) resulted too high numbers, exceeding the
masks, so wrong values like 0xfffffffe were written. to be safe we should
always use the bitmask when writing parts of a register.
2.) using a (s32) cast for q_coff is a wrong conversion to signed, since we
convert to a signed value later by substracting 128. this resulted in too low
numbers for Q many times, which were limited to -16 by the boundary check later
on.
3.) checked everything against the HAL sources and took over comments and minor
optimizations from there.
4.) we can't use ENABLE_BITS when we want to write a number (the number can
contain zeros). also always write the correction values first and set ENABLE
bit last, like the HAL does.
Signed-off-by: Bruno Randolf <[email protected]>
---
v2: use clamp() as Bob suggested
---
drivers/net/wireless/ath/ath5k/phy.c | 37 +++++++++++++++++-----------------
drivers/net/wireless/ath/ath5k/reg.h | 1 +
2 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 3fa4f4d..b7f2949 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -1386,38 +1386,39 @@ static int ath5k_hw_rf511x_calibrate(struct ath5k_hw *ah,
goto done;
/* Calibration has finished, get the results and re-run */
+
+ /* workaround against empty results which can apparently happen on 5212 */
for (i = 0; i <= 10; i++) {
iq_corr = ath5k_hw_reg_read(ah, AR5K_PHY_IQRES_CAL_CORR);
i_pwr = ath5k_hw_reg_read(ah, AR5K_PHY_IQRES_CAL_PWR_I);
q_pwr = ath5k_hw_reg_read(ah, AR5K_PHY_IQRES_CAL_PWR_Q);
+ ATH5K_DBG_UNLIMIT(ah->ah_sc, ATH5K_DEBUG_CALIBRATE,
+ "iq_corr:%x i_pwr:%x q_pwr:%x", iq_corr, i_pwr, q_pwr);
+ if (i_pwr && q_pwr)
+ break;
}
i_coffd = ((i_pwr >> 1) + (q_pwr >> 1)) >> 7;
q_coffd = q_pwr >> 7;
- /* No correction */
- if (i_coffd == 0 || q_coffd == 0)
+ /* protect against divide by 0 and loss of sign bits */
+ if (i_coffd == 0 || q_coffd < 2)
goto done;
- i_coff = ((-iq_corr) / i_coffd);
+ i_coff = (-iq_corr) / i_coffd;
+ i_coff = clamp(i_coff, -32, 31); /* signed 6 bit */
- /* Boundary check */
- if (i_coff > 31)
- i_coff = 31;
- if (i_coff < -32)
- i_coff = -32;
+ q_coff = (i_pwr / q_coffd) - 128;
+ q_coff = clamp(q_coff, -16, 15); /* signed 5 bit */
- q_coff = (((s32)i_pwr / q_coffd) - 128);
+ ATH5K_DBG_UNLIMIT(ah->ah_sc, ATH5K_DEBUG_CALIBRATE,
+ "new I:%d Q:%d (i_coffd:%x q_coffd:%x)",
+ i_coff, q_coff, i_coffd, q_coffd);
- /* Boundary check */
- if (q_coff > 15)
- q_coff = 15;
- if (q_coff < -16)
- q_coff = -16;
-
- /* Commit new I/Q value */
- AR5K_REG_ENABLE_BITS(ah, AR5K_PHY_IQ, AR5K_PHY_IQ_CORR_ENABLE |
- ((u32)q_coff) | ((u32)i_coff << AR5K_PHY_IQ_CORR_Q_I_COFF_S));
+ /* Commit new I/Q values (set enable bit last to match HAL sources) */
+ AR5K_REG_WRITE_BITS(ah, AR5K_PHY_IQ, AR5K_PHY_IQ_CORR_Q_I_COFF, i_coff);
+ AR5K_REG_WRITE_BITS(ah, AR5K_PHY_IQ, AR5K_PHY_IQ_CORR_Q_Q_COFF, q_coff);
+ AR5K_REG_ENABLE_BITS(ah, AR5K_PHY_IQ, AR5K_PHY_IQ_CORR_ENABLE);
/* Re-enable calibration -if we don't we'll commit
* the same values again and again */
diff --git a/drivers/net/wireless/ath/ath5k/reg.h b/drivers/net/wireless/ath/ath5k/reg.h
index 4cb9c5d..1464f89 100644
--- a/drivers/net/wireless/ath/ath5k/reg.h
+++ b/drivers/net/wireless/ath/ath5k/reg.h
@@ -2187,6 +2187,7 @@
*/
#define AR5K_PHY_IQ 0x9920 /* Register Address */
#define AR5K_PHY_IQ_CORR_Q_Q_COFF 0x0000001f /* Mask for q correction info */
+#define AR5K_PHY_IQ_CORR_Q_Q_COFF_S 0
#define AR5K_PHY_IQ_CORR_Q_I_COFF 0x000007e0 /* Mask for i correction info */
#define AR5K_PHY_IQ_CORR_Q_I_COFF_S 5
#define AR5K_PHY_IQ_CORR_ENABLE 0x00000800 /* Enable i/q correction */
On Tue, Mar 9, 2010 at 12:56 AM, Bruno Randolf <[email protected]> wrote:
> On Tuesday 09 March 2010 12:32:33 Luis R. Rodriguez wrote:
>> Perhaps a little more elaboration on the commit log on the impact and
>> how this helps and how much would help.
>
> ok. to stop the confusion, i'll add cc: stable.
:) thanks!
FWIW, I tested the patches, and they worked well for my not-very-thorough
tests. I think there was a regression in 2.6.32 time-frame that I never
pinned down, hopefully this corrects some things. I did not test the 5211
stuff though, that one patch is probably not for stable unless someone can
test that.
--
Bob Copeland %% http://www.bobcopeland.com
On Tuesday 09 March 2010 09:47:09 Luis R. Rodriguez wrote:
> On Mon, Mar 8, 2010 at 4:34 PM, Bruno Randolf <[email protected]> wrote:
> > On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
> >> >> Thanks Bruno, are these stable fixes?
> >> >
> >> > hi luis!
> >> >
> >> > i think so. the behaviour before was completely broken, now it's
> >> > better.
> >> >
> >> > but i'm not sure about that whole Cc: [email protected] thing...
> >> > (sorry i've been away for a while)... i read
> >> > Documentation/stable_kernel_rules.txt but still not sure if that
> >> > applies for this patch.
> >>
> >> Just add:
> >>
> >> Cc: [email protected]
> >>
> >> below your Singed-off-by on the commit log entry. That list will get
> >> spammed once the patch is merged on Linus' tree.
> >
> > i understand that.
> >
> > the question is more if my patch justifies bothering 'stable' or not.
> >
> > as i said, in my point of view ath5k has several problems right now
> > (performace and stability), and i guess nobody will be using it seriously
> > in actual production use (does anyone?). so i think it does not really
> > matter if this or any of my other patches go into stable sooner or
> > later. does it?
>
> 2.6.32 will be used by a lot of "enterprise" releases, I'd prefer
> connection stability fixes do indeed go in for 2.6.32 for ath5k, this
> seems like one. I'll let John be the judge.
sure, as i said, i don't mind. :)
bruno
On Mon, Mar 8, 2010 at 4:34 PM, Bruno Randolf <[email protected]> wrote:
> On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
>> >> Thanks Bruno, are these stable fixes?
>> >
>> > hi luis!
>> >
>> > i think so. the behaviour before was completely broken, now it's better.
>> >
>> > but i'm not sure about that whole Cc: [email protected] thing... (sorry
>> > i've been away for a while)... i read
>> > Documentation/stable_kernel_rules.txt but still not sure if that applies
>> > for this patch.
>>
>> Just add:
>>
>> Cc: [email protected]
>>
>> below your Singed-off-by on the commit log entry. That list will get
>> spammed once the patch is merged on Linus' tree.
>
> i understand that.
>
> the question is more if my patch justifies bothering 'stable' or not.
>
> as i said, in my point of view ath5k has several problems right now
> (performace and stability), and i guess nobody will be using it seriously in
> actual production use (does anyone?). so i think it does not really matter if
> this or any of my other patches go into stable sooner or later. does it?
2.6.32 will be used by a lot of "enterprise" releases, I'd prefer
connection stability fixes do indeed go in for 2.6.32 for ath5k, this
seems like one. I'll let John be the judge.
Luis
On Monday 08 March 2010 12:56:52 Luis R. Rodriguez wrote:
> On Sun, Mar 7, 2010 at 6:59 PM, Bruno Randolf <[email protected]> wrote:
> > I/Q calibration was completely broken, resulting in a high number of CRC
> > errors on received packets. before i could see around 10% to 20% CRC
> > errors, with this patch they are between 0% and 3%.
> >
> > 1.) the removal of the mask in commit "ath5k: Fix I/Q calibration
> > (f1cf2dbd0f798b71b1590e7aca6647f2caef1649)" resulted in no mask beeing
> > used when writing the I/Q values into the register. additional errors in
> > the calculation of the values (see 2.) resulted too high numbers,
> > exceeding the masks, so wrong values like 0xfffffffe were written. to be
> > safe we should always use the bitmask when writing parts of a register.
> >
> > 2.) using a (s32) cast for q_coff is a wrong conversion to signed, since
> > we convert to a signed value later by substracting 128. this resulted in
> > too low numbers for Q many times, which were limited to -16 by the
> > boundary check later on.
> >
> > 3.) checked everything against the HAL sources and took over comments and
> > minor optimizations from there.
> >
> > 4.) we can't use ENABLE_BITS when we want to write a number (the number
> > can contain zeros). also always write the correction values first and
> > set ENABLE bit last, like the HAL does.
> >
> > Signed-off-by: Bruno Randolf <[email protected]>
> > ---
> > v2: use clamp() as Bob suggested
>
> Thanks Bruno, are these stable fixes?
hi luis!
i think so. the behaviour before was completely broken, now it's better.
but i'm not sure about that whole Cc: [email protected] thing... (sorry i've
been away for a while)... i read Documentation/stable_kernel_rules.txt but
still not sure if that applies for this patch.
given the current state of ath5k (which is usable but far from being perfect
or performant) i think it does not really matter if this or any other of my
fixes go into stable quickly or not... anyhow i personally don't mind either
way and can also add the CC if you want.
bruno
El 08/03/2010 3:59, Bruno Randolf escribió:
>
> 4.) we can't use ENABLE_BITS when we want to write a number (the number can
> contain zeros). also always write the correction values first and set ENABLE
> bit last, like the HAL does.
>
Hi Bruno,
does not ath5k_hw_commit_eeprom_settings() have the same problem described
above when committing the IQ values from the EEPROM? legacy-hal does differently
too.
--
==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5 14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================
On Mon, Mar 8, 2010 at 8:21 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Mon, Mar 8, 2010 at 4:50 PM, Bruno Randolf <[email protected]> wrote:
>>> > as i said, in my point of view ath5k has several problems right now
>>> > (performace and stability), and i guess nobody will be using it seriously
>>> > in actual production use (does anyone?).
Yes, people do use ath5k in production. Some large companies.
>>> 2.6.32 will be used by a lot of "enterprise" releases, I'd prefer
>>> connection stability fixes do indeed go in for 2.6.32 for ath5k
>> sure, as i said, i don't mind. :)
>
> Alright lets skip stable for this.
Wow this whole line of conversation is confusing :)
If this fixes a calibration bug it needs to go to stable.
--
Bob Copeland %% http://www.bobcopeland.com
On Mon, Mar 8, 2010 at 4:50 PM, Bruno Randolf <[email protected]> wrote:
> On Tuesday 09 March 2010 09:47:09 Luis R. Rodriguez wrote:
>> On Mon, Mar 8, 2010 at 4:34 PM, Bruno Randolf <[email protected]> wrote:
>> > On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
>> >> >> Thanks Bruno, are these stable fixes?
>> >> >
>> >> > hi luis!
>> >> >
>> >> > i think so. the behaviour before was completely broken, now it's
>> >> > better.
>> >> >
>> >> > but i'm not sure about that whole Cc: [email protected] thing...
>> >> > (sorry i've been away for a while)... i read
>> >> > Documentation/stable_kernel_rules.txt but still not sure if that
>> >> > applies for this patch.
>> >>
>> >> Just add:
>> >>
>> >> Cc: [email protected]
>> >>
>> >> below your Singed-off-by on the commit log entry. That list will get
>> >> spammed once the patch is merged on Linus' tree.
>> >
>> > i understand that.
>> >
>> > the question is more if my patch justifies bothering 'stable' or not.
>> >
>> > as i said, in my point of view ath5k has several problems right now
>> > (performace and stability), and i guess nobody will be using it seriously
>> > in actual production use (does anyone?). so i think it does not really
>> > matter if this or any of my other patches go into stable sooner or
>> > later. does it?
>>
>> 2.6.32 will be used by a lot of "enterprise" releases, I'd prefer
>> connection stability fixes do indeed go in for 2.6.32 for ath5k, this
>> seems like one. I'll let John be the judge.
>
> sure, as i said, i don't mind. :)
Alright lets skip stable for this.
Luis
On Mon, Mar 8, 2010 at 7:10 PM, Bob Copeland <[email protected]> wrote:
> On Mon, Mar 8, 2010 at 8:21 PM, Luis R. Rodriguez <[email protected]> wrote:
>> On Mon, Mar 8, 2010 at 4:50 PM, Bruno Randolf <[email protected]> wrote:
>
>>>> > as i said, in my point of view ath5k has several problems right now
>>>> > (performace and stability), and i guess nobody will be using it seriously
>>>> > in actual production use (does anyone?).
>
> Yes, people do use ath5k in production. Some large companies.
>
>>>> 2.6.32 will be used by a lot of "enterprise" releases, I'd prefer
>>>> connection stability fixes do indeed go in for 2.6.32 for ath5k
>
>>> sure, as i said, i don't mind. :)
>>
>> Alright lets skip stable for this.
>
> Wow this whole line of conversation is confusing :)
Hehe. sorry well I was talking to Bruno about the "stable"
qualifications of this fix, and it doesn't fix an oops or serious bug,
but it certainly can improve performance but I haven't myself seen
numbers and would hate to justify just about pushing anything
upstream.
> If this fixes a calibration bug it needs to go to stable.
Perhaps a little more elaboration on the commit log on the impact and
how this helps and how much would help.
Luis
On Monday 08 March 2010 21:45:55 Jorge Boncompte [DTI2] wrote:
> El 08/03/2010 3:59, Bruno Randolf escribió:
> > 4.) we can't use ENABLE_BITS when we want to write a number (the number
> > can contain zeros). also always write the correction values first and
> > set ENABLE bit last, like the HAL does.
>
> Hi Bruno,
>
> does not ath5k_hw_commit_eeprom_settings() have the same problem
described
> above when committing the IQ values from the EEPROM? legacy-hal does
> differently too.
yes, but i have fixed that in the second patch of this series:
[PATCH 2/3] ath5k: read eeprom IQ calibration values correctly for G mode
i forgot to mention that in the changelog.
bruno
On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
> >> Thanks Bruno, are these stable fixes?
> >
> > hi luis!
> >
> > i think so. the behaviour before was completely broken, now it's better.
> >
> > but i'm not sure about that whole Cc: [email protected] thing... (sorry
> > i've been away for a while)... i read
> > Documentation/stable_kernel_rules.txt but still not sure if that applies
> > for this patch.
>
> Just add:
>
> Cc: [email protected]
>
> below your Singed-off-by on the commit log entry. That list will get
> spammed once the patch is merged on Linus' tree.
i understand that.
the question is more if my patch justifies bothering 'stable' or not.
as i said, in my point of view ath5k has several problems right now
(performace and stability), and i guess nobody will be using it seriously in
actual production use (does anyone?). so i think it does not really matter if
this or any of my other patches go into stable sooner or later. does it?
bruno
On Sun, Mar 7, 2010 at 8:17 PM, Bruno Randolf <[email protected]> wrote:
> On Monday 08 March 2010 12:56:52 Luis R. Rodriguez wrote:
>> On Sun, Mar 7, 2010 at 6:59 PM, Bruno Randolf <[email protected]> wrote:
>> > I/Q calibration was completely broken, resulting in a high number of CRC
>> > errors on received packets. before i could see around 10% to 20% CRC
>> > errors, with this patch they are between 0% and 3%.
>> >
>> > 1.) the removal of the mask in commit "ath5k: Fix I/Q calibration
>> > (f1cf2dbd0f798b71b1590e7aca6647f2caef1649)" resulted in no mask beeing
>> > used when writing the I/Q values into the register. additional errors in
>> > the calculation of the values (see 2.) resulted too high numbers,
>> > exceeding the masks, so wrong values like 0xfffffffe were written. to be
>> > safe we should always use the bitmask when writing parts of a register.
>> >
>> > 2.) using a (s32) cast for q_coff is a wrong conversion to signed, since
>> > we convert to a signed value later by substracting 128. this resulted in
>> > too low numbers for Q many times, which were limited to -16 by the
>> > boundary check later on.
>> >
>> > 3.) checked everything against the HAL sources and took over comments and
>> > minor optimizations from there.
>> >
>> > 4.) we can't use ENABLE_BITS when we want to write a number (the number
>> > can contain zeros). also always write the correction values first and
>> > set ENABLE bit last, like the HAL does.
>> >
>> > Signed-off-by: Bruno Randolf <[email protected]>
>> > ---
>> > v2: use clamp() as Bob suggested
>>
>> Thanks Bruno, are these stable fixes?
>
> hi luis!
>
> i think so. the behaviour before was completely broken, now it's better.
>
> but i'm not sure about that whole Cc: [email protected] thing... (sorry i've
> been away for a while)... i read Documentation/stable_kernel_rules.txt but
> still not sure if that applies for this patch.
Just add:
Cc: [email protected]
below your Singed-off-by on the commit log entry. That list will get
spammed once the patch is merged on Linus' tree.
Luis
El 09/03/2010 1:24, Bruno Randolf escribió:
> On Monday 08 March 2010 21:45:55 Jorge Boncompte [DTI2] wrote:
>> El 08/03/2010 3:59, Bruno Randolf escribió:
>>> 4.) we can't use ENABLE_BITS when we want to write a number (the number
>>> can contain zeros). also always write the correction values first and
>>> set ENABLE bit last, like the HAL does.
>>
>> Hi Bruno,
>>
>> does not ath5k_hw_commit_eeprom_settings() have the same problem
> described
>> above when committing the IQ values from the EEPROM? legacy-hal does
>> differently too.
>
> yes, but i have fixed that in the second patch of this series:
>
> [PATCH 2/3] ath5k: read eeprom IQ calibration values correctly for G mode
>
> i forgot to mention that in the changelog.
Sorry, did not noticed that your first submission had 3 patches.
--
==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5 14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================
On Tuesday 09 March 2010 12:32:33 Luis R. Rodriguez wrote:
> On Mon, Mar 8, 2010 at 7:10 PM, Bob Copeland <[email protected]> wrote:
> > On Mon, Mar 8, 2010 at 8:21 PM, Luis R. Rodriguez <[email protected]>
wrote:
> >> On Mon, Mar 8, 2010 at 4:50 PM, Bruno Randolf <[email protected]> wrote:
> >>>> > as i said, in my point of view ath5k has several problems right now
> >>>> > (performace and stability), and i guess nobody will be using it
> >>>> > seriously in actual production use (does anyone?).
> >
> > Yes, people do use ath5k in production. Some large companies.
> >
> >>>> 2.6.32 will be used by a lot of "enterprise" releases, I'd prefer
> >>>> connection stability fixes do indeed go in for 2.6.32 for ath5k
> >>>
> >>> sure, as i said, i don't mind. :)
> >>
> >> Alright lets skip stable for this.
> >
> > Wow this whole line of conversation is confusing :)
>
> Hehe. sorry well I was talking to Bruno about the "stable"
> qualifications of this fix, and it doesn't fix an oops or serious bug,
> but it certainly can improve performance but I haven't myself seen
> numbers and would hate to justify just about pushing anything
> upstream.
>
> > If this fixes a calibration bug it needs to go to stable.
>
> Perhaps a little more elaboration on the commit log on the impact and
> how this helps and how much would help.
ok. to stop the confusion, i'll add cc: stable.
bruno
On Sun, Mar 7, 2010 at 6:59 PM, Bruno Randolf <[email protected]> wrote:
> I/Q calibration was completely broken, resulting in a high number of CRC errors
> on received packets. before i could see around 10% to 20% CRC errors, with this
> patch they are between 0% and 3%.
>
> 1.) the removal of the mask in commit "ath5k: Fix I/Q calibration
> (f1cf2dbd0f798b71b1590e7aca6647f2caef1649)" resulted in no mask beeing used
> when writing the I/Q values into the register. additional errors in the
> calculation of the values (see 2.) resulted too high numbers, exceeding the
> masks, so wrong values like 0xfffffffe were written. to be safe we should
> always use the bitmask when writing parts of a register.
>
> 2.) using a (s32) cast for q_coff is a wrong conversion to signed, since we
> convert to a signed value later by substracting 128. this resulted in too low
> numbers for Q many times, which were limited to -16 by the boundary check later
> on.
>
> 3.) checked everything against the HAL sources and took over comments and minor
> optimizations from there.
>
> 4.) we can't use ENABLE_BITS when we want to write a number (the number can
> contain zeros). also always write the correction values first and set ENABLE
> bit last, like the HAL does.
>
> Signed-off-by: Bruno Randolf <[email protected]>
> ---
> v2: use clamp() as Bob suggested
Thanks Bruno, are these stable fixes?
Luis