Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment

[Hiago De Franco <hiagofranco@gmail.com>]

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