Re: [PATCHv6 4/7] ARM: OMAP: hwmod: Add support for per hwmod/module context lost count

[Tero Kristo <t-kristo@ti.com>]

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