Message-ID: <20250521041306.GA28017@nxa18884-linux>
Date: Wed, 21 May 2025 12:13:06 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Hiago De Franco <hiagofranco@...il.com>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	linux-pm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Bjorn Andersson <andersson@...nel.org>,
	Hiago De Franco <hiago.franco@...adex.com>, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	daniel.baluta@....com, iuliana.prodan@....nxp.com,
	Fabio Estevam <festevam@...il.com>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for
 remote core attachment

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@...il.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@...il.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@...il.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@...il.com> wrote:
>> > > > > > >
>> > > > > > > From: Hiago De Franco <hiago.franco@...adex.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@...adex.com>
>> > > > > > > Suggested-by: Peng Fan <peng.fan@....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

Powered by blists - more mailing lists