Re: [PATCH 5/5] lite5200b suspend: low-power mode

[Domen Puncer <domen.puncer@telargo.com>]

On 15/03/07 08:09 -0600, Grant Likely wrote:
> On 3/15/07, Domen Puncer <domen.puncer@telargo.com> wrote:
> >Low-power mode implementation for Lite5200b.
> >Some I/O registers are also saved here.
> >
> >A patch to U-Boot that wakes up SDRAM, and transfers control
> >to address saved at physical 0x0 is needed.
> 
> I don't see any structural problems with this code, but I have a few
> comments below.  I'm also concerned about the blind register
> save/restore by memcpy_to/fromio.  I haven't looked at the chip
> documentation, but it looks scary.  Is it safe to restore those
> registers in that manner?
> 
...
> >+       /* map registers */
> >+       mbar = ioremap_nocache(0xf0000000, 0x8000);
> 
> Magic numbers?  Really?  This should be retrieved from the device
> tree.  There is always the possibility of mbar getting moved.
> 
...
> >+       gpw = mbar + 0xc00;
> >+       bes = mbar + 0x1200;
> >+       xlb = mbar + 0x1f00;
> 
> Again, magic numbers

Well... the code is only applicable for Lite5200b/mpc5200
and numbers are from specs.
And it's shorter than mpc52xx_find_and_map() lines.
I guess I could rewrite it.

> >+       _memcpy_fromio(&scdm, cdm, sizeof(*cdm));
> >+       _memcpy_fromio(&sxlb, xlb, sizeof(*xlb));
> >+       _memcpy_fromio(&sgps, gps, sizeof(*gps));
> >+       _memcpy_fromio(&sgpw, gpw, sizeof(*gpw));
> 
> Hmmm.  I have not dug into this deeply, but blind save/restore to
> blocks of registers scares me.

Seems to work (tm), I'll look at datasheet.

> >+// about 2000 cpu cycles for one sdram cycle here
> >+// just increase, to be on the safe side?
> >+#define TCK    5000
> 
> Please avoid c++ comments

// are in ANSI C99 too, but ok, I know kernel folks don't like them :-)


> >+#define DONT_DEBUG 1
> 
> Convention is to #define DEBUG to enable debugging (as opposed to
> #defining something to disable it)

Yeah, makes sense.


	Domen