On Wed, Aug 20, 2025 at 12:48:55PM +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. > > > > 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 <[email protected]> > > --- > > If this approach is preferred, will convert all subsystem drivers to use the > > same set of API's using context and completely get away with qcom_mdt_load() > > > > -Mukesh > > > > drivers/remoteproc/qcom_q6v5_pas.c | 53 ++++++++++++++--------------- > > drivers/soc/qcom/mdt_loader.c | 26 ++++++++++---- > > include/linux/soc/qcom/mdt_loader.h | 22 ++++++------ > > 3 files changed, 56 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c > > b/drivers/remoteproc/qcom_q6v5_pas.c > > index 55a7da801183..e376c0338576 100644 > > --- a/drivers/remoteproc/qcom_q6v5_pas.c > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > > @@ -115,8 +115,8 @@ struct qcom_pas { > > struct qcom_rproc_ssr ssr_subdev; > > struct qcom_sysmon *sysmon; > > - struct qcom_scm_pas_metadata pas_metadata; > > - struct qcom_scm_pas_metadata dtb_pas_metadata; > > + struct qcom_scm_pas_ctx *pas_ctx; > > + struct qcom_scm_pas_ctx *dtb_pas_ctx; > > }; > > static void qcom_pas_segment_dump(struct rproc *rproc, > > @@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc) > > * auth_and_reset() was successful, but in other cases clean it up > > * here. > > */ > > - qcom_scm_pas_metadata_release(&pas->pas_metadata); > > + qcom_scm_pas_metadata_release(pas->pas_ctx->metadata); > > if (pas->dtb_pas_id) > > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata); > > return 0; > > } > > @@ -235,15 +235,8 @@ static int qcom_pas_load(struct rproc *rproc, const > > struct firmware *fw) > > return ret; > > } > > - 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); > > if (ret) > > goto release_dtb_metadata; > > } > > @@ -251,9 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const > > struct firmware *fw) > > return 0; > > release_dtb_metadata: > > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > > - > > -release_dtb_firmware: > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata); > > release_firmware(pas->dtb_firmware); > > return ret; > > @@ -301,14 +292,8 @@ static int qcom_pas_start(struct rproc *rproc) > > } > > } > > - ret = qcom_mdt_pas_init(pas->dev, pas->firmware, rproc->firmware, > > pas->pas_id, > > - pas->mem_phys, &pas->pas_metadata); > > - if (ret) > > - goto disable_px_supply; > > - > > - ret = qcom_mdt_load_no_init(pas->dev, pas->firmware, rproc->firmware, > > - pas->mem_region, pas->mem_phys, > > pas->mem_size, > > - &pas->mem_reloc); > > + ret = qcom_mdt_pas_load(pas->pas_ctx, pas->firmware, rproc->firmware, > > + pas->mem_region, &pas->dtb_mem_reloc); > > if (ret) > > goto release_pas_metadata; > > @@ -328,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc) > > goto release_pas_metadata; > > } > > - qcom_scm_pas_metadata_release(&pas->pas_metadata); > > + qcom_scm_pas_metadata_release(pas->pas_ctx->metadata); > > if (pas->dtb_pas_id) > > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata); > > /* firmware is used to pass reference from qcom_pas_start(), drop it > > now */ > > pas->firmware = NULL; > > @@ -338,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc) > > return 0; > > release_pas_metadata: > > - qcom_scm_pas_metadata_release(&pas->pas_metadata); > > + qcom_scm_pas_metadata_release(pas->pas_ctx->metadata); > > if (pas->dtb_pas_id) > > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata); > > disable_px_supply: > > if (pas->px_supply) > > regulator_disable(pas->px_supply); > > @@ -774,6 +759,18 @@ static int qcom_pas_probe(struct platform_device *pdev) > > } > > qcom_add_ssr_subdev(rproc, &pas->ssr_subdev, desc->ssr_name); > > + > > + pas->pas_ctx = qcom_scm_pas_ctx_init(pas->dev, pas->pas_id, > > pas->mem_phys, > > + pas->mem_size, true); > > + if (!pas->pas_ctx) > > + goto remove_ssr_sysmon; > > this function already returns -ENOMEM you don't set ret to any particular > value so if qcom_scm_pas_ctx_init() returned NULL, you would exit your probe > function here "error out" with returning ret = 0
Ack. > > Please ERR_PTR() in qcom_scm_pas_ctx_init() and return the error up the call > stack via your remove_ssr_sysmon jump label. Sure. > > --- > bod -- -Mukesh Ojha
