Message-ID: <CAPDyKFppte-Rh-xNybEz8dshrD2gfqJHUpiL1bPWzZ6pBig6+g@mail.gmail.com>
Date: Thu, 23 Oct 2025 15:46:54 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Marek Szyprowski <m.szyprowski@...sung.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>, 
	André Draszik <andre.draszik@...aro.org>, 
	Alim Akhtar <alim.akhtar@...sung.com>, Conor Dooley <conor+dt@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Peter Griffin <peter.griffin@...aro.org>, 
	Tudor Ambarus <tudor.ambarus@...aro.org>, Will McVicker <willmcvicker@...gle.com>, 
	kernel-team@...roid.com, linux-arm-kernel@...ts.infradead.org, 
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 08/10] pmdomain: samsung: selectively handle enforced sync_state

On Thu, 23 Oct 2025 at 14:17, Marek Szyprowski <m.szyprowski@...sung.com> wrote:
>
> On 23.10.2025 12:02, Ulf Hansson wrote:
> > On Wed, 22 Oct 2025 at 20:39, Marek Szyprowski <m.szyprowski@...sung.com> wrote:
> >> On 22.10.2025 13:06, Ulf Hansson wrote:
> >>> On Thu, 16 Oct 2025 at 17:58, André Draszik <andre.draszik@...aro.org> wrote:
> >>>> Unconditionally calling of_genpd_sync_state() causes issues on
> >>>> platforms with child domains as the parent domain will be turned off
> >>>> before the child domain was even registered during boot.
> >>>>
> >>>> This in particular is an issue for the upcoming Google gs101 support -
> >>>> all operations on child domains registered after the parent domain
> >>>> misbehave.
> >>>>
> >>>> Add a flag to the probe data to be able to sync_state conditionally
> >>>> only, and enable that flag on the two platforms currently supported by
> >>>> this driver.
> >>>>
> >>>> Signed-off-by: André Draszik <andre.draszik@...aro.org>
> >>>>
> >>>> ---
> >>>> v2:
> >>>> * use bool for need_early_sync_state (Krzysztof)
> >>>> ---
> >>>>    drivers/pmdomain/samsung/exynos-pm-domains.c | 5 ++++-
> >>>>    1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c b/drivers/pmdomain/samsung/exynos-pm-domains.c
> >>>> index 638d286b57f716140b2401092415644a6805870e..15a1582aa92103a07335eb681600d9415369fefd 100644
> >>>> --- a/drivers/pmdomain/samsung/exynos-pm-domains.c
> >>>> +++ b/drivers/pmdomain/samsung/exynos-pm-domains.c
> >>>> @@ -20,6 +20,7 @@
> >>>>    struct exynos_pm_domain_config {
> >>>>           /* Value for LOCAL_PWR_CFG and STATUS fields for each domain */
> >>>>           u32 local_pwr_cfg;
> >>>> +       bool need_early_sync_state;
> >>>>    };
> >>>>
> >>>>    /*
> >>>> @@ -69,10 +70,12 @@ static int exynos_pd_power_off(struct generic_pm_domain *domain)
> >>>>
> >>>>    static const struct exynos_pm_domain_config exynos4210_cfg = {
> >>>>           .local_pwr_cfg          = 0x7,
> >>>> +       .need_early_sync_state  = true,
> >>>>    };
> >>>>
> >>>>    static const struct exynos_pm_domain_config exynos5433_cfg = {
> >>>>           .local_pwr_cfg          = 0xf,
> >>>> +       .need_early_sync_state  = true,
> >>>>    };
> >>>>
> >>>>    static const struct of_device_id exynos_pm_domain_of_match[] = {
> >>>> @@ -179,7 +182,7 @@ static int exynos_pd_probe(struct platform_device *pdev)
> >>>>            * reset during boot. As a temporary hack to manage this, let's enforce
> >>>>            * a sync_state.
> >>>>            */
> >>>> -       if (!ret)
> >>>> +       if (pm_domain_cfg->need_early_sync_state && !ret)
> >>>>                   of_genpd_sync_state(np);
> >>> The call to of_genpd_sync_state() was intended as a temporary solution here.
> >>>
> >>> Potentially, if we would be able to distinguish what PM domain that is
> >>> causing the problem on the Exynos platforms, we could set
> >>> GENPD_FLAG_NO_STAY_ON for that genpd instead.
> >> Well, this of_genpd_sync_state() "workaround" has to be applied only to
> >> the power domain of the display controller device. It can be replaced by
> >> the following check on the legacy Exynos systems:
> >>
> >> if (IS_ENABLED(CONFIG_ARM) &&
> >> of_device_is_compatible(np, "samsung,exynos4210-pd") &&
> >> (strstr(pd->pd.name, "LCD") || strstr(pd->pd.name, "DISP")))
> >> pd->pd.flags = GENPD_FLAG_NO_STAY_ON;
> > Oh wait, perhaps better to just power-off these PM domains before
> > calling pm_genpd_init(), if that can be done safely?
> >
> > At least that would guarantee the reset to happen before the display
> > driver gets probed. Instead of relying on genpd_power_off_unused()
> > (late_initcall_sync) to do it.
>
> Well, yes, this works too:
>
> if ((of_device_is_compatible(np, "samsung,exynos4210-pd") &&
>      (strstr(pd->pd.name, "LCD") || strstr(pd->pd.name, "DISP"))))
>           exynos_pd_power_off(&pd->pd);

Okay!

Should we (you or me?) send a patch and change the code like this?

>
> >> I assume that this information cannot be coded in device tree to make it
> >> somehow generic...
> > Right, in principle we would need a new DT property for a power-domain
> > provider, like "broken-hw-reset", because we don't have a reset-line
> > to pull.
>
> It is not a matter of broken reset at all. It is a matter of software
> configuration and the lack of 'protocol' to pass the information that
> the display controller is configured to display splash screen from the
> system memory at given address and newly instantiated drivers must to be
> aware of that.
>
> Turning display-related power domain off simply resets all that
> configuration, so drivers can start from good known 'unconfigured' state.

Alright, thanks for clarifying! Yeah, a DT property would be wrong for this.

In fact, there are some drivers [1] that make use of the genpd on/off
notifiers to manage a reset of its device's PM domain. It works, but
it's a bit messy. Perhaps it could be simplified with some new helper
functions, if more consumers drivers need something similar.

Kind regards
Uffe

[1]
commit c11fa1204fe9 ("drm/msm/a6xx: Use genpd notifier to ensure
cx-gdsc collapse")

Powered by blists - more mailing lists