LKML Archive on lore.kernel.org
 help / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Sibi Sankar <sibis@codeaurora.org>
Cc: p.zabel@pengutronix.de, robh+dt@kernel.org,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, georgi.djakov@linaro.org,
	jassisinghbrar@gmail.com, ohad@wizery.com, mark.rutland@arm.com,
	kyan@codeaurora.org, sricharan@codeaurora.org,
	akdwived@codeaurora.org, linux-arm-msm@vger.kernel.org,
	tsoni@codeaurora.org
Subject: Re: [PATCH v4 5/5] remoteproc: qcom: Always assert and deassert reset signals in SDM845
Date: Fri, 18 May 2018 14:47:08 -0700
Message-ID: <20180518214708.GU14924@minitux> (raw)
In-Reply-To: <20180425150843.26657-6-sibis@codeaurora.org>

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

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25 15:08 [PATCH v4 0/5] Add support for remoteproc modem-pil on SDM845 SoCs Sibi Sankar
2018-04-25 15:08 ` [PATCH v4 1/5] dt-bindings: reset: Add AOSS reset bindings for " Sibi Sankar
2018-04-27 10:24   ` Philipp Zabel
2018-04-27 10:45     ` Sibi S
2018-04-25 15:08 ` [PATCH v4 2/5] reset: qcom: AOSS (always on subsystem) reset controller Sibi Sankar
2018-04-27 10:41   ` Philipp Zabel
2018-04-27 11:15     ` Sibi S
2018-04-27 11:54       ` Philipp Zabel
2018-04-25 15:08 ` [PATCH v4 3/5] dt-bindings: remoteproc: Add Q6v5 Modem PIL binding for SDM845 Sibi Sankar
2018-04-27 14:32   ` Rob Herring
2018-04-25 15:08 ` [PATCH v4 4/5] remoteproc: qcom: Add support for mss remoteproc on SDM845 Sibi Sankar
2018-05-18 21:31   ` Bjorn Andersson
2018-05-21 16:51     ` Sibi S
2018-04-25 15:08 ` [PATCH v4 5/5] remoteproc: qcom: Always assert and deassert reset signals in SDM845 Sibi Sankar
2018-05-18 21:47   ` Bjorn Andersson [this message]
2018-05-21 16:57     ` Sibi S

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180518214708.GU14924@minitux \
    --to=bjorn.andersson@linaro.org \
    --cc=akdwived@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=georgi.djakov@linaro.org \
    --cc=jassisinghbrar@gmail.com \
    --cc=kyan@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=ohad@wizery.com \
    --cc=p.zabel@pengutronix.de \
    --cc=robh+dt@kernel.org \
    --cc=sibis@codeaurora.org \
    --cc=sricharan@codeaurora.org \
    --cc=tsoni@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox