On Wed, Aug 20, 2025 at 12:40:51PM +0100, Bryan O'Donoghue wrote:
> 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.
> >
> > Unify the metadata loading process for both remoteproc and
> > non-remoteproc subsystems by introducing a dedicated PAS context
> > initialization function.
>
> You've introduced what PAS is in the cover letter but you haven't done so in
> the commit log where you use it.
>
> "Peripheral Authentication Service (PAS)" should be defined in this patch
> somewhere so we know what PAS means.
Ack.
>
> >
> > By introducing qcom_scm_pas_ctx_init(), we can standardize the API usage
> > across subsystems and reduce the number of parameters passed to MDT
> > loader functions, improving code clarity and maintainability.
> >
> > Signed-off-by: Mukesh Ojha <mukesh.o...@oss.qualcomm.com>
> > ---
> > drivers/firmware/qcom/qcom_scm.c | 26 ++++++++++++++++++++++++++
> > include/linux/firmware/qcom/qcom_scm.h | 11 +++++++++++
> > 2 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm.c
> > b/drivers/firmware/qcom/qcom_scm.c
> > index 96d5cf40a74c..33187d4f4aef 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -558,6 +558,32 @@ static void qcom_scm_set_download_mode(u32 dload_mode)
> > dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
> > }
> > +void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral,
> > phys_addr_t mem_phys,
> > + size_t mem_size, bool save_mdt_ctx)
> > +{
> > + struct qcom_scm_pas_ctx *ctx;
> > +
> > + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return NULL;
> > +
> > + ctx->dev = dev;
> > + ctx->peripheral = peripheral;
> > + ctx->mem_phys = mem_phys;
> > + ctx->mem_size = mem_size;
> > + ctx->save_mdt_ctx = save_mdt_ctx;
> > + ctx->metadata = NULL;
> > +
> > + if (save_mdt_ctx) {
>
> You could check metadata != NULL and drop the bool ctx->save_mdt_ctx
> entirely.
Ack.
>
> ---
> bod
--
-Mukesh Ojha
