Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Thu, 29 May 2025 at 22:15, Hiago De Franco wrote: > > On Thu, May 29, 2025 at 03:54:47AM +, Peng Fan wrote: > > [...] > > > > We are making progress ;-) > > > > > > With the patches you shared Ulf (I added them on top of the current > > > master branch), it works as expected, dev_pm_genpd_is_on() returns 0 > > > when I boot the kernel without M4 running and it returns 1 when I > > > boot the kernel with M4 running with a hello-world demo. > > > > > > However now I tried to, if dev_pm_genpd_is_on() returns 1, put the > > > DETACHED state, something as > > > > > > if (dev_pm_genpd_is_on(priv->pd_list->pd_devs[0])) > > > priv->rproc->state = RPROC_DETACHED; > > > > > > In this case I used 0 because I understand this is the > > > IMX_SC_R_M4_0_PID0 defined in my device tree overlay: > > > > > > power-domains = <&pd IMX_SC_R_M4_0_PID0>, > > > <&pd IMX_SC_R_M4_0_MU_1A>; > > > > > > But in this case, the kernel does not boot anymore, I see the "Starting > > > kernel..." and nothing else. > > > > Please add "earlycon" in bootargs to see where it hangs. > > Thanks Peng! I was able to catch the kernel panic yesterday, however I > must say that today I was doing the tests again and the issue is gone. > Sorry, I might have done something wrong yesterday with the tests. > Anyway, here is the log: > > [1.271163] remoteproc remoteproc0: imx-rproc is available > [1.280296] remoteproc remoteproc0: attaching to imx-rproc > [1.285756] Unable to handle kernel paging request at virtual address > 80005ae3dd79 > [1.293624] Mem abort info: > [1.294655] mmc0: SDHCI controller on 5b01.mmc [5b01.mmc] using > ADMA > [1.296386] ESR = 0x9605 > [1.307194] EC = 0x25: DABT (current EL), IL = 32 bits > [1.312473] SET = 0, FnV = 0 > [1.315566] EA = 0, S1PTW = 0 > [1.318649] FSC = 0x05: level 1 translation fault > [1.323510] Data abort info: > [1.326370] ISV = 0, ISS = 0x0005, ISS2 = 0x > [1.331846] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > [1.336882] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > [1.342182] swapper pgtable: 4k pages, 48-bit VAs, pgdp=96bc1000 > [1.348870] [80005ae3dd79] pgd=, p4d=100097054003, > pud= > [1.357565] Internal error: Oops: 9605 [#1] SMP > [1.363198] Modules linked in: > [1.366236] CPU: 2 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted > 6.15.0-03667-g3f5f09105c40-dirty #826 PREEMPT > [1.376405] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation > Board V3 (DT) > [1.384313] Workqueue: events_unbound deferred_probe_work_func > [1.390128] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [1.397076] pc : rproc_handle_resources.constprop.0+0x78/0x1d0 > [1.402896] lr : rproc_boot+0x368/0x56c > [1.406717] sp : 8000819c3990 > [1.410017] x29: 8000819c3990 x28: 80005ae3dd7d x27: > > [1.417145] x26: x25: 015ec038 x24: > 800080f0c0a8 > [1.424268] x23: 8000813a6110 x22: d999ad79 x21: > 015ec000 > [1.431392] x20: 26665683 x19: 80005ae3dd79 x18: > 0006 > [1.438516] x17: 01799400 x16: 01798e00 x15: > 4addd15cca11c529 > [1.445639] x14: 53ebce6d5564d787 x13: 4addd15cca11c529 x12: > 53ebce6d5564d787 > [1.452763] x11: 95a1e33b6b190674 x10: 9e3c9abdb41ca345 x9 : > ab17b4eaffd6fd1c > [1.459887] x8 : d5da055de4cfbb87 x7 : dfd7fa31596acbbc x6 : > 9946d97107d0dcca > [1.467011] x5 : 010c7800 x4 : 03fc x3 : > 010c7780 > [1.474134] x2 : fff0 x1 : 8000814a3000 x0 : > 8000814a3000 > [1.481261] Call trace: > [1.483690] rproc_handle_resources.constprop.0+0x78/0x1d0 (P) > [1.487705] mmc0: new HS400 MMC card at address 0001 > [1.489502] rproc_boot+0x368/0x56c > [1.495349] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB > [1.497929] rproc_add+0x184/0x190 > [1.504356] mmcblk0: p1 p2 > [1.505747] imx_rproc_probe+0x458/0x528 > [1.509238] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB > [1.512437] platform_probe+0x68/0xc0 > [1.512452] really_probe+0xc0/0x38c > [1.520584] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB > [1.520951] __driver_probe_device+0x7c/0x15c > [1.527522] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (242:0) > [1.529377] driver_probe_device+0x3c/0x10c > [1.544263] __device_attach_driver+0xbc/0x158 > [1.548586] bus_for_each_drv+0x84/0xe0 > [1.552407] __device_attach+0x9c/0x1ac > [1.556231] device_initial_probe+0x14/0x20 > [1.560401] bus_probe_device+0xac/0xb0 > [1.564221] deferred_probe_work_func+0x9c/0xec > [1.568741] process_one_work+0x14c/0x28c > [1.572735] worker_thread+0x2cc/0x3d4 > [1.576473] kthread+0x12c/0x208 > [1.579687] ret_from_
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Thu, May 29, 2025 at 03:54:47AM +, Peng Fan wrote: [...] > > We are making progress ;-) > > > > With the patches you shared Ulf (I added them on top of the current > > master branch), it works as expected, dev_pm_genpd_is_on() returns 0 > > when I boot the kernel without M4 running and it returns 1 when I > > boot the kernel with M4 running with a hello-world demo. > > > > However now I tried to, if dev_pm_genpd_is_on() returns 1, put the > > DETACHED state, something as > > > > if (dev_pm_genpd_is_on(priv->pd_list->pd_devs[0])) > > priv->rproc->state = RPROC_DETACHED; > > > > In this case I used 0 because I understand this is the > > IMX_SC_R_M4_0_PID0 defined in my device tree overlay: > > > > power-domains = <&pd IMX_SC_R_M4_0_PID0>, > > <&pd IMX_SC_R_M4_0_MU_1A>; > > > > But in this case, the kernel does not boot anymore, I see the "Starting > > kernel..." and nothing else. > > Please add "earlycon" in bootargs to see where it hangs. Thanks Peng! I was able to catch the kernel panic yesterday, however I must say that today I was doing the tests again and the issue is gone. Sorry, I might have done something wrong yesterday with the tests. Anyway, here is the log: [1.271163] remoteproc remoteproc0: imx-rproc is available [1.280296] remoteproc remoteproc0: attaching to imx-rproc [1.285756] Unable to handle kernel paging request at virtual address 80005ae3dd79 [1.293624] Mem abort info: [1.294655] mmc0: SDHCI controller on 5b01.mmc [5b01.mmc] using ADMA [1.296386] ESR = 0x9605 [1.307194] EC = 0x25: DABT (current EL), IL = 32 bits [1.312473] SET = 0, FnV = 0 [1.315566] EA = 0, S1PTW = 0 [1.318649] FSC = 0x05: level 1 translation fault [1.323510] Data abort info: [1.326370] ISV = 0, ISS = 0x0005, ISS2 = 0x [1.331846] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [1.336882] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [1.342182] swapper pgtable: 4k pages, 48-bit VAs, pgdp=96bc1000 [1.348870] [80005ae3dd79] pgd=, p4d=100097054003, pud= [1.357565] Internal error: Oops: 9605 [#1] SMP [1.363198] Modules linked in: [1.366236] CPU: 2 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.15.0-03667-g3f5f09105c40-dirty #826 PREEMPT [1.376405] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT) [1.384313] Workqueue: events_unbound deferred_probe_work_func [1.390128] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [1.397076] pc : rproc_handle_resources.constprop.0+0x78/0x1d0 [1.402896] lr : rproc_boot+0x368/0x56c [1.406717] sp : 8000819c3990 [1.410017] x29: 8000819c3990 x28: 80005ae3dd7d x27: [1.417145] x26: x25: 015ec038 x24: 800080f0c0a8 [1.424268] x23: 8000813a6110 x22: d999ad79 x21: 015ec000 [1.431392] x20: 26665683 x19: 80005ae3dd79 x18: 0006 [1.438516] x17: 01799400 x16: 01798e00 x15: 4addd15cca11c529 [1.445639] x14: 53ebce6d5564d787 x13: 4addd15cca11c529 x12: 53ebce6d5564d787 [1.452763] x11: 95a1e33b6b190674 x10: 9e3c9abdb41ca345 x9 : ab17b4eaffd6fd1c [1.459887] x8 : d5da055de4cfbb87 x7 : dfd7fa31596acbbc x6 : 9946d97107d0dcca [1.467011] x5 : 010c7800 x4 : 03fc x3 : 010c7780 [1.474134] x2 : fff0 x1 : 8000814a3000 x0 : 8000814a3000 [1.481261] Call trace: [1.483690] rproc_handle_resources.constprop.0+0x78/0x1d0 (P) [1.487705] mmc0: new HS400 MMC card at address 0001 [1.489502] rproc_boot+0x368/0x56c [1.495349] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB [1.497929] rproc_add+0x184/0x190 [1.504356] mmcblk0: p1 p2 [1.505747] imx_rproc_probe+0x458/0x528 [1.509238] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB [1.512437] platform_probe+0x68/0xc0 [1.512452] really_probe+0xc0/0x38c [1.520584] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB [1.520951] __driver_probe_device+0x7c/0x15c [1.527522] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (242:0) [1.529377] driver_probe_device+0x3c/0x10c [1.544263] __device_attach_driver+0xbc/0x158 [1.548586] bus_for_each_drv+0x84/0xe0 [1.552407] __device_attach+0x9c/0x1ac [1.556231] device_initial_probe+0x14/0x20 [1.560401] bus_probe_device+0xac/0xb0 [1.564221] deferred_probe_work_func+0x9c/0xec [1.568741] process_one_work+0x14c/0x28c [1.572735] worker_thread+0x2cc/0x3d4 [1.576473] kthread+0x12c/0x208 [1.579687] ret_from_fork+0x10/0x20 [1.583253] Code: 8b36c033 9100127c 54000924 d503201f (b9400261) [1.589337] ---[ end trace ]--- But again, the issue is not happening anymore ;-) I will keep testing to see if the issue happens again, but for now is working fine, I
RE: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
> Subject: Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode > check for remote core attachment > > On Tue, May 27, 2025 at 10:45:25AM -0300, Hiago De Franco wrote: > > [...] > > > > > > > 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! > > We are making progress ;-) > > With the patches you shared Ulf (I added them on top of the current > master branch), it works as expected, dev_pm_genpd_is_on() returns 0 > when I boot the kernel without M4 running and it returns 1 when I > boot the kernel with M4 running with a hello-world demo. > > However now I tried to, if dev_pm_genpd_is_on() returns 1, put the > DETACHED state, something as > > if (dev_pm_genpd_is_on(priv->pd_list->pd_devs[0])) > priv->rproc->state = RPROC_DETACHED; > > In this case I used 0 because I understand this is the > IMX_SC_R_M4_0_PID0 defined in my device tree overlay: > > power-domains = <&pd IMX_SC_R_M4_0_PID0>, > <&pd IMX_SC_R_M4_0_MU_1A>; > > But in this case, the kernel does not boot anymore, I see the "Starting > kernel..." and nothing else. Please add "earlycon" in bootargs to see where it hangs. > > I am using the pm_runtime functions before rproc_add(): > > @@ -1146,6 +1154,9 @@ static int imx_rproc_probe(struct > platform_device *pdev) > } > } > > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); I think only make this apply for i.MX8QX/8QM/DX, then no impact to other patforms. Regards, Peng
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Tue, May 27, 2025 at 10:45:25AM -0300, Hiago De Franco wrote:
[...]
> >
> > 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!
We are making progress ;-)
With the patches you shared Ulf (I added them on top of the current
master branch), it works as expected, dev_pm_genpd_is_on() returns 0
when I boot the kernel without M4 running and it returns 1 when I boot
the kernel with M4 running with a hello-world demo.
However now I tried to, if dev_pm_genpd_is_on() returns 1, put the
DETACHED state, something as
if (dev_pm_genpd_is_on(priv->pd_list->pd_devs[0]))
priv->rproc->state = RPROC_DETACHED;
In this case I used 0 because I understand this is the
IMX_SC_R_M4_0_PID0 defined in my device tree overlay:
power-domains = <&pd IMX_SC_R_M4_0_PID0>,
<&pd IMX_SC_R_M4_0_MU_1A>;
But in this case, the kernel does not boot anymore, I see the "Starting
kernel..." and nothing else.
I am using the pm_runtime functions before rproc_add():
@@ -1146,6 +1154,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
}
}
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+
ret = rproc_add(rproc);
if (ret) {
dev_err(dev, "rproc_add failed\n");
and calling dev_pm_genpd_is_on() after dev_pm_domain_attach_list() has
been called.
With kernel not starting I cannot even debug it. Do you have any
suggestion on why this might be happening?
>
> >
> > [...]
> >
> > Kind regards
> > Uffe
> >
> > [1]
> > https://lore.kernel.org/all/[email protected]/
>
> Best regards,
> Hiago
Best regards,
Hiago
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 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
> > >> 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?
>
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Tue, 27 May 2025 at 03:29, Peng Fan 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
> >> 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_p
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 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 po
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Mon, May 26, 2025 at 12:07:49PM +0200, Ulf Hansson wrote:
> On Fri, 23 May 2025 at 21:17, Hiago De Franco 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
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Fri, 23 May 2025 at 21:17, Hiago De Franco 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().
>
> Do you have any suggestions? Am I doing something wrong with these PDs?
>
> Best regards,
> Hiago.
Kind regards
Uffe
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
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(). 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);
+
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.
I tried to move pm_runtime_get_sync() to .prepare function but it make
the kernel not boot anymore (with the SCU fault reset).
Do you have any suggestions? Am I doing something wrong with these PDs?
Best regards,
Hiago.
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Wed, 21 May 2025 at 05:09, Peng Fan wrote: > > 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 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 > >>> > 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 > >>> > > > 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 > >>> > > > > > wrote: > >>> > > > > > > > >>> > > > > > > From: Hiago De Franco > >>> > > > > > > > >>> > > > > > > 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 > >>> > > > > > > Suggested-by: Peng Fan > >>> > > > > > > --- > >>> > > > > > > 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 th
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
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 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
>>> > 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
>>> > > > 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
>>> > > > > > wrote:
>>> > > > > > >
>>> > > > > > > From: Hiago De Franco
>>> > > > > > >
>>> > > > > > > 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
>>> > > > > > > Suggested-by: Peng Fan
>>> > > > > > > ---
>>> > > > > > > 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;
>>> > > >
>>> > > > ge
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 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 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
>> > > > 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
>> > > > > > wrote:
>> > > > > > >
>> > > > > > > From: Hiago De Franco
>> > > > > > >
>> > > > > > > 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
>> > > > > > > Suggested-by: Peng Fan
>> > > > > > > ---
>> > > > > > > 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
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Mon, 19 May 2025 at 19:24, Hiago De Franco 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 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
> > > > 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
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Hiago De Franco
> > > > > > >
> > > > > > > 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
> > > > > > > Suggested-by: Peng Fan
> > > > > > > ---
> > > > > > > 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)
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
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 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
> > > 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
> > > > > wrote:
> > > > > >
> > > > > > From: Hiago De Franco
> > > > > >
> > > > > > 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
> > > > > > Suggested-by: Peng Fan
> > > > > > ---
> > > > > > 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
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Mon, 12 May 2025 at 05:46, Peng Fan wrote:
>
> On Fri, May 09, 2025 at 04:13:08PM -0300, Hiago De Franco 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 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
> >> > > wrote:
> >> > > >
> >> > > > From: Hiago De Franco
> >> > > >
> >> > > > 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
> >> > > > Suggested-by: Peng Fan
> >> > > > ---
> >> > > > 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:
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Mon, 19 May 2025 at 16:39, Ulf Hansson wrote:
>
> On Mon, 12 May 2025 at 05:46, Peng Fan wrote:
> >
> > On Fri, May 09, 2025 at 04:13:08PM -0300, Hiago De Franco 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
> > >> 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
> > >> > > wrote:
> > >> > > >
> > >> > > > From: Hiago De Franco
> > >> > > >
> > >> > > > 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
> > >> > > > Suggested-by: Peng Fan
> > >> > > > ---
> > >> > > > 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
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Fri, 9 May 2025 at 21:13, Hiago De Franco 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 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
> > > > wrote:
> > > > >
> > > > > From: Hiago De Franco
> > > > >
> > > > > 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
> > > > > Suggested-by: Peng Fan
> > > > > ---
> > > > > 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
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
Hi Peng, On Mon, May 12, 2025 at 12:56:13PM +0800, Peng Fan wrote: > > Ulf's new API dev_pm_genpd_is_on needs to run after power domain attached. > > But if run after power domain attached, there is no API to know whether > M4 is kicked by bootloader or now. > > Even imx_rproc_attach_pd has a check for single power domain, but I just > give a look again on current i.MX8QM/QX, all are using two power domain > entries. > > > > >> > >> In this way we don't need to export unnecessary firmware functions > >> from firmware/imx/misc.c, as patch1/3 does. > > > I think still need to export firmware API. My idea is > 1. introduce a new firmware API and put under firmware/imx/power.c > 2. Use this new firmware API in imx_rproc.c > 3. Replace scu-pd.c to use this new firmware API. > > Or > 1. Export the API in scu-pd.c > 2. Use the API in imx_rproc.c > > With approach two, you need to handle them in three trees in one patchset: > imx/pd/rproc. > > With approach one, you need to handle two trees in one patchset: imx/rproc > tree, > then after done, pd tree This patch series is already implementing approach one, as I understand, right? The difference is I moved this API from drivers/pmdomain/imx/scu-pd.c to firmware/imx/misc.c, and exported it there. But you think I should create this new file firmware/imx/power.c and move the API from scu-pd.c to power.c, is my understanding correct? > > Regards, > Peng Best Regards, Hiago.
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Fri, May 09, 2025 at 04:13:08PM -0300, Hiago De Franco 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 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
>> > > wrote:
>> > > >
>> > > > From: Hiago De Franco
>> > > >
>> > > > 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
>> > > > Suggested-by: Peng Fan
>> > > > ---
>> > > > 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 funct
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
Hi Ulf, On Thu, May 08, 2025 at 12:03:33PM +0200, Ulf Hansson wrote: >On Wed, 7 May 2025 at 18:02, Hiago De Franco wrote: >> >> From: Hiago De Franco >> >> 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 >> Suggested-by: Peng Fan >> --- >> 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? If M4 is not powered up by bootloader, attach_pd is to power up the related power domains. And the rproc->state should be OFFLINE. If M4 is powered up by bootloader, the rproc->state should be set as DETACHED, then attach_pd here will not touch the real pd, because scu-pd driver set is_off as false when doing pm_genpd_init. In this case, we still need attach_pd to avoid power shutdown when pd_ignore_unused and also need to support linux stop/start m4 even it is started by bootloader. So we could not reverse the logic. Regards, Peng > >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. > >> } >> >> -- >> 2.39.5 >> > >Kind regards >Uffe
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Fri, May 09, 2025 at 12:37:02PM +0200, Ulf Hansson wrote:
> On Thu, 8 May 2025 at 22:28, Hiago De Franco 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
> > > wrote:
> > > >
> > > > From: Hiago De Franco
> > > >
> > > > 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
> > > > Suggested-by: Peng Fan
> > > > ---
> > > > 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?
>
> In this way we don't need to export unnecessary firmware functions
> from firmware/imx/misc.c, as patch
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Thu, 8 May 2025 at 22:28, Hiago De Franco 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 wrote:
> > >
> > > From: Hiago De Franco
> > >
> > > 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
> > > Suggested-by: Peng Fan
> > > ---
> > > 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?
In this way we don't need to export unnecessary firmware functions
from firmware/imx/misc.c, as patch1/3 does.
If you think it can work, I can help to cook a formal patch for the
above helper that you can fold into your series. Let me know.
>
> With that we can correcly attach to the remote core running, which was
> not possible before, where the function returns at "return
> imx_rproc_attach_pd(priv);" with the RPROC_OFFLINE state to
> rproc_validate().
I see, thanks for clarifying!
Kind regards
Uffe
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
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 wrote: > > > > From: Hiago De Franco > > > > 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 > > Suggested-by: Peng Fan > > --- > > 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. With that we can correcly attach to the remote core running, which was not possible before, where the function returns at "return imx_rproc_attach_pd(priv);" with the RPROC_OFFLINE state to rproc_validate(). > > > } > > > > -- > > 2.39.5 > > > > Kind regards > Uffe Best Regards, Hiago.
Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment
On Wed, 7 May 2025 at 18:02, Hiago De Franco wrote: > > From: Hiago De Franco > > 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 > Suggested-by: Peng Fan > --- > 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. > } > > -- > 2.39.5 > Kind regards Uffe
