| Message ID | 20141201144507.18248.9856.stgit@potku.adurom.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 1 December 2014 at 15:45, Kalle Valo <kvalo@qca.qualcomm.com> wrote: [...] > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > index 514c219263a5..92b04fe73151 100644 > --- a/drivers/net/wireless/ath/ath10k/core.h > +++ b/drivers/net/wireless/ath/ath10k/core.h > @@ -120,6 +120,7 @@ struct ath10k_mem_chunk { > }; > > struct ath10k_wmi { > + unsigned int op_version; I wonder - can't we have this as `enum ath10k_fw_wmi_op_version op_version;` ? > @@ -378,8 +379,9 @@ enum ath10k_fw_features { > /* Firmware does not support P2P */ > ATH10K_FW_FEATURE_NO_P2P = 3, > > - /* Firmware 10.2 feature bit. The ATH10K_FW_FEATURE_WMI_10X feature bit > - * is required to be set as well. > + /* Firmware 10.2 feature bit. The ATH10K_FW_FEATURE_WMI_10X feature > + * bit is required to be set as well. Deprecated, don't use in new > + * code. Just out of curiosity - any plans how long this is going to be depracated until removed/replaced? > */ > ATH10K_FW_FEATURE_WMI_10_2 = 4, > > diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h > index dfedfd0e0f34..04aaf9af3ca0 100644 > --- a/drivers/net/wireless/ath/ath10k/hw.h > +++ b/drivers/net/wireless/ath/ath10k/hw.h > @@ -58,6 +58,16 @@ enum ath10k_fw_ie_type { > ATH10K_FW_IE_FEATURES = 2, > ATH10K_FW_IE_FW_IMAGE = 3, > ATH10K_FW_IE_OTP_IMAGE = 4, > + > + /* WMI "operations" interface version, 32 bit value. Supported from > + * FW API 4 and above. */ > + ATH10K_FW_IE_WMI_OP_VERSION = 5, Hmm.. shouldn't we bump up the firmware filename from -3 to -4 and try loading 4..3..2..1? Micha?
Michal Kazior <michal.kazior@tieto.com> writes: > On 1 December 2014 at 15:45, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > [...] >> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h >> index 514c219263a5..92b04fe73151 100644 >> --- a/drivers/net/wireless/ath/ath10k/core.h >> +++ b/drivers/net/wireless/ath/ath10k/core.h >> @@ -120,6 +120,7 @@ struct ath10k_mem_chunk { >> }; >> >> struct ath10k_wmi { >> + unsigned int op_version; > > I wonder - can't we have this as `enum ath10k_fw_wmi_op_version op_version;` ? Oh yes, I was even planning to change it but apparently forgot. Will fix in v4. >> @@ -378,8 +379,9 @@ enum ath10k_fw_features { >> /* Firmware does not support P2P */ >> ATH10K_FW_FEATURE_NO_P2P = 3, >> >> - /* Firmware 10.2 feature bit. The ATH10K_FW_FEATURE_WMI_10X feature bit >> - * is required to be set as well. >> + /* Firmware 10.2 feature bit. The ATH10K_FW_FEATURE_WMI_10X feature >> + * bit is required to be set as well. Deprecated, don't use in new >> + * code. > > Just out of curiosity - any plans how long this is going to be > depracated until removed/replaced? So there are two parts: 1) In the driver we should not use these deprated flags with new code, and preferably remove existing uses bit by bit. But for backwards compatibility we will keep the flags in ath10k_core_init_firmware_features() so that we can set correct wmi.op_version when using firmware images which don't have ATH10K_FW_IE_WMI_OP_VERSION. 2) In firmware images we will continue use the deprecated flags with main, 10.1 and 10.2 firmware branches, at least for the time being. This makes it possible to have new firmware releases working on older drivers. >> diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h >> index dfedfd0e0f34..04aaf9af3ca0 100644 >> --- a/drivers/net/wireless/ath/ath10k/hw.h >> +++ b/drivers/net/wireless/ath/ath10k/hw.h >> @@ -58,6 +58,16 @@ enum ath10k_fw_ie_type { >> ATH10K_FW_IE_FEATURES = 2, >> ATH10K_FW_IE_FW_IMAGE = 3, >> ATH10K_FW_IE_OTP_IMAGE = 4, >> + >> + /* WMI "operations" interface version, 32 bit value. Supported from >> + * FW API 4 and above. */ >> + ATH10K_FW_IE_WMI_OP_VERSION = 5, > > Hmm.. shouldn't we bump up the firmware filename from -3 to -4 and try > loading 4..3..2..1? I was thinking that we add FW API 4 only once we have all necessary changes for the new hw. For example, in case we need HTT changes as well.
On 1 December 2014 at 15:45, Kalle Valo <kvalo@qca.qualcomm.com> wrote: [...] > @@ -562,6 +562,17 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name) > ar->otp_len = ie_len; > > break; > + case ATH10K_FW_IE_WMI_OP_VERSION: > + if (ie_len != sizeof(u32)) > + break; > + > + version = (__le32 *)data; > + > + ar->wmi.op_version = le32_to_cpup(version); > + > + ath10k_dbg(ar, ATH10K_DBG_BOOT, "found fw ie wmi op version %d\n", > + ar->wmi.op_version); Hmm.. wouldn't it make sense to have a ATH10K_FW_WMI_OP_VERSION_MAX (or _LAST) and verify if the IE value doesn't exceed the last known value? This way switch()es won't need default handler by design and thus we'll get the benefit of compiler telling us about unhandled cases when we add new enums. Micha?
On 1 December 2014 at 15:45, Kalle Valo <kvalo@qca.qualcomm.com> wrote: [...] > @@ -801,11 +812,24 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) > } > > if (test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features)) { > - ar->max_num_peers = TARGET_10X_NUM_PEERS; > - ar->max_num_stations = TARGET_10X_NUM_STATIONS; > + if (test_bit(ATH10K_FW_FEATURE_WMI_10_2, ar->fw_features)) > + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_10_2; > + else > + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_10_1; > } else { > + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_MAIN; > + } You always overwrite ar->wmi.op_version with MAIN if it's not 10.x which means TLV is/wiil be effectively overwritten. Perhaps the op_version enum values should start with 1 so that 0 can be used as "unset" and only in that case should the above fallback be attempted. Also, I think it might be a good idea to reset ar->wmi.op_version in ath10k_core_fetch_firmware_api_n (and api_1 as well) so that if any attempt fails (e.g. due to IE binary corruption) ar->wmi.op_version isn't propagated/left unchanged to another fw API attempt. Micha?
Michal Kazior <michal.kazior@tieto.com> writes: > On 1 December 2014 at 15:45, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > [...] >> @@ -801,11 +812,24 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) >> } >> >> if (test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features)) { >> - ar->max_num_peers = TARGET_10X_NUM_PEERS; >> - ar->max_num_stations = TARGET_10X_NUM_STATIONS; >> + if (test_bit(ATH10K_FW_FEATURE_WMI_10_2, ar->fw_features)) >> + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_10_2; >> + else >> + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_10_1; >> } else { >> + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_MAIN; >> + } > > You always overwrite ar->wmi.op_version with MAIN if it's not 10.x > which means TLV is/wiil be effectively overwritten. > > Perhaps the op_version enum values should start with 1 so that 0 can > be used as "unset" and only in that case should the above fallback be > attempted. That's a good idea, I'll add that. > Also, I think it might be a good idea to reset ar->wmi.op_version in > ath10k_core_fetch_firmware_api_n (and api_1 as well) so that if any > attempt fails (e.g. due to IE binary corruption) ar->wmi.op_version > isn't propagated/left unchanged to another fw API attempt. Yeah, we should do that. But that's an existing problem, not something introduced by this patchset (we have the same problem with ar->fw_features) . So I would like to fix that later and added it to the TODO list: http://wireless.kernel.org/en/users/Drivers/ath10k/todo
Michal Kazior <michal.kazior@tieto.com> writes: > On 1 December 2014 at 15:45, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > [...] >> @@ -562,6 +562,17 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name) >> ar->otp_len = ie_len; >> >> break; >> + case ATH10K_FW_IE_WMI_OP_VERSION: >> + if (ie_len != sizeof(u32)) >> + break; >> + >> + version = (__le32 *)data; >> + >> + ar->wmi.op_version = le32_to_cpup(version); >> + >> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "found fw ie wmi op version %d\n", >> + ar->wmi.op_version); > > Hmm.. wouldn't it make sense to have a ATH10K_FW_WMI_OP_VERSION_MAX > (or _LAST) and verify if the IE value doesn't exceed the last known > value? This way switch()es won't need default handler by design and > thus we'll get the benefit of compiler telling us about unhandled > cases when we add new enums. A good idea, I'll add that.
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index bd6315952ad0..f8ddc8087db9 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -447,7 +447,7 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name) int ie_id, i, index, bit, ret; struct ath10k_fw_ie *hdr; const u8 *data; - __le32 *timestamp; + __le32 *timestamp, *version; /* first fetch the firmware file (firmware-*.bin) */ ar->firmware = ath10k_fetch_fw_file(ar, ar->hw_params.fw.dir, name); @@ -562,6 +562,17 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name) ar->otp_len = ie_len; break; + case ATH10K_FW_IE_WMI_OP_VERSION: + if (ie_len != sizeof(u32)) + break; + + version = (__le32 *)data; + + ar->wmi.op_version = le32_to_cpup(version); + + ath10k_dbg(ar, ATH10K_DBG_BOOT, "found fw ie wmi op version %d\n", + ar->wmi.op_version); + break; default: ath10k_warn(ar, "Unknown FW IE: %u\n", le32_to_cpu(hdr->id)); @@ -801,11 +812,24 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) } if (test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features)) { - ar->max_num_peers = TARGET_10X_NUM_PEERS; - ar->max_num_stations = TARGET_10X_NUM_STATIONS; + if (test_bit(ATH10K_FW_FEATURE_WMI_10_2, ar->fw_features)) + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_10_2; + else + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_10_1; } else { + ar->wmi.op_version = ATH10K_FW_WMI_OP_VERSION_MAIN; + } + + switch (ar->wmi.op_version) { + case ATH10K_FW_WMI_OP_VERSION_MAIN: ar->max_num_peers = TARGET_NUM_PEERS; ar->max_num_stations = TARGET_NUM_STATIONS; + break; + case ATH10K_FW_WMI_OP_VERSION_10_1: + case ATH10K_FW_WMI_OP_VERSION_10_2: + ar->max_num_peers = TARGET_10X_NUM_PEERS; + ar->max_num_stations = TARGET_10X_NUM_STATIONS; + break; } return 0; diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 514c219263a5..92b04fe73151 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -120,6 +120,7 @@ struct ath10k_mem_chunk { }; struct ath10k_wmi { + unsigned int op_version; enum ath10k_htc_ep_id eid; struct completion service_ready; struct completion unified_ready; @@ -369,7 +370,7 @@ enum ath10k_fw_features { /* wmi_mgmt_rx_hdr contains extra RSSI information */ ATH10K_FW_FEATURE_EXT_WMI_MGMT_RX = 0, - /* firmware from 10X branch */ + /* Firmware from 10X branch. Deprecated, don't use in new code. */ ATH10K_FW_FEATURE_WMI_10X = 1, /* firmware support tx frame management over WMI, otherwise it's HTT */ @@ -378,8 +379,9 @@ enum ath10k_fw_features { /* Firmware does not support P2P */ ATH10K_FW_FEATURE_NO_P2P = 3, - /* Firmware 10.2 feature bit. The ATH10K_FW_FEATURE_WMI_10X feature bit - * is required to be set as well. + /* Firmware 10.2 feature bit. The ATH10K_FW_FEATURE_WMI_10X feature + * bit is required to be set as well. Deprecated, don't use in new + * code. */ ATH10K_FW_FEATURE_WMI_10_2 = 4, diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h index dfedfd0e0f34..04aaf9af3ca0 100644 --- a/drivers/net/wireless/ath/ath10k/hw.h +++ b/drivers/net/wireless/ath/ath10k/hw.h @@ -58,6 +58,16 @@ enum ath10k_fw_ie_type { ATH10K_FW_IE_FEATURES = 2, ATH10K_FW_IE_FW_IMAGE = 3, ATH10K_FW_IE_OTP_IMAGE = 4, + + /* WMI "operations" interface version, 32 bit value. Supported from + * FW API 4 and above. */ + ATH10K_FW_IE_WMI_OP_VERSION = 5, +}; + +enum ath10k_fw_wmi_op_version { + ATH10K_FW_WMI_OP_VERSION_MAIN = 0, + ATH10K_FW_WMI_OP_VERSION_10_1 = 1, + ATH10K_FW_WMI_OP_VERSION_10_2 = 2, }; /* Known pecularities:
Instead of using feature flags, add new 32 bit variable for managing different WMI versions. This makes it firmware interface tests a bit less convoluted, especially when we add one more interface. Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> --- drivers/net/wireless/ath/ath10k/core.c | 30 +++++++++++++++++++++++++++--- drivers/net/wireless/ath/ath10k/core.h | 8 +++++--- drivers/net/wireless/ath/ath10k/hw.h | 10 ++++++++++ 3 files changed, 42 insertions(+), 6 deletions(-)