[PATCH v3 0/6] Add coredump support for Q6v5 Modem remoteproc

[PATCH v3 0/6] Add coredump support for Q6v5 Modem remoteproc

[Sibi Sankar]

This patch series add coredump support for modem on SDM845, MSM8996
and MSM8916 SoCs. Modem requires the mba to be loaded before a
coredump can be performed and this is achieved using a custom per
segment dump function.

V3:
  [bjorn]:replace prepare/unprepare ops with a more generalised per segment
  dump function

V2:
  Introduce prepare/unprepare ops for rproc coredump

Sibi Sankar (6):
  remoteproc: Introduce custom dump function for each remoteproc segment
  remoteproc: Add mechanism for custom dump function assignment
  remoteproc: qcom: q6v5-pil: Refactor mba load/unload sequence
  remoteproc: qcom: q6v5-pil: Add custom dump function for modem
  remoteproc: qcom: q6v5-pil: Register segments/dumpfn for coredump
  remoteproc: qcom: q6v5-pil: Assign the relocated address

 drivers/remoteproc/qcom_q6v5_pil.c   | 307 ++++++++++++++++++---------
 drivers/remoteproc/remoteproc_core.c |  52 ++++-
 include/linux/remoteproc.h           |   8 +
 3 files changed, 264 insertions(+), 103 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 1/6] remoteproc: Introduce custom dump function for each remoteproc segment

[Sibi Sankar]

Introduce custom dump function per remoteproc segment. It is responsible
for filling the device memory segment associated with coredump

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
 include/linux/remoteproc.h           |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 283b258f5e0f..ec56cd822b26 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc)
 		phdr->p_align = 0;
 
 		ptr = rproc_da_to_va(rproc, segment->da, segment->size);
-		if (!ptr) {
-			dev_err(&rproc->dev,
+
+		if (segment->dump) {
+			segment->dump(rproc, ptr, segment->size, data + offset);
+		} else {
+			if (!ptr) {
+				dev_err(&rproc->dev,
 				"invalid coredump segment (%pad, %zu)\n",
 				&segment->da, segment->size);
-			memset(data + offset, 0xff, segment->size);
-		} else {
-			memcpy(data + offset, ptr, segment->size);
+				memset(data + offset, 0xff, segment->size);
+			} else {
+				memcpy(data + offset, ptr, segment->size);
+			}
 		}
 
 		offset += phdr->p_filesz;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e3c5d856b6da..0fbb01a9955c 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -399,6 +399,8 @@ enum rproc_crash_type {
  * @node:	list node related to the rproc segment list
  * @da:		device address of the segment
  * @size:	size of the segment
+ * @dump:	custom dump function to fill device memory segment associated
+ *		with coredump
  */
 struct rproc_dump_segment {
 	struct list_head node;
@@ -406,6 +408,7 @@ struct rproc_dump_segment {
 	dma_addr_t da;
 	size_t size;
 
+	void (*dump)(struct rproc *rproc, void *ptr, size_t len, void *priv);
 	loff_t offset;
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/6] remoteproc: Add mechanism for custom dump function assignment

[Sibi Sankar]

This patch adds a mechanism for assigning each rproc segment with
a custom dump function. It is to be called for each rproc segment
during coredump if assigned.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/remoteproc_core.c | 37 ++++++++++++++++++++++++++++
 include/linux/remoteproc.h           |  5 ++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ec56cd822b26..3d70b7dd1f18 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1120,6 +1120,43 @@ int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size)
 }
 EXPORT_SYMBOL(rproc_coredump_add_segment);
 
+/**
+ * rproc_coredump_add_custom_segment() - add segment of device memory to
+ *					 coredump and extend it with custom
+ *					 dump function
+ * @rproc:	handle of a remote processor
+ * @da:		device address
+ * @size:	size of segment
+ * @dumpfn:	custom dump function called for each segment during coredump
+ *
+ * Add device memory to the list of segments to be included in a coredump for
+ * the remoteproc and associate the segment with the given custom dump
+ * function.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int rproc_coredump_add_custom_segment(struct rproc *rproc,
+				      dma_addr_t da, size_t size,
+				      void (*dumpfn)(struct rproc *rproc,
+						     void *ptr, size_t len,
+						     void *priv))
+{
+	struct rproc_dump_segment *segment;
+
+	segment = kzalloc(sizeof(*segment), GFP_KERNEL);
+	if (!segment)
+		return -ENOMEM;
+
+	segment->da = da;
+	segment->size = size;
+	segment->dump = dumpfn;
+
+	list_add_tail(&segment->node, &rproc->dump_segments);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_coredump_add_custom_segment);
+
 /**
  * rproc_coredump() - perform coredump
  * @rproc:	rproc handle
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 0fbb01a9955c..5a1ad008ec28 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -560,6 +560,11 @@ int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
+int rproc_coredump_add_custom_segment(struct rproc *rproc,
+				      dma_addr_t da, size_t size,
+				      void (*dumpfn)(struct rproc *rproc,
+						     void *ptr, size_t len,
+						     void *priv));
 
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/6] remoteproc: qcom: q6v5-pil: Refactor mba load/unload sequence

[Sibi Sankar]

Refactor re-useable parts of mba load/unload sequence into mba_load and
mba_reclaim respectively and introduce mba_load flag. This is done in
order to prevent code duplication for modem coredump which requires the
mba to be loaded.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 243 +++++++++++++++++------------
 1 file changed, 144 insertions(+), 99 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index d7a4b9eca5d2..eacf9f0bf49e 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -165,6 +165,7 @@ struct q6v5 {
 	int proxy_reg_count;
 
 	bool running;
+	bool mba_loaded;
 
 	phys_addr_t mba_phys;
 	void *mba_region;
@@ -669,6 +670,145 @@ static bool q6v5_phdr_valid(const struct elf32_phdr *phdr)
 	return true;
 }
 
+static int q6v5_mba_load(struct q6v5 *qproc)
+{
+	int ret;
+	int xfermemop_ret;
+
+	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
+				    qproc->proxy_reg_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable proxy supplies\n");
+		return ret;
+	}
+
+	ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
+			      qproc->proxy_clk_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable proxy clocks\n");
+		goto disable_proxy_reg;
+	}
+
+	ret = q6v5_regulator_enable(qproc, qproc->active_regs,
+				    qproc->active_reg_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable supplies\n");
+		goto disable_proxy_clk;
+	}
+
+	ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks,
+			      qproc->reset_clk_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable reset clocks\n");
+		goto disable_vdd;
+	}
+
+	ret = q6v5_reset_deassert(qproc);
+	if (ret) {
+		dev_err(qproc->dev, "failed to deassert mss restart\n");
+		goto disable_reset_clks;
+	}
+
+	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
+			      qproc->active_clk_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable clocks\n");
+		goto assert_reset;
+	}
+
+	/* Assign MBA image access in DDR to q6 */
+	ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true,
+				      qproc->mba_phys, qproc->mba_size);
+	if (ret) {
+		dev_err(qproc->dev,
+			"assigning Q6 access to mba memory failed: %d\n", ret);
+		goto disable_active_clks;
+	}
+
+	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
+
+	ret = q6v5proc_reset(qproc);
+	if (ret)
+		goto reclaim_mba;
+
+	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
+	if (ret == -ETIMEDOUT) {
+		dev_err(qproc->dev, "MBA boot timed out\n");
+		goto halt_axi_ports;
+	} else if (ret != RMB_MBA_XPU_UNLOCKED &&
+		   ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
+		dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
+		ret = -EINVAL;
+		goto halt_axi_ports;
+	}
+
+	qproc->mba_loaded = true;
+	return 0;
+
+halt_axi_ports:
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
+
+reclaim_mba:
+	xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
+						qproc->mba_phys,
+						qproc->mba_size);
+	if (xfermemop_ret) {
+		dev_err(qproc->dev,
+			"Failed to reclaim mba buffer, system may become unstable\n");
+	}
+
+disable_active_clks:
+	q6v5_clk_disable(qproc->dev, qproc->active_clks,
+			 qproc->active_clk_count);
+assert_reset:
+	q6v5_reset_assert(qproc);
+disable_reset_clks:
+	q6v5_clk_disable(qproc->dev, qproc->reset_clks,
+			 qproc->reset_clk_count);
+disable_vdd:
+	q6v5_regulator_disable(qproc, qproc->active_regs,
+			       qproc->active_reg_count);
+disable_proxy_clk:
+	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
+			 qproc->proxy_clk_count);
+disable_proxy_reg:
+	q6v5_regulator_disable(qproc, qproc->proxy_regs,
+			       qproc->proxy_reg_count);
+
+	return ret;
+}
+
+static void q6v5_mba_reclaim(struct q6v5 *qproc)
+{
+	int xfermemop_ret;
+
+	qproc->mba_loaded = false;
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
+	xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
+						qproc->mba_phys,
+						qproc->mba_size);
+	if (xfermemop_ret) {
+		dev_err(qproc->dev,
+			"Failed to reclaim mba buffer, system may become unstable\n");
+	}
+
+	q6v5_clk_disable(qproc->dev, qproc->active_clks,
+			 qproc->active_clk_count);
+	q6v5_reset_assert(qproc);
+	q6v5_clk_disable(qproc->dev, qproc->reset_clks,
+			 qproc->reset_clk_count);
+	q6v5_regulator_disable(qproc, qproc->active_regs,
+			       qproc->active_reg_count);
+	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
+			 qproc->proxy_clk_count);
+	q6v5_regulator_disable(qproc, qproc->proxy_regs,
+			       qproc->proxy_reg_count);
+}
+
 static int q6v5_mpss_load(struct q6v5 *qproc)
 {
 	const struct elf32_phdr *phdrs;
@@ -792,72 +932,9 @@ static int q6v5_start(struct rproc *rproc)
 
 	qcom_q6v5_prepare(&qproc->q6v5);
 
-	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
-				    qproc->proxy_reg_count);
-	if (ret) {
-		dev_err(qproc->dev, "failed to enable proxy supplies\n");
-		goto disable_irqs;
-	}
-
-	ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
-			      qproc->proxy_clk_count);
-	if (ret) {
-		dev_err(qproc->dev, "failed to enable proxy clocks\n");
-		goto disable_proxy_reg;
-	}
-
-	ret = q6v5_regulator_enable(qproc, qproc->active_regs,
-				    qproc->active_reg_count);
-	if (ret) {
-		dev_err(qproc->dev, "failed to enable supplies\n");
-		goto disable_proxy_clk;
-	}
-
-	ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks,
-			      qproc->reset_clk_count);
-	if (ret) {
-		dev_err(qproc->dev, "failed to enable reset clocks\n");
-		goto disable_vdd;
-	}
-
-	ret = q6v5_reset_deassert(qproc);
-	if (ret) {
-		dev_err(qproc->dev, "failed to deassert mss restart\n");
-		goto disable_reset_clks;
-	}
-
-	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
-			      qproc->active_clk_count);
-	if (ret) {
-		dev_err(qproc->dev, "failed to enable clocks\n");
-		goto assert_reset;
-	}
-
-	/* Assign MBA image access in DDR to q6 */
-	ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true,
-				      qproc->mba_phys, qproc->mba_size);
-	if (ret) {
-		dev_err(qproc->dev,
-			"assigning Q6 access to mba memory failed: %d\n", ret);
-		goto disable_active_clks;
-	}
-
-	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
-
-	ret = q6v5proc_reset(qproc);
+	ret = q6v5_mba_load(qproc);
 	if (ret)
-		goto reclaim_mba;
-
-	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
-	if (ret == -ETIMEDOUT) {
-		dev_err(qproc->dev, "MBA boot timed out\n");
-		goto halt_axi_ports;
-	} else if (ret != RMB_MBA_XPU_UNLOCKED &&
-		   ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
-		dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
-		ret = -EINVAL;
-		goto halt_axi_ports;
-	}
+		goto disable_irqs;
 
 	dev_info(qproc->dev, "MBA booted, loading mpss\n");
 
@@ -886,40 +963,7 @@ static int q6v5_start(struct rproc *rproc)
 						false, qproc->mpss_phys,
 						qproc->mpss_size);
 	WARN_ON(xfermemop_ret);
-
-halt_axi_ports:
-	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
-	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
-	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
-
-reclaim_mba:
-	xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
-						qproc->mba_phys,
-						qproc->mba_size);
-	if (xfermemop_ret) {
-		dev_err(qproc->dev,
-			"Failed to reclaim mba buffer, system may become unstable\n");
-	}
-
-disable_active_clks:
-	q6v5_clk_disable(qproc->dev, qproc->active_clks,
-			 qproc->active_clk_count);
-
-assert_reset:
-	q6v5_reset_assert(qproc);
-disable_reset_clks:
-	q6v5_clk_disable(qproc->dev, qproc->reset_clks,
-			 qproc->reset_clk_count);
-disable_vdd:
-	q6v5_regulator_disable(qproc, qproc->active_regs,
-			       qproc->active_reg_count);
-disable_proxy_clk:
-	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
-			 qproc->proxy_clk_count);
-disable_proxy_reg:
-	q6v5_regulator_disable(qproc, qproc->proxy_regs,
-			       qproc->proxy_reg_count);
-
+	q6v5_mba_reclaim(qproc);
 disable_irqs:
 	qcom_q6v5_unprepare(&qproc->q6v5);
 
@@ -933,6 +977,7 @@ static int q6v5_stop(struct rproc *rproc)
 	u32 val;
 
 	qproc->running = false;
+	qproc->mba_loaded = false;
 
 	ret = qcom_q6v5_request_stop(&qproc->q6v5);
 	if (ret == -ETIMEDOUT)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem

[Sibi Sankar]

The per segment dump function is responsible for loading the mba
before device memory segments associated with coredump can be populated
and for cleaning up the resources post coredump.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index eacf9f0bf49e..ac3342f9ea5a 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -182,6 +182,7 @@ struct q6v5 {
 	struct qcom_sysmon *sysmon;
 	bool need_mem_protection;
 	bool has_alt_reset;
+	u32 valid_mask;
 	int mpss_perm;
 	int mba_perm;
 	int version;
@@ -924,6 +925,30 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	return ret < 0 ? ret : 0;
 }
 
+static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr, size_t len,
+								   void *priv)
+{
+	int ret = 0;
+	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	static u32 pending_mask;
+
+	/* Unlock mba before copying segments */
+	if (!qproc->mba_loaded)
+		ret = q6v5_mba_load(qproc);
+
+	if (!ptr || ret)
+		memset(priv, 0xff, len);
+	else
+		memcpy(priv, ptr, len);
+
+	pending_mask++;
+	if (pending_mask == qproc->valid_mask) {
+		if (qproc->mba_loaded)
+			q6v5_mba_reclaim(qproc);
+		pending_mask = 0;
+	}
+}
+
 static int q6v5_start(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 5/6] remoteproc: qcom: q6v5-pil: Register segments/dumpfn for coredump

[Sibi Sankar]

Register the MDT segments and custom dumpfn with the remoteproc core
dump functionality.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 40 ++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index ac3342f9ea5a..22bb049c3e7f 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -1058,10 +1058,50 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 	return qproc->mpss_region + offset;
 }
 
+static int qcom_q6v5_register_dump_segments(struct rproc *rproc,
+				const struct firmware *fw_unused)
+{
+	const struct firmware *fw;
+	const struct elf32_phdr *phdrs;
+	const struct elf32_phdr *phdr;
+	const struct elf32_hdr *ehdr;
+	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	int ret;
+	int i;
+
+	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	if (ret < 0) {
+		dev_err(qproc->dev, "unable to load modem.mdt\n");
+		return ret;
+	}
+
+	qproc->valid_mask = 0;
+	ehdr = (struct elf32_hdr *)fw->data;
+	phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		phdr = &phdrs[i];
+
+		if (!q6v5_phdr_valid(phdr))
+			continue;
+
+		ret = rproc_coredump_add_custom_segment(rproc, phdr->p_paddr,
+					phdr->p_memsz, qcom_q6v5_dump_segment);
+		if (ret)
+			break;
+
+		qproc->valid_mask++;
+	}
+
+	release_firmware(fw);
+	return ret;
+}
+
 static const struct rproc_ops q6v5_ops = {
 	.start = q6v5_start,
 	.stop = q6v5_stop,
 	.da_to_va = q6v5_da_to_va,
+	.parse_fw = qcom_q6v5_register_dump_segments,
 	.load = q6v5_load,
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 6/6] remoteproc: qcom: q6v5-pil: Assign the relocated address

[Sibi Sankar]

Assign the relocated base of the modem image, as the offsets
from the virtual memory might not be based on the physical
address.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 22bb049c3e7f..b1296d614b8b 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -862,6 +862,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	}
 
 	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
+	qproc->mpss_reloc = mpss_reloc;
 	/* Load firmware segments */
 	for (i = 0; i < ehdr->e_phnum; i++) {
 		phdr = &phdrs[i];
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/6] remoteproc: Introduce custom dump function for each remoteproc segment

[Vinod]

Hi Sibi,

On 27-07-18, 20:49, Sibi Sankar wrote:
> Introduce custom dump function per remoteproc segment. It is responsible
> for filling the device memory segment associated with coredump
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
>  include/linux/remoteproc.h           |  3 +++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 283b258f5e0f..ec56cd822b26 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc)
>  		phdr->p_align = 0;
>  
>  		ptr = rproc_da_to_va(rproc, segment->da, segment->size);
> -		if (!ptr) {
> -			dev_err(&rproc->dev,
> +
> +		if (segment->dump) {
> +			segment->dump(rproc, ptr, segment->size, data + offset);

Am not sure I follow, you are calling this w/o checking if ptr is valid,
so you maybe passing null to segment->dump() ?

> +		} else {
> +			if (!ptr) {
> +				dev_err(&rproc->dev,
>  				"invalid coredump segment (%pad, %zu)\n",
>  				&segment->da, segment->size);
> -			memset(data + offset, 0xff, segment->size);
> -		} else {
> -			memcpy(data + offset, ptr, segment->size);
> +				memset(data + offset, 0xff, segment->size);
> +			} else {
> +				memcpy(data + offset, ptr, segment->size);
> +			}

-- 
~Vinod

Re: [PATCH v3 1/6] remoteproc: Introduce custom dump function for each remoteproc segment

[Sibi Sankar]

Hi Vinod,
Thanks for the review,

On 08/07/2018 11:45 AM, Vinod wrote:
> Hi Sibi,
> 
> On 27-07-18, 20:49, Sibi Sankar wrote:
>> Introduce custom dump function per remoteproc segment. It is responsible
>> for filling the device memory segment associated with coredump
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
>>   include/linux/remoteproc.h           |  3 +++
>>   2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 283b258f5e0f..ec56cd822b26 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc)
>>   		phdr->p_align = 0;
>>   
>>   		ptr = rproc_da_to_va(rproc, segment->da, segment->size);
>> -		if (!ptr) {
>> -			dev_err(&rproc->dev,
>> +
>> +		if (segment->dump) {
>> +			segment->dump(rproc, ptr, segment->size, data + offset);
> 
> Am not sure I follow, you are calling this w/o checking if ptr is valid,
> so you maybe passing null to segment->dump() ?
> 

the rationale behind passing ptr directly to dump_fn is that it will
help in tracking the segments being core dumped (q6v5_pil in particular
requires to unlock mba before dumping and cleanup after all the segments
are dumped which is currently decided based on a mask that is
maintained). It also allows the remoteproc driver to fill the memory as
needed (instead of the default 0xff). This is applicable to drivers that
implement dump_fn, for others the default behavior is maintained.

>> +		} else {
>> +			if (!ptr) {
>> +				dev_err(&rproc->dev,
>>   				"invalid coredump segment (%pad, %zu)\n",
>>   				&segment->da, segment->size);
>> -			memset(data + offset, 0xff, segment->size);
>> -		} else {
>> -			memcpy(data + offset, ptr, segment->size);
>> +				memset(data + offset, 0xff, segment->size);
>> +			} else {
>> +				memcpy(data + offset, ptr, segment->size);
>> +			}
> 

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/6] remoteproc: Introduce custom dump function for each remoteproc segment

[Bjorn Andersson]

On Fri 27 Jul 08:19 PDT 2018, Sibi Sankar wrote:

> Introduce custom dump function per remoteproc segment. It is responsible
> for filling the device memory segment associated with coredump
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
>  include/linux/remoteproc.h           |  3 +++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 283b258f5e0f..ec56cd822b26 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc)
>  		phdr->p_align = 0;
>  
>  		ptr = rproc_da_to_va(rproc, segment->da, segment->size);
> -		if (!ptr) {
> -			dev_err(&rproc->dev,
> +
> +		if (segment->dump) {
> +			segment->dump(rproc, ptr, segment->size, data + offset);

rproc_da_to_va() is an exported symbol, so if you pass segment to the
dump function the driver can, if it needs to, call the function itself.

A typical use case, that I see, is to use the custom dump function to
write out CPU or hardware state to the dump file, in which case the "da"
won't be valid.


So please make this call dump(rproc, segment, data + offset) and move
the rproc_da_to_va() into the else block.

> +		} else {
> +			if (!ptr) {
> +				dev_err(&rproc->dev,
>  				"invalid coredump segment (%pad, %zu)\n",
>  				&segment->da, segment->size);
> -			memset(data + offset, 0xff, segment->size);
> -		} else {
> -			memcpy(data + offset, ptr, segment->size);
> +				memset(data + offset, 0xff, segment->size);
> +			} else {
> +				memcpy(data + offset, ptr, segment->size);
> +			}
>  		}
>  
>  		offset += phdr->p_filesz;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e3c5d856b6da..0fbb01a9955c 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -399,6 +399,8 @@ enum rproc_crash_type {
>   * @node:	list node related to the rproc segment list
>   * @da:		device address of the segment
>   * @size:	size of the segment
> + * @dump:	custom dump function to fill device memory segment associated
> + *		with coredump
>   */
>  struct rproc_dump_segment {
>  	struct list_head node;
> @@ -406,6 +408,7 @@ struct rproc_dump_segment {
>  	dma_addr_t da;
>  	size_t size;
>  
> +	void (*dump)(struct rproc *rproc, void *ptr, size_t len, void *priv);

"priv" isn't the best name to represent the memory to which you expect
dump to write to. Please call it "dest".

>  	loff_t offset;
>  };

Regards,
Bjorn

Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem

[Bjorn Andersson]

On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
[..]
> +static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr, size_t len,
> +								   void *priv)
> +{
> +	int ret = 0;
> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> +	static u32 pending_mask;

I dislike that this is a static variable. And it tracks the segments
that has already been dumped, i.e. the !pending.

> +
> +	/* Unlock mba before copying segments */
> +	if (!qproc->mba_loaded)
> +		ret = q6v5_mba_load(qproc);
> +
> +	if (!ptr || ret)
> +		memset(priv, 0xff, len);
> +	else
> +		memcpy(priv, ptr, len);
> +
> +	pending_mask++;

This is a "count" and not a "mask".

I can see a few different cases where one would like to be able to pass
custom data/information from the segment-registration to the dump
function. So how about adding a "void *priv" to the dump segment.

For this particular case we could typecast segment->priv to an unsigned
long (as this is always the same size) and use that as a bitmask, which
we use to update pending_mask.

> +	if (pending_mask == qproc->valid_mask) {
> +		if (qproc->mba_loaded)
> +			q6v5_mba_reclaim(qproc);
> +		pending_mask = 0;
> +	}

I think it would be cleaner to reset pending_mask in the start function,
and then return early in this function when we have dumped all the
segments.

If so can pending_mask == 0 and pending_mask == all be the triggers for
loading and reclaiming the mba? So we don't have two different trackers
for this?

> +}
> +

Regards,
Bjorn

Re: [PATCH v3 5/6] remoteproc: qcom: q6v5-pil: Register segments/dumpfn for coredump

[Bjorn Andersson]

On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:

> Register the MDT segments and custom dumpfn with the remoteproc core
> dump functionality.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 40 ++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index ac3342f9ea5a..22bb049c3e7f 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1058,10 +1058,50 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
>  	return qproc->mpss_region + offset;
>  }
>  
> +static int qcom_q6v5_register_dump_segments(struct rproc *rproc,
> +				const struct firmware *fw_unused)

How about naming it mba_fw instead of unused? Just as unused, but easier
to understand why it isn't used.

> +{
> +	const struct firmware *fw;
> +	const struct elf32_phdr *phdrs;
> +	const struct elf32_phdr *phdr;
> +	const struct elf32_hdr *ehdr;
> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;

No need for an explicit typecast from void *.

The rest looks good!

Regards,
Bjorn

Re: [PATCH v3 6/6] remoteproc: qcom: q6v5-pil: Assign the relocated address

[Bjorn Andersson]

On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:

> Assign the relocated base of the modem image, as the offsets
> from the virtual memory might not be based on the physical
> address.
> 

In order to get this merged before the first call to rproc_da_to_va() I
applied this patch to rproc-next.

Thanks,
Bjorn

> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 22bb049c3e7f..b1296d614b8b 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -862,6 +862,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	}
>  
>  	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> +	qproc->mpss_reloc = mpss_reloc;
>  	/* Load firmware segments */
>  	for (i = 0; i < ehdr->e_phnum; i++) {
>  		phdr = &phdrs[i];
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v3 1/6] remoteproc: Introduce custom dump function for each remoteproc segment

[Sibi Sankar]

Hi Bjorn,
Thanks for the review :)

On 2018-10-08 11:53, Bjorn Andersson wrote:
> On Fri 27 Jul 08:19 PDT 2018, Sibi Sankar wrote:
> 
>> Introduce custom dump function per remoteproc segment. It is 
>> responsible
>> for filling the device memory segment associated with coredump
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
>>  include/linux/remoteproc.h           |  3 +++
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>> b/drivers/remoteproc/remoteproc_core.c
>> index 283b258f5e0f..ec56cd822b26 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc 
>> *rproc)
>>  		phdr->p_align = 0;
>> 
>>  		ptr = rproc_da_to_va(rproc, segment->da, segment->size);
>> -		if (!ptr) {
>> -			dev_err(&rproc->dev,
>> +
>> +		if (segment->dump) {
>> +			segment->dump(rproc, ptr, segment->size, data + offset);
> 
> rproc_da_to_va() is an exported symbol, so if you pass segment to the
> dump function the driver can, if it needs to, call the function itself.
> 
> A typical use case, that I see, is to use the custom dump function to
> write out CPU or hardware state to the dump file, in which case the 
> "da"
> won't be valid.
> 
> 
> So please make this call dump(rproc, segment, data + offset) and move
> the rproc_da_to_va() into the else block.
> 

yup will redefine it.

>> +		} else {
>> +			if (!ptr) {
>> +				dev_err(&rproc->dev,
>>  				"invalid coredump segment (%pad, %zu)\n",
>>  				&segment->da, segment->size);
>> -			memset(data + offset, 0xff, segment->size);
>> -		} else {
>> -			memcpy(data + offset, ptr, segment->size);
>> +				memset(data + offset, 0xff, segment->size);
>> +			} else {
>> +				memcpy(data + offset, ptr, segment->size);
>> +			}
>>  		}
>> 
>>  		offset += phdr->p_filesz;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index e3c5d856b6da..0fbb01a9955c 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -399,6 +399,8 @@ enum rproc_crash_type {
>>   * @node:	list node related to the rproc segment list
>>   * @da:		device address of the segment
>>   * @size:	size of the segment
>> + * @dump:	custom dump function to fill device memory segment 
>> associated
>> + *		with coredump
>>   */
>>  struct rproc_dump_segment {
>>  	struct list_head node;
>> @@ -406,6 +408,7 @@ struct rproc_dump_segment {
>>  	dma_addr_t da;
>>  	size_t size;
>> 
>> +	void (*dump)(struct rproc *rproc, void *ptr, size_t len, void 
>> *priv);
> 
> "priv" isn't the best name to represent the memory to which you expect
> dump to write to. Please call it "dest".
> 

will rename it

>>  	loff_t offset;
>>  };
> 
> Regards,
> Bjorn

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem

[Sibi Sankar]

Hi Bjorn,
Thanks for the review !

On 2018-10-08 12:15, Bjorn Andersson wrote:
> On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
>> b/drivers/remoteproc/qcom_q6v5_pil.c
> [..]
>> +static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr, 
>> size_t len,
>> +								   void *priv)
>> +{
>> +	int ret = 0;
>> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> +	static u32 pending_mask;
> 
> I dislike that this is a static variable. And it tracks the segments
> that has already been dumped, i.e. the !pending.
> 
>> +
>> +	/* Unlock mba before copying segments */
>> +	if (!qproc->mba_loaded)
>> +		ret = q6v5_mba_load(qproc);
>> +
>> +	if (!ptr || ret)
>> +		memset(priv, 0xff, len);
>> +	else
>> +		memcpy(priv, ptr, len);
>> +
>> +	pending_mask++;
> 
> This is a "count" and not a "mask".
> 

will rename it to count in the next re-spin. We can implement this as
a mask as well, the only disadvantage I see in that case is the need
for another flag to determine if mba is loaded since the mask for the
first segment may not be zero (the first segment may not be valid).

> I can see a few different cases where one would like to be able to pass
> custom data/information from the segment-registration to the dump
> function. So how about adding a "void *priv" to the dump segment.
> 
> For this particular case we could typecast segment->priv to an unsigned
> long (as this is always the same size) and use that as a bitmask, which
> we use to update pending_mask.
> 

sure will do the same

>> +	if (pending_mask == qproc->valid_mask) {
>> +		if (qproc->mba_loaded)
>> +			q6v5_mba_reclaim(qproc);
>> +		pending_mask = 0;
>> +	}
> 
> I think it would be cleaner to reset pending_mask in the start 
> function,
> and then return early in this function when we have dumped all the
> segments.
> 
> If so can pending_mask == 0 and pending_mask == all be the triggers for
> loading and reclaiming the mba? So we don't have two different trackers
> for this?
> 

with the private data stored per dump segment this becomes much simpler 
:)

>> +}
>> +
> 
> Regards,
> Bjorn

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v3 5/6] remoteproc: qcom: q6v5-pil: Register segments/dumpfn for coredump

[Sibi Sankar]

Hi Bjorn,
Thanks for the review !

On 2018-10-08 12:18, Bjorn Andersson wrote:
> On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:
> 
>> Register the MDT segments and custom dumpfn with the remoteproc core
>> dump functionality.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_q6v5_pil.c | 40 
>> ++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
>> b/drivers/remoteproc/qcom_q6v5_pil.c
>> index ac3342f9ea5a..22bb049c3e7f 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -1058,10 +1058,50 @@ static void *q6v5_da_to_va(struct rproc 
>> *rproc, u64 da, int len)
>>  	return qproc->mpss_region + offset;
>>  }
>> 
>> +static int qcom_q6v5_register_dump_segments(struct rproc *rproc,
>> +				const struct firmware *fw_unused)
> 
> How about naming it mba_fw instead of unused? Just as unused, but 
> easier
> to understand why it isn't used.
> 

sure

>> +{
>> +	const struct firmware *fw;
>> +	const struct elf32_phdr *phdrs;
>> +	const struct elf32_phdr *phdr;
>> +	const struct elf32_hdr *ehdr;
>> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> 
> No need for an explicit typecast from void *.
> 

will remove it

> The rest looks good!
> 
> Regards,
> Bjorn

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.