| 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.
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 Thu, May 29, 2025 at 03:54:47AM +0000, Peng Fan wrote: [...] > > 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. > > Please add "earlycon" in bootargs to see where it hangs. Thanks Peng! I was able to catch the kernel panic yesterday, however I must say that today I was doing the tests again and the issue is gone. Sorry, I might have done something wrong yesterday with the tests. Anyway, here is the log: [ 1.271163] remoteproc remoteproc0: imx-rproc is available [ 1.280296] remoteproc remoteproc0: attaching to imx-rproc [ 1.285756] Unable to handle kernel paging request at virtual address ffff80005ae3dd79 [ 1.293624] Mem abort info: [ 1.294655] mmc0: SDHCI controller on 5b010000.mmc [5b010000.mmc] using ADMA [ 1.296386] ESR = 0x0000000096000005 [ 1.307194] EC = 0x25: DABT (current EL), IL = 32 bits [ 1.312473] SET = 0, FnV = 0 [ 1.315566] EA = 0, S1PTW = 0 [ 1.318649] FSC = 0x05: level 1 translation fault [ 1.323510] Data abort info: [ 1.326370] ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 [ 1.331846] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 1.336882] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 1.342182] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000096bc1000 [ 1.348870] [ffff80005ae3dd79] pgd=0000000000000000, p4d=1000000097054003, pud=0000000000000000 [ 1.357565] Internal error: Oops: 0000000096000005 [#1] SMP [ 1.363198] Modules linked in: [ 1.366236] CPU: 2 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.15.0-03667-g3f5f09105c40-dirty #826 PREEMPT [ 1.376405] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT) [ 1.384313] Workqueue: events_unbound deferred_probe_work_func [ 1.390128] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 1.397076] pc : rproc_handle_resources.constprop.0+0x78/0x1d0 [ 1.402896] lr : rproc_boot+0x368/0x56c [ 1.406717] sp : ffff8000819c3990 [ 1.410017] x29: ffff8000819c3990 x28: ffff80005ae3dd7d x27: 0000000000000000 [ 1.417145] x26: 0000000000000000 x25: ffff0000015ec038 x24: ffff800080f0c0a8 [ 1.424268] x23: ffff8000813a6110 x22: 00000000d999ad79 x21: ffff0000015ec000 [ 1.431392] x20: 0000000026665683 x19: ffff80005ae3dd79 x18: 0000000000000006 [ 1.438516] x17: ffff000001799400 x16: ffff000001798e00 x15: 4addd15cca11c529 [ 1.445639] x14: 53ebce6d5564d787 x13: 4addd15cca11c529 x12: 53ebce6d5564d787 [ 1.452763] x11: 95a1e33b6b190674 x10: 9e3c9abdb41ca345 x9 : ab17b4eaffd6fd1c [ 1.459887] x8 : d5da055de4cfbb87 x7 : dfd7fa31596acbbc x6 : 9946d97107d0dcca [ 1.467011] x5 : ffff0000010c7800 x4 : 00000000000003fc x3 : ffff0000010c7780 [ 1.474134] x2 : fffffffffffffff0 x1 : ffff8000814a3000 x0 : ffff8000814a3000 [ 1.481261] Call trace: [ 1.483690] rproc_handle_resources.constprop.0+0x78/0x1d0 (P) [ 1.487705] mmc0: new HS400 MMC card at address 0001 [ 1.489502] rproc_boot+0x368/0x56c [ 1.495349] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB [ 1.497929] rproc_add+0x184/0x190 [ 1.504356] mmcblk0: p1 p2 [ 1.505747] imx_rproc_probe+0x458/0x528 [ 1.509238] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB [ 1.512437] platform_probe+0x68/0xc0 [ 1.512452] really_probe+0xc0/0x38c [ 1.520584] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB [ 1.520951] __driver_probe_device+0x7c/0x15c [ 1.527522] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (242:0) [ 1.529377] driver_probe_device+0x3c/0x10c [ 1.544263] __device_attach_driver+0xbc/0x158 [ 1.548586] bus_for_each_drv+0x84/0xe0 [ 1.552407] __device_attach+0x9c/0x1ac [ 1.556231] device_initial_probe+0x14/0x20 [ 1.560401] bus_probe_device+0xac/0xb0 [ 1.564221] deferred_probe_work_func+0x9c/0xec [ 1.568741] process_one_work+0x14c/0x28c [ 1.572735] worker_thread+0x2cc/0x3d4 [ 1.576473] kthread+0x12c/0x208 [ 1.579687] ret_from_fork+0x10/0x20 [ 1.583253] Code: 8b36c033 9100127c 54000924 d503201f (b9400261) [ 1.589337] ---[ end trace 0000000000000000 ]--- But again, the issue is not happening anymore ;-) I will keep testing to see if the issue happens again, but for now is working fine, I can now attach to the remote processor. This is the git diff on top of Ulf patches I have been testing: diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c index 6da25e2c81d2..661a6aad40a8 100644 --- a/drivers/pmdomain/core.c +++ b/drivers/pmdomain/core.c @@ -599,6 +599,23 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) } EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); +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; +} +EXPORT_SYMBOL_GPL(dev_pm_genpd_is_on); + /** * dev_pm_genpd_set_next_wakeup - Notify PM framework of an impending wakeup. * diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c index 627e57a88db2..9688370f9bb5 100644 --- a/drivers/remoteproc/imx_rproc.c +++ b/drivers/remoteproc/imx_rproc.c @@ -18,6 +18,7 @@ #include <linux/of_reserved_mem.h> #include <linux/platform_device.h> #include <linux/pm_domain.h> +#include <linux/pm_runtime.h> #include <linux/reboot.h> #include <linux/regmap.h> #include <linux/remoteproc.h> @@ -891,9 +892,7 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv) { struct device *dev = priv->dev; int ret; - struct dev_pm_domain_attach_data pd_data = { - .pd_flags = PD_FLAG_DEV_LINK_ON, - }; + bool test; /* * If there is only one power-domain entry, the platform driver framework @@ -902,7 +901,16 @@ 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); + ret = dev_pm_domain_attach_list(dev, NULL, &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); + if (test) { + priv->rproc->state = RPROC_DETACHED; + break; + } + } return ret < 0 ? ret : 0; } @@ -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"); diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 756b842dcd30..16d1fca2a8c5 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -95,6 +95,7 @@ extern void pm_runtime_put_suppliers(struct device *dev); extern void pm_runtime_new_link(struct device *dev); extern void pm_runtime_drop_link(struct device_link *link); extern void pm_runtime_release_supplier(struct device_link *link); +bool dev_pm_genpd_is_on(struct device *dev); int devm_pm_runtime_set_active_enabled(struct device *dev); extern int devm_pm_runtime_enable(struct device *dev); This is the rproc output when bootaux is used: root@colibri-imx8x-07308754:~# dmesg | grep hfranco [ 0.478475] hfranco: returned pd devs is 2 [ 0.478496] hfranco: returned value is 1 root@colibri-imx8x-07308754:~# dmesg | grep rproc [ 0.478797] remoteproc remoteproc0: imx-rproc is available [ 0.478878] remoteproc remoteproc0: attaching to imx-rproc [ 0.478961] remoteproc remoteproc0: remote processor imx-rproc is now attached I will cleanup everything and try to come up with a patch. Ulf, in this case, as your patches have not yet been merged, should I wait for them? Thanks for all the help guys. > > > > > 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); > > I think only make this apply for i.MX8QX/8QM/DX, then no > impact to other patforms. All right, I will check how to do this. Thanks. > > Regards, > Peng Best regards, Hiago.
On Thu, 29 May 2025 at 22:15, Hiago De Franco <hiagofranco@gmail.com> wrote: > > On Thu, May 29, 2025 at 03:54:47AM +0000, Peng Fan wrote: > > [...] > > > > 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. > > > > Please add "earlycon" in bootargs to see where it hangs. > > Thanks Peng! I was able to catch the kernel panic yesterday, however I > must say that today I was doing the tests again and the issue is gone. > Sorry, I might have done something wrong yesterday with the tests. > Anyway, here is the log: > > [ 1.271163] remoteproc remoteproc0: imx-rproc is available > [ 1.280296] remoteproc remoteproc0: attaching to imx-rproc > [ 1.285756] Unable to handle kernel paging request at virtual address ffff80005ae3dd79 > [ 1.293624] Mem abort info: > [ 1.294655] mmc0: SDHCI controller on 5b010000.mmc [5b010000.mmc] using ADMA > [ 1.296386] ESR = 0x0000000096000005 > [ 1.307194] EC = 0x25: DABT (current EL), IL = 32 bits > [ 1.312473] SET = 0, FnV = 0 > [ 1.315566] EA = 0, S1PTW = 0 > [ 1.318649] FSC = 0x05: level 1 translation fault > [ 1.323510] Data abort info: > [ 1.326370] ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 > [ 1.331846] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > [ 1.336882] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > [ 1.342182] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000096bc1000 > [ 1.348870] [ffff80005ae3dd79] pgd=0000000000000000, p4d=1000000097054003, pud=0000000000000000 > [ 1.357565] Internal error: Oops: 0000000096000005 [#1] SMP > [ 1.363198] Modules linked in: > [ 1.366236] CPU: 2 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.15.0-03667-g3f5f09105c40-dirty #826 PREEMPT > [ 1.376405] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT) > [ 1.384313] Workqueue: events_unbound deferred_probe_work_func > [ 1.390128] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 1.397076] pc : rproc_handle_resources.constprop.0+0x78/0x1d0 > [ 1.402896] lr : rproc_boot+0x368/0x56c > [ 1.406717] sp : ffff8000819c3990 > [ 1.410017] x29: ffff8000819c3990 x28: ffff80005ae3dd7d x27: 0000000000000000 > [ 1.417145] x26: 0000000000000000 x25: ffff0000015ec038 x24: ffff800080f0c0a8 > [ 1.424268] x23: ffff8000813a6110 x22: 00000000d999ad79 x21: ffff0000015ec000 > [ 1.431392] x20: 0000000026665683 x19: ffff80005ae3dd79 x18: 0000000000000006 > [ 1.438516] x17: ffff000001799400 x16: ffff000001798e00 x15: 4addd15cca11c529 > [ 1.445639] x14: 53ebce6d5564d787 x13: 4addd15cca11c529 x12: 53ebce6d5564d787 > [ 1.452763] x11: 95a1e33b6b190674 x10: 9e3c9abdb41ca345 x9 : ab17b4eaffd6fd1c > [ 1.459887] x8 : d5da055de4cfbb87 x7 : dfd7fa31596acbbc x6 : 9946d97107d0dcca > [ 1.467011] x5 : ffff0000010c7800 x4 : 00000000000003fc x3 : ffff0000010c7780 > [ 1.474134] x2 : fffffffffffffff0 x1 : ffff8000814a3000 x0 : ffff8000814a3000 > [ 1.481261] Call trace: > [ 1.483690] rproc_handle_resources.constprop.0+0x78/0x1d0 (P) > [ 1.487705] mmc0: new HS400 MMC card at address 0001 > [ 1.489502] rproc_boot+0x368/0x56c > [ 1.495349] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB > [ 1.497929] rproc_add+0x184/0x190 > [ 1.504356] mmcblk0: p1 p2 > [ 1.505747] imx_rproc_probe+0x458/0x528 > [ 1.509238] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB > [ 1.512437] platform_probe+0x68/0xc0 > [ 1.512452] really_probe+0xc0/0x38c > [ 1.520584] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB > [ 1.520951] __driver_probe_device+0x7c/0x15c > [ 1.527522] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (242:0) > [ 1.529377] driver_probe_device+0x3c/0x10c > [ 1.544263] __device_attach_driver+0xbc/0x158 > [ 1.548586] bus_for_each_drv+0x84/0xe0 > [ 1.552407] __device_attach+0x9c/0x1ac > [ 1.556231] device_initial_probe+0x14/0x20 > [ 1.560401] bus_probe_device+0xac/0xb0 > [ 1.564221] deferred_probe_work_func+0x9c/0xec > [ 1.568741] process_one_work+0x14c/0x28c > [ 1.572735] worker_thread+0x2cc/0x3d4 > [ 1.576473] kthread+0x12c/0x208 > [ 1.579687] ret_from_fork+0x10/0x20 > [ 1.583253] Code: 8b36c033 9100127c 54000924 d503201f (b9400261) > [ 1.589337] ---[ end trace 0000000000000000 ]--- > > But again, the issue is not happening anymore ;-) I will keep testing to > see if the issue happens again, but for now is working fine, I can now > attach to the remote processor. > > This is the git diff on top of Ulf patches I have been testing: > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > index 6da25e2c81d2..661a6aad40a8 100644 > --- a/drivers/pmdomain/core.c > +++ b/drivers/pmdomain/core.c > @@ -599,6 +599,23 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) > } > EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); > > +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; > +} > +EXPORT_SYMBOL_GPL(dev_pm_genpd_is_on); > + > /** > * dev_pm_genpd_set_next_wakeup - Notify PM framework of an impending wakeup. > * > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c > index 627e57a88db2..9688370f9bb5 100644 > --- a/drivers/remoteproc/imx_rproc.c > +++ b/drivers/remoteproc/imx_rproc.c > @@ -18,6 +18,7 @@ > #include <linux/of_reserved_mem.h> > #include <linux/platform_device.h> > #include <linux/pm_domain.h> > +#include <linux/pm_runtime.h> > #include <linux/reboot.h> > #include <linux/regmap.h> > #include <linux/remoteproc.h> > @@ -891,9 +892,7 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv) > { > struct device *dev = priv->dev; > int ret; > - struct dev_pm_domain_attach_data pd_data = { > - .pd_flags = PD_FLAG_DEV_LINK_ON, > - }; > + bool test; > > /* > * If there is only one power-domain entry, the platform driver framework > @@ -902,7 +901,16 @@ 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); > + ret = dev_pm_domain_attach_list(dev, NULL, &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); > + if (test) { > + priv->rproc->state = RPROC_DETACHED; > + break; > + } > + } > return ret < 0 ? ret : 0; > } > > @@ -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"); > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 756b842dcd30..16d1fca2a8c5 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -95,6 +95,7 @@ extern void pm_runtime_put_suppliers(struct device *dev); > extern void pm_runtime_new_link(struct device *dev); > extern void pm_runtime_drop_link(struct device_link *link); > extern void pm_runtime_release_supplier(struct device_link *link); > +bool dev_pm_genpd_is_on(struct device *dev); > > int devm_pm_runtime_set_active_enabled(struct device *dev); > extern int devm_pm_runtime_enable(struct device *dev); > > This is the rproc output when bootaux is used: > > root@colibri-imx8x-07308754:~# dmesg | grep hfranco > [ 0.478475] hfranco: returned pd devs is 2 > [ 0.478496] hfranco: returned value is 1 > root@colibri-imx8x-07308754:~# dmesg | grep rproc > [ 0.478797] remoteproc remoteproc0: imx-rproc is available > [ 0.478878] remoteproc remoteproc0: attaching to imx-rproc > [ 0.478961] remoteproc remoteproc0: remote processor imx-rproc is now attached > > I will cleanup everything and try to come up with a patch. Ulf, in this > case, as your patches have not yet been merged, should I wait for them? I think you can state in the cover-letter that your series depends on mine, so please go ahead and submit them. > > Thanks for all the help guys. > > > > > > > > > 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); > > > > I think only make this apply for i.MX8QX/8QM/DX, then no > > impact to other patforms. In general I think we should avoid such quirks in drivers, unless it's really needed. Just wanted to share my opinion, but it's totally up to you. [...] Kind regards Uffe
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); }