Re: [PATCH v3 04/12] soc: qcom: mdtloader: Add context aware qcom_mdt_pas_load() helper

Bryan O'Donoghue
2025-09-21T21:50:02Z

On 20/09/2025 20:41, Mukesh Ojha wrote:
> Currently, remoteproc and non-remoteproc subsystems use different
> variants of the MDT loader helper API, primarily due to the handling of
> the metadata context. Remoteproc subsystems retain this context until
> authentication and reset, while non-remoteproc subsystems (e.g., video,
> graphics) do not need to retain it and it is freed inside
> qcom_scm_pas_init() based on metadata context parameter being NULL.

The above paragraph seems almost 1:1 with the commit log of the previous
patch.

You'd be better off describing in the commit log what you mean by
context here. Not a thread context which is the common meaning of
context. Instead of restating the paragraph from your previous commit,
give the reader of your commit log new information - for example - what
do you mean by context here ?

> Add context aware qcom_mdt_pas_load() function which uses context
> returned from qcom_scm_pas_ctx_init() and use it till subsystems
> is out of set.

"Add a context function qcom_mdt_pas_load() which uses a context
pointer/handle? return from qcom_scm_pas_init() then use that handle
until? a subsystem is out of reset ?

Your commit log doesn't match the code below, which simply provides the
API you will then subsequently build on.

This will also help in unifying the API used by
> remoteproc and non-remoteproc subsystems drivers in future and
> will also help in early setting context to all the PAS SMC function
> to do appropriate things when SoC is running with gunyah hypervisor
> or in absence of it.

Your argument for unification should go into the cover letter - this
specific commit is adding a function - not performing that unification
so in my opinion - as a reviewer I should see this argument for
unification @ the cover-letter level and/or at the commit which
implements the unification not the commit that adds the API to
facilitate the unification.
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
> drivers/soc/qcom/mdt_loader.c | 15 +++++++++++++++
> include/linux/soc/qcom/mdt_loader.h | 10 ++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index a5c80d4fcc36..2ef079797f05 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -486,5 +486,20 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
> }
> EXPORT_SYMBOL_GPL(qcom_mdt_load_no_init);
>
> +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> + const char *firmware, void *mem_region, phys_addr_t *reloc_base)
> +{
> + int ret;
> +
> + ret = qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->pas_id,
> + ctx->mem_phys, ctx->metadata);
> + if (ret)
> + return ret;
> +
> + return __qcom_mdt_load(ctx->dev, fw, firmware, mem_region, ctx->mem_phys,
> + ctx->mem_size, reloc_base);
> +}
> +EXPORT_SYMBOL_GPL(qcom_mdt_pas_load);
> +
> MODULE_DESCRIPTION("Firmware parser for Qualcomm MDT format");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index 8ea8230579a2..36b8b331ce5f 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -11,6 +11,7 @@
> struct device;
> struct firmware;
> struct qcom_scm_pas_metadata;
> +struct qcom_scm_pas_ctx;
>
> #if IS_ENABLED(CONFIG_QCOM_MDT_LOADER)
>
> @@ -23,6 +24,9 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw,
> phys_addr_t mem_phys, size_t mem_size,
> phys_addr_t *reloc_base);
>
> +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> + const char *firmware, void *mem_region, phys_addr_t *reloc_base);
> +
> int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
> const char *fw_name, void *mem_region,
> phys_addr_t mem_phys, size_t mem_size,
> @@ -52,6 +56,12 @@ static inline int qcom_mdt_load(struct device *dev, const struct firmware *fw,
> return -ENODEV;
> }
>
> +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> + const char *firmware, void *mem_region, phys_addr_t *reloc_base)
> +{
> + return -ENODEV;
> +}
> +
> static inline int qcom_mdt_load_no_init(struct device *dev,
> const struct firmware *fw,
> const char *fw_name, void *mem_region,
>
> --
> 2.50.1
>
>