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);
--
2.43.0Hi 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
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.