[PATCH 0/3] ath10k: download firmware via diag ce for qca6174 and qca9377

[PATCH 0/3] ath10k: download firmware via diag ce for qca6174 and qca9377

[Carl Huang]

Downloading firmware via diag ce can greatly reduce the downloading
time to ~40ms from ~500ms for a 700k bytes firmware.

Carl Huang (3):
  ath10k: optimize pci diag mem read & write operations
  ath10k: support to access target space below 1M for qca6174 and
    qca9377
  ath10k: download firmware via diag Copy Engine for QCA6174 and
    QCA9377.

 drivers/net/wireless/ath/ath10k/bmi.c  |  23 ++++
 drivers/net/wireless/ath/ath10k/bmi.h  |  31 ++++++
 drivers/net/wireless/ath/ath10k/core.c |  36 +++++--
 drivers/net/wireless/ath/ath10k/hw.c   | 185 +++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/hw.h   |  19 ++++
 drivers/net/wireless/ath/ath10k/pci.c  |  59 +++++++----
 drivers/net/wireless/ath/ath10k/pci.h  |   3 +-
 7 files changed, 329 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH 1/3] ath10k: optimize pci diag mem read & write operations

[Carl Huang]

Delay 1ms is too long for both diag read and write operations.
This is observed when writing a big memory buffer to target or
reading a big memory buffer from target. Take writing/reading
512k bytes as example, the delay itself is 256ms as the maximum
length of every write/read is 2k size.

Reduce the delay to 50us for read and write operations.

Take the ath10k_pci_targ_cpu_to_ce_addr() out of loop and put it
in the beginning of the loop for ath10k_pci_diag_read_mem().

The ath10k_pci_targ_cpu_to_ce_addr() is to convert the address
from target cpu's perspective to CE's perspective, so it makes
no sense to convert a CE's perspective address again in the loop.
It's a wrong implementation but happens to work.

If the target address is below 1M space, then the convert in the loop
from the second time becomes wrong because the previously converted address
is larger than 1M. The counterpart ath10k_pci_diag_write_mem() has the
correct implementation.

With this change, ath10k_pci_diage_read_mem() works correctly no matter
the target address is below 1M or above 1M.

It's tested with QCA6174 hw3.2 and
firmware-6.bin_WLAN.RM.4.4.1-00111-QCARMSWP-1. QCA9377 is also affected.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/pci.c | 40 +++++++++++++++++++----------------
 drivers/net/wireless/ath/ath10k/pci.h |  3 ++-
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index af2cf55..7edbfe5 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -931,6 +931,15 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 		goto done;
 	}
 
+	/* The address supplied by the caller is in the
+	 * Target CPU virtual address space.
+	 *
+	 * In order to use this address with the diagnostic CE,
+	 * convert it from Target CPU virtual address space
+	 * to CE address space
+	 */
+	address = ath10k_pci_targ_cpu_to_ce_addr(ar, address);
+
 	remaining_bytes = nbytes;
 	ce_data = ce_data_base;
 	while (remaining_bytes) {
@@ -942,16 +951,6 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 			goto done;
 
 		/* Request CE to send from Target(!) address to Host buffer */
-		/*
-		 * The address supplied by the caller is in the
-		 * Target CPU virtual address space.
-		 *
-		 * In order to use this address with the diagnostic CE,
-		 * convert it from Target CPU virtual address space
-		 * to CE address space
-		 */
-		address = ath10k_pci_targ_cpu_to_ce_addr(ar, address);
-
 		ret = ath10k_ce_send_nolock(ce_diag, NULL, (u32)address, nbytes, 0,
 					    0);
 		if (ret)
@@ -960,8 +959,10 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 		i = 0;
 		while (ath10k_ce_completed_send_next_nolock(ce_diag,
 							    NULL) != 0) {
-			mdelay(1);
-			if (i++ > DIAG_ACCESS_CE_TIMEOUT_MS) {
+			udelay(DIAG_ACCESS_CE_WAIT_US);
+			i += DIAG_ACCESS_CE_WAIT_US;
+
+			if (i > DIAG_ACCESS_CE_TIMEOUT_US) {
 				ret = -EBUSY;
 				goto done;
 			}
@@ -972,9 +973,10 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 							    (void **)&buf,
 							    &completed_nbytes)
 								!= 0) {
-			mdelay(1);
+			udelay(DIAG_ACCESS_CE_WAIT_US);
+			i += DIAG_ACCESS_CE_WAIT_US;
 
-			if (i++ > DIAG_ACCESS_CE_TIMEOUT_MS) {
+			if (i > DIAG_ACCESS_CE_TIMEOUT_US) {
 				ret = -EBUSY;
 				goto done;
 			}
@@ -1119,9 +1121,10 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
 		i = 0;
 		while (ath10k_ce_completed_send_next_nolock(ce_diag,
 							    NULL) != 0) {
-			mdelay(1);
+			udelay(DIAG_ACCESS_CE_WAIT_US);
+			i += DIAG_ACCESS_CE_WAIT_US;
 
-			if (i++ > DIAG_ACCESS_CE_TIMEOUT_MS) {
+			if (i > DIAG_ACCESS_CE_TIMEOUT_US) {
 				ret = -EBUSY;
 				goto done;
 			}
@@ -1132,9 +1135,10 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
 							    (void **)&buf,
 							    &completed_nbytes)
 								!= 0) {
-			mdelay(1);
+			udelay(DIAG_ACCESS_CE_WAIT_US);
+			i += DIAG_ACCESS_CE_WAIT_US;
 
-			if (i++ > DIAG_ACCESS_CE_TIMEOUT_MS) {
+			if (i > DIAG_ACCESS_CE_TIMEOUT_US) {
 				ret = -EBUSY;
 				goto done;
 			}
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 0ed4366..e8d8633 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -207,7 +207,8 @@ static inline struct ath10k_pci *ath10k_pci_priv(struct ath10k *ar)
 #define CDC_WAR_DATA_CE     4
 
 /* Wait up to this many Ms for a Diagnostic Access CE operation to complete */
-#define DIAG_ACCESS_CE_TIMEOUT_MS 10
+#define DIAG_ACCESS_CE_TIMEOUT_US 10000 /* 10 ms */
+#define DIAG_ACCESS_CE_WAIT_US	50
 
 void ath10k_pci_write32(struct ath10k *ar, u32 offset, u32 value);
 void ath10k_pci_soc_write32(struct ath10k *ar, u32 addr, u32 val);
-- 
2.7.4

Re: [PATCH 1/3] ath10k: optimize pci diag mem read & write operations

[Kalle Valo]

Carl Huang <cjhuang@codeaurora.org> wrote:

> Delay 1ms is too long for both diag read and write operations.
> This is observed when writing a big memory buffer to target or
> reading a big memory buffer from target. Take writing/reading
> 512k bytes as example, the delay itself is 256ms as the maximum
> length of every write/read is 2k size.
> 
> Reduce the delay to 50us for read and write operations.
> 
> Take the ath10k_pci_targ_cpu_to_ce_addr() out of loop and put it
> in the beginning of the loop for ath10k_pci_diag_read_mem().
> 
> The ath10k_pci_targ_cpu_to_ce_addr() is to convert the address
> from target cpu's perspective to CE's perspective, so it makes
> no sense to convert a CE's perspective address again in the loop.
> It's a wrong implementation but happens to work.
> 
> If the target address is below 1M space, then the convert in the loop
> from the second time becomes wrong because the previously converted address
> is larger than 1M. The counterpart ath10k_pci_diag_write_mem() has the
> correct implementation.
> 
> With this change, ath10k_pci_diage_read_mem() works correctly no matter
> the target address is below 1M or above 1M.
> 
> It's tested with QCA6174 hw3.2 and
> firmware-6.bin_WLAN.RM.4.4.1-00111-QCARMSWP-1. QCA9377 is also affected.
> 
> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

3 patches applied to ath-next branch of ath.git, thanks.

d56bbeea25d1 ath10k: optimize pci diag mem read & write operations
bc346c9a24a4 ath10k: support to access target space below 1M for qca6174 and qca9377
39501ea64116 ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

-- 
https://patchwork.kernel.org/patch/10581149/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/3] ath10k: support to access target space below 1M for qca6174 and qca9377

[Carl Huang]

To download firmware via diag interface, driver needs to write the target
memory space below 1M. It means the bit20 should be zero for the converted
address if the target memory space is below 1M. Otherwise, bit20 is one if
the target address is larger or equal to 1M space.

As downloading firmware via diag interface is only required for qca6174
and qca9377, a new specific function is introduced to convert the target
address to ce address: ath10k_pci_qca6174_targ_cpu_to_ce_addri().
This function supports to convert any target address to ce address.

It's tested with QCA6174 hw3.2 and
firmware-6.bin_WLAN.RM.4.4.1-00111-QCARMSWP-1. QCA9377 is also affected.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/pci.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7edbfe5..c82f17c 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -870,6 +870,21 @@ static u32 ath10k_pci_qca988x_targ_cpu_to_ce_addr(struct ath10k *ar, u32 addr)
 	return val;
 }
 
+/* Refactor from ath10k_pci_qca988x_targ_cpu_to_ce_addr.
+ * Support to access target space below 1M for qca6174 and qca9377.
+ * If target space is below 1M, the bit[20] of converted CE addr is 0.
+ * Otherwise bit[20] of converted CE addr is 1.
+ */
+static u32 ath10k_pci_qca6174_targ_cpu_to_ce_addr(struct ath10k *ar, u32 addr)
+{
+	u32 val = 0, region = addr & 0xfffff;
+
+	val = (ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS + CORE_CTRL_ADDRESS)
+				 & 0x7ff) << 21;
+	val |= ((addr >= 0x100000) ? 0x100000 : 0) | region;
+	return val;
+}
+
 static u32 ath10k_pci_qca99x0_targ_cpu_to_ce_addr(struct ath10k *ar, u32 addr)
 {
 	u32 val = 0, region = addr & 0xfffff;
@@ -3514,7 +3529,7 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 		pci_ps = true;
 		pci_soft_reset = ath10k_pci_warm_reset;
 		pci_hard_reset = ath10k_pci_qca6174_chip_reset;
-		targ_cpu_to_ce_addr = ath10k_pci_qca988x_targ_cpu_to_ce_addr;
+		targ_cpu_to_ce_addr = ath10k_pci_qca6174_targ_cpu_to_ce_addr;
 		break;
 	case QCA99X0_2_0_DEVICE_ID:
 		hw_rev = ATH10K_HW_QCA99X0;
@@ -3542,7 +3557,7 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 		pci_ps = true;
 		pci_soft_reset = NULL;
 		pci_hard_reset = ath10k_pci_qca6174_chip_reset;
-		targ_cpu_to_ce_addr = ath10k_pci_qca988x_targ_cpu_to_ce_addr;
+		targ_cpu_to_ce_addr = ath10k_pci_qca6174_targ_cpu_to_ce_addr;
 		break;
 	default:
 		WARN_ON(1);
-- 
2.7.4

[PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Carl Huang]

Downloading firmware via BMI protocol takes too long time. For example,
a ~700K bytes firmware takes about 500ms to download via BMI protocol.
This is too long especially in suspend and resume scenario where firmware
is re-downloaded unless WoWLAN is enabled. Downloading firmware via diag CE
can reduce the time to ~40ms for a ~700K bytes firmware binary.

Ath10k driver parses the firmware to segments and downloads the segments
to the specified address directly. If the firmware is compressed or has
unsupported segments, ath10k driver will try BMI download again.

It's tested with QCA6174 hw3.2 and
firmware-6.bin_WLAN.RM.4.4.1-00111-QCARMSWP-1. QCA9377 is also affected.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/bmi.c  |  23 ++++
 drivers/net/wireless/ath/ath10k/bmi.h  |  31 ++++++
 drivers/net/wireless/ath/ath10k/core.c |  36 +++++--
 drivers/net/wireless/ath/ath10k/hw.c   | 185 +++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/hw.h   |  19 ++++
 5 files changed, 288 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/bmi.c b/drivers/net/wireless/ath/ath10k/bmi.c
index af4978d..1750b18 100644
--- a/drivers/net/wireless/ath/ath10k/bmi.c
+++ b/drivers/net/wireless/ath/ath10k/bmi.c
@@ -459,3 +459,26 @@ int ath10k_bmi_fast_download(struct ath10k *ar,
 
 	return ret;
 }
+
+int ath10k_bmi_set_start(struct ath10k *ar, u32 address)
+{
+	struct bmi_cmd cmd;
+	u32 cmdlen = sizeof(cmd.id) + sizeof(cmd.set_app_start);
+	int ret;
+
+	if (ar->bmi.done_sent) {
+		ath10k_warn(ar, "bmi set start command disallowed\n");
+		return -EBUSY;
+	}
+
+	cmd.id = __cpu_to_le32(BMI_SET_APP_START);
+	cmd.set_app_start.addr = __cpu_to_le32(address);
+
+	ret = ath10k_hif_exchange_bmi_msg(ar, &cmd, cmdlen, NULL, NULL);
+	if (ret) {
+		ath10k_warn(ar, "unable to set start to the device:%d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/net/wireless/ath/ath10k/bmi.h b/drivers/net/wireless/ath/ath10k/bmi.h
index 9a39681..770a19d 100644
--- a/drivers/net/wireless/ath/ath10k/bmi.h
+++ b/drivers/net/wireless/ath/ath10k/bmi.h
@@ -190,6 +190,35 @@ struct bmi_target_info {
 	u32 type;
 };
 
+struct bmi_segmented_file_header {
+	__le32 magic_num;
+	__le32 file_flags;
+	u8 data[];
+};
+
+struct bmi_segmented_metadata {
+	__le32 addr;
+	__le32 length;
+	u8 data[];
+};
+
+#define BMI_SGMTFILE_MAGIC_NUM          0x544d4753 /* "SGMT" */
+#define BMI_SGMTFILE_FLAG_COMPRESS      1
+
+/* Special values for bmi_segmented_metadata.length (all have high bit set) */
+
+/* end of segmented data */
+#define BMI_SGMTFILE_DONE               0xffffffff
+
+/* Board Data segment */
+#define BMI_SGMTFILE_BDDATA             0xfffffffe
+
+/* set beginning address */
+#define BMI_SGMTFILE_BEGINADDR          0xfffffffd
+
+/* immediate function execution */
+#define BMI_SGMTFILE_EXEC               0xfffffffc
+
 /* in jiffies */
 #define BMI_COMMUNICATION_TIMEOUT_HZ (3 * HZ)
 
@@ -239,4 +268,6 @@ int ath10k_bmi_fast_download(struct ath10k *ar, u32 address,
 			     const void *buffer, u32 length);
 int ath10k_bmi_read_soc_reg(struct ath10k *ar, u32 address, u32 *reg_val);
 int ath10k_bmi_write_soc_reg(struct ath10k *ar, u32 address, u32 reg_val);
+int ath10k_bmi_set_start(struct ath10k *ar, u32 address);
+
 #endif /* _BMI_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index c40cd12..78a0515 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -91,6 +91,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.rx_ring_fill_level = HTT_RX_RING_FILL_LEVEL,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA988X_HW_2_0_VERSION,
@@ -124,6 +125,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA9887_HW_1_0_VERSION,
@@ -157,6 +159,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -189,6 +192,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -221,6 +225,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA6174_HW_3_0_VERSION,
@@ -253,6 +258,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -288,6 +294,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = true,
 	},
 	{
 		.id = QCA99X0_HW_2_0_DEV_VERSION,
@@ -326,6 +333,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA9984_HW_1_0_DEV_VERSION,
@@ -369,6 +377,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA9888_HW_2_0_DEV_VERSION,
@@ -411,6 +420,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA9377_HW_1_0_DEV_VERSION,
@@ -443,6 +453,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -477,6 +488,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = true,
 	},
 	{
 		.id = QCA4019_HW_1_0_DEV_VERSION,
@@ -516,6 +528,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = false,
 		.shadow_reg_support = false,
 		.rri_on_ddr = false,
+		.fw_diag_ce_download = false,
 	},
 	{
 		.id = WCN3990_HW_1_0_DEV_VERSION,
@@ -539,6 +552,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.per_ce_irq = true,
 		.shadow_reg_support = true,
 		.rri_on_ddr = true,
+		.fw_diag_ce_download = false,
 	},
 };
 
@@ -1135,14 +1149,24 @@ static int ath10k_download_fw(struct ath10k *ar)
 		   "boot uploading firmware image %pK len %d\n",
 		   data, data_len);
 
-	ret = ath10k_bmi_fast_download(ar, address, data, data_len);
-	if (ret) {
-		ath10k_err(ar, "failed to download firmware: %d\n",
-			   ret);
-		return ret;
+	/* Check if device supports to download firmware via
+	 * diag copy engine. Downloading firmware via diag CE
+	 * greatly reduces the time to download firmware.
+	 */
+	if (ar->hw_params.fw_diag_ce_download) {
+		ret = ath10k_hw_diag_fast_download(ar, address,
+						   data, data_len);
+		if (ret == 0)
+			/* firmware upload via diag ce was successful */
+			return 0;
+
+		ath10k_warn(ar,
+			    "failed to upload firmware via diag ce, trying BMI: %d",
+			    ret);
 	}
 
-	return ret;
+	return ath10k_bmi_fast_download(ar, address,
+					data, data_len);
 }
 
 static void ath10k_core_free_board_files(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index 677535b..25ee1c6 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -16,6 +16,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include "core.h"
 #include "hw.h"
 #include "hif.h"
@@ -918,6 +919,190 @@ static int ath10k_hw_qca6174_enable_pll_clock(struct ath10k *ar)
 	return 0;
 }
 
+/* Program CPU_ADDR_MSB to allow different memory
+ * region access.
+ */
+static void ath10k_hw_map_target_mem(struct ath10k *ar, u32 msb)
+{
+	u32 address = SOC_CORE_BASE_ADDRESS + FW_RAM_CONFIG_ADDRESS;
+
+	ath10k_hif_write32(ar, address, msb);
+}
+
+/* 1. Write to memory region of target, such as IRAM adn DRAM.
+ * 2. Target address( 0 ~ 00100000 & 0x00400000~0x00500000)
+ *    can be written directly. See ath10k_pci_targ_cpu_to_ce_addr() too.
+ * 3. In order to access the region other than the above,
+ *    we need to set the value of register CPU_ADDR_MSB.
+ * 4. Target memory access space is limited to 1M size. If the size is larger
+ *    than 1M, need to split it and program CPU_ADDR_MSB accordingly.
+ */
+static int ath10k_hw_diag_segment_msb_download(struct ath10k *ar,
+					       const void *buffer,
+					       u32 address,
+					       u32 length)
+{
+	u32 addr = address & REGION_ACCESS_SIZE_MASK;
+	int ret, remain_size, size;
+	const u8 *buf;
+
+	ath10k_hw_map_target_mem(ar, CPU_ADDR_MSB_REGION_VAL(address));
+
+	if (addr + length > REGION_ACCESS_SIZE_LIMIT) {
+		size = REGION_ACCESS_SIZE_LIMIT - addr;
+		remain_size = length - size;
+
+		ret = ath10k_hif_diag_write(ar, address, buffer, size);
+		if (ret) {
+			ath10k_warn(ar,
+				    "failed to download the first %d bytes segment to address:0x%x: %d\n",
+				    size, address, ret);
+			goto done;
+		}
+
+		/* Change msb to the next memory region*/
+		ath10k_hw_map_target_mem(ar,
+					 CPU_ADDR_MSB_REGION_VAL(address) + 1);
+		buf = buffer +  size;
+		ret = ath10k_hif_diag_write(ar,
+					    address & ~REGION_ACCESS_SIZE_MASK,
+					    buf, remain_size);
+		if (ret) {
+			ath10k_warn(ar,
+				    "failed to download the second %d bytes segment to address:0x%x: %d\n",
+				    remain_size,
+				    address & ~REGION_ACCESS_SIZE_MASK,
+				    ret);
+			goto done;
+		}
+	} else {
+		ret = ath10k_hif_diag_write(ar, address, buffer, length);
+		if (ret) {
+			ath10k_warn(ar,
+				    "failed to download the only %d bytes segment to address:0x%x: %d\n",
+				    length, address, ret);
+			goto done;
+		}
+	}
+
+done:
+	/* Change msb to DRAM */
+	ath10k_hw_map_target_mem(ar,
+				 CPU_ADDR_MSB_REGION_VAL(DRAM_BASE_ADDRESS));
+	return ret;
+}
+
+static int ath10k_hw_diag_segment_download(struct ath10k *ar,
+					   const void *buffer,
+					   u32 address,
+					   u32 length)
+{
+	if (address >= DRAM_BASE_ADDRESS + REGION_ACCESS_SIZE_LIMIT)
+		/* Needs to change MSB for memory write */
+		return ath10k_hw_diag_segment_msb_download(ar, buffer,
+							   address, length);
+	else
+		return ath10k_hif_diag_write(ar, address, buffer, length);
+}
+
+int ath10k_hw_diag_fast_download(struct ath10k *ar,
+				 u32 address,
+				 const void *buffer,
+				 u32 length)
+{
+	const u8 *buf = buffer;
+	bool sgmt_end = false;
+	u32 base_addr = 0;
+	u32 base_len = 0;
+	u32 left = 0;
+	struct bmi_segmented_file_header *hdr;
+	struct bmi_segmented_metadata *metadata;
+	int ret = 0;
+
+	if (length < sizeof(*hdr))
+		return -EINVAL;
+
+	/* check firmware header. If it has no correct magic number
+	 * or it's compressed, returns error.
+	 */
+	hdr = (struct bmi_segmented_file_header *)buf;
+	if (hdr->magic_num != BMI_SGMTFILE_MAGIC_NUM) {
+		ath10k_dbg(ar, ATH10K_DBG_BOOT,
+			   "Not a supported firmware, magic_num:0x%x\n",
+			   hdr->magic_num);
+		return -EINVAL;
+	}
+
+	if (hdr->file_flags != 0) {
+		ath10k_dbg(ar, ATH10K_DBG_BOOT,
+			   "Not a supported firmware, file_flags:0x%x\n",
+			   hdr->file_flags);
+		return -EINVAL;
+	}
+
+	metadata = (struct bmi_segmented_metadata *)hdr->data;
+	left = length - sizeof(*hdr);
+
+	while (left > 0) {
+		base_addr = metadata->addr;
+		base_len = metadata->length;
+		buf = metadata->data;
+		left -= sizeof(*metadata);
+
+		switch (base_len) {
+		case BMI_SGMTFILE_BEGINADDR:
+			/* base_addr is the start address to run */
+			ret = ath10k_bmi_set_start(ar, base_addr);
+			base_len = 0;
+			break;
+		case BMI_SGMTFILE_DONE:
+			/* no more segment */
+			base_len = 0;
+			sgmt_end = true;
+			ret = 0;
+			break;
+		case BMI_SGMTFILE_BDDATA:
+		case BMI_SGMTFILE_EXEC:
+			ath10k_warn(ar,
+				    "firmware has unsupported segment:%d\n",
+				    base_len);
+			ret = -EINVAL;
+			break;
+		default:
+			if (base_len > left) {
+				/* sanity check */
+				ath10k_warn(ar,
+					    "firmware has invalid segment length, %d > %d\n",
+					    base_len, left);
+				ret = -EINVAL;
+				break;
+			}
+
+			ret = ath10k_hw_diag_segment_download(ar,
+							      buf,
+							      base_addr,
+							      base_len);
+
+			if (ret)
+				ath10k_warn(ar,
+					    "failed to download firmware via diag interface:%d\n",
+					    ret);
+			break;
+		}
+
+		if (ret || sgmt_end)
+			break;
+
+		metadata = (struct bmi_segmented_metadata *)(buf + base_len);
+		left -= base_len;
+	}
+
+	if (ret == 0)
+		ath10k_dbg(ar, ATH10K_DBG_BOOT,
+			   "boot firmware fast diag download successfully.\n");
+	return ret;
+}
+
 const struct ath10k_hw_ops qca988x_ops = {
 	.set_coverage_class = ath10k_hw_qca988x_set_coverage_class,
 };
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 977f79e..1598315 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -389,6 +389,11 @@ extern const struct ath10k_hw_ce_regs qcax_ce_regs;
 void ath10k_hw_fill_survey_time(struct ath10k *ar, struct survey_info *survey,
 				u32 cc, u32 rcc, u32 cc_prev, u32 rcc_prev);
 
+int ath10k_hw_diag_fast_download(struct ath10k *ar,
+				 u32 address,
+				 const void *buffer,
+				 u32 length);
+
 #define QCA_REV_988X(ar) ((ar)->hw_rev == ATH10K_HW_QCA988X)
 #define QCA_REV_9887(ar) ((ar)->hw_rev == ATH10K_HW_QCA9887)
 #define QCA_REV_6174(ar) ((ar)->hw_rev == ATH10K_HW_QCA6174)
@@ -589,6 +594,9 @@ struct ath10k_hw_params {
 
 	/* Number of bytes to be the offset for each FFT sample */
 	int spectral_bin_offset;
+
+	/* target supporting fw download via diag ce */
+	bool fw_diag_ce_download;
 };
 
 struct htt_rx_desc;
@@ -1124,4 +1132,15 @@ ath10k_rx_desc_get_l3_pad_bytes(struct ath10k_hw_params *hw,
 #define RTC_SYNC_STATUS_PLL_CHANGING_MASK	0x00000020
 /* qca6174 PLL offset/mask end */
 
+/* CPU_ADDR_MSB is a register, bit[3:0] is to specify which memory
+ * region is accessed. The memory region size is 1M.
+ * If host wants to access 0xX12345 at target, then CPU_ADDR_MSB[3:0]
+ * is 0xX.
+ * The following MACROs are defined to get the 0xX and the size limit.
+ */
+#define CPU_ADDR_MSB_REGION_MASK	GENMASK(23, 20)
+#define CPU_ADDR_MSB_REGION_VAL(X)	FIELD_GET(CPU_ADDR_MSB_REGION_MASK, X)
+#define REGION_ACCESS_SIZE_LIMIT	0x100000
+#define REGION_ACCESS_SIZE_MASK		(REGION_ACCESS_SIZE_LIMIT - 1)
+
 #endif /* _HW_H_ */
-- 
2.7.4

Re: [PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Kalle Valo]

Carl Huang <cjhuang@codeaurora.org> writes:

> Downloading firmware via BMI protocol takes too long time. For example,
> a ~700K bytes firmware takes about 500ms to download via BMI protocol.
> This is too long especially in suspend and resume scenario where firmware
> is re-downloaded unless WoWLAN is enabled. Downloading firmware via diag CE
> can reduce the time to ~40ms for a ~700K bytes firmware binary.
>
> Ath10k driver parses the firmware to segments and downloads the segments
> to the specified address directly. If the firmware is compressed or has
> unsupported segments, ath10k driver will try BMI download again.
>
> It's tested with QCA6174 hw3.2 and
> firmware-6.bin_WLAN.RM.4.4.1-00111-QCARMSWP-1. QCA9377 is also affected.
>
> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>

There were new warnings:

drivers/net/wireless/ath/ath10k/hw.c:1029:16: warning: restricted __le32 degrades to integer
drivers/net/wireless/ath/ath10k/hw.c:1047:27: warning: incorrect type in assignment (different base types)
drivers/net/wireless/ath/ath10k/hw.c:1047:27:    expected unsigned int [unsigned] [usertype] base_addr
drivers/net/wireless/ath/ath10k/hw.c:1047:27:    got restricted __le32 [usertype] addr
drivers/net/wireless/ath/ath10k/hw.c:1048:26: warning: incorrect type in assignment (different base types)
drivers/net/wireless/ath/ath10k/hw.c:1048:26:    expected unsigned int [unsigned] [usertype] base_len
drivers/net/wireless/ath/ath10k/hw.c:1048:26:    got restricted __le32 [usertype] length

I fixed those in the pending branch, please review:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=f6f6f597654693ab122ecd950b56343489b91e59

-- 
Kalle Valo

Re: [PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Carl Huang]

On 2018-09-05 12:52, Kalle Valo wrote:
> Carl Huang <cjhuang@codeaurora.org> writes:
> 
>> Downloading firmware via BMI protocol takes too long time. For 
>> example,
>> a ~700K bytes firmware takes about 500ms to download via BMI protocol.
>> This is too long especially in suspend and resume scenario where 
>> firmware
>> is re-downloaded unless WoWLAN is enabled. Downloading firmware via 
>> diag CE
>> can reduce the time to ~40ms for a ~700K bytes firmware binary.
>> 
>> Ath10k driver parses the firmware to segments and downloads the 
>> segments
>> to the specified address directly. If the firmware is compressed or 
>> has
>> unsupported segments, ath10k driver will try BMI download again.
>> 
>> It's tested with QCA6174 hw3.2 and
>> firmware-6.bin_WLAN.RM.4.4.1-00111-QCARMSWP-1. QCA9377 is also 
>> affected.
>> 
>> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
> 
> There were new warnings:
> 
> drivers/net/wireless/ath/ath10k/hw.c:1029:16: warning: restricted
> __le32 degrades to integer
> drivers/net/wireless/ath/ath10k/hw.c:1047:27: warning: incorrect type
> in assignment (different base types)
> drivers/net/wireless/ath/ath10k/hw.c:1047:27:    expected unsigned int
> [unsigned] [usertype] base_addr
> drivers/net/wireless/ath/ath10k/hw.c:1047:27:    got restricted __le32
> [usertype] addr
> drivers/net/wireless/ath/ath10k/hw.c:1048:26: warning: incorrect type
> in assignment (different base types)
> drivers/net/wireless/ath/ath10k/hw.c:1048:26:    expected unsigned int
> [unsigned] [usertype] base_len
> drivers/net/wireless/ath/ath10k/hw.c:1048:26:    got restricted __le32
> [usertype] length
> 
> I fixed those in the pending branch, please review:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=f6f6f597654693ab122ecd950b56343489b91e59

Kalle,

Thank you for fixing the warnings. I don't see any issues with this 
warning fix.


Thanks,
Carl

Re: [PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Brian Norris]

Hi,

I see this patch is already merged, but some small comments. Might be
worth a follow-up patch eventually.

On Thu, Aug 30, 2018 at 10:29:42AM +0800, Carl Huang wrote:
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index c40cd12..78a0515 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c

...

> @@ -1135,14 +1149,24 @@ static int ath10k_download_fw(struct ath10k *ar)
>  		   "boot uploading firmware image %pK len %d\n",
>  		   data, data_len);
>  
> -	ret = ath10k_bmi_fast_download(ar, address, data, data_len);
> -	if (ret) {
> -		ath10k_err(ar, "failed to download firmware: %d\n",
> -			   ret);
> -		return ret;
> +	/* Check if device supports to download firmware via
> +	 * diag copy engine. Downloading firmware via diag CE
> +	 * greatly reduces the time to download firmware.
> +	 */
> +	if (ar->hw_params.fw_diag_ce_download) {
> +		ret = ath10k_hw_diag_fast_download(ar, address,
> +						   data, data_len);
> +		if (ret == 0)
> +			/* firmware upload via diag ce was successful */
> +			return 0;
> +
> +		ath10k_warn(ar,
> +			    "failed to upload firmware via diag ce, trying BMI: %d",

I believe ath10k_*() logging is still supposed to have '\n' newlines at
the end. But then, the omission occurs all over the place, and it seems
printk() still tends to give you newlines anyway. Seems like we should
still be consistent though.

> +			    ret);
>  	}
>  
> -	return ret;
> +	return ath10k_bmi_fast_download(ar, address,
> +					data, data_len);

You no longer have an error message for BMI failures here (you used to).

>  }
>  
>  static void ath10k_core_free_board_files(struct ath10k *ar)

Brian

Re: [PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Brian Norris]

Hi,

On Thu, Aug 30, 2018 at 10:29:42AM +0800, Carl Huang wrote:
> diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
> index 677535b..25ee1c6 100644
> --- a/drivers/net/wireless/ath/ath10k/hw.c
> +++ b/drivers/net/wireless/ath/ath10k/hw.c
...
> +int ath10k_hw_diag_fast_download(struct ath10k *ar,
> +				 u32 address,
> +				 const void *buffer,
> +				 u32 length)
> +{
> +	const u8 *buf = buffer;
> +	bool sgmt_end = false;
> +	u32 base_addr = 0;
> +	u32 base_len = 0;
> +	u32 left = 0;
> +	struct bmi_segmented_file_header *hdr;
> +	struct bmi_segmented_metadata *metadata;
> +	int ret = 0;
> +
> +	if (length < sizeof(*hdr))
> +		return -EINVAL;
> +
> +	/* check firmware header. If it has no correct magic number
> +	 * or it's compressed, returns error.
> +	 */
> +	hdr = (struct bmi_segmented_file_header *)buf;
> +	if (hdr->magic_num != BMI_SGMTFILE_MAGIC_NUM) {
> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
> +			   "Not a supported firmware, magic_num:0x%x\n",
> +			   hdr->magic_num);
> +		return -EINVAL;
> +	}
> +
> +	if (hdr->file_flags != 0) {
> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
> +			   "Not a supported firmware, file_flags:0x%x\n",
> +			   hdr->file_flags);
> +		return -EINVAL;
> +	}
> +
> +	metadata = (struct bmi_segmented_metadata *)hdr->data;
> +	left = length - sizeof(*hdr);
> +
> +	while (left > 0) {
> +		base_addr = metadata->addr;
> +		base_len = metadata->length;
> +		buf = metadata->data;
> +		left -= sizeof(*metadata);

You need to ensure 'left >= sizeof(*metadata)' before this block. I can
send a patch.

Brian

> +
> +		switch (base_len) {
> +		case BMI_SGMTFILE_BEGINADDR:
> +			/* base_addr is the start address to run */
> +			ret = ath10k_bmi_set_start(ar, base_addr);
> +			base_len = 0;
> +			break;
> +		case BMI_SGMTFILE_DONE:
> +			/* no more segment */
> +			base_len = 0;
> +			sgmt_end = true;
> +			ret = 0;
> +			break;
> +		case BMI_SGMTFILE_BDDATA:
> +		case BMI_SGMTFILE_EXEC:
> +			ath10k_warn(ar,
> +				    "firmware has unsupported segment:%d\n",
> +				    base_len);
> +			ret = -EINVAL;
> +			break;
> +		default:
> +			if (base_len > left) {
> +				/* sanity check */
> +				ath10k_warn(ar,
> +					    "firmware has invalid segment length, %d > %d\n",
> +					    base_len, left);
> +				ret = -EINVAL;
> +				break;
> +			}
> +
> +			ret = ath10k_hw_diag_segment_download(ar,
> +							      buf,
> +							      base_addr,
> +							      base_len);
> +
> +			if (ret)
> +				ath10k_warn(ar,
> +					    "failed to download firmware via diag interface:%d\n",
> +					    ret);
> +			break;
> +		}
> +
> +		if (ret || sgmt_end)
> +			break;
> +
> +		metadata = (struct bmi_segmented_metadata *)(buf + base_len);
> +		left -= base_len;
> +	}
> +
> +	if (ret == 0)
> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
> +			   "boot firmware fast diag download successfully.\n");
> +	return ret;
> +}
> +
>  const struct ath10k_hw_ops qca988x_ops = {
>  	.set_coverage_class = ath10k_hw_qca988x_set_coverage_class,
>  };

Re: [PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Brian Norris]

Hi again!

One last (?) comment:

On Thu, Aug 30, 2018 at 10:29:42AM +0800, Carl Huang wrote:

> diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
> index 677535b..25ee1c6 100644
> --- a/drivers/net/wireless/ath/ath10k/hw.c
> +++ b/drivers/net/wireless/ath/ath10k/hw.c

> @@ -918,6 +919,190 @@ static int ath10k_hw_qca6174_enable_pll_clock(struct ath10k *ar)

> +static int ath10k_hw_diag_segment_download(struct ath10k *ar,
> +					   const void *buffer,
> +					   u32 address,
> +					   u32 length)
> +{
> +	if (address >= DRAM_BASE_ADDRESS + REGION_ACCESS_SIZE_LIMIT)
> +		/* Needs to change MSB for memory write */
> +		return ath10k_hw_diag_segment_msb_download(ar, buffer,
> +							   address, length);
> +	else
> +		return ath10k_hif_diag_write(ar, address, buffer, length);
> +}
> +
> +int ath10k_hw_diag_fast_download(struct ath10k *ar,
> +				 u32 address,
> +				 const void *buffer,
> +				 u32 length)
> +{
> +	const u8 *buf = buffer;
> +	bool sgmt_end = false;
> +	u32 base_addr = 0;
> +	u32 base_len = 0;
> +	u32 left = 0;
> +	struct bmi_segmented_file_header *hdr;
> +	struct bmi_segmented_metadata *metadata;
> +	int ret = 0;
> +
> +	if (length < sizeof(*hdr))
> +		return -EINVAL;
> +
> +	/* check firmware header. If it has no correct magic number
> +	 * or it's compressed, returns error.
> +	 */
> +	hdr = (struct bmi_segmented_file_header *)buf;
> +	if (hdr->magic_num != BMI_SGMTFILE_MAGIC_NUM) {
> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
> +			   "Not a supported firmware, magic_num:0x%x\n",
> +			   hdr->magic_num);
> +		return -EINVAL;
> +	}
> +
> +	if (hdr->file_flags != 0) {
> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
> +			   "Not a supported firmware, file_flags:0x%x\n",
> +			   hdr->file_flags);
> +		return -EINVAL;
> +	}
> +
> +	metadata = (struct bmi_segmented_metadata *)hdr->data;
> +	left = length - sizeof(*hdr);
> +
> +	while (left > 0) {
> +		base_addr = metadata->addr;
> +		base_len = metadata->length;
> +		buf = metadata->data;
> +		left -= sizeof(*metadata);
> +
> +		switch (base_len) {
...
> +		default:
> +			if (base_len > left) {
> +				/* sanity check */
> +				ath10k_warn(ar,
> +					    "firmware has invalid segment length, %d > %d\n",
> +					    base_len, left);
> +				ret = -EINVAL;
> +				break;
> +			}
> +
> +			ret = ath10k_hw_diag_segment_download(ar,
> +							      buf,
> +							      base_addr,
> +							      base_len);

This 'base_len' is determined by the firmware blob and in common cases
is over 500K. The PCI implementation currently tries to
dma_alloc_coherent() a bounce buffer for this. Many systems can't
acquire that large of contiguous DMA memory reliably, so this is isn't
very effective. Can we improve the strategy here? Do you lose a lot of
speed if you do this in smaller (a few pages?) chunks instead?

Brian

> +
> +			if (ret)
> +				ath10k_warn(ar,
> +					    "failed to download firmware via diag interface:%d\n",
> +					    ret);
> +			break;
...

Re: [PATCH 3/3] ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.

[Carl Huang]

On 2018-09-22 08:53, Brian Norris wrote:
> Hi again!
> 
> One last (?) comment:
> 
> On Thu, Aug 30, 2018 at 10:29:42AM +0800, Carl Huang wrote:
> 
>> diff --git a/drivers/net/wireless/ath/ath10k/hw.c 
>> b/drivers/net/wireless/ath/ath10k/hw.c
>> index 677535b..25ee1c6 100644
>> --- a/drivers/net/wireless/ath/ath10k/hw.c
>> +++ b/drivers/net/wireless/ath/ath10k/hw.c
> 
>> @@ -918,6 +919,190 @@ static int 
>> ath10k_hw_qca6174_enable_pll_clock(struct ath10k *ar)
> 
>> +static int ath10k_hw_diag_segment_download(struct ath10k *ar,
>> +					   const void *buffer,
>> +					   u32 address,
>> +					   u32 length)
>> +{
>> +	if (address >= DRAM_BASE_ADDRESS + REGION_ACCESS_SIZE_LIMIT)
>> +		/* Needs to change MSB for memory write */
>> +		return ath10k_hw_diag_segment_msb_download(ar, buffer,
>> +							   address, length);
>> +	else
>> +		return ath10k_hif_diag_write(ar, address, buffer, length);
>> +}
>> +
>> +int ath10k_hw_diag_fast_download(struct ath10k *ar,
>> +				 u32 address,
>> +				 const void *buffer,
>> +				 u32 length)
>> +{
>> +	const u8 *buf = buffer;
>> +	bool sgmt_end = false;
>> +	u32 base_addr = 0;
>> +	u32 base_len = 0;
>> +	u32 left = 0;
>> +	struct bmi_segmented_file_header *hdr;
>> +	struct bmi_segmented_metadata *metadata;
>> +	int ret = 0;
>> +
>> +	if (length < sizeof(*hdr))
>> +		return -EINVAL;
>> +
>> +	/* check firmware header. If it has no correct magic number
>> +	 * or it's compressed, returns error.
>> +	 */
>> +	hdr = (struct bmi_segmented_file_header *)buf;
>> +	if (hdr->magic_num != BMI_SGMTFILE_MAGIC_NUM) {
>> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
>> +			   "Not a supported firmware, magic_num:0x%x\n",
>> +			   hdr->magic_num);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->file_flags != 0) {
>> +		ath10k_dbg(ar, ATH10K_DBG_BOOT,
>> +			   "Not a supported firmware, file_flags:0x%x\n",
>> +			   hdr->file_flags);
>> +		return -EINVAL;
>> +	}
>> +
>> +	metadata = (struct bmi_segmented_metadata *)hdr->data;
>> +	left = length - sizeof(*hdr);
>> +
>> +	while (left > 0) {
>> +		base_addr = metadata->addr;
>> +		base_len = metadata->length;
>> +		buf = metadata->data;
>> +		left -= sizeof(*metadata);
>> +
>> +		switch (base_len) {
> ...
>> +		default:
>> +			if (base_len > left) {
>> +				/* sanity check */
>> +				ath10k_warn(ar,
>> +					    "firmware has invalid segment length, %d > %d\n",
>> +					    base_len, left);
>> +				ret = -EINVAL;
>> +				break;
>> +			}
>> +
>> +			ret = ath10k_hw_diag_segment_download(ar,
>> +							      buf,
>> +							      base_addr,
>> +							      base_len);
> 
> This 'base_len' is determined by the firmware blob and in common cases
> is over 500K. The PCI implementation currently tries to
> dma_alloc_coherent() a bounce buffer for this. Many systems can't
> acquire that large of contiguous DMA memory reliably, so this is isn't
> very effective. Can we improve the strategy here? Do you lose a lot of
> speed if you do this in smaller (a few pages?) chunks instead?
> 

It's a sound complaint. I'll submit a patch for it.


> Brian
> 
>> +
>> +			if (ret)
>> +				ath10k_warn(ar,
>> +					    "failed to download firmware via diag interface:%d\n",
>> +					    ret);
>> +			break;
> ...