On Thu, Nov 26, 2009 at 00:54:38, Kevin Hilman wrote:
> Sekhar Nori <nsek...@ti.com> writes:
>
[...]
> > + * Note on SLEEPCOUNT:
> > + * The SLEEPCOUNT feature is mainly intended for cases in which
> > + * the internal oscillator is used. The internal oscillator is
> > + * fully disabled in deep sleep mode. When you exist deep sleep
> > + * mode, the oscillator will be turned on and will generate very
> > + * small oscillations which will not be detected by the deep sleep
> > + * counter. Eventually those oscillations will grow to an amplitude
> > + * large enough to start incrementing the deep sleep counter.
> > + * In this case recommendation from hardware enginners is that the
Oops "engineers" :)
> > diff --git a/arch/arm/mach-davinci/pm.c b/arch/arm/mach-davinci/pm.c
> > new file mode 100644
> > index 0000000..de378dc
> > --- /dev/null
> > +++ b/arch/arm/mach-davinci/pm.c
> > @@ -0,0 +1,172 @@
> > +/*
[...]
> > +
> > +#include "clock.h"
> > +
> > +#define DEEPSLEEP_SLEEPCOUNT_MASK 0xFFFF
> > +
> > +static void (*suspend) (struct davinci_pm_config *);
>
> Maybe davinci_sram_suspend is a better name here.
Okay.
>
> > +static struct davinci_pm_config *pdata;
> > +
> > +static void davinci_sram_push(void *dest, void *src, unsigned int size)
> > +{
> > + memcpy(dest, src, size);
> > + flush_icache_range((unsigned long)dest, (unsigned long)(dest + size));
> > +}
> > +
> > +static inline void davinci_pm_and(void __iomem *base, int offset, unsigned
> > and)
> > +{
> > + unsigned val = __raw_readl(base + offset);
> > + val &= and;
> > + __raw_writel(val, base + offset);
> > +}
> > +
> > +static inline void davinci_pm_or(void __iomem *base, int offset, unsigned
> > or)
> > +{
> > + unsigned val = __raw_readl(base + offset);
> > + val |= or;
> > + __raw_writel(val, base + offset);
> > +}
> > +
> > +static inline void davinci_pm_modify(void __iomem *base, int offset,
> > + unsigned and, unsigned or)
> > +{
> > + unsigned val = __raw_readl(base + offset);
> > + val &= and;
> > + val |= or;
> > + __raw_writel(val, base + offset);
> > +}
>
> I'm not sure these helper functions help in readability.
>
> I think the functions would be more readable using accessing the
> registers directly, and could also be more efficient. e.g. a single
> read up front followed by modify/write accesses.
>
> At least in the suspend func below, all the accesses are to the same
> register, so i don't see the value in these helper funcs.
This comes from a desire to transliterate the user guide
step-by-step procedure into C code. Also, it is not always
possible to just read once and write once as there is a
requirement to wait after writing certain bits (reset wait,
lock wait).
Let me know, I can always redo the way you suggest and we
can then choose the one that looks better.
>
> > +static void davinci_pm_suspend(void)
> > +{
> > + DEFINE_SPINLOCK(lock);
>
> What is this local lock protecting? and why is an IRQ-save lock. I'm
> guessing what you is to be sure this is an IRQs off section, but I'm
> pretty sure the higher level suspend code has disabled IRQs by now.
>
Honestly, I just saw that preempt_count() is still 0 in
davinci_pm_suspend() and went ahead with the locking. I
will check and remove the locking.
Thanks,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
