Hi,
First 2 patches address some irq issues.
Other 3 fix and cleanup device resetting.
Hopefully this will make ath10k a little more
robust when it comes to warm reset.
As a side effect of the changes ath10k now boots a
little faster (slightly over a second instead of
two).
Michal Kazior (5):
ath10k: re-disable interrupts after target init
ath10k: mask/unmask msi fw irq
ath10k: make warm reset a bit safer and faster
ath10k: split reset logic from power up
ath10k: replace power up/down with reset callback
drivers/net/wireless/ath/ath10k/core.c | 7 +-
drivers/net/wireless/ath/ath10k/hif.h | 22 +--
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/mac.c | 8 +-
drivers/net/wireless/ath/ath10k/pci.c | 299 +++++++++++++++--------------
drivers/net/wireless/ath/ath10k/testmode.c | 8 +-
6 files changed, 164 insertions(+), 181 deletions(-)
--
1.8.5.3
Michal Kazior <[email protected]> writes:
> The power up/down didn't make much sense any more
> since hif_stop already stops the device
> compeletely. The target lifecycle was never symmetric
> so don't bother trying to make it look like it is
> and expose a reset hif callback instead of power
> up/down callbacks.
>
> This removes redundant reset calls and thus makes
> device boot/stop/recovery a bit faster.
>
> Signed-off-by: Michal Kazior <[email protected]>
About this I'm not that sure. The reason why I wanted to have power_up()
and power_down() is the case when we need to control the target power
via a gpio line, which I anticipate we will need soon. If you remove
these how could we control the gpio line?
Wouldn't it be the same that you just make hif_power_up() do the same as
hif_reset() and hif_power_down() doesn't do anything (for now)? Or am I
missing something?
--
Kalle Valo
The power up procedure was overly complex due to
warm/cold reset workarounds and issues.
Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 151 ++++++++++++++++++----------------
1 file changed, 80 insertions(+), 71 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index dad92de..a63fe7c 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1781,10 +1781,86 @@ static int ath10k_pci_warm_reset(struct ath10k *ar)
return 0;
}
-static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
+static int ath10k_pci_chip_reset(struct ath10k *ar)
+{
+ int i, ret;
+ u32 val;
+
+ ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot chip reset\n");
+
+ /* Some hardware revisions (e.g. CUS223v2) has issues with cold reset.
+ * It is thus preferred to use warm reset which is safer but may not be
+ * able to recover the device from all possible fail scenarios.
+ *
+ * Warm reset doesn't always work on first try so attempt it a few
+ * times before giving up.
+ */
+ for (i = 0; i < ATH10K_PCI_NUM_WARM_RESET_ATTEMPTS; i++) {
+ ret = ath10k_pci_warm_reset(ar);
+ if (ret) {
+ ath10k_warn(ar, "failed to warm reset attempt %d of %d: %d\n",
+ i + 1, ATH10K_PCI_NUM_WARM_RESET_ATTEMPTS,
+ ret);
+ continue;
+ }
+
+ /* FIXME: Sometimes copy engine doesn't recover after warm
+ * reset. In most cases this needs cold reset. In some of these
+ * cases the device is in such a state that a cold reset may
+ * lock up the host.
+ *
+ * Reading any host interest register via copy engine is
+ * sufficient to verify if device is capable of booting
+ * firmware blob.
+ */
+ ret = ath10k_pci_ce_init(ar);
+ if (ret) {
+ ath10k_warn(ar, "failed to init copy engine: %d\n",
+ ret);
+ continue;
+ }
+
+ ret = ath10k_pci_diag_read32(ar, QCA988X_HOST_INTEREST_ADDRESS,
+ &val);
+ if (ret) {
+ ath10k_warn(ar, "failed to poke copy engine: %d\n",
+ ret);
+ continue;
+ }
+
+ ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot chip reset complete (warm)\n");
+ return 0;
+ }
+
+ if (ath10k_pci_reset_mode == ATH10K_PCI_RESET_WARM_ONLY) {
+ ath10k_warn(ar, "refusing cold reset as requested\n");
+ return -EPERM;
+ }
+
+ ret = ath10k_pci_cold_reset(ar);
+ if (ret) {
+ ath10k_warn(ar, "failed to cold reset: %d\n", ret);
+ return ret;
+ }
+
+ ret = ath10k_pci_wait_for_target_init(ar);
+ if (ret) {
+ ath10k_warn(ar, "failed to wait for target after cold reset: %d\n",
+ ret);
+ return ret;
+ }
+
+ ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot chip reset complete (cold)\n");
+
+ return 0;
+}
+
+static int ath10k_pci_hif_power_up(struct ath10k *ar)
{
int ret;
+ ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif power up\n");
+
/*
* Bring the target up cleanly.
*
@@ -1795,13 +1871,9 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
* is in an unexpected state. We try to catch that here in order to
* reset the Target and retry the probe.
*/
- if (cold_reset)
- ret = ath10k_pci_cold_reset(ar);
- else
- ret = ath10k_pci_warm_reset(ar);
-
+ ret = ath10k_pci_chip_reset(ar);
if (ret) {
- ath10k_err(ar, "failed to reset target: %d\n", ret);
+ ath10k_err(ar, "failed to reset chip: %d\n", ret);
goto err;
}
@@ -1811,12 +1883,6 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
goto err;
}
- ret = ath10k_pci_wait_for_target_init(ar);
- if (ret) {
- ath10k_err(ar, "failed to wait for target to init: %d\n", ret);
- goto err_ce;
- }
-
ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err(ar, "failed to setup init config: %d\n", ret);
@@ -1833,68 +1899,11 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
err_ce:
ath10k_pci_ce_deinit(ar);
- ath10k_pci_warm_reset(ar);
-err:
- return ret;
-}
-
-static int ath10k_pci_hif_power_up_warm(struct ath10k *ar)
-{
- int i, ret;
-
- /*
- * Sometime warm reset succeeds after retries.
- *
- * FIXME: It might be possible to tune ath10k_pci_warm_reset() to work
- * at first try.
- */
- for (i = 0; i < ATH10K_PCI_NUM_WARM_RESET_ATTEMPTS; i++) {
- ret = __ath10k_pci_hif_power_up(ar, false);
- if (ret == 0)
- break;
-
- ath10k_warn(ar, "failed to warm reset (attempt %d out of %d): %d\n",
- i + 1, ATH10K_PCI_NUM_WARM_RESET_ATTEMPTS, ret);
- }
+err:
return ret;
}
-static int ath10k_pci_hif_power_up(struct ath10k *ar)
-{
- int ret;
-
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif power up\n");
-
- /*
- * Hardware CUS232 version 2 has some issues with cold reset and the
- * preferred (and safer) way to perform a device reset is through a
- * warm reset.
- *
- * Warm reset doesn't always work though so fall back to cold reset may
- * be necessary.
- */
- ret = ath10k_pci_hif_power_up_warm(ar);
- if (ret) {
- ath10k_warn(ar, "failed to power up target using warm reset: %d\n",
- ret);
-
- if (ath10k_pci_reset_mode == ATH10K_PCI_RESET_WARM_ONLY)
- return ret;
-
- ath10k_warn(ar, "trying cold reset\n");
-
- ret = __ath10k_pci_hif_power_up(ar, true);
- if (ret) {
- ath10k_err(ar, "failed to power up target using cold reset too (%d)\n",
- ret);
- return ret;
- }
- }
-
- return 0;
-}
-
static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif power down\n");
--
1.8.5.3
On 13 October 2014 10:01, Kalle Valo <[email protected]> wrote:
> Michal Kazior <[email protected]> writes:
>
>> The power up/down didn't make much sense any more
>> since hif_stop already stops the device
>> compeletely. The target lifecycle was never symmetric
>> so don't bother trying to make it look like it is
>> and expose a reset hif callback instead of power
>> up/down callbacks.
>>
>> This removes redundant reset calls and thus makes
>> device boot/stop/recovery a bit faster.
>>
>> Signed-off-by: Michal Kazior <[email protected]>
>
> About this I'm not that sure. The reason why I wanted to have power_up()
> and power_down() is the case when we need to control the target power
> via a gpio line, which I anticipate we will need soon. If you remove
> these how could we control the gpio line?
>
> Wouldn't it be the same that you just make hif_power_up() do the same as
> hif_reset() and hif_power_down() doesn't do anything (for now)? Or am I
> missing something?
Hmm.. I guess that's fine as well. I'll rework it to keep power_up
(which will basically do reset) and power_down (nop for now).
Michał
Michal Kazior <[email protected]> writes:
> The power up procedure was overly complex due to
> warm/cold reset workarounds and issues.
>
> Signed-off-by: Michal Kazior <[email protected]>
[...]
> + ret = ath10k_pci_wait_for_target_init(ar);
> + if (ret) {
> + ath10k_warn(ar, "failed to wait for target after cold reset: %d\n",
> + ret);
Checkpatch warns:
ath10k/pci.c:1849: CHECK: Alignment should match open parenthesis
--
Kalle Valo
The power up/down didn't make much sense any more
since hif_stop already stops the device
compeletely. The target lifecycle was never symmetric
so don't bother trying to make it look like it is
and expose a reset hif callback instead of power
up/down callbacks.
This removes redundant reset calls and thus makes
device boot/stop/recovery a bit faster.
Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 7 +------
drivers/net/wireless/ath/ath10k/hif.h | 22 ++++++----------------
drivers/net/wireless/ath/ath10k/mac.c | 8 ++------
drivers/net/wireless/ath/ath10k/pci.c | 12 ++----------
drivers/net/wireless/ath/ath10k/testmode.c | 8 ++------
5 files changed, 13 insertions(+), 44 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 37e3166..5000348 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -915,7 +915,7 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
struct bmi_target_info target_info;
int ret = 0;
- ret = ath10k_hif_power_up(ar);
+ ret = ath10k_hif_reset(ar);
if (ret) {
ath10k_err(ar, "could not start pci hif (%d)\n", ret);
return ret;
@@ -925,7 +925,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
ret = ath10k_bmi_get_target_info(ar, &target_info);
if (ret) {
ath10k_err(ar, "could not get target info (%d)\n", ret);
- ath10k_hif_power_down(ar);
return ret;
}
@@ -935,14 +934,12 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
ret = ath10k_init_hw_params(ar);
if (ret) {
ath10k_err(ar, "could not get hw params (%d)\n", ret);
- ath10k_hif_power_down(ar);
return ret;
}
ret = ath10k_core_fetch_firmware_files(ar);
if (ret) {
ath10k_err(ar, "could not fetch firmware files (%d)\n", ret);
- ath10k_hif_power_down(ar);
return ret;
}
@@ -952,7 +949,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
if (ret) {
ath10k_err(ar, "could not init core (%d)\n", ret);
ath10k_core_free_firmware_files(ar);
- ath10k_hif_power_down(ar);
mutex_unlock(&ar->conf_mutex);
return ret;
}
@@ -962,7 +958,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
mutex_unlock(&ar->conf_mutex);
- ath10k_hif_power_down(ar);
return 0;
}
diff --git a/drivers/net/wireless/ath/ath10k/hif.h b/drivers/net/wireless/ath/ath10k/hif.h
index 30301f5..26a8bcf 100644
--- a/drivers/net/wireless/ath/ath10k/hif.h
+++ b/drivers/net/wireless/ath/ath10k/hif.h
@@ -56,11 +56,13 @@ struct ath10k_hif_ops {
void *request, u32 request_len,
void *response, u32 *response_len);
+ /* Reset the device and puts it into BMI */
+ int (*reset)(struct ath10k *ar);
+
/* Post BMI phase, after FW is loaded. Starts regular operation */
int (*start)(struct ath10k *ar);
- /* Clean up what start() did. This does not revert to BMI phase. If
- * desired so, call power_down() and power_up() */
+ /* Clean up what start() did. This does not revert to BMI */
void (*stop)(struct ath10k *ar);
int (*map_service_to_pipe)(struct ath10k *ar, u16 service_id,
@@ -84,13 +86,6 @@ struct ath10k_hif_ops {
u16 (*get_free_queue_number)(struct ath10k *ar, u8 pipe_id);
- /* Power up the device and enter BMI transfer mode for FW download */
- int (*power_up)(struct ath10k *ar);
-
- /* Power down the device and free up resources. stop() must be called
- * before this if start() was called earlier */
- void (*power_down)(struct ath10k *ar);
-
int (*suspend)(struct ath10k *ar);
int (*resume)(struct ath10k *ar);
};
@@ -161,14 +156,9 @@ static inline u16 ath10k_hif_get_free_queue_number(struct ath10k *ar,
return ar->hif.ops->get_free_queue_number(ar, pipe_id);
}
-static inline int ath10k_hif_power_up(struct ath10k *ar)
-{
- return ar->hif.ops->power_up(ar);
-}
-
-static inline void ath10k_hif_power_down(struct ath10k *ar)
+static inline int ath10k_hif_reset(struct ath10k *ar)
{
- ar->hif.ops->power_down(ar);
+ return ar->hif.ops->reset(ar);
}
static inline int ath10k_hif_suspend(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 75e0aeb..001ff1a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2392,7 +2392,6 @@ void ath10k_halt(struct ath10k *ar)
ath10k_scan_finish(ar);
ath10k_peer_cleanup_all(ar);
ath10k_core_stop(ar);
- ath10k_hif_power_down(ar);
spin_lock_bh(&ar->data_lock);
list_for_each_entry(arvif, &ar->arvifs, list)
@@ -2495,7 +2494,7 @@ static int ath10k_start(struct ieee80211_hw *hw)
goto err;
}
- ret = ath10k_hif_power_up(ar);
+ ret = ath10k_hif_reset(ar);
if (ret) {
ath10k_err(ar, "Could not init hif: %d\n", ret);
goto err_off;
@@ -2504,7 +2503,7 @@ static int ath10k_start(struct ieee80211_hw *hw)
ret = ath10k_core_start(ar, ATH10K_FIRMWARE_MODE_NORMAL);
if (ret) {
ath10k_err(ar, "Could not init core: %d\n", ret);
- goto err_power_down;
+ goto err_off;
}
ret = ath10k_wmi_pdev_set_param(ar, ar->wmi.pdev_param->pmf_qos, 1);
@@ -2551,9 +2550,6 @@ static int ath10k_start(struct ieee80211_hw *hw)
err_core_stop:
ath10k_core_stop(ar);
-err_power_down:
- ath10k_hif_power_down(ar);
-
err_off:
ar->state = ATH10K_STATE_OFF;
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a63fe7c..78226a6 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1855,7 +1855,7 @@ static int ath10k_pci_chip_reset(struct ath10k *ar)
return 0;
}
-static int ath10k_pci_hif_power_up(struct ath10k *ar)
+static int ath10k_pci_hif_reset(struct ath10k *ar)
{
int ret;
@@ -1904,13 +1904,6 @@ err:
return ret;
}
-static void ath10k_pci_hif_power_down(struct ath10k *ar)
-{
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif power down\n");
-
- ath10k_pci_warm_reset(ar);
-}
-
#ifdef CONFIG_PM
#define ATH10K_PCI_PM_CONTROL 0x44
@@ -1964,6 +1957,7 @@ static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
.tx_sg = ath10k_pci_hif_tx_sg,
.diag_read = ath10k_pci_hif_diag_read,
.exchange_bmi_msg = ath10k_pci_hif_exchange_bmi_msg,
+ .reset = ath10k_pci_hif_reset,
.start = ath10k_pci_hif_start,
.stop = ath10k_pci_hif_stop,
.map_service_to_pipe = ath10k_pci_hif_map_service_to_pipe,
@@ -1971,8 +1965,6 @@ static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
.send_complete_check = ath10k_pci_hif_send_complete_check,
.set_callbacks = ath10k_pci_hif_set_callbacks,
.get_free_queue_number = ath10k_pci_hif_get_free_queue_number,
- .power_up = ath10k_pci_hif_power_up,
- .power_down = ath10k_pci_hif_power_down,
#ifdef CONFIG_PM
.suspend = ath10k_pci_hif_suspend,
.resume = ath10k_pci_hif_resume,
diff --git a/drivers/net/wireless/ath/ath10k/testmode.c b/drivers/net/wireless/ath/ath10k/testmode.c
index 483db9c..0bb3df6 100644
--- a/drivers/net/wireless/ath/ath10k/testmode.c
+++ b/drivers/net/wireless/ath/ath10k/testmode.c
@@ -195,7 +195,7 @@ static int ath10k_tm_cmd_utf_start(struct ath10k *ar, struct nlattr *tb[])
memset(ar->fw_features, 0, sizeof(ar->fw_features));
__set_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features);
- ret = ath10k_hif_power_up(ar);
+ ret = ath10k_hif_reset(ar);
if (ret) {
ath10k_err(ar, "failed to power up hif (testmode): %d\n", ret);
ar->state = ATH10K_STATE_OFF;
@@ -206,7 +206,7 @@ static int ath10k_tm_cmd_utf_start(struct ath10k *ar, struct nlattr *tb[])
if (ret) {
ath10k_err(ar, "failed to start core (testmode): %d\n", ret);
ar->state = ATH10K_STATE_OFF;
- goto err_power_down;
+ goto err_fw_features;
}
ar->state = ATH10K_STATE_UTF;
@@ -217,9 +217,6 @@ static int ath10k_tm_cmd_utf_start(struct ath10k *ar, struct nlattr *tb[])
return 0;
-err_power_down:
- ath10k_hif_power_down(ar);
-
err_fw_features:
/* return the original firmware features */
memcpy(ar->fw_features, ar->testmode.orig_fw_features,
@@ -239,7 +236,6 @@ static void __ath10k_tm_cmd_utf_stop(struct ath10k *ar)
lockdep_assert_held(&ar->conf_mutex);
ath10k_core_stop(ar);
- ath10k_hif_power_down(ar);
spin_lock_bh(&ar->data_lock);
--
1.8.5.3
One of the problems with warm reset I've found is
that it must be guaranteed that copy engine
registers are not being accessed while being
reset. Otherwise in worst case scenario the host
may lock up.
Instead of using sleeps and hoping the device is
operational in some arbitrary timeframes use
firmware indication register.
As a side effect this makes driver
boot/stop/recovery faster.
Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 108 +++++++++++++++-------------------
1 file changed, 46 insertions(+), 62 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index cdc03e4..dad92de 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1708,89 +1708,73 @@ static void ath10k_pci_warm_reset_si0(struct ath10k *ar)
msleep(10);
}
-static int ath10k_pci_warm_reset(struct ath10k *ar)
+static void ath10k_pci_warm_reset_cpu(struct ath10k *ar)
{
u32 val;
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot warm reset\n");
-
- spin_lock_bh(&ar->data_lock);
-
- ar->stats.fw_warm_reset_counter++;
-
- spin_unlock_bh(&ar->data_lock);
-
- /* debug */
- val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
- PCIE_INTR_CAUSE_ADDRESS);
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot host cpu intr cause: 0x%08x\n",
- val);
-
- val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
- CPU_INTR_ADDRESS);
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot target cpu intr cause: 0x%08x\n",
- val);
-
- /* disable pending irqs */
- ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
- PCIE_INTR_ENABLE_ADDRESS, 0);
-
- ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
- PCIE_INTR_CLR_ADDRESS, ~0);
-
- msleep(100);
-
- /* clear fw indicator */
ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS, 0);
- /* clear target LF timer interrupts */
val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
- SOC_LF_TIMER_CONTROL0_ADDRESS);
- ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS +
- SOC_LF_TIMER_CONTROL0_ADDRESS,
- val & ~SOC_LF_TIMER_CONTROL0_ENABLE_MASK);
+ SOC_RESET_CONTROL_ADDRESS);
+ ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS + SOC_RESET_CONTROL_ADDRESS,
+ val | SOC_RESET_CONTROL_CPU_WARM_RST_MASK);
+}
+
+static void ath10k_pci_warm_reset_ce(struct ath10k *ar)
+{
+ u32 val;
- /* reset CE */
val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
SOC_RESET_CONTROL_ADDRESS);
+
ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS + SOC_RESET_CONTROL_ADDRESS,
val | SOC_RESET_CONTROL_CE_RST_MASK);
- val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
- SOC_RESET_CONTROL_ADDRESS);
msleep(10);
-
- /* unreset CE */
ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS + SOC_RESET_CONTROL_ADDRESS,
val & ~SOC_RESET_CONTROL_CE_RST_MASK);
+}
+
+static void ath10k_pci_warm_reset_clear_lf(struct ath10k *ar)
+{
+ u32 val;
+
val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
- SOC_RESET_CONTROL_ADDRESS);
- msleep(10);
+ SOC_LF_TIMER_CONTROL0_ADDRESS);
+ ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS +
+ SOC_LF_TIMER_CONTROL0_ADDRESS,
+ val & ~SOC_LF_TIMER_CONTROL0_ENABLE_MASK);
+}
- ath10k_pci_warm_reset_si0(ar);
+static int ath10k_pci_warm_reset(struct ath10k *ar)
+{
+ int ret;
+
+ ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot warm reset\n");
- /* debug */
- val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
- PCIE_INTR_CAUSE_ADDRESS);
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot host cpu intr cause: 0x%08x\n",
- val);
+ spin_lock_bh(&ar->data_lock);
+ ar->stats.fw_warm_reset_counter++;
+ spin_unlock_bh(&ar->data_lock);
- val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
- CPU_INTR_ADDRESS);
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot target cpu intr cause: 0x%08x\n",
- val);
+ ath10k_pci_irq_disable(ar);
- /* CPU warm reset */
- val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
- SOC_RESET_CONTROL_ADDRESS);
- ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS + SOC_RESET_CONTROL_ADDRESS,
- val | SOC_RESET_CONTROL_CPU_WARM_RST_MASK);
+ /* Make sure the target CPU is not doing anything dangerous, e.g. if it
+ * were to access copy engine while host performs copy engine reset
+ * then it is possible for the device to confuse pci-e controller to
+ * the point of bringing host system to a complete stop (i.e. hang).
+ */
+ ath10k_pci_warm_reset_si0(ar);
+ ath10k_pci_warm_reset_cpu(ar);
+ ath10k_pci_wait_for_target_init(ar);
- val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
- SOC_RESET_CONTROL_ADDRESS);
- ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot target reset state: 0x%08x\n",
- val);
+ ath10k_pci_warm_reset_clear_lf(ar);
+ ath10k_pci_warm_reset_ce(ar);
+ ath10k_pci_warm_reset_cpu(ar);
- msleep(100);
+ ret = ath10k_pci_wait_for_target_init(ar);
+ if (ret) {
+ ath10k_warn(ar, "failed to wait for target init: %d\n", ret);
+ return ret;
+ }
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot warm reset complete\n");
--
1.8.5.3
On 9 October 2014 10:22, Michal Kazior <[email protected]> wrote:
> One of the problems with warm reset I've found is
> that it must be guaranteed that copy engine
> registers are not being accessed while being
> reset. Otherwise in worst case scenario the host
> may lock up.
>
> Instead of using sleeps and hoping the device is
> operational in some arbitrary timeframes use
> firmware indication register.
>
> As a side effect this makes driver
> boot/stop/recovery faster.
>
> Signed-off-by: Michal Kazior <[email protected]>
[...]
> +static int ath10k_pci_warm_reset(struct ath10k *ar)
> +{
> + int ret;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot warm reset\n");
>
> - /* debug */
> - val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
> - PCIE_INTR_CAUSE_ADDRESS);
> - ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot host cpu intr cause: 0x%08x\n",
> - val);
> + spin_lock_bh(&ar->data_lock);
> + ar->stats.fw_warm_reset_counter++;
> + spin_unlock_bh(&ar->data_lock);
>
> - val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
> - CPU_INTR_ADDRESS);
> - ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot target cpu intr cause: 0x%08x\n",
> - val);
> + ath10k_pci_irq_disable(ar);
>
> - /* CPU warm reset */
> - val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
> - SOC_RESET_CONTROL_ADDRESS);
> - ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS + SOC_RESET_CONTROL_ADDRESS,
> - val | SOC_RESET_CONTROL_CPU_WARM_RST_MASK);
> + /* Make sure the target CPU is not doing anything dangerous, e.g. if it
> + * were to access copy engine while host performs copy engine reset
> + * then it is possible for the device to confuse pci-e controller to
> + * the point of bringing host system to a complete stop (i.e. hang).
> + */
> + ath10k_pci_warm_reset_si0(ar);
> + ath10k_pci_warm_reset_cpu(ar);
> + ath10k_pci_wait_for_target_init(ar);
Since warm_reset now calls wait_for_target_init (and thus can report a
target register dump on crash) and is called in pci_probe CE must be
initialized (which isn't the case in the code now). This can lead to
system crash on very early firmware crash during probing. Kudos to
Janusz for finding this out!
I'll respin a v2 later.
Michał
This was the final missing bit to making sure the
device doesn't assert interrupts to host.
This should fix possible race when target crashes
during driver teardown.
Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/pci.c | 25 +++++++++++++++++++++++--
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 5dd6551..6242319 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -289,6 +289,7 @@ struct ath10k_pktlog_hdr {
#define SI_RX_DATA1_OFFSET 0x00000014
#define CORE_CTRL_CPU_INTR_MASK 0x00002000
+#define CORE_CTRL_PCIE_REG_31_MASK 0x00000800
#define CORE_CTRL_ADDRESS 0x0000
#define PCIE_INTR_ENABLE_ADDRESS 0x0008
#define PCIE_INTR_CAUSE_ADDRESS 0x000c
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 6077095..cdc03e4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1143,6 +1143,26 @@ static void ath10k_pci_hif_get_default_pipe(struct ath10k *ar,
&dl_is_polled);
}
+static void ath10k_pci_irq_msi_fw_mask(struct ath10k *ar)
+{
+ u32 val;
+
+ val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS + CORE_CTRL_ADDRESS);
+ val &= ~CORE_CTRL_PCIE_REG_31_MASK;
+
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + CORE_CTRL_ADDRESS, val);
+}
+
+static void ath10k_pci_irq_msi_fw_unmask(struct ath10k *ar)
+{
+ u32 val;
+
+ val = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS + CORE_CTRL_ADDRESS);
+ val |= CORE_CTRL_PCIE_REG_31_MASK;
+
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + CORE_CTRL_ADDRESS, val);
+}
+
static void ath10k_pci_irq_disable(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -1150,7 +1170,7 @@ static void ath10k_pci_irq_disable(struct ath10k *ar)
ath10k_ce_disable_interrupts(ar);
ath10k_pci_disable_and_clear_legacy_irq(ar);
- /* FIXME: How to mask all MSI interrupts? */
+ ath10k_pci_irq_msi_fw_mask(ar);
for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
synchronize_irq(ar_pci->pdev->irq + i);
@@ -1160,7 +1180,7 @@ static void ath10k_pci_irq_enable(struct ath10k *ar)
{
ath10k_ce_enable_interrupts(ar);
ath10k_pci_enable_legacy_irq(ar);
- /* FIXME: How to unmask all MSI interrupts? */
+ ath10k_pci_irq_msi_fw_unmask(ar);
}
static int ath10k_pci_hif_start(struct ath10k *ar)
@@ -2285,6 +2305,7 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
} while (time_before(jiffies, timeout));
ath10k_pci_disable_and_clear_legacy_irq(ar);
+ ath10k_pci_irq_msi_fw_mask(ar);
if (val == 0xffffffff) {
ath10k_err(ar, "failed to read device register, device is gone\n");
--
1.8.5.3
If MSI isn't configured device ROM program expects
legacy interrupts to be enabled before it can
fully boot. Don't forget to disable legacy
interrupts after that.
While at it re-use the legacy irq enabling helper
instead of calling ath10k_pci_write32().
Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 730bb18..6077095 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2279,14 +2279,13 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
if (ar_pci->num_msi_intrs == 0)
/* Fix potential race by repeating CORE_BASE writes */
- ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
- PCIE_INTR_ENABLE_ADDRESS,
- PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL);
+ ath10k_pci_enable_legacy_irq(ar);
mdelay(10);
} while (time_before(jiffies, timeout));
+ ath10k_pci_disable_and_clear_legacy_irq(ar);
+
if (val == 0xffffffff) {
ath10k_err(ar, "failed to read device register, device is gone\n");
return -EIO;
--
1.8.5.3