On Tuesday 27 November 2012 04:53 PM, Tomi Valkeinen wrote:

On 2012-11-26 14:14, Archit Taneja wrote:

quoted

So Rajendra and I found the problem.

The function _omap4_update_context_lost() reads the register
RM_DSS_DSS_CONTEXT for all DSS hwmods, and increments the count if we
read a non zero value. The issue is that the DSS's parent platform
device (tied to dss_core hwmod) is called first when resuming, it
correctly reads the register and observes that DSS lost context, and
then clears the register.

When the children hwmods are enabled, the see that the registers are
cleared, and hence never increment their count.

One option is to make the DSS driver use the context lost count of the
hwmod corresponding to the parent platform device. It sort of makes a
bit of sense as all the DSS platform devices belong to the same power
domain, so considering only the parent's context lost count is not so bad.

The second option would be to have some usecounting mechanism in
omap_hwmod where different hwmods belonging to the same power domain
don't have their PM_CONTEXT registers cleared until all the hwmods are
enabled.

The first option is easier to implement, here is a patch for the DISPC
driver:


 From 619276fa0e62b90875475eb345a310f1223e82f6 Mon Sep 17 00:00:00 2001
From: Archit Taneja <redacted>
Date: Mon, 26 Nov 2012 17:22:27 +0530
Subject: [PATCH] OMAPDSS: DISPC: Use DISPC's parent device to get context
  lost count

When enabling a hwmod, omap_hwmod refers to the register mentioned in the
hwmod struct's member 'prcm.omap4.context_offs' to see whether context was
lost or not. It increments the context lost count for the hwmod and then
clears
the register.

All the DSS hwmods have the same register(RM_DSS_DSS_CONTEXT) as
context_offs.
When DSS is enabled, the first hwmod to be enabled is the "dss_core"
hwmod since
it's the parent platform device. The dss_core hwmod updates it's context
lost
count correctly and clears the register. When the hwmods corresponding
to the
children platform devices are enabled. They see that the register is
clear, and
don't increment their context lost count. Therefore, all the children
platfrom
devices never report a change in context.

The DISPC driver currently gets the context lost count for DSS from it's
corresponsing platform device instance. The DISPC platform device is one
of the
child devices, and doesn't report the context lost count correctly.

Make the DISPC driver get the context lost count from it's parent. The
parent
platform device's hwmod is the only one which correctly updates the
context lost
count.

Signed-off-by: Archit Taneja <redacted>
---
  drivers/video/omap2/dss/dispc.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c

b/drivers/video/omap2/dss/dispc.c
index a5ab354..d9dfc4ad 100644

--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c

@@ -374,7 +374,7 @@ static void dispc_save_context(void)
      if (dss_has_feature(FEAT_CORE_CLK_DIV))
          SR(DIVISOR);

-    dispc.ctx_loss_cnt = dss_get_ctx_loss_count(&dispc.pdev->dev);
+    dispc.ctx_loss_cnt = dss_get_ctx_loss_count(dispc.pdev->dev.parent);
      dispc.ctx_valid = true;

      DSSDBG("context saved, ctx_loss_count %d\n", dispc.ctx_loss_cnt);

@@ -389,7 +389,7 @@ static void dispc_restore_context(void)
      if (!dispc.ctx_valid)
          return;

-    ctx = dss_get_ctx_loss_count(&dispc.pdev->dev);
+    ctx = dss_get_ctx_loss_count(dispc.pdev->dev.parent);

      if (ctx >= 0 && ctx == dispc.ctx_loss_cnt)
          return;

Hmm, well this feels like a hack. DISPC driver doesn't know how the DSS
modules are arranged, which module belongs to which power domain, etc.

If it cannot be fixed in the arch code, I guess we could just have
dss_get_ctx_loss_count(void) function which always returns the
dss_core's ctx loss count, and define that on all the platforms omapdss
is used, the dss_core's ctx loss count is the same as ctx loss count for
all the dss submodules.

I think the above is true for all OMAPs. But it feels like a hack too,
but not as bad as the above patch.

Yes, a function taking in no platform device in dss's core.c would be 
less hacky. I guess we would need this for now, because a solution in 
omap_hwmod would be more complex and it may not be ready by the merge 
window.

Archit