From: Luca Weiss <luca@z3ntu.xyz>
To: linux-arm-msm@vger.kernel.org
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
Ohad Ben-Cohen <ohad@wizery.com>,
Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>,
linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
Sibi Sankar <sibis@codeaurora.org>,
Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
Brian Masney <masneyb@onstation.org>
Subject: Re: [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading
Date: Sun, 10 Nov 2019 15:05:54 +0100 [thread overview]
Message-ID: <393350950.66DGQb6nHQ@g550jk> (raw)
In-Reply-To: <20191109004033.1496871-3-bjorn.andersson@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 7811 bytes --]
Hi Bjorn,
with your patches and modifications in qcom-msm8974.dtsi, I can boot the modem
successfully on the Fairphone 2, without the 'hack' commit we had in the tree
before! Thanks!
Tested-by: Luca Weiss <luca@z3ntu.xyz>
On Samstag, 9. November 2019 01:40:33 CET Bjorn Andersson wrote:
> The code used to sync with the MBA after each segment loaded and this is
> still what's done downstream. So reduce the delta towards downstream by
> switching to a model where the content is iteratively validated.
>
> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v1:
> - Picked up Jeff's r-b and t-b
>
> drivers/remoteproc/qcom_q6v5_mss.c | 76 ++++++++++++++++++++----------
> 1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
> b/drivers/remoteproc/qcom_q6v5_mss.c index efab574b2e12..914d5546e1cf
> 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -358,23 +358,29 @@ static void q6v5_pds_disable(struct q6v5 *qproc,
> struct device **pds, }
>
> static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm,
> - bool remote_owner, phys_addr_t
addr,
> + bool local, bool remote,
phys_addr_t addr,
> size_t size)
> {
> - struct qcom_scm_vmperm next;
> + struct qcom_scm_vmperm next[2];
> + int perms = 0;
>
> if (!qproc->need_mem_protection)
> return 0;
> - if (remote_owner && *current_perm == BIT(QCOM_SCM_VMID_MSS_MSA))
> - return 0;
> - if (!remote_owner && *current_perm == BIT(QCOM_SCM_VMID_HLOS))
> - return 0;
>
> - next.vmid = remote_owner ? QCOM_SCM_VMID_MSS_MSA :
QCOM_SCM_VMID_HLOS;
> - next.perm = remote_owner ? QCOM_SCM_PERM_RW : QCOM_SCM_PERM_RWX;
> + if (local) {
> + next[perms].vmid = QCOM_SCM_VMID_HLOS;
> + next[perms].perm = QCOM_SCM_PERM_RWX;
> + perms++;
> + }
> +
> + if (remote) {
> + next[perms].vmid = QCOM_SCM_VMID_MSS_MSA;
> + next[perms].perm = QCOM_SCM_PERM_RW;
> + perms++;
> + }
>
> return qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
> - current_perm, &next, 1);
> + current_perm, next, perms);
> }
>
> static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
> @@ -681,7 +687,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc,
> const struct firmware *fw)
>
> /* Hypervisor mapping to access metadata by modem */
> mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> - ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, phys,
size);
> + ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
phys,
> size); if (ret) {
> dev_err(qproc->dev,
> "assigning Q6 access to metadata failed:
%d\n", ret);
> @@ -699,7 +705,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc,
> const struct firmware *fw) dev_err(qproc->dev, "MPSS header authentication
> failed: %d\n", ret);
>
> /* Metadata authentication done, remove modem access */
> - xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false,
phys,
> size); + xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true,
> false, phys, size); if (xferop_ret)
> dev_warn(qproc->dev,
> "mdt buffer not reclaimed system may become
unstable\n");
> @@ -786,7 +792,7 @@ static int q6v5_mba_load(struct q6v5 *qproc)
> }
>
> /* Assign MBA image access in DDR to q6 */
> - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true,
> + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, true,
> qproc->mba_phys, qproc-
>mba_size);
> if (ret) {
> dev_err(qproc->dev,
> @@ -820,8 +826,8 @@ static int q6v5_mba_load(struct q6v5 *qproc)
> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>
> reclaim_mba:
> - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm,
false,
> - qproc-
>mba_phys,
> + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm,
true,
> + false,
qproc->mba_phys,
> qproc-
>mba_size);
> if (xfermemop_ret) {
> dev_err(qproc->dev,
> @@ -888,7 +894,7 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc)
> /* In case of failure or coredump scenario where reclaiming MBA
memory
> * could not happen reclaim it here.
> */
> - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
> + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, false,
> qproc->mba_phys,
> qproc->mba_size);
> WARN_ON(ret);
> @@ -915,6 +921,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> phys_addr_t boot_addr;
> phys_addr_t min_addr = PHYS_ADDR_MAX;
> phys_addr_t max_addr = 0;
> + u32 code_length;
> bool relocate = false;
> char *fw_name;
> size_t fw_name_len;
> @@ -965,9 +972,19 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> }
>
> /* Try to reset ownership back to Linux */
> - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false,
> + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, false,
> qproc->mpss_phys, qproc-
>mpss_size);
>
> + /* Share ownership between Linux and MSS, during segment loading */
> + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, true,
> + qproc->mpss_phys, qproc-
>mpss_size);
> + if (ret) {
> + dev_err(qproc->dev,
> + "assigning Q6 access to mpss memory failed:
%d\n", ret);
> + ret = -EAGAIN;
> + goto release_firmware;
> + }
> +
> mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> qproc->mpss_reloc = mpss_reloc;
> /* Load firmware segments */
> @@ -1016,10 +1033,24 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> phdr->p_memsz - phdr->p_filesz);
> }
> size += phdr->p_memsz;
> +
> + code_length = readl(qproc->rmb_base +
RMB_PMI_CODE_LENGTH_REG);
> + if (!code_length) {
> + boot_addr = relocate ? qproc->mpss_phys :
min_addr;
> + writel(boot_addr, qproc->rmb_base +
RMB_PMI_CODE_START_REG);
> + writel(RMB_CMD_LOAD_READY, qproc->rmb_base +
RMB_MBA_COMMAND_REG);
> + }
> + writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> +
> + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG);
> + if (ret < 0) {
> + dev_err(qproc->dev, "MPSS authentication
failed: %d\n", ret);
> + goto release_firmware;
> + }
> }
>
> /* Transfer ownership of modem ddr region to q6 */
> - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true,
> + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false,
true,
> qproc->mpss_phys, qproc-
>mpss_size);
> if (ret) {
> dev_err(qproc->dev,
> @@ -1028,11 +1059,6 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> goto release_firmware;
> }
>
> - boot_addr = relocate ? qproc->mpss_phys : min_addr;
> - writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> - writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> - writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> -
> ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
> if (ret == -ETIMEDOUT)
> dev_err(qproc->dev, "MPSS authentication timed out\n");
> @@ -1061,7 +1087,7 @@ static void qcom_q6v5_dump_segment(struct rproc
> *rproc, ret = q6v5_mba_load(qproc);
>
> /* Try to reset ownership back to Linux */
> - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
false,
> + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true,
false,
> qproc->mpss_phys,
qproc->mpss_size);
> }
>
> @@ -1101,8 +1127,8 @@ static int q6v5_start(struct rproc *rproc)
> goto reclaim_mpss;
> }
>
> - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm,
false,
> - qproc-
>mba_phys,
> + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm,
true,
> + false,
qproc->mba_phys,
> qproc-
>mba_size);
> if (xfermemop_ret)
> dev_err(qproc->dev,
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-11-10 14:05 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-09 0:40 [PATCH v2 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson
2019-11-09 0:40 ` [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson
2019-11-12 17:01 ` Jeffrey Hugo
2019-11-13 18:02 ` Jeffrey Hugo
2019-11-09 0:40 ` [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson
2019-11-10 14:05 ` Luca Weiss [this message]
2019-11-11 6:45 ` Bjorn Andersson
2019-11-11 6:45 ` Bjorn Andersson
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=393350950.66DGQb6nHQ@g550jk \
--to=luca@z3ntu.xyz \
--cc=akdwived@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=jeffrey.l.hugo@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=masneyb@onstation.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.