On Mon, 12 May 2025 at 05:46, Peng Fan <peng....@oss.nxp.com> wrote:
>
> 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 <hiagofra...@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 <hiagofra...@gmail.com>
> >> > > wrote:
> >> > > >
> >> > > > From: Hiago De Franco <hiago.fra...@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.fra...@toradex.com>
> >> > > > Suggested-by: Peng Fan <peng....@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.
Correct, but you need to provide the correct "dev" to it. See my other
reply to Hiago.
>
> But if run after power domain attached, there is no API to know whether
> M4 is kicked by bootloader or now.
As long as you have multiple PM domains attached for a device, genpd
will *not* power on the PM domain(s).
Genpd does a power-on in the single PM domain case (for legacy
reasons), but that should not be a problem here, right?
So what am I missing?
>
> 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.
Kind regards
Uffe
