diff mbox

QCA9984 VHT160 support

Message ID 572a14ee-d1b5-7b85-d292-51f583f34f17@dd-wrt.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Sebastian Gottschall May 17, 2016, 11:43 a.m. UTC 
Attached you will find a testing patch for VHT160 support. i tested it 
today on a QCA9984 device and it seems to work.
feel free to make any corrections

Comments

Michal Kazior May 17, 2016, noon UTC | #1 
On 17 May 2016 at 13:43, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote:
> Attached you will find a testing patch for VHT160 support. i tested it today
> on a QCA9984 device and it seems to work.
> feel free to make any corrections

Hi,

Send patches inline, please. Preferably via git send-email. It's a lot
easier to review them this way.

From a quick glance vht capabilities setup looks weird:

> if (ar->vht_cap_info &  IEEE80211_VHT_CAP_SHORT_GI_160) {
>   vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ;
>   vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ;
> }

Why? Is this really necessary?

I would expect IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ and
IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ to be already set in
ar->vht_cap_info if the chip supports 160 or 80+80.

You also don't seem to handle 80+80 because you just as phymode to 160
regardless of center frequencies:

> !ath10k_peer_assoc_h_vht_masked(vht_mcs_mask)) {
>    if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>      phymode = MODE_11AC_VHT80;
>    else if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>      phymode = MODE_11AC_VHT160;
>    else if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
>      phymode = MODE_11AC_VHT40;
>    else if (sta->bandwidth == IEEE80211_STA_RX_BW_20)

You should probably look at sta->vht_cap and associated chanctx to
check if it isn't 80+80.


Micha?
Michal Kazior May 17, 2016, 12:18 p.m. UTC | #2 
+ re-adding ath10k list


On 17 May 2016 at 14:05, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote:
>Am 17.05.2016 um 14:00 schrieb Michal Kazior:
>> On 17 May 2016 at 13:43, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote:
>>> Attached you will find a testing patch for VHT160 support. i tested it today
>>> on a QCA9984 device and it seems to work.
>>> feel free to make any corrections
>> Hi,
>>
>> Send patches inline, please. Preferably via git send-email. It's a lot
>> easier to review them this way.
> unfortunaty i dont work with git right now and i wanted to send it to
> the list first for getting comments.
> since the patch is also very small, it should be that complicated
>>
>>  From a quick glance vht capabilities setup looks weird:
>>
>>> if (ar->vht_cap_info &  IEEE80211_VHT_CAP_SHORT_GI_160) {
>>>    vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ;
>>>    vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ;
>>> }
>> Why? Is this really necessary?
> the firmware does not report correct flags. just SHORT_GI_160.
> i reviewed the firmware sourcecodes and found out that this is maybe a
> misstake of the QCA developers.
> so we need to add the 160MHZ support flags locally using the
> SHORT_GI_160 which is always set on QCA9984.
> otherwise mac80211 will not be capable of running vht160.

I'm not sure if SGI160 implies 80+80.


>> I would expect IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ and
>> IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ to be already set in
>> ar->vht_cap_info if the chip supports 160 or 80+80.
>me too. but it isnt :-)

Sigh. A comment explaining that would be really nice.


>> You also don't seem to handle 80+80 because you just as phymode to 160
>> regardless of center frequencies:
>>
>>> !ath10k_peer_assoc_h_vht_masked(vht_mcs_mask)) {
>>>     if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>       phymode = MODE_11AC_VHT80;
>>>     else if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>       phymode = MODE_11AC_VHT160;
>>>     else if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
>>>       phymode = MODE_11AC_VHT40;
>>>     else if (sta->bandwidth == IEEE80211_STA_RX_BW_20)
> there is no 80+80 STA_RX_BW flag. i dont know how to handle this. and
> yes, just 160 mhz is tested right now. right now it looks like mac80211
> is not 80+80 ready.

Hmm.. I wouldn't advertise 80+80 support then. All the more since
there's no way of telling whether hw/fw actually supports it in the
first place.


Michal
Sebastian Gottschall May 17, 2016, 12:22 p.m. UTC | #3 
Am 17.05.2016 um 14:18 schrieb Michal Kazior:
> + re-adding ath10k list
>
>
> On 17 May 2016 at 14:05, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote:
>> Am 17.05.2016 um 14:00 schrieb Michal Kazior:
>>> On 17 May 2016 at 13:43, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote:
>>>> Attached you will find a testing patch for VHT160 support. i tested it today
>>>> on a QCA9984 device and it seems to work.
>>>> feel free to make any corrections
>>> Hi,
>>>
>>> Send patches inline, please. Preferably via git send-email. It's a lot
>>> easier to review them this way.
>> unfortunaty i dont work with git right now and i wanted to send it to
>> the list first for getting comments.
>> since the patch is also very small, it should be that complicated
>>>   From a quick glance vht capabilities setup looks weird:
>>>
>>>> if (ar->vht_cap_info &  IEEE80211_VHT_CAP_SHORT_GI_160) {
>>>>     vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ;
>>>>     vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ;
>>>> }
>>> Why? Is this really necessary?
>> the firmware does not report correct flags. just SHORT_GI_160.
>> i reviewed the firmware sourcecodes and found out that this is maybe a
>> misstake of the QCA developers.
>> so we need to add the 160MHZ support flags locally using the
>> SHORT_GI_160 which is always set on QCA9984.
>> otherwise mac80211 will not be capable of running vht160.
> I'm not sure if SGI160 implies 80+80.
according to the firmware sourcecode of 9984 it seems so. thats the only 
information source i have
>
>
>>> I would expect IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ and
>>> IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ to be already set in
>>> ar->vht_cap_info if the chip supports 160 or 80+80.
>> me too. but it isnt :-)
> Sigh. A comment explaining that would be really nice.
there is no comment available. the only information source is the 
firmware sourcecode for the 9984 which i reviewed.
let me give you a small snippet
#if defined(CONFIG_160MHZ_SUPPORT)
#define VHT_CAP_WITH_STBC_1SS_VHT160 ((VHT_CAP_WITH_STBC_1SS) | 
(WMI_VHT_CAP_SGI_160MHZ))
#define VHT_CAP_WITH_STBC_2SS_VHT160 ((VHT_CAP_WITH_STBC_2SS) | 
(WMI_VHT_CAP_SGI_160MHZ))
#define VHT_CAP_WITH_STBC_3SS_VHT160 ((VHT_CAP_WITH_STBC_3SS) | 
(WMI_VHT_CAP_SGI_160MHZ))
#endif
#if defined(CONFIG_160MHZ_SUPPORT)
     dev_info->vht_cap_info  = VHT_CAP_WITH_STBC_1SS_VHT160;
#else
     dev_info->vht_cap_info  = VHT_CAP_WITH_STBC_1SS_VHT80;
#endif


so the firmware simply does not set the correct flags. thats all.
>
>
>>> You also don't seem to handle 80+80 because you just as phymode to 160
>>> regardless of center frequencies:
>>>
>>>> !ath10k_peer_assoc_h_vht_masked(vht_mcs_mask)) {
>>>>      if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>        phymode = MODE_11AC_VHT80;
>>>>      else if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>        phymode = MODE_11AC_VHT160;
>>>>      else if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
>>>>        phymode = MODE_11AC_VHT40;
>>>>      else if (sta->bandwidth == IEEE80211_STA_RX_BW_20)
>> there is no 80+80 STA_RX_BW flag. i dont know how to handle this. and
>> yes, just 160 mhz is tested right now. right now it looks like mac80211
>> is not 80+80 ready.
> Hmm.. I wouldn't advertise 80+80 support then. All the more since
> there's no way of telling whether hw/fw actually supports it in the
> first place.
we can leave it out at the end, but the firmware seem to support it. so 
far i just tested 160 in ap mode yet, but found other
things in ath10k which need to be fixed. i'm on it right now
>
>
> Michal
>
Kalle Valo May 17, 2016, 1:20 p.m. UTC | #4 
Michal Kazior <michal.kazior@tieto.com> writes:

> On 17 May 2016 at 13:43, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote:
>> Attached you will find a testing patch for VHT160 support. i tested it today
>> on a QCA9984 device and it seems to work.
>> feel free to make any corrections
>
> Send patches inline, please. Preferably via git send-email. It's a lot
> easier to review them this way.

Also remember to read Developer's Certificate of Origin from
Documentation/SubmittingPatches and add Signed-off-by. Otherwise we
cannot use the patch.
Kalle Valo Sept. 14, 2016, 10:37 a.m. UTC | #5 
Sebastian Gottschall <s.gottschall@dd-wrt.com> writes:

> Attached you will find a testing patch for VHT160 support. i tested it
> today on a QCA9984 device and it seems to work.
> feel free to make any corrections

Signed-off-by is missing so I can't take this. Can you resend, please?

Also remember to CC linux-wireless.
Sebastian Gottschall Sept. 14, 2016, 2:38 p.m. UTC | #6 
Am 14.09.2016 um 12:37 schrieb Valo, Kalle:
> Sebastian Gottschall <s.gottschall@dd-wrt.com> writes:
>
>> Attached you will find a testing patch for VHT160 support. i tested it
>> today on a QCA9984 device and it seems to work.
>> feel free to make any corrections
> Signed-off-by is missing so I can't take this. Can you resend, please
i can send you a updated version. it tested with vht160 so far. however. 
on codeaurore there is a newer firmware for 9984 which provides the 
correct vht flags straight from the firmware
but its crashing in vht160 mode. can you ask the firmware team to 
provide a bugfixed version for the official ath10k? with 80p80 and 
vht160 support?
80p80 works somewhat bug has wrong vht flags per station, vht160 works 
with my quirks so far which are all related to the buggy firmware
>
> Also remember to CC linux-wireless.
>
Kalle Valo Sept. 14, 2016, 2:44 p.m. UTC | #7 
Sebastian Gottschall <s.gottschall@dd-wrt.com> writes:

> Am 14.09.2016 um 12:37 schrieb Valo, Kalle:
>> Sebastian Gottschall <s.gottschall@dd-wrt.com> writes:
>>
>>> Attached you will find a testing patch for VHT160 support. i tested it
>>> today on a QCA9984 device and it seems to work.
>>> feel free to make any corrections
>> Signed-off-by is missing so I can't take this. Can you resend, please
>
> i can send you a updated version. it tested with vht160 so far.

Great, thanks.

> however. on codeaurore there is a newer firmware for 9984 which
> provides the correct vht flags straight from the firmware but its
> crashing in vht160 mode. can you ask the firmware team to provide a
> bugfixed version for the official ath10k? with 80p80 and vht160
> support? 80p80 works somewhat bug has wrong vht flags per station,
> vht160 works with my quirks so far which are all related to the buggy
> firmware

I can ask but I need more info. What's the exact location of the newer
firmware image? Is there any detailed description about the wrong vht
flags bug?
diff mbox

Patch

Index: drivers/net/wireless/ath/ath9k/gpio.c
===================================================================
Index: drivers/net/wireless/ath/ath10k/mac.c
===================================================================
--- drivers/net/wireless/ath/ath10k/mac.c	(revision 2910)
+++ drivers/net/wireless/ath/ath10k/mac.c	(working copy)
@@ -539,10 +539,14 @@ 
 		case NL80211_CHAN_WIDTH_80:
 			phymode = MODE_11AC_VHT80;
 			break;
+		case NL80211_CHAN_WIDTH_160:
+			phymode = MODE_11AC_VHT160;
+			break;
+		case NL80211_CHAN_WIDTH_80P80:
+			phymode = MODE_11AC_VHT80_80;
+			break;
 		case NL80211_CHAN_WIDTH_5:
 		case NL80211_CHAN_WIDTH_10:
-		case NL80211_CHAN_WIDTH_80P80:
-		case NL80211_CHAN_WIDTH_160:
 			phymode = MODE_UNKNOWN;
 			break;
 		}
@@ -2499,6 +2503,8 @@ 
 		    !ath10k_peer_assoc_h_vht_masked(vht_mcs_mask)) {
 			if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
 				phymode = MODE_11AC_VHT80;
+			else if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
+				phymode = MODE_11AC_VHT160;
 			else if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
 				phymode = MODE_11AC_VHT40;
 			else if (sta->bandwidth == IEEE80211_STA_RX_BW_20)
@@ -4185,6 +4191,10 @@ 
 
 		vht_cap.cap |= val;
 	}
+	if (ar->vht_cap_info &  IEEE80211_VHT_CAP_SHORT_GI_160) {
+		vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ;
+		vht_cap.cap |= IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ;
+	}
 
 	mcs_map = 0;
 	for (i = 0; i < 8; i++) {
@@ -6798,6 +6808,9 @@ 
 			bw = WMI_PEER_CHWIDTH_80MHZ;
 			break;
 		case IEEE80211_STA_RX_BW_160:
+			bw = WMI_PEER_CHWIDTH_160MHZ;
+			break;
+		default:
 			ath10k_warn(ar, "Invalid bandwidth %d in rc update for %pM\n",
 				    sta->bandwidth, sta->addr);
 			bw = WMI_PEER_CHWIDTH_20MHZ;
Index: drivers/net/wireless/ath/ath10k/htt_rx.c
===================================================================
--- drivers/net/wireless/ath/ath10k/htt_rx.c	(revision 2910)
+++ drivers/net/wireless/ath/ath10k/htt_rx.c	(working copy)
@@ -707,6 +707,10 @@ 
 		/* 80MHZ */
 		case 2:
 			status->vht_flag |= RX_VHT_FLAG_80MHZ;
+			break;
+		case 3:
+			status->vht_flag |= RX_VHT_FLAG_160MHZ;
+			break;
 		}
 
 		status->flag |= RX_FLAG_VHT;
@@ -2193,6 +2197,8 @@ 
 	case MODE_11AC_VHT20:
 	case MODE_11AC_VHT40:
 	case MODE_11AC_VHT80:
+	case MODE_11AC_VHT160:
+	case MODE_11AC_VHT80_80:
 		band = NL80211_BAND_5GHZ;
 		break;
 	case MODE_11G:
Index: drivers/net/wireless/ath/ath10k/wmi.h
===================================================================
--- drivers/net/wireless/ath/ath10k/wmi.h	(revision 2910)
+++ drivers/net/wireless/ath/ath10k/wmi.h	(working copy)
@@ -1723,8 +1723,10 @@ 
 	MODE_11AC_VHT20_2G = 11,
 	MODE_11AC_VHT40_2G = 12,
 	MODE_11AC_VHT80_2G = 13,
-	MODE_UNKNOWN    = 14,
-	MODE_MAX        = 14
+	MODE_11AC_VHT80_80 = 14,
+	MODE_11AC_VHT160 = 15,
+	MODE_UNKNOWN    = 16,
+	MODE_MAX        = 16
 };
 
 static inline const char *ath10k_wmi_phymode_str(enum wmi_phy_mode mode)
@@ -1752,6 +1754,10 @@ 
 		return "11ac-vht40";
 	case MODE_11AC_VHT80:
 		return "11ac-vht80";
+	case MODE_11AC_VHT160:
+		return "11ac-vht160";
+	case MODE_11AC_VHT80_80:
+		return "11ac-vht80+80";
 	case MODE_11AC_VHT20_2G:
 		return "11ac-vht20-2g";
 	case MODE_11AC_VHT40_2G:
@@ -5748,6 +5754,7 @@ 
 	WMI_PEER_CHWIDTH_20MHZ = 0,
 	WMI_PEER_CHWIDTH_40MHZ = 1,
 	WMI_PEER_CHWIDTH_80MHZ = 2,
+	WMI_PEER_CHWIDTH_160MHZ = 3,
 };
 
 enum wmi_peer_param {