linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal
@ 2020-05-31 19:39 Tony Lindgren
  2020-05-31 19:39 ` [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal Tony Lindgren
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
  To: linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, Laurent Pinchart, Tomi Valkeinen, dri-devel

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(-)

-- 
2.26.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-05-31 19:39 [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal Tony Lindgren
@ 2020-05-31 19:39 ` Tony Lindgren
  2020-06-03 12:33   ` Tomi Valkeinen
  2020-05-31 19:39 ` [PATCH 2/5] bus: ti-sysc: Use optional clocks on for enable and wait for softreset bit Tony Lindgren
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 23+ messages in thread
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
  To: linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart, Tomi Valkeinen

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 = {
-- 
2.26.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 2/5] bus: ti-sysc: Use optional clocks on for enable and wait for softreset bit
  2020-05-31 19:39 [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal Tony Lindgren
  2020-05-31 19:39 ` [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal Tony Lindgren
@ 2020-05-31 19:39 ` Tony Lindgren
  2020-05-31 19:39 ` [PATCH 3/5] bus: ti-sysc: Ignore clockactivity unless specified as a quirk Tony Lindgren
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
  To: linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, Laurent Pinchart, Tomi Valkeinen, dri-devel

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);
-- 
2.26.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 3/5] bus: ti-sysc: Ignore clockactivity unless specified as a quirk
  2020-05-31 19:39 [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal Tony Lindgren
  2020-05-31 19:39 ` [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal Tony Lindgren
  2020-05-31 19:39 ` [PATCH 2/5] bus: ti-sysc: Use optional clocks on for enable and wait for softreset bit Tony Lindgren
@ 2020-05-31 19:39 ` Tony Lindgren
  2020-05-31 19:39 ` [PATCH 4/5] bus: ti-sysc: Fix uninitialized framedonetv_irq Tony Lindgren
  2020-05-31 19:39 ` [PATCH 5/5] ARM: OMAP2+: Fix legacy mode dss_reset Tony Lindgren
  4 siblings, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
  To: linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, Laurent Pinchart, Tomi Valkeinen, dri-devel

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 */
-- 
2.26.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 4/5] bus: ti-sysc: Fix uninitialized framedonetv_irq
  2020-05-31 19:39 [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal Tony Lindgren
                   ` (2 preceding siblings ...)
  2020-05-31 19:39 ` [PATCH 3/5] bus: ti-sysc: Ignore clockactivity unless specified as a quirk Tony Lindgren
@ 2020-05-31 19:39 ` Tony Lindgren
  2020-05-31 19:39 ` [PATCH 5/5] ARM: OMAP2+: Fix legacy mode dss_reset Tony Lindgren
  4 siblings, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
  To: linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, Laurent Pinchart, Tomi Valkeinen, dri-devel

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:
-- 
2.26.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 5/5] ARM: OMAP2+: Fix legacy mode dss_reset
  2020-05-31 19:39 [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal Tony Lindgren
                   ` (3 preceding siblings ...)
  2020-05-31 19:39 ` [PATCH 4/5] bus: ti-sysc: Fix uninitialized framedonetv_irq Tony Lindgren
@ 2020-05-31 19:39 ` Tony Lindgren
  4 siblings, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
  To: linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, Laurent Pinchart, Tomi Valkeinen, dri-devel

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, },
-- 
2.26.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-05-31 19:39 ` [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal Tony Lindgren
@ 2020-06-03 12:33   ` Tomi Valkeinen
  2020-06-03 14:06     ` Tony Lindgren
  0 siblings, 1 reply; 23+ messages in thread
From: Tomi Valkeinen @ 2020-06-03 12:33 UTC (permalink / raw)
  To: Tony Lindgren, linux-omap
  Cc: Andrew F . Davis, Dave Gerlach, Faiz Abbas, Greg Kroah-Hartman,
	Grygorii Strashko, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-03 12:33   ` Tomi Valkeinen
@ 2020-06-03 14:06     ` Tony Lindgren
  2020-06-09  7:04       ` Tomi Valkeinen
  0 siblings, 1 reply; 23+ messages in thread
From: Tony Lindgren @ 2020-06-03 14:06 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-03 14:06     ` Tony Lindgren
@ 2020-06-09  7:04       ` Tomi Valkeinen
  2020-06-09 15:19         ` Tony Lindgren
  0 siblings, 1 reply; 23+ messages in thread
From: Tomi Valkeinen @ 2020-06-09  7:04 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09  7:04       ` Tomi Valkeinen
@ 2020-06-09 15:19         ` Tony Lindgren
  2020-06-09 15:26           ` Tomi Valkeinen
  0 siblings, 1 reply; 23+ messages in thread
From: Tony Lindgren @ 2020-06-09 15:19 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09 15:19         ` Tony Lindgren
@ 2020-06-09 15:26           ` Tomi Valkeinen
  2020-06-09 16:52             ` Tony Lindgren
  2020-06-11 14:00             ` Grygorii Strashko
  0 siblings, 2 replies; 23+ messages in thread
From: Tomi Valkeinen @ 2020-06-09 15:26 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09 15:26           ` Tomi Valkeinen
@ 2020-06-09 16:52             ` Tony Lindgren
  2020-06-09 17:10               ` Tony Lindgren
  2020-06-11 14:00             ` Grygorii Strashko
  1 sibling, 1 reply; 23+ messages in thread
From: Tony Lindgren @ 2020-06-09 16:52 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09 16:52             ` Tony Lindgren
@ 2020-06-09 17:10               ` Tony Lindgren
  2020-06-09 17:26                 ` Tony Lindgren
  2020-06-10 11:47                 ` Tomi Valkeinen
  0 siblings, 2 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-06-09 17:10 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09 17:10               ` Tony Lindgren
@ 2020-06-09 17:26                 ` Tony Lindgren
  2020-06-10 11:47                 ` Tomi Valkeinen
  1 sibling, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-06-09 17:26 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09 17:10               ` Tony Lindgren
  2020-06-09 17:26                 ` Tony Lindgren
@ 2020-06-10 11:47                 ` Tomi Valkeinen
  2020-06-10 22:41                   ` Tony Lindgren
  1 sibling, 1 reply; 23+ messages in thread
From: Tomi Valkeinen @ 2020-06-10 11:47 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-10 11:47                 ` Tomi Valkeinen
@ 2020-06-10 22:41                   ` Tony Lindgren
  0 siblings, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-06-10 22:41 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Grygorii Strashko, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-09 15:26           ` Tomi Valkeinen
  2020-06-09 16:52             ` Tony Lindgren
@ 2020-06-11 14:00             ` Grygorii Strashko
  2020-06-11 14:32               ` Tony Lindgren
  2020-06-16 13:01               ` Tomi Valkeinen
  1 sibling, 2 replies; 23+ messages in thread
From: Grygorii Strashko @ 2020-06-11 14:00 UTC (permalink / raw)
  To: Tomi Valkeinen, Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart



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?

-- 
Best regards,
grygorii

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-11 14:00             ` Grygorii Strashko
@ 2020-06-11 14:32               ` Tony Lindgren
  2020-06-16 13:01               ` Tomi Valkeinen
  1 sibling, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2020-06-11 14:32 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: Tomi Valkeinen, linux-omap, Andrew F . Davis, Dave Gerlach,
	Faiz Abbas, Greg Kroah-Hartman, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-11 14:00             ` Grygorii Strashko
  2020-06-11 14:32               ` Tony Lindgren
@ 2020-06-16 13:01               ` Tomi Valkeinen
  2020-06-16 15:30                 ` Tony Lindgren
  1 sibling, 1 reply; 23+ messages in thread
From: Tomi Valkeinen @ 2020-06-16 13:01 UTC (permalink / raw)
  To: Grygorii Strashko, Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-16 13:01               ` Tomi Valkeinen
@ 2020-06-16 15:30                 ` Tony Lindgren
  2020-06-16 16:56                   ` Grygorii Strashko
  0 siblings, 1 reply; 23+ messages in thread
From: Tony Lindgren @ 2020-06-16 15:30 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Grygorii Strashko, linux-omap, Andrew F . Davis, Dave Gerlach,
	Faiz Abbas, Greg Kroah-Hartman, Keerthy, Nishanth Menon,
	Peter Ujfalusi, Roger Quadros, Suman Anna, Tero Kristo,
	linux-kernel, linux-arm-kernel, dri-devel, Laurent Pinchart

* 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-16 15:30                 ` Tony Lindgren
@ 2020-06-16 16:56                   ` Grygorii Strashko
  2020-06-17  6:04                     ` Tomi Valkeinen
  0 siblings, 1 reply; 23+ messages in thread
From: Grygorii Strashko @ 2020-06-16 16:56 UTC (permalink / raw)
  To: Tony Lindgren, Tomi Valkeinen
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart



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.

-- 
Best regards,
grygorii

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-16 16:56                   ` Grygorii Strashko
@ 2020-06-17  6:04                     ` Tomi Valkeinen
  2020-06-17 12:49                       ` Grygorii Strashko
  0 siblings, 1 reply; 23+ messages in thread
From: Tomi Valkeinen @ 2020-06-17  6:04 UTC (permalink / raw)
  To: Grygorii Strashko, Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
  2020-06-17  6:04                     ` Tomi Valkeinen
@ 2020-06-17 12:49                       ` Grygorii Strashko
  0 siblings, 0 replies; 23+ messages in thread
From: Grygorii Strashko @ 2020-06-17 12:49 UTC (permalink / raw)
  To: Tomi Valkeinen, Tony Lindgren
  Cc: linux-omap, Andrew F . Davis, Dave Gerlach, Faiz Abbas,
	Greg Kroah-Hartman, Keerthy, Nishanth Menon, Peter Ujfalusi,
	Roger Quadros, Suman Anna, Tero Kristo, linux-kernel,
	linux-arm-kernel, dri-devel, Laurent Pinchart



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.




-- 
Best regards,
grygorii

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-06-17 12:50 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-31 19:39 [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal Tony Lindgren
2020-05-31 19:39 ` [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal Tony Lindgren
2020-06-03 12:33   ` Tomi Valkeinen
2020-06-03 14:06     ` Tony Lindgren
2020-06-09  7:04       ` Tomi Valkeinen
2020-06-09 15:19         ` Tony Lindgren
2020-06-09 15:26           ` Tomi Valkeinen
2020-06-09 16:52             ` Tony Lindgren
2020-06-09 17:10               ` Tony Lindgren
2020-06-09 17:26                 ` Tony Lindgren
2020-06-10 11:47                 ` Tomi Valkeinen
2020-06-10 22:41                   ` Tony Lindgren
2020-06-11 14:00             ` Grygorii Strashko
2020-06-11 14:32               ` Tony Lindgren
2020-06-16 13:01               ` Tomi Valkeinen
2020-06-16 15:30                 ` Tony Lindgren
2020-06-16 16:56                   ` Grygorii Strashko
2020-06-17  6:04                     ` Tomi Valkeinen
2020-06-17 12:49                       ` Grygorii Strashko
2020-05-31 19:39 ` [PATCH 2/5] bus: ti-sysc: Use optional clocks on for enable and wait for softreset bit Tony Lindgren
2020-05-31 19:39 ` [PATCH 3/5] bus: ti-sysc: Ignore clockactivity unless specified as a quirk Tony Lindgren
2020-05-31 19:39 ` [PATCH 4/5] bus: ti-sysc: Fix uninitialized framedonetv_irq Tony Lindgren
2020-05-31 19:39 ` [PATCH 5/5] ARM: OMAP2+: Fix legacy mode dss_reset Tony Lindgren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).