diff mbox

[v2] remoteproc: Introduce prepare/unprepare ops for rproc coredump

Message ID 20180521184559.20864-1-sibis@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sibi Sankar May 21, 2018, 6:45 p.m. UTC
In some occasions the remoteproc device might need to
prepare some hardware before the coredump can be performed
and cleanup the state afterwards.

Q6V5 modem requires the mba to be loaded before the
coredump and some cleanup of the resources afterwards.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

This patch depends on the following patches:
https://patchwork.kernel.org/patch/10416047/
https://patchwork.kernel.org/patch/10416031/

 drivers/remoteproc/qcom_q6v5_pil.c       | 62 ++++++++++++++++++++----
 drivers/remoteproc/remoteproc_core.c     |  5 ++
 drivers/remoteproc/remoteproc_internal.h | 16 ++++++
 include/linux/remoteproc.h               |  4 ++
 4 files changed, 77 insertions(+), 10 deletions(-)

Comments

kernel test robot May 22, 2018, 10:39 a.m. UTC | #1
Hi Sibi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.17-rc6]
[also build test ERROR on next-20180517]
[cannot apply to remoteproc/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sibi-Sankar/remoteproc-Introduce-prepare-unprepare-ops-for-rproc-coredump/20180522-133348
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/remoteproc/qcom_q6v5_pil.c: In function 'q6v5_start':
>> drivers/remoteproc/qcom_q6v5_pil.c:796:3: error: implicit declaration of function 'q6v5_enable_irqs'; did you mean 'enable_irq'? [-Werror=implicit-function-declaration]
      q6v5_enable_irqs(qproc);
      ^~~~~~~~~~~~~~~~
      enable_irq
   cc1: some warnings being treated as errors

vim +796 drivers/remoteproc/qcom_q6v5_pil.c

   725	
   726	static int q6v5_start(struct rproc *rproc)
   727	{
   728		struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
   729		int xfermemop_ret;
   730		int ret;
   731	
   732		ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
   733					    qproc->proxy_reg_count);
   734		if (ret) {
   735			dev_err(qproc->dev, "failed to enable proxy supplies\n");
   736			goto clear_coredump_pending;
   737		}
   738	
   739		ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
   740				      qproc->proxy_clk_count);
   741		if (ret) {
   742			dev_err(qproc->dev, "failed to enable proxy clocks\n");
   743			goto disable_proxy_reg;
   744		}
   745	
   746		ret = q6v5_regulator_enable(qproc, qproc->active_regs,
   747					    qproc->active_reg_count);
   748		if (ret) {
   749			dev_err(qproc->dev, "failed to enable supplies\n");
   750			goto disable_proxy_clk;
   751		}
   752		ret = reset_control_deassert(qproc->mss_restart);
   753		if (ret) {
   754			dev_err(qproc->dev, "failed to deassert mss restart\n");
   755			goto disable_vdd;
   756		}
   757	
   758		ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
   759				      qproc->active_clk_count);
   760		if (ret) {
   761			dev_err(qproc->dev, "failed to enable clocks\n");
   762			goto assert_reset;
   763		}
   764	
   765		/* Assign MBA image access in DDR to q6 */
   766		xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true,
   767							qproc->mba_phys,
   768							qproc->mba_size);
   769		if (xfermemop_ret) {
   770			dev_err(qproc->dev,
   771				"assigning Q6 access to mba memory failed: %d\n",
   772				xfermemop_ret);
   773			goto disable_active_clks;
   774		}
   775	
   776		writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
   777	
   778		ret = q6v5proc_reset(qproc);
   779		if (ret)
   780			goto reclaim_mba;
   781	
   782		ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
   783		if (ret == -ETIMEDOUT) {
   784			dev_err(qproc->dev, "MBA boot timed out\n");
   785			goto halt_axi_ports;
   786		} else if (ret != RMB_MBA_XPU_UNLOCKED &&
   787			   ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
   788			dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
   789			ret = -EINVAL;
   790			goto halt_axi_ports;
   791		}
   792	
   793		if (qproc->coredump_pending) {
   794			dev_info(qproc->dev, "MBA booted, skipping mpss for coredump\n");
   795			qproc->coredump_pending = false;
 > 796			q6v5_enable_irqs(qproc);
   797			xfermemop_ret = q6v5_xfer_mem_ownership(qproc,
   798								&qproc->mba_perm, false,
   799								qproc->mba_phys,
   800								qproc->mba_size);
   801			if (xfermemop_ret)
   802				dev_err(qproc->dev, "Failed to reclaim mba buffer\n");
   803			return 0;
   804		}
   805	
   806		dev_info(qproc->dev, "MBA booted, loading mpss\n");
   807	
   808		ret = q6v5_mpss_load(qproc);
   809		if (ret)
   810			goto reclaim_mpss;
   811	
   812		ret = wait_for_completion_timeout(&qproc->start_done,
   813						  msecs_to_jiffies(5000));
   814		if (ret == 0) {
   815			dev_err(qproc->dev, "start timed out\n");
   816			ret = -ETIMEDOUT;
   817			goto reclaim_mpss;
   818		}
   819	
   820		xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
   821							qproc->mba_phys,
   822							qproc->mba_size);
   823		if (xfermemop_ret)
   824			dev_err(qproc->dev,
   825				"Failed to reclaim mba buffer system may become unstable\n");
   826		qproc->running = true;
   827	
   828		q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
   829				 qproc->proxy_clk_count);
   830		q6v5_regulator_disable(qproc, qproc->proxy_regs,
   831				       qproc->proxy_reg_count);
   832	
   833		return 0;
   834	
   835	reclaim_mpss:
   836		xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
   837							false, qproc->mpss_phys,
   838							qproc->mpss_size);
   839		WARN_ON(xfermemop_ret);
   840	
   841	halt_axi_ports:
   842		q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
   843		q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
   844		q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
   845	
   846	reclaim_mba:
   847		xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
   848							qproc->mba_phys,
   849							qproc->mba_size);
   850		if (xfermemop_ret) {
   851			dev_err(qproc->dev,
   852				"Failed to reclaim mba buffer, system may become unstable\n");
   853		}
   854	
   855	disable_active_clks:
   856		q6v5_clk_disable(qproc->dev, qproc->active_clks,
   857				 qproc->active_clk_count);
   858	
   859	assert_reset:
   860		reset_control_assert(qproc->mss_restart);
   861	disable_vdd:
   862		q6v5_regulator_disable(qproc, qproc->active_regs,
   863				       qproc->active_reg_count);
   864	disable_proxy_clk:
   865		q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
   866				 qproc->proxy_clk_count);
   867	disable_proxy_reg:
   868		q6v5_regulator_disable(qproc, qproc->proxy_regs,
   869				       qproc->proxy_reg_count);
   870	clear_coredump_pending:
   871		qproc->coredump_pending = false;
   872	
   873		return ret;
   874	}
   875	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Bjorn Andersson May 29, 2018, 5:05 a.m. UTC | #2
On Mon 21 May 11:45 PDT 2018, Sibi Sankar wrote:

> In some occasions the remoteproc device might need to
> prepare some hardware before the coredump can be performed
> and cleanup the state afterwards.
> 
> Q6V5 modem requires the mba to be loaded before the
> coredump and some cleanup of the resources afterwards.
> 

This describes two different changes, so please put it in two+ patches.

[..]
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index dfdaede9139e..010819e01279 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -333,6 +333,8 @@ struct firmware;
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>   * @da_to_va:	optional platform hook to perform address translations
>   * @load_rsc_table:	load resource table from firmware image
> + * @prepare_coredump:	prepare function, called before coredump
> + * @unprepare_coredump:	unprepare function, called post coredump

I believe there will be other cases where we will need driver-specific
logic to extract the memory content of the segments, e.g. through custom
hardware sequences or non-mmio reads.

To support this I think we should extend the struct rproc_dump_segment
to carry an optional "dump" function that if specified will be used
instead of the memcpy in rproc_coredump(). Drivers can then for each
segment specify this function, if needed.

Through some restructuring in the msa driver and your patch you should
be able to implement this using such a mechanism instead - and it would
be useful to these other cases as well.


PS. I hope we can get away from some of the conditionals in your patch
through some restructuring of the code.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sibi Sankar May 29, 2018, 2:06 p.m. UTC | #3
Hi Bjorn,
Thanks for the review.


On 05/29/2018 10:35 AM, Bjorn Andersson wrote:
> On Mon 21 May 11:45 PDT 2018, Sibi Sankar wrote:
> 
>> In some occasions the remoteproc device might need to
>> prepare some hardware before the coredump can be performed
>> and cleanup the state afterwards.
>>
>> Q6V5 modem requires the mba to be loaded before the
>> coredump and some cleanup of the resources afterwards.
>>
> 
> This describes two different changes, so please put it in two+ patches.
>


The second change just describes an example of a remoteproc device
that requires remoteproc coredump prepare and unprepare but sure
with restructuring required in msa it definitely will require 2+
patches.


> [..]
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index dfdaede9139e..010819e01279 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -333,6 +333,8 @@ struct firmware;
>>    * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>    * @da_to_va:	optional platform hook to perform address translations
>>    * @load_rsc_table:	load resource table from firmware image
>> + * @prepare_coredump:	prepare function, called before coredump
>> + * @unprepare_coredump:	unprepare function, called post coredump
> 
> I believe there will be other cases where we will need driver-specific
> logic to extract the memory content of the segments, e.g. through custom
> hardware sequences or non-mmio reads.
> 
> To support this I think we should extend the struct rproc_dump_segment
> to carry an optional "dump" function that if specified will be used
> instead of the memcpy in rproc_coredump(). Drivers can then for each
> segment specify this function, if needed.
> 

In q6 modem mba needs to unlocked just once for all the segments to
be dumped but as you say other remote processors may need it for each
segment. This logic should be internal to the dump function anyway. So
will use the dump function approach. What about the cleanup path, can we
still reserve it till the coredump is done?



> Through some restructuring in the msa driver and your patch you should
> be able to implement this using such a mechanism instead - and it would
> be useful to these other cases as well.
> 
> 
> PS. I hope we can get away from some of the conditionals in your patch
> through some restructuring of the code.
> 


Thought you might preferred the conditionals with the least amount of
code addition/change but having prepare and unprepare coredump again
calling q6v5_start/stop respectively seemed a little hacky anyway.
Sure will restructure msa as needed :)


> Regards,
> Bjorn
>
diff mbox

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 7656731a6c51..4a33e50b6aeb 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -173,6 +173,7 @@  struct q6v5 {
 	struct completion start_done;
 	struct completion stop_done;
 	bool running;
+	bool coredump_pending;
 
 	phys_addr_t mba_phys;
 	void *mba_region;
@@ -742,6 +743,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];
@@ -816,7 +818,7 @@  static int q6v5_start(struct rproc *rproc)
 				    qproc->proxy_reg_count);
 	if (ret) {
 		dev_err(qproc->dev, "failed to enable proxy supplies\n");
-		return ret;
+		goto clear_coredump_pending;
 	}
 
 	ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
@@ -880,6 +882,19 @@  static int q6v5_start(struct rproc *rproc)
 		goto halt_axi_ports;
 	}
 
+	if (qproc->coredump_pending) {
+		dev_info(qproc->dev, "MBA booted, skipping mpss for coredump\n");
+		qproc->coredump_pending = false;
+		q6v5_enable_irqs(qproc);
+		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\n");
+		return 0;
+	}
+
 	dev_info(qproc->dev, "MBA booted, loading mpss\n");
 
 	ret = q6v5_mpss_load(qproc);
@@ -945,6 +960,8 @@  static int q6v5_start(struct rproc *rproc)
 disable_proxy_reg:
 	q6v5_regulator_disable(qproc, qproc->proxy_regs,
 			       qproc->proxy_reg_count);
+clear_coredump_pending:
+	qproc->coredump_pending = false;
 
 	return ret;
 }
@@ -955,17 +972,19 @@  static int q6v5_stop(struct rproc *rproc)
 	int ret;
 	u32 val;
 
-	qproc->running = false;
-
-	qcom_smem_state_update_bits(qproc->state,
-				    BIT(qproc->stop_bit), BIT(qproc->stop_bit));
+	if (qproc->running) {
+		qproc->running = false;
+		qcom_smem_state_update_bits(qproc->state,
+				BIT(qproc->stop_bit), BIT(qproc->stop_bit));
 
-	ret = wait_for_completion_timeout(&qproc->stop_done,
-					  msecs_to_jiffies(5000));
-	if (ret == 0)
-		dev_err(qproc->dev, "timed out on wait\n");
+		ret = wait_for_completion_timeout(&qproc->stop_done,
+				msecs_to_jiffies(5000));
+		if (ret == 0)
+			dev_err(qproc->dev, "timed out on wait\n");
 
-	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0);
+		qcom_smem_state_update_bits(qproc->state,
+				BIT(qproc->stop_bit), 0);
+	}
 
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
@@ -1017,10 +1036,31 @@  static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 	return qproc->mpss_region + offset;
 }
 
+static int qcom_mpss_register_dump_segments(struct rproc *rproc,
+				const struct firmware *fw_unused)
+{
+	const struct firmware *fw;
+	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	int ret;
+
+	ret = request_firmware(&fw, "modem.mdt", qproc->dev);
+	if (ret < 0) {
+		dev_err(qproc->dev, "unable to load modem.mdt\n");
+		return ret;
+	}
+	ret = qcom_register_dump_segments(rproc, fw);
+
+	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_mpss_register_dump_segments,
+	.prepare_coredump = q6v5_start,
+	.unprepare_coredump = q6v5_stop,
 	.load = q6v5_load,
 };
 
@@ -1036,6 +1076,7 @@  static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
 		return IRQ_HANDLED;
 	}
 
+	qproc->coredump_pending = true;
 	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, MPSS_CRASH_REASON_SMEM, &len);
 	if (!IS_ERR(msg) && len > 0 && msg[0])
 		dev_err(qproc->dev, "watchdog received: %s\n", msg);
@@ -1053,6 +1094,7 @@  static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
 	size_t len;
 	char *msg;
 
+	qproc->coredump_pending = true;
 	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, MPSS_CRASH_REASON_SMEM, &len);
 	if (!IS_ERR(msg) && len > 0 && msg[0])
 		dev_err(qproc->dev, "fatal error received: %s\n", msg);
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a9609d971f7f..8c254d9b5c67 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1083,6 +1083,9 @@  static void rproc_coredump(struct rproc *rproc)
 	if (list_empty(&rproc->dump_segments))
 		return;
 
+	if (rproc_prepare_coredump(rproc))
+		return;
+
 	data_size = sizeof(*ehdr);
 	list_for_each_entry(segment, &rproc->dump_segments, node) {
 		data_size += sizeof(*phdr) + segment->size;
@@ -1139,6 +1142,8 @@  static void rproc_coredump(struct rproc *rproc)
 	}
 
 	dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
+
+	rproc_unprepare_coredump(rproc);
 }
 
 /**
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 7570beb035b5..22a1b276e110 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -96,6 +96,22 @@  static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+static inline int rproc_prepare_coredump(struct rproc *rproc)
+{
+	if (rproc->ops->prepare_coredump)
+		return rproc->ops->prepare_coredump(rproc);
+
+	return 0;
+}
+
+static inline int rproc_unprepare_coredump(struct rproc *rproc)
+{
+	if (rproc->ops->unprepare_coredump)
+		return rproc->ops->unprepare_coredump(rproc);
+
+	return 0;
+}
+
 static inline
 struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
 						   const struct firmware *fw)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index dfdaede9139e..010819e01279 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -333,6 +333,8 @@  struct firmware;
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
  * @da_to_va:	optional platform hook to perform address translations
  * @load_rsc_table:	load resource table from firmware image
+ * @prepare_coredump:	prepare function, called before coredump
+ * @unprepare_coredump:	unprepare function, called post coredump
  * @find_loaded_rsc_table: find the loaded resouce table
  * @load:		load firmeware to memory, where the remote processor
  *			expects to find it
@@ -345,6 +347,8 @@  struct rproc_ops {
 	void (*kick)(struct rproc *rproc, int vqid);
 	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
 	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
+	int (*prepare_coredump)(struct rproc *rproc);
+	int (*unprepare_coredump)(struct rproc *rproc);
 	struct resource_table *(*find_loaded_rsc_table)(
 				struct rproc *rproc, const struct firmware *fw);
 	int (*load)(struct rproc *rproc, const struct firmware *fw);