| Message ID | 20220128025513.97188-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | soc: qcom: mdt_loader: Support Qualcomm SM8450 | expand |
On 28/01/2022 05:55, Bjorn Andersson wrote: > The Qualcomm SM8450 platform comes with both some smaller changes in the > firmware packaging and a new requirement to hold onto the metadata buffer until > PAS auth_and_reset has been completed. > > Extend the PAS api and rework the mdt_loader to meet these new requirements, > then wire this up with the PAS remoteproc driver and finally add the SM8450 > remoteproc instances. > > Bjorn Andersson (13): > firmware: qcom: scm: Introduce pas_metadata context > soc: qcom: mdt_loader: Split out split-file-loader > soc: qcom: mdt_loader: Allow hash segment to be split out > soc: qcom: mdt_loader: Allow hash to reside in any segment > soc: qcom: mdt_loader: Extend check for split firmware > soc: qcom: mdt_loader: Reorder parts of __qcom_mdt_load() > soc: qcom: mdt_loader: Always invoke PAS mem_setup > soc: qcom: mdt_loader: Extract PAS operations > remoteproc: qcom: pas: Carry PAS metadata context > dt-bindings: remoteproc: qcom: pas: Add SM8450 PAS compatibles > remoteproc: qcom: pas: Add SM8450 remoteproc support > arm64: dts: qcom: sm8450: Add remoteproc enablers and instances > arm64: dts: qcom: sm8450-qrd: Enable remoteproc instances Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Minor nitpicks: - I'd reorder the series by moving patch 1 (pas_metadata) closer to patch 8&9 (pas metadata usage) - I would have added pas_metadata as an argument to qcom_mdt_load(). However I see, why you didn't want to add another argument to the list. > > .../bindings/remoteproc/qcom,adsp.yaml | 16 + > arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 20 ++ > arch/arm64/boot/dts/qcom/sm8450.dtsi | 297 ++++++++++++++++++ > drivers/firmware/qcom_scm.c | 39 ++- > drivers/remoteproc/qcom_q6v5_mss.c | 7 +- > drivers/remoteproc/qcom_q6v5_pas.c | 36 ++- > drivers/soc/qcom/mdt_loader.c | 232 +++++++++----- > include/linux/qcom_scm.h | 10 +- > include/linux/soc/qcom/mdt_loader.h | 17 +- > 9 files changed, 579 insertions(+), 95 deletions(-) >
On Thu 03 Feb 07:11 PST 2022, Dmitry Baryshkov wrote: > On 28/01/2022 05:55, Bjorn Andersson wrote: > > The Qualcomm SM8450 platform comes with both some smaller changes in the > > firmware packaging and a new requirement to hold onto the metadata buffer until > > PAS auth_and_reset has been completed. > > > > Extend the PAS api and rework the mdt_loader to meet these new requirements, > > then wire this up with the PAS remoteproc driver and finally add the SM8450 > > remoteproc instances. > > > > Bjorn Andersson (13): > > firmware: qcom: scm: Introduce pas_metadata context > > soc: qcom: mdt_loader: Split out split-file-loader > > soc: qcom: mdt_loader: Allow hash segment to be split out > > soc: qcom: mdt_loader: Allow hash to reside in any segment > > soc: qcom: mdt_loader: Extend check for split firmware > > soc: qcom: mdt_loader: Reorder parts of __qcom_mdt_load() > > soc: qcom: mdt_loader: Always invoke PAS mem_setup > > soc: qcom: mdt_loader: Extract PAS operations > > remoteproc: qcom: pas: Carry PAS metadata context > > dt-bindings: remoteproc: qcom: pas: Add SM8450 PAS compatibles > > remoteproc: qcom: pas: Add SM8450 remoteproc support > > arm64: dts: qcom: sm8450: Add remoteproc enablers and instances > > arm64: dts: qcom: sm8450-qrd: Enable remoteproc instances > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Thanks. > Minor nitpicks: > - I'd reorder the series by moving patch 1 (pas_metadata) closer to patch > 8&9 (pas metadata usage) For a while the design where such that I would merge the first patch into a immutable branch and then merge the soc/qcom and remoteproc changes separately. But as you can see, in the end the remoteproc patch ended up depending on the mdt_loader changes. I like your suggestion, so I can move the scm change down to keep things together. > - I would have added pas_metadata as an argument to qcom_mdt_load(). > However I see, why you didn't want to add another argument to the list. > I looked at that, but I was already unhappy with the argument explosion in that function prototype. By splitting out the difference between qcom_mdt_load() and qcom_mdt_load_no_init() into a separate function will allow some cleanup and better reuse in the client drivers. As we bring up the various clients on SM8450 we will need to perform the same modifications that was done to the remoteproc driver, by doing it like this we don't need to change the prototype twice. Regards, Bjorn > > > > .../bindings/remoteproc/qcom,adsp.yaml | 16 + > > arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 20 ++ > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 297 ++++++++++++++++++ > > drivers/firmware/qcom_scm.c | 39 ++- > > drivers/remoteproc/qcom_q6v5_mss.c | 7 +- > > drivers/remoteproc/qcom_q6v5_pas.c | 36 ++- > > drivers/soc/qcom/mdt_loader.c | 232 +++++++++----- > > include/linux/qcom_scm.h | 10 +- > > include/linux/soc/qcom/mdt_loader.h | 17 +- > > 9 files changed, 579 insertions(+), 95 deletions(-) > > > > > -- > With best wishes > Dmitry
On Thu, 27 Jan 2022 18:55:00 -0800, Bjorn Andersson wrote: > The Qualcomm SM8450 platform comes with both some smaller changes in the > firmware packaging and a new requirement to hold onto the metadata buffer until > PAS auth_and_reset has been completed. > > Extend the PAS api and rework the mdt_loader to meet these new requirements, > then wire this up with the PAS remoteproc driver and finally add the SM8450 > remoteproc instances. > > [...] Applied, thanks! [12/13] arm64: dts: qcom: sm8450: Add remoteproc enablers and instances commit: e57f31b02784bdafd35940ea2592df8151ea1190 [13/13] arm64: dts: qcom: sm8450-qrd: Enable remoteproc instances commit: b48007d6d098dfeb57888c7bc50fbfb99e4e3695 Best regards,
Hello: This series was applied to qcom/linux.git (for-next) by Bjorn Andersson <bjorn.andersson@linaro.org>: On Thu, 27 Jan 2022 18:55:00 -0800 you wrote: > The Qualcomm SM8450 platform comes with both some smaller changes in the > firmware packaging and a new requirement to hold onto the metadata buffer until > PAS auth_and_reset has been completed. > > Extend the PAS api and rework the mdt_loader to meet these new requirements, > then wire this up with the PAS remoteproc driver and finally add the SM8450 > remoteproc instances. > > [...] Here is the summary with links: - [01/13] firmware: qcom: scm: Introduce pas_metadata context https://git.kernel.org/qcom/c/3a99f121fe0b - [02/13] soc: qcom: mdt_loader: Split out split-file-loader https://git.kernel.org/qcom/c/26c1f17013a8 - [03/13] soc: qcom: mdt_loader: Allow hash segment to be split out https://git.kernel.org/qcom/c/8bd42e2341a7 - [04/13] soc: qcom: mdt_loader: Allow hash to reside in any segment https://git.kernel.org/qcom/c/64fb5eb87d58 - [05/13] soc: qcom: mdt_loader: Extend check for split firmware https://git.kernel.org/qcom/c/ea90330fa329 - [06/13] soc: qcom: mdt_loader: Reorder parts of __qcom_mdt_load() https://git.kernel.org/qcom/c/75d7213ce191 - [07/13] soc: qcom: mdt_loader: Always invoke PAS mem_setup https://git.kernel.org/qcom/c/ebeb20a9cd3f - [08/13] soc: qcom: mdt_loader: Extract PAS operations https://git.kernel.org/qcom/c/f4e526ff7e38 - [09/13] remoteproc: qcom: pas: Carry PAS metadata context (no matching commit) - [10/13] dt-bindings: remoteproc: qcom: pas: Add SM8450 PAS compatibles (no matching commit) - [11/13] remoteproc: qcom: pas: Add SM8450 remoteproc support (no matching commit) - [12/13] arm64: dts: qcom: sm8450: Add remoteproc enablers and instances https://git.kernel.org/qcom/c/e57f31b02784 - [13/13] arm64: dts: qcom: sm8450-qrd: Enable remoteproc instances https://git.kernel.org/qcom/c/b48007d6d098 You are awesome, thank you!
On 28. 01. 2022. 03:55, Bjorn Andersson wrote: > Starting with Qualcomm SM8450, some new security enhancements has been > done in the secure world, which results in the requirement to keep the > metadata segment accessible by the secure world from init_image() until > auth_and_reset(). > > Introduce a "PAS metadata context" object that can be passed to > init_image() for tracking the mapped memory and a related release > function for client drivers to release the mapping once either > auth_and_reset() has been invoked or in error handling paths on the way > there. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Hi Bjorn, I know a lot of time has passed since this patch, but this patch breaks qcom_scm_pas_auth_and_reset SCM call. I have had MDT loader and SCM changes from this series reverted for a long time but was unable to find the exact culprit nor how to fix it. I even ported the TZ log driver to see if anything is obvious there but nope, calling qcom_scm_pas_auth_and_reset after this patch is just failling which is then causing Q6 WCSS on IPQ8074 to fail probing so WLAN doesnt work. Since I am out of ideas, I was hoping that you maybe have an idea what could be the issue, I can provide the TZ log with all of the SCM calls when it works and when not if required. Regards, Robert > --- > drivers/firmware/qcom_scm.c | 39 ++++++++++++++++++++++++++++++----- > drivers/soc/qcom/mdt_loader.c | 2 +- > include/linux/qcom_scm.h | 10 ++++++++- > 3 files changed, 44 insertions(+), 7 deletions(-) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 7db8066b19fd..3218d13cbf83 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -435,10 +435,16 @@ static void qcom_scm_set_download_mode(bool enable) > * and optional blob of data used for authenticating the metadata > * and the rest of the firmware > * @size: size of the metadata > + * @ctx: optional metadata context > * > - * Returns 0 on success. > + * Return: 0 on success. > + * > + * Upon successful return, the PAS metadata context (@ctx) will be used to > + * track the metadata allocation, this needs to be released by invoking > + * qcom_scm_pas_metadata_release() by the caller. > */ > -int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size) > +int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size, > + struct qcom_scm_pas_metadata *ctx) > { > dma_addr_t mdata_phys; > void *mdata_buf; > @@ -467,7 +473,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size) > > ret = qcom_scm_clk_enable(); > if (ret) > - goto free_metadata; > + goto out; > > desc.args[1] = mdata_phys; > > @@ -475,13 +481,36 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size) > > qcom_scm_clk_disable(); > > -free_metadata: > - dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys); > +out: > + if (ret < 0 || !ctx) { > + dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys); > + } else if (ctx) { > + ctx->ptr = mdata_buf; > + ctx->phys = mdata_phys; > + ctx->size = size; > + } > > return ret ? : res.result[0]; > } > EXPORT_SYMBOL(qcom_scm_pas_init_image); > > +/** > + * qcom_scm_pas_metadata_release() - release metadata context > + * @ctx: metadata context > + */ > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx) > +{ > + if (!ctx->ptr) > + return; > + > + dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys); > + > + ctx->ptr = NULL; > + ctx->phys = 0; > + ctx->size = 0; > +} > +EXPORT_SYMBOL(qcom_scm_pas_metadata_release); > + > /** > * qcom_scm_pas_mem_setup() - Prepare the memory related to a given peripheral > * for firmware loading > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c > index 72fc2b539213..b00586db5391 100644 > --- a/drivers/soc/qcom/mdt_loader.c > +++ b/drivers/soc/qcom/mdt_loader.c > @@ -171,7 +171,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw, > goto out; > } > > - ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len); > + ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, NULL); > > kfree(metadata); > if (ret) { > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h > index 81cad9e1e412..4d8371410b05 100644 > --- a/include/linux/qcom_scm.h > +++ b/include/linux/qcom_scm.h > @@ -68,8 +68,16 @@ extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus); > extern void qcom_scm_cpu_power_down(u32 flags); > extern int qcom_scm_set_remote_state(u32 state, u32 id); > > +struct qcom_scm_pas_metadata { > + void *ptr; > + dma_addr_t phys; > + ssize_t size; > +}; > + > extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, > - size_t size); > + size_t size, > + struct qcom_scm_pas_metadata *ctx); > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx); > extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, > phys_addr_t size); > extern int qcom_scm_pas_auth_and_reset(u32 peripheral);