Re: [EXTERNAL] Re: [PATCH v2 1/2] remoteproc: k3-r5: Wait for core0 power-up before powering up core1

[Beleswar Prasad Padhi]

Hello,

On 26/04/24 22:39, Mathieu Poirier wrote:

Good day, On Wed, Apr 24, 2024 at 06: 35: 03PM +0530, Beleswar Padhi wrote: >
From: Apurva Nandan  > > PSC controller has a limitation that
it can only power-up the second core > when the first core is in ON
ZjQcmQRYFpfptBannerStart
This message was sent from outside of Texas Instruments.
Do not click links or open attachments unless you recognize the source of this
email and know the content is safe. If you wish to report this message to IT
Security, please forward the message as an attachment to phish...@list.ti.com
ZjQcmQRYFpfptBannerEnd

Good day,

On Wed, Apr 24, 2024 at 06:35:03PM +0530, Beleswar Padhi wrote:
> From: Apurva Nandan 
> 
> PSC controller has a limitation that it can only power-up the second core

> when the first core is in ON state. Power-state for core0 should be equal
> to or higher than core1, else the kernel is seen hanging during rproc
> loading.
> 
> Make the powering up of cores sequential, by waiting for the current core

> to power-up before proceeding to the next core, with a timeout of 2sec.
> Add a wait queue event in k3_r5_cluster_rproc_init call, that will wait
> for the current core to be released from reset before proceeding with the
> next core.
> 
> Fixes: 6dedbd1d5443 ("remoteproc: k3-r5: Add a remoteproc driver for R5F subsystem")
> 
> Signed-off-by: Apurva Nandan 


You need to add your own SoB as well.

> ---
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c

> index ad3415a3851b..5a9bd5d4a2ea 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -103,12 +103,14 @@ struct k3_r5_soc_data {
>   * @dev: cached device pointer
>   * @mode: Mode to configure the Cluster - Split or LockStep
>   * @cores: list of R5 cores within the cluster
> + * @core_transition: wait queue to sync core state changes
>   * @soc_data: SoC-specific feature data for a R5FSS
>   */
>  struct k3_r5_cluster {
>struct device *dev;
>enum cluster_mode mode;
>struct list_head cores;
> +  wait_queue_head_t core_transition;
>const struct k3_r5_soc_data *soc_data;
>  };
>  
> @@ -128,6 +130,7 @@ struct k3_r5_cluster {

>   * @atcm_enable: flag to control ATCM enablement
>   * @btcm_enable: flag to control BTCM enablement
>   * @loczrama: flag to dictate which TCM is at device address 0x0
> + * @released_from_reset: flag to signal when core is out of reset
>   */
>  struct k3_r5_core {
>struct list_head elem;
> @@ -144,6 +147,7 @@ struct k3_r5_core {
>u32 atcm_enable;
>u32 btcm_enable;
>u32 loczrama;
> +  bool released_from_reset;
>  };
>  
>  /**

> @@ -460,6 +464,8 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
>ret);
>return ret;
>}
> +  core->released_from_reset = true;
> +  wake_up_interruptible(&cluster->core_transition);
>  
>  	/*

> * Newer IP revisions like on J7200 SoCs support h/w auto-initialization
> @@ -1140,6 +1146,7 @@ static int k3_r5_rproc_configure_mode(struct 
k3_r5_rproc *kproc)
>return ret;
>}
>  
> +	core->released_from_reset = c_state;


I understand why this is needed but it line could be very cryptic for people
trying to understand this driver.  Please add a comment to describe what is
happening here.

Thanks for the review. I will send v3 addressing these comments shortly!


>ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl,
> &stat);
>if (ret < 0) {
> @@ -1280,6 +1287,26 @@ 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(dev,
> +  "Timed out waiting for %s core to power up!\n",
> +  rproc->name);
> +  return ret;
> +  }
>}
>  
>  	return 0;

> @@ -1709,6 +1736,7 @@ static int k3_r5_probe(struct platform_device *pdev)
>clu

Re: [PATCH 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe

[Beleswar Prasad Padhi]

Hi Andrew,

On 30/05/24 19:46, Andrew Davis wrote:

On 5/30/24 4:07 AM, Beleswar Padhi wrote:

Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 66 
  1 file changed, 21 insertions(+), 45 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index 26362a509ae3..157e8fd57665 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -391,6 +391,7 @@ static int k3_r5_rproc_request_mbox(struct rproc 
*rproc)

  struct mbox_client *client = &kproc->client;
  struct device *dev = kproc->dev;
  int ret;
+    long err;
    client->dev = dev;
  client->tx_done = NULL;
@@ -400,10 +401,9 @@ static int k3_r5_rproc_request_mbox(struct rproc 
*rproc)

    kproc->mbox = mbox_request_channel(client, 0);
  if (IS_ERR(kproc->mbox)) {
-    ret = -EBUSY;
-    dev_err(dev, "mbox_request_channel failed: %ld\n",
-    PTR_ERR(kproc->mbox));
-    return ret;
+    err = PTR_ERR(kproc->mbox);
+    dev_err(dev, "mbox_request_channel failed: %ld\n", err);
+    return (err == -EPROBE_DEFER) ? -EPROBE_DEFER : -EBUSY;


Why turn all other errors into EBUSY? If you just return the error 
as-is you

can simply make these 3 lines just:

return dev_err_probe(dev, PTR_ERR(kproc->mbox), "mbox_request_channel 
failed\n");



  }
    /*
@@ -552,10 +552,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  u32 boot_addr;
  int ret;
  -    ret = k3_r5_rproc_request_mbox(rproc);
-    if (ret)
-    return ret;
-
  boot_addr = rproc->bootaddr;
  /* TODO: add boot_addr sanity checking */
  dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", 
boot_addr);

@@ -564,7 +560,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  core = kproc->core;
  ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
  if (ret)
-    goto put_mbox;
+    goto out;


The label "out" doesn't do anything, just directly `return ret;` here and
in the other cases below.


    /* unhalt/run all applicable cores */
  if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
@@ -581,12 +577,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  dev_err(dev, "%s: can not start core 1 before core 0\n",
  __func__);
  ret = -EPERM;
-    goto put_mbox;
+    goto out;
  }
    ret = k3_r5_core_run(core);
  if (ret)
-    goto put_mbox;
+    goto out;
  }
    return 0;
@@ -596,8 +592,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  if (k3_r5_core_halt(core))
  dev_warn(core->dev, "core halt back failed\n");
  }
-put_mbox:
-    mbox_free_channel(kproc->mbox);
+out:
  return ret;
  }
  @@ -658,8 +653,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  goto out;
  }
  -    mbox_free_channel(kproc->mbox);
-
  return 0;
    unroll_core_halt:
@@ -674,42 +667,21 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  /*
   * Attach to a running R5F remote processor (IPC-only mode)
   *
- * The R5F attach callback only needs to request the mailbox, the 
remote

- * processor is already booted, so there is no need to issue any TI-SCI
- * commands to boot the R5F cores in IPC-only mode. This callback is 
invoked

- * only in IPC-only mode.
+ * The R5F attach callback is a NOP. The remote processor is already 
booted, and
+ * all required resources have been acquired during probe routine, 
so there is
+ * no need to issue any TI-SCI commands to boot the R5F cores in 
IPC-only mode.
+ * This callback is invoked only in IPC-only mode and exists for 
sanity sake.

   */
-static int k3_r5_rproc_attach(struct rproc *rproc)
-{
-    struct k3_r5_rproc *kproc = rproc->priv;
-    struct device *dev = kproc->dev;
-    int ret;
-
-    ret = k3_r5_rproc_request_mbox(rproc);
-    if (ret)
-    return ret;
-
-    dev_info(dev, "R5F core initialized in IPC-only mode\n");
-    return 0;
-}
+static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
    /*
   * Detach from a running R5F remote processor (IPC-only mode)
   *
- * The R5F detach callback performs the opposite operation to attach 
callback
- * and only needs to release the mailbox, the R5F cores are not 
stopped and
- * will be left in booted state in IPC-only mode. This callback is 
invoked

- * only in IPC-only mode.
+ * The R5F detach callback is a NOP. The R5F cores are not stopped 
and will be
+ * left in booted state in IPC-only mode. This callback is invoked 
only in

+ * IPC-only mode and exists for sanity sake.
   */
-sta

Re: [PATCH v2 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe

[Beleswar Prasad Padhi]

Hi Andrew,

On 04/06/24 22:40, Andrew Davis wrote:

On 6/4/24 12:17 AM, Beleswar Padhi wrote:

Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 74 +---
  1 file changed, 26 insertions(+), 48 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index 26362a509ae3c..7e02e3472ce25 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -194,6 +194,10 @@ static void k3_r5_rproc_mbox_callback(struct 
mbox_client *client, void *data)

  const char *name = kproc->rproc->name;
  u32 msg = omap_mbox_message(data);
  +    /* Do not forward message to a detached core */


s/to/from

This is the receive side from the core.


+    if (kproc->rproc->state == RPROC_DETACHED)
+    return;
+


Do we need a similar check when sending messages to the core in
k3_r5_rproc_kick()? No one should be sending anything as they
all should have detached at this point, but something to double
check on.


  dev_dbg(dev, "mbox msg: 0x%x\n", msg);
    switch (msg) {
@@ -399,12 +403,9 @@ static int k3_r5_rproc_request_mbox(struct rproc 
*rproc)

  client->knows_txdone = false;
    kproc->mbox = mbox_request_channel(client, 0);
-    if (IS_ERR(kproc->mbox)) {
-    ret = -EBUSY;
-    dev_err(dev, "mbox_request_channel failed: %ld\n",
-    PTR_ERR(kproc->mbox));
-    return ret;
-    }
+    if (IS_ERR(kproc->mbox))
+    return dev_err_probe(dev, PTR_ERR(kproc->mbox),
+ "mbox_request_channel failed\n");


This is good cleanup, but maybe something for its own patch.


    /*
   * Ping the remote processor, this is only for sanity-sake for 
now;

@@ -552,10 +553,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  u32 boot_addr;
  int ret;
  -    ret = k3_r5_rproc_request_mbox(rproc);
-    if (ret)
-    return ret;
-
  boot_addr = rproc->bootaddr;
  /* TODO: add boot_addr sanity checking */
  dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", 
boot_addr);

@@ -564,7 +561,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  core = kproc->core;
  ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
  if (ret)
-    goto put_mbox;
+    return ret;
    /* unhalt/run all applicable cores */
  if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
@@ -580,13 +577,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
  dev_err(dev, "%s: can not start core 1 before core 0\n",
  __func__);
-    ret = -EPERM;
-    goto put_mbox;
+    return -EPERM;
  }
    ret = k3_r5_core_run(core);
  if (ret)
-    goto put_mbox;
+    return ret;
  }
    return 0;
@@ -596,8 +592,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  if (k3_r5_core_halt(core))
  dev_warn(core->dev, "core halt back failed\n");
  }
-put_mbox:
-    mbox_free_channel(kproc->mbox);
  return ret;
  }
  @@ -658,8 +652,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  goto out;
  }
  -    mbox_free_channel(kproc->mbox);
-
  return 0;
    unroll_core_halt:
@@ -674,42 +666,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  /*
   * Attach to a running R5F remote processor (IPC-only mode)
   *
- * The R5F attach callback only needs to request the mailbox, the 
remote

- * processor is already booted, so there is no need to issue any TI-SCI
- * commands to boot the R5F cores in IPC-only mode. This callback is 
invoked

- * only in IPC-only mode.
+ * The R5F attach callback is a NOP. The remote processor is already 
booted, and
+ * all required resources have been acquired during probe routine, 
so there is
+ * no need to issue any TI-SCI commands to boot the R5F cores in 
IPC-only mode.

+ * This callback is invoked only in IPC-only mode and exists because
+ * rproc_validate() checks for its existence.
   */
-static int k3_r5_rproc_attach(struct rproc *rproc)
-{
-    struct k3_r5_rproc *kproc = rproc->priv;
-    struct device *dev = kproc->dev;
-    int ret;
-
-    ret = k3_r5_rproc_request_mbox(rproc);
-    if (ret)
-    return ret;
-
-    dev_info(dev, "R5F core initialized in IPC-only mode\n");
-    return 0;
-}
+static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }


I wonder if rproc_validate() should be updated to allow not
having an attach/detach for cases like this. Then we could drop
this function completely.



Not sure if we can update rproc_validate() for this usecase. Idea

Re: [PATCH v2 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe

[Beleswar Prasad Padhi]

Hi Andrew,

On 04/06/24 22:40, Andrew Davis wrote:

On 6/4/24 12:17 AM, Beleswar Padhi wrote:

Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 74 +---
  1 file changed, 26 insertions(+), 48 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index 26362a509ae3c..7e02e3472ce25 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -194,6 +194,10 @@ static void k3_r5_rproc_mbox_callback(struct 
mbox_client *client, void *data)

  const char *name = kproc->rproc->name;
  u32 msg = omap_mbox_message(data);
  +    /* Do not forward message to a detached core */


s/to/from

This is the receive side from the core.


+    if (kproc->rproc->state == RPROC_DETACHED)
+    return;
+


Do we need a similar check when sending messages to the core in
k3_r5_rproc_kick()? No one should be sending anything as they
all should have detached at this point, but something to double
check on.

Will add this in the next revision.



  dev_dbg(dev, "mbox msg: 0x%x\n", msg);
    switch (msg) {
@@ -399,12 +403,9 @@ static int k3_r5_rproc_request_mbox(struct rproc 
*rproc)

  client->knows_txdone = false;
    kproc->mbox = mbox_request_channel(client, 0);
-    if (IS_ERR(kproc->mbox)) {
-    ret = -EBUSY;
-    dev_err(dev, "mbox_request_channel failed: %ld\n",
-    PTR_ERR(kproc->mbox));
-    return ret;
-    }
+    if (IS_ERR(kproc->mbox))
+    return dev_err_probe(dev, PTR_ERR(kproc->mbox),
+ "mbox_request_channel failed\n");


This is good cleanup, but maybe something for its own patch.
I think this cleanup is dependent to this patch itself. The current 
patch moves the mbox_handle_request to probe routine. And the cleanup 
returns an -EDEFER_PROBE ERR code. So, this cleanup is only valid if the 
current patch is applied. Else, if this err code is returned at any 
point after creation of child devices, it could lead to a infinite 
loop[0]. Please correct me if I am wrong..?


[0]: 
https://www.kernel.org/doc/html/v6.5-rc3/driver-api/driver-model/driver.html#callbacks



    /*
   * Ping the remote processor, this is only for sanity-sake for 
now;

@@ -552,10 +553,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  u32 boot_addr;
  int ret;
  -    ret = k3_r5_rproc_request_mbox(rproc);
-    if (ret)
-    return ret;
-
  boot_addr = rproc->bootaddr;
  /* TODO: add boot_addr sanity checking */
  dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", 
boot_addr);

@@ -564,7 +561,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  core = kproc->core;
  ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
  if (ret)
-    goto put_mbox;
+    return ret;
    /* unhalt/run all applicable cores */
  if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
@@ -580,13 +577,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
  dev_err(dev, "%s: can not start core 1 before core 0\n",
  __func__);
-    ret = -EPERM;
-    goto put_mbox;
+    return -EPERM;
  }
    ret = k3_r5_core_run(core);
  if (ret)
-    goto put_mbox;
+    return ret;
  }
    return 0;
@@ -596,8 +592,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  if (k3_r5_core_halt(core))
  dev_warn(core->dev, "core halt back failed\n");
  }
-put_mbox:
-    mbox_free_channel(kproc->mbox);
  return ret;
  }
  @@ -658,8 +652,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  goto out;
  }
  -    mbox_free_channel(kproc->mbox);
-
  return 0;
    unroll_core_halt:
@@ -674,42 +666,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  /*
   * Attach to a running R5F remote processor (IPC-only mode)
   *
- * The R5F attach callback only needs to request the mailbox, the 
remote

- * processor is already booted, so there is no need to issue any TI-SCI
- * commands to boot the R5F cores in IPC-only mode. This callback is 
invoked

- * only in IPC-only mode.
+ * The R5F attach callback is a NOP. The remote processor is already 
booted, and
+ * all required resources have been acquired during probe routine, 
so there is
+ * no need to issue any TI-SCI commands to boot the R5F cores in 
IPC-only mode.

+ * This callback is invoked only in IPC-only mode and exists because
+ * rproc_validate() checks for its existence.
   */
-static int k3_r5_rproc_attach(struct rproc *rproc)
-{
-    struct k3_r5_r

Re: [PATCH v3 1/3] remoteproc: k3-r5: Use devm_rproc_alloc() helper

[Beleswar Prasad Padhi]

Hi Andrew,

On 07/08/24 19:07, Andrew Davis wrote:

On 8/7/24 1:22 AM, Beleswar Padhi wrote:

Use the device lifecycle managed allocation function. This helps prevent
mistakes like freeing out of order in cleanup functions and forgetting
to free on error paths.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index 39a47540c590..dbcd8840ae8d 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1259,8 +1259,8 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

  goto out;
  }
  -    rproc = rproc_alloc(cdev, dev_name(cdev), &k3_r5_rproc_ops,
-    fw_name, sizeof(*kproc));
+    rproc = devm_rproc_alloc(cdev, dev_name(cdev), 
&k3_r5_rproc_ops,

+ fw_name, sizeof(*kproc));
  if (!rproc) {
  ret = -ENOMEM;
  goto out;
@@ -1352,7 +1352,6 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

  err_add:
  k3_r5_reserved_mem_exit(kproc);
  err_config:
-    rproc_free(rproc);
  core->rproc = NULL;


I'd remove this line too, we don't check for NULL later, we check
core->rproc->status, which will cause a nullptr dereference error
instead of correctly detecting that the core is "offline".



Thanks for catching this! Will post revision with this change.



Same below.

Andrew


  out:
  /* undo core0 upon any failures on core1 in split-mode */
@@ -1399,7 +1398,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
    k3_r5_reserved_mem_exit(kproc);
  -    rproc_free(rproc);
  core->rproc = NULL;
  }
  }

Re: [PATCH v3 3/3] remoteproc: k3-dsp: Acquire mailbox handle during probe routine

[Beleswar Prasad Padhi]

On 07/08/24 19:21, Andrew Davis wrote:

On 8/7/24 1:22 AM, Beleswar Padhi wrote:

Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.

Fixes: b8431920391d ("remoteproc: k3-dsp: Add support for IPC-only 
mode for all K3 DSPs")


Not sure this patch counts as a "fix". There was a bug yes, and this 
certainly
is an improvment that solves the issue, but I like to reserve "Fixes" 
tags
for more serious issues. Otherwise this patch will be backported to 
"stable"
versions where it might cause things to be *less stable* given the 
size of



Understood. Will remove Fixes tag in revision.


the "fix". Also, the commit you selected isn't the source of the issue,
it only adds another instance of it, getting the mailbox after probe has
been the case since the first version of this driver.



Correct. My idea was, since we are also making k3_attach()/detach() 
functions NOP, select the later commit because that was where the 
k3_attach()/detach() functions were introduced as well as the "mailbox 
after probe" issue was present.




Rest of the patch LGTM,

Acked-by: Andrew Davis 

BTW, I've folded this change (getting mbox in probe) into the new
K3 M4 driver[0] I just posted, so we should be aligned here accross
all K3 rproc drivers.



Thanks for this!




Andrew

[0] 
https://lore.kernel.org/linux-arm-kernel/20240802152109.137243-4-...@ti.com/

[...]

Re: [PATCH v4 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe routine

[Beleswar Prasad Padhi]

Hi Mathieu,

On 14-08-2024 21:22, Mathieu Poirier wrote:
Hi Beleswar, On Thu, Aug 08, 2024 at 01: 11: 26PM +0530, Beleswar 
Padhi wrote: > Acquire the mailbox handle during device probe and do 
not release handle > in stop/detach routine or error paths. This 
removes the redundant > requests for

ZjQcmQRYFpfptBannerStart
Report Suspicious
 


ZjQcmQRYFpfptBannerEnd
Hi Beleswar,

On Thu, Aug 08, 2024 at 01:11:26PM +0530, Beleswar Padhi wrote:
> Acquire the mailbox handle during device probe and do not release handle
> in stop/detach routine or error paths. This removes the redundant
> requests for mbox handle later during rproc start/attach. This also
> allows to defer remoteproc driver's probe if mailbox is not probed yet.
> 
> Signed-off-by: Beleswar Padhi 

> ---
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 78 +---
>  1 file changed, 30 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c

> index 57067308b3c0..8a63a9360c0f 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -194,6 +194,10 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client 
*client, void *data)
>const char *name = kproc->rproc->name;
>u32 msg = omap_mbox_message(data);
>  
> +	/* Do not forward message from a detached core */

> +  if (kproc->rproc->state == RPROC_DETACHED)
> +  return;
> +
>dev_dbg(dev, "mbox msg: 0x%x\n", msg);
>  
>  	switch (msg) {

> @@ -229,6 +233,10 @@ static void k3_r5_rproc_kick(struct rproc *rproc, int 
vqid)
>mbox_msg_t msg = (mbox_msg_t)vqid;
>int ret;
>  
> +	/* Do not forward message to a detached core */

> +  if (kproc->rproc->state == RPROC_DETACHED)
> +  return;
> +
>/* send the index of the triggered virtqueue in the mailbox payload */
>ret = mbox_send_message(kproc->mbox, (void *)msg);
>if (ret < 0)
> @@ -399,12 +407,9 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
>client->knows_txdone = false;
>  
>  	kproc->mbox = mbox_request_channel(client, 0);

> -  if (IS_ERR(kproc->mbox)) {
> -  ret = -EBUSY;
> -  dev_err(dev, "mbox_request_channel failed: %ld\n",
> -  PTR_ERR(kproc->mbox));
> -  return ret;
> -  }
> +  if (IS_ERR(kproc->mbox))
> +  return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> +   "mbox_request_channel failed\n");
>  
>  	/*

> * Ping the remote processor, this is only for sanity-sake for now;
> @@ -552,10 +557,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>u32 boot_addr;
>int ret;
>  
> -	ret = k3_r5_rproc_request_mbox(rproc);

> -  if (ret)
> -  return ret;
> -
>boot_addr = rproc->bootaddr;
>/* TODO: add boot_addr sanity checking */
>dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", boot_addr);
> @@ -564,7 +565,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>core = kproc->core;
>ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
>if (ret)
> -  goto put_mbox;
> +  return ret;
>  
>  	/* unhalt/run all applicable cores */

>if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> @@ -580,13 +581,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
>dev_err(dev, "%s: can not start core 1 before core 0\n",
>__func__);
> -  ret = -EPERM;
> -  goto put_mbox;
> +  return -EPERM;
>}
>  
>  		ret = k3_r5_core_run(core);

>if (ret)
> -  goto put_mbox;
> +  return ret;
>}
>  
>  	return 0;

> @@ -596,8 +596,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>if (k3_r5_core_halt(core))
>dev_warn(core->dev, "core halt back failed\n");
>}
> -put_mbox:
> -  mbox_free_channel(kproc->mbox);
>return ret;
>  }
>  
> @@ -658,8 +656,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)

>goto out;
>}
>  
> -	mbox_free_channel(kproc->mbox);

> -
>return 0;
>  
>  unroll_core_halt:

> @@ -674,42 +670,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>  /*
>   * Attach to a running R5F remote processor (IPC-only mode)
>   *
> - * The R5F attach callback only needs to request the mailbox, the remote
> - * processor is already booted, so there is no need to issue any TI-SCI
> - * commands to boot the R5F cores in IPC-only mode. This callback is invoked
> - * only in IPC-only mode.
> + * The R5F attach callback is a NOP. The remote processor is already booted, 
and
> + * all required resources have been acquired during probe routine, so there 
is
> + * no need to issue any TI-SCI commands to boot the R5F c

Re: [PATCH] remoteproc: k3-r5: Fix driver shutdown

[Beleswar Prasad Padhi]

Hi Jan,

On 19-08-2024 22:17, Jan Kiszka wrote:

From: Jan Kiszka 

When k3_r5_cluster_rproc_exit is run, core 1 is shutdown and removed
first. When core 0 should then be stopped before its removal, it will
find core1->rproc as NULL already and crashes. Happens on rmmod e.g.



Did you check this on top of -next-20240820 tag? There was a series[0] 
which was merged recently which fixed this condition. I don't see this 
issue when trying on top of -next-20240820 tag.

[0]: https://lore.kernel.org/all/20240808074127.2688131-1-b-pa...@ti.com/



Fixes: 3c8a9066d584 ("remoteproc: k3-r5: Do not allow core1 to power up before core0 
via sysfs")
CC: sta...@vger.kernel.org
Signed-off-by: Jan Kiszka 
---

There might be one more because I can still make this driver crash
after an operator error. Were error scenarios tested at all?



Can you point out what is this issue more specifically, and I can take 
this up then.




  drivers/remoteproc/ti_k3_r5_remoteproc.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index eb09d2e9b32a..9ebd7a34e638 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -646,7 +646,8 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
/* 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->rproc->state != RPROC_OFFLINE) {
+   if (core != core1 && core1->rproc &&
+   core1->rproc->state != RPROC_OFFLINE) {
dev_err(dev, "%s: can not stop core 0 before core 1\n",
__func__);
ret = -EPERM;

Re: [PATCH] remoteproc: k3-r5: Fix driver shutdown

[Beleswar Prasad Padhi]

On 20-08-2024 15:09, Jan Kiszka wrote:

On 20.08.24 11:30, Beleswar Prasad Padhi wrote:

Hi Jan,

On 19-08-2024 22:17, Jan Kiszka wrote:

From: Jan Kiszka 

When k3_r5_cluster_rproc_exit is run, core 1 is shutdown and removed
first. When core 0 should then be stopped before its removal, it will
find core1->rproc as NULL already and crashes. Happens on rmmod e.g.


Did you check this on top of -next-20240820 tag? There was a series[0]
which was merged recently which fixed this condition. I don't see this
issue when trying on top of -next-20240820 tag.
[0]: https://lore.kernel.org/all/20240808074127.2688131-1-b-pa...@ti.com/


I didn't try those yet, I was on 6.11-rcX. But from reading them
quickly, I'm not seeing the two issues I found directly addressed there.


Check the comment by Andrew Davis[0], that addresses the above issue.

[0]: 
https://lore.kernel.org/all/0bba5293-a55d-4f13-887c-272a54d6e...@ti.com/





Fixes: 3c8a9066d584 ("remoteproc: k3-r5: Do not allow core1 to power
up before core0 via sysfs")
CC: sta...@vger.kernel.org
Signed-off-by: Jan Kiszka 
---

There might be one more because I can still make this driver crash
after an operator error. Were error scenarios tested at all?


Can you point out what is this issue more specifically, and I can take
this up then.

Try starting core1 before core0, and then again - system will hang or
If you are trying to stop and then start the cores from sysfs, that is 
not yet supported. The hang is thus expected.

crash while trying to wipe ATCM. I do not understand this problem yet -
same VA is used, and I cannot see where/how the region should have been
unmapped in between.

Jan


   drivers/remoteproc/ti_k3_r5_remoteproc.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index eb09d2e9b32a..9ebd7a34e638 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -646,7 +646,8 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
   /* 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->rproc->state != RPROC_OFFLINE) {
+    if (core != core1 && core1->rproc &&
+    core1->rproc->state != RPROC_OFFLINE) {
   dev_err(dev, "%s: can not stop core 0 before core 1\n",
   __func__);
   ret = -EPERM;

Re: [PATCH] remoteproc: k3-r5: Fix driver shutdown

[Beleswar Prasad Padhi]

On 20-08-2024 19:50, Jan Kiszka wrote:

On 20.08.24 11:48, Beleswar Prasad Padhi wrote:

On 20-08-2024 15:09, Jan Kiszka wrote:

On 20.08.24 11:30, Beleswar Prasad Padhi wrote:

Hi Jan,

On 19-08-2024 22:17, Jan Kiszka wrote:

From: Jan Kiszka 

When k3_r5_cluster_rproc_exit is run, core 1 is shutdown and removed
first. When core 0 should then be stopped before its removal, it will
find core1->rproc as NULL already and crashes. Happens on rmmod e.g.

Did you check this on top of -next-20240820 tag? There was a series[0]
which was merged recently which fixed this condition. I don't see this
issue when trying on top of -next-20240820 tag.
[0]:
https://lore.kernel.org/all/20240808074127.2688131-1-b-pa...@ti.com/


I didn't try those yet, I was on 6.11-rcX. But from reading them
quickly, I'm not seeing the two issues I found directly addressed there.

Check the comment by Andrew Davis[0], that addresses the above issue.

[0]:
https://lore.kernel.org/all/0bba5293-a55d-4f13-887c-272a54d6e...@ti.com/


OK, then someone still needs to update his patch accordingly.
That comment was addressed in the v4 series revision[1] and was merged 
to linux-next, available with tag -next-20240820. Request you to please 
check if the issue persists with -next-20240820 tag. I checked myself, 
and was not able to reproduce.

[1]: https://lore.kernel.org/all/Zr9nbWnADDB+ZOlg@p14s/



Fixes: 3c8a9066d584 ("remoteproc: k3-r5: Do not allow core1 to power
up before core0 via sysfs")
CC: sta...@vger.kernel.org
Signed-off-by: Jan Kiszka 
---

There might be one more because I can still make this driver crash
after an operator error. Were error scenarios tested at all?

Can you point out what is this issue more specifically, and I can take
this up then.

Try starting core1 before core0, and then again - system will hang or

If you are trying to stop and then start the cores from sysfs, that is
not yet supported. The hang is thus expected.

What? Then the driver is broken, even more. Why wasn't it fully implemented?
The driver is capable of starting a core and stopping it all well. The 
problem is, when we stop a core from sysfs (without resetting the SoC 
itself), the remotecore is powered off, but its resources are not 
relinquished. So when we start it back, there could be some memory 
corruptions. This feature of "graceful shutdown" of remotecores is 
almost implemented and will be posted to this driver soon. Request you 
to try out after that.


With graceful shutdown feature, this will be the flow:
1. We issue a core stop operation from sysfs.
2. The remoteproc driver sends a special "SHUTDOWN" mailbox message to 
the remotecore.
3. The remotecore relinquishes all of its acquired resources through 
Device Manager Firmware and sends an ACK back.

4. The remotecore enters WFI state and then is resetted through Host core.
5. Then, if we try to do the core start operation from sysfs, core 
should be up as expected.


Thanks,
Beleswar


Jan

Re: [PATCH] remoteproc: k3-r5: Fix driver shutdown

[Beleswar Prasad Padhi]

On 20-08-2024 20:29, Beleswar Prasad Padhi wrote:


On 20-08-2024 19:50, Jan Kiszka wrote:

On 20.08.24 11:48, Beleswar Prasad Padhi wrote:

On 20-08-2024 15:09, Jan Kiszka wrote:

On 20.08.24 11:30, Beleswar Prasad Padhi wrote:

Hi Jan,

On 19-08-2024 22:17, Jan Kiszka wrote:

From: Jan Kiszka 

When k3_r5_cluster_rproc_exit is run, core 1 is shutdown and removed
first. When core 0 should then be stopped before its removal, it 
will

find core1->rproc as NULL already and crashes. Happens on rmmod e.g.
Did you check this on top of -next-20240820 tag? There was a 
series[0]
which was merged recently which fixed this condition. I don't see 
this

issue when trying on top of -next-20240820 tag.
[0]:
https://lore.kernel.org/all/20240808074127.2688131-1-b-pa...@ti.com/


I didn't try those yet, I was on 6.11-rcX. But from reading them
quickly, I'm not seeing the two issues I found directly addressed 
there.

Check the comment by Andrew Davis[0], that addresses the above issue.

[0]:
https://lore.kernel.org/all/0bba5293-a55d-4f13-887c-272a54d6e...@ti.com/ 




OK, then someone still needs to update his patch accordingly.
That comment was addressed in the v4 series revision[1] and was merged 
to linux-next, available with tag -next-20240820. Request you to 
please check if the issue persists with -next-20240820 tag. I checked 
myself, and was not able to reproduce.

[1]: https://lore.kernel.org/all/Zr9nbWnADDB+ZOlg@p14s/



Fixes: 3c8a9066d584 ("remoteproc: k3-r5: Do not allow core1 to power
up before core0 via sysfs")
CC: sta...@vger.kernel.org
Signed-off-by: Jan Kiszka 
---

There might be one more because I can still make this driver crash
after an operator error. Were error scenarios tested at all?
Can you point out what is this issue more specifically, and I can 
take

this up then.

Try starting core1 before core0, and then again - system will hang or

If you are trying to stop and then start the cores from sysfs, that is
not yet supported. The hang is thus expected.
What? Then the driver is broken, even more. Why wasn't it fully 
implemented?



Just wanted to point out that this "graceful shutdown" feature is 
majorly dependent on the Device Manager Firmware(point 3) and minimal 
changes to the remoteproc driver (point 2 and 4). Thus, as soon as 
Firmware is capable, we will send out the patches for this feature.


The driver is capable of starting a core and stopping it all well. The 
problem is, when we stop a core from sysfs (without resetting the SoC 
itself), the remotecore is powered off, but its resources are not 
relinquished. So when we start it back, there could be some memory 
corruptions. This feature of "graceful shutdown" of remotecores is 
almost implemented and will be posted to this driver soon. Request you 
to try out after that.


With graceful shutdown feature, this will be the flow:
1. We issue a core stop operation from sysfs.
2. The remoteproc driver sends a special "SHUTDOWN" mailbox message to 
the remotecore.
3. The remotecore relinquishes all of its acquired resources through 
Device Manager Firmware and sends an ACK back.
4. The remotecore enters WFI state and then is resetted through Host 
core.
5. Then, if we try to do the core start operation from sysfs, core 
should be up as expected.


Thanks,
Beleswar


Jan

Re: [PATCH] remoteproc: k3-r5: Fix error handling when power-up failed

[Beleswar Prasad Padhi]

On 19-08-2024 20:54, Jan Kiszka wrote:

From: Jan Kiszka 

By simply bailing out, the driver was violating its rule and internal



Using device lifecycle managed functions to register the rproc 
(devm_rproc_add()), bailing out with an error code will work.



assumptions that either both or no rproc should be initialized. E.g.,
this could cause the first core to be available but not the second one,
leading to crashes on its shutdown later on while trying to dereference
that second instance.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up 
core1")
Signed-off-by: Jan Kiszka 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 39a47540c590..eb09d2e9b32a 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1332,7 +1332,7 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)
dev_err(dev,
"Timed out waiting for %s core to power up!\n",
rproc->name);
-   return ret;
+   goto err_powerup;
}
}
  
@@ -1348,6 +1348,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)

}
}
  
+err_powerup:

rproc_del(rproc);



Please use devm_rproc_add() to avoid having to do rproc_del() manually 
here.



  err_add:
k3_r5_reserved_mem_exit(kproc);

Re: [PATCH] remoteproc: k3-r5: Fix error handling when power-up failed

[Beleswar Prasad Padhi]

On 21-08-2024 23:40, Jan Kiszka wrote:

On 21.08.24 07:30, Beleswar Prasad Padhi wrote:

On 19-08-2024 20:54, Jan Kiszka wrote:

From: Jan Kiszka 

By simply bailing out, the driver was violating its rule and internal


Using device lifecycle managed functions to register the rproc
(devm_rproc_add()), bailing out with an error code will work.


assumptions that either both or no rproc should be initialized. E.g.,
this could cause the first core to be available but not the second one,
leading to crashes on its shutdown later on while trying to dereference
that second instance.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up
before powering up core1")
Signed-off-by: Jan Kiszka 
---
   drivers/remoteproc/ti_k3_r5_remoteproc.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 39a47540c590..eb09d2e9b32a 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1332,7 +1332,7 @@ static int k3_r5_cluster_rproc_init(struct
platform_device *pdev)
   dev_err(dev,
   "Timed out waiting for %s core to power up!\n",
   rproc->name);
-    return ret;
+    goto err_powerup;
   }
   }
   @@ -1348,6 +1348,7 @@ static int k3_r5_cluster_rproc_init(struct
platform_device *pdev)
   }
   }
   +err_powerup:
   rproc_del(rproc);


Please use devm_rproc_add() to avoid having to do rproc_del() manually
here.

This is just be the tip of the iceberg. The whole code needs to be
reworked accordingly so that we can drop these goto, not just this one.



You are correct. Unfortunately, the organic growth of this driver has 
resulted in a need to refactor. I plan on doing this and post the 
refactoring soon. This should be part of the overall refactoring as 
suggested by Mathieu[2]. But for the immediate problem, your fix does 
patch things up.. hence:


Acked-by: Beleswar Padhi 

[2]: https://lore.kernel.org/all/Zr4w8Vj0mVo5sBsJ@p14s/


Just look at k3_r5_reserved_mem_init. Your change in [1] was also too
early in this regard, breaking current error handling additionally.




Curious, Could you point out how does the change in [1] breaks current 
error handling?




I'll stop my whac-a-mole. Someone needs to sit down and do that for the
complete code consistently. And test the error cases.

Jan

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=f3f11cfe890733373ddbb1ce8991ccd4ee5e79e1


   err_add:
   k3_r5_reserved_mem_exit(kproc);

Re: [PATCH] remoteproc: k3-r5: Fix error handling when power-up failed

[Beleswar Prasad Padhi]

On 22-08-2024 10:57, Jan Kiszka wrote:

On 22.08.24 07:22, Beleswar Prasad Padhi wrote:

On 21-08-2024 23:40, Jan Kiszka wrote:

On 21.08.24 07:30, Beleswar Prasad Padhi wrote:

On 19-08-2024 20:54, Jan Kiszka wrote:

From: Jan Kiszka 

By simply bailing out, the driver was violating its rule and internal

Using device lifecycle managed functions to register the rproc
(devm_rproc_add()), bailing out with an error code will work.


assumptions that either both or no rproc should be initialized. E.g.,
this could cause the first core to be available but not the second one,
leading to crashes on its shutdown later on while trying to dereference
that second instance.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up
before powering up core1")
Signed-off-by: Jan Kiszka 
---
    drivers/remoteproc/ti_k3_r5_remoteproc.c | 3 ++-
    1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 39a47540c590..eb09d2e9b32a 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1332,7 +1332,7 @@ static int k3_r5_cluster_rproc_init(struct
platform_device *pdev)
    dev_err(dev,
    "Timed out waiting for %s core to power up!\n",
    rproc->name);
-    return ret;
+    goto err_powerup;
    }
    }
    @@ -1348,6 +1348,7 @@ static int k3_r5_cluster_rproc_init(struct
platform_device *pdev)
    }
    }
    +err_powerup:
    rproc_del(rproc);

Please use devm_rproc_add() to avoid having to do rproc_del() manually
here.

This is just be the tip of the iceberg. The whole code needs to be
reworked accordingly so that we can drop these goto, not just this one.


You are correct. Unfortunately, the organic growth of this driver has
resulted in a need to refactor. I plan on doing this and post the
refactoring soon. This should be part of the overall refactoring as
suggested by Mathieu[2]. But for the immediate problem, your fix does
patch things up.. hence:

Acked-by: Beleswar Padhi 

[2]: https://lore.kernel.org/all/Zr4w8Vj0mVo5sBsJ@p14s/


Just look at k3_r5_reserved_mem_init. Your change in [1] was also too
early in this regard, breaking current error handling additionally.



Curious, Could you point out how does the change in [1] breaks current
error handling?


Same story: You leave the inner loop of k3_r5_cluster_rproc_init() via
return without that loop having been converted to support this.



The rproc has been allocated via devm_rproc_alloc[3] before the 
return[4] at k3_r5_cluster_rproc_init. Thus, it is capable of freeing 
the rproc just based on error codes. It was tested.
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/ti_k3_r5_remoteproc.c#n1238
[4]: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/ti_k3_r5_remoteproc.c#n1259




Jan

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Beleswar Prasad Padhi]

Hi All,

On 22/08/24 21:47, Mathieu Poirier wrote:

Hi Baolu,

Sorry for the late reply, this slipped through the cracks.

On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> An iommu domain is allocated in rproc_enable_iommu() and is attached to
> rproc->dev.parent in the same function.
> 
> Use iommu_paging_domain_alloc() to make it explicit.
> 
> Signed-off-by: Lu Baolu 

> Reviewed-by: Jason Gunthorpe 
> Link: 
https://lore.kernel.org/r/2024061008.88197-13-baolu...@linux.intel.com
> ---
>  drivers/remoteproc/remoteproc_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

> index f276956f2c5c..eb66f78ec8b7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc *rproc)
>return 0;
>}
>  
> -	domain = iommu_domain_alloc(dev->bus);

> -  if (!domain) {
> +  domain = iommu_paging_domain_alloc(dev);

I'm a little hesitant here.  Function iommu_domain_alloc() passes NULL two the
second argument of __iommu_domain_alloc() while iommu_paging_domain_alloc()
provides a 'dev'.  I don't have any HW to test on and I am not familiar enough
with the IOMMU subsystem to confidently more forward.

I am asking the Qualcomm (Bjorn and friends) and TI crew (Beleswar, Andrew,
Nishanth and Hari) to test this patch on their IOMMU devices and get back to me
with a "Tested-by".



Just a heads-up. Currently, I am seeing boot failures while booting 
remotecores in TI's IOMMU devices with mainline kernel. Working on the 
the fix, once it is done, will apply the above patch and test it ASAP.


Thanks,
Beleswar



Thanks,
Mathieu

> +  if (IS_ERR(domain)) {
>dev_err(dev, "can't alloc iommu domain\n");
> -  return -ENOMEM;
> +  return PTR_ERR(domain);
>}
>  
>  	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> -- 
> 2.34.1
> 

Re: [PATCH v2] remoteproc: k3-r5: Delay notification of wakeup event

[Beleswar Prasad Padhi]

Hi Mathieu,

On 20-08-2024 16:20, Beleswar Padhi wrote:

From: Udit Kumar 

Few times, core1 was scheduled to boot first before core0, which leads
to error:

'k3_r5_rproc_start: can not start core 1 before core 0'.

This was happening due to some scheduling between prepare and start
callback. The probe function waits for event, which is getting
triggered by prepare callback. To avoid above condition move event
trigger to start instead of prepare callback.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up 
core1")



Please put this patch on hold. I have some additional changelog that 
should go in v3.


Thanks,
Beleswar


Signed-off-by: Udit Kumar 
[ Applied wakeup event trigger only for Split-Mode booted rprocs ]
Signed-off-by: Beleswar Padhi 
---
v2: Changelog:
* Mathieu
1) Rebased changes on top of -next-20240820 tag.

Link to v1:
https://lore.kernel.org/all/20240809060132.308642-1-b-pa...@ti.com/

  drivers/remoteproc/ti_k3_r5_remoteproc.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 8a63a9360c0f..e61e53381abc 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -469,8 +469,6 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
ret);
return ret;
}
-   core->released_from_reset = true;
-   wake_up_interruptible(&cluster->core_transition);
  
  	/*

 * Newer IP revisions like on J7200 SoCs support h/w auto-initialization
@@ -587,6 +585,9 @@ static int k3_r5_rproc_start(struct rproc *rproc)
ret = k3_r5_core_run(core);
if (ret)
return ret;
+
+   core->released_from_reset = true;
+   wake_up_interruptible(&cluster->core_transition);
}
  
  	return 0;

Re: [PATCH v2] remoteproc: k3-r5: Delay notification of wakeup event

[Beleswar Prasad Padhi]

On 03-09-2024 20:02, Mathieu Poirier wrote:

On Tue, 3 Sept 2024 at 04:15, Beleswar Prasad Padhi  wrote:

Hi Mathieu,

On 20-08-2024 16:20, Beleswar Padhi wrote:

From: Udit Kumar 

Few times, core1 was scheduled to boot first before core0, which leads
to error:

'k3_r5_rproc_start: can not start core 1 before core 0'.

This was happening due to some scheduling between prepare and start
callback. The probe function waits for event, which is getting
triggered by prepare callback. To avoid above condition move event
trigger to start instead of prepare callback.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up 
core1")


Please put this patch on hold. I have some additional changelog that
should go in v3.


I applied this patch a couple of weeks ago - are those changes to the
code?  If so please send another patch on top of rproc-next.



Understood. Those are code changes, I will post another patch series for 
the same.


Thanks,
Beleswar




Thanks,
Beleswar


Signed-off-by: Udit Kumar 
[ Applied wakeup event trigger only for Split-Mode booted rprocs ]
Signed-off-by: Beleswar Padhi 
---
v2: Changelog:
* Mathieu
1) Rebased changes on top of -next-20240820 tag.

Link to v1:
https://lore.kernel.org/all/20240809060132.308642-1-b-pa...@ti.com/

   drivers/remoteproc/ti_k3_r5_remoteproc.c | 5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 8a63a9360c0f..e61e53381abc 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -469,8 +469,6 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
   ret);
   return ret;
   }
- core->released_from_reset = true;
- wake_up_interruptible(&cluster->core_transition);

   /*
* Newer IP revisions like on J7200 SoCs support h/w auto-initialization
@@ -587,6 +585,9 @@ static int k3_r5_rproc_start(struct rproc *rproc)
   ret = k3_r5_core_run(core);
   if (ret)
   return ret;
+
+ core->released_from_reset = true;
+ wake_up_interruptible(&cluster->core_transition);
   }

   return 0;

Re: [PATCH] remoteproc: k3-r5: Decouple firmware booting from probe routine

[Beleswar Prasad Padhi]

Hi Mathieu,

On 06-09-2024 22:17, Mathieu Poirier wrote:

On Fri, Sep 06, 2024 at 03:10:45PM +0530, Beleswar Padhi wrote:

The current implementation of the waiting mechanism in probe() waits for
the 'released_from_reset' flag to be set which is done in
k3_r5_rproc_prepare() as part of rproc_fw_boot(). This causes unexpected

If you are looking at rproc-next, @released_from_reset is set in
k3_r5_rproc_start().



You are correct. Apologies, this flag is set in the start() function 
(still a part of rproc_fw_boot()), not prepare(). I wanted to point out 
@released_from_reset is set in rproc_fw_boot() routine, and is checked 
for in the probe() routine.



Moreover, the waiting mechanic happens in
k3_r5_cluster_rproc_init(), which makes reading your changelog highly confusing.



Yes, the mechanism is in the k3_r5_cluster_rproc_init() function which 
is called from k3_r5_probe(), hence I referred to it being called in the 
probe routine. The point I wanted to make was, any error while booting 
firmware would end up in a probe failure. Apologies for not making it 
clearer.





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() and
k3_r5_rproc_start/stop() functions. This allows the probe routine to
complete without depending on firmware booting, while still maintaining
the required power-synchronization between cores.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up 
core1")
Signed-off-by: Beleswar Padhi 
---
Posted this as a Fix as this was breaking usecases where we wanted to load a
firmware by writing to sysfs handles in userspace.

  drivers/remoteproc/ti_k3_r5_remoteproc.c | 170 ---
  1 file changed, 118 insertions(+), 52 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 747ee467da88..df8f124f4248 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -131,6 +131,7 @@ struct k3_r5_cluster {
   * @btcm_enable: flag to control BTCM enablement
   * @loczrama: flag to dictate which TCM is at device address 0x0
   * @released_from_reset: flag to signal when core is out of reset
+ * @unhalted: flag to signal when core is unhalted
   */
  struct k3_r5_core {
struct list_head elem;
@@ -148,6 +149,7 @@ struct k3_r5_core {
u32 btcm_enable;
u32 loczrama;
bool released_from_reset;
+   bool unhalted;

Yet another flag?  @released_from_reset is not enough?



So, technically @released_from_reset should maintain the sync between 
_prepare() of #core0 and #core1. But with commit 8fa052c29e50 
("remoteproc: k3-r5: Delay notification of wakeup event"), we are trying 
to maintain the sync of both _prepare() and _start() with just this one 
flag by pushing the notification from prepare() to start(). Having two 
flags is a cleanup attempt, where @released_from_reset handles 
_prepare() sync and @unhalted handles _start() sync.



  And why does it need to
be "unhalted" rather than something like "running"?



"running" sounds like a better name for this flag. Thank you!


  I will not move forward
with this patch until I get an R-B and a T-B from two other people at TI.

The above and the exchange with Jan Kiszka is furthering my belief that this
driver is up for a serious refactoring exercise.  From hereon I will only
consider bug fixes.



I understand the concern. I will do the refactor and possibly include 
this patch as part of that refactoring series.


Thanks,
Beleswar



Thanks,
Mathieu


  };
  
  /**

@@ -448,13 +450,33 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
  {
struct k3_r5_rproc *kproc = rproc->priv;
struct k3_r5_cluster *cluster = kproc->cluster;
-   struct k3_r5_core *core = kproc->core;
+   struct k3_r5_core *core0, *core1, *core = kproc->core;
struct device *dev = kproc->dev;
u32 ctrl = 0, cfg = 0, stat = 0;
u64 boot_vec = 0;
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.
+*/
+   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 == false) {
+   ret = wait_event_interruptible_timeout(cluster->core_transitio

Re: [PATCH] remoteproc: k3-r5: Decouple firmware booting from probe routine

[Beleswar Prasad Padhi]

On 07/09/24 15:41, Kumar, Udit wrote:


On 9/6/2024 3:10 PM, Beleswar Padhi wrote:

The current implementation of the waiting mechanism in probe() waits for
the 'released_from_reset' flag to be set which is done in
k3_r5_rproc_prepare() as part of rproc_fw_boot(). This 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.


I won't say failure, I will say this is behavior of driver.

Driver expect firmware to be available , but I agree driver should be 
able to execute


with/without firmware availability.



To address this, the waiting mechanism is refactored out of the probe
routine into the appropriate k3_r5_rproc_prepare/unprepare() and
k3_r5_rproc_start/stop() functions. This allows the probe routine to
complete without depending on firmware booting, while still maintaining
the required power-synchronization between cores.

Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up 
before powering up core1")

Signed-off-by: Beleswar Padhi 
---
Posted this as a Fix as this was breaking usecases where we wanted to 
load a

firmware by writing to sysfs handles in userspace.

  drivers/remoteproc/ti_k3_r5_remoteproc.c | 170 ---
  1 file changed, 118 insertions(+), 52 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

[..]
+    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 == false) {
+    ret = 
wait_event_interruptible_timeout(cluster->core_transition,

+   core0->released_from_reset,
+   msecs_to_jiffies(2000));

only one wait in start should be good enough,



Won't that cause race conditions, where prepare for core1 can be called 
before core0?



+    if (ret <= 0) {
+    dev_err(dev, "can not power up core1 before core0");
+    return -EPERM;
+    }
+    }
+
  ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, 
&stat);

  if (ret < 0)
  return ret;
@@ -470,6 +492,12 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
  return ret;
  }
  +    /* Notify all threads in the wait queue when core state has 
changed so

+ * that threads waiting for this condition can be executed.
+ */
+    core->released_from_reset = true;
+    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
@@ -515,14 +543,46 @@ static int k3_r5_rproc_unprepare(struct rproc 
*rproc)

  {
  struct k3_r5_rproc *kproc = rproc->priv;
  struct k3_r5_cluster *cluster = kproc->cluster;
-    struct k3_r5_core *core = kproc->core;
+    struct k3_r5_core *core0, *core1, *core = kproc->core;



why you need wait in unprepare/stop,

In case you are failing during firmware load or so then already we are 
in bad state.


if this is call from user land then, i don't except anyone will auto 
trigger stopping of core-0


IMO, in unprepare/stop, if action is attempted on core1 with core-0 
ON, simply return error.



Yes, you are correct. Will include this change in revision. Thanks!





[..]
@@ -629,7 +702,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  struct k3_r5_rproc *kproc = rproc->priv;
  struct k3_r5_cluster *cluster = kproc->cluster;
  struct device *dev = kproc->dev;

Re: [RFC PATCH] remoteproc: k3-r5: Fix check performed in k3_r5_rproc_{mbox_callback/kick}

[Beleswar Prasad Padhi]

Hi Mathieu,

On 17/09/24 14:07, Mathieu Poirier wrote:

On Mon, 16 Sept 2024 at 23:20, Kumar, Udit  wrote:

On 9/16/2024 8:50 PM, Mathieu Poirier wrote:

On Mon, 16 Sept 2024 at 02:31, Siddharth Vadapalli  wrote:

Commit f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during
probe routine") introduced a check in the "k3_r5_rproc_mbox_callback()" and
"k3_r5_rproc_kick()" callbacks, causing them to exit if the remote core's
state is "RPROC_DETACHED". However, the "__rproc_attach()" function that is
responsible for attaching to a remote core, updates the state of the remote
core to "RPROC_ATTACHED" only after invoking "rproc_start_subdevices()".

The "rproc_start_subdevices()" function triggers the probe of the Virtio
RPMsg devices associated with the remote core, which require that the
"k3_r5_rproc_kick()" and "k3_r5_rproc_mbox_callback()" callbacks are
functional. Hence, drop the check in the callbacks.

Honestly, I am very tempted to just revert f3f11cfe8907 and ea1d6fb5b571.


Please don't :) , it will break rproc in general for k3 devices.


Why not - it is already broken anyway.  Reverting the patches will
force TI to actually think about the feature in terms of design,
completeness and testing.  The merge window opened on Sunday - I'm not
going to merge whack-a-mole patches and hope the right fix comes
along.



Apologies for causing this trouble, Mathieu. I have accumulated various 
use-cases of the driver, including this, and hereon will keep in mind 
while posting further patches.





Couple of solutions for this race around condition (in mine preference
order)


This is for the TI team to discuss _and_ test thoroughly.  From hereon
and until I see things improve, all patches from TI will need to be
tagged with R-B and T-B tags (collected on the mailing lists) from two
different individuals before I look at them.



Understood, that is a fair ask. Hereon, I will also attach my test logs 
for all the usecases I've tested a patch with, to give more visibility 
on the testing done.





1) In
https://elixir.bootlin.com/linux/v6.11/source/drivers/remoteproc/ti_k3_r5_remoteproc.c#L190
have a check , if probe in is progress or not

2)
https://elixir.bootlin.com/linux/v6.11/source/drivers/remoteproc/ti_k3_r5_remoteproc.c#L1205
-- correct the state to ON or something else

3) Move condition
https://elixir.bootlin.com/linux/v6.11/source/drivers/remoteproc/remoteproc_core.c#L1360
before rproc_start_subdevices

calling




Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during probe 
routine")
Signed-off-by: Siddharth Vadapalli 
---

Hello,

Since the commit being fixed is not yet a part of Mainline Linux, this
patch is based on linux-next tagged next-20240913.

An alternative to this patch will be a change to the "__rproc_attach()"
function in the "remoteproc_core.c" driver with
rproc->state = RPROC_ATTACHED;
being set after "rproc_attach_device()" is invoked, but __before__
invoking "rproc_start_subdevices()". Since this change will be performed
in the common Remoteproc Core, it appeared to me that fixing it in the
TI remoteproc driver is the correct approach.

The equivalent of this patch for ti_k3_dsp_remoteproc.c might also be
required, which I shall post if the current patch is acceptable.

Kindly review and share your feedback on this patch.

Regards,
Siddharth.

   drivers/remoteproc/ti_k3_r5_remoteproc.c | 8 
   1 file changed, 8 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 747ee467da88..4894461aa65f 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -194,10 +194,6 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client 
*client, void *data)
  const char *name = kproc->rproc->name;
  u32 msg = omap_mbox_message(data);

-   /* Do not forward message from a detached core */
-   if (kproc->rproc->state == RPROC_DETACHED)
-   return;
-
  dev_dbg(dev, "mbox msg: 0x%x\n", msg);

  switch (msg) {
@@ -233,10 +229,6 @@ static void k3_r5_rproc_kick(struct rproc *rproc, int vqid)
  mbox_msg_t msg = (mbox_msg_t)vqid;
  int ret;

-   /* Do not forward message to a detached core */
-   if (kproc->rproc->state == RPROC_DETACHED)
-   return;
-
  /* send the index of the triggered virtqueue in the mailbox payload */
  ret = mbox_send_message(kproc->mbox, (void *)msg);
  if (ret < 0)
--
2.40.1

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Beleswar Prasad Padhi]

On 29-08-2024 11:47, Beleswar Prasad Padhi wrote:

Hi All,

On 22/08/24 21:47, Mathieu Poirier wrote:

Hi Baolu,

Sorry for the late reply, this slipped through the cracks.

On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> An iommu domain is allocated in rproc_enable_iommu() and is 
attached to

> rproc->dev.parent in the same function.
> > Use iommu_paging_domain_alloc() to make it explicit.
> > Signed-off-by: Lu Baolu 
> Reviewed-by: Jason Gunthorpe 
> Link: 
https://lore.kernel.org/r/2024061008.88197-13-baolu...@linux.intel.com

> ---
>  drivers/remoteproc/remoteproc_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> > diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c

> index f276956f2c5c..eb66f78ec8b7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc 
*rproc)

>  return 0;
>  }
>  > -    domain = iommu_domain_alloc(dev->bus);
> -    if (!domain) {
> +    domain = iommu_paging_domain_alloc(dev);

I'm a little hesitant here.  Function iommu_domain_alloc() passes 
NULL two the
second argument of __iommu_domain_alloc() while 
iommu_paging_domain_alloc()
provides a 'dev'.  I don't have any HW to test on and I am not 
familiar enough

with the IOMMU subsystem to confidently more forward.

I am asking the Qualcomm (Bjorn and friends) and TI crew (Beleswar, 
Andrew,
Nishanth and Hari) to test this patch on their IOMMU devices and get 
back to me

with a "Tested-by".



Just a heads-up. Currently, I am seeing boot failures while booting 
remotecores in TI's IOMMU devices with mainline kernel. Working on the 
the fix, once it is done, will apply the above patch and test it ASAP.



Since commit 17de3f5fdd35, the IOMMU framework has changed how it 
handles callback ops, moving away from using bus->iommu_ops. The 
omap-iommu driver has not yet been updated to align with these framework 
changes. Therefore, omap-iommu, and hence, omap-remoteproc have been 
broken since 17de3f5fdd35.


This patch is a step in the right direction for adapting the 
remoteproc_core framework to the newer IOMMU framework by looking for 
iommu_ops attached to the dev structure instead of dev->bus. Due to time 
constraints, I was unable to update the omap-iommu driver to accommodate 
these changes, so, I could not test this patch. However, logically, this 
patch LGTM. So,


Acked-by: Beleswar Padhi 

Thanks,
Beleswar



Thanks,
Beleswar



Thanks,
Mathieu

> +    if (IS_ERR(domain)) {
>  dev_err(dev, "can't alloc iommu domain\n");
> -    return -ENOMEM;
> +    return PTR_ERR(domain);
>  }
>  >  iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> -- > 2.34.1
>

Re: [PATCH v8 01/20] remoteproc: k3-m4: Prevent Mailbox level IPC with detached core

[Beleswar Prasad Padhi]

On 03/01/25 15:42, Beleswar Padhi wrote:

Inter-Processor Communication is facilitated through mailbox payloads,
which typically contains the index of the triggered virtqueue having the
actual data to be consumed, but the payload can also be used for trivial
communication, like sending an echo message or notifying a crash etc.
When the core is detached, the virtqueues are freed, and thus the Virtio
level IPC is not functional. However, Mailbox IPC is still possible with
trivial payloads.

Therefore, introduce checks in k3_m4_rproc_kick() and
k3_m4_rproc_mbox_callback() functions to return early without parsing
the payload when core is detached, and is not undergoing an attach
operation in IPC-only mode.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_m4_remoteproc.c | 41 
  1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c 
b/drivers/remoteproc/ti_k3_m4_remoteproc.c
index a16fb165fced..3201c3684a86 100644
--- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
@@ -50,6 +50,7 @@ struct k3_m4_rproc_mem_data {
  /**
   * struct k3_m4_rproc - k3 remote processor driver structure
   * @dev: cached device pointer
+ * @rproc: remoteproc device handle
   * @mem: internal memory regions data
   * @num_mems: number of internal memory regions
   * @rmem: reserved memory regions data
@@ -60,9 +61,11 @@ struct k3_m4_rproc_mem_data {
   * @ti_sci_id: TI-SCI device identifier
   * @mbox: mailbox channel handle
   * @client: mailbox client to request the mailbox channel
+ * @is_attach_ongoing: flag to indicate if IPC-only "attach()" is in progress



Note: This variable naming will be changed to 'is_attached' (for better 
clarity) in the next revision. I wish to take more feedback for the 
whole series before posting next revision.


Thanks,
Beleswar


   */
  struct k3_m4_rproc {
struct device *dev;
+   struct rproc *rproc;
struct k3_m4_rproc_mem *mem;
int num_mems;
struct k3_m4_rproc_mem *rmem;
@@ -73,6 +76,7 @@ struct k3_m4_rproc {
u32 ti_sci_id;
struct mbox_chan *mbox;
struct mbox_client client;
+   bool is_attach_ongoing;
  };
  
  /**

@@ -93,8 +97,16 @@ static void k3_m4_rproc_mbox_callback(struct mbox_client 
*client, void *data)
  {
struct device *dev = client->dev;
struct rproc *rproc = dev_get_drvdata(dev);
+   struct k3_m4_rproc *kproc = rproc->priv;
u32 msg = (u32)(uintptr_t)(data);
  
+	/*

+* Do not forward messages from a detached core, except when the core
+* is in the process of being attached in IPC-only mode.
+*/
+   if (!kproc->is_attach_ongoing && kproc->rproc->state == RPROC_DETACHED)
+   return;
+
dev_dbg(dev, "mbox msg: 0x%x\n", msg);
  
  	switch (msg) {

@@ -135,6 +147,12 @@ static void k3_m4_rproc_kick(struct rproc *rproc, int vqid)
u32 msg = (u32)vqid;
int ret;
  
+	/*

+* Do not forward messages to a detached core, except when the core
+* is in the process of being attached in IPC-only mode.
+*/
+   if (!kproc->is_attach_ongoing && kproc->rproc->state == RPROC_DETACHED)
+   return;
/*
 * Send the index of the triggered virtqueue in the mailbox payload.
 * NOTE: msg is cast to uintptr_t to prevent compiler warnings when
@@ -515,15 +533,19 @@ static int k3_m4_rproc_stop(struct rproc *rproc)
  /*
   * Attach to a running M4 remote processor (IPC-only mode)
   *
- * The remote processor is already booted, so there is no need to issue any
- * TI-SCI commands to boot the M4 core. This callback is used only in IPC-only
- * mode.
+ * This rproc attach callback only needs to set the "is_attach_ongoing" flag to
+ * notify k3_m4_rproc_{kick/mbox_callback} functions that the core is in the
+ * process of getting attached in IPC-only mode. The remote processor is 
already
+ * booted, so there is no need to issue any TI-SCI commands to boot the M4 
core.
+ * This callback is used only in IPC-only mode.
   */
  static int k3_m4_rproc_attach(struct rproc *rproc)
  {
struct k3_m4_rproc *kproc = rproc->priv;
int ret;
  
+	kproc->is_attach_ongoing = true;

+
ret = k3_m4_rproc_ping_mbox(kproc);
if (ret)
return ret;
@@ -534,12 +556,18 @@ static int k3_m4_rproc_attach(struct rproc *rproc)
  /*
   * Detach from a running M4 remote processor (IPC-only mode)
   *
- * This rproc detach callback performs the opposite operation to attach
- * callback, the M4 core is not stopped and will be left to continue to
- * run its booted firmware. This callback is invoked only in IPC-only mode.
+ * This rproc detach callback performs the opposite operation to attach 
callback
+ * and only needs to clear the "is_attach_ongoing" flag to ensure no mailbox
+ * messages are sent to or received from a detached core. The M4 core is not
+ * st

Re: [PATCH 4/5] remoteproc: k3-r5: Use devm_ioremap_wc() helper

[Beleswar Prasad Padhi]

On 17/12/24 21:33, Andrew Davis wrote:

On 12/4/24 5:11 AM, Beleswar Padhi wrote:

Use a device lifecycle managed ioremap helper function. This helps
prevent mistakes like unmapping out of order in cleanup functions and
forgetting to unmap on all error paths.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 +---
  1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index 2966cb210403..1a7681502f62 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1004,17 +1004,13 @@ static int k3_r5_reserved_mem_init(struct 
k3_r5_rproc *kproc)

  /* use remaining reserved memory regions for static carveouts */
  for (i = 0; i < num_rmems; i++) {
  rmem_np = of_parse_phandle(np, "memory-region", i + 1);
-    if (!rmem_np) {
-    ret = -EINVAL;
-    goto unmap_rmem;
-    }
+    if (!rmem_np)
+    return -EINVAL;
    rmem = of_reserved_mem_lookup(rmem_np);
  of_node_put(rmem_np);
-    if (!rmem) {
-    ret = -EINVAL;
-    goto unmap_rmem;
-    }
+    if (!rmem)
+    return -EINVAL;
    kproc->rmem[i].bus_addr = rmem->base;
  /*
@@ -1029,12 +1025,11 @@ static int k3_r5_reserved_mem_init(struct 
k3_r5_rproc *kproc)

   */
  kproc->rmem[i].dev_addr = (u32)rmem->base;
  kproc->rmem[i].size = rmem->size;
-    kproc->rmem[i].cpu_addr = ioremap_wc(rmem->base, rmem->size);
+    kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, 
rmem->size);

  if (!kproc->rmem[i].cpu_addr) {
  dev_err(dev, "failed to map reserved memory#%d at %pa 
of size %pa\n",

  i + 1, &rmem->base, &rmem->size);
-    ret = -ENOMEM;
-    goto unmap_rmem;
+    return -ENOMEM;
  }
    dev_dbg(dev, "reserved memory%d: bus addr %pa size 0x%zx 
va %pK da 0x%x\n",
@@ -1045,19 +1040,6 @@ static int k3_r5_reserved_mem_init(struct 
k3_r5_rproc *kproc)

  kproc->num_rmems = num_rmems;
    return 0;
-
-unmap_rmem:
-    for (i--; i >= 0; i--)
-    iounmap(kproc->rmem[i].cpu_addr);
-    return ret;
-}
-
-static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
-{
-    int i;
-
-    for (i = 0; i < kproc->num_rmems; i++)
-    iounmap(kproc->rmem[i].cpu_addr);
  }
    /*
@@ -1285,10 +1267,8 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

  }
    ret = rproc_add(rproc);
-    if (ret) {
-    dev_err(dev, "rproc_add failed, ret = %d\n", ret);
-    goto err_add;
-    }
+    if (ret)
+    dev_err_probe(dev, ret, "rproc_add failed\n");


Did you mean to return the result of dev_err_probe() here? Without that



Thanks for catching this. Should return here. Will fix in v2.

Thanks,
Beleswar


now anything between here and where err_add used to be will get run.
Might be more clear that this is correct by still doing a "goto out;"

Andrew


    /* create only one rproc in lockstep, single-cpu or
   * single core mode
@@ -1333,8 +1313,6 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

    err_powerup:
  rproc_del(rproc);
-err_add:
-    k3_r5_reserved_mem_exit(kproc);
  out:
  /* undo core0 upon any failures on core1 in split-mode */
  if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
@@ -1379,8 +1357,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
  mbox_free_channel(kproc->mbox);
    rproc_del(rproc);
-
-    k3_r5_reserved_mem_exit(kproc);
  }
  }

Re: [PATCH 3/5] remoteproc: k3-r5: Add devm action to release tsp

[Beleswar Prasad Padhi]

Hi Andrew,

On 17/12/24 21:30, Andrew Davis wrote:

On 12/4/24 5:11 AM, Beleswar Padhi wrote:

Use a device lifecycle managed action to release tsp ti_sci_proc handle.
This helps prevent mistakes like releasing out of order in cleanup
functions and forgetting to release on error paths.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index 0753a5c35c7e..2966cb210403 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1513,6 +1513,13 @@ static int 
k3_r5_core_of_get_sram_memories(struct platform_device *pdev,

  return 0;
  }
  +static void k3_r5_release_tsp(void *data)
+{
+    struct ti_sci_proc *tsp = data;
+
+    ti_sci_proc_release(tsp);
+}
+
  static int k3_r5_core_of_init(struct platform_device *pdev)
  {
  struct device *dev = &pdev->dev;
@@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct 
platform_device *pdev)

  goto err;
  }
  +    ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, 
core->tsp);

+    if (ret)
+    goto err;
+
  platform_set_drvdata(pdev, core);
  devres_close_group(dev, k3_r5_core_of_init);
  @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct 
platform_device *pdev)

   */
  static void k3_r5_core_of_exit(struct platform_device *pdev)
  {
-    struct k3_r5_core *core = platform_get_drvdata(pdev);
  struct device *dev = &pdev->dev;
-    int ret;
-
-    ret = ti_sci_proc_release(core->tsp);
-    if (ret)
-    dev_err(dev, "failed to release proc, ret = %d\n", ret);


One thing to remember is devm unrolling happens after remove(). So
here you are changing the order things happen. ti_sci_proc_release()
now will get called after the below functions. This most likely
isn't wrong, but to make review easier it helps to start from the
last called function in remove() and work backwards so nothing
is reordered.



That's a great insight! Will send out v2 following this order.

Thanks,
Beleswar



Andrew


  platform_set_drvdata(pdev, NULL);
  devres_release_group(dev, k3_r5_core_of_init);

Re: [PATCH v2 0/5] Use Device Lifecycle managed functions in TI R5 Remoteproc driver

[Beleswar Prasad Padhi]

On 12/19/2024 8:22 PM, Andrew Davis wrote:

On 12/19/24 5:05 AM, Beleswar Padhi wrote:
This series uses various devm_ helpers to simplify device removal 
path in

ti_k3_r5_remoteproc driver. This is the first series in the TI K3
Remoteproc refactoring as long planned since [0].

Testing Done:
1. Tested boot of R5F remoteprocs in MCU and MAIN voltage domain in both
IPC-Only mode and Kernel remoteproc mode in all Jacinto K3 devices.
2. Tested Lockstep, Split and Single-CPU Mode configuration (wherever
applicable) of R5F remoteprocs in all Jacinto K3 devices.
3. Tested shutdown of R5F remoteprocs from Linux userspace and also by
executing `modprobe -r ti_k3_r5_remoteproc`.


Did you also test that you could then start the cores back up?

I think that might need some firmware fixes we are working on, so
might not work even before these patches, but just wanted to check
if we have tried it yet.



Yes, the above graceful shutdown feature is part of firmware fixes. 
Also, some support needs to be added in the Linux driver, which sends a 
special "SHUTDOWN" mailbox message to the remotecore which signals the 
firmware to relinquish all resources and shutdown gracefully. So, 
support for turning the remoteprocs back on is not yet added, and not 
tested.




4. Tested that each patch in this series generates no new 
warnings/errors.


Was that with `make W=1 C=1`? Sparse checks will be done during -next
testing so good to check for those too.



Yes, did that testing.



Otherwise patches all look good to me, for the series:

Reviewed-by: Andrew Davis 



Thanks!





v2: Changelog:
1. Re-ordered patches in the series to use devm functions starting from
the last called function in remove(), to ease review. [Andrew]
2. Fixed a missing return after dev_err_probe() call in [PATCH v2 3/5]
("remoteproc: k3-r5: Use devm_ioremap_wc() helper"). [Andrew]
3. Removed redundant rproc_del() call in [PATCH v2 4/5] ("remoteproc:
k3-r5: Use devm_rproc_add() helper").

Link to v1:
https://lore.kernel.org/all/2024120430.2218497-1-b-pa...@ti.com/

[0]: https://lore.kernel.org/all/Zr4w8Vj0mVo5sBsJ@p14s/

Beleswar Padhi (5):
   remoteproc: k3-r5: Add devm action to release reserved memory
   remoteproc: k3-r5: Use devm_kcalloc() helper
   remoteproc: k3-r5: Use devm_ioremap_wc() helper
   remoteproc: k3-r5: Use devm_rproc_add() helper
   remoteproc: k3-r5: Add devm action to release tsp

  drivers/remoteproc/ti_k3_r5_remoteproc.c | 88 ++--
  1 file changed, 35 insertions(+), 53 deletions(-)

Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick}

[Beleswar Prasad Padhi]

On 22/01/25 00:17, Andrew Davis wrote:

On 12/24/24 3:14 AM, Beleswar Padhi wrote:

Commit f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during
probe routine") introduced a check in the "k3_r5_rproc_mbox_callback()"
and "k3_r5_rproc_kick()" callbacks to exit if the remote core's state
was "RPROC_DETACHED". However, this caused issues in IPC-only mode, as
the default state of the core is set to "RPROC_DETACHED", and the
transition to "RPROC_ATTACHED" happens only after the "__rproc_attach()"
function has invoked "rproc_start_subdevices()".



Sounds like the core issue was making assumptions about the state of a
variable that is usually only used internally by the rproc core 
(rproc->state).



Yes that is right, so we now combined internal rproc state 
(is_attach_ongoing) with core framework's rproc->state.




Instead, what would be the harm in just dropping the state check?
Messages coming from a detached core should be processed the same.



Taking a look at k3_r5/dsp_rproc_mbox_callback(), today we don't really 
do much other than just log a print to console when we receive 
non-virtio mailbox messages. If we get a virtio mailbox message from a 
detached core, that will anyways be dropped in the further layers. So, I 
am okay with removing these checks entirely. These additional checks 
would be needed when we decide to do something more than just logging 
prints, a problem for another day though.


I will respin the series with these checks dropped.

Thanks,
Beleswar



Andrew


The "rproc_start_subdevices()" function triggers the probe of Virtio
RPMsg subdevices, which require the mailbox callbacks to be functional.
To resolve this, a new variable, "is_attach_ongoing", is introduced to
distinguish between core states: when a core is actually detached and
when it is in the process of being attached. The callbacks are updated
to return early only if the core is actually detached and not during an
ongoing attach operation in IPC-only mode.

Reported-by: Siddharth Vadapalli 
Closes: 
https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapa...@ti.com/
Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle 
during probe routine")

Signed-off-by: Beleswar Padhi 
---
Link to RFC version:
https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapa...@ti.com/

Improvements in v1:
1. Ensured these mbox callbacks are functional when the core is in 
the proccess

of getting attached in IPC-Only mode.
2. Ensured these mbox callbacks are _not_ functional when the core 
state is

actually detached.

  drivers/remoteproc/ti_k3_r5_remoteproc.c | 53 +---
  1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index dbc513c5569c..e218a803fdb5 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -131,6 +131,7 @@ struct k3_r5_cluster {
   * @btcm_enable: flag to control BTCM enablement
   * @loczrama: flag to dictate which TCM is at device address 0x0
   * @released_from_reset: flag to signal when core is out of reset
+ * @is_attach_ongoing: flag to indicate if IPC-only "attach()" is in 
progress

   */
  struct k3_r5_core {
  struct list_head elem;
@@ -148,6 +149,7 @@ struct k3_r5_core {
  u32 btcm_enable;
  u32 loczrama;
  bool released_from_reset;
+    bool is_attach_ongoing;
  };
    /**
@@ -194,8 +196,11 @@ static void k3_r5_rproc_mbox_callback(struct 
mbox_client *client, void *data)

  const char *name = kproc->rproc->name;
  u32 msg = omap_mbox_message(data);
  -    /* Do not forward message from a detached core */
-    if (kproc->rproc->state == RPROC_DETACHED)
+    /*
+ * Do not forward messages from a detached core, except when the 
core

+ * is in the process of being attached in IPC-only mode.
+ */
+    if (!kproc->core->is_attach_ongoing && kproc->rproc->state == 
RPROC_DETACHED)

  return;
    dev_dbg(dev, "mbox msg: 0x%x\n", msg);
@@ -233,8 +238,11 @@ static void k3_r5_rproc_kick(struct rproc 
*rproc, int vqid)

  mbox_msg_t msg = (mbox_msg_t)vqid;
  int ret;
  -    /* Do not forward message to a detached core */
-    if (kproc->rproc->state == RPROC_DETACHED)
+    /*
+ * Do not forward messages to a detached core, except when the 
core is

+ * in the process of being attached in IPC-only mode.
+ */
+    if (!kproc->core->is_attach_ongoing && kproc->rproc->state == 
RPROC_DETACHED)

  return;
    /* send the index of the triggered virtqueue in the mailbox 
payload */

@@ -671,22 +679,39 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  /*
   * Attach to a running R5F remote processor (IPC-only mode)
   *
- * The R5F attach callback is a NOP. The remote processor is already 
booted, and
- * all required resources have been acquired during probe routine, 
so there is
- * no need to issue any TI-SCI commands to boot the R5F cores in 

Re: [PATCH v8 00/20] Refactor TI K3 DSP and M4 Drivers

[Beleswar Prasad Padhi]

Missed few changelog. Adding below.

On 03/01/25 15:42, Beleswar Padhi wrote:

This series refactors a lot of functions & callbacks from ti_k3_dsp_remoteproc.c
and ti_k3_m4_remoteproc.c drivers. This is the third and final series as part of
the refactoring of K3 remoteproc drivers. The patches for internal refactoring
and bug fixes of TI K3 R5 remoteproc driver has been already posted[0][1]. Since
the R5 driver has worked out separate data structures and reset logic than the
DSP/M4 drivers, I have excluded R5 from this refactoring.

NOTE:
This series is _dependent_ upon the [PATCH 2/3] of below series:
https://lore.kernel.org/all/20241224091457.1050233-3-b-pa...@ti.com/

Testing Done:
1. Tested boot of C66x DSPs, C71x DSPs across Jacinto J7* devices in Remoteproc
mode and IPC-Only mode.
2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc mode and
IPC-Only mode.
3. Tested Core stop and detach operations from sysfs for C66x DSPs, C71x DSPs
and M4F.
4. Tested device removal paths by executing 'modprobe -r ti_k3_dsp_remoteproc'
and 'modprobe -r ti_k3_m4_remoteproc'.
5. Tested usecases where firmware not available at device probe time, but later
in sysfs, able to load firmware into a remotecore and start it. [C66x, C71x, M4]
6. Tested that each patch in this series generates no new warnings/errors.

v8: Changelog:
1. Broken down refactoring into patches, each patch dealing with one function
for ease in review. [Andrew]



2. Introduced checks to prevent mailbox IPC with detached M4 core.
3. Refined reset/release from reset logic for DSP cores that do not have 
a local reset in PATCH #6 and PATCH #7 of this series.
4. Refactored additional .start()/.stop()/.attach()/.detach()/mbox 
.rx_callback()/request_mbox() functions in this series.


Thanks,
Beleswar



Links to older versions:
v7: https://lore.kernel.org/all/20240202175538.1705-1-hnaga...@ti.com/
v6: https://lore.kernel.org/all/20230913111644.29889-1-hnaga...@ti.com/
v5: https://lore.kernel.org/all/20230808044529.25925-1-hnaga...@ti.com/
v4: https://lore.kernel.org/all/20230801141117.2559-1-hnaga...@ti.com/
v3: 
https://lore.kernel.org/all/20230302171450.1598576-1-martyn.we...@collabora.com/
v2: 
https://lore.kernel.org/all/2023030323.1532479-4-martyn.we...@collabora.com/
v1: https://lore.kernel.org/all/20220110040650.18186-1-hnaga...@ti.com/

Thanks,
Beleswar

[0]: https://lore.kernel.org/all/20241219110545.1898883-1-b-pa...@ti.com/
[1]: https://lore.kernel.org/all/20241224091457.1050233-1-b-pa...@ti.com/

Beleswar Padhi (20):
   remoteproc: k3-m4: Prevent Mailbox level IPC with detached core
   remoteproc: k3: Refactor shared data structures
   remoteproc: k3: Refactor mailbox rx_callback functions into common
 driver
   remoteproc: k3: Refactor .kick rproc ops into common driver
   remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info
   remoteproc: k3: Refactor rproc_reset() implementation into common
 driver
   remoteproc: k3: Refactor rproc_release() implementation into common
 driver
   remoteproc: k3: Refactor rproc_request_mbox() implementations into
 common driver
   remoteproc: k3: Refactor .prepare rproc ops into common driver
   remoteproc: k3: Refactor .unprepare rproc ops into common driver
   remoteproc: k3: Refactor .start rproc ops into common driver
   remoteproc: k3: Refactor .stop rproc ops into common driver
   remoteproc: k3: Refactor .attach rproc ops into common driver
   remoteproc: k3: Refactor .detach rproc ops into common driver
   remoteproc: k3: Refactor .get_loaded_rsc_table ops into common driver
   remoteproc: k3: Refactor .da_to_va rproc ops into common driver
   remoteproc: k3: Refactor of_get_memories() functions into common
 driver
   remoteproc: k3: Refactor mem_release() functions into common driver
   remoteproc: k3: Refactor reserved_mem_init() functions into common
 driver
   remoteproc: k3: Refactor release_tsp() functions into common driver

  drivers/remoteproc/Makefile   |   4 +-
  drivers/remoteproc/ti_k3_common.c | 586 
  drivers/remoteproc/ti_k3_common.h | 113 
  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 643 +-
  drivers/remoteproc/ti_k3_m4_remoteproc.c  | 583 ++--
  5 files changed, 765 insertions(+), 1164 deletions(-)
  create mode 100644 drivers/remoteproc/ti_k3_common.c
  create mode 100644 drivers/remoteproc/ti_k3_common.h

Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick}

[Beleswar Prasad Padhi]

On 03/01/25 16:18, Kumar, Udit wrote:


On 12/24/2024 2:44 PM, Beleswar Padhi wrote:

Commit f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during
probe routine") introduced a check in the "k3_r5_rproc_mbox_callback()"
and "k3_r5_rproc_kick()" callbacks to exit if the remote core's state
was "RPROC_DETACHED". However, this caused issues in IPC-only mode, as
the default state of the core is set to "RPROC_DETACHED", and the
transition to "RPROC_ATTACHED" happens only after the "__rproc_attach()"
function has invoked "rproc_start_subdevices()".

The "rproc_start_subdevices()" function triggers the probe of Virtio
RPMsg subdevices, which require the mailbox callbacks to be functional.
To resolve this, a new variable, "is_attach_ongoing", is introduced to
distinguish between core states: when a core is actually detached and
when it is in the process of being attached. The callbacks are updated
to return early only if the core is actually detached and not during an
ongoing attach operation in IPC-only mode.

Reported-by: Siddharth Vadapalli 
Closes: 
https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapa...@ti.com/
Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle 
during probe routine")

Signed-off-by: Beleswar Padhi 
---
Link to RFC version:
https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapa...@ti.com/

Improvements in v1:
1. Ensured these mbox callbacks are functional when the core is in 
the proccess

of getting attached in IPC-Only mode.
2. Ensured these mbox callbacks are _not_ functional when the core 
state is

actually detached.

  drivers/remoteproc/ti_k3_r5_remoteproc.c | 53 +---
  1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index dbc513c5569c..e218a803fdb5 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -131,6 +131,7 @@ struct k3_r5_cluster {
   * @btcm_enable: flag to control BTCM enablement
   * @loczrama: flag to dictate which TCM is at device address 0x0
   * @released_from_reset: flag to signal when core is out of reset
+ * @is_attach_ongoing: flag to indicate if IPC-only "attach()" is in 
progress


Variable name is misleading, Core are attached after call to rproc_add 
from this driver.



Right, but from all locations where this variable is checked 
(mbox_kick()/mbox_callback()), .attach would be still in progress.




but variables says still attach_ongoing , I suggest to use is_attached



Thought so, but that is confusing with rproc->state. E.g, in 
mbox_kick(), rproc->state = detached, but we are saying 
kproc->is_attached = true.


What do you think?





   */
  struct k3_r5_core {
  struct list_head elem;
@@ -148,6 +149,7 @@ struct k3_r5_core {
  u32 btcm_enable;
  u32 loczrama;
  bool released_from_reset;
+    bool is_attach_ongoing;
  };
    /**
@@ -194,8 +196,11 @@ static void k3_r5_rproc_mbox_callback(struct 
mbox_client *client, void *data)

  const char *name = kproc->rproc->name;
  u32 msg = omap_mbox_message(data);
  -    /* Do not forward message from a detached core */
-    if (kproc->rproc->state == RPROC_DETACHED)
+    /*
+ * Do not forward messages from a detached core, except when the 
core

+ * is in the process of being attached in IPC-only mode.
+ */
+    if (!kproc->core->is_attach_ongoing && kproc->rproc->state == 
RPROC_DETACHED)

  return;
    dev_dbg(dev, "mbox msg: 0x%x\n", msg);
@@ -233,8 +238,11 @@ static void k3_r5_rproc_kick(struct rproc 
*rproc, int vqid)

  mbox_msg_t msg = (mbox_msg_t)vqid;
  int ret;
  -    /* Do not forward message to a detached core */
-    if (kproc->rproc->state == RPROC_DETACHED)
+    /*
+ * Do not forward messages to a detached core, except when the 
core is

+ * in the process of being attached in IPC-only mode.
+ */
+    if (!kproc->core->is_attach_ongoing && kproc->rproc->state == 
RPROC_DETACHED)

  return;
    /* send the index of the triggered virtqueue in the mailbox 
payload */

@@ -671,22 +679,39 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
  /*
   * Attach to a running R5F remote processor (IPC-only mode)
   *
- * The R5F attach callback is a NOP. The remote processor is already 
booted, and
- * all required resources have been acquired during probe routine, 
so there is
- * no need to issue any TI-SCI commands to boot the R5F cores in 
IPC-only mode.

- * This callback is invoked only in IPC-only mode and exists because
- * rproc_validate() checks for its existence.
+ * The R5F attach callback only needs to set the "is_attach_ongoing" 
flag to
+ * notify k3_r5_rproc_{kick/mbox_callback} functions that the core 
is in the
+ * process of getting attached in IPC-only mode. The remote 
processor is
+ * already booted, and all required resources have been acquired 
during probe
+ * routine, so there is no need to issue

Re: [PATCH v8 00/20] Refactor TI K3 DSP and M4 Drivers

[Beleswar Prasad Padhi]

Hi Andrew,

On 08/01/25 20:33, Andrew Davis wrote:

On 1/3/25 4:12 AM, Beleswar Padhi wrote:
This series refactors a lot of functions & callbacks from 
ti_k3_dsp_remoteproc.c
and ti_k3_m4_remoteproc.c drivers. This is the third and final series 
as part of
the refactoring of K3 remoteproc drivers. The patches for internal 
refactoring
and bug fixes of TI K3 R5 remoteproc driver has been already 
posted[0][1]. Since
the R5 driver has worked out separate data structures and reset logic 
than the

DSP/M4 drivers, I have excluded R5 from this refactoring.



Diffstat looks great, 765 (+), 1164 (-), good to see all that 
duplicated code
factored away. But R5 is the largest of the 3 drivers and really needs 
it the

most.

Looking at the data structure in R5 preventing this I see what should be
the normal "struct k3_rproc" structure is really split into two,
"struct k3_r5_rproc"  and "struct k3_r5_core". The first containing a 
single

instance of the latter. There is no reason for this split I can see, just
combine the two structs.

Next, there are some members of the struct that we don't need, such as
atcm_enable and the others that are only used in probe (or functions
called as part of probe). We only use these as a way to collect this
info in one function, and use in a later one. Instead you could either
fetch this info at the time of use. Or move these members into the
cluster level "struct k3_r5_cluster".

Speaking of "struct k3_r5_cluster", it is silly for cluster to keep a 
list
of cores. There are two, and will only ever be two. No clue why a list 
was
chosen as the data structure to hold two pointers, switch this two an 
array
of size two, or even just two pointers. This also cleans up a bunch of 
the
weird "list_for_each" logic and loops that have to then check if they 
have

found with core0 or core1. Instead, just directly access core0 or core1.

That gets rid of member "struct list_head" from the combined struct,
and would you look at that, the struct now matches DSP/M4 :)

I'd suggest doing the above fixups to R5 first, then you can do
this series here after that and include R5.



Thanks a lot for suggesting this detailed plan! I agree with your 
assessment, and I will post a series addressing this.


Thanks,
Beleswar



Thanks,
Andrew


NOTE:
This series is _dependent_ upon the [PATCH 2/3] of below series:
https://lore.kernel.org/all/20241224091457.1050233-3-b-pa...@ti.com/

Testing Done:
1. Tested boot of C66x DSPs, C71x DSPs across Jacinto J7* devices in 
Remoteproc

mode and IPC-Only mode.
2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc 
mode and

IPC-Only mode.
3. Tested Core stop and detach operations from sysfs for C66x DSPs, 
C71x DSPs

and M4F.
4. Tested device removal paths by executing 'modprobe -r 
ti_k3_dsp_remoteproc'

and 'modprobe -r ti_k3_m4_remoteproc'.
5. Tested usecases where firmware not available at device probe time, 
but later
in sysfs, able to load firmware into a remotecore and start it. 
[C66x, C71x, M4]
6. Tested that each patch in this series generates no new 
warnings/errors.


v8: Changelog:
1. Broken down refactoring into patches, each patch dealing with one 
function

for ease in review. [Andrew]

Links to older versions:
v7: https://lore.kernel.org/all/20240202175538.1705-1-hnaga...@ti.com/
v6: https://lore.kernel.org/all/20230913111644.29889-1-hnaga...@ti.com/
v5: https://lore.kernel.org/all/20230808044529.25925-1-hnaga...@ti.com/
v4: https://lore.kernel.org/all/20230801141117.2559-1-hnaga...@ti.com/
v3: 
https://lore.kernel.org/all/20230302171450.1598576-1-martyn.we...@collabora.com/
v2: 
https://lore.kernel.org/all/2023030323.1532479-4-martyn.we...@collabora.com/

v1: https://lore.kernel.org/all/20220110040650.18186-1-hnaga...@ti.com/

Thanks,
Beleswar

[0]: 
https://lore.kernel.org/all/20241219110545.1898883-1-b-pa...@ti.com/
[1]: 
https://lore.kernel.org/all/20241224091457.1050233-1-b-pa...@ti.com/


Beleswar Padhi (20):
   remoteproc: k3-m4: Prevent Mailbox level IPC with detached core
   remoteproc: k3: Refactor shared data structures
   remoteproc: k3: Refactor mailbox rx_callback functions into common
 driver
   remoteproc: k3: Refactor .kick rproc ops into common driver
   remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info
   remoteproc: k3: Refactor rproc_reset() implementation into common
 driver
   remoteproc: k3: Refactor rproc_release() implementation into common
 driver
   remoteproc: k3: Refactor rproc_request_mbox() implementations into
 common driver
   remoteproc: k3: Refactor .prepare rproc ops into common driver
   remoteproc: k3: Refactor .unprepare rproc ops into common driver
   remoteproc: k3: Refactor .start rproc ops into common driver
   remoteproc: k3: Refactor .stop rproc ops into common driver
   remoteproc: k3: Refactor .attach rproc ops into common driver
   remoteproc: k3: Refactor .detach rproc ops into common driver
   remoteproc: k3: Refactor .get_loaded_rsc_table ops 

Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick}

[Beleswar Prasad Padhi]

Hi Hari,

On 27/12/24 20:08, Hari Nagalla wrote:

On 12/24/24 03:14, Beleswar Padhi wrote:

  /**
@@ -194,8 +196,11 @@ static void k3_r5_rproc_mbox_callback(struct 
mbox_client *client, void *data)

  const char *name = kproc->rproc->name;
  u32 msg = omap_mbox_message(data);
  -    /* Do not forward message from a detached core */
-    if (kproc->rproc->state == RPROC_DETACHED)
+    /*
+ * Do not forward messages from a detached core, except when the 
core

+ * is in the process of being attached in IPC-only mode.
+ */
+    if (!kproc->core->is_attach_ongoing && kproc->rproc->state == 
RPROC_DETACHED)

  return;
Instead of introducing a new state variable, is it possible to use 
device virtio status? 



See below related comment.


And i wonder what if you remove this conditional check altogether? If 
the device is detached and with no virtio queues, does not the mailbox 
message gets dropped?



This check is necessary for mailbox level communication between cores. 
Some Mbox messages directly use payloads like 
RP_MBOX_ECHO_REQUEST/RP_MBOX_CRASH etc. which do not rely on virtqueue 
read/writes for communication (see omap_remoteproc.h). In that case, 
mailbox message won't be dropped even if virtio queues are not 
initialized. IMO, when we say core is detached in "IPC-only" mode, this 
mbox communication should also not happen. What do you think?

Re: [PATCH v9 05/26] remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info

[Beleswar Prasad Padhi]

Hi Andrew,

On 07/04/25 19:13, Andrew Davis wrote:

On 3/17/25 7:06 AM, Beleswar Padhi wrote:

The ti_k3_m4_remoteproc.c driver previously hardcoded device memory
region addresses and names. Change this to use the k3_rproc_mem_data
structure to store memory information. This aligns with DSP and R5
drivers, and can be refactored out later.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_m4_remoteproc.c | 60 ++--
  1 file changed, 45 insertions(+), 15 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c 
b/drivers/remoteproc/ti_k3_m4_remoteproc.c

index d0ee7a8d460d..e83bef7cfddf 100644
--- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
@@ -20,9 +20,6 @@
  #include "remoteproc_internal.h"
  #include "ti_sci_proc.h"
  -#define K3_M4_IRAM_DEV_ADDR 0x0
-#define K3_M4_DRAM_DEV_ADDR 0x3
-


So two patches ago when you did this same thing for R5, you kept the
K3_R5_TCM_DEV_ADDR define. But here you remove the adress #defines.
I don't care if you leave them or keep them, but just do the same
either way for both M4 and R5.



Actually the K3_R5_TCM_DEV_ADDR define is used in multiple places in the 
R5 driver even after migrating to `k3_r5_rproc_mem_data`. Ex - 
k3_r5_core_of_get_internal_memories() uses this define to override the 
`kproc->mem[i].dev_addr` initially assigned by 
k3_rproc_of_get_memories() based on `loczrama`.


The M4 driver does not need these defines after migrating to 
`k3_m4_rproc_mem_data` as its IRAM/DRAM dev addresses are fixed. R5 
core's ATCM/BTCM dev addresses are not fixed.


Besides, keeping K3_M4_IRAM_DEV_ADDR will throw unused var warnings.

Thanks,
Beleswar



Andrew


  /**
   * struct k3_m4_rproc_mem - internal memory structure
   * @cpu_addr: MPU virtual address of the memory region
@@ -38,15 +35,29 @@ struct k3_m4_rproc_mem {
  };
    /**
- * struct k3_m4_rproc_mem_data - memory definitions for a remote 
processor

+ * struct k3_m4_mem_data - memory definitions for a remote processor
   * @name: name for this memory entry
   * @dev_addr: device address for the memory entry
   */
-struct k3_m4_rproc_mem_data {
+struct k3_m4_mem_data {
  const char *name;
  const u32 dev_addr;
  };
  +/**
+ * struct k3_m4_dev_data - device data structure for a M4 core
+ * @mems: pointer to memory definitions for a M4 core
+ * @num_mems: number of memory regions in @mems
+ * @boot_align_addr: boot vector address alignment granularity
+ * @uses_lreset: flag to denote the need for local reset management
+ */
+struct k3_m4_dev_data {
+    const struct k3_m4_mem_data *mems;
+    u32 num_mems;
+    u32 boot_align_addr;
+    bool uses_lreset;
+};
+
  /**
   * struct k3_m4_rproc - k3 remote processor driver structure
   * @dev: cached device pointer
@@ -56,6 +67,7 @@ struct k3_m4_rproc_mem_data {
   * @rmem: reserved memory regions data
   * @num_rmems: number of reserved memory regions
   * @reset: reset control handle
+ * @data: pointer to M4-specific device data
   * @tsp: TI-SCI processor control handle
   * @ti_sci: TI-SCI handle
   * @ti_sci_id: TI-SCI device identifier
@@ -71,6 +83,7 @@ struct k3_m4_rproc {
  struct k3_m4_rproc_mem *rmem;
  int num_rmems;
  struct reset_control *reset;
+    const struct k3_m4_dev_data *data;
  struct ti_sci_proc *tsp;
  const struct ti_sci_handle *ti_sci;
  u32 ti_sci_id;
@@ -336,14 +349,13 @@ static void *k3_m4_rproc_da_to_va(struct rproc 
*rproc, u64 da, size_t len, bool

  static int k3_m4_rproc_of_get_memories(struct platform_device *pdev,
 struct k3_m4_rproc *kproc)
  {
-    static const char * const mem_names[] = { "iram", "dram" };
-    static const u32 mem_addrs[] = { K3_M4_IRAM_DEV_ADDR, 
K3_M4_DRAM_DEV_ADDR };

+    const struct k3_m4_dev_data *data = kproc->data;
  struct device *dev = &pdev->dev;
  struct resource *res;
  int num_mems;
  int i;
  -    num_mems = ARRAY_SIZE(mem_names);
+    num_mems = kproc->data->num_mems;
  kproc->mem = devm_kcalloc(kproc->dev, num_mems,
    sizeof(*kproc->mem), GFP_KERNEL);
  if (!kproc->mem)
@@ -351,17 +363,17 @@ static int k3_m4_rproc_of_get_memories(struct 
platform_device *pdev,

    for (i = 0; i < num_mems; i++) {
  res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-   mem_names[i]);
+   data->mems[i].name);
  if (!res) {
  dev_err(dev, "found no memory resource for %s\n",
-    mem_names[i]);
+    data->mems[i].name);
  return -EINVAL;
  }
  if (!devm_request_mem_region(dev, res->start,
   resource_size(res),
   dev_name(dev))) {
  dev_err(dev, "could not request %s region for resource\n",
-    mem_names[i]);
+    data->mems[i].name);
  return -EBUSY;
  }
  @@ -369,15 +381,15 @@ st

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

[Beleswar Prasad Padhi]

Hi Andrew,

On 07/04/25 19:15, Andrew Davis wrote:

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 
---


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.



Thanks I will incorporate those changes in the revision. Please let me 
know if you have finished reviewing this patchset, so I can re-spin v10.


Thanks,
Beleswar



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

Re: [PATCH v9 01/26] remoteproc: k3-r5: Re-order internal memory initialization function

[Beleswar Prasad Padhi]

On 07/04/25 18:59, Andrew Davis wrote:

On 3/17/25 7:05 AM, Beleswar Padhi wrote:

The core's internal memory data structure will be refactored to be part
of the k3_r5_rproc structure in a future commit. As a result, internal
memory initialization will need to be performed inside
k3_r5_cluster_rproc_init() after rproc_alloc().

Therefore, move the internal memory initialization function,
k3_r5_core_of_get_internal_memories() above k3_r5_rproc_init() so that
it can be invoked from there.

Signed-off-by: Beleswar Padhi 
---


Just to keep things organized, does it make sense to also move
the other k3_r5_core_of_get_*_memories() up with this?

Also, you move k3_r5_release_tsp() up too but don't mention
that in the commit message.



Sure, I will incorporate these changes in the next revision.

Thanks,
Beleswar



Andrew


  drivers/remoteproc/ti_k3_r5_remoteproc.c | 158 +++
  1 file changed, 79 insertions(+), 79 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index dbc513c5569c..b2738b9a1b2d 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1199,6 +1199,85 @@ static int k3_r5_rproc_configure_mode(struct 
k3_r5_rproc *kproc)

  return ret;
  }
  +static int k3_r5_core_of_get_internal_memories(struct 
platform_device *pdev,

+   struct k3_r5_core *core)
+{
+    static const char * const mem_names[] = {"atcm", "btcm"};
+    struct device *dev = &pdev->dev;
+    struct resource *res;
+    int num_mems;
+    int i;
+
+    num_mems = ARRAY_SIZE(mem_names);
+    core->mem = devm_kcalloc(dev, num_mems, sizeof(*core->mem), 
GFP_KERNEL);

+    if (!core->mem)
+    return -ENOMEM;
+
+    for (i = 0; i < num_mems; i++) {
+    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+   mem_names[i]);
+    if (!res) {
+    dev_err(dev, "found no memory resource for %s\n",
+    mem_names[i]);
+    return -EINVAL;
+    }
+    if (!devm_request_mem_region(dev, res->start,
+ resource_size(res),
+ dev_name(dev))) {
+    dev_err(dev, "could not request %s region for resource\n",
+    mem_names[i]);
+    return -EBUSY;
+    }
+
+    /*
+ * TCMs are designed in general to support RAM-like backing
+ * memories. So, map these as Normal Non-Cached memories. This
+ * also avoids/fixes any potential alignment faults due to
+ * unaligned data accesses when using memcpy() or memset()
+ * functions (normally seen with device type memory).
+ */
+    core->mem[i].cpu_addr = devm_ioremap_wc(dev, res->start,
+    resource_size(res));
+    if (!core->mem[i].cpu_addr) {
+    dev_err(dev, "failed to map %s memory\n", mem_names[i]);
+    return -ENOMEM;
+    }
+    core->mem[i].bus_addr = res->start;
+
+    /*
+ * TODO:
+ * The R5F cores can place ATCM & BTCM anywhere in its address
+ * based on the corresponding Region Registers in the System
+ * Control coprocessor. For now, place ATCM and BTCM at
+ * addresses 0 and 0x4101 (same as the bus address on AM65x
+ * SoCs) based on loczrama setting
+ */
+    if (!strcmp(mem_names[i], "atcm")) {
+    core->mem[i].dev_addr = core->loczrama ?
+    0 : K3_R5_TCM_DEV_ADDR;
+    } else {
+    core->mem[i].dev_addr = core->loczrama ?
+    K3_R5_TCM_DEV_ADDR : 0;
+    }
+    core->mem[i].size = resource_size(res);
+
+    dev_dbg(dev, "memory %5s: bus addr %pa size 0x%zx va %pK da 
0x%x\n",

+    mem_names[i], &core->mem[i].bus_addr,
+    core->mem[i].size, core->mem[i].cpu_addr,
+    core->mem[i].dev_addr);
+    }
+    core->num_mems = num_mems;
+
+    return 0;
+}
+
+static void k3_r5_release_tsp(void *data)
+{
+    struct ti_sci_proc *tsp = data;
+
+    ti_sci_proc_release(tsp);
+}
+
  static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
  {
  struct k3_r5_cluster *cluster = platform_get_drvdata(pdev);
@@ -1358,78 +1437,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
  }
  }
  -static int k3_r5_core_of_get_internal_memories(struct 
platform_device *pdev,

-   struct k3_r5_core *core)
-{
-    static const char * const mem_names[] = {"atcm", "btcm"};
-    struct device *dev = &pdev->dev;
-    struct resource *res;
-    int num_mems;
-    int i;
-
-    num_mems = ARRAY_SIZE(mem_names);
-    core->mem = devm_kcalloc(dev, num_mems, sizeof(*core->mem), 
GFP_KERNEL);

-    if (!core->mem)
-    return -ENOMEM;
-
-    for (i = 0; i < num_mems; i++) {
-    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-   mem_names[i]);
-    if

Re: [PATCH v9 02/26] remoteproc: k3-r5: Refactor Data Structures to Align with DSP and M4

[Beleswar Prasad Padhi]

On 07/04/25 19:03, Andrew Davis wrote:

On 3/17/25 7:05 AM, Beleswar Padhi wrote:

Currently, struct members such as mem, num_mems, reset, tsp, ti_sci and
ti_sci_id are part of the k3_r5_core structure. To align the rproc->priv
data structure of the R5 remote processor with that of the DSP and M4,
move the above members from k3_r5_core to k3_r5_rproc.

Additionally, introduce a void *priv pointer in k3_r5_rproc that can be
typecasted to point to the k3_r5_core structure. This abstraction is
done to ensure common functionalities across R5, DSP and M4 drivers can
be refactored at a later stage.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 381 ---
  1 file changed, 198 insertions(+), 183 deletions(-)


[... snip ...]
@@ -1284,6 +1290,7 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

  struct device *dev = &pdev->dev;
  struct k3_r5_rproc *kproc;
  struct k3_r5_core *core, *core1;
+    struct device_node *np;
  struct device *cdev;
  const char *fw_name;
  struct rproc *rproc;
@@ -1292,6 +1299,7 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

  core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem);
  list_for_each_entry(core, &cluster->cores, elem) {
  cdev = core->dev;
+    np = dev_of_node(cdev);
  ret = rproc_of_parse_firmware(cdev, 0, &fw_name);
  if (ret) {
  dev_err(dev, "failed to parse firmware-name property, 
ret = %d\n",
@@ -1312,11 +1320,63 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

  rproc->recovery_disabled = true;
    kproc = rproc->priv;
-    kproc->cluster = cluster;
-    kproc->core = core;
+    kproc->priv = core;
  kproc->dev = cdev;
  kproc->rproc = rproc;
-    core->rproc = rproc;
+    core->kproc = kproc;
+
+    kproc->ti_sci = devm_ti_sci_get_by_phandle(cdev, "ti,sci");
+    if (IS_ERR(kproc->ti_sci)) {
+    ret = dev_err_probe(cdev, PTR_ERR(kproc->ti_sci),
+    "failed to get ti-sci handle\n");
+    kproc->ti_sci = NULL;
+    goto out;
+    }
+
+    ret = of_property_read_u32(np, "ti,sci-dev-id", 
&kproc->ti_sci_id);

+    if (ret) {
+    dev_err(cdev, "missing 'ti,sci-dev-id' property\n");
+    goto out;
+    }
+
+    kproc->reset = devm_reset_control_get_exclusive(cdev, NULL);
+    if (IS_ERR_OR_NULL(kproc->reset)) {
+    ret = PTR_ERR_OR_ZERO(kproc->reset);
+    if (!ret)
+    ret = -ENODEV;
+    dev_err_probe(cdev, ret, "failed to get reset handle\n");
+    goto out;
+    }
+
+    kproc->tsp = ti_sci_proc_of_get_tsp(cdev, kproc->ti_sci);
+    if (IS_ERR(kproc->tsp)) {
+    ret = dev_err_probe(cdev, PTR_ERR(kproc->tsp),
+    "failed to construct ti-sci proc control\n");
+    goto out;
+    }
+
+    ret = 
k3_r5_core_of_get_internal_memories(to_platform_device(cdev), kproc);

+    if (ret) {
+    dev_err(cdev, "failed to get internal memories, ret = 
%d\n",

+    ret);
+    goto out;
+    }
+
+    ret = ti_sci_proc_request(kproc->tsp);
+    if (ret < 0) {
+    dev_err(cdev, "ti_sci_proc_request failed, ret = %d\n", 
ret);

+    goto out;
+    }
+
+    ret = devm_add_action_or_reset(cdev, k3_r5_release_tsp, 
kproc->tsp);

+    if (ret)
+    goto out;
+    }
+
+    list_for_each_entry(core, &cluster->cores, elem) {
+    cdev = core->dev;
+    kproc = core->kproc;
+    rproc = kproc->rproc;
    ret = k3_r5_rproc_request_mbox(rproc);
  if (ret)
@@ -1330,7 +1390,7 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

    ret = k3_r5_rproc_configure(kproc);
  if (ret) {
-    dev_err(dev, "initial configure failed, ret = %d\n",
+    dev_err(cdev, "initial configure failed, ret = %d\n",
  ret);
  goto out;
  }
@@ -1340,14 +1400,14 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

    ret = k3_r5_reserved_mem_init(kproc);
  if (ret) {
-    dev_err(dev, "reserved memory init failed, ret = %d\n",
+    dev_err(cdev, "reserved memory init failed, ret = %d\n",
  ret);
  goto out;
  }
  -    ret = devm_rproc_add(dev, rproc);
+    ret = devm_rproc_add(cdev, rproc);
  if (ret) {
-    dev_err_probe(dev, ret, "rproc_add failed\n");
+    dev_err_probe(cdev, ret, "rproc_add failed\n");
  goto out;
  }
  @@ -1373,7 +1433,7 @@ static int k3_r5_cluster_rproc_init(struct 
platform_device *pdev)

 core->released_from_reset,
 msecs_to_jiffies(2000));
  if (ret 

Re: [PATCH v9 00/26] Refactor TI K3 R5, DSP and M4 Remoteproc Drivers

[Beleswar Prasad Padhi]

On 17/03/25 17:35, Beleswar Padhi wrote:

This series refactors a lot of functions & callbacks from
ti_k3_dsp_remoteproc.c, ti_k3_r5_remoteproc.c and ti_k3_m4_remoteproc.c
drivers. This is a consolidated and final series as part of the
refactoring of K3 remoteproc drivers. Below is the breakdown:
1. PATCHES #1-#5 does the pre-cleanup and aligns R5, DSP, M4 data structures.
2. PATCHES #6-#8 fixes important bugs in R5 and DSP drivers before refactoring
them into a common driver.
3. PATCHES #9-#26 does the actual refactoring from into ti_k3_common.c driver.



Requesting review on this patchset.

Thanks,
Beleswar



NOTE:
This series supersedes below series:
https://lore.kernel.org/all/20250219091042.263819-1-b-pa...@ti.com/
https://lore.kernel.org/all/20250103101231.1508151-1-b-pa...@ti.com/
https://lore.kernel.org/all/20250108063727.1416324-1-b-pa...@ti.com/

Testing Done:
1. Tested boot of R5Fs, C66x DSPs, C71x DSPs across Jacinto J7* devices in
remoteproc mode and IPC-Only mode.
2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc mode and
IPC-Only mode.
3. Tested Core stop and detach operations from sysfs for R5Fs, C66x DSPs, C71x 
DSPs
4. Tested device removal paths by executing 'modprobe -r ti_k3_dsp_remoteproc'
and 'modprobe -r ti_k3_r5_remoteproc'.
5. Tested usecases where firmware not available at device probe time, but
later in sysfs, able to load firmware into a remotecore and start it. [R5Fs]
6. Tested that each patch in this series generates no new warnings/errors.

v9: Changelog:
1. Added R5 cleanup & refactoring along with existing DSP, M4 refactoring into 
this series. [Andrew]
2. Dropped Mailbox level IPC checks across R5, DSP, M4 drivers in IPC-only 
mode. [Andrew]

Link to v8:
https://lore.kernel.org/all/20250103101231.1508151-1-b-pa...@ti.com/

v8: Changelog:
1. Broken down refactoring into patches, each patch dealing with one function
for ease in review. [Andrew]

Links to older versions:
v7: https://lore.kernel.org/all/20240202175538.1705-1-hnaga...@ti.com/
v6: https://lore.kernel.org/all/20230913111644.29889-1-hnaga...@ti.com/
v5: https://lore.kernel.org/all/20230808044529.25925-1-hnaga...@ti.com/
v4: https://lore.kernel.org/all/20230801141117.2559-1-hnaga...@ti.com/
v3: 
https://lore.kernel.org/all/20230302171450.1598576-1-martyn.we...@collabora.com/
v2:
https://lore.kernel.org/all/2023030323.1532479-4-martyn.we...@collabora.com/
v1: https://lore.kernel.org/all/20220110040650.18186-1-hnaga...@ti.com/

Thanks,
Beleswar

Beleswar Padhi (24):
   remoteproc: k3-r5: Re-order internal memory initialization function
   remoteproc: k3-r5: Refactor Data Structures to Align with DSP and M4
   remoteproc: k3-r5: Use k3_r5_rproc_mem_data structure for memory info
   remoteproc: k3-{m4/dsp}: Align internal rproc data structure with R5
   remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info
   remoteproc: k3-r5: Refactor sequential core power up/down operations
   remoteproc: k3: Refactor shared data structures
   remoteproc: k3: Refactor mailbox rx_callback functions into common
 driver
   remoteproc: k3: Refactor .kick rproc ops into common driver
   remoteproc: k3: Refactor rproc_reset() implementation into common
 driver
   remoteproc: k3: Refactor rproc_release() implementation into common
 driver
   remoteproc: k3: Refactor rproc_request_mbox() implementations into
 common driver
   remoteproc: k3: Refactor .prepare rproc ops into common driver
   remoteproc: k3: Refactor .unprepare rproc ops into common driver
   remoteproc: k3: Refactor .start rproc ops into common driver
   remoteproc: k3: Refactor .stop rproc ops into common driver
   remoteproc: k3: Refactor .attach rproc ops into common driver
   remoteproc: k3: Refactor .detach rproc ops into common driver
   remoteproc: k3: Refactor .get_loaded_rsc_table ops into common driver
   remoteproc: k3: Refactor .da_to_va rproc ops into common driver
   remoteproc: k3: Refactor of_get_memories() functions into common
 driver
   remoteproc: k3: Refactor mem_release() functions into common driver
   remoteproc: k3: Refactor reserved_mem_init() functions into common
 driver
   remoteproc: k3: Refactor release_tsp() functions into common driver

Siddharth Vadapalli (2):
   remoteproc: k3-r5: Drop check performed in
 k3_r5_rproc_{mbox_callback/kick}
   remoteproc: k3-dsp: Drop check performed in
 k3_dsp_rproc_{mbox_callback/kick}

  drivers/remoteproc/Makefile   |   4 +-
  drivers/remoteproc/ti_k3_common.c | 552 +
  drivers/remoteproc/ti_k3_common.h | 113 +++
  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 618 +--
  drivers/remoteproc/ti_k3_m4_remoteproc.c  | 583 +-
  drivers/remoteproc/ti_k3_r5_remoteproc.c  | 898 +++---
  6 files changed, 1021 insertions(+), 1747 deletions(-)
  create mode 100644 drivers/remoteproc/ti_k3_common.c
  create mode 100644 drivers/remoteproc/ti_k3_common.h

Re: [PATCH v10 13/33] remoteproc: k3: Refactor .kick rproc ops into common driver

[Beleswar Prasad Padhi]

On 21/04/25 20:28, Andrew Davis wrote:
> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>> The .kick rproc ops implementations in TI K3 R5, DSP and M4 remoteproc
>> drivers sends a mailbox message to the remote processor in the same
>> way. Refactor the implementations into a common function
>> 'k3_rproc_kick()' in the ti_k3_common.c driver.
>>
>> Signed-off-by: Beleswar Padhi 
>> ---
>> v10: Changelog:
>> None
>>
>> Link to v9:
>> https://lore.kernel.org/all/20250317120622.1746415-12-b-pa...@ti.com/
>>
>>   drivers/remoteproc/ti_k3_common.c | 25 ++
>>   drivers/remoteproc/ti_k3_common.h |  1 +
>>   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 22 +--
>>   drivers/remoteproc/ti_k3_m4_remoteproc.c  | 26 +--
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c  | 17 +--
>>   5 files changed, 29 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_common.c 
>> b/drivers/remoteproc/ti_k3_common.c
>> index 7b45e3b416186..aace308b49b0e 100644
>> --- a/drivers/remoteproc/ti_k3_common.c
>> +++ b/drivers/remoteproc/ti_k3_common.c
>> @@ -80,5 +80,30 @@ void k3_rproc_mbox_callback(struct mbox_client *client, 
>> void *data)
>>   }
>>   EXPORT_SYMBOL_GPL(k3_rproc_mbox_callback);
>>   +/*
>> + * Kick the remote processor to notify about pending unprocessed messages.
>> + * The vqid usage is not used and is inconsequential, as the kick is 
>> performed
>> + * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
>> + * the remote processor is expected to process both its Tx and Rx 
>> virtqueues.
>> + */
>
> This comment is wrong. Looks like this is a copy paste error that ended up
> in every ti_k3_*_remoteproc.c driver. This whole "simulated GPIO" thing
> is only true for the K2 DSP (keystone_remoteproc.c), all the K3 devices
> have proper mailbox interrupts.
>
> Anyway, no need to remove it here, this patch is just a refactor and since
> it was already in every driver you are factoring this out from I'd suggest
> fixing it in a later patch.


Sure, I can put a patch later to fix this comment.

Thanks,
Beleswar

>
> For this patch,
>
> Acked-by: Andrew Davis 
>
>> +void k3_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> +    struct k3_rproc *kproc = rproc->priv;
>> +    struct device *dev = kproc->dev;
>> +    u32 msg = (u32)vqid;
>> +    int ret;
>> +
>> +    /*
>> + * Send the index of the triggered virtqueue in the mailbox payload.
>> + * NOTE: msg is cast to uintptr_t to prevent compiler warnings when
>> + * void* is 64bit. It is safely cast back to u32 in the mailbox driver.
>> + */
>> +    ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
>> +    if (ret < 0)
>> +    dev_err(dev, "failed to send mailbox message, status = %d\n",
>> +    ret);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_rproc_kick);
>> +
>>   MODULE_LICENSE("GPL");
>>   MODULE_DESCRIPTION("TI K3 common Remoteproc code");
>> diff --git a/drivers/remoteproc/ti_k3_common.h 
>> b/drivers/remoteproc/ti_k3_common.h
>> index 785bb4b17d02f..6ae7ac4ec5696 100644
>> --- a/drivers/remoteproc/ti_k3_common.h
>> +++ b/drivers/remoteproc/ti_k3_common.h
>> @@ -89,4 +89,5 @@ struct k3_rproc {
>>   };
>>     void k3_rproc_mbox_callback(struct mbox_client *client, void *data);
>> +void k3_rproc_kick(struct rproc *rproc, int vqid);
>>   #endif /* REMOTEPROC_TI_K3_COMMON_H */
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index 7bd1d5a790cb2..476f4e69d2c11 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -24,26 +24,6 @@
>>     #define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK    (SZ_16M - 1)
>>   -/*
>> - * Kick the remote processor to notify about pending unprocessed messages.
>> - * The vqid usage is not used and is inconsequential, as the kick is 
>> performed
>> - * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
>> - * the remote processor is expected to process both its Tx and Rx 
>> virtqueues.
>> - */
>> -static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
>> -{
>> -    struct k3_rproc *kproc = rproc->priv;
>> -    struct device *dev = rproc->dev.parent;
>> -    mbox_msg_t msg = (mbox_msg_t)vqid;
>> -    int ret;
>> -
>> -    /* send the index of the triggered virtqueue in the mailbox payload */
>> -    ret = mbox_send_message(kproc->mbox, (void *)msg);
>> -    if (ret < 0)
>> -    dev_err(dev, "failed to send mailbox message (%pe)\n",
>> -    ERR_PTR(ret));
>> -}
>> -
>>   /* Put the DSP processor into reset */
>>   static int k3_dsp_rproc_reset(struct k3_rproc *kproc)
>>   {
>> @@ -342,7 +322,7 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, 
>> u64 da, size_t len, bool
>>   static const struct rproc_ops k3_dsp_rproc_ops = {
>>   .start    = k3_dsp_rproc_start,
>>   .stop    = k3_dsp_rproc_stop,
>> -    .kick    = k3_dsp_rp

Re: [PATCH v10 15/33] remoteproc: k3: Refactor rproc_reset() implementation into common driver

[Beleswar Prasad Padhi]

Hi Andrew,

On 21/04/25 20:12, Andrew Davis wrote:
> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>> The rproc_reset() implementations in TI K3 DSP and M4 remoteproc drivers
>> assert reset in the same way. Refactor the above function into the
>> ti_k3_common.c driver as k3_rproc_reset() and use it throughout DSP and
>> M4 drivers for resetting the remote processor.
>>
>> Signed-off-by: Beleswar Padhi 
>> ---
>> v10: Changelog:
>> 1. Split [v9 12/26] into [v10 14/33] and [v10 15/33] patches.
>>
>> Link to v9:
>> https://lore.kernel.org/all/20250317120622.1746415-13-b-pa...@ti.com/
>>
>>   drivers/remoteproc/ti_k3_common.c | 25 
>>   drivers/remoteproc/ti_k3_common.h |  1 +
>>   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 28 ++-
>>   drivers/remoteproc/ti_k3_m4_remoteproc.c  | 16 +++--
>>   4 files changed, 31 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_common.c 
>> b/drivers/remoteproc/ti_k3_common.c
>> index aace308b49b0e..19bb6c337af77 100644
>> --- a/drivers/remoteproc/ti_k3_common.c
>> +++ b/drivers/remoteproc/ti_k3_common.c
>> @@ -105,5 +105,30 @@ void k3_rproc_kick(struct rproc *rproc, int vqid)
>>   }
>>   EXPORT_SYMBOL_GPL(k3_rproc_kick);
>>   +/* Put the remote processor into reset */
>> +int k3_rproc_reset(struct k3_rproc *kproc)
>> +{
>> +    struct device *dev = kproc->dev;
>> +    int ret;
>> +
>> +    if (kproc->data->uses_lreset) {
>> +    ret = reset_control_assert(kproc->reset);
>> +    if (ret)
>> +    dev_err(dev, "local-reset assert failed (%pe)\n", ERR_PTR(ret));
>> +    return ret;
>> +    }
>> +
>> +    ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>> +    kproc->ti_sci_id);
>> +    if (ret) {
>> +    dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>> +    if (reset_control_deassert(kproc->reset))
>> +    dev_warn(dev, "local-reset deassert back failed\n");
>> +    }
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(k3_rproc_reset);
>> +
>>   MODULE_LICENSE("GPL");
>>   MODULE_DESCRIPTION("TI K3 common Remoteproc code");
>> diff --git a/drivers/remoteproc/ti_k3_common.h 
>> b/drivers/remoteproc/ti_k3_common.h
>> index 6ae7ac4ec5696..f3400fc774766 100644
>> --- a/drivers/remoteproc/ti_k3_common.h
>> +++ b/drivers/remoteproc/ti_k3_common.h
>> @@ -90,4 +90,5 @@ struct k3_rproc {
>>     void k3_rproc_mbox_callback(struct mbox_client *client, void *data);
>>   void k3_rproc_kick(struct rproc *rproc, int vqid);
>> +int k3_rproc_reset(struct k3_rproc *kproc);
>>   #endif /* REMOTEPROC_TI_K3_COMMON_H */
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index 0a8c9e61393d2..f8a5282df5b71 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -24,30 +24,6 @@
>>     #define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK    (SZ_16M - 1)
>>   -/* Put the DSP processor into reset */
>> -static int k3_dsp_rproc_reset(struct k3_rproc *kproc)
>> -{
>> -    struct device *dev = kproc->dev;
>> -    int ret;
>> -
>> -    if (kproc->data->uses_lreset) {
>> -    ret = reset_control_assert(kproc->reset);
>> -    if (ret)
>> -    dev_err(dev, "local-reset assert failed (%pe)\n", ERR_PTR(ret));
>> -    return ret;
>> -    }
>> -
>> -    ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>> -    kproc->ti_sci_id);
>> -    if (ret) {
>> -    dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>> -    if (reset_control_deassert(kproc->reset))
>> -    dev_warn(dev, "local-reset deassert back failed\n");
>> -    }
>> -
>> -    return ret;
>> -}
>> -
>>   /* Release the DSP processor from reset */
>>   static int k3_dsp_rproc_release(struct k3_rproc *kproc)
>>   {
>> @@ -201,7 +177,7 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
>>   {
>>   struct k3_rproc *kproc = rproc->priv;
>>   -    k3_dsp_rproc_reset(kproc);
>> +    k3_rproc_reset(kproc);
>>     return 0;
>>   }
>> @@ -565,7 +541,7 @@ static int k3_dsp_rproc_probe(struct platform_device 
>> *pdev)
>>   return dev_err_probe(dev, ret, "failed to get reset 
>> status\n");
>>   } else if (ret == 0) {
>>   dev_warn(dev, "local reset is deasserted for device\n");
>> -    k3_dsp_rproc_reset(kproc);
>> +    k3_rproc_reset(kproc);
>>   }
>>   }
>>   }
>> diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>> index 8a6917259ce60..7d5b75be2e4f8 100644
>> --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>> @@ -65,11 +65,9 @@ static int k3_m4_rproc_prepare(struct rproc *rproc)
>>    * Ensure the local reset is asserted so the core doesn't
>>    * execute bogus code when the module reset is releas

Re: [PATCH v10 15/33] remoteproc: k3: Refactor rproc_reset() implementation into common driver

[Beleswar Prasad Padhi]

On 22/04/25 19:51, Andrew Davis wrote:
> On 4/22/25 12:53 AM, Beleswar Prasad Padhi wrote:
>> Hi Andrew,
>>
>> On 21/04/25 20:12, Andrew Davis wrote:
>>> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>>>> The rproc_reset() implementations in TI K3 DSP and M4 remoteproc drivers
>>>> assert reset in the same way. Refactor the above function into the
>>>> ti_k3_common.c driver as k3_rproc_reset() and use it throughout DSP and
>>>> M4 drivers for resetting the remote processor.
>>>>
>>>> Signed-off-by: Beleswar Padhi 
>>>> ---
>>>> v10: Changelog:
>>>> 1. Split [v9 12/26] into [v10 14/33] and [v10 15/33] patches.
>>>>
>>>> Link to v9:
>>>> https://lore.kernel.org/all/20250317120622.1746415-13-b-pa...@ti.com/
>>>>
>>>>    drivers/remoteproc/ti_k3_common.c | 25 
>>>>    drivers/remoteproc/ti_k3_common.h |  1 +
>>>>    drivers/remoteproc/ti_k3_dsp_remoteproc.c | 28 ++-
>>>>    drivers/remoteproc/ti_k3_m4_remoteproc.c  | 16 +++--
>>>>    4 files changed, 31 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/ti_k3_common.c 
>>>> b/drivers/remoteproc/ti_k3_common.c
>>>> index aace308b49b0e..19bb6c337af77 100644
>>>> --- a/drivers/remoteproc/ti_k3_common.c
>>>> +++ b/drivers/remoteproc/ti_k3_common.c
>>>> @@ -105,5 +105,30 @@ void k3_rproc_kick(struct rproc *rproc, int vqid)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(k3_rproc_kick);
>>>>    +/* Put the remote processor into reset */
>>>> +int k3_rproc_reset(struct k3_rproc *kproc)
>>>> +{
>>>> +    struct device *dev = kproc->dev;
>>>> +    int ret;
>>>> +
>>>> +    if (kproc->data->uses_lreset) {
>>>> +    ret = reset_control_assert(kproc->reset);
>>>> +    if (ret)
>>>> +    dev_err(dev, "local-reset assert failed (%pe)\n", 
>>>> ERR_PTR(ret));
>>>> +    return ret;
>>>> +    }
>>>> +
>>>> +    ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>>> +    kproc->ti_sci_id);
>>>> +    if (ret) {
>>>> +    dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>>>> +    if (reset_control_deassert(kproc->reset))
>>>> +    dev_warn(dev, "local-reset deassert back failed\n");
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(k3_rproc_reset);
>>>> +
>>>>    MODULE_LICENSE("GPL");
>>>>    MODULE_DESCRIPTION("TI K3 common Remoteproc code");
>>>> diff --git a/drivers/remoteproc/ti_k3_common.h 
>>>> b/drivers/remoteproc/ti_k3_common.h
>>>> index 6ae7ac4ec5696..f3400fc774766 100644
>>>> --- a/drivers/remoteproc/ti_k3_common.h
>>>> +++ b/drivers/remoteproc/ti_k3_common.h
>>>> @@ -90,4 +90,5 @@ struct k3_rproc {
>>>>      void k3_rproc_mbox_callback(struct mbox_client *client, void *data);
>>>>    void k3_rproc_kick(struct rproc *rproc, int vqid);
>>>> +int k3_rproc_reset(struct k3_rproc *kproc);
>>>>    #endif /* REMOTEPROC_TI_K3_COMMON_H */
>>>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c 
>>>> b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> index 0a8c9e61393d2..f8a5282df5b71 100644
>>>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> @@ -24,30 +24,6 @@
>>>>      #define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK    (SZ_16M - 1)
>>>>    -/* Put the DSP processor into reset */
>>>> -static int k3_dsp_rproc_reset(struct k3_rproc *kproc)
>>>> -{
>>>> -    struct device *dev = kproc->dev;
>>>> -    int ret;
>>>> -
>>>> -    if (kproc->data->uses_lreset) {
>>>> -    ret = reset_control_assert(kproc->reset);
>>>> -    if (ret)
>>>> -    dev_err(dev, "local-reset assert failed (%pe)\n", 
>>>> ERR_PTR(ret));
>>>> -    return ret;
>>>> -    }
>>>> -
>>>> -    ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>>> -    kproc-&

Re: [PATCH v10 00/33] Refactor TI K3 R5, DSP and M4 Remoteproc Drivers

[Beleswar Prasad Padhi]

Hi Judith,

On 24/04/25 02:36, Judith Mendez wrote:
> Hi Beleswar,
>
> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>> This series refactors a lot of functions & callbacks from
>> ti_k3_dsp_remoteproc.c, ti_k3_r5_remoteproc.c and ti_k3_m4_remoteproc.c
>> drivers. This is a consolidated and final series as part of the
>> refactoring of K3 remoteproc drivers. Below is the breakdown:
>> 1. PATCHES #1-#3 fixes important bugs in R5 and DSP drivers before 
>> refactoring
>> them into a common driver.
>> 2. PATCHES #4-#10 does the pre-cleanup and aligns R5, DSP, M4 data 
>> structures.
>> 3. PATCHES #11-#33 does the actual refactoring R5, DSP and M4 drivers into
>> ti_k3_common.c driver.
>>
>
> I noticed that for am62ax DSP, local reset is not enabled, which is an
> issue, but I see that it was not enabled before your patches so it could
> be a follow-up patch once these patches are merged.


Yes that is planned after current refactoring series is merged.

>
> Also, I have tested basic functionality on am64x EVM: 
> https://gist.github.com/jmenti/9e7fb3cbb7a34fc1800092e8fa6cce87
>
> so for the series,
>
> Tested-by: Judith Mendez 


Thanks a lot for testing.
Beleswar

>
>
>
>

Re: [PATCH 2/2] remoteproc: k3-r5: Refactor Data Structures to Align with DSP and M4

[Beleswar Prasad Padhi]

Hi Andrew,

On 26/02/25 20:14, Andrew Davis wrote:

On 2/19/25 3:10 AM, Beleswar Padhi wrote:

Currently, struct members such as mem, num_mems, reset, tsp, ti_sci and
ti_sci_id are part of the k3_r5_core structure. To align the rproc->priv
data structure of the R5 remote processor with that of the DSP and M4,
move the above members from k3_r5_core to k3_r5_rproc.

Additionally, introduce a void *priv pointer in k3_r5_rproc that can be
typecasted to point to the k3_r5_core structure. This abstraction is
done to ensure common functionalities across R5, DSP and M4 drivers can
be refactored at a later stage.

Signed-off-by: Beleswar Padhi 
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 380 ---
  1 file changed, 197 insertions(+), 183 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c

index b2738b9a1b2d..ae7d0ea27e40 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -114,19 +114,16 @@ struct k3_r5_cluster {
  const struct k3_r5_soc_data *soc_data;
  };
  +struct k3_r5_rproc;
+
  /**
   * struct k3_r5_core - K3 R5 core structure
   * @elem: linked list item
   * @dev: cached device pointer
- * @rproc: rproc handle representing this core
- * @mem: internal memory regions data
+ * @kproc: K3 rproc handle representing this core
+ * @cluster: cached pointer to parent cluster structure
   * @sram: on-chip SRAM memory regions data
- * @num_mems: number of internal memory regions
   * @num_sram: number of on-chip SRAM memory regions
- * @reset: reset control handle
- * @tsp: TI-SCI processor control handle
- * @ti_sci: TI-SCI handle
- * @ti_sci_id: TI-SCI device identifier
   * @atcm_enable: flag to control ATCM enablement
   * @btcm_enable: flag to control BTCM enablement
   * @loczrama: flag to dictate which TCM is at device address 0x0
@@ -135,15 +132,10 @@ struct k3_r5_cluster {
  struct k3_r5_core {
  struct list_head elem;
  struct device *dev;
-    struct rproc *rproc;
-    struct k3_r5_mem *mem;
+    struct k3_r5_rproc *kproc;
+    struct k3_r5_cluster *cluster;
  struct k3_r5_mem *sram;
-    int num_mems;
  int num_sram;
-    struct reset_control *reset;
-    struct ti_sci_proc *tsp;
-    const struct ti_sci_handle *ti_sci;
-    u32 ti_sci_id;
  u32 atcm_enable;
  u32 btcm_enable;
  u32 loczrama;
@@ -153,23 +145,33 @@ struct k3_r5_core {
  /**
   * struct k3_r5_rproc - K3 remote processor state
   * @dev: cached device pointer
- * @cluster: cached pointer to parent cluster structure
- * @mbox: mailbox channel handle
- * @client: mailbox client to request the mailbox channel
   * @rproc: rproc handle
- * @core: cached pointer to r5 core structure being used
+ * @mem: internal memory regions data
+ * @num_mems: number of internal memory regions
   * @rmem: reserved memory regions data
   * @num_rmems: number of reserved memory regions
+ * @reset: reset control handle
+ * @tsp: TI-SCI processor control handle
+ * @ti_sci: TI-SCI handle
+ * @ti_sci_id: TI-SCI device identifier
+ * @mbox: mailbox channel handle
+ * @client: mailbox client to request the mailbox channel
+ * @priv: Remote processor private data
   */
  struct k3_r5_rproc {
  struct device *dev;
-    struct k3_r5_cluster *cluster;
-    struct mbox_chan *mbox;
-    struct mbox_client client;
  struct rproc *rproc;
-    struct k3_r5_core *core;
+    struct k3_r5_mem *mem;
+    int num_mems;
  struct k3_r5_mem *rmem;
  int num_rmems;
+    struct reset_control *reset;
+    struct ti_sci_proc *tsp;
+    const struct ti_sci_handle *ti_sci;
+    u32 ti_sci_id;
+    struct mbox_chan *mbox;


So you are just re-ordering these member variable? Might be good to
just keep them in the old spot to make this patch easier to parse.



Not just re-ordering. We have moved the mem, num_mems, reset, tsp, 
ti_sci, ti_sci_id from k3_r5_core to k3_r5_rproc. Then re-ordering was 
done to match the k3_m4_rproc/k3_dsp_rproc struct. But right, can skip 
the re-ordering part!





+    struct mbox_client client;
+    void *priv;


Why do we need this "priv" again?



Idea for having a priv void pointer is to let the R5/DSP/M4 individual 
drivers have an instance of any private data structure they want to. In 
case of R5, this would be k3_r5_core, in case of DSP/M4 priv can be a 
null pointer. Some functions like k3_r5_rproc_prepare, 
k3_r5_rproc_start, k3_r5_rproc_configure etc. need to extract the R5 
private DS k3_r5_core somehow indirectly from the 'rproc' pointer. This 
void priv allows to do that while ensuring we can align all the 
R5/DSP/M4 data structures into "k3_rproc".




The othes (k3_m4_rproc/k3_dsp_rproc)
do not have this, so you'll have to drop it to make these structs
common at some point.



Currently not. Instead of dropping the priv, we will add the priv in 
DSP/M4 data structures (can be null ptrs which will never be 
dereferenced) in the revision of following series[0],


[

Re: [PATCH v11 12/35] remoteproc: k3: Refactor mailbox rx_callback functions into common driver

[Beleswar Prasad Padhi]

Hi Mathieu,

On 08/05/25 21:16, Mathieu Poirier wrote:
> On Fri, Apr 25, 2025 at 04:11:12PM +0530, Beleswar Padhi wrote:
>> The mailbox .rx_callback implementations in TI K3 R5, DSP and M4
>> remoteproc drivers handle inbound mailbox messages in the same way.
>> Introduce a common driver 'ti_k3_common.c' and refactor the
>> implementations into a common function 'k3_rproc_mbox_callback'() in it.
>>
>> Signed-off-by: Beleswar Padhi 
>> Tested-by: Judith Mendez 
>> ---
>> v11: Changelog:
>> 1. Carried T/B tag.
>>
>> Link to v10:
>> https://lore.kernel.org/all/20250417182001.3903905-13-b-pa...@ti.com/
>>
>> v10: Changelog:
>> None
>>
>> Link to v9:
>> https://lore.kernel.org/all/20250317120622.1746415-11-b-pa...@ti.com/
>>
>>  drivers/remoteproc/Makefile   |  4 +-
>>  drivers/remoteproc/ti_k3_common.c | 84 +++
>>  drivers/remoteproc/ti_k3_common.h |  1 +
>>  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 50 +-
>>  drivers/remoteproc/ti_k3_m4_remoteproc.c  | 49 +
>>  drivers/remoteproc/ti_k3_r5_remoteproc.c  | 50 +-
>>  6 files changed, 90 insertions(+), 148 deletions(-)
>>  create mode 100644 drivers/remoteproc/ti_k3_common.c
>>
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index 5ff4e2fee4abd..e30908ca4bfcd 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -36,7 +36,7 @@ obj-$(CONFIG_RCAR_REMOTEPROC)  += rcar_rproc.o
>>  obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
>>  obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
>>  obj-$(CONFIG_STM32_RPROC)   += stm32_rproc.o
>> -obj-$(CONFIG_TI_K3_DSP_REMOTEPROC)  += ti_k3_dsp_remoteproc.o
>> -obj-$(CONFIG_TI_K3_M4_REMOTEPROC)   += ti_k3_m4_remoteproc.o
>> +obj-$(CONFIG_TI_K3_DSP_REMOTEPROC)  += ti_k3_dsp_remoteproc.o ti_k3_common.o
>> +obj-$(CONFIG_TI_K3_M4_REMOTEPROC)   += ti_k3_m4_remoteproc.o ti_k3_common.o
>>  obj-$(CONFIG_TI_K3_R5_REMOTEPROC)   += ti_k3_r5_remoteproc.o
> The R5 driver doesn't need to be compile with ti_k3_common.c?


Thanks for catching this! I will fix this in revision.

All the existing K3 devices had one of the DSP/M4 rprocs along with a R5 rproc. 
So this was never caught with tests.

Thanks,
Beleswar

>
>>  obj-$(CONFIG_XLNX_R5_REMOTEPROC)+= xlnx_r5_remoteproc.o
>> diff --git a/drivers/remoteproc/ti_k3_common.c 
>> b/drivers/remoteproc/ti_k3_common.c
>> new file mode 100644
>> index 0..7b45e3b416186
>> --- /dev/null
>> +++ b/drivers/remoteproc/ti_k3_common.c
>> @@ -0,0 +1,84 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * TI K3 Remote Processor(s) driver common code
>> + *
>> + * Refactored out of ti_k3_r5_remoteproc.c, ti_k3_dsp_remoteproc.c and
>> + * ti_k3_m4_remoteproc.c.
>> + *
>> + * ti_k3_dsp_remoteproc.c:
>> + * Copyright (C) 2018-2022 Texas Instruments Incorporated - 
>> https://www.ti.com/
>> + *  Suman Anna 
>> + *
>> + * ti_k3_m4_remoteproc.c:
>> + * Copyright (C) 2021-2024 Texas Instruments Incorporated - 
>> https://www.ti.com/
>> + *  Hari Nagalla 
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "omap_remoteproc.h"
>> +#include "remoteproc_internal.h"
>> +#include "ti_sci_proc.h"
>> +#include "ti_k3_common.h"
>> +
>> +/**
>> + * k3_rproc_mbox_callback() - inbound mailbox message handler
>> + * @client: mailbox client pointer used for requesting the mailbox channel
>> + * @data: mailbox payload
>> + *
>> + * This handler is invoked by the K3 mailbox driver whenever a mailbox
>> + * message is received. Usually, the mailbox payload simply contains
>> + * the index of the virtqueue that is kicked by the remote processor,
>> + * and we let remoteproc core handle it.
>> + *
>> + * In addition to virtqueue indices, we also have some out-of-band values
>> + * that indicate different events. Those values are deliberately very
>> + * large so they don't coincide with virtqueue indices.
>> + */
>> +void k3_rproc_mbox_callback(struct mbox_client *client, void *data)
>> +{
>> +struct k3_rproc *kproc = container_of(client, struct k3_rproc, client);
>> +struct device *dev = kproc->rproc->dev.parent;
>> +struct rproc *rproc = kproc->rproc;
>> +u32 msg = (u32)(uintptr_t)(data);
>> +
>> +dev_dbg(dev, "mbox msg: 0x%x\n", msg);
>> +
>> +switch (msg) {
>> +case RP_MBOX_CRASH:
>> +/*
>> + * remoteproc detected an exception, but error recovery is not
>> + * supported. So, just log this for now
>> + */
>> +dev_err(dev, "K3 rproc %s crashed\n", rproc->name);
>> +break;
>> +case RP_MBOX_ECHO_REPLY:
>> +dev_info(dev, "received echo reply from %s\n", rproc->name);
>> +break;
>> +default:
>> +/* silently handle all other valid messages */
>> +if (msg >= R

Re: [PATCH v11 00/35] Refactor TI K3 R5, DSP and M4 Remoteproc Drivers

[Beleswar Prasad Padhi]

Hi Mathieu,

On 09/05/25 22:39, Mathieu Poirier wrote:
> On Fri, Apr 25, 2025 at 04:11:00PM +0530, Beleswar Padhi wrote:
>> This series refactors a lot of functions & callbacks from
>> ti_k3_dsp_remoteproc.c, ti_k3_r5_remoteproc.c and ti_k3_m4_remoteproc.c
>> drivers. This is a consolidated and final series as part of the
>> refactoring of K3 remoteproc drivers. Below is the breakdown:
>> 1. PATCHES #1-#3 fixes important bugs in R5 and DSP drivers before 
>> refactoring
>> them into a common driver.
>> 2. PATCHES #4-#10 does the pre-cleanup and aligns R5, DSP, M4 data 
>> structures.
>> 3. PATCHES #11-#35 does the actual refactoring R5, DSP and M4 drivers into
>> ti_k3_common.c driver.
>>
>> NOTE:
>> This series supersedes below series:
>> https://lore.kernel.org/all/20250219091042.263819-1-b-pa...@ti.com/
>> https://lore.kernel.org/all/20250417182001.3903905-1-b-pa...@ti.com/
>> https://lore.kernel.org/all/20250108063727.1416324-1-b-pa...@ti.com/
>>
>> Testing Done:
>> 1. Tested boot of R5Fs, C66x DSPs, C71x DSPs across Jacinto J7* devices in
>> remoteproc mode and IPC-Only mode.
>> 2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc mode and
>> IPC-Only mode.
>> 3. Tested Core stop and detach operations from sysfs for R5Fs, C66x DSPs, 
>> C71x DSPs
>> 4. Tested device removal paths by executing 'modprobe -r 
>> ti_k3_dsp_remoteproc'
>> and 'modprobe -r ti_k3_r5_remoteproc'.
>> 5. Tested usecases where firmware not available at device probe time, but
>> later in sysfs, able to load firmware into a remotecore and start it. [R5Fs]
>> 6. Tested that each patch in this series generates no new warnings/errors.
>> 7. Tested IPC on AM64x EVM Device. [Thanks to Judith].
>>
>> v11: Changelog:
>> 1. New patches: [v11 15/35] and [v11 18/35].
>> Broken down rproc_reset() and rproc_release() refactoring patches into more
>> atomic changes.
>> 2. Carried T/B on all patches from Judith.
>> 3. Carried A/B on [PATCH v11 13/35] by Andrew.
>>
>> Link to v10:
>> https://lore.kernel.org/all/20250417182001.3903905-1-b-pa...@ti.com/
>>
>> v10: Changelog:
>> 1. Re-ordered Bug Fixes to the start of the series, before starting 
>> pre-cleanup
>> and finally the actual refactor. [Andrew]
>> 2. Broken down commits into more atomic changes for ease of review. [Andrew].
>> 3. Updated commit messages to have uniform flow throughout the series.
>> 4. Carried R/B tags in applicable patches.
>> 5. Further patch specific changelog is attached with patches.
>>
>> Link to v9:
>> https://lore.kernel.org/all/20250317120622.1746415-1-b-pa...@ti.com/
>>
>> v9: Changelog:
>> 1. Added R5 cleanup & refactoring along with existing DSP, M4 refactoring 
>> into this series. [Andrew]
>> 2. Dropped Mailbox level IPC checks across R5, DSP, M4 drivers in IPC-only 
>> mode. [Andrew] 
>>
>> Link to v8:
>> https://lore.kernel.org/all/20250103101231.1508151-1-b-pa...@ti.com/
>>
>> v8: Changelog:
>> 1. Broken down refactoring into patches, each patch dealing with one function
>> for ease in review. [Andrew]
>>
>> Links to older versions:
>> v7: https://lore.kernel.org/all/20240202175538.1705-1-hnaga...@ti.com/
>> v6: https://lore.kernel.org/all/20230913111644.29889-1-hnaga...@ti.com/
>> v5: https://lore.kernel.org/all/20230808044529.25925-1-hnaga...@ti.com/
>> v4: https://lore.kernel.org/all/20230801141117.2559-1-hnaga...@ti.com/
>> v3: 
>> https://lore.kernel.org/all/20230302171450.1598576-1-martyn.we...@collabora.com/
>> v2: 
>> https://lore.kernel.org/all/2023030323.1532479-4-martyn.we...@collabora.com/
>> v1: https://lore.kernel.org/all/20220110040650.18186-1-hnaga...@ti.com/
>>
>> Thanks,
>> Beleswar
>>
>> Beleswar Padhi (33):
>>   remoteproc: k3-r5: Refactor sequential core power up/down operations
>>   remoteproc: k3-r5: Re-order internal memory initialization functions
>>   remoteproc: k3-r5: Re-order k3_r5_release_tsp() function
>>   remoteproc: k3-r5: Refactor Data Structures to Align with DSP and M4
>>   remoteproc: k3-r5: Use k3_r5_rproc_mem_data structure for memory info
>>   remoteproc: k3-{m4/dsp}: Add a void ptr member in rproc internal
>> struct
>>   remoteproc: k3-m4: Add pointer to rproc struct within k3_m4_rproc
>>   remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info
>>   remoteproc: k3: Refactor shared data structures
>>   remoteproc: k3: Refactor mailbox rx_callback functions into common
>> driver
>>   remoteproc: k3: Refactor .kick rproc ops into common driver
>>   remoteproc: k3-dsp: Correct Reset logic for devices without lresets
>>   remoteproc: k3-m4: Introduce central function to put rproc into reset
>>   remoteproc: k3: Refactor rproc_reset() implementation into common
>> driver
>>   remoteproc: k3-dsp: Correct Reset deassert logic for devices w/o
>> lresets
>>   remoteproc: k3-m4: Introduce central function to release rproc from
>> reset
>>   remoteproc: k3: Refactor rproc_release() implementation into common
>> driver
>>   remoteproc: k3-m4: Ping the mbox while acquirin

Re: [PATCH v12 04/36] remoteproc: k3-m4: Don't assert reset in detach routine

[Beleswar Prasad Padhi]

On 5/16/2025 9:15 PM, Mathieu Poirier wrote:

On Tue, May 13, 2025 at 11:14:38AM +0530, Beleswar Padhi wrote:

The rproc_detach() function invokes __rproc_detach() before
rproc_unprepare_device(). The __rproc_detach() function sets the
rproc->state to "RPROC_DETACHED".

However, the TI K3 M4 driver erroneously looks for "RPROC_ATTACHED"
state in its .unprepare ops to identify IPC-only mode; which leads to
resetting the rproc in detach routine.

Therefore, correct the IPC-only mode detection logic to look for
"RPROC_DETACHED" in k3_m4_rproc_unprepare() function.


This driver has been upstream for 9 whole months, it is hard for me to believe
this but was just noticed.  Martyn from Collabora should be CC'ed on this, and I
will also need the required R-b/T-b tags.



Cc: Martyn Welch martyn.we...@collabora.com

Requesting Andrew/Judith for review and test too.



Typically bug fixes are not part of refactoring exercises.



Typically, yes. But the refactor depends on this fix. This 
k3_m4_rproc_unprepare() function is entirely refactored to common driver 
in [PATCH v12 26/36].


So, If the refactor is picked without this patch fix, the mainline M4 
driver would be fixed, but the older stable kernels would always have 
this bug. Let me know what you think.


Thanks,
Beleswar


  I suggest to apply
this set without this patch - you can then work on fixing this bug.

Thanks,
Mathieu


Fixes: ebcf9008a895 ("remoteproc: k3-m4: Add a remoteproc driver for M4F 
subsystem")
Signed-off-by: Beleswar Padhi 
---
v12: Changelog:
1. New patch. Fixup a state detection logic.

  drivers/remoteproc/ti_k3_m4_remoteproc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c 
b/drivers/remoteproc/ti_k3_m4_remoteproc.c
index a16fb165fcedd..6cd50b16a8e82 100644
--- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
@@ -228,7 +228,7 @@ static int k3_m4_rproc_unprepare(struct rproc *rproc)
int ret;
  
  	/* If the core is going to be detached do not assert the module reset */

-   if (rproc->state == RPROC_ATTACHED)
+   if (rproc->state == RPROC_DETACHED)
return 0;
  
  	ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,

--
2.34.1

Re: [PATCH v12 04/36] remoteproc: k3-m4: Don't assert reset in detach routine

[Beleswar Prasad Padhi]

Hi Mathieu,

On 19/05/25 20:07, Mathieu Poirier wrote:
> On Sat, May 17, 2025 at 06:53:29PM +0530, Beleswar Prasad Padhi wrote:
>> On 5/16/2025 9:15 PM, Mathieu Poirier wrote:
>>> On Tue, May 13, 2025 at 11:14:38AM +0530, Beleswar Padhi wrote:
>>>> The rproc_detach() function invokes __rproc_detach() before
>>>> rproc_unprepare_device(). The __rproc_detach() function sets the
>>>> rproc->state to "RPROC_DETACHED".
>>>>
>>>> However, the TI K3 M4 driver erroneously looks for "RPROC_ATTACHED"
>>>> state in its .unprepare ops to identify IPC-only mode; which leads to
>>>> resetting the rproc in detach routine.
>>>>
>>>> Therefore, correct the IPC-only mode detection logic to look for
>>>> "RPROC_DETACHED" in k3_m4_rproc_unprepare() function.
>>>>
>>> This driver has been upstream for 9 whole months, it is hard for me to 
>>> believe
>>> this but was just noticed.  Martyn from Collabora should be CC'ed on this, 
>>> and I
>>> will also need the required R-b/T-b tags.
>>
>> Cc: Martyn Welch martyn.we...@collabora.com
>>
>> Requesting Andrew/Judith for review and test too.
>>
>>> Typically bug fixes are not part of refactoring exercises.
>>
>> Typically, yes. But the refactor depends on this fix. This
>> k3_m4_rproc_unprepare() function is entirely refactored to common driver in
>> [PATCH v12 26/36].
>>
>> So, If the refactor is picked without this patch fix, the mainline M4 driver
>> would be fixed, but the older stable kernels would always have this bug. Let
>> me know what you think.
>>
> I suggest you send this patch on its own and then the series (without this
> patch) with a note in the cover letter that it depends on the fix.  That way 
> we
> get the best of both worlds.


Sure. If I get any comments/reviews on this version, I will re-spin this patch 
separately than the series.

Thanks,
Beleswar

>
>> Thanks,
>> Beleswar
>>
>>>   I suggest to apply
>>> this set without this patch - you can then work on fixing this bug.
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> Fixes: ebcf9008a895 ("remoteproc: k3-m4: Add a remoteproc driver for M4F 
>>>> subsystem")
>>>> Signed-off-by: Beleswar Padhi 
>>>> ---
>>>> v12: Changelog:
>>>> 1. New patch. Fixup a state detection logic.
>>>>
>>>>   drivers/remoteproc/ti_k3_m4_remoteproc.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c 
>>>> b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>>> index a16fb165fcedd..6cd50b16a8e82 100644
>>>> --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>>> @@ -228,7 +228,7 @@ static int k3_m4_rproc_unprepare(struct rproc *rproc)
>>>>int ret;
>>>>/* If the core is going to be detached do not assert the module reset */
>>>> -  if (rproc->state == RPROC_ATTACHED)
>>>> +  if (rproc->state == RPROC_DETACHED)
>>>>return 0;
>>>>ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>>> -- 
>>>> 2.34.1
>>>>

Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Beleswar Prasad Padhi]

Hi Dawei,

On 30/05/25 18:20, Dawei Li wrote:
> HI Beleswar,
>
> Thanks for reviewing.
>
> On Fri, May 30, 2025 at 03:15:28PM +0530, Beleswar Prasad Padhi wrote:
>> Hi Dawei,
>>
>> On 19/05/25 20:38, Dawei Li wrote:
>>> Implement RPMSG_CREATE_EPT_FD_IOCTL, new uAPI for rpmsg ctrl, which
>>> shares most of operations of RPMSG_CREATE_EPT_IOCTL except that it
>>> returns fd representing eptdev to userspace directly.
>>>
>>> Possible calling procedures for userspace are:
>>> - fd = open("/dev/rpmsg_ctrlX")
>>> - ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &info);
>>> - fd_ep = info.fd
>>
>> We are returning a new fd to userspace from inside an IOCTL itself. Is this a
>> standard way of doing things in Kernel space? (see below related comment)
> Yes, anon_get_{fd,file} are used extensively in kernel for returning a new
> fd to userspace which is associated with an unique data structure in kernel
> space, in different ways:
>
> - via ioctl(), some examples are:
>
>  - KVM ioctl(s)
>- KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
>- KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
>- KVM_CREATE_DEVICE -> kvm_ioctl_create_device
>- KVM_CREATE_VM -> kvm_dev_ioctl_create_vm 
>
>  - DMA buf/fence/sync ioctls
>- DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
>- SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
>- Couples of driver implement DMA buf by using anon file _implicitly_:
>  - UDMABUF_CREATE -> udmabuf_ioctl_create
>  - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
>
>  - gpiolib ioctls:
>- GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
>- GPIO_V2_GET_LINE_IOCTL
>
>  -  IOMMUFD ioctls:
>
>  -  VFIO Ioctls:
>
>  - 
>
>
> - via other specific syscalls:
>  - epoll_create1
>  - bpf 
>  - perf_event_open
>  - inotify_init
>  - ...


Thank you for the extensive list of examples!

>
>>> - operations on fd_ep(write, read, poll ioctl)
>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
>>> - close(fd_ep)
>>
>> Can we rely on the userspace to close() the fd_ep? (if not done, could be a
>> memory leak..).. Opposed to fd, which we can rely on the userspace to
>> close() since they initiated the open() call. I am just trying to understand 
>> if
>> this is a standard way of doing things...
> Good question.
>
> When userland gets a fd from kernel, it's userland's duty to manage and 
> release
> the resource when it's done with it, because kernel never knows when the fd 
> and
> its resourcen are not needed by userland except process is on exiting. The 
> fact
> remains true no matter how fd is generated from kernel:
> - open()
> - ioctl()
> - Other syscalls(epoll_create1, e.g, as listed above)
>
> As a result, kernel & driver provide fops->release() to achieve resource
> release when fd is not needed for userland, some callers of it maybe:
> - Userland call close() explicitly
> - Kernel does the dirty job when user process exits(if some fds are
>   still opened):
>   - Userland call exit() explicitly.
>   - User process was killed by some signals.
>
> Maybe some comments/docs are needed in uAPI?


Perhaps yes. It makes sense to me now. Thanks for addressing my queries!

>
>>> - close(fd)
>>>
> [snip]
>
>>> +
>>> +   if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
>>> +   cmd == RPMSG_RELEASE_DEV_IOCTL) {
>>> +   if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>> +   return -EFAULT;
>>> +
>>> +   memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>>> +   chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>> +   chinfo.src = eptinfo.src;
>>> +   chinfo.dst = eptinfo.dst;
>>> +   } else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
>>
>> Maybe we can put this 'else if condition' in the first 'if' and treat other
>> conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
>> ioctl with a different struct type.
> Good point! I will try to address it in next respin.


Thanks,
Beleswar

>
>> Thanks,
>> Beleswar
>>
>>> +   if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
>>> +   return -EFAULT;
>>> +
>>> +   memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
>>> +   chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>> +   chinfo.src = ept_fd_info.src;
>>> +   chinfo.dst = ept_fd_info.dst;
>>> +   }
>>>  
> [snip]
>
> Thanks,
>
>   Dawei

Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Beleswar Prasad Padhi]

Hi Dawei,

On 19/05/25 20:38, Dawei Li wrote:
> Implement RPMSG_CREATE_EPT_FD_IOCTL, new uAPI for rpmsg ctrl, which
> shares most of operations of RPMSG_CREATE_EPT_IOCTL except that it
> returns fd representing eptdev to userspace directly.
>
> Possible calling procedures for userspace are:
> - fd = open("/dev/rpmsg_ctrlX")
> - ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &info);
> - fd_ep = info.fd


We are returning a new fd to userspace from inside an IOCTL itself. Is this a
standard way of doing things in Kernel space? (see below related comment)

> - operations on fd_ep(write, read, poll ioctl)
> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> - close(fd_ep)


Can we rely on the userspace to close() the fd_ep? (if not done, could be a
memory leak..).. Opposed to fd, which we can rely on the userspace to
close() since they initiated the open() call. I am just trying to understand if
this is a standard way of doing things...

> - close(fd)
>
> Signed-off-by: Dawei Li 
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 38 ++
>  include/uapi/linux/rpmsg.h | 24 
>  2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 28f57945ccd9..9f2f118ceb7b 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -75,19 +75,32 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned 
> int cmd,
>   unsigned long arg)
>  {
>   struct rpmsg_ctrldev *ctrldev = fp->private_data;
> + struct rpmsg_endpoint_fd_info ept_fd_info;
>   void __user *argp = (void __user *)arg;
>   struct rpmsg_endpoint_info eptinfo;
>   struct rpmsg_channel_info chinfo;
>   struct rpmsg_device *rpdev;
>   int ret = 0;
> -
> - if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> - return -EFAULT;
> -
> - memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> - chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> - chinfo.src = eptinfo.src;
> - chinfo.dst = eptinfo.dst;
> + int fd = -1;
> +
> + if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
> + cmd == RPMSG_RELEASE_DEV_IOCTL) {
> + if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> + return -EFAULT;
> +
> + memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> + chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> + chinfo.src = eptinfo.src;
> + chinfo.dst = eptinfo.dst;
> + } else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {


Maybe we can put this 'else if condition' in the first 'if' and treat other
conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
ioctl with a different struct type.

Thanks,
Beleswar

> + if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
> + return -EFAULT;
> +
> + memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
> + chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> + chinfo.src = ept_fd_info.src;
> + chinfo.dst = ept_fd_info.dst;
> + }
>  
>   mutex_lock(&ctrldev->ctrl_lock);
>   switch (cmd) {
> @@ -110,6 +123,15 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, 
> unsigned int cmd,
>   chinfo.name, ret);
>   break;
>  
> + case RPMSG_CREATE_EPT_FD_IOCTL:
> + ret = rpmsg_anonymous_eptdev_create(ctrldev->rpdev, 
> &ctrldev->dev, chinfo,
> + ept_fd_info.flags, &fd);
> + if (!ret) {
> + ept_fd_info.fd = fd;
> + ret = copy_to_user(argp, &ept_fd_info, 
> sizeof(ept_fd_info));
> + }
> + break;
> +
>   default:
>   ret = -EINVAL;
>   }
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index f0c8da2b185b..e7057bd23577 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -53,4 +53,28 @@ struct rpmsg_endpoint_info {
>   */
>  #define RPMSG_SET_INCOMING_FLOWCONTROL _IOR(0xb5, 0x6, int)
>  
> +/**
> + * struct rpmsg_endpoint_fd_info - endpoint & fd info representation
> + * @name: name of service
> + * @src: local address. To set to RPMSG_ADDR_ANY if not used.
> + * @dst: destination address. To set to RPMSG_ADDR_ANY if not used.
> + * @flags: file flags of endpoint device, valid flags:
> + * O_RDONLY/O_WRONLY/O_RDWR
> + * O_NONBLOCK
> + * O_CLOEXEC
> + * @fd: fd returned from driver
> + */
> +struct rpmsg_endpoint_fd_info {
> + char name[32];
> + __u32 src;
> + __u32 dst;
> + __u32 flags;
> + __s32 fd;
> +};
> +
> +/**
> + * Instantiate a new rmpsg endpoint which is represented by fd
> + */
> +#define RPMSG_CREATE_EPT_FD_IOCTL _IOWR(0xb5, 0x7, struct 
> rpmsg_endpoint_fd_info)
> +
>  #endif

Re: [PATCH v4] remoteproc: k3-dsp: Fix an error handling path in k3_dsp_rproc_probe()

[Beleswar Prasad Padhi]

On 03/07/25 21:57, Mathieu Poirier wrote:
> Andrew, Hari and Beleswar - please test this on your side and get back to me
> with the results.
>
> Thanks,
> Mathieu
>
> On Fri, Jun 27, 2025 at 11:42:33PM +0200, Christophe JAILLET wrote:
>> IF an error occurs after a successful k3_rproc_request_mbox() call,
>> mbox_free_channel() should be called to avoid a leak.
>>
>> Such a call is missing in the error handing path of k3_dsp_rproc_probe().
>> It is also missing both in the error handling path of k3_m4_rproc_probe()
>> and in (in-existent) corresponding remove function.
>>
>> Switch to managed resources to avoid these leaks and simplify the code.
>>
>> Signed-off-by: Christophe JAILLET 


The subject line seems to be calling out changes only in dsp driver,
however patch takes care of all DSP/M4/R5 drivers. Maybe
something to update.

With above addressed,

Reviewed-by: Beleswar Padhi 
Tested-by: Beleswar Padhi 

Tested IPC with Normal boot/Late Attach and in various core
configurations: Lockstep/Split/Single CPU etc.

Thanks,
Beleswar

>> ---
>> Compile tested only.
>>
>> This is an update of [1].
>> The code has been heavily refactored recently with things moved to
>> ti_k3_common.c
>>
>> This new version also fixes a leak in k3_m4_rproc_probe(). In this file,
>> mbox_free_channel() was missing.
>>
>> Being very different from the v3, I've removed the previous review tags.
>>
>> [1]: 
>> https://lore.kernel.org/all/591e219df99da6f02c9d402f7854bc3ab23e76f9.1726328417.git.christophe.jail...@wanadoo.fr/
>> ---
>>  drivers/remoteproc/ti_k3_common.c | 12 +++-
>>  drivers/remoteproc/ti_k3_dsp_remoteproc.c |  2 --
>>  drivers/remoteproc/ti_k3_r5_remoteproc.c  |  2 --
>>  3 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_common.c 
>> b/drivers/remoteproc/ti_k3_common.c
>> index d4f20900f33b..7a9c3fb80fec 100644
>> --- a/drivers/remoteproc/ti_k3_common.c
>> +++ b/drivers/remoteproc/ti_k3_common.c
>> @@ -155,6 +155,13 @@ int k3_rproc_release(struct k3_rproc *kproc)
>>  }
>>  EXPORT_SYMBOL_GPL(k3_rproc_release);
>>  
>> +static void k3_rproc_free_channel(void *data)
>> +{
>> +struct k3_rproc *kproc = data;
>> +
>> +mbox_free_channel(kproc->mbox);
>> +}
>> +
>>  int k3_rproc_request_mbox(struct rproc *rproc)
>>  {
>>  struct k3_rproc *kproc = rproc->priv;
>> @@ -173,6 +180,10 @@ int k3_rproc_request_mbox(struct rproc *rproc)
>>  return dev_err_probe(dev, PTR_ERR(kproc->mbox),
>>   "mbox_request_channel failed\n");
>>  
>> +ret = devm_add_action_or_reset(dev, k3_rproc_free_channel, kproc);
>> +if (ret)
>> +return ret;
>> +
>>  /*
>>   * Ping the remote processor, this is only for sanity-sake for now;
>>   * there is no functional effect whatsoever.
>> @@ -183,7 +194,6 @@ int k3_rproc_request_mbox(struct rproc *rproc)
>>  ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_ECHO_REQUEST);
>>  if (ret < 0) {
>>  dev_err(dev, "mbox_send_message failed (%pe)\n", ERR_PTR(ret));
>> -mbox_free_channel(kproc->mbox);
>>  return ret;
>>  }
>>  
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index 7a72933bd403..d6ceea6dc920 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -175,8 +175,6 @@ static void k3_dsp_rproc_remove(struct platform_device 
>> *pdev)
>>  if (ret)
>>  dev_err(dev, "failed to detach proc (%pe)\n", 
>> ERR_PTR(ret));
>>  }
>> -
>> -mbox_free_channel(kproc->mbox);
>>  }
>>  
>>  static const struct k3_rproc_mem_data c66_mems[] = {
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index ca5ff280d2dc..04f23295ffc1 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -1206,8 +1206,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
>>  return;
>>  }
>>  }
>> -
>> -mbox_free_channel(kproc->mbox);
>>  }
>>  }
>>  
>> -- 
>> 2.50.0
>>

Re: System can not go into suspend when remoteproc is probed on AM62X

[Beleswar Prasad Padhi]

On 7/26/2025 12:59 AM, Andrew Davis wrote:

On 7/25/25 10:07 AM, Hiago De Franco wrote:

Hello everyone,

I noticed something that I am trying to debug, maybe you have any idea
or tips to help me debugging this issue.

On AM62 and AM62P SoCs that I tested, when the remote proc driver is
probed, suspend to RAM mode does not work anymore. Without the
remote proc driver enabled, everything works just fine.

See the driver being probed with AM62 and Cortex M4:

root@verdin-am62-15479173:~# dmesg | grep -i -E 
"remoteproc|rproc|omap-mailbox"
[   10.321304] omap-mailbox 2900.mailbox: omap mailbox rev 
0x66fc9100
[   10.518369] k3-m4-rproc 500.m4fss: assigned reserved memory 
node m4f-dma-memory@9cb0
[   10.560055] k3-m4-rproc 500.m4fss: configured M4F for 
remoteproc mode

[   10.600283] remoteproc remoteproc0: 500.m4fss is available
[   10.615269] remoteproc remoteproc0: Direct firmware load for 
am62-mcu-m4f0_0-fw failed with error -2

[   10.650058] remoteproc remoteproc0: powering up 500.m4fss
[   10.677073] remoteproc remoteproc0: Direct firmware load for 
am62-mcu-m4f0_0-fw failed with error -2

[   10.696173] remoteproc remoteproc0: request_firmware failed: -2
[   11.953278] remoteproc remoteproc1: 30074000.pru is available
[   11.985475] remoteproc remoteproc2: 30078000.pru is available

And then when trying to to go into suspend:

root@verdin-am62-15479173:~# echo mem > /sys/power/state
[   41.727649] PM: suspend entry (deep)
[   41.738557] Filesystems sync: 0.006 seconds
[   41.751535] Freezing user space processes
[   41.758692] Freezing user space processes completed (elapsed 0.002 
seconds)

[   41.765763] OOM killer disabled.
[   41.768999] Freezing remaining freezable tasks
[   41.774858] Freezing remaining freezable tasks completed (elapsed 
0.001 seconds)
[   41.782333] printk: Suspending console(s) (use no_console_suspend 
to debug)
[   41.830945] omap-mailbox 2900.mailbox: fifo 1 has unexpected 
unread messages
[   41.830980] omap-mailbox 2900.mailbox: PM: dpm_run_callback(): 
platform_pm_suspend returns -16
[   41.831013] omap-mailbox 2900.mailbox: PM: failed to suspend: 
error -16
[   41.831040] PM: Some devices failed to suspend, or early wake 
event detected

[   41.851206] am65-cpsw-nuss 800.ethernet: set new flow-id-base 19
[   41.861919] am65-cpsw-nuss 800.ethernet end0: PHY 
[8000f00.mdio:00] driver [TI DP83867] (irq=354)
[   41.862921] am65-cpsw-nuss 800.ethernet end0: configuring for 
phy/rgmii-rxid link mode

[   41.868493] usb-conn-gpio connector: repeated role: device
[   42.012894] OOM killer enabled.
[   42.016050] Restarting tasks: Starting
[   42.024121] Restarting tasks: Done
[   42.033660] random: crng reseeded on system resumption
[   42.040482] PM: suspend exit

I believe the issue happens at this line:

[   41.830945] omap-mailbox 2900.mailbox: fifo 1 has unexpected 
unread messages


When the remoteproc driver is probed, the omap-mailbox drivers sends a
message to Cortex-M4 which is not consumed. Please notice in this case
there is no firmware running on M4, the driver is only set to "okay"
into the DTB.

See the debug message with the message being sent ("hfranco"):

root@verdin-am62-15479173:~# dmesg | grep -i -E 
"remoteproc|rproc|omap-mailbox|hfranco"
[   10.321304] omap-mailbox 2900.mailbox: omap mailbox rev 
0x66fc9100
[   10.518369] k3-m4-rproc 500.m4fss: assigned reserved memory 
node m4f-dma-memory@9cb0
[   10.560055] k3-m4-rproc 500.m4fss: configured M4F for 
remoteproc mode

[   10.577664] hfranco: sending msg 0xff03, name mbox-m4-0
[   10.600283] remoteproc remoteproc0: 500.m4fss is available
[   10.615269] remoteproc remoteproc0: Direct firmware load for 
am62-mcu-m4f0_0-fw failed with error -2

[   10.650058] remoteproc remoteproc0: powering up 500.m4fss
[   10.677073] remoteproc remoteproc0: Direct firmware load for 
am62-mcu-m4f0_0-fw failed with error -2

[   10.696173] remoteproc remoteproc0: request_firmware failed: -2
[   11.953278] remoteproc remoteproc1: 30074000.pru is available
[   11.985475] remoteproc remoteproc2: 30078000.pru is available

AFAIK, the message in sent when 'send_data' callback is called inside
mailbox.c, which triggers omap_mbox_chan_send_data() from
omap-mailbox.c. If I skip this message, suspend to RAM works again, as
the mailbox will be empty.

Do you know why this message needs to be sent? Is there a way we can
overcome this issue? Commit 9f0cee984a25 ("mailbox/omap: check for any
unread messages during suspend") introduced this check.



So the issue then looks to be this message we send here when we setup
the mailbox[0]. This mailbox setup is done during probe() for the K3
rproc drivers now (mailbox setup used to be done during
rproc_{start,attach}() before [1]). Moving mailbox setup to probe
is correct, but we should have factored out the test message sending
code out of mailbox setup so it could have been left in
rproc_{start,attach}().