Re: [PATCH v2 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe
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.
Re: [PATCH 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe
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 = >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.
*/
-static
Re: [EXTERNAL] Re: [PATCH v2 1/2] remoteproc: k3-r5: Wait for core0 power-up before powering up core1
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(>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, _vec, , ,
> );
>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)
>cluster->dev = dev;
>
