Hi,
This is a partial re-spin which includes the
not-yet-applied patches that fix/change warm
reset.
v3:
* change CE ring cleanup (new patch #1)
Michal Kazior (4):
ath10k: change ce ring cleanup logic
ath10k: make warm reset a bit safer and faster
ath10k: split reset logic from power up
ath10k: don't reset chip on power_down
drivers/net/wireless/ath/ath10k/ce.c | 6 -
drivers/net/wireless/ath/ath10k/pci.c | 345 +++++++++++++++++-----------------
2 files changed, 176 insertions(+), 175 deletions(-)
--
1.8.5.3
Michal Kazior <[email protected]> writes:
> This is a partial re-spin which includes the
> not-yet-applied patches that fix/change warm
> reset.
>
> v3:
> * change CE ring cleanup (new patch #1)
>
>
> Michal Kazior (4):
> ath10k: change ce ring cleanup logic
> ath10k: make warm reset a bit safer and faster
> ath10k: split reset logic from power up
> ath10k: don't reset chip on power_down
Thanks, all four patches applied.
--
Kalle Valo
The power up procedure was overly complex due to
warm/cold reset workarounds and issues.
Signed-off-by: Michal Kazior <[email protected]>
---
Notes:
v2:
* fix indentation [Kalle]
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 af36730..117e14d 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1792,10 +1792,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_init_pipes(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.
*
@@ -1806,13 +1882,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;
}
@@ -1822,12 +1894,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);
@@ -1844,68 +1910,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
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]>
---
Notes:
v2:
* fix kernel panic on early fw crash due to CE not being fully initialized [Janusz]
drivers/net/wireless/ath/ath10k/pci.c | 110 +++++++++++++++-------------------
1 file changed, 48 insertions(+), 62 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a8a3e1b..af36730 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1717,89 +1717,75 @@ 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_init_pipes(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);
+ ath10k_pci_init_pipes(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
Make ath10k_pci_init_pipes() effectively only
alter shared target-host data.
The per_transfer_context is a host-only thing.
It is necessary to preserve it's contents for a
more robust ring cleanup.
This is required for future warm reset fixes.
Signed-off-by: Michal Kazior <[email protected]>
---
Notes:
v3:
* introduced to fix memleak - warm reset will
reinit rings/pipes in the future so it was
necessary to rework how clean up of CE rings is
performed
drivers/net/wireless/ath/ath10k/ce.c | 6 ---
drivers/net/wireless/ath/ath10k/pci.c | 82 ++++++++++++++++++++---------------
2 files changed, 46 insertions(+), 42 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 9b89ac1..878e1ec 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -835,9 +835,6 @@ static int ath10k_ce_init_src_ring(struct ath10k *ar,
nentries = roundup_pow_of_two(attr->src_nentries);
- memset(src_ring->per_transfer_context, 0,
- nentries * sizeof(*src_ring->per_transfer_context));
-
src_ring->sw_index = ath10k_ce_src_ring_read_index_get(ar, ctrl_addr);
src_ring->sw_index &= src_ring->nentries_mask;
src_ring->hw_index = src_ring->sw_index;
@@ -872,9 +869,6 @@ static int ath10k_ce_init_dest_ring(struct ath10k *ar,
nentries = roundup_pow_of_two(attr->dest_nentries);
- memset(dest_ring->per_transfer_context, 0,
- nentries * sizeof(*dest_ring->per_transfer_context));
-
dest_ring->sw_index = ath10k_ce_dest_ring_read_index_get(ar, ctrl_addr);
dest_ring->sw_index &= dest_ring->nentries_mask;
dest_ring->write_index =
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 4a4740b..a8a3e1b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1196,64 +1196,74 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
return 0;
}
-static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
+static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pci_pipe)
{
struct ath10k *ar;
- struct ath10k_pci *ar_pci;
- struct ath10k_ce_pipe *ce_hdl;
- u32 buf_sz;
- struct sk_buff *netbuf;
- u32 ce_data;
+ struct ath10k_ce_pipe *ce_pipe;
+ struct ath10k_ce_ring *ce_ring;
+ struct sk_buff *skb;
+ int i;
- buf_sz = pipe_info->buf_sz;
+ ar = pci_pipe->hif_ce_state;
+ ce_pipe = pci_pipe->ce_hdl;
+ ce_ring = ce_pipe->dest_ring;
- /* Unused Copy Engine */
- if (buf_sz == 0)
+ if (!ce_ring)
return;
- ar = pipe_info->hif_ce_state;
- ar_pci = ath10k_pci_priv(ar);
- ce_hdl = pipe_info->ce_hdl;
+ if (!pci_pipe->buf_sz)
+ return;
- while (ath10k_ce_revoke_recv_next(ce_hdl, (void **)&netbuf,
- &ce_data) == 0) {
- dma_unmap_single(ar->dev, ATH10K_SKB_CB(netbuf)->paddr,
- netbuf->len + skb_tailroom(netbuf),
+ for (i = 0; i < ce_ring->nentries; i++) {
+ skb = ce_ring->per_transfer_context[i];
+ if (!skb)
+ continue;
+
+ ce_ring->per_transfer_context[i] = NULL;
+
+ dma_unmap_single(ar->dev, ATH10K_SKB_CB(skb)->paddr,
+ skb->len + skb_tailroom(skb),
DMA_FROM_DEVICE);
- dev_kfree_skb_any(netbuf);
+ dev_kfree_skb_any(skb);
}
}
-static void ath10k_pci_tx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
+static void ath10k_pci_tx_pipe_cleanup(struct ath10k_pci_pipe *pci_pipe)
{
struct ath10k *ar;
struct ath10k_pci *ar_pci;
- struct ath10k_ce_pipe *ce_hdl;
- struct sk_buff *netbuf;
- u32 ce_data;
- unsigned int nbytes;
+ struct ath10k_ce_pipe *ce_pipe;
+ struct ath10k_ce_ring *ce_ring;
+ struct ce_desc *ce_desc;
+ struct sk_buff *skb;
unsigned int id;
- u32 buf_sz;
+ int i;
- buf_sz = pipe_info->buf_sz;
+ ar = pci_pipe->hif_ce_state;
+ ar_pci = ath10k_pci_priv(ar);
+ ce_pipe = pci_pipe->ce_hdl;
+ ce_ring = ce_pipe->src_ring;
- /* Unused Copy Engine */
- if (buf_sz == 0)
+ if (!ce_ring)
return;
- ar = pipe_info->hif_ce_state;
- ar_pci = ath10k_pci_priv(ar);
- ce_hdl = pipe_info->ce_hdl;
+ if (!pci_pipe->buf_sz)
+ return;
- while (ath10k_ce_cancel_send_next(ce_hdl, (void **)&netbuf,
- &ce_data, &nbytes, &id) == 0) {
- /* no need to call tx completion for NULL pointers */
- if (!netbuf)
+ ce_desc = ce_ring->shadow_base;
+ if (WARN_ON(!ce_desc))
+ return;
+
+ for (i = 0; i < ce_ring->nentries; i++) {
+ skb = ce_ring->per_transfer_context[i];
+ if (!skb)
continue;
- ar_pci->msg_callbacks_current.tx_completion(ar,
- netbuf,
- id);
+ ce_ring->per_transfer_context[i] = NULL;
+ id = MS(__le16_to_cpu(ce_desc[i].flags),
+ CE_DESC_FLAGS_META_DATA);
+
+ ar_pci->msg_callbacks_current.tx_completion(ar, skb, id);
}
}
--
1.8.5.3
Currently hif_power_up performs effectively a
reset and hif_stop resets the chip as well so
there's no point in resetting here.
Signed-off-by: Michal Kazior <[email protected]>
---
Notes:
v2:
* this patch replaces `replace power up/down with reset callback` [Kalle]
drivers/net/wireless/ath/ath10k/pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 117e14d..63f374ed 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1919,7 +1919,9 @@ 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);
+ /* Currently hif_power_up performs effectively a reset and hif_stop
+ * resets the chip as well so there's no point in resetting here.
+ */
}
#ifdef CONFIG_PM
--
1.8.5.3