On 15/03/07 08:09 -0600, Grant Likely wrote: > On 3/15/07, Domen Puncer <[EMAIL PROTECTED]> 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 _______________________________________________ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded