[PATCH v2 1/2] dt: bindings: add new DT entry for ath11k PCI device support

[PATCH v2 1/2] dt: bindings: add new DT entry for ath11k PCI device support

[Anilkumar Kolli]

Ath11k driver supports PCI devices such as QCN9074/QCA6390.
Ath11k firmware uses host DDR memory. DT entry is used to reserve
these host DDR memory regions, send these memory base
addresses using DT entries.

Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
---
V2:
  - Use reserved-memory (Rob)

 .../bindings/net/wireless/qcom,ath11k.yaml         | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
index 85c2f699d602..5a8994f6cb10 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
@@ -150,6 +150,12 @@ properties:
       string to uniquely identify variant of the calibration data in the
       board-2.bin for designs with colliding bus and device specific ids
 
+  memory-region:
+    maxItems: 1
+    description:
+      phandle to a node describing reserved memory (System RAM memory)
+      used by ath11k firmware (see bindings/reserved-memory/reserved-memory.txt)
+
 required:
   - compatible
   - reg
@@ -279,3 +285,45 @@ examples:
                           "tcl2host-status-ring";
         qcom,rproc = <&q6v5_wcss>;
     };
+
+    memory {
+        device_type = "memory";
+        reg = <0x0 0x40000000 0x0 0x20000000>;
+    };
+
+    reserved-memory {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+
+        qcn9074_pcie0: qcn9074_pcie0@51100000 {
+            no-map;
+            reg = <0x0 0x51100000 0x0 0x03500000>;
+        };
+
+        qcn9074_pcie1: qcn9074_pcie1@54600000 {
+            no-map;
+            reg = <0x0 0x54600000 0x0 0x03500000>;
+        };
+    };
+
+    pcie0_rp: pcie0_rp {
+        reg = <0 0 0 0 0>;
+
+        status = "ok";
+        ath11k_0: ath11k@0 {
+            reg = <0 0 0 0 0 >;
+            memory-region = <&qcn9074_pcie0>;
+        };
+    };
+
+    pcie1_rp: pcie1_rp {
+        reg = <0 0 0 0 0>;
+
+        status = "ok";
+        ath11k_1: ath11k@1 {
+            reg = <0 0 0 0 0 >;
+            memory-region = <&qcn9074_pcie1>;
+        };
+    };
+
-- 
2.7.4

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

[Anilkumar Kolli]

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@codeaurora.org>
---
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;
 
 	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;
+		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);
+	if (!reg) {
+		no_dt_entry = 1;
+		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);
+		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);
+	}
+
 	mhi_ctrl->sbl_size = SZ_512K;
 	mhi_ctrl->seg_len = SZ_512K;
 	mhi_ctrl->fbc_download = true;
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 3d353e7c9d5c..21d0a511d9c7 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/of.h>
 
 #include "pci.h"
 #include "core.h"
@@ -1225,7 +1226,7 @@ static int ath11k_pci_probe(struct pci_dev *pdev,
 {
 	struct ath11k_base *ab;
 	struct ath11k_pci *ab_pci;
-	u32 soc_hw_version_major, soc_hw_version_minor;
+	u32 soc_hw_version_major, soc_hw_version_minor, addr;
 	int ret;
 
 	ab = ath11k_core_alloc(&pdev->dev, sizeof(*ab_pci), ATH11K_BUS_PCI,
@@ -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;
+
 	ret = ath11k_pci_claim(ab_pci, pdev);
 	if (ret) {
 		ath11k_err(ab, "failed to claim device: %d\n", ret);
diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index fa73118de6db..da58ce36b477 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -1815,10 +1815,11 @@ static void ath11k_qmi_free_target_mem_chunk(struct ath11k_base *ab)
 {
 	int i;
 
-	if (ab->bus_params.fixed_mem_region)
-		return;
-
 	for (i = 0; i < ab->qmi.mem_seg_count; i++) {
+		if (ab->bus_params.fixed_mem_region &&
+		    ab->qmi.target_mem[i].iaddr)
+			iounmap(ab->qmi.target_mem[i].iaddr);
+
 		if (!ab->qmi.target_mem[i].vaddr)
 			continue;
 
@@ -1866,10 +1867,62 @@ static int ath11k_qmi_alloc_target_mem_chunk(struct ath11k_base *ab)
 
 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);
+			if (!reg) {
+				ath11k_dbg(ab, ATH11K_DBG_QMI,
+					   "qmi fail to get reg from hremote\n");
+				return 0;
+			}
+
+			reg_end = reg + len / (aw * sw);
+
+			do {
+				start = of_read_number(reg, aw);
+				reg += aw;
+				size = of_read_number(reg, sw);
+				reg += sw;
+			} while (reg < reg_end);
+
+			ath11k_dbg(ab, ATH11K_DBG_QMI,
+				   "qmi ddr start addr is 0x%llx len is %lld\n",
+				   start, size);
+
+			ab->qmi.target_mem[idx].paddr = start;
+			ab->qmi.target_mem[idx].iaddr =
+				ioremap(ab->qmi.target_mem[idx].paddr,
+					ab->qmi.target_mem[i].size);
+			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
+			host_ddr_sz = 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].vaddr = NULL;
@@ -1884,10 +1937,16 @@ static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
 			}
 
 			if (ath11k_cold_boot_cal && ab->hw_params.cold_boot_calib) {
-				ab->qmi.target_mem[idx].paddr =
-						     ATH11K_QMI_CALDB_ADDRESS;
-				ab->qmi.target_mem[idx].vaddr =
-						     (void *)ATH11K_QMI_CALDB_ADDRESS;
+				if (hremote_node) {
+					ab->qmi.target_mem[idx].paddr =
+							start + host_ddr_sz;
+					ab->qmi.target_mem[idx].iaddr =
+						ioremap(ab->qmi.target_mem[idx].paddr,
+							ab->qmi.target_mem[i].size);
+				} else {
+					ab->qmi.target_mem[idx].paddr =
+						ATH11K_QMI_CALDB_ADDRESS;
+				}
 			} else {
 				ab->qmi.target_mem[idx].paddr = 0;
 				ab->qmi.target_mem[idx].vaddr = NULL;
diff --git a/drivers/net/wireless/ath/ath11k/qmi.h b/drivers/net/wireless/ath/ath11k/qmi.h
index 3bb0f9ef7996..f8d45b7dc821 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.h
+++ b/drivers/net/wireless/ath/ath11k/qmi.h
@@ -95,6 +95,7 @@ struct target_mem_chunk {
 	u32 type;
 	dma_addr_t paddr;
 	u32 *vaddr;
+	void __iomem *iaddr;
 };
 
 struct target_info {
-- 
2.7.4

Re: [PATCH v2 1/2] dt: bindings: add new DT entry for ath11k PCI device support

[Rob Herring]

On Tue, 16 Nov 2021 22:30:57 +0530, Anilkumar Kolli wrote:
> Ath11k driver supports PCI devices such as QCN9074/QCA6390.
> Ath11k firmware uses host DDR memory. DT entry is used to reserve
> these host DDR memory regions, send these memory base
> addresses using DT entries.
> 
> Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
> ---
> V2:
>   - Use reserved-memory (Rob)
> 
>  .../bindings/net/wireless/qcom,ath11k.yaml         | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:160.13-31: Warning (reg_format): /example-0/pcie0_rp:reg: property has invalid length (20 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:164.17-36: Warning (reg_format): /example-0/pcie0_rp/ath11k@0:reg: property has invalid length (20 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:170.13-31: Warning (reg_format): /example-0/pcie1_rp:reg: property has invalid length (20 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:174.17-36: Warning (reg_format): /example-0/pcie1_rp/ath11k@1:reg: property has invalid length (20 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:138.16-141.11: Warning (unit_address_vs_reg): /example-0/memory: node has a reg or ranges property, but no unit name
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:159.28-167.11: Warning (unit_address_vs_reg): /example-0/pcie0_rp: node has a reg or ranges property, but no unit name
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:169.28-177.11: Warning (unit_address_vs_reg): /example-0/pcie1_rp: node has a reg or ranges property, but no unit name
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:163.32-166.15: Warning (avoid_default_addr_size): /example-0/pcie0_rp/ath11k@0: Relying on default #address-cells value
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:163.32-166.15: Warning (avoid_default_addr_size): /example-0/pcie0_rp/ath11k@0: Relying on default #size-cells value
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:173.32-176.15: Warning (avoid_default_addr_size): /example-0/pcie1_rp/ath11k@1: Relying on default #address-cells value
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dts:173.32-176.15: Warning (avoid_default_addr_size): /example-0/pcie1_rp/ath11k@1: Relying on default #size-cells value
Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: Warning (unique_unit_address): Failed prerequisite 'avoid_default_addr_size'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: reserved-memory: qcn9074_pcie0@51100000:reg:0: [0, 1360003072, 0, 55574528] is too long
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.example.dt.yaml: reserved-memory: qcn9074_pcie1@54600000:reg:0: [0, 1415577600, 0, 55574528] is too long
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1555958

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Kalle Valo]

Anilkumar Kolli <akolli@codeaurora.org> 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@codeaurora.org>
> ---
> 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.

>  	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.

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

> +	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 *'?

> +		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?

> @@ -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.

>  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?

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

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

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

[Anilkumar Kolli]

On 2021-11-17 13:35, Kalle Valo wrote:
> Anilkumar Kolli <akolli@codeaurora.org> 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@codeaurora.org>
>> ---
>> 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.

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

[Kalle Valo]

Anilkumar Kolli <akolli@codeaurora.org> writes:

> On 2021-11-17 13:35, Kalle Valo wrote:
>> Anilkumar Kolli <akolli@codeaurora.org> 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@codeaurora.org>

[...]

>>> +		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.

My point here was that doesn't this mixing of __be32 and u32 when cause
a sparse warning?

>
>>> +		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 ?

I suspect that a cast is not needed, but not sure. No time to check either.

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

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

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

[Sven Eckelmann]

On Tuesday, 16 November 2021 18:00:58 CET Anilkumar Kolli wrote:
[...]
> @@ -1866,10 +1867,62 @@ static int ath11k_qmi_alloc_target_mem_chunk(struct ath11k_base *ab)
>  
>  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;
> +			}
> +

Why aren't you using something like of_address_to_resource? Or some other 
__of_get_address related function.

> +			aw = of_n_addr_cells(hremote_node);
> +			sw = of_n_size_cells(hremote_node);
> +
> +			reg = (unsigned int *)of_get_property(hremote_node, "reg", &len);
> +			if (!reg) {
> +				ath11k_dbg(ab, ATH11K_DBG_QMI,
> +					   "qmi fail to get reg from hremote\n");
> +				return 0;
> +			}
> +
> +			reg_end = reg + len / (aw * sw);

Why are you multiplying aw with sw? And why are you then dividing len by it?

1. if you calculate (u32 *)x + 1 then then you would increase the address by 4 bytes.
2. for each address + size pair you would have (aw + sw) * 4 bytes
3. len is in bytes

So if you want to get the first byte after a full u32 reg buffer then it would be:

    reg + len / sizeof(u32);

If you would want to get the amount of full address+size pairs

    len / sizeof(u32) / (aw + sw)


> +
> +			do {
> +				start = of_read_number(reg, aw);
> +				reg += aw;
> +				size = of_read_number(reg, sw);
> +				reg += sw;
> +			} while (reg < reg_end);

What are you trying to achieve with this loop?

Kind regards,
	Sven