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 <hiagofra...@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 <hiagofra...@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 <hiagofra...@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
>>> > > > > > <hiagofra...@gmail.com> wrote:
>>> > > > > > >
>>> > > > > > > From: Hiago De Franco <hiago.fra...@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.fra...@toradex.com>
>>> > > > > > > Suggested-by: Peng Fan <peng....@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-hiagofra...@gmail.com/T/#t),
>>> but I am a bit confused which path we should take, the initial approach
>>> proposed or using these PD functions. Maybe we can discuss this in the
>>> new v3 patch series?
>>
>>I think it would be better if we can avoid sharing low-level firmware
>>functions for PM domains. I am worried that they may become abused for
>>other future use-cases.
>>
>>So, if possible, I would rather make us try to use
>>dev_pm_genpd_is_on() (or something along those lines), but let's see
>>what Peng thinks about it before we make the decision.
>
>There are two power domains for this m4:
>power-domains = <&pd IMX_SC_R_M4_0_PID0>, <&pd IMX_SC_R_M4_0_MU_1A>;
>
>So before attach the pd, dev_pm_genpd_is_on should also return false
>per my understanding. If run dev_pm_genpd_is_on after attaching the pd,
>the pd will be powered on. So we are not able to know whether M4 is started
>by bootloader or not.
Could we use PD_FLAG_NO_DEV_LINK when attach the PD, then
use dev_pm_genpd_is_on to detect the status of genpd?
we set is_off as true when pm_genpd_init if the PD is physical ON.
Thanks,
Peng
>
>Hiago's case needs the real power status before attaching the
>pd to set remoteproc as DETACHED(M4 kicked by bootloader) or OFFLINE(
>M4 not kicked by bootloader) state.
>
>Seems there is no other SCFW API to check whether M4 is started by
>bootloader.
>
>I not have good idea as of now except directly checking the real
>power status to indicate M4 started by bootloader or not. Or using a
>device tree property runtime added by bootloader,
>saying "fsl,rproc-started"?
>
>
>Thanks,
>Peng
>
>>
>>[...]
>>
>>Kind regards
>>Uffe
>
