Re: [PATCH v3 21/24] pmdomain: core: Leave powered-on genpds on until late_initcall_sync

Re: [PATCH v3 21/24] pmdomain: core: Leave powered-on genpds on until late_initcall_sync

[Jon Hunter]

Hi Ulf,

On 10/07/2025 15:54, Ulf Hansson wrote:
> On Thu, 10 Jul 2025 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>
>> On 01.07.2025 13:47, Ulf Hansson wrote:
>>> 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 <saravanak@google.com>
>>> Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Colibri iMX8X
>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> 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/
>> 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.

Have you found any resolution for this? I have also noticed a boot 
regression on one of our Tegra210 boards and bisect is pointing to this 
commit. I don't see any particular crash, but a hang on boot.

If there is any debug we can enable to see which pmdomain is the problem 
let me know.

Thanks!
Jon

-- 
nvpublic

Re: [PATCH v3 21/24] pmdomain: core: Leave powered-on genpds on until late_initcall_sync

[Ulf Hansson]

On Tue, 15 Jul 2025 at 12:28, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Ulf,
>
> On 10/07/2025 15:54, Ulf Hansson wrote:
> > On Thu, 10 Jul 2025 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >>
> >> On 01.07.2025 13:47, Ulf Hansson wrote:
> >>> 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 <saravanak@google.com>
> >>> Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Colibri iMX8X
> >>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>
> >> 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/
> >> 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.
>
> Have you found any resolution for this? I have also noticed a boot
> regression on one of our Tegra210 boards and bisect is pointing to this
> commit. I don't see any particular crash, but a hang on boot.

Thanks for reporting!

For Exynos we opt-out from the behaviour by enforcing a sync_state of
all PM domains upfront [1], which means before any devices get
attached.

Even if that defeats the purpose of the $subject series, this was one
way forward that solved the problem. When the boot-ordering problem
(that's how I understood the issue) for Exynos gets resolved, we
should be able to drop the hack, at least that's the idea.

>
> If there is any debug we can enable to see which pmdomain is the problem
> let me know.

There aren't many debug prints in genpd that I think makes much sense
to enable, but you can always give it a try. Since you are hanging,
obviously you can't look at the genpd debugfs data...

Note that, the interesting PM domains are those that are powered-on
when calling pm_genpd_init(). As a start, I would add some debug
prints in () to see which PM domains that are relevant to track.
Potentially you could then try to power them off and register them
accordingly with genpd. One by one, to see which of them is causing
the problem.

Another option could be to add a new genpd config flag
(GENPD_FLAG_DONT_STAY_ON or something along those lines), that informs
genpd to not set the genpd->stay_on in pm_genpd_init(). Then
tegra_powergate_add() would have to set GENPD_FLAG_DONT_STAY_ON for
those genpds that really need it.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/20250711114719.189441-1-ulf.hansson@linaro.org/

Re: [PATCH v3 21/24] pmdomain: core: Leave powered-on genpds on until late_initcall_sync

[Ulf Hansson]

On Tue, 15 Jul 2025 at 13:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 15 Jul 2025 at 12:28, Jon Hunter <jonathanh@nvidia.com> wrote:
> >
> > Hi Ulf,
> >
> > On 10/07/2025 15:54, Ulf Hansson wrote:
> > > On Thu, 10 Jul 2025 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > >>
> > >> On 01.07.2025 13:47, Ulf Hansson wrote:
> > >>> 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 <saravanak@google.com>
> > >>> Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Colibri iMX8X
> > >>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
> > >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > >>
> > >> 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/
> > >> 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.
> >
> > Have you found any resolution for this? I have also noticed a boot
> > regression on one of our Tegra210 boards and bisect is pointing to this
> > commit. I don't see any particular crash, but a hang on boot.
>
> Thanks for reporting!
>
> For Exynos we opt-out from the behaviour by enforcing a sync_state of
> all PM domains upfront [1], which means before any devices get
> attached.
>
> Even if that defeats the purpose of the $subject series, this was one
> way forward that solved the problem. When the boot-ordering problem
> (that's how I understood the issue) for Exynos gets resolved, we
> should be able to drop the hack, at least that's the idea.
>
> >
> > If there is any debug we can enable to see which pmdomain is the problem
> > let me know.
>
> There aren't many debug prints in genpd that I think makes much sense
> to enable, but you can always give it a try. Since you are hanging,
> obviously you can't look at the genpd debugfs data...
>
> Note that, the interesting PM domains are those that are powered-on
> when calling pm_genpd_init(). As a start, I would add some debug
> prints in () to see which PM domains that are relevant to track.

/s/()/tegra_powergate_add()

> Potentially you could then try to power them off and register them
> accordingly with genpd. One by one, to see which of them is causing
> the problem.
>
> Another option could be to add a new genpd config flag
> (GENPD_FLAG_DONT_STAY_ON or something along those lines), that informs
> genpd to not set the genpd->stay_on in pm_genpd_init(). Then
> tegra_powergate_add() would have to set GENPD_FLAG_DONT_STAY_ON for
> those genpds that really need it.
>
> Kind regards
> Uffe
>
> [1]
> https://lore.kernel.org/all/20250711114719.189441-1-ulf.hansson@linaro.org/

Re: [PATCH v3 21/24] pmdomain: core: Leave powered-on genpds on until late_initcall_sync

[Jon Hunter]

On 15/07/2025 12:34, Ulf Hansson wrote:

...

>>> Have you found any resolution for this? I have also noticed a boot
>>> regression on one of our Tegra210 boards and bisect is pointing to this
>>> commit. I don't see any particular crash, but a hang on boot.
>>
>> Thanks for reporting!
>>
>> For Exynos we opt-out from the behaviour by enforcing a sync_state of
>> all PM domains upfront [1], which means before any devices get
>> attached.
>>
>> Even if that defeats the purpose of the $subject series, this was one
>> way forward that solved the problem. When the boot-ordering problem
>> (that's how I understood the issue) for Exynos gets resolved, we
>> should be able to drop the hack, at least that's the idea.
>>
>>>
>>> If there is any debug we can enable to see which pmdomain is the problem
>>> let me know.
>>
>> There aren't many debug prints in genpd that I think makes much sense
>> to enable, but you can always give it a try. Since you are hanging,
>> obviously you can't look at the genpd debugfs data...
>>
>> Note that, the interesting PM domains are those that are powered-on
>> when calling pm_genpd_init(). As a start, I would add some debug
>> prints in () to see which PM domains that are relevant to track.
> 
> /s/()/tegra_powergate_add()


I have been able to track this down to a problem in the Tegra PMC driver 
where we are registering the power-domains and I have sent a fix [0]. 
Looks like we have been getting lucky up until now.

Cheers!
Jon

[0] 
https://lore.kernel.org/linux-tegra/20250731121832.213671-1-jonathanh@nvidia.com/T/#u

-- 
nvpublic

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Jon Hunter]

Hi Ulf,

On 01/07/2025 12:47, Ulf Hansson wrote:
> Unless the typical platform driver that act as genpd provider, has its own
> ->sync_state() callback implemented let's default to use
> of_genpd_sync_state().
> 
> More precisely, while adding a genpd OF provider let's assign the
> ->sync_state() callback, in case the fwnode has a device and its driver
> doesn't have the ->sync_state() set already. In this way the typical
> platform driver doesn't need to assign ->sync_state(), unless it has some
> additional things to manage beyond genpds.
> 
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Colibri iMX8X
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/pmdomain/core.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index ca47f91b9e91..5cef6de60c72 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2600,6 +2600,11 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
>   	return ret;
>   }
>   
> +static void genpd_sync_state(struct device *dev)
> +{
> +	return of_genpd_sync_state(dev->of_node);
> +}
> +
>   /**
>    * of_genpd_add_provider_simple() - Register a simple PM domain provider
>    * @np: Device node pointer associated with the PM domain provider.
> @@ -2628,6 +2633,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
>   	if (!dev && !genpd_is_no_sync_state(genpd)) {
>   		genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
>   		device_set_node(&genpd->dev, fwnode);
> +	} else {
> +		dev_set_drv_sync_state(dev, genpd_sync_state);
>   	}
>   
>   	put_device(dev);
> @@ -2700,6 +2707,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>   	dev = get_dev_from_fwnode(fwnode);
>   	if (!dev)
>   		sync_state = true;
> +	else
> +		dev_set_drv_sync_state(dev, genpd_sync_state);
>   
>   	put_device(dev);
>   

Following this change I am seeing the following warning on our Tegra194
devices ...

  WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 17000000.gpu
  WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
  WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 15380000.nvjpg
  WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 154c0000.nvenc
  WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 15a80000.nvenc

Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
and so should Tegra be using of_genpd_sync_state() by default?

Thanks
Jon

[0] https://lore.kernel.org/all/20250701114733.636510-10-ulf.hansson@linaro.org/
-- 
nvpublic

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Ulf Hansson]

On Thu, 31 Jul 2025 at 17:07, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Ulf,
>
> On 01/07/2025 12:47, Ulf Hansson wrote:
> > Unless the typical platform driver that act as genpd provider, has its own
> > ->sync_state() callback implemented let's default to use
> > of_genpd_sync_state().
> >
> > More precisely, while adding a genpd OF provider let's assign the
> > ->sync_state() callback, in case the fwnode has a device and its driver
> > doesn't have the ->sync_state() set already. In this way the typical
> > platform driver doesn't need to assign ->sync_state(), unless it has some
> > additional things to manage beyond genpds.
> >
> > Suggested-by: Saravana Kannan <saravanak@google.com>
> > Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Colibri iMX8X
> > Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >   drivers/pmdomain/core.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index ca47f91b9e91..5cef6de60c72 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -2600,6 +2600,11 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
> >       return ret;
> >   }
> >
> > +static void genpd_sync_state(struct device *dev)
> > +{
> > +     return of_genpd_sync_state(dev->of_node);
> > +}
> > +
> >   /**
> >    * of_genpd_add_provider_simple() - Register a simple PM domain provider
> >    * @np: Device node pointer associated with the PM domain provider.
> > @@ -2628,6 +2633,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
> >       if (!dev && !genpd_is_no_sync_state(genpd)) {
> >               genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
> >               device_set_node(&genpd->dev, fwnode);
> > +     } else {
> > +             dev_set_drv_sync_state(dev, genpd_sync_state);
> >       }
> >
> >       put_device(dev);
> > @@ -2700,6 +2707,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> >       dev = get_dev_from_fwnode(fwnode);
> >       if (!dev)
> >               sync_state = true;
> > +     else
> > +             dev_set_drv_sync_state(dev, genpd_sync_state);
> >
> >       put_device(dev);
> >
>
> Following this change I am seeing the following warning on our Tegra194
> devices ...
>
>   WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 17000000.gpu
>   WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
>   WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 15380000.nvjpg
>   WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 154c0000.nvenc
>   WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 15a80000.nvenc
>
> Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
> and so should Tegra be using of_genpd_sync_state() by default?

This is a different power-domain provider (bpmp) in
drivers/firmware/tegra/bpmp.c and
drivers/pmdomain/tegra/powergate-bpmp.c.

For the bpmp we don't need GENPD_FLAG_NO_SYNC_STATE, as the
power-domain provider is described along with the
"nvidia,tegra186-bpmp" compatible string. In the other case
(drivers/soc/tegra/pmc.c) the "core-domain" and "powergates" are
described through child-nodes, while ->sync_state() is managed by the
parent-device-node.

In the bpmp case there is no ->sync_state() callback assigned, which
means genpd decides to assign a default one.

The reason for the warnings above is because we are still waiting for
those devices to be probed, hence the ->sync_state() callback is still
waiting to be invoked. Enforcing ->sync_state() callback to be invoked
can be done via user-space if that is needed.

Did that make sense?

>
> Thanks
> Jon
>
> [0] https://lore.kernel.org/all/20250701114733.636510-10-ulf.hansson@linaro.org/
> --
> nvpublic
>

Kind regards
Uffe

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Jon Hunter]

Hi Ulf,

On 11/08/2025 13:11, Ulf Hansson wrote:
> On Thu, 31 Jul 2025 at 17:07, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Hi Ulf,
>>
>> On 01/07/2025 12:47, Ulf Hansson wrote:
>>> Unless the typical platform driver that act as genpd provider, has its own
>>> ->sync_state() callback implemented let's default to use
>>> of_genpd_sync_state().
>>>
>>> More precisely, while adding a genpd OF provider let's assign the
>>> ->sync_state() callback, in case the fwnode has a device and its driver
>>> doesn't have the ->sync_state() set already. In this way the typical
>>> platform driver doesn't need to assign ->sync_state(), unless it has some
>>> additional things to manage beyond genpds.
>>>
>>> Suggested-by: Saravana Kannan <saravanak@google.com>
>>> Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Colibri iMX8X
>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # TI AM62A,Xilinx ZynqMP ZCU106
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>    drivers/pmdomain/core.c | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>> index ca47f91b9e91..5cef6de60c72 100644
>>> --- a/drivers/pmdomain/core.c
>>> +++ b/drivers/pmdomain/core.c
>>> @@ -2600,6 +2600,11 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
>>>        return ret;
>>>    }
>>>
>>> +static void genpd_sync_state(struct device *dev)
>>> +{
>>> +     return of_genpd_sync_state(dev->of_node);
>>> +}
>>> +
>>>    /**
>>>     * of_genpd_add_provider_simple() - Register a simple PM domain provider
>>>     * @np: Device node pointer associated with the PM domain provider.
>>> @@ -2628,6 +2633,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>>        if (!dev && !genpd_is_no_sync_state(genpd)) {
>>>                genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
>>>                device_set_node(&genpd->dev, fwnode);
>>> +     } else {
>>> +             dev_set_drv_sync_state(dev, genpd_sync_state);
>>>        }
>>>
>>>        put_device(dev);
>>> @@ -2700,6 +2707,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>>>        dev = get_dev_from_fwnode(fwnode);
>>>        if (!dev)
>>>                sync_state = true;
>>> +     else
>>> +             dev_set_drv_sync_state(dev, genpd_sync_state);
>>>
>>>        put_device(dev);
>>>
>>
>> Following this change I am seeing the following warning on our Tegra194
>> devices ...
>>
>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 17000000.gpu
>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 15380000.nvjpg
>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 154c0000.nvenc
>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 15a80000.nvenc
>>
>> Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
>> and so should Tegra be using of_genpd_sync_state() by default?
> 
> This is a different power-domain provider (bpmp) in
> drivers/firmware/tegra/bpmp.c and
> drivers/pmdomain/tegra/powergate-bpmp.c.
> 
> For the bpmp we don't need GENPD_FLAG_NO_SYNC_STATE, as the
> power-domain provider is described along with the
> "nvidia,tegra186-bpmp" compatible string. In the other case
> (drivers/soc/tegra/pmc.c) the "core-domain" and "powergates" are
> described through child-nodes, while ->sync_state() is managed by the
> parent-device-node.
> 
> In the bpmp case there is no ->sync_state() callback assigned, which
> means genpd decides to assign a default one.
> 
> The reason for the warnings above is because we are still waiting for
> those devices to be probed, hence the ->sync_state() callback is still
> waiting to be invoked. Enforcing ->sync_state() callback to be invoked
> can be done via user-space if that is needed.
> 
> Did that make sense?

Sorry for the delay, I was on vacation. Yes makes sense and drivers for 
some of the above drivers are not yet upstreamed to mainline and so this 
would be expected for now.

Thanks
Jon

-- 
nvpublic

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Jon Hunter]

Hi Ulf,

On 03/09/2025 13:33, Jon Hunter wrote:

...

>>> Following this change I am seeing the following warning on our Tegra194
>>> devices ...
>>>
>>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 
>>> 17000000.gpu
>>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
>>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 
>>> 15380000.nvjpg
>>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 
>>> 154c0000.nvenc
>>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 
>>> 15a80000.nvenc
>>>
>>> Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
>>> and so should Tegra be using of_genpd_sync_state() by default?
>>
>> This is a different power-domain provider (bpmp) in
>> drivers/firmware/tegra/bpmp.c and
>> drivers/pmdomain/tegra/powergate-bpmp.c.
>>
>> For the bpmp we don't need GENPD_FLAG_NO_SYNC_STATE, as the
>> power-domain provider is described along with the
>> "nvidia,tegra186-bpmp" compatible string. In the other case
>> (drivers/soc/tegra/pmc.c) the "core-domain" and "powergates" are
>> described through child-nodes, while ->sync_state() is managed by the
>> parent-device-node.
>>
>> In the bpmp case there is no ->sync_state() callback assigned, which
>> means genpd decides to assign a default one.
>>
>> The reason for the warnings above is because we are still waiting for
>> those devices to be probed, hence the ->sync_state() callback is still
>> waiting to be invoked. Enforcing ->sync_state() callback to be invoked
>> can be done via user-space if that is needed.
>>
>> Did that make sense?
> 
> Sorry for the delay, I was on vacation. Yes makes sense and drivers for 
> some of the above drivers are not yet upstreamed to mainline and so this 
> would be expected for now.


I have been doing more testing and do see a lot of "tegra-bpmp bpmp: 
sync_state() pending due to" on our platforms for basically are driver 
that is built as a module. It seems a bit noisy given that these do 
eventually probe OK. I am wondering if this should be more of a 
dev_info() or dev_dbg() print?

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Ulf Hansson]

On Wed, 24 Sept 2025 at 13:41, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Ulf,
>
> On 03/09/2025 13:33, Jon Hunter wrote:
>
> ...
>
> >>> Following this change I am seeing the following warning on our Tegra194
> >>> devices ...
> >>>
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 17000000.gpu
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 15380000.nvjpg
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 154c0000.nvenc
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 15a80000.nvenc
> >>>
> >>> Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
> >>> and so should Tegra be using of_genpd_sync_state() by default?
> >>
> >> This is a different power-domain provider (bpmp) in
> >> drivers/firmware/tegra/bpmp.c and
> >> drivers/pmdomain/tegra/powergate-bpmp.c.
> >>
> >> For the bpmp we don't need GENPD_FLAG_NO_SYNC_STATE, as the
> >> power-domain provider is described along with the
> >> "nvidia,tegra186-bpmp" compatible string. In the other case
> >> (drivers/soc/tegra/pmc.c) the "core-domain" and "powergates" are
> >> described through child-nodes, while ->sync_state() is managed by the
> >> parent-device-node.
> >>
> >> In the bpmp case there is no ->sync_state() callback assigned, which
> >> means genpd decides to assign a default one.
> >>
> >> The reason for the warnings above is because we are still waiting for
> >> those devices to be probed, hence the ->sync_state() callback is still
> >> waiting to be invoked. Enforcing ->sync_state() callback to be invoked
> >> can be done via user-space if that is needed.
> >>
> >> Did that make sense?
> >
> > Sorry for the delay, I was on vacation. Yes makes sense and drivers for
> > some of the above drivers are not yet upstreamed to mainline and so this
> > would be expected for now.
>
>
> I have been doing more testing and do see a lot of "tegra-bpmp bpmp:
> sync_state() pending due to" on our platforms for basically are driver
> that is built as a module. It seems a bit noisy given that these do
> eventually probe OK. I am wondering if this should be more of a
> dev_info() or dev_dbg() print?

Yes, I agree. We have had similar reports for other platforms too.

I intend to send a patch for this in the next day or so.

Kind regards
Uffe

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Jon Hunter]

On 24/09/2025 16:53, Ulf Hansson wrote:
> On Wed, 24 Sept 2025 at 13:41, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Hi Ulf,
>>
>> On 03/09/2025 13:33, Jon Hunter wrote:
>>
>> ...
>>
>>>>> Following this change I am seeing the following warning on our Tegra194
>>>>> devices ...
>>>>>
>>>>>     WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
>>>>> 17000000.gpu
>>>>>     WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
>>>>>     WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
>>>>> 15380000.nvjpg
>>>>>     WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
>>>>> 154c0000.nvenc
>>>>>     WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
>>>>> 15a80000.nvenc
>>>>>
>>>>> Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
>>>>> and so should Tegra be using of_genpd_sync_state() by default?
>>>>
>>>> This is a different power-domain provider (bpmp) in
>>>> drivers/firmware/tegra/bpmp.c and
>>>> drivers/pmdomain/tegra/powergate-bpmp.c.
>>>>
>>>> For the bpmp we don't need GENPD_FLAG_NO_SYNC_STATE, as the
>>>> power-domain provider is described along with the
>>>> "nvidia,tegra186-bpmp" compatible string. In the other case
>>>> (drivers/soc/tegra/pmc.c) the "core-domain" and "powergates" are
>>>> described through child-nodes, while ->sync_state() is managed by the
>>>> parent-device-node.
>>>>
>>>> In the bpmp case there is no ->sync_state() callback assigned, which
>>>> means genpd decides to assign a default one.
>>>>
>>>> The reason for the warnings above is because we are still waiting for
>>>> those devices to be probed, hence the ->sync_state() callback is still
>>>> waiting to be invoked. Enforcing ->sync_state() callback to be invoked
>>>> can be done via user-space if that is needed.
>>>>
>>>> Did that make sense?
>>>
>>> Sorry for the delay, I was on vacation. Yes makes sense and drivers for
>>> some of the above drivers are not yet upstreamed to mainline and so this
>>> would be expected for now.
>>
>>
>> I have been doing more testing and do see a lot of "tegra-bpmp bpmp:
>> sync_state() pending due to" on our platforms for basically are driver
>> that is built as a module. It seems a bit noisy given that these do
>> eventually probe OK. I am wondering if this should be more of a
>> dev_info() or dev_dbg() print?
> 
> Yes, I agree. We have had similar reports for other platforms too.
> 
> I intend to send a patch for this in the next day or so.

OK great! Please CC me and I will be happy to test.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Saravana Kannan]

On Wed, Sep 24, 2025 at 4:41 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Ulf,
>
> On 03/09/2025 13:33, Jon Hunter wrote:
>
> ...
>
> >>> Following this change I am seeing the following warning on our Tegra194
> >>> devices ...
> >>>
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 17000000.gpu
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to 3960000.cec
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 15380000.nvjpg
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 154c0000.nvenc
> >>>    WARNING KERN tegra-bpmp bpmp: sync_state() pending due to
> >>> 15a80000.nvenc
> >>>
> >>> Per your change [0], the 'GENPD_FLAG_NO_SYNC_STATE' is set for Tegra
> >>> and so should Tegra be using of_genpd_sync_state() by default?
> >>
> >> This is a different power-domain provider (bpmp) in
> >> drivers/firmware/tegra/bpmp.c and
> >> drivers/pmdomain/tegra/powergate-bpmp.c.
> >>
> >> For the bpmp we don't need GENPD_FLAG_NO_SYNC_STATE, as the
> >> power-domain provider is described along with the
> >> "nvidia,tegra186-bpmp" compatible string. In the other case
> >> (drivers/soc/tegra/pmc.c) the "core-domain" and "powergates" are
> >> described through child-nodes, while ->sync_state() is managed by the
> >> parent-device-node.
> >>
> >> In the bpmp case there is no ->sync_state() callback assigned, which
> >> means genpd decides to assign a default one.
> >>
> >> The reason for the warnings above is because we are still waiting for
> >> those devices to be probed, hence the ->sync_state() callback is still
> >> waiting to be invoked. Enforcing ->sync_state() callback to be invoked
> >> can be done via user-space if that is needed.
> >>
> >> Did that make sense?
> >
> > Sorry for the delay, I was on vacation. Yes makes sense and drivers for
> > some of the above drivers are not yet upstreamed to mainline and so this
> > would be expected for now.
>
>
> I have been doing more testing and do see a lot of "tegra-bpmp bpmp:
> sync_state() pending due to" on our platforms for basically are driver
> that is built as a module.

It being "built as a module" is not reason enough for this warning to
happen though. One of the main points of fw_devlink is for things to
work just as well with modules.

In this particular system, do you never plan to load the modules? Or
is the module load just missing this timeout by a few seconds or
something?

If these can be turned off, why not turn these off using the sysfs
file or the timeout commandline option to turn them off? You are
burning power by leaving these on. A warning seems appropriate to me.

-Saravana

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Jon Hunter]

On 25/09/2025 23:31, Saravana Kannan wrote:

...

>> I have been doing more testing and do see a lot of "tegra-bpmp bpmp:
>> sync_state() pending due to" on our platforms for basically are driver
>> that is built as a module.
> 
> It being "built as a module" is not reason enough for this warning to
> happen though. One of the main points of fw_devlink is for things to
> work just as well with modules.
> 
> In this particular system, do you never plan to load the modules? Or
> is the module load just missing this timeout by a few seconds or
> something?

We absolutely do load the drivers. Initially, I observed cases where 
drivers are missing, but doing more testing with the necessary drivers 
present, I still see such messages. A lot of our test infrastructure is 
set up to use NFS for mounting to the rootfs and so I am wondering if 
that can also be a factor?

> If these can be turned off, why not turn these off using the sysfs
> file or the timeout commandline option to turn them off? You are
> burning power by leaving these on. A warning seems appropriate to me.

Again the drivers get loaded, so that shouldn't be the case.

Jon

-- 
nvpublic

Re: [PATCH v3 20/24] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Ulf Hansson]

On Fri, 26 Sept 2025 at 17:32, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 25/09/2025 23:31, Saravana Kannan wrote:
>
> ...
>
> >> I have been doing more testing and do see a lot of "tegra-bpmp bpmp:
> >> sync_state() pending due to" on our platforms for basically are driver
> >> that is built as a module.
> >
> > It being "built as a module" is not reason enough for this warning to
> > happen though. One of the main points of fw_devlink is for things to
> > work just as well with modules.
> >
> > In this particular system, do you never plan to load the modules? Or
> > is the module load just missing this timeout by a few seconds or
> > something?
>
> We absolutely do load the drivers. Initially, I observed cases where
> drivers are missing, but doing more testing with the necessary drivers
> present, I still see such messages. A lot of our test infrastructure is
> set up to use NFS for mounting to the rootfs and so I am wondering if
> that can also be a factor?
>
> > If these can be turned off, why not turn these off using the sysfs
> > file or the timeout commandline option to turn them off? You are
> > burning power by leaving these on. A warning seems appropriate to me.
>
> Again the drivers get loaded, so that shouldn't be the case.
>

I realized that we also have an orthogonal, but related problem.

In fact, the genpd in question actually may already have been
powered-off, completely defeating the purpose of the printed warning
that we are discussing.

In genpd we enable the ->sync_state() support, no matter whether the
genpd is powered-on or powered-off at initialization. Although, it's
only when a genpd is powered-on at init, when we prevent it from being
powered-off until the ->sync_state() callback gets invoked. Ideally,
we should be able to skip enabling the ->sync_state() support for
genpd in these cases, possibly silencing some of these prints. I will
have a look at this and get back to you.

Kind regards
Uffe