| Message ID | CAOMWX4dWnJSOwLVvrAfMd7a9eKE97ywR0TrS6imHFA67V5QZag@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-07-19 at 00:55 -0500, Menon, Nishanth wrote: > On Wed, Jul 18, 2012 at 4:15 AM, Tero Kristo <t-kristo@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-kristo@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
On Thu, Jul 19, 2012 at 4:49 AM, Tero Kristo <t-kristo@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
On Thu, Jul 19, 2012 at 3:19 PM, Tero Kristo <t-kristo@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-kristo@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-kristo@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/
On Thu, 2012-07-19 at 16:05 +0530, Shilimkar, Santosh wrote: > On Thu, Jul 19, 2012 at 3:19 PM, Tero Kristo <t-kristo@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-kristo@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-kristo@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
On Thu, 2012-07-19 at 05:27 -0500, Menon, Nishanth wrote: > On Thu, Jul 19, 2012 at 4:49 AM, Tero Kristo <t-kristo@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
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,