Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
Minor nitpick:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com wrote:
+/**
+ * _omap4_get_context_lost - get context loss counter for a hwmod
Documentation missing for oh
btw, you might be interested in using http://www.omappedia.org/wiki/Kmake
to provide list of kerneldoc errors in addition to other easily
catchable errors..
+ *
+ * Returns the in-memory context loss counter for a hwmod.
+ */
+static int _omap4_get_context_lost(struct omap_hwmod *oh)
+{
+ return oh-prcm.omap4.context_lost_counter;
+}
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Thu, 2012-07-19 at 00:55 -0500, Menon, Nishanth wrote:
On Wed, Jul 18, 2012 at 4:15 AM, Tero Kristo t-kri...@ti.com wrote:
On Tue, 2012-07-17 at 02:59 -0500, Menon, Nishanth wrote:
Couple of minor comments:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com wrote:
[...]
/**
+ * _omap4_update_context_lost - increment hwmod context loss counter
if
+ * hwmod context was lost, and clear hardware context loss reg
+ * @oh: hwmod to check for context loss
+ *
+ * If the PRCM indicates that the hwmod @oh lost context, increment
+ * our in-memory context loss counter, and clear the RM_*_CONTEXT
+ * bits. No return value.
+ */
+static void _omap4_update_context_lost(struct omap_hwmod *oh)
+{
+ u32 r;
+
+ if (oh-prcm.omap4.context_offs == USHRT_MAX)
+ return;
would'nt it be better to return a dummy incremental counter instead of
returning no context loss (count = 0)?
I guess you are right, this way we may have some extra context restores
for modules which don't have context offs defined, rather than not
restoring them at all. Only thing I can think might prevent this is if
there are modules that never lose context but don't have context
register? How about omap5+?
there has been an interesting debate ongoing with HWAUTO and context
loss count handling - since we update only on _enable, this might
actually be interesting to consider:
enable
idle
un_idle (lost context)
read counter - no update
Now to handle modules that never loose context - they have to be in
wakeup domain.. should we consider a flag for those? would'nt matter
o5 or not, context is still the same.. this issue could be resolved if
counter update is done even when a check is done.
Yea, that would be an option. I think I'll add a flag for not losing
context ever.
+
+ r =
omap4_prminst_read_inst_reg(oh-clkdm-pwrdm.ptr-prcm_partition,
+
oh-clkdm-pwrdm.ptr-prcm_offs,
+ oh-prcm.omap4.context_offs);
+
+ if (!r)
+ return;
+
+ oh-prcm.omap4.context_lost_counter++;
need to be careful about counter overflow.
Well, this code can't do much for that even if it overflows... the type
for the context_lost_counter is unsigned though, maybe it should be
expanded if you are worried...?
it can hit 0 with overflow(no context loss). that will not be good, right?
Zero doesn't mean no context loss. If counter was previous MAX_INT, if
it goes to zero it is still a context loss, as the counter value
differs. Drivers do check against diff in the context loss counter, and
if there is one, they do restore which is the right way to handle it. No
need to unnecessarily make this more complicated than it is.
-Tero
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Thu, Jul 19, 2012 at 4:49 AM, Tero Kristo t-kri...@ti.com wrote: Zero doesn't mean no context loss. If counter was previous MAX_INT, if it goes to zero it is still a context loss, as the counter value differs. Drivers do check against diff in the context loss counter, and if there is one, they do restore which is the right way to handle it. No need to unnecessarily make this more complicated than it is. so we flip the responsibility of overflow to drivers. considering a similar scenario of jiffies /* * These inlines deal with timer wrapping correctly. You are * strongly encouraged to use them * 1. Because people otherwise forget * 2. Because if the timer wrap changes in future you won't have to * alter your driver code. * * time_after(a,b) returns true if the time a is after time b. ... */ from past experience, it is highly possible that drivers never get this right. if the intent is just to let the drivers know context was lost, why not go back to the alternate possibility of a bool lost_context which tells the driver if it lost context since it last called the lost_context api. Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Thu, Jul 19, 2012 at 3:19 PM, Tero Kristo t-kri...@ti.com wrote:
On Thu, 2012-07-19 at 00:55 -0500, Menon, Nishanth wrote:
On Wed, Jul 18, 2012 at 4:15 AM, Tero Kristo t-kri...@ti.com wrote:
On Tue, 2012-07-17 at 02:59 -0500, Menon, Nishanth wrote:
Couple of minor comments:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com
wrote:
[...]
/**
+ * _omap4_update_context_lost - increment hwmod context loss
counter
if
+ * hwmod context was lost, and clear hardware context loss reg
+ * @oh: hwmod to check for context loss
+ *
+ * If the PRCM indicates that the hwmod @oh lost context,
increment
+ * our in-memory context loss counter, and clear the RM_*_CONTEXT
+ * bits. No return value.
+ */
+static void _omap4_update_context_lost(struct omap_hwmod *oh)
+{
+ u32 r;
+
+ if (oh-prcm.omap4.context_offs == USHRT_MAX)
+ return;
would'nt it be better to return a dummy incremental counter instead
of
returning no context loss (count = 0)?
I guess you are right, this way we may have some extra context
restores
for modules which don't have context offs defined, rather than not
restoring them at all. Only thing I can think might prevent this is if
there are modules that never lose context but don't have context
register? How about omap5+?
there has been an interesting debate ongoing with HWAUTO and context
loss count handling - since we update only on _enable, this might
actually be interesting to consider:
enable
idle
un_idle (lost context)
read counter - no update
Now to handle modules that never loose context - they have to be in
wakeup domain.. should we consider a flag for those? would'nt matter
o5 or not, context is still the same.. this issue could be resolved if
counter update is done even when a check is done.
Yea, that would be an option. I think I'll add a flag for not losing
context ever.
You just access the module power domain from hwmod and then
you already know whether it is AON or not. The flag idea
was discussed in context of [1]. See if you can re-use that same idea.
regards
Santosh
[1] https://patchwork.kernel.org/patch/1133491/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Thu, 2012-07-19 at 16:05 +0530, Shilimkar, Santosh wrote:
On Thu, Jul 19, 2012 at 3:19 PM, Tero Kristo t-kri...@ti.com wrote:
On Thu, 2012-07-19 at 00:55 -0500, Menon, Nishanth wrote:
On Wed, Jul 18, 2012 at 4:15 AM, Tero Kristo t-kri...@ti.com wrote:
On Tue, 2012-07-17 at 02:59 -0500, Menon, Nishanth wrote:
Couple of minor comments:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com
wrote:
[...]
/**
+ * _omap4_update_context_lost - increment hwmod context loss
counter
if
+ * hwmod context was lost, and clear hardware context loss reg
+ * @oh: hwmod to check for context loss
+ *
+ * If the PRCM indicates that the hwmod @oh lost context,
increment
+ * our in-memory context loss counter, and clear the RM_*_CONTEXT
+ * bits. No return value.
+ */
+static void _omap4_update_context_lost(struct omap_hwmod *oh)
+{
+ u32 r;
+
+ if (oh-prcm.omap4.context_offs == USHRT_MAX)
+ return;
would'nt it be better to return a dummy incremental counter instead
of
returning no context loss (count = 0)?
I guess you are right, this way we may have some extra context
restores
for modules which don't have context offs defined, rather than not
restoring them at all. Only thing I can think might prevent this is if
there are modules that never lose context but don't have context
register? How about omap5+?
there has been an interesting debate ongoing with HWAUTO and context
loss count handling - since we update only on _enable, this might
actually be interesting to consider:
enable
idle
un_idle (lost context)
read counter - no update
Now to handle modules that never loose context - they have to be in
wakeup domain.. should we consider a flag for those? would'nt matter
o5 or not, context is still the same.. this issue could be resolved if
counter update is done even when a check is done.
Yea, that would be an option. I think I'll add a flag for not losing
context ever.
You just access the module power domain from hwmod and then
you already know whether it is AON or not. The flag idea
was discussed in context of [1]. See if you can re-use that same idea.
That looks better still, thanks for the tip.
-Tero
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Thu, 2012-07-19 at 05:27 -0500, Menon, Nishanth wrote: On Thu, Jul 19, 2012 at 4:49 AM, Tero Kristo t-kri...@ti.com wrote: Zero doesn't mean no context loss. If counter was previous MAX_INT, if it goes to zero it is still a context loss, as the counter value differs. Drivers do check against diff in the context loss counter, and if there is one, they do restore which is the right way to handle it. No need to unnecessarily make this more complicated than it is. so we flip the responsibility of overflow to drivers. considering a similar scenario of jiffies /* * These inlines deal with timer wrapping correctly. You are * strongly encouraged to use them * 1. Because people otherwise forget * 2. Because if the timer wrap changes in future you won't have to * alter your driver code. * * time_after(a,b) returns true if the time a is after time b. ... */ from past experience, it is highly possible that drivers never get this right. if the intent is just to let the drivers know context was lost, why not go back to the alternate possibility of a bool lost_context which tells the driver if it lost context since it last called the lost_context api. This goes to the discussion whether the API of lost context stuff is correct or not, and goes out of scope for this set. I am just attempting to bring omap4 to omap3 level in the first place, we can discuss about the potential API problems separately, and a change like that should be relatively easy to implement anyway but will break several drivers. -Tero -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Tue, 2012-07-17 at 02:59 -0500, Menon, Nishanth wrote:
Couple of minor comments:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com wrote:
[...]
/**
+ * _omap4_update_context_lost - increment hwmod context loss counter if
+ * hwmod context was lost, and clear hardware context loss reg
+ * @oh: hwmod to check for context loss
+ *
+ * If the PRCM indicates that the hwmod @oh lost context, increment
+ * our in-memory context loss counter, and clear the RM_*_CONTEXT
+ * bits. No return value.
+ */
+static void _omap4_update_context_lost(struct omap_hwmod *oh)
+{
+ u32 r;
+
+ if (oh-prcm.omap4.context_offs == USHRT_MAX)
+ return;
would'nt it be better to return a dummy incremental counter instead of
returning no context loss (count = 0)?
I guess you are right, this way we may have some extra context restores
for modules which don't have context offs defined, rather than not
restoring them at all. Only thing I can think might prevent this is if
there are modules that never lose context but don't have context
register? How about omap5+?
+
+ r =
omap4_prminst_read_inst_reg(oh-clkdm-pwrdm.ptr-prcm_partition,
+ oh-clkdm-pwrdm.ptr-prcm_offs,
+ oh-prcm.omap4.context_offs);
+
+ if (!r)
+ return;
+
+ oh-prcm.omap4.context_lost_counter++;
need to be careful about counter overflow.
Well, this code can't do much for that even if it overflows... the type
for the context_lost_counter is unsigned though, maybe it should be
expanded if you are worried...?
-Tero
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
On Wed, Jul 18, 2012 at 4:15 AM, Tero Kristo t-kri...@ti.com wrote:
On Tue, 2012-07-17 at 02:59 -0500, Menon, Nishanth wrote:
Couple of minor comments:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com wrote:
[...]
/**
+ * _omap4_update_context_lost - increment hwmod context loss counter
if
+ * hwmod context was lost, and clear hardware context loss reg
+ * @oh: hwmod to check for context loss
+ *
+ * If the PRCM indicates that the hwmod @oh lost context, increment
+ * our in-memory context loss counter, and clear the RM_*_CONTEXT
+ * bits. No return value.
+ */
+static void _omap4_update_context_lost(struct omap_hwmod *oh)
+{
+ u32 r;
+
+ if (oh-prcm.omap4.context_offs == USHRT_MAX)
+ return;
would'nt it be better to return a dummy incremental counter instead of
returning no context loss (count = 0)?
I guess you are right, this way we may have some extra context restores
for modules which don't have context offs defined, rather than not
restoring them at all. Only thing I can think might prevent this is if
there are modules that never lose context but don't have context
register? How about omap5+?
there has been an interesting debate ongoing with HWAUTO and context
loss count handling - since we update only on _enable, this might
actually be interesting to consider:
enable
idle
un_idle (lost context)
read counter - no update
Now to handle modules that never loose context - they have to be in
wakeup domain.. should we consider a flag for those? would'nt matter
o5 or not, context is still the same.. this issue could be resolved if
counter update is done even when a check is done.
+
+ r =
omap4_prminst_read_inst_reg(oh-clkdm-pwrdm.ptr-prcm_partition,
+
oh-clkdm-pwrdm.ptr-prcm_offs,
+ oh-prcm.omap4.context_offs);
+
+ if (!r)
+ return;
+
+ oh-prcm.omap4.context_lost_counter++;
need to be careful about counter overflow.
Well, this code can't do much for that even if it overflows... the type
for the context_lost_counter is unsigned though, maybe it should be
expanded if you are worried...?
it can hit 0 with overflow(no context loss). that will not be good, right?
How about something like:
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index eac813a..5fb9572 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1606,6 +1606,18 @@ static void _reconfigure_io_chain(void)
spin_unlock_irqrestore(io_chain_lock, flags);
}
+static inline void _omap4_inc_context_loss(unsigned int *v)
+{
+
+ /*
+* Context loss count has to be a non-negative value.
+* Clear the sign bit to get a value range from 1 to
+* INT_MAX.
+*/
+ *v = (*v + 1) INT_MAX;
+ *v = *v ? *v : 1;
+}
+
/**
* _omap4_update_context_lost - increment hwmod context loss counter if
* hwmod context was lost, and clear hardware context loss reg
@@ -1629,7 +1641,7 @@ static void _omap4_update_context_lost(struct
omap_hwmod *oh)
if (!r)
return;
- oh-prcm.omap4.context_lost_counter++;
+ _omap4_inc_context_loss(oh-prcm.omap4.context_lost_counter);
omap4_prminst_write_inst_reg(r, oh-clkdm-pwrdm.ptr-prcm_partition,
oh-clkdm-pwrdm.ptr-prcm_offs,
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count
Couple of minor comments:
On Mon, Jun 11, 2012 at 10:26 AM, Tero Kristo t-kri...@ti.com wrote:
[...]
/**
+ * _omap4_update_context_lost - increment hwmod context loss counter if
+ * hwmod context was lost, and clear hardware context loss reg
+ * @oh: hwmod to check for context loss
+ *
+ * If the PRCM indicates that the hwmod @oh lost context, increment
+ * our in-memory context loss counter, and clear the RM_*_CONTEXT
+ * bits. No return value.
+ */
+static void _omap4_update_context_lost(struct omap_hwmod *oh)
+{
+ u32 r;
+
+ if (oh-prcm.omap4.context_offs == USHRT_MAX)
+ return;
would'nt it be better to return a dummy incremental counter instead of
returning no context loss (count = 0)?
+
+ r = omap4_prminst_read_inst_reg(oh-clkdm-pwrdm.ptr-prcm_partition,
+ oh-clkdm-pwrdm.ptr-prcm_offs,
+ oh-prcm.omap4.context_offs);
+
+ if (!r)
+ return;
+
+ oh-prcm.omap4.context_lost_counter++;
need to be careful about counter overflow.
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
