| Message ID | 20250507160056.11876-4-hiagofranco@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] firmware: imx: move get power mode function from scu-pd.c to misc.c | expand |
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 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.
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); }