* [PATCH 0/3] Rework TI K3 R5F remoteproc driver
@ 2024-12-24 9:14 Beleswar Padhi
2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Beleswar Padhi @ 2024-12-24 9:14 UTC (permalink / raw)
To: andersson, mathieu.poirier
Cc: afd, hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka,
christophe.jaillet, jkangas, eballetbo, b-padhi, linux-remoteproc,
linux-kernel
This series cleans up the TI R5F remoteproc driver by addressing various bugs.
This is also the second series as part of the refactoring of K3 remoteproc
drivers[0]. The third and final series for K3 Refactoring will be posted soon
which will deal with the TI DSP and TI M4 drivers. The R5F driver takes care of
configuring dual core R5Fs in Lockstep/Split mode and therefore has worked out
separate data structures & reset logic than the DSP/M4 drivers which deal with
single core CPUs. Therefore, I wish to exclude R5F driver from the common
refactoring in the upcoming final series.
NOTE:
This series is _dependent_ upon the below series which does devm_ cleanup
https://lore.kernel.org/all/20241219110545.1898883-1-b-padhi@ti.com/
Bugs fixed in this series:
1. Fixed IPC-Only mode attach for R5F cores. PATCH #1
2. Fixed IPC-Only mode attach for DSP cores. (Included in this series, as this
was related to point 1 and fix is similar) PATCH #2
3. Fixed support to load firmware from userspace by refactoring wait mechanism
logic into prepare()/start() ops. PATCH #3
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`.
4. Tested usecases where firmware not available at Kernel boot time, but later
in sysfs, able to load firmware into a remotecore and start it.
5. Tested that each patch in this series generates no new warnings/errors.
Exception: Using the "wait_event_interruptible_timeout" macro in PATCH #3 raises
a -Wshadow warning, which is expected as it is called out in the implementation
of the macro itself[1].
Thanks,
Beleswar
[0]: https://lore.kernel.org/all/20241219110545.1898883-1-b-padhi@ti.com/
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/wait.h#n289
Beleswar Padhi (3):
remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick}
remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick}
remoteproc: k3-r5: Refactor sequential core power up/down operations
drivers/remoteproc/ti_k3_dsp_remoteproc.c | 53 +++++--
drivers/remoteproc/ti_k3_r5_remoteproc.c | 167 +++++++++++++---------
2 files changed, 139 insertions(+), 81 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2024-12-24 9:14 [PATCH 0/3] Rework TI K3 R5F remoteproc driver Beleswar Padhi @ 2024-12-24 9:14 ` Beleswar Padhi 2024-12-27 14:38 ` Hari Nagalla ` (3 more replies) 2024-12-24 9:14 ` [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} Beleswar Padhi 2024-12-24 9:14 ` [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations Beleswar Padhi 2 siblings, 4 replies; 14+ messages in thread From: Beleswar Padhi @ 2024-12-24 9:14 UTC (permalink / raw) To: andersson, mathieu.poirier Cc: afd, hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, b-padhi, linux-remoteproc, linux-kernel 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 <s-vadapalli@ti.com> Closes: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during probe routine") Signed-off-by: Beleswar Padhi <b-padhi@ti.com> --- Link to RFC version: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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 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 any TI-SCI commands to boot the R5F + * cores in IPC-only mode. This callback is invoked only in IPC-only mode. */ -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; } +static int k3_r5_rproc_attach(struct rproc *rproc) +{ + struct k3_r5_rproc *kproc = rproc->priv; + + kproc->core->is_attach_ongoing = true; + + return 0; +} /* * Detach from a running R5F remote processor (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. + * The R5F 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 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. */ -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; } +static int k3_r5_rproc_detach(struct rproc *rproc) +{ + struct k3_r5_rproc *kproc = rproc->priv; + + kproc->core->is_attach_ongoing = false; + + return 0; +} /* * This function implements the .get_loaded_rsc_table() callback and is used -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi @ 2024-12-27 14:38 ` Hari Nagalla 2024-12-30 4:06 ` Beleswar Prasad Padhi 2025-01-03 6:05 ` Siddharth Vadapalli ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Hari Nagalla @ 2024-12-27 14:38 UTC (permalink / raw) To: Beleswar Padhi, andersson, mathieu.poirier Cc: afd, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel 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? 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2024-12-27 14:38 ` Hari Nagalla @ 2024-12-30 4:06 ` Beleswar Prasad Padhi 0 siblings, 0 replies; 14+ messages in thread From: Beleswar Prasad Padhi @ 2024-12-30 4:06 UTC (permalink / raw) To: Hari Nagalla, andersson, mathieu.poirier Cc: afd, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi 2024-12-27 14:38 ` Hari Nagalla @ 2025-01-03 6:05 ` Siddharth Vadapalli 2025-01-03 10:48 ` Kumar, Udit 2025-01-21 18:47 ` Andrew Davis 3 siblings, 0 replies; 14+ messages in thread From: Siddharth Vadapalli @ 2025-01-03 6:05 UTC (permalink / raw) To: Beleswar Padhi Cc: andersson, mathieu.poirier, afd, hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel On Tue, Dec 24, 2024 at 02:44:55PM +0530, 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 <s-vadapalli@ti.com> > Closes: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ > Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during probe routine") > Signed-off-by: Beleswar Padhi <b-padhi@ti.com> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> Regards, Siddharth. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi 2024-12-27 14:38 ` Hari Nagalla 2025-01-03 6:05 ` Siddharth Vadapalli @ 2025-01-03 10:48 ` Kumar, Udit 2025-01-03 10:57 ` Beleswar Prasad Padhi 2025-01-21 18:47 ` Andrew Davis 3 siblings, 1 reply; 14+ messages in thread From: Kumar, Udit @ 2025-01-03 10:48 UTC (permalink / raw) To: Beleswar Padhi, andersson, mathieu.poirier Cc: afd, hnagalla, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel 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 <s-vadapalli@ti.com> > Closes: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ > Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during probe routine") > Signed-off-by: Beleswar Padhi <b-padhi@ti.com> > --- > Link to RFC version: > https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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. but variables says still attach_ongoing , I suggest to use is_attached > */ > 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 any TI-SCI commands to boot the R5F > + * cores in IPC-only mode. This callback is invoked only in IPC-only mode. > */ > -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; } > +static int k3_r5_rproc_attach(struct rproc *rproc) > +{ > + struct k3_r5_rproc *kproc = rproc->priv; > + > + kproc->core->is_attach_ongoing = true; > + > + return 0; > +} > > /* > * Detach from a running R5F remote processor (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. > + * The R5F 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 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. > */ > -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; } > +static int k3_r5_rproc_detach(struct rproc *rproc) > +{ > + struct k3_r5_rproc *kproc = rproc->priv; > + > + kproc->core->is_attach_ongoing = false; > + > + return 0; > +} > > /* > * This function implements the .get_loaded_rsc_table() callback and is used ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2025-01-03 10:48 ` Kumar, Udit @ 2025-01-03 10:57 ` Beleswar Prasad Padhi 2025-01-03 13:04 ` Kumar, Udit 0 siblings, 1 reply; 14+ messages in thread From: Beleswar Prasad Padhi @ 2025-01-03 10:57 UTC (permalink / raw) To: Kumar, Udit, andersson, mathieu.poirier Cc: afd, hnagalla, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel 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 <s-vadapalli@ti.com> >> Closes: >> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ >> Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle >> during probe routine") >> Signed-off-by: Beleswar Padhi <b-padhi@ti.com> >> --- >> Link to RFC version: >> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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 any TI-SCI commands to boot >> the R5F >> + * cores in IPC-only mode. This callback is invoked only in IPC-only >> mode. >> */ >> -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; } >> +static int k3_r5_rproc_attach(struct rproc *rproc) >> +{ >> + struct k3_r5_rproc *kproc = rproc->priv; >> + >> + kproc->core->is_attach_ongoing = true; >> + >> + return 0; >> +} >> /* >> * Detach from a running R5F remote processor (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. >> + * The R5F 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 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. >> */ >> -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; } >> +static int k3_r5_rproc_detach(struct rproc *rproc) >> +{ >> + struct k3_r5_rproc *kproc = rproc->priv; >> + >> + kproc->core->is_attach_ongoing = false; >> + >> + return 0; >> +} >> /* >> * This function implements the .get_loaded_rsc_table() callback >> and is used ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2025-01-03 10:57 ` Beleswar Prasad Padhi @ 2025-01-03 13:04 ` Kumar, Udit 0 siblings, 0 replies; 14+ messages in thread From: Kumar, Udit @ 2025-01-03 13:04 UTC (permalink / raw) To: Beleswar Prasad Padhi, andersson, mathieu.poirier Cc: afd, hnagalla, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel, u-kumar1 On 1/3/2025 4:27 PM, Beleswar Prasad Padhi wrote: > > 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 <s-vadapalli@ti.com> >>> Closes: >>> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ >>> >>> Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle >>> during probe routine") >>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com> >>> --- >>> Link to RFC version: >>> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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? In code, you are doing same work, if core is attached. In remote proc core driver POV, you are is attaching state but action you are taking as attached. Since, variable is being internal to driver, i still suggest to use is_attached > >> >> >>> */ >>> 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 any TI-SCI commands to >>> boot the R5F >>> + * cores in IPC-only mode. This callback is invoked only in >>> IPC-only mode. >>> */ >>> -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; } >>> +static int k3_r5_rproc_attach(struct rproc *rproc) >>> +{ >>> + struct k3_r5_rproc *kproc = rproc->priv; >>> + >>> + kproc->core->is_attach_ongoing = true; >>> + >>> + return 0; >>> +} >>> /* >>> * Detach from a running R5F remote processor (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. >>> + * The R5F 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 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. >>> */ >>> -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; } >>> +static int k3_r5_rproc_detach(struct rproc *rproc) >>> +{ >>> + struct k3_r5_rproc *kproc = rproc->priv; >>> + >>> + kproc->core->is_attach_ongoing = false; >>> + >>> + return 0; >>> +} >>> /* >>> * This function implements the .get_loaded_rsc_table() callback >>> and is used ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi ` (2 preceding siblings ...) 2025-01-03 10:48 ` Kumar, Udit @ 2025-01-21 18:47 ` Andrew Davis 2025-01-23 4:43 ` Beleswar Prasad Padhi 3 siblings, 1 reply; 14+ messages in thread From: Andrew Davis @ 2025-01-21 18:47 UTC (permalink / raw) To: Beleswar Padhi, andersson, mathieu.poirier Cc: hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel 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). Instead, what would be the harm in just dropping the state check? Messages coming from a detached core should be processed the same. 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 <s-vadapalli@ti.com> > Closes: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ > Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during probe routine") > Signed-off-by: Beleswar Padhi <b-padhi@ti.com> > --- > Link to RFC version: > https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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 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 any TI-SCI commands to boot the R5F > + * cores in IPC-only mode. This callback is invoked only in IPC-only mode. > */ > -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; } > +static int k3_r5_rproc_attach(struct rproc *rproc) > +{ > + struct k3_r5_rproc *kproc = rproc->priv; > + > + kproc->core->is_attach_ongoing = true; > + > + return 0; > +} > > /* > * Detach from a running R5F remote processor (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. > + * The R5F 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 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. > */ > -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; } > +static int k3_r5_rproc_detach(struct rproc *rproc) > +{ > + struct k3_r5_rproc *kproc = rproc->priv; > + > + kproc->core->is_attach_ongoing = false; > + > + return 0; > +} > > /* > * This function implements the .get_loaded_rsc_table() callback and is used ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} 2025-01-21 18:47 ` Andrew Davis @ 2025-01-23 4:43 ` Beleswar Prasad Padhi 0 siblings, 0 replies; 14+ messages in thread From: Beleswar Prasad Padhi @ 2025-01-23 4:43 UTC (permalink / raw) To: Andrew Davis, andersson, mathieu.poirier Cc: hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel 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 <s-vadapalli@ti.com> >> Closes: >> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ >> Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle >> during probe routine") >> Signed-off-by: Beleswar Padhi <b-padhi@ti.com> >> --- >> Link to RFC version: >> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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 >> 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 any TI-SCI commands to boot >> the R5F >> + * cores in IPC-only mode. This callback is invoked only in IPC-only >> mode. >> */ >> -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; } >> +static int k3_r5_rproc_attach(struct rproc *rproc) >> +{ >> + struct k3_r5_rproc *kproc = rproc->priv; >> + >> + kproc->core->is_attach_ongoing = true; >> + >> + return 0; >> +} >> /* >> * Detach from a running R5F remote processor (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. >> + * The R5F 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 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. >> */ >> -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; } >> +static int k3_r5_rproc_detach(struct rproc *rproc) >> +{ >> + struct k3_r5_rproc *kproc = rproc->priv; >> + >> + kproc->core->is_attach_ongoing = false; >> + >> + return 0; >> +} >> /* >> * This function implements the .get_loaded_rsc_table() callback >> and is used ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} 2024-12-24 9:14 [PATCH 0/3] Rework TI K3 R5F remoteproc driver Beleswar Padhi 2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi @ 2024-12-24 9:14 ` Beleswar Padhi 2025-01-03 6:06 ` Siddharth Vadapalli 2024-12-24 9:14 ` [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations Beleswar Padhi 2 siblings, 1 reply; 14+ messages in thread From: Beleswar Padhi @ 2024-12-24 9:14 UTC (permalink / raw) To: andersson, mathieu.poirier Cc: afd, hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, b-padhi, linux-remoteproc, linux-kernel Commit ea1d6fb5b571 ("remoteproc: k3-dsp: Acquire mailbox handle during probe routine") introduced a check in the "k3_dsp_rproc_mbox_callback()" and "k3_dsp_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 <s-vadapalli@ti.com> Closes: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ Fixes: ea1d6fb5b571 ("remoteproc: k3-dsp: Acquire mailbox handle during probe routine") Signed-off-by: Beleswar Padhi <b-padhi@ti.com> --- Link to RFC version: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@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_dsp_remoteproc.c | 53 +++++++++++++++++------ 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c index a695890254ff..f20fc2db077b 100644 --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c @@ -76,6 +76,7 @@ struct k3_dsp_dev_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 */ struct k3_dsp_rproc { struct device *dev; @@ -91,6 +92,7 @@ struct k3_dsp_rproc { u32 ti_sci_id; struct mbox_chan *mbox; struct mbox_client client; + bool is_attach_ongoing; }; /** @@ -115,8 +117,11 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data) const char *name = kproc->rproc->name; u32 msg = omap_mbox_message(data); - /* Do not forward messages 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->is_attach_ongoing && kproc->rproc->state == RPROC_DETACHED) return; dev_dbg(dev, "mbox msg: 0x%x\n", msg); @@ -159,8 +164,11 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid) mbox_msg_t msg = (mbox_msg_t)vqid; int ret; - /* Do not forward messages 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->is_attach_ongoing && kproc->rproc->state == RPROC_DETACHED) return; /* send the index of the triggered virtqueue in the mailbox payload */ @@ -357,22 +365,39 @@ static int k3_dsp_rproc_stop(struct rproc *rproc) /* * Attach to a running DSP remote processor (IPC-only mode) * - * This rproc 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 DSP core. This callback - * is invoked only in IPC-only mode and exists because rproc_validate() checks - * for its existence. + * This rproc attach callback only needs to set the "is_attach_ongoing" flag to + * notify k3_dsp_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 any TI-SCI commands to boot the DSP core. This + * callback is invoked only in IPC-only mode. */ -static int k3_dsp_rproc_attach(struct rproc *rproc) { return 0; } +static int k3_dsp_rproc_attach(struct rproc *rproc) +{ + struct k3_dsp_rproc *kproc = rproc->priv; + + kproc->is_attach_ongoing = true; + + return 0; +} /* * Detach from a running DSP remote processor (IPC-only mode) * - * This rproc detach callback is a NOP. The DSP core is not stopped and will be - * left to continue to run its booted firmware. This callback is invoked only in - * IPC-only mode and exists for sanity sake. + * 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 DSP core is not + * stopped and will be left to continue to run its booted firmware. This callback + * is invoked only in IPC-only mode. */ -static int k3_dsp_rproc_detach(struct rproc *rproc) { return 0; } +static int k3_dsp_rproc_detach(struct rproc *rproc) +{ + struct k3_dsp_rproc *kproc = rproc->priv; + + kproc->is_attach_ongoing = false; + + return 0; +} /* * This function implements the .get_loaded_rsc_table() callback and is used -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} 2024-12-24 9:14 ` [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} Beleswar Padhi @ 2025-01-03 6:06 ` Siddharth Vadapalli 0 siblings, 0 replies; 14+ messages in thread From: Siddharth Vadapalli @ 2025-01-03 6:06 UTC (permalink / raw) To: Beleswar Padhi Cc: andersson, mathieu.poirier, afd, hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel On Tue, Dec 24, 2024 at 02:44:56PM +0530, Beleswar Padhi wrote: > Commit ea1d6fb5b571 ("remoteproc: k3-dsp: Acquire mailbox handle during > probe routine") introduced a check in the "k3_dsp_rproc_mbox_callback()" > and "k3_dsp_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 <s-vadapalli@ti.com> > Closes: https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/ > Fixes: ea1d6fb5b571 ("remoteproc: k3-dsp: Acquire mailbox handle during probe routine") > Signed-off-by: Beleswar Padhi <b-padhi@ti.com> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> Regards, Siddharth. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations 2024-12-24 9:14 [PATCH 0/3] Rework TI K3 R5F remoteproc driver Beleswar Padhi 2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi 2024-12-24 9:14 ` [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} Beleswar Padhi @ 2024-12-24 9:14 ` Beleswar Padhi 2025-01-03 10:48 ` Kumar, Udit 2 siblings, 1 reply; 14+ messages in thread From: Beleswar Padhi @ 2024-12-24 9:14 UTC (permalink / raw) To: andersson, mathieu.poirier Cc: afd, hnagalla, u-kumar1, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, b-padhi, linux-remoteproc, linux-kernel The existing implementation of the waiting mechanism in "k3_r5_cluster_rproc_init()" waits for the "released_from_reset" flag to be set as part of the firmware boot process in "k3_r5_rproc_start()". The "k3_r5_cluster_rproc_init()" function is invoked in the probe routine which causes unexpected failures in cases where the firmware is unavailable at boot time, resulting in probe failure and removal of the remoteproc handles in the sysfs paths. To address this, the waiting mechanism is refactored out of the probe routine into the appropriate "k3_r5_rproc_{prepare/unprepare}()" functions. This allows the probe routine to complete without depending on firmware booting, while still maintaining the required power-synchronization between cores. Further, this wait mechanism is dropped from "k3_r5_rproc_{start/stop}()" functions as they deal with Core Run/Halt operations, and as such, there is no constraint in Running or Halting the cores of a cluster in order. Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up core1") Signed-off-by: Beleswar Padhi <b-padhi@ti.com> --- Link to v1: https://lore.kernel.org/all/20240906094045.2428977-1-b-padhi@ti.com/ v2: Changelog: 1. Improved commit message to call out functions correctly. [Mathieu] 2. Removed sequential wait/checks from .start()/.stop() ops as there is no constraint for Core Run/Halt operations. drivers/remoteproc/ti_k3_r5_remoteproc.c | 114 ++++++++++++----------- 1 file changed, 61 insertions(+), 53 deletions(-) diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c index e218a803fdb5..15e5a10801cd 100644 --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c @@ -456,13 +456,36 @@ 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 *core = kproc->core, *core0, *core1; 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. + * + * 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(core->tsp, &boot_vec, &cfg, &ctrl, &stat); if (ret < 0) return ret; @@ -478,6 +501,13 @@ 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 @@ -523,10 +553,30 @@ 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 *core = kproc->core, *core0, *core1; struct device *dev = kproc->dev; int ret; + /* + * Ensure power-down of cores is sequential in split mode. Core1 must + * power down before Core0 to maintain the expected state. By placing + * the wait mechanism here in .unprepare() ops, this condition is + * enforced for rproc stop or shutdown requests from sysfs and device + * removal as well. + */ + core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem); + core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem); + if (cluster->mode == CLUSTER_MODE_SPLIT && core == core0 && + core1->released_from_reset) { + ret = wait_event_interruptible_timeout(cluster->core_transition, + !core1->released_from_reset, + msecs_to_jiffies(2000)); + if (ret <= 0) { + dev_err(dev, "can not power down core0 before core1"); + return -EPERM; + } + } + /* Re-use LockStep-mode reset logic for Single-CPU mode */ ret = (cluster->mode == CLUSTER_MODE_LOCKSTEP || cluster->mode == CLUSTER_MODE_SINGLECPU) ? @@ -534,6 +584,13 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc) if (ret) dev_err(dev, "unable to disable cores, ret = %d\n", ret); + /* + * Notify all threads in the wait queue when core state has changed so + * that threads waiting for this condition can be executed. + */ + core->released_from_reset = false; + wake_up_interruptible(&cluster->core_transition); + return ret; } @@ -559,7 +616,7 @@ static int k3_r5_rproc_start(struct rproc *rproc) struct k3_r5_rproc *kproc = rproc->priv; struct k3_r5_cluster *cluster = kproc->cluster; struct device *dev = kproc->dev; - struct k3_r5_core *core0, *core; + struct k3_r5_core *core; u32 boot_addr; int ret; @@ -581,21 +638,9 @@ static int k3_r5_rproc_start(struct rproc *rproc) goto unroll_core_run; } } else { - /* do not allow core 1 to start before core 0 */ - core0 = list_first_entry(&cluster->cores, struct k3_r5_core, - elem); - if (core != core0 && core0->rproc->state == RPROC_OFFLINE) { - dev_err(dev, "%s: can not start core 1 before core 0\n", - __func__); - return -EPERM; - } - ret = k3_r5_core_run(core); if (ret) return ret; - - core->released_from_reset = true; - wake_up_interruptible(&cluster->core_transition); } return 0; @@ -636,8 +681,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; - struct k3_r5_core *core1, *core = kproc->core; + struct k3_r5_core *core = kproc->core; int ret; /* halt all applicable cores */ @@ -650,16 +694,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc) } } } else { - /* do not allow core 0 to stop before core 1 */ - core1 = list_last_entry(&cluster->cores, struct k3_r5_core, - elem); - if (core != core1 && core1->rproc->state != RPROC_OFFLINE) { - dev_err(dev, "%s: can not stop core 0 before core 1\n", - __func__); - ret = -EPERM; - goto out; - } - ret = k3_r5_core_halt(core); if (ret) goto out; @@ -1154,12 +1188,6 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) return reset_ctrl_status; } - /* - * Skip the waiting mechanism for sequential power-on of cores if the - * core has already been booted by another entity. - */ - core->released_from_reset = c_state; - ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat); if (ret < 0) { @@ -1304,26 +1332,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev) cluster->mode == CLUSTER_MODE_SINGLECPU || cluster->mode == CLUSTER_MODE_SINGLECORE) break; - - /* - * R5 cores require to be powered on sequentially, core0 - * should be in higher power state than core1 in a cluster - * So, wait for current core to power up before proceeding - * to next core and put timeout of 2sec for each core. - * - * This waiting mechanism is necessary because - * rproc_auto_boot_callback() for core1 can be called before - * core0 due to thread execution order. - */ - ret = wait_event_interruptible_timeout(cluster->core_transition, - core->released_from_reset, - msecs_to_jiffies(2000)); - if (ret <= 0) { - dev_err(dev, - "Timed out waiting for %s core to power up!\n", - rproc->name); - goto out; - } } return 0; -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations 2024-12-24 9:14 ` [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations Beleswar Padhi @ 2025-01-03 10:48 ` Kumar, Udit 0 siblings, 0 replies; 14+ messages in thread From: Kumar, Udit @ 2025-01-03 10:48 UTC (permalink / raw) To: Beleswar Padhi, andersson, mathieu.poirier Cc: afd, hnagalla, s-vadapalli, srk, jan.kiszka, christophe.jaillet, jkangas, eballetbo, linux-remoteproc, linux-kernel On 12/24/2024 2:44 PM, Beleswar Padhi wrote: > The existing implementation of the waiting mechanism in > "k3_r5_cluster_rproc_init()" waits for the "released_from_reset" flag to > be set as part of the firmware boot process in "k3_r5_rproc_start()". > The "k3_r5_cluster_rproc_init()" function is invoked in the probe > routine which causes unexpected failures in cases where the firmware is > unavailable at boot time, resulting in probe failure and removal of the > remoteproc handles in the sysfs paths. > > To address this, the waiting mechanism is refactored out of the probe > routine into the appropriate "k3_r5_rproc_{prepare/unprepare}()" > functions. This allows the probe routine to complete without depending > on firmware booting, while still maintaining the required > power-synchronization between cores. > > Further, this wait mechanism is dropped from > "k3_r5_rproc_{start/stop}()" functions as they deal with Core Run/Halt > operations, and as such, there is no constraint in Running or Halting > the cores of a cluster in order. > > Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up before powering up core1") > Signed-off-by: Beleswar Padhi <b-padhi@ti.com> > --- > Link to v1: > https://lore.kernel.org/all/20240906094045.2428977-1-b-padhi@ti.com/ > > v2: Changelog: > 1. Improved commit message to call out functions correctly. [Mathieu] > 2. Removed sequential wait/checks from .start()/.stop() ops as there is no > constraint for Core Run/Halt operations. > > drivers/remoteproc/ti_k3_r5_remoteproc.c | 114 ++++++++++++----------- > 1 file changed, 61 insertions(+), 53 deletions(-) > > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c > index e218a803fdb5..15e5a10801cd 100644 > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c > @@ -456,13 +456,36 @@ 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 *core = kproc->core, *core0, *core1; > 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. > + * > + * 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(core->tsp, &boot_vec, &cfg, &ctrl, &stat); > if (ret < 0) > return ret; > @@ -478,6 +501,13 @@ 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); I think, you should signal this only for core-0, > + > /* > * 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 > @@ -523,10 +553,30 @@ 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 *core = kproc->core, *core0, *core1; > struct device *dev = kproc->dev; > int ret; > > + /* > + * Ensure power-down of cores is sequential in split mode. Core1 must > + * power down before Core0 to maintain the expected state. By placing > + * the wait mechanism here in .unprepare() ops, this condition is > + * enforced for rproc stop or shutdown requests from sysfs and device > + * removal as well. > + */ > + core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem); > + core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem); > + if (cluster->mode == CLUSTER_MODE_SPLIT && core == core0 && > + core1->released_from_reset) { > + ret = wait_event_interruptible_timeout(cluster->core_transition, > + !core1->released_from_reset, > + msecs_to_jiffies(2000)); > + if (ret <= 0) { > + dev_err(dev, "can not power down core0 before core1"); > + return -EPERM; > + } > + } > + > /* Re-use LockStep-mode reset logic for Single-CPU mode */ > ret = (cluster->mode == CLUSTER_MODE_LOCKSTEP || > cluster->mode == CLUSTER_MODE_SINGLECPU) ? > @@ -534,6 +584,13 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc) > if (ret) > dev_err(dev, "unable to disable cores, ret = %d\n", ret); > > + /* > + * Notify all threads in the wait queue when core state has changed so > + * that threads waiting for this condition can be executed. > + */ > + core->released_from_reset = false; > + wake_up_interruptible(&cluster->core_transition); > + > return ret; > } > > @@ -559,7 +616,7 @@ static int k3_r5_rproc_start(struct rproc *rproc) > struct k3_r5_rproc *kproc = rproc->priv; > struct k3_r5_cluster *cluster = kproc->cluster; > struct device *dev = kproc->dev; > - struct k3_r5_core *core0, *core; > + struct k3_r5_core *core; > u32 boot_addr; > int ret; > > @@ -581,21 +638,9 @@ static int k3_r5_rproc_start(struct rproc *rproc) > goto unroll_core_run; > } > } else { > - /* do not allow core 1 to start before core 0 */ > - core0 = list_first_entry(&cluster->cores, struct k3_r5_core, > - elem); > - if (core != core0 && core0->rproc->state == RPROC_OFFLINE) { > - dev_err(dev, "%s: can not start core 1 before core 0\n", > - __func__); > - return -EPERM; > - } > - > ret = k3_r5_core_run(core); > if (ret) > return ret; > - > - core->released_from_reset = true; > - wake_up_interruptible(&cluster->core_transition); > } > > return 0; > @@ -636,8 +681,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; > - struct k3_r5_core *core1, *core = kproc->core; > + struct k3_r5_core *core = kproc->core; > int ret; > > /* halt all applicable cores */ > @@ -650,16 +694,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc) > } > } > } else { > - /* do not allow core 0 to stop before core 1 */ > - core1 = list_last_entry(&cluster->cores, struct k3_r5_core, > - elem); > - if (core != core1 && core1->rproc->state != RPROC_OFFLINE) { > - dev_err(dev, "%s: can not stop core 0 before core 1\n", > - __func__); > - ret = -EPERM; > - goto out; > - } > - > ret = k3_r5_core_halt(core); > if (ret) > goto out; > @@ -1154,12 +1188,6 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > return reset_ctrl_status; > } > > - /* > - * Skip the waiting mechanism for sequential power-on of cores if the > - * core has already been booted by another entity. > - */ > - core->released_from_reset = c_state; > - > ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, > &stat); > if (ret < 0) { > @@ -1304,26 +1332,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev) > cluster->mode == CLUSTER_MODE_SINGLECPU || > cluster->mode == CLUSTER_MODE_SINGLECORE) > break; > - > - /* > - * R5 cores require to be powered on sequentially, core0 > - * should be in higher power state than core1 in a cluster > - * So, wait for current core to power up before proceeding > - * to next core and put timeout of 2sec for each core. > - * > - * This waiting mechanism is necessary because > - * rproc_auto_boot_callback() for core1 can be called before > - * core0 due to thread execution order. > - */ > - ret = wait_event_interruptible_timeout(cluster->core_transition, > - core->released_from_reset, > - msecs_to_jiffies(2000)); > - if (ret <= 0) { > - dev_err(dev, > - "Timed out waiting for %s core to power up!\n", > - rproc->name); > - goto out; > - } > } > > return 0; ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-01-23 4:43 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-24 9:14 [PATCH 0/3] Rework TI K3 R5F remoteproc driver Beleswar Padhi
2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi
2024-12-27 14:38 ` Hari Nagalla
2024-12-30 4:06 ` Beleswar Prasad Padhi
2025-01-03 6:05 ` Siddharth Vadapalli
2025-01-03 10:48 ` Kumar, Udit
2025-01-03 10:57 ` Beleswar Prasad Padhi
2025-01-03 13:04 ` Kumar, Udit
2025-01-21 18:47 ` Andrew Davis
2025-01-23 4:43 ` Beleswar Prasad Padhi
2024-12-24 9:14 ` [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} Beleswar Padhi
2025-01-03 6:06 ` Siddharth Vadapalli
2024-12-24 9:14 ` [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations Beleswar Padhi
2025-01-03 10:48 ` Kumar, Udit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).