ath5k: fix I/Q calibration (for real)

PATCH Superseded Bruno Randolf 4 participants 15 messages Mar 8, 2010 – Mar 9, 2010 Atheros ATH5K wireless driver

Share & tools

Bruno Randolf

author

2010-03-08 02:59 UTC #0 lore raw

PATCH v2 1/1 ath5k: fix I/Q calibration (for real)

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(-)

drivers/net/wireless/ath/ath5k/phy.c +19 ~~-18~~

index 3fa4f4d..b7f2949 100644
@@ -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 */

drivers/net/wireless/ath/ath5k/reg.h +1 -0

index 4cb9c5d..1464f89 100644
@@ -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 */

Luis R. Rodriguez

2010-03-08 03:57 UTC #1 lore raw

Quoted 19 lines · Bruno Randolf

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.

Quoted 7 lines

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

Bruno Randolf

author

2010-03-08 04:17 UTC #2 lore raw

Quoted 24 lines · On Monday 08 March 2010 12:56:52 Luis R. Rodriguez wrote:

Quoted 23 lines · Bruno Randolf
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.

Quoted 5 lines

Quoted 3 lines
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

Jorge Boncompte [DTI2]

2010-03-08 12:51 UTC #12 lore raw

In reply to Bruno Randolf

Quoted 5 lines · Bruno Randolf

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.

Signature · 10 lines

==============================================================
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.
==============================================================

Luis R. Rodriguez

2010-03-08 16:25 UTC #3 lore raw

In reply to Bruno Randolf

Quoted 31 lines · Bruno Randolf

Quoted 24 lines · On Monday 08 March 2010 12:56:52 Luis R. Rodriguez wrote:
Quoted 23 lines · Bruno Randolf
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.
Quoted 5 lines
Quoted 3 lines
Signed-off-by Bruno Randolf <[email protected]>
---
v2: use clamp() as Bob suggested
Thanks Bruno, are these stable fixes?

Quoted 7 lines

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

Bruno Randolf

author

2010-03-09 00:24 UTC #13 lore raw

In reply to Jorge Boncompte [DTI2]

Quoted 8 lines · On Monday 08 March 2010 21:45:55 Jorge Boncompte [DTI2] wrote:

Quoted 3 lines · Bruno Randolf
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

Quoted 2 lines

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

Bruno Randolf

author

2010-03-09 00:35 UTC #4 lore raw

In reply to Luis R. Rodriguez

Quoted 11 lines · On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:

Quoted 10 lines
Quoted 1 line
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.

Quoted 6 lines

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

Jorge Boncompte [DTI2]

2010-03-09 00:39 UTC #14 lore raw

In reply to Bruno Randolf

Quoted 10 lines · Bruno Randolf

Quoted 8 lines · On Monday 08 March 2010 21:45:55 Jorge Boncompte [DTI2] wrote:
Quoted 3 lines · Bruno Randolf
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

Quoted 8 lines

Quoted 2 lines
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.

Signature · 10 lines

==============================================================
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.
==============================================================

Luis R. Rodriguez

2010-03-09 00:47 UTC #5 lore raw

In reply to Bruno Randolf

Quoted 19 lines · Bruno Randolf

Quoted 11 lines · On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
Quoted 10 lines
Quoted 1 line
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.
Quoted 6 lines
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.

Quoted 8 lines

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

Bruno Randolf

author

2010-03-09 00:50 UTC #6 lore raw

Quoted 31 lines · On Tuesday 09 March 2010 09:47:09 Luis R. Rodriguez wrote:

Quoted 22 lines · Bruno Randolf
Quoted 12 lines · On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
Quoted 11 lines
Quoted 1 line
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.
Quoted 6 lines
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.
Quoted 7 lines
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?

Quoted 3 lines

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

Luis R. Rodriguez

2010-03-09 01:21 UTC #7 lore raw

Quoted 32 lines · Bruno Randolf

Quoted 31 lines · On Tuesday 09 March 2010 09:47:09 Luis R. Rodriguez wrote:
Quoted 29 lines · Bruno Randolf
Quoted 12 lines · On Tuesday 09 March 2010 01:24:48 Luis R. Rodriguez wrote:
Quoted 11 lines
Quoted 1 line
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.
Quoted 6 lines
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?

Quoted 5 lines

Quoted 3 lines
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

Bob Copeland

2010-03-09 03:10 UTC #8 lore raw

Quoted 5 lines · Luis R. Rodriguez

Quoted 3 lines · Bruno Randolf
Quoted 3 lines
Quoted 3 lines
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.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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.

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Luis R. Rodriguez

2010-03-09 03:32 UTC #9 lore raw

Quoted 9 lines · Bob Copeland

Quoted 5 lines · Luis R. Rodriguez
Quoted 3 lines · Bruno Randolf
Quoted 3 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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.

Quoted 1 line

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

Bruno Randolf

author

2010-03-09 05:56 UTC #10 lore raw

Quoted 2 lines · On Tuesday 09 March 2010 12:32:33 Luis R. Rodriguez wrote:

Quoted 1 line · Bob Copeland
On Mon, Mar 8, 2010 at 8:21 PM, Luis R. Rodriguez <[email protected]>

Quoted 22 lines · wrote:

Quoted 7 lines
Quoted 4 lines · Bruno Randolf
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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.

Quoted 4 lines

Quoted 1 line
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

Bob Copeland

2010-03-09 06:45 UTC #11 lore raw

Quoted 5 lines · Bruno Randolf

Quoted 2 lines · 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.

Signature · 1 line

Bob Copeland %% www.bobcopeland.com