Re: [PATCH v2 2/2] ath11k: Use reserved host DDR addresses from DT for PCI devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-11-17 13:35, Kalle Valo wrote:
Anilkumar Kolli <akolli@xxxxxxxxxxxxxx> writes:

Host DDR memory (contiguous 45 MB in mode-0 or 15 MB in mode-2)
is reserved through DT entries for firmware usage. Send the base
address and size from DT entries.

If DT entry is available, PCI devices work with
fixed_mem_region else host allocates multiple segments.

IPQ8074 on HK10 board supports multiple PCI devices.
IPQ8074 + QCN9074 is tested with this patch.

Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.4.0.1-01838-QCAHKSWPL_SILICONZ-1

Signed-off-by: Anilkumar Kolli <akolli@xxxxxxxxxxxxxx>
---
V2:
  - Use of_ API to read from dt node (Rob)

 drivers/net/wireless/ath/ath11k/mhi.c | 42 ++++++++++++++++++--
 drivers/net/wireless/ath/ath11k/pci.c | 10 ++++-
drivers/net/wireless/ath/ath11k/qmi.c | 75 +++++++++++++++++++++++++++++++----
 drivers/net/wireless/ath/ath11k/qmi.h |  1 +
 4 files changed, 116 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index 26c7ae242db6..0ea6ab7edeb9 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -3,6 +3,7 @@

 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/of.h>

 #include "core.h"
 #include "debug.h"
@@ -316,7 +317,11 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci)
 	struct ath11k_base *ab = ab_pci->ab;
 	struct mhi_controller *mhi_ctrl;
 	struct mhi_controller_config *ath11k_mhi_config;
-	int ret;
+	struct device_node *np;
+	int ret, len, sw, aw;
+	u32 *reg, *reg_end;
+	unsigned long start, size;
+	bool no_dt_entry = 0;

Use only true or false with bools.

Sure.

 	mhi_ctrl = mhi_alloc_controller();
 	if (!mhi_ctrl)
@@ -339,8 +344,39 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci)
 		return ret;
 	}

-	mhi_ctrl->iova_start = 0;
-	mhi_ctrl->iova_stop = 0xffffffff;
+	np = of_find_node_by_type(NULL, "memory");
+	if (!np) {
+		no_dt_entry = 1;

true

+		goto no_dt_entry;
+	}
+
+	aw = of_n_addr_cells(np);
+	sw = of_n_size_cells(np);
+
+	reg = (unsigned int *)of_get_property(np, "reg", &len);

Always try to avoid casts. of_get_property() returns 'const void *' so
you are casting out const here, which is a bug. You don't need to do
casting with void pointers so remove it here.

Sure.

But is reg property really unsigned int? And not u32 or dma_addr_t, for
example?

From DT specification, each value in reg is a u32.

In our example
    memory {
        device_type = "memory";
        reg = <0x0 0x40000000 0x0 0x20000000>;
    };

    0x0 is higer 32-bit addr of RAM
    0x40000000 is lower 32-bit addr of RAM
    0x0 is size
    0x20000000 is [1GB RAM] size

+	if (!reg) {
+		no_dt_entry = 1;

true

+		goto no_dt_entry;
+	}
+
+	reg_end = reg + len / (aw * sw);
+
+	do {
+		start = of_read_number(reg, aw);
+		reg += aw;
+		size = of_read_number(reg, sw);

of_read_number() takes 'const __be32 *cell' but reg is 'u32 *'?

Yes.

+		reg += sw;
+	} while (reg < reg_end);
+
+no_dt_entry:
+	if (no_dt_entry) {
+		mhi_ctrl->iova_start = 0;
+		mhi_ctrl->iova_stop = 0xFFFFFFFF;
+	} else {
+		mhi_ctrl->iova_start = (dma_addr_t)(start + 0x1000000);
+		mhi_ctrl->iova_stop = (dma_addr_t)(start + size);

I don't like casts, they hide bugs like the const issue above. Is there
any way to do this without casts?


u64 start, size;

    if (of_property_read_u32_array(np, "reg", reg, 4)) {
         ath11k_dbg(ab, ATH11K_DBG_QMI,
                    "qmi fail to get reg from hremote\n");
         return 0;
    }
    start = reg[0] + reg[1];
    size = reg[2] + reg[3];

I will this code in next patch. still needs typecast form u64 to dma_addr_t. no ?

@@ -1245,6 +1246,13 @@ static int ath11k_pci_probe(struct pci_dev *pdev,
 	pci_set_drvdata(pdev, ab);
 	spin_lock_init(&ab_pci->window_lock);

+ /* Set fixed_mem_region to true for platforms support reserved memory + * from DT. If memory is reserved from DT for FW, ath11k driver need not
+	 * allocate memory.
+	 */
+	if (!of_property_read_u32(ab->dev->of_node, "memory-region", &addr))
+		ab->bus_params.fixed_mem_region = true;

ab->bus_params is supposed to be const to catch this, but clearly it's
not. The idea of bus_params is to have static bus level configuration,
please instead use dev_flags or something else.

Sure.

 static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
 {
-	int i, idx;
+	struct device *dev = ab->dev;
+	struct device_node *hremote_node = NULL;
+	phandle hremote_phandle;
+	int i, idx, len, sw, aw, host_ddr_sz;
+	u32 *reg, *reg_end;
+	u64 start, size;

 	for (i = 0, idx = 0; i < ab->qmi.mem_seg_count; i++) {
 		switch (ab->qmi.target_mem[i].type) {
+		case HOST_DDR_REGION_TYPE:
+			if (of_property_read_u32(dev->of_node, "memory-region",
+						 &hremote_phandle)) {
+				ath11k_dbg(ab, ATH11K_DBG_QMI,
+					   "qmi fail to get hremote phandle\n");
+				return 0;
+			}
+
+			hremote_node = of_find_node_by_phandle(hremote_phandle);
+			if (!hremote_node) {
+				ath11k_dbg(ab, ATH11K_DBG_QMI,
+					   "qmi fail to get hremote_node\n");
+				return 0;
+			}
+
+			aw = of_n_addr_cells(hremote_node);
+			sw = of_n_size_cells(hremote_node);
+
+			reg = (unsigned int *)of_get_property(hremote_node, "reg", &len);

No need to cast. And I guess you are again removing the const?


- Anil.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux