| Message ID | 1375949298-7159-4-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Michal Kazior <michal.kazior@tieto.com> writes: > New firmware comes with new HTT protocol version. > In 3.0 the separate mgmt tx command has been > removed. All traffic is to be pushed through data > tx (tx_frm) command with a twist - FW seems to not > be able (yet?) to access tx fragment table so for > manamgement frames frame pointer is passed > directly. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> [...] > static int ath10k_htt_verify_version(struct ath10k_htt *htt) > { > - ath10k_dbg(ATH10K_DBG_HTT, > - "htt target version %d.%d; host version %d.%d\n", > - htt->target_version_major, > - htt->target_version_minor, > - HTT_CURRENT_VERSION_MAJOR, > - HTT_CURRENT_VERSION_MINOR); > - > - if (htt->target_version_major != HTT_CURRENT_VERSION_MAJOR) { > + ath10k_dbg(ATH10K_DBG_HTT, "htt target version %d.%d\n", > + htt->target_version_major, htt->target_version_minor); This debug print is good to have, but with the new htt version it would be good to print it always using the info level. For example, can we add it to the same line with "firmware %s booted" string? (Please take into account that the info messages still need to be compact, by default they should not be longer than five lines or so.) > + if (htt->target_version_major != 2 && > + htt->target_version_major != 3) { > ath10k_err("htt major versions are incompatible!\n"); > return -ENOTSUPP; > } Print the htt version in the error message as well? > @@ -401,10 +401,15 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) > goto err; > } > > - txfrag = dev_alloc_skb(frag_len); > - if (!txfrag) { > - res = -ENOMEM; > - goto err; > + /* Since HTT 3.0 there is no separate mgmt tx command. However in case > + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx > + * fragment list host driver specifies directly frame pointer. */ > + if (!(htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control))) { I think using "< 3" is more readable: if (htt->target_version_major < 3 && !ieee80211_is_mgmt(hdr->frame_control)) { ... } And is the single line too long? Didn't count it, though. > @@ -427,23 +432,30 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) > if (res) > goto err; > > - /* tx fragment list must be terminated with zero-entry */ > - skb_put(txfrag, frag_len); > - tx_frags = (struct htt_data_tx_desc_frag *)txfrag->data; > - tx_frags[0].paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr); > - tx_frags[0].len = __cpu_to_le32(msdu->len); > - tx_frags[1].paddr = __cpu_to_le32(0); > - tx_frags[1].len = __cpu_to_le32(0); > - > - res = ath10k_skb_map(dev, txfrag); > - if (res) > - goto err; > + /* Since HTT 3.0 there is no separate mgmt tx command. However in case > + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx > + * fragment list host driver specifies directly frame pointer. */ > + if (!(htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control))) { Ditto.
On 8 August 2013 11:05, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> New firmware comes with new HTT protocol version. >> In 3.0 the separate mgmt tx command has been >> removed. All traffic is to be pushed through data >> tx (tx_frm) command with a twist - FW seems to not >> be able (yet?) to access tx fragment table so for >> manamgement frames frame pointer is passed >> directly. >> >> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > > [...] > >> static int ath10k_htt_verify_version(struct ath10k_htt *htt) >> { >> - ath10k_dbg(ATH10K_DBG_HTT, >> - "htt target version %d.%d; host version %d.%d\n", >> - htt->target_version_major, >> - htt->target_version_minor, >> - HTT_CURRENT_VERSION_MAJOR, >> - HTT_CURRENT_VERSION_MINOR); >> - >> - if (htt->target_version_major != HTT_CURRENT_VERSION_MAJOR) { >> + ath10k_dbg(ATH10K_DBG_HTT, "htt target version %d.%d\n", >> + htt->target_version_major, htt->target_version_minor); > > This debug print is good to have, but with the new htt version it would > be good to print it always using the info level. For example, can we add > it to the same line with "firmware %s booted" string? HTT target version is not known when firmware boots up. It's not known until everything other (HTC, WMI) is set up. We then send a version request command and we get a response. We need to print it in a separate line. > (Please take into account that the info messages still need to be > compact, by default they should not be longer than five lines or so.) > >> + if (htt->target_version_major != 2 && >> + htt->target_version_major != 3) { >> ath10k_err("htt major versions are incompatible!\n"); >> return -ENOTSUPP; >> } > > Print the htt version in the error message as well? Target version is printed in ath10k_dbg() now. If we change that to ath10k_info() I don't see any reason to print the version again on error. We may want to print the list of supported HTT major version numbers though? >> @@ -401,10 +401,15 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) >> goto err; >> } >> >> - txfrag = dev_alloc_skb(frag_len); >> - if (!txfrag) { >> - res = -ENOMEM; >> - goto err; >> + /* Since HTT 3.0 there is no separate mgmt tx command. However in case >> + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx >> + * fragment list host driver specifies directly frame pointer. */ >> + if (!(htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control))) { > > I think using "< 3" is more readable: > > if (htt->target_version_major < 3 && > !ieee80211_is_mgmt(hdr->frame_control)) { > ... > } I don't have a problem with changing that. > And is the single line too long? Didn't count it, though. Ah, I didn't check that. Sorry. Good catch. Pozdrawiam / Best regards, Micha? Kazior. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michal Kazior <michal.kazior@tieto.com> writes: > On 8 August 2013 11:05, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> This debug print is good to have, but with the new htt version it would >> be good to print it always using the info level. For example, can we add >> it to the same line with "firmware %s booted" string? > > HTT target version is not known when firmware boots up. It's not known > until everything other (HTC, WMI) is set up. We then send a version > request command and we get a response. Oh, missed that. > We need to print it in a separate line. Or could we print the "firmware booted" message later? >> (Please take into account that the info messages still need to be >> compact, by default they should not be longer than five lines or so.) >> >>> + if (htt->target_version_major != 2 && >>> + htt->target_version_major != 3) { >>> ath10k_err("htt major versions are incompatible!\n"); >>> return -ENOTSUPP; >>> } >> >> Print the htt version in the error message as well? > > Target version is printed in ath10k_dbg() now. If we change that to > ath10k_info() I don't see any reason to print the version again on > error. Frequently users just copy the error message, that's why it's better to have the firmware's htt version in the error message as well. > We may want to print the list of supported HTT major version numbers > though? That's good to have as well, at least in the debug messages.
On 8 August 2013 11:22, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> On 8 August 2013 11:05, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > >>> This debug print is good to have, but with the new htt version it would >>> be good to print it always using the info level. For example, can we add >>> it to the same line with "firmware %s booted" string? >> >> HTT target version is not known when firmware boots up. It's not known >> until everything other (HTC, WMI) is set up. We then send a version >> request command and we get a response. > > Oh, missed that. > >> We need to print it in a separate line. > > Or could we print the "firmware booted" message later? I'm worried it may be error-prone in case of firmware loading failure in-between (i.e. firmware is booted, but WMI init fails). We'd need to print the firmware version in the error path then. > >>> (Please take into account that the info messages still need to be >>> compact, by default they should not be longer than five lines or so.) >>> >>>> + if (htt->target_version_major != 2 && >>>> + htt->target_version_major != 3) { >>>> ath10k_err("htt major versions are incompatible!\n"); >>>> return -ENOTSUPP; >>>> } >>> >>> Print the htt version in the error message as well? >> >> Target version is printed in ath10k_dbg() now. If we change that to >> ath10k_info() I don't see any reason to print the version again on >> error. > > Frequently users just copy the error message, that's why it's better to > have the firmware's htt version in the error message as well. Good point. Pozdrawiam / Best regards, Micha? Kazior. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michal Kazior <michal.kazior@tieto.com> writes: > On 8 August 2013 11:22, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Michal Kazior <michal.kazior@tieto.com> writes: >> >>> On 8 August 2013 11:05, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> >>>> This debug print is good to have, but with the new htt version it would >>>> be good to print it always using the info level. For example, can we add >>>> it to the same line with "firmware %s booted" string? >>> >>> HTT target version is not known when firmware boots up. It's not known >>> until everything other (HTC, WMI) is set up. We then send a version >>> request command and we get a response. >> >> Oh, missed that. >> >>> We need to print it in a separate line. >> >> Or could we print the "firmware booted" message later? > > I'm worried it may be error-prone in case of firmware loading failure > in-between (i.e. firmware is booted, but WMI init fails). We'd need to > print the firmware version in the error path then. True, let's just print in a separate line. We can worry about compacting it later.
diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c index 39342c5..44facc1 100644 --- a/drivers/net/wireless/ath/ath10k/htt.c +++ b/drivers/net/wireless/ath/ath10k/htt.c @@ -104,21 +104,15 @@ err_htc_attach: static int ath10k_htt_verify_version(struct ath10k_htt *htt) { - ath10k_dbg(ATH10K_DBG_HTT, - "htt target version %d.%d; host version %d.%d\n", - htt->target_version_major, - htt->target_version_minor, - HTT_CURRENT_VERSION_MAJOR, - HTT_CURRENT_VERSION_MINOR); - - if (htt->target_version_major != HTT_CURRENT_VERSION_MAJOR) { + ath10k_dbg(ATH10K_DBG_HTT, "htt target version %d.%d\n", + htt->target_version_major, htt->target_version_minor); + + if (htt->target_version_major != 2 && + htt->target_version_major != 3) { ath10k_err("htt major versions are incompatible!\n"); return -ENOTSUPP; } - if (htt->target_version_minor != HTT_CURRENT_VERSION_MINOR) - ath10k_warn("htt minor version differ but still compatible\n"); - return 0; } diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h index de45d02..2488623 100644 --- a/drivers/net/wireless/ath/ath10k/htt.h +++ b/drivers/net/wireless/ath/ath10k/htt.h @@ -23,9 +23,6 @@ #include "htc.h" #include "rx_desc.h" -#define HTT_CURRENT_VERSION_MAJOR 2 -#define HTT_CURRENT_VERSION_MINOR 1 - enum htt_dbg_stats_type { HTT_DBG_STATS_WAL_PDEV_TXRX = 1 << 0, HTT_DBG_STATS_RX_REORDER = 1 << 1, @@ -45,6 +42,9 @@ enum htt_h2t_msg_type { /* host-to-target */ HTT_H2T_MSG_TYPE_SYNC = 4, HTT_H2T_MSG_TYPE_AGGR_CFG = 5, HTT_H2T_MSG_TYPE_FRAG_DESC_BANK_CFG = 6, + + /* This command is used for sending management frames in HTT < 3.0. + * HTT >= 3.0 uses TX_FRM for everything. */ HTT_H2T_MSG_TYPE_MGMT_TX = 7, HTT_H2T_NUM_MSGS /* keep this last */ diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c index 656c254..dce128a 100644 --- a/drivers/net/wireless/ath/ath10k/htt_tx.c +++ b/drivers/net/wireless/ath/ath10k/htt_tx.c @@ -401,10 +401,15 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) goto err; } - txfrag = dev_alloc_skb(frag_len); - if (!txfrag) { - res = -ENOMEM; - goto err; + /* Since HTT 3.0 there is no separate mgmt tx command. However in case + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx + * fragment list host driver specifies directly frame pointer. */ + if (!(htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control))) { + txfrag = dev_alloc_skb(frag_len); + if (!txfrag) { + res = -ENOMEM; + goto err; + } } if (!IS_ALIGNED((unsigned long)txdesc->data, 4)) { @@ -427,23 +432,30 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) if (res) goto err; - /* tx fragment list must be terminated with zero-entry */ - skb_put(txfrag, frag_len); - tx_frags = (struct htt_data_tx_desc_frag *)txfrag->data; - tx_frags[0].paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr); - tx_frags[0].len = __cpu_to_le32(msdu->len); - tx_frags[1].paddr = __cpu_to_le32(0); - tx_frags[1].len = __cpu_to_le32(0); - - res = ath10k_skb_map(dev, txfrag); - if (res) - goto err; + /* Since HTT 3.0 there is no separate mgmt tx command. However in case + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx + * fragment list host driver specifies directly frame pointer. */ + if (!(htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control))) { + /* tx fragment list must be terminated with zero-entry */ + skb_put(txfrag, frag_len); + tx_frags = (struct htt_data_tx_desc_frag *)txfrag->data; + tx_frags[0].paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr); + tx_frags[0].len = __cpu_to_le32(msdu->len); + tx_frags[1].paddr = __cpu_to_le32(0); + tx_frags[1].len = __cpu_to_le32(0); + + res = ath10k_skb_map(dev, txfrag); + if (res) + goto err; + + ath10k_dbg(ATH10K_DBG_HTT, "txfrag 0x%llx\n", + (unsigned long long) ATH10K_SKB_CB(txfrag)->paddr); + ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "txfrag: ", + txfrag->data, frag_len); + } - ath10k_dbg(ATH10K_DBG_HTT, "txfrag 0x%llx msdu 0x%llx\n", - (unsigned long long) ATH10K_SKB_CB(txfrag)->paddr, + ath10k_dbg(ATH10K_DBG_HTT, "msdu 0x%llx\n", (unsigned long long) ATH10K_SKB_CB(msdu)->paddr); - ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "txfrag: ", - txfrag->data, frag_len); ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "msdu: ", msdu->data, msdu->len); @@ -459,8 +471,16 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) if (!ieee80211_has_protected(hdr->frame_control)) flags0 |= HTT_DATA_TX_DESC_FLAGS0_NO_ENCRYPT; flags0 |= HTT_DATA_TX_DESC_FLAGS0_MAC_HDR_PRESENT; - flags0 |= SM(ATH10K_HW_TXRX_NATIVE_WIFI, - HTT_DATA_TX_DESC_FLAGS0_PKT_TYPE); + + /* Since HTT 3.0 there is no separate mgmt tx command. However in case + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx + * fragment list host driver specifies directly frame pointer. */ + if (htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control)) + flags0 |= SM(ATH10K_HW_TXRX_MGMT, + HTT_DATA_TX_DESC_FLAGS0_PKT_TYPE); + else + flags0 |= SM(ATH10K_HW_TXRX_NATIVE_WIFI, + HTT_DATA_TX_DESC_FLAGS0_PKT_TYPE); flags1 = 0; flags1 |= SM((u16)vdev_id, HTT_DATA_TX_DESC_FLAGS1_VDEV_ID); @@ -468,7 +488,13 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) flags1 |= HTT_DATA_TX_DESC_FLAGS1_CKSUM_L3_OFFLOAD; flags1 |= HTT_DATA_TX_DESC_FLAGS1_CKSUM_L4_OFFLOAD; - frags_paddr = ATH10K_SKB_CB(txfrag)->paddr; + /* Since HTT 3.0 there is no separate mgmt tx command. However in case + * of mgmt tx using TX_FRM there is not tx fragment list. Instead of tx + * fragment list host driver specifies directly frame pointer. */ + if (htt->target_version_major >= 3 && ieee80211_is_mgmt(hdr->frame_control)) + frags_paddr = ATH10K_SKB_CB(msdu)->paddr; + else + frags_paddr = ATH10K_SKB_CB(txfrag)->paddr; cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_TX_FRM; cmd->data_tx.flags0 = flags0; diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h index 44ed5af..9c5d6fc 100644 --- a/drivers/net/wireless/ath/ath10k/hw.h +++ b/drivers/net/wireless/ath/ath10k/hw.h @@ -53,6 +53,9 @@ enum ath10k_hw_txrx_mode { ATH10K_HW_TXRX_RAW = 0, ATH10K_HW_TXRX_NATIVE_WIFI = 1, ATH10K_HW_TXRX_ETHERNET = 2, + + /* Valid for HTT >= 3.0. Used for management frames in TX_FRM. */ + ATH10K_HW_TXRX_MGMT = 3, }; enum ath10k_mcast2ucast_mode { diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index fb1f24f..aca423c 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -1480,6 +1480,12 @@ static void ath10k_tx_htt(struct ath10k *ar, struct sk_buff *skb) struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data; int ret; + if (ar->htt.target_version_major >= 3) { + /* Since HTT 3.0 there is no separate mgmt tx command */ + ret = ath10k_htt_tx(&ar->htt, skb); + goto exit; + } + if (ieee80211_is_mgmt(hdr->frame_control)) ret = ath10k_htt_mgmt_tx(&ar->htt, skb); else if (ieee80211_is_nullfunc(hdr->frame_control)) @@ -1491,6 +1497,7 @@ static void ath10k_tx_htt(struct ath10k *ar, struct sk_buff *skb) else ret = ath10k_htt_tx(&ar->htt, skb); +exit: if (ret) { ath10k_warn("tx failed (%d). dropping packet.\n", ret); ieee80211_free_txskb(ar->hw, skb); @@ -1726,7 +1733,9 @@ static void ath10k_tx(struct ieee80211_hw *hw, /* we must calculate tid before we apply qos workaround * as we'd lose the qos control field */ - if (ieee80211_is_data_qos(hdr->frame_control) && + if (ieee80211_is_mgmt(hdr->frame_control)) + tid = HTT_DATA_TX_EXT_TID_MGMT; + else if (ieee80211_is_data_qos(hdr->frame_control) && is_unicast_ether_addr(ieee80211_get_DA(hdr))) { u8 *qc = ieee80211_get_qos_ctl(hdr); tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
New firmware comes with new HTT protocol version. In 3.0 the separate mgmt tx command has been removed. All traffic is to be pushed through data tx (tx_frm) command with a twist - FW seems to not be able (yet?) to access tx fragment table so for manamgement frames frame pointer is passed directly. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- drivers/net/wireless/ath/ath10k/htt.c | 16 +++---- drivers/net/wireless/ath/ath10k/htt.h | 6 +-- drivers/net/wireless/ath/ath10k/htt_tx.c | 70 ++++++++++++++++++++---------- drivers/net/wireless/ath/ath10k/hw.h | 3 ++ drivers/net/wireless/ath/ath10k/mac.c | 11 ++++- 5 files changed, 69 insertions(+), 37 deletions(-)