Messages in this thread | | | Subject | Re: [PATCH v4 5/5] remoteproc: qcom: Always assert and deassert reset signals in SDM845 | From | Sibi S <> | Date | Mon, 21 May 2018 22:27:47 +0530 |
| |
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 >
-- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
| |