Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
On 5/26/2017 12:43 AM, Bjorn Andersson wrote:
On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
This patch refactor code to first load all firmware blobs
and then update modem proc to authenticate and boot fw.
Nice, I like this! Just some style details below.
Thanks. Sure will correct.
Also make a trivial change in a error log.
Signed-off-by: Avaneesh Kumar Dwivedi
---
drivers/remoteproc/qcom_q6v5_pil.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a..2626954 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const
struct firmware *fw)
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
if (ret == -ETIMEDOUT)
- dev_err(qproc->dev, "MPSS header authentication timed out\n");
+ dev_err(qproc->dev, "metadata authentication timed out\n");
else if (ret < 0)
- dev_err(qproc->dev, "MPSS header authentication failed: %d\n",
ret);
+ dev_err(qproc->dev,
+ "metadata authentication failed: %d\n", ret);
I'm happy to accept these changes if they better describe the errors,
but please send them in a separate patch in that case - and please don't
line break that second print.
OK. Will reconsider.
dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
bool relocate = false;
char seg_name[10];
ssize_t offset;
- size_t size;
+ size_t size = 0;
void *ptr;
int ret;
int i;
@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
}
mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
-
+ /* Load firmware completely before letting mss to start
+* authentication and then boot firmware
+*/
/*
* The first line of a multi-line comment should be empty.
*/
Also your comment tend to tell the story of your change, that's what the
commit message is for, the comment should describe the code regardless
of history;
I think it makes sense to have a comment in the lines of:
/* Load the firmware segments */
OK.
for (i = 0; i < ehdr->e_phnum; i++) {
phdr = &phdrs[i];
@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
memset(ptr + phdr->p_filesz, 0,
phdr->p_memsz - phdr->p_filesz);
}
-
- size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
- if (!size) {
- 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);
- }
-
size += phdr->p_memsz;
- writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
}
Please put a blank line here, to separate the steps like "paragraphs".
OK
+ /* Transfer ownership of modem region with modem fw */
+ 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);
I originally did something similar, but ran into some issue - so I will
test this on 8974 and 8916 as soon as my devboards are back online.
OK
Regards,
Bjorn
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Hi Bjorn,
On 5/26/2017 12:33 AM, Bjorn Andersson wrote:
> On Mon 22 May 06:26 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
>> On 5/22/2017 4:07 PM, Sricharan R wrote:
>>> Hi,
>>>
>>> On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
On 5/20/2017 8:25 AM, Sricharan R wrote:
> Hi Bjorn/Avaneesh,
>
> On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
> [..]
>> -
>> -size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>> -if (!size) {
>> -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);
>> -}
>> -
>>size += phdr->p_memsz;
>> -writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>}
> So while moving this down, can we use qcom_mdt_load instead for
> the mpss image loading part above ?
qcom_mdt_load() can not be used to load segments for mpss, as MPSS
blobs are self authenticated. while qcom_mdt_load() is used in
cases where authentication of loaded blobs is done by trustzone.
for that qcom_mdt_load() does extra steps to send pas_id to
trustzone and mem_setup() etc.
>>> Right, so my intention of asking this was if the code which does the
>>> calculation and loads the segments in qcom_mdt_load can somehow be
>>> abstracted out, so that future self authenticating rproc (even mpss
>>> in this case) can use them to load mdt ?
>> As i understand, you want to replace the piece of code which does
>> parse mdt and load individual firmware blobs in a separate routine.
>> So that it can be called by any one without again doing parsing and
>> loading for self authentication.? Till now only MPSS does rely on
>> self authentication, all other subsystems use qcom_mdt_load(). I
>> think this is reason why the qcom_mdt_load() equivalent routine has
>> not been used. Bjorn may further add in this.
>
> I have not been able to come up with a clean way to provide a useful
> mdt-loader abstraction that works for the SCM PILs, the
> self-authenticated PILs and the non-PIL SCM users.
>
> Further more with the upcoming ramdump support we will need to extract
> segment information from the mdt header, so we will have to revisit this
> topic.
>
>
> Regardless, I would prefer that we follow up with such refactoring after
> getting this series sorted out.
>
ok, agree. While trying to add Q6 support for ipq8074, which is again
a self-authenticating PIL with mdt, i can try this.
>>>
>> +/* Transfer ownership of modem region with modem fw */
>> +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);
> For ipq8074 [1], wcnss core has an Q6V5 version of the ip for
> which the initialization/boot sequence is pretty much the same
> as that has been added for msm8996 in this series. So wanted to
> understand if its better to use this remoteproc itself by
> keeping the Q6 and mpss parts separately (or) add a new
> remoteproc ?
Bjorn can better answer this query, but i believe this remoteproc
can be extended to load mpss part by adding private initialization
for the IP.
>>> ya, the mpss part can be separated out, so that this can be a Q6 +
>>> MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right
>>> way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same
>>> what you have added for msm8996.
>> Again, i believe yes but leave Bjorn to make final comment.
>
> It definitely sounds like there's room for reuse here, how much of the
> initialization and authentication sequences are common between the two?
The initialization sequence is exactly the same as what was done for
msm8996(Q6) one added in this series. So for reusing this driver for Q6,
the Q6 + MPSS has to be decoupled and driver has to look common for
Q6 + any, (ie) Q6 + mpss (or) Q6 + wcnss. Incase if that's not neat,
atleast the Q6 initialization sequence can be reused.
Regards,
Sricharan
>
> Regards,
> Bjorn
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
> This patch refactor code to first load all firmware blobs
> and then update modem proc to authenticate and boot fw.
Nice, I like this! Just some style details below.
> Also make a trivial change in a error log.
>
> Signed-off-by: Avaneesh Kumar Dwivedi
> ---
> drivers/remoteproc/qcom_q6v5_pil.c | 25 -
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8fd697a..2626954 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc,
> const struct firmware *fw)
>
> ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
> if (ret == -ETIMEDOUT)
> - dev_err(qproc->dev, "MPSS header authentication timed out\n");
> + dev_err(qproc->dev, "metadata authentication timed out\n");
> else if (ret < 0)
> - dev_err(qproc->dev, "MPSS header authentication failed: %d\n",
> ret);
> + dev_err(qproc->dev,
> + "metadata authentication failed: %d\n", ret);
I'm happy to accept these changes if they better describe the errors,
but please send them in a separate patch in that case - and please don't
line break that second print.
>
> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>
> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> bool relocate = false;
> char seg_name[10];
> ssize_t offset;
> - size_t size;
> + size_t size = 0;
> void *ptr;
> int ret;
> int i;
> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> }
>
> mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> -
> + /* Load firmware completely before letting mss to start
> + * authentication and then boot firmware
> + */
/*
* The first line of a multi-line comment should be empty.
*/
Also your comment tend to tell the story of your change, that's what the
commit message is for, the comment should describe the code regardless
of history;
I think it makes sense to have a comment in the lines of:
/* Load the firmware segments */
> for (i = 0; i < ehdr->e_phnum; i++) {
> phdr = &phdrs[i];
>
> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> memset(ptr + phdr->p_filesz, 0,
> phdr->p_memsz - phdr->p_filesz);
> }
> -
> - size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> - if (!size) {
> - 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);
> - }
> -
> size += phdr->p_memsz;
> - writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> }
Please put a blank line here, to separate the steps like "paragraphs".
> + /* Transfer ownership of modem region with modem fw */
> + 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);
I originally did something similar, but ran into some issue - so I will
test this on 8974 and 8916 as soon as my devboards are back online.
Regards,
Bjorn
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
On Mon 22 May 06:26 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 5/22/2017 4:07 PM, Sricharan R wrote:
> > Hi,
> >
> > On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
> > >
> > > On 5/20/2017 8:25 AM, Sricharan R wrote:
> > > > Hi Bjorn/Avaneesh,
> > > >
> > > > On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
[..]
> > > > > -
> > > > > -size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> > > > > -if (!size) {
> > > > > -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);
> > > > > -}
> > > > > -
> > > > >size += phdr->p_memsz;
> > > > > -writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> > > > >}
> > > > So while moving this down, can we use qcom_mdt_load instead for
> > > > the mpss image loading part above ?
> > > qcom_mdt_load() can not be used to load segments for mpss, as MPSS
> > > blobs are self authenticated. while qcom_mdt_load() is used in
> > > cases where authentication of loaded blobs is done by trustzone.
> > > for that qcom_mdt_load() does extra steps to send pas_id to
> > > trustzone and mem_setup() etc.
> > Right, so my intention of asking this was if the code which does the
> > calculation and loads the segments in qcom_mdt_load can somehow be
> > abstracted out, so that future self authenticating rproc (even mpss
> > in this case) can use them to load mdt ?
> As i understand, you want to replace the piece of code which does
> parse mdt and load individual firmware blobs in a separate routine.
> So that it can be called by any one without again doing parsing and
> loading for self authentication.? Till now only MPSS does rely on
> self authentication, all other subsystems use qcom_mdt_load(). I
> think this is reason why the qcom_mdt_load() equivalent routine has
> not been used. Bjorn may further add in this.
I have not been able to come up with a clean way to provide a useful
mdt-loader abstraction that works for the SCM PILs, the
self-authenticated PILs and the non-PIL SCM users.
Further more with the upcoming ramdump support we will need to extract
segment information from the mdt header, so we will have to revisit this
topic.
Regardless, I would prefer that we follow up with such refactoring after
getting this series sorted out.
> >
> > > > > +/* Transfer ownership of modem region with modem fw */
> > > > > +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);
> > > > For ipq8074 [1], wcnss core has an Q6V5 version of the ip for
> > > > which the initialization/boot sequence is pretty much the same
> > > > as that has been added for msm8996 in this series. So wanted to
> > > > understand if its better to use this remoteproc itself by
> > > > keeping the Q6 and mpss parts separately (or) add a new
> > > > remoteproc ?
> > > Bjorn can better answer this query, but i believe this remoteproc
> > > can be extended to load mpss part by adding private initialization
> > > for the IP.
> > ya, the mpss part can be separated out, so that this can be a Q6 +
> > MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right
> > way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same
> > what you have added for msm8996.
> Again, i believe yes but leave Bjorn to make final comment.
It definitely sounds like there's room for reuse here, how much of the
initialization and authentication sequences are common between the two?
Regards,
Bjorn
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
On 5/22/2017 4:07 PM, Sricharan R wrote:
Hi,
On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
On 5/20/2017 8:25 AM, Sricharan R wrote:
Hi Bjorn/Avaneesh,
On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
This patch refactor code to first load all firmware blobs
and then update modem proc to authenticate and boot fw.
Also make a trivial change in a error log.
Signed-off-by: Avaneesh Kumar Dwivedi
---
drivers/remoteproc/qcom_q6v5_pil.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a..2626954 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const
struct firmware *fw)
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
if (ret == -ETIMEDOUT)
-dev_err(qproc->dev, "MPSS header authentication timed out\n");
+dev_err(qproc->dev, "metadata authentication timed out\n");
else if (ret < 0)
-dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
+dev_err(qproc->dev,
+"metadata authentication failed: %d\n", ret);
dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
bool relocate = false;
char seg_name[10];
ssize_t offset;
-size_t size;
+size_t size = 0;
void *ptr;
int ret;
int i;
@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
}
mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
-
+/* Load firmware completely before letting mss to start
+ * authentication and then boot firmware
+ */
for (i = 0; i < ehdr->e_phnum; i++) {
phdr = &phdrs[i];
@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
memset(ptr + phdr->p_filesz, 0,
phdr->p_memsz - phdr->p_filesz);
}
-
-size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
-if (!size) {
-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);
-}
-
size += phdr->p_memsz;
-writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
}
So while moving this down, can we use qcom_mdt_load instead for the mpss
image loading part above ?
qcom_mdt_load() can not be used to load segments for mpss, as MPSS blobs are
self authenticated.
while qcom_mdt_load() is used in cases where authentication of loaded blobs is
done by trustzone.
for that qcom_mdt_load() does extra steps to send pas_id to trustzone and
mem_setup() etc.
Right, so my intention of asking this was if the code which does the
calculation and
loads the segments in qcom_mdt_load can somehow be abstracted out, so that
future
self authenticating rproc (even mpss in this case) can use them to load mdt ?
As i understand, you want to replace the piece of code which does parse
mdt and load individual firmware blobs in a separate routine.
So that it can be called by any one without again doing parsing and
loading for self authentication.?
Till now only MPSS does rely on self authentication, all other
subsystems use qcom_mdt_load().
I think this is reason why the qcom_mdt_load() equivalent routine has
not been used.
Bjorn may further add in this.
+/* Transfer ownership of modem region with modem fw */
+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);
For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
initialization/boot sequence is pretty much the same as that has been added
for msm8996 in this series. So wanted to understand if its better to
use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
add a new remoteproc ?
Bjorn can better answer this query, but i believe this remoteproc can be
extended to load
mpss part by adding private initialization for the IP.
ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6
+ WCNSS
remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS
remoteproc,
as the Q6 part is same what you have added for msm8996.
Again, i believe yes but leave Bjorn to make final comment.
Regards,
Sricharan
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
Sricharan
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 1);
if (ret == -ETIMEDOUT)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
i
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Hi,
On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
>
>
> On 5/20/2017 8:25 AM, Sricharan R wrote:
>> Hi Bjorn/Avaneesh,
>>
>> On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
>>> This patch refactor code to first load all firmware blobs
>>> and then update modem proc to authenticate and boot fw.
>>> Also make a trivial change in a error log.
>>>
>>> Signed-off-by: Avaneesh Kumar Dwivedi
>>> ---
>>> drivers/remoteproc/qcom_q6v5_pil.c | 25 -
>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
>>> b/drivers/remoteproc/qcom_q6v5_pil.c
>>> index 8fd697a..2626954 100644
>>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>>> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc,
>>> const struct firmware *fw)
>>> ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS,
>>> 1000);
>>> if (ret == -ETIMEDOUT)
>>> -dev_err(qproc->dev, "MPSS header authentication timed out\n");
>>> +dev_err(qproc->dev, "metadata authentication timed out\n");
>>> else if (ret < 0)
>>> -dev_err(qproc->dev, "MPSS header authentication failed: %d\n",
>>> ret);
>>> +dev_err(qproc->dev,
>>> +"metadata authentication failed: %d\n", ret);
>>> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>> bool relocate = false;
>>> char seg_name[10];
>>> ssize_t offset;
>>> -size_t size;
>>> +size_t size = 0;
>>> void *ptr;
>>> int ret;
>>> int i;
>>> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>> }
>>> mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
>>> -
>>> +/* Load firmware completely before letting mss to start
>>> + * authentication and then boot firmware
>>> + */
>>> for (i = 0; i < ehdr->e_phnum; i++) {
>>> phdr = &phdrs[i];
>>> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>> memset(ptr + phdr->p_filesz, 0,
>>> phdr->p_memsz - phdr->p_filesz);
>>> }
>>> -
>>> -size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>> -if (!size) {
>>> -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);
>>> -}
>>> -
>>> size += phdr->p_memsz;
>>> -writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>> }
>> So while moving this down, can we use qcom_mdt_load instead for the mpss
>> image loading part above ?
> qcom_mdt_load() can not be used to load segments for mpss, as MPSS blobs are
> self authenticated.
> while qcom_mdt_load() is used in cases where authentication of loaded blobs
> is done by trustzone.
> for that qcom_mdt_load() does extra steps to send pas_id to trustzone and
> mem_setup() etc.
Right, so my intention of asking this was if the code which does the
calculation and
loads the segments in qcom_mdt_load can somehow be abstracted out, so that
future
self authenticating rproc (even mpss in this case) can use them to load mdt ?
>>> +/* Transfer ownership of modem region with modem fw */
>>> +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);
>> For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
>> initialization/boot sequence is pretty much the same as that has been added
>> for msm8996 in this series. So wanted to understand if its better to
>> use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
>> add a new remoteproc ?
> Bjorn can better answer this query, but i believe this remoteproc can be
> extended to load
> mpss part by adding private initialization for the IP.
ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6
+ WCNSS
remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS
remoteproc,
as the Q6 part is same what you have added for msm8996.
Regards,
Sricharan
>>
>> [1] https://patchwork.kernel.org/patch/9711725/
>>
>> Regards,
>> Sricharan
>>
>>> ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 1);
>>> if (ret == -ETIMEDOUT)
>>>
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
On 5/20/2017 8:25 AM, Sricharan R wrote:
Hi Bjorn/Avaneesh,
On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
This patch refactor code to first load all firmware blobs
and then update modem proc to authenticate and boot fw.
Also make a trivial change in a error log.
Signed-off-by: Avaneesh Kumar Dwivedi
---
drivers/remoteproc/qcom_q6v5_pil.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a..2626954 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const
struct firmware *fw)
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
if (ret == -ETIMEDOUT)
- dev_err(qproc->dev, "MPSS header authentication timed out\n");
+ dev_err(qproc->dev, "metadata authentication timed out\n");
else if (ret < 0)
- dev_err(qproc->dev, "MPSS header authentication failed: %d\n",
ret);
+ dev_err(qproc->dev,
+ "metadata authentication failed: %d\n", ret);
dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
bool relocate = false;
char seg_name[10];
ssize_t offset;
- size_t size;
+ size_t size = 0;
void *ptr;
int ret;
int i;
@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
}
mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
-
+ /* Load firmware completely before letting mss to start
+* authentication and then boot firmware
+*/
for (i = 0; i < ehdr->e_phnum; i++) {
phdr = &phdrs[i];
@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
memset(ptr + phdr->p_filesz, 0,
phdr->p_memsz - phdr->p_filesz);
}
-
- size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
- if (!size) {
- 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);
- }
-
size += phdr->p_memsz;
- writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
}
So while moving this down, can we use qcom_mdt_load instead for the mpss
image loading part above ?
qcom_mdt_load() can not be used to load segments for mpss, as MPSS blobs
are self authenticated.
while qcom_mdt_load() is used in cases where authentication of loaded
blobs is done by trustzone.
for that qcom_mdt_load() does extra steps to send pas_id to trustzone
and mem_setup() etc.
+ /* Transfer ownership of modem region with modem fw */
+ 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);
For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
initialization/boot sequence is pretty much the same as that has been added
for msm8996 in this series. So wanted to understand if its better to
use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
add a new remoteproc ?
Bjorn can better answer this query, but i believe this remoteproc can be
extended to load
mpss part by adding private initialization for the IP.
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
Sricharan
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 1);
if (ret == -ETIMEDOUT)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Hi Bjorn/Avaneesh,
On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
> This patch refactor code to first load all firmware blobs
> and then update modem proc to authenticate and boot fw.
> Also make a trivial change in a error log.
>
> Signed-off-by: Avaneesh Kumar Dwivedi
> ---
> drivers/remoteproc/qcom_q6v5_pil.c | 25 -
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8fd697a..2626954 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc,
> const struct firmware *fw)
>
> ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
> if (ret == -ETIMEDOUT)
> - dev_err(qproc->dev, "MPSS header authentication timed out\n");
> + dev_err(qproc->dev, "metadata authentication timed out\n");
> else if (ret < 0)
> - dev_err(qproc->dev, "MPSS header authentication failed: %d\n",
> ret);
> + dev_err(qproc->dev,
> + "metadata authentication failed: %d\n", ret);
>
> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>
> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> bool relocate = false;
> char seg_name[10];
> ssize_t offset;
> - size_t size;
> + size_t size = 0;
> void *ptr;
> int ret;
> int i;
> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> }
>
> mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> -
> + /* Load firmware completely before letting mss to start
> + * authentication and then boot firmware
> + */
> for (i = 0; i < ehdr->e_phnum; i++) {
> phdr = &phdrs[i];
>
> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> memset(ptr + phdr->p_filesz, 0,
> phdr->p_memsz - phdr->p_filesz);
> }
> -
> - size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> - if (!size) {
> - 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);
> - }
> -
> size += phdr->p_memsz;
> - writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> }
So while moving this down, can we use qcom_mdt_load instead for the mpss
image loading part above ?
> + /* Transfer ownership of modem region with modem fw */
> + 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);
For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
initialization/boot sequence is pretty much the same as that has been added
for msm8996 in this series. So wanted to understand if its better to
use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
add a new remoteproc ?
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
Sricharan
>
> ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 1);
> if (ret == -ETIMEDOUT)
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
[RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
This patch refactor code to first load all firmware blobs
and then update modem proc to authenticate and boot fw.
Also make a trivial change in a error log.
Signed-off-by: Avaneesh Kumar Dwivedi
---
drivers/remoteproc/qcom_q6v5_pil.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a..2626954 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const
struct firmware *fw)
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
if (ret == -ETIMEDOUT)
- dev_err(qproc->dev, "MPSS header authentication timed out\n");
+ dev_err(qproc->dev, "metadata authentication timed out\n");
else if (ret < 0)
- dev_err(qproc->dev, "MPSS header authentication failed: %d\n",
ret);
+ dev_err(qproc->dev,
+ "metadata authentication failed: %d\n", ret);
dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
bool relocate = false;
char seg_name[10];
ssize_t offset;
- size_t size;
+ size_t size = 0;
void *ptr;
int ret;
int i;
@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
}
mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
-
+ /* Load firmware completely before letting mss to start
+* authentication and then boot firmware
+*/
for (i = 0; i < ehdr->e_phnum; i++) {
phdr = &phdrs[i];
@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
memset(ptr + phdr->p_filesz, 0,
phdr->p_memsz - phdr->p_filesz);
}
-
- size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
- if (!size) {
- 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);
- }
-
size += phdr->p_memsz;
- writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
}
+ /* Transfer ownership of modem region with modem fw */
+ 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, 1);
if (ret == -ETIMEDOUT)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
