On Wed 25 Apr 07:50 PDT 2018, Sibi Sankar wrote: > Introduce interrupt handler for smp2p ready interrupt and > handle start completion. Remove the proxy votes for clocks > and regulators in the handover interrupt context. Disable > wdog and fatal interrupts on remoteproc device stop and > re-enable them on remoteproc device start. Can't the enable/disable dance be split out into a separate commit? Making the introduction of them cleaner in the git history? > > Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx> > --- > drivers/remoteproc/qcom_q6v5_pil.c | 71 +++++++++++++++++++++++++----- > 1 file changed, 60 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > index 296eb3f8b551..7e2d04d4f2f0 100644 > --- a/drivers/remoteproc/qcom_q6v5_pil.c > +++ b/drivers/remoteproc/qcom_q6v5_pil.c > @@ -143,6 +143,10 @@ struct q6v5 { > struct qcom_smem_state *state; > unsigned stop_bit; > > + unsigned int handover_interrupt; > + unsigned int wdog_interrupt; > + unsigned int fatal_interrupt; Make these "int", and write "irq" instead of "interrupt". > + > struct clk *active_clks[8]; > struct clk *proxy_clks[4]; > int active_clk_count; > @@ -170,6 +174,7 @@ struct q6v5 { > struct qcom_rproc_ssr ssr_subdev; > struct qcom_sysmon *sysmon; > bool need_mem_protection; > + bool unvoted_flag; > int mpss_perm; > int mba_perm; > int version; > @@ -727,6 +732,7 @@ static int q6v5_start(struct rproc *rproc) > int xfermemop_ret; > int ret; > > + qproc->unvoted_flag = false; > ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, > qproc->proxy_reg_count); > if (ret) { > @@ -793,9 +799,16 @@ static int q6v5_start(struct rproc *rproc) > if (ret) > goto reclaim_mpss; > > + enable_irq(qproc->handover_interrupt); > + enable_irq(qproc->wdog_interrupt); > + enable_irq(qproc->fatal_interrupt); > + > ret = wait_for_completion_timeout(&qproc->start_done, > msecs_to_jiffies(5000)); > if (ret == 0) { > + disable_irq(qproc->handover_interrupt); > + disable_irq(qproc->wdog_interrupt); > + disable_irq(qproc->fatal_interrupt); > dev_err(qproc->dev, "start timed out\n"); > ret = -ETIMEDOUT; > goto reclaim_mpss; > @@ -809,11 +822,6 @@ static int q6v5_start(struct rproc *rproc) > "Failed to reclaim mba buffer system may become unstable\n"); > qproc->running = true; > > - q6v5_clk_disable(qproc->dev, qproc->proxy_clks, > - qproc->proxy_clk_count); > - q6v5_regulator_disable(qproc, qproc->proxy_regs, > - qproc->proxy_reg_count); > - > return 0; > > reclaim_mpss: > @@ -892,6 +900,16 @@ static int q6v5_stop(struct rproc *rproc) > WARN_ON(ret); > > reset_control_assert(qproc->mss_restart); > + disable_irq(qproc->handover_interrupt); > + if (!qproc->unvoted_flag) { > + q6v5_clk_disable(qproc->dev, qproc->proxy_clks, > + qproc->proxy_clk_count); > + q6v5_regulator_disable(qproc, qproc->proxy_regs, > + qproc->proxy_reg_count); > + } Perhaps break this out into a separate function and call it from the two places? > + disable_irq(qproc->wdog_interrupt); > + disable_irq(qproc->fatal_interrupt); Any particular reason why you didn't group the disable_irq() calls together? Would look prettier than spreading them on each side of the resource disable. > + > q6v5_clk_disable(qproc->dev, qproc->active_clks, > qproc->active_clk_count); > q6v5_regulator_disable(qproc, qproc->active_regs, [..] > @@ -1184,19 +1221,31 @@ static int q6v5_probe(struct platform_device *pdev) > > qproc->version = desc->version; > qproc->need_mem_protection = desc->need_mem_protection; > - ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); > + ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt, > + &qproc->wdog_interrupt); I think it's time to inline this function instead. You can omit the first error handling and rely on request_irq to fail if you pass it an invalid irq number. > + if (ret < 0) > + goto free_rproc; > + disable_irq(qproc->wdog_interrupt); I presume this is to balance the IRQ enable/disable later? > + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html