diff mbox series

[v4,07/13] wifi: ath12k: add support for fixed QMI firmware memory

Message ID 20241210074159.2637933-8-quic_rajkbhag@quicinc.com (mailing list archive)
State Deferred
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: add Ath12k AHB driver support for IPQ5332 | expand

Commit Message

Raj Kumar Bhagat Dec. 10, 2024, 7:41 a.m. UTC
IPQ5332 firmware supports only fixed QMI firmware memory.

Hence, add support to read reserved fixed memory region from
device-tree and provide the reserved memory segments for
firmware to use during QMI firmware memory request.

Note that the ability to set the fixed memory will be introduced in
a subsequent patch. Currently, the flag remains unset by default,
ensuring that existing chipsets are unaffected.

Tested-on: IPQ5332 hw1.0 AHB WLAN.WBE.1.3.1-00130-QCAHKSWPL_SILICONZ-1
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00210-QCAHKSWPL_SILICONZ-1

Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.h |   1 +
 drivers/net/wireless/ath/ath12k/hw.c   |   4 +
 drivers/net/wireless/ath/ath12k/hw.h   |   1 +
 drivers/net/wireless/ath/ath12k/qmi.c  | 141 +++++++++++++++++++++++--
 4 files changed, 141 insertions(+), 6 deletions(-)

Comments

Krzysztof Kozlowski Dec. 10, 2024, 2:43 p.m. UTC | #1
On 10/12/2024 08:41, Raj Kumar Bhagat wrote:
> +		case CALDB_MEM_REGION_TYPE:
> +			/* Cold boot calibration is not enabled in Ath12k. Hence,
> +			 * assign paddr = 0.
> +			 * Once cold boot calibration is enabled add support to
> +			 * assign reserved memory from DT.
> +			 */
> +			ab->qmi.target_mem[idx].paddr = 0;
> +			ab->qmi.target_mem[idx].v.ioaddr = NULL;
> +			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
> +			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
> +			idx++;
> +			break;
> +		case M3_DUMP_REGION_TYPE:
> +			dev_node = of_find_node_by_name(NULL, "m3_dump");

NAK

That's neither correct name nor documented in the bindings. You created
now undocumented ABI. Even with incorrect name. :/


> +			if (!dev_node || of_address_to_resource(dev_node, 0, &m3_res)) {
> +				ath12k_err(ab, "m3_dump not defined in device-tree\n");
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +


Best regards,
Krzysztof
Raj Kumar Bhagat Dec. 10, 2024, 5 p.m. UTC | #2
On 12/10/2024 8:13 PM, Krzysztof Kozlowski wrote:
> On 10/12/2024 08:41, Raj Kumar Bhagat wrote:
>> +		case CALDB_MEM_REGION_TYPE:
>> +			/* Cold boot calibration is not enabled in Ath12k. Hence,
>> +			 * assign paddr = 0.
>> +			 * Once cold boot calibration is enabled add support to
>> +			 * assign reserved memory from DT.
>> +			 */
>> +			ab->qmi.target_mem[idx].paddr = 0;
>> +			ab->qmi.target_mem[idx].v.ioaddr = NULL;
>> +			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
>> +			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
>> +			idx++;
>> +			break;
>> +		case M3_DUMP_REGION_TYPE:
>> +			dev_node = of_find_node_by_name(NULL, "m3_dump");
> 
> NAK
> 
> That's neither correct name nor documented in the bindings. You created
> now undocumented ABI. Even with incorrect name. :/
> 

Most of the Device Tree related concern in this series are from the
undocumented ABIs and wrong naming (use of '_' instead of '-'):
"m3_dump" and "mlo_global_mem_0".

To address the undocumented ABIs, "memory-region" and "memory-region-names"
should be used to reference all the reserved memory required. This should
include "m3_dump" and "mlo_global_mem_0" memory region.

If the above approach valid to address undocumented ABIs?
Krzysztof Kozlowski Dec. 11, 2024, 8:52 a.m. UTC | #3
On Tue, Dec 10, 2024 at 10:30:54PM +0530, Raj Kumar Bhagat wrote:
> On 12/10/2024 8:13 PM, Krzysztof Kozlowski wrote:
> > On 10/12/2024 08:41, Raj Kumar Bhagat wrote:
> >> +		case CALDB_MEM_REGION_TYPE:
> >> +			/* Cold boot calibration is not enabled in Ath12k. Hence,
> >> +			 * assign paddr = 0.
> >> +			 * Once cold boot calibration is enabled add support to
> >> +			 * assign reserved memory from DT.
> >> +			 */
> >> +			ab->qmi.target_mem[idx].paddr = 0;
> >> +			ab->qmi.target_mem[idx].v.ioaddr = NULL;
> >> +			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
> >> +			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
> >> +			idx++;
> >> +			break;
> >> +		case M3_DUMP_REGION_TYPE:
> >> +			dev_node = of_find_node_by_name(NULL, "m3_dump");
> > 
> > NAK
> > 
> > That's neither correct name nor documented in the bindings. You created
> > now undocumented ABI. Even with incorrect name. :/
> > 
> 
> Most of the Device Tree related concern in this series are from the
> undocumented ABIs and wrong naming (use of '_' instead of '-'):
> "m3_dump" and "mlo_global_mem_0".
> 
> To address the undocumented ABIs, "memory-region" and "memory-region-names"
> should be used to reference all the reserved memory required. This should
> include "m3_dump" and "mlo_global_mem_0" memory region.

You already use them, so this code here and explanation is confusing.

> 
> If the above approach valid to address undocumented ABIs?

Dunno. Change all node names, run dtbs_check. Do you see errors? Now run
your driver. Does it work correctly? If yes, then ABI seems documented.
If not, ABI is not documented.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index e45ff85b675d..5e49fd6c8bc9 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -235,6 +235,7 @@  enum ath12k_dev_flags {
 	ATH12K_FLAG_CE_IRQ_ENABLED,
 	ATH12K_FLAG_EXT_IRQ_ENABLED,
 	ATH12K_FLAG_QMI_FW_READY_COMPLETE,
+	ATH12K_FLAG_FIXED_MEM_REGION,
 };
 
 struct ath12k_tx_conf {
diff --git a/drivers/net/wireless/ath/ath12k/hw.c b/drivers/net/wireless/ath/ath12k/hw.c
index 494963b19fe9..4db4bb1bea95 100644
--- a/drivers/net/wireless/ath/ath12k/hw.c
+++ b/drivers/net/wireless/ath/ath12k/hw.c
@@ -1318,6 +1318,7 @@  static const struct ath12k_hw_params ath12k_hw_params[] = {
 		.needs_m3_fw = true,
 		.ce_ie_addr = NULL,
 		.ce_remap = NULL,
+		.bdf_addr = 0,
 	},
 	{
 		.name = "wcn7850 hw2.0",
@@ -1402,6 +1403,7 @@  static const struct ath12k_hw_params ath12k_hw_params[] = {
 		.needs_m3_fw = true,
 		.ce_ie_addr = NULL,
 		.ce_remap = NULL,
+		.bdf_addr = 0,
 	},
 	{
 		.name = "qcn9274 hw2.0",
@@ -1482,6 +1484,7 @@  static const struct ath12k_hw_params ath12k_hw_params[] = {
 		.needs_m3_fw = true,
 		.ce_ie_addr = NULL,
 		.ce_remap = NULL,
+		.bdf_addr = 0,
 	},
 	{
 		.name = "ipq5332 hw1.0",
@@ -1557,6 +1560,7 @@  static const struct ath12k_hw_params ath12k_hw_params[] = {
 		.needs_m3_fw = false,
 		.ce_ie_addr = &ath12k_ce_ie_addr_ipq5332,
 		.ce_remap = &ath12k_ce_remap_ipq5332,
+		.bdf_addr = 0x4B500000,
 	},
 };
 
diff --git a/drivers/net/wireless/ath/ath12k/hw.h b/drivers/net/wireless/ath/ath12k/hw.h
index ee37e50476a4..524fab05768f 100644
--- a/drivers/net/wireless/ath/ath12k/hw.h
+++ b/drivers/net/wireless/ath/ath12k/hw.h
@@ -224,6 +224,7 @@  struct ath12k_hw_params {
 	bool needs_m3_fw;
 	const struct ce_ie_addr *ce_ie_addr;
 	const struct ce_remap *ce_remap;
+	u32 bdf_addr;
 };
 
 struct ath12k_hw_ops {
diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
index 7a29a24b9268..337347035605 100644
--- a/drivers/net/wireless/ath/ath12k/qmi.c
+++ b/drivers/net/wireless/ath/ath12k/qmi.c
@@ -11,6 +11,8 @@ 
 #include "debug.h"
 #include <linux/of.h>
 #include <linux/firmware.h>
+#include <linux/of_address.h>
+#include <linux/ioport.h>
 
 #define SLEEP_CLOCK_SELECT_INTERNAL_BIT	0x02
 #define HOST_CSTATE_BIT			0x04
@@ -2295,7 +2297,8 @@  int ath12k_qmi_respond_fw_mem_request(struct ath12k_base *ab)
 	 * failure to firmware and firmware then request multiple blocks of
 	 * small chunk size memory.
 	 */
-	if (ab->qmi.target_mem_delayed) {
+	if (!test_bit(ATH12K_FLAG_FIXED_MEM_REGION, &ab->dev_flags) &&
+	    ab->qmi.target_mem_delayed) {
 		delayed = true;
 		ath12k_dbg(ab, ATH12K_DBG_QMI, "qmi delays mem_request %d\n",
 			   ab->qmi.mem_seg_count);
@@ -2358,6 +2361,11 @@  static void ath12k_qmi_free_target_mem_chunk(struct ath12k_base *ab)
 	int i;
 
 	for (i = 0; i < ab->qmi.mem_seg_count; i++) {
+		if (test_bit(ATH12K_FLAG_FIXED_MEM_REGION, &ab->dev_flags) &&
+		    ab->qmi.target_mem[i].v.ioaddr) {
+			iounmap(ab->qmi.target_mem[i].v.ioaddr);
+			ab->qmi.target_mem[i].v.ioaddr = NULL;
+		}
 		if (!ab->qmi.target_mem[i].v.addr)
 			continue;
 
@@ -2436,6 +2444,118 @@  static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
 	return 0;
 }
 
+static int ath12k_qmi_assign_target_mem_chunk(struct ath12k_base *ab)
+{
+	struct device *dev = ab->dev;
+	struct device_node *mem_node, *dev_node;
+	struct resource res, m3_res;
+	int i, idx, ret;
+
+	for (i = 0, idx = 0; i < ab->qmi.mem_seg_count; i++) {
+		switch (ab->qmi.target_mem[i].type) {
+		case HOST_DDR_REGION_TYPE:
+			mem_node = of_parse_phandle(dev->of_node, "memory-region", 0);
+			if (!mem_node) {
+				ath12k_dbg(ab, ATH12K_DBG_QMI,
+					   "memory-region not defined in device-tree\n");
+				ret = -ENODEV;
+				goto out;
+			}
+
+			ret = of_address_to_resource(mem_node, 0, &res);
+			of_node_put(mem_node);
+			if (ret) {
+				ath12k_dbg(ab, ATH12K_DBG_QMI,
+					   "fail to get reg from memory-region\n");
+				goto out;
+			}
+
+			if (res.end - res.start + 1 < ab->qmi.target_mem[i].size) {
+				ath12k_dbg(ab, ATH12K_DBG_QMI,
+					   "failed to assign mem type %d req size %d avail size %lld\n",
+					   ab->qmi.target_mem[i].type,
+					   ab->qmi.target_mem[i].size,
+					   (res.end - res.start + 1));
+				ret = -EINVAL;
+				goto out;
+			}
+
+			ab->qmi.target_mem[idx].paddr = res.start;
+			ab->qmi.target_mem[idx].v.ioaddr =
+				ioremap(ab->qmi.target_mem[idx].paddr,
+					ab->qmi.target_mem[i].size);
+			if (!ab->qmi.target_mem[idx].v.ioaddr) {
+				ret = -EIO;
+				goto out;
+			}
+			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
+			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
+			idx++;
+			break;
+		case BDF_MEM_REGION_TYPE:
+			ab->qmi.target_mem[idx].paddr = ab->hw_params->bdf_addr;
+			ab->qmi.target_mem[idx].v.ioaddr = NULL;
+			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
+			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
+			idx++;
+			break;
+
+		case CALDB_MEM_REGION_TYPE:
+			/* Cold boot calibration is not enabled in Ath12k. Hence,
+			 * assign paddr = 0.
+			 * Once cold boot calibration is enabled add support to
+			 * assign reserved memory from DT.
+			 */
+			ab->qmi.target_mem[idx].paddr = 0;
+			ab->qmi.target_mem[idx].v.ioaddr = NULL;
+			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
+			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
+			idx++;
+			break;
+		case M3_DUMP_REGION_TYPE:
+			dev_node = of_find_node_by_name(NULL, "m3_dump");
+			if (!dev_node || of_address_to_resource(dev_node, 0, &m3_res)) {
+				ath12k_err(ab, "m3_dump not defined in device-tree\n");
+				ret = -EINVAL;
+				goto out;
+			}
+
+			if (m3_res.end - m3_res.start + 1 < ab->qmi.target_mem[i].size) {
+				ath12k_dbg(ab, ATH12K_DBG_QMI,
+					   "failed to assign mem type %d req size %d avail size %lld\n",
+					   ab->qmi.target_mem[i].type,
+					   ab->qmi.target_mem[i].size,
+					   (m3_res.end - m3_res.start + 1));
+				ret = -EINVAL;
+				goto out;
+			}
+
+			ab->qmi.target_mem[idx].paddr = m3_res.start;
+			ab->qmi.target_mem[idx].v.ioaddr =
+				ioremap(ab->qmi.target_mem[idx].paddr,
+					ab->qmi.target_mem[i].size);
+			if (!ab->qmi.target_mem[idx].v.ioaddr) {
+				ret = -EIO;
+				goto out;
+			}
+			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
+			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
+			idx++;
+			break;
+		default:
+			ath12k_warn(ab, "qmi ignore invalid mem req type %d\n",
+				    ab->qmi.target_mem[i].type);
+			break;
+		}
+	}
+	ab->qmi.mem_seg_count = idx;
+
+	return 0;
+out:
+	ath12k_qmi_free_target_mem_chunk(ab);
+	return ret;
+}
+
 /* clang stack usage explodes if this is inlined */
 static noinline_for_stack
 int ath12k_qmi_request_target_cap(struct ath12k_base *ab)
@@ -3270,11 +3390,20 @@  static void ath12k_qmi_msg_mem_request_cb(struct qmi_handle *qmi_hdl,
 			   msg->mem_seg[i].type, msg->mem_seg[i].size);
 	}
 
-	ret = ath12k_qmi_alloc_target_mem_chunk(ab);
-	if (ret) {
-		ath12k_warn(ab, "qmi failed to alloc target memory: %d\n",
-			    ret);
-		return;
+	if (test_bit(ATH12K_FLAG_FIXED_MEM_REGION, &ab->dev_flags)) {
+		ret = ath12k_qmi_assign_target_mem_chunk(ab);
+		if (ret) {
+			ath12k_warn(ab, "failed to assign qmi target memory: %d\n",
+				    ret);
+			return;
+		}
+	} else {
+		ret = ath12k_qmi_alloc_target_mem_chunk(ab);
+		if (ret) {
+			ath12k_warn(ab, "qmi failed to alloc target memory: %d\n",
+				    ret);
+			return;
+		}
 	}
 
 	ath12k_qmi_driver_event_post(qmi, ATH12K_QMI_EVENT_REQUEST_MEM, NULL);