From: Hiago De Franco <hiago.franco@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@toradex.com>
Suggested-by: Peng Fan <peng.fan@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);
}
--
2.39.5On Wed, 7 May 2025 at 18:02, Hiago De Franco <hiagofranco@gmail.com> wrote: > > From: Hiago De Franco <hiago.franco@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@toradex.com> > Suggested-by: Peng Fan <peng.fan@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. > } > > -- > 2.39.5 > Kind regards Uffe
Hi Ulf, 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@gmail.com> wrote: >> >> From: Hiago De Franco <hiago.franco@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@toradex.com> >> Suggested-by: Peng Fan <peng.fan@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? If M4 is not powered up by bootloader, attach_pd is to power up the related power domains. And the rproc->state should be OFFLINE. If M4 is powered up by bootloader, the rproc->state should be set as DETACHED, then attach_pd here will not touch the real pd, because scu-pd driver set is_off as false when doing pm_genpd_init. In this case, we still need attach_pd to avoid power shutdown when pd_ignore_unused and also need to support linux stop/start m4 even it is started by bootloader. So we could not reverse the logic. Regards, Peng > >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. > >> } >> >> -- >> 2.39.5 >> > >Kind regards >Uffe
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@gmail.com> wrote: > > > > From: Hiago De Franco <hiago.franco@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@toradex.com> > > Suggested-by: Peng Fan <peng.fan@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. 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(). > > > } > > > > -- > > 2.39.5 > > > > Kind regards > Uffe Best Regards, Hiago.
On Thu, 8 May 2025 at 22:28, Hiago De Franco <hiagofranco@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@gmail.com> wrote:
> > >
> > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> > > Suggested-by: Peng Fan <peng.fan@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?
In this way we don't need to export unnecessary firmware functions
from firmware/imx/misc.c, as patch1/3 does.
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
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@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@gmail.com> wrote:
> > > >
> > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> > > > Suggested-by: Peng Fan <peng.fan@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?
>
> In this way we don't need to export unnecessary firmware functions
> from firmware/imx/misc.c, as patch1/3 does.
>
> 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.
On Fri, 9 May 2025 at 21:13, Hiago De Franco <hiagofranco@gmail.com> 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@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@gmail.com> wrote:
> > > > >
> > > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> > > > > Suggested-by: Peng Fan <peng.fan@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?
As your device has multiple PM domains and those gets attached with
dev_pm_domain_attach_list(), the device(s) that you should use with
dev_pm_genpd_is_on() are in imx_rproc->pd_list->pd_devs[n].
>
> >
> > In this way we don't need to export unnecessary firmware functions
> > from firmware/imx/misc.c, as patch1/3 does.
> >
> > 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
Hi Ulf,
On Mon, May 19, 2025 at 04:33:30PM +0200, Ulf Hansson wrote:
> On Fri, 9 May 2025 at 21:13, Hiago De Franco <hiagofranco@gmail.com> 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@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@gmail.com> wrote:
> > > > > >
> > > > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> > > > > > Suggested-by: Peng Fan <peng.fan@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?
>
> As your device has multiple PM domains and those gets attached with
> dev_pm_domain_attach_list(), the device(s) that you should use with
> dev_pm_genpd_is_on() are in imx_rproc->pd_list->pd_devs[n].
Ok got it, thanks for sharing.
I just send the v3 with the changes Peng proposed (here
https://lore.kernel.org/lkml/20250519171514.61974-1-hiagofranco@gmail.com/T/#t),
but I am a bit confused which path we should take, the initial approach
proposed or using these PD functions. Maybe we can discuss this in the
new v3 patch series?
>
> >
> > >
> > > In this way we don't need to export unnecessary firmware functions
> > > from firmware/imx/misc.c, as patch1/3 does.
> > >
> > > 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
Thanks,
Hiago.
On Mon, 19 May 2025 at 19:24, Hiago De Franco <hiagofranco@gmail.com> wrote:
>
> Hi Ulf,
>
> On Mon, May 19, 2025 at 04:33:30PM +0200, Ulf Hansson wrote:
> > On Fri, 9 May 2025 at 21:13, Hiago De Franco <hiagofranco@gmail.com> 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@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@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> > > > > > > Suggested-by: Peng Fan <peng.fan@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?
> >
> > As your device has multiple PM domains and those gets attached with
> > dev_pm_domain_attach_list(), the device(s) that you should use with
> > dev_pm_genpd_is_on() are in imx_rproc->pd_list->pd_devs[n].
>
> Ok got it, thanks for sharing.
>
> I just send the v3 with the changes Peng proposed (here
> https://lore.kernel.org/lkml/20250519171514.61974-1-hiagofranco@gmail.com/T/#t),
> but I am a bit confused which path we should take, the initial approach
> proposed or using these PD functions. Maybe we can discuss this in the
> new v3 patch series?
I think it would be better if we can avoid sharing low-level firmware
functions for PM domains. I am worried that they may become abused for
other future use-cases.
So, if possible, I would rather make us try to use
dev_pm_genpd_is_on() (or something along those lines), but let's see
what Peng thinks about it before we make the decision.
[...]
Kind regards
Uffe
Hi Ulf,
On Tue, May 20, 2025 at 02:21:49PM +0200, Ulf Hansson wrote:
>On Mon, 19 May 2025 at 19:24, Hiago De Franco <hiagofranco@gmail.com> wrote:
>>
>> Hi Ulf,
>>
>> On Mon, May 19, 2025 at 04:33:30PM +0200, Ulf Hansson wrote:
>> > On Fri, 9 May 2025 at 21:13, Hiago De Franco <hiagofranco@gmail.com> 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@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@gmail.com> wrote:
>> > > > > > >
>> > > > > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
>> > > > > > > Suggested-by: Peng Fan <peng.fan@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?
>> >
>> > As your device has multiple PM domains and those gets attached with
>> > dev_pm_domain_attach_list(), the device(s) that you should use with
>> > dev_pm_genpd_is_on() are in imx_rproc->pd_list->pd_devs[n].
>>
>> Ok got it, thanks for sharing.
>>
>> I just send the v3 with the changes Peng proposed (here
>> https://lore.kernel.org/lkml/20250519171514.61974-1-hiagofranco@gmail.com/T/#t),
>> but I am a bit confused which path we should take, the initial approach
>> proposed or using these PD functions. Maybe we can discuss this in the
>> new v3 patch series?
>
>I think it would be better if we can avoid sharing low-level firmware
>functions for PM domains. I am worried that they may become abused for
>other future use-cases.
>
>So, if possible, I would rather make us try to use
>dev_pm_genpd_is_on() (or something along those lines), but let's see
>what Peng thinks about it before we make the decision.
There are two power domains for this m4:
power-domains = <&pd IMX_SC_R_M4_0_PID0>, <&pd IMX_SC_R_M4_0_MU_1A>;
So before attach the pd, dev_pm_genpd_is_on should also return false
per my understanding. If run dev_pm_genpd_is_on after attaching the pd,
the pd will be powered on. So we are not able to know whether M4 is started
by bootloader or not.
Hiago's case needs the real power status before attaching the
pd to set remoteproc as DETACHED(M4 kicked by bootloader) or OFFLINE(
M4 not kicked by bootloader) state.
Seems there is no other SCFW API to check whether M4 is started by
bootloader.
I not have good idea as of now except directly checking the real
power status to indicate M4 started by bootloader or not. Or using a
device tree property runtime added by bootloader,
saying "fsl,rproc-started"?
Thanks,
Peng
>
>[...]
>
>Kind regards
>Uffe
On Wed, May 21, 2025 at 12:13:06PM +0800, Peng Fan wrote:
>Hi Ulf,
>
>On Tue, May 20, 2025 at 02:21:49PM +0200, Ulf Hansson wrote:
>>On Mon, 19 May 2025 at 19:24, Hiago De Franco <hiagofranco@gmail.com> wrote:
>>>
>>> Hi Ulf,
>>>
>>> On Mon, May 19, 2025 at 04:33:30PM +0200, Ulf Hansson wrote:
>>> > On Fri, 9 May 2025 at 21:13, Hiago De Franco <hiagofranco@gmail.com> 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@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@gmail.com> wrote:
>>> > > > > > >
>>> > > > > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
>>> > > > > > > Suggested-by: Peng Fan <peng.fan@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?
>>> >
>>> > As your device has multiple PM domains and those gets attached with
>>> > dev_pm_domain_attach_list(), the device(s) that you should use with
>>> > dev_pm_genpd_is_on() are in imx_rproc->pd_list->pd_devs[n].
>>>
>>> Ok got it, thanks for sharing.
>>>
>>> I just send the v3 with the changes Peng proposed (here
>>> https://lore.kernel.org/lkml/20250519171514.61974-1-hiagofranco@gmail.com/T/#t),
>>> but I am a bit confused which path we should take, the initial approach
>>> proposed or using these PD functions. Maybe we can discuss this in the
>>> new v3 patch series?
>>
>>I think it would be better if we can avoid sharing low-level firmware
>>functions for PM domains. I am worried that they may become abused for
>>other future use-cases.
>>
>>So, if possible, I would rather make us try to use
>>dev_pm_genpd_is_on() (or something along those lines), but let's see
>>what Peng thinks about it before we make the decision.
>
>There are two power domains for this m4:
>power-domains = <&pd IMX_SC_R_M4_0_PID0>, <&pd IMX_SC_R_M4_0_MU_1A>;
>
>So before attach the pd, dev_pm_genpd_is_on should also return false
>per my understanding. If run dev_pm_genpd_is_on after attaching the pd,
>the pd will be powered on. So we are not able to know whether M4 is started
>by bootloader or not.
Could we use PD_FLAG_NO_DEV_LINK when attach the PD, then
use dev_pm_genpd_is_on to detect the status of genpd?
we set is_off as true when pm_genpd_init if the PD is physical ON.
Thanks,
Peng
>
>Hiago's case needs the real power status before attaching the
>pd to set remoteproc as DETACHED(M4 kicked by bootloader) or OFFLINE(
>M4 not kicked by bootloader) state.
>
>Seems there is no other SCFW API to check whether M4 is started by
>bootloader.
>
>I not have good idea as of now except directly checking the real
>power status to indicate M4 started by bootloader or not. Or using a
>device tree property runtime added by bootloader,
>saying "fsl,rproc-started"?
>
>
>Thanks,
>Peng
>
>>
>>[...]
>>
>>Kind regards
>>Uffe
>
On Wed, 21 May 2025 at 05:09, Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> On Wed, May 21, 2025 at 12:13:06PM +0800, Peng Fan wrote:
> >Hi Ulf,
> >
> >On Tue, May 20, 2025 at 02:21:49PM +0200, Ulf Hansson wrote:
> >>On Mon, 19 May 2025 at 19:24, Hiago De Franco <hiagofranco@gmail.com> wrote:
> >>>
> >>> Hi Ulf,
> >>>
> >>> On Mon, May 19, 2025 at 04:33:30PM +0200, Ulf Hansson wrote:
> >>> > On Fri, 9 May 2025 at 21:13, Hiago De Franco <hiagofranco@gmail.com> 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@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@gmail.com> wrote:
> >>> > > > > > >
> >>> > > > > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> >>> > > > > > > Suggested-by: Peng Fan <peng.fan@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?
> >>> >
> >>> > As your device has multiple PM domains and those gets attached with
> >>> > dev_pm_domain_attach_list(), the device(s) that you should use with
> >>> > dev_pm_genpd_is_on() are in imx_rproc->pd_list->pd_devs[n].
> >>>
> >>> Ok got it, thanks for sharing.
> >>>
> >>> I just send the v3 with the changes Peng proposed (here
> >>> https://lore.kernel.org/lkml/20250519171514.61974-1-hiagofranco@gmail.com/T/#t),
> >>> but I am a bit confused which path we should take, the initial approach
> >>> proposed or using these PD functions. Maybe we can discuss this in the
> >>> new v3 patch series?
> >>
> >>I think it would be better if we can avoid sharing low-level firmware
> >>functions for PM domains. I am worried that they may become abused for
> >>other future use-cases.
> >>
> >>So, if possible, I would rather make us try to use
> >>dev_pm_genpd_is_on() (or something along those lines), but let's see
> >>what Peng thinks about it before we make the decision.
> >
> >There are two power domains for this m4:
> >power-domains = <&pd IMX_SC_R_M4_0_PID0>, <&pd IMX_SC_R_M4_0_MU_1A>;
> >
> >So before attach the pd, dev_pm_genpd_is_on should also return false
> >per my understanding. If run dev_pm_genpd_is_on after attaching the pd,
> >the pd will be powered on. So we are not able to know whether M4 is started
> >by bootloader or not.
>
>
> Could we use PD_FLAG_NO_DEV_LINK when attach the PD, then
> use dev_pm_genpd_is_on to detect the status of genpd?
>
> we set is_off as true when pm_genpd_init if the PD is physical ON.
>
You should not provide any flag (or attach_data to
dev_pm_domain_attach_list()) at all. In other words just call
dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
drivers/remoteproc/imx_dsp_rproc.c does it.
In this way, the device_link is created by making the platform->dev
the consumer and by keeping the supplier-devices (corresponding to the
genpds) in RPM_SUSPENDED state.
The PM domains (genpds) are then left in their current state, which
should allow us to call dev_pm_genpd_is_on() for the corresponding
supplier-devices, to figure out whether the bootloader turned them on
or not, I think.
Moreover, to make sure the genpds are turned on when needed, we also
need to call pm_runtime_enable(platform->dev) and
pm_runtime_get_sync(platform->dev). The easiest approach is probably
to do that during ->probe() - and then as an improvement on top you
may want to implement more fine-grained support for runtime PM.
[...]
Kind regards
Uffe
Hi Ulf,
On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
> You should not provide any flag (or attach_data to
> dev_pm_domain_attach_list()) at all. In other words just call
> dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
> drivers/remoteproc/imx_dsp_rproc.c does it.
>
> In this way, the device_link is created by making the platform->dev
> the consumer and by keeping the supplier-devices (corresponding to the
> genpds) in RPM_SUSPENDED state.
>
> The PM domains (genpds) are then left in their current state, which
> should allow us to call dev_pm_genpd_is_on() for the corresponding
> supplier-devices, to figure out whether the bootloader turned them on
> or not, I think.
>
> Moreover, to make sure the genpds are turned on when needed, we also
> need to call pm_runtime_enable(platform->dev) and
> pm_runtime_get_sync(platform->dev). The easiest approach is probably
> to do that during ->probe() - and then as an improvement on top you
> may want to implement more fine-grained support for runtime PM.
>
> [...]
>
> Kind regards
> Uffe
I did some tests here and I might be missing something. I used the
dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
@@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
if (dev->pm_domain)
return 0;
ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
+ printk("hfranco: returned pd devs is %d", ret);
+ for (int i = 0; i < ret; i++) {
+ test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
+ printk("hfranco: returned value is %d", test);
+ }
return ret < 0 ? ret : 0;
}
This was a quick test to check the returned value, and it always return
1 for both pds, even if I did not boot the remote core.
So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
it and passed NULL to dev_pm_domain_attach_list(). Booting the kernel
now it correctly reports 0 for both pds, however when I start the
remote core with a hello world firmware and boot the kernel, the CPU
resets with a fault reset ("Reset cause: SCFW fault reset").
I added both pm functions to probe, just to test:
@@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
goto err_put_clk;
}
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+
return 0
Now the kernel boot with the remote core running, but it still returns
0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
or without the remote core running.
I tried to move pm_runtime_get_sync() to .prepare function but it make
the kernel not boot anymore (with the SCU fault reset).
Do you have any suggestions? Am I doing something wrong with these PDs?
Best regards,
Hiago.
On Fri, 23 May 2025 at 21:17, Hiago De Franco <hiagofranco@gmail.com> wrote:
>
> Hi Ulf,
>
> On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
> > You should not provide any flag (or attach_data to
> > dev_pm_domain_attach_list()) at all. In other words just call
> > dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
> > drivers/remoteproc/imx_dsp_rproc.c does it.
> >
> > In this way, the device_link is created by making the platform->dev
> > the consumer and by keeping the supplier-devices (corresponding to the
> > genpds) in RPM_SUSPENDED state.
> >
> > The PM domains (genpds) are then left in their current state, which
> > should allow us to call dev_pm_genpd_is_on() for the corresponding
> > supplier-devices, to figure out whether the bootloader turned them on
> > or not, I think.
> >
> > Moreover, to make sure the genpds are turned on when needed, we also
> > need to call pm_runtime_enable(platform->dev) and
> > pm_runtime_get_sync(platform->dev). The easiest approach is probably
> > to do that during ->probe() - and then as an improvement on top you
> > may want to implement more fine-grained support for runtime PM.
> >
> > [...]
> >
> > Kind regards
> > Uffe
>
> I did some tests here and I might be missing something. I used the
> dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
>
> @@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
> if (dev->pm_domain)
> return 0;
>
> ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
> + printk("hfranco: returned pd devs is %d", ret);
> + for (int i = 0; i < ret; i++) {
> + test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
> + printk("hfranco: returned value is %d", test);
> + }
> return ret < 0 ? ret : 0;
> }
>
> This was a quick test to check the returned value, and it always return
> 1 for both pds, even if I did not boot the remote core.
>
> So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
> it and passed NULL to dev_pm_domain_attach_list().
Right, that's exactly what we should be doing.
> Booting the kernel
> now it correctly reports 0 for both pds, however when I start the
> remote core with a hello world firmware and boot the kernel, the CPU
> resets with a fault reset ("Reset cause: SCFW fault reset").
>
> I added both pm functions to probe, just to test:
>
> @@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
> goto err_put_clk;
> }
>
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
> +
Indeed, calling pm_runtime_enable() and then pm_runtime_get_sync()
should turn on the PM domains for the device, which I assume is needed
at some point.
Although, I wonder if this may be a bit too late, I would expect that
you at least need to call these *before* the call to rproc_add(), as I
assume the rproc-core may start using the device/driver beyond that
point.
> return 0
>
> Now the kernel boot with the remote core running, but it still returns
> 0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
> or without the remote core running.
dev_pm_genpd_is_on() is returning the current status of the PM domain
(genpd) for the device.
Could it be that the genpd provider doesn't register its PM domains
with the state that the HW is really in? pm_genpd_init() is the call
that allows the genpd provider to specify the initial state.
I think we need Peng's help here to understand what goes on.
>
> I tried to move pm_runtime_get_sync() to .prepare function but it make
> the kernel not boot anymore (with the SCU fault reset).
Try move pm_runtime_enable() before rproc_add().
>
> Do you have any suggestions? Am I doing something wrong with these PDs?
>
> Best regards,
> Hiago.
Kind regards
Uffe
On Mon, May 26, 2025 at 12:07:49PM +0200, Ulf Hansson wrote:
> On Fri, 23 May 2025 at 21:17, Hiago De Franco <hiagofranco@gmail.com> wrote:
> >
> > Hi Ulf,
> >
> > On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
> > > You should not provide any flag (or attach_data to
> > > dev_pm_domain_attach_list()) at all. In other words just call
> > > dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
> > > drivers/remoteproc/imx_dsp_rproc.c does it.
> > >
> > > In this way, the device_link is created by making the platform->dev
> > > the consumer and by keeping the supplier-devices (corresponding to the
> > > genpds) in RPM_SUSPENDED state.
> > >
> > > The PM domains (genpds) are then left in their current state, which
> > > should allow us to call dev_pm_genpd_is_on() for the corresponding
> > > supplier-devices, to figure out whether the bootloader turned them on
> > > or not, I think.
> > >
> > > Moreover, to make sure the genpds are turned on when needed, we also
> > > need to call pm_runtime_enable(platform->dev) and
> > > pm_runtime_get_sync(platform->dev). The easiest approach is probably
> > > to do that during ->probe() - and then as an improvement on top you
> > > may want to implement more fine-grained support for runtime PM.
> > >
> > > [...]
> > >
> > > Kind regards
> > > Uffe
> >
> > I did some tests here and I might be missing something. I used the
> > dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
> >
> > @@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
> > if (dev->pm_domain)
> > return 0;
> >
> > ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
> > + printk("hfranco: returned pd devs is %d", ret);
> > + for (int i = 0; i < ret; i++) {
> > + test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
> > + printk("hfranco: returned value is %d", test);
> > + }
> > return ret < 0 ? ret : 0;
> > }
> >
> > This was a quick test to check the returned value, and it always return
> > 1 for both pds, even if I did not boot the remote core.
> >
> > So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
> > it and passed NULL to dev_pm_domain_attach_list().
>
> Right, that's exactly what we should be doing.
>
> > Booting the kernel
> > now it correctly reports 0 for both pds, however when I start the
> > remote core with a hello world firmware and boot the kernel, the CPU
> > resets with a fault reset ("Reset cause: SCFW fault reset").
> >
> > I added both pm functions to probe, just to test:
> >
> > @@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
> > goto err_put_clk;
> > }
> >
> > + pm_runtime_enable(dev);
> > + pm_runtime_get_sync(dev);
> > +
>
> Indeed, calling pm_runtime_enable() and then pm_runtime_get_sync()
> should turn on the PM domains for the device, which I assume is needed
> at some point.
>
> Although, I wonder if this may be a bit too late, I would expect that
> you at least need to call these *before* the call to rproc_add(), as I
> assume the rproc-core may start using the device/driver beyond that
> point.
>
> > return 0
> >
> > Now the kernel boot with the remote core running, but it still returns
> > 0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
> > or without the remote core running.
>
> dev_pm_genpd_is_on() is returning the current status of the PM domain
> (genpd) for the device.
>
> Could it be that the genpd provider doesn't register its PM domains
> with the state that the HW is really in? pm_genpd_init() is the call
> that allows the genpd provider to specify the initial state.
>
> I think we need Peng's help here to understand what goes on.
>
> >
> > I tried to move pm_runtime_get_sync() to .prepare function but it make
> > the kernel not boot anymore (with the SCU fault reset).
>
> Try move pm_runtime_enable() before rproc_add().
Thanks Ulf, that indeed made it work, at least now the kernel does not
reset anymore with the SCU fault reset. However I am still only getting
0 from dev_pm_genpd_is_on(), no matter what the state of the remote
core. Maybe I am missing something in between?
Peng, do you know what could be the issue here?
>
> >
> > Do you have any suggestions? Am I doing something wrong with these PDs?
> >
> > Best regards,
> > Hiago.
>
> Kind regards
> Uffe
Best regards,
Hiago
On Mon, May 26, 2025 at 09:05:10PM -0300, Hiago De Franco wrote:
>On Mon, May 26, 2025 at 12:07:49PM +0200, Ulf Hansson wrote:
>> On Fri, 23 May 2025 at 21:17, Hiago De Franco <hiagofranco@gmail.com> wrote:
>> >
>> > Hi Ulf,
>> >
>> > On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
>> > > You should not provide any flag (or attach_data to
>> > > dev_pm_domain_attach_list()) at all. In other words just call
>> > > dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
>> > > drivers/remoteproc/imx_dsp_rproc.c does it.
>> > >
>> > > In this way, the device_link is created by making the platform->dev
>> > > the consumer and by keeping the supplier-devices (corresponding to the
>> > > genpds) in RPM_SUSPENDED state.
>> > >
>> > > The PM domains (genpds) are then left in their current state, which
>> > > should allow us to call dev_pm_genpd_is_on() for the corresponding
>> > > supplier-devices, to figure out whether the bootloader turned them on
>> > > or not, I think.
>> > >
>> > > Moreover, to make sure the genpds are turned on when needed, we also
>> > > need to call pm_runtime_enable(platform->dev) and
>> > > pm_runtime_get_sync(platform->dev). The easiest approach is probably
>> > > to do that during ->probe() - and then as an improvement on top you
>> > > may want to implement more fine-grained support for runtime PM.
>> > >
>> > > [...]
>> > >
>> > > Kind regards
>> > > Uffe
>> >
>> > I did some tests here and I might be missing something. I used the
>> > dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
>> >
>> > @@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
>> > if (dev->pm_domain)
>> > return 0;
>> >
>> > ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
>> > + printk("hfranco: returned pd devs is %d", ret);
>> > + for (int i = 0; i < ret; i++) {
>> > + test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
>> > + printk("hfranco: returned value is %d", test);
>> > + }
>> > return ret < 0 ? ret : 0;
>> > }
>> >
>> > This was a quick test to check the returned value, and it always return
>> > 1 for both pds, even if I did not boot the remote core.
>> >
>> > So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
>> > it and passed NULL to dev_pm_domain_attach_list().
>>
>> Right, that's exactly what we should be doing.
>>
>> > Booting the kernel
>> > now it correctly reports 0 for both pds, however when I start the
>> > remote core with a hello world firmware and boot the kernel, the CPU
>> > resets with a fault reset ("Reset cause: SCFW fault reset").
>> >
>> > I added both pm functions to probe, just to test:
>> >
>> > @@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
>> > goto err_put_clk;
>> > }
>> >
>> > + pm_runtime_enable(dev);
>> > + pm_runtime_get_sync(dev);
>> > +
>>
>> Indeed, calling pm_runtime_enable() and then pm_runtime_get_sync()
>> should turn on the PM domains for the device, which I assume is needed
>> at some point.
>>
>> Although, I wonder if this may be a bit too late, I would expect that
>> you at least need to call these *before* the call to rproc_add(), as I
>> assume the rproc-core may start using the device/driver beyond that
>> point.
>>
>> > return 0
>> >
>> > Now the kernel boot with the remote core running, but it still returns
>> > 0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
>> > or without the remote core running.
>>
>> dev_pm_genpd_is_on() is returning the current status of the PM domain
>> (genpd) for the device.
>>
>> Could it be that the genpd provider doesn't register its PM domains
>> with the state that the HW is really in? pm_genpd_init() is the call
>> that allows the genpd provider to specify the initial state.
>>
>> I think we need Peng's help here to understand what goes on.
>>
>> >
>> > I tried to move pm_runtime_get_sync() to .prepare function but it make
>> > the kernel not boot anymore (with the SCU fault reset).
>>
>> Try move pm_runtime_enable() before rproc_add().
>
>Thanks Ulf, that indeed made it work, at least now the kernel does not
>reset anymore with the SCU fault reset. However I am still only getting
>0 from dev_pm_genpd_is_on(), no matter what the state of the remote
>core. Maybe I am missing something in between?
>
>Peng, do you know what could be the issue here?
imx_rproc_attach_pd
->dev_pm_domain_attach_list
->genpd_dev_pm_attach_by_id
->genpd_queue_power_off_work
->cm40_pid0 is powered off because the genpd is set with is_off=false
So dev_pm_genpd_is_on will return false after attach.
This means that with U-Boot kick M4, cm40 might be powered off when
attaching the pd even with LINK_ON set, because genpd is set with is_off=false.
The reason we set genpd to match real hardware status is to avoid RPC call
and to save power. But seems it could not work well with U-boot kicking M4.
I not have good idea on how to address this issue. The current driver
could work with linux kick M4, M4 packed in flash.bin and M4 in a standalone
partition.
Regards,
Peng
>
>>
>> >
>> > Do you have any suggestions? Am I doing something wrong with these PDs?
>> >
>> > Best regards,
>> > Hiago.
>>
>> Kind regards
>> Uffe
>
>Best regards,
>Hiago
On Tue, 27 May 2025 at 03:29, Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> On Mon, May 26, 2025 at 09:05:10PM -0300, Hiago De Franco wrote:
> >On Mon, May 26, 2025 at 12:07:49PM +0200, Ulf Hansson wrote:
> >> On Fri, 23 May 2025 at 21:17, Hiago De Franco <hiagofranco@gmail.com> wrote:
> >> >
> >> > Hi Ulf,
> >> >
> >> > On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
> >> > > You should not provide any flag (or attach_data to
> >> > > dev_pm_domain_attach_list()) at all. In other words just call
> >> > > dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
> >> > > drivers/remoteproc/imx_dsp_rproc.c does it.
> >> > >
> >> > > In this way, the device_link is created by making the platform->dev
> >> > > the consumer and by keeping the supplier-devices (corresponding to the
> >> > > genpds) in RPM_SUSPENDED state.
> >> > >
> >> > > The PM domains (genpds) are then left in their current state, which
> >> > > should allow us to call dev_pm_genpd_is_on() for the corresponding
> >> > > supplier-devices, to figure out whether the bootloader turned them on
> >> > > or not, I think.
> >> > >
> >> > > Moreover, to make sure the genpds are turned on when needed, we also
> >> > > need to call pm_runtime_enable(platform->dev) and
> >> > > pm_runtime_get_sync(platform->dev). The easiest approach is probably
> >> > > to do that during ->probe() - and then as an improvement on top you
> >> > > may want to implement more fine-grained support for runtime PM.
> >> > >
> >> > > [...]
> >> > >
> >> > > Kind regards
> >> > > Uffe
> >> >
> >> > I did some tests here and I might be missing something. I used the
> >> > dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
> >> >
> >> > @@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
> >> > if (dev->pm_domain)
> >> > return 0;
> >> >
> >> > ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
> >> > + printk("hfranco: returned pd devs is %d", ret);
> >> > + for (int i = 0; i < ret; i++) {
> >> > + test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
> >> > + printk("hfranco: returned value is %d", test);
> >> > + }
> >> > return ret < 0 ? ret : 0;
> >> > }
> >> >
> >> > This was a quick test to check the returned value, and it always return
> >> > 1 for both pds, even if I did not boot the remote core.
> >> >
> >> > So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
> >> > it and passed NULL to dev_pm_domain_attach_list().
> >>
> >> Right, that's exactly what we should be doing.
> >>
> >> > Booting the kernel
> >> > now it correctly reports 0 for both pds, however when I start the
> >> > remote core with a hello world firmware and boot the kernel, the CPU
> >> > resets with a fault reset ("Reset cause: SCFW fault reset").
> >> >
> >> > I added both pm functions to probe, just to test:
> >> >
> >> > @@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
> >> > goto err_put_clk;
> >> > }
> >> >
> >> > + pm_runtime_enable(dev);
> >> > + pm_runtime_get_sync(dev);
> >> > +
> >>
> >> Indeed, calling pm_runtime_enable() and then pm_runtime_get_sync()
> >> should turn on the PM domains for the device, which I assume is needed
> >> at some point.
> >>
> >> Although, I wonder if this may be a bit too late, I would expect that
> >> you at least need to call these *before* the call to rproc_add(), as I
> >> assume the rproc-core may start using the device/driver beyond that
> >> point.
> >>
> >> > return 0
> >> >
> >> > Now the kernel boot with the remote core running, but it still returns
> >> > 0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
> >> > or without the remote core running.
> >>
> >> dev_pm_genpd_is_on() is returning the current status of the PM domain
> >> (genpd) for the device.
> >>
> >> Could it be that the genpd provider doesn't register its PM domains
> >> with the state that the HW is really in? pm_genpd_init() is the call
> >> that allows the genpd provider to specify the initial state.
> >>
> >> I think we need Peng's help here to understand what goes on.
> >>
> >> >
> >> > I tried to move pm_runtime_get_sync() to .prepare function but it make
> >> > the kernel not boot anymore (with the SCU fault reset).
> >>
> >> Try move pm_runtime_enable() before rproc_add().
> >
> >Thanks Ulf, that indeed made it work, at least now the kernel does not
> >reset anymore with the SCU fault reset. However I am still only getting
> >0 from dev_pm_genpd_is_on(), no matter what the state of the remote
> >core. Maybe I am missing something in between?
> >
> >Peng, do you know what could be the issue here?
>
> imx_rproc_attach_pd
> ->dev_pm_domain_attach_list
> ->genpd_dev_pm_attach_by_id
> ->genpd_queue_power_off_work
> ->cm40_pid0 is powered off because the genpd is set with is_off=false
>
> So dev_pm_genpd_is_on will return false after attach.
>
> This means that with U-Boot kick M4, cm40 might be powered off when
> attaching the pd even with LINK_ON set, because genpd is set with is_off=false.
>
> The reason we set genpd to match real hardware status is to avoid RPC call
> and to save power. But seems it could not work well with U-boot kicking M4.
>
> I not have good idea on how to address this issue. The current driver
> could work with linux kick M4, M4 packed in flash.bin and M4 in a standalone
> partition.
Thanks for the detailed analysis!
This is a very similar issue as many other genpd providers are
suffering from - and something that I have been working on recently to
fix.
A few days ago I posted a new version of a series [1], which is based
upon using the fw_devlink and ->sync_state() support. In principle, we
need to prevent genpd from power-off a PM domain if it was powered-on
during boot , until all the consumer-drivers of a PM domain have been
probed.
I had a look at the DT description of how imx describes power-domain
providers/consumers, along with the corresponding genpd provider
implementation in drivers/pmdomain/imx/scu-pd.c. Unless I missed
something, I think [1] should do the trick for you, without any
further changes. Can you please give it a try and see if that solves
this problem?
[...]
Kind regards
Uffe
[1]
https://lore.kernel.org/all/20250523134025.75130-1-ulf.hansson@linaro.org/
On Tue, May 27, 2025 at 01:58:46PM +0200, Ulf Hansson wrote:
> On Tue, 27 May 2025 at 03:29, Peng Fan <peng.fan@oss.nxp.com> wrote:
> >
> > On Mon, May 26, 2025 at 09:05:10PM -0300, Hiago De Franco wrote:
> > >On Mon, May 26, 2025 at 12:07:49PM +0200, Ulf Hansson wrote:
> > >> On Fri, 23 May 2025 at 21:17, Hiago De Franco <hiagofranco@gmail.com> wrote:
> > >> >
> > >> > Hi Ulf,
> > >> >
> > >> > On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
> > >> > > You should not provide any flag (or attach_data to
> > >> > > dev_pm_domain_attach_list()) at all. In other words just call
> > >> > > dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
> > >> > > drivers/remoteproc/imx_dsp_rproc.c does it.
> > >> > >
> > >> > > In this way, the device_link is created by making the platform->dev
> > >> > > the consumer and by keeping the supplier-devices (corresponding to the
> > >> > > genpds) in RPM_SUSPENDED state.
> > >> > >
> > >> > > The PM domains (genpds) are then left in their current state, which
> > >> > > should allow us to call dev_pm_genpd_is_on() for the corresponding
> > >> > > supplier-devices, to figure out whether the bootloader turned them on
> > >> > > or not, I think.
> > >> > >
> > >> > > Moreover, to make sure the genpds are turned on when needed, we also
> > >> > > need to call pm_runtime_enable(platform->dev) and
> > >> > > pm_runtime_get_sync(platform->dev). The easiest approach is probably
> > >> > > to do that during ->probe() - and then as an improvement on top you
> > >> > > may want to implement more fine-grained support for runtime PM.
> > >> > >
> > >> > > [...]
> > >> > >
> > >> > > Kind regards
> > >> > > Uffe
> > >> >
> > >> > I did some tests here and I might be missing something. I used the
> > >> > dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
> > >> >
> > >> > @@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
> > >> > if (dev->pm_domain)
> > >> > return 0;
> > >> >
> > >> > ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
> > >> > + printk("hfranco: returned pd devs is %d", ret);
> > >> > + for (int i = 0; i < ret; i++) {
> > >> > + test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
> > >> > + printk("hfranco: returned value is %d", test);
> > >> > + }
> > >> > return ret < 0 ? ret : 0;
> > >> > }
> > >> >
> > >> > This was a quick test to check the returned value, and it always return
> > >> > 1 for both pds, even if I did not boot the remote core.
> > >> >
> > >> > So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
> > >> > it and passed NULL to dev_pm_domain_attach_list().
> > >>
> > >> Right, that's exactly what we should be doing.
> > >>
> > >> > Booting the kernel
> > >> > now it correctly reports 0 for both pds, however when I start the
> > >> > remote core with a hello world firmware and boot the kernel, the CPU
> > >> > resets with a fault reset ("Reset cause: SCFW fault reset").
> > >> >
> > >> > I added both pm functions to probe, just to test:
> > >> >
> > >> > @@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
> > >> > goto err_put_clk;
> > >> > }
> > >> >
> > >> > + pm_runtime_enable(dev);
> > >> > + pm_runtime_get_sync(dev);
> > >> > +
> > >>
> > >> Indeed, calling pm_runtime_enable() and then pm_runtime_get_sync()
> > >> should turn on the PM domains for the device, which I assume is needed
> > >> at some point.
> > >>
> > >> Although, I wonder if this may be a bit too late, I would expect that
> > >> you at least need to call these *before* the call to rproc_add(), as I
> > >> assume the rproc-core may start using the device/driver beyond that
> > >> point.
> > >>
> > >> > return 0
> > >> >
> > >> > Now the kernel boot with the remote core running, but it still returns
> > >> > 0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
> > >> > or without the remote core running.
> > >>
> > >> dev_pm_genpd_is_on() is returning the current status of the PM domain
> > >> (genpd) for the device.
> > >>
> > >> Could it be that the genpd provider doesn't register its PM domains
> > >> with the state that the HW is really in? pm_genpd_init() is the call
> > >> that allows the genpd provider to specify the initial state.
> > >>
> > >> I think we need Peng's help here to understand what goes on.
> > >>
> > >> >
> > >> > I tried to move pm_runtime_get_sync() to .prepare function but it make
> > >> > the kernel not boot anymore (with the SCU fault reset).
> > >>
> > >> Try move pm_runtime_enable() before rproc_add().
> > >
> > >Thanks Ulf, that indeed made it work, at least now the kernel does not
> > >reset anymore with the SCU fault reset. However I am still only getting
> > >0 from dev_pm_genpd_is_on(), no matter what the state of the remote
> > >core. Maybe I am missing something in between?
> > >
> > >Peng, do you know what could be the issue here?
> >
> > imx_rproc_attach_pd
> > ->dev_pm_domain_attach_list
> > ->genpd_dev_pm_attach_by_id
> > ->genpd_queue_power_off_work
> > ->cm40_pid0 is powered off because the genpd is set with is_off=false
> >
> > So dev_pm_genpd_is_on will return false after attach.
> >
> > This means that with U-Boot kick M4, cm40 might be powered off when
> > attaching the pd even with LINK_ON set, because genpd is set with is_off=false.
> >
> > The reason we set genpd to match real hardware status is to avoid RPC call
> > and to save power. But seems it could not work well with U-boot kicking M4.
> >
> > I not have good idea on how to address this issue. The current driver
> > could work with linux kick M4, M4 packed in flash.bin and M4 in a standalone
> > partition.
>
> Thanks for the detailed analysis!
>
> This is a very similar issue as many other genpd providers are
> suffering from - and something that I have been working on recently to
> fix.
>
> A few days ago I posted a new version of a series [1], which is based
> upon using the fw_devlink and ->sync_state() support. In principle, we
> need to prevent genpd from power-off a PM domain if it was powered-on
> during boot , until all the consumer-drivers of a PM domain have been
> probed.
>
> I had a look at the DT description of how imx describes power-domain
> providers/consumers, along with the corresponding genpd provider
> implementation in drivers/pmdomain/imx/scu-pd.c. Unless I missed
> something, I think [1] should do the trick for you, without any
> further changes. Can you please give it a try and see if that solves
> this problem?
Cool! I can give a try and provide an answer soon. Thanks!
>
> [...]
>
> Kind regards
> Uffe
>
> [1]
> https://lore.kernel.org/all/20250523134025.75130-1-ulf.hansson@linaro.org/
Best regards,
Hiago
On Tue, May 27, 2025 at 10:45:25AM -0300, Hiago De Franco wrote:
[...]
> >
> > Thanks for the detailed analysis!
> >
> > This is a very similar issue as many other genpd providers are
> > suffering from - and something that I have been working on recently to
> > fix.
> >
> > A few days ago I posted a new version of a series [1], which is based
> > upon using the fw_devlink and ->sync_state() support. In principle, we
> > need to prevent genpd from power-off a PM domain if it was powered-on
> > during boot , until all the consumer-drivers of a PM domain have been
> > probed.
> >
> > I had a look at the DT description of how imx describes power-domain
> > providers/consumers, along with the corresponding genpd provider
> > implementation in drivers/pmdomain/imx/scu-pd.c. Unless I missed
> > something, I think [1] should do the trick for you, without any
> > further changes. Can you please give it a try and see if that solves
> > this problem?
>
> Cool! I can give a try and provide an answer soon. Thanks!
We are making progress ;-)
With the patches you shared Ulf (I added them on top of the current
master branch), it works as expected, dev_pm_genpd_is_on() returns 0
when I boot the kernel without M4 running and it returns 1 when I boot
the kernel with M4 running with a hello-world demo.
However now I tried to, if dev_pm_genpd_is_on() returns 1, put the
DETACHED state, something as
if (dev_pm_genpd_is_on(priv->pd_list->pd_devs[0]))
priv->rproc->state = RPROC_DETACHED;
In this case I used 0 because I understand this is the
IMX_SC_R_M4_0_PID0 defined in my device tree overlay:
power-domains = <&pd IMX_SC_R_M4_0_PID0>,
<&pd IMX_SC_R_M4_0_MU_1A>;
But in this case, the kernel does not boot anymore, I see the "Starting
kernel..." and nothing else.
I am using the pm_runtime functions before rproc_add():
@@ -1146,6 +1154,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
}
}
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+
ret = rproc_add(rproc);
if (ret) {
dev_err(dev, "rproc_add failed\n");
and calling dev_pm_genpd_is_on() after dev_pm_domain_attach_list() has
been called.
With kernel not starting I cannot even debug it. Do you have any
suggestion on why this might be happening?
>
> >
> > [...]
> >
> > Kind regards
> > Uffe
> >
> > [1]
> > https://lore.kernel.org/all/20250523134025.75130-1-ulf.hansson@linaro.org/
>
> Best regards,
> Hiago
Best regards,
Hiago
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@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@gmail.com> wrote:
>> > > >
>> > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
>> > > > Suggested-by: Peng Fan <peng.fan@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.
On Mon, 12 May 2025 at 05:46, Peng Fan <peng.fan@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 <hiagofranco@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@gmail.com> wrote:
> >> > > >
> >> > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> >> > > > Suggested-by: Peng Fan <peng.fan@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
On Mon, 19 May 2025 at 16:39, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 12 May 2025 at 05:46, Peng Fan <peng.fan@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 <hiagofranco@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@gmail.com> wrote:
> > >> > > >
> > >> > > > From: Hiago De Franco <hiago.franco@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@toradex.com>
> > >> > > > Suggested-by: Peng Fan <peng.fan@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?
Aha, PD_FLAG_DEV_LINK_ON is being used when you attach to the PM
domains. I would re-work things in the driver (if needed) so you can
avoid using this flag, then the PM domains should stay power-off until
there is a call to pm_runtime_get_sync().
[...]
Kind regards
Uffe
Hi Peng, On Mon, May 12, 2025 at 12:56:13PM +0800, Peng Fan wrote: > > 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 This patch series is already implementing approach one, as I understand, right? The difference is I moved this API from drivers/pmdomain/imx/scu-pd.c to firmware/imx/misc.c, and exported it there. But you think I should create this new file firmware/imx/power.c and move the API from scu-pd.c to power.c, is my understanding correct? > > Regards, > Peng Best Regards, Hiago.
© 2016 - 2026 Red Hat, Inc.