On Wed, 14 Mar 2007 00:14:00 -0500
Larry Finger <larry.finger@xxxxxxxxxxxx> wrote:
> 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.
>
> Stefano Brivio wrote:
> > 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 <stefano.brivio@xxxxxxxxx>
> > ----
> >
> > diff --git a/drivers/net/wireless/bcm43xx/bcm43xx.h
> > b/drivers/net/wireless/bcm43xx/bcm43xx.h index 95ff175..7023327 100644
> > --- a/drivers/net/wireless/bcm43xx/bcm43xx.h
> > +++ b/drivers/net/wireless/bcm43xx/bcm43xx.h
> > @@ -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;
> 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.
> > + 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;
> 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.
> > +};
> > +
> > 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;
> >
> > diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> > 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
> > @@ -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;
> > +
> > + 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.
> > + 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.
> > + 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
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
