[PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment

[Peng Fan]

On Fri, May 09, 2025 at 04:13:08PM -0300, Hiago De Franco wrote:
>On Fri, May 09, 2025 at 12:37:02PM +0200, Ulf Hansson wrote:
>> On Thu, 8 May 2025 at 22:28, Hiago De Franco <hiagofranco at gmail.com> wrote:
>> >
>> > Hello,
>> >
>> > On Thu, May 08, 2025 at 12:03:33PM +0200, Ulf Hansson wrote:
>> > > On Wed, 7 May 2025 at 18:02, Hiago De Franco <hiagofranco at gmail.com> wrote:
>> > > >
>> > > > From: Hiago De Franco <hiago.franco at toradex.com>
>> > > >
>> > > > When the remote core is started before Linux boots (e.g., by the
>> > > > bootloader), the driver currently is not able to attach because it only
>> > > > checks for cores running in different partitions. If the core was kicked
>> > > > by the bootloader, it is in the same partition as Linux and it is
>> > > > already up and running.
>> > > >
>> > > > This adds power mode verification through the SCU interface, enabling
>> > > > the driver to detect when the remote core is already running and
>> > > > properly attach to it.
>> > > >
>> > > > Signed-off-by: Hiago De Franco <hiago.franco at toradex.com>
>> > > > Suggested-by: Peng Fan <peng.fan at nxp.com>
>> > > > ---
>> > > > v2: Dropped unecessary include. Removed the imx_rproc_is_on function, as
>> > > > suggested.
>> > > > ---
>> > > >  drivers/remoteproc/imx_rproc.c | 13 +++++++++++++
>> > > >  1 file changed, 13 insertions(+)
>> > > >
>> > > > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>> > > > index 627e57a88db2..9b6e9e41b7fc 100644
>> > > > --- a/drivers/remoteproc/imx_rproc.c
>> > > > +++ b/drivers/remoteproc/imx_rproc.c
>> > > > @@ -949,6 +949,19 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>> > > >                         if (of_property_read_u32(dev->of_node, "fsl,entry-address", &priv->entry))
>> > > >                                 return -EINVAL;
>> > > >
>> > > > +                       /*
>> > > > +                        * If remote core is already running (e.g. kicked by
>> > > > +                        * the bootloader), attach to it.
>> > > > +                        */
>> > > > +                       ret = imx_sc_pm_get_resource_power_mode(priv->ipc_handle,
>> > > > +                                                               priv->rsrc_id);
>> > > > +                       if (ret < 0)
>> > > > +                               dev_err(dev, "failed to get power resource %d mode, ret %d\n",
>> > > > +                                       priv->rsrc_id, ret);
>> > > > +
>> > > > +                       if (ret == IMX_SC_PM_PW_MODE_ON)
>> > > > +                               priv->rproc->state = RPROC_DETACHED;
>> > > > +
>> > > >                         return imx_rproc_attach_pd(priv);
>> > >
>> > > Why is it important to potentially set "priv->rproc->state =
>> > > RPROC_DETACHED" before calling imx_rproc_attach_pd()?
>> > >
>> > > Would it be possible to do it the other way around? First calling
>> > > imx_rproc_attach_pd() then get the power-mode to know if
>> > > RPROC_DETACHED should be set or not?
>> > >
>> > > The main reason why I ask, is because of how we handle the single PM
>> > > domain case. In that case, the PM domain has already been attached
>> > > (and powered-on) before we reach this point.
>> >
>> > I am not sure if I understood correcly, let me know if I missed
>> > something. From my understanding in this case it does not matter, since
>> > the RPROC_DETACHED will only be a flag to trigger the attach callback
>> > from rproc_validate(), when rproc_add() is called inside
>> > remoteproc_core.c.
>> 
>> Okay, I see.
>> 
>> To me, it sounds like we should introduce a new genpd helper function
>> instead. Something along the lines of this (drivers/pmdomain/core.c)
>> 
>> bool dev_pm_genpd_is_on(struct device *dev)
>> {
>>         struct generic_pm_domain *genpd;
>>         bool is_on;
>> 
>>         genpd = dev_to_genpd_safe(dev);
>>         if (!genpd)
>>                 return false;
>> 
>>         genpd_lock(genpd);
>>         is_on = genpd_status_on(genpd);
>>         genpd_unlock(genpd);
>> 
>>         return is_on;
>> }
>> 
>> After imx_rproc_attach_pd() has run, we have the devices that
>> correspond to the genpd(s). Those can then be passed as in-parameters
>> to the above function to get the power-state of their PM domains
>> (genpds). Based on that, we can decide if priv->rproc->state should be
>> to RPROC_DETACHED or not. Right?
>
>Got your idea, I think it should work yes, I am not so sure how. From
>what I can see these power domains are managed by
>drivers/pmdomain/imx/scu-pd.c and by enabling the debug messages I can
>see the power mode is correct when the remote core is powered on:
>
>[    0.317369] imx-scu-pd system-controller:power-controller: cm40-pid0 : IMX_SC_PM_PW_MODE_ON
>
>and powered off:
>
>[    0.314953] imx-scu-pd system-controller:power-controller: cm40-pid0 : IMX_SC_PM_PW_MODE_OFF
>
>But I cannot see how to integrate this into the dev_pm_genpd_is_on() you
>proposed. For a quick check, I added this function and it always return
>NULL at dev_to_genpd_safe(). Can you help me to understand this part?

Ulf's new API dev_pm_genpd_is_on needs to run after power domain attached.

But if run after power domain attached, there is no API to know whether
M4 is kicked by bootloader or now.

Even imx_rproc_attach_pd has a check for single power domain, but I just
give a look again on current i.MX8QM/QX, all are using two power domain
entries.

>
>> 
>> In this way we don't need to export unnecessary firmware functions
>> from firmware/imx/misc.c, as patch1/3 does.


I think still need to export firmware API. My idea is
1. introduce a new firmware API and put under firmware/imx/power.c
2. Use this new firmware API in imx_rproc.c
3. Replace scu-pd.c to use this new firmware API.

Or
1. Export the API in scu-pd.c
2. Use the API in imx_rproc.c

With approach two, you need to handle them in three trees in one patchset:
imx/pd/rproc.

With approach one, you need to handle two trees in one patchset: imx/rproc tree,
then after done, pd tree

Regards,
Peng
>> 
>> If you think it can work, I can help to cook a formal patch for the
>> above helper that you can fold into your series. Let me know.
>> 
>> >
>> > With that we can correcly attach to the remote core running, which was
>> > not possible before, where the function returns at "return
>> > imx_rproc_attach_pd(priv);" with the RPROC_OFFLINE state to
>> > rproc_validate().
>> 
>> I see, thanks for clarifying!
>> 
>> Kind regards
>> Uffe
>
>Thank you!
>Hiago.