| 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.
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
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.
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); }