| Message ID | 20170917194013.8658-4-erik.stromdahl@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
Erik Stromdahl <erik.stromdahl@gmail.com> writes: > Added ability to set bus type and configure the max number of > peers in the ath10k_hw_params struct. > > With this functionality it is possible to have a different > hw configuration depending on bus type for the same radio > chipset. > > E.g. SDIO and USB devices using the same chipset as PCIe > devices will potentially use different board files and perhaps > other configuration parameters. > > One such parameter is the max number of peers. > Instead of using a default value (suitable for PCIe devices) > derived from the WMI op version, a per target value can be > used instead. > > This is needed by the QCA9377 USB device in order to prevent > the target fw to crash after HTT RX ring cfg is issued. > > Apparently, the QCA9377 HL device does not seem to handle the > same amount of peers as the LL devices. > > Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> I was a bit torn about this, I definitely see the need for this but on the other hand it creates duplicate data (for example two entries for QCA9377 chip). I guess this is the right approach, at least I cannot come up anything better. But this patch should be split into two: 1) add bus field to struct ath10k_hw_params 2) add max_num_peers field to struct ath10k_hw_params And it seems 2) is already implemented in commit 9f2992fea580 ("ath10k: wmi: get wmi init parameter values from hw params"), so hopefully we only need 1) anymore. > --- a/drivers/net/wireless/ath/ath10k/core.c > +++ b/drivers/net/wireless/ath/ath10k/core.c > @@ -1663,9 +1663,19 @@ static int ath10k_init_hw_params(struct ath10k *ar) > for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { > hw_params = &ath10k_hw_params_list[i]; > > - if (hw_params->id == ar->target_version && > - hw_params->dev_id == ar->dev_id) > - break; > + if (ar->is_high_latency) { > + /* High latency devices will use different fw depending > + * on if it is a USB or SDIO device. > + */ > + if (hw_params->bus == ar->hif.bus && > + hw_params->id == ar->target_version && > + hw_params->dev_id == ar->dev_id) > + break; > + } else { > + if (hw_params->id == ar->target_version && > + hw_params->dev_id == ar->dev_id) > + break; > + } I don't like the is_high_latency test here at all. The bus field should be checked with all entries, not just high latency ones. And because of this even most of the hw_param bus field entries were not initialised. So only thing to do is to initialise the bus field for all the entries and the ugly test here can be removed. Just remember that QCA4019 uses AHB, I think all the rest is PCI. Or do we have AHB devices supported? > @@ -550,6 +557,18 @@ struct ath10k_hw_params { > */ > int vht160_mcs_rx_highest; > int vht160_mcs_tx_highest; > + > + /* max_num_peers can be used to override the setting derived from > + * the WMI op version. If this value is non-zero, it will always > + * be used instead of the default value derived from the WMI op > + * version. > + */ > + int max_num_peers; > + > + /* Specifies whether or not the device is a high latency device */ > + bool is_high_latency; > + > + enum ath10k_bus bus; > }; Please move the bus field between dev_id and name fields. It's so important that it should not be in the end.
On 2017-12-22 16:19, Kalle Valo wrote: > Erik Stromdahl <erik.stromdahl@gmail.com> writes: > >> Added ability to set bus type and configure the max number of >> peers in the ath10k_hw_params struct. >> >> With this functionality it is possible to have a different >> hw configuration depending on bus type for the same radio >> chipset. >> >> E.g. SDIO and USB devices using the same chipset as PCIe >> devices will potentially use different board files and perhaps >> other configuration parameters. >> >> One such parameter is the max number of peers. >> Instead of using a default value (suitable for PCIe devices) >> derived from the WMI op version, a per target value can be >> used instead. >> >> This is needed by the QCA9377 USB device in order to prevent >> the target fw to crash after HTT RX ring cfg is issued. >> >> Apparently, the QCA9377 HL device does not seem to handle the >> same amount of peers as the LL devices. >> >> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> > > I was a bit torn about this, I definitely see the need for this but on > the other hand it creates duplicate data (for example two entries for > QCA9377 chip). I guess this is the right approach, at least I cannot > come up anything better. > > But this patch should be split into two: > > 1) add bus field to struct ath10k_hw_params > > 2) add max_num_peers field to struct ath10k_hw_params > > And it seems 2) is already implemented in commit 9f2992fea580 ("ath10k: > wmi: get wmi init parameter values from hw params"), so hopefully we > only need 1) anymore. > Before commit 9f2992fea580a48135591873e5e3ac7e01444207, TARGET_TLV_NUM_PEERS was used both in the WMI TLV init command and as the value of *max_num_peers* in *struct ath10k* (ar->max_num_peers). commit 9f2992fea580a48135591873e5e3ac7e01444207 does not set *ar->max_num_peers* to the value of *ar->hw_param->num_peers*. Is this correct? As I see it, there is a possible mismatch between what is written to the device in the WMI init message and the value of *ar->max_num_peers*. Do we still need *max_num_peers* in *struct ath10k* now that we have the *num_peers* member in *struct ath10k_hw_params*? I am currently rewriting my HL patches and I was thinking about adding a separate patch related to this. >> --- a/drivers/net/wireless/ath/ath10k/core.c >> +++ b/drivers/net/wireless/ath/ath10k/core.c >> @@ -1663,9 +1663,19 @@ static int ath10k_init_hw_params(struct ath10k *ar) >> for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { >> hw_params = &ath10k_hw_params_list[i]; >> >> - if (hw_params->id == ar->target_version && >> - hw_params->dev_id == ar->dev_id) >> - break; >> + if (ar->is_high_latency) { >> + /* High latency devices will use different fw depending >> + * on if it is a USB or SDIO device. >> + */ >> + if (hw_params->bus == ar->hif.bus && >> + hw_params->id == ar->target_version && >> + hw_params->dev_id == ar->dev_id) >> + break; >> + } else { >> + if (hw_params->id == ar->target_version && >> + hw_params->dev_id == ar->dev_id) >> + break; >> + } > > I don't like the is_high_latency test here at all. The bus field should > be checked with all entries, not just high latency ones. And because of > this even most of the hw_param bus field entries were not initialised. > > So only thing to do is to initialise the bus field for all the entries > and the ugly test here can be removed. Just remember that QCA4019 uses > AHB, I think all the rest is PCI. Or do we have AHB devices supported? I noticed that there has been introduced a new bus type (SNOC). Do you know which devices are SNOC devices? Btw, what the heck is SNOC anyway? > >> @@ -550,6 +557,18 @@ struct ath10k_hw_params { >> */ >> int vht160_mcs_rx_highest; >> int vht160_mcs_tx_highest; >> + >> + /* max_num_peers can be used to override the setting derived from >> + * the WMI op version. If this value is non-zero, it will always >> + * be used instead of the default value derived from the WMI op >> + * version. >> + */ >> + int max_num_peers; >> + >> + /* Specifies whether or not the device is a high latency device */ >> + bool is_high_latency; >> + >> + enum ath10k_bus bus; >> }; > > Please move the bus field between dev_id and name fields. It's so > important that it should not be in the end. >
Erik Stromdahl <erik.stromdahl@gmail.com> writes: > On 2017-12-22 16:19, Kalle Valo wrote: > >> I was a bit torn about this, I definitely see the need for this but on >> the other hand it creates duplicate data (for example two entries for >> QCA9377 chip). I guess this is the right approach, at least I cannot >> come up anything better. >> >> But this patch should be split into two: >> >> 1) add bus field to struct ath10k_hw_params >> >> 2) add max_num_peers field to struct ath10k_hw_params >> >> And it seems 2) is already implemented in commit 9f2992fea580 ("ath10k: >> wmi: get wmi init parameter values from hw params"), so hopefully we >> only need 1) anymore. >> > > Before commit 9f2992fea580a48135591873e5e3ac7e01444207, > TARGET_TLV_NUM_PEERS was used both in the WMI TLV init command > and as the value of *max_num_peers* in *struct ath10k* (ar->max_num_peers). > > commit 9f2992fea580a48135591873e5e3ac7e01444207 does not set > *ar->max_num_peers* to the value of *ar->hw_param->num_peers*. > > Is this correct? > > As I see it, there is a possible mismatch between what is written > to the device in the WMI init message and the value of *ar->max_num_peers*. > > Do we still need *max_num_peers* in *struct ath10k* now that we have the > *num_peers* member in *struct ath10k_hw_params*? A good point, I didn't thought of that during review. No time to investigate this right now, but maybe Rakesh and Govind (CCed) can comment? > I am currently rewriting my HL patches and I was thinking about adding > a separate patch related to this. Yeah, a separate patch to sort that out is a good idea. >>> --- a/drivers/net/wireless/ath/ath10k/core.c >>> +++ b/drivers/net/wireless/ath/ath10k/core.c >>> @@ -1663,9 +1663,19 @@ static int ath10k_init_hw_params(struct ath10k *ar) >>> for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { >>> hw_params = &ath10k_hw_params_list[i]; >>> - if (hw_params->id == ar->target_version && >>> - hw_params->dev_id == ar->dev_id) >>> - break; >>> + if (ar->is_high_latency) { >>> + /* High latency devices will use different fw depending >>> + * on if it is a USB or SDIO device. >>> + */ >>> + if (hw_params->bus == ar->hif.bus && >>> + hw_params->id == ar->target_version && >>> + hw_params->dev_id == ar->dev_id) >>> + break; >>> + } else { >>> + if (hw_params->id == ar->target_version && >>> + hw_params->dev_id == ar->dev_id) >>> + break; >>> + } >> >> I don't like the is_high_latency test here at all. The bus field should >> be checked with all entries, not just high latency ones. And because of >> this even most of the hw_param bus field entries were not initialised. >> >> So only thing to do is to initialise the bus field for all the entries >> and the ugly test here can be removed. Just remember that QCA4019 uses >> AHB, I think all the rest is PCI. Or do we have AHB devices supported? > > I noticed that there has been introduced a new bus type (SNOC). > Do you know which devices are SNOC devices? SNOC is for wcn3990. > Btw, what the heck is SNOC anyway? I have forgetten already what the acronym meant but it's basically some sort of shared memory communication method with the firmware.
>> A good point, I didn't thought of that during review. No time to investigate this right now, but maybe Rakesh and Govind (CCed) can comment? Yes, ar->max_num_peers needs to be assigned with ar->hw_param->num_peers. This can create mismatch for wcn3990 target if we create multiple peers( TARGET_HL_10_TLV_NUM_PEERS vs TARGET_TLV_NUM_PEERS). We will fix this and raise this as separate change. >>> Btw, what the heck is SNOC anyway? SNOC is system NOC(network on chip). WCN3990 is integrated chipset connected over SNOC and only RF part is discrete to the SoC. BR, Govind -----Original Message----- From: Kalle Valo [mailto:kvalo@codeaurora.org] Sent: Monday, January 8, 2018 7:12 PM To: Erik Stromdahl <erik.stromdahl@gmail.com> Cc: linux-wireless@vger.kernel.org; ath10k@lists.infradead.org; Rakesh Pillai <pillair@qti.qualcomm.com>; Govind Singh <govinds@qti.qualcomm.com> Subject: Re: [RFC v3 03/11] ath10k: per target configurablity of various items Erik Stromdahl <erik.stromdahl@gmail.com> writes: > On 2017-12-22 16:19, Kalle Valo wrote: > >> I was a bit torn about this, I definitely see the need for this but >> on the other hand it creates duplicate data (for example two entries >> for >> QCA9377 chip). I guess this is the right approach, at least I cannot >> come up anything better. >> >> But this patch should be split into two: >> >> 1) add bus field to struct ath10k_hw_params >> >> 2) add max_num_peers field to struct ath10k_hw_params >> >> And it seems 2) is already implemented in commit 9f2992fea580 ("ath10k: >> wmi: get wmi init parameter values from hw params"), so hopefully we >> only need 1) anymore. >> > > Before commit 9f2992fea580a48135591873e5e3ac7e01444207, > TARGET_TLV_NUM_PEERS was used both in the WMI TLV init command and as > the value of *max_num_peers* in *struct ath10k* (ar->max_num_peers). > > commit 9f2992fea580a48135591873e5e3ac7e01444207 does not set > *ar->max_num_peers* to the value of *ar->hw_param->num_peers*. > > Is this correct? > > As I see it, there is a possible mismatch between what is written to > the device in the WMI init message and the value of *ar->max_num_peers*. > > Do we still need *max_num_peers* in *struct ath10k* now that we have > the > *num_peers* member in *struct ath10k_hw_params*? A good point, I didn't thought of that during review. No time to investigate this right now, but maybe Rakesh and Govind (CCed) can comment? > I am currently rewriting my HL patches and I was thinking about adding > a separate patch related to this. Yeah, a separate patch to sort that out is a good idea. >>> --- a/drivers/net/wireless/ath/ath10k/core.c >>> +++ b/drivers/net/wireless/ath/ath10k/core.c >>> @@ -1663,9 +1663,19 @@ static int ath10k_init_hw_params(struct ath10k *ar) >>> for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { >>> hw_params = &ath10k_hw_params_list[i]; >>> - if (hw_params->id == ar->target_version && >>> - hw_params->dev_id == ar->dev_id) >>> - break; >>> + if (ar->is_high_latency) { >>> + /* High latency devices will use different fw depending >>> + * on if it is a USB or SDIO device. >>> + */ >>> + if (hw_params->bus == ar->hif.bus && >>> + hw_params->id == ar->target_version && >>> + hw_params->dev_id == ar->dev_id) >>> + break; >>> + } else { >>> + if (hw_params->id == ar->target_version && >>> + hw_params->dev_id == ar->dev_id) >>> + break; >>> + } >> >> I don't like the is_high_latency test here at all. The bus field >> should be checked with all entries, not just high latency ones. And >> because of this even most of the hw_param bus field entries were not initialised. >> >> So only thing to do is to initialise the bus field for all the >> entries and the ugly test here can be removed. Just remember that >> QCA4019 uses AHB, I think all the rest is PCI. Or do we have AHB devices supported? > > I noticed that there has been introduced a new bus type (SNOC). > Do you know which devices are SNOC devices? SNOC is for wcn3990. > Btw, what the heck is SNOC anyway? I have forgetten already what the acronym meant but it's basically some sort of shared memory communication method with the firmware. -- Kalle Valo
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index f1924c974a12..a4a326c89e0d 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1663,9 +1663,19 @@ static int ath10k_init_hw_params(struct ath10k *ar) for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { hw_params = &ath10k_hw_params_list[i]; - if (hw_params->id == ar->target_version && - hw_params->dev_id == ar->dev_id) - break; + if (ar->is_high_latency) { + /* High latency devices will use different fw depending + * on if it is a USB or SDIO device. + */ + if (hw_params->bus == ar->hif.bus && + hw_params->id == ar->target_version && + hw_params->dev_id == ar->dev_id) + break; + } else { + if (hw_params->id == ar->target_version && + hw_params->dev_id == ar->dev_id) + break; + } } if (i == ARRAY_SIZE(ath10k_hw_params_list)) { @@ -1764,6 +1774,7 @@ static void ath10k_core_set_coverage_class_work(struct work_struct *work) static int ath10k_core_init_firmware_features(struct ath10k *ar) { struct ath10k_fw_file *fw_file = &ar->normal_mode_fw.fw_file; + int max_num_peers; if (test_bit(ATH10K_FW_FEATURE_WMI_10_2, fw_file->fw_features) && !test_bit(ATH10K_FW_FEATURE_WMI_10X, fw_file->fw_features)) { @@ -1843,7 +1854,7 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) switch (fw_file->wmi_op_version) { case ATH10K_FW_WMI_OP_VERSION_MAIN: - ar->max_num_peers = TARGET_NUM_PEERS; + max_num_peers = TARGET_NUM_PEERS; ar->max_num_stations = TARGET_NUM_STATIONS; ar->max_num_vdevs = TARGET_NUM_VDEVS; ar->htt.max_num_pending_tx = TARGET_NUM_MSDU_DESC; @@ -1855,10 +1866,10 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) case ATH10K_FW_WMI_OP_VERSION_10_2: case ATH10K_FW_WMI_OP_VERSION_10_2_4: if (ath10k_peer_stats_enabled(ar)) { - ar->max_num_peers = TARGET_10X_TX_STATS_NUM_PEERS; + max_num_peers = TARGET_10X_TX_STATS_NUM_PEERS; ar->max_num_stations = TARGET_10X_TX_STATS_NUM_STATIONS; } else { - ar->max_num_peers = TARGET_10X_NUM_PEERS; + max_num_peers = TARGET_10X_NUM_PEERS; ar->max_num_stations = TARGET_10X_NUM_STATIONS; } ar->max_num_vdevs = TARGET_10X_NUM_VDEVS; @@ -1867,7 +1878,7 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) ar->max_spatial_stream = WMI_MAX_SPATIAL_STREAM; break; case ATH10K_FW_WMI_OP_VERSION_TLV: - ar->max_num_peers = TARGET_TLV_NUM_PEERS; + max_num_peers = TARGET_TLV_NUM_PEERS; ar->max_num_stations = TARGET_TLV_NUM_STATIONS; ar->max_num_vdevs = TARGET_TLV_NUM_VDEVS; ar->max_num_tdls_vdevs = TARGET_TLV_NUM_TDLS_VDEVS; @@ -1878,7 +1889,7 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) ar->max_spatial_stream = WMI_MAX_SPATIAL_STREAM; break; case ATH10K_FW_WMI_OP_VERSION_10_4: - ar->max_num_peers = TARGET_10_4_NUM_PEERS; + max_num_peers = TARGET_10_4_NUM_PEERS; ar->max_num_stations = TARGET_10_4_NUM_STATIONS; ar->num_active_peers = TARGET_10_4_ACTIVE_PEERS; ar->max_num_vdevs = TARGET_10_4_NUM_VDEVS; @@ -1896,10 +1907,16 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar) break; case ATH10K_FW_WMI_OP_VERSION_UNSET: case ATH10K_FW_WMI_OP_VERSION_MAX: + default: WARN_ON(1); return -EINVAL; } + if (ar->hw_params.max_num_peers) + ar->max_num_peers = ar->hw_params.max_num_peers; + else + ar->max_num_peers = max_num_peers; + /* Backwards compatibility for firmwares without * ATH10K_FW_IE_HTT_OP_VERSION. */ diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index dc9ecf773d51..64dadcd6e531 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -88,13 +88,6 @@ struct ath10k; -enum ath10k_bus { - ATH10K_BUS_PCI, - ATH10K_BUS_AHB, - ATH10K_BUS_SDIO, - ATH10K_BUS_USB, -}; - static inline const char *ath10k_bus_str(enum ath10k_bus bus) { switch (bus) { diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h index 0c089f6dd3d9..8cf7b963f3d4 100644 --- a/drivers/net/wireless/ath/ath10k/hw.h +++ b/drivers/net/wireless/ath/ath10k/hw.h @@ -20,6 +20,13 @@ #include "targaddrs.h" +enum ath10k_bus { + ATH10K_BUS_PCI, + ATH10K_BUS_AHB, + ATH10K_BUS_SDIO, + ATH10K_BUS_USB, +}; + #define ATH10K_FW_DIR "ath10k" #define QCA988X_2_0_DEVICE_ID (0x003c) @@ -550,6 +557,18 @@ struct ath10k_hw_params { */ int vht160_mcs_rx_highest; int vht160_mcs_tx_highest; + + /* max_num_peers can be used to override the setting derived from + * the WMI op version. If this value is non-zero, it will always + * be used instead of the default value derived from the WMI op + * version. + */ + int max_num_peers; + + /* Specifies whether or not the device is a high latency device */ + bool is_high_latency; + + enum ath10k_bus bus; }; struct htt_rx_desc; @@ -660,6 +679,9 @@ ath10k_rx_desc_get_l3_pad_bytes(struct ath10k_hw_params *hw, #define TARGET_TLV_NUM_MSDU_DESC (1024 + 32) #define TARGET_TLV_NUM_WOW_PATTERNS 22 +/* Target specific defines for QCA9377 high latency firmware */ +#define TARGET_QCA9377_HL_NUM_PEERS 15 + /* Diagnostic Window */ #define CE_DIAG_PIPE 7 diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c index 7616c1c4bbd3..34e977049f00 100644 --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c @@ -1406,7 +1406,7 @@ static struct sk_buff *ath10k_wmi_tlv_op_gen_init(struct ath10k *ar) cmd->num_host_mem_chunks = __cpu_to_le32(ar->wmi.num_mem_chunks); cfg->num_vdevs = __cpu_to_le32(TARGET_TLV_NUM_VDEVS); - cfg->num_peers = __cpu_to_le32(TARGET_TLV_NUM_PEERS); + cfg->num_peers = __cpu_to_le32(ar->max_num_peers); if (test_bit(WMI_SERVICE_RX_FULL_REORDER, ar->wmi.svc_map)) { cfg->num_offload_peers = __cpu_to_le32(TARGET_TLV_NUM_VDEVS); @@ -1417,7 +1417,7 @@ static struct sk_buff *ath10k_wmi_tlv_op_gen_init(struct ath10k *ar) } cfg->num_peer_keys = __cpu_to_le32(2); - cfg->num_tids = __cpu_to_le32(TARGET_TLV_NUM_TIDS); + cfg->num_tids = __cpu_to_le32(ar->max_num_peers * 2); cfg->ast_skid_limit = __cpu_to_le32(0x10); cfg->tx_chain_mask = __cpu_to_le32(0x7); cfg->rx_chain_mask = __cpu_to_le32(0x7);
Added ability to set bus type and configure the max number of peers in the ath10k_hw_params struct. With this functionality it is possible to have a different hw configuration depending on bus type for the same radio chipset. E.g. SDIO and USB devices using the same chipset as PCIe devices will potentially use different board files and perhaps other configuration parameters. One such parameter is the max number of peers. Instead of using a default value (suitable for PCIe devices) derived from the WMI op version, a per target value can be used instead. This is needed by the QCA9377 USB device in order to prevent the target fw to crash after HTT RX ring cfg is issued. Apparently, the QCA9377 HL device does not seem to handle the same amount of peers as the LL devices. Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> --- drivers/net/wireless/ath/ath10k/core.c | 33 +++++++++++++++++++++++-------- drivers/net/wireless/ath/ath10k/core.h | 7 ------- drivers/net/wireless/ath/ath10k/hw.h | 22 +++++++++++++++++++++ drivers/net/wireless/ath/ath10k/wmi-tlv.c | 4 ++-- 4 files changed, 49 insertions(+), 17 deletions(-)