LinuxLists.cc - [PATCH 00/11] SDIO support for ath10k

[permalink] [raw]

Subject: [PATCH 09/11] ath10k_sdio: virtual scatter gather for receive

From: Alagu Sankar <[email protected]>

The existing implementation of initiating multiple sdio transfers for
receive bundling is slowing down the receive speed. Combining the
transfers using a scatter gather method would be ideal. This results in
significant performance improvement.

Since the sg implementation for sdio transfers are not reliable due to
buffer start and size alignment, a virtual scatter gather implementation
is used.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/htc.h | 1 +
drivers/net/wireless/ath/ath10k/sdio.c | 122 ++++++++++++++++++++++++---------
drivers/net/wireless/ath/ath10k/sdio.h | 5 +-
3 files changed, 93 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index 24663b0..5d87908 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -58,6 +58,7 @@ enum ath10k_htc_tx_flags {
};

enum ath10k_htc_rx_flags {
+ ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK = 0x01,
ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02,
ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0
};
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index bb6fa67..45df9db 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -35,6 +35,7 @@
#include "sdio.h"

#define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
+#define ATH10K_SDIO_VSG_BUF_SIZE (32 * 1024)

static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
u32 len, bool incr);
@@ -430,6 +431,7 @@ static int ath10k_sdio_mbox_rx_process_packet(struct ath10k *ar,
int ret;

payload_len = le16_to_cpu(htc_hdr->len);
+ skb->len = payload_len + sizeof(struct ath10k_htc_hdr);

if (trailer_present) {
trailer = skb->data + sizeof(*htc_hdr) +
@@ -468,12 +470,13 @@ static int ath10k_sdio_mbox_rx_process_packets(struct ath10k *ar,
enum ath10k_htc_ep_id id;
int ret, i, *n_lookahead_local;
u32 *lookaheads_local;
+ int lookahd_idx = 0;

for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
lookaheads_local = lookaheads;
n_lookahead_local = n_lookahead;

- id = ((struct ath10k_htc_hdr *)&lookaheads[i])->eid;
+ id = ((struct ath10k_htc_hdr *)&lookaheads[lookahd_idx++])->eid;

if (id >= ATH10K_HTC_EP_COUNT) {
ath10k_warn(ar, "invalid endpoint in look-ahead: %d\n",
@@ -496,6 +499,7 @@ static int ath10k_sdio_mbox_rx_process_packets(struct ath10k *ar,
/* Only read lookahead's from RX trailers
* for the last packet in a bundle.
*/
+ lookahd_idx--;
lookaheads_local = NULL;
n_lookahead_local = NULL;
}
@@ -529,11 +533,11 @@ static int ath10k_sdio_mbox_rx_process_packets(struct ath10k *ar,
return ret;
}

-static int ath10k_sdio_mbox_alloc_pkt_bundle(struct ath10k *ar,
- struct ath10k_sdio_rx_data *rx_pkts,
- struct ath10k_htc_hdr *htc_hdr,
- size_t full_len, size_t act_len,
- size_t *bndl_cnt)
+static int ath10k_sdio_mbox_alloc_bundle(struct ath10k *ar,
+ struct ath10k_sdio_rx_data *rx_pkts,
+ struct ath10k_htc_hdr *htc_hdr,
+ size_t full_len, size_t act_len,
+ size_t *bndl_cnt)
{
int ret, i;

@@ -574,6 +578,7 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
size_t full_len, act_len;
bool last_in_bundle;
int ret, i;
+ int pkt_cnt = 0;

if (n_lookaheads > ATH10K_SDIO_MAX_RX_MSGS) {
ath10k_warn(ar,
@@ -616,16 +621,22 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
* optimally fetched as a full bundle.
*/
size_t bndl_cnt;
-
- ret = ath10k_sdio_mbox_alloc_pkt_bundle(ar,
- &ar_sdio->rx_pkts[i],
- htc_hdr,
- full_len,
- act_len,
- &bndl_cnt);
-
- n_lookaheads += bndl_cnt;
- i += bndl_cnt;
+ struct ath10k_sdio_rx_data *rx_pkts =
+ &ar_sdio->rx_pkts[pkt_cnt];
+
+ ret = ath10k_sdio_mbox_alloc_bundle(ar,
+ rx_pkts,
+ htc_hdr,
+ full_len,
+ act_len,
+ &bndl_cnt);
+
+ if (ret) {
+ ath10k_warn(ar, "alloc_bundle error %d\n", ret);
+ goto err;
+ }
+
+ pkt_cnt += bndl_cnt;
/*Next buffer will be the last in the bundle */
last_in_bundle = true;
}
@@ -634,14 +645,18 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
* ATH10K_HTC_FLAG_BUNDLE_MASK flag set, all bundled
* packet skb's have been allocated in the previous step.
*/
- ret = ath10k_sdio_mbox_alloc_rx_pkt(&ar_sdio->rx_pkts[i],
+ if (htc_hdr->flags & ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK)
+ full_len += ATH10K_HIF_MBOX_BLOCK_SIZE;
+
+ ret = ath10k_sdio_mbox_alloc_rx_pkt(&ar_sdio->rx_pkts[pkt_cnt],
act_len,
full_len,
last_in_bundle,
last_in_bundle);
+ pkt_cnt++;
}

- ar_sdio->n_rx_pkts = i;
+ ar_sdio->n_rx_pkts = pkt_cnt;

return 0;

@@ -655,41 +670,71 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
return ret;
}

-static int ath10k_sdio_mbox_rx_packet(struct ath10k *ar,
- struct ath10k_sdio_rx_data *pkt)
+static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
- struct sk_buff *skb = pkt->skb;
+ struct ath10k_sdio_rx_data *pkt = &ar_sdio->rx_pkts[0];
+ struct sk_buff *skb;
int ret;

+ skb = pkt->skb;
ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
skb->data, pkt->alloc_len, false);
- pkt->status = ret;
- if (!ret)
+ if (ret) {
+ ar_sdio->n_rx_pkts = 0;
+ ath10k_sdio_mbox_free_rx_pkt(pkt);
+ } else {
+ pkt->status = ret;
skb_put(skb, pkt->act_len);
+ }

return ret;
}

-static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
+static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
+ struct ath10k_sdio_rx_data *pkt;
int ret, i;
+ u32 pkt_offset, virt_pkt_len;

+ virt_pkt_len = 0;
for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
- ret = ath10k_sdio_mbox_rx_packet(ar,
- &ar_sdio->rx_pkts[i]);
- if (ret)
+ virt_pkt_len += ar_sdio->rx_pkts[i].alloc_len;
+ }
+ if (virt_pkt_len < ATH10K_SDIO_DMA_BUF_SIZE) {
+ ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
+ ar_sdio->vsg_buffer, virt_pkt_len,
+ false);
+ if (ret) {
+ i = 0;
goto err;
+ }
+ } else {
+ ath10k_err(ar, "size exceeding limit %d\n", virt_pkt_len);
+ }
+
+ pkt_offset = 0;
+ for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
+ struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
+
+ pkt = &ar_sdio->rx_pkts[i];
+ memcpy(skb->data, ar_sdio->vsg_buffer + pkt_offset,
+ pkt->alloc_len);
+ pkt->status = 0;
+ skb_put(skb, pkt->act_len);
+ pkt_offset += pkt->alloc_len;
}

return 0;

err:
/* Free all packets that was not successfully fetched. */
- for (; i < ar_sdio->n_rx_pkts; i++)
+ for (i = 0; i < ar_sdio->n_rx_pkts; i++)
ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);

+ ar_sdio->n_rx_pkts = 0;
+
return ret;
}

@@ -732,7 +777,10 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar,
*/
*done = false;

- ret = ath10k_sdio_mbox_rx_fetch(ar);
+ if (ar_sdio->n_rx_pkts > 1)
+ ret = ath10k_sdio_mbox_rx_fetch_bundle(ar);
+ else
+ ret = ath10k_sdio_mbox_rx_fetch(ar);

/* Process fetched packets. This will potentially update
* n_lookaheads depending on if the packets contain lookahead
@@ -1136,7 +1184,7 @@ static int ath10k_sdio_bmi_get_rx_lookahead(struct ath10k *ar)
MBOX_HOST_INT_STATUS_ADDRESS,
&rx_word);
if (ret) {
- ath10k_warn(ar, "unable to read RX_LOOKAHEAD_VALID: %d\n", ret);
+ ath10k_warn(ar, "unable to read rx_lookahd: %d\n", ret);
return ret;
}

@@ -1480,7 +1528,7 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
skb = items[i].transfer_context;
padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
skb->len);
- skb_trim(skb, padded_len);
+ skb->len = padded_len;

/* Write TX data to the end of the mbox address space */
address = ar_sdio->mbox_addr[eid] + ar_sdio->mbox_size[eid] -
@@ -1508,7 +1556,8 @@ static int ath10k_sdio_hif_enable_intrs(struct ath10k *ar)
/* Enable all but CPU interrupts */
regs->int_status_en = FIELD_PREP(MBOX_INT_STATUS_ENABLE_ERROR_MASK, 1) |
FIELD_PREP(MBOX_INT_STATUS_ENABLE_CPU_MASK, 1) |
- FIELD_PREP(MBOX_INT_STATUS_ENABLE_COUNTER_MASK, 1);
+ FIELD_PREP(MBOX_INT_STATUS_ENABLE_COUNTER_MASK,
+ 1);

/* NOTE: There are some cases where HIF can do detection of
* pending mbox messages which is disabled now.
@@ -2024,6 +2073,12 @@ static int ath10k_sdio_probe(struct sdio_func *func,
goto err_free_bmi_buf;
}

+ ar_sdio->vsg_buffer = kzalloc(ATH10K_SDIO_VSG_BUF_SIZE, GFP_KERNEL);
+ if (!ar_sdio->vsg_buffer) {
+ ret = -ENOMEM;
+ goto err_free_bmi_buf;
+ }
+
ar_sdio->func = func;
sdio_set_drvdata(func, ar_sdio);

@@ -2081,7 +2136,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
}

/* TODO: remove this once SDIO support is fully implemented */
- ath10k_warn(ar, "WARNING: ath10k SDIO support is incomplete, don't expect anything to work!\n");
+ ath10k_warn(ar, "WARNING: ath10k SDIO support is experimental\n");

return 0;

@@ -2115,6 +2170,7 @@ static void ath10k_sdio_remove(struct sdio_func *func)
ath10k_core_unregister(ar);
ath10k_core_destroy(ar);
kfree(ar_sdio->dma_buffer);
+ kfree(ar_sdio->vsg_buffer);
}

static const struct sdio_device_id ath10k_sdio_devices[] = {
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
index 718b8b7..8b6a86a 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -149,8 +149,8 @@ struct ath10k_sdio_irq_proc_regs {
u8 rx_lookahead_valid;
u8 host_int_status2;
u8 gmbox_rx_avail;
- __le32 rx_lookahead[2];
- __le32 rx_gmbox_lookahead_alias[2];
+ __le32 rx_lookahead[2 * ATH10K_HIF_MBOX_NUM_MAX];
+ __le32 int_status_enable;
};

struct ath10k_sdio_irq_enable_regs {
@@ -207,6 +207,7 @@ struct ath10k_sdio {
struct ath10k *ar;
struct ath10k_sdio_irq_data irq_data;

+ u8 *vsg_buffer;
u8 *dma_buffer;

/* protects access to dma_buffer */
--
1.9.1

2017-09-30 17:38:35

[permalink] [raw]

Subject: [PATCH 04/11] ath10k_sdio: reduce transmit msdu count

From: Alagu Sankar <[email protected]>

Reduce the transmit MSDU count for SDIO, to match with the descriptors
as used by the firmware. This also acts as a high watermark level for
transmit. Too many packets to the firmware results in transmit overflow
interrupt.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 6 +++++-
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 +-
3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 86247c8..9de49f5 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1968,7 +1968,11 @@ static int ath10k_core_init_firmware_features(struct ath10k *ar)
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;
- ar->htt.max_num_pending_tx = TARGET_TLV_NUM_MSDU_DESC;
+ if (ar->hif.bus == ATH10K_BUS_SDIO)
+ ar->htt.max_num_pending_tx =
+ TARGET_TLV_NUM_MSDU_DESC_HL;
+ else
+ ar->htt.max_num_pending_tx = TARGET_TLV_NUM_MSDU_DESC;
ar->wow.max_num_patterns = TARGET_TLV_NUM_WOW_PATTERNS;
ar->fw_stats_req_mask = WMI_STAT_PDEV | WMI_STAT_VDEV |
WMI_STAT_PEER;
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 7c9f6f9..b870a92 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -688,6 +688,7 @@ struct ath10k_hw_ops {
#define TARGET_TLV_NUM_TDLS_VDEVS 1
#define TARGET_TLV_NUM_TIDS ((TARGET_TLV_NUM_PEERS) * 2)
#define TARGET_TLV_NUM_MSDU_DESC (1024 + 32)
+#define TARGET_TLV_NUM_MSDU_DESC_HL 64
#define TARGET_TLV_NUM_WOW_PATTERNS 22

/* Target specific defines for QCA9377 high latency firmware */
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 34e9770..3842caa 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -1440,7 +1440,7 @@ static struct sk_buff *ath10k_wmi_tlv_op_gen_init(struct ath10k *ar)
cfg->rx_skip_defrag_timeout_dup_detection_check = __cpu_to_le32(0);
cfg->vow_config = __cpu_to_le32(0);
cfg->gtk_offload_max_vdev = __cpu_to_le32(2);
- cfg->num_msdu_desc = __cpu_to_le32(TARGET_TLV_NUM_MSDU_DESC);
+ cfg->num_msdu_desc = __cpu_to_le32(ar->htt.max_num_pending_tx);
cfg->max_frag_entries = __cpu_to_le32(2);
cfg->num_tdls_vdevs = __cpu_to_le32(TARGET_TLV_NUM_TDLS_VDEVS);
cfg->num_tdls_conn_table_entries = __cpu_to_le32(0x20);
--
1.9.1

2017-09-30 17:38:32

[permalink] [raw]

Subject: [PATCH 03/11] ath10k_sdio: DMA bounce buffers for read write

From: Alagu Sankar <[email protected]>

Some SD host controllers still need bounce buffers for SDIO data
transfers. While the transfers worked fine on x86 platforms,
this is found to be required for i.MX6 based systems.

Changes are similar to and derived from the ath6kl sdio driver.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/sdio.c | 59 ++++++++++++++++++++++++++++++++--
drivers/net/wireless/ath/ath10k/sdio.h | 5 +++
2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 03a69e5..77d4fa4 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -34,8 +34,20 @@
#include "trace.h"
#include "sdio.h"

+#define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
+
/* inlined helper functions */

+/* Macro to check if DMA buffer is WORD-aligned and DMA-able.
+ * Most host controllers assume the buffer is DMA'able and will
+ * bug-check otherwise (i.e. buffers on the stack). virt_addr_valid
+ * check fails on stack memory.
+ */
+static inline bool buf_needs_bounce(const u8 *buf)
+{
+ return ((unsigned long)buf & 0x3) || !virt_addr_valid(buf);
+}
+
static inline int ath10k_sdio_calc_txrx_padded_len(struct ath10k_sdio *ar_sdio,
size_t len)
{
@@ -303,15 +315,29 @@ static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, size_t len)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
struct sdio_func *func = ar_sdio->func;
+ bool bounced = false;
+ u8 *tbuf = NULL;
int ret;

+ if (buf_needs_bounce(buf)) {
+ if (!ar_sdio->dma_buffer)
+ return -ENOMEM;
+ mutex_lock(&ar_sdio->dma_buffer_mutex);
+ tbuf = ar_sdio->dma_buffer;
+ bounced = true;
+ } else {
+ tbuf = buf;
+ }
+
sdio_claim_host(func);

- ret = sdio_memcpy_fromio(func, buf, addr, len);
+ ret = sdio_memcpy_fromio(func, tbuf, addr, len);
if (ret) {
ath10k_warn(ar, "failed to read from address 0x%x: %d\n",
addr, ret);
goto out;
+ } else if (bounced) {
+ memcpy(buf, tbuf, len);
}

ath10k_dbg(ar, ATH10K_DBG_SDIO, "sdio read addr 0x%x buf 0x%p len %zu\n",
@@ -320,6 +346,8 @@ static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, size_t len)

out:
sdio_release_host(func);
+ if (bounced)
+ mutex_unlock(&ar_sdio->dma_buffer_mutex);

return ret;
}
@@ -328,14 +356,27 @@ static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, size_
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
struct sdio_func *func = ar_sdio->func;
+ bool bounced = false;
+ u8 *tbuf = NULL;
int ret;

+ if (buf_needs_bounce(buf)) {
+ if (!ar_sdio->dma_buffer)
+ return -ENOMEM;
+ mutex_lock(&ar_sdio->dma_buffer_mutex);
+ tbuf = ar_sdio->dma_buffer;
+ memcpy(tbuf, buf, len);
+ bounced = true;
+ } else {
+ tbuf = (u8 *)buf;
+ }
+
sdio_claim_host(func);

/* For some reason toio() doesn't have const for the buffer, need
* an ugly hack to workaround that.
*/
- ret = sdio_memcpy_toio(func, addr, (void *)buf, len);
+ ret = sdio_memcpy_toio(func, addr, (void *)tbuf, len);
if (ret) {
ath10k_warn(ar, "failed to write to address 0x%x: %d\n",
addr, ret);
@@ -348,6 +389,8 @@ static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, size_

out:
sdio_release_host(func);
+ if (bounced)
+ mutex_unlock(&ar_sdio->dma_buffer_mutex);

return ret;
}
@@ -1978,6 +2021,12 @@ static int ath10k_sdio_probe(struct sdio_func *func,
goto err_free_en_reg;
}

+ ar_sdio->dma_buffer = kzalloc(ATH10K_SDIO_DMA_BUF_SIZE, GFP_KERNEL);
+ if (!ar_sdio->dma_buffer) {
+ ret = -ENOMEM;
+ goto err_free_bmi_buf;
+ }
+
ar_sdio->func = func;
sdio_set_drvdata(func, ar_sdio);

@@ -1987,6 +2036,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
spin_lock_init(&ar_sdio->lock);
spin_lock_init(&ar_sdio->wr_async_lock);
mutex_init(&ar_sdio->irq_data.mtx);
+ mutex_init(&ar_sdio->dma_buffer_mutex);

INIT_LIST_HEAD(&ar_sdio->bus_req_freeq);
INIT_LIST_HEAD(&ar_sdio->wr_asyncq);
@@ -1995,7 +2045,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
ar_sdio->workqueue = create_singlethread_workqueue("ath10k_sdio_wq");
if (!ar_sdio->workqueue) {
ret = -ENOMEM;
- goto err_free_bmi_buf;
+ goto err_dma;
}

for (i = 0; i < ATH10K_SDIO_BUS_REQUEST_MAX_NUM; i++)
@@ -2040,6 +2090,8 @@ static int ath10k_sdio_probe(struct sdio_func *func,

err_free_wq:
destroy_workqueue(ar_sdio->workqueue);
+err_dma:
+ kfree(ar_sdio->dma_buffer);
err_free_bmi_buf:
kfree(ar_sdio->bmi_buf);
err_free_en_reg:
@@ -2065,6 +2117,7 @@ static void ath10k_sdio_remove(struct sdio_func *func)
cancel_work_sync(&ar_sdio->wr_async_work);
ath10k_core_unregister(ar);
ath10k_core_destroy(ar);
+ kfree(ar_sdio->dma_buffer);
}

static const struct sdio_device_id ath10k_sdio_devices[] = {
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
index 4ff7b54..718b8b7 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -207,6 +207,11 @@ struct ath10k_sdio {
struct ath10k *ar;
struct ath10k_sdio_irq_data irq_data;

+ u8 *dma_buffer;
+
+ /* protects access to dma_buffer */
+ struct mutex dma_buffer_mutex;
+
/* temporary buffer for BMI requests */
u8 *bmi_buf;

--
1.9.1

2017-09-30 17:38:41

[permalink] [raw]

Subject: [PATCH 07/11] ath10k_sdio: fix rssi indication

From: Alagu Sankar <[email protected]>

Receive descriptor of sdio device does not include the rssi. notify
mac80211 accordingly. Without the fix, the rssi value indicated by
iw changes between the actual value and -95.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index f025363..21adcad 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1646,9 +1646,16 @@ static bool ath10k_htt_rx_proc_rx_ind_hl(struct ath10k_htt *htt,
hdr = (struct ieee80211_hdr *)skb->data;
rx_status = IEEE80211_SKB_RXCB(skb);
rx_status->chains |= BIT(0);
- rx_status->signal = ATH10K_DEFAULT_NOISE_FLOOR +
- rx->ppdu.combined_rssi;
- rx_status->flag &= ~RX_FLAG_NO_SIGNAL_VAL;
+
+ if (ar->hif.bus == ATH10K_BUS_SDIO) {
+ /* SDIO firmware does not provide signal */
+ rx_status->signal = 0;
+ rx_status->flag |= RX_FLAG_NO_SIGNAL_VAL;
+ } else {
+ rx_status->signal = ATH10K_DEFAULT_NOISE_FLOOR +
+ rx->ppdu.combined_rssi;
+ rx_status->flag &= ~RX_FLAG_NO_SIGNAL_VAL;
+ }

spin_lock_bh(&ar->data_lock);
ch = ar->scan_channel;
--
1.9.1

2017-09-30 17:38:39

[permalink] [raw]

Subject: [PATCH 06/11] ath10k_sdio: high latency fixes for beacon buffer

From: Alagu Sankar <[email protected]>

Beacon buffer for high latency devices does not use dma. other similar
buffer allocation methods in the driver have already been modified for
high latency path. Fix the beacon buffer allocation left out in the
earlier high latency changes.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index f374d1b..c48785b 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -944,8 +944,12 @@ static void ath10k_mac_vif_beacon_cleanup(struct ath10k_vif *arvif)
ath10k_mac_vif_beacon_free(arvif);

if (arvif->beacon_buf) {
- dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
- arvif->beacon_buf, arvif->beacon_paddr);
+ if (ar->is_high_latency)
+ kfree(arvif->beacon_buf);
+ else
+ dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
+ arvif->beacon_buf,
+ arvif->beacon_paddr);
arvif->beacon_buf = NULL;
}
}
@@ -5052,10 +5056,17 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
if (vif->type == NL80211_IFTYPE_ADHOC ||
vif->type == NL80211_IFTYPE_MESH_POINT ||
vif->type == NL80211_IFTYPE_AP) {
- arvif->beacon_buf = dma_zalloc_coherent(ar->dev,
- IEEE80211_MAX_FRAME_LEN,
- &arvif->beacon_paddr,
- GFP_ATOMIC);
+ if (ar->is_high_latency) {
+ arvif->beacon_buf = kmalloc(IEEE80211_MAX_FRAME_LEN,
+ GFP_KERNEL);
+ arvif->beacon_paddr = (dma_addr_t)arvif->beacon_buf;
+ } else {
+ arvif->beacon_buf =
+ dma_zalloc_coherent(ar->dev,
+ IEEE80211_MAX_FRAME_LEN,
+ &arvif->beacon_paddr,
+ GFP_ATOMIC);
+ }
if (!arvif->beacon_buf) {
ret = -ENOMEM;
ath10k_warn(ar, "failed to allocate beacon buffer: %d\n",
@@ -5244,8 +5255,12 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,

err:
if (arvif->beacon_buf) {
- dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
- arvif->beacon_buf, arvif->beacon_paddr);
+ if (ar->is_high_latency)
+ kfree(arvif->beacon_buf);
+ else
+ dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
+ arvif->beacon_buf,
+ arvif->beacon_paddr);
arvif->beacon_buf = NULL;
}

--
1.9.1

2017-09-30 17:38:30

[permalink] [raw]

Subject: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

From: Alagu Sankar <[email protected]>

The QCA9377-3 WB396 sdio reference card does not get initialized
due to the conflict in uart gpio pins. This fix is not required
for other QCA9377 sdio cards.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index b4f66cd..86247c8 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
return ret;
}

- if (!uart_print)
+ if (!uart_print) {
+ /* Hack: override dbg TX pin to avoid side effects of default
+ * GPIO_6 in QCA9377 WB396 reference card
+ */
+ if (ar->hif.bus == ATH10K_BUS_SDIO)
+ ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
+ ar->hw_params.uart_pin);
return 0;
+ }

ret = ath10k_bmi_write32(ar, hi_dbg_uart_txpin, ar->hw_params.uart_pin);
if (ret) {
--
1.9.1

2017-09-30 17:38:37

[permalink] [raw]

Subject: [PATCH 05/11] ath10k_sdio: use clean packet headers

From: Alagu Sankar <[email protected]>

HTC header carries junk values that may be interpreted by the firmware
differently. Enable credit update only if flow control is enabled for
the corresponding endpoint.

PLL clock setting sequence does not mask the PLL_CONTROL
register value. Side effect of not masking the values is not known as
the entire pll clock setting sequence is undocumented.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/htc.c | 4 +++-
drivers/net/wireless/ath/ath10k/hw.c | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 75c2a3e..23e7216 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -84,11 +84,13 @@ static void ath10k_htc_prepare_tx_skb(struct ath10k_htc_ep *ep,
struct ath10k_htc_hdr *hdr;

hdr = (struct ath10k_htc_hdr *)skb->data;
+ memset(hdr, 0, sizeof(struct ath10k_htc_hdr));

hdr->eid = ep->eid;
hdr->len = __cpu_to_le16(skb->len - sizeof(*hdr));
hdr->flags = 0;
- hdr->flags |= ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE;
+ if (ep->tx_credit_flow_enabled)
+ hdr->flags |= ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE;

spin_lock_bh(&ep->htc->tx_lock);
hdr->seq_no = ep->seq_no++;
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index 07df7c6..2092392 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -812,6 +812,8 @@ static int ath10k_hw_qca6174_enable_pll_clock(struct ath10k *ar)
if (ret)
return -EINVAL;

+ reg_val &= ~(WLAN_PLL_CONTROL_REFDIV_MASK | WLAN_PLL_CONTROL_DIV_MASK |
+ WLAN_PLL_CONTROL_NOPWD_MASK);
reg_val |= (SM(hw_clk->refdiv, WLAN_PLL_CONTROL_REFDIV) |
SM(hw_clk->div, WLAN_PLL_CONTROL_DIV) |
SM(1, WLAN_PLL_CONTROL_NOPWD));
--
1.9.1

2017-09-30 17:38:49

[permalink] [raw]

Subject: [PATCH 11/11] ath10k_sdio: hif start once addition

From: Alagu Sankar <[email protected]>

sdio hif interface uses the start_once optimization to avoid unnecessary
firmware downloading. simulate hif_stop in sdio as core_stop does not
handle this due to start_once optimization.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/sdio.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 11fbf6e..c6f23a9 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2255,6 +2255,12 @@ static void ath10k_sdio_remove(struct sdio_func *func)
cancel_work_sync(&ar_sdio->wr_async_work);
ath10k_core_unregister(ar);
ath10k_core_destroy(ar);
+
+ if (ar->is_started && ar->hw_params.start_once) {
+ ath10k_hif_stop(ar);
+ ath10k_hif_power_down(ar);
+ }
+
kfree(ar_sdio->dma_buffer);
kfree(ar_sdio->vsg_buffer);
}
--
1.9.1

2017-09-30 17:38:47

[permalink] [raw]

Subject: [PATCH 10/11] ath10k_sdio: enable firmware crash dump

From: Alagu Sankar <[email protected]>

Handle firmware crash and gracefully restart the sdio driver on firmware
failures. The caldata prefetch is disabled for sdio as the data read in
prefetch is resulting in errors, when enabled.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/debug.c | 3 ++
drivers/net/wireless/ath/ath10k/sdio.c | 86 +++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index eed4e9c..3a1982f 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1255,6 +1255,9 @@ static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
if (WARN_ON(ar->hw_params.cal_data_len > ATH10K_DEBUG_CAL_DATA_LEN))
return -EINVAL;

+ if (ar->hif.bus == ATH10K_BUS_SDIO)
+ return -EINVAL;
+
hi_addr = host_interest_item_address(HI_ITEM(hi_board_data));

ret = ath10k_hif_diag_read(ar, hi_addr, &addr, sizeof(addr));
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 45df9db..11fbf6e 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -28,6 +28,7 @@
#include "core.h"
#include "bmi.h"
#include "debug.h"
+#include "coredump.h"
#include "hif.h"
#include "htc.h"
#include "targaddrs.h"
@@ -37,6 +38,8 @@
#define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
#define ATH10K_SDIO_VSG_BUF_SIZE (32 * 1024)

+static int ath10k_sdio_hif_diag_read(struct ath10k *ar, u32 address, void *buf,
+ size_t buf_len);
static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
u32 len, bool incr);
static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
@@ -810,6 +813,86 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar,
return ret;
}

+static int __ath10k_sdio_diag_read_hi(struct ath10k *ar, void *dest, u32 src,
+ u32 len)
+{
+ u32 host_addr, addr;
+ int ret;
+
+ host_addr = host_interest_item_address(src);
+
+ ret = ath10k_sdio_hif_diag_read(ar, host_addr, &addr, sizeof(addr));
+ if (ret != 0) {
+ ath10k_warn(ar, "failed to get firmware hi address %d: %d\n",
+ src, ret);
+ return ret;
+ }
+
+ ret = ath10k_sdio_hif_diag_read(ar, addr, dest, len);
+ if (ret != 0) {
+ ath10k_warn(ar, "failed to copy memory from %d (%d B): %d\n",
+ addr, len, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+#define ath10k_sdio_diag_read_hi(ar, dest, src, len) \
+ __ath10k_sdio_diag_read_hi(ar, dest, HI_ITEM(src), len)
+
+static void ath10k_sdio_dump_registers(struct ath10k *ar,
+ struct ath10k_fw_crash_data *crash_data)
+{
+ __le32 reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
+ int i, ret;
+
+ ret = ath10k_sdio_diag_read_hi(ar, &reg_dump_values[0],
+ hi_failure_state,
+ sizeof(reg_dump_values));
+ if (ret) {
+ ath10k_err(ar, "failed to read firmware dump area: %d\n", ret);
+ return;
+ }
+
+ BUILD_BUG_ON(REG_DUMP_COUNT_QCA988X % 4);
+
+ ath10k_err(ar, "firmware register dump:\n");
+ for (i = 0; i < REG_DUMP_COUNT_QCA988X; i += 4)
+ ath10k_err(ar, "[%02d]: 0x%08X 0x%08X 0x%08X 0x%08X\n",
+ i,
+ __le32_to_cpu(reg_dump_values[i]),
+ __le32_to_cpu(reg_dump_values[i + 1]),
+ __le32_to_cpu(reg_dump_values[i + 2]),
+ __le32_to_cpu(reg_dump_values[i + 3]));
+
+ if (!crash_data)
+ return;
+
+ for (i = 0; i < REG_DUMP_COUNT_QCA988X; i++)
+ crash_data->registers[i] = reg_dump_values[i];
+}
+
+static void ath10k_sdio_fw_crashed_dump(struct ath10k *ar)
+{
+ struct ath10k_fw_crash_data *crash_data;
+ char guid[UUID_STRING_LEN + 1];
+
+ ar->stats.fw_crash_counter++;
+ crash_data = ath10k_coredump_new(ar);
+
+ if (crash_data)
+ scnprintf(guid, sizeof(guid), "%pUl", &crash_data->guid);
+ else
+ scnprintf(guid, sizeof(guid), "n/a");
+
+ ath10k_err(ar, "firmware crashed! (guid %s)\n", guid);
+ ath10k_print_driver_info(ar);
+ ath10k_sdio_dump_registers(ar, crash_data);
+
+ queue_work(ar->workqueue, &ar->restart_work);
+}
+
static int ath10k_sdio_mbox_proc_dbg_intr(struct ath10k *ar)
{
u32 val;
@@ -933,6 +1016,9 @@ static int ath10k_sdio_mbox_proc_cpu_intr(struct ath10k *ar)
goto out;
}

+ if (cpu_int_status & 0x1)
+ ath10k_sdio_fw_crashed_dump(ar);
+
out:
mutex_unlock(&irq_data->mtx);
return ret;
--
1.9.1

2017-09-30 17:38:29

[permalink] [raw]

Subject: [PATCH 01/11] ath10k_sdio: sdio htt data transfer fixes

From: Alagu Sankar <[email protected]>

The ath10k sdio firmware does not allow transmitting packets with the
reduced tx completion HI_ACS option. sdio firmware uses 1544 as
alternate credit size, which is not big enough for the maximum sized
mac80211 frames. Disable both these HI_ACS flags for SDIO.

Transmit completion for SDIO is similar to PCIe, via the T2H message
HTT_T2H_MSG_TYPE_TX_COMPL_IND. Modify the high latency path to allow
SDIO modules to use the htt transmit completion path. This differs from
the high latency path taken by the USB devices.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 20 +++++++++++++++++---
drivers/net/wireless/ath/ath10k/htt_rx.c | 6 ++++--
drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +++++++++++++++++++-----
3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 4351341..b4f66cd 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -494,11 +494,25 @@ static void ath10k_init_sdio(struct ath10k *ar)
ath10k_bmi_write32(ar, hi_mbox_isr_yield_limit, 99);
ath10k_bmi_read32(ar, hi_acs_flags, &param);

- param |= (HI_ACS_FLAGS_SDIO_SWAP_MAILBOX_SET |
- HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET |
- HI_ACS_FLAGS_ALT_DATA_CREDIT_SIZE);
+ /* Data transfer is not initiated, when reduced Tx completion
+ * is used for SDIO. disable it until fixed
+ */
+ param &= ~HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET;
+
+ /* Alternate credit size of 1544 as used by SDIO firmware is
+ * not big enough for mac80211 / native wifi frames. disable it
+ */
+ param &= ~HI_ACS_FLAGS_ALT_DATA_CREDIT_SIZE;

+ param |= HI_ACS_FLAGS_SDIO_SWAP_MAILBOX_SET;
ath10k_bmi_write32(ar, hi_acs_flags, param);
+
+ /* Explicitly set fwlog prints to zero as target may turn it on
+ * based on scratch registers.
+ */
+ ath10k_bmi_read32(ar, hi_option_flag, &param);
+ param |= HI_OPTION_DISABLE_DBGLOG;
+ ath10k_bmi_write32(ar, hi_option_flag, param);
}

static int ath10k_init_configure_target(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 569edd0..f025363 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1764,7 +1764,9 @@ static void ath10k_htt_rx_tx_compl_ind(struct ath10k *ar,
* Note that with only one concurrent reader and one concurrent
* writer, you don't need extra locking to use these macro.
*/
- if (!kfifo_put(&htt->txdone_fifo, tx_done)) {
+ if (ar->hif.bus == ATH10K_BUS_SDIO) {
+ ath10k_txrx_tx_unref(htt, &tx_done);
+ } else if (!kfifo_put(&htt->txdone_fifo, tx_done)) {
ath10k_warn(ar, "txdone fifo overrun, msdu_id %d status %d\n",
tx_done.msdu_id, tx_done.status);
ath10k_txrx_tx_unref(htt, &tx_done);
@@ -2541,7 +2543,7 @@ bool ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
break;
}
case HTT_T2H_MSG_TYPE_TX_COMPL_IND:
- if (!ar->is_high_latency)
+ if (!(ar->hif.bus == ATH10K_BUS_USB))
ath10k_htt_rx_tx_compl_ind(htt->ar, skb);
break;
case HTT_T2H_MSG_TYPE_SEC_IND: {
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index c74fc13..d7f59a2 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -153,7 +153,7 @@ void ath10k_htt_tx_txq_update(struct ieee80211_hw *hw,

void ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
{
- if (htt->ar->is_high_latency)
+ if (htt->ar->hif.bus == ATH10K_BUS_USB)
return;

lockdep_assert_held(&htt->tx_lock);
@@ -165,7 +165,7 @@ void ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)

int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)
{
- if (htt->ar->is_high_latency)
+ if (htt->ar->hif.bus == ATH10K_BUS_USB)
return 0;

lockdep_assert_held(&htt->tx_lock);
@@ -454,7 +454,7 @@ void ath10k_htt_tx_destroy(struct ath10k_htt *htt)
return;

ath10k_htt_tx_free_cont_txbuf(htt);
- if (!htt->ar->is_high_latency)
+ if (!(htt->ar->hif.bus == ATH10K_BUS_USB))
ath10k_htt_tx_free_txq(htt);
ath10k_htt_tx_free_cont_frag_desc(htt);
ath10k_htt_tx_free_txdone_fifo(htt);
@@ -475,7 +475,8 @@ void ath10k_htt_tx_free(struct ath10k_htt *htt)

void ath10k_htt_htc_tx_complete(struct ath10k *ar, struct sk_buff *skb)
{
- dev_kfree_skb_any(skb);
+ if (!(ar->hif.bus == ATH10K_BUS_SDIO))
+ dev_kfree_skb_any(skb);
}

void ath10k_htt_hif_tx_complete(struct ath10k *ar, struct sk_buff *skb)
@@ -975,6 +976,7 @@ int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txmode,
u8 tid = ath10k_htt_tx_get_tid(msdu, is_eth);
u8 flags0 = 0;
u16 flags1 = 0;
+ u16 msdu_id = 0;

data_len = msdu->len;

@@ -1022,6 +1024,18 @@ int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txmode,
}
}

+ if (ar->hif.bus == ATH10K_BUS_SDIO) {
+ flags1 |= HTT_DATA_TX_DESC_FLAGS1_POSTPONED;
+ spin_lock_bh(&htt->tx_lock);
+ res = ath10k_htt_tx_alloc_msdu_id(htt, msdu);
+ spin_unlock_bh(&htt->tx_lock);
+ if (res < 0) {
+ ath10k_err(ar, "msdu_id allocation failed %d\n", res);
+ goto out;
+ }
+ msdu_id = res;
+ }
+
skb_push(msdu, sizeof(*cmd_hdr));
skb_push(msdu, sizeof(*tx_desc));
cmd_hdr = (struct htt_cmd_hdr *)msdu->data;
@@ -1031,7 +1045,7 @@ int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txmode,
tx_desc->flags0 = flags0;
tx_desc->flags1 = __cpu_to_le16(flags1);
tx_desc->len = __cpu_to_le16(data_len);
- tx_desc->id = 0;
+ tx_desc->id = __cpu_to_le16(msdu_id);
tx_desc->frags_paddr = 0; /* always zero */
/* Initialize peer_id to INVALID_PEER because this is NOT
* Reinjection path
--
1.9.1

2017-09-30 17:38:43

[permalink] [raw]

Subject: [PATCH 08/11] ath10k_sdio: common read write

From: Alagu Sankar <[email protected]>

convert different read write functions in sdio hif to bring it under a
single read-write path. This helps in having a common dma bounce buffer
implementation. Also helps in address modification that is required
specific to change in certain mbox addresses of sdio_write.

Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/sdio.c | 131 ++++++++++++++++-----------------
1 file changed, 64 insertions(+), 67 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 77d4fa4..bb6fa67 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -36,6 +36,11 @@

#define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)

+static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
+ u32 len, bool incr);
+static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
+ u32 len, bool incr);
+
/* inlined helper functions */

/* Macro to check if DMA buffer is WORD-aligned and DMA-able.
@@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
struct sdio_func *func = ar_sdio->func;
unsigned char byte, asyncintdelay = 2;
int ret;
+ u32 addr;

ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");

@@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);

- ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
- CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
- byte);
+ addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
+ ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);
if (ret) {
ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
goto out;
@@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)

static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
{
- struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
- struct sdio_func *func = ar_sdio->func;
+ __le32 *buf;
int ret;

- sdio_claim_host(func);
+ buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;

- sdio_writel(func, val, addr, &ret);
+ *buf = cpu_to_le32(val);
+
+ ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);
if (ret) {
ath10k_warn(ar, "failed to write 0x%x to address 0x%x: %d\n",
val, addr, ret);
@@ -250,15 +258,13 @@ static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
addr, val);

out:
- sdio_release_host(func);
+ kfree(buf);

return ret;
}

static int ath10k_sdio_writesb32(struct ath10k *ar, u32 addr, u32 val)
{
- struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
- struct sdio_func *func = ar_sdio->func;
__le32 *buf;
int ret;

@@ -268,9 +274,7 @@ static int ath10k_sdio_writesb32(struct ath10k *ar, u32 addr, u32 val)

*buf = cpu_to_le32(val);

- sdio_claim_host(func);
-
- ret = sdio_writesb(func, addr, buf, sizeof(*buf));
+ ret = ath10k_sdio_write(ar, addr, buf, sizeof(*buf), false);
if (ret) {
ath10k_warn(ar, "failed to write value 0x%x to fixed sb address 0x%x: %d\n",
val, addr, ret);
@@ -281,8 +285,6 @@ static int ath10k_sdio_writesb32(struct ath10k *ar, u32 addr, u32 val)
addr, val);

out:
- sdio_release_host(func);
-
kfree(buf);

return ret;
@@ -290,28 +292,33 @@ static int ath10k_sdio_writesb32(struct ath10k *ar, u32 addr, u32 val)

static int ath10k_sdio_read32(struct ath10k *ar, u32 addr, u32 *val)
{
- struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
- struct sdio_func *func = ar_sdio->func;
+ __le32 *buf;
int ret;

- sdio_claim_host(func);
- *val = sdio_readl(func, addr, &ret);
+ buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = ath10k_sdio_read(ar, addr, buf, sizeof(*val), true);
if (ret) {
ath10k_warn(ar, "failed to read from address 0x%x: %d\n",
addr, ret);
goto out;
}

+ *val = le32_to_cpu(*buf);
+
ath10k_dbg(ar, ATH10K_DBG_SDIO, "sdio read32 addr 0x%x val 0x%x\n",
addr, *val);

out:
- sdio_release_host(func);
+ kfree(buf);

return ret;
}

-static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, size_t len)
+static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
+ size_t len, bool incr)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
struct sdio_func *func = ar_sdio->func;
@@ -330,8 +337,12 @@ static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, size_t len)
}

sdio_claim_host(func);
+ if (incr)
+ ret = sdio_memcpy_fromio(func, tbuf, addr, len);
+ else
+ ret = sdio_readsb(func, tbuf, addr, len);
+ sdio_release_host(func);

- ret = sdio_memcpy_fromio(func, tbuf, addr, len);
if (ret) {
ath10k_warn(ar, "failed to read from address 0x%x: %d\n",
addr, ret);
@@ -345,14 +356,14 @@ static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, size_t len)
ath10k_dbg_dump(ar, ATH10K_DBG_SDIO_DUMP, NULL, "sdio read ", buf, len);

out:
- sdio_release_host(func);
if (bounced)
mutex_unlock(&ar_sdio->dma_buffer_mutex);

return ret;
}

-static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, size_t len)
+static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
+ size_t len, bool incr)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
struct sdio_func *func = ar_sdio->func;
@@ -371,12 +382,18 @@ static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, size_
tbuf = (u8 *)buf;
}

+ if (addr == ar_sdio->mbox_info.htc_addr)
+ addr += (ATH10K_HIF_MBOX_WIDTH - len);
+
sdio_claim_host(func);

/* For some reason toio() doesn't have const for the buffer, need
* an ugly hack to workaround that.
*/
- ret = sdio_memcpy_toio(func, addr, (void *)tbuf, len);
+ if (incr)
+ ret = sdio_memcpy_toio(func, addr, (void *)tbuf, len);
+ else
+ ret = sdio_writesb(func, addr, tbuf, len);
if (ret) {
ath10k_warn(ar, "failed to write to address 0x%x: %d\n",
addr, ret);
@@ -389,39 +406,13 @@ static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, size_

out:
sdio_release_host(func);
+
if (bounced)
mutex_unlock(&ar_sdio->dma_buffer_mutex);

return ret;
}

-static int ath10k_sdio_readsb(struct ath10k *ar, u32 addr, void *buf, size_t len)
-{
- struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
- struct sdio_func *func = ar_sdio->func;
- int ret;
-
- sdio_claim_host(func);
-
- len = round_down(len, ar_sdio->mbox_info.block_size);
-
- ret = sdio_readsb(func, buf, addr, len);
- if (ret) {
- ath10k_warn(ar, "failed to read from fixed (sb) address 0x%x: %d\n",
- addr, ret);
- goto out;
- }
-
- ath10k_dbg(ar, ATH10K_DBG_SDIO, "sdio readsb addr 0x%x buf 0x%p len %zu\n",
- addr, buf, len);
- ath10k_dbg_dump(ar, ATH10K_DBG_SDIO_DUMP, NULL, "sdio readsb ", buf, len);
-
-out:
- sdio_release_host(func);
-
- return ret;
-}
-
/* HIF mbox functions */

static int ath10k_sdio_mbox_rx_process_packet(struct ath10k *ar,
@@ -671,8 +662,8 @@ static int ath10k_sdio_mbox_rx_packet(struct ath10k *ar,
struct sk_buff *skb = pkt->skb;
int ret;

- ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
- skb->data, pkt->alloc_len);
+ ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
+ skb->data, pkt->alloc_len, false);
pkt->status = ret;
if (!ret)
skb_put(skb, pkt->act_len);
@@ -932,7 +923,7 @@ static int ath10k_sdio_mbox_read_int_status(struct ath10k *ar,
* registers and the lookahead registers.
*/
ret = ath10k_sdio_read(ar, MBOX_HOST_INT_STATUS_ADDRESS,
- irq_proc_reg, sizeof(*irq_proc_reg));
+ irq_proc_reg, sizeof(*irq_proc_reg), true);
if (ret)
goto out;

@@ -1177,7 +1168,8 @@ static int ath10k_sdio_bmi_exchange_msg(struct ath10k *ar,
addr = ar_sdio->mbox_info.htc_addr;

memcpy(ar_sdio->bmi_buf, req, req_len);
- ret = ath10k_sdio_write(ar, addr, ar_sdio->bmi_buf, req_len);
+ ret = ath10k_sdio_write(ar, addr, ar_sdio->bmi_buf, req_len,
+ true);
if (ret) {
ath10k_warn(ar,
"unable to send the bmi data to the device: %d\n",
@@ -1241,7 +1233,7 @@ static int ath10k_sdio_bmi_exchange_msg(struct ath10k *ar,

/* We always read from the start of the mbox address */
addr = ar_sdio->mbox_info.htc_addr;
- ret = ath10k_sdio_read(ar, addr, ar_sdio->bmi_buf, *resp_len);
+ ret = ath10k_sdio_read(ar, addr, ar_sdio->bmi_buf, *resp_len, true);
if (ret) {
ath10k_warn(ar,
"unable to read the bmi data from the device: %d\n",
@@ -1298,7 +1290,7 @@ static void __ath10k_sdio_write_async(struct ath10k *ar,
int ret;

skb = req->skb;
- ret = ath10k_sdio_write(ar, req->address, skb->data, skb->len);
+ ret = ath10k_sdio_write(ar, req->address, skb->data, skb->len, true);
if (ret)
ath10k_warn(ar, "failed to write skb to 0x%x asynchronously: %d",
req->address, ret);
@@ -1405,7 +1397,7 @@ static int ath10k_sdio_hif_disable_intrs(struct ath10k *ar)

memset(regs, 0, sizeof(*regs));
ret = ath10k_sdio_write(ar, MBOX_INT_STATUS_ENABLE_ADDRESS,
- &regs->int_status_en, sizeof(*regs));
+ &regs->int_status_en, sizeof(*regs), true);
if (ret)
ath10k_warn(ar, "unable to disable sdio interrupts: %d\n", ret);

@@ -1540,7 +1532,7 @@ static int ath10k_sdio_hif_enable_intrs(struct ath10k *ar)
ATH10K_SDIO_TARGET_DEBUG_INTR_MASK);

ret = ath10k_sdio_write(ar, MBOX_INT_STATUS_ENABLE_ADDRESS,
- &regs->int_status_en, sizeof(*regs));
+ &regs->int_status_en, sizeof(*regs), true);
if (ret)
ath10k_warn(ar,
"failed to update mbox interrupt status register : %d\n",
@@ -1555,7 +1547,8 @@ static int ath10k_sdio_hif_set_mbox_sleep(struct ath10k *ar, bool enable_sleep)
u32 val;
int ret;

- ret = ath10k_sdio_read32(ar, ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL, &val);
+ ret = ath10k_sdio_read32(ar, ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL,
+ &val);
if (ret) {
ath10k_warn(ar, "failed to read fifo/chip control register: %d\n",
ret);
@@ -1567,7 +1560,8 @@ static int ath10k_sdio_hif_set_mbox_sleep(struct ath10k *ar, bool enable_sleep)
else
val |= ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL_DISABLE_SLEEP_ON;

- ret = ath10k_sdio_write32(ar, ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL, val);
+ ret = ath10k_sdio_write32(ar, ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL,
+ val);
if (ret) {
ath10k_warn(ar, "failed to write to FIFO_TIMEOUT_AND_CHIP_CONTROL: %d",
ret);
@@ -1587,12 +1581,14 @@ static int ath10k_sdio_hif_diag_read(struct ath10k *ar, u32 address, void *buf,
/* set window register to start read cycle */
ret = ath10k_sdio_write32(ar, MBOX_WINDOW_READ_ADDR_ADDRESS, address);
if (ret) {
- ath10k_warn(ar, "failed to set mbox window read address: %d", ret);
+ ath10k_warn(ar, "failed to set mbox window read address: %d",
+ ret);
return ret;
}

/* read the data */
- ret = ath10k_sdio_read(ar, MBOX_WINDOW_DATA_ADDRESS, buf, buf_len);
+ ret = ath10k_sdio_read(ar, MBOX_WINDOW_DATA_ADDRESS, buf, buf_len,
+ true);
if (ret) {
ath10k_warn(ar, "failed to read from mbox window data address: %d\n",
ret);
@@ -1630,7 +1626,8 @@ static int ath10k_sdio_hif_diag_write_mem(struct ath10k *ar, u32 address,
int ret;

/* set write data */
- ret = ath10k_sdio_write(ar, MBOX_WINDOW_DATA_ADDRESS, data, nbytes);
+ ret = ath10k_sdio_write(ar, MBOX_WINDOW_DATA_ADDRESS, data, nbytes,
+ true);
if (ret) {
ath10k_warn(ar,
"failed to write 0x%p to mbox window data address: %d\n",
@@ -1641,7 +1638,7 @@ static int ath10k_sdio_hif_diag_write_mem(struct ath10k *ar, u32 address,
/* set window register, which starts the write cycle */
ret = ath10k_sdio_write32(ar, MBOX_WINDOW_WRITE_ADDR_ADDRESS, address);
if (ret) {
- ath10k_warn(ar, "failed to set mbox window write address: %d", ret);
+ ath10k_warn(ar, "failed to set mbox window register: %d", ret);
return ret;
}

@@ -1691,7 +1688,7 @@ static int ath10k_sdio_hif_start(struct ath10k *ar)

ret = ath10k_sdio_hif_diag_read32(ar, addr, &val);
if (ret) {
- ath10k_warn(ar, "unable to read hi_acs_flags address: %d\n", ret);
+ ath10k_warn(ar, "unable to read hi_acs_flags : %d\n", ret);
return ret;
}

--
1.9.1

2017-10-04 09:49:24

[permalink] [raw]

Subject: Re: [PATCH 08/11] ath10k_sdio: common read write

[email protected] writes:

> From: Alagu Sankar <[email protected]>
>
> convert different read write functions in sdio hif to bring it under a
> single read-write path. This helps in having a common dma bounce buffer
> implementation. Also helps in address modification that is required
> specific to change in certain mbox addresses of sdio_write.
>
> Signed-off-by: Alagu Sankar <[email protected]>

This didn't compile for me:

drivers/net/wireless/ath/ath10k/sdio.c:320:12: error: conflicting types for=
'ath10k_sdio_read'
drivers/net/wireless/ath/ath10k/sdio.c:39:12: note: previous declaration of=
'ath10k_sdio_read' was here
drivers/net/wireless/ath/ath10k/sdio.c:365:12: error: conflicting types for=
'ath10k_sdio_write'
drivers/net/wireless/ath/ath10k/sdio.c:41:12: note: previous declaration of=
'ath10k_sdio_write' was here
drivers/net/wireless/ath/ath10k/sdio.c:39:12: warning: 'ath10k_sdio_read' u=
sed but never defined
drivers/net/wireless/ath/ath10k/sdio.c:41:12: warning: 'ath10k_sdio_write' =
used but never defined

gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

I fixed it like below in the pending branch. But I'll review more
carefully later, I have quite a lot of patches pending right now.

--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -37,9 +37,9 @@
#define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
=20
static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
- u32 len, bool incr);
+ size_t len, bool incr);
static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
- u32 len, bool incr);
+ size_t len, bool incr);
=20
/* inlined helper functions */

--=20
Kalle Valo=

2017-10-02 09:06:45

[permalink] [raw]

Subject: Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

Hi Alagu,

On 2017-10-02 09:02, Alagu Sankar wrote:
> Hi Steve,
>
> On 2017-10-02 04:17, Steve deRosier wrote:
>> Hi Alagu,
>>
>>
>> On Sat, Sep 30, 2017 at 10:37 AM, <[email protected]> wrote:
>>>
>>> From: Alagu Sankar <[email protected]>
>>>
>>> The QCA9377-3 WB396 sdio reference card does not get initialized
>>> due to the conflict in uart gpio pins. This fix is not required
>>> for other QCA9377 sdio cards.
>>>
>>> Signed-off-by: Alagu Sankar <[email protected]>
>>> ---
>>> drivers/net/wireless/ath/ath10k/core.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
>>> index b4f66cd..86247c8 100644
>>> --- a/drivers/net/wireless/ath/ath10k/core.c
>>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>>> @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
>>>                 return ret;
>>>         }
>>>
>>> -       if (!uart_print)
>>> +       if (!uart_print) {
>>> +               /* Hack: override dbg TX pin to avoid side effects of default
>>> +                * GPIO_6 in QCA9377 WB396 reference card
>>> +                */
>>> +               if (ar->hif.bus == ATH10K_BUS_SDIO)
>>> +                       ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
>>> +                                          ar->hw_params.uart_pin);
>>
>> If it is indeed a "hack", then I don't think the maintainer should
>> accept this upstream. If you want it upstream you need a clean enough
>> implementation that doesn't need to be labeled a "hack".
>
> It is a hack as per the qcacld reference driver.
>
>> Your commit message states that this is only needed for a very
>> specific card and not for other QCA9377 sdio cards. Yet, you're doing
>> this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
>> quirk and it's limited to a particular implementation of the device.
>> My suggestion: if it can be automatically determined, then do so
>> explicitly. If not, then it needs to be a DT setting or a module
>> parameter or something like that so the platform maker can decide to
>> do it. Having it affect all users of a SDIO QCA9377 when it doesn't
>> apply doesn't seem like a good idea to me.
>>
>>
>> - Steve
>
> Got it. The qcacld reference driver had it for all the QCA9377 sdio cards.
> But we found it to be a problem only for the WB396 reference card. Will
> have this checked again and release a v2 patch accordingly.
>
While you are at it, you might as well change the commit comments to:

"ath10k: sdio: <description>"

or perhaps just:

"ath10k: <description>"

> Best Regards,
> Alagu Sankar
>
> _______________________________________________
> ath10k mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k

2017-10-02 07:58:08

[permalink] [raw]

Subject: Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

Hi Steve,

On 2017-10-02 04:17, Steve deRosier wrote:
> Hi Alagu,
>
>
> On Sat, Sep 30, 2017 at 10:37 AM, <[email protected]> wrote:
>>
>> From: Alagu Sankar <[email protected]>
>>
>> The QCA9377-3 WB396 sdio reference card does not get initialized
>> due to the conflict in uart gpio pins. This fix is not required
>> for other QCA9377 sdio cards.
>>
>> Signed-off-by: Alagu Sankar <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/core.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.c
>> b/drivers/net/wireless/ath/ath10k/core.c
>> index b4f66cd..86247c8 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.c
>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>> @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
>> return ret;
>> }
>>
>> - if (!uart_print)
>> + if (!uart_print) {
>> + /* Hack: override dbg TX pin to avoid side effects of
>> default
>> + * GPIO_6 in QCA9377 WB396 reference card
>> + */
>> + if (ar->hif.bus == ATH10K_BUS_SDIO)
>> + ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
>> + ar->hw_params.uart_pin);
>
> If it is indeed a "hack", then I don't think the maintainer should
> accept this upstream. If you want it upstream you need a clean enough
> implementation that doesn't need to be labeled a "hack".

It is a hack as per the qcacld reference driver.

> Your commit message states that this is only needed for a very
> specific card and not for other QCA9377 sdio cards. Yet, you're doing
> this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
> quirk and it's limited to a particular implementation of the device.
> My suggestion: if it can be automatically determined, then do so
> explicitly. If not, then it needs to be a DT setting or a module
> parameter or something like that so the platform maker can decide to
> do it. Having it affect all users of a SDIO QCA9377 when it doesn't
> apply doesn't seem like a good idea to me.
>
>
> - Steve

Got it. The qcacld reference driver had it for all the QCA9377 sdio
cards.
But we found it to be a problem only for the WB396 reference card. Will
have this checked again and release a v2 patch accordingly.

Best Regards,
Alagu Sankar

2017-10-05 15:12:13

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Hi Alagu,

First of all, thank you for sharing your patches, this will be a very
nice improvement to have SDIO QCA9377 working with ath10k.

I've tried your series with Nitrogen7 [1] platform which is supported in
mainline already. It uses BD-SDMAC [2] which uses the same module as the
SX-SDMAC.

Below are some questions/remarks I have after the testing.

On Sat, Sep 30, 2017 at 11:07:37PM +0530, silexcommon at gmail.com wrote:
> From: Alagu Sankar <alagusankar at silex-india.com>
>
> This patchset, generated against master-pending branch, enables a fully
> functional SDIO interface driver for ath10k. Patches have been verified on
> QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point
> and P2P modes.
>
> The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1

Quick question on the firmware, is it the one from Kalle's repository?[3]

If so, where does this firmware comes from? Is 00061 the firmware
version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 output:
Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1

> with the board data from respective SDIO card vendors.

About the board-sdio.bin, is it just a copy of your bdwlan30.bin?

> Receive performance
> matches the QCA reference driver when used with SDIO3.0 enabled platforms.
> iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s

Nice performances. Unfortunately I don't get better than 30Mbits/s in TX
and 50Mbits/s in RX:
# iperf -c 192.168.1.1
------------------------------------------------------------
Client connecting to 192.168.1.1, TCP port 5001
TCP window size: 43.8 KByte (default)
------------------------------------------------------------
[ 3] local 192.168.1.115 port 41354 connected with 192.168.1.1 port
5001
[ ID] Interval Transfer Bandwidth
[ 3] 0.0-10.0 sec 34.9 MBytes 29.2 Mbits/sec
# iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.1.115 port 5001 connected with 192.168.1.1 port
50646
[ ID] Interval Transfer Bandwidth
[ 4] 0.0-10.0 sec 63.1 MBytes 52.7 Mbits/sec

Do you have any idea why? Note that qcacld-2.0 driver on that same
platform (same OS) gives the performances you advertize (150Mbits/s).

> This patchset differs from the previous high latency patches, specific to SDIO.
> HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs the
> firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without
> this flag, the management frames are not sent out by the firmware. Possibility
> of management frames being sent via WMI and data frames through the reduced Tx
> completion needs to be probed further.
>
> Further improvements can be done on the transmit path by implementing packet
> bundle. Scatter Gather is another area of improvement for both Transmit and
> Receive, but may not work on all platforms
>
> Known issues: Surprise removal of the card, when the device is in connected
> state, delays sdio function remove due to delayed WMI command failures.
> Existing ath10k framework can not differentiate between a kernel module
> removae and the surprise removal of teh card.

Here are some questions:
- Is it normal to see a warning about board-2.bin, shouldn't it look for
board-sdio.bin only?
[ 14.696704] ath10k_sdio mmc1:0001:1: Direct firmware load for
ath10k/QCA9377/hw1.0/board-2.bin failed with error -2

- Did you have pre-cal and/or cal binaries for your testing?
[ 14.067159] ath10k_sdio mmc1:0001:1: Direct firmware load for
ath10k/pre-cal-sdio-mmc1:0001:1.bin failed with error -2
[ 14.149257] ath10k_sdio mmc1:0001:1: Direct firmware load for
ath10k/cal-sdio-mmc1:0001:1.bin failed with error -2

Also noticed that the "tx bitrate" output of 'iw link' is wrong in my
case:
# iw wlan0 link
Connected to 00:00:00:00:00:b0 (on wlan0)
SSID: TPLINK_AC_5G
freq: 5180
RX: 72072365 bytes (67934 packets)
TX: 79084128 bytes (73649 packets)
signal: -35 dBm
tx bitrate: 6.0 MBit/s

bss flags: short-slot-time
dtim period: 2
beacon int: 100

When connecting using qcacld driver it shows 433MBit/s as expected. Is
it working properly in your case?

As a FYI, I've built Erik's tree[4] for this testing, should I use
another tree instead?

Let me know your thoughts.

Regards,
Gary

[1] https://boundarydevices.com/product/nitrogen7/
[2] https://boundarydevices.com/product/bd_sdmac_wifi/
[3] https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested
[4] https://github.com/erstrom/linux-ath

2017-10-04 06:22:24

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Hi Erik,

We will work to have this support mainlined as soon as possible. Would
appreciate your support
in making sure that the patches do not affect the USB high latency path.

On 02-10-2017 14:32, Erik Stromdahl wrote:
> Hi Alagu,
>
> It is great to see that we are finally about have fully working
> mainline support for QCA9377 SDIO chipsets!
>
> Great job!
>
> On 2017-09-30 19:37, [email protected] wrote:
>> From: Alagu Sankar <[email protected]>
>>
>> This patchset, generated against master-pending branch, enables a fully
>> functional SDIO interface driver for ath10k. Patches have been
>> verified on
>> QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station,
>> Access Point
>> and P2P modes.
>>
>> The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
>> with the board data from respective SDIO card vendors. Receive
>> performance
>> matches the QCA reference driver when used with SDIO3.0 enabled
>> platforms.
>> iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
>>
> Can you share any scripts etc. (wrapping hostapd and wpa_supplicant
> stuff)
> or provide some more info about you test setup?
>
I am not using any specific scripts for this. The standard ones from the
ath10k configuration page
https://wireless.wiki.kernel.org/en/users/drivers/ath10k/configuration
works just fine with the
NL80211 path.
> I made a quick socat based test on an old laptop (I don't think it has
> SDIO
> 3.0 support) and I did unfortunately not get the same figures as you
> did :(
>
If it is SDIO v1.x, then the max bus speed is only 100Mbit/s. With v2.x
it is 200Mbit/s and 3.x it is
832 Mbit/s. Throughput primarily depends on it. We used i.MX6 SoloX
Sabre SDB platform
which supports SDIO3.x and could see the maximum performance.
>> This patchset differs from the previous high latency patches,
>> specific to SDIO.
>> HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This
>> instructs the
>> firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets.
>> Without
>> this flag, the management frames are not sent out by the firmware.
>> Possibility
>> of management frames being sent via WMI and data frames through the
>> reduced Tx
>> completion needs to be probed further.
>>
> Ah, so that explains why I couldn't see any messages in the air.
>
>> Further improvements can be done on the transmit path by implementing
>> packet
>> bundle. Scatter Gather is another area of improvement for both
>> Transmit and
>> Receive, but may not work on all platforms
>>
>> Known issues: Surprise removal of the card, when the device is in
>> connected
>> state, delays sdio function remove due to delayed WMI command failures.
>> Existing ath10k framework can not differentiate between a kernel module
>> removal and the surprise removal of teh card.
>>
>> Alagu Sankar (11):
>> ath10k_sdio: sdio htt data transfer fixes
>> ath10k_sdio: wb396 reference card fix
>> ath10k_sdio: DMA bounce buffers for read write
>> ath10k_sdio: reduce transmit msdu count
>> ath10k_sdio: use clean packet headers
>> ath10k_sdio: high latency fixes for beacon buffer
>> ath10k_sdio: fix rssi indication
>> ath10k_sdio: common read write
>> ath10k_sdio: virtual scatter gather for receive
>> ath10k_sdio: enable firmware crash dump
>> ath10k_sdio: hif start once addition
>>
>> drivers/net/wireless/ath/ath10k/core.c | 35 ++-
>> drivers/net/wireless/ath/ath10k/debug.c | 3 +
>> drivers/net/wireless/ath/ath10k/htc.c | 4 +-
>> drivers/net/wireless/ath/ath10k/htc.h | 1 +
>> drivers/net/wireless/ath/ath10k/htt_rx.c | 19 +-
>> drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +-
>> drivers/net/wireless/ath/ath10k/hw.c | 2 +
>> drivers/net/wireless/ath/ath10k/hw.h | 1 +
>> drivers/net/wireless/ath/ath10k/mac.c | 31 ++-
>> drivers/net/wireless/ath/ath10k/sdio.c | 398
>> ++++++++++++++++++++++--------
>> drivers/net/wireless/ath/ath10k/sdio.h | 10 +-
>> drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 +-
>> 12 files changed, 403 insertions(+), 127 deletions(-)
>>
>

2017-10-05 10:09:39

[permalink] [raw]

Subject: Re: [08/11] ath10k_sdio: common read write

Hi Alagu,

On Sat, Sep 30, 2017 at 11:07:45PM +0530, [email protected] wrote:
> From: Alagu Sankar <[email protected]>
>
> convert different read write functions in sdio hif to bring it under a
> single read-write path. This helps in having a common dma bounce buffer
> implementation. Also helps in address modification that is required
> specific to change in certain mbox addresses of sdio_write.
>
> Signed-off-by: Alagu Sankar <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/sdio.c | 131 ++++++++++++++++-----------------
> 1 file changed, 64 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index 77d4fa4..bb6fa67 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -36,6 +36,11 @@
>
> #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
>
> +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
> + u32 len, bool incr);
> +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
> + u32 len, bool incr);
> +

As mentioned by Kalle, u32 needs to be size_t.

> /* inlined helper functions */
>
> /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
> @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
> struct sdio_func *func = ar_sdio->func;
> unsigned char byte, asyncintdelay = 2;
> int ret;
> + u32 addr;
>
> ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");
>
> @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
> CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
> CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);
>
> - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
> - CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> - byte);
> + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);

Not sure this part is needed.

> if (ret) {
> ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
> goto out;
> @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)
>
> static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
> {
> - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> - struct sdio_func *func = ar_sdio->func;
> + __le32 *buf;
> int ret;
>
> - sdio_claim_host(func);
> + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
>
> - sdio_writel(func, val, addr, &ret);
> + *buf = cpu_to_le32(val);
> +
> + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);

Shouldn't we use buf instead of val? buf seems pretty useless otherwise.

Regards,
Gary

2017-10-06 11:16:19

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Hi Alagu,

On Thu, Oct 05, 2017 at 10:54:26PM +0530, Alagu Sankar wrote:
> Hi Gary,
>
> On 05-10-2017 20:42, Gary Bisson wrote:
> > Hi Alagu,
> >
> > First of all, thank you for sharing your patches, this will be a very
> > nice improvement to have SDIO QCA9377 working with ath10k.
> >
> > I've tried your series with Nitrogen7 [1] platform which is supported in
> > mainline already. It uses BD-SDMAC [2] which uses the same module as the
> > SX-SDMAC.
> >
> > Below are some questions/remarks I have after the testing.
> >
> > On Sat, Sep 30, 2017 at 11:07:37PM +0530, silexcommon at gmail.com wrote:
> > > From: Alagu Sankar <alagusankar at silex-india.com>
> > >
> > > This patchset, generated against master-pending branch, enables a fully
> > > functional SDIO interface driver for ath10k. Patches have been verified on
> > > QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point
> > > and P2P modes.
> > >
> > > The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
> > Quick question on the firmware, is it the one from Kalle's repository?[3]
> >
> > If so, where does this firmware comes from? Is 00061 the firmware
> > version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 output:
> > Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1
> Yes, it is from
> https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested.
> I have also used custom firmware from QCA/Silex as used in qcacld-2.0 driver
> without any issue. You need to use the firmware merger tool from
> https://github.com/erstrom/linux-ath/wiki/Firmware to combine the
> qwlan30.bin and otp30.bin to generate the firmware-sdio.bin.

Good to know, thanks. Maybe Kalle can tell us more about the firmware
itself, what's the difference between the version 0.0.0.60 and 0.0.0.61?

> > > with the board data from respective SDIO card vendors.
> > About the board-sdio.bin, is it just a copy of your bdwlan30.bin?
> Yes board-sdio.bin is a copy of bdwlan30.bin

Thanks for confirming it.

> > > Receive performance
> > > matches the QCA reference driver when used with SDIO3.0 enabled platforms.
> > > iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
> > Nice performances. Unfortunately I don't get better than 30Mbits/s in TX
> > and 50Mbits/s in RX:
> > # iperf -c 192.168.1.1
> > ------------------------------------------------------------
> > Client connecting to 192.168.1.1, TCP port 5001
> > TCP window size: 43.8 KByte (default)
> > ------------------------------------------------------------
> > [ 3] local 192.168.1.115 port 41354 connected with 192.168.1.1 port
> > 5001
> > [ ID] Interval Transfer Bandwidth
> > [ 3] 0.0-10.0 sec 34.9 MBytes 29.2 Mbits/sec
> > # iperf -s
> > ------------------------------------------------------------
> > Server listening on TCP port 5001
> > TCP window size: 85.3 KByte (default)
> > ------------------------------------------------------------
> > [ 4] local 192.168.1.115 port 5001 connected with 192.168.1.1 port
> > 50646
> > [ ID] Interval Transfer Bandwidth
> > [ 4] 0.0-10.0 sec 63.1 MBytes 52.7 Mbits/sec
> >
> > Do you have any idea why? Note that qcacld-2.0 driver on that same
> > platform (same OS) gives the performances you advertize (150Mbits/s).
> For some reason, if I use the imx_v6_v7_defconfig as is, performance is very
> poor. In fact, the firmware download itself will take about 6 seconds. This
> can also be seen when you up/down the wlan0 interface. For the i.MX6 SoloX
> board, I used the configuration of 4.1.15 as provided by the BSP from
> NXP/Freescale.

Do you mean that you've used 4.1.15 kernel or just the 4.1.15
configuration on latest 4.14?

> This improved the performance quite a bit. I haven't had a
> chance to spend time on this to figure out the difference and reason.
> Another difference is that the default device tree for SoloX did not have
> the correct settings to support SDIO3.x. Had to modify them, but did not
> include the device tree patches here as it is not meant for this group.

Doh! That's why I don't get better performances, my platform limits the
SDIO bus to 50MHz right now.

Unfortunately SDIO3.0 doesn't seem stable on i.MX7, it is missing a few
patches, I'll start a thread with Shawn/Fabio about it.

> > > This patchset differs from the previous high latency patches, specific to SDIO.
> > > HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs the
> > > firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without
> > > this flag, the management frames are not sent out by the firmware. Possibility
> > > of management frames being sent via WMI and data frames through the reduced Tx
> > > completion needs to be probed further.
> > >
> > > Further improvements can be done on the transmit path by implementing packet
> > > bundle. Scatter Gather is another area of improvement for both Transmit and
> > > Receive, but may not work on all platforms
> > >
> > > Known issues: Surprise removal of the card, when the device is in connected
> > > state, delays sdio function remove due to delayed WMI command failures.
> > > Existing ath10k framework can not differentiate between a kernel module
> > > removae and the surprise removal of teh card.
> > Here are some questions:
> > - Is it normal to see a warning about board-2.bin, shouldn't it look for
> > board-sdio.bin only?
> > [ 14.696704] ath10k_sdio mmc1:0001:1: Direct firmware load for
> > ath10k/QCA9377/hw1.0/board-2.bin failed with error -2
> This was only noticed in the latest update. Like the different firmware
> versions, the driver also looks for the board-2.bin now. I have only tested
> with the board-sdio.bin

Good, thanks.

> > - Did you have pre-cal and/or cal binaries for your testing?
> > [ 14.067159] ath10k_sdio mmc1:0001:1: Direct firmware load for
> > ath10k/pre-cal-sdio-mmc1:0001:1.bin failed with error -2
> > [ 14.149257] ath10k_sdio mmc1:0001:1: Direct firmware load for
> > ath10k/cal-sdio-mmc1:0001:1.bin failed with error -2
> No. I did not use the pre-cal and cal binaries.
> > Also noticed that the "tx bitrate" output of 'iw link' is wrong in my
> > case:
> > # iw wlan0 link
> > Connected to 00:00:00:00:00:b0 (on wlan0)
> > SSID: TPLINK_AC_5G
> > freq: 5180
> > RX: 72072365 bytes (67934 packets)
> > TX: 79084128 bytes (73649 packets)
> > signal: -35 dBm
> > tx bitrate: 6.0 MBit/s
> >
> > bss flags: short-slot-time
> > dtim period: 2
> > beacon int: 100
> >
> > When connecting using qcacld driver it shows 433MBit/s as expected. Is
> > it working properly in your case?
> Tx rate is not updated properly. I will include it in the list of known
> issues.

I'll try to see if it is complex to fix, I had a similar issue on qcacld
which was straightforward to fix.

Regards,
Gary

2017-10-02 09:02:55

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Hi Alagu,

It is great to see that we are finally about have fully working
mainline support for QCA9377 SDIO chipsets!

Great job!

On 2017-09-30 19:37, [email protected] wrote:
> From: Alagu Sankar <[email protected]>
>
> This patchset, generated against master-pending branch, enables a fully
> functional SDIO interface driver for ath10k. Patches have been verified on
> QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point
> and P2P modes.
>
> The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
> with the board data from respective SDIO card vendors. Receive performance
> matches the QCA reference driver when used with SDIO3.0 enabled platforms.
> iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
>
Can you share any scripts etc. (wrapping hostapd and wpa_supplicant stuff)
or provide some more info about you test setup?

I made a quick socat based test on an old laptop (I don't think it has SDIO
3.0 support) and I did unfortunately not get the same figures as you did :(

> This patchset differs from the previous high latency patches, specific to SDIO.
> HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs the
> firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without
> this flag, the management frames are not sent out by the firmware. Possibility
> of management frames being sent via WMI and data frames through the reduced Tx
> completion needs to be probed further.
>
Ah, so that explains why I couldn't see any messages in the air.

> Further improvements can be done on the transmit path by implementing packet
> bundle. Scatter Gather is another area of improvement for both Transmit and
> Receive, but may not work on all platforms
>
> Known issues: Surprise removal of the card, when the device is in connected
> state, delays sdio function remove due to delayed WMI command failures.
> Existing ath10k framework can not differentiate between a kernel module
> removal and the surprise removal of teh card.
>
> Alagu Sankar (11):
> ath10k_sdio: sdio htt data transfer fixes
> ath10k_sdio: wb396 reference card fix
> ath10k_sdio: DMA bounce buffers for read write
> ath10k_sdio: reduce transmit msdu count
> ath10k_sdio: use clean packet headers
> ath10k_sdio: high latency fixes for beacon buffer
> ath10k_sdio: fix rssi indication
> ath10k_sdio: common read write
> ath10k_sdio: virtual scatter gather for receive
> ath10k_sdio: enable firmware crash dump
> ath10k_sdio: hif start once addition
>
> drivers/net/wireless/ath/ath10k/core.c | 35 ++-
> drivers/net/wireless/ath/ath10k/debug.c | 3 +
> drivers/net/wireless/ath/ath10k/htc.c | 4 +-
> drivers/net/wireless/ath/ath10k/htc.h | 1 +
> drivers/net/wireless/ath/ath10k/htt_rx.c | 19 +-
> drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +-
> drivers/net/wireless/ath/ath10k/hw.c | 2 +
> drivers/net/wireless/ath/ath10k/hw.h | 1 +
> drivers/net/wireless/ath/ath10k/mac.c | 31 ++-
> drivers/net/wireless/ath/ath10k/sdio.c | 398 ++++++++++++++++++++++--------
> drivers/net/wireless/ath/ath10k/sdio.h | 10 +-
> drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 +-
> 12 files changed, 403 insertions(+), 127 deletions(-)
>

2017-10-05 17:24:36

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Hi Gary,

On 05-10-2017 20:42, Gary Bisson wrote:
> Hi Alagu,
>
> First of all, thank you for sharing your patches, this will be a very
> nice improvement to have SDIO QCA9377 working with ath10k.
>
> I've tried your series with Nitrogen7 [1] platform which is supported in
> mainline already. It uses BD-SDMAC [2] which uses the same module as the
> SX-SDMAC.
>
> Below are some questions/remarks I have after the testing.
>
> On Sat, Sep 30, 2017 at 11:07:37PM +0530, silexcommon at gmail.com wrote:
>> From: Alagu Sankar <alagusankar at silex-india.com>
>>
>> This patchset, generated against master-pending branch, enables a fully
>> functional SDIO interface driver for ath10k. Patches have been verified on
>> QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point
>> and P2P modes.
>>
>> The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
> Quick question on the firmware, is it the one from Kalle's repository?[3]
>
> If so, where does this firmware comes from? Is 00061 the firmware
> version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 output:
> Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1
Yes, it is from
https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested.
I have also used custom firmware from QCA/Silex as used in qcacld-2.0
driver without any issue. You need to use the firmware merger tool from
https://github.com/erstrom/linux-ath/wiki/Firmware to combine the
qwlan30.bin and otp30.bin to generate the firmware-sdio.bin.
>> with the board data from respective SDIO card vendors.
> About the board-sdio.bin, is it just a copy of your bdwlan30.bin?
Yes board-sdio.bin is a copy of bdwlan30.bin
>> Receive performance
>> matches the QCA reference driver when used with SDIO3.0 enabled platforms.
>> iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
> Nice performances. Unfortunately I don't get better than 30Mbits/s in TX
> and 50Mbits/s in RX:
> # iperf -c 192.168.1.1
> ------------------------------------------------------------
> Client connecting to 192.168.1.1, TCP port 5001
> TCP window size: 43.8 KByte (default)
> ------------------------------------------------------------
> [ 3] local 192.168.1.115 port 41354 connected with 192.168.1.1 port
> 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 34.9 MBytes 29.2 Mbits/sec
> # iperf -s
> ------------------------------------------------------------
> Server listening on TCP port 5001
> TCP window size: 85.3 KByte (default)
> ------------------------------------------------------------
> [ 4] local 192.168.1.115 port 5001 connected with 192.168.1.1 port
> 50646
> [ ID] Interval Transfer Bandwidth
> [ 4] 0.0-10.0 sec 63.1 MBytes 52.7 Mbits/sec
>
> Do you have any idea why? Note that qcacld-2.0 driver on that same
> platform (same OS) gives the performances you advertize (150Mbits/s).
For some reason, if I use the imx_v6_v7_defconfig as is, performance is
very poor. In fact, the firmware download itself will take about 6
seconds. This can also be seen when you up/down the wlan0 interface. For
the i.MX6 SoloX board, I used the configuration of 4.1.15 as provided by
the BSP from NXP/Freescale. This improved the performance quite a bit.
I haven't had a chance to spend time on this to figure out the
difference and reason. Another difference is that the default device
tree for SoloX did not have the correct settings to support SDIO3.x.
Had to modify them, but did not include the device tree patches here as
it is not meant for this group.
>> This patchset differs from the previous high latency patches, specific to SDIO.
>> HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs the
>> firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without
>> this flag, the management frames are not sent out by the firmware. Possibility
>> of management frames being sent via WMI and data frames through the reduced Tx
>> completion needs to be probed further.
>>
>> Further improvements can be done on the transmit path by implementing packet
>> bundle. Scatter Gather is another area of improvement for both Transmit and
>> Receive, but may not work on all platforms
>>
>> Known issues: Surprise removal of the card, when the device is in connected
>> state, delays sdio function remove due to delayed WMI command failures.
>> Existing ath10k framework can not differentiate between a kernel module
>> removae and the surprise removal of teh card.
> Here are some questions:
> - Is it normal to see a warning about board-2.bin, shouldn't it look for
> board-sdio.bin only?
> [ 14.696704] ath10k_sdio mmc1:0001:1: Direct firmware load for
> ath10k/QCA9377/hw1.0/board-2.bin failed with error -2
This was only noticed in the latest update. Like the different firmware
versions, the driver also looks for the board-2.bin now. I have only
tested with the board-sdio.bin
> - Did you have pre-cal and/or cal binaries for your testing?
> [ 14.067159] ath10k_sdio mmc1:0001:1: Direct firmware load for
> ath10k/pre-cal-sdio-mmc1:0001:1.bin failed with error -2
> [ 14.149257] ath10k_sdio mmc1:0001:1: Direct firmware load for
> ath10k/cal-sdio-mmc1:0001:1.bin failed with error -2
No. I did not use the pre-cal and cal binaries.
> Also noticed that the "tx bitrate" output of 'iw link' is wrong in my
> case:
> # iw wlan0 link
> Connected to 00:00:00:00:00:b0 (on wlan0)
> SSID: TPLINK_AC_5G
> freq: 5180
> RX: 72072365 bytes (67934 packets)
> TX: 79084128 bytes (73649 packets)
> signal: -35 dBm
> tx bitrate: 6.0 MBit/s
>
> bss flags: short-slot-time
> dtim period: 2
> beacon int: 100
>
> When connecting using qcacld driver it shows 433MBit/s as expected. Is
> it working properly in your case?
Tx rate is not updated properly. I will include it in the list of known
issues.
> As a FYI, I've built Erik's tree[4] for this testing, should I use
> another tree instead?
I use the Kalle's ath10k tree, but when I last looked, they were pretty
much the same, so it should not be a problem.
> Let me know your thoughts.
>
> Regards,
> Gary
>
> [1] https://boundarydevices.com/product/nitrogen7/
> [2] https://boundarydevices.com/product/bd_sdmac_wifi/
> [3] https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested
> [4] https://github.com/erstrom/linux-ath
>
Regards,
Alagu

2017-10-04 08:55:52

[permalink] [raw]

Subject: Re: [PATCH 01/11] ath10k_sdio: sdio htt data transfer fixes

Arend van Spriel <[email protected]> writes:

> On 9/30/2017 7:37 PM, [email protected] wrote:
>> From: Alagu Sankar <[email protected]>
>>
>
> [...]
>
>>
>> Signed-off-by: Alagu Sankar <[email protected]>
>
> Not really have a specific remark for this patch, but for the entire
> series. These patches are sent using an anonymous email address, apart
> from 'silex' being in there, which does not show up in the certificate
> of origin. Just wondering if this is acceptable?

As long as there's the "correct" From header as the first line in the
commit log, which git then uses as the author instead of the From line
from email header. And I see Alagu doing that here so there shouldn't be
any problems.

--=20
Kalle Valo=

2017-10-02 07:36:54

by Arend van Spriel

[permalink] [raw]

Subject: Re: [PATCH 01/11] ath10k_sdio: sdio htt data transfer fixes

On 9/30/2017 7:37 PM, [email protected] wrote:
> From: Alagu Sankar <[email protected]>
>

[...]

>
> Signed-off-by: Alagu Sankar <[email protected]>

Not really have a specific remark for this patch, but for the entire
series. These patches are sent using an anonymous email address, apart
from 'silex' being in there, which does not show up in the certificate
of origin. Just wondering if this is acceptable?

Regards,
Arend

> ---
> drivers/net/wireless/ath/ath10k/core.c | 20 +++++++++++++++++---
> drivers/net/wireless/ath/ath10k/htt_rx.c | 6 ++++--
> drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +++++++++++++++++++-----
> 3 files changed, 40 insertions(+), 10 deletions(-)

2017-10-02 07:44:21

[permalink] [raw]

Subject: Re: [PATCH 01/11] ath10k_sdio: sdio htt data transfer fixes

Hi Arend,

On 2017-10-02 13:06, Arend van Spriel wrote:
> On 9/30/2017 7:37 PM, [email protected] wrote:
>> From: Alagu Sankar <[email protected]>
>>
>
> [...]
>
>>
>> Signed-off-by: Alagu Sankar <[email protected]>
>
> Not really have a specific remark for this patch, but for the entire
> series. These patches are sent using an anonymous email address, apart
> from 'silex' being in there, which does not show up in the certificate
> of origin. Just wondering if this is acceptable?
>
> Regards,
> Arend
>
>> ---
>> drivers/net/wireless/ath/ath10k/core.c | 20 +++++++++++++++++---
>> drivers/net/wireless/ath/ath10k/htt_rx.c | 6 ++++--
>> drivers/net/wireless/ath/ath10k/htt_tx.c | 24
>> +++++++++++++++++++-----
>> 3 files changed, 40 insertions(+), 10 deletions(-)
>
Could not use git send-email from the official ID due to mail server
restrictions. If this is not acceptable, I will figure out a way to
overcome this.

Regards,
Alagu Sankar

2017-10-04 19:56:17

[permalink] [raw]

Subject: Re: [PATCH 09/11] ath10k_sdio: virtual scatter gather for receive

On 2017-09-30 19:37, [email protected] wrote:
> From: Alagu Sankar <[email protected]>
>
> The existing implementation of initiating multiple sdio transfers for
> receive bundling is slowing down the receive speed. Combining the
> transfers using a scatter gather method would be ideal. This results in
> significant performance improvement.
>
> Since the sg implementation for sdio transfers are not reliable due to
> buffer start and size alignment, a virtual scatter gather implementation
> is used.
>
> Signed-off-by: Alagu Sankar <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/htc.h | 1 +
> drivers/net/wireless/ath/ath10k/sdio.c | 122 ++++++++++++++++++++++++---------
> drivers/net/wireless/ath/ath10k/sdio.h | 5 +-
> 3 files changed, 93 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
> index 24663b0..5d87908 100644
> --- a/drivers/net/wireless/ath/ath10k/htc.h
> +++ b/drivers/net/wireless/ath/ath10k/htc.h
> @@ -58,6 +58,7 @@ enum ath10k_htc_tx_flags {
> };
>
> enum ath10k_htc_rx_flags {
> + ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK = 0x01,
> ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02,
> ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0
> };
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index bb6fa67..45df9db 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -35,6 +35,7 @@
> #include "sdio.h"
>
> #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
> +#define ATH10K_SDIO_VSG_BUF_SIZE (32 * 1024)
>
> static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
> u32 len, bool incr);
> @@ -430,6 +431,7 @@ static int ath10k_sdio_mbox_rx_process_packet(struct ath10k *ar,
> int ret;
>
> payload_len = le16_to_cpu(htc_hdr->len);
> + skb->len = payload_len + sizeof(struct ath10k_htc_hdr);
>
> if (trailer_present) {
> trailer = skb->data + sizeof(*htc_hdr) +
> @@ -468,12 +470,13 @@ static int ath10k_sdio_mbox_rx_process_packets(struct ath10k *ar,
> enum ath10k_htc_ep_id id;
> int ret, i, *n_lookahead_local;
> u32 *lookaheads_local;
> + int lookahd_idx = 0;

I think the variable should be named *lookahead_idx* instead of *lookahd_idx*,
since all other variables are using the string lookahead without abbreviations.

>
> for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> lookaheads_local = lookaheads;
> n_lookahead_local = n_lookahead;
>
> - id = ((struct ath10k_htc_hdr *)&lookaheads[i])->eid;
> + id = ((struct ath10k_htc_hdr *)&lookaheads[lookahd_idx++])->eid;
>
> if (id >= ATH10K_HTC_EP_COUNT) {
> ath10k_warn(ar, "invalid endpoint in look-ahead: %d\n",
> @@ -496,6 +499,7 @@ static int ath10k_sdio_mbox_rx_process_packets(struct ath10k *ar,
> /* Only read lookahead's from RX trailers
> * for the last packet in a bundle.
> */
> + lookahd_idx--;
> lookaheads_local = NULL;
> n_lookahead_local = NULL;
> }
> @@ -529,11 +533,11 @@ static int ath10k_sdio_mbox_rx_process_packets(struct ath10k *ar,
> return ret;
> }
>
> -static int ath10k_sdio_mbox_alloc_pkt_bundle(struct ath10k *ar,
> - struct ath10k_sdio_rx_data *rx_pkts,
> - struct ath10k_htc_hdr *htc_hdr,
> - size_t full_len, size_t act_len,
> - size_t *bndl_cnt)
> +static int ath10k_sdio_mbox_alloc_bundle(struct ath10k *ar,
> + struct ath10k_sdio_rx_data *rx_pkts,
> + struct ath10k_htc_hdr *htc_hdr,
> + size_t full_len, size_t act_len,
> + size_t *bndl_cnt)
> {
> int ret, i;
>
> @@ -574,6 +578,7 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
> size_t full_len, act_len;
> bool last_in_bundle;
> int ret, i;
> + int pkt_cnt = 0;
>
> if (n_lookaheads > ATH10K_SDIO_MAX_RX_MSGS) {
> ath10k_warn(ar,
> @@ -616,16 +621,22 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
> * optimally fetched as a full bundle.
> */
> size_t bndl_cnt;
> -
> - ret = ath10k_sdio_mbox_alloc_pkt_bundle(ar,
> - &ar_sdio->rx_pkts[i],
> - htc_hdr,
> - full_len,
> - act_len,
> - &bndl_cnt);
> -
> - n_lookaheads += bndl_cnt;
> - i += bndl_cnt;
> + struct ath10k_sdio_rx_data *rx_pkts =
> + &ar_sdio->rx_pkts[pkt_cnt];
> +
> + ret = ath10k_sdio_mbox_alloc_bundle(ar,
> + rx_pkts,
> + htc_hdr,
> + full_len,
> + act_len,
> + &bndl_cnt);
> +
> + if (ret) {
> + ath10k_warn(ar, "alloc_bundle error %d\n", ret);
> + goto err;
> + }
> +
> + pkt_cnt += bndl_cnt;
> /*Next buffer will be the last in the bundle */
> last_in_bundle = true;
> }
> @@ -634,14 +645,18 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
> * ATH10K_HTC_FLAG_BUNDLE_MASK flag set, all bundled
> * packet skb's have been allocated in the previous step.
> */
> - ret = ath10k_sdio_mbox_alloc_rx_pkt(&ar_sdio->rx_pkts[i],
> + if (htc_hdr->flags & ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK)
> + full_len += ATH10K_HIF_MBOX_BLOCK_SIZE;
> +
> + ret = ath10k_sdio_mbox_alloc_rx_pkt(&ar_sdio->rx_pkts[pkt_cnt],
> act_len,
> full_len,
> last_in_bundle,
> last_in_bundle);
> + pkt_cnt++;
> }
>
> - ar_sdio->n_rx_pkts = i;
> + ar_sdio->n_rx_pkts = pkt_cnt;
>
> return 0;
>
> @@ -655,41 +670,71 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
> return ret;
> }
>
> -static int ath10k_sdio_mbox_rx_packet(struct ath10k *ar,
> - struct ath10k_sdio_rx_data *pkt)
> +static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
> {
> struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> - struct sk_buff *skb = pkt->skb;
> + struct ath10k_sdio_rx_data *pkt = &ar_sdio->rx_pkts[0];
> + struct sk_buff *skb;
> int ret;
>
> + skb = pkt->skb;
> ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
> skb->data, pkt->alloc_len, false);
> - pkt->status = ret;
> - if (!ret)
> + if (ret) {
> + ar_sdio->n_rx_pkts = 0;
> + ath10k_sdio_mbox_free_rx_pkt(pkt);
> + } else {
> + pkt->status = ret;
> skb_put(skb, pkt->act_len);
> + }
>
> return ret;
> }
>
> -static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
> +static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
> {
> struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> + struct ath10k_sdio_rx_data *pkt;
> int ret, i;
> + u32 pkt_offset, virt_pkt_len;
>
> + virt_pkt_len = 0;
> for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> - ret = ath10k_sdio_mbox_rx_packet(ar,
> - &ar_sdio->rx_pkts[i]);
> - if (ret)
> + virt_pkt_len += ar_sdio->rx_pkts[i].alloc_len;
> + }
> + if (virt_pkt_len < ATH10K_SDIO_DMA_BUF_SIZE) {
> + ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
> + ar_sdio->vsg_buffer, virt_pkt_len,
> + false);
> + if (ret) {
> + i = 0;
> goto err;
> + }
> + } else {
> + ath10k_err(ar, "size exceeding limit %d\n", virt_pkt_len);
> + }
> +
> + pkt_offset = 0;
> + for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> + struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
> +
> + pkt = &ar_sdio->rx_pkts[i];
> + memcpy(skb->data, ar_sdio->vsg_buffer + pkt_offset,
> + pkt->alloc_len);
> + pkt->status = 0;
> + skb_put(skb, pkt->act_len);
> + pkt_offset += pkt->alloc_len;
> }
>
> return 0;
>
> err:
> /* Free all packets that was not successfully fetched. */

Change comment to: /* Free all packets */
since all packets are freed and not only those that was not successfully fetched.

> - for (; i < ar_sdio->n_rx_pkts; i++)
> + for (i = 0; i < ar_sdio->n_rx_pkts; i++)
> ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);
>
> + ar_sdio->n_rx_pkts = 0;
> +
> return ret;
> }
>
> @@ -732,7 +777,10 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar,
> */
> *done = false;
>
> - ret = ath10k_sdio_mbox_rx_fetch(ar);
> + if (ar_sdio->n_rx_pkts > 1)
> + ret = ath10k_sdio_mbox_rx_fetch_bundle(ar);
> + else
> + ret = ath10k_sdio_mbox_rx_fetch(ar);

ret is not checked at all (I noticed this is the case in the current code as well).
I think it would be wise to break the loop if error:

if (ret)
break;

>
> /* Process fetched packets. This will potentially update
> * n_lookaheads depending on if the packets contain lookahead
> @@ -1136,7 +1184,7 @@ static int ath10k_sdio_bmi_get_rx_lookahead(struct ath10k *ar)
> MBOX_HOST_INT_STATUS_ADDRESS,
> &rx_word);
> if (ret) {
> - ath10k_warn(ar, "unable to read RX_LOOKAHEAD_VALID: %d\n", ret);
> + ath10k_warn(ar, "unable to read rx_lookahd: %d\n", ret);

Change print to "unable to read RX lookahead: %d\n" as it is more descriptive

> return ret;
> }
>
> @@ -1480,7 +1528,7 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
> skb = items[i].transfer_context;
> padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
> skb->len);
> - skb_trim(skb, padded_len);
> + skb->len = padded_len;

Why this change?
I think the skb_ family of functions is the preferred way to manipulate skb's

>
> /* Write TX data to the end of the mbox address space */
> address = ar_sdio->mbox_addr[eid] + ar_sdio->mbox_size[eid] -
> @@ -1508,7 +1556,8 @@ static int ath10k_sdio_hif_enable_intrs(struct ath10k *ar)
> /* Enable all but CPU interrupts */
> regs->int_status_en = FIELD_PREP(MBOX_INT_STATUS_ENABLE_ERROR_MASK, 1) |
> FIELD_PREP(MBOX_INT_STATUS_ENABLE_CPU_MASK, 1) |
> - FIELD_PREP(MBOX_INT_STATUS_ENABLE_COUNTER_MASK, 1);
> + FIELD_PREP(MBOX_INT_STATUS_ENABLE_COUNTER_MASK,
> + 1);

Is this a checkpatch-fix?
I would recommend creating a separate patch for style issues.

>
> /* NOTE: There are some cases where HIF can do detection of
> * pending mbox messages which is disabled now.
> @@ -2024,6 +2073,12 @@ static int ath10k_sdio_probe(struct sdio_func *func,
> goto err_free_bmi_buf;
> }
>
> + ar_sdio->vsg_buffer = kzalloc(ATH10K_SDIO_VSG_BUF_SIZE, GFP_KERNEL);
> + if (!ar_sdio->vsg_buffer) {
> + ret = -ENOMEM;
> + goto err_free_bmi_buf;
> + }
> +
> ar_sdio->func = func;
> sdio_set_drvdata(func, ar_sdio);
>
> @@ -2081,7 +2136,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
> }
>
> /* TODO: remove this once SDIO support is fully implemented */
> - ath10k_warn(ar, "WARNING: ath10k SDIO support is incomplete, don't expect anything to work!\n");
> + ath10k_warn(ar, "WARNING: ath10k SDIO support is experimental\n");
>
> return 0;
>
> @@ -2115,6 +2170,7 @@ static void ath10k_sdio_remove(struct sdio_func *func)
> ath10k_core_unregister(ar);
> ath10k_core_destroy(ar);
> kfree(ar_sdio->dma_buffer);
> + kfree(ar_sdio->vsg_buffer);
> }
>
> static const struct sdio_device_id ath10k_sdio_devices[] = {
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
> index 718b8b7..8b6a86a 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.h
> +++ b/drivers/net/wireless/ath/ath10k/sdio.h
> @@ -149,8 +149,8 @@ struct ath10k_sdio_irq_proc_regs {
> u8 rx_lookahead_valid;
> u8 host_int_status2;
> u8 gmbox_rx_avail;
> - __le32 rx_lookahead[2];
> - __le32 rx_gmbox_lookahead_alias[2];
> + __le32 rx_lookahead[2 * ATH10K_HIF_MBOX_NUM_MAX];
> + __le32 int_status_enable;
> };
>
> struct ath10k_sdio_irq_enable_regs {
> @@ -207,6 +207,7 @@ struct ath10k_sdio {
> struct ath10k *ar;
> struct ath10k_sdio_irq_data irq_data;
>
> + u8 *vsg_buffer;
> u8 *dma_buffer;
>
> /* protects access to dma_buffer */
>

2017-10-01 22:47:20

by Steve deRosier

[permalink] [raw]

Subject: Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

Hi Alagu,

On Sat, Sep 30, 2017 at 10:37 AM, <[email protected]> wrote:
>
> From: Alagu Sankar <[email protected]>
>
> The QCA9377-3 WB396 sdio reference card does not get initialized
> due to the conflict in uart gpio pins. This fix is not required
> for other QCA9377 sdio cards.
>
> Signed-off-by: Alagu Sankar <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index b4f66cd..86247c8 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
> return ret;
> }
>
> - if (!uart_print)
> + if (!uart_print) {
> + /* Hack: override dbg TX pin to avoid side effects of default
> + * GPIO_6 in QCA9377 WB396 reference card
> + */
> + if (ar->hif.bus == ATH10K_BUS_SDIO)
> + ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
> + ar->hw_params.uart_pin);

If it is indeed a "hack", then I don't think the maintainer should
accept this upstream. If you want it upstream you need a clean enough
implementation that doesn't need to be labeled a "hack".

Your commit message states that this is only needed for a very
specific card and not for other QCA9377 sdio cards. Yet, you're doing
this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
quirk and it's limited to a particular implementation of the device.
My suggestion: if it can be automatically determined, then do so
explicitly. If not, then it needs to be a DT setting or a module
parameter or something like that so the platform maker can decide to
do it. Having it affect all users of a SDIO QCA9377 when it doesn't
apply doesn't seem like a good idea to me.

- Steve

2017-10-05 17:33:20

[permalink] [raw]

Subject: Re: [08/11] ath10k_sdio: common read write

Hi Gary,

On 2017-10-05 15:39, Gary Bisson wrote:
> Hi Alagu,
>
> On Sat, Sep 30, 2017 at 11:07:45PM +0530, [email protected] wrote:
>> From: Alagu Sankar <[email protected]>
>>
>> convert different read write functions in sdio hif to bring it under a
>> single read-write path. This helps in having a common dma bounce
>> buffer
>> implementation. Also helps in address modification that is required
>> specific to change in certain mbox addresses of sdio_write.
>>
>> Signed-off-by: Alagu Sankar <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/sdio.c | 131
>> ++++++++++++++++-----------------
>> 1 file changed, 64 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c
>> b/drivers/net/wireless/ath/ath10k/sdio.c
>> index 77d4fa4..bb6fa67 100644
>> --- a/drivers/net/wireless/ath/ath10k/sdio.c
>> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
>> @@ -36,6 +36,11 @@
>>
>> #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
>>
>> +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
>> + u32 len, bool incr);
>> +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void
>> *buf,
>> + u32 len, bool incr);
>> +
>
> As mentioned by Kalle, u32 needs to be size_t.
Yes, the compiler I used is probably a step older and did not catch
this.
>
>> /* inlined helper functions */
>>
>> /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
>> @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
>> struct sdio_func *func = ar_sdio->func;
>> unsigned char byte, asyncintdelay = 2;
>> int ret;
>> + u32 addr;
>>
>> ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");
>>
>> @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
>> CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
>> CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);
>>
>> - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
>> - CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
>> - byte);
>> + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
>> + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);
>
> Not sure this part is needed.
This is to overcome checkpatch warning. Too big a names for the function
and macro
not helping in there. Will have to move it as a separate patch.
>
>> if (ret) {
>> ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
>> goto out;
>> @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)
>>
>> static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
>> {
>> - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> - struct sdio_func *func = ar_sdio->func;
>> + __le32 *buf;
>> int ret;
>>
>> - sdio_claim_host(func);
>> + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>>
>> - sdio_writel(func, val, addr, &ret);
>> + *buf = cpu_to_le32(val);
>> +
>> + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);
>
> Shouldn't we use buf instead of val? buf seems pretty useless
> otherwise.
Yes, thanks for pointing this out. will be corrected in v2.
>
> Regards,
> Gary
>
> _______________________________________________
> ath10k mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k

2017-10-04 15:54:00

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

On 2017-10-04 08:22, Alagu Sankar wrote:
> Hi Erik,
>
> We will work to have this support mainlined as soon as possible. Would appreciate your support
> in making sure that the patches do not affect the USB high latency path.
>
I have added the patches in my own ath-repo and I have tested with the
WUSB6100M without any problems.

> On 02-10-2017 14:32, Erik Stromdahl wrote:
>> Hi Alagu,
>>
>> It is great to see that we are finally about have fully working
>> mainline support for QCA9377 SDIO chipsets!
>>
>> Great job!
>>
>> On 2017-09-30 19:37, [email protected] wrote:
>>> From: Alagu Sankar <[email protected]>
>>>
>>> This patchset, generated against master-pending branch, enables a fully
>>> functional SDIO interface driver for ath10k.? Patches have been verified on
>>> QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point
>>> and P2P modes.
>>>
>>> The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
>>> with the board data from respective SDIO card vendors. Receive performance
>>> matches the QCA reference driver when used with SDIO3.0 enabled platforms.
>>> iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
>>>
>> Can you share any scripts etc. (wrapping hostapd and wpa_supplicant stuff)
>> or provide some more info about you test setup?
>>
> I am not using any specific scripts for this. The standard ones from the ath10k configuration page
> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/configuration works just fine with the
> NL80211 path.
>> I made a quick socat based test on an old laptop (I don't think it has SDIO
>> 3.0 support) and I did unfortunately not get the same figures as you did :(
>>
> If it is SDIO v1.x, then the max bus speed is only 100Mbit/s.? With v2.x it is 200Mbit/s and 3.x it is
> 832 Mbit/s.? Throughput primarily depends on it. We used i.MX6 SoloX Sabre SDB platform
> which supports SDIO3.x and could see the maximum performance.
>>> This patchset differs from the previous high latency patches, specific to SDIO.
>>> HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs the
>>> firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without
>>> this flag, the management frames are not sent out by the firmware. Possibility
>>> of management frames being sent via WMI and data frames through the reduced Tx
>>> completion needs to be probed further.
>>>
>> Ah, so that explains why I couldn't see any messages in the air.
>>
>>> Further improvements can be done on the transmit path by implementing packet
>>> bundle. Scatter Gather is another area of improvement for both Transmit and
>>> Receive, but may not work on all platforms
>>>
>>> Known issues: Surprise removal of the card, when the device is in connected
>>> state, delays sdio function remove due to delayed WMI command failures.
>>> Existing ath10k framework can not differentiate between a kernel module
>>> removal and the surprise removal of teh card.
>>>
>>> Alagu Sankar (11):
>>> ?? ath10k_sdio: sdio htt data transfer fixes
>>> ?? ath10k_sdio: wb396 reference card fix
>>> ?? ath10k_sdio: DMA bounce buffers for read write
>>> ?? ath10k_sdio: reduce transmit msdu count
>>> ?? ath10k_sdio: use clean packet headers
>>> ?? ath10k_sdio: high latency fixes for beacon buffer
>>> ?? ath10k_sdio: fix rssi indication
>>> ?? ath10k_sdio: common read write
>>> ?? ath10k_sdio: virtual scatter gather for receive
>>> ?? ath10k_sdio: enable firmware crash dump
>>> ?? ath10k_sdio: hif start once addition
>>>
>>> ? drivers/net/wireless/ath/ath10k/core.c??? |? 35 ++-
>>> ? drivers/net/wireless/ath/ath10k/debug.c?? |?? 3 +
>>> ? drivers/net/wireless/ath/ath10k/htc.c???? |?? 4 +-
>>> ? drivers/net/wireless/ath/ath10k/htc.h???? |?? 1 +
>>> ? drivers/net/wireless/ath/ath10k/htt_rx.c? |? 19 +-
>>> ? drivers/net/wireless/ath/ath10k/htt_tx.c? |? 24 +-
>>> ? drivers/net/wireless/ath/ath10k/hw.c????? |?? 2 +
>>> ? drivers/net/wireless/ath/ath10k/hw.h????? |?? 1 +
>>> ? drivers/net/wireless/ath/ath10k/mac.c???? |? 31 ++-
>>> ? drivers/net/wireless/ath/ath10k/sdio.c??? | 398 ++++++++++++++++++++++--------
>>> ? drivers/net/wireless/ath/ath10k/sdio.h??? |? 10 +-
>>> ? drivers/net/wireless/ath/ath10k/wmi-tlv.c |?? 2 +-
>>> ? 12 files changed, 403 insertions(+), 127 deletions(-)
>>>
>>
>

2017-12-22 16:08:45

[permalink] [raw]

Subject: Re: [PATCH 03/11] ath10k_sdio: DMA bounce buffers for read write

[email protected] writes:

> From: Alagu Sankar <[email protected]>
>
> Some SD host controllers still need bounce buffers for SDIO data
> transfers. While the transfers worked fine on x86 platforms,
> this is found to be required for i.MX6 based systems.
>
> Changes are similar to and derived from the ath6kl sdio driver.
>
> Signed-off-by: Alagu Sankar <[email protected]>

Why is the bounce buffer needed exactly, what are the symptoms etc? To
me this sounds like an ugly workaround for a SDIO controller driver bug.

--=20
Kalle Valo=

2017-12-18 16:20:06

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Hi Alagu,

On Fri, Oct 06, 2017 at 01:16:13PM +0200, Gary Bisson wrote:
> Hi Alagu,
>
> On Thu, Oct 05, 2017 at 10:54:26PM +0530, Alagu Sankar wrote:
> > Hi Gary,
> >
> > On 05-10-2017 20:42, Gary Bisson wrote:
> > > Hi Alagu,
> > >
> > > First of all, thank you for sharing your patches, this will be a very
> > > nice improvement to have SDIO QCA9377 working with ath10k.
> > >
> > > I've tried your series with Nitrogen7 [1] platform which is supported in
> > > mainline already. It uses BD-SDMAC [2] which uses the same module as the
> > > SX-SDMAC.
> > >
> > > Below are some questions/remarks I have after the testing.
> > >
> > > On Sat, Sep 30, 2017 at 11:07:37PM +0530, silexcommon at gmail.com wrote:
> > > > From: Alagu Sankar <alagusankar at silex-india.com>
> > > >
> > > > This patchset, generated against master-pending branch, enables a fully
> > > > functional SDIO interface driver for ath10k. Patches have been verified on
> > > > QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point
> > > > and P2P modes.
> > > >
> > > > The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
> > > Quick question on the firmware, is it the one from Kalle's repository?[3]
> > >
> > > If so, where does this firmware comes from? Is 00061 the firmware
> > > version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 output:
> > > Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1
> > Yes, it is from
> > https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested.
> > I have also used custom firmware from QCA/Silex as used in qcacld-2.0 driver
> > without any issue. You need to use the firmware merger tool from
> > https://github.com/erstrom/linux-ath/wiki/Firmware to combine the
> > qwlan30.bin and otp30.bin to generate the firmware-sdio.bin.
>
> Good to know, thanks. Maybe Kalle can tell us more about the firmware
> itself, what's the difference between the version 0.0.0.60 and 0.0.0.61?

Any update on this, is there a release notes for this 0.0.0.61 firmware?

> > > > with the board data from respective SDIO card vendors.
> > > About the board-sdio.bin, is it just a copy of your bdwlan30.bin?
> > Yes board-sdio.bin is a copy of bdwlan30.bin
>
> Thanks for confirming it.
>
> > > > Receive performance
> > > > matches the QCA reference driver when used with SDIO3.0 enabled platforms.
> > > > iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
> > > Nice performances. Unfortunately I don't get better than 30Mbits/s in TX
> > > and 50Mbits/s in RX:
> > > # iperf -c 192.168.1.1
> > > ------------------------------------------------------------
> > > Client connecting to 192.168.1.1, TCP port 5001
> > > TCP window size: 43.8 KByte (default)
> > > ------------------------------------------------------------
> > > [ 3] local 192.168.1.115 port 41354 connected with 192.168.1.1 port
> > > 5001
> > > [ ID] Interval Transfer Bandwidth
> > > [ 3] 0.0-10.0 sec 34.9 MBytes 29.2 Mbits/sec
> > > # iperf -s
> > > ------------------------------------------------------------
> > > Server listening on TCP port 5001
> > > TCP window size: 85.3 KByte (default)
> > > ------------------------------------------------------------
> > > [ 4] local 192.168.1.115 port 5001 connected with 192.168.1.1 port
> > > 50646
> > > [ ID] Interval Transfer Bandwidth
> > > [ 4] 0.0-10.0 sec 63.1 MBytes 52.7 Mbits/sec
> > >
> > > Do you have any idea why? Note that qcacld-2.0 driver on that same
> > > platform (same OS) gives the performances you advertize (150Mbits/s).
> > For some reason, if I use the imx_v6_v7_defconfig as is, performance is very
> > poor. In fact, the firmware download itself will take about 6 seconds. This
> > can also be seen when you up/down the wlan0 interface. For the i.MX6 SoloX
> > board, I used the configuration of 4.1.15 as provided by the BSP from
> > NXP/Freescale.
>
> Do you mean that you've used 4.1.15 kernel or just the 4.1.15
> configuration on latest 4.14?

As a FYI, I've tried your ath10k patches (along with a few backported
patches from Erik) on the NXP-fork of 4.9 kernel [1].

I confirm that it provides pretty decent performances:
- ath10k: 110Mbit/s
- qcacld-2.0: 125Mbit/s

Here are some details about the setup:
- TPLink AC router
- Nitrogen7 (i.MX7) with BD-SDMAC
- Kernel 4.9.68 (NXP fork [1])
- FW 0.0.0.61 from ath10k-firmware repo

It is definitely a nice alternative to qcacld driver. When do you plan
on sending a v2 out?

Regards,
Gary

[1] https://github.com/boundarydevices/linux-imx6/commits/test-ath10k

2017-12-27 19:26:16

by Adrian Chadd

[permalink] [raw]

Subject: Re: [PATCH 03/11] ath10k_sdio: DMA bounce buffers for read write

[top post for emphasis]

Arend is right. You won't be the only driver which has issues with a
controller that doesn't handle non-aligned data payloads. Please push
it into the stack or the controller side, but not in the driver side.
That'll be a forever game of whack-a-mole.

-adrian

(I'm living this dream right now and it's unfun)

On 27 December 2017 at 10:49, Arend van Spriel
<[email protected]> wrote:
> On 12/25/2017 1:26 PM, Alagu Sankar wrote:
>>
>> On 2017-12-22 21:38, Kalle Valo wrote:
>>>
>>> [email protected] writes:
>>>
>>>> From: Alagu Sankar <[email protected]>
>>>>
>>>> Some SD host controllers still need bounce buffers for SDIO data
>>>> transfers. While the transfers worked fine on x86 platforms,
>>>> this is found to be required for i.MX6 based systems.
>>>>
>>>> Changes are similar to and derived from the ath6kl sdio driver.
>>>>
>>>> Signed-off-by: Alagu Sankar <[email protected]>
>>>
>>>
>>> Why is the bounce buffer needed exactly, what are the symptoms etc? To
>>> me this sounds like an ugly workaround for a SDIO controller driver bug.
>>
>>
>> We faced problems with i.MX6. The authentication frame sent by the
>> driver never reached the air. The host driver accepted the buffer, but
>> did not send out the packet to the sdio module. No errors reported
>> anywhere, but the buffer is not accepted due to alignment. The same
>> driver however works fine without bounce buffer on x86 platform with
>> stdhci drivers. To make it compliant with all host controllers, we
>> introduced the bounce buffers, similar to what was done in ath6kl_sdio
>> drivers.
>
>
> As mentioned by Adrian the comment from Kalle is that you are solving an
> issue caused by the sdio host controller. Although strictly speaking it may
> not be a driver bug, but a requirement of the host controller hardware.
> Either way it seems the obvious place to solve this is in the sdio host
> controller driver to which the issue applies. Or make it a generic quirk
> which can be enabled for sdio host controller drivers that need it. However,
> there may reasons to do it in the networking driver. For instance, the
> buffer you want to transfer might be the data buffer of an sk_buff you got
> from the networking stack and you want to have a zero-copy solution towards
> the wireless device.
>
> Your solution checks for 4-byte alignment which is a requirement for ADMA as
> per SDIO spec. However, I have come across host controllers which have
> different alignment requirements. Also when CONFIG_ARCH_DMA_ADDR_T_64BIT is
> enabled the alignment changes from 4 to 8 bytes. So it seems you are solving
> a specific case you have come across, but you may want to design for more
> flexibility.
>
> Hope this helps.
>
> Regards,
> Arend

2017-12-27 18:49:12

by Arend van Spriel

[permalink] [raw]

Subject: Re: [PATCH 03/11] ath10k_sdio: DMA bounce buffers for read write

On 12/25/2017 1:26 PM, Alagu Sankar wrote:
> On 2017-12-22 21:38, Kalle Valo wrote:
>> [email protected] writes:
>>
>>> From: Alagu Sankar <[email protected]>
>>>
>>> Some SD host controllers still need bounce buffers for SDIO data
>>> transfers. While the transfers worked fine on x86 platforms,
>>> this is found to be required for i.MX6 based systems.
>>>
>>> Changes are similar to and derived from the ath6kl sdio driver.
>>>
>>> Signed-off-by: Alagu Sankar <[email protected]>
>>
>> Why is the bounce buffer needed exactly, what are the symptoms etc? To
>> me this sounds like an ugly workaround for a SDIO controller driver bug.
>
> We faced problems with i.MX6. The authentication frame sent by the
> driver never reached the air. The host driver accepted the buffer, but
> did not send out the packet to the sdio module. No errors reported
> anywhere, but the buffer is not accepted due to alignment. The same
> driver however works fine without bounce buffer on x86 platform with
> stdhci drivers. To make it compliant with all host controllers, we
> introduced the bounce buffers, similar to what was done in ath6kl_sdio
> drivers.

As mentioned by Adrian the comment from Kalle is that you are solving an
issue caused by the sdio host controller. Although strictly speaking it
may not be a driver bug, but a requirement of the host controller
hardware. Either way it seems the obvious place to solve this is in the
sdio host controller driver to which the issue applies. Or make it a
generic quirk which can be enabled for sdio host controller drivers that
need it. However, there may reasons to do it in the networking driver.
For instance, the buffer you want to transfer might be the data buffer
of an sk_buff you got from the networking stack and you want to have a
zero-copy solution towards the wireless device.

Your solution checks for 4-byte alignment which is a requirement for
ADMA as per SDIO spec. However, I have come across host controllers
which have different alignment requirements. Also when
CONFIG_ARCH_DMA_ADDR_T_64BIT is enabled the alignment changes from 4 to
8 bytes. So it seems you are solving a specific case you have come
across, but you may want to design for more flexibility.

Hope this helps.

Regards,
Arend

2017-12-25 16:11:12

by Adrian Chadd

[permalink] [raw]

Subject: Re: [PATCH 03/11] ath10k_sdio: DMA bounce buffers for read write

Hi,

I think Kalle is pointing out that maybe it's the SDHCI driver
responsibility to do the bounce buffering?

-adrian

2017-12-22 16:25:36

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

[email protected] writes:

> From: Alagu Sankar <[email protected]>
>
> This patchset, generated against master-pending branch, enables a fully
> functional SDIO interface driver for ath10k. Patches have been verified =
on
> QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access=
Point
> and P2P modes.
>
> The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1
> with the board data from respective SDIO card vendors. Receive performanc=
e
> matches the QCA reference driver when used with SDIO3.0 enabled platforms=
.
> iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s
>
> This patchset differs from the previous high latency patches, specific to=
SDIO.
> HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instruc=
ts the
> firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Witho=
ut
> this flag, the management frames are not sent out by the firmware. Possib=
ility
> of management frames being sent via WMI and data frames through the reduc=
ed Tx
> completion needs to be probed further.
>
> Further improvements can be done on the transmit path by implementing pac=
ket
> bundle. Scatter Gather is another area of improvement for both Transmit a=
nd
> Receive, but may not work on all platforms
>
> Known issues: Surprise removal of the card, when the device is in connect=
ed
> state, delays sdio function remove due to delayed WMI command failures.
> Existing ath10k framework can not differentiate between a kernel module
> removal and the surprise removal of teh card.
>
> Alagu Sankar (11):
> ath10k_sdio: sdio htt data transfer fixes
> ath10k_sdio: wb396 reference card fix
> ath10k_sdio: DMA bounce buffers for read write
> ath10k_sdio: reduce transmit msdu count
> ath10k_sdio: use clean packet headers
> ath10k_sdio: high latency fixes for beacon buffer
> ath10k_sdio: fix rssi indication
> ath10k_sdio: common read write
> ath10k_sdio: virtual scatter gather for receive
> ath10k_sdio: enable firmware crash dump
> ath10k_sdio: hif start once addition

Sorry, I run out of time to review this in detail. To make the review
easier I recommend to split this patchset into two sets, first set
containing only the bare essential to get basic functionality working
(for example ping working on x86) and the second set containing all the
optimisations (the bounce buffer stuff etc).

And try to make the first set as small as possible so that we can get it
faster applied.

--=20
Kalle Valo=

2017-12-25 13:21:47

[permalink] [raw]

Subject: Re: [PATCH 03/11] ath10k_sdio: DMA bounce buffers for read write

On 2017-12-22 21:38, Kalle Valo wrote:
> [email protected] writes:
>
>> From: Alagu Sankar <[email protected]>
>>
>> Some SD host controllers still need bounce buffers for SDIO data
>> transfers. While the transfers worked fine on x86 platforms,
>> this is found to be required for i.MX6 based systems.
>>
>> Changes are similar to and derived from the ath6kl sdio driver.
>>
>> Signed-off-by: Alagu Sankar <[email protected]>
>
> Why is the bounce buffer needed exactly, what are the symptoms etc? To
> me this sounds like an ugly workaround for a SDIO controller driver
> bug.

We faced problems with i.MX6. The authentication frame sent by the
driver never reached the air. The host driver accepted the buffer, but
did not send out the packet to the sdio module. No errors reported
anywhere, but the buffer is not accepted due to alignment. The same
driver however works fine without bounce buffer on x86 platform with
stdhci drivers. To make it compliant with all host controllers, we
introduced the bounce buffers, similar to what was done in ath6kl_sdio
drivers.

2017-12-08 14:42:17

[permalink] [raw]

Subject: Re: [08/11] ath10k_sdio: common read write

Hi Alagu,

On Thu, Oct 05, 2017 at 11:03:12PM +0530, Alagu Sankar wrote:
> Hi Gary,
>
>
> On 2017-10-05 15:39, Gary Bisson wrote:
> > Hi Alagu,
> >
> > On Sat, Sep 30, 2017 at 11:07:45PM +0530, [email protected] wrote:
> > > From: Alagu Sankar <[email protected]>
> > >
> > > convert different read write functions in sdio hif to bring it under a
> > > single read-write path. This helps in having a common dma bounce
> > > buffer
> > > implementation. Also helps in address modification that is required
> > > specific to change in certain mbox addresses of sdio_write.
> > >
> > > Signed-off-by: Alagu Sankar <[email protected]>
> > > ---
> > > drivers/net/wireless/ath/ath10k/sdio.c | 131
> > > ++++++++++++++++-----------------
> > > 1 file changed, 64 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/sdio.c
> > > b/drivers/net/wireless/ath/ath10k/sdio.c
> > > index 77d4fa4..bb6fa67 100644
> > > --- a/drivers/net/wireless/ath/ath10k/sdio.c
> > > +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> > > @@ -36,6 +36,11 @@
> > >
> > > #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
> > >
> > > +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
> > > + u32 len, bool incr);
> > > +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const
> > > void *buf,
> > > + u32 len, bool incr);
> > > +
> >
> > As mentioned by Kalle, u32 needs to be size_t.
> Yes, the compiler I used is probably a step older and did not catch this.
> >
> > > /* inlined helper functions */
> > >
> > > /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
> > > @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
> > > struct sdio_func *func = ar_sdio->func;
> > > unsigned char byte, asyncintdelay = 2;
> > > int ret;
> > > + u32 addr;
> > >
> > > ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");
> > >
> > > @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
> > > CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
> > > CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);
> > >
> > > - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
> > > - CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> > > - byte);
> > > + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> > > + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);
> >
> > Not sure this part is needed.
> This is to overcome checkpatch warning. Too big a names for the function and
> macro
> not helping in there. Will have to move it as a separate patch.
> >
> > > if (ret) {
> > > ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
> > > goto out;
> > > @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)
> > >
> > > static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
> > > {
> > > - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> > > - struct sdio_func *func = ar_sdio->func;
> > > + __le32 *buf;
> > > int ret;
> > >
> > > - sdio_claim_host(func);
> > > + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > >
> > > - sdio_writel(func, val, addr, &ret);
> > > + *buf = cpu_to_le32(val);
> > > +
> > > + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);
> >
> > Shouldn't we use buf instead of val? buf seems pretty useless otherwise.
> Yes, thanks for pointing this out. will be corrected in v2.

Do you have any timeframe on the v2?

Regards,
Gary

2017-12-22 16:21:37

[permalink] [raw]

Subject: Re: [PATCH 00/11] SDIO support for ath10k

Gary Bisson <[email protected]> writes:

> On Fri, Oct 06, 2017 at 01:16:13PM +0200, Gary Bisson wrote:
>> On Thu, Oct 05, 2017 at 10:54:26PM +0530, Alagu Sankar wrote:
>> > On 05-10-2017 20:42, Gary Bisson wrote:
>> > >=20
>> > > First of all, thank you for sharing your patches, this will be a ver=
y
>> > > nice improvement to have SDIO QCA9377 working with ath10k.
>> > >=20
>> > > I've tried your series with Nitrogen7 [1] platform which is supporte=
d in
>> > > mainline already. It uses BD-SDMAC [2] which uses the same module as=
the
>> > > SX-SDMAC.
>> > >=20
>> > > Below are some questions/remarks I have after the testing.
>> > >=20
>> > > Quick question on the firmware, is it the one from Kalle's repositor=
y?[3]
>> > >=20
>> > > If so, where does this firmware comes from? Is 00061 the firmware
>> > > version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 outpu=
t:
>> > > Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1
>> >
>> > Yes, it is from
>> > https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/unt=
ested.
>> > I have also used custom firmware from QCA/Silex as used in qcacld-2.0 =
driver
>> > without any issue. You need to use the firmware merger tool from
>> > https://github.com/erstrom/linux-ath/wiki/Firmware to combine the
>> > qwlan30.bin and otp30.bin to generate the firmware-sdio.bin.
>>=20
>> Good to know, thanks. Maybe Kalle can tell us more about the firmware
>> itself, what's the difference between the version 0.0.0.60 and 0.0.0.61?
>
> Any update on this, is there a release notes for this 0.0.0.61 firmware?

Unfortunately I don't have any changelogs for the firmware releases.

> As a FYI, I've tried your ath10k patches (along with a few backported
> patches from Erik) on the NXP-fork of 4.9 kernel [1].
>
> I confirm that it provides pretty decent performances:
> - ath10k: 110Mbit/s
> - qcacld-2.0: 125Mbit/s
>
> Here are some details about the setup:
> - TPLink AC router
> - Nitrogen7 (i.MX7) with BD-SDMAC
> - Kernel 4.9.68 (NXP fork [1])
> - FW 0.0.0.61 from ath10k-firmware repo

Nice, thanks for the report. This is always helpful.

--=20
Kalle Valo=

2018-01-08 12:58:57