All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Sibi Sankar <sibis@codeaurora.org>
Cc: agross@kernel.org, linux-arm-msm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	evgreen@chromium.org, ohad@wizery.com
Subject: Re: [PATCH 2/3] remoteproc: qcom_q6v5_mss: Add MBA log extraction support
Date: Thu, 16 Jul 2020 21:57:41 -0700	[thread overview]
Message-ID: <20200717045741.GC2922385@builder.lan> (raw)
In-Reply-To: <20200716123630.21892-3-sibis@codeaurora.org>

On Thu 16 Jul 05:36 PDT 2020, Sibi Sankar wrote:

> On SC7180 the MBA firmware stores the bootup text logs in a 4K segment
> at the beginning of the MBA region. Add support to extract the logs
> which will be useful to debug mba boot/authentication issues.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 41 ++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 95e21ed607cb9..4ddf084b2c6fc 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/devcoredump.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -37,6 +38,8 @@
>  
>  #define MPSS_CRASH_REASON_SMEM		421
>  
> +#define MBA_LOG_SIZE			SZ_4K
> +
>  /* RMB Status Register Values */
>  #define RMB_PBL_SUCCESS			0x1
>  
> @@ -139,6 +142,7 @@ struct rproc_hexagon_res {
>  	int version;
>  	bool need_mem_protection;
>  	bool has_alt_reset;
> +	bool has_mba_logs;
>  	bool has_spare_reg;
>  };
>  
> @@ -200,6 +204,7 @@ struct q6v5 {
>  	struct qcom_sysmon *sysmon;
>  	bool need_mem_protection;
>  	bool has_alt_reset;
> +	bool has_mba_logs;
>  	bool has_spare_reg;
>  	int mpss_perm;
>  	int mba_perm;
> @@ -518,6 +523,19 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  	return val;
>  }
>  
> +static void q6v5_dump_mba_logs(struct q6v5 *qproc)
> +{
> +	struct rproc *rproc = qproc->rproc;
> +	void *data;
> +
> +	data = vmalloc(MBA_LOG_SIZE);
> +	if (!data)
> +		return;
> +
> +	memcpy(data, qproc->mba_region, MBA_LOG_SIZE);
> +	dev_coredumpv(&rproc->dev, data, MBA_LOG_SIZE, GFP_KERNEL);
> +}
> +
>  static int q6v5proc_reset(struct q6v5 *qproc)
>  {
>  	u32 val;
> @@ -838,6 +856,7 @@ static int q6v5_mba_load(struct q6v5 *qproc)
>  {
>  	int ret;
>  	int xfermemop_ret;
> +	bool mba_load_err = false;
>  
>  	qcom_q6v5_prepare(&qproc->q6v5);
>  
> @@ -931,7 +950,7 @@ static int q6v5_mba_load(struct q6v5 *qproc)
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
> -
> +	mba_load_err = true;
>  reclaim_mba:
>  	xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true,
>  						false, qproc->mba_phys,
> @@ -939,6 +958,8 @@ static int q6v5_mba_load(struct q6v5 *qproc)
>  	if (xfermemop_ret) {
>  		dev_err(qproc->dev,
>  			"Failed to reclaim mba buffer, system may become unstable\n");
> +	} else if (qproc->has_mba_logs & mba_load_err) {
> +		q6v5_dump_mba_logs(qproc);
>  	}
>  
>  disable_active_clks:
> @@ -968,7 +989,7 @@ static int q6v5_mba_load(struct q6v5 *qproc)
>  	return ret;
>  }
>  
> -static void q6v5_mba_reclaim(struct q6v5 *qproc)
> +static void q6v5_mba_reclaim(struct q6v5 *qproc, bool err_path)
>  {
>  	int ret;
>  	u32 val;
> @@ -1006,6 +1027,9 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc)
>  				      qproc->mba_size);
>  	WARN_ON(ret);
>  
> +	if (qproc->has_mba_logs && err_path && !ret)

Wouldn't it be possible to just call q6v5_dump_mba_logs() directly after
the return from q6v5_mba_reclaim()? That way we can avoid passing the
bool to indicate if the reclaim should also dump some stuff.

Sure we don't have a way to tell if the assign_mem failed, but we're
going to crash shortly anyways (which is something we should change).



I think you should move the has_mba_logs into q6v5_dump_mba_logs(),
making it cause an early return.

Regards,
Bjorn

> +		q6v5_dump_mba_logs(qproc);
> +
>  	ret = qcom_q6v5_unprepare(&qproc->q6v5);
>  	if (ret) {
>  		q6v5_pds_disable(qproc, qproc->proxy_pds,
> @@ -1255,7 +1279,7 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc,
>  						false, true,
>  						qproc->mpss_phys,
>  						qproc->mpss_size);
> -			q6v5_mba_reclaim(qproc);
> +			q6v5_mba_reclaim(qproc, false);
>  		}
>  	}
>  }
> @@ -1297,7 +1321,7 @@ static int q6v5_start(struct rproc *rproc)
>  	return 0;
>  
>  reclaim_mpss:
> -	q6v5_mba_reclaim(qproc);
> +	q6v5_mba_reclaim(qproc, true);
>  
>  	return ret;
>  }
> @@ -1313,7 +1337,7 @@ static int q6v5_stop(struct rproc *rproc)
>  	if (ret == -ETIMEDOUT)
>  		dev_err(qproc->dev, "timed out on wait\n");
>  
> -	q6v5_mba_reclaim(qproc);
> +	q6v5_mba_reclaim(qproc, false);
>  
>  	return 0;
>  }
> @@ -1717,6 +1741,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  
>  	qproc->version = desc->version;
>  	qproc->need_mem_protection = desc->need_mem_protection;
> +	qproc->has_mba_logs = desc->has_mba_logs;
>  
>  	ret = qcom_q6v5_init(&qproc->q6v5, pdev, rproc, MPSS_CRASH_REASON_SMEM,
>  			     qcom_msa_handover);
> @@ -1808,6 +1833,7 @@ static const struct rproc_hexagon_res sc7180_mss = {
>  	},
>  	.need_mem_protection = true,
>  	.has_alt_reset = false,
> +	.has_mba_logs = true,
>  	.has_spare_reg = true,
>  	.version = MSS_SC7180,
>  };
> @@ -1843,6 +1869,7 @@ static const struct rproc_hexagon_res sdm845_mss = {
>  	},
>  	.need_mem_protection = true,
>  	.has_alt_reset = true,
> +	.has_mba_logs = true,
>  	.has_spare_reg = false,
>  	.version = MSS_SDM845,
>  };
> @@ -1870,6 +1897,7 @@ static const struct rproc_hexagon_res msm8998_mss = {
>  	},
>  	.need_mem_protection = true,
>  	.has_alt_reset = false,
> +	.has_mba_logs = false,
>  	.has_spare_reg = false,
>  	.version = MSS_MSM8998,
>  };
> @@ -1900,6 +1928,7 @@ static const struct rproc_hexagon_res msm8996_mss = {
>  	},
>  	.need_mem_protection = true,
>  	.has_alt_reset = false,
> +	.has_mba_logs = false,
>  	.has_spare_reg = false,
>  	.version = MSS_MSM8996,
>  };
> @@ -1933,6 +1962,7 @@ static const struct rproc_hexagon_res msm8916_mss = {
>  	},
>  	.need_mem_protection = false,
>  	.has_alt_reset = false,
> +	.has_mba_logs = false,
>  	.has_spare_reg = false,
>  	.version = MSS_MSM8916,
>  };
> @@ -1974,6 +2004,7 @@ static const struct rproc_hexagon_res msm8974_mss = {
>  	},
>  	.need_mem_protection = false,
>  	.has_alt_reset = false,
> +	.has_mba_logs = false,
>  	.has_spare_reg = false,
>  	.version = MSS_MSM8974,
>  };
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

  parent reply	other threads:[~2020-07-17  4:59 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-16 12:36 [PATCH 0/3] Add modem debug features Sibi Sankar
2020-07-16 12:36 ` [PATCH 1/3] remoteproc: qcom_q6v5_mss: Add modem debug policy support Sibi Sankar
2020-07-17  4:41   ` Bjorn Andersson
2020-07-17  5:07     ` Sibi Sankar
2020-07-16 12:36 ` [PATCH 2/3] remoteproc: qcom_q6v5_mss: Add MBA log extraction support Sibi Sankar
2020-07-16 13:43   ` Manivannan Sadhasivam
2020-07-16 14:58     ` Sibi Sankar
2020-07-17 14:38       ` Manivannan Sadhasivam
2020-07-17  4:57   ` Bjorn Andersson [this message]
2020-07-17  5:22     ` Sibi Sankar
2020-07-17  5:52       ` Bjorn Andersson
2020-07-16 12:36 ` [PATCH 3/3] remoteproc: qcom_q6v5_mss: Update MBA log info Sibi Sankar
2020-07-17  4:59   ` Bjorn Andersson
2020-07-17  5:28     ` Sibi Sankar

Reply instructions:

You may reply publicly 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=20200717045741.GC2922385@builder.lan \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=evgreen@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ohad@wizery.com \
    --cc=sibis@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.