drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
From: Vitor Soares <vitor.soares@toradex.com>
The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
for both runtime PM and system sleep. This causes the DSI clocks to be
disabled twice: once during runtime suspend and again during system
suspend, resulting in a WARN message from the clock framework when
attempting to disable already-disabled clocks.
[ 84.384540] clk:231:5 already disabled
[ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181 clk_core_disable+0xa4/0xac
...
[ 84.579183] Call trace:
[ 84.581624] clk_core_disable+0xa4/0xac
[ 84.585457] clk_disable+0x30/0x4c
[ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
[ 84.593651] pm_generic_suspend+0x2c/0x44
[ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
[ 84.601670] dpm_run_callback+0x8c/0x14c
[ 84.605588] __device_suspend+0x1a0/0x56c
[ 84.609594] dpm_suspend+0x17c/0x21c
[ 84.613165] dpm_suspend_start+0xa0/0xa8
[ 84.617083] suspend_devices_and_enter+0x12c/0x634
[ 84.621872] pm_suspend+0x1fc/0x368
To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls
by checking if the device is already runtime suspended.
Cc: <stable@vger.kernel.org> # 6.1.x
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b022dd6e6b6e..62179e55e032 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = {
.transfer = cdns_dsi_transfer,
};
-static int __maybe_unused cdns_dsi_resume(struct device *dev)
+static int cdns_dsi_resume(struct device *dev)
{
struct cdns_dsi *dsi = dev_get_drvdata(dev);
@@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev)
return 0;
}
-static int __maybe_unused cdns_dsi_suspend(struct device *dev)
+static int cdns_dsi_suspend(struct device *dev)
{
struct cdns_dsi *dsi = dev_get_drvdata(dev);
@@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
return 0;
}
-static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend, cdns_dsi_resume,
- NULL);
+static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
+ cdns_dsi_resume, NULL);
static int cdns_dsi_drm_probe(struct platform_device *pdev)
{
@@ -1427,7 +1427,7 @@ static struct platform_driver cdns_dsi_platform_driver = {
.driver = {
.name = "cdns-dsi",
.of_match_table = cdns_dsi_of_match,
- .pm = &cdns_dsi_pm_ops,
+ .pm = pm_ptr(&cdns_dsi_pm_ops),
},
};
module_platform_driver(cdns_dsi_platform_driver);
--
2.34.1Hi,
On 28/04/2025 12:40, Vitor Soares wrote:
> From: Vitor Soares <vitor.soares@toradex.com>
>
> The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
> for both runtime PM and system sleep. This causes the DSI clocks to be
> disabled twice: once during runtime suspend and again during system
> suspend, resulting in a WARN message from the clock framework when
> attempting to disable already-disabled clocks.
>
> [ 84.384540] clk:231:5 already disabled
> [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181 clk_core_disable+0xa4/0xac
> ...
> [ 84.579183] Call trace:
> [ 84.581624] clk_core_disable+0xa4/0xac
> [ 84.585457] clk_disable+0x30/0x4c
> [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
> [ 84.593651] pm_generic_suspend+0x2c/0x44
> [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
> [ 84.601670] dpm_run_callback+0x8c/0x14c
> [ 84.605588] __device_suspend+0x1a0/0x56c
> [ 84.609594] dpm_suspend+0x17c/0x21c
> [ 84.613165] dpm_suspend_start+0xa0/0xa8
> [ 84.617083] suspend_devices_and_enter+0x12c/0x634
> [ 84.621872] pm_suspend+0x1fc/0x368
>
> To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
> DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls
> by checking if the device is already runtime suspended.
>
> Cc: <stable@vger.kernel.org> # 6.1.x
> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> ---
> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index b022dd6e6b6e..62179e55e032 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = {
> .transfer = cdns_dsi_transfer,
> };
>
> -static int __maybe_unused cdns_dsi_resume(struct device *dev)
> +static int cdns_dsi_resume(struct device *dev)
> {
> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>
> @@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev)
> return 0;
> }
>
> -static int __maybe_unused cdns_dsi_suspend(struct device *dev)
> +static int cdns_dsi_suspend(struct device *dev)
> {
> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>
> @@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
> return 0;
> }
>
> -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend, cdns_dsi_resume,
> - NULL);
> +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> + cdns_dsi_resume, NULL);
I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When
the system is suspended, the bridge drivers will get a call to the
*_disable() hook, which then disables the device. If the bridge driver
would additionally do something in its system suspend hook, it would
conflict with normal disable path.
I think bridges/panels should only deal with runtime PM.
Tomi
On Tue, 2025-04-29 at 09:32 +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 28/04/2025 12:40, Vitor Soares wrote:
> > From: Vitor Soares <vitor.soares@toradex.com>
> >
> > The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
> > for both runtime PM and system sleep. This causes the DSI clocks to be
> > disabled twice: once during runtime suspend and again during system
> > suspend, resulting in a WARN message from the clock framework when
> > attempting to disable already-disabled clocks.
> >
> > [ 84.384540] clk:231:5 already disabled
> > [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181
> > clk_core_disable+0xa4/0xac
> > ...
> > [ 84.579183] Call trace:
> > [ 84.581624] clk_core_disable+0xa4/0xac
> > [ 84.585457] clk_disable+0x30/0x4c
> > [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
> > [ 84.593651] pm_generic_suspend+0x2c/0x44
> > [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
> > [ 84.601670] dpm_run_callback+0x8c/0x14c
> > [ 84.605588] __device_suspend+0x1a0/0x56c
> > [ 84.609594] dpm_suspend+0x17c/0x21c
> > [ 84.613165] dpm_suspend_start+0xa0/0xa8
> > [ 84.617083] suspend_devices_and_enter+0x12c/0x634
> > [ 84.621872] pm_suspend+0x1fc/0x368
> >
> > To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
> > DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls
> > by checking if the device is already runtime suspended.
> >
> > Cc: <stable@vger.kernel.org> # 6.1.x
> > Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > ---
> > drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > index b022dd6e6b6e..62179e55e032 100644
> > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = {
> > .transfer = cdns_dsi_transfer,
> > };
> >
> > -static int __maybe_unused cdns_dsi_resume(struct device *dev)
> > +static int cdns_dsi_resume(struct device *dev)
> > {
> > struct cdns_dsi *dsi = dev_get_drvdata(dev);
> >
> > @@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct
> > device *dev)
> > return 0;
> > }
> >
> > -static int __maybe_unused cdns_dsi_suspend(struct device *dev)
> > +static int cdns_dsi_suspend(struct device *dev)
> > {
> > struct cdns_dsi *dsi = dev_get_drvdata(dev);
> >
> > @@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct
> > device *dev)
> > return 0;
> > }
> >
> > -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> > cdns_dsi_resume,
> > - NULL);
> > +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> > + cdns_dsi_resume, NULL);
>
> I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When
> the system is suspended, the bridge drivers will get a call to the
> *_disable() hook, which then disables the device. If the bridge driver
> would additionally do something in its system suspend hook, it would
> conflict with normal disable path.
>
> I think bridges/panels should only deal with runtime PM.
>
> Tomi
>
In the proposed change, we make use of pm_runtime_force_suspend() during
system-wide suspend. If the device is already suspended, this call is a
no-op and disables runtime PM to prevent spurious wakeups during the
suspend period. Otherwise, it triggers the device’s runtime_suspend()
callback.
I briefly reviewed other bridge drivers, and those that implement runtime
PM appear to follow a similar approach, relying solely on runtime PM
callbacks and using pm_runtime_force_suspend()/resume() to handle
system-wide transitions.
Best regards,
Vitor Soares
Hi,
On 05/05/2025 17:45, Vitor Soares wrote:
> On Tue, 2025-04-29 at 09:32 +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 28/04/2025 12:40, Vitor Soares wrote:
>>> From: Vitor Soares <vitor.soares@toradex.com>
>>>
>>> The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
>>> for both runtime PM and system sleep. This causes the DSI clocks to be
>>> disabled twice: once during runtime suspend and again during system
>>> suspend, resulting in a WARN message from the clock framework when
>>> attempting to disable already-disabled clocks.
>>>
>>> [ 84.384540] clk:231:5 already disabled
>>> [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181
>>> clk_core_disable+0xa4/0xac
>>> ...
>>> [ 84.579183] Call trace:
>>> [ 84.581624] clk_core_disable+0xa4/0xac
>>> [ 84.585457] clk_disable+0x30/0x4c
>>> [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
>>> [ 84.593651] pm_generic_suspend+0x2c/0x44
>>> [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
>>> [ 84.601670] dpm_run_callback+0x8c/0x14c
>>> [ 84.605588] __device_suspend+0x1a0/0x56c
>>> [ 84.609594] dpm_suspend+0x17c/0x21c
>>> [ 84.613165] dpm_suspend_start+0xa0/0xa8
>>> [ 84.617083] suspend_devices_and_enter+0x12c/0x634
>>> [ 84.621872] pm_suspend+0x1fc/0x368
>>>
>>> To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
>>> DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls
>>> by checking if the device is already runtime suspended.
>>>
>>> Cc: <stable@vger.kernel.org> # 6.1.x
>>> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
>>> Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
>>> ---
>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> index b022dd6e6b6e..62179e55e032 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = {
>>> .transfer = cdns_dsi_transfer,
>>> };
>>>
>>> -static int __maybe_unused cdns_dsi_resume(struct device *dev)
>>> +static int cdns_dsi_resume(struct device *dev)
>>> {
>>> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>>>
>>> @@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct
>>> device *dev)
>>> return 0;
>>> }
>>>
>>> -static int __maybe_unused cdns_dsi_suspend(struct device *dev)
>>> +static int cdns_dsi_suspend(struct device *dev)
>>> {
>>> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>>>
>>> @@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct
>>> device *dev)
>>> return 0;
>>> }
>>>
>>> -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
>>> cdns_dsi_resume,
>>> - NULL);
>>> +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
>>> + cdns_dsi_resume, NULL);
>>
>> I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When
>> the system is suspended, the bridge drivers will get a call to the
>> *_disable() hook, which then disables the device. If the bridge driver
>> would additionally do something in its system suspend hook, it would
>> conflict with normal disable path.
>>
>> I think bridges/panels should only deal with runtime PM.
>>
>> Tomi
>>
>
> In the proposed change, we make use of pm_runtime_force_suspend() during
> system-wide suspend. If the device is already suspended, this call is a
> no-op and disables runtime PM to prevent spurious wakeups during the
> suspend period. Otherwise, it triggers the device’s runtime_suspend()
> callback.
>
> I briefly reviewed other bridge drivers, and those that implement runtime
> PM appear to follow a similar approach, relying solely on runtime PM
> callbacks and using pm_runtime_force_suspend()/resume() to handle
> system-wide transitions.
Yes, I see such a solution in some of the bridge and panel drivers. I'm
probably missing something here, as I don't think it's correct.
Why do we need to set the system suspend/resume hooks? What is the
scenario where those will be called, and the
pm_runtime_force_suspend()/resume() do something that's not already done
via the normal DRM pipeline enable/disable?
Tomi
On Mon, 2025-05-05 at 18:30 +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 05/05/2025 17:45, Vitor Soares wrote:
> > On Tue, 2025-04-29 at 09:32 +0300, Tomi Valkeinen wrote:
> > > Hi,
> > >
> > > On 28/04/2025 12:40, Vitor Soares wrote:
> > > > From: Vitor Soares <vitor.soares@toradex.com>
> > > >
> > > > The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
> > > > for both runtime PM and system sleep. This causes the DSI clocks to be
> > > > disabled twice: once during runtime suspend and again during system
> > > > suspend, resulting in a WARN message from the clock framework when
> > > > attempting to disable already-disabled clocks.
> > > >
> > > > [ 84.384540] clk:231:5 already disabled
> > > > [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181
> > > > clk_core_disable+0xa4/0xac
> > > > ...
> > > > [ 84.579183] Call trace:
> > > > [ 84.581624] clk_core_disable+0xa4/0xac
> > > > [ 84.585457] clk_disable+0x30/0x4c
> > > > [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
> > > > [ 84.593651] pm_generic_suspend+0x2c/0x44
> > > > [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
> > > > [ 84.601670] dpm_run_callback+0x8c/0x14c
> > > > [ 84.605588] __device_suspend+0x1a0/0x56c
> > > > [ 84.609594] dpm_suspend+0x17c/0x21c
> > > > [ 84.613165] dpm_suspend_start+0xa0/0xa8
> > > > [ 84.617083] suspend_devices_and_enter+0x12c/0x634
> > > > [ 84.621872] pm_suspend+0x1fc/0x368
> > > >
> > > > To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
> > > > DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls
> > > > by checking if the device is already runtime suspended.
> > > >
> > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > > Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> > > > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > > > ---
> > > > drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > index b022dd6e6b6e..62179e55e032 100644
> > > > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops
> > > > = {
> > > > .transfer = cdns_dsi_transfer,
> > > > };
> > > >
> > > > -static int __maybe_unused cdns_dsi_resume(struct device *dev)
> > > > +static int cdns_dsi_resume(struct device *dev)
> > > > {
> > > > struct cdns_dsi *dsi = dev_get_drvdata(dev);
> > > >
> > > > @@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct
> > > > device *dev)
> > > > return 0;
> > > > }
> > > >
> > > > -static int __maybe_unused cdns_dsi_suspend(struct device *dev)
> > > > +static int cdns_dsi_suspend(struct device *dev)
> > > > {
> > > > struct cdns_dsi *dsi = dev_get_drvdata(dev);
> > > >
> > > > @@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct
> > > > device *dev)
> > > > return 0;
> > > > }
> > > >
> > > > -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> > > > cdns_dsi_resume,
> > > > - NULL);
> > > > +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> > > > + cdns_dsi_resume, NULL);
> > >
> > > I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When
> > > the system is suspended, the bridge drivers will get a call to the
> > > *_disable() hook, which then disables the device. If the bridge driver
> > > would additionally do something in its system suspend hook, it would
> > > conflict with normal disable path.
> > >
> > > I think bridges/panels should only deal with runtime PM.
> > >
> > > Tomi
> > >
> >
> > In the proposed change, we make use of pm_runtime_force_suspend() during
> > system-wide suspend. If the device is already suspended, this call is a
> > no-op and disables runtime PM to prevent spurious wakeups during the
> > suspend period. Otherwise, it triggers the device’s runtime_suspend()
> > callback.
> >
> > I briefly reviewed other bridge drivers, and those that implement runtime
> > PM appear to follow a similar approach, relying solely on runtime PM
> > callbacks and using pm_runtime_force_suspend()/resume() to handle
> > system-wide transitions.
>
> Yes, I see such a solution in some of the bridge and panel drivers. I'm
> probably missing something here, as I don't think it's correct.
>
> Why do we need to set the system suspend/resume hooks? What is the
> scenario where those will be called, and the
> pm_runtime_force_suspend()/resume() do something that's not already done
> via the normal DRM pipeline enable/disable?
>
> Tomi
>
I'm not a DRM expert, but my understanding is that there might be edge cases
where the system suspend sequence occurs without the DRM core properly disabling
the bridge — for example, due to a bug or if the bridge is not bound to an
active pipeline. In such cases, having suspend/resume callbacks ensures that the
device is still properly suspended and resumed.
Additionally, pm_runtime_force_suspend() disables runtime PM for the device
during system suspend, preventing unintended wakeups (e.g., via IRQs, delayed
work, or sysfs access) until pm_runtime_force_resume() is invoked.
From my perspective, the use of pm_runtime_force_suspend() and
pm_runtime_force_resume() serves as a safety mechanism to guarantee a well-
defined and race-free state during system suspend.
Best regards,
Vitor Soares
Hi,
On 05/05/2025 20:47, Vitor Soares wrote:
> On Mon, 2025-05-05 at 18:30 +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 05/05/2025 17:45, Vitor Soares wrote:
>>> On Tue, 2025-04-29 at 09:32 +0300, Tomi Valkeinen wrote:
>>>> Hi,
>>>>
>>>> On 28/04/2025 12:40, Vitor Soares wrote:
>>>>> From: Vitor Soares <vitor.soares@toradex.com>
>>>>>
>>>>> The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
>>>>> for both runtime PM and system sleep. This causes the DSI clocks to be
>>>>> disabled twice: once during runtime suspend and again during system
>>>>> suspend, resulting in a WARN message from the clock framework when
>>>>> attempting to disable already-disabled clocks.
>>>>>
>>>>> [ 84.384540] clk:231:5 already disabled
>>>>> [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181
>>>>> clk_core_disable+0xa4/0xac
>>>>> ...
>>>>> [ 84.579183] Call trace:
>>>>> [ 84.581624] clk_core_disable+0xa4/0xac
>>>>> [ 84.585457] clk_disable+0x30/0x4c
>>>>> [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
>>>>> [ 84.593651] pm_generic_suspend+0x2c/0x44
>>>>> [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
>>>>> [ 84.601670] dpm_run_callback+0x8c/0x14c
>>>>> [ 84.605588] __device_suspend+0x1a0/0x56c
>>>>> [ 84.609594] dpm_suspend+0x17c/0x21c
>>>>> [ 84.613165] dpm_suspend_start+0xa0/0xa8
>>>>> [ 84.617083] suspend_devices_and_enter+0x12c/0x634
>>>>> [ 84.621872] pm_suspend+0x1fc/0x368
>>>>>
>>>>> To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
>>>>> DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls
>>>>> by checking if the device is already runtime suspended.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> # 6.1.x
>>>>> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
>>>>> Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
>>>>> ---
>>>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
>>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>>> index b022dd6e6b6e..62179e55e032 100644
>>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>>> @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops
>>>>> = {
>>>>> .transfer = cdns_dsi_transfer,
>>>>> };
>>>>>
>>>>> -static int __maybe_unused cdns_dsi_resume(struct device *dev)
>>>>> +static int cdns_dsi_resume(struct device *dev)
>>>>> {
>>>>> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>>>>>
>>>>> @@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct
>>>>> device *dev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int __maybe_unused cdns_dsi_suspend(struct device *dev)
>>>>> +static int cdns_dsi_suspend(struct device *dev)
>>>>> {
>>>>> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>>>>>
>>>>> @@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct
>>>>> device *dev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
>>>>> cdns_dsi_resume,
>>>>> - NULL);
>>>>> +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
>>>>> + cdns_dsi_resume, NULL);
>>>>
>>>> I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When
>>>> the system is suspended, the bridge drivers will get a call to the
>>>> *_disable() hook, which then disables the device. If the bridge driver
>>>> would additionally do something in its system suspend hook, it would
>>>> conflict with normal disable path.
>>>>
>>>> I think bridges/panels should only deal with runtime PM.
>>>>
>>>> Tomi
>>>>
>>>
>>> In the proposed change, we make use of pm_runtime_force_suspend() during
>>> system-wide suspend. If the device is already suspended, this call is a
>>> no-op and disables runtime PM to prevent spurious wakeups during the
>>> suspend period. Otherwise, it triggers the device’s runtime_suspend()
>>> callback.
>>>
>>> I briefly reviewed other bridge drivers, and those that implement runtime
>>> PM appear to follow a similar approach, relying solely on runtime PM
>>> callbacks and using pm_runtime_force_suspend()/resume() to handle
>>> system-wide transitions.
>>
>> Yes, I see such a solution in some of the bridge and panel drivers. I'm
>> probably missing something here, as I don't think it's correct.
>>
>> Why do we need to set the system suspend/resume hooks? What is the
>> scenario where those will be called, and the
>> pm_runtime_force_suspend()/resume() do something that's not already done
>> via the normal DRM pipeline enable/disable?
>>
>> Tomi
>>
>
> I'm not a DRM expert, but my understanding is that there might be edge cases
> where the system suspend sequence occurs without the DRM core properly disabling
> the bridge — for example, due to a bug or if the bridge is not bound to an
> active pipeline. In such cases, having suspend/resume callbacks ensures that the
> device is still properly suspended and resumed.
>
> Additionally, pm_runtime_force_suspend() disables runtime PM for the device
> during system suspend, preventing unintended wakeups (e.g., via IRQs, delayed
> work, or sysfs access) until pm_runtime_force_resume() is invoked.
>
> From my perspective, the use of pm_runtime_force_suspend() and
> pm_runtime_force_resume() serves as a safety mechanism to guarantee a well-
> defined and race-free state during system suspend.
But then we must be sure that the suspend sequence is just right.
At least in tidss's case, tidss_drv.c has tidss_suspend() which calls
drm_mode_config_helper_suspend(), which, if I recall right, will then
disable the pipeline. This must happen before the bridge's system
suspend call, otherwise the bridge might go to suspend while the
pipeline is still running, which might cause errors on the still-running
pipeline entities, and probably crash the bridge's disable() call. If a
bridge is a platform device, I don't think there's any ordering between
the tidss's and the bridge's suspend calls.
If the bridge is not bound to a pipeline, why would it be enabled in the
first place?
For the bug case... We're in random territory, then. If the driver is
bugging, are you sure it's safe and useful to suspend it? Or would it be
better to not do anything...
I'm not nacking the patch, as this approach seems to be used in multiple
drivers. It just rings multiple alarm bells here, and I don't understand
how exactly it's supposed to work. That said, the driver is using
UNIVERSAL_DEV_PM_OPS(), so I think switching to
DEFINE_RUNTIME_DEV_PM_OPS() is at least not worse (well, I can't be
quite sure even about that =).
Tomi
On Mon, 2025-05-05 at 21:03 +0300, Tomi Valkeinen wrote:
Hello,
> Hi,
>
> On 05/05/2025 20:47, Vitor Soares wrote:
> > On Mon, 2025-05-05 at 18:30 +0300, Tomi Valkeinen wrote:
> > > Hi,
> > >
> > > On 05/05/2025 17:45, Vitor Soares wrote:
> > > > On Tue, 2025-04-29 at 09:32 +0300, Tomi Valkeinen wrote:
> > > > > Hi,
> > > > >
> > > > > On 28/04/2025 12:40, Vitor Soares wrote:
> > > > > > From: Vitor Soares <vitor.soares@toradex.com>
> > > > > >
> > > > > > The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided
> > > > > > callbacks
> > > > > > for both runtime PM and system sleep. This causes the DSI clocks to
> > > > > > be
> > > > > > disabled twice: once during runtime suspend and again during system
> > > > > > suspend, resulting in a WARN message from the clock framework when
> > > > > > attempting to disable already-disabled clocks.
> > > > > >
> > > > > > [ 84.384540] clk:231:5 already disabled
> > > > > > [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181
> > > > > > clk_core_disable+0xa4/0xac
> > > > > > ...
> > > > > > [ 84.579183] Call trace:
> > > > > > [ 84.581624] clk_core_disable+0xa4/0xac
> > > > > > [ 84.585457] clk_disable+0x30/0x4c
> > > > > > [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi]
> > > > > > [ 84.593651] pm_generic_suspend+0x2c/0x44
> > > > > > [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c
> > > > > > [ 84.601670] dpm_run_callback+0x8c/0x14c
> > > > > > [ 84.605588] __device_suspend+0x1a0/0x56c
> > > > > > [ 84.609594] dpm_suspend+0x17c/0x21c
> > > > > > [ 84.613165] dpm_suspend_start+0xa0/0xa8
> > > > > > [ 84.617083] suspend_devices_and_enter+0x12c/0x634
> > > > > > [ 84.621872] pm_suspend+0x1fc/0x368
> > > > > >
> > > > > > To address this issue, replace UNIVERSAL_DEV_PM_OPS() with
> > > > > > DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume
> > > > > > calls
> > > > > > by checking if the device is already runtime suspended.
> > > > > >
> > > > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > > > > Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> > > > > > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++-----
> > > > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > > > b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > > > index b022dd6e6b6e..62179e55e032 100644
> > > > > > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> > > > > > @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops
> > > > > > cdns_dsi_ops
> > > > > > = {
> > > > > > .transfer = cdns_dsi_transfer,
> > > > > > };
> > > > > >
> > > > > > -static int __maybe_unused cdns_dsi_resume(struct device *dev)
> > > > > > +static int cdns_dsi_resume(struct device *dev)
> > > > > > {
> > > > > > struct cdns_dsi *dsi = dev_get_drvdata(dev);
> > > > > >
> > > > > > @@ -1269,7 +1269,7 @@ static int __maybe_unused
> > > > > > cdns_dsi_resume(struct
> > > > > > device *dev)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > -static int __maybe_unused cdns_dsi_suspend(struct device *dev)
> > > > > > +static int cdns_dsi_suspend(struct device *dev)
> > > > > > {
> > > > > > struct cdns_dsi *dsi = dev_get_drvdata(dev);
> > > > > >
> > > > > > @@ -1279,8 +1279,8 @@ static int __maybe_unused
> > > > > > cdns_dsi_suspend(struct
> > > > > > device *dev)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> > > > > > cdns_dsi_resume,
> > > > > > - NULL);
> > > > > > +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend,
> > > > > > + cdns_dsi_resume, NULL);
> > > > >
> > > > > I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When
> > > > > the system is suspended, the bridge drivers will get a call to the
> > > > > *_disable() hook, which then disables the device. If the bridge driver
> > > > > would additionally do something in its system suspend hook, it would
> > > > > conflict with normal disable path.
> > > > >
> > > > > I think bridges/panels should only deal with runtime PM.
> > > > >
> > > > > Tomi
> > > > >
> > > >
> > > > In the proposed change, we make use of pm_runtime_force_suspend() during
> > > > system-wide suspend. If the device is already suspended, this call is a
> > > > no-op and disables runtime PM to prevent spurious wakeups during the
> > > > suspend period. Otherwise, it triggers the device’s runtime_suspend()
> > > > callback.
> > > >
> > > > I briefly reviewed other bridge drivers, and those that implement
> > > > runtime
> > > > PM appear to follow a similar approach, relying solely on runtime PM
> > > > callbacks and using pm_runtime_force_suspend()/resume() to handle
> > > > system-wide transitions.
> > >
> > > Yes, I see such a solution in some of the bridge and panel drivers. I'm
> > > probably missing something here, as I don't think it's correct.
> > >
> > > Why do we need to set the system suspend/resume hooks? What is the
> > > scenario where those will be called, and the
> > > pm_runtime_force_suspend()/resume() do something that's not already done
> > > via the normal DRM pipeline enable/disable?
> > >
> > > Tomi
> > >
> >
> > I'm not a DRM expert, but my understanding is that there might be edge cases
> > where the system suspend sequence occurs without the DRM core properly
> > disabling
> > the bridge — for example, due to a bug or if the bridge is not bound to an
> > active pipeline. In such cases, having suspend/resume callbacks ensures that
> > the
> > device is still properly suspended and resumed.
> >
> > Additionally, pm_runtime_force_suspend() disables runtime PM for the device
> > during system suspend, preventing unintended wakeups (e.g., via IRQs,
> > delayed
> > work, or sysfs access) until pm_runtime_force_resume() is invoked.
> >
> > From my perspective, the use of pm_runtime_force_suspend() and
> > pm_runtime_force_resume() serves as a safety mechanism to guarantee a well-
> > defined and race-free state during system suspend.
>
> But then we must be sure that the suspend sequence is just right.
>
> At least in tidss's case, tidss_drv.c has tidss_suspend() which calls
> drm_mode_config_helper_suspend(), which, if I recall right, will then
> disable the pipeline. This must happen before the bridge's system
> suspend call, otherwise the bridge might go to suspend while the
> pipeline is still running, which might cause errors on the still-running
> pipeline entities, and probably crash the bridge's disable() call. If a
> bridge is a platform device, I don't think there's any ordering between
> the tidss's and the bridge's suspend calls.
>
> If the bridge is not bound to a pipeline, why would it be enabled in the
> first place?
>
> For the bug case... We're in random territory, then. If the driver is
> bugging, are you sure it's safe and useful to suspend it? Or would it be
> better to not do anything...
>
> I'm not nacking the patch, as this approach seems to be used in multiple
> drivers. It just rings multiple alarm bells here, and I don't understand
> how exactly it's supposed to work. That said, the driver is using
> UNIVERSAL_DEV_PM_OPS(), so I think switching to
> DEFINE_RUNTIME_DEV_PM_OPS() is at least not worse (well, I can't be
> quite sure even about that =).
>
> Tomi
>
I conducted further tests based on your concerns, specifically regarding the
suspend ordering between the tidss_suspend() and the bridge suspend. Here are my
observations:
- The bridge (controlled via TI SCI PD) suspends after tidss_suspend(), which
uses device-specific PM operations (platform_pm_suspend).
- I attempted to influence the probe/suspend order via DT node placement and
delays, but that had no effect on suspend sequencing.
- I added some debug prints and the pm_runtime_force_suspend() is invoked
before cdns_dsi_suspend(). However, I did not observe any misbehavior during
suspend/resume.
- I also tested with only runtime PM support in the driver (without
pm_runtime_force_suspend/resume()), and I couldn't detect any functional
difference nor the issue originally addressed in this patch.
Given that, I will send a v2 of the patch implementing only runtime PM support.
If issues arise in the future due to the lack of explicit
pm_runtime_force_suspend/resume() handling, we can revisit and address them at
that time with clearer justification.
Best regards,
Vitor Soares
© 2016 - 2026 Red Hat, Inc.