| Message ID | 20250507160056.11876-4-hiagofranco@gmail.com |
|---|---|
| State | Superseded |
| 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.
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
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 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, 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.
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); }