Skip to contentSkip to conversation

Add coredump support for Q6v5 Modem remoteproc

PATCH Superseded Sibi Sankar 3 participants 16 messages Jul 27, 2018 – Oct 9, 2018

Subsystems (2)
Arm Qualcomm mailing list Remote processor (remoteproc)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Sibi Sankar

author
#0 lore raw

v3 cover Add coredump support for Q6v5 Modem remoteproc

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(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

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

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

Signed-off-by Sibi Sankar <[email protected]>
---
 drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
 include/linux/remoteproc.h           |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)
drivers/remoteproc/remoteproc_core.c +10 -5
index 283b258f5e0f..ec56cd822b26 100644
@@ -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;
include/linux/remoteproc.h +3 -0
index e3c5d856b6da..0fbb01a9955c 100644
@@ -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;
 };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#2 lore raw

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

Hi Sibi,

Quoted 14 lines · 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 <[email protected]>
---
 drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
 include/linux/remoteproc.h           |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)
drivers/remoteproc/remoteproc_core.c +0 -0
index 283b258f5e0f..ec56cd822b26 100644

drivers/remoteproc/remoteproc_core.c static void rproc_coredump(struct rproc *rproc) · line 1188

Quoted 8 lines
drivers/remoteproc/remoteproc_core.c +3 -2
@@ -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() ?

Quoted 12 lines
drivers/remoteproc/remoteproc_core.c +7 -3
+		} 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);
+			}
Signature · 1 line
~Vinod
author
#3 lore raw

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

Hi Vinod,
Thanks for the review,

Quoted 19 lines · Vinod

Hi Sibi,

Quoted 16 lines · 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 <[email protected]>
---
  drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
  include/linux/remoteproc.h           |  3 +++
  2 files changed, 13 insertions(+), 5 deletions(-)
drivers/remoteproc/remoteproc_core.c +0 -0
index 283b258f5e0f..ec56cd822b26 100644
@@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc)
  		phdr->p_align = 0;
Quoted 10 lines

drivers/remoteproc/remoteproc_core.c static void rproc_coredump(struct rproc *rproc) · line 1188

Quoted 6 lines
drivers/remoteproc/remoteproc_core.c +3 -2
  		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.

Quoted 13 lines
Quoted 12 lines
drivers/remoteproc/remoteproc_core.c +7 -3
+		} 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);
+			}
Signature · 3 lines
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#4 lore raw

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

In reply to Sibi Sankar
Quoted 14 lines · 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 <[email protected]>
---
 drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
 include/linux/remoteproc.h           |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)
drivers/remoteproc/remoteproc_core.c +0 -0
index 283b258f5e0f..ec56cd822b26 100644

drivers/remoteproc/remoteproc_core.c static void rproc_coredump(struct rproc *rproc) · line 1188

Quoted 8 lines
drivers/remoteproc/remoteproc_core.c +3 -2
@@ -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.

Quoted 29 lines
drivers/remoteproc/remoteproc_core.c +7 -3
+		} 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;
include/linux/remoteproc.h +2 -0
index e3c5d856b6da..0fbb01a9955c 100644
@@ -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;

include/linux/remoteproc.h struct rproc_dump_segment { · line 411

Quoted 4 lines
include/linux/remoteproc.h +1 -0
@@ -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".

include/linux/remoteproc.h struct rproc_dump_segment { · line 413

Quoted 2 lines
include/linux/remoteproc.h +0 -0
 	loff_t offset;
 };

Regards,
Bjorn

author
#5 lore raw

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

Hi Bjorn,
Thanks for the review :)

Quoted 29 lines · Bjorn Andersson
Quoted 14 lines · 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 <[email protected]>
---
 drivers/remoteproc/remoteproc_core.c | 15 ++++++++++-----
 include/linux/remoteproc.h           |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)
drivers/remoteproc/remoteproc_core.c +0 -0
index 283b258f5e0f..ec56cd822b26 100644

drivers/remoteproc/remoteproc_core.c static void rproc_coredump(struct rproc · line 1189

Quoted 9 lines
drivers/remoteproc/remoteproc_core.c +3 -2
@@ -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.

Quoted 8 lines

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.

Quoted 33 lines
Quoted 33 lines
drivers/remoteproc/remoteproc_core.c +7 -3
+		} 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);
+			}
 		}
drivers/remoteproc/remoteproc_core.c +0 -0
 		offset += phdr->p_filesz;
include/linux/remoteproc.h +1 -0
index e3c5d856b6da..0fbb01a9955c 100644
@@ -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 {

include/linux/remoteproc.h +0 -0
 	struct list_head node;

@@ -406,6 +408,7 @@ struct rproc_dump_segment {
 	dma_addr_t da;
 	size_t size;
Quoted 6 lines

include/linux/remoteproc.h struct rproc_dump_segment { · line 412

Quoted 2 lines
include/linux/remoteproc.h +1 -0
+	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

Quoted 5 lines

include/linux/remoteproc.h struct rproc_dump_segment { · line 413

Quoted 2 lines
include/linux/remoteproc.h +0 -0
 	loff_t offset;
 };

Regards,
Bjorn

Signature · 3 lines
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/remoteproc/remoteproc_core.c | 37 ++++++++++++++++++++++++++++
 include/linux/remoteproc.h           |  5 ++++
 2 files changed, 42 insertions(+)
drivers/remoteproc/remoteproc_core.c +37 -0
index ec56cd822b26..3d70b7dd1f18 100644
@@ -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
include/linux/remoteproc.h +5 -0
index 0fbb01a9955c..5a1ad008ec28 100644
@@ -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)
 {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#7 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 243 +++++++++++++++++------------
 1 file changed, 144 insertions(+), 99 deletions(-)
drivers/remoteproc/qcom_q6v5_pil.c +144 -99
index d7a4b9eca5d2..eacf9f0bf49e 100644
@@ -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)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#8 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/remoteproc/qcom_q6v5_pil.c +25 -0
index eacf9f0bf49e..ac3342f9ea5a 100644
@@ -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;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#9 lore raw

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

Quoted 1 line · Sibi Sankar
drivers/remoteproc/qcom_q6v5_pil.c +0 -0

[..]

drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 933

Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6 -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;

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

drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 944

Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c +11 -0
+
+	/* 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.

drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 949

Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5 -0
+	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?

drivers/remoteproc/qcom_q6v5_pil.c

Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +2 -0
+}
+

Regards,
Bjorn

author
#10 lore raw

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

Hi Bjorn,
Thanks for the review !

Quoted 14 lines · Bjorn Andersson
Quoted 1 line · Sibi Sankar
drivers/remoteproc/qcom_q6v5_pil.c +0 -0

[..]

drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 933

Quoted 7 lines

+static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr, size_t len,

drivers/remoteproc/qcom_q6v5_pil.c +5 -0
+								   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.

Quoted 14 lines

drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 944

Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c +11 -0
+
+	/* 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).

Quoted 8 lines

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

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 949

Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5 -0
+	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.

Quoted 4 lines

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 :)

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_pil.c

Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +2 -0
+}
+

Regards,
Bjorn

Signature · 3 lines
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
author
#11 lore raw

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

In reply to Sibi Sankar

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

Signed-off-by Sibi Sankar <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 40 ++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/remoteproc/qcom_q6v5_pil.c +40 -0
index ac3342f9ea5a..22bb049c3e7f 100644
@@ -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,
 };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#12 lore raw

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

Quoted 13 lines · Sibi Sankar

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

Signed-off-by Sibi Sankar <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 40 ++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/remoteproc/qcom_q6v5_pil.c +0 -0
index ac3342f9ea5a..22bb049c3e7f 100644

drivers/remoteproc/qcom_q6v5_pil.c static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len) · line 1062

Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +2 -0
@@ -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.

drivers/remoteproc/qcom_q6v5_pil.c static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len) · line 1068

Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6 -0
+{
+	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

author
#13 lore raw

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

Hi Bjorn,
Thanks for the review !

Quoted 20 lines · Bjorn Andersson
Quoted 18 lines · Sibi Sankar

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

Signed-off-by Sibi Sankar <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 40 
++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/remoteproc/qcom_q6v5_pil.c +0 -0
index ac3342f9ea5a..22bb049c3e7f 100644
@@ -1058,10 +1058,50 @@ static void *q6v5_da_to_va(struct rproc 
 *rproc, u64 da, int len)
 	return qproc->mpss_region + offset;
 }
Quoted 6 lines

drivers/remoteproc/qcom_q6v5_pil.c static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len) · line 1062

Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +2 -0
+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

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_pil.c static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len) · line 1068

Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6 -0
+{
+	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

Quoted 4 lines

The rest looks good!

Regards,
Bjorn

Signature · 3 lines
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
author
#14 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 1 +
 1 file changed, 1 insertion(+)
drivers/remoteproc/qcom_q6v5_pil.c +1 -0
index 22bb049c3e7f..b1296d614b8b 100644
@@ -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];
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#15 lore raw

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

Quoted 4 lines · 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.

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

Thanks,
Bjorn

Quoted 21 lines
Signed-off-by Sibi Sankar <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 1 +
 1 file changed, 1 insertion(+)
drivers/remoteproc/qcom_q6v5_pil.c +1 -0
index 22bb049c3e7f..b1296d614b8b 100644
@@ -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];
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project