Skip to contentSkip to conversation

bcm43xx: ACI fixes

PATCH NAK Stefano Brivio 3 participants 10 messages Mar 12, 2007 – Mar 14, 2007 Networking drivers (wireless)

lore
Share & tools
lore

Stefano Brivio

author
#0 lore raw

PATCH 1/1 bcm43xx: ACI fixes

Please test the following patch. It syncs ACI (Adjacent Channels Interference) code to specs. I can't test it right now. Will port to mac80211 branch ASAP.

Signed-off-by Stefano Brivio <[email protected]>

----

drivers/net/wireless/bcm43xx/bcm43xx.h +27 -2
index 95ff175..7023327 100644
@@ -398,6 +398,9 @@
 #define BCM43xx_DEFAULT_SHORT_RETRY_LIMIT	7
 #define BCM43xx_DEFAULT_LONG_RETRY_LIMIT	4
 
+/* Statscounter offsets. */
+#define BCM43xx_SC_ACI		0x2E
+
 /* FIXME: the next line is a guess as to what the maximum RSSI value might be */
 #define RX_RSSI_MAX				60
 

@@ -550,7 +553,11 @@ struct bcm43xx_phyinfo {
 	u8 connected:1,
 	   calibrated:1,
 	   is_locked:1, /* used in bcm43xx_phy_{un}lock() */
-	   dyn_tssi_tbl:1; /* used in bcm43xx_phy_init_tssi2dbm_table() */
+	   dyn_tssi_tbl:1, /* used in bcm43xx_phy_init_tssi2dbm_table() */
+	   g_interfmode:1; /* bit 4000 in PHY_G_CRS, used in periodic tasks */
+	u8 g_interfmode_timer; /* how much time ago g_interfmode was unset */
+	u16 g_sc_saved; /* used in periodic tasks for ACI */
+
 	/* LO Measurement Data.
 	 * Use bcm43xx_get_lopair() to get a value.
 	 */

@@ -629,7 +636,9 @@ struct bcm43xx_radioinfo {
 	/* ACI (adjacent channel interference) flags. */
 	u8 aci_enable:1,
 	   aci_wlan_automatic:1,
-	   aci_hw_rssi:1;
+	   aci_hw_rssi:1,
+	   aci_delay:5;
+	u8 aci_start;
 };
 
 /* Data structures for DMA transmission, per 80211 core. */

@@ -699,6 +708,18 @@ struct bcm43xx_noise_calculation {
 	s8 samples[8][4];
 };
 
+/* Statscounter data (currently ACI only). */
+struct bcm43xx_statscounter_saved {
+	u16 aci;
+};
+
+/* Values for ACI moving average calculation. */
+struct bcm43xx_aci_saved {
+	u16 value[8];
+	u8 next:3,
+	   set:3;
+};
+
 struct bcm43xx_stats {
 	u8 noise;
 	struct iw_statistics wstats;

@@ -812,6 +833,10 @@ struct bcm43xx_private {
 	u32 irq_savedstate;
 	/* Link Quality calculation context. */
 	struct bcm43xx_noise_calculation noisecalc;
+	/* Statscounter tracking. */
+	struct bcm43xx_statscounter_saved sc;
+	/* ACI tracking. */
+	struct bcm43xx_aci_saved aci;
 	/* if > 0 MAC is suspended. if == 0 MAC is enabled. */
 	int mac_suspended;
drivers/net/wireless/bcm43xx/bcm43xx_main.c +92 -15

147 lines not on this page · raw message

drivers/net/wireless/bcm43xx/bcm43xx_phy.c +0 -0
NAK
#1 lore raw
Quoted 7 lines · Stefano Brivio

Please test the following patch. It syncs ACI (Adjacent Channels Interference) code to specs. I can't test it right now. Will port to mac80211 branch ASAP.

Signed-off-by Stefano Brivio <[email protected]>

----

NACK - this patch really kills performance on my 4311. I'll see if I can find the problem.

Larry

#2 lore raw
Quoted 10 lines · Larry Finger
Quoted 7 lines · Stefano Brivio

Please test the following patch. It syncs ACI (Adjacent Channels Interference) code to specs. I can't test it right now. Will port to mac80211 branch ASAP.

Signed-off-by Stefano Brivio <[email protected]>

----

NACK - this patch really kills performance on my 4311. I'll see if I can find the problem.

Forget the above. The performance was killed by my AP, which needed to be rebooted. I will send this patch on to John.

Larry

author
#3 lore raw
Quoted 7 lines · On Mon, 12 Mar 2007 00:51:19 -0500 Larry Finger <[email protected]>…
Quoted 6 lines · Larry Finger
Quoted 5 lines · Stefano Brivio

Please test the following patch. It syncs ACI (Adjacent Channels Interference) code to specs. I can't test it right now. Will port to mac80211 branch ASAP.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Signed-off-by Stefano Brivio <[email protected]>

----

NACK - this patch really kills performance on my 4311. I'll see if I can find the problem.

Forget the above. The performance was killed by my AP, which needed to be rebooted. I will send this patch on to John.

Well, wait. I'd want to see some performance improvements before the patch is applied. That's why I didn't send it to John and put RFT in the subject. Are there any?

--
Ciao
Stefano

#4 lore raw
Quoted 5 lines · Stefano Brivio

On Mon, 12 Mar 2007 00:51:19 -0500

Well, wait. I'd want to see some performance improvements before the patch is applied. That's why I didn't send it to John and put RFT in the subject. Are there any?

No, I don't see any significant difference in RX or TX throughput with this patch. So far, I have only tested on the 4311. Results for the 4306 and 4318 will come later.

Larry

#5 lore raw
Quoted 9 lines · Larry Finger
Quoted 5 lines · Stefano Brivio

On Mon, 12 Mar 2007 00:51:19 -0500

Well, wait. I'd want to see some performance improvements before the patch is applied. That's why I didn't send it to John and put RFT in the subject. Are there any?

No, I don't see any significant difference in RX or TX throughput with this patch. So far, I have only tested on the 4311. Results for the 4306 and 4318 will come later.

Not that interference mitigation can not improve the signal, if there is no interference disturbing it. ;) It _is_ already best possible signal, then.

Signature · 1 line
Greetings Michael.
#6 lore raw
Quoted 7 lines · Michael Buesch
Quoted 6 lines · Larry Finger
Quoted 5 lines · Stefano Brivio

On Mon, 12 Mar 2007 00:51:19 -0500

Well, wait. I'd want to see some performance improvements before the patch is applied. That's why I didn't send it to John and put RFT in the subject. Are there any?

Quoted 6 lines
Quoted 2 lines

No, I don't see any significant difference in RX or TX throughput with this patch. So far, I have only tested on the 4311. Results for the 4306 and 4318 will come later.

Not that interference mitigation can not improve the signal, if there is no interference disturbing it. ;) It _is_ already best possible signal, then.

Of course. My environment is rather quiet. At least, the patch does no harm.

Larry

#7 lore raw
In reply to Stefano Brivio

Stefano,

As noted earlier on the list, I have started testing this patch. In setting the various interference modes, I have found some bugs in other places that were not previously tested. Thanks for this contribution.

I have some comments below.

Quoted 36 lines · Stefano Brivio

Please test the following patch. It syncs ACI (Adjacent Channels Interference) code to specs. I can't test it right now. Will port to mac80211 branch ASAP.

Signed-off-by Stefano Brivio <[email protected]>

----

drivers/net/wireless/bcm43xx/bcm43xx.h +8 -1
index 95ff175..7023327 100644
@@ -398,6 +398,9 @@
 #define BCM43xx_DEFAULT_SHORT_RETRY_LIMIT	7
 #define BCM43xx_DEFAULT_LONG_RETRY_LIMIT	4
 
+/* Statscounter offsets. */
+#define BCM43xx_SC_ACI		0x2E
+
 /* FIXME: the next line is a guess as to what the maximum RSSI value might be */
 #define RX_RSSI_MAX				60
 

@@ -550,7 +553,11 @@ struct bcm43xx_phyinfo {
 	u8 connected:1,
 	   calibrated:1,
 	   is_locked:1, /* used in bcm43xx_phy_{un}lock() */
-	   dyn_tssi_tbl:1; /* used in bcm43xx_phy_init_tssi2dbm_table() */
+	   dyn_tssi_tbl:1, /* used in bcm43xx_phy_init_tssi2dbm_table() */
+	   g_interfmode:1; /* bit 4000 in PHY_G_CRS, used in periodic tasks */
+	u8 g_interfmode_timer; /* how much time ago g_interfmode was unset */
+	u16 g_sc_saved; /* used in periodic tasks for ACI */
+
 	/* LO Measurement Data.
 	 * Use bcm43xx_get_lopair() to get a value.
 	 */

drivers/net/wireless/bcm43xx/bcm43xx.h struct bcm43xx_radioinfo { · line 640

Quoted 6 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +2 -1
@@ -629,7 +636,9 @@ struct bcm43xx_radioinfo {
 	/* ACI (adjacent channel interference) flags. */
 	u8 aci_enable:1,
 	   aci_wlan_automatic:1,
-	   aci_hw_rssi:1;
+	   aci_hw_rssi:1,
+	   aci_delay:5;

Should this be a bit field? Why not just make it a u8?

Quoted 8 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +1 -0
+	u8 aci_start;
 };
 
 /* Data structures for DMA transmission, per 80211 core. */

@@ -699,6 +708,18 @@ struct bcm43xx_noise_calculation {
 	s8 samples[8][4];
 };

drivers/net/wireless/bcm43xx/bcm43xx.h struct bcm43xx_noise_calculation { · line 720

Quoted 10 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +10 -0
+/* Statscounter data (currently ACI only). */
+struct bcm43xx_statscounter_saved {
+	u16 aci;
+};
+
+/* Values for ACI moving average calculation. */
+struct bcm43xx_aci_saved {
+	u16 value[8];
+	u8 next:3,
+	   set:3;

As I will explain later, I don't think 'set' is needed. I also recommend having 'next' be a plain u8.

Quoted 39 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +6 -0
+};
+
 struct bcm43xx_stats {
 	u8 noise;
 	struct iw_statistics wstats;

@@ -812,6 +833,10 @@ struct bcm43xx_private {
 	u32 irq_savedstate;
 	/* Link Quality calculation context. */
 	struct bcm43xx_noise_calculation noisecalc;
+	/* Statscounter tracking. */
+	struct bcm43xx_statscounter_saved sc;
+	/* ACI tracking. */
+	struct bcm43xx_aci_saved aci;
 	/* if > 0 MAC is suspended. if == 0 MAC is enabled. */
 	int mac_suspended;
drivers/net/wireless/bcm43xx/bcm43xx_main.c +15 -0
index e594af4..27ec519 100644
@@ -3059,6 +3059,44 @@ static void bcm43xx_pcie_mdio_write(stru
 	bcm43xx_write32(bcm, BCM43xx_PCIECORE_MDIO_CTL, 0);
 }
 
+/* We don't use any statscounter other than ACI for now */
+static u16 bcm43xx_sc_read(struct bcm43xx_private *bcm)
+{
+	struct bcm43xx_statscounter_saved *sc = &bcm->sc;
+	u16 tmp;
+
+	tmp = bcm43xx_shm_read16(bcm, BCM43xx_SHM_SHARED, 0x80 + BCM43xx_SC_ACI)
+		- sc->aci;
+	if (tmp)
+		sc->aci += tmp;
+
+	return tmp;
+}
+
+static u16 bcm43xx_aci_moving_average(struct bcm43xx_private *bcm)

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3086

Quoted 10 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +10 -0
+{
+	struct bcm43xx_aci_saved *aci = &bcm->aci;
+
+	u8 i;
+	u16 avg;
+	u32 sum = 0;
+
+	aci->value[aci->next] = bcm43xx_sc_read(bcm);
+	if (unlikely(aci->set < 7))
+		aci->set++;

I don't think the variable aci->set is needed as the divisor is alway 8 in the calculation of the average below. Any error in the average shouldn't be important as it will occur only in the first cycle through the value buffer. The for loop below should end at i < 8. There will be extra calculations only during the first buffer cycle.

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3089

Quoted 3 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +3 -0
+	for (i = 0; i < aci->set; i++)
+		sum += aci->value[i];
+	avg = sum / (u16)8;

Why the cast here? I didn't get any gcc warnings without it. I expect this ends up as a shift right anyway.

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3095

Quoted 6 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +6 -0
+	if ((sum % 8) > 3)
+		avg++;
+	if (aci->next < 7)
+		aci->next++;
+	else
+		aci->next = 0;

Why not auto-increment aci->next in the line where bcm43xx_sc_read is called. Then you only need to test for next > 7 and reset it to zero then.

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3098

Quoted 3 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +3 -0
+
+	return avg;
+}

Larry

author
#8 lore raw
Quoted 48 lines · On Wed, 14 Mar 2007 00:14:00 -0500 Larry Finger <[email protected]>…

Stefano,

As noted earlier on the list, I have started testing this patch. In setting the various interference modes, I have found some bugs in other places that were not previously tested. Thanks for this contribution.

I have some comments below.

Quoted 39 lines · Stefano Brivio

Please test the following patch. It syncs ACI (Adjacent Channels Interference) code to specs. I can't test it right now. Will port to mac80211 branch ASAP.

Signed-off-by Stefano Brivio <[email protected]>

----

(quoted diff) +0 -0

b/drivers/net/wireless/bcm43xx/bcm43xx.h index 95ff175..7023327 100644

drivers/net/wireless/bcm43xx/bcm43xx.h +3 -0
@@ -398,6 +398,9 @@
 #define BCM43xx_DEFAULT_SHORT_RETRY_LIMIT	7
 #define BCM43xx_DEFAULT_LONG_RETRY_LIMIT	4
 
+/* Statscounter offsets. */
+#define BCM43xx_SC_ACI		0x2E
+
 /* FIXME: the next line is a guess as to what the maximum RSSI value

might be */ #define RX_RSSI_MAX 60

drivers/net/wireless/bcm43xx/bcm43xx.h +0 -1
@@ -550,7 +553,11 @@ struct bcm43xx_phyinfo {
 	u8 connected:1,
 	   calibrated:1,
 	   is_locked:1, /* used in bcm43xx_phy_{un}lock() */
-	   dyn_tssi_tbl:1; /* used in

bcm43xx_phy_init_tssi2dbm_table() */

drivers/net/wireless/bcm43xx/bcm43xx.h +1 -0
+	   dyn_tssi_tbl:1, /* used in

bcm43xx_phy_init_tssi2dbm_table() */

drivers/net/wireless/bcm43xx/bcm43xx.h +1 -0
+	   g_interfmode:1; /* bit 4000 in PHY_G_CRS, used in periodic

tasks */

drivers/net/wireless/bcm43xx/bcm43xx.h +1 -0
+	u8 g_interfmode_timer; /* how much time ago g_interfmode was

unset */

drivers/net/wireless/bcm43xx/bcm43xx.h +2 -0
+	u16 g_sc_saved; /* used in periodic tasks for ACI */
+
 	/* LO Measurement Data.
 	 * Use bcm43xx_get_lopair() to get a value.
 	 */
Quoted 8 lines

drivers/net/wireless/bcm43xx/bcm43xx.h struct bcm43xx_radioinfo { · line 640

Quoted 7 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +2 -1
@@ -629,7 +636,9 @@ struct bcm43xx_radioinfo {
 	/* ACI (adjacent channel interference) flags. */
 	u8 aci_enable:1,
 	   aci_wlan_automatic:1,
-	   aci_hw_rssi:1;
+	   aci_hw_rssi:1,
+	   aci_delay:5;

Should this be a bit field? Why not just make it a u8?

We only need 5 bits, as aci_delay will never be > 20.

Quoted 8 lines
Quoted 8 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +1 -0
+	u8 aci_start;
 };
 
 /* Data structures for DMA transmission, per 80211 core. */

@@ -699,6 +708,18 @@ struct bcm43xx_noise_calculation {
 	s8 samples[8][4];
 };
Quoted 12 lines

drivers/net/wireless/bcm43xx/bcm43xx.h struct bcm43xx_noise_calculation { · line 720

Quoted 10 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +10 -0
+/* Statscounter data (currently ACI only). */
+struct bcm43xx_statscounter_saved {
+	u16 aci;
+};
+
+/* Values for ACI moving average calculation. */
+struct bcm43xx_aci_saved {
+	u16 value[8];
+	u8 next:3,
+	   set:3;

As I will explain later, I don't think 'set' is needed. I also recommend having 'next' be a plain u8.

Both values will never be > 7.

Quoted 47 lines
Quoted 47 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +6 -0
+};
+
 struct bcm43xx_stats {
 	u8 noise;
 	struct iw_statistics wstats;

@@ -812,6 +833,10 @@ struct bcm43xx_private {
 	u32 irq_savedstate;
 	/* Link Quality calculation context. */
 	struct bcm43xx_noise_calculation noisecalc;
+	/* Statscounter tracking. */
+	struct bcm43xx_statscounter_saved sc;
+	/* ACI tracking. */
+	struct bcm43xx_aci_saved aci;
 	/* if > 0 MAC is suspended. if == 0 MAC is enabled. */
 	int mac_suspended;
(quoted diff) +0 -0

b/drivers/net/wireless/bcm43xx/bcm43xx_main.c index e594af4..27ec519 100644 --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c

drivers/net/wireless/bcm43xx/bcm43xx_main.c +22 -0
@@ -3059,6 +3059,44 @@ static void bcm43xx_pcie_mdio_write(stru
 	bcm43xx_write32(bcm, BCM43xx_PCIECORE_MDIO_CTL, 0);
 }
 
+/* We don't use any statscounter other than ACI for now */
+static u16 bcm43xx_sc_read(struct bcm43xx_private *bcm)
+{
+	struct bcm43xx_statscounter_saved *sc = &bcm->sc;
+	u16 tmp;
+
+	tmp = bcm43xx_shm_read16(bcm, BCM43xx_SHM_SHARED, 0x80 +
 BCM43xx_SC_ACI)
+		- sc->aci;
+	if (tmp)
+		sc->aci += tmp;
+
+	return tmp;
+}
+
+static u16 bcm43xx_aci_moving_average(struct bcm43xx_private *bcm)
+{
+	struct bcm43xx_aci_saved *aci = &bcm->aci;
+
+	u8 i;
+	u16 avg;
+	u32 sum = 0;
+
Quoted 8 lines

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3086

Quoted 3 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +3 -0
+	aci->value[aci->next] = bcm43xx_sc_read(bcm);
+	if (unlikely(aci->set < 7))
+		aci->set++;

I don't think the variable aci->set is needed as the divisor is alway 8 in the calculation of the average below. Any error in the average shouldn't be important as it will occur only in the first cycle through the value buffer. The for loop below should end at i < 8. There will be extra calculations only during the first buffer cycle.

Well, first cycle would last 8 seconds. I think that it's quite relevant.

Quoted 5 lines

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3089

Quoted 3 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +3 -0
+	for (i = 0; i < aci->set; i++)
+		sum += aci->value[i];
+	avg = sum / (u16)8;

Why the cast here? I didn't get any gcc warnings without it. I expect this ends up as a shift right anyway.

Right.

Quoted 8 lines

drivers/net/wireless/bcm43xx/bcm43xx_main.c static void bcm43xx_pcie_mdio_write(stru · line 3095

Quoted 6 lines
drivers/net/wireless/bcm43xx/bcm43xx_main.c +6 -0
+	if ((sum % 8) > 3)
+		avg++;
+	if (aci->next < 7)
+		aci->next++;
+	else
+		aci->next = 0;

Why not auto-increment aci->next in the line where bcm43xx_sc_read is called. Then you only need to test for next > 7 and reset it to zero then.

It looks to be the same thing to me. Anyway, ok, your way is more elegant.

--
Ciao
Stefano

#9 lore raw
Quoted 6 lines · Stefano Brivio
Quoted 4 lines

drivers/net/wireless/bcm43xx/bcm43xx.h struct bcm43xx_radioinfo { · line 640

Quoted 3 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +2 -1
-	   aci_hw_rssi:1;
+	   aci_hw_rssi:1,
+	   aci_delay:5;

Should this be a bit field? Why not just make it a u8?

We only need 5 bits, as aci_delay will never be > 20.

I actually try to move away from the foo:x notation, as it generates bad code. There's no benefit in saving 3 bits here and adding 100byte of additional code, which is generated by gcc to mask and shift the bits. :)

Quoted 21 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +1 -0
+	u8 aci_start;
 };
 
 /* Data structures for DMA transmission, per 80211 core. */

@@ -699,6 +708,18 @@ struct bcm43xx_noise_calculation {
 	s8 samples[8][4];
 };
Quoted 12 lines

drivers/net/wireless/bcm43xx/bcm43xx.h struct bcm43xx_noise_calculation { · line 720

Quoted 10 lines
drivers/net/wireless/bcm43xx/bcm43xx.h +10 -0
+/* Statscounter data (currently ACI only). */
+struct bcm43xx_statscounter_saved {
+	u16 aci;
+};
+
+/* Values for ACI moving average calculation. */
+struct bcm43xx_aci_saved {
+	u16 value[8];
+	u8 next:3,
+	   set:3;

As I will explain later, I don't think 'set' is needed. I also recommend having 'next' be a plain u8.

Quoted 1 line

Both values will never be > 7.

Please do u8 nevertheless to get better code.

Signature · 1 line
Greetings Michael.