On Sat, Jul 26, 2025 at 12:48:14PM -0500, Andrew Davis wrote:
> On 7/26/25 9:39 AM, Hiago De Franco wrote:
> > Hi Andrew, Beleswar,
> >
> > On Fri, Jul 25, 2025 at 02:29:22PM -0500, Andrew Davis wrote:
> > >
> > > So the issue then looks to be this message we send here when we setup
> > > the mailbox[0]. This mailbox setup is done during probe() for the K3
> > > rproc drivers now (mailbox setup used to be done during
> > > rproc_{start,attach}() before [1]). Moving mailbox setup to probe
> > > is correct, but we should have factored out the test message sending
> > > code out of mailbox setup so it could have been left in
> > > rproc_{start,attach}(). That way we only send this message if the
> > > core is going to be started, no sense in sending that message if
> > > we are not even going to run the core..
> > >
> > > Fix might be as simple as [2] (not tested, if this works feel free
> > > to send as a fix)
> >
> > I tested the patch and it works, thanks!
> >
> > >
> > > Andrew
> > >
> > > [0]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/ti_k3_common.c#n176
> > > [1]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f3f11cfe890733373ddbb1ce8991ccd4ee5e79e1
> > > [2]
> > >
> > > diff --git a/drivers/remoteproc/ti_k3_common.c
> > > b/drivers/remoteproc/ti_k3_common.c
> > > index a70d4879a8bea..657a200fa9040 100644
> > > --- a/drivers/remoteproc/ti_k3_common.c
> > > +++ b/drivers/remoteproc/ti_k3_common.c
> > > @@ -198,6 +198,22 @@ int k3_rproc_reset(struct k3_rproc *kproc)
> > > }
> > > EXPORT_SYMBOL_GPL(k3_rproc_reset);
> > > +static int k3_rproc_ping(struct k3_rproc *kproc)
> > > +{
> > > + /*
> > > + * Ping the remote processor, this is only for sanity-sake for
> > > now;
> > > + * there is no functional effect whatsoever.
> > > + *
> > > + * Note that the reply will _not_ arrive immediately: this message
> > > + * will wait in the mailbox fifo until the remote processor is
> > > booted.
> > > + */
> > > + int ret = mbox_send_message(kproc->mbox, (void
> > > *)RP_MBOX_ECHO_REQUEST);
> > > + if (ret < 0)
> > > + dev_err(kproc->dev, "mbox_send_message failed (%pe)\n",
> > > ERR_PTR(ret));
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /* Release the remote processor from reset */
> > > int k3_rproc_release(struct k3_rproc *kproc)
> > > {
> > > @@ -221,6 +237,8 @@ int k3_rproc_release(struct k3_rproc *kproc)
> > > if (ret)
> > > dev_err(dev, "module-reset deassert failed (%pe)\n",
> > > ERR_PTR(ret));
> > > + k3_rproc_ping(kproc);
> > > +
> > > return ret;
> > > }
> > > EXPORT_SYMBOL_GPL(k3_rproc_release);
> > > @@ -243,20 +261,6 @@ int k3_rproc_request_mbox(struct rproc *rproc)
> > > return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> > > "mbox_request_channel failed\n");
> > > - /*
> > > - * Ping the remote processor, this is only for sanity-sake for
> > > now;
> > > - * there is no functional effect whatsoever.
> > > - *
> > > - * Note that the reply will _not_ arrive immediately: this message
> > > - * will wait in the mailbox fifo until the remote processor is
> > > booted.
> > > - */
> > > - ret = mbox_send_message(kproc->mbox, (void
> > > *)RP_MBOX_ECHO_REQUEST);
> > > - if (ret < 0) {
> > > - dev_err(dev, "mbox_send_message failed (%pe)\n",
> > > ERR_PTR(ret));
> > > - mbox_free_channel(kproc->mbox);
> > > - return ret;
> > > - }
> > > -
> > > return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(k3_rproc_request_mbox);
> > > @@ -397,7 +401,12 @@ EXPORT_SYMBOL_GPL(k3_rproc_stop);
> > > * remote core. This callback is invoked only in IPC-only mode and
> > > exists
> > > * because rproc_validate() checks for its existence.
> > > */
> > > -int k3_rproc_attach(struct rproc *rproc) { return 0; }
> > > +int k3_rproc_attach(struct rproc *rproc)
> > > +{
> > > + k3_rproc_ping(rproc->priv);
> > > +
> > > + return 0;
> > > +}
> > > EXPORT_SYMBOL_GPL(k3_rproc_attach);
> > > /*
> > >
> >
> > On Sat, Jul 26, 2025 at 07:47:34PM +0530, Beleswar Prasad Padhi wrote:
> > > >
> > > > So the issue then looks to be this message we send here when we setup
> > > > the mailbox[0]. This mailbox setup is done during probe() for the K3
> > > > rproc drivers now (mailbox setup used to be done during
> > > > rproc_{start,attach}() before [1]). Moving mailbox setup to probe
> > > > is correct, but we should have factored out the test message sending
> > > > code out of mailbox setup so it could have been left in
> > > > rproc_{start,attach}().
> > >
> > >
> > > Or, how about we don't send that test mbox message at all. It does not
> > > actually check if the remoteproc was able to receive and respond to the
> > > message. It only verifies if the write to the mbox queue was successful.
> > > And
> > > most firmwares anyways don't reply to that mailbox-level echo message.
> >
> > I was thinking about the same.
> >
> > I tested the patch and it indeed works, however when I boot the remote
> > core with a hello world firwmare from the TI MCU SDK (with IPC enabled
> > with the sysconfig), the ping is sent but M4 never replies to it, which
> > at the end causes an unread message to stay there. Later, if I stop the
> > remote processor, I can not got into suspend mode again because of this
> > message.
> >
> > So I believe we should never send the message or clear the mailbox when
> > the remote processor is stopped, but I was not able to find a way to
> > clear the mailbox. So, is it ok if we never send the ping?
> >
>
> So right now it is okay to not send that ping, and in the past I've
> thought about removing it (it is a bit of a legacy hold-over from
> the OMAP RProc driver. Back then we would send other messages like
> suspend and shutdown requests, you can see the different messages
> here[0]. Actually using those messages never got upstream, only the
> ping message part did.
>
> For K3 we want to start making use of all these other messages and
> upstream the support for the same. So removing the ping test message
> felt like a step backwards as it is a good placeholder for the more
> important messages we want to send later. But as said, removing it
> is probably fine for now.
>
> The second thing on the roadmap is to better deal with messages left
> in the mailbox queue when we try to suspend. Basically on suspend the
> mailbox IP is powered down and all messages waiting will be lost, this
> can cause issues. Instead of blocking suspend, one other option would
> be to attempt to read out these messages and restore them on resume.
> This state saving would match what most other IP drivers. This would
> fix issues like the above were the firmware doesn't consume a message
> for whatever reason. But until we implement that, either we throw out
> messages on suspend, or we block suspend.
Got it, thanks for you explanation Andrew. I am out this week, so next
week I will propose a patch so we can turn around this issue.
Thanks!
Hiago.
>
> Andrew
>
> [0] drivers/remoteproc/omap_remoteproc.h
>
> > Best regards,
> > Hiago.
> >
> > >
> > > Thanks,
> > > Beleswar
> > >
> > > > That way we only send this message if the
> > > > core is going to be started, no sense in sending that message if
> > > > we are not even going to run the core..
> > > >
> > > > Fix might be as simple as [2] (not tested, if this works feel free
> > > > to send as a fix)
> > > >
> > > > Andrew
> > > >
> > > > [0]
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/ti_k3_common.c#n176
> > > > [1]
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f3f11cfe890733373ddbb1ce8991ccd4ee5e79e1
> > > > [2]
> > > >
> > > > diff --git a/drivers/remoteproc/ti_k3_common.c
> > > > b/drivers/remoteproc/ti_k3_common.c
> > > > index a70d4879a8bea..657a200fa9040 100644
> > > > --- a/drivers/remoteproc/ti_k3_common.c
> > > > +++ b/drivers/remoteproc/ti_k3_common.c
> > > > @@ -198,6 +198,22 @@ int k3_rproc_reset(struct k3_rproc *kproc)
> > > > }
> > > > EXPORT_SYMBOL_GPL(k3_rproc_reset);
> > > >
> > > > +static int k3_rproc_ping(struct k3_rproc *kproc)
> > > > +{
> > > > + /*
> > > > + * Ping the remote processor, this is only for sanity-sake for
> > > > now;
> > > > + * there is no functional effect whatsoever.
> > > > + *
> > > > + * Note that the reply will _not_ arrive immediately: this
> > > > message
> > > > + * will wait in the mailbox fifo until the remote processor is
> > > > booted.
> > > > + */
> > > > + int ret = mbox_send_message(kproc->mbox, (void
> > > > *)RP_MBOX_ECHO_REQUEST);
> > > > + if (ret < 0)
> > > > + dev_err(kproc->dev, "mbox_send_message failed (%pe)\n",
> > > > ERR_PTR(ret));
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > /* Release the remote processor from reset */
> > > > int k3_rproc_release(struct k3_rproc *kproc)
> > > > {
> > > > @@ -221,6 +237,8 @@ int k3_rproc_release(struct k3_rproc *kproc)
> > > > if (ret)
> > > > dev_err(dev, "module-reset deassert failed (%pe)\n",
> > > > ERR_PTR(ret));
> > > >
> > > > + k3_rproc_ping(kproc);
> > > > +
> > > > return ret;
> > > > }
> > > > EXPORT_SYMBOL_GPL(k3_rproc_release);
> > > > @@ -243,20 +261,6 @@ int k3_rproc_request_mbox(struct rproc *rproc)
> > > > return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> > > > "mbox_request_channel failed\n");
> > > >
> > > > - /*
> > > > - * Ping the remote processor, this is only for sanity-sake for
> > > > now;
> > > > - * there is no functional effect whatsoever.
> > > > - *
> > > > - * Note that the reply will _not_ arrive immediately: this
> > > > message
> > > > - * will wait in the mailbox fifo until the remote processor is
> > > > booted.
> > > > - */
> > > > - ret = mbox_send_message(kproc->mbox, (void
> > > > *)RP_MBOX_ECHO_REQUEST);
> > > > - if (ret < 0) {
> > > > - dev_err(dev, "mbox_send_message failed (%pe)\n",
> > > > ERR_PTR(ret));
> > > > - mbox_free_channel(kproc->mbox);
> > > > - return ret;
> > > > - }
> > > > -
> > > > return 0;
> > > > }
> > > > EXPORT_SYMBOL_GPL(k3_rproc_request_mbox);
> > > > @@ -397,7 +401,12 @@ EXPORT_SYMBOL_GPL(k3_rproc_stop);
> > > > * remote core. This callback is invoked only in IPC-only mode and
> > > > exists
> > > > * because rproc_validate() checks for its existence.
> > > > */
> > > > -int k3_rproc_attach(struct rproc *rproc) { return 0; }
> > > > +int k3_rproc_attach(struct rproc *rproc)
> > > > +{
> > > > + k3_rproc_ping(rproc->priv);
> > > > +
> > > > + return 0;
> > > > +}
> > > > EXPORT_SYMBOL_GPL(k3_rproc_attach);
> > > >
> > > > /*
> > > >
>
