Re: [PATCH v2 2/5] remoteproc: imx_rproc: Add runtime ops copy to support dynamic behavior

[Iuliana Prodan <iuliana.prodan@nxp.com>]

On 10/31/2025 4:24 AM, Peng Fan wrote:
> Structure imx_rproc_dcfg contains a const pointer to imx_rproc_plat_ops,
> which defines the start/stop/detect_mode operations for a remote processor.
> To preserve the const correctness of the static configuration while
> allowing runtime modification of ops behavior, introduce a new
> imx_rproc_plat_ops member in struct imx_rproc named `ops`.
> 
> During initialization, dcfg->ops is assigned to priv->ops.
> Enable the driver to safely override ops at runtime without affecting the
> original const configuration.
> 
> Improve flexibility for platforms that require dynamic operation switching
> (e.g. i.MX95 Logical Machine ops and CPU ops).
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/remoteproc/imx_rproc.c | 27 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 02e155e967942d745de4ccd96f9008e4211f9b36..82cb8c7866ccb79061c46068d2039f38feebbb3c 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -116,6 +116,7 @@ struct imx_rproc {
>   	u32				entry;		/* cpu start address */
>   	u32				core_index;
>   	struct dev_pm_domain_list	*pd_list;
> +	const struct imx_rproc_plat_ops	*ops;
>   };
>   
>   static const struct imx_rproc_att imx_rproc_att_imx93[] = {
> @@ -315,7 +316,6 @@ static int imx_rproc_scu_api_start(struct rproc *rproc)
>   static int imx_rproc_start(struct rproc *rproc)
>   {
>   	struct imx_rproc *priv = rproc->priv;
> -	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>   	struct device *dev = priv->dev;
>   	int ret;
>   
> @@ -323,10 +323,10 @@ static int imx_rproc_start(struct rproc *rproc)
>   	if (ret)
>   		return ret;
>   
> -	if (!dcfg->ops || !dcfg->ops->start)
> +	if (!priv->ops || !priv->ops->start)
>   		return -EOPNOTSUPP;
>   
> -	ret = dcfg->ops->start(rproc);
> +	ret = priv->ops->start(rproc);
>   	if (ret)
>   		dev_err(dev, "Failed to enable remote core!\n");
>   
> @@ -372,14 +372,13 @@ static int imx_rproc_scu_api_stop(struct rproc *rproc)
>   static int imx_rproc_stop(struct rproc *rproc)
>   {
>   	struct imx_rproc *priv = rproc->priv;
> -	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>   	struct device *dev = priv->dev;
>   	int ret;
>   
> -	if (!dcfg->ops || !dcfg->ops->stop)
> +	if (!priv->ops || !priv->ops->stop)
>   		return -EOPNOTSUPP;
>   
> -	ret = dcfg->ops->stop(rproc);
> +	ret = priv->ops->stop(rproc);
>   	if (ret)
>   		dev_err(dev, "Failed to stop remote core\n");
>   	else
> @@ -590,12 +589,11 @@ static int imx_rproc_scu_api_detach(struct rproc *rproc)
>   static int imx_rproc_detach(struct rproc *rproc)
>   {
>   	struct imx_rproc *priv = rproc->priv;
> -	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>   
> -	if (!dcfg->ops || !dcfg->ops->detach)
> +	if (!priv->ops || !priv->ops->detach)
>   		return -EOPNOTSUPP;
>   
> -	return dcfg->ops->detach(rproc);
> +	return priv->ops->detach(rproc);
>   }
>   
>   static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> @@ -995,18 +993,16 @@ static int imx_rproc_scu_api_detect_mode(struct rproc *rproc)
>   
>   static int imx_rproc_detect_mode(struct imx_rproc *priv)
>   {
> -	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> -
>   	/*
>   	 * To i.MX{7,8} ULP, Linux is under control of RTOS, no need
> -	 * dcfg->ops or dcfg->ops->detect_mode, it is state RPROC_DETACHED.
> +	 * priv->ops or priv->ops->detect_mode, it is state RPROC_DETACHED.
>   	 */
> -	if (!dcfg->ops || !dcfg->ops->detect_mode) {
> +	if (!priv->ops || !priv->ops->detect_mode) {
>   		priv->rproc->state = RPROC_DETACHED;
>   		return 0;
>   	}
>   
> -	return dcfg->ops->detect_mode(priv->rproc);
> +	return priv->ops->detect_mode(priv->rproc);
>   }
>   
>   static int imx_rproc_sys_off_handler(struct sys_off_data *data)
> @@ -1056,6 +1052,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
>   	priv->dcfg = dcfg;
>   	priv->dev = dev;
>   
> +	if (dcfg->ops)
> +		priv->ops = dcfg->ops;

Will priv->ops ever be modified after initialization in a multi-threaded 
context?
If so, you might need synchronization.

Also, you can remove the NULL check since it's safe to assign NULL.

Iulia

> +
>   	dev_set_drvdata(dev, rproc);
>   	priv->workqueue = create_workqueue(dev_name(dev));
>   	if (!priv->workqueue) {
>