Hello all,
this patches currently pending for review or ack. Can please some one
do it untill i forgt them :)
Oleksij Rempel (2):
ath9k_htc: add STBC TX support
ath9k: remove useless flag conversation.
drivers/net/wireless/ath/ath9k/ar9003_mac.c | 5 +++--
drivers/net/wireless/ath/ath9k/htc.h | 2 ++
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 4 ++++
drivers/net/wireless/ath/ath9k/mac.c | 11 +++++++----
drivers/net/wireless/ath/ath9k/mac.h | 1 +
drivers/net/wireless/ath/ath9k/recv.c | 5 +----
7 files changed, 21 insertions(+), 10 deletions(-)
--
1.8.1.2
On 4 May 2013 04:16, Felix Fietkau <[email protected]> wrote:
>> So, should i remove ATH_RC_TX_STBC_FLAG from my patch?
> I extensively reviewed this part, and it's really crazy. Here's what
> happens:
>
> ath_rate_newassoc_11n takes ATH_RC_* flags, converts them to WLAN_RC_*.
> rcSibUpdate_11n interprets the WLAN_RC_* flags as ATH_RC_* and converts
> them to WLAN_RC_* again. For most flags this is pretty much a no-op
> because the definitions are identical.
> For STBC the result 'accidentally' still contains WLAN_RC_STBC_FLAG, but
> only because ath_rate_newassoc_11n converts ATH_RC_RX_STBC_FLAG to
> WLAN_RC_STBC_FLAG and WLAN_RC_STBC_FLAG overlaps with ATH_RC_TX_STBC_FLAG.
> In the end it doesn't matter anymore, because nothing in the code takes
> the STBC info from the capflags.
>
> STBC is used if ATH_NODE_ATHEROS(an)->stbc is non-zero, and this gets
> set by ath_rate_newassoc_11n before all of those incredibly moronic
> conversions happen.
It smells like left-over from the 7.x driver code this is based off of.
Adrian
Am 02.05.2013 18:55, schrieb Adrian Chadd:
> On 2 May 2013 01:11, Oleksij Rempel <[email protected]> wrote:
>
>> +#define WLAN_RC_TX_STBC_FLAG 0x20 /* TX STBC */
>> +#define WLAN_RC_RX_STBC_FLAG 0xC0 /* RX STBC ,2 bits */
>
> I thought we covered this; why are you marking two bits here?
becouse firmware checks for two bits (and then use it as bool ;)), so i
pass what firmware can handle.
> Atheros 11n hardware only supports 1-stream STBC RX.
Did you got my email with lots of assumptions and questions?
What do you mean by 1-stream STBC RX? After i did some home work on STBC
i see that it encoded from at least two spatial streams.
Is
1-stream STBC RX = 2 spatial streams with mirrored data?
and
2-stream STBC RX = 4 spatial streams with mirrored data?
or
1-stream STBC RX = compatibility mode for one stream hardware(so only of
two streams received)?
That would make sense for 1x1:1 hardware, but if you say all atheros N
hardware support only 1-stream STBC RX, will mean that STBC is useless
on this hardware.
> Have you verified that we're actually negotiating 1-stream STBC RX
> with a peer? (Ie, by looking at packet captures?)
Yes, i wrote this too :) I verified it, we negotiate only 1-stream STBC RX.
--
Regards,
Oleksij
On 2013-05-04 1:08 PM, Oleksij Rempel wrote:
> Am 04.05.2013 12:02, schrieb Felix Fietkau:
>> On 2013-05-04 8:50 AM, Oleksij Rempel wrote:
>>> Am 02.05.2013 22:15, schrieb Adrian Chadd:
>>>> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
>>>
>>> Does it mean, i should change this patch and provide a patch for
>>> firmware too?
>>> I still do not think, changing peer caps i a good idea in any case.
>>> I mena this part of patch:
>>> + if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
>>> + caps |= WLAN_RC_TX_STBC_FLAG;
>>>
>>>
>>> Behaviour with this patch will be fallowing:
>>> - peer provide caps, even if it is RX-STBC12
>>> - we pass this information to firmware and ratecontroller of FW, do
>>> right decision based on hardware it has.
>>>
>>> You suggestion, if i understand it correctly, is to filter caps:
>>> - if peer provide more than we can, we should tell firmware the value
>>> what we can. So, if peer say it can RX-STBC12, we should tell firmware
>>> that peer is RX-STBC1.
>>> In my opinion, this kind of filter is a source for hidden errors.
>> I think the discussion regarding RX-STBC12 vs RX-STBC1 is purely
>> hypothetical. The hardware that this firmware was designed for only
>> supports sending STBC for MCS0-7. This will not change.
>>
>> Also, there's no reason to tell the firmware about both rx and tx STBC
>> capabilities, the only thing it needs to know is what tells the rate
>> control part to enable/disable STBC.
>
> As you can see, in version 2 of this path there was no more discussion
> about it. I just did it.
>
>> In addition to that, using the WLAN_RC_* flags is wrong, you need to use
>> the ATH_RC_* flags, as this is what ath_rate_newassoc_11n checks for in
>> the firmware.
>
> Renamed.
>
>> The only STBC related flag that actually gets used (when
>> passed from the driver) is ATH_RC_RX_STBC_FLAG.
>
> Well, may be it is not used at the end. But it is present on some places
> in the firmware.
> For example here:
> void
> rcSibUpdate_11n(struct ath_softc_tgt *sc, struct ath_node_target *pSib,
> A_UINT32 capflag, A_BOOL keepState, struct
> ieee80211_rate *pRateSet)
> {
> rcSibUpdate_ht(sc,
> pSib,
> ((capflag & ATH_RC_DS_FLAG) ? WLAN_RC_DS_FLAG
> : 0) |
> ((capflag & ATH_RC_HT40_SGI_FLAG) ?
> WLAN_RC_HT40_SGI_FLAG : 0) |
> ((capflag & ATH_RC_HT_FLAG) ? WLAN_RC_HT_FLAG
> : 0) |
> ((capflag & ATH_RC_CW40_FLAG) ? WLAN_RC_40_FLAG
> : 0) |
> ((capflag & ATH_RC_TX_STBC_FLAG) ?
> WLAN_RC_STBC_FLAG : 0),
> keepState,
> pRateSet);
>
>
>
> So, should i remove ATH_RC_TX_STBC_FLAG from my patch?
I extensively reviewed this part, and it's really crazy. Here's what
happens:
ath_rate_newassoc_11n takes ATH_RC_* flags, converts them to WLAN_RC_*.
rcSibUpdate_11n interprets the WLAN_RC_* flags as ATH_RC_* and converts
them to WLAN_RC_* again. For most flags this is pretty much a no-op
because the definitions are identical.
For STBC the result 'accidentally' still contains WLAN_RC_STBC_FLAG, but
only because ath_rate_newassoc_11n converts ATH_RC_RX_STBC_FLAG to
WLAN_RC_STBC_FLAG and WLAN_RC_STBC_FLAG overlaps with ATH_RC_TX_STBC_FLAG.
In the end it doesn't matter anymore, because nothing in the code takes
the STBC info from the capflags.
STBC is used if ATH_NODE_ATHEROS(an)->stbc is non-zero, and this gets
set by ath_rate_newassoc_11n before all of those incredibly moronic
conversions happen.
- Felix
current firmware will enable STBC_TX, only if other peer support it.
This patch provide ht_peer_caps to firmware.
FW versions 1.3, 1.4 should be able to work with it.
Tested on ar7010+ar9280 and ar7010+ar9287.
- v2. Use one bit instead of two for RX STBC flags.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc.h | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 4 ++++
3 files changed, 10 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index d3b099d..037e9b8 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -142,6 +142,9 @@ struct ath9k_htc_target_aggr {
#define WLAN_RC_40_FLAG 0x02
#define WLAN_RC_SGI_FLAG 0x04
#define WLAN_RC_HT_FLAG 0x08
+#define WLAN_RC_TX_STBC_FLAG 0x20 /* TX STBC */
+/* RX STBC. Warning! FW checks for 0xC0. But we support only 1 stream anyway */
+#define WLAN_RC_RX_STBC_FLAG 0x40
struct ath9k_htc_rateset {
u8 rs_nrates;
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index a47f5e0..c79c5ac 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -517,6 +517,9 @@ static void setup_ht_cap(struct ath9k_htc_priv *priv,
ath_dbg(common, CONFIG, "TX streams %d, RX streams: %d\n",
tx_streams, rx_streams);
+ if (tx_streams >= 2)
+ ht_info->cap |= IEEE80211_HT_CAP_TX_STBC;
+
if (tx_streams != rx_streams) {
ht_info->mcs.tx_params |= IEEE80211_HT_MCS_TX_RX_DIFF;
ht_info->mcs.tx_params |= ((tx_streams - 1) <<
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index 0743a47..af08b4a 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -623,6 +623,10 @@ static void ath9k_htc_setup_rate(struct ath9k_htc_priv *priv,
trate->rates.ht_rates.rs_nrates = j;
caps = WLAN_RC_HT_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_RX_STBC)
+ caps |= WLAN_RC_RX_STBC_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
+ caps |= WLAN_RC_TX_STBC_FLAG;
if (sta->ht_cap.mcs.rx_mask[1])
caps |= WLAN_RC_DS_FLAG;
if ((sta->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) &&
--
1.8.1.2
I've just reviewed it myself:
* oan->stbc is enabled only if the hardware itself supports STBC; so
it's an extra sanity check in case the firmware is told to enable STBC
in the WMI capflag field.
* is oan->htinfo used anywhere that may involve STBC?
* .. we should check whether ath9k_htc ever set the STBC flags on
AR9271, or we'd end up confusing the hardware.
* .. I don't think that is important though, as we weren't _doing_ STBC, right?
* Why are the ATH_RC_* flags used in newassoc_11n? This comes from the
WMI WMI_RC_STATE_CHANGE_CMDID capflag field; where are _those_
defined?
Grr, so many things to tidy up.
Adrian
Am 04.05.2013 19:50, schrieb Adrian Chadd:
> I've just reviewed it myself:
>
> * oan->stbc is enabled only if the hardware itself supports STBC; so
> it's an extra sanity check in case the firmware is told to enable STBC
> in the WMI capflag field.
all STBC parts are not compiled for AR9271. Currently firmware will do
sanity check on htc_7010.fw and always return 1, and wont do any check
on htc_9271.fw (this part is just not compiled).
> * is oan->htinfo used anywhere that may involve STBC?
hmm...oan->htinfo?
> * .. we should check whether ath9k_htc ever set the STBC flags on
> AR9271, or we'd end up confusing the hardware.
no, never. there are legion guardians ;) last on is on preparing tx
descriptor.
> * .. I don't think that is important though, as we weren't _doing_ STBC, right?
correct, currently STBC is not working on linux ath9k_htc
> * Why are the ATH_RC_* flags used in newassoc_11n? This comes from the
> WMI WMI_RC_STATE_CHANGE_CMDID capflag field; where are _those_
> defined?
they are in target_firmware/wlan/if_athrate.h
and well, there are a bit more problems thin them.
See attachment :) Attached patch can go on top of my merge request for
firmware. So, i will probably need to make Patch_v5 for ath9k_htc... since.
Felix,
> In addition to that, using the WLAN_RC_* flags is wrong, you need to use
> the ATH_RC_* flags,...
After some firmware cleaning i will need to use WLAN_RC_* flags :)
WLAN_RC_* have same values like ATH_RC_*...
> Grr, so many things to tidy up.
--
Regards,
Oleksij
Am 04.05.2013 08:50, schrieb Oleksij Rempel:
> Am 02.05.2013 22:15, schrieb Adrian Chadd:
>> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
>
> Does it mean, i should change this patch and provide a patch for
> firmware too?
> I still do not think, changing peer caps i a good idea in any case.
> I mena this part of patch:
> + if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
> + caps |= WLAN_RC_TX_STBC_FLAG;
>
oops... my error. I see the problem now.
I do not pass flags provided by peer.
--
Regards,
Oleksij
some flags used only outside of ath9k - In this case we can use
"enum mac80211_rx_flags" and pass it upstream without extra
conversation.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/ar9003_mac.c | 5 +++--
drivers/net/wireless/ath/ath9k/mac.c | 11 +++++++----
drivers/net/wireless/ath/ath9k/mac.h | 1 +
drivers/net/wireless/ath/ath9k/recv.c | 5 +----
4 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_mac.c b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
index 301bf72..5163abd 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_mac.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
@@ -469,6 +469,7 @@ int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
rxs->rs_status = 0;
rxs->rs_flags = 0;
+ rxs->flag = 0;
rxs->rs_datalen = rxsp->status2 & AR_DataLen;
rxs->rs_tstamp = rxsp->status3;
@@ -493,8 +494,8 @@ int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
rxs->rs_isaggr = (rxsp->status11 & AR_RxAggr) ? 1 : 0;
rxs->rs_moreaggr = (rxsp->status11 & AR_RxMoreAggr) ? 1 : 0;
rxs->rs_antenna = (MS(rxsp->status4, AR_RxAntenna) & 0x7);
- rxs->rs_flags = (rxsp->status4 & AR_GI) ? ATH9K_RX_GI : 0;
- rxs->rs_flags |= (rxsp->status4 & AR_2040) ? ATH9K_RX_2040 : 0;
+ rxs->flag |= (rxsp->status4 & AR_GI) ? RX_FLAG_SHORT_GI : 0;
+ rxs->flag |= (rxsp->status4 & AR_2040) ? RX_FLAG_40MHZ : 0;
rxs->evm0 = rxsp->status6;
rxs->evm1 = rxsp->status7;
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 498fee0..a52081d 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -547,6 +547,7 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
rs->rs_status = 0;
rs->rs_flags = 0;
+ rs->flag = 0;
rs->rs_datalen = ads.ds_rxstatus1 & AR_DataLen;
rs->rs_tstamp = ads.AR_RcvTimestamp;
@@ -586,10 +587,12 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
rs->rs_moreaggr =
(ads.ds_rxstatus8 & AR_RxMoreAggr) ? 1 : 0;
rs->rs_antenna = MS(ads.ds_rxstatus3, AR_RxAntenna);
- rs->rs_flags =
- (ads.ds_rxstatus3 & AR_GI) ? ATH9K_RX_GI : 0;
- rs->rs_flags |=
- (ads.ds_rxstatus3 & AR_2040) ? ATH9K_RX_2040 : 0;
+
+ /* directly mapped flags for ieee80211_rx_status */
+ rs->flag |=
+ (ads.ds_rxstatus3 & AR_GI) ? RX_FLAG_SHORT_GI : 0;
+ rs->flag |=
+ (ads.ds_rxstatus3 & AR_2040) ? RX_FLAG_40MHZ : 0;
if (ads.ds_rxstatus8 & AR_PreDelimCRCErr)
rs->rs_flags |= ATH9K_RX_DELIM_CRC_PRE;
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index 5865f92..3f1e775 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -149,6 +149,7 @@ struct ath_rx_status {
u32 evm2;
u32 evm3;
u32 evm4;
+ u32 flag; /* see enum mac80211_rx_flags */
};
struct ath_htc_rx_status {
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 8be2b5d..b4b758d 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -868,10 +868,7 @@ static int ath9k_process_rate(struct ath_common *common,
if (rx_stats->rs_rate & 0x80) {
/* HT rate */
rxs->flag |= RX_FLAG_HT;
- if (rx_stats->rs_flags & ATH9K_RX_2040)
- rxs->flag |= RX_FLAG_40MHZ;
- if (rx_stats->rs_flags & ATH9K_RX_GI)
- rxs->flag |= RX_FLAG_SHORT_GI;
+ rxs->flag |= rx_stats->flag;
rxs->rate_idx = rx_stats->rs_rate & 0x7f;
return 0;
}
--
1.8.1.2
On 2013-05-04 8:50 AM, Oleksij Rempel wrote:
> Am 02.05.2013 22:15, schrieb Adrian Chadd:
>> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
>
> Does it mean, i should change this patch and provide a patch for
> firmware too?
> I still do not think, changing peer caps i a good idea in any case.
> I mena this part of patch:
> + if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
> + caps |= WLAN_RC_TX_STBC_FLAG;
>
>
> Behaviour with this patch will be fallowing:
> - peer provide caps, even if it is RX-STBC12
> - we pass this information to firmware and ratecontroller of FW, do
> right decision based on hardware it has.
>
> You suggestion, if i understand it correctly, is to filter caps:
> - if peer provide more than we can, we should tell firmware the value
> what we can. So, if peer say it can RX-STBC12, we should tell firmware
> that peer is RX-STBC1.
> In my opinion, this kind of filter is a source for hidden errors.
I think the discussion regarding RX-STBC12 vs RX-STBC1 is purely
hypothetical. The hardware that this firmware was designed for only
supports sending STBC for MCS0-7. This will not change.
Also, there's no reason to tell the firmware about both rx and tx STBC
capabilities, the only thing it needs to know is what tells the rate
control part to enable/disable STBC.
In addition to that, using the WLAN_RC_* flags is wrong, you need to use
the ATH_RC_* flags, as this is what ath_rate_newassoc_11n checks for in
the firmware. The only STBC related flag that actually gets used (when
passed from the driver) is ATH_RC_RX_STBC_FLAG.
- Felix
Hi Felix,
thank you for your explanation and being pation with me.
I learn it by my self and keywords you gave help me to find needed
information. So, i continue to digg in to google books and wikis now.
I see now, that my initial assumption that STBC some thing like
"frequency diversity" is wrong. Well, it say by itself Space and Time,
no freq :)
Am 02.05.2013 20:01, schrieb Felix Fietkau:
> On 2013-05-02 7:32 PM, Oleksij Rempel wrote:
>> Am 02.05.2013 18:55, schrieb Adrian Chadd:
>>> On 2 May 2013 01:11, Oleksij Rempel <[email protected]> wrote:
>>>
>>>> +#define WLAN_RC_TX_STBC_FLAG 0x20 /* TX STBC */
>>>> +#define WLAN_RC_RX_STBC_FLAG 0xC0 /* RX STBC ,2 bits */
>>>
>>> I thought we covered this; why are you marking two bits here?
>>
>> becouse firmware checks for two bits (and then use it as bool ;)), so i
>> pass what firmware can handle.
>>
>>> Atheros 11n hardware only supports 1-stream STBC RX.
>>
>> Did you got my email with lots of assumptions and questions?
>> What do you mean by 1-stream STBC RX? After i did some home work on STBC
>> i see that it encoded from at least two spatial streams.
>> Is
>> 1-stream STBC RX = 2 spatial streams with mirrored data?
>> and
>> 2-stream STBC RX = 4 spatial streams with mirrored data?
>>
>> or
>>
>> 1-stream STBC RX = compatibility mode for one stream hardware(so only of
>> two streams received)?
> When you're talking about 'streams', please specify where you're talking
> about Spatial Streams (Nss, defined by the MCS), or Space-Time Streams
> (Nsts). STBC is useful whenever the number of possible Space-Time
> streams exceeds the number of Spatial streams, i.e. if the number of tx
> chains is bigger than the number of spatial streams.
> There's an asymmetry between Rx and Tx here. If a receiver has 1 chain
> and the transmitter has 2 chains, tx can use 2 Space-Time streams to
> encode 1 Spatial stream to improve the reliability of the signal.
> The HT STBC capability field indicates the maximum number of Spatial
> Streams, not Space-Time streams. Atheros hardware only supports STBC
> with Nss = 1, so announcing 2-stream STBC is definitely wrong.
Ok, i finally found it on ieee 802.11 specification.
For STBC:
Nsts=2 - Nss=1
Nsts=3 - Nss=2
Nsts=4 - Nss=2
Nsts=4 - Nss=3
>> That would make sense for 1x1:1 hardware, but if you say all atheros N
>> hardware support only 1-stream STBC RX, will mean that STBC is useless
>> on this hardware.
> Only STBC With Nss=1, Nsts=2 is supported, but this does not make it
> useless at all. It helps, even if the receiver only has one antenna.
Found it too.. :)
Thx!
--
Regards,
Oleksij
On 2 May 2013 01:11, Oleksij Rempel <[email protected]> wrote:
> +#define WLAN_RC_TX_STBC_FLAG 0x20 /* TX STBC */
> +#define WLAN_RC_RX_STBC_FLAG 0xC0 /* RX STBC ,2 bits */
I thought we covered this; why are you marking two bits here?
Atheros 11n hardware only supports 1-stream STBC RX.
Have you verified that we're actually negotiating 1-stream STBC RX
with a peer? (Ie, by looking at packet captures?)
Adrian
Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
Adrian
current firmware will enable STBC_TX, only if other peer support it.
This patch provide ht_peer_caps to firmware.
FW versions 1.3, 1.4 should be able to work with it.
Tested on ar7010+ar9280 and ar7010+ar9287.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc.h | 2 ++
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 4 ++++
3 files changed, 9 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index d3b099d..db4a793 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -142,6 +142,8 @@ struct ath9k_htc_target_aggr {
#define WLAN_RC_40_FLAG 0x02
#define WLAN_RC_SGI_FLAG 0x04
#define WLAN_RC_HT_FLAG 0x08
+#define WLAN_RC_TX_STBC_FLAG 0x20 /* TX STBC */
+#define WLAN_RC_RX_STBC_FLAG 0xC0 /* RX STBC ,2 bits */
struct ath9k_htc_rateset {
u8 rs_nrates;
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index a47f5e0..c79c5ac 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -517,6 +517,9 @@ static void setup_ht_cap(struct ath9k_htc_priv *priv,
ath_dbg(common, CONFIG, "TX streams %d, RX streams: %d\n",
tx_streams, rx_streams);
+ if (tx_streams >= 2)
+ ht_info->cap |= IEEE80211_HT_CAP_TX_STBC;
+
if (tx_streams != rx_streams) {
ht_info->mcs.tx_params |= IEEE80211_HT_MCS_TX_RX_DIFF;
ht_info->mcs.tx_params |= ((tx_streams - 1) <<
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index 0743a47..af08b4a 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -623,6 +623,10 @@ static void ath9k_htc_setup_rate(struct ath9k_htc_priv *priv,
trate->rates.ht_rates.rs_nrates = j;
caps = WLAN_RC_HT_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_RX_STBC)
+ caps |= WLAN_RC_RX_STBC_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
+ caps |= WLAN_RC_TX_STBC_FLAG;
if (sta->ht_cap.mcs.rx_mask[1])
caps |= WLAN_RC_DS_FLAG;
if ((sta->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) &&
--
1.8.1.2
current firmware will enable STBC_TX, only if other peer support it.
This patch provide ht_peer_caps to firmware.
FW versions 1.3, 1.4 should be able to work with it.
Tested on ar7010+ar9280 and ar7010+ar9287.
- v2. Use one bit instead of two for RX STBC flags.
- v3. Use ATH_RC_*_STBC_FLAG instead of WLAN_RC_RX_STBC_FLAG
- v4. Remove TX flag. This flag is used only to keep AR7010 warm.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc.h | 1 +
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 ++
3 files changed, 6 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index d3b099d..c56a0d4 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -142,6 +142,7 @@ struct ath9k_htc_target_aggr {
#define WLAN_RC_40_FLAG 0x02
#define WLAN_RC_SGI_FLAG 0x04
#define WLAN_RC_HT_FLAG 0x08
+#define ATH_RC_RX_STBC_FLAG 0x40
struct ath9k_htc_rateset {
u8 rs_nrates;
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index a47f5e0..c79c5ac 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -517,6 +517,9 @@ static void setup_ht_cap(struct ath9k_htc_priv *priv,
ath_dbg(common, CONFIG, "TX streams %d, RX streams: %d\n",
tx_streams, rx_streams);
+ if (tx_streams >= 2)
+ ht_info->cap |= IEEE80211_HT_CAP_TX_STBC;
+
if (tx_streams != rx_streams) {
ht_info->mcs.tx_params |= IEEE80211_HT_MCS_TX_RX_DIFF;
ht_info->mcs.tx_params |= ((tx_streams - 1) <<
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index 0743a47..c744135 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -623,6 +623,8 @@ static void ath9k_htc_setup_rate(struct ath9k_htc_priv *priv,
trate->rates.ht_rates.rs_nrates = j;
caps = WLAN_RC_HT_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_RX_STBC)
+ caps |= ATH_RC_RX_STBC_FLAG;
if (sta->ht_cap.mcs.rx_mask[1])
caps |= WLAN_RC_DS_FLAG;
if ((sta->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) &&
--
1.8.1.2
Am 02.05.2013 22:15, schrieb Adrian Chadd:
> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
Does it mean, i should change this patch and provide a patch for
firmware too?
I still do not think, changing peer caps i a good idea in any case.
I mena this part of patch:
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
+ caps |= WLAN_RC_TX_STBC_FLAG;
Behaviour with this patch will be fallowing:
- peer provide caps, even if it is RX-STBC12
- we pass this information to firmware and ratecontroller of FW, do
right decision based on hardware it has.
You suggestion, if i understand it correctly, is to filter caps:
- if peer provide more than we can, we should tell firmware the value
what we can. So, if peer say it can RX-STBC12, we should tell firmware
that peer is RX-STBC1.
In my opinion, this kind of filter is a source for hidden errors.
--
Regards,
Oleksij
Am 04.05.2013 12:02, schrieb Felix Fietkau:
> On 2013-05-04 8:50 AM, Oleksij Rempel wrote:
>> Am 02.05.2013 22:15, schrieb Adrian Chadd:
>>> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
>>
>> Does it mean, i should change this patch and provide a patch for
>> firmware too?
>> I still do not think, changing peer caps i a good idea in any case.
>> I mena this part of patch:
>> + if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
>> + caps |= WLAN_RC_TX_STBC_FLAG;
>>
>>
>> Behaviour with this patch will be fallowing:
>> - peer provide caps, even if it is RX-STBC12
>> - we pass this information to firmware and ratecontroller of FW, do
>> right decision based on hardware it has.
>>
>> You suggestion, if i understand it correctly, is to filter caps:
>> - if peer provide more than we can, we should tell firmware the value
>> what we can. So, if peer say it can RX-STBC12, we should tell firmware
>> that peer is RX-STBC1.
>> In my opinion, this kind of filter is a source for hidden errors.
> I think the discussion regarding RX-STBC12 vs RX-STBC1 is purely
> hypothetical. The hardware that this firmware was designed for only
> supports sending STBC for MCS0-7. This will not change.
>
> Also, there's no reason to tell the firmware about both rx and tx STBC
> capabilities, the only thing it needs to know is what tells the rate
> control part to enable/disable STBC.
As you can see, in version 2 of this path there was no more discussion
about it. I just did it.
> In addition to that, using the WLAN_RC_* flags is wrong, you need to use
> the ATH_RC_* flags, as this is what ath_rate_newassoc_11n checks for in
> the firmware.
Renamed.
> The only STBC related flag that actually gets used (when
> passed from the driver) is ATH_RC_RX_STBC_FLAG.
Well, may be it is not used at the end. But it is present on some places
in the firmware.
For example here:
void
rcSibUpdate_11n(struct ath_softc_tgt *sc, struct ath_node_target *pSib,
A_UINT32 capflag, A_BOOL keepState, struct
ieee80211_rate *pRateSet)
{
rcSibUpdate_ht(sc,
pSib,
((capflag & ATH_RC_DS_FLAG) ? WLAN_RC_DS_FLAG
: 0) |
((capflag & ATH_RC_HT40_SGI_FLAG) ?
WLAN_RC_HT40_SGI_FLAG : 0) |
((capflag & ATH_RC_HT_FLAG) ? WLAN_RC_HT_FLAG
: 0) |
((capflag & ATH_RC_CW40_FLAG) ? WLAN_RC_40_FLAG
: 0) |
((capflag & ATH_RC_TX_STBC_FLAG) ?
WLAN_RC_STBC_FLAG : 0),
keepState,
pRateSet);
So, should i remove ATH_RC_TX_STBC_FLAG from my patch?
--
Regards,
Oleksij
current firmware will enable STBC_TX, only if other peer support it.
This patch provide ht_peer_caps to firmware.
FW versions 1.3, 1.4 should be able to work with it.
Tested on ar7010+ar9280 and ar7010+ar9287.
- v2. Use one bit instead of two for RX STBC flags.
- v3. Use ATH_RC_*_STBC_FLAG instead of WLAN_RC_RX_STBC_FLAG
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc.h | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 4 ++++
3 files changed, 10 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index d3b099d..7e7fe62 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -142,6 +142,9 @@ struct ath9k_htc_target_aggr {
#define WLAN_RC_40_FLAG 0x02
#define WLAN_RC_SGI_FLAG 0x04
#define WLAN_RC_HT_FLAG 0x08
+#define ATH_RC_TX_STBC_FLAG 0x20 /* TX STBC */
+/* RX STBC. Warning! FW checks for 0xC0. But we support only 1 stream anyway */
+#define ATH_RC_RX_STBC_FLAG 0x40
struct ath9k_htc_rateset {
u8 rs_nrates;
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index a47f5e0..c79c5ac 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -517,6 +517,9 @@ static void setup_ht_cap(struct ath9k_htc_priv *priv,
ath_dbg(common, CONFIG, "TX streams %d, RX streams: %d\n",
tx_streams, rx_streams);
+ if (tx_streams >= 2)
+ ht_info->cap |= IEEE80211_HT_CAP_TX_STBC;
+
if (tx_streams != rx_streams) {
ht_info->mcs.tx_params |= IEEE80211_HT_MCS_TX_RX_DIFF;
ht_info->mcs.tx_params |= ((tx_streams - 1) <<
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index 0743a47..d4205e7 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -623,6 +623,10 @@ static void ath9k_htc_setup_rate(struct ath9k_htc_priv *priv,
trate->rates.ht_rates.rs_nrates = j;
caps = WLAN_RC_HT_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_RX_STBC)
+ caps |= ATH_RC_RX_STBC_FLAG;
+ if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
+ caps |= ATH_RC_TX_STBC_FLAG;
if (sta->ht_cap.mcs.rx_mask[1])
caps |= WLAN_RC_DS_FLAG;
if ((sta->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) &&
--
1.8.1.2
Am 04.05.2013 13:16, schrieb Felix Fietkau:
> On 2013-05-04 1:08 PM, Oleksij Rempel wrote:
>> Am 04.05.2013 12:02, schrieb Felix Fietkau:
>>> On 2013-05-04 8:50 AM, Oleksij Rempel wrote:
>>>> Am 02.05.2013 22:15, schrieb Adrian Chadd:
>>>>> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
>>>>
>>>> Does it mean, i should change this patch and provide a patch for
>>>> firmware too?
>>>> I still do not think, changing peer caps i a good idea in any case.
>>>> I mena this part of patch:
>>>> + if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
>>>> + caps |= WLAN_RC_TX_STBC_FLAG;
>>>>
>>>>
>>>> Behaviour with this patch will be fallowing:
>>>> - peer provide caps, even if it is RX-STBC12
>>>> - we pass this information to firmware and ratecontroller of FW, do
>>>> right decision based on hardware it has.
>>>>
>>>> You suggestion, if i understand it correctly, is to filter caps:
>>>> - if peer provide more than we can, we should tell firmware the value
>>>> what we can. So, if peer say it can RX-STBC12, we should tell firmware
>>>> that peer is RX-STBC1.
>>>> In my opinion, this kind of filter is a source for hidden errors.
>>> I think the discussion regarding RX-STBC12 vs RX-STBC1 is purely
>>> hypothetical. The hardware that this firmware was designed for only
>>> supports sending STBC for MCS0-7. This will not change.
>>>
>>> Also, there's no reason to tell the firmware about both rx and tx STBC
>>> capabilities, the only thing it needs to know is what tells the rate
>>> control part to enable/disable STBC.
>>
>> As you can see, in version 2 of this path there was no more discussion
>> about it. I just did it.
>>
>>> In addition to that, using the WLAN_RC_* flags is wrong, you need to use
>>> the ATH_RC_* flags, as this is what ath_rate_newassoc_11n checks for in
>>> the firmware.
>>
>> Renamed.
>>
>>> The only STBC related flag that actually gets used (when
>>> passed from the driver) is ATH_RC_RX_STBC_FLAG.
>>
>> Well, may be it is not used at the end. But it is present on some places
>> in the firmware.
>> For example here:
>> void
>> rcSibUpdate_11n(struct ath_softc_tgt *sc, struct ath_node_target *pSib,
>> A_UINT32 capflag, A_BOOL keepState, struct
>> ieee80211_rate *pRateSet)
>> {
>> rcSibUpdate_ht(sc,
>> pSib,
>> ((capflag & ATH_RC_DS_FLAG) ? WLAN_RC_DS_FLAG
>> : 0) |
>> ((capflag & ATH_RC_HT40_SGI_FLAG) ?
>> WLAN_RC_HT40_SGI_FLAG : 0) |
>> ((capflag & ATH_RC_HT_FLAG) ? WLAN_RC_HT_FLAG
>> : 0) |
>> ((capflag & ATH_RC_CW40_FLAG) ? WLAN_RC_40_FLAG
>> : 0) |
>> ((capflag & ATH_RC_TX_STBC_FLAG) ?
>> WLAN_RC_STBC_FLAG : 0),
>> keepState,
>> pRateSet);
>>
>>
>>
>> So, should i remove ATH_RC_TX_STBC_FLAG from my patch?
> I extensively reviewed this part, and it's really crazy. Here's what
> happens:
>
> ath_rate_newassoc_11n takes ATH_RC_* flags, converts them to WLAN_RC_*.
> rcSibUpdate_11n interprets the WLAN_RC_* flags as ATH_RC_* and converts
> them to WLAN_RC_* again. For most flags this is pretty much a no-op
> because the definitions are identical.
> For STBC the result 'accidentally' still contains WLAN_RC_STBC_FLAG, but
> only because ath_rate_newassoc_11n converts ATH_RC_RX_STBC_FLAG to
> WLAN_RC_STBC_FLAG and WLAN_RC_STBC_FLAG overlaps with ATH_RC_TX_STBC_FLAG.
> In the end it doesn't matter anymore, because nothing in the code takes
> the STBC info from the capflags.
>
> STBC is used if ATH_NODE_ATHEROS(an)->stbc is non-zero, and this gets
> set by ath_rate_newassoc_11n before all of those incredibly moronic
> conversions happen.
Ok, thx.
I'll remove it from my patch. And will remove it from firmware.
Even if you wont to remove bigger part of firmware, i thing it wont
happen this year?
--
Regards,
Oleksij
On 2013-05-02 7:32 PM, Oleksij Rempel wrote:
> Am 02.05.2013 18:55, schrieb Adrian Chadd:
>> On 2 May 2013 01:11, Oleksij Rempel <[email protected]> wrote:
>>
>>> +#define WLAN_RC_TX_STBC_FLAG 0x20 /* TX STBC */
>>> +#define WLAN_RC_RX_STBC_FLAG 0xC0 /* RX STBC ,2 bits */
>>
>> I thought we covered this; why are you marking two bits here?
>
> becouse firmware checks for two bits (and then use it as bool ;)), so i
> pass what firmware can handle.
>
>> Atheros 11n hardware only supports 1-stream STBC RX.
>
> Did you got my email with lots of assumptions and questions?
> What do you mean by 1-stream STBC RX? After i did some home work on STBC
> i see that it encoded from at least two spatial streams.
> Is
> 1-stream STBC RX = 2 spatial streams with mirrored data?
> and
> 2-stream STBC RX = 4 spatial streams with mirrored data?
>
> or
>
> 1-stream STBC RX = compatibility mode for one stream hardware(so only of
> two streams received)?
When you're talking about 'streams', please specify where you're talking
about Spatial Streams (Nss, defined by the MCS), or Space-Time Streams
(Nsts). STBC is useful whenever the number of possible Space-Time
streams exceeds the number of Spatial streams, i.e. if the number of tx
chains is bigger than the number of spatial streams.
There's an asymmetry between Rx and Tx here. If a receiver has 1 chain
and the transmitter has 2 chains, tx can use 2 Space-Time streams to
encode 1 Spatial stream to improve the reliability of the signal.
The HT STBC capability field indicates the maximum number of Spatial
Streams, not Space-Time streams. Atheros hardware only supports STBC
with Nss = 1, so announcing 2-stream STBC is definitely wrong.
> That would make sense for 1x1:1 hardware, but if you say all atheros N
> hardware support only 1-stream STBC RX, will mean that STBC is useless
> on this hardware.
Only STBC With Nss=1, Nsts=2 is supported, but this does not make it
useless at all. It helps, even if the receiver only has one antenna.
- Felix