* [PATCH 1/2] ath10k: make firmware text debug messages more verbose.
@ 2014-09-22 20:54 greearb
2014-09-22 20:54 ` [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation greearb
2014-09-23 13:13 ` [PATCH 1/2] ath10k: make firmware text debug messages more verbose Kalle Valo
0 siblings, 2 replies; 6+ messages in thread
From: greearb @ 2014-09-22 20:54 UTC (permalink / raw)
To: linux-wireless; +Cc: ath10k, Ben Greear
From: Ben Greear <greearb@candelatech.com>
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 <greearb@candelatech.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation.
2014-09-22 20:54 [PATCH 1/2] ath10k: make firmware text debug messages more verbose greearb
@ 2014-09-22 20:54 ` greearb
2014-09-23 8:53 ` Michal Kazior
2014-09-23 13:13 ` [PATCH 1/2] ath10k: make firmware text debug messages more verbose Kalle Valo
1 sibling, 1 reply; 6+ messages in thread
From: greearb @ 2014-09-22 20:54 UTC (permalink / raw)
To: linux-wireless; +Cc: ath10k, Ben Greear
From: Ben Greear <greearb@candelatech.com>
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 <greearb@candelatech.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation.
2014-09-22 20:54 ` [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation greearb
@ 2014-09-23 8:53 ` Michal Kazior
2014-09-23 16:57 ` Ben Greear
0 siblings, 1 reply; 6+ messages in thread
From: Michal Kazior @ 2014-09-23 8:53 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless, ath10k@lists.infradead.org
On 22 September 2014 22:54, <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> 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 <greearb@candelatech.com>
> ---
> 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ł
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation.
2014-09-23 8:53 ` Michal Kazior
@ 2014-09-23 16:57 ` Ben Greear
0 siblings, 0 replies; 6+ messages in thread
From: Ben Greear @ 2014-09-23 16:57 UTC (permalink / raw)
To: Michal Kazior; +Cc: linux-wireless, ath10k@lists.infradead.org
On 09/23/2014 01:53 AM, Michal Kazior wrote:
> On 22 September 2014 22:54, <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> 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 <greearb@candelatech.com>
>> ---
>> 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 <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ath10k: make firmware text debug messages more verbose.
2014-09-22 20:54 [PATCH 1/2] ath10k: make firmware text debug messages more verbose greearb
2014-09-22 20:54 ` [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation greearb
@ 2014-09-23 13:13 ` Kalle Valo
2014-09-23 15:00 ` Ben Greear
1 sibling, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2014-09-23 13:13 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless, ath10k
greearb@candelatech.com writes:
> From: Ben Greear <greearb@candelatech.com>
>
> 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 <greearb@candelatech.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ath10k: make firmware text debug messages more verbose.
2014-09-23 13:13 ` [PATCH 1/2] ath10k: make firmware text debug messages more verbose Kalle Valo
@ 2014-09-23 15:00 ` Ben Greear
0 siblings, 0 replies; 6+ messages in thread
From: Ben Greear @ 2014-09-23 15:00 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, ath10k
On 09/23/2014 06:13 AM, Kalle Valo wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> 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 <greearb@candelatech.com>
>> ---
>> 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 <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-23 16:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-22 20:54 [PATCH 1/2] ath10k: make firmware text debug messages more verbose greearb
2014-09-22 20:54 ` [PATCH 2/2] ath10k: apply chainmask settings to vdev on creation greearb
2014-09-23 8:53 ` Michal Kazior
2014-09-23 16:57 ` Ben Greear
2014-09-23 13:13 ` [PATCH 1/2] ath10k: make firmware text debug messages more verbose Kalle Valo
2014-09-23 15:00 ` Ben Greear
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).