| Message ID | 1496344641-6291-4-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote: > MSS proc on msm8996 can not access fw loaded region without stage > second translation of memory pages where mpss image are loaded. > This patch in order to enable mss boot on msm8996 invoke scm call > to switch or share ownership between apps and modem. > Overall this looks good now, I have two minor notes that I want you to fix up though. > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > @@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, > return &table; > } > > +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, > + int image, phys_addr_t addr, Rather than relying on a static int to keep track of current permissions pass a "int *current_perms", that you update on success. Add int mba_perm and int mpss_perm to the struct q6v5 and initialize them in probe and just keep the metadata_perm on the stack in q6v5_mpss_init_image. > + size_t size) > +{ > + static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)}, > + {BIT(QCOM_SCM_VMID_HLOS)}, > + {BIT(QCOM_SCM_VMID_HLOS)} }; > + struct qcom_scm_vmperm next[] = {{0} }; You don't need to initialize this, and if you just keep it "struct qcom_scm_vmperm next" you can pass it as &next in the qcom_scm_assign_mem() call. > + int ret; > + > + if (!qproc->need_mem_protection) > + return 0; > + > + if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) { And rather than making this flip back and forth with every call, I think it's more robust if you pass the new expected owner as a parameter to the function. Simplest way I can think of it to add a "bool remote_owner" as a parameter; it makes the changes minimal and works with the naming of the function. > + next->vmid = QCOM_SCM_VMID_MSS_MSA; > + next->perm = QCOM_SCM_PERM_RW; > + } else { > + next->vmid = QCOM_SCM_VMID_HLOS; > + next->perm = QCOM_SCM_PERM_RWX; > + } > + > + ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), > + current_owner[image][0], next, 1); > + if (ret < 0) { > + pr_err("Failed to assign %s memory access in range %p to %p ret = %d\n", > + (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"), > + (void *)addr, (void *)(addr + size), ret); > + return ret; > + } > + > + current_owner[image][0] = ret; > + return 0; > +} > + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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 lot many for such a blazing fast response :)
regarding your points.
a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two
additional inputs i.e. *next_perm and *next_vmid
and that based on successful return of qcom_scm_assign () they
should be treated as *current_perm and *current_vmid
by caller? if so caller will have to do flipping between MSS and
HLOS, isn't it?
Also code churning will increase this way, and also logging the way
is being handled now may require to change again.
or am i misunderstanding your proposal?
Sorry for inconvenience, but if you could through little more light,
that will help.
-Avaneesh
On 6/2/2017 2:12 AM, Bjorn Andersson wrote:
> On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>
>> MSS proc on msm8996 can not access fw loaded region without stage
>> second translation of memory pages where mpss image are loaded.
>> This patch in order to enable mss boot on msm8996 invoke scm call
>> to switch or share ownership between apps and modem.
>>
> Overall this looks good now, I have two minor notes that I want you to
> fix up though.
>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
>> return &table;
>> }
>>
>> +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc,
>> + int image, phys_addr_t addr,
> Rather than relying on a static int to keep track of current permissions
> pass a "int *current_perms", that you update on success.
>
> Add int mba_perm and int mpss_perm to the struct q6v5 and initialize
> them in probe and just keep the metadata_perm on the stack in
> q6v5_mpss_init_image.
>
>> + size_t size)
>> +{
>> + static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)},
>> + {BIT(QCOM_SCM_VMID_HLOS)},
>> + {BIT(QCOM_SCM_VMID_HLOS)} };
>> + struct qcom_scm_vmperm next[] = {{0} };
> You don't need to initialize this, and if you just keep it "struct
> qcom_scm_vmperm next" you can pass it as &next in the
> qcom_scm_assign_mem() call.
>
>> + int ret;
>> +
>> + if (!qproc->need_mem_protection)
>> + return 0;
>> +
>> + if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) {
> And rather than making this flip back and forth with every call, I think
> it's more robust if you pass the new expected owner as a parameter to
> the function. Simplest way I can think of it to add a "bool
> remote_owner" as a parameter; it makes the changes minimal and works
> with the naming of the function.
>
>> + next->vmid = QCOM_SCM_VMID_MSS_MSA;
>> + next->perm = QCOM_SCM_PERM_RW;
>> + } else {
>> + next->vmid = QCOM_SCM_VMID_HLOS;
>> + next->perm = QCOM_SCM_PERM_RWX;
>> + }
>> +
>> + ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
>> + current_owner[image][0], next, 1);
>> + if (ret < 0) {
>> + pr_err("Failed to assign %s memory access in range %p to %p ret = %d\n",
>> + (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"),
>> + (void *)addr, (void *)(addr + size), ret);
>> + return ret;
>> + }
>> +
>> + current_owner[image][0] = ret;
>> + return 0;
>> +}
>> +
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > Hi Bjorn, > > Thanks lot many for such a blazing fast response :) > > regarding your points. > > a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two > additional inputs i.e. *next_perm and *next_vmid > You have two cases; assign to HLOS and assign to MSS, so I imagine that you pass a single indicator of which you want to assign. I.e. rather than looking at what the current state is and flipping you pass the conditional of that if statement as a parameter. > and that based on successful return of qcom_scm_assign () they should be > treated as *current_perm and *current_vmid > Instead of your index, you take a "int *curr_perms", which you use as the current vmid list and you assign at the end of the function (like you do today). So to transfer the ownership to the MSS you would make a function call like: ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true); mpss_owner would have to be initialize to HLOS before calling this, but will always be holding the current value. > by caller? if so caller will have to do flipping between MSS and HLOS, > isn't it? > As far as I can see each call to this driver is either a "transfer to MSS" or "transfer to HLOS", so that shouldn't be a problem. > Also code churning will increase this way, and also logging the way is > being handled now may require to change again. > > or am i misunderstanding your proposal? > Could be that I'm missing something, if above doesn't make sense please do let me know. > Sorry for inconvenience, but if you could through little more light, that > will help. > There's no inconvenience, thanks for reaching out for clarifications on my comments. Regards, Bjorn > > -Avaneesh > > > On 6/2/2017 2:12 AM, Bjorn Andersson wrote: > > On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote: > > > > > MSS proc on msm8996 can not access fw loaded region without stage > > > second translation of memory pages where mpss image are loaded. > > > This patch in order to enable mss boot on msm8996 invoke scm call > > > to switch or share ownership between apps and modem. > > > > > Overall this looks good now, I have two minor notes that I want you to > > fix up though. > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > > > @@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, > > > return &table; > > > } > > > +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, > > > + int image, phys_addr_t addr, > > Rather than relying on a static int to keep track of current permissions > > pass a "int *current_perms", that you update on success. > > > > Add int mba_perm and int mpss_perm to the struct q6v5 and initialize > > them in probe and just keep the metadata_perm on the stack in > > q6v5_mpss_init_image. > > > > > + size_t size) > > > +{ > > > + static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)}, > > > + {BIT(QCOM_SCM_VMID_HLOS)}, > > > + {BIT(QCOM_SCM_VMID_HLOS)} }; > > > + struct qcom_scm_vmperm next[] = {{0} }; > > You don't need to initialize this, and if you just keep it "struct > > qcom_scm_vmperm next" you can pass it as &next in the > > qcom_scm_assign_mem() call. > > > > > + int ret; > > > + > > > + if (!qproc->need_mem_protection) > > > + return 0; > > > + > > > + if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) { > > And rather than making this flip back and forth with every call, I think > > it's more robust if you pass the new expected owner as a parameter to > > the function. Simplest way I can think of it to add a "bool > > remote_owner" as a parameter; it makes the changes minimal and works > > with the naming of the function. > > > > > + next->vmid = QCOM_SCM_VMID_MSS_MSA; > > > + next->perm = QCOM_SCM_PERM_RW; > > > + } else { > > > + next->vmid = QCOM_SCM_VMID_HLOS; > > > + next->perm = QCOM_SCM_PERM_RWX; > > > + } > > > + > > > + ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), > > > + current_owner[image][0], next, 1); > > > + if (ret < 0) { > > > + pr_err("Failed to assign %s memory access in range %p to %p ret = %d\n", > > > + (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"), > > > + (void *)addr, (void *)(addr + size), ret); > > > + return ret; > > > + } > > > + > > > + current_owner[image][0] = ret; > > > + return 0; > > > +} > > > + > > Regards, > > Bjorn > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project. > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/2/2017 11:25 PM, Bjorn Andersson wrote: > On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > >> Hi Bjorn, >> >> Thanks lot many for such a blazing fast response :) >> >> regarding your points. >> >> a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two >> additional inputs i.e. *next_perm and *next_vmid >> > You have two cases; assign to HLOS and assign to MSS, so I imagine that > you pass a single indicator of which you want to assign. I.e. rather > than looking at what the current state is and flipping you pass the > conditional of that if statement as a parameter. OK > >> and that based on successful return of qcom_scm_assign () they should be >> treated as *current_perm and *current_vmid >> > Instead of your index, you take a "int *curr_perms", which you use as > the current vmid list and you assign at the end of the function (like > you do today). > > So to transfer the ownership to the MSS you would make a function call > like: > > ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true); > > mpss_owner would have to be initialize to HLOS before calling this, but > will always be holding the current value. i am not finding compelling enough region to carry an input pointer to hold current ownership specially when i am carrying a boolean flag to check whether next->vmid will be MSS or HLOS I mean where am i going to use this current owner info in mss rproc driver, i am yet not getting enough reason. while the local array did job of maintaining and flipping the ownership based on info if which image ownership transfer is being called. > >> by caller? if so caller will have to do flipping between MSS and HLOS, >> isn't it? >> > As far as I can see each call to this driver is either a "transfer to > MSS" or "transfer to HLOS", so that shouldn't be a problem. Yes this job will be done by bool flag, but again what is then use of carry pointers mpss_owner, mba_owner. > >> Also code churning will increase this way, and also logging the way is >> being handled now may require to change again. >> >> or am i misunderstanding your proposal? >> > Could be that I'm missing something, if above doesn't make sense please > do let me know. I can not say it does not make sense, probably something subtle i am missing to see. > >> Sorry for inconvenience, but if you could through little more light, that >> will help. >> > There's no inconvenience, thanks for reaching out for clarifications on > my comments. Thanks for such a nice gesture. i feel that maintaining the local array for ownership switching looks, too raw way of doing something. may be i can improve that, but first i need to get understanding of your vision in suggesting above changes. > > Regards, > Bjorn > >> -Avaneesh >> >> >> On 6/2/2017 2:12 AM, Bjorn Andersson wrote: >>> On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote: >>> >>>> MSS proc on msm8996 can not access fw loaded region without stage >>>> second translation of memory pages where mpss image are loaded. >>>> This patch in order to enable mss boot on msm8996 invoke scm call >>>> to switch or share ownership between apps and modem. >>>> >>> Overall this looks good now, I have two minor notes that I want you to >>> fix up though. >>> >>>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >>>> @@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, >>>> return &table; >>>> } >>>> +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, >>>> + int image, phys_addr_t addr, >>> Rather than relying on a static int to keep track of current permissions >>> pass a "int *current_perms", that you update on success. >>> >>> Add int mba_perm and int mpss_perm to the struct q6v5 and initialize >>> them in probe and just keep the metadata_perm on the stack in >>> q6v5_mpss_init_image. >>> >>>> + size_t size) >>>> +{ >>>> + static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)}, >>>> + {BIT(QCOM_SCM_VMID_HLOS)}, >>>> + {BIT(QCOM_SCM_VMID_HLOS)} }; >>>> + struct qcom_scm_vmperm next[] = {{0} }; >>> You don't need to initialize this, and if you just keep it "struct >>> qcom_scm_vmperm next" you can pass it as &next in the >>> qcom_scm_assign_mem() call. >>> >>>> + int ret; >>>> + >>>> + if (!qproc->need_mem_protection) >>>> + return 0; >>>> + >>>> + if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) { >>> And rather than making this flip back and forth with every call, I think >>> it's more robust if you pass the new expected owner as a parameter to >>> the function. Simplest way I can think of it to add a "bool >>> remote_owner" as a parameter; it makes the changes minimal and works >>> with the naming of the function. >>> >>>> + next->vmid = QCOM_SCM_VMID_MSS_MSA; >>>> + next->perm = QCOM_SCM_PERM_RW; >>>> + } else { >>>> + next->vmid = QCOM_SCM_VMID_HLOS; >>>> + next->perm = QCOM_SCM_PERM_RWX; >>>> + } >>>> + >>>> + ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), >>>> + current_owner[image][0], next, 1); >>>> + if (ret < 0) { >>>> + pr_err("Failed to assign %s memory access in range %p to %p ret = %d\n", >>>> + (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"), >>>> + (void *)addr, (void *)(addr + size), ret); >>>> + return ret; >>>> + } >>>> + >>>> + current_owner[image][0] = ret; >>>> + return 0; >>>> +} >>>> + >>> Regards, >>> Bjorn >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- >> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project. >>
On Wed 07 Jun 09:27 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > > > On 6/2/2017 11:25 PM, Bjorn Andersson wrote: > > On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > > > > > Hi Bjorn, > > > > > > Thanks lot many for such a blazing fast response :) > > > > > > regarding your points. > > > > > > a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two > > > additional inputs i.e. *next_perm and *next_vmid > > > > > You have two cases; assign to HLOS and assign to MSS, so I imagine that > > you pass a single indicator of which you want to assign. I.e. rather > > than looking at what the current state is and flipping you pass the > > conditional of that if statement as a parameter. > OK > > > > > and that based on successful return of qcom_scm_assign () they should be > > > treated as *current_perm and *current_vmid > > > > > Instead of your index, you take a "int *curr_perms", which you use as > > the current vmid list and you assign at the end of the function (like > > you do today). > > > > So to transfer the ownership to the MSS you would make a function call > > like: > > > > ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true); > > > > mpss_owner would have to be initialize to HLOS before calling this, but > > will always be holding the current value. > i am not finding compelling enough region to carry an input pointer to hold > current ownership > specially when i am carrying a boolean flag to check whether next->vmid will > be MSS or HLOS > I mean where am i going to use this current owner info in mss rproc driver, > i am yet not getting enough reason. > while the local array did job of maintaining and flipping the ownership > based on info if which image ownership transfer is being called. > As far as I can see your patch works fine, every code path will end up calling xfer_mem() an even number of times, meaning that when we're done the ownership is on the HLOS side. But the reason I don't like this flip-flop mechanism is that it forces us to _always_ exit every code path with an even number of calls. Meaning that if we ever refactor any of this code and accidentally add another flip, we will start seeing "random" crashes. This is the reason why I want the code to be explicit in "transfer permission to X". The reason for not using the "destination owner" for figuring out the current owner is that in the even that you call "transfer permission to HLOS" twice in a row, you will call TZ saying that the current ownership is MSS and the call will fail. In this case the calling code has no chance to know if we failed because we have called xfer_mem() an odd number of times or something else and although we are good (HLOS is owner) we have to treat this as a fatal error. So, it's all about future maintainability - not about it currently working. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index f5f8850..266efad 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -110,6 +110,7 @@ struct rproc_hexagon_res { struct qcom_mss_reg_res *active_supply; char **proxy_clk_names; char **active_clk_names; + bool need_mem_protection; }; struct q6v5 { @@ -151,6 +152,7 @@ struct q6v5 { phys_addr_t mpss_reloc; void *mpss_region; size_t mpss_size; + bool need_mem_protection; struct qcom_rproc_subdev smd_subdev; }; @@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, return &table; } +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, + int image, phys_addr_t addr, + size_t size) +{ + static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)}, + {BIT(QCOM_SCM_VMID_HLOS)}, + {BIT(QCOM_SCM_VMID_HLOS)} }; + struct qcom_scm_vmperm next[] = {{0} }; + int ret; + + if (!qproc->need_mem_protection) + return 0; + + if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) { + next->vmid = QCOM_SCM_VMID_MSS_MSA; + next->perm = QCOM_SCM_PERM_RW; + } else { + next->vmid = QCOM_SCM_VMID_HLOS; + next->perm = QCOM_SCM_PERM_RWX; + } + + ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), + current_owner[image][0], next, 1); + if (ret < 0) { + pr_err("Failed to assign %s memory access in range %p to %p ret = %d\n", + (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"), + (void *)addr, (void *)(addr + size), ret); + return ret; + } + + current_owner[image][0] = ret; + return 0; +} + static int q6v5_load(struct rproc *rproc, const struct firmware *fw) { struct q6v5 *qproc = rproc->priv; @@ -450,6 +486,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) { unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; dma_addr_t phys; + int xferop_ret; void *ptr; int ret; @@ -461,6 +498,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) memcpy(ptr, fw->data, fw->size); + /* Hypervisor mapping to access metadata by modem */ + ret = q6v5_xfer_mem_ownership(qproc, 0, phys, fw->size); + if (ret) + return -EAGAIN; writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG); writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); @@ -470,6 +511,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) else if (ret < 0) dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret); + /* Metadata authentication done, remove modem access */ + xferop_ret = q6v5_xfer_mem_ownership(qproc, 0, phys, fw->size); + if (xferop_ret) + dev_warn(qproc->dev, + "mdt buffer not reclaimed system may become unstable\n"); dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); return ret < 0 ? ret : 0; @@ -579,6 +625,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc) /* Transfer ownership of modem ddr region with q6*/ boot_addr = relocate ? qproc->mpss_phys : min_addr; + ret = q6v5_xfer_mem_ownership(qproc, 2, + qproc->mpss_phys, qproc->mpss_size); + if (ret) + return -EAGAIN; 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); @@ -598,6 +648,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) static int q6v5_start(struct rproc *rproc) { struct q6v5 *qproc = (struct q6v5 *)rproc->priv; + int xfermemop_ret; int ret; ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, @@ -633,6 +684,11 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + /* Assign MBA image access in DDR to q6 */ + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, 1, + qproc->mba_phys, qproc->mba_size); + if (xfermemop_ret) + goto assert_reset; writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); ret = q6v5proc_reset(qproc); @@ -654,16 +710,21 @@ static int q6v5_start(struct rproc *rproc) ret = q6v5_mpss_load(qproc); if (ret) - goto halt_axi_ports; + goto reclaim_mem; ret = wait_for_completion_timeout(&qproc->start_done, msecs_to_jiffies(5000)); if (ret == 0) { dev_err(qproc->dev, "start timed out\n"); ret = -ETIMEDOUT; - goto halt_axi_ports; + goto reclaim_mem; } + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, 1, + qproc->mba_phys, qproc->mba_size); + if (xfermemop_ret) + dev_err(qproc->dev, + "Failed to reclaim mba buffer system may become unstable\n"); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks, @@ -673,12 +734,21 @@ static int q6v5_start(struct rproc *rproc) return 0; +reclaim_mem: + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, 2, + qproc->mpss_phys, qproc->mpss_size); + WARN_ON(xfermemop_ret); halt_axi_ports: 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); q6v5_clk_disable(qproc->dev, qproc->active_clks, qproc->active_clk_count); + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, 1, + qproc->mba_phys, qproc->mba_size); + if (xfermemop_ret) + dev_err(qproc->dev, "Failed to reclaim mba buffer, system may become unstable\n"); + assert_reset: reset_control_assert(qproc->mss_restart); disable_vdd: @@ -698,6 +768,7 @@ static int q6v5_stop(struct rproc *rproc) { struct q6v5 *qproc = (struct q6v5 *)rproc->priv; int ret; + u32 val; qproc->running = false; @@ -715,6 +786,9 @@ static int q6v5_stop(struct rproc *rproc) q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); + ret = q6v5_xfer_mem_ownership(qproc, 2, + qproc->mpss_phys, qproc->mpss_size); + WARN_ON(ret); reset_control_assert(qproc->mss_restart); q6v5_clk_disable(qproc->dev, qproc->active_clks, qproc->active_clk_count); @@ -1012,6 +1086,7 @@ static int q6v5_probe(struct platform_device *pdev) if (ret) goto free_rproc; + qproc->need_mem_protection = desc->need_mem_protection; ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); if (ret < 0) goto free_rproc; @@ -1087,6 +1162,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .need_mem_protection = false, }; static const struct rproc_hexagon_res msm8974_mss = { @@ -1124,6 +1200,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .need_mem_protection = false, }; static const struct of_device_id q6v5_of_match[] = {
MSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_pil.c | 81 +++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-)