linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader
@ 2025-06-02 13:19 Hiago De Franco
  2025-06-02 13:19 ` [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on Hiago De Franco
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Hiago De Franco @ 2025-06-02 13:19 UTC (permalink / raw)
  To: Mathieu Poirier, Ulf Hansson, linux-pm, linux-remoteproc
  Cc: Shawn Guo, Sascha Hauer, Bjorn Andersson, Hiago De Franco, imx,
	linux-arm-kernel, linux-kernel, Peng Fan, daniel.baluta,
	iuliana.prodan, Rafael J . Wysocki

From: Hiago De Franco <hiago.franco@toradex.com>

This patch series depends on Ulf's patches that are currently under review,
"pmdomain: Add generic ->sync_state() support to genpd" [1]. Without them,
this series is not going to work.

For the i.MX8X and i.MX8 family SoCs, currently when the remotecore is
started by the bootloader and the M core and A core are in the same
partition, the driver is not capable to detect the remote core and
report the correct state of it.

This patch series implement a new function, dev_pm_genpd_is_on(), which
returns the power status of a given power domain (M core power domains
IMX_SC_R_M4_0_PID0 and IMX_SC_R_M4_0_MU_1A in this case). If it is already
powered on, the driver will attach to it.

Finally, the imx_rproc_clk_enable() function was also changed to make it
return before dev_clk_get() is called, as it currently generates an SCU
fault reset if the remote core is already running and the kernel tries
to enable the clock again. These changes are a follow up from a v1 sent
to imx_rproc [2] and from a reported regression [3].

[1] https://lore.kernel.org/all/20250523134025.75130-1-ulf.hansson@linaro.org/
[2] https://lore.kernel.org/lkml/20250423155131.101473-1-hiagofranco@gmail.com/
[3] https://lore.kernel.org/lkml/20250404141713.ac2ntcsjsf7epdfa@hiago-nb/

v4:
- dev_pm_genpd_is_on() introduced to drivers/pmdomain/core.c
- imx_rproc.c updated to use the generic power domains instead of the
  SCU API call, which depends on Ulf's patch series.

v3:
- https://lore.kernel.org/all/20250519171514.61974-1-hiagofranco@gmail.com/

v2:
- https://lore.kernel.org/lkml/20250507160056.11876-1-hiagofranco@gmail.com/

v1:
- https://lore.kernel.org/lkml/20250505154849.64889-1-hiagofranco@gmail.com/

Hiago De Franco (3):
  pmdomain: core: introduce dev_pm_genpd_is_on
  remoteproc: imx_rproc: skip clock enable when M-core is managed by the
    SCU
  remoteproc: imx_rproc: detect and attach to pre-booted remote cores

 drivers/pmdomain/core.c        | 27 +++++++++++++++++++++++++++
 drivers/remoteproc/imx_rproc.c | 33 ++++++++++++++++++++++++++-------
 include/linux/pm_domain.h      |  6 ++++++
 3 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.39.5



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on
  2025-06-02 13:19 [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Hiago De Franco
@ 2025-06-02 13:19 ` Hiago De Franco
  2025-06-11 15:32   ` Bjorn Andersson
  2025-06-02 13:19 ` [PATCH v4 2/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU Hiago De Franco
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Hiago De Franco @ 2025-06-02 13:19 UTC (permalink / raw)
  To: Mathieu Poirier, Ulf Hansson, linux-pm, linux-remoteproc
  Cc: Shawn Guo, Sascha Hauer, Bjorn Andersson, Hiago De Franco, imx,
	linux-arm-kernel, linux-kernel, Peng Fan, daniel.baluta,
	iuliana.prodan, Rafael J . Wysocki

From: Hiago De Franco <hiago.franco@toradex.com>

This helper function returns the current power status of a given generic
power domain.

As example, remoteproc/imx_rproc.c can now use this function to check
the power status of the remote core to properly set "attached" or
"offline" modes.

Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
---
v4: New patch.
---
 drivers/pmdomain/core.c   | 27 +++++++++++++++++++++++++++
 include/linux/pm_domain.h |  6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index ff5c7f2b69ce..bcb74d10960c 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -758,6 +758,33 @@ int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_rpm_always_on);
 
+/**
+ * dev_pm_genpd_is_on - Get device's power status
+ *
+ * @dev: Device to get the current power status
+ *
+ * This function checks whether the generic power domain is on or not by
+ * verifying if genpd_status_on equals GENPD_STATE_ON.
+ *
+ * Return: 'true' if the device's power domain is on, 'false' otherwise.
+ */
+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;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_is_on);
+
 /**
  * pm_genpd_inc_rejected() - Adjust the rejected/usage counts for an idle-state.
  *
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 0b18160901a2..c12580b6579b 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -301,6 +301,7 @@ void dev_pm_genpd_synced_poweroff(struct device *dev);
 int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
 bool dev_pm_genpd_get_hwmode(struct device *dev);
 int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);
+bool dev_pm_genpd_is_on(struct device *dev);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -393,6 +394,11 @@ static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
 	return -EOPNOTSUPP;
 }
 
+static inline bool dev_pm_genpd_is_on(struct device *dev)
+{
+	return false;
+}
+
 #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
 #endif
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 2/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU
  2025-06-02 13:19 [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Hiago De Franco
  2025-06-02 13:19 ` [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on Hiago De Franco
@ 2025-06-02 13:19 ` Hiago De Franco
  2025-06-02 13:19 ` [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores Hiago De Franco
  2025-06-03 12:09 ` [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Ulf Hansson
  3 siblings, 0 replies; 19+ messages in thread
From: Hiago De Franco @ 2025-06-02 13:19 UTC (permalink / raw)
  To: Mathieu Poirier, Ulf Hansson, linux-pm, linux-remoteproc
  Cc: Shawn Guo, Sascha Hauer, Bjorn Andersson, Hiago De Franco, imx,
	linux-arm-kernel, linux-kernel, Peng Fan, daniel.baluta,
	iuliana.prodan, Rafael J . Wysocki, Peng Fan

From: Hiago De Franco <hiago.franco@toradex.com>

For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
by the bootloader, M-core and Linux are in same SCFW (System Controller
Firmware) partition, so linux has permission to control M-core.

But when M-core is started, the SCFW will automatically enable the clock
and configure the rate, and any users that want to enable the clock will
get error 'LOCKED' from SCFW. So current imx_rproc.c probe function
fails because clk_prepare_enable also fails. With that, the M-core power
domain is powered off when it is still running, causing a SCU (System
Controller Unit) fault reset, and the system restarts.

To address the issue, ignore handling the clk for i.MX8X and i.MX8 M-core,
because SCFW will automatically enable and configure the clock.

Suggested-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
---
v4: Unchanged.
v3: Unchanged.
v2: Commit description updated, as suggested. Fixed Peng Fan email.
v1: https://lore.kernel.org/lkml/20250505154849.64889-2-hiagofranco@gmail.com/
---
 drivers/remoteproc/imx_rproc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 74299af1d7f1..627e57a88db2 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1029,8 +1029,8 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
 	struct device *dev = priv->dev;
 	int ret;
 
-	/* Remote core is not under control of Linux */
-	if (dcfg->method == IMX_RPROC_NONE)
+	/* Remote core is not under control of Linux or it is managed by SCU API */
+	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
 		return 0;
 
 	priv->clk = devm_clk_get(dev, NULL);
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-02 13:19 [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Hiago De Franco
  2025-06-02 13:19 ` [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on Hiago De Franco
  2025-06-02 13:19 ` [PATCH v4 2/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU Hiago De Franco
@ 2025-06-02 13:19 ` Hiago De Franco
  2025-06-04  3:19   ` Peng Fan
  2025-06-03 12:09 ` [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Ulf Hansson
  3 siblings, 1 reply; 19+ messages in thread
From: Hiago De Franco @ 2025-06-02 13:19 UTC (permalink / raw)
  To: Mathieu Poirier, Ulf Hansson, linux-pm, linux-remoteproc
  Cc: Shawn Guo, Sascha Hauer, Bjorn Andersson, Hiago De Franco, imx,
	linux-arm-kernel, linux-kernel, Peng Fan, daniel.baluta,
	iuliana.prodan, Rafael J . Wysocki

From: Hiago De Franco <hiago.franco@toradex.com>

When the remote core is started before Linux boots (e.g., by the
bootloader), the driver currently is not able to attach because it only
checks for cores running in different partitions. If the core was kicked
by the bootloader, it is in the same partition as Linux and it is
already up and running.

This adds power mode verification through dev_pm_genpd_is_on(), enabling
the driver to detect when the remote core is already running and
properly attach to it if all the power domain devices are on.

To accomplish this, we need to avoid passing any attach_data or flags to
dev_pm_domain_attach_list(), letting the platform device become a
consumer of the power domain provider. With that the current power state
of the genpds will not change, allowing the detection of the remote core
power state.

We enable and sync the device runtime PM during probe to make sure the
power domains are correctly managed when the core is controlled by the
kernel.

Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
---
v4: Changed to use the new dev_pm_genpd_is_on() function instead, as
suggested by Ulf. This will now get the power status of the two remote
cores power domains to decided if imx_rpoc needs to attach or not. In
order to do that, pm_runtime_enable() and pm_runtime_get_sync() were
introduced and pd_data was removed.
v3: Unchanged.
v2: Dropped unecessary include. Removed the imx_rproc_is_on function, as
suggested.
v1: https://lore.kernel.org/lkml/20250505154849.64889-4-hiagofranco@gmail.com/
---
 drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 627e57a88db2..6f9680142704 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -18,6 +18,7 @@
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
@@ -890,10 +891,8 @@ static int imx_rproc_partition_notify(struct notifier_block *nb,
 static int imx_rproc_attach_pd(struct imx_rproc *priv)
 {
 	struct device *dev = priv->dev;
-	int ret;
-	struct dev_pm_domain_attach_data pd_data = {
-		.pd_flags = PD_FLAG_DEV_LINK_ON,
-	};
+	int ret, i;
+	bool detached = true;
 
 	/*
 	 * If there is only one power-domain entry, the platform driver framework
@@ -902,7 +901,22 @@ 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);
+	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
+	/*
+	 * If all the power domain devices are already turned on, the remote
+	 * core is already up when the kernel booted (e.g. kicked by the
+	 * bootloader). In this case attach to it.
+	 */
+	for (i = 0; i < ret; i++) {
+		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
+			detached = false;
+			break;
+		}
+	}
+
+	if (detached)
+		priv->rproc->state = RPROC_DETACHED;
+
 	return ret < 0 ? ret : 0;
 }
 
@@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (dcfg->method == IMX_RPROC_SCU_API) {
+		pm_runtime_enable(dev);
+		pm_runtime_get_sync(dev);
+	}
+
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader
  2025-06-02 13:19 [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Hiago De Franco
                   ` (2 preceding siblings ...)
  2025-06-02 13:19 ` [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores Hiago De Franco
@ 2025-06-03 12:09 ` Ulf Hansson
  2025-06-03 14:21   ` Hiago De Franco
  3 siblings, 1 reply; 19+ messages in thread
From: Ulf Hansson @ 2025-06-03 12:09 UTC (permalink / raw)
  To: Hiago De Franco
  Cc: Mathieu Poirier, linux-pm, linux-remoteproc, Shawn Guo,
	Sascha Hauer, Bjorn Andersson, Hiago De Franco, imx,
	linux-arm-kernel, linux-kernel, Peng Fan, daniel.baluta,
	iuliana.prodan, Rafael J . Wysocki

On Mon, 2 Jun 2025 at 15:19, Hiago De Franco <hiagofranco@gmail.com> wrote:
>
> From: Hiago De Franco <hiago.franco@toradex.com>
>
> This patch series depends on Ulf's patches that are currently under review,
> "pmdomain: Add generic ->sync_state() support to genpd" [1]. Without them,
> this series is not going to work.
>
> For the i.MX8X and i.MX8 family SoCs, currently when the remotecore is
> started by the bootloader and the M core and A core are in the same
> partition, the driver is not capable to detect the remote core and
> report the correct state of it.
>
> This patch series implement a new function, dev_pm_genpd_is_on(), which
> returns the power status of a given power domain (M core power domains
> IMX_SC_R_M4_0_PID0 and IMX_SC_R_M4_0_MU_1A in this case). If it is already
> powered on, the driver will attach to it.
>
> Finally, the imx_rproc_clk_enable() function was also changed to make it
> return before dev_clk_get() is called, as it currently generates an SCU
> fault reset if the remote core is already running and the kernel tries
> to enable the clock again. These changes are a follow up from a v1 sent
> to imx_rproc [2] and from a reported regression [3].
>
> [1] https://lore.kernel.org/all/20250523134025.75130-1-ulf.hansson@linaro.org/
> [2] https://lore.kernel.org/lkml/20250423155131.101473-1-hiagofranco@gmail.com/
> [3] https://lore.kernel.org/lkml/20250404141713.ac2ntcsjsf7epdfa@hiago-nb/
>
> v4:
> - dev_pm_genpd_is_on() introduced to drivers/pmdomain/core.c
> - imx_rproc.c updated to use the generic power domains instead of the
>   SCU API call, which depends on Ulf's patch series.
>
> v3:
> - https://lore.kernel.org/all/20250519171514.61974-1-hiagofranco@gmail.com/
>
> v2:
> - https://lore.kernel.org/lkml/20250507160056.11876-1-hiagofranco@gmail.com/
>
> v1:
> - https://lore.kernel.org/lkml/20250505154849.64889-1-hiagofranco@gmail.com/
>
> Hiago De Franco (3):
>   pmdomain: core: introduce dev_pm_genpd_is_on
>   remoteproc: imx_rproc: skip clock enable when M-core is managed by the
>     SCU
>   remoteproc: imx_rproc: detect and attach to pre-booted remote cores
>
>  drivers/pmdomain/core.c        | 27 +++++++++++++++++++++++++++
>  drivers/remoteproc/imx_rproc.c | 33 ++++++++++++++++++++++++++-------
>  include/linux/pm_domain.h      |  6 ++++++
>  3 files changed, 59 insertions(+), 7 deletions(-)
>

We are awaiting further feedback on the other series [1]. As you have
tested it, may I add your tested-by tag for it? Or perhaps if you can
send a reply to the cover-letter for it?

Anyway, for $subject series:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader
  2025-06-03 12:09 ` [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Ulf Hansson
@ 2025-06-03 14:21   ` Hiago De Franco
  0 siblings, 0 replies; 19+ messages in thread
From: Hiago De Franco @ 2025-06-03 14:21 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Mathieu Poirier, linux-pm, linux-remoteproc, Shawn Guo,
	Sascha Hauer, Bjorn Andersson, Hiago De Franco, imx,
	linux-arm-kernel, linux-kernel, Peng Fan, daniel.baluta,
	iuliana.prodan, Rafael J . Wysocki

On Tue, Jun 03, 2025 at 02:09:14PM +0200, Ulf Hansson wrote:
> On Mon, 2 Jun 2025 at 15:19, Hiago De Franco <hiagofranco@gmail.com> wrote:
> >
> > From: Hiago De Franco <hiago.franco@toradex.com>
> >
> > This patch series depends on Ulf's patches that are currently under review,
> > "pmdomain: Add generic ->sync_state() support to genpd" [1]. Without them,
> > this series is not going to work.
> >
> > For the i.MX8X and i.MX8 family SoCs, currently when the remotecore is
> > started by the bootloader and the M core and A core are in the same
> > partition, the driver is not capable to detect the remote core and
> > report the correct state of it.
> >
> > This patch series implement a new function, dev_pm_genpd_is_on(), which
> > returns the power status of a given power domain (M core power domains
> > IMX_SC_R_M4_0_PID0 and IMX_SC_R_M4_0_MU_1A in this case). If it is already
> > powered on, the driver will attach to it.
> >
> > Finally, the imx_rproc_clk_enable() function was also changed to make it
> > return before dev_clk_get() is called, as it currently generates an SCU
> > fault reset if the remote core is already running and the kernel tries
> > to enable the clock again. These changes are a follow up from a v1 sent
> > to imx_rproc [2] and from a reported regression [3].
> >
> > [1] https://lore.kernel.org/all/20250523134025.75130-1-ulf.hansson@linaro.org/
> > [2] https://lore.kernel.org/lkml/20250423155131.101473-1-hiagofranco@gmail.com/
> > [3] https://lore.kernel.org/lkml/20250404141713.ac2ntcsjsf7epdfa@hiago-nb/
> >
> > v4:
> > - dev_pm_genpd_is_on() introduced to drivers/pmdomain/core.c
> > - imx_rproc.c updated to use the generic power domains instead of the
> >   SCU API call, which depends on Ulf's patch series.
> >
> > v3:
> > - https://lore.kernel.org/all/20250519171514.61974-1-hiagofranco@gmail.com/
> >
> > v2:
> > - https://lore.kernel.org/lkml/20250507160056.11876-1-hiagofranco@gmail.com/
> >
> > v1:
> > - https://lore.kernel.org/lkml/20250505154849.64889-1-hiagofranco@gmail.com/
> >
> > Hiago De Franco (3):
> >   pmdomain: core: introduce dev_pm_genpd_is_on
> >   remoteproc: imx_rproc: skip clock enable when M-core is managed by the
> >     SCU
> >   remoteproc: imx_rproc: detect and attach to pre-booted remote cores
> >
> >  drivers/pmdomain/core.c        | 27 +++++++++++++++++++++++++++
> >  drivers/remoteproc/imx_rproc.c | 33 ++++++++++++++++++++++++++-------
> >  include/linux/pm_domain.h      |  6 ++++++
> >  3 files changed, 59 insertions(+), 7 deletions(-)
> >
> 
> We are awaiting further feedback on the other series [1]. As you have
> tested it, may I add your tested-by tag for it? Or perhaps if you can
> send a reply to the cover-letter for it?
> 
> Anyway, for $subject series:
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Sure, I replied there with my Tested-by. Thanks for the review.

> 
> Kind regards
> Uffe
> 

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-02 13:19 ` [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores Hiago De Franco
@ 2025-06-04  3:19   ` Peng Fan
  2025-06-05 13:25     ` Hiago De Franco
                       ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Peng Fan @ 2025-06-04  3:19 UTC (permalink / raw)
  To: Hiago De Franco, Mathieu Poirier, Ulf Hansson,
	linux-pm@vger.kernel.org, linux-remoteproc@vger.kernel.org
  Cc: Shawn Guo, Sascha Hauer, Bjorn Andersson, Hiago De Franco,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

> Subject: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to
> pre-booted remote cores
> 
> From: Hiago De Franco <hiago.franco@toradex.com>
> 
> When the remote core is started before Linux boots (e.g., by the
> bootloader), the driver currently is not able to attach because it only
> checks for cores running in different partitions. If the core was kicked
> by the bootloader, it is in the same partition as Linux and it is already
> up and running.
> 
> This adds power mode verification through dev_pm_genpd_is_on(),
> enabling the driver to detect when the remote core is already running
> and properly attach to it if all the power domain devices are on.
> 
> To accomplish this, we need to avoid passing any attach_data or flags
> to dev_pm_domain_attach_list(), letting the platform device become a
> consumer of the power domain provider. With that the current power
> state of the genpds will not change, allowing the detection of the
> remote core power state.
> 
> We enable and sync the device runtime PM during probe to make sure
> the power domains are correctly managed when the core is controlled
> by the kernel.
> 
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> ---
> v4: Changed to use the new dev_pm_genpd_is_on() function instead,
> as suggested by Ulf. This will now get the power status of the two
> remote cores power domains to decided if imx_rpoc needs to attach or
> not. In order to do that, pm_runtime_enable() and
> pm_runtime_get_sync() were introduced and pd_data was removed.
> v3: Unchanged.
> v2: Dropped unecessary include. Removed the imx_rproc_is_on
> function, as suggested.
> v1:
> ---
>  drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c
> b/drivers/remoteproc/imx_rproc.c index
> 627e57a88db2..6f9680142704 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -18,6 +18,7 @@
>  #include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> @@ -890,10 +891,8 @@ static int imx_rproc_partition_notify(struct
> notifier_block *nb,  static int imx_rproc_attach_pd(struct imx_rproc
> *priv)  {
>  	struct device *dev = priv->dev;
> -	int ret;
> -	struct dev_pm_domain_attach_data pd_data = {
> -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> -	};
> +	int ret, i;
> +	bool detached = true;
> 
>  	/*
>  	 * If there is only one power-domain entry, the platform driver
> framework @@ -902,7 +901,22 @@ 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);
> +	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
> +	/*
> +	 * If all the power domain devices are already turned on, the
> remote
> +	 * core is already up when the kernel booted (e.g. kicked by
> the
> +	 * bootloader). In this case attach to it.
> +	 */
> +	for (i = 0; i < ret; i++) {
> +		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
> +			detached = false;
> +			break;
> +		}
> +	}
> +
> +	if (detached)
> +		priv->rproc->state = RPROC_DETACHED;
> +
>  	return ret < 0 ? ret : 0;
>  }
> 
> @@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct
> platform_device *pdev)
>  		}
>  	}
> 
> +	if (dcfg->method == IMX_RPROC_SCU_API) {
> +		pm_runtime_enable(dev);
> +		pm_runtime_get_sync(dev);

Need put and disable in imx_rproc_remove.

BTW: Has this patchset tested with M4 in a separate partition,
saying M4 image packed in flash.bin?

Regards,
Peng
> +	}
> +
>  	ret = rproc_add(rproc);
>  	if (ret) {
>  		dev_err(dev, "rproc_add failed\n");
> --
> 2.39.5
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-04  3:19   ` Peng Fan
@ 2025-06-05 13:25     ` Hiago De Franco
  2025-06-09 17:31     ` Hiago De Franco
  2025-06-11 15:36     ` Bjorn Andersson
  2 siblings, 0 replies; 19+ messages in thread
From: Hiago De Franco @ 2025-06-05 13:25 UTC (permalink / raw)
  To: Peng Fan
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

Hi Peng,

On Wed, Jun 04, 2025 at 03:19:52AM +0000, Peng Fan wrote:
> > Subject: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to
> > pre-booted remote cores
> > 
> > From: Hiago De Franco <hiago.franco@toradex.com>
> > 
> > When the remote core is started before Linux boots (e.g., by the
> > bootloader), the driver currently is not able to attach because it only
> > checks for cores running in different partitions. If the core was kicked
> > by the bootloader, it is in the same partition as Linux and it is already
> > up and running.
> > 
> > This adds power mode verification through dev_pm_genpd_is_on(),
> > enabling the driver to detect when the remote core is already running
> > and properly attach to it if all the power domain devices are on.
> > 
> > To accomplish this, we need to avoid passing any attach_data or flags
> > to dev_pm_domain_attach_list(), letting the platform device become a
> > consumer of the power domain provider. With that the current power
> > state of the genpds will not change, allowing the detection of the
> > remote core power state.
> > 
> > We enable and sync the device runtime PM during probe to make sure
> > the power domains are correctly managed when the core is controlled
> > by the kernel.
> > 
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > ---
> > v4: Changed to use the new dev_pm_genpd_is_on() function instead,
> > as suggested by Ulf. This will now get the power status of the two
> > remote cores power domains to decided if imx_rpoc needs to attach or
> > not. In order to do that, pm_runtime_enable() and
> > pm_runtime_get_sync() were introduced and pd_data was removed.
> > v3: Unchanged.
> > v2: Dropped unecessary include. Removed the imx_rproc_is_on
> > function, as suggested.
> > v1:
> > ---
> >  drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index
> > 627e57a88db2..6f9680142704 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/of_reserved_mem.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/reboot.h>
> >  #include <linux/regmap.h>
> >  #include <linux/remoteproc.h>
> > @@ -890,10 +891,8 @@ static int imx_rproc_partition_notify(struct
> > notifier_block *nb,  static int imx_rproc_attach_pd(struct imx_rproc
> > *priv)  {
> >  	struct device *dev = priv->dev;
> > -	int ret;
> > -	struct dev_pm_domain_attach_data pd_data = {
> > -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> > -	};
> > +	int ret, i;
> > +	bool detached = true;
> > 
> >  	/*
> >  	 * If there is only one power-domain entry, the platform driver
> > framework @@ -902,7 +901,22 @@ 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);
> > +	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
> > +	/*
> > +	 * If all the power domain devices are already turned on, the
> > remote
> > +	 * core is already up when the kernel booted (e.g. kicked by
> > the
> > +	 * bootloader). In this case attach to it.
> > +	 */
> > +	for (i = 0; i < ret; i++) {
> > +		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
> > +			detached = false;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (detached)
> > +		priv->rproc->state = RPROC_DETACHED;
> > +
> >  	return ret < 0 ? ret : 0;
> >  }
> > 
> > @@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct
> > platform_device *pdev)
> >  		}
> >  	}
> > 
> > +	if (dcfg->method == IMX_RPROC_SCU_API) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> 
> Need put and disable in imx_rproc_remove.

I will add it in a v5, thanks.

> 
> BTW: Has this patchset tested with M4 in a separate partition,
> saying M4 image packed in flash.bin?

Not yet, I will prepare this test today and let you know.

> 
> Regards,
> Peng
> > +	}
> > +
> >  	ret = rproc_add(rproc);
> >  	if (ret) {
> >  		dev_err(dev, "rproc_add failed\n");
> > --
> > 2.39.5
> > 
> 

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-04  3:19   ` Peng Fan
  2025-06-05 13:25     ` Hiago De Franco
@ 2025-06-09 17:31     ` Hiago De Franco
  2025-06-11  3:27       ` Peng Fan
  2025-06-11 15:36     ` Bjorn Andersson
  2 siblings, 1 reply; 19+ messages in thread
From: Hiago De Franco @ 2025-06-09 17:31 UTC (permalink / raw)
  To: Peng Fan
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

On Wed, Jun 04, 2025 at 03:19:52AM +0000, Peng Fan wrote:
> > Subject: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to
> > pre-booted remote cores
> > 
> > From: Hiago De Franco <hiago.franco@toradex.com>
> > 
> > When the remote core is started before Linux boots (e.g., by the
> > bootloader), the driver currently is not able to attach because it only
> > checks for cores running in different partitions. If the core was kicked
> > by the bootloader, it is in the same partition as Linux and it is already
> > up and running.
> > 
> > This adds power mode verification through dev_pm_genpd_is_on(),
> > enabling the driver to detect when the remote core is already running
> > and properly attach to it if all the power domain devices are on.
> > 
> > To accomplish this, we need to avoid passing any attach_data or flags
> > to dev_pm_domain_attach_list(), letting the platform device become a
> > consumer of the power domain provider. With that the current power
> > state of the genpds will not change, allowing the detection of the
> > remote core power state.
> > 
> > We enable and sync the device runtime PM during probe to make sure
> > the power domains are correctly managed when the core is controlled
> > by the kernel.
> > 
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > ---
> > v4: Changed to use the new dev_pm_genpd_is_on() function instead,
> > as suggested by Ulf. This will now get the power status of the two
> > remote cores power domains to decided if imx_rpoc needs to attach or
> > not. In order to do that, pm_runtime_enable() and
> > pm_runtime_get_sync() were introduced and pd_data was removed.
> > v3: Unchanged.
> > v2: Dropped unecessary include. Removed the imx_rproc_is_on
> > function, as suggested.
> > v1:
> > ---
> >  drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index
> > 627e57a88db2..6f9680142704 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/of_reserved_mem.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/reboot.h>
> >  #include <linux/regmap.h>
> >  #include <linux/remoteproc.h>
> > @@ -890,10 +891,8 @@ static int imx_rproc_partition_notify(struct
> > notifier_block *nb,  static int imx_rproc_attach_pd(struct imx_rproc
> > *priv)  {
> >  	struct device *dev = priv->dev;
> > -	int ret;
> > -	struct dev_pm_domain_attach_data pd_data = {
> > -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> > -	};
> > +	int ret, i;
> > +	bool detached = true;
> > 
> >  	/*
> >  	 * If there is only one power-domain entry, the platform driver
> > framework @@ -902,7 +901,22 @@ 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);
> > +	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
> > +	/*
> > +	 * If all the power domain devices are already turned on, the
> > remote
> > +	 * core is already up when the kernel booted (e.g. kicked by
> > the
> > +	 * bootloader). In this case attach to it.
> > +	 */
> > +	for (i = 0; i < ret; i++) {
> > +		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
> > +			detached = false;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (detached)
> > +		priv->rproc->state = RPROC_DETACHED;
> > +
> >  	return ret < 0 ? ret : 0;
> >  }
> > 
> > @@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct
> > platform_device *pdev)
> >  		}
> >  	}
> > 
> > +	if (dcfg->method == IMX_RPROC_SCU_API) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> 
> Need put and disable in imx_rproc_remove.
> 
> BTW: Has this patchset tested with M4 in a separate partition,
> saying M4 image packed in flash.bin?

Sorry for the delay.

I tested it now and there must be something missing on my U-Boot:

Disable imx8x-cm4 rsrc 278 not owned
Disable imx8x-cm4 rsrc 297 not owned

It removes my nodes from the DT before starting the kernel, so I cannot
attach. Do you know what should I do in this case?

But apart from that, at least the imx-rproc does not crash or anything.

> 
> Regards,
> Peng
> > +	}
> > +
> >  	ret = rproc_add(rproc);
> >  	if (ret) {
> >  		dev_err(dev, "rproc_add failed\n");
> > --
> > 2.39.5
> > 
> 

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-09 17:31     ` Hiago De Franco
@ 2025-06-11  3:27       ` Peng Fan
  2025-06-12 17:03         ` Hiago De Franco
  0 siblings, 1 reply; 19+ messages in thread
From: Peng Fan @ 2025-06-11  3:27 UTC (permalink / raw)
  To: Hiago De Franco
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

> Subject: Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach
> to pre-booted remote cores
> 
> On Wed, Jun 04, 2025 at 03:19:52AM +0000, Peng Fan wrote:
> > > Subject: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach
> to
> > > pre-booted remote cores
> > >
> > > From: Hiago De Franco <hiago.franco@toradex.com>
> > >
> > > When the remote core is started before Linux boots (e.g., by the
> > > bootloader), the driver currently is not able to attach because it
> > > only checks for cores running in different partitions. If the core
> > > was kicked by the bootloader, it is in the same partition as Linux
> > > and it is already up and running.
> > >
> > > This adds power mode verification through dev_pm_genpd_is_on(),
> > > enabling the driver to detect when the remote core is already
> > > running and properly attach to it if all the power domain devices
> are on.
> > >
> > > To accomplish this, we need to avoid passing any attach_data or
> > > flags to dev_pm_domain_attach_list(), letting the platform device
> > > become a consumer of the power domain provider. With that the
> > > current power state of the genpds will not change, allowing the
> > > detection of the remote core power state.
> > >
> > > We enable and sync the device runtime PM during probe to make
> sure
> > > the power domains are correctly managed when the core is
> controlled
> > > by the kernel.
> > >
> > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > > ---
> > > v4: Changed to use the new dev_pm_genpd_is_on() function
> instead, as
> > > suggested by Ulf. This will now get the power status of the two
> > > remote cores power domains to decided if imx_rpoc needs to
> attach or
> > > not. In order to do that, pm_runtime_enable() and
> > > pm_runtime_get_sync() were introduced and pd_data was removed.
> > > v3: Unchanged.
> > > v2: Dropped unecessary include. Removed the imx_rproc_is_on
> > > function, as suggested.
> > > v1:
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++--
> ---
> > >  1 file changed, 24 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index
> > > 627e57a88db2..6f9680142704 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/of_reserved_mem.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_domain.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/remoteproc.h>
> > > @@ -890,10 +891,8 @@ static int
> imx_rproc_partition_notify(struct
> > > notifier_block *nb,  static int imx_rproc_attach_pd(struct imx_rproc
> > > *priv)  {
> > >  	struct device *dev = priv->dev;
> > > -	int ret;
> > > -	struct dev_pm_domain_attach_data pd_data = {
> > > -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> > > -	};
> > > +	int ret, i;
> > > +	bool detached = true;
> > >
> > >  	/*
> > >  	 * If there is only one power-domain entry, the platform driver
> > > framework @@ -902,7 +901,22 @@ 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);
> > > +	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
> > > +	/*
> > > +	 * If all the power domain devices are already turned on, the
> > > remote
> > > +	 * core is already up when the kernel booted (e.g. kicked by
> > > the
> > > +	 * bootloader). In this case attach to it.
> > > +	 */
> > > +	for (i = 0; i < ret; i++) {
> > > +		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
> > > +			detached = false;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (detached)
> > > +		priv->rproc->state = RPROC_DETACHED;
> > > +
> > >  	return ret < 0 ? ret : 0;
> > >  }
> > >
> > > @@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct
> > > platform_device *pdev)
> > >  		}
> > >  	}
> > >
> > > +	if (dcfg->method == IMX_RPROC_SCU_API) {
> > > +		pm_runtime_enable(dev);
> > > +		pm_runtime_get_sync(dev);
> >
> > Need put and disable in imx_rproc_remove.
> >
> > BTW: Has this patchset tested with M4 in a separate partition, saying
> > M4 image packed in flash.bin?
> 
> Sorry for the delay.
> 
> I tested it now and there must be something missing on my U-Boot:
> 
> Disable imx8x-cm4 rsrc 278 not owned
> Disable imx8x-cm4 rsrc 297 not owned
> 
> It removes my nodes from the DT before starting the kernel, so I cannot
> attach. Do you know what should I do in this case?

In separate partition case, UBoot will check the permission
by checking the rsrc-id, saying power domain id.

You may need to remove the power-domains property
from M4 node.

Regards,
Peng

> 
> But apart from that, at least the imx-rproc does not crash or anything.
> 
> >
> > Regards,
> > Peng
> > > +	}
> > > +
> > >  	ret = rproc_add(rproc);
> > >  	if (ret) {
> > >  		dev_err(dev, "rproc_add failed\n");
> > > --
> > > 2.39.5
> > >
> >
> 
> Best Regards,
> Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on
  2025-06-02 13:19 ` [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on Hiago De Franco
@ 2025-06-11 15:32   ` Bjorn Andersson
  2025-06-12 17:31     ` Hiago De Franco
  0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Andersson @ 2025-06-11 15:32 UTC (permalink / raw)
  To: Hiago De Franco
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm, linux-remoteproc,
	Shawn Guo, Sascha Hauer, Hiago De Franco, imx, linux-arm-kernel,
	linux-kernel, Peng Fan, daniel.baluta, iuliana.prodan,
	Rafael J . Wysocki

On Mon, Jun 02, 2025 at 10:19:03AM -0300, Hiago De Franco wrote:
> From: Hiago De Franco <hiago.franco@toradex.com>
> 
> This helper function returns the current power status of a given generic
> power domain.
> 

Please correct me if I'm wrong, but this returns the momentary status of
the device's associated genpd, and as genpds can be shared among devices
wouldn't there be a risk that you think the genpd is on but then that
other device powers it off?

> As example, remoteproc/imx_rproc.c can now use this function to check
> the power status of the remote core to properly set "attached" or
> "offline" modes.

I presume this example works because there is a dedicated, single usage,
genpd for the remoteproc instance?

> 
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> ---
> v4: New patch.
> ---
>  drivers/pmdomain/core.c   | 27 +++++++++++++++++++++++++++
>  include/linux/pm_domain.h |  6 ++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index ff5c7f2b69ce..bcb74d10960c 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -758,6 +758,33 @@ int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_genpd_rpm_always_on);
>  
> +/**
> + * dev_pm_genpd_is_on - Get device's power status

Functions in kernel-doc should have () prefix

> + *
> + * @dev: Device to get the current power status
> + *
> + * This function checks whether the generic power domain is on or not by
> + * verifying if genpd_status_on equals GENPD_STATE_ON.
> + *

If my understanding is correct, I'd like a warning here saying that this
is dangerous if the underlying genpd is shared.

Regards,
Bjorn

> + * Return: 'true' if the device's power domain is on, 'false' otherwise.
> + */
> +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;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_is_on);
> +
>  /**
>   * pm_genpd_inc_rejected() - Adjust the rejected/usage counts for an idle-state.
>   *
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 0b18160901a2..c12580b6579b 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -301,6 +301,7 @@ void dev_pm_genpd_synced_poweroff(struct device *dev);
>  int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
>  bool dev_pm_genpd_get_hwmode(struct device *dev);
>  int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);
> +bool dev_pm_genpd_is_on(struct device *dev);
>  
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -393,6 +394,11 @@ static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
>  	return -EOPNOTSUPP;
>  }
>  
> +static inline bool dev_pm_genpd_is_on(struct device *dev)
> +{
> +	return false;
> +}
> +
>  #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
>  #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
>  #endif
> -- 
> 2.39.5
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-04  3:19   ` Peng Fan
  2025-06-05 13:25     ` Hiago De Franco
  2025-06-09 17:31     ` Hiago De Franco
@ 2025-06-11 15:36     ` Bjorn Andersson
  2025-06-12 17:05       ` Hiago De Franco
  2 siblings, 1 reply; 19+ messages in thread
From: Bjorn Andersson @ 2025-06-11 15:36 UTC (permalink / raw)
  To: Peng Fan
  Cc: Hiago De Franco, Mathieu Poirier, Ulf Hansson,
	linux-pm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	Shawn Guo, Sascha Hauer, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

On Wed, Jun 04, 2025 at 03:19:52AM +0000, Peng Fan wrote:
> > Subject: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to
> > pre-booted remote cores
> > 
> > From: Hiago De Franco <hiago.franco@toradex.com>
> > 
> > When the remote core is started before Linux boots (e.g., by the
> > bootloader), the driver currently is not able to attach because it only
> > checks for cores running in different partitions. If the core was kicked
> > by the bootloader, it is in the same partition as Linux and it is already
> > up and running.
> > 
> > This adds power mode verification through dev_pm_genpd_is_on(),
> > enabling the driver to detect when the remote core is already running
> > and properly attach to it if all the power domain devices are on.
> > 
> > To accomplish this, we need to avoid passing any attach_data or flags
> > to dev_pm_domain_attach_list(), letting the platform device become a
> > consumer of the power domain provider. With that the current power
> > state of the genpds will not change, allowing the detection of the
> > remote core power state.
> > 
> > We enable and sync the device runtime PM during probe to make sure
> > the power domains are correctly managed when the core is controlled
> > by the kernel.
> > 
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > ---
> > v4: Changed to use the new dev_pm_genpd_is_on() function instead,
> > as suggested by Ulf. This will now get the power status of the two
> > remote cores power domains to decided if imx_rpoc needs to attach or
> > not. In order to do that, pm_runtime_enable() and
> > pm_runtime_get_sync() were introduced and pd_data was removed.
> > v3: Unchanged.
> > v2: Dropped unecessary include. Removed the imx_rproc_is_on
> > function, as suggested.
> > v1:
> > ---
> >  drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index
> > 627e57a88db2..6f9680142704 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/of_reserved_mem.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/reboot.h>
> >  #include <linux/regmap.h>
> >  #include <linux/remoteproc.h>
> > @@ -890,10 +891,8 @@ static int imx_rproc_partition_notify(struct
> > notifier_block *nb,  static int imx_rproc_attach_pd(struct imx_rproc
> > *priv)  {
> >  	struct device *dev = priv->dev;
> > -	int ret;
> > -	struct dev_pm_domain_attach_data pd_data = {
> > -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> > -	};
> > +	int ret, i;
> > +	bool detached = true;
> > 
> >  	/*
> >  	 * If there is only one power-domain entry, the platform driver
> > framework @@ -902,7 +901,22 @@ 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);
> > +	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
> > +	/*
> > +	 * If all the power domain devices are already turned on, the
> > remote
> > +	 * core is already up when the kernel booted (e.g. kicked by
> > the
> > +	 * bootloader). In this case attach to it.
> > +	 */
> > +	for (i = 0; i < ret; i++) {
> > +		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
> > +			detached = false;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (detached)
> > +		priv->rproc->state = RPROC_DETACHED;
> > +
> >  	return ret < 0 ? ret : 0;
> >  }
> > 
> > @@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct
> > platform_device *pdev)
> >  		}
> >  	}
> > 
> > +	if (dcfg->method == IMX_RPROC_SCU_API) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> 
> Need put and disable in imx_rproc_remove.
> 

Note that you also need to pm_runtime_put() if pm_runtime_get_sync()
returns an error. So the suggestion is to use the convenience helper
pm_runtime_resume_and_get() to handle this for you.

Probably a good idea to check the return value regardless.

Regards,
Bjorn

> BTW: Has this patchset tested with M4 in a separate partition,
> saying M4 image packed in flash.bin?
> 
> Regards,
> Peng
> > +	}
> > +
> >  	ret = rproc_add(rproc);
> >  	if (ret) {
> >  		dev_err(dev, "rproc_add failed\n");
> > --
> > 2.39.5
> > 
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-11  3:27       ` Peng Fan
@ 2025-06-12 17:03         ` Hiago De Franco
  2025-06-16 16:05           ` Hiago De Franco
  0 siblings, 1 reply; 19+ messages in thread
From: Hiago De Franco @ 2025-06-12 17:03 UTC (permalink / raw)
  To: Peng Fan
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

Hi Peng,

On Wed, Jun 11, 2025 at 03:27:09AM +0000, Peng Fan wrote:
> > 
> > Sorry for the delay.
> > 
> > I tested it now and there must be something missing on my U-Boot:
> > 
> > Disable imx8x-cm4 rsrc 278 not owned
> > Disable imx8x-cm4 rsrc 297 not owned
> > 
> > It removes my nodes from the DT before starting the kernel, so I cannot
> > attach. Do you know what should I do in this case?
> 
> In separate partition case, UBoot will check the permission
> by checking the rsrc-id, saying power domain id.
> 
> You may need to remove the power-domains property
> from M4 node.

Without the power-domains property, rproc gives me a kernel panic:

[    1.253234] remoteproc remoteproc0: imx-rproc is available
[    1.258501] remoteproc remoteproc0: attaching to imx-rproc
[    1.263950] Unable to handle kernel paging request at virtual address ffff80005ae57d39
[    1.271812] Mem abort info:
[    1.274575]   ESR = 0x0000000096000005
[    1.278299]   EC = 0x25: DABT (current EL), IL = 32 bits
[    1.282581] mmc0: SDHCI controller on 5b010000.mmc [5b010000.mmc] using ADMA
[    1.283607]   SET = 0, FnV = 0
[    1.293701]   EA = 0, S1PTW = 0
[    1.296815]   FSC = 0x05: level 1 translation fault
[    1.301699] Data abort info:
[    1.304545]   ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
[    1.310079]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[    1.315073]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    1.320367] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000096bf3000
[    1.327061] [ffff80005ae57d39] pgd=0000000000000000, p4d=1000000097085003, pud=0000000000000000
[    1.335750] Internal error: Oops: 0000000096000005 [#1]  SMP
[    1.341373] Modules linked in:
[    1.344414] CPU: 3 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.16.0-rc1-00024-gfe5d6ab20714-dirty
 #857 PREEMPT
[    1.354932] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT)
[    1.362837] Workqueue: events_unbound deferred_probe_work_func
[    1.368651] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    1.375601] pc : rproc_handle_resources.constprop.0+0x78/0x1d0
[    1.381421] lr : rproc_boot+0x368/0x578
[    1.385242] sp : ffff8000819f3990
[    1.388542] x29: ffff8000819f3990 x28: ffff80005ae57d3d x27: 0000000000000000
[    1.395671] x26: 0000000000000000 x25: ffff0000016ee038 x24: ffff800080f3c680
[    1.402793] x23: ffff8000813d6da8 x22: 00000000d999ad39 x21: ffff0000016ee000
[    1.409917] x20: 00000000266656c3 x19: ffff80005ae57d39 x18: 0000000000000006
[    1.417040] x17: ffff000002020600 x16: ffff000002020000 x15: 4addd15cca11c529
[    1.424164] x14: 73ebceed5d6cd787 x13: 4addd15cca11c529 x12: 73ebceed5d6cd787
[    1.431288] x11: 95a4e33b6b190664 x10: 9e3cdabdb09ca345 x9 : ab3734eafdd6fd1c
[    1.438412] x8 : d58a055de4cfb385 x7 : de97fab1791acbbe x6 : 9946d97107d0dcda
[    1.445535] x5 : ffff0000032b2c00 x4 : 00000000000003fc x3 : ffff0000032b2b80
[    1.452659] x2 : fffffffffffffff0 x1 : ffff8000814bd000 x0 : ffff8000814bd000
[    1.459786] Call trace:
[    1.462215]  rproc_handle_resources.constprop.0+0x78/0x1d0 (P)
[    1.468036]  rproc_boot+0x368/0x578
[    1.471510]  rproc_add+0x180/0x18c
[    1.474898]  imx_rproc_probe+0x3e4/0x540
[    1.475274] mmc0: new HS400 MMC card at address 0001
[    1.478799]  platform_probe+0x68/0xc0
[    1.484628] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB
[    1.487400]  really_probe+0xc0/0x38c
[    1.487412]  __driver_probe_device+0x7c/0x15c
[    1.487424]  driver_probe_device+0x3c/0x10c
[    1.493941]  mmcblk0: p1 p2
[    1.495392]  __device_attach_driver+0xbc/0x158
[    1.495405]  bus_for_each_drv+0x84/0xe0
[    1.495417]  __device_attach+0x9c/0x1ac
[    1.500468] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB
[    1.503906]  device_initial_probe+0x14/0x20
[    1.503918]  bus_probe_device+0xac/0xb0
[    1.503929]  deferred_probe_work_func+0x9c/0xec
[    1.509863] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB
[    1.511117]  process_one_work+0x14c/0x28c
[    1.511132]  worker_thread+0x2cc/0x3d4
[    1.511142]  kthread+0x12c/0x208
[    1.511157]  ret_from_fork+0x10/0x20
[    1.517964] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (241:0)
[    1.518770] Code: 8b36c033 9100127c 54000924 d503201f (b9400261)
[    1.518777] ---[ end trace 0000000000000000 ]---

Currently I have the M4 partiton defined into the SCU code:

            /* Create partition */
            BRD_ERR(rm_partition_create(pt_boot, &pt_m4_0, SC_FALSE,
                SC_TRUE, SC_FALSE, SC_TRUE, SC_FALSE, SC_R_M4_0_PID0,
                rsrc_list, ARRAY_SIZE(rsrc_list),
                pad_list, ARRAY_SIZE(pad_list),
                NULL, 0));

            /* Name partition for debug */
            PARTITION_NAME(pt_m4_0, "MCU0");
            
            /* Allow AP to use SYSTEM (not production!) */
            BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_SYSTEM,
                pt_boot, SC_RM_PERM_SEC_RW));

            /* Move M4 0 TCM */
            BRD_ERR(rm_find_memreg(pt_boot, &mr, 0x034FE0000ULL,
                0x034FE0000ULL));
            BRD_ERR(rm_assign_memreg(pt_boot, pt_m4_0, mr));

            /* Move partition to be owned by SC */
            BRD_ERR(rm_set_parent(pt_boot, pt_m4_0, SC_PT));

            /* Check if booting with the no_ap flag set */
            if (no_ap != SC_FALSE)
            {
                /* Move boot to be owned by M4 0 for Android Automotive */
                BRD_ERR(rm_set_parent(SC_PT, pt_boot, pt_m4_0));
            }
        }

        /* Allow all to access the SEMA42s */
        BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_M4_0_SEMA42,
            SC_RM_PT_ALL, SC_RM_PERM_FULL));

I believe this SCU code is correct, at least this is more or less what
NXP provides as example, right?

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-11 15:36     ` Bjorn Andersson
@ 2025-06-12 17:05       ` Hiago De Franco
  0 siblings, 0 replies; 19+ messages in thread
From: Hiago De Franco @ 2025-06-12 17:05 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Peng Fan, Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

Hi Bjorn,

On Wed, Jun 11, 2025 at 10:36:33AM -0500, Bjorn Andersson wrote:
> On Wed, Jun 04, 2025 at 03:19:52AM +0000, Peng Fan wrote:
> > > Subject: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to
> > > pre-booted remote cores
> > > 
> > > From: Hiago De Franco <hiago.franco@toradex.com>
> > > 
> > > When the remote core is started before Linux boots (e.g., by the
> > > bootloader), the driver currently is not able to attach because it only
> > > checks for cores running in different partitions. If the core was kicked
> > > by the bootloader, it is in the same partition as Linux and it is already
> > > up and running.
> > > 
> > > This adds power mode verification through dev_pm_genpd_is_on(),
> > > enabling the driver to detect when the remote core is already running
> > > and properly attach to it if all the power domain devices are on.
> > > 
> > > To accomplish this, we need to avoid passing any attach_data or flags
> > > to dev_pm_domain_attach_list(), letting the platform device become a
> > > consumer of the power domain provider. With that the current power
> > > state of the genpds will not change, allowing the detection of the
> > > remote core power state.
> > > 
> > > We enable and sync the device runtime PM during probe to make sure
> > > the power domains are correctly managed when the core is controlled
> > > by the kernel.
> > > 
> > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > > ---
> > > v4: Changed to use the new dev_pm_genpd_is_on() function instead,
> > > as suggested by Ulf. This will now get the power status of the two
> > > remote cores power domains to decided if imx_rpoc needs to attach or
> > > not. In order to do that, pm_runtime_enable() and
> > > pm_runtime_get_sync() were introduced and pd_data was removed.
> > > v3: Unchanged.
> > > v2: Dropped unecessary include. Removed the imx_rproc_is_on
> > > function, as suggested.
> > > v1:
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 29 ++++++++++++++++++++++++-----
> > >  1 file changed, 24 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index
> > > 627e57a88db2..6f9680142704 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/of_reserved_mem.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_domain.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/remoteproc.h>
> > > @@ -890,10 +891,8 @@ static int imx_rproc_partition_notify(struct
> > > notifier_block *nb,  static int imx_rproc_attach_pd(struct imx_rproc
> > > *priv)  {
> > >  	struct device *dev = priv->dev;
> > > -	int ret;
> > > -	struct dev_pm_domain_attach_data pd_data = {
> > > -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> > > -	};
> > > +	int ret, i;
> > > +	bool detached = true;
> > > 
> > >  	/*
> > >  	 * If there is only one power-domain entry, the platform driver
> > > framework @@ -902,7 +901,22 @@ 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);
> > > +	ret = dev_pm_domain_attach_list(dev, NULL, &priv->pd_list);
> > > +	/*
> > > +	 * If all the power domain devices are already turned on, the
> > > remote
> > > +	 * core is already up when the kernel booted (e.g. kicked by
> > > the
> > > +	 * bootloader). In this case attach to it.
> > > +	 */
> > > +	for (i = 0; i < ret; i++) {
> > > +		if (!dev_pm_genpd_is_on(priv->pd_list->pd_devs[i])) {
> > > +			detached = false;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (detached)
> > > +		priv->rproc->state = RPROC_DETACHED;
> > > +
> > >  	return ret < 0 ? ret : 0;
> > >  }
> > > 
> > > @@ -1146,6 +1160,11 @@ static int imx_rproc_probe(struct
> > > platform_device *pdev)
> > >  		}
> > >  	}
> > > 
> > > +	if (dcfg->method == IMX_RPROC_SCU_API) {
> > > +		pm_runtime_enable(dev);
> > > +		pm_runtime_get_sync(dev);
> > 
> > Need put and disable in imx_rproc_remove.
> > 
> 
> Note that you also need to pm_runtime_put() if pm_runtime_get_sync()
> returns an error. So the suggestion is to use the convenience helper
> pm_runtime_resume_and_get() to handle this for you.
> 
> Probably a good idea to check the return value regardless.

Thanks for the review, noted, I can include this in the next version.

> 
> Regards,
> Bjorn
> 
> > BTW: Has this patchset tested with M4 in a separate partition,
> > saying M4 image packed in flash.bin?
> > 
> > Regards,
> > Peng
> > > +	}
> > > +
> > >  	ret = rproc_add(rproc);
> > >  	if (ret) {
> > >  		dev_err(dev, "rproc_add failed\n");
> > > --
> > > 2.39.5
> > > 
> >

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on
  2025-06-11 15:32   ` Bjorn Andersson
@ 2025-06-12 17:31     ` Hiago De Franco
  2025-06-16 13:13       ` Ulf Hansson
  0 siblings, 1 reply; 19+ messages in thread
From: Hiago De Franco @ 2025-06-12 17:31 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm, linux-remoteproc,
	Shawn Guo, Sascha Hauer, Hiago De Franco, imx, linux-arm-kernel,
	linux-kernel, Peng Fan, daniel.baluta, iuliana.prodan,
	Rafael J . Wysocki

On Wed, Jun 11, 2025 at 10:32:28AM -0500, Bjorn Andersson wrote:
> On Mon, Jun 02, 2025 at 10:19:03AM -0300, Hiago De Franco wrote:
> > From: Hiago De Franco <hiago.franco@toradex.com>
> > 
> > This helper function returns the current power status of a given generic
> > power domain.
> > 
> 
> Please correct me if I'm wrong, but this returns the momentary status of
> the device's associated genpd, and as genpds can be shared among devices
> wouldn't there be a risk that you think the genpd is on but then that
> other device powers it off?

I am not fully familiar with the genpd's, so my knowledge might be
limited, but I think this is correct, if the genpd is shared.

> 
> > As example, remoteproc/imx_rproc.c can now use this function to check
> > the power status of the remote core to properly set "attached" or
> > "offline" modes.
> 
> I presume this example works because there is a dedicated, single usage,
> genpd for the remoteproc instance?

Peng might correct if I am wrong, but yes, I believe this is correct.

> 
> > 
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > ---
> > v4: New patch.
> > ---
> >  drivers/pmdomain/core.c   | 27 +++++++++++++++++++++++++++
> >  include/linux/pm_domain.h |  6 ++++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index ff5c7f2b69ce..bcb74d10960c 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -758,6 +758,33 @@ int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_genpd_rpm_always_on);
> >  
> > +/**
> > + * dev_pm_genpd_is_on - Get device's power status
> 
> Functions in kernel-doc should have () prefix

Thanks, I will correct this is next patch version.

> 
> > + *
> > + * @dev: Device to get the current power status
> > + *
> > + * This function checks whether the generic power domain is on or not by
> > + * verifying if genpd_status_on equals GENPD_STATE_ON.
> > + *
> 
> If my understanding is correct, I'd like a warning here saying that this
> is dangerous if the underlying genpd is shared.

I believe this is correct, maybe Peng or Ulf can also comment here, but
if that is the case then I can update the comment.

> 
> Regards,
> Bjorn
> 
> > + * Return: 'true' if the device's power domain is on, 'false' otherwise.
> > + */
> > +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;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_genpd_is_on);
> > +
> >  /**
> >   * pm_genpd_inc_rejected() - Adjust the rejected/usage counts for an idle-state.
> >   *
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 0b18160901a2..c12580b6579b 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -301,6 +301,7 @@ void dev_pm_genpd_synced_poweroff(struct device *dev);
> >  int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
> >  bool dev_pm_genpd_get_hwmode(struct device *dev);
> >  int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);
> > +bool dev_pm_genpd_is_on(struct device *dev);
> >  
> >  extern struct dev_power_governor simple_qos_governor;
> >  extern struct dev_power_governor pm_domain_always_on_gov;
> > @@ -393,6 +394,11 @@ static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
> >  	return -EOPNOTSUPP;
> >  }
> >  
> > +static inline bool dev_pm_genpd_is_on(struct device *dev)
> > +{
> > +	return false;
> > +}
> > +
> >  #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
> >  #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
> >  #endif
> > -- 
> > 2.39.5
> >

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on
  2025-06-12 17:31     ` Hiago De Franco
@ 2025-06-16 13:13       ` Ulf Hansson
  0 siblings, 0 replies; 19+ messages in thread
From: Ulf Hansson @ 2025-06-16 13:13 UTC (permalink / raw)
  To: Hiago De Franco
  Cc: Bjorn Andersson, Mathieu Poirier, linux-pm, linux-remoteproc,
	Shawn Guo, Sascha Hauer, Hiago De Franco, imx, linux-arm-kernel,
	linux-kernel, Peng Fan, daniel.baluta, iuliana.prodan,
	Rafael J . Wysocki

On Thu, 12 Jun 2025 at 19:31, Hiago De Franco <hiagofranco@gmail.com> wrote:
>
> On Wed, Jun 11, 2025 at 10:32:28AM -0500, Bjorn Andersson wrote:
> > On Mon, Jun 02, 2025 at 10:19:03AM -0300, Hiago De Franco wrote:
> > > From: Hiago De Franco <hiago.franco@toradex.com>
> > >
> > > This helper function returns the current power status of a given generic
> > > power domain.
> > >
> >
> > Please correct me if I'm wrong, but this returns the momentary status of
> > the device's associated genpd, and as genpds can be shared among devices
> > wouldn't there be a risk that you think the genpd is on but then that
> > other device powers it off?
>
> I am not fully familiar with the genpd's, so my knowledge might be
> limited, but I think this is correct, if the genpd is shared.
>
> >
> > > As example, remoteproc/imx_rproc.c can now use this function to check
> > > the power status of the remote core to properly set "attached" or
> > > "offline" modes.
> >
> > I presume this example works because there is a dedicated, single usage,
> > genpd for the remoteproc instance?
>
> Peng might correct if I am wrong, but yes, I believe this is correct.
>
> >
> > >
> > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > > ---
> > > v4: New patch.
> > > ---
> > >  drivers/pmdomain/core.c   | 27 +++++++++++++++++++++++++++
> > >  include/linux/pm_domain.h |  6 ++++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > index ff5c7f2b69ce..bcb74d10960c 100644
> > > --- a/drivers/pmdomain/core.c
> > > +++ b/drivers/pmdomain/core.c
> > > @@ -758,6 +758,33 @@ int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_pm_genpd_rpm_always_on);
> > >
> > > +/**
> > > + * dev_pm_genpd_is_on - Get device's power status
> >
> > Functions in kernel-doc should have () prefix
>
> Thanks, I will correct this is next patch version.
>
> >
> > > + *
> > > + * @dev: Device to get the current power status
> > > + *
> > > + * This function checks whether the generic power domain is on or not by
> > > + * verifying if genpd_status_on equals GENPD_STATE_ON.
> > > + *
> >
> > If my understanding is correct, I'd like a warning here saying that this
> > is dangerous if the underlying genpd is shared.
>
> I believe this is correct, maybe Peng or Ulf can also comment here, but
> if that is the case then I can update the comment.

Good point!

I would not say that it's "dangerous", while I agree that we need to
extend the comment to make it really clear that it returns the current
power status of the genpd, which potentially may change beyond
returning from the function and especially if the genpd has multiple
consumers.

[...]

Kind regards
Uffe


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-12 17:03         ` Hiago De Franco
@ 2025-06-16 16:05           ` Hiago De Franco
  2025-06-17  2:39             ` Peng Fan
  0 siblings, 1 reply; 19+ messages in thread
From: Hiago De Franco @ 2025-06-16 16:05 UTC (permalink / raw)
  To: Peng Fan
  Cc: Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Peng Fan (OSS), Daniel Baluta,
	Iuliana Prodan (OSS), Rafael J . Wysocki

Hi Peng,

On Thu, Jun 12, 2025 at 02:03:17PM -0300, Hiago De Franco wrote:
> Hi Peng,
> 
> On Wed, Jun 11, 2025 at 03:27:09AM +0000, Peng Fan wrote:
> > > 
> > > Sorry for the delay.
> > > 
> > > I tested it now and there must be something missing on my U-Boot:
> > > 
> > > Disable imx8x-cm4 rsrc 278 not owned
> > > Disable imx8x-cm4 rsrc 297 not owned
> > > 
> > > It removes my nodes from the DT before starting the kernel, so I cannot
> > > attach. Do you know what should I do in this case?
> > 
> > In separate partition case, UBoot will check the permission
> > by checking the rsrc-id, saying power domain id.
> > 
> > You may need to remove the power-domains property
> > from M4 node.
> 
> Without the power-domains property, rproc gives me a kernel panic:
> 
> [    1.253234] remoteproc remoteproc0: imx-rproc is available
> [    1.258501] remoteproc remoteproc0: attaching to imx-rproc
> [    1.263950] Unable to handle kernel paging request at virtual address ffff80005ae57d39
> [    1.271812] Mem abort info:
> [    1.274575]   ESR = 0x0000000096000005
> [    1.278299]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    1.282581] mmc0: SDHCI controller on 5b010000.mmc [5b010000.mmc] using ADMA
> [    1.283607]   SET = 0, FnV = 0
> [    1.293701]   EA = 0, S1PTW = 0
> [    1.296815]   FSC = 0x05: level 1 translation fault
> [    1.301699] Data abort info:
> [    1.304545]   ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
> [    1.310079]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [    1.315073]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [    1.320367] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000096bf3000
> [    1.327061] [ffff80005ae57d39] pgd=0000000000000000, p4d=1000000097085003, pud=0000000000000000
> [    1.335750] Internal error: Oops: 0000000096000005 [#1]  SMP
> [    1.341373] Modules linked in:
> [    1.344414] CPU: 3 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.16.0-rc1-00024-gfe5d6ab20714-dirty
>  #857 PREEMPT
> [    1.354932] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT)
> [    1.362837] Workqueue: events_unbound deferred_probe_work_func
> [    1.368651] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    1.375601] pc : rproc_handle_resources.constprop.0+0x78/0x1d0
> [    1.381421] lr : rproc_boot+0x368/0x578
> [    1.385242] sp : ffff8000819f3990
> [    1.388542] x29: ffff8000819f3990 x28: ffff80005ae57d3d x27: 0000000000000000
> [    1.395671] x26: 0000000000000000 x25: ffff0000016ee038 x24: ffff800080f3c680
> [    1.402793] x23: ffff8000813d6da8 x22: 00000000d999ad39 x21: ffff0000016ee000
> [    1.409917] x20: 00000000266656c3 x19: ffff80005ae57d39 x18: 0000000000000006
> [    1.417040] x17: ffff000002020600 x16: ffff000002020000 x15: 4addd15cca11c529
> [    1.424164] x14: 73ebceed5d6cd787 x13: 4addd15cca11c529 x12: 73ebceed5d6cd787
> [    1.431288] x11: 95a4e33b6b190664 x10: 9e3cdabdb09ca345 x9 : ab3734eafdd6fd1c
> [    1.438412] x8 : d58a055de4cfb385 x7 : de97fab1791acbbe x6 : 9946d97107d0dcda
> [    1.445535] x5 : ffff0000032b2c00 x4 : 00000000000003fc x3 : ffff0000032b2b80
> [    1.452659] x2 : fffffffffffffff0 x1 : ffff8000814bd000 x0 : ffff8000814bd000
> [    1.459786] Call trace:
> [    1.462215]  rproc_handle_resources.constprop.0+0x78/0x1d0 (P)
> [    1.468036]  rproc_boot+0x368/0x578
> [    1.471510]  rproc_add+0x180/0x18c
> [    1.474898]  imx_rproc_probe+0x3e4/0x540
> [    1.475274] mmc0: new HS400 MMC card at address 0001
> [    1.478799]  platform_probe+0x68/0xc0
> [    1.484628] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB
> [    1.487400]  really_probe+0xc0/0x38c
> [    1.487412]  __driver_probe_device+0x7c/0x15c
> [    1.487424]  driver_probe_device+0x3c/0x10c
> [    1.493941]  mmcblk0: p1 p2
> [    1.495392]  __device_attach_driver+0xbc/0x158
> [    1.495405]  bus_for_each_drv+0x84/0xe0
> [    1.495417]  __device_attach+0x9c/0x1ac
> [    1.500468] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB
> [    1.503906]  device_initial_probe+0x14/0x20
> [    1.503918]  bus_probe_device+0xac/0xb0
> [    1.503929]  deferred_probe_work_func+0x9c/0xec
> [    1.509863] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB
> [    1.511117]  process_one_work+0x14c/0x28c
> [    1.511132]  worker_thread+0x2cc/0x3d4
> [    1.511142]  kthread+0x12c/0x208
> [    1.511157]  ret_from_fork+0x10/0x20
> [    1.517964] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (241:0)
> [    1.518770] Code: 8b36c033 9100127c 54000924 d503201f (b9400261)
> [    1.518777] ---[ end trace 0000000000000000 ]---
> 
> Currently I have the M4 partiton defined into the SCU code:
> 
>             /* Create partition */
>             BRD_ERR(rm_partition_create(pt_boot, &pt_m4_0, SC_FALSE,
>                 SC_TRUE, SC_FALSE, SC_TRUE, SC_FALSE, SC_R_M4_0_PID0,
>                 rsrc_list, ARRAY_SIZE(rsrc_list),
>                 pad_list, ARRAY_SIZE(pad_list),
>                 NULL, 0));
> 
>             /* Name partition for debug */
>             PARTITION_NAME(pt_m4_0, "MCU0");
>             
>             /* Allow AP to use SYSTEM (not production!) */
>             BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_SYSTEM,
>                 pt_boot, SC_RM_PERM_SEC_RW));
> 
>             /* Move M4 0 TCM */
>             BRD_ERR(rm_find_memreg(pt_boot, &mr, 0x034FE0000ULL,
>                 0x034FE0000ULL));
>             BRD_ERR(rm_assign_memreg(pt_boot, pt_m4_0, mr));
> 
>             /* Move partition to be owned by SC */
>             BRD_ERR(rm_set_parent(pt_boot, pt_m4_0, SC_PT));
> 
>             /* Check if booting with the no_ap flag set */
>             if (no_ap != SC_FALSE)
>             {
>                 /* Move boot to be owned by M4 0 for Android Automotive */
>                 BRD_ERR(rm_set_parent(SC_PT, pt_boot, pt_m4_0));
>             }
>         }
> 
>         /* Allow all to access the SEMA42s */
>         BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_M4_0_SEMA42,
>             SC_RM_PT_ALL, SC_RM_PERM_FULL));
> 
> I believe this SCU code is correct, at least this is more or less what
> NXP provides as example, right?

I tested the same SCU code and DTS overlay (removing the power-domains)
with the current master branch of Linux and I got the same kernel panic.
Maybe this part is already broken? Can you also test this on your side
to check if this is currently working?

> 
> Best Regards,
> Hiago.

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-16 16:05           ` Hiago De Franco
@ 2025-06-17  2:39             ` Peng Fan
  2025-06-17 15:56               ` Hiago De Franco
  0 siblings, 1 reply; 19+ messages in thread
From: Peng Fan @ 2025-06-17  2:39 UTC (permalink / raw)
  To: Hiago De Franco
  Cc: Peng Fan, Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Daniel Baluta, Iuliana Prodan (OSS),
	Rafael J . Wysocki

On Mon, Jun 16, 2025 at 01:05:11PM -0300, Hiago De Franco wrote:
>Hi Peng,
>
>On Thu, Jun 12, 2025 at 02:03:17PM -0300, Hiago De Franco wrote:
>> Hi Peng,
>> 
>> On Wed, Jun 11, 2025 at 03:27:09AM +0000, Peng Fan wrote:
>> > > 
>> > > Sorry for the delay.
>> > > 
>> > > I tested it now and there must be something missing on my U-Boot:
>> > > 
>> > > Disable imx8x-cm4 rsrc 278 not owned
>> > > Disable imx8x-cm4 rsrc 297 not owned
>> > > 
>> > > It removes my nodes from the DT before starting the kernel, so I cannot
>> > > attach. Do you know what should I do in this case?
>> > 
>> > In separate partition case, UBoot will check the permission
>> > by checking the rsrc-id, saying power domain id.
>> > 
>> > You may need to remove the power-domains property
>> > from M4 node.
>> 
>> Without the power-domains property, rproc gives me a kernel panic:
>> 
>> [    1.253234] remoteproc remoteproc0: imx-rproc is available
>> [    1.258501] remoteproc remoteproc0: attaching to imx-rproc
>> [    1.263950] Unable to handle kernel paging request at virtual address ffff80005ae57d39
>> [    1.271812] Mem abort info:
>> [    1.274575]   ESR = 0x0000000096000005
>> [    1.278299]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [    1.282581] mmc0: SDHCI controller on 5b010000.mmc [5b010000.mmc] using ADMA
>> [    1.283607]   SET = 0, FnV = 0
>> [    1.293701]   EA = 0, S1PTW = 0
>> [    1.296815]   FSC = 0x05: level 1 translation fault
>> [    1.301699] Data abort info:
>> [    1.304545]   ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
>> [    1.310079]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>> [    1.315073]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>> [    1.320367] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000096bf3000
>> [    1.327061] [ffff80005ae57d39] pgd=0000000000000000, p4d=1000000097085003, pud=0000000000000000
>> [    1.335750] Internal error: Oops: 0000000096000005 [#1]  SMP
>> [    1.341373] Modules linked in:
>> [    1.344414] CPU: 3 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.16.0-rc1-00024-gfe5d6ab20714-dirty
>>  #857 PREEMPT
>> [    1.354932] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT)
>> [    1.362837] Workqueue: events_unbound deferred_probe_work_func
>> [    1.368651] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [    1.375601] pc : rproc_handle_resources.constprop.0+0x78/0x1d0
>> [    1.381421] lr : rproc_boot+0x368/0x578
>> [    1.385242] sp : ffff8000819f3990
>> [    1.388542] x29: ffff8000819f3990 x28: ffff80005ae57d3d x27: 0000000000000000
>> [    1.395671] x26: 0000000000000000 x25: ffff0000016ee038 x24: ffff800080f3c680
>> [    1.402793] x23: ffff8000813d6da8 x22: 00000000d999ad39 x21: ffff0000016ee000
>> [    1.409917] x20: 00000000266656c3 x19: ffff80005ae57d39 x18: 0000000000000006
>> [    1.417040] x17: ffff000002020600 x16: ffff000002020000 x15: 4addd15cca11c529
>> [    1.424164] x14: 73ebceed5d6cd787 x13: 4addd15cca11c529 x12: 73ebceed5d6cd787
>> [    1.431288] x11: 95a4e33b6b190664 x10: 9e3cdabdb09ca345 x9 : ab3734eafdd6fd1c
>> [    1.438412] x8 : d58a055de4cfb385 x7 : de97fab1791acbbe x6 : 9946d97107d0dcda
>> [    1.445535] x5 : ffff0000032b2c00 x4 : 00000000000003fc x3 : ffff0000032b2b80
>> [    1.452659] x2 : fffffffffffffff0 x1 : ffff8000814bd000 x0 : ffff8000814bd000
>> [    1.459786] Call trace:
>> [    1.462215]  rproc_handle_resources.constprop.0+0x78/0x1d0 (P)
>> [    1.468036]  rproc_boot+0x368/0x578
>> [    1.471510]  rproc_add+0x180/0x18c
>> [    1.474898]  imx_rproc_probe+0x3e4/0x540
>> [    1.475274] mmc0: new HS400 MMC card at address 0001
>> [    1.478799]  platform_probe+0x68/0xc0
>> [    1.484628] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB
>> [    1.487400]  really_probe+0xc0/0x38c
>> [    1.487412]  __driver_probe_device+0x7c/0x15c
>> [    1.487424]  driver_probe_device+0x3c/0x10c
>> [    1.493941]  mmcblk0: p1 p2
>> [    1.495392]  __device_attach_driver+0xbc/0x158
>> [    1.495405]  bus_for_each_drv+0x84/0xe0
>> [    1.495417]  __device_attach+0x9c/0x1ac
>> [    1.500468] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB
>> [    1.503906]  device_initial_probe+0x14/0x20
>> [    1.503918]  bus_probe_device+0xac/0xb0
>> [    1.503929]  deferred_probe_work_func+0x9c/0xec
>> [    1.509863] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB
>> [    1.511117]  process_one_work+0x14c/0x28c
>> [    1.511132]  worker_thread+0x2cc/0x3d4
>> [    1.511142]  kthread+0x12c/0x208
>> [    1.511157]  ret_from_fork+0x10/0x20
>> [    1.517964] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (241:0)
>> [    1.518770] Code: 8b36c033 9100127c 54000924 d503201f (b9400261)
>> [    1.518777] ---[ end trace 0000000000000000 ]---
>> 
>> Currently I have the M4 partiton defined into the SCU code:
>> 
>>             /* Create partition */
>>             BRD_ERR(rm_partition_create(pt_boot, &pt_m4_0, SC_FALSE,
>>                 SC_TRUE, SC_FALSE, SC_TRUE, SC_FALSE, SC_R_M4_0_PID0,
>>                 rsrc_list, ARRAY_SIZE(rsrc_list),
>>                 pad_list, ARRAY_SIZE(pad_list),
>>                 NULL, 0));
>> 
>>             /* Name partition for debug */
>>             PARTITION_NAME(pt_m4_0, "MCU0");
>>             
>>             /* Allow AP to use SYSTEM (not production!) */
>>             BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_SYSTEM,
>>                 pt_boot, SC_RM_PERM_SEC_RW));
>> 
>>             /* Move M4 0 TCM */
>>             BRD_ERR(rm_find_memreg(pt_boot, &mr, 0x034FE0000ULL,
>>                 0x034FE0000ULL));
>>             BRD_ERR(rm_assign_memreg(pt_boot, pt_m4_0, mr));
>> 
>>             /* Move partition to be owned by SC */
>>             BRD_ERR(rm_set_parent(pt_boot, pt_m4_0, SC_PT));
>> 
>>             /* Check if booting with the no_ap flag set */
>>             if (no_ap != SC_FALSE)
>>             {
>>                 /* Move boot to be owned by M4 0 for Android Automotive */
>>                 BRD_ERR(rm_set_parent(SC_PT, pt_boot, pt_m4_0));
>>             }
>>         }
>> 
>>         /* Allow all to access the SEMA42s */
>>         BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_M4_0_SEMA42,
>>             SC_RM_PT_ALL, SC_RM_PERM_FULL));
>> 
>> I believe this SCU code is correct, at least this is more or less what
>> NXP provides as example, right?
>
>I tested the same SCU code and DTS overlay (removing the power-domains)
>with the current master branch of Linux and I got the same kernel panic.
>Maybe this part is already broken? Can you also test this on your side
>to check if this is currently working?

Does your M4 publish a resource table when M4 image built in flash.bin?

There is no common method to verify whether resource table is valid,
so if your M4 image not publish a resource table, there maybe garbage
data in it, so rproc_handle_resources may crash. NXP downstream
added a sanity check in lf-6.12 Q1 release, you may give a look.

Regards,
Peng

>
>> 
>> Best Regards,
>> Hiago.
>
>Best Regards,
>Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores
  2025-06-17  2:39             ` Peng Fan
@ 2025-06-17 15:56               ` Hiago De Franco
  0 siblings, 0 replies; 19+ messages in thread
From: Hiago De Franco @ 2025-06-17 15:56 UTC (permalink / raw)
  To: Peng Fan
  Cc: Peng Fan, Mathieu Poirier, Ulf Hansson, linux-pm@vger.kernel.org,
	linux-remoteproc@vger.kernel.org, Shawn Guo, Sascha Hauer,
	Bjorn Andersson, Hiago De Franco, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Daniel Baluta, Iuliana Prodan (OSS),
	Rafael J . Wysocki

On Tue, Jun 17, 2025 at 10:39:58AM +0800, Peng Fan wrote:
> On Mon, Jun 16, 2025 at 01:05:11PM -0300, Hiago De Franco wrote:
> >Hi Peng,
> >
> >On Thu, Jun 12, 2025 at 02:03:17PM -0300, Hiago De Franco wrote:
> >> Hi Peng,
> >> 
> >> On Wed, Jun 11, 2025 at 03:27:09AM +0000, Peng Fan wrote:
> >> > > 
> >> > > Sorry for the delay.
> >> > > 
> >> > > I tested it now and there must be something missing on my U-Boot:
> >> > > 
> >> > > Disable imx8x-cm4 rsrc 278 not owned
> >> > > Disable imx8x-cm4 rsrc 297 not owned
> >> > > 
> >> > > It removes my nodes from the DT before starting the kernel, so I cannot
> >> > > attach. Do you know what should I do in this case?
> >> > 
> >> > In separate partition case, UBoot will check the permission
> >> > by checking the rsrc-id, saying power domain id.
> >> > 
> >> > You may need to remove the power-domains property
> >> > from M4 node.
> >> 
> >> Without the power-domains property, rproc gives me a kernel panic:
> >> 
> >> [    1.253234] remoteproc remoteproc0: imx-rproc is available
> >> [    1.258501] remoteproc remoteproc0: attaching to imx-rproc
> >> [    1.263950] Unable to handle kernel paging request at virtual address ffff80005ae57d39
> >> [    1.271812] Mem abort info:
> >> [    1.274575]   ESR = 0x0000000096000005
> >> [    1.278299]   EC = 0x25: DABT (current EL), IL = 32 bits
> >> [    1.282581] mmc0: SDHCI controller on 5b010000.mmc [5b010000.mmc] using ADMA
> >> [    1.283607]   SET = 0, FnV = 0
> >> [    1.293701]   EA = 0, S1PTW = 0
> >> [    1.296815]   FSC = 0x05: level 1 translation fault
> >> [    1.301699] Data abort info:
> >> [    1.304545]   ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
> >> [    1.310079]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> >> [    1.315073]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> >> [    1.320367] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000096bf3000
> >> [    1.327061] [ffff80005ae57d39] pgd=0000000000000000, p4d=1000000097085003, pud=0000000000000000
> >> [    1.335750] Internal error: Oops: 0000000096000005 [#1]  SMP
> >> [    1.341373] Modules linked in:
> >> [    1.344414] CPU: 3 UID: 0 PID: 47 Comm: kworker/u16:3 Not tainted 6.16.0-rc1-00024-gfe5d6ab20714-dirty
> >>  #857 PREEMPT
> >> [    1.354932] Hardware name: Toradex Colibri iMX8QXP on Colibri Evaluation Board V3 (DT)
> >> [    1.362837] Workqueue: events_unbound deferred_probe_work_func
> >> [    1.368651] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >> [    1.375601] pc : rproc_handle_resources.constprop.0+0x78/0x1d0
> >> [    1.381421] lr : rproc_boot+0x368/0x578
> >> [    1.385242] sp : ffff8000819f3990
> >> [    1.388542] x29: ffff8000819f3990 x28: ffff80005ae57d3d x27: 0000000000000000
> >> [    1.395671] x26: 0000000000000000 x25: ffff0000016ee038 x24: ffff800080f3c680
> >> [    1.402793] x23: ffff8000813d6da8 x22: 00000000d999ad39 x21: ffff0000016ee000
> >> [    1.409917] x20: 00000000266656c3 x19: ffff80005ae57d39 x18: 0000000000000006
> >> [    1.417040] x17: ffff000002020600 x16: ffff000002020000 x15: 4addd15cca11c529
> >> [    1.424164] x14: 73ebceed5d6cd787 x13: 4addd15cca11c529 x12: 73ebceed5d6cd787
> >> [    1.431288] x11: 95a4e33b6b190664 x10: 9e3cdabdb09ca345 x9 : ab3734eafdd6fd1c
> >> [    1.438412] x8 : d58a055de4cfb385 x7 : de97fab1791acbbe x6 : 9946d97107d0dcda
> >> [    1.445535] x5 : ffff0000032b2c00 x4 : 00000000000003fc x3 : ffff0000032b2b80
> >> [    1.452659] x2 : fffffffffffffff0 x1 : ffff8000814bd000 x0 : ffff8000814bd000
> >> [    1.459786] Call trace:
> >> [    1.462215]  rproc_handle_resources.constprop.0+0x78/0x1d0 (P)
> >> [    1.468036]  rproc_boot+0x368/0x578
> >> [    1.471510]  rproc_add+0x180/0x18c
> >> [    1.474898]  imx_rproc_probe+0x3e4/0x540
> >> [    1.475274] mmc0: new HS400 MMC card at address 0001
> >> [    1.478799]  platform_probe+0x68/0xc0
> >> [    1.484628] mmcblk0: mmc0:0001 Q2J55L 7.09 GiB
> >> [    1.487400]  really_probe+0xc0/0x38c
> >> [    1.487412]  __driver_probe_device+0x7c/0x15c
> >> [    1.487424]  driver_probe_device+0x3c/0x10c
> >> [    1.493941]  mmcblk0: p1 p2
> >> [    1.495392]  __device_attach_driver+0xbc/0x158
> >> [    1.495405]  bus_for_each_drv+0x84/0xe0
> >> [    1.495417]  __device_attach+0x9c/0x1ac
> >> [    1.500468] mmcblk0boot0: mmc0:0001 Q2J55L 16.0 MiB
> >> [    1.503906]  device_initial_probe+0x14/0x20
> >> [    1.503918]  bus_probe_device+0xac/0xb0
> >> [    1.503929]  deferred_probe_work_func+0x9c/0xec
> >> [    1.509863] mmcblk0boot1: mmc0:0001 Q2J55L 16.0 MiB
> >> [    1.511117]  process_one_work+0x14c/0x28c
> >> [    1.511132]  worker_thread+0x2cc/0x3d4
> >> [    1.511142]  kthread+0x12c/0x208
> >> [    1.511157]  ret_from_fork+0x10/0x20
> >> [    1.517964] mmcblk0rpmb: mmc0:0001 Q2J55L 4.00 MiB, chardev (241:0)
> >> [    1.518770] Code: 8b36c033 9100127c 54000924 d503201f (b9400261)
> >> [    1.518777] ---[ end trace 0000000000000000 ]---
> >> 
> >> Currently I have the M4 partiton defined into the SCU code:
> >> 
> >>             /* Create partition */
> >>             BRD_ERR(rm_partition_create(pt_boot, &pt_m4_0, SC_FALSE,
> >>                 SC_TRUE, SC_FALSE, SC_TRUE, SC_FALSE, SC_R_M4_0_PID0,
> >>                 rsrc_list, ARRAY_SIZE(rsrc_list),
> >>                 pad_list, ARRAY_SIZE(pad_list),
> >>                 NULL, 0));
> >> 
> >>             /* Name partition for debug */
> >>             PARTITION_NAME(pt_m4_0, "MCU0");
> >>             
> >>             /* Allow AP to use SYSTEM (not production!) */
> >>             BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_SYSTEM,
> >>                 pt_boot, SC_RM_PERM_SEC_RW));
> >> 
> >>             /* Move M4 0 TCM */
> >>             BRD_ERR(rm_find_memreg(pt_boot, &mr, 0x034FE0000ULL,
> >>                 0x034FE0000ULL));
> >>             BRD_ERR(rm_assign_memreg(pt_boot, pt_m4_0, mr));
> >> 
> >>             /* Move partition to be owned by SC */
> >>             BRD_ERR(rm_set_parent(pt_boot, pt_m4_0, SC_PT));
> >> 
> >>             /* Check if booting with the no_ap flag set */
> >>             if (no_ap != SC_FALSE)
> >>             {
> >>                 /* Move boot to be owned by M4 0 for Android Automotive */
> >>                 BRD_ERR(rm_set_parent(SC_PT, pt_boot, pt_m4_0));
> >>             }
> >>         }
> >> 
> >>         /* Allow all to access the SEMA42s */
> >>         BRD_ERR(rm_set_peripheral_permissions(SC_PT, SC_R_M4_0_SEMA42,
> >>             SC_RM_PT_ALL, SC_RM_PERM_FULL));
> >> 
> >> I believe this SCU code is correct, at least this is more or less what
> >> NXP provides as example, right?
> >
> >I tested the same SCU code and DTS overlay (removing the power-domains)
> >with the current master branch of Linux and I got the same kernel panic.
> >Maybe this part is already broken? Can you also test this on your side
> >to check if this is currently working?
> 
> Does your M4 publish a resource table when M4 image built in flash.bin?
> 
> There is no common method to verify whether resource table is valid,
> so if your M4 image not publish a resource table, there maybe garbage
> data in it, so rproc_handle_resources may crash. NXP downstream
> added a sanity check in lf-6.12 Q1 release, you may give a look.

Thanks Peng, I think the hello_world does not have a resource table, I
tried with the ping-pong and it worked, the remote core attached
succesfully.

I will be preparing the v5 now with all the comments addressed.

Best Regards,
Hiago.


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2025-06-17 17:12 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-02 13:19 [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Hiago De Franco
2025-06-02 13:19 ` [PATCH v4 1/3] pmdomain: core: introduce dev_pm_genpd_is_on Hiago De Franco
2025-06-11 15:32   ` Bjorn Andersson
2025-06-12 17:31     ` Hiago De Franco
2025-06-16 13:13       ` Ulf Hansson
2025-06-02 13:19 ` [PATCH v4 2/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU Hiago De Franco
2025-06-02 13:19 ` [PATCH v4 3/3] remoteproc: imx_rproc: detect and attach to pre-booted remote cores Hiago De Franco
2025-06-04  3:19   ` Peng Fan
2025-06-05 13:25     ` Hiago De Franco
2025-06-09 17:31     ` Hiago De Franco
2025-06-11  3:27       ` Peng Fan
2025-06-12 17:03         ` Hiago De Franco
2025-06-16 16:05           ` Hiago De Franco
2025-06-17  2:39             ` Peng Fan
2025-06-17 15:56               ` Hiago De Franco
2025-06-11 15:36     ` Bjorn Andersson
2025-06-12 17:05       ` Hiago De Franco
2025-06-03 12:09 ` [PATCH v4 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader Ulf Hansson
2025-06-03 14:21   ` Hiago De Franco

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).