All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Archit Taneja <archit@ti.com>
Cc: t-kristo@ti.com, Rajendra Nayak <rnayak@ti.com>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org,
	Tony Lindgren <tony@atomide.com>,
	"Shilimkar, Santosh" <santosh.shilimkar@ti.com>
Subject: Re: OMAP4430 produces boot warnings
Date: Tue, 27 Nov 2012 13:23:35 +0200	[thread overview]
Message-ID: <50B4A2B7.9080806@ti.com> (raw)
In-Reply-To: <50B35D0E.6000509@ti.com>

[-- Attachment #1: Type: text/plain, Size: 4357 bytes --]

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

> 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 <archit@ti.com>
> 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 <archit@ti.com>
> ---
>  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.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: tomi.valkeinen@ti.com (Tomi Valkeinen)
To: linux-arm-kernel@lists.infradead.org
Subject: OMAP4430 produces boot warnings
Date: Tue, 27 Nov 2012 13:23:35 +0200	[thread overview]
Message-ID: <50B4A2B7.9080806@ti.com> (raw)
In-Reply-To: <50B35D0E.6000509@ti.com>

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

> 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 <archit@ti.com>
> 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 <archit@ti.com>
> ---
>  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.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 899 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121127/3cc8cff2/attachment.sig>

  reply	other threads:[~2012-11-27 11:23 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-21 23:03 OMAP4430 produces boot warnings Russell King - ARM Linux
2012-11-21 23:03 ` Russell King - ARM Linux
2012-11-22 12:42 ` Archit Taneja
2012-11-22 12:42   ` Archit Taneja
2012-11-22 13:42   ` Tomi Valkeinen
2012-11-22 13:42     ` Tomi Valkeinen
2012-11-22 14:34     ` Tero Kristo
2012-11-22 14:34       ` Tero Kristo
2012-11-22 14:44       ` Tomi Valkeinen
2012-11-22 14:44         ` Tomi Valkeinen
2012-11-23  9:34         ` Tero Kristo
2012-11-23  9:34           ` Tero Kristo
2012-11-26  6:48           ` Archit Taneja
2012-11-26  6:48             ` Archit Taneja
2012-11-26 12:14             ` Archit Taneja
2012-11-26 12:14               ` Archit Taneja
2012-11-27 11:23               ` Tomi Valkeinen [this message]
2012-11-27 11:23                 ` Tomi Valkeinen
2012-11-27 11:56                 ` Archit Taneja
2012-11-27 11:56                   ` Archit Taneja
2012-11-27 12:21                   ` Tomi Valkeinen
2012-11-27 12:21                     ` Tomi Valkeinen
2012-11-27 12:31                     ` Tero Kristo
2012-11-27 12:31                       ` Tero Kristo
2012-11-28 10:44                       ` Archit Taneja
2012-11-28 10:44                         ` Archit Taneja

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50B4A2B7.9080806@ti.com \
    --to=tomi.valkeinen@ti.com \
    --cc=archit@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=rnayak@ti.com \
    --cc=santosh.shilimkar@ti.com \
    --cc=t-kristo@ti.com \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.