From: Ben Greear <[email protected]>
There are not many of these messages producted by the
firmware, but they are generally fairly useful, so print
them at info level.
Signed-off-by: Ben Greear <[email protected]>
---
drivers/net/wireless/ath/ath10k/wmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index cd60940..6be62ac 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -2050,7 +2050,7 @@ static void ath10k_wmi_event_debug_print(struct ath10k *ar,
/* the last byte is always reserved for the null character */
buf[i] = '\0';
- ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi event debug print '%s'\n", buf);
+ ath10k_info(ar, "wmi event debug print '%s'\n", buf);
}
static void ath10k_wmi_event_pdev_qvit(struct ath10k *ar, struct sk_buff *skb)
--
1.7.11.7
From: Ben Greear <[email protected]>
It appears it takes more than just setting the
hardware's chainmask to make things work well. Without
this patch, a vdev would only use 1x1 rates when chainmask
was set to 0x3.
Setting the 'nss' (number of spatial streams) on the vdev
helps the firmware's rate-control algorithm work properly.
Tested on CT firmware, but probably this works (and
is required) on normal firmware.
Signed-off-by: Ben Greear <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 4dc5a40..855c71c 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2767,6 +2767,17 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
return ret;
}
+static u32 get_nss_from_chainmask(u16 chain_mask)
+{
+ if ((chain_mask & 0x15) == 0x15)
+ return 4;
+ else if ((chain_mask & 0x7) == 0x7)
+ return 3;
+ else if ((chain_mask & 0x3) == 0x3)
+ return 2;
+ return 1;
+}
+
/*
* TODO:
* Figure out how to handle WMI_VDEV_SUBTYPE_P2P_DEVICE,
@@ -2868,6 +2879,19 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
goto err_vdev_delete;
}
+ if (ar->cfg_tx_chainmask) {
+ u16 nss = get_nss_from_chainmask(ar->cfg_tx_chainmask);
+ vdev_param = ar->wmi.vdev_param->nss;
+ ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
+ nss);
+ if (ret) {
+ ath10k_warn(ar, "failed to set vdev %i chainmask 0x%x, nss %i: %d\n",
+ arvif->vdev_id, ar->cfg_tx_chainmask, nss,
+ ret);
+ goto err_vdev_delete;
+ }
+ }
+
if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
ret = ath10k_peer_create(ar, arvif->vdev_id, vif->addr);
if (ret) {
--
1.7.11.7
On 09/23/2014 01:53 AM, Michal Kazior wrote:
> On 22 September 2014 22:54, <[email protected]> wrote:
>> From: Ben Greear <[email protected]>
>>
>> It appears it takes more than just setting the
>> hardware's chainmask to make things work well. Without
>> this patch, a vdev would only use 1x1 rates when chainmask
>> was set to 0x3.
>>
>> Setting the 'nss' (number of spatial streams) on the vdev
>> helps the firmware's rate-control algorithm work properly.
>>
>> Tested on CT firmware, but probably this works (and
>> is required) on normal firmware.
>>
>> Signed-off-by: Ben Greear <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/mac.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index 4dc5a40..855c71c 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2767,6 +2767,17 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
>> return ret;
>> }
>>
>> +static u32 get_nss_from_chainmask(u16 chain_mask)
>> +{
>> + if ((chain_mask & 0x15) == 0x15)
>> + return 4;
>> + else if ((chain_mask & 0x7) == 0x7)
>> + return 3;
>> + else if ((chain_mask & 0x3) == 0x3)
>> + return 2;
>> + return 1;
>> +}
>
> So a chainmask of `BIT(0) | BIT(2) = 0x5` is nss=1?
That is how the firmware treats it.
> Why not just use `hweight16()` kernel macro? Or do we want to forbid
> odd chainmasks (in which case additional checks need to be added as
> well)?
Even after looking at the hweight16 code I don't know what it is supposed
to do. I think open-coding is plenty adequate.
Maybe the driver should only allow a chainmask of 1, 3, 7, (and eventually 15 when 4x4 exists),
but maybe also that can just be on the user to get it right. It is possible that there
is some use for having a different chainmask, but it would require someone with good knowledge
of the hardware to answer that I think.
>> + if (ar->cfg_tx_chainmask) {
>> + u16 nss = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>> + vdev_param = ar->wmi.vdev_param->nss;
>> + ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
>> + nss);
>> + if (ret) {
>> + ath10k_warn(ar, "failed to set vdev %i chainmask 0x%x, nss %i: %d\n",
>> + arvif->vdev_id, ar->cfg_tx_chainmask, nss,
>> + ret);
>> + goto err_vdev_delete;
>> + }
>> + }
>> +
>
> If this is required won't setting up peer nss (after association) be
> necessary as well?
>
> Shouldn't the vdev nss param be set in __ath10k_set_antenna() as well?
> What about peer nss (assuming it's necessary to update it) in that
> case?
Did the second patch address all of this? I may have missed more cases,
but at least with the second patch it seems to work for me reliably.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 09/23/2014 06:13 AM, Kalle Valo wrote:
> [email protected] writes:
>
>> From: Ben Greear <[email protected]>
>>
>> There are not many of these messages producted by the
>> firmware, but they are generally fairly useful, so print
>> them at info level.
>>
>> Signed-off-by: Ben Greear <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/wmi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>> index cd60940..6be62ac 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -2050,7 +2050,7 @@ static void ath10k_wmi_event_debug_print(struct ath10k *ar,
>> /* the last byte is always reserved for the null character */
>> buf[i] = '\0';
>>
>> - ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi event debug print '%s'\n", buf);
>> + ath10k_info(ar, "wmi event debug print '%s'\n", buf);
>
> This does not bring any extra value to normal users, it's only useful to
> firmware engineers. That's why it needs to stay as a debug message.
> Maybe we should add a new debug level just for these events, would that
> help?
It will help users trying to tune the maximum resource combinations (vdevs + peers + tx-descriptors + skid-len, etc)
because my firmware prints out remaining RAM/IRAM after booting up.
But, a separate debug flag would be fine.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
[email protected] writes:
> From: Ben Greear <[email protected]>
>
> There are not many of these messages producted by the
> firmware, but they are generally fairly useful, so print
> them at info level.
>
> Signed-off-by: Ben Greear <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/wmi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index cd60940..6be62ac 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -2050,7 +2050,7 @@ static void ath10k_wmi_event_debug_print(struct ath10k *ar,
> /* the last byte is always reserved for the null character */
> buf[i] = '\0';
>
> - ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi event debug print '%s'\n", buf);
> + ath10k_info(ar, "wmi event debug print '%s'\n", buf);
This does not bring any extra value to normal users, it's only useful to
firmware engineers. That's why it needs to stay as a debug message.
Maybe we should add a new debug level just for these events, would that
help?
--
Kalle Valo
On 22 September 2014 22:54, <[email protected]> wrote:
> From: Ben Greear <[email protected]>
>
> It appears it takes more than just setting the
> hardware's chainmask to make things work well. Without
> this patch, a vdev would only use 1x1 rates when chainmask
> was set to 0x3.
>
> Setting the 'nss' (number of spatial streams) on the vdev
> helps the firmware's rate-control algorithm work properly.
>
> Tested on CT firmware, but probably this works (and
> is required) on normal firmware.
>
> Signed-off-by: Ben Greear <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/mac.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 4dc5a40..855c71c 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2767,6 +2767,17 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
> return ret;
> }
>
> +static u32 get_nss_from_chainmask(u16 chain_mask)
> +{
> + if ((chain_mask & 0x15) == 0x15)
> + return 4;
> + else if ((chain_mask & 0x7) == 0x7)
> + return 3;
> + else if ((chain_mask & 0x3) == 0x3)
> + return 2;
> + return 1;
> +}
So a chainmask of `BIT(0) | BIT(2) = 0x5` is nss=1?
Why not just use `hweight16()` kernel macro? Or do we want to forbid
odd chainmasks (in which case additional checks need to be added as
well)?
> +
> /*
> * TODO:
> * Figure out how to handle WMI_VDEV_SUBTYPE_P2P_DEVICE,
> @@ -2868,6 +2879,19 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
> goto err_vdev_delete;
> }
>
> + if (ar->cfg_tx_chainmask) {
> + u16 nss = get_nss_from_chainmask(ar->cfg_tx_chainmask);
> + vdev_param = ar->wmi.vdev_param->nss;
> + ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
> + nss);
> + if (ret) {
> + ath10k_warn(ar, "failed to set vdev %i chainmask 0x%x, nss %i: %d\n",
> + arvif->vdev_id, ar->cfg_tx_chainmask, nss,
> + ret);
> + goto err_vdev_delete;
> + }
> + }
> +
If this is required won't setting up peer nss (after association) be
necessary as well?
Shouldn't the vdev nss param be set in __ath10k_set_antenna() as well?
What about peer nss (assuming it's necessary to update it) in that
case?
Michał