While looking at the cold reset problem I did some cleanup
to pci.c.
---
Kalle Valo (8):
ath10k: cleanup ath10k_pci_wait_for_target_init()
ath10k: add module parameter to disable cold reset
ath10k: fix name of target_ps module parameter
ath10k: advertise only firmware API 2 files
ath10k: delete ar_pci->fw_indicator_address
ath10k: improve pci debug messages
ath10k: add module parameter values to the pci info print
ath10k: print chip id during boot
drivers/net/wireless/ath/ath10k/core.c | 9 ++
drivers/net/wireless/ath/ath10k/hw.h | 1
drivers/net/wireless/ath/ath10k/pci.c | 125 ++++++++++++++++++++++----------
drivers/net/wireless/ath/ath10k/pci.h | 3 -
4 files changed, 92 insertions(+), 46 deletions(-)
ath10k_pci_wait_for_target_init() did really follow the style used elsewhere in
ath10k. Use ath10k_pci_read/write() wrappers, simplify the while loop and
improve warning messages.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 9d242d801d9d..0425c76daf3f 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2385,30 +2385,37 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar)
static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int wait_limit = 300; /* 3 sec */
+ const int wait = 3000; /* ms */
+ unsigned long timeout;
int ret;
+ u32 val;
ret = ath10k_pci_wake(ar);
if (ret) {
- ath10k_err("failed to wake up target: %d\n", ret);
+ ath10k_err("failed to wake up target for init: %d\n", ret);
return ret;
}
- while (wait_limit-- &&
- !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) &
- FW_IND_INITIALIZED)) {
+ timeout = jiffies + msecs_to_jiffies(wait);
+
+ do {
+ val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
+ if (val == FW_IND_INITIALIZED)
+ break;
+
if (ar_pci->num_msi_intrs == 0)
/* Fix potential race by repeating CORE_BASE writes */
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
+ ath10k_pci_soc_write32(ar, PCIE_INTR_ENABLE_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK |
+ PCIE_INTR_CE_MASK_ALL);
+
mdelay(10);
- }
+ } while (time_before(jiffies, timeout));
- if (wait_limit < 0) {
- ath10k_err("target stalled\n");
- ret = -EIO;
+ if (val != FW_IND_INITIALIZED) {
+ ath10k_err("failed to receive initialized event from target after %d ms: %d\n",
+ wait, val);
+ ret = -ETIMEDOUT;
goto out;
}
On 26 March 2014 10:28, Kalle Valo <[email protected]> wrote:
> Michal Kazior <[email protected]> writes:
[...]
>>> - while (wait_limit-- &&
>>> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) &
>>> - FW_IND_INITIALIZED)) {
>>> + timeout = jiffies + msecs_to_jiffies(wait);
>>> +
>>> + do {
>>> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>>> + if (val == FW_IND_INITIALIZED)
>>> + break;
Ah, for some reason I've missed this earlier. You seem to replace &
with ==. I wonder if that's okay?
FW indicator may contain FW_IND_EVENT_PENDING (i.e. firmware crashed
bit) too. It might be a good idea to check for that too and return a
different errno?
>>> +
>>
>> It might be worth to add:
>>
>> if (val == 0xFFFFFFFF)
>> return -EIO;
>
> What does receiving 0xFFFFFFFF mean here? PCI bus kaput?
A simple grepping (find drivers/net/ethernet/ -type f -name '*.[ch]' |
xargs grep -nC5 ioread32 | grep -C5 -i 0xffffffff) turns out results
like this:
drivers/net/ethernet/cisco/enic/vnic_rq.c-200- }
drivers/net/ethernet/cisco/enic/vnic_rq.c-201-
drivers/net/ethernet/cisco/enic/vnic_rq.c-202- /* Use current
fetch_index as the ring starting point */
drivers/net/ethernet/cisco/enic/vnic_rq.c:203: fetch_index =
ioread32(&rq->ctrl->fetch_index);
drivers/net/ethernet/cisco/enic/vnic_rq.c-204-
drivers/net/ethernet/cisco/enic/vnic_rq.c-205- if (fetch_index ==
0xFFFFFFFF) { /* check for hardware gone */
drivers/net/ethernet/cisco/enic/vnic_rq.c-206- /* Hardware
surprise removal: reset fetch_index */
drivers/net/ethernet/cisco/enic/vnic_rq.c-207- fetch_index = 0;
drivers/net/ethernet/cisco/enic/vnic_rq.c-208- }
> Do we really want to stop trying after receiving that? What harm would
> it cause to keep on trying? We would return an error anyway after the
> timeout, right?
Yes. It's harmless but having the check allows to discern a real
timeout (target doesn't wake up for some reason) and a pci-e link
issue.
Michał
It always contains the same constant, no need to have a separate variable for it.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 16 ++++++----------
drivers/net/wireless/ath/ath10k/pci.h | 3 ---
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index e70dde1beaca..0d8efba160dd 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1750,16 +1750,15 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- u32 fw_indicator_address, fw_indicator;
+ u32 fw_indicator;
ath10k_pci_wake(ar);
- fw_indicator_address = ar_pci->fw_indicator_address;
- fw_indicator = ath10k_pci_read32(ar, fw_indicator_address);
+ fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
if (fw_indicator & FW_IND_EVENT_PENDING) {
/* ACK: clear Target-side pending event */
- ath10k_pci_write32(ar, fw_indicator_address,
+ ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
fw_indicator & ~FW_IND_EVENT_PENDING);
if (ar_pci->started) {
@@ -1778,7 +1777,6 @@ static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
static int ath10k_pci_warm_reset(struct ath10k *ar)
{
- struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret = 0;
u32 val;
@@ -1810,7 +1808,7 @@ static int ath10k_pci_warm_reset(struct ath10k *ar)
msleep(100);
/* clear fw indicator */
- ath10k_pci_write32(ar, ar_pci->fw_indicator_address, 0);
+ ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS, 0);
/* clear target LF timer interrupts */
val = ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS +
@@ -2151,7 +2149,6 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
static void ath10k_pci_early_irq_tasklet(unsigned long data)
{
struct ath10k *ar = (struct ath10k *)data;
- struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
u32 fw_ind;
int ret;
@@ -2162,9 +2159,9 @@ static void ath10k_pci_early_irq_tasklet(unsigned long data)
return;
}
- fw_ind = ath10k_pci_read32(ar, ar_pci->fw_indicator_address);
+ fw_ind = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
if (fw_ind & FW_IND_EVENT_PENDING) {
- ath10k_pci_write32(ar, ar_pci->fw_indicator_address,
+ ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
fw_ind & ~FW_IND_EVENT_PENDING);
/* Some structures are unavailable during early boot or at
@@ -2537,7 +2534,6 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
}
ar_pci->ar = ar;
- ar_pci->fw_indicator_address = FW_INDICATOR_ADDRESS;
atomic_set(&ar_pci->keep_awake_count, 0);
pci_set_drvdata(pdev, ar);
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index b43fdb4f7319..dfdebb4157aa 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -189,9 +189,6 @@ struct ath10k_pci {
struct ath10k_hif_cb msg_callbacks_current;
- /* Target address used to signal a pending firmware event */
- u32 fw_indicator_address;
-
/* Copy Engine used for Diagnostic Accesses */
struct ath10k_ce_pipe *ce_diag;
As cold reset is not reliable with CUS223 boards, make it possible
to disable cold reset entirely and only use warm reset. This makes it also
easier to debug warm reset problems.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 0425c76daf3f..d968aedc4863 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -39,8 +39,14 @@ enum ath10k_pci_irq_mode {
ATH10K_PCI_IRQ_MSI = 2,
};
+enum ath10k_pci_reset_mode {
+ ATH10K_PCI_RESET_AUTO = 0,
+ ATH10K_PCI_RESET_WARM_ONLY = 1,
+};
+
static unsigned int ath10k_target_ps;
static unsigned int ath10k_pci_irq_mode = ATH10K_PCI_IRQ_AUTO;
+static unsigned int ath10k_pci_reset_mode = ATH10K_PCI_RESET_AUTO;
module_param(ath10k_target_ps, uint, 0644);
MODULE_PARM_DESC(ath10k_target_ps, "Enable ath10k Target (SoC) PS option");
@@ -48,6 +54,9 @@ MODULE_PARM_DESC(ath10k_target_ps, "Enable ath10k Target (SoC) PS option");
module_param_named(irq_mode, ath10k_pci_irq_mode, uint, 0644);
MODULE_PARM_DESC(irq_mode, "0: auto, 1: legacy, 2: msi (default: 0)");
+module_param_named(reset_mode, ath10k_pci_reset_mode, uint, 0644);
+MODULE_PARM_DESC(reset_mode, "0: auto, 1: warm only (default: 0)");
+
#define QCA988X_2_0_DEVICE_ID (0x003c)
static DEFINE_PCI_DEVICE_TABLE(ath10k_pci_id_table) = {
@@ -1966,9 +1975,14 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
*/
ret = __ath10k_pci_hif_power_up(ar, false);
if (ret) {
- ath10k_warn("failed to power up target using warm reset (%d), trying cold reset\n",
+ ath10k_warn("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("trying cold reset\n");
+
ret = __ath10k_pci_hif_power_up(ar, true);
if (ret) {
ath10k_err("failed to power up target using cold reset too (%d)\n",
This makes it easier to debug what kind of board is used.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 471c219e5137..0077ba17e5c0 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -844,9 +844,12 @@ int ath10k_core_start(struct ath10k *ar)
INIT_LIST_HEAD(&ar->arvifs);
if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
- ath10k_info("%s (0x%x) fw %s api %d htt %d.%d\n",
- ar->hw_params.name, ar->target_version,
- ar->hw->wiphy->fw_version, ar->fw_api,
+ ath10k_info("%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
+ ar->hw_params.name,
+ ar->target_version,
+ ar->chip_id,
+ ar->hw->wiphy->fw_version,
+ ar->fw_api,
ar->htt.target_version_major,
ar->htt.target_version_minor);
Hopefully this makes it easier to debug problems in the future.
Signed-off-by: Kalle Valo <[email protected]>
---
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 5842fa51f9c5..783cfa035b11 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1952,7 +1952,9 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
irq_mode = "legacy";
if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
- ath10k_info("pci irq %s\n", irq_mode);
+ ath10k_info("pci irq %s irq_mode %d reset_mode %d\n",
+ irq_mode, ath10k_pci_irq_mode,
+ ath10k_pci_reset_mode);
return 0;
Michal Kazior <[email protected]> writes:
> On 25 March 2014 10:15, Kalle Valo <[email protected]> wrote:
>> ath10k_pci_wait_for_target_init() did really follow the style used elsewhere in
>> ath10k. Use ath10k_pci_read/write() wrappers, simplify the while loop and
>> improve warning messages.
>>
>> Signed-off-by: Kalle Valo <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/pci.c | 33 ++++++++++++++++++++-------------
>> 1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index 9d242d801d9d..0425c76daf3f 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -2385,30 +2385,37 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar)
>> static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
>> {
>> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>> - int wait_limit = 300; /* 3 sec */
>> + const int wait = 3000; /* ms */
>
> We probably should have a #define for this.
Yeah, I'll change that.
>> - while (wait_limit-- &&
>> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) &
>> - FW_IND_INITIALIZED)) {
>> + timeout = jiffies + msecs_to_jiffies(wait);
>> +
>> + do {
>> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>> + if (val == FW_IND_INITIALIZED)
>> + break;
>> +
>
> It might be worth to add:
>
> if (val == 0xFFFFFFFF)
> return -EIO;
What does receiving 0xFFFFFFFF mean here? PCI bus kaput?
Do we really want to stop trying after receiving that? What harm would
it cause to keep on trying? We would return an error anyway after the
timeout, right?
>> + } while (time_before(jiffies, timeout));
>>
>> - if (wait_limit < 0) {
>> - ath10k_err("target stalled\n");
>> - ret = -EIO;
>> + if (val != FW_IND_INITIALIZED) {
>> + ath10k_err("failed to receive initialized event from target after %d ms: %d\n",
>> + wait, val);
>
> `val` is u32 so it shouldn't be %d. %08x makes most sense I guess.
I'll change it.
Thanks for the review!
--
Kalle Valo
The parameter name was ath10k_target_ps, but actually it should be just
target_ps. Module parameter names should not use the ath10k_ prefix.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index d968aedc4863..2c5a83d2748a 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -44,12 +44,12 @@ enum ath10k_pci_reset_mode {
ATH10K_PCI_RESET_WARM_ONLY = 1,
};
-static unsigned int ath10k_target_ps;
+static unsigned int ath10k_pci_target_ps;
static unsigned int ath10k_pci_irq_mode = ATH10K_PCI_IRQ_AUTO;
static unsigned int ath10k_pci_reset_mode = ATH10K_PCI_RESET_AUTO;
-module_param(ath10k_target_ps, uint, 0644);
-MODULE_PARM_DESC(ath10k_target_ps, "Enable ath10k Target (SoC) PS option");
+module_param_named(target_ps, ath10k_pci_target_ps, uint, 0644);
+MODULE_PARM_DESC(target_ps, "Enable ath10k Target (SoC) PS option");
module_param_named(irq_mode, ath10k_pci_irq_mode, uint, 0644);
MODULE_PARM_DESC(irq_mode, "0: auto, 1: legacy, 2: msi (default: 0)");
@@ -2524,7 +2524,7 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
goto err_ar_pci;
}
- if (ath10k_target_ps)
+ if (ath10k_pci_target_ps)
set_bit(ATH10K_PCI_FEATURE_SOC_POWER_SAVE, ar_pci->features);
ath10k_pci_dump_features(ar_pci);
To make it easier to debug pci problems improve the log messages in pci.c. Also
change some debug messages to warning messages to more easily catch problems.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 45 ++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 0d8efba160dd..5842fa51f9c5 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -451,8 +451,8 @@ done:
__le32_to_cpu(((__le32 *)data_buf)[i]);
}
} else
- ath10k_dbg(ATH10K_DBG_PCI, "%s failure (0x%x)\n",
- __func__, address);
+ ath10k_warn("failed to read diag value at 0x%x: %d\n",
+ address, ret);
if (data_buf)
pci_free_consistent(ar_pci->pdev, orig_nbytes,
@@ -602,8 +602,8 @@ done:
}
if (ret != 0)
- ath10k_dbg(ATH10K_DBG_PCI, "%s failure (0x%x)\n", __func__,
- address);
+ ath10k_warn("failed to write diag value at 0x%x: %d\n",
+ address, ret);
return ret;
}
@@ -812,6 +812,9 @@ unlock:
static u16 ath10k_pci_hif_get_free_queue_number(struct ath10k *ar, u8 pipe)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+
+ ath10k_dbg(ATH10K_DBG_PCI, "pci hif get free queue number\n");
+
return ath10k_ce_num_free_src_entries(ar_pci->pipe_info[pipe].ce_hdl);
}
@@ -863,6 +866,8 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
static void ath10k_pci_hif_send_complete_check(struct ath10k *ar, u8 pipe,
int force)
{
+ ath10k_dbg(ATH10K_DBG_PCI, "pci hif send complete check\n");
+
if (!force) {
int resources;
/*
@@ -889,7 +894,7 @@ static void ath10k_pci_hif_set_callbacks(struct ath10k *ar,
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);
+ ath10k_dbg(ATH10K_DBG_PCI, "pci hif set callbacks\n");
memcpy(&ar_pci->msg_callbacks_current, callbacks,
sizeof(ar_pci->msg_callbacks_current));
@@ -947,6 +952,8 @@ static int ath10k_pci_hif_map_service_to_pipe(struct ath10k *ar,
{
int ret = 0;
+ ath10k_dbg(ATH10K_DBG_PCI, "pci hif map service\n");
+
/* polling for received messages not supported */
*dl_is_polled = 0;
@@ -1006,6 +1013,8 @@ static void ath10k_pci_hif_get_default_pipe(struct ath10k *ar,
{
int ul_is_polled, dl_is_polled;
+ ath10k_dbg(ATH10K_DBG_PCI, "pci hif get default pipe\n");
+
(void)ath10k_pci_hif_map_service_to_pipe(ar,
ATH10K_HTC_SVC_ID_RSVD_CTRL,
ul_pipe,
@@ -1107,6 +1116,8 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret, ret_early;
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
+
ath10k_pci_free_early_irq(ar);
ath10k_pci_kill_tasklet(ar);
@@ -1261,7 +1272,7 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;
- ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot hif stop\n");
ret = ath10k_ce_disable_interrupts(ar);
if (ret)
@@ -1780,7 +1791,7 @@ static int ath10k_pci_warm_reset(struct ath10k *ar)
int ret = 0;
u32 val;
- ath10k_dbg(ATH10K_DBG_BOOT, "boot performing warm chip reset\n");
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot warm reset\n");
ret = ath10k_do_pci_wake(ar);
if (ret) {
@@ -1963,6 +1974,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
{
int ret;
+ ath10k_dbg(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
@@ -1996,6 +2009,8 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot hif power down\n");
+
ath10k_pci_free_early_irq(ar);
ath10k_pci_kill_tasklet(ar);
ath10k_pci_deinit_irq(ar);
@@ -2401,6 +2416,8 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
int ret;
u32 val;
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot waiting target to initialise\n");
+
ret = ath10k_pci_wake(ar);
if (ret) {
ath10k_err("failed to wake up target for init: %d\n", ret);
@@ -2411,6 +2428,9 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
do {
val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
+
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot target indicator %x\n", val);
+
if (val == FW_IND_INITIALIZED)
break;
@@ -2430,6 +2450,8 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
goto out;
}
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot target initialised\n");
+
out:
ath10k_pci_sleep(ar);
return ret;
@@ -2440,6 +2462,8 @@ static int ath10k_pci_cold_reset(struct ath10k *ar)
int i, ret;
u32 val;
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot cold reset\n");
+
ret = ath10k_do_pci_wake(ar);
if (ret) {
ath10k_err("failed to wake up target: %d\n",
@@ -2471,6 +2495,9 @@ static int ath10k_pci_cold_reset(struct ath10k *ar)
}
ath10k_do_pci_sleep(ar);
+
+ ath10k_dbg(ATH10K_DBG_BOOT, "boot cold reset complete\n");
+
return 0;
}
@@ -2502,7 +2529,7 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
struct ath10k_pci *ar_pci;
u32 lcr_val, chip_id;
- ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);
+ ath10k_dbg(ATH10K_DBG_PCI, "pci probe\n");
ar_pci = kzalloc(sizeof(*ar_pci), GFP_KERNEL);
if (ar_pci == NULL)
@@ -2643,7 +2670,7 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
struct ath10k *ar = pci_get_drvdata(pdev);
struct ath10k_pci *ar_pci;
- ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);
+ ath10k_dbg(ATH10K_DBG_PCI, "pci remove\n");
if (!ar)
return;
Michal Kazior <[email protected]> writes:
> On 26 March 2014 10:28, Kalle Valo <[email protected]> wrote:
>> Michal Kazior <[email protected]> writes:
>
> [...]
>
>>>> - while (wait_limit-- &&
>>>> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) &
>>>> - FW_IND_INITIALIZED)) {
>>>> + timeout = jiffies + msecs_to_jiffies(wait);
>>>> +
>>>> + do {
>>>> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>>>> + if (val == FW_IND_INITIALIZED)
>>>> + break;
>
> Ah, for some reason I've missed this earlier. You seem to replace &
> with ==. I wonder if that's okay?
No, it's not. Good catch, I'll change it back to '&'.
> FW indicator may contain FW_IND_EVENT_PENDING (i.e. firmware crashed
> bit) too.
I'm guessing that FW_IND_INITIALIZED should be the only bit set if
initialisation has happened correctly, but we cannot be certain (right
now). Hence I'll change it back to '&'.
> It might be a good idea to check for that too and return a different
> errno?
Maybe. But to keep this patch simple, I don't add that right now. We can
create a new patch for that.
>>> It might be worth to add:
>>>
>>> if (val == 0xFFFFFFFF)
>>> return -EIO;
>>
>> What does receiving 0xFFFFFFFF mean here? PCI bus kaput?
>
> A simple grepping (find drivers/net/ethernet/ -type f -name '*.[ch]' |
> xargs grep -nC5 ioread32 | grep -C5 -i 0xffffffff) turns out results
> like this:
>
> drivers/net/ethernet/cisco/enic/vnic_rq.c-200- }
> drivers/net/ethernet/cisco/enic/vnic_rq.c-201-
> drivers/net/ethernet/cisco/enic/vnic_rq.c-202- /* Use current
> fetch_index as the ring starting point */
> drivers/net/ethernet/cisco/enic/vnic_rq.c:203: fetch_index =
> ioread32(&rq->ctrl->fetch_index);
> drivers/net/ethernet/cisco/enic/vnic_rq.c-204-
> drivers/net/ethernet/cisco/enic/vnic_rq.c-205- if (fetch_index ==
> 0xFFFFFFFF) { /* check for hardware gone */
> drivers/net/ethernet/cisco/enic/vnic_rq.c-206- /* Hardware
> surprise removal: reset fetch_index */
> drivers/net/ethernet/cisco/enic/vnic_rq.c-207- fetch_index = 0;
> drivers/net/ethernet/cisco/enic/vnic_rq.c-208- }
>
>
>> Do we really want to stop trying after receiving that? What harm would
>> it cause to keep on trying? We would return an error anyway after the
>> timeout, right?
>
> Yes. It's harmless but having the check allows to discern a real
> timeout (target doesn't wake up for some reason) and a pci-e link
> issue.
Ok, I'll add the test you suggested.
--
Kalle Valo
On 25 March 2014 10:15, Kalle Valo <[email protected]> wrote:
> ath10k_pci_wait_for_target_init() did really follow the style used elsewhere in
> ath10k. Use ath10k_pci_read/write() wrappers, simplify the while loop and
> improve warning messages.
>
> Signed-off-by: Kalle Valo <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/pci.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 9d242d801d9d..0425c76daf3f 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2385,30 +2385,37 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar)
> static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
> {
> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> - int wait_limit = 300; /* 3 sec */
> + const int wait = 3000; /* ms */
We probably should have a #define for this.
> + unsigned long timeout;
> int ret;
> + u32 val;
>
> ret = ath10k_pci_wake(ar);
> if (ret) {
> - ath10k_err("failed to wake up target: %d\n", ret);
> + ath10k_err("failed to wake up target for init: %d\n", ret);
> return ret;
> }
>
> - while (wait_limit-- &&
> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) &
> - FW_IND_INITIALIZED)) {
> + timeout = jiffies + msecs_to_jiffies(wait);
> +
> + do {
> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
> + if (val == FW_IND_INITIALIZED)
> + break;
> +
It might be worth to add:
if (val == 0xFFFFFFFF)
return -EIO;
> if (ar_pci->num_msi_intrs == 0)
> /* Fix potential race by repeating CORE_BASE writes */
> - iowrite32(PCIE_INTR_FIRMWARE_MASK |
> - PCIE_INTR_CE_MASK_ALL,
> - ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
> - PCIE_INTR_ENABLE_ADDRESS));
> + ath10k_pci_soc_write32(ar, PCIE_INTR_ENABLE_ADDRESS,
> + PCIE_INTR_FIRMWARE_MASK |
> + PCIE_INTR_CE_MASK_ALL);
> +
> mdelay(10);
> - }
> + } while (time_before(jiffies, timeout));
>
> - if (wait_limit < 0) {
> - ath10k_err("target stalled\n");
> - ret = -EIO;
> + if (val != FW_IND_INITIALIZED) {
> + ath10k_err("failed to receive initialized event from target after %d ms: %d\n",
> + wait, val);
`val` is u32 so it shouldn't be %d. %08x makes most sense I guess.
Michał
We do not really support older firmware API 1 anymore, so better remove
MODULE_FIRMWARE() declarations for them and only list for API 2 files.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/pci.c | 3 +--
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 35fc44e281f5..007e855f4ba9 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -28,6 +28,7 @@
#define QCA988X_HW_2_0_CHIP_ID_REV 0x2
#define QCA988X_HW_2_0_FW_DIR "ath10k/QCA988X/hw2.0"
#define QCA988X_HW_2_0_FW_FILE "firmware.bin"
+#define QCA988X_HW_2_0_FW_2_FILE "firmware-2.bin"
#define QCA988X_HW_2_0_OTP_FILE "otp.bin"
#define QCA988X_HW_2_0_BOARD_DATA_FILE "board.bin"
#define QCA988X_HW_2_0_PATCH_LOAD_ADDR 0x1234
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2c5a83d2748a..e70dde1beaca 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2701,6 +2701,5 @@ module_exit(ath10k_pci_exit);
MODULE_AUTHOR("Qualcomm Atheros");
MODULE_DESCRIPTION("Driver support for Atheros QCA988X PCIe devices");
MODULE_LICENSE("Dual BSD/GPL");
-MODULE_FIRMWARE(QCA988X_HW_2_0_FW_DIR "/" QCA988X_HW_2_0_FW_FILE);
-MODULE_FIRMWARE(QCA988X_HW_2_0_FW_DIR "/" QCA988X_HW_2_0_OTP_FILE);
+MODULE_FIRMWARE(QCA988X_HW_2_0_FW_DIR "/" QCA988X_HW_2_0_FW_2_FILE);
MODULE_FIRMWARE(QCA988X_HW_2_0_FW_DIR "/" QCA988X_HW_2_0_BOARD_DATA_FILE);