| Message ID | 20240621150058.319524-5-richard.genoud@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remoteproc: k3-r5: Introduce suspend to ram support | expand |
On Fri, Jun 21, 2024 at 05:00:58PM +0200, Richard Genoud wrote: > Introduce software IPC handshake between the K3-R5 remote proc driver > and the R5 MCU to gracefully stop/reset the remote core. > > Upon a stop request, K3-R5 remote proc driver sends a RP_MBOX_SHUTDOWN > mailbox message to the remote R5 core. > The remote core is expected to: > - relinquish all the resources acquired through Device Manager (DM) > - disable its interrupts > - send back a mailbox acknowledgment RP_MBOX_SHUDOWN_ACK > - enter WFI state. > > Meanwhile, the K3-R5 remote proc driver does: > - wait for the RP_MBOX_SHUTDOWN_ACK from the remote core > - wait for the remote proc to enter WFI state > - reset the remote core through device manager > > Based on work from: Hari Nagalla <hnagalla@ti.com> > Why is this needed now and what happens to system with a new kernel driver and an older K3R5 firmware? Thanks, Mathieu > Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> > --- > drivers/remoteproc/omap_remoteproc.h | 9 +++++- > drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 ++++++++++++++++++++++++ > 2 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h > index 828e13256c02..c008f11fa2a4 100644 > --- a/drivers/remoteproc/omap_remoteproc.h > +++ b/drivers/remoteproc/omap_remoteproc.h > @@ -42,6 +42,11 @@ > * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor > * on a suspend request > * > + * @RP_MBOX_SHUTDOWN: shutdown request for the remote processor > + * > + * @RP_MBOX_SHUTDOWN_ACK: successful response from remote processor for a > + * shutdown request. The remote processor should be in WFI state short after. > + * > * Introduce new message definitions if any here. > * > * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core > @@ -59,7 +64,9 @@ enum omap_rp_mbox_messages { > RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, > RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, > RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, > - RP_MBOX_END_MSG = 0xFFFFFF14, > + RP_MBOX_SHUTDOWN = 0xFFFFFF14, > + RP_MBOX_SHUTDOWN_ACK = 0xFFFFFF15, > + RP_MBOX_END_MSG = 0xFFFFFF16, > }; > > #endif /* _OMAP_RPMSG_H */ > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c > index a2ead87952c7..918a15e1dd9a 100644 > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c > @@ -21,6 +21,7 @@ > #include <linux/pm_runtime.h> > #include <linux/remoteproc.h> > #include <linux/suspend.h> > +#include <linux/iopoll.h> > #include <linux/reset.h> > #include <linux/slab.h> > > @@ -172,8 +173,23 @@ struct k3_r5_rproc { > struct k3_r5_core *core; > struct k3_r5_mem *rmem; > int num_rmems; > + struct completion shutdown_complete; > }; > > +/* > + * This will return true if the remote core is in Wait For Interrupt state. > + */ > +static bool k3_r5_is_core_in_wfi(struct k3_r5_core *core) > +{ > + int ret; > + u64 boot_vec; > + u32 cfg, ctrl, stat; > + > + ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat); > + > + return !ret ? !!(stat & PROC_BOOT_STATUS_FLAG_R5_WFI) : false; > +} > + > /** > * k3_r5_rproc_mbox_callback() - inbound mailbox message handler > * @client: mailbox client pointer used for requesting the mailbox channel > @@ -209,6 +225,10 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client *client, void *data) > case RP_MBOX_ECHO_REPLY: > dev_info(dev, "received echo reply from %s\n", name); > break; > + case RP_MBOX_SHUTDOWN_ACK: > + dev_dbg(dev, "received shutdown_ack from %s\n", name); > + complete(&kproc->shutdown_complete); > + break; > default: > /* silently handle all other valid messages */ > if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) > @@ -634,6 +654,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc) > struct k3_r5_cluster *cluster = kproc->cluster; > struct device *dev = kproc->dev; > struct k3_r5_core *core1, *core = kproc->core; > + bool wfi; > int ret; > > > @@ -650,6 +671,24 @@ static int k3_r5_rproc_stop(struct rproc *rproc) > } > } > > + /* Send SHUTDOWN message to remote proc */ > + reinit_completion(&kproc->shutdown_complete); > + ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_SHUTDOWN); > + if (ret < 0) { > + dev_err(dev, "Sending SHUTDOWN message failed: %d. Halting core anyway.\n", ret); > + } else { > + ret = wait_for_completion_timeout(&kproc->shutdown_complete, > + msecs_to_jiffies(1000)); > + if (ret == 0) { > + dev_err(dev, "Timeout waiting SHUTDOWN_ACK message. Halting core anyway.\n"); > + } else { > + ret = readx_poll_timeout(k3_r5_is_core_in_wfi, core, > + wfi, wfi, 200, 2000); > + if (ret) > + dev_err(dev, "Timeout waiting for remote proc to be in WFI state. Halting core anyway.\n"); > + } > + } > + > /* halt all applicable cores */ > if (cluster->mode == CLUSTER_MODE_LOCKSTEP) { > list_for_each_entry(core, &cluster->cores, elem) { > @@ -1410,6 +1449,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev) > goto err_config; > } > > + init_completion(&kproc->shutdown_complete); > init_rmem: > k3_r5_adjust_tcm_sizes(kproc); >
On 6/21/24 10:00 AM, Richard Genoud wrote: > Introduce software IPC handshake between the K3-R5 remote proc driver > and the R5 MCU to gracefully stop/reset the remote core. > > Upon a stop request, K3-R5 remote proc driver sends a RP_MBOX_SHUTDOWN > mailbox message to the remote R5 core. > The remote core is expected to: > - relinquish all the resources acquired through Device Manager (DM) > - disable its interrupts > - send back a mailbox acknowledgment RP_MBOX_SHUDOWN_ACK > - enter WFI state. > > Meanwhile, the K3-R5 remote proc driver does: > - wait for the RP_MBOX_SHUTDOWN_ACK from the remote core > - wait for the remote proc to enter WFI state > - reset the remote core through device manager > > Based on work from: Hari Nagalla <hnagalla@ti.com> > > Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> > --- > drivers/remoteproc/omap_remoteproc.h | 9 +++++- > drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 ++++++++++++++++++++++++ > 2 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h > index 828e13256c02..c008f11fa2a4 100644 > --- a/drivers/remoteproc/omap_remoteproc.h > +++ b/drivers/remoteproc/omap_remoteproc.h > @@ -42,6 +42,11 @@ > * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor > * on a suspend request > * > + * @RP_MBOX_SHUTDOWN: shutdown request for the remote processor > + * > + * @RP_MBOX_SHUTDOWN_ACK: successful response from remote processor for a > + * shutdown request. The remote processor should be in WFI state short after. > + * > * Introduce new message definitions if any here. > * > * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core > @@ -59,7 +64,9 @@ enum omap_rp_mbox_messages { > RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, > RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, > RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, > - RP_MBOX_END_MSG = 0xFFFFFF14, > + RP_MBOX_SHUTDOWN = 0xFFFFFF14, > + RP_MBOX_SHUTDOWN_ACK = 0xFFFFFF15, > + RP_MBOX_END_MSG = 0xFFFFFF16, > }; > > #endif /* _OMAP_RPMSG_H */ > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c > index a2ead87952c7..918a15e1dd9a 100644 > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c > @@ -21,6 +21,7 @@ > #include <linux/pm_runtime.h> > #include <linux/remoteproc.h> > #include <linux/suspend.h> > +#include <linux/iopoll.h> > #include <linux/reset.h> > #include <linux/slab.h> > > @@ -172,8 +173,23 @@ struct k3_r5_rproc { > struct k3_r5_core *core; > struct k3_r5_mem *rmem; > int num_rmems; > + struct completion shutdown_complete; > }; > > +/* > + * This will return true if the remote core is in Wait For Interrupt state. > + */ > +static bool k3_r5_is_core_in_wfi(struct k3_r5_core *core) > +{ > + int ret; > + u64 boot_vec; > + u32 cfg, ctrl, stat; > + > + ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat); > + > + return !ret ? !!(stat & PROC_BOOT_STATUS_FLAG_R5_WFI) : false; Too fancy for me :) Just return if (ret) right after get_status(). Looks like this function is called in a polling loop, if ti_sci_proc_get_status() fails once, it won't get better, no need to keep checking, we should just error out of the polling loop. Andrew > +} > + > /** > * k3_r5_rproc_mbox_callback() - inbound mailbox message handler > * @client: mailbox client pointer used for requesting the mailbox channel > @@ -209,6 +225,10 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client *client, void *data) > case RP_MBOX_ECHO_REPLY: > dev_info(dev, "received echo reply from %s\n", name); > break; > + case RP_MBOX_SHUTDOWN_ACK: > + dev_dbg(dev, "received shutdown_ack from %s\n", name); > + complete(&kproc->shutdown_complete); > + break; > default: > /* silently handle all other valid messages */ > if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) > @@ -634,6 +654,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc) > struct k3_r5_cluster *cluster = kproc->cluster; > struct device *dev = kproc->dev; > struct k3_r5_core *core1, *core = kproc->core; > + bool wfi; > int ret; > > > @@ -650,6 +671,24 @@ static int k3_r5_rproc_stop(struct rproc *rproc) > } > } > > + /* Send SHUTDOWN message to remote proc */ > + reinit_completion(&kproc->shutdown_complete); > + ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_SHUTDOWN); > + if (ret < 0) { > + dev_err(dev, "Sending SHUTDOWN message failed: %d. Halting core anyway.\n", ret); > + } else { > + ret = wait_for_completion_timeout(&kproc->shutdown_complete, > + msecs_to_jiffies(1000)); > + if (ret == 0) { > + dev_err(dev, "Timeout waiting SHUTDOWN_ACK message. Halting core anyway.\n"); > + } else { > + ret = readx_poll_timeout(k3_r5_is_core_in_wfi, core, > + wfi, wfi, 200, 2000); > + if (ret) > + dev_err(dev, "Timeout waiting for remote proc to be in WFI state. Halting core anyway.\n"); > + } > + } > + > /* halt all applicable cores */ > if (cluster->mode == CLUSTER_MODE_LOCKSTEP) { > list_for_each_entry(core, &cluster->cores, elem) { > @@ -1410,6 +1449,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev) > goto err_config; > } > > + init_completion(&kproc->shutdown_complete); > init_rmem: > k3_r5_adjust_tcm_sizes(kproc); > >
[adding Vibhore in Cc] Le 28/06/2024 à 23:20, Mathieu Poirier a écrit : > On Fri, Jun 21, 2024 at 05:00:58PM +0200, Richard Genoud wrote: >> Introduce software IPC handshake between the K3-R5 remote proc driver >> and the R5 MCU to gracefully stop/reset the remote core. >> >> Upon a stop request, K3-R5 remote proc driver sends a RP_MBOX_SHUTDOWN >> mailbox message to the remote R5 core. >> The remote core is expected to: >> - relinquish all the resources acquired through Device Manager (DM) >> - disable its interrupts >> - send back a mailbox acknowledgment RP_MBOX_SHUDOWN_ACK >> - enter WFI state. >> >> Meanwhile, the K3-R5 remote proc driver does: >> - wait for the RP_MBOX_SHUTDOWN_ACK from the remote core >> - wait for the remote proc to enter WFI state >> - reset the remote core through device manager >> >> Based on work from: Hari Nagalla <hnagalla@ti.com> >> > > Why is this needed now and what happens to system with a new kernel driver and > an older K3R5 firmware? Without this patch, the IPC firwmares from recent TI PDK prevent the suspend: platform 5d00000.r5f: ti-sci processor set_control failed: -19 remoteproc remoteproc1: can't stop rproc: -19 platform 5c00000.r5f: Failed to shutdown rproc (-19) platform 5c00000.r5f: k3_r5_rproc_suspend failed (-19) With a new kernel driver and an old firmware, this will add a timeout before stopping it, and the message: platform 5c00000.r5f: Timeout waiting SHUTDOWN_ACK message. Halting core anyway. (tested on old FW 09.00.00.01 and new FW 09.02.01.18, on J7200) Regards, Richard > > Thanks, > Mathieu > >> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> >> --- >> drivers/remoteproc/omap_remoteproc.h | 9 +++++- >> drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 ++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h >> index 828e13256c02..c008f11fa2a4 100644 >> --- a/drivers/remoteproc/omap_remoteproc.h >> +++ b/drivers/remoteproc/omap_remoteproc.h >> @@ -42,6 +42,11 @@ >> * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor >> * on a suspend request >> * >> + * @RP_MBOX_SHUTDOWN: shutdown request for the remote processor >> + * >> + * @RP_MBOX_SHUTDOWN_ACK: successful response from remote processor for a >> + * shutdown request. The remote processor should be in WFI state short after. >> + * >> * Introduce new message definitions if any here. >> * >> * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core >> @@ -59,7 +64,9 @@ enum omap_rp_mbox_messages { >> RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, >> RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, >> RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, >> - RP_MBOX_END_MSG = 0xFFFFFF14, >> + RP_MBOX_SHUTDOWN = 0xFFFFFF14, >> + RP_MBOX_SHUTDOWN_ACK = 0xFFFFFF15, >> + RP_MBOX_END_MSG = 0xFFFFFF16, >> }; >> >> #endif /* _OMAP_RPMSG_H */ >> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c >> index a2ead87952c7..918a15e1dd9a 100644 >> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c >> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c >> @@ -21,6 +21,7 @@ >> #include <linux/pm_runtime.h> >> #include <linux/remoteproc.h> >> #include <linux/suspend.h> >> +#include <linux/iopoll.h> >> #include <linux/reset.h> >> #include <linux/slab.h> >> >> @@ -172,8 +173,23 @@ struct k3_r5_rproc { >> struct k3_r5_core *core; >> struct k3_r5_mem *rmem; >> int num_rmems; >> + struct completion shutdown_complete; >> }; >> >> +/* >> + * This will return true if the remote core is in Wait For Interrupt state. >> + */ >> +static bool k3_r5_is_core_in_wfi(struct k3_r5_core *core) >> +{ >> + int ret; >> + u64 boot_vec; >> + u32 cfg, ctrl, stat; >> + >> + ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat); >> + >> + return !ret ? !!(stat & PROC_BOOT_STATUS_FLAG_R5_WFI) : false; >> +} >> + >> /** >> * k3_r5_rproc_mbox_callback() - inbound mailbox message handler >> * @client: mailbox client pointer used for requesting the mailbox channel >> @@ -209,6 +225,10 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client *client, void *data) >> case RP_MBOX_ECHO_REPLY: >> dev_info(dev, "received echo reply from %s\n", name); >> break; >> + case RP_MBOX_SHUTDOWN_ACK: >> + dev_dbg(dev, "received shutdown_ack from %s\n", name); >> + complete(&kproc->shutdown_complete); >> + break; >> default: >> /* silently handle all other valid messages */ >> if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) >> @@ -634,6 +654,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc) >> struct k3_r5_cluster *cluster = kproc->cluster; >> struct device *dev = kproc->dev; >> struct k3_r5_core *core1, *core = kproc->core; >> + bool wfi; >> int ret; >> >> >> @@ -650,6 +671,24 @@ static int k3_r5_rproc_stop(struct rproc *rproc) >> } >> } >> >> + /* Send SHUTDOWN message to remote proc */ >> + reinit_completion(&kproc->shutdown_complete); >> + ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_SHUTDOWN); >> + if (ret < 0) { >> + dev_err(dev, "Sending SHUTDOWN message failed: %d. Halting core anyway.\n", ret); >> + } else { >> + ret = wait_for_completion_timeout(&kproc->shutdown_complete, >> + msecs_to_jiffies(1000)); >> + if (ret == 0) { >> + dev_err(dev, "Timeout waiting SHUTDOWN_ACK message. Halting core anyway.\n"); >> + } else { >> + ret = readx_poll_timeout(k3_r5_is_core_in_wfi, core, >> + wfi, wfi, 200, 2000); >> + if (ret) >> + dev_err(dev, "Timeout waiting for remote proc to be in WFI state. Halting core anyway.\n"); >> + } >> + } >> + >> /* halt all applicable cores */ >> if (cluster->mode == CLUSTER_MODE_LOCKSTEP) { >> list_for_each_entry(core, &cluster->cores, elem) { >> @@ -1410,6 +1449,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev) >> goto err_config; >> } >> >> + init_completion(&kproc->shutdown_complete); >> init_rmem: >> k3_r5_adjust_tcm_sizes(kproc); >>
Le 29/06/2024 à 00:50, Andrew Davis a écrit : > On 6/21/24 10:00 AM, Richard Genoud wrote: >> Introduce software IPC handshake between the K3-R5 remote proc driver >> and the R5 MCU to gracefully stop/reset the remote core. >> >> Upon a stop request, K3-R5 remote proc driver sends a RP_MBOX_SHUTDOWN >> mailbox message to the remote R5 core. >> The remote core is expected to: >> - relinquish all the resources acquired through Device Manager (DM) >> - disable its interrupts >> - send back a mailbox acknowledgment RP_MBOX_SHUDOWN_ACK >> - enter WFI state. >> >> Meanwhile, the K3-R5 remote proc driver does: >> - wait for the RP_MBOX_SHUTDOWN_ACK from the remote core >> - wait for the remote proc to enter WFI state >> - reset the remote core through device manager >> >> Based on work from: Hari Nagalla <hnagalla@ti.com> >> >> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> >> --- >> drivers/remoteproc/omap_remoteproc.h | 9 +++++- >> drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 ++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/omap_remoteproc.h >> b/drivers/remoteproc/omap_remoteproc.h >> index 828e13256c02..c008f11fa2a4 100644 >> --- a/drivers/remoteproc/omap_remoteproc.h >> +++ b/drivers/remoteproc/omap_remoteproc.h >> @@ -42,6 +42,11 @@ >> * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote >> processor >> * on a suspend request >> * >> + * @RP_MBOX_SHUTDOWN: shutdown request for the remote processor >> + * >> + * @RP_MBOX_SHUTDOWN_ACK: successful response from remote processor >> for a >> + * shutdown request. The remote processor should be in WFI state >> short after. >> + * >> * Introduce new message definitions if any here. >> * >> * @RP_MBOX_END_MSG: Indicates end of known/defined messages from >> remote core >> @@ -59,7 +64,9 @@ enum omap_rp_mbox_messages { >> RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, >> RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, >> RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, >> - RP_MBOX_END_MSG = 0xFFFFFF14, >> + RP_MBOX_SHUTDOWN = 0xFFFFFF14, >> + RP_MBOX_SHUTDOWN_ACK = 0xFFFFFF15, >> + RP_MBOX_END_MSG = 0xFFFFFF16, >> }; >> #endif /* _OMAP_RPMSG_H */ >> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c >> b/drivers/remoteproc/ti_k3_r5_remoteproc.c >> index a2ead87952c7..918a15e1dd9a 100644 >> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c >> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c >> @@ -21,6 +21,7 @@ >> #include <linux/pm_runtime.h> >> #include <linux/remoteproc.h> >> #include <linux/suspend.h> >> +#include <linux/iopoll.h> >> #include <linux/reset.h> >> #include <linux/slab.h> >> @@ -172,8 +173,23 @@ struct k3_r5_rproc { >> struct k3_r5_core *core; >> struct k3_r5_mem *rmem; >> int num_rmems; >> + struct completion shutdown_complete; >> }; >> +/* >> + * This will return true if the remote core is in Wait For Interrupt >> state. >> + */ >> +static bool k3_r5_is_core_in_wfi(struct k3_r5_core *core) >> +{ >> + int ret; >> + u64 boot_vec; >> + u32 cfg, ctrl, stat; >> + >> + ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, >> &stat); >> + >> + return !ret ? !!(stat & PROC_BOOT_STATUS_FLAG_R5_WFI) : false; > > Too fancy for me :) Just return if (ret) right after get_status(). Ok, too much punctuation :) > > Looks like this function is called in a polling loop, if > ti_sci_proc_get_status() fails once, it won't get better, > no need to keep checking, we should just error out of > the polling loop. Ok Thanks ! > > Andrew > >> +} >> + >> /** >> * k3_r5_rproc_mbox_callback() - inbound mailbox message handler >> * @client: mailbox client pointer used for requesting the mailbox >> channel >> @@ -209,6 +225,10 @@ static void k3_r5_rproc_mbox_callback(struct >> mbox_client *client, void *data) >> case RP_MBOX_ECHO_REPLY: >> dev_info(dev, "received echo reply from %s\n", name); >> break; >> + case RP_MBOX_SHUTDOWN_ACK: >> + dev_dbg(dev, "received shutdown_ack from %s\n", name); >> + complete(&kproc->shutdown_complete); >> + break; >> default: >> /* silently handle all other valid messages */ >> if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) >> @@ -634,6 +654,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc) >> struct k3_r5_cluster *cluster = kproc->cluster; >> struct device *dev = kproc->dev; >> struct k3_r5_core *core1, *core = kproc->core; >> + bool wfi; >> int ret; >> @@ -650,6 +671,24 @@ static int k3_r5_rproc_stop(struct rproc *rproc) >> } >> } >> + /* Send SHUTDOWN message to remote proc */ >> + reinit_completion(&kproc->shutdown_complete); >> + ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_SHUTDOWN); >> + if (ret < 0) { >> + dev_err(dev, "Sending SHUTDOWN message failed: %d. Halting >> core anyway.\n", ret); >> + } else { >> + ret = wait_for_completion_timeout(&kproc->shutdown_complete, >> + msecs_to_jiffies(1000)); >> + if (ret == 0) { >> + dev_err(dev, "Timeout waiting SHUTDOWN_ACK message. >> Halting core anyway.\n"); >> + } else { >> + ret = readx_poll_timeout(k3_r5_is_core_in_wfi, core, >> + wfi, wfi, 200, 2000); >> + if (ret) >> + dev_err(dev, "Timeout waiting for remote proc to be >> in WFI state. Halting core anyway.\n"); >> + } >> + } >> + >> /* halt all applicable cores */ >> if (cluster->mode == CLUSTER_MODE_LOCKSTEP) { >> list_for_each_entry(core, &cluster->cores, elem) { >> @@ -1410,6 +1449,7 @@ static int k3_r5_cluster_rproc_init(struct >> platform_device *pdev) >> goto err_config; >> } >> + init_completion(&kproc->shutdown_complete); >> init_rmem: >> k3_r5_adjust_tcm_sizes(kproc); >>
Hi Richard,
kernel test robot noticed the following build warnings:
[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on linus/master v6.10-rc6 next-20240701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Genoud/remoteproc-k3-r5-Fix-IPC-only-mode-detection/20240625-201619
base: git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link: https://lore.kernel.org/r/20240621150058.319524-5-richard.genoud%40bootlin.com
patch subject: [PATCH 4/4] remoteproc: k3-r5: support for graceful stop of remote cores
config: arm64-randconfig-003-20240701
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build):
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407020526.IJBgqeV4-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/remoteproc/ti_k3_r5_remoteproc.c:118: warning: Function parameter or struct member 'pm_notifier' not described in 'k3_r5_cluster'
>> drivers/remoteproc/ti_k3_r5_remoteproc.c:177: warning: Function parameter or struct member 'shutdown_complete' not described in 'k3_r5_rproc'
vim +177 drivers/remoteproc/ti_k3_r5_remoteproc.c
6dedbd1d544389 Suman Anna 2020-10-02 155
6dedbd1d544389 Suman Anna 2020-10-02 156 /**
6dedbd1d544389 Suman Anna 2020-10-02 157 * struct k3_r5_rproc - K3 remote processor state
6dedbd1d544389 Suman Anna 2020-10-02 158 * @dev: cached device pointer
6dedbd1d544389 Suman Anna 2020-10-02 159 * @cluster: cached pointer to parent cluster structure
6dedbd1d544389 Suman Anna 2020-10-02 160 * @mbox: mailbox channel handle
6dedbd1d544389 Suman Anna 2020-10-02 161 * @client: mailbox client to request the mailbox channel
6dedbd1d544389 Suman Anna 2020-10-02 162 * @rproc: rproc handle
6dedbd1d544389 Suman Anna 2020-10-02 163 * @core: cached pointer to r5 core structure being used
6dedbd1d544389 Suman Anna 2020-10-02 164 * @rmem: reserved memory regions data
6dedbd1d544389 Suman Anna 2020-10-02 165 * @num_rmems: number of reserved memory regions
6dedbd1d544389 Suman Anna 2020-10-02 166 */
6dedbd1d544389 Suman Anna 2020-10-02 167 struct k3_r5_rproc {
6dedbd1d544389 Suman Anna 2020-10-02 168 struct device *dev;
6dedbd1d544389 Suman Anna 2020-10-02 169 struct k3_r5_cluster *cluster;
6dedbd1d544389 Suman Anna 2020-10-02 170 struct mbox_chan *mbox;
6dedbd1d544389 Suman Anna 2020-10-02 171 struct mbox_client client;
6dedbd1d544389 Suman Anna 2020-10-02 172 struct rproc *rproc;
6dedbd1d544389 Suman Anna 2020-10-02 173 struct k3_r5_core *core;
6dedbd1d544389 Suman Anna 2020-10-02 174 struct k3_r5_mem *rmem;
6dedbd1d544389 Suman Anna 2020-10-02 175 int num_rmems;
04ad7e52fa3358 Richard Genoud 2024-06-21 176 struct completion shutdown_complete;
6dedbd1d544389 Suman Anna 2020-10-02 @177 };
6dedbd1d544389 Suman Anna 2020-10-02 178
diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h index 828e13256c02..c008f11fa2a4 100644 --- a/drivers/remoteproc/omap_remoteproc.h +++ b/drivers/remoteproc/omap_remoteproc.h @@ -42,6 +42,11 @@ * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor * on a suspend request * + * @RP_MBOX_SHUTDOWN: shutdown request for the remote processor + * + * @RP_MBOX_SHUTDOWN_ACK: successful response from remote processor for a + * shutdown request. The remote processor should be in WFI state short after. + * * Introduce new message definitions if any here. * * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core @@ -59,7 +64,9 @@ enum omap_rp_mbox_messages { RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, - RP_MBOX_END_MSG = 0xFFFFFF14, + RP_MBOX_SHUTDOWN = 0xFFFFFF14, + RP_MBOX_SHUTDOWN_ACK = 0xFFFFFF15, + RP_MBOX_END_MSG = 0xFFFFFF16, }; #endif /* _OMAP_RPMSG_H */ diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c index a2ead87952c7..918a15e1dd9a 100644 --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c @@ -21,6 +21,7 @@ #include <linux/pm_runtime.h> #include <linux/remoteproc.h> #include <linux/suspend.h> +#include <linux/iopoll.h> #include <linux/reset.h> #include <linux/slab.h> @@ -172,8 +173,23 @@ struct k3_r5_rproc { struct k3_r5_core *core; struct k3_r5_mem *rmem; int num_rmems; + struct completion shutdown_complete; }; +/* + * This will return true if the remote core is in Wait For Interrupt state. + */ +static bool k3_r5_is_core_in_wfi(struct k3_r5_core *core) +{ + int ret; + u64 boot_vec; + u32 cfg, ctrl, stat; + + ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat); + + return !ret ? !!(stat & PROC_BOOT_STATUS_FLAG_R5_WFI) : false; +} + /** * k3_r5_rproc_mbox_callback() - inbound mailbox message handler * @client: mailbox client pointer used for requesting the mailbox channel @@ -209,6 +225,10 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client *client, void *data) case RP_MBOX_ECHO_REPLY: dev_info(dev, "received echo reply from %s\n", name); break; + case RP_MBOX_SHUTDOWN_ACK: + dev_dbg(dev, "received shutdown_ack from %s\n", name); + complete(&kproc->shutdown_complete); + break; default: /* silently handle all other valid messages */ if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) @@ -634,6 +654,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc) struct k3_r5_cluster *cluster = kproc->cluster; struct device *dev = kproc->dev; struct k3_r5_core *core1, *core = kproc->core; + bool wfi; int ret; @@ -650,6 +671,24 @@ static int k3_r5_rproc_stop(struct rproc *rproc) } } + /* Send SHUTDOWN message to remote proc */ + reinit_completion(&kproc->shutdown_complete); + ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_SHUTDOWN); + if (ret < 0) { + dev_err(dev, "Sending SHUTDOWN message failed: %d. Halting core anyway.\n", ret); + } else { + ret = wait_for_completion_timeout(&kproc->shutdown_complete, + msecs_to_jiffies(1000)); + if (ret == 0) { + dev_err(dev, "Timeout waiting SHUTDOWN_ACK message. Halting core anyway.\n"); + } else { + ret = readx_poll_timeout(k3_r5_is_core_in_wfi, core, + wfi, wfi, 200, 2000); + if (ret) + dev_err(dev, "Timeout waiting for remote proc to be in WFI state. Halting core anyway.\n"); + } + } + /* halt all applicable cores */ if (cluster->mode == CLUSTER_MODE_LOCKSTEP) { list_for_each_entry(core, &cluster->cores, elem) { @@ -1410,6 +1449,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev) goto err_config; } + init_completion(&kproc->shutdown_complete); init_rmem: k3_r5_adjust_tcm_sizes(kproc);
Introduce software IPC handshake between the K3-R5 remote proc driver and the R5 MCU to gracefully stop/reset the remote core. Upon a stop request, K3-R5 remote proc driver sends a RP_MBOX_SHUTDOWN mailbox message to the remote R5 core. The remote core is expected to: - relinquish all the resources acquired through Device Manager (DM) - disable its interrupts - send back a mailbox acknowledgment RP_MBOX_SHUDOWN_ACK - enter WFI state. Meanwhile, the K3-R5 remote proc driver does: - wait for the RP_MBOX_SHUTDOWN_ACK from the remote core - wait for the remote proc to enter WFI state - reset the remote core through device manager Based on work from: Hari Nagalla <hnagalla@ti.com> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> --- drivers/remoteproc/omap_remoteproc.h | 9 +++++- drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-)