Search Linux Wireless

Re: [PATCH 1/2] b43: implement PPR (Power Per Rate) management/API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jul 20, 2014 at 1:00 PM, Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
> Broadcom hardware supports auto-adjustment of TX power depending on the
> currently used rate. So far all calculations were handled without any
> helpers (API) using big arrays and magic offsets.
> It seems Broadcom recently decided to clean this up by developing PPR.
> Their wlc_ppr.h can be found in open parts of the SDK.
> As we plan to implement support for rate-based TX power it makes sense
> to also implement our version of PPR as well.

Nice one.

> Signed-off-by: Rafał Miłecki <zajec5@xxxxxxxxx>
> ---
>  drivers/net/wireless/b43/Makefile |   1 +
>  drivers/net/wireless/b43/b43.h    |   7 ++
>  drivers/net/wireless/b43/ppr.c    | 210 ++++++++++++++++++++++++++++++++++++++
>  drivers/net/wireless/b43/ppr.h    |  21 ++++
>  4 files changed, 239 insertions(+)
>  create mode 100644 drivers/net/wireless/b43/ppr.c
>  create mode 100644 drivers/net/wireless/b43/ppr.h
>
> diff --git a/drivers/net/wireless/b43/Makefile b/drivers/net/wireless/b43/Makefile
> index 6e00b88..9f7965a 100644
> --- a/drivers/net/wireless/b43/Makefile
> +++ b/drivers/net/wireless/b43/Makefile
> @@ -18,6 +18,7 @@ b43-y                         += xmit.o
>  b43-y                          += dma.o
>  b43-y                          += pio.o
>  b43-y                          += rfkill.o
> +b43-y                          += ppr.o

Is this only used for N-PHY, or also for others? If not, it might make
sense to only include it if N-PHY support is selected. Not sure if
it's large enough to matter much.

>  b43-$(CONFIG_B43_LEDS)         += leds.o
>  b43-$(CONFIG_B43_PCMCIA)       += pcmcia.o
>  b43-$(CONFIG_B43_SDIO)         += sdio.o
> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
> index 4113b69..6cfd86d 100644
> --- a/drivers/net/wireless/b43/b43.h
> +++ b/drivers/net/wireless/b43/b43.h
> @@ -791,6 +791,13 @@ struct b43_firmware {
>         bool pcm_request_failed;
>  };
>
> +enum b43_band {
> +       B43_BAND_2G = 0,
> +       B43_BAND_5G_LO = 1,
> +       B43_BAND_5G_MI = 2,
> +       B43_BAND_5G_HI = 3,
> +};
> +
>  /* Device (802.11 core) initialization status. */
>  enum {
>         B43_STAT_UNINIT = 0,    /* Uninitialized. */
> diff --git a/drivers/net/wireless/b43/ppr.c b/drivers/net/wireless/b43/ppr.c
> new file mode 100644
> index 0000000..36c4b419
> --- /dev/null
> +++ b/drivers/net/wireless/b43/ppr.c
> @@ -0,0 +1,210 @@
> +#include "ppr.h"
> +#include "b43.h"

This file could use a license/copyright header. It looks a bit odd without one.

> +
> +#define B43_PPR_CCK_RATES_NUM          4
> +#define B43_PPR_OFDM_RATES_NUM         8
> +#define B43_PPR_MCS_RATES_NUM          8
> +
> +#define B43_PPR_RATES_NUM      (B43_PPR_CCK_RATES_NUM +        \
> +                                B43_PPR_OFDM_RATES_NUM * 2 +   \
> +                                B43_PPR_MCS_RATES_NUM * 4)
> +
> +struct b43_ppr_rates {
> +       u8 cck[B43_PPR_CCK_RATES_NUM];
> +       u8 ofdm[B43_PPR_OFDM_RATES_NUM];
> +       u8 ofdm_20_cdd[B43_PPR_OFDM_RATES_NUM];
> +       u8 mcs_20[B43_PPR_MCS_RATES_NUM]; /* SISO */
> +       u8 mcs_20_cdd[B43_PPR_MCS_RATES_NUM];
> +       u8 mcs_20_stbc[B43_PPR_MCS_RATES_NUM];
> +       u8 mcs_20_sdm[B43_PPR_MCS_RATES_NUM];
> +} __packed;

not sure if __packed makes a difference if *all* members are u8.

> +
> +struct b43_ppr {
> +       /* All powers are in qdbm (Q5.2) */
> +       union {
> +               u8 __all_rates[B43_PPR_RATES_NUM];
> +               struct b43_ppr_rates rates;
> +       } __packed;

Same here.

> +};
> +
> +#define ppr_for_each_entry(ppr, i, entry)                              \
> +       for (i = 0, entry = &(ppr)->__all_rates[i];                     \
> +            i < B43_PPR_RATES_NUM;                                     \
> +            i++, entry++)
> +
> +struct b43_ppr *b43_ppr_alloc(struct b43_wldev *dev)
> +{
> +       return kzalloc(sizeof(struct b43_ppr), GFP_KERNEL);
> +}
> +
> +void b43_ppr_clear(struct b43_wldev *dev, struct b43_ppr *ppr)
> +{
> +       memset(ppr, 0, sizeof(*ppr));
> +}
> +
> +void b43_ppr_add(struct b43_wldev *dev, struct b43_ppr *ppr, int diff)
> +{
> +       int i;
> +       u8 *rate;
> +
> +       ppr_for_each_entry(ppr, i, rate) {
> +               *rate = clamp_val(*rate + diff, 0, 127);
> +       }
> +}
> +
> +void b43_ppr_apply_max(struct b43_wldev *dev, struct b43_ppr *ppr, u8 max)
> +{
> +       int i;
> +       u8 *rate;
> +
> +       ppr_for_each_entry(ppr, i, rate) {
> +               *rate = min(*rate, max);
> +       }
> +}
> +
> +void b43_ppr_apply_min(struct b43_wldev *dev, struct b43_ppr *ppr, u8 min)
> +{
> +       int i;
> +       u8 *rate;
> +
> +       ppr_for_each_entry(ppr, i, rate) {
> +               *rate = max(*rate, min);
> +       }
> +}
> +
> +u8 b43_ppr_get_max(struct b43_wldev *dev, struct b43_ppr *ppr)
> +{
> +       u8 res = 0;
> +       int i;
> +       u8 *rate;
> +
> +       ppr_for_each_entry(ppr, i, rate) {
> +               res = max(*rate, res);
> +       }
> +
> +       return res;
> +}
> +
> +bool b43_ppr_load_max_from_sprom(struct b43_wldev *dev, struct b43_ppr *ppr,
> +                                enum b43_band band)
> +{
> +       struct b43_ppr_rates *rates = &ppr->rates;
> +       struct ssb_sprom *sprom = dev->dev->bus_sprom;
> +       struct b43_phy *phy = &dev->phy;
> +       u8 maxpwr, off;
> +       u32 *sprom_ofdm_po;

sprom->ofdm2gpo is a single u32, you gain nothing by using a pointer
and dereferencing it (actually you will double your stack usage on 64
bit system).

> +       u16 *sprom_mcs_po;
> +       u8 extra_cdd_po, extra_stbc_po;
> +       int i;
> +
> +       switch (band) {
> +       case B43_BAND_2G:
> +               maxpwr = min(sprom->core_pwr_info[0].maxpwr_2g,
> +                            sprom->core_pwr_info[1].maxpwr_2g);
> +               sprom_ofdm_po = &sprom->ofdm2gpo;
> +               sprom_mcs_po = sprom->mcs2gpo;
> +               extra_cdd_po = (sprom->cddpo >> 0) & 0xf;
> +               extra_stbc_po = (sprom->stbcpo >> 0) & 0xf;
> +               break;
> +       case B43_BAND_5G_LO:
> +               maxpwr = min(sprom->core_pwr_info[0].maxpwr_5gl,
> +                            sprom->core_pwr_info[1].maxpwr_5gl);
> +               sprom_ofdm_po = &sprom->ofdm5glpo;
> +               sprom_mcs_po = sprom->mcs5glpo;
> +               extra_cdd_po = (sprom->cddpo >> 8) & 0xf;
> +               extra_stbc_po = (sprom->stbcpo >> 8) & 0xf;
> +               break;
> +       case B43_BAND_5G_MI:
> +               maxpwr = min(sprom->core_pwr_info[0].maxpwr_5g,
> +                            sprom->core_pwr_info[1].maxpwr_5g);
> +               sprom_ofdm_po = &sprom->ofdm5gpo;
> +               sprom_mcs_po = sprom->mcs5gpo;
> +               extra_cdd_po = (sprom->cddpo >> 4) & 0xf;
> +               extra_stbc_po = (sprom->stbcpo >> 4) & 0xf;
> +               break;
> +       case B43_BAND_5G_HI:
> +               maxpwr = min(sprom->core_pwr_info[0].maxpwr_5gh,
> +                            sprom->core_pwr_info[1].maxpwr_5gh);
> +               sprom_ofdm_po = &sprom->ofdm5ghpo;
> +               sprom_mcs_po = sprom->mcs5ghpo;
> +               extra_cdd_po = (sprom->cddpo >> 12) & 0xf;
> +               extra_stbc_po = (sprom->stbcpo >> 12) & 0xf;
> +               break;
> +       default:
> +               BUG_ON(1);

Is it really that critical that it should halt the system? Maybe a
WARN_ON[_ONCE](1); is enough.

> +               return false;
> +       }
> +
> +       if (band == B43_BAND_2G) {
> +               for (i = 0; i < 4; i++) {
> +                       off = ((sprom->cck2gpo >> (i * 4)) & 0xF) * 2;
> +                       rates->cck[i] = maxpwr - off;
> +               }
> +       }
> +
> +       /* OFDM */
> +       for (i = 0; i < 8; i++) {
> +               off = ((*sprom_ofdm_po >> (i * 4)) & 0xF) * 2;
> +               rates->ofdm[i] = maxpwr - off;
> +       }
> +
> +       /* MCS 20 SISO */
> +       rates->mcs_20[0] = rates->ofdm[0];
> +       rates->mcs_20[1] = rates->ofdm[2];
> +       rates->mcs_20[2] = rates->ofdm[3];
> +       rates->mcs_20[3] = rates->ofdm[4];
> +       rates->mcs_20[4] = rates->ofdm[5];
> +       rates->mcs_20[5] = rates->ofdm[6];
> +       rates->mcs_20[6] = rates->ofdm[7];
> +       rates->mcs_20[7] = rates->ofdm[7];
> +
> +       /* MCS 20 CDD */
> +       for (i = 0; i < 4; i++) {
> +               off = ((sprom_mcs_po[0] >> (i * 4)) & 0xF) * 2;

You should decide whether to use upper case or lower case hex (above
is all lower).

> +               rates->mcs_20_cdd[i] = maxpwr - off;
> +               if (phy->type == B43_PHYTYPE_N && phy->rev >= 3)
> +                       rates->mcs_20_cdd[i] -= extra_cdd_po;
> +       }
> +       for (i = 0; i < 4; i++) {
> +               off = ((sprom_mcs_po[1] >> (i * 4)) & 0xF) * 2;
> +               rates->mcs_20_cdd[4 + i] = maxpwr - off;
> +               if (phy->type == B43_PHYTYPE_N && phy->rev >= 3)
> +                       rates->mcs_20_cdd[i] -= extra_cdd_po;

Did you intend to reduce rates->mcs_20_cdd[4 + i] here?

> +       }
> +
> +       /* OFDM 20 CDD */
> +       rates->ofdm_20_cdd[0] = rates->mcs_20_cdd[0];
> +       rates->ofdm_20_cdd[1] = rates->mcs_20_cdd[0];
> +       rates->ofdm_20_cdd[2] = rates->mcs_20_cdd[1];
> +       rates->ofdm_20_cdd[3] = rates->mcs_20_cdd[2];
> +       rates->ofdm_20_cdd[4] = rates->mcs_20_cdd[3];
> +       rates->ofdm_20_cdd[5] = rates->mcs_20_cdd[4];
> +       rates->ofdm_20_cdd[6] = rates->mcs_20_cdd[5];
> +       rates->ofdm_20_cdd[7] = rates->mcs_20_cdd[6];
> +
> +       /* MCS 20 STBC */
> +       for (i = 0; i < 4; i++) {
> +               off = ((sprom_mcs_po[0] >> (i * 4)) & 0xF) * 2;
> +               rates->mcs_20_stbc[i] = maxpwr - off;
> +               if (phy->type == B43_PHYTYPE_N && phy->rev >= 3)
> +                       rates->mcs_20_stbc[i] -= extra_stbc_po;
> +       }
> +       for (i = 0; i < 4; i++) {
> +               off = ((sprom_mcs_po[1] >> (i * 4)) & 0xF) * 2;
> +               rates->mcs_20_stbc[4 + i] = maxpwr - off;
> +               if (phy->type == B43_PHYTYPE_N && phy->rev >= 3)
> +                       rates->mcs_20_stbc[i] -= extra_stbc_po;

Same question here, did you intend rates->mcs_20_stbc[4 + i]?

> +       }
> +
> +       /* MCS 20 SDM */
> +       for (i = 0; i < 4; i++) {
> +               off = ((sprom_mcs_po[2] >> (i * 4)) & 0xF) * 2;
> +               rates->mcs_20_sdm[i] = maxpwr - off;
> +       }
> +       for (i = 0; i < 4; i++) {
> +               off = ((sprom_mcs_po[3] >> (i * 4)) & 0xF) * 2;
> +               rates->mcs_20_sdm[4 + i] = maxpwr - off;

I guess you did ;-)

> +       }
> +
> +       return true;
> +}

Rest looks fine.


Jonas
--
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




[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux