Re: [PATCH v2 02/11] soc: qcom: mdtloader: Add context aware qcom_mdt_pas_load() helper

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 19/08/2025 17:54, 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 require it.
> 
> 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. This will also help in unifying the API used by
> remoteproc and non-remoteproc subsystems drivers.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---

I'm struggling with the logic here a little bit.

You take this function which calls qcom_mtd_load_no_init();

>   
> -		ret = qcom_mdt_pas_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name,
> -					pas->dtb_pas_id, pas->dtb_mem_phys,
> -					&pas->dtb_pas_metadata);
> -		if (ret)
> -			goto release_dtb_firmware;
> -
> -		ret = qcom_mdt_load_no_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name,
> -					    pas->dtb_mem_region, pas->dtb_mem_phys,
> -					    pas->dtb_mem_size, &pas->dtb_mem_reloc);
> +		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware, pas->dtb_firmware_name,
> +					pas->dtb_mem_region, &pas->dtb_mem_reloc);

and then turn it into

> +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->peripheral,
> +				  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);

Surely you want to qcom_mdt_load_no_init ?

On current kernel
return __qcom_mdt_load(ctx->dev, fw, firmware, mem_region, ctx,
		       mem_phys, ctx->mem_size,
		       reloc_base, true);

but that's a functional change

> +}
> +EXPORT_SYMBOL_GPL(qcom_mdt_pas_load);
the no_init version of this looks like this:

int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
                           const char *firmware, int pas_id,
                           void *mem_region, phys_addr_t mem_phys,
                           size_t mem_size, phys_addr_t *reloc_base)
{
         return __qcom_mdt_load(dev, fw, firmware, pas_id, mem_region,
			       mem_phys, mem_size, reloc_base, false);
}
EXPORT_SYMBOL_GPL(qcom_mdt_load_no_init);

So is it really the intention of this patch to change the callsites 
where qcom_mdt_pas_load() away from the init version to the no_init 
version ?

Maybe its a symptom of patch collision but hmm, having a hard time 
cherry-picking this to test.

---
bod