Message-ID: <20250527023921.GA14252@nxa18884-linux>
Date: Tue, 27 May 2025 10:39:21 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Hiago De Franco <hiagofranco@...il.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>,
	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

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@...il.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

Powered by blists - more mailing lists