| Message ID | 1416838646-18801-1-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Michal Kazior <michal.kazior@tieto.com> writes: > This will make it easier to extend and maintain > list of supported hardware. > > As a requirement this moves chip_id checking a > little later because target_version isn't known > until BMI can be queried. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> The reason why I originally added the chip_id check was that QCA988X hw 1.0 failed badly (ath10k might have even crashed, don't remember anymore) and I added this chip_id as an ugly workaround to detect hw1.0 early. Most likely with this patch the problem comes back again. I don't know what's a good solution, need to think more. Any ideas?
On 26 November 2014 at 08:21, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> This will make it easier to extend and maintain >> list of supported hardware. >> >> As a requirement this moves chip_id checking a >> little later because target_version isn't known >> until BMI can be queried. >> >> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > > The reason why I originally added the chip_id check was that QCA988X hw > 1.0 failed badly (ath10k might have even crashed, don't remember > anymore) and I added this chip_id as an ugly workaround to detect hw1.0 > early. Most likely with this patch the problem comes back again. Hmm.. good point. I think it mostly failed during firmware transfer to target but since BMI also needs CE then it might as well fail mid-way. > I don't know what's a good solution, need to think more. Any ideas? Hmm.. I have a couple of ideas: 1. Don't use BMI to read target_version. Instead use a register (assuming there is one). This implies each transport (we have pci only now) would need to be able to read it somehow unless we support a fallback to BMI if it wasn't prepped by the transport. 2. Have a dedicatd pci-specific structure: struct ath10k_pci_supported_chip { u16 dev_id; u32 chip_id; }; struct ath10k_pci_supported_chip ath10k_pci_supported_chips[] = { { QCA988X_2_0_DEVICE_ID, QCA988X_HW_2_0_CHIP_ID_REV }, // ... }; Probably the simplest and has least impact. 3. Don't use target_version to decide hw_params. Use pci device id instead. This is a bit of a problem because ath10k_hw_params_list is in core and is supposed to be transport-agnostic (but should it really be? I can imagine theoretical usb transport could have devices reporting identical target_version as a pci one but would need different firmware files to be used). This probably needs more thinking through. Micha?
Michal Kazior <michal.kazior@tieto.com> writes: > 2. Have a dedicatd pci-specific structure: > > struct ath10k_pci_supported_chip { > u16 dev_id; > u32 chip_id; > }; > > struct ath10k_pci_supported_chip ath10k_pci_supported_chips[] = { > { QCA988X_2_0_DEVICE_ID, QCA988X_HW_2_0_CHIP_ID_REV }, > // ... > }; > > Probably the simplest and has least impact. Another idea which is a variation of this: In ath10k_core_register() we iterate through ath10k_hw_params_list and make sure that the chip id is supported and if not, bail out. If the chip id is found from the array continue the registration process like in this patch. Basically this would be a whitelist check.
On 27 November 2014 at 08:30, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> 2. Have a dedicatd pci-specific structure: >> >> struct ath10k_pci_supported_chip { >> u16 dev_id; >> u32 chip_id; >> }; >> >> struct ath10k_pci_supported_chip ath10k_pci_supported_chips[] = { >> { QCA988X_2_0_DEVICE_ID, QCA988X_HW_2_0_CHIP_ID_REV }, >> // ... >> }; >> >> Probably the simplest and has least impact. > > Another idea which is a variation of this: > > In ath10k_core_register() we iterate through ath10k_hw_params_list and > make sure that the chip id is supported and if not, bail out. If the > chip id is found from the array continue the registration process like > in this patch. Basically this would be a whitelist check. This won't work because chip ids can overlap between different hardware designs and you'd get false positives. Micha?
Michal Kazior <michal.kazior@tieto.com> writes: > On 27 November 2014 at 08:30, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Michal Kazior <michal.kazior@tieto.com> writes: >> >>> 2. Have a dedicatd pci-specific structure: >>> >>> struct ath10k_pci_supported_chip { >>> u16 dev_id; >>> u32 chip_id; >>> }; >>> >>> struct ath10k_pci_supported_chip ath10k_pci_supported_chips[] = { >>> { QCA988X_2_0_DEVICE_ID, QCA988X_HW_2_0_CHIP_ID_REV }, >>> // ... >>> }; >>> >>> Probably the simplest and has least impact. >> >> Another idea which is a variation of this: >> >> In ath10k_core_register() we iterate through ath10k_hw_params_list and >> make sure that the chip id is supported and if not, bail out. If the >> chip id is found from the array continue the registration process like >> in this patch. Basically this would be a whitelist check. > > This won't work because chip ids can overlap between different > hardware designs and you'd get false positives. Ah, let's forget that then.
Michal Kazior <michal.kazior@tieto.com> writes: > Hmm.. I have a couple of ideas: [...] > 2. Have a dedicatd pci-specific structure: > > struct ath10k_pci_supported_chip { > u16 dev_id; > u32 chip_id; > }; > > struct ath10k_pci_supported_chip ath10k_pci_supported_chips[] = { > { QCA988X_2_0_DEVICE_ID, QCA988X_HW_2_0_CHIP_ID_REV }, > // ... > }; > > Probably the simplest and has least impact. Yeah, this seems to be the best. Can you send v2?
On 28 November 2014 at 08:10, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> Hmm.. I have a couple of ideas: > > [...] > >> 2. Have a dedicatd pci-specific structure: >> >> struct ath10k_pci_supported_chip { >> u16 dev_id; >> u32 chip_id; >> }; >> >> struct ath10k_pci_supported_chip ath10k_pci_supported_chips[] = { >> { QCA988X_2_0_DEVICE_ID, QCA988X_HW_2_0_CHIP_ID_REV }, >> // ... >> }; >> >> Probably the simplest and has least impact. > > Yeah, this seems to be the best. Can you send v2? Ok. I'll go with this approach and post a v2 when I'm done. Micha?
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index f660553..3904ce3 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -43,11 +43,21 @@ MODULE_PARM_DESC(uart_print, "Uart target debugging"); MODULE_PARM_DESC(p2p, "Enable ath10k P2P support"); MODULE_PARM_DESC(skip_otp, "Skip otp failure for calibration in testmode"); +static u32 qca988x_supported_chips[] = { + /* QCA988X_HW_1_0_CHIP_ID_REV is not supported. It's way too buggy and + * crashes because ath10k doesn't have hacks and workarounds for it. + */ + + QCA988X_HW_2_0_CHIP_ID_REV +}; + static const struct ath10k_hw_params ath10k_hw_params_list[] = { { .id = QCA988X_HW_2_0_VERSION, .name = "qca988x hw2.0", .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR, + .supported_chips = qca988x_supported_chips, + .num_supported_chips = ARRAY_SIZE(qca988x_supported_chips), .fw = { .dir = QCA988X_HW_2_0_FW_DIR, .fw = QCA988X_HW_2_0_FW_FILE, @@ -719,10 +729,26 @@ static int ath10k_init_uart(struct ath10k *ar) return 0; } +static int ath10k_core_check_chip_id(struct ath10k *ar, u32 chip_id, + const struct ath10k_hw_params *hw_params) +{ + u32 hw_revision = MS(chip_id, SOC_CHIP_ID_REV); + int i; + + ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot chip_id 0x%08x hw_revision 0x%x\n", + chip_id, hw_revision); + + for (i = 0; i < hw_params->num_supported_chips; i++) + if (hw_params->supported_chips[i] == hw_revision) + return 0; + + return -ENOTSUPP; +} + static int ath10k_init_hw_params(struct ath10k *ar) { const struct ath10k_hw_params *uninitialized_var(hw_params); - int i; + int i, ret; for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { hw_params = &ath10k_hw_params_list[i]; @@ -737,6 +763,13 @@ static int ath10k_init_hw_params(struct ath10k *ar) return -EINVAL; } + ret = ath10k_core_check_chip_id(ar, ar->chip_id, hw_params); + if (ret) { + ath10k_err(ar, "unsupported hardware %s chip_id 0x%08x\n", + hw_params->name, ar->chip_id); + return ret; + } + ar->hw_params = *hw_params; ath10k_dbg(ar, ATH10K_DBG_BOOT, "Hardware name %s version 0x%x\n", @@ -1055,34 +1088,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar) return 0; } -static int ath10k_core_check_chip_id(struct ath10k *ar) -{ - u32 hw_revision = MS(ar->chip_id, SOC_CHIP_ID_REV); - - ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot chip_id 0x%08x hw_revision 0x%x\n", - ar->chip_id, hw_revision); - - /* Check that we are not using hw1.0 (some of them have same pci id - * as hw2.0) before doing anything else as ath10k crashes horribly - * due to missing hw1.0 workarounds. */ - switch (hw_revision) { - case QCA988X_HW_1_0_CHIP_ID_REV: - ath10k_err(ar, "ERROR: qca988x hw1.0 is not supported\n"); - return -EOPNOTSUPP; - - case QCA988X_HW_2_0_CHIP_ID_REV: - /* known hardware revision, continue normally */ - return 0; - - default: - ath10k_warn(ar, "Warning: hardware revision unknown (0x%x), expect problems\n", - ar->chip_id); - return 0; - } - - return 0; -} - static void ath10k_core_register_work(struct work_struct *work) { struct ath10k *ar = container_of(work, struct ath10k, register_work); @@ -1130,16 +1135,7 @@ err: int ath10k_core_register(struct ath10k *ar, u32 chip_id) { - int status; - ar->chip_id = chip_id; - - status = ath10k_core_check_chip_id(ar); - if (status) { - ath10k_err(ar, "Unsupported chip id 0x%08x\n", ar->chip_id); - return status; - } - queue_work(ar->workqueue, &ar->register_work); return 0; diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 8f86bd3..d37ffd6 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -475,7 +475,8 @@ struct ath10k { u32 id; const char *name; u32 patch_load_addr; - + const u32 *supported_chips; + int num_supported_chips; struct ath10k_hw_params_fw { const char *dir; const char *fw;
This will make it easier to extend and maintain list of supported hardware. As a requirement this moves chip_id checking a little later because target_version isn't known until BMI can be queried. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- drivers/net/wireless/ath/ath10k/core.c | 72 ++++++++++++++++------------------ drivers/net/wireless/ath/ath10k/core.h | 3 +- 2 files changed, 36 insertions(+), 39 deletions(-)