Hi all,
Recent omapdrm related changes to drop legacy platform data caused a suspend and resume regression. I must have only tested suspend and resume only with the changes preparing to drop the platform data, but looks like I forgot to test suspend after dropping the platform data.
There seem to be other longer term DSS regressions remaining too. Looks like at least panel-simple currently does not probe. It fails with "panel-simple display: Reject override mode: panel has a fixed mode". I think the solution there is to drop the board specific panel-timing related dts lines, but that still seems to be pending. Anyways, no luck getting the LCD enabled on am437x-sk-evm with v5.6 or v5.7-rc kernels.
Regards,
Tony
Tony Lindgren (5): drm/omap: Fix suspend resume regression after platform data removal bus: ti-sysc: Use optional clocks on for enable and wait for softreset bit bus: ti-sysc: Ignore clockactivity unless specified as a quirk bus: ti-sysc: Fix uninitialized framedonetv_irq ARM: OMAP2+: Fix legacy mode dss_reset
arch/arm/mach-omap2/omap_hwmod.c | 2 +- drivers/bus/ti-sysc.c | 93 ++++++++++++++++++------ drivers/gpu/drm/omapdrm/dss/dispc.c | 6 +- drivers/gpu/drm/omapdrm/dss/dsi.c | 6 +- drivers/gpu/drm/omapdrm/dss/dss.c | 6 +- drivers/gpu/drm/omapdrm/dss/venc.c | 6 +- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 4 +- 7 files changed, 79 insertions(+), 44 deletions(-)
When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined.
Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend.
Fixes: cef766300353 ("drm/omap: Prepare DSS for probing without legacy platform data") Reported-by: Faiz Abbas faiz_abbas@ti.com Cc: dri-devel@lists.freedesktop.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Tony Lindgren tony@atomide.com --- drivers/gpu/drm/omapdrm/dss/dispc.c | 6 ++---- drivers/gpu/drm/omapdrm/dss/dsi.c | 6 ++---- drivers/gpu/drm/omapdrm/dss/dss.c | 6 ++---- drivers/gpu/drm/omapdrm/dss/venc.c | 6 ++---- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 4 +--- 5 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -4933,10 +4933,8 @@ static int dispc_runtime_resume(struct device *dev) return 0; }
-static const struct dev_pm_ops dispc_pm_ops = { - .runtime_suspend = dispc_runtime_suspend, - .runtime_resume = dispc_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(dispc_pm_ops, dispc_runtime_suspend, + dispc_runtime_resume, NULL);
struct platform_driver omap_dispchw_driver = { .probe = dispc_probe, diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5464,10 +5464,8 @@ static int dsi_runtime_resume(struct device *dev) return 0; }
-static const struct dev_pm_ops dsi_pm_ops = { - .runtime_suspend = dsi_runtime_suspend, - .runtime_resume = dsi_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(dsi_pm_ops, dsi_runtime_suspend, + dsi_runtime_resume, NULL);
struct platform_driver omap_dsihw_driver = { .probe = dsi_probe, diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c --- a/drivers/gpu/drm/omapdrm/dss/dss.c +++ b/drivers/gpu/drm/omapdrm/dss/dss.c @@ -1611,10 +1611,8 @@ static int dss_runtime_resume(struct device *dev) return 0; }
-static const struct dev_pm_ops dss_pm_ops = { - .runtime_suspend = dss_runtime_suspend, - .runtime_resume = dss_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(dss_pm_ops, dss_runtime_suspend, + dss_runtime_resume, NULL);
struct platform_driver omap_dsshw_driver = { .probe = dss_probe, diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c --- a/drivers/gpu/drm/omapdrm/dss/venc.c +++ b/drivers/gpu/drm/omapdrm/dss/venc.c @@ -942,10 +942,8 @@ static int venc_runtime_resume(struct device *dev) return 0; }
-static const struct dev_pm_ops venc_pm_ops = { - .runtime_suspend = venc_runtime_suspend, - .runtime_resume = venc_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(venc_pm_ops, venc_runtime_suspend, + venc_runtime_resume, NULL);
static const struct of_device_id venc_of_match[] = { { .compatible = "ti,omap2-venc", }, diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -1169,7 +1169,6 @@ int tiler_map_show(struct seq_file *s, void *arg) } #endif
-#ifdef CONFIG_PM_SLEEP static int omap_dmm_resume(struct device *dev) { struct tcm_area area; @@ -1193,9 +1192,8 @@ static int omap_dmm_resume(struct device *dev)
return 0; } -#endif
-static SIMPLE_DEV_PM_OPS(omap_dmm_pm_ops, NULL, omap_dmm_resume); +static UNIVERSAL_DEV_PM_OPS(omap_dmm_pm_ops, NULL, omap_dmm_resume, NULL);
#if defined(CONFIG_OF) static const struct dmm_platform_data dmm_omap4_platform_data = {
Hi Tony,
On 31/05/2020 22:39, Tony Lindgren wrote:
When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined.
Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend.
I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner.
omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c:
SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume)
omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends).
So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)...
Tomi
* Tomi Valkeinen tomi.valkeinen@ti.com [200603 12:34]:
Hi Tony,
On 31/05/2020 22:39, Tony Lindgren wrote:
When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined.
Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend.
I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner.
OK. I presume you talk about the order of dss child devices here.
omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c:
We had omap_device sort of brute forcing things to idle on suspend which only really works for interconnect target modules with one device in them.
SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume)
omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends).
OK thanks for explaining, I missed that part.
So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)...
Yes it seems something has changed. When diffing the dmesg debug output on suspend and resume, context save and restore functions are no longer called as the PM runtime suspend and resume functions are no longer called on suspend and resume.
I'll drop this patch, and will be applying the rest of the series to fixes if no objections.
Thanks,
Tony
On 03/06/2020 17:06, Tony Lindgren wrote:
- Tomi Valkeinen tomi.valkeinen@ti.com [200603 12:34]:
Hi Tony,
On 31/05/2020 22:39, Tony Lindgren wrote:
When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined.
Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend.
I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner.
OK. I presume you talk about the order of dss child devices here.
Yes, but not only that.
E.g. the dispc driver hasn't been designed to be suspended while active. The only way to properly suspend the dispc HW is to first disable the outputs, wait until they've finished with their current frame, and only then can things be shut down.
The suspend machinery doesn't handle all that (and it couldn't anyway, due to the dependencies to other DSS devices in the pipeline).
omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c:
We had omap_device sort of brute forcing things to idle on suspend which only really works for interconnect target modules with one device in them.
SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume)
omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends).
OK thanks for explaining, I missed that part.
So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)...
Yes it seems something has changed. When diffing the dmesg debug output on suspend and resume, context save and restore functions are no longer called as the PM runtime suspend and resume functions are no longer called on suspend and resume.
I now tested with AM4 SK, and I still can't get system suspend/resume work (without DSS). I have no clue about how to fix that. But if I use pm_test to prevent total suspend, I can reproduce this (or at least looks the same).
And now that I look at this, I have a recollection that I've seen this before. What happens is that the system suspend hook (omap_drm_suspend) gets called fine, and it turns off the displays, which leads to dispc_runtime_puts etc. All goes fine.
But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully.
I think the PM framework does this when starting system suspend process. Maybe this was also happening earlier, but omap_device used to do the final suspend (so omapdrm depended on that functionality, after all...).
Tomi
* Tomi Valkeinen tomi.valkeinen@ti.com [200609 07:05]:
On 03/06/2020 17:06, Tony Lindgren wrote:
- Tomi Valkeinen tomi.valkeinen@ti.com [200603 12:34]:
Hi Tony,
On 31/05/2020 22:39, Tony Lindgren wrote:
When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined.
Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend.
I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner.
OK. I presume you talk about the order of dss child devices here.
Yes, but not only that.
E.g. the dispc driver hasn't been designed to be suspended while active. The only way to properly suspend the dispc HW is to first disable the outputs, wait until they've finished with their current frame, and only then can things be shut down.
OK
The suspend machinery doesn't handle all that (and it couldn't anyway, due to the dependencies to other DSS devices in the pipeline).
OK I replied to your patch with some untested comments that might simplify it potentially.
omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c:
We had omap_device sort of brute forcing things to idle on suspend which only really works for interconnect target modules with one device in them.
SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume)
omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends).
OK thanks for explaining, I missed that part.
So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)...
Yes it seems something has changed. When diffing the dmesg debug output on suspend and resume, context save and restore functions are no longer called as the PM runtime suspend and resume functions are no longer called on suspend and resume.
I now tested with AM4 SK, and I still can't get system suspend/resume work (without DSS). I have no clue about how to fix that. But if I use pm_test to prevent total suspend, I can reproduce this (or at least looks the same).
OK
And now that I look at this, I have a recollection that I've seen this before. What happens is that the system suspend hook (omap_drm_suspend) gets called fine, and it turns off the displays, which leads to dispc_runtime_puts etc. All goes fine.
But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully.
Hmm no idea about that. My guess is that there might be an issue that was masked earlier with omap_device calling the child runtime_suspend.
Currently I'm only able to rmmod -f omapdrm, not sure if these issues might be related.
As we now have the interconnect target module as the parent so usage counts might be different but should balance out the same way as earlier.
I think the PM framework does this when starting system suspend process. Maybe this was also happening earlier, but omap_device used to do the final suspend (so omapdrm depended on that functionality, after all...).
Yes the different handling seems to be the main part of the issue.
Regards,
Tony
On 09/06/2020 18:19, Tony Lindgren wrote:
But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully.
Hmm no idea about that. My guess is that there might be an issue that was masked earlier with omap_device calling the child runtime_suspend.
Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device.
Currently I'm only able to rmmod -f omapdrm, not sure if these issues might be related.
Hmm, I always use modules, and can unload omapdrm and drm fine. But there's a sequence that must be followed. However, the sequence starts with unloading omapdrm... What behavior you see with rmmod?
Tomi
* Tomi Valkeinen tomi.valkeinen@ti.com [200609 15:27]:
On 09/06/2020 18:19, Tony Lindgren wrote:
Currently I'm only able to rmmod -f omapdrm, not sure if these issues might be related.
Hmm, I always use modules, and can unload omapdrm and drm fine. But there's a sequence that must be followed. However, the sequence starts with unloading omapdrm... What behavior you see with rmmod?
Hmm maybe it's output specific somehow?
I just tried again with the following with v5.7. I see the omapdrm usage count issue happen at least on duovero, but don't seem to currently get /dev/fb0 initialized on x15 with these:
modprobe omapdrm #modprobe connector_hdmi # up to v5.6 modprobe display-connector # starting with v5.7-rc1 modprobe ti-tpd12s015 # beagle-x15 modprobe omapdss
# rmmod omapdrm rmmod: ERROR: Module omapdrm is in use
# lsmod | grep omapdrm omapdrm 65536 1 omapdss_base 16384 2 omapdrm,omapdss drm_kms_helper 155648 3 omapdss_base,omapdrm,omapdss drm 372736 7 ti_tpd12s015,omapdss_base,display_connector,omapdrm,omapdss,drm_kms_helper
On beagle-x15 I see these errors after modprobe:
DSS: OMAP DSS rev 6.1 omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes aic_dvdd_fixed: disabling ldousb: disabling
Maybe I'm missing some related module on x15?
Regards,
Tony
* Tony Lindgren tony@atomide.com [200609 16:53]:
- Tomi Valkeinen tomi.valkeinen@ti.com [200609 15:27]:
On 09/06/2020 18:19, Tony Lindgren wrote:
Currently I'm only able to rmmod -f omapdrm, not sure if these issues might be related.
Hmm, I always use modules, and can unload omapdrm and drm fine. But there's a sequence that must be followed. However, the sequence starts with unloading omapdrm... What behavior you see with rmmod?
Hmm maybe it's output specific somehow?
I just tried again with the following with v5.7. I see the omapdrm usage count issue happen at least on duovero, but don't seem to currently get /dev/fb0 initialized on x15 with these:
modprobe omapdrm #modprobe connector_hdmi # up to v5.6 modprobe display-connector # starting with v5.7-rc1 modprobe ti-tpd12s015 # beagle-x15 modprobe omapdss
# rmmod omapdrm rmmod: ERROR: Module omapdrm is in use
# lsmod | grep omapdrm omapdrm 65536 1 omapdss_base 16384 2 omapdrm,omapdss drm_kms_helper 155648 3 omapdss_base,omapdrm,omapdss drm 372736 7 ti_tpd12s015,omapdss_base,display_connector,omapdrm,omapdss,drm_kms_helper
I'm also seeing the rmmod omapdrm issue on am437x-sk-evm:
modprobe pwm-omap-dmtimer modprobe pwm-tiecap modprobe pwm_bl modprobe omapdrm modprobe panel-simple modprobe display-connector # starting with v5.7-rc1 modprobe omapdss
# rmmod omapdrm rmmod: ERROR: Module omapdrm is in use
On beagle-x15 I see these errors after modprobe:
DSS: OMAP DSS rev 6.1 omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes aic_dvdd_fixed: disabling ldousb: disabling
Maybe I'm missing some related module on x15?
Still did not figure what I might be missing on x15 :)
Regards,
Tony
* Tony Lindgren tony@atomide.com [200609 17:11]:
I'm also seeing the rmmod omapdrm issue on am437x-sk-evm:
Oops sorry this is a user error. I've forgotten I need to unbind the fb vtcon first :) thanks for hinting that Tomi!
I can rmmod omapdrm just fine after doing:
# echo 0 > /sys/class/vtconsole/vtcon1/bind
Still did not figure what I might be missing on x15 :)
So this is really the only issue that I have but sounds like Tomi has it working and I'm just missing some module.
Regards,
Tony
On 09/06/2020 20:10, Tony Lindgren wrote:
On beagle-x15 I see these errors after modprobe:
DSS: OMAP DSS rev 6.1 omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes aic_dvdd_fixed: disabling ldousb: disabling
Maybe I'm missing some related module on x15?
Still did not figure what I might be missing on x15 :)
The log above shows that nothing is missing, omapdrm has probed fine. But it cannot see anything connected to the hdmi port. Are you booting with correct dtb for your x15 revision? And you have a monitor connected? =)
Tomi
* Tomi Valkeinen tomi.valkeinen@ti.com [200610 11:48]:
On 09/06/2020 20:10, Tony Lindgren wrote:
On beagle-x15 I see these errors after modprobe:
DSS: OMAP DSS rev 6.1 omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes aic_dvdd_fixed: disabling ldousb: disabling
Maybe I'm missing some related module on x15?
Still did not figure what I might be missing on x15 :)
The log above shows that nothing is missing, omapdrm has probed fine. But it cannot see anything connected to the hdmi port. Are you booting with correct dtb for your x15 revision? And you have a monitor connected? =)
Oh you're right, I forgot to connect the HDMI cable back to X15 :) No wonder it's no longer working for me..
Regards,
Tony
On 09/06/2020 18:26, Tomi Valkeinen wrote:
On 09/06/2020 18:19, Tony Lindgren wrote:
But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully.
Hmm no idea about that. My guess is that there might be an issue that was masked earlier with omap_device calling the child runtime_suspend.
Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device.
I think I might have an idea what is going wrong.
Before: +----------------------+ |omap_device_pm_domain | +---------------+------+------+ | device | +-------------+ | omap_device | +-------------+
omap_device is embedded in DD device and PM handled by omap_device_pm_domain.
static int _od_suspend_noirq(struct device *dev) { ...
ret = pm_generic_suspend_noirq(dev); [1] ^^ device suspend_noirq call
if (!ret && !pm_runtime_status_suspended(dev)) { if (pm_generic_runtime_suspend(dev) == 0) { [2] ^^ device pm_runtime_suspend force call
omap_device_idle(pdev); [3] ^^ omap_device disable od->flags |= OMAP_DEVICE_SUSPENDED; } }
return ret; }
Now: +------------+ |ti sysc dev | +-+----------+ | | | +-------------+ | | device | +-->+ | +-------------+
With new approach the omap_device is not embedded in DD Device anymore, instead ti-sysc (hwmod replacement) became parent of DD Device.
As result suspend sequence became the following (Note. All PM runtime PUT calls became NOP during suspend by design):
device |-> suspend() - in case of dss omap_drm_suspend() and Co if defined |-> suspend_noirq() - in case of dss *not defined", equal to step [1] above ..
ti sysc dev (ti-sysc is parent, so called after device) |-> sysc_noirq_suspend |-> pm_runtime_force_suspend() |-> sysc_runtime_suspend() - equal to step [3] above
And step [2] is missing as of now!
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
* Grygorii Strashko grygorii.strashko@ti.com [200611 13:59]:
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
Sounds good to me, makes it behave like a standard Linux driver now :)
Regards,
Tony
On 11/06/2020 17:00, Grygorii Strashko wrote:
On 09/06/2020 18:26, Tomi Valkeinen wrote:
On 09/06/2020 18:19, Tony Lindgren wrote:
But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully.
Hmm no idea about that. My guess is that there might be an issue that was masked earlier with omap_device calling the child runtime_suspend.
Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device.
I think I might have an idea what is going wrong.
Before: +----------------------+ |omap_device_pm_domain | +---------------+------+------+ | device | +-------------+ | omap_device | +-------------+
omap_device is embedded in DD device and PM handled by omap_device_pm_domain.
static int _od_suspend_noirq(struct device *dev) { ...
ret = pm_generic_suspend_noirq(dev); [1] ^^ device suspend_noirq call
if (!ret && !pm_runtime_status_suspended(dev)) { if (pm_generic_runtime_suspend(dev) == 0) { [2] ^^ device pm_runtime_suspend force call
omap_device_idle(pdev); [3] ^^ omap_device disable od->flags |= OMAP_DEVICE_SUSPENDED; } }
return ret; }
Now: +------------+ |ti sysc dev | +-+----------+ | | | +-------------+ | | device | +-->+ | +-------------+
With new approach the omap_device is not embedded in DD Device anymore, instead ti-sysc (hwmod replacement) became parent of DD Device.
As result suspend sequence became the following (Note. All PM runtime PUT calls became NOP during suspend by design):
device |-> suspend() - in case of dss omap_drm_suspend() and Co if defined |-> suspend_noirq() - in case of dss *not defined", equal to step [1] above ..
ti sysc dev (ti-sysc is parent, so called after device) |-> sysc_noirq_suspend |-> pm_runtime_force_suspend() |-> sysc_runtime_suspend() - equal to step [3] above
And step [2] is missing as of now!
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early?
Tomi
* Tomi Valkeinen tomi.valkeinen@ti.com [200616 13:02]:
On 11/06/2020 17:00, Grygorii Strashko wrote:
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early?
Well up to you as far as I'm concerned. The noirq phase comes with serious limitations, for let's say i2c bus usage if needed. Probably also harder to debug for suspend and resume.
Regards,
Tony
On 16/06/2020 18:30, Tony Lindgren wrote:
- Tomi Valkeinen tomi.valkeinen@ti.com [200616 13:02]:
On 11/06/2020 17:00, Grygorii Strashko wrote:
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early?
Well up to you as far as I'm concerned. The noirq phase comes with serious limitations, for let's say i2c bus usage if needed. Probably also harder to debug for suspend and resume.
Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still work and there is no sync between suspend and pm_runtime. The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled.
On 16/06/2020 19:56, Grygorii Strashko wrote:
On 16/06/2020 18:30, Tony Lindgren wrote:
- Tomi Valkeinen tomi.valkeinen@ti.com [200616 13:02]:
On 11/06/2020 17:00, Grygorii Strashko wrote:
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early?
Well up to you as far as I'm concerned. The noirq phase comes with serious limitations, for let's say i2c bus usage if needed. Probably also harder to debug for suspend and resume.
Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still work and there is no sync between suspend and pm_runtime. The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled.
Yes, but which one... Do you know what the diff is with late/noirq from driver's perspective? I guess noirq is atomic context, late is nto?
Dispc's suspend uses synchronize_irq(), so that rules out noirq. Although the call is not needed if using noirq version, so that could also be managed with small change. But I wonder if there's any benefit in using noirq versus late.
Tomi
On 17/06/2020 09:04, Tomi Valkeinen wrote:
On 16/06/2020 19:56, Grygorii Strashko wrote:
On 16/06/2020 18:30, Tony Lindgren wrote:
- Tomi Valkeinen tomi.valkeinen@ti.com [200616 13:02]:
On 11/06/2020 17:00, Grygorii Strashko wrote:
I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding:
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
Am I missing smth?
Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early?
Well up to you as far as I'm concerned. The noirq phase comes with serious limitations, for let's say i2c bus usage if needed. Probably also harder to debug for suspend and resume.
Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still work and there is no sync between suspend and pm_runtime. The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled.
Yes, but which one... Do you know what the diff is with late/noirq from driver's perspective? I guess noirq is atomic context, late is nto?
noirq is *not* atomic, jus IRQs (non-wakeup) will be disabled (disbale_irq())
Dispc's suspend uses synchronize_irq(), so that rules out noirq. Although the call is not needed if using noirq version, so that could also be managed with small change. But I wonder if there's any benefit in using noirq versus late.
Some modules reset automatically when idled, and when re-enabled, we must wait for the automatic OCP softreset to complete. And if optional clocks are configured, we need to keep the clocks on while waiting for the reset to complete.
Let's fix the issue by moving the OCP softreset code to a separate function sysc_wait_softreset(), and call it also from sysc_enable_module() with the optional clocks enabled.
This is based on what we're already doing for legacy platform data booting in _enable_sysc().
Fixes: 7324a7a0d5e2 ("bus: ti-sysc: Implement display subsystem reset quirk") Reported-by: Faiz Abbas faiz_abbas@ti.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Tony Lindgren tony@atomide.com --- drivers/bus/ti-sysc.c | 81 ++++++++++++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 20 deletions(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -221,6 +221,36 @@ static u32 sysc_read_sysstatus(struct sysc *ddata) return sysc_read(ddata, offset); }
+/* Poll on reset status */ +static int sysc_wait_softreset(struct sysc *ddata) +{ + u32 sysc_mask, syss_done, rstval; + int sysc_offset, syss_offset, error = 0; + + sysc_offset = ddata->offsets[SYSC_SYSCONFIG]; + syss_offset = ddata->offsets[SYSC_SYSSTATUS]; + sysc_mask = BIT(ddata->cap->regbits->srst_shift); + + if (ddata->cfg.quirks & SYSS_QUIRK_RESETDONE_INVERTED) + syss_done = 0; + else + syss_done = ddata->cfg.syss_mask; + + if (syss_offset >= 0) { + error = readx_poll_timeout(sysc_read_sysstatus, ddata, rstval, + (rstval & ddata->cfg.syss_mask) == + syss_done, + 100, MAX_MODULE_SOFTRESET_WAIT); + + } else if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) { + error = readx_poll_timeout(sysc_read_sysconfig, ddata, rstval, + !(rstval & sysc_mask), + 100, MAX_MODULE_SOFTRESET_WAIT); + } + + return error; +} + static int sysc_add_named_clock_from_child(struct sysc *ddata, const char *name, const char *optfck_name) @@ -925,8 +955,34 @@ static int sysc_enable_module(struct device *dev) struct sysc *ddata; const struct sysc_regbits *regbits; u32 reg, idlemodes, best_mode; + int error;
ddata = dev_get_drvdata(dev); + + /* + * Some modules like DSS reset automatically on idle. Enable optional + * reset clocks and wait for OCP softreset to complete. + */ + if (ddata->cfg.quirks & SYSC_QUIRK_OPT_CLKS_IN_RESET) { + error = sysc_enable_opt_clocks(ddata); + if (error) { + dev_err(ddata->dev, + "Optional clocks failed for enable: %i\n", + error); + return error; + } + } + error = sysc_wait_softreset(ddata); + if (error) + dev_warn(ddata->dev, "OCP softreset timed out\n"); + if (ddata->cfg.quirks & SYSC_QUIRK_OPT_CLKS_IN_RESET) + sysc_disable_opt_clocks(ddata); + + /* + * Some subsystem private interconnects, like DSS top level module, + * need only the automatic OCP softreset handling with no sysconfig + * register bits to configure. + */ if (ddata->offsets[SYSC_SYSCONFIG] == -ENODEV) return 0;
@@ -1828,11 +1884,10 @@ static int sysc_legacy_init(struct sysc *ddata) */ static int sysc_reset(struct sysc *ddata) { - int sysc_offset, syss_offset, sysc_val, rstval, error = 0; - u32 sysc_mask, syss_done; + int sysc_offset, sysc_val, error; + u32 sysc_mask;
sysc_offset = ddata->offsets[SYSC_SYSCONFIG]; - syss_offset = ddata->offsets[SYSC_SYSSTATUS];
if (ddata->legacy_mode || ddata->cap->regbits->srst_shift < 0 || @@ -1841,11 +1896,6 @@ static int sysc_reset(struct sysc *ddata)
sysc_mask = BIT(ddata->cap->regbits->srst_shift);
- if (ddata->cfg.quirks & SYSS_QUIRK_RESETDONE_INVERTED) - syss_done = 0; - else - syss_done = ddata->cfg.syss_mask; - if (ddata->pre_reset_quirk) ddata->pre_reset_quirk(ddata);
@@ -1862,18 +1912,9 @@ static int sysc_reset(struct sysc *ddata) if (ddata->post_reset_quirk) ddata->post_reset_quirk(ddata);
- /* Poll on reset status */ - if (syss_offset >= 0) { - error = readx_poll_timeout(sysc_read_sysstatus, ddata, rstval, - (rstval & ddata->cfg.syss_mask) == - syss_done, - 100, MAX_MODULE_SOFTRESET_WAIT); - - } else if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) { - error = readx_poll_timeout(sysc_read_sysconfig, ddata, rstval, - !(rstval & sysc_mask), - 100, MAX_MODULE_SOFTRESET_WAIT); - } + error = sysc_wait_softreset(ddata); + if (error) + dev_warn(ddata->dev, "OCP softreset timed out\n");
if (ddata->reset_done_quirk) ddata->reset_done_quirk(ddata);
We must ignore the clockactivity bit for most modules and not set it unless specified for the module with SYSC_QUIRK_USE_CLOCKACT. Otherwise the interface clock can be automatically gated constantly causing unexpected performance issues.
Fixes: ae9ae12e9daa ("bus: ti-sysc: Handle clockactivity for enable and disable") Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Tony Lindgren tony@atomide.com --- drivers/bus/ti-sysc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -989,10 +989,13 @@ static int sysc_enable_module(struct device *dev) regbits = ddata->cap->regbits; reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
- /* Set CLOCKACTIVITY, we only use it for ick */ + /* + * Set CLOCKACTIVITY, we only use it for ick. And we only configure it + * based on the SYSC_QUIRK_USE_CLOCKACT flag, not based on the hardware + * capabilities. See the old HWMOD_SET_DEFAULT_CLOCKACT flag. + */ if (regbits->clkact_shift >= 0 && - (ddata->cfg.quirks & SYSC_QUIRK_USE_CLOCKACT || - ddata->cfg.sysc_val & BIT(regbits->clkact_shift))) + (ddata->cfg.quirks & SYSC_QUIRK_USE_CLOCKACT)) reg |= SYSC_CLOCACT_ICK << regbits->clkact_shift;
/* Set SIDLE mode */
We are currently only setting the framedonetv_irq disabled for the SoCs that don't have it. But we are never setting it enabled for the SoCs that have it. Let's initialized it to true by default.
Fixes: 7324a7a0d5e2 ("bus: ti-sysc: Implement display subsystem reset quirk") Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Tony Lindgren tony@atomide.com --- drivers/bus/ti-sysc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -1553,7 +1553,7 @@ static u32 sysc_quirk_dispc(struct sysc *ddata, int dispc_offset, bool lcd_en, digit_en, lcd2_en = false, lcd3_en = false; const int lcd_en_mask = BIT(0), digit_en_mask = BIT(1); int manager_count; - bool framedonetv_irq; + bool framedonetv_irq = true; u32 val, irq_mask = 0;
switch (sysc_soc->soc) { @@ -1570,6 +1570,7 @@ static u32 sysc_quirk_dispc(struct sysc *ddata, int dispc_offset, break; case SOC_AM4: manager_count = 1; + framedonetv_irq = false; break; case SOC_UNKNOWN: default:
We must check for "dss_core" instead of "dss" to avoid also matching also "dss_dispc". This only matters for the mixed case of data configured in device tree but with legacy booting ti,hwmods property still enabled.
Fixes: 8b30919a4e3c ("ARM: OMAP2+: Handle reset quirks for dynamically allocated modules") Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Tony Lindgren tony@atomide.com --- arch/arm/mach-omap2/omap_hwmod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -3489,7 +3489,7 @@ static const struct omap_hwmod_reset dra7_reset_quirks[] = { };
static const struct omap_hwmod_reset omap_reset_quirks[] = { - { .match = "dss", .len = 3, .reset = omap_dss_reset, }, + { .match = "dss_core", .len = 8, .reset = omap_dss_reset, }, { .match = "hdq1w", .len = 5, .reset = omap_hdq1w_reset, }, { .match = "i2c", .len = 3, .reset = omap_i2c_reset, }, { .match = "wd_timer", .len = 8, .reset = omap2_wd_timer_reset, },
dri-devel@lists.freedesktop.org
-
Grygorii Strashko -
Tomi Valkeinen -
Tony Lindgren