Re: [PATCH v9 08/26] remoteproc: k3-r5: Refactor sequential core power up/down operations

[Andrew Davis <afd@ti.com>]

On 3/17/25 7:06 AM, Beleswar Padhi wrote:
> The existing implementation of the waiting mechanism in
> "k3_r5_cluster_rproc_init()" waits for the "released_from_reset" flag to
> be set as part of the firmware boot process in "k3_r5_rproc_start()".
> The "k3_r5_cluster_rproc_init()" function is invoked in the probe
> routine which causes unexpected failures in cases where the firmware is
> unavailable at boot time, resulting in probe failure and removal of the
> remoteproc handles in the sysfs paths.
> 
> To address this, the waiting mechanism is refactored out of the probe
> routine into the appropriate "k3_r5_rproc_{prepare/unprepare}()"
> functions. This allows the probe routine to complete without depending
> on firmware booting, while still maintaining the required
> power-synchronization between cores.
> 
> Further, this wait mechanism is dropped from
> "k3_r5_rproc_{start/stop}()" functions as they deal with Core Run/Halt
> operations, and as such, there is no constraint in Running or Halting
> the cores of a cluster in order.
> 
> Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up core1")
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---

Same as the above two patches in this series, these are all valid fixes, but should be
done first before the refactoring begins, so move them to the start of the series.

Andrew

>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 114 +++++++++++++----------
>   1 file changed, 65 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index c0e4da82775d..30081eafbd36 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -475,7 +475,7 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
>   static int k3_r5_rproc_prepare(struct rproc *rproc)
>   {
>   	struct k3_r5_rproc *kproc = rproc->priv;
> -	struct k3_r5_core *core = kproc->priv;
> +	struct k3_r5_core *core = kproc->priv, *core0, *core1;
>   	struct k3_r5_cluster *cluster = core->cluster;
>   	struct device *dev = kproc->dev;
>   	u32 ctrl = 0, cfg = 0, stat = 0;
> @@ -483,6 +483,29 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
>   	bool mem_init_dis;
>   	int ret;
>   
> +	/*
> +	 * R5 cores require to be powered on sequentially, core0 should be in
> +	 * higher power state than core1 in a cluster. So, wait for core0 to
> +	 * power up before proceeding to core1 and put timeout of 2sec. This
> +	 * waiting mechanism is necessary because rproc_auto_boot_callback() for
> +	 * core1 can be called before core0 due to thread execution order.
> +	 *
> +	 * By placing the wait mechanism here in .prepare() ops, this condition
> +	 * is enforced for rproc boot requests from sysfs as well.
> +	 */
> +	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
> +	core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem);
> +	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1 &&
> +	    !core0->released_from_reset) {
> +		ret = wait_event_interruptible_timeout(cluster->core_transition,
> +						       core0->released_from_reset,
> +						       msecs_to_jiffies(2000));
> +		if (ret <= 0) {
> +			dev_err(dev, "can not power up core1 before core0");
> +			return -EPERM;
> +		}
> +	}
> +
>   	ret = ti_sci_proc_get_status(kproc->tsp, &boot_vec, &cfg, &ctrl, &stat);
>   	if (ret < 0)
>   		return ret;
> @@ -498,6 +521,14 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
>   		return ret;
>   	}
>   
> +	/*
> +	 * Notify all threads in the wait queue when core0 state has changed so
> +	 * that threads waiting for this condition can be executed.
> +	 */
> +	core->released_from_reset = true;
> +	if (core == core0)
> +		wake_up_interruptible(&cluster->core_transition);
> +
>   	/*
>   	 * Newer IP revisions like on J7200 SoCs support h/w auto-initialization
>   	 * of TCMs, so there is no need to perform the s/w memzero. This bit is
> @@ -542,11 +573,31 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
>   static int k3_r5_rproc_unprepare(struct rproc *rproc)
>   {
>   	struct k3_r5_rproc *kproc = rproc->priv;
> -	struct k3_r5_core *core = kproc->priv;
> +	struct k3_r5_core *core = kproc->priv, *core0, *core1;
>   	struct k3_r5_cluster *cluster = core->cluster;
>   	struct device *dev = kproc->dev;
>   	int ret;
>   
> +	/*
> +	 * Ensure power-down of cores is sequential in split mode. Core1 must
> +	 * power down before Core0 to maintain the expected state. By placing
> +	 * the wait mechanism here in .unprepare() ops, this condition is
> +	 * enforced for rproc stop or shutdown requests from sysfs and device
> +	 * removal as well.
> +	 */
> +	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
> +	core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem);
> +	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core0 &&
> +	    core1->released_from_reset) {
> +		ret = wait_event_interruptible_timeout(cluster->core_transition,
> +						       !core1->released_from_reset,
> +						       msecs_to_jiffies(2000));
> +		if (ret <= 0) {
> +			dev_err(dev, "can not power down core0 before core1");
> +			return -EPERM;
> +		}
> +	}
> +
>   	/* Re-use LockStep-mode reset logic for Single-CPU mode */
>   	ret = (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
>   	       cluster->mode == CLUSTER_MODE_SINGLECPU) ?
> @@ -554,6 +605,14 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
>   	if (ret)
>   		dev_err(dev, "unable to disable cores, ret = %d\n", ret);
>   
> +	/*
> +	 * Notify all threads in the wait queue when core1 state has changed so
> +	 * that threads waiting for this condition can be executed.
> +	 */
> +	core->released_from_reset = false;
> +	if (core == core1)
> +		wake_up_interruptible(&cluster->core_transition);
> +
>   	return ret;
>   }
>   
> @@ -577,7 +636,7 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
>   static int k3_r5_rproc_start(struct rproc *rproc)
>   {
>   	struct k3_r5_rproc *kproc = rproc->priv;
> -	struct k3_r5_core *core0, *core = kproc->priv;
> +	struct k3_r5_core *core = kproc->priv;
>   	struct k3_r5_cluster *cluster = core->cluster;
>   	struct device *dev = kproc->dev;
>   	u32 boot_addr;
> @@ -600,21 +659,9 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>   				goto unroll_core_run;
>   		}
>   	} else {
> -		/* do not allow core 1 to start before core 0 */
> -		core0 = list_first_entry(&cluster->cores, struct k3_r5_core,
> -					 elem);
> -		if (core != core0 && core0->kproc->rproc->state == RPROC_OFFLINE) {
> -			dev_err(dev, "%s: can not start core 1 before core 0\n",
> -				__func__);
> -			return -EPERM;
> -		}
> -
> -		ret = k3_r5_core_run(core->kproc);
> +		ret = k3_r5_core_run(kproc);
>   		if (ret)
>   			return ret;
> -
> -		core->released_from_reset = true;
> -		wake_up_interruptible(&cluster->core_transition);
>   	}
>   
>   	return 0;
> @@ -654,9 +701,8 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>   static int k3_r5_rproc_stop(struct rproc *rproc)
>   {
>   	struct k3_r5_rproc *kproc = rproc->priv;
> -	struct k3_r5_core *core1, *core = kproc->priv;
> +	struct k3_r5_core *core = kproc->priv;
>   	struct k3_r5_cluster *cluster = core->cluster;
> -	struct device *dev = kproc->dev;
>   	int ret;
>   
>   	/* halt all applicable cores */
> @@ -669,17 +715,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>   			}
>   		}
>   	} else {
> -		/* do not allow core 0 to stop before core 1 */
> -		core1 = list_last_entry(&cluster->cores, struct k3_r5_core,
> -					elem);
> -		if (core != core1 && core1->kproc->rproc->state != RPROC_OFFLINE) {
> -			dev_err(dev, "%s: can not stop core 0 before core 1\n",
> -				__func__);
> -			ret = -EPERM;
> -			goto out;
> -		}
> -
> -		ret = k3_r5_core_halt(core->kproc);
> +		ret = k3_r5_core_halt(kproc);
>   		if (ret)
>   			goto out;
>   	}
> @@ -1441,26 +1477,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
>   		    cluster->mode == CLUSTER_MODE_SINGLECORE)
>   			break;
> -
> -		/*
> -		 * R5 cores require to be powered on sequentially, core0
> -		 * should be in higher power state than core1 in a cluster
> -		 * So, wait for current core to power up before proceeding
> -		 * to next core and put timeout of 2sec for each core.
> -		 *
> -		 * This waiting mechanism is necessary because
> -		 * rproc_auto_boot_callback() for core1 can be called before
> -		 * core0 due to thread execution order.
> -		 */
> -		ret = wait_event_interruptible_timeout(cluster->core_transition,
> -						       core->released_from_reset,
> -						       msecs_to_jiffies(2000));
> -		if (ret <= 0) {
> -			dev_err(cdev,
> -				"Timed out waiting for %s core to power up!\n",
> -				rproc->name);
> -			goto out;
> -		}
>   	}
>   
>   	return 0;