[PATCH v2 0/2] wifi: ath11k: Add support for sram dump

[PATCH v2 0/2] wifi: ath11k: Add support for sram dump

[Baochen Qiang]

On-board sram contains valuable information for debug, this patch adds
a new interface named "sram" in debugfs with which we can dump sram
content using following cmd:

        cat /sys/kernel/debug/ath11k/<pdev name>/sram > sram.dump

v2:
 1: rebased on latest ath.git.
 2. drop the patch "ath11k: Move pdev debugfs creation ahead"
    per Kalle's comments.

Baochen Qiang (2):
  wifi: ath11k: Split PCI write/read functions
  wifi: ath11k: Implement sram dump interface

 drivers/net/wireless/ath/ath11k/core.c    |  7 ++
 drivers/net/wireless/ath/ath11k/debugfs.c | 62 ++++++++++++++++++
 drivers/net/wireless/ath/ath11k/hif.h     | 10 +++
 drivers/net/wireless/ath/ath11k/hw.c      | 10 +++
 drivers/net/wireless/ath/ath11k/hw.h      |  9 +++
 drivers/net/wireless/ath/ath11k/pci.c     |  1 +
 drivers/net/wireless/ath/ath11k/pcic.c    | 79 +++++++++++++++++------
 drivers/net/wireless/ath/ath11k/pcic.h    |  1 +
 8 files changed, 161 insertions(+), 18 deletions(-)


base-commit: b93992f874a92c264a0d0bda3bbf44b84eff4321
-- 
2.25.1

[PATCH v2 1/2] wifi: ath11k: Split PCI write/read functions

[Baochen Qiang]

ath11k_pcic_write32/read32 tries to do wake up before doing actual
write/read work, which means each time a u32 is written/read, wake
up is performed. This is not necessary in case where we do a
large amount of write/read, because only one time of wake up is needed.
So split each one into two parts, the first part does wake up and
release, and the second one does actual write/read work.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1

Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
---
v2:
  1: rebased on latest ath.git

 drivers/net/wireless/ath/ath11k/pcic.c | 50 ++++++++++++++++----------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c
index 1adf20ebef27..15ca069e501f 100644
--- a/drivers/net/wireless/ath/ath11k/pcic.c
+++ b/drivers/net/wireless/ath/ath11k/pcic.c
@@ -140,49 +140,63 @@ int ath11k_pcic_init_msi_config(struct ath11k_base *ab)
 }
 EXPORT_SYMBOL(ath11k_pcic_init_msi_config);
 
+static void ath11k_pcic_do_write32(struct ath11k_base *ab, u32 offset, u32 value)
+{
+	if (offset < ATH11K_PCI_WINDOW_START)
+		iowrite32(value, ab->mem  + offset);
+	else
+		ab->pci.ops->window_write32(ab, offset, value);
+}
+
 void ath11k_pcic_write32(struct ath11k_base *ab, u32 offset, u32 value)
 {
 	int ret = 0;
+	bool wakeup_required;
 
 	/* for offset beyond BAR + 4K - 32, may
 	 * need to wakeup the device to access.
 	 */
-	if (test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
-	    offset >= ATH11K_PCI_ACCESS_ALWAYS_OFF && ab->pci.ops->wakeup)
+	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
+			  offset >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
+	if (wakeup_required && ab->pci.ops->wakeup)
 		ret = ab->pci.ops->wakeup(ab);
 
-	if (offset < ATH11K_PCI_WINDOW_START)
-		iowrite32(value, ab->mem  + offset);
-	else
-		ab->pci.ops->window_write32(ab, offset, value);
+	ath11k_pcic_do_write32(ab, offset, value);
 
-	if (test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
-	    offset >= ATH11K_PCI_ACCESS_ALWAYS_OFF && ab->pci.ops->release &&
-	    !ret)
+	if (wakeup_required && !ret && ab->pci.ops->release)
 		ab->pci.ops->release(ab);
 }
 EXPORT_SYMBOL(ath11k_pcic_write32);
 
+static u32 ath11k_pcic_do_read32(struct ath11k_base *ab, u32 offset)
+{
+	u32 val;
+
+	if (offset < ATH11K_PCI_WINDOW_START)
+		val = ioread32(ab->mem + offset);
+	else
+		val = ab->pci.ops->window_read32(ab, offset);
+
+	return val;
+}
+
 u32 ath11k_pcic_read32(struct ath11k_base *ab, u32 offset)
 {
 	int ret = 0;
 	u32 val;
+	bool wakeup_required;
 
 	/* for offset beyond BAR + 4K - 32, may
 	 * need to wakeup the device to access.
 	 */
-	if (test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
-	    offset >= ATH11K_PCI_ACCESS_ALWAYS_OFF && ab->pci.ops->wakeup)
+	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
+			  offset >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
+	if (wakeup_required && ab->pci.ops->wakeup)
 		ret = ab->pci.ops->wakeup(ab);
 
-	if (offset < ATH11K_PCI_WINDOW_START)
-		val = ioread32(ab->mem + offset);
-	else
-		val = ab->pci.ops->window_read32(ab, offset);
+	val = ath11k_pcic_do_read32(ab, offset);
 
-	if (test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
-	    offset >= ATH11K_PCI_ACCESS_ALWAYS_OFF && ab->pci.ops->release &&
-	    !ret)
+	if (wakeup_required && !ret && ab->pci.ops->release)
 		ab->pci.ops->release(ab);
 
 	return val;
-- 
2.25.1

[PATCH v2 2/2] wifi: ath11k: Implement sram dump interface

[Baochen Qiang]

Currently this feature is enabled for QCA6390/WCN6855.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1

Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
---
v2:
  1: rebased on latest ath.git

 drivers/net/wireless/ath/ath11k/core.c    |  7 +++
 drivers/net/wireless/ath/ath11k/debugfs.c | 62 +++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/hif.h     | 10 ++++
 drivers/net/wireless/ath/ath11k/hw.c      | 10 ++++
 drivers/net/wireless/ath/ath11k/hw.h      |  9 ++++
 drivers/net/wireless/ath/ath11k/pci.c     |  1 +
 drivers/net/wireless/ath/ath11k/pcic.c    | 29 +++++++++++
 drivers/net/wireless/ath/ath11k/pcic.h    |  1 +
 8 files changed, 129 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index c3e9e4f7bc24..44003d39ff7b 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -106,6 +106,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = false,
 		.fixed_fw_mem = false,
 		.support_off_channel_tx = false,
+		.sram_dump = NULL,
 	},
 	{
 		.hw_rev = ATH11K_HW_IPQ6018_HW10,
@@ -177,6 +178,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = false,
 		.fixed_fw_mem = false,
 		.support_off_channel_tx = false,
+		.sram_dump = NULL,
 	},
 	{
 		.name = "qca6390 hw2.0",
@@ -247,6 +249,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = false,
 		.fixed_fw_mem = false,
 		.support_off_channel_tx = true,
+		.sram_dump = &sram_dump_qca6390,
 	},
 	{
 		.name = "qcn9074 hw1.0",
@@ -317,6 +320,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = false,
 		.fixed_fw_mem = false,
 		.support_off_channel_tx = false,
+		.sram_dump = NULL,
 	},
 	{
 		.name = "wcn6855 hw2.0",
@@ -387,6 +391,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = false,
 		.fixed_fw_mem = false,
 		.support_off_channel_tx = true,
+		.sram_dump = &sram_dump_wcn6855,
 	},
 	{
 		.name = "wcn6855 hw2.1",
@@ -456,6 +461,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = false,
 		.fixed_fw_mem = false,
 		.support_off_channel_tx = true,
+		.sram_dump = &sram_dump_wcn6855,
 	},
 	{
 		.name = "wcn6750 hw1.0",
@@ -525,6 +531,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 		.hybrid_bus_type = true,
 		.fixed_fw_mem = true,
 		.support_off_channel_tx = false,
+		.sram_dump = NULL,
 	},
 };
 
diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c
index 9648e0017393..ff7b35ac4154 100644
--- a/drivers/net/wireless/ath/ath11k/debugfs.c
+++ b/drivers/net/wireless/ath/ath11k/debugfs.c
@@ -14,6 +14,7 @@
 #include "dp_tx.h"
 #include "debugfs_htt_stats.h"
 #include "peer.h"
+#include "hif.h"
 
 static const char *htt_bp_umac_ring[HTT_SW_UMAC_RING_IDX_MAX] = {
 	"REO2SW1_RING",
@@ -982,6 +983,63 @@ static const struct file_operations fops_fw_dbglog = {
 	.llseek = default_llseek,
 };
 
+static int ath11k_open_sram_dump(struct inode *inode, struct file *file)
+{
+	struct ath11k_base *ab = inode->i_private;
+	u8 *buf;
+	u32 start, end;
+	int ret;
+
+	start = ab->hw_params.sram_dump->start;
+	end = ab->hw_params.sram_dump->end;
+
+	buf = vmalloc(end - start + 1);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ath11k_hif_dump_sram(ab, buf, start, end);
+	if (ret) {
+		ath11k_err(ab, "failed to dump sram: %d\n", ret);
+		vfree(buf);
+		return ret;
+	}
+
+	file->private_data = buf;
+	return 0;
+}
+
+static ssize_t ath11k_read_sram_dump(struct file *file,
+				     char __user *user_buf,
+				     size_t count, loff_t *ppos)
+{
+	struct ath11k_base *ab = file->f_inode->i_private;
+	const char *buf = file->private_data;
+	int len;
+	u32 start, end;
+
+	start = ab->hw_params.sram_dump->start;
+	end = ab->hw_params.sram_dump->end;
+	len = end - start + 1;
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static int ath11k_release_sram_dump(struct inode *inode, struct file *file)
+{
+	vfree(file->private_data);
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static const struct file_operations fops_sram_dump = {
+	.open = ath11k_open_sram_dump,
+	.read = ath11k_read_sram_dump,
+	.release = ath11k_release_sram_dump,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 int ath11k_debugfs_pdev_create(struct ath11k_base *ab)
 {
 	if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags))
@@ -997,6 +1055,10 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab)
 	debugfs_create_file("soc_dp_stats", 0600, ab->debugfs_soc, ab,
 			    &fops_soc_dp_stats);
 
+	if (ab->hw_params.sram_dump)
+		debugfs_create_file("sram", 0400, ab->debugfs_soc, ab,
+				    &fops_sram_dump);
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/ath/ath11k/hif.h b/drivers/net/wireless/ath/ath11k/hif.h
index e9366f786fbb..8fcf7500e5c6 100644
--- a/drivers/net/wireless/ath/ath11k/hif.h
+++ b/drivers/net/wireless/ath/ath11k/hif.h
@@ -29,6 +29,7 @@ struct ath11k_hif_ops {
 	void (*ce_irq_enable)(struct ath11k_base *ab);
 	void (*ce_irq_disable)(struct ath11k_base *ab);
 	void (*get_ce_msi_idx)(struct ath11k_base *ab, u32 ce_id, u32 *msi_idx);
+	int (*dump_sram)(struct ath11k_base *ab, u8 *buf, u32 start, u32 end);
 };
 
 static inline void ath11k_hif_ce_irq_enable(struct ath11k_base *ab)
@@ -134,4 +135,13 @@ static inline void ath11k_get_ce_msi_idx(struct ath11k_base *ab, u32 ce_id,
 	else
 		*msi_data_idx = ce_id;
 }
+
+static inline int ath11k_hif_dump_sram(struct ath11k_base *ab, u8 *buf,
+				       u32 start, u32 end)
+{
+	if (!ab->hif.ops->dump_sram)
+		return -EOPNOTSUPP;
+
+	return ab->hif.ops->dump_sram(ab, buf, start, end);
+}
 #endif /* _HIF_H_ */
diff --git a/drivers/net/wireless/ath/ath11k/hw.c b/drivers/net/wireless/ath/ath11k/hw.c
index 96db85c55585..ff30ba852200 100644
--- a/drivers/net/wireless/ath/ath11k/hw.c
+++ b/drivers/net/wireless/ath/ath11k/hw.c
@@ -2359,3 +2359,13 @@ const struct cfg80211_sar_capa ath11k_hw_sar_capa_wcn6855 = {
 	.num_freq_ranges = (ARRAY_SIZE(ath11k_hw_sar_freq_ranges_wcn6855)),
 	.freq_ranges = ath11k_hw_sar_freq_ranges_wcn6855,
 };
+
+const struct ath11k_hw_sram_dump sram_dump_qca6390 = {
+	.start = 0x01400000,
+	.end = 0x0171ffff,
+};
+
+const struct ath11k_hw_sram_dump sram_dump_wcn6855 = {
+	.start = 0x01400000,
+	.end = 0x0177ffff,
+};
diff --git a/drivers/net/wireless/ath/ath11k/hw.h b/drivers/net/wireless/ath/ath11k/hw.h
index bb5ac940e470..2c3988e03aae 100644
--- a/drivers/net/wireless/ath/ath11k/hw.h
+++ b/drivers/net/wireless/ath/ath11k/hw.h
@@ -126,6 +126,11 @@ struct ath11k_hw_hal_params {
 	enum hal_rx_buf_return_buf_manager rx_buf_rbm;
 };
 
+struct ath11k_hw_sram_dump {
+	u32 start;
+	u32 end;
+};
+
 struct ath11k_hw_params {
 	const char *name;
 	u16 hw_rev;
@@ -200,6 +205,7 @@ struct ath11k_hw_params {
 	bool hybrid_bus_type;
 	bool fixed_fw_mem;
 	bool support_off_channel_tx;
+	const struct ath11k_hw_sram_dump *sram_dump;
 };
 
 struct ath11k_hw_ops {
@@ -397,4 +403,7 @@ static inline const char *ath11k_bd_ie_type_str(enum ath11k_bd_ie_type type)
 }
 
 extern const struct cfg80211_sar_capa ath11k_hw_sar_capa_wcn6855;
+
+extern const struct ath11k_hw_sram_dump sram_dump_qca6390;
+extern const struct ath11k_hw_sram_dump sram_dump_wcn6855;
 #endif
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 5bd34a6273d9..f853bf4a12c7 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -697,6 +697,7 @@ static const struct ath11k_hif_ops ath11k_pci_hif_ops = {
 	.ce_irq_enable = ath11k_pci_hif_ce_irq_enable,
 	.ce_irq_disable = ath11k_pci_hif_ce_irq_disable,
 	.get_ce_msi_idx = ath11k_pcic_get_ce_msi_idx,
+	.dump_sram = ath11k_pcic_dump_sram,
 };
 
 static void ath11k_pci_read_hw_version(struct ath11k_base *ab, u32 *major, u32 *minor)
diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c
index 15ca069e501f..c9027eafe65b 100644
--- a/drivers/net/wireless/ath/ath11k/pcic.c
+++ b/drivers/net/wireless/ath/ath11k/pcic.c
@@ -203,6 +203,35 @@ u32 ath11k_pcic_read32(struct ath11k_base *ab, u32 offset)
 }
 EXPORT_SYMBOL(ath11k_pcic_read32);
 
+int ath11k_pcic_dump_sram(struct ath11k_base *ab, u8 *buf,
+			  u32 start, u32 end)
+{
+	int ret = 0;
+	bool wakeup_required;
+	u32 *data = (u32 *)buf;
+	u32 i;
+
+	/* for offset beyond BAR + 4K - 32, may
+	 * need to wakeup the device to access.
+	 */
+	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
+			  end >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
+	if (wakeup_required && ab->pci.ops->wakeup) {
+		ret = ab->pci.ops->wakeup(ab);
+		if (ret)
+			ath11k_warn(ab, "%s: failed to do wakeup: %d\n", __func__, ret);
+	}
+
+	for (i = start; i < end + 1; i += 4)
+		*data++ = ath11k_pcic_do_read32(ab, i);
+
+	if (wakeup_required && !ret && ab->pci.ops->release)
+		ab->pci.ops->release(ab);
+
+	return 0;
+}
+EXPORT_SYMBOL(ath11k_pcic_dump_sram);
+
 void ath11k_pcic_get_msi_address(struct ath11k_base *ab, u32 *msi_addr_lo,
 				 u32 *msi_addr_hi)
 {
diff --git a/drivers/net/wireless/ath/ath11k/pcic.h b/drivers/net/wireless/ath/ath11k/pcic.h
index 0afbb34510db..40353246ab0b 100644
--- a/drivers/net/wireless/ath/ath11k/pcic.h
+++ b/drivers/net/wireless/ath/ath11k/pcic.h
@@ -45,4 +45,5 @@ void ath11k_pcic_ce_irq_disable_sync(struct ath11k_base *ab);
 int ath11k_pcic_init_msi_config(struct ath11k_base *ab);
 int ath11k_pcic_register_pci_ops(struct ath11k_base *ab,
 				 const struct ath11k_pci_ops *pci_ops);
+int ath11k_pcic_dump_sram(struct ath11k_base *ab, u8 *buf, u32 start, u32 end);
 #endif
-- 
2.25.1

Re: [PATCH v2 1/2] wifi: ath11k: Split PCI write/read functions

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> ath11k_pcic_write32/read32 tries to do wake up before doing actual
> write/read work, which means each time a u32 is written/read, wake
> up is performed. This is not necessary in case where we do a
> large amount of write/read, because only one time of wake up is needed.
> So split each one into two parts, the first part does wake up and
> release, and the second one does actual write/read work.
>
> Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
>
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> ---
> v2:
>   1: rebased on latest ath.git
>
>  drivers/net/wireless/ath/ath11k/pcic.c | 50 ++++++++++++++++----------
>  1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath11k/pcic.c b/drivers/net/wireless/ath/ath11k/pcic.c
> index 1adf20ebef27..15ca069e501f 100644
> --- a/drivers/net/wireless/ath/ath11k/pcic.c
> +++ b/drivers/net/wireless/ath/ath11k/pcic.c
> @@ -140,49 +140,63 @@ int ath11k_pcic_init_msi_config(struct ath11k_base *ab)
>  }
>  EXPORT_SYMBOL(ath11k_pcic_init_msi_config);
>  
> +static void ath11k_pcic_do_write32(struct ath11k_base *ab, u32 offset, u32 value)

In the pending branch I renamed this __ath11k_pcic_write32(). I find
that more readable than using the word "do" in the middle.

> +static u32 ath11k_pcic_do_read32(struct ath11k_base *ab, u32 offset)

And this to __ath11k_pcic_do_read32().

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v2 2/2] wifi: ath11k: Implement sram dump interface

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> Currently this feature is enabled for QCA6390/WCN6855.
>
> Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
>
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>

I did quite a few changes to this patch in the pending branch, please
check my changes:

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

I improved the commit log.

> --- a/drivers/net/wireless/ath/ath11k/hw.h
> +++ b/drivers/net/wireless/ath/ath11k/hw.h
> @@ -126,6 +126,11 @@ struct ath11k_hw_hal_params {
>  	enum hal_rx_buf_return_buf_manager rx_buf_rbm;
>  };
>  
> +struct ath11k_hw_sram_dump {
> +	u32 start;
> +	u32 end;
> +};
> +
>  struct ath11k_hw_params {
>  	const char *name;
>  	u16 hw_rev;
> @@ -200,6 +205,7 @@ struct ath11k_hw_params {
>  	bool hybrid_bus_type;
>  	bool fixed_fw_mem;
>  	bool support_off_channel_tx;
> +	const struct ath11k_hw_sram_dump *sram_dump;
>  };

Instead of separate structures I used inline structures:

		.sram_dump = {
			.start = 0x01400000,
			.end = 0x0177ffff,
		},

> --- a/drivers/net/wireless/ath/ath11k/pcic.c
> +++ b/drivers/net/wireless/ath/ath11k/pcic.c
> @@ -203,6 +203,35 @@ u32 ath11k_pcic_read32(struct ath11k_base *ab, u32 offset)
>  }
>  EXPORT_SYMBOL(ath11k_pcic_read32);
>  
> +int ath11k_pcic_dump_sram(struct ath11k_base *ab, u8 *buf,
> +			  u32 start, u32 end)
> +{
> +	int ret = 0;
> +	bool wakeup_required;
> +	u32 *data = (u32 *)buf;

I changed buf to a void pointer, then the cast is not needed.

> +	u32 i;
> +
> +	/* for offset beyond BAR + 4K - 32, may
> +	 * need to wakeup the device to access.
> +	 */
> +	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
> +			  end >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
> +	if (wakeup_required && ab->pci.ops->wakeup) {
> +		ret = ab->pci.ops->wakeup(ab);
> +		if (ret)
> +			ath11k_warn(ab, "%s: failed to do wakeup: %d\n", __func__, ret);
> +	}

I changed the error handling so that if wakeup() fails we do not
continue and just return an error.

> +	for (i = start; i < end + 1; i += 4)
> +		*data++ = ath11k_pcic_do_read32(ab, i);
> +
> +	if (wakeup_required && !ret && ab->pci.ops->release)
> +		ab->pci.ops->release(ab);

At the same time I removed the ret check here.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ath11k_pcic_dump_sram);

I renamed this to ath11k_pcic_read() as I feel it's more descriptive
what the function really does. It's not really care is this for sram
dump or something else.

I also renamed hif.h interface to ath11k_hif_read().

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v2 1/2] wifi: ath11k: Split PCI write/read functions

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> wrote:

> ath11k_pcic_write32/read32 tries to do wake up before doing actual
> write/read work, which means each time a u32 is written/read, wake
> up is performed. This is not necessary in case where we do a
> large amount of write/read, because only one time of wake up is needed.
> So split each one into two parts, the first part does wake up and
> release, and the second one does actual write/read work.
> 
> Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
> 
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

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

90aad48eb56f wifi: ath11k: Split PCI write/read functions
876eb84882a8 wifi: ath11k: implement SRAM dump debugfs interface

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220802075533.1744-2-quic_bqiang@quicinc.com/

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

Re: [PATCH v2 2/2] wifi: ath11k: Implement sram dump interface

[Baochen Qiang]

On 9/9/2022 6:48 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
>
>> Currently this feature is enabled for QCA6390/WCN6855.
>>
>> Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> I did quite a few changes to this patch in the pending branch, please
> check my changes:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=074477aacb419493da6fb4d96fa9d12390c3b40e
>
> I improved the commit log.
>
>> --- a/drivers/net/wireless/ath/ath11k/hw.h
>> +++ b/drivers/net/wireless/ath/ath11k/hw.h
>> @@ -126,6 +126,11 @@ struct ath11k_hw_hal_params {
>>   	enum hal_rx_buf_return_buf_manager rx_buf_rbm;
>>   };
>>   
>> +struct ath11k_hw_sram_dump {
>> +	u32 start;
>> +	u32 end;
>> +};
>> +
>>   struct ath11k_hw_params {
>>   	const char *name;
>>   	u16 hw_rev;
>> @@ -200,6 +205,7 @@ struct ath11k_hw_params {
>>   	bool hybrid_bus_type;
>>   	bool fixed_fw_mem;
>>   	bool support_off_channel_tx;
>> +	const struct ath11k_hw_sram_dump *sram_dump;
>>   };
> Instead of separate structures I used inline structures:
>
> 		.sram_dump = {
> 			.start = 0x01400000,
> 			.end = 0x0177ffff,
> 		},
>
>> --- a/drivers/net/wireless/ath/ath11k/pcic.c
>> +++ b/drivers/net/wireless/ath/ath11k/pcic.c
>> @@ -203,6 +203,35 @@ u32 ath11k_pcic_read32(struct ath11k_base *ab, u32 offset)
>>   }
>>   EXPORT_SYMBOL(ath11k_pcic_read32);
>>   
>> +int ath11k_pcic_dump_sram(struct ath11k_base *ab, u8 *buf,
>> +			  u32 start, u32 end)
>> +{
>> +	int ret = 0;
>> +	bool wakeup_required;
>> +	u32 *data = (u32 *)buf;
> I changed buf to a void pointer, then the cast is not needed.
>
>> +	u32 i;
>> +
>> +	/* for offset beyond BAR + 4K - 32, may
>> +	 * need to wakeup the device to access.
>> +	 */
>> +	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
>> +			  end >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
>> +	if (wakeup_required && ab->pci.ops->wakeup) {
>> +		ret = ab->pci.ops->wakeup(ab);
>> +		if (ret)
>> +			ath11k_warn(ab, "%s: failed to do wakeup: %d\n", __func__, ret);
>> +	}
> I changed the error handling so that if wakeup() fails we do not
> continue and just return an error.

I prefer to keep the original design, because in that case we still have 
something to check after firmware crashes.

I admit that the dump content may be invalid if wakeup fails, but we can 
know that by checking kernel log, so we can avoid misleading.

>
>> +	for (i = start; i < end + 1; i += 4)
>> +		*data++ = ath11k_pcic_do_read32(ab, i);
>> +
>> +	if (wakeup_required && !ret && ab->pci.ops->release)
>> +		ab->pci.ops->release(ab);
> At the same time I removed the ret check here.
>
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(ath11k_pcic_dump_sram);
> I renamed this to ath11k_pcic_read() as I feel it's more descriptive
> what the function really does. It's not really care is this for sram
> dump or something else.
>
> I also renamed hif.h interface to ath11k_hif_read().
>

Re: [PATCH v2 2/2] wifi: ath11k: Implement sram dump interface

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

>>> +	u32 i;
>>> +
>>> +	/* for offset beyond BAR + 4K - 32, may
>>> +	 * need to wakeup the device to access.
>>> +	 */
>>> +	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
>>> +			  end >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
>>> +	if (wakeup_required && ab->pci.ops->wakeup) {
>>> +		ret = ab->pci.ops->wakeup(ab);
>>> +		if (ret)
>>> +			ath11k_warn(ab, "%s: failed to do wakeup: %d\n", __func__, ret);
>>> +	}
>>
>> I changed the error handling so that if wakeup() fails we do not
>> continue and just return an error.
>
> I prefer to keep the original design, because in that case we still
> have something to check after firmware crashes.
>
> I admit that the dump content may be invalid if wakeup fails, but we
> can know that by checking kernel log, so we can avoid misleading.

Too late now, I already applied the patch. You need to submit a new
patch to change the logic. And if we really want to ignore the wakeup
failure there should be a proper comment in the code explaining the idea,
and maybe improve the warning message to make it more understandable for
the user.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v2 2/2] wifi: ath11k: Implement sram dump interface

[Baochen Qiang]

On 9/13/2022 3:14 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
>
>>>> +	u32 i;
>>>> +
>>>> +	/* for offset beyond BAR + 4K - 32, may
>>>> +	 * need to wakeup the device to access.
>>>> +	 */
>>>> +	wakeup_required = test_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags) &&
>>>> +			  end >= ATH11K_PCI_ACCESS_ALWAYS_OFF;
>>>> +	if (wakeup_required && ab->pci.ops->wakeup) {
>>>> +		ret = ab->pci.ops->wakeup(ab);
>>>> +		if (ret)
>>>> +			ath11k_warn(ab, "%s: failed to do wakeup: %d\n", __func__, ret);
>>>> +	}
>>> I changed the error handling so that if wakeup() fails we do not
>>> continue and just return an error.
>> I prefer to keep the original design, because in that case we still
>> have something to check after firmware crashes.
>>
>> I admit that the dump content may be invalid if wakeup fails, but we
>> can know that by checking kernel log, so we can avoid misleading.
> Too late now, I already applied the patch. You need to submit a new
> patch to change the logic. And if we really want to ignore the wakeup
> failure there should be a proper comment in the code explaining the idea,
> and maybe improve the warning message to make it more understandable for
> the user.
Sure Kalle, I will send a new patch for that.