On Thu, 10 Jul 2025 at 14:26, Marek Szyprowski [off-list ref] wrote:

On 01.07.2025 13:47, Ulf Hansson wrote:

quoted

Powering-off a genpd that was on during boot, before all of its consumer
devices have been probed, is certainly prone to problems.

As a step to improve this situation, let's prevent these genpds from being
powered-off until genpd_power_off_unused() gets called, which is a
late_initcall_sync().

Note that, this still doesn't guarantee that all the consumer devices has
been probed before we allow to power-off the genpds. Yet, this should be a
step in the right direction.

Suggested-by: Saravana Kannan <redacted>
Tested-by: Hiago De Franco <redacted> # Colibri iMX8X
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
Signed-off-by: Ulf Hansson <redacted>

This change has a side effect on some Exynos based boards, which have
display and bootloader is configured to setup a splash screen on it.
Since today's linux-next, those boards fails to boot, because of the
IOMMU page fault.

Thanks for reporting, let's try to fix this as soon as possible then.

This happens because the display controller is enabled and configured to
perform the scanout from the spash-screen buffer until the respective
driver will reset it in driver probe() function. This however doesn't
work with IOMMU, which is being probed earlier than the display
controller driver, what in turn causes IOMMU page fault once the IOMMU
driver gets attached. This worked before applying this patch, because
the power domain of display controller was simply turned off early
effectively reseting the display controller.

I can certainly try to help to find a solution, but I believe I need
some more details of what is happening.

Perhaps you can point me to some relevant DTS file to start with?

This has been discussed a bit recently:
https://lore.kernel.org/all/544ad69cba52a9b87447e3ac1c7fa8c3@disroot.org/ (local)
and I can add a workaround for this issue in the bootloaders of those
boards, but this is something that has to be somehow addressed in a
generic way.

It kind of sounds like there is a missing power-domain not being
described in DT for the IOMMU, but I might have understood the whole
thing wrong.

Let's see if we can work something out in the next few days, otherwise
we need to find another way to let some genpds for these platforms to
opt out from this new behaviour.

Kind regards
Uffe

quoted

---
  drivers/pmdomain/core.c   | 10 ++++++++--
  include/linux/pm_domain.h |  1 +
  2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 5cef6de60c72..18951ed6295d 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c

@@ -931,11 +931,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
       * The domain is already in the "power off" state.
       * System suspend is in progress.
       * The domain is configured as always on.
+      * The domain was on at boot and still need to stay on.
       * The domain has a subdomain being powered on.
       */
      if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
          genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
-         atomic_read(&genpd->sd_count) > 0)
+         genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
              return;

      /*

@@ -1346,8 +1347,12 @@ static int __init genpd_power_off_unused(void)
      pr_info("genpd: Disabling unused power domains\n");
      mutex_lock(&gpd_list_lock);

-     list_for_each_entry(genpd, &gpd_list, gpd_list_node)
+     list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+             genpd_lock(genpd);
+             genpd->stay_on = false;
+             genpd_unlock(genpd);
              genpd_queue_power_off_work(genpd);
+     }

      mutex_unlock(&gpd_list_lock);

@@ -2352,6 +2357,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
      INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
      atomic_set(&genpd->sd_count, 0);
      genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
+     genpd->stay_on = !is_off;
      genpd->sync_state = GENPD_SYNC_STATE_OFF;
      genpd->device_count = 0;
      genpd->provider = NULL;

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index d68e07dadc99..99556589f45e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h

@@ -199,6 +199,7 @@ struct generic_pm_domain {
      unsigned int performance_state; /* Aggregated max performance state */
      cpumask_var_t cpus;             /* A cpumask of the attached CPUs */
      bool synced_poweroff;           /* A consumer needs a synced poweroff */
+     bool stay_on;                   /* Stay powered-on during boot. */
      enum genpd_sync_state sync_state; /* How sync_state is managed. */
      int (*power_off)(struct generic_pm_domain *domain);
      int (*power_on)(struct generic_pm_domain *domain);

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland