"Nori, Sekhar" <nsek...@ti.com> writes:
> 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.
I think those helper functions do not help readability. Just do the
raw read/modify/writes and I think it will greatly improve
readability.
If you want to track the docs, you could use comments for that
including a reference to the doc and relevant sections.
>>
>> > +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.
If the upper layers of the suspend code don't disable interrupts, then
I suggest you just use a local_irq_save/restore block here instad of a
spinlock.
Kevin
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
