| Message ID | 20180425150843.26657-6-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 25 Apr 08:08 PDT 2018, Sibi Sankar wrote: > SDM845 brings a new reset signal ALT_RESET which is a part of the MSS > subsystem hence requires some of the active clks to be enabled before > assert/deassert > > Reset the modem if the BOOT FSM does timeout > > Reset assert/deassert sequence vary across SoCs adding reset, adding > start/stop helper functions to handle SoC specific reset sequences > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/remoteproc/qcom_q6v5_pil.c | 81 ++++++++++++++++++++++++++++-- > 1 file changed, 76 insertions(+), 5 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c [..] > @@ -349,6 +356,32 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > return 0; > } > > +static int q6v5_reset_assert(struct q6v5 *qproc) > +{ > + return reset_control_assert(qproc->mss_restart); > +} > + > +static int q6v5_reset_deassert(struct q6v5 *qproc) > +{ > + return reset_control_deassert(qproc->mss_restart); > +} > + > +static int q6v5_alt_reset_assert(struct q6v5 *qproc) > +{ > + return reset_control_reset(qproc->mss_restart); > +} > + > +static int q6v5_alt_reset_deassert(struct q6v5 *qproc) > +{ > + /* Ensure alt reset is written before restart reg */ > + writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET); > + > + reset_control_reset(qproc->mss_restart); > + > + writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); > + return 0; > +} > + Rather than having these four functions and scattering jumps to some function pointer in the code I think it will be shorter and cleaner to just have the q6v5_reset_{asert,deassert}() functions and in there check if has_alt_reset and take appropriate action. > static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms) > { > unsigned long timeout; > @@ -424,6 +457,8 @@ static int q6v5proc_reset(struct q6v5 *qproc) > val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT); > if (ret) { > dev_err(qproc->dev, "Boot FSM failed to complete.\n"); > + /* Reset the modem so that boot FSM is in reset state */ > + qproc->reset_deassert(qproc); A thing like this typically should go into it's own patch, to keep a clear record of why it was changed, but as this is simply amending the previous patch it indicates that that one wasn't complete. So if you reorder the two patches you can just put this directly into the sdm845 patch, making it "complete". (This also means that I want to merge the handover vs ready interrupt patch before that one, so please include it in the next revision of the series). > return ret; > } > > @@ -792,12 +827,20 @@ static int q6v5_start(struct rproc *rproc) > dev_err(qproc->dev, "failed to enable supplies\n"); > goto disable_proxy_clk; > } > - ret = reset_control_deassert(qproc->mss_restart); > + > + ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks, > + qproc->reset_clk_count); Remind me, why can't you always enable the active clock before deasserting reset? That way we wouldn't have to split out the iface clock handling to be just slightly longer than the active clocks. > if (ret) { > - dev_err(qproc->dev, "failed to deassert mss restart\n"); > + dev_err(qproc->dev, "failed to enable reset clocks\n"); > goto disable_vdd; > } > > + ret = qproc->reset_deassert(qproc); > + if (ret) { > + dev_err(qproc->dev, "failed to deassert mss restart\n"); > + goto disable_reset_clks; > + } > + [..] > @@ -1335,8 +1399,11 @@ static const struct rproc_hexagon_res sdm845_mss = { > "prng", > NULL > }, > - .active_clk_names = (char*[]){ > + .reset_clk_names = (char*[]){ > "iface", > + NULL > + }, > + .active_clk_names = (char*[]){ Again, if you reorder your patches to first add the support for alt_reset and then introduce sdm845 you don't need to modify the previous patch directly to make it work. > "bus", > "mem", > "gpll0_mss", 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
Hi Bjorn, Thanks for the review. Will make all the required changes in v5. On 05/19/2018 03:17 AM, Bjorn Andersson wrote: > On Wed 25 Apr 08:08 PDT 2018, Sibi Sankar wrote: > >> SDM845 brings a new reset signal ALT_RESET which is a part of the MSS >> subsystem hence requires some of the active clks to be enabled before >> assert/deassert >> >> Reset the modem if the BOOT FSM does timeout >> >> Reset assert/deassert sequence vary across SoCs adding reset, adding >> start/stop helper functions to handle SoC specific reset sequences >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/qcom_q6v5_pil.c | 81 ++++++++++++++++++++++++++++-- >> 1 file changed, 76 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > [..] >> @@ -349,6 +356,32 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> return 0; >> } >> >> +static int q6v5_reset_assert(struct q6v5 *qproc) >> +{ >> + return reset_control_assert(qproc->mss_restart); >> +} >> + >> +static int q6v5_reset_deassert(struct q6v5 *qproc) >> +{ >> + return reset_control_deassert(qproc->mss_restart); >> +} >> + >> +static int q6v5_alt_reset_assert(struct q6v5 *qproc) >> +{ >> + return reset_control_reset(qproc->mss_restart); >> +} >> + >> +static int q6v5_alt_reset_deassert(struct q6v5 *qproc) >> +{ >> + /* Ensure alt reset is written before restart reg */ >> + writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET); >> + >> + reset_control_reset(qproc->mss_restart); >> + >> + writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); >> + return 0; >> +} >> + > > Rather than having these four functions and scattering jumps to some > function pointer in the code I think it will be shorter and cleaner to > just have the q6v5_reset_{asert,deassert}() functions and in there check > if has_alt_reset and take appropriate action. > yes this seems simpler >> static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms) >> { >> unsigned long timeout; >> @@ -424,6 +457,8 @@ static int q6v5proc_reset(struct q6v5 *qproc) >> val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT); >> if (ret) { >> dev_err(qproc->dev, "Boot FSM failed to complete.\n"); >> + /* Reset the modem so that boot FSM is in reset state */ >> + qproc->reset_deassert(qproc); > > > A thing like this typically should go into it's own patch, to keep a > clear record of why it was changed, but as this is simply amending the > previous patch it indicates that that one wasn't complete. > > So if you reorder the two patches you can just put this directly into > the sdm845 patch, making it "complete". > > (This also means that I want to merge the handover vs ready interrupt > patch before that one, so please include it in the next revision of the > series). > Will re-order them. >> return ret; >> } >> >> @@ -792,12 +827,20 @@ static int q6v5_start(struct rproc *rproc) >> dev_err(qproc->dev, "failed to enable supplies\n"); >> goto disable_proxy_clk; >> } >> - ret = reset_control_deassert(qproc->mss_restart); >> + >> + ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks, >> + qproc->reset_clk_count); > > Remind me, why can't you always enable the active clock before > deasserting reset? That way we wouldn't have to split out the iface > clock handling to be just slightly longer than the active clocks. > Have to introduce reset clks for backward compatibility, both msm8916 and msm8996 require the mss_reset to be deasserted before enabling the active clks. >> if (ret) { >> - dev_err(qproc->dev, "failed to deassert mss restart\n"); >> + dev_err(qproc->dev, "failed to enable reset clocks\n"); >> goto disable_vdd; >> } >> >> + ret = qproc->reset_deassert(qproc); >> + if (ret) { >> + dev_err(qproc->dev, "failed to deassert mss restart\n"); >> + goto disable_reset_clks; >> + } >> + > [..] >> @@ -1335,8 +1399,11 @@ static const struct rproc_hexagon_res sdm845_mss = { >> "prng", >> NULL >> }, >> - .active_clk_names = (char*[]){ >> + .reset_clk_names = (char*[]){ >> "iface", >> + NULL >> + }, >> + .active_clk_names = (char*[]){ > > Again, if you reorder your patches to first add the support for > alt_reset and then introduce sdm845 you don't need to modify the > previous patch directly to make it work. > yeah I'll re-order them >> "bus", >> "mem", >> "gpll0_mss", > > Regards, > Bjorn >
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index 4d9504e8bf8e..99ef3f51c528 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -130,9 +130,11 @@ struct rproc_hexagon_res { struct qcom_mss_reg_res *proxy_supply; struct qcom_mss_reg_res *active_supply; char **proxy_clk_names; + char **reset_clk_names; char **active_clk_names; int version; bool need_mem_protection; + bool has_alt_reset; }; struct q6v5 { @@ -157,8 +159,10 @@ struct q6v5 { unsigned int fatal_interrupt; struct clk *active_clks[8]; + struct clk *reset_clks[4]; struct clk *proxy_clks[4]; int active_clk_count; + int reset_clk_count; int proxy_clk_count; struct reg_info active_regs[1]; @@ -179,6 +183,9 @@ struct q6v5 { void *mpss_region; size_t mpss_size; + int (*reset_deassert)(struct q6v5 *qproc); + int (*reset_assert)(struct q6v5 *qproc); + struct qcom_rproc_glink glink_subdev; struct qcom_rproc_subdev smd_subdev; struct qcom_rproc_ssr ssr_subdev; @@ -349,6 +356,32 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) return 0; } +static int q6v5_reset_assert(struct q6v5 *qproc) +{ + return reset_control_assert(qproc->mss_restart); +} + +static int q6v5_reset_deassert(struct q6v5 *qproc) +{ + return reset_control_deassert(qproc->mss_restart); +} + +static int q6v5_alt_reset_assert(struct q6v5 *qproc) +{ + return reset_control_reset(qproc->mss_restart); +} + +static int q6v5_alt_reset_deassert(struct q6v5 *qproc) +{ + /* Ensure alt reset is written before restart reg */ + writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET); + + reset_control_reset(qproc->mss_restart); + + writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); + return 0; +} + static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms) { unsigned long timeout; @@ -424,6 +457,8 @@ static int q6v5proc_reset(struct q6v5 *qproc) val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT); if (ret) { dev_err(qproc->dev, "Boot FSM failed to complete.\n"); + /* Reset the modem so that boot FSM is in reset state */ + qproc->reset_deassert(qproc); return ret; } @@ -792,12 +827,20 @@ static int q6v5_start(struct rproc *rproc) dev_err(qproc->dev, "failed to enable supplies\n"); goto disable_proxy_clk; } - ret = reset_control_deassert(qproc->mss_restart); + + ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks, + qproc->reset_clk_count); if (ret) { - dev_err(qproc->dev, "failed to deassert mss restart\n"); + dev_err(qproc->dev, "failed to enable reset clocks\n"); goto disable_vdd; } + ret = qproc->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) { @@ -888,7 +931,10 @@ static int q6v5_start(struct rproc *rproc) qproc->active_clk_count); assert_reset: - reset_control_assert(qproc->mss_restart); + qproc->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); @@ -938,7 +984,7 @@ static int q6v5_stop(struct rproc *rproc) qproc->mpss_phys, qproc->mpss_size); WARN_ON(ret); - reset_control_assert(qproc->mss_restart); + qproc->reset_assert(qproc); disable_irq(qproc->handover_interrupt); if (!qproc->unvoted_flag) { q6v5_clk_disable(qproc->dev, qproc->proxy_clks, @@ -949,6 +995,8 @@ static int q6v5_stop(struct rproc *rproc) disable_irq(qproc->wdog_interrupt); disable_irq(qproc->fatal_interrupt); + q6v5_clk_disable(qproc->dev, qproc->reset_clks, + qproc->reset_clk_count); q6v5_clk_disable(qproc->dev, qproc->active_clks, qproc->active_clk_count); q6v5_regulator_disable(qproc, qproc->active_regs, @@ -1211,6 +1259,14 @@ static int q6v5_probe(struct platform_device *pdev) qproc->rproc = rproc; platform_set_drvdata(pdev, qproc); + if (desc->has_alt_reset) { + qproc->reset_deassert = q6v5_alt_reset_deassert; + qproc->reset_assert = q6v5_alt_reset_assert; + } else { + qproc->reset_deassert = q6v5_reset_deassert; + qproc->reset_assert = q6v5_reset_assert; + } + init_completion(&qproc->start_done); init_completion(&qproc->stop_done); @@ -1230,6 +1286,14 @@ static int q6v5_probe(struct platform_device *pdev) } qproc->proxy_clk_count = ret; + ret = q6v5_init_clocks(&pdev->dev, qproc->reset_clks, + desc->reset_clk_names); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to get reset clocks.\n"); + goto free_rproc; + } + qproc->reset_clk_count = ret; + ret = q6v5_init_clocks(&pdev->dev, qproc->active_clks, desc->active_clk_names); if (ret < 0) { @@ -1335,8 +1399,11 @@ static const struct rproc_hexagon_res sdm845_mss = { "prng", NULL }, - .active_clk_names = (char*[]){ + .reset_clk_names = (char*[]){ "iface", + NULL + }, + .active_clk_names = (char*[]){ "bus", "mem", "gpll0_mss", @@ -1345,6 +1412,7 @@ static const struct rproc_hexagon_res sdm845_mss = { NULL }, .need_mem_protection = true, + .has_alt_reset = true, .version = MSS_SDM845, }; @@ -1363,6 +1431,7 @@ static const struct rproc_hexagon_res msm8996_mss = { NULL }, .need_mem_protection = true, + .has_alt_reset = false, .version = MSS_MSM8996, }; @@ -1394,6 +1463,7 @@ static const struct rproc_hexagon_res msm8916_mss = { NULL }, .need_mem_protection = false, + .has_alt_reset = false, .version = MSS_MSM8916, }; @@ -1433,6 +1503,7 @@ static const struct rproc_hexagon_res msm8974_mss = { NULL }, .need_mem_protection = false, + .has_alt_reset = false, .version = MSS_MSM8974, };
SDM845 brings a new reset signal ALT_RESET which is a part of the MSS subsystem hence requires some of the active clks to be enabled before assert/deassert Reset the modem if the BOOT FSM does timeout Reset assert/deassert sequence vary across SoCs adding reset, adding start/stop helper functions to handle SoC specific reset sequences Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_pil.c | 81 ++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 5 deletions(-)