Series that add 80211n spec support to wl1271: scan, connection via MCS rates
Wl12xx driver add HT support:
? Scan includes HT IE
? Connection to HT devices
? HT protection
? Traffic via MCS rates
? A-MSDU in RX path
Missing functionality is:
? BA session as Initiator and Receiver
Shahar Levi (3):
wl1271: 11n Support, Add Definitions
wl1271: 11n Support, ACX Commands
wl1271: 11n Support, functionality and configuration ability
drivers/net/wireless/wl12xx/Kconfig | 10 +++
drivers/net/wireless/wl12xx/Makefile | 1 +
drivers/net/wireless/wl12xx/wl1271.h | 2 +-
drivers/net/wireless/wl12xx/wl1271_acx.c | 91 +++++++++++++++++++++++++++
drivers/net/wireless/wl12xx/wl1271_acx.h | 95 +++++++++++++++++++++++++++++
drivers/net/wireless/wl12xx/wl1271_conf.h | 4 +
drivers/net/wireless/wl12xx/wl1271_main.c | 78 ++++++++++++++++++-----
drivers/net/wireless/wl12xx/wl1271_rx.c | 4 +
drivers/net/wireless/wl12xx/wl1271_tx.c | 9 +++
9 files changed, 276 insertions(+), 18 deletions(-)
On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
> Add 11n ability in scan, connection and using MCS rates.
> The configuration is temporary due to the code incomplete and
> still in testing process. That plans to be remove in the future.
>
> Signed-off-by: Shahar Levi <[email protected]>
> ---
Some comments below. Again, mostly style-related.
> diff --git a/drivers/net/wireless/wl12xx/Makefile b/drivers/net/wireless/wl12xx/Makefile
> index 0d334d6..92465c8 100644
> --- a/drivers/net/wireless/wl12xx/Makefile
> +++ b/drivers/net/wireless/wl12xx/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_WL1271_SDIO) += wl1271_sdio.o
>
> # small builtin driver bit
> obj-$(CONFIG_WL12XX_PLATFORM_DATA) += wl12xx_platform_data.o
> +
I guess this extra empty line was included accidentally here? There are
no other changes in the Makefile, so please remove this change.
> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
> index bef2c24..7dd1257 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
> @@ -841,10 +841,21 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>
> /* peek into the rates configured in the STA entry */
> spin_lock_irqsave(&wl->wl_lock, flags);
> - if (sta && sta->supp_rates[conf->channel->band] != wl->sta_rate_set) {
> + if (sta &&
> + (sta->supp_rates[conf->channel->band] !=
> + (wl->sta_rate_set & HW_BG_RATES_MASK))) {
> wl->sta_rate_set = sta->supp_rates[conf->channel->band];
Shouldn't this be |= here like the one below? Otherwise the HT rates
will be zeroed. Okay, this would be fixed below, because the zeroed HT
rates will not match the configured rates, but in some cases that will
be unnecessary.
> set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
> }
> +
> + if (sta &&
> + sta->ht_cap.ht_supported &&
> + ((wl->sta_rate_set >> HW_HT_RATES_OFFSET) !=
> + sta->ht_cap.mcs.rx_mask[0])) {
> + wl->sta_rate_set |=
> + (sta->ht_cap.mcs.rx_mask[0] << HW_HT_RATES_OFFSET);
> + set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
> + }
> spin_unlock_irqrestore(&wl->wl_lock, flags);
>
> /* queue the packet */
> @@ -1697,6 +1708,7 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
> {
> enum wl1271_cmd_ps_mode mode;
> struct wl1271 *wl = hw->priv;
> + struct ieee80211_sta *sta = ieee80211_find_sta(vif, bss_conf->bssid);
> bool do_join = false;
> bool set_assoc = false;
> int ret;
> @@ -1915,6 +1927,21 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
> }
> }
>
> + if (sta) {
> + if (changed & BSS_CHANGED_HT) {
> + ret = wl1271_acx_set_ht_capabilities(wl,
> + &sta->ht_cap,
> + true);
> + ret = wl1271_acx_set_ht_information(wl,
> + bss_conf->ht_operation_mode);
> + }
> + else
> + if (changed & BSS_CHANGED_ASSOC)
> + ret = wl1271_acx_set_ht_capabilities(wl,
> + &sta->ht_cap,
> + false);
> + }
> +
The closing brace is indented wrongly here. And you should also use
braces in the else block (see Documentation/CodingStyle).
But in any case, you could reduce the indentation and prevent unaligned
parameters in the wl1271_acx_* calls if you do this:
if (sta && changed & BSS_CHANGED_HT) {
and
if (sta && changed & BSS_CHANGED_ASSOC)
> @@ -2255,6 +2285,7 @@ static struct ieee80211_supported_band wl1271_band_5ghz = {
> .n_channels = ARRAY_SIZE(wl1271_channels_5ghz),
> .bitrates = wl1271_rates_5ghz,
> .n_bitrates = ARRAY_SIZE(wl1271_rates_5ghz),
> + .ht_cap = WL12xx_HT_CAP,
> };
You forgot the #ifdef around .ht_cap here. Actually, it might be nicer
to put the #ifdef around the macro itself, so you can explicitly set
the .ht_supported flag to false if 11n is not configured.
> static const u8 *wl1271_band_rate_to_idx[] = {
> diff --git a/drivers/net/wireless/wl12xx/wl1271_rx.c b/drivers/net/wireless/wl12xx/wl1271_rx.c
> index 94da5dd..109a470 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_rx.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_rx.c
> @@ -53,6 +53,10 @@ static void wl1271_rx_status(struct wl1271 *wl,
> status->band = wl->band;
> status->rate_idx = wl1271_rate_to_idx(wl, desc->rate);
>
> + /* 11n support */
> + if (desc->rate <= CONF_HW_RXTX_RATE_MCS0)
> + status->flag |= RX_FLAG_HT;
> +
> status->signal = desc->rssi;
Should this be #ifdef'ed also?
> /*
> diff --git a/drivers/net/wireless/wl12xx/wl1271_tx.c b/drivers/net/wireless/wl12xx/wl1271_tx.c
> index 1b8295c..af54fef 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_tx.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_tx.c
> @@ -236,6 +236,15 @@ u32 wl1271_tx_enabled_rates_get(struct wl1271 *wl, u32 rate_set)
> rate_set >>= 1;
> }
>
> + /* MCS rates indication are on bits 16 - 23 */
> + rate_set >>= HW_HT_RATES_OFFSET - band->n_bitrates;
> +
> + for (bit = 0; bit < 8; bit++) {
> + if (rate_set & 0x1)
> + enabled_rates |= (CONF_HW_BIT_RATE_MCS_0 << bit);
> + rate_set >>= 1;
> + }
> +
And this? #ifdef here as well, maybe?
--
Cheers,
Luca.
On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
> Two acx commands: ht_capabilities & ht_information, 11n sta capabilities macro.
>
> Signed-off-by: Shahar Levi <[email protected]>
> ---
Looks good! Some minor, mostly style-related, comments below.
> diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
> index 779b130..777c937 100644
> --- a/drivers/net/wireless/wl12xx/wl1271.h
> +++ b/drivers/net/wireless/wl12xx/wl1271.h
> @@ -427,7 +427,7 @@ struct wl1271 {
> /* Our association ID */
> u16 aid;
>
> - /* currently configured rate set */
> + /* currently configured rate set: bits0-15 BG, bits 16-23 MCS */
There a space missing after "bits" and I think you could use something a
bit clearer, for example:
/*
* currently configured rate set:
* bits 0-15 - 802.11abg rates
* bits 16-23 - 802.11n MCS index mask
*/
Maybe also a small comment saying that we support only 1 stream, thus
only 8 bits for the MCS rates (0-7)?
> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
> index ebb341d..5cf5a0d 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
> @@ -964,6 +964,96 @@ struct wl1271_acx_rssi_snr_avg_weights {
> u8 snr_data;
> };
>
> +/* 11n command to the FW */
> +/* Name: ACX_PEER_HT_CAP
> +* Desc: Configure HT capabilities - declare the capabilities of the peer
> +* we are connected to.
> +* Type: Configuration
> +* Access: Write Only
> +* Length:
> +*/
We don't have this style of comment elsewhere, wouldn't it be better to
use something more free format?
> +struct wl1271_acx_ht_capabilities {
> + struct acx_header header;
> +
> + /*
> + * bit 0 - Allow HT Operation
> + * bit 1 - Allow Greenfield format in TX
> + * bit 2 - Allow Short GI in TX
> + * bit 3 - Allow L-SIG TXOP Protection in TX
> + * bit 4 - Allow HT Control fields in TX.
> + * Note, driver will still leave space for HT control in packets
> + * regardless of the value of this field. FW will be responsible to
> + * drop the HT field from any frame when this Bit is set to 0.
Please align the Note with the "Allow HT Control..." above to make it
clear that the note is about bit 4.
> + * 5 - Allow RD initiation in TXOP. FW is allowed to initate RD.
> + * Exact policy setting for this feature is TBD.
> + * Note, this bit can only be set to 1 if bit 3 is set to 1.
"bit 5 -" instead of "5 -" and also align the "Exact policy..." and
"Note".
> + /*
> + * This the maximum a-mpdu length supported by the AP. The FW may not
Upper-case the a-mpdu to A-MPDU for consistency.
> + * exceed this length when sending A-MPDUs
> + */
> + u8 ampdu_max_length;
> +
> + /*
> + * This is the minimal spacing required when sending A-MPDUs to the AP
> + */
> + u8 ampdu_min_spacing;
> +} __packed;
> +
> +/* HT Capabilites Fw Bit Mask Mapping */
> +#define WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION BIT(0)
> +#define WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT BIT(1)
> +#define WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS BIT(2)
> +#define WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION BIT(3)
> +#define WL1271_ACX_FW_CAP_BIT_MASK_HT_CONTROL_FIELDS BIT(4)
> +#define WL1271_ACX_FW_CAP_BIT_MASK_RD_INITIATION BIT(5)
No need for the TABs after the #defines.
> +/* Name: ACX_HT_BSS_OPERATION
> + * Desc: Configure HT capabilities - AP rules for behavior in the BSS.
> + * Type: Configuration
> + * Access: Write Only
> + * Length:
> + */
Same as my previous comment. No directly copied comments from TI's
reference driver, please.
> +struct wl1271_acx_ht_information {
> + struct acx_header header;
> +
> + u8 rifs_mode; /* Values: 0 - RIFS not allowed, 1 - RIFS allowed */
Put the comments before the value, as everywhere else.
> + u8 ht_protection; /* Values: 0 - 3 like in spec */
Same here.
> + /*
> + * Values: 0 - GF protection not required, 1 - GF protection required
> + */
> + u8 gf_protection;
If the comment fits in one line, don't use the extra lines with /* and
*/.
> + /*
> + * Values: 0 - TX Burst limit not required, 1 - TX Burst Limit required
> + */
> + u8 ht_tx_burst_limit;
Same here.
> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
> index 60c50d1..732e9aa 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
> +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
> @@ -91,6 +91,10 @@ enum {
> CONF_HW_RXTX_RATE_UNSUPPORTED = 0xff
> };
>
> +#define HW_RX_HIGHEST_RATE 65
> +#define HW_BG_RATES_MASK 0xffff
> +#define HW_HT_RATES_OFFSET 16
We preceed all the other macros with CONF_, please do the same for
these.
> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
> index cb18f22..bef2c24 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
> @@ -2122,6 +2122,19 @@ static const u8 wl1271_rate_to_idx_2ghz[] = {
> 0 /* CONF_HW_RXTX_RATE_1 */
> };
>
> +/* 11n sta capabilities */
Use STA for consistency.
> +#define WL12xx_HT_CAP { \
As we discussed separately, please use WL1271_HT_CAP. Later, when we
rename the driver, we can change it to WL12XX_HT_CAP together with
everything else. Let's keep everything as WL1271 for now, for
consistency.
--
Cheers,
Luca.
Added ACX command to the FW for 11n support.
Signed-off-by: Shahar Levi <[email protected]>
---
drivers/net/wireless/wl12xx/wl1271_acx.c | 91 ++++++++++++++++++++++++++++++
drivers/net/wireless/wl12xx/wl1271_acx.h | 5 ++
2 files changed, 96 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
index 6189934..cc6b7d8 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.c
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
@@ -1226,6 +1226,97 @@ out:
return ret;
}
+int wl1271_acx_set_ht_capabilities(struct wl1271 *wl,
+ struct ieee80211_sta_ht_cap *ht_cap,
+ bool allow_ht_operation)
+{
+ struct wl1271_acx_ht_capabilities *acx;
+ /*
+ * Note, currently this value will be set to FFFFFFFFFFFF to indicate
+ * it is relevant for all peers since we only support HT in
+ * infrastructure mode. Later on this field will be relevant to
+ * IBSS/DLS operation */
+ u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+ int ret = 0;
+
+ wl1271_debug(DEBUG_ACX, "acx ht capabilities setting");
+
+ acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+ if (!acx) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ /* Allow HT Operation ? */
+ if (true == allow_ht_operation) {
+ acx->ht_capabilites =
+ WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION;
+ acx->ht_capabilites |=
+ ((ht_cap->cap & IEEE80211_HT_CAP_GRN_FLD) ?
+ WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT : 0);
+ acx->ht_capabilites |=
+ ((ht_cap->cap & IEEE80211_HT_CAP_SGI_20) ?
+ WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS : 0);
+ acx->ht_capabilites |=
+ ((ht_cap->cap & IEEE80211_HT_CAP_LSIG_TXOP_PROT) ?
+ WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION : 0);
+
+ /* get date from A-MPDU parameters field */
+ acx->ampdu_max_length = ht_cap->ampdu_factor;
+ acx->ampdu_min_spacing = ht_cap->ampdu_density;
+
+ memcpy(acx->mac_address, mac_address, ETH_ALEN);
+ }
+ /* HT operations are not allowed */
+ else
+ acx->ht_capabilites = 0;
+
+ ret = wl1271_cmd_configure(wl, ACX_PEER_HT_CAP, acx, sizeof(*acx));
+ if (ret < 0) {
+ wl1271_warning("acx ht capabilities setting failed: %d", ret);
+ goto out;
+ }
+
+out:
+ kfree(acx);
+ return ret;
+}
+
+int wl1271_acx_set_ht_information(struct wl1271 *wl,
+ u16 ht_operation_mode)
+{
+ struct wl1271_acx_ht_information *acx;
+ int ret = 0;
+
+ wl1271_debug(DEBUG_ACX, "acx ht information setting");
+
+ acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+ if (!acx) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ acx->ht_protection =
+ (u8)(ht_operation_mode & IEEE80211_HT_OP_MODE_PROTECTION);
+ acx->rifs_mode = 0;
+ acx->gf_protection = 0;
+ acx->ht_tx_burst_limit = 0;
+ acx->dual_cts_protection = 0;
+
+ ret = wl1271_cmd_configure(wl,
+ ACX_HT_BSS_OPERATION,
+ acx,
+ sizeof(*acx));
+ if (ret < 0) {
+ wl1271_warning("acx ht information setting failed: %d", ret);
+ goto out;
+ }
+
+out:
+ kfree(acx);
+ return ret;
+}
+
int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime)
{
struct wl1271_acx_fw_tsf_information *tsf_info;
diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
index 5cf5a0d..abd4c70 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.h
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
@@ -1183,6 +1183,11 @@ int wl1271_acx_keep_alive_config(struct wl1271 *wl, u8 index, u8 tpl_valid);
int wl1271_acx_rssi_snr_trigger(struct wl1271 *wl, bool enable,
s16 thold, u8 hyst);
int wl1271_acx_rssi_snr_avg_weights(struct wl1271 *wl);
+int wl1271_acx_set_ht_capabilities(struct wl1271 *wl,
+ struct ieee80211_sta_ht_cap *ht_cap,
+ bool allow_ht_operation);
+int wl1271_acx_set_ht_information(struct wl1271 *wl,
+ u16 ht_operation_mode);
int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime);
#endif /* __WL1271_ACX_H__ */
--
1.6.0.4
On Sun, 2010-10-10 at 17:24 +0200, ext Shahar Levi wrote:
> >> static const u8 *wl1271_band_rate_to_idx[] = {
> >> diff --git a/drivers/net/wireless/wl12xx/wl1271_rx.c
> b/drivers/net/wireless/wl12xx/wl1271_rx.c
> >> index 94da5dd..109a470 100644
> >> --- a/drivers/net/wireless/wl12xx/wl1271_rx.c
> >> +++ b/drivers/net/wireless/wl12xx/wl1271_rx.c
> >> @@ -53,6 +53,10 @@ static void wl1271_rx_status(struct wl1271 *wl,
> >> status->band = wl->band;
> >> status->rate_idx = wl1271_rate_to_idx(wl, desc->rate);
> >>
> >> + /* 11n support */
> >> + if (desc->rate <= CONF_HW_RXTX_RATE_MCS0)
> >> + status->flag |= RX_FLAG_HT;
> >> +
> >> status->signal = desc->rssi;
> >
> > Should this be #ifdef'ed also?
>
> No, in case of HT not supported the HW will not set MCS rate.
True, but if you don't have the ifdef you're adding unnecessary code in
the RX data path.
> >> diff --git a/drivers/net/wireless/wl12xx/wl1271_tx.c
> b/drivers/net/wireless/wl12xx/wl1271_tx.c
> >> index 1b8295c..af54fef 100644
> >> --- a/drivers/net/wireless/wl12xx/wl1271_tx.c
> >> +++ b/drivers/net/wireless/wl12xx/wl1271_tx.c
> >> @@ -236,6 +236,15 @@ u32 wl1271_tx_enabled_rates_get(struct wl1271
> *wl, u32 rate_set)
> >> rate_set >>= 1;
> >> }
> >>
> >> + /* MCS rates indication are on bits 16 - 23 */
> >> + rate_set >>= HW_HT_RATES_OFFSET - band->n_bitrates;
> >> +
> >> + for (bit = 0; bit < 8; bit++) {
> >> + if (rate_set & 0x1)
> >> + enabled_rates |= (CONF_HW_BIT_RATE_MCS_0 <<
> bit);
> >> + rate_set >>= 1;
> >> + }
> >> +
> >
> > And this? #ifdef here as well, maybe?
>
> No, in case of HT not supported MCS bit in rate_set will not be set.
Same here, this loop will add more CPU cycles in the TX data path
unnecessarily.
--
Cheers,
Luca.
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBKb2hhbm5lcyBCZXJnIFttYWls
dG86am9oYW5uZXNAc2lwc29sdXRpb25zLm5ldF0NCj4gU2VudDogV2VkbmVzZGF5LCBPY3RvYmVy
IDA2LCAyMDEwIDg6MjEgUE0NCj4gVG86IExldmksIFNoYWhhcg0KPiBDYzogbGludXgtd2lyZWxl
c3NAdmdlci5rZXJuZWwub3JnOyBMdWNpYW5vIENvZWxobw0KPiBTdWJqZWN0OiBSZTogW1BBVENI
IHYyIDAzLzAzXSB3bDEyNzE6IDExbiBTdXBwb3J0LCBmdW5jdGlvbmFsaXR5IGFuZA0KPiBjb25m
aWd1cmF0aW9uIGFiaWxpdHkNCj4gDQo+IE9uIFdlZCwgMjAxMC0xMC0wNiBhdCAyMDowOCArMDIw
MCwgU2hhaGFyIExldmkgd3JvdGU6DQo+IA0KPiA+ICAJLyogcGVlayBpbnRvIHRoZSByYXRlcyBj
b25maWd1cmVkIGluIHRoZSBTVEEgZW50cnkgKi8NCj4gPiAgCXNwaW5fbG9ja19pcnFzYXZlKCZ3
bC0+d2xfbG9jaywgZmxhZ3MpOw0KPiA+IC0JaWYgKHN0YSAmJiBzdGEtPnN1cHBfcmF0ZXNbY29u
Zi0+Y2hhbm5lbC0+YmFuZF0gIT0gd2wtPnN0YV9yYXRlX3NldCkgew0KPiA+ICsJaWYgKHN0YSAm
Jg0KPiA+ICsJICAgIChzdGEtPnN1cHBfcmF0ZXNbY29uZi0+Y2hhbm5lbC0+YmFuZF0gIT0NCj4g
PiArCSAgICAod2wtPnN0YV9yYXRlX3NldCAmIEhXX0JHX1JBVEVTX01BU0spKSkgew0KPiA+ICAJ
CXdsLT5zdGFfcmF0ZV9zZXQgPSBzdGEtPnN1cHBfcmF0ZXNbY29uZi0+Y2hhbm5lbC0+YmFuZF07
DQo+ID4gIAkJc2V0X2JpdChXTDEyNzFfRkxBR19TVEFfUkFURVNfQ0hBTkdFRCwgJndsLT5mbGFn
cyk7DQo+ID4gIAl9DQo+IA0KPiA/Pz8NCj4gDQo+IEhvdyBjYW4gbm9uLUJHIHJhdGVzIGV2ZXIg
c2hvdyB1cCB0aGVyZT8gSXMgdGhpcyByZWFsbHkgYW4gMTFhIGNoYW5nZT8NCj4gDQo+IGpvaGFu
bmVzDQoNCg0KVGhhbmtzIGZvciB0aGUgcmV2aWV3Lg0KTm9uIEJHIHJhdGVzIHNldCBpbiB0aGUg
bmV4dCBpZjoNCisJaWYgKHN0YSAmJg0KKwkgICAgc3RhLT5odF9jYXAuaHRfc3VwcG9ydGVkICYm
DQorCSAgICAoKHdsLT5zdGFfcmF0ZV9zZXQgPj4gSFdfSFRfUkFURVNfT0ZGU0VUKSAhPQ0KKwkg
ICAgICBzdGEtPmh0X2NhcC5tY3MucnhfbWFza1swXSkpIHsNCisJCXdsLT5zdGFfcmF0ZV9zZXQg
fD0NCisJCShzdGEtPmh0X2NhcC5tY3MucnhfbWFza1swXSA8PCBIV19IVF9SQVRFU19PRkZTRVQp
Ow0KKwkJc2V0X2JpdChXTDEyNzFfRkxBR19TVEFfUkFURVNfQ0hBTkdFRCwgJndsLT5mbGFncyk7
DQpzdGFfcmF0ZV9zZXQgYml0cyBhcmU6IGJpdHMwLTE1IEJHLCBiaXRzIDE2LTIzIE1DUy4NCkRp
ZCB5b3UgbWVhbnQgMTFuIGNoYW5nZT8NCg0KUmVnYXJkcywNClNoYWhhcg0K
Series that add 80211n spec support to wl1271: scan, connection via MCS rates
Wl12xx driver add HT support:
? Scan includes HT IE
? Connection to HT devices
? HT protection
? Traffic via MCS rates
? A-MSDU in RX path
Missing functionality is:
? BA session as Initiator and Receiver
Shahar Levi (3):
wl1271: 11n Support, Add Definitions
wl1271: 11n Support, ACX Commands
wl1271: 11n Support, functionality and configuration ability
drivers/net/wireless/wl12xx/Kconfig | 10 +++
drivers/net/wireless/wl12xx/Makefile | 1 +
drivers/net/wireless/wl12xx/wl1271.h | 2 +-
drivers/net/wireless/wl12xx/wl1271_acx.c | 91 +++++++++++++++++++++++++++
drivers/net/wireless/wl12xx/wl1271_acx.h | 95 +++++++++++++++++++++++++++++
drivers/net/wireless/wl12xx/wl1271_conf.h | 4 +
drivers/net/wireless/wl12xx/wl1271_main.c | 78 ++++++++++++++++++-----
drivers/net/wireless/wl12xx/wl1271_rx.c | 4 +
drivers/net/wireless/wl12xx/wl1271_tx.c | 9 +++
9 files changed, 276 insertions(+), 18 deletions(-)
On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
> Added ACX command to the FW for 11n support.
>
> Signed-off-by: Shahar Levi <[email protected]>
> ---
Again, mostly coding-style comments.
> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
> index 6189934..cc6b7d8 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_acx.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
> @@ -1226,6 +1226,97 @@ out:
> return ret;
> }
>
> +int wl1271_acx_set_ht_capabilities(struct wl1271 *wl,
> + struct ieee80211_sta_ht_cap *ht_cap,
> + bool allow_ht_operation)
> +{
> + struct wl1271_acx_ht_capabilities *acx;
> + /*
> + * Note, currently this value will be set to FFFFFFFFFFFF to indicate
> + * it is relevant for all peers since we only support HT in
> + * infrastructure mode. Later on this field will be relevant to
> + * IBSS/DLS operation */
> + u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
I didn't realize this when reviewing patch 01/03, but you could actually
remove the Note from the wl1271_acx.h file and leave it here. There's
no need to have it repeated there and here.
> + /* Allow HT Operation ? */
> + if (true == allow_ht_operation) {
I know that some people like to put the constant in the left side of the
comparison operator to avoid problems with mistyping == as =, but
reading this in the opposite way is less intuitive (like top-posting? ;)
and the compiler, if called with reasonable options, will warn you if
you make that mistake. So, summarizing, please use it like this
instead, then you don't even risk mistyping == in the first place :P
if (allow_ht_operation) {
> + acx->ht_capabilites =
> + WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION;
Add a more indentation on the second line, it's easier to read.
> + acx->ht_capabilites |=
> + ((ht_cap->cap & IEEE80211_HT_CAP_GRN_FLD) ?
> + WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT : 0);
> + acx->ht_capabilites |=
> + ((ht_cap->cap & IEEE80211_HT_CAP_SGI_20) ?
> + WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS : 0);
> + acx->ht_capabilites |=
> + ((ht_cap->cap & IEEE80211_HT_CAP_LSIG_TXOP_PROT) ?
> + WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION : 0);
Same thing for all these other assignments.
> +
> + /* get date from A-MPDU parameters field */
"get data"
> + acx->ampdu_max_length = ht_cap->ampdu_factor;
> + acx->ampdu_min_spacing = ht_cap->ampdu_density;
> +
> + memcpy(acx->mac_address, mac_address, ETH_ALEN);
> + }
> + /* HT operations are not allowed */
> + else
> + acx->ht_capabilites = 0;
According to Documentation/CodingStyle, you should write the else block
like this:
if (...) {
...
} else {
acx->ht_capabilities = 0;
}
> +int wl1271_acx_set_ht_information(struct wl1271 *wl,
> + u16 ht_operation_mode)
> +{
> + struct wl1271_acx_ht_information *acx;
> + int ret = 0;
> +
> + wl1271_debug(DEBUG_ACX, "acx ht information setting");
> +
> + acx = kzalloc(sizeof(*acx), GFP_KERNEL);
> + if (!acx) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + acx->ht_protection =
> + (u8)(ht_operation_mode & IEEE80211_HT_OP_MODE_PROTECTION);
> + acx->rifs_mode = 0;
> + acx->gf_protection = 0;
> + acx->ht_tx_burst_limit = 0;
> + acx->dual_cts_protection = 0;
> +
> + ret = wl1271_cmd_configure(wl,
> + ACX_HT_BSS_OPERATION,
> + acx,
> + sizeof(*acx));
Doesn't this fit in less lines?
--
Cheers,
Luca.
On Wed, 2010-10-06 at 21:38 +0200, Levi, Shahar wrote:
> > > /* peek into the rates configured in the STA entry */
> > > spin_lock_irqsave(&wl->wl_lock, flags);
> > > - if (sta && sta->supp_rates[conf->channel->band] != wl->sta_rate_set) {
> > > + if (sta &&
> > > + (sta->supp_rates[conf->channel->band] !=
> > > + (wl->sta_rate_set & HW_BG_RATES_MASK))) {
> > > wl->sta_rate_set = sta->supp_rates[conf->channel->band];
> > > set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
> > > }
> >
> > ???
> >
> > How can non-BG rates ever show up there? Is this really an 11a change?
> >
> > johannes
>
>
> Thanks for the review.
> Non BG rates set in the next if:
> + if (sta &&
> + sta->ht_cap.ht_supported &&
> + ((wl->sta_rate_set >> HW_HT_RATES_OFFSET) !=
> + sta->ht_cap.mcs.rx_mask[0])) {
> + wl->sta_rate_set |=
> + (sta->ht_cap.mcs.rx_mask[0] << HW_HT_RATES_OFFSET);
> + set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
> sta_rate_set bits are: bits0-15 BG, bits 16-23 MCS.
> Did you meant 11n change?
Never mind, I misread the patch.
johannes
Two acx commands: ht_capabilities & ht_information, 11n sta capabilities macro.
Signed-off-by: Shahar Levi <[email protected]>
---
drivers/net/wireless/wl12xx/wl1271.h | 2 +-
drivers/net/wireless/wl12xx/wl1271_acx.h | 90 +++++++++++++++++++++++++++++
drivers/net/wireless/wl12xx/wl1271_conf.h | 4 +
drivers/net/wireless/wl12xx/wl1271_main.c | 13 ++++
4 files changed, 108 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
index 779b130..777c937 100644
--- a/drivers/net/wireless/wl12xx/wl1271.h
+++ b/drivers/net/wireless/wl12xx/wl1271.h
@@ -427,7 +427,7 @@ struct wl1271 {
/* Our association ID */
u16 aid;
- /* currently configured rate set */
+ /* currently configured rate set: bits0-15 BG, bits 16-23 MCS */
u32 sta_rate_set;
u32 basic_rate_set;
u32 basic_rate;
diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
index ebb341d..5cf5a0d 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.h
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
@@ -964,6 +964,96 @@ struct wl1271_acx_rssi_snr_avg_weights {
u8 snr_data;
};
+/* 11n command to the FW */
+/* Name: ACX_PEER_HT_CAP
+* Desc: Configure HT capabilities - declare the capabilities of the peer
+* we are connected to.
+* Type: Configuration
+* Access: Write Only
+* Length:
+*/
+struct wl1271_acx_ht_capabilities {
+ struct acx_header header;
+
+ /*
+ * bit 0 - Allow HT Operation
+ * bit 1 - Allow Greenfield format in TX
+ * bit 2 - Allow Short GI in TX
+ * bit 3 - Allow L-SIG TXOP Protection in TX
+ * bit 4 - Allow HT Control fields in TX.
+ * Note, driver will still leave space for HT control in packets
+ * regardless of the value of this field. FW will be responsible to
+ * drop the HT field from any frame when this Bit is set to 0.
+ * 5 - Allow RD initiation in TXOP. FW is allowed to initate RD.
+ * Exact policy setting for this feature is TBD.
+ * Note, this bit can only be set to 1 if bit 3 is set to 1.
+ */
+ __le32 ht_capabilites;
+
+ /*
+ * Indicates to which peer these capabilities are relevant.
+ * Note, currently this value will be set to FFFFFFFFFFFF to indicate it
+ * is relevant for all peers since we only support HT in infrastructure
+ * mode. Later on this field will be relevant to IBSS/DLS operation
+ */
+ u8 mac_address[ETH_ALEN];
+
+ /*
+ * This the maximum a-mpdu length supported by the AP. The FW may not
+ * exceed this length when sending A-MPDUs
+ */
+ u8 ampdu_max_length;
+
+ /*
+ * This is the minimal spacing required when sending A-MPDUs to the AP
+ */
+ u8 ampdu_min_spacing;
+} __packed;
+
+/* HT Capabilites Fw Bit Mask Mapping */
+#define WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION BIT(0)
+#define WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT BIT(1)
+#define WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS BIT(2)
+#define WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION BIT(3)
+#define WL1271_ACX_FW_CAP_BIT_MASK_HT_CONTROL_FIELDS BIT(4)
+#define WL1271_ACX_FW_CAP_BIT_MASK_RD_INITIATION BIT(5)
+
+
+/* Name: ACX_HT_BSS_OPERATION
+ * Desc: Configure HT capabilities - AP rules for behavior in the BSS.
+ * Type: Configuration
+ * Access: Write Only
+ * Length:
+ */
+struct wl1271_acx_ht_information {
+ struct acx_header header;
+
+ u8 rifs_mode; /* Values: 0 - RIFS not allowed, 1 - RIFS allowed */
+
+ u8 ht_protection; /* Values: 0 - 3 like in spec */
+
+ /*
+ * Values: 0 - GF protection not required, 1 - GF protection required
+ */
+ u8 gf_protection;
+
+ /*
+ * Values: 0 - TX Burst limit not required, 1 - TX Burst Limit required
+ */
+ u8 ht_tx_burst_limit;
+
+ /*
+ * Values: 0 - Dual CTS protection not required,
+ * 1 Dual CTS Protection required
+ * Note: When this value is set to 1 FW will protect all TXOP with RTS
+ * frame and will not use CTS-to-self regardless of the value of the
+ * ACX_CTS_PROTECTION information element
+ */
+ u8 dual_cts_protection;
+
+ u8 padding[3];
+} __packed;
+
struct wl1271_acx_fw_tsf_information {
struct acx_header header;
diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
index 60c50d1..732e9aa 100644
--- a/drivers/net/wireless/wl12xx/wl1271_conf.h
+++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
@@ -91,6 +91,10 @@ enum {
CONF_HW_RXTX_RATE_UNSUPPORTED = 0xff
};
+#define HW_RX_HIGHEST_RATE 65
+#define HW_BG_RATES_MASK 0xffff
+#define HW_HT_RATES_OFFSET 16
+
enum {
CONF_SG_DISABLE = 0,
CONF_SG_PROTECTIVE,
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index cb18f22..bef2c24 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -2122,6 +2122,19 @@ static const u8 wl1271_rate_to_idx_2ghz[] = {
0 /* CONF_HW_RXTX_RATE_1 */
};
+/* 11n sta capabilities */
+#define WL12xx_HT_CAP { \
+ .cap = IEEE80211_HT_CAP_GRN_FLD | IEEE80211_HT_CAP_SGI_20, \
+ .ht_supported = true, \
+ .ampdu_factor = IEEE80211_HT_MAX_AMPDU_8K, \
+ .ampdu_density = IEEE80211_HT_MPDU_DENSITY_8, \
+ .mcs = { \
+ .rx_mask = { 0xff, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, \
+ .rx_highest = cpu_to_le16(HW_RX_HIGHEST_RATE), \
+ .tx_params = IEEE80211_HT_MCS_TX_DEFINED, \
+ }, \
+}
+
/* can't be const, mac80211 writes to this */
static struct ieee80211_supported_band wl1271_band_2ghz = {
.channels = wl1271_channels,
--
1.6.0.4
Thanks for the review. All fixed on v3.
Cheers,
Shahar
Luciano Coelho wrote:
> On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
>> Added ACX command to the FW for 11n support.
>>
>> Signed-off-by: Shahar Levi <[email protected]>
>> ---
>
> Again, mostly coding-style comments.
>
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
>> index 6189934..cc6b7d8 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
>> @@ -1226,6 +1226,97 @@ out:
>> return ret;
>> }
>>
>> +int wl1271_acx_set_ht_capabilities(struct wl1271 *wl,
>> + struct ieee80211_sta_ht_cap *ht_cap,
>> + bool allow_ht_operation)
>> +{
>> + struct wl1271_acx_ht_capabilities *acx;
>> + /*
>> + * Note, currently this value will be set to FFFFFFFFFFFF to indicate
>> + * it is relevant for all peers since we only support HT in
>> + * infrastructure mode. Later on this field will be relevant to
>> + * IBSS/DLS operation */
>> + u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
>
> I didn't realize this when reviewing patch 01/03, but you could actually
> remove the Note from the wl1271_acx.h file and leave it here. There's
> no need to have it repeated there and here.
>
>
>> + /* Allow HT Operation ? */
>> + if (true == allow_ht_operation) {
>
> I know that some people like to put the constant in the left side of the
> comparison operator to avoid problems with mistyping == as =, but
> reading this in the opposite way is less intuitive (like top-posting? ;)
> and the compiler, if called with reasonable options, will warn you if
> you make that mistake. So, summarizing, please use it like this
> instead, then you don't even risk mistyping == in the first place :P
>
> if (allow_ht_operation) {
>
>
>> + acx->ht_capabilites =
>> + WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION;
>
> Add a more indentation on the second line, it's easier to read.
>
>
>> + acx->ht_capabilites |=
>> + ((ht_cap->cap & IEEE80211_HT_CAP_GRN_FLD) ?
>> + WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT : 0);
>> + acx->ht_capabilites |=
>> + ((ht_cap->cap & IEEE80211_HT_CAP_SGI_20) ?
>> + WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS : 0);
>> + acx->ht_capabilites |=
>> + ((ht_cap->cap & IEEE80211_HT_CAP_LSIG_TXOP_PROT) ?
>> + WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION : 0);
>
> Same thing for all these other assignments.
>
>
>> +
>> + /* get date from A-MPDU parameters field */
>
> "get data"
>
>
>> + acx->ampdu_max_length = ht_cap->ampdu_factor;
>> + acx->ampdu_min_spacing = ht_cap->ampdu_density;
>> +
>> + memcpy(acx->mac_address, mac_address, ETH_ALEN);
>> + }
>> + /* HT operations are not allowed */
>> + else
>> + acx->ht_capabilites = 0;
>
> According to Documentation/CodingStyle, you should write the else block
> like this:
>
> if (...) {
> ...
> } else {
> acx->ht_capabilities = 0;
> }
>
>
>> +int wl1271_acx_set_ht_information(struct wl1271 *wl,
>> + u16 ht_operation_mode)
>> +{
>> + struct wl1271_acx_ht_information *acx;
>> + int ret = 0;
>> +
>> + wl1271_debug(DEBUG_ACX, "acx ht information setting");
>> +
>> + acx = kzalloc(sizeof(*acx), GFP_KERNEL);
>> + if (!acx) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + acx->ht_protection =
>> + (u8)(ht_operation_mode & IEEE80211_HT_OP_MODE_PROTECTION);
>> + acx->rifs_mode = 0;
>> + acx->gf_protection = 0;
>> + acx->ht_tx_burst_limit = 0;
>> + acx->dual_cts_protection = 0;
>> +
>> + ret = wl1271_cmd_configure(wl,
>> + ACX_HT_BSS_OPERATION,
>> + acx,
>> + sizeof(*acx));
>
> Doesn't this fit in less lines?
>
Thanks for the review.
all fixed on v3, one commend inline.
Cheers,
Shahar
Luciano Coelho wrote:
> On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
>> Two acx commands: ht_capabilities & ht_information, 11n sta capabilities macro.
>>
>> Signed-off-by: Shahar Levi <[email protected]>
>> ---
>
> Looks good! Some minor, mostly style-related, comments below.
>
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
>> index 779b130..777c937 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271.h
>> @@ -427,7 +427,7 @@ struct wl1271 {
>> /* Our association ID */
>> u16 aid;
>>
>> - /* currently configured rate set */
>> + /* currently configured rate set: bits0-15 BG, bits 16-23 MCS */
>
> There a space missing after "bits" and I think you could use something a
> bit clearer, for example:
>
> /*
> * currently configured rate set:
> * bits 0-15 - 802.11abg rates
> * bits 16-23 - 802.11n MCS index mask
> */
>
> Maybe also a small comment saying that we support only 1 stream, thus
> only 8 bits for the MCS rates (0-7)?
>
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> index ebb341d..5cf5a0d 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> @@ -964,6 +964,96 @@ struct wl1271_acx_rssi_snr_avg_weights {
>> u8 snr_data;
>> };
>>
>> +/* 11n command to the FW */
>> +/* Name: ACX_PEER_HT_CAP
>> +* Desc: Configure HT capabilities - declare the capabilities of the peer
>> +* we are connected to.
>> +* Type: Configuration
>> +* Access: Write Only
>> +* Length:
>> +*/
>
> We don't have this style of comment elsewhere, wouldn't it be better to
> use something more free format?
>
>
>> +struct wl1271_acx_ht_capabilities {
>> + struct acx_header header;
>> +
>> + /*
>> + * bit 0 - Allow HT Operation
>> + * bit 1 - Allow Greenfield format in TX
>> + * bit 2 - Allow Short GI in TX
>> + * bit 3 - Allow L-SIG TXOP Protection in TX
>> + * bit 4 - Allow HT Control fields in TX.
>> + * Note, driver will still leave space for HT control in packets
>> + * regardless of the value of this field. FW will be responsible to
>> + * drop the HT field from any frame when this Bit is set to 0.
>
> Please align the Note with the "Allow HT Control..." above to make it
> clear that the note is about bit 4.
>
>
>> + * 5 - Allow RD initiation in TXOP. FW is allowed to initate RD.
>> + * Exact policy setting for this feature is TBD.
>> + * Note, this bit can only be set to 1 if bit 3 is set to 1.
>
> "bit 5 -" instead of "5 -" and also align the "Exact policy..." and
> "Note".
>
>
>> + /*
>> + * This the maximum a-mpdu length supported by the AP. The FW may not
>
> Upper-case the a-mpdu to A-MPDU for consistency.
>
>> + * exceed this length when sending A-MPDUs
>> + */
>> + u8 ampdu_max_length;
>> +
>> + /*
>> + * This is the minimal spacing required when sending A-MPDUs to the AP
>> + */
>> + u8 ampdu_min_spacing;
>> +} __packed;
>> +
>> +/* HT Capabilites Fw Bit Mask Mapping */
>> +#define WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION BIT(0)
>> +#define WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT BIT(1)
>> +#define WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS BIT(2)
>> +#define WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION BIT(3)
>> +#define WL1271_ACX_FW_CAP_BIT_MASK_HT_CONTROL_FIELDS BIT(4)
>> +#define WL1271_ACX_FW_CAP_BIT_MASK_RD_INITIATION BIT(5)
>
> No need for the TABs after the #defines.
>
>
>> +/* Name: ACX_HT_BSS_OPERATION
>> + * Desc: Configure HT capabilities - AP rules for behavior in the BSS.
>> + * Type: Configuration
>> + * Access: Write Only
>> + * Length:
>> + */
>
> Same as my previous comment. No directly copied comments from TI's
> reference driver, please.
>
>
>> +struct wl1271_acx_ht_information {
>> + struct acx_header header;
>> +
>> + u8 rifs_mode; /* Values: 0 - RIFS not allowed, 1 - RIFS allowed */
>
> Put the comments before the value, as everywhere else.
>
>
>> + u8 ht_protection; /* Values: 0 - 3 like in spec */
>
> Same here.
>
>
>> + /*
>> + * Values: 0 - GF protection not required, 1 - GF protection required
>> + */
>> + u8 gf_protection;
> Move to wl1271_main.c, not configurable setting (HW depended).
> If the comment fits in one line, don't use the extra lines with /* and
> */.
>
>
>> + /*
>> + * Values: 0 - TX Burst limit not required, 1 - TX Burst Limit required
>> + */
>> + u8 ht_tx_burst_limit;
>
> Same here.
>
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> index 60c50d1..732e9aa 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> @@ -91,6 +91,10 @@ enum {
>> CONF_HW_RXTX_RATE_UNSUPPORTED = 0xff
>> };
>>
>> +#define HW_RX_HIGHEST_RATE 65
Move to wl1271_main.c, not configurable setting (HW depended).
>> +#define HW_BG_RATES_MASK 0xffff
>> +#define HW_HT_RATES_OFFSET 16
Move to wl1271.h, not configurable setting.
>
> We preceed all the other macros with CONF_, please do the same for
> these.
>
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
>> index cb18f22..bef2c24 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
>> @@ -2122,6 +2122,19 @@ static const u8 wl1271_rate_to_idx_2ghz[] = {
>> 0 /* CONF_HW_RXTX_RATE_1 */
>> };
>>
>> +/* 11n sta capabilities */
>
> Use STA for consistency.
>
>
>> +#define WL12xx_HT_CAP { \
>
> As we discussed separately, please use WL1271_HT_CAP. Later, when we
> rename the driver, we can change it to WL12XX_HT_CAP together with
> everything else. Let's keep everything as WL1271 for now, for
> consistency.
>
>
Add 11n ability in scan, connection and using MCS rates.
The configuration is temporary due to the code incomplete and
still in testing process. That plans to be remove in the future.
Signed-off-by: Shahar Levi <[email protected]>
---
drivers/net/wireless/wl12xx/Kconfig | 10 ++++
drivers/net/wireless/wl12xx/Makefile | 1 +
drivers/net/wireless/wl12xx/wl1271_main.c | 65 +++++++++++++++++++++--------
drivers/net/wireless/wl12xx/wl1271_rx.c | 4 ++
drivers/net/wireless/wl12xx/wl1271_tx.c | 9 ++++
5 files changed, 72 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/Kconfig b/drivers/net/wireless/wl12xx/Kconfig
index 4a8bb25..7ecb94d 100644
--- a/drivers/net/wireless/wl12xx/Kconfig
+++ b/drivers/net/wireless/wl12xx/Kconfig
@@ -52,6 +52,16 @@ config WL1271
If you choose to build a module, it'll be called wl1271. Say N if
unsure.
+config WL1271_HT
+ bool "TI wl1271 802.11 HT support (EXPERIMENTAL)"
+ depends on WL1271 && EXPERIMENTAL
+ default n
+ ---help---
+ This will enable 802.11 HT support for TI wl1271 chipset.
+
+ That configuration is temporary due to the code incomplete and
+ still in testing process.
+
config WL1271_SPI
tristate "TI wl1271 SPI support"
depends on WL1271 && SPI_MASTER
diff --git a/drivers/net/wireless/wl12xx/Makefile b/drivers/net/wireless/wl12xx/Makefile
index 0d334d6..92465c8 100644
--- a/drivers/net/wireless/wl12xx/Makefile
+++ b/drivers/net/wireless/wl12xx/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_WL1271_SDIO) += wl1271_sdio.o
# small builtin driver bit
obj-$(CONFIG_WL12XX_PLATFORM_DATA) += wl12xx_platform_data.o
+
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index bef2c24..7dd1257 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -841,10 +841,21 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
/* peek into the rates configured in the STA entry */
spin_lock_irqsave(&wl->wl_lock, flags);
- if (sta && sta->supp_rates[conf->channel->band] != wl->sta_rate_set) {
+ if (sta &&
+ (sta->supp_rates[conf->channel->band] !=
+ (wl->sta_rate_set & HW_BG_RATES_MASK))) {
wl->sta_rate_set = sta->supp_rates[conf->channel->band];
set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
}
+
+ if (sta &&
+ sta->ht_cap.ht_supported &&
+ ((wl->sta_rate_set >> HW_HT_RATES_OFFSET) !=
+ sta->ht_cap.mcs.rx_mask[0])) {
+ wl->sta_rate_set |=
+ (sta->ht_cap.mcs.rx_mask[0] << HW_HT_RATES_OFFSET);
+ set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
+ }
spin_unlock_irqrestore(&wl->wl_lock, flags);
/* queue the packet */
@@ -1697,6 +1708,7 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
{
enum wl1271_cmd_ps_mode mode;
struct wl1271 *wl = hw->priv;
+ struct ieee80211_sta *sta = ieee80211_find_sta(vif, bss_conf->bssid);
bool do_join = false;
bool set_assoc = false;
int ret;
@@ -1915,6 +1927,21 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
}
}
+ if (sta) {
+ if (changed & BSS_CHANGED_HT) {
+ ret = wl1271_acx_set_ht_capabilities(wl,
+ &sta->ht_cap,
+ true);
+ ret = wl1271_acx_set_ht_information(wl,
+ bss_conf->ht_operation_mode);
+ }
+ else
+ if (changed & BSS_CHANGED_ASSOC)
+ ret = wl1271_acx_set_ht_capabilities(wl,
+ &sta->ht_cap,
+ false);
+ }
+
if (changed & BSS_CHANGED_ARP_FILTER) {
__be32 addr = bss_conf->arp_addr_list[0];
WARN_ON(wl->bss_type != BSS_TYPE_STA_BSS);
@@ -2095,14 +2122,14 @@ static struct ieee80211_channel wl1271_channels[] = {
/* mapping to indexes for wl1271_rates */
static const u8 wl1271_rate_to_idx_2ghz[] = {
/* MCS rates are used only with 11n */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS7 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS6 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS5 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS4 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS3 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS2 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS1 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS0 */
+ 7, /* CONF_HW_RXTX_RATE_MCS7 */
+ 6, /* CONF_HW_RXTX_RATE_MCS6 */
+ 5, /* CONF_HW_RXTX_RATE_MCS5 */
+ 4, /* CONF_HW_RXTX_RATE_MCS4 */
+ 3, /* CONF_HW_RXTX_RATE_MCS3 */
+ 2, /* CONF_HW_RXTX_RATE_MCS2 */
+ 1, /* CONF_HW_RXTX_RATE_MCS1 */
+ 0, /* CONF_HW_RXTX_RATE_MCS0 */
11, /* CONF_HW_RXTX_RATE_54 */
10, /* CONF_HW_RXTX_RATE_48 */
@@ -2141,6 +2168,9 @@ static struct ieee80211_supported_band wl1271_band_2ghz = {
.n_channels = ARRAY_SIZE(wl1271_channels),
.bitrates = wl1271_rates,
.n_bitrates = ARRAY_SIZE(wl1271_rates),
+#ifdef CONFIG_WL1271_HT
+ .ht_cap = WL12xx_HT_CAP,
+#endif
};
/* 5 GHz data rates for WL1273 */
@@ -2223,14 +2253,14 @@ static struct ieee80211_channel wl1271_channels_5ghz[] = {
/* mapping to indexes for wl1271_rates_5ghz */
static const u8 wl1271_rate_to_idx_5ghz[] = {
/* MCS rates are used only with 11n */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS7 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS6 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS5 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS4 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS3 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS2 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS1 */
- CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS0 */
+ 7, /* CONF_HW_RXTX_RATE_MCS7 */
+ 6, /* CONF_HW_RXTX_RATE_MCS6 */
+ 5, /* CONF_HW_RXTX_RATE_MCS5 */
+ 4, /* CONF_HW_RXTX_RATE_MCS4 */
+ 3, /* CONF_HW_RXTX_RATE_MCS3 */
+ 2, /* CONF_HW_RXTX_RATE_MCS2 */
+ 1, /* CONF_HW_RXTX_RATE_MCS1 */
+ 0, /* CONF_HW_RXTX_RATE_MCS0 */
7, /* CONF_HW_RXTX_RATE_54 */
6, /* CONF_HW_RXTX_RATE_48 */
@@ -2255,6 +2285,7 @@ static struct ieee80211_supported_band wl1271_band_5ghz = {
.n_channels = ARRAY_SIZE(wl1271_channels_5ghz),
.bitrates = wl1271_rates_5ghz,
.n_bitrates = ARRAY_SIZE(wl1271_rates_5ghz),
+ .ht_cap = WL12xx_HT_CAP,
};
static const u8 *wl1271_band_rate_to_idx[] = {
diff --git a/drivers/net/wireless/wl12xx/wl1271_rx.c b/drivers/net/wireless/wl12xx/wl1271_rx.c
index 94da5dd..109a470 100644
--- a/drivers/net/wireless/wl12xx/wl1271_rx.c
+++ b/drivers/net/wireless/wl12xx/wl1271_rx.c
@@ -53,6 +53,10 @@ static void wl1271_rx_status(struct wl1271 *wl,
status->band = wl->band;
status->rate_idx = wl1271_rate_to_idx(wl, desc->rate);
+ /* 11n support */
+ if (desc->rate <= CONF_HW_RXTX_RATE_MCS0)
+ status->flag |= RX_FLAG_HT;
+
status->signal = desc->rssi;
/*
diff --git a/drivers/net/wireless/wl12xx/wl1271_tx.c b/drivers/net/wireless/wl12xx/wl1271_tx.c
index 1b8295c..af54fef 100644
--- a/drivers/net/wireless/wl12xx/wl1271_tx.c
+++ b/drivers/net/wireless/wl12xx/wl1271_tx.c
@@ -236,6 +236,15 @@ u32 wl1271_tx_enabled_rates_get(struct wl1271 *wl, u32 rate_set)
rate_set >>= 1;
}
+ /* MCS rates indication are on bits 16 - 23 */
+ rate_set >>= HW_HT_RATES_OFFSET - band->n_bitrates;
+
+ for (bit = 0; bit < 8; bit++) {
+ if (rate_set & 0x1)
+ enabled_rates |= (CONF_HW_BIT_RATE_MCS_0 << bit);
+ rate_set >>= 1;
+ }
+
return enabled_rates;
}
--
1.6.0.4
Thanks for the review. comments inline.
Cheers,
Shahar
Luciano Coelho wrote:
> On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
>> Add 11n ability in scan, connection and using MCS rates.
>> The configuration is temporary due to the code incomplete and
>> still in testing process. That plans to be remove in the future.
>>
>> Signed-off-by: Shahar Levi <[email protected]>
>> ---
>
> Some comments below. Again, mostly style-related.
>
>
>> diff --git a/drivers/net/wireless/wl12xx/Makefile b/drivers/net/wireless/wl12xx/Makefile
>> index 0d334d6..92465c8 100644
>> --- a/drivers/net/wireless/wl12xx/Makefile
>> +++ b/drivers/net/wireless/wl12xx/Makefile
>> @@ -19,3 +19,4 @@ obj-$(CONFIG_WL1271_SDIO) += wl1271_sdio.o
>>
>> # small builtin driver bit
>> obj-$(CONFIG_WL12XX_PLATFORM_DATA) += wl12xx_platform_data.o
>> +
>
> I guess this extra empty line was included accidentally here? There are
> no other changes in the Makefile, so please remove this change.
fixed
>
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
>> index bef2c24..7dd1257 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
>> @@ -841,10 +841,21 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>>
>> /* peek into the rates configured in the STA entry */
>> spin_lock_irqsave(&wl->wl_lock, flags);
>> - if (sta && sta->supp_rates[conf->channel->band] != wl->sta_rate_set) {
>> + if (sta &&
>> + (sta->supp_rates[conf->channel->band] !=
>> + (wl->sta_rate_set & HW_BG_RATES_MASK))) {
>> wl->sta_rate_set = sta->supp_rates[conf->channel->band];
>
> Shouldn't this be |= here like the one below? Otherwise the HT rates
> will be zeroed. Okay, this would be fixed below, because the zeroed HT
> rates will not match the configured rates, but in some cases that will
> be unnecessary.
I add line that clean MCS bits before ssetting it:
if (sta &&
sta->ht_cap.ht_supported &&
((wl->sta_rate_set >> HW_HT_RATES_OFFSET) !=rate_set
sta->ht_cap.mcs.rx_mask[0])) {
/* Clean MCS bits before setting them */
wl->sta_rate_set &= CONF_HW_BG_RATES_MASK;
wl->sta_rate_set |= (sta->ht_cap.mcs.rx_mask[0] << HW_HT_RATES_OFFSET);
set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
}
>
>
>> set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
>> }
>> +
>> + if (sta &&
>> + sta->ht_cap.ht_supported &&
>> + ((wl->sta_rate_set >> HW_HT_RATES_OFFSET) !=
>> + sta->ht_cap.mcs.rx_mask[0])) {
>> + wl->sta_rate_set |=
>> + (sta->ht_cap.mcs.rx_mask[0] << HW_HT_RATES_OFFSET);
>> + set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
>> + }
>> spin_unlock_irqrestore(&wl->wl_lock, flags);
>>
>> /* queue the packet */
>> @@ -1697,6 +1708,7 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
>> {
>> enum wl1271_cmd_ps_mode mode;
>> struct wl1271 *wl = hw->priv;
>> + struct ieee80211_sta *sta = ieee80211_find_sta(vif, bss_conf->bssid);
>> bool do_join = false;
>> bool set_assoc = false;
>> int ret;
>> @@ -1915,6 +1927,21 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
>> }
>> }
>>
>> + if (sta) {
>> + if (changed & BSS_CHANGED_HT) {
>> + ret = wl1271_acx_set_ht_capabilities(wl,
>> + &sta->ht_cap,
>> + true);
>> + ret = wl1271_acx_set_ht_information(wl,
>> + bss_conf->ht_operation_mode);
>> + }
>> + else
>> + if (changed & BSS_CHANGED_ASSOC)
>> + ret = wl1271_acx_set_ht_capabilities(wl,
>> + &sta->ht_cap,
>> + false);
>> + }
>> +
>
>
> The closing brace is indented wrongly here. And you should also use
> braces in the else block (see Documentation/CodingStyle).
>
> But in any case, you could reduce the indentation and prevent unaligned
> parameters in the wl1271_acx_* calls if you do this:
>
> if (sta && changed & BSS_CHANGED_HT) {
>
> and
>
> if (sta && changed & BSS_CHANGED_ASSOC)
fixed
>
>
>> @@ -2255,6 +2285,7 @@ static struct ieee80211_supported_band wl1271_band_5ghz = {
>> .n_channels = ARRAY_SIZE(wl1271_channels_5ghz),
>> .bitrates = wl1271_rates_5ghz,
>> .n_bitrates = ARRAY_SIZE(wl1271_rates_5ghz),
>> + .ht_cap = WL12xx_HT_CAP,
>> };
>
> You forgot the #ifdef around .ht_cap here. Actually, it might be nicer
> to put the #ifdef around the macro itself, so you can explicitly set
> the .ht_supported flag to false if 11n is not configured.
fixed
>
>
>> static const u8 *wl1271_band_rate_to_idx[] = {
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_rx.c b/drivers/net/wireless/wl12xx/wl1271_rx.c
>> index 94da5dd..109a470 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_rx.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_rx.c
>> @@ -53,6 +53,10 @@ static void wl1271_rx_status(struct wl1271 *wl,
>> status->band = wl->band;
>> status->rate_idx = wl1271_rate_to_idx(wl, desc->rate);
>>
>> + /* 11n support */
>> + if (desc->rate <= CONF_HW_RXTX_RATE_MCS0)
>> + status->flag |= RX_FLAG_HT;
>> +
>> status->signal = desc->rssi;
>
> Should this be #ifdef'ed also?
No, in case of HT not supported the HW will not set MCS rate.
>
>
>> /*
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_tx.c b/drivers/net/wireless/wl12xx/wl1271_tx.c
>> index 1b8295c..af54fef 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_tx.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_tx.c
>> @@ -236,6 +236,15 @@ u32 wl1271_tx_enabled_rates_get(struct wl1271 *wl, u32 rate_set)
>> rate_set >>= 1;
>> }
>>
>> + /* MCS rates indication are on bits 16 - 23 */
>> + rate_set >>= HW_HT_RATES_OFFSET - band->n_bitrates;
>> +
>> + for (bit = 0; bit < 8; bit++) {
>> + if (rate_set & 0x1)
>> + enabled_rates |= (CONF_HW_BIT_RATE_MCS_0 << bit);
>> + rate_set >>= 1;
>> + }
>> +
>
> And this? #ifdef here as well, maybe?
No, in case of HT not supported MCS bit in rate_set will not be set.
>
>
On Wed, 2010-10-06 at 20:08 +0200, Shahar Levi wrote:
> /* peek into the rates configured in the STA entry */
> spin_lock_irqsave(&wl->wl_lock, flags);
> - if (sta && sta->supp_rates[conf->channel->band] != wl->sta_rate_set) {
> + if (sta &&
> + (sta->supp_rates[conf->channel->band] !=
> + (wl->sta_rate_set & HW_BG_RATES_MASK))) {
> wl->sta_rate_set = sta->supp_rates[conf->channel->band];
> set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags);
> }
???
How can non-BG rates ever show up there? Is this really an 11a change?
johannes
On Thu, 2010-10-07 at 20:20 +0200, ext Luciano Coelho wrote:
> > diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h
> b/drivers/net/wireless/wl12xx/wl1271_conf.h
> > index 60c50d1..732e9aa 100644
> > --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
> > +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
> > @@ -91,6 +91,10 @@ enum {
> > CONF_HW_RXTX_RATE_UNSUPPORTED = 0xff
> > };
> >
> > +#define HW_RX_HIGHEST_RATE 65
> > +#define HW_BG_RATES_MASK 0xffff
> > +#define HW_HT_RATES_OFFSET 16
>
> We preceed all the other macros with CONF_, please do the same for
> these.
Actually, we use the wl1271_conf.h file only for driver configuration
parameters. These macros should be elsewhere, such as in the wl1271.h
file. And no need to put CONF_ before them in that case.
--
Cheers,
Luca.
Luciano Coelho wrote:
> True, but if you don't have the ifdef you're adding unnecessary code in
> the RX data path.
>
>
> Same here, this loop will add more CPU cycles in the TX data path
> unnecessarily.
>
you right, will be fix on v3.
Thanks,
Shahar