Message-ID: <20250527134525.f7yzs4ww64xxmjmr@hiago-nb>
Date: Tue, 27 May 2025 10:45:25 -0300
From: Hiago De Franco <hiagofranco@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Peng Fan <peng.fan@....nxp.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

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

Powered by blists - more mailing lists