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

["Grant Likely" <grant.likely@secretlab.ca>]

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?

Cheers,
g.

>
>
> Signed-off-by: Domen Puncer <domen.puncer@telargo.com>
>
> ---
>  arch/powerpc/platforms/52xx/Makefile         |    3
>  arch/powerpc/platforms/52xx/lite5200_pm.c    |  125 ++++++++
>  arch/powerpc/platforms/52xx/lite5200_sleep.S |  419 +++++++++++++++++++++++++++
>  3 files changed, 547 insertions(+)
>
> Index: grant.git/arch/powerpc/platforms/52xx/lite5200_pm.c
> ===================================================================
> --- /dev/null
> +++ grant.git/arch/powerpc/platforms/52xx/lite5200_pm.c
> @@ -0,0 +1,125 @@
<snip>
> +static int lite5200_pm_prepare(suspend_state_t state)
> +{
> +       /* deep sleep? let mpc52xx code handle that */
> +       if (state == PM_SUSPEND_STANDBY)
> +               return mpc52xx_pm_prepare(state);
> +
> +       if (state != PM_SUSPEND_MEM)
> +               return -EINVAL;
> +
> +       /* 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.

> +static void lite5200_save_regs(void)
> +{
> +       _memcpy_fromio(&sbes, bes, sizeof(*bes));
> +       _memcpy_fromio(&spic, pic, sizeof(*pic));
> +       _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.

> +
> +       memcpy(saved_sram, sdma.sram, sdma.sram_size);
> +}
> +
> +static void lite5200_restore_regs(void)
> +{
> +       memcpy(sdma.sram, saved_sram, sdma.sram_size);
> +
> +       _memcpy_toio(gpw, &sgpw, sizeof(*gpw));
> +       _memcpy_toio(gps, &sgps, sizeof(*gps));
> +       _memcpy_toio(xlb, &sxlb, sizeof(*xlb));
> +       _memcpy_toio(cdm, &scdm, sizeof(*cdm));
> +       _memcpy_toio(pic, &spic, sizeof(*pic));
> +       _memcpy_toio(bes, &sbes, sizeof(*bes));
> +}
> +
> +static int lite5200_pm_enter(suspend_state_t state)
> +{
> +       /* deep sleep? let mpc52xx code handle that */
> +       if (state == PM_SUSPEND_STANDBY) {
> +               return mpc52xx_pm_enter(state);
> +       }
> +
> +       cdm = mbar + 0x200;
> +       pic = mbar + 0x500;
> +       gps = mbar + 0xb00;
> +       gpw = mbar + 0xc00;
> +       bes = mbar + 0x1200;
> +       xlb = mbar + 0x1f00;

Again, magic numbers


> Index: grant.git/arch/powerpc/platforms/52xx/lite5200_sleep.S
> ===================================================================
> --- /dev/null
> +++ grant.git/arch/powerpc/platforms/52xx/lite5200_sleep.S
> @@ -0,0 +1,419 @@
> +#include <asm/reg.h>
> +#include <asm/ppc_asm.h>
> +#include <asm/processor.h>
> +#include <asm/cache.h>
> +
> +
> +#define SDRAM_MODE     0x100
> +#define SDRAM_CTRL     0x104
> +#define SC_MODE_EN     (1<<31)
> +#define SC_CKE         (1<<30)
> +#define SC_REF_EN      (1<<28)
> +#define SC_SOFT_PRE    (1<<1)
> +
> +#define GPIOW_GPIOE    0xc00
> +#define GPIOW_ODE      0xc04
> +#define GPIOW_DDR      0xc08
> +#define GPIOW_DVO      0xc0c
> +#define GPIOW_INTEN    0xc10
> +
> +#define CDM_CE         0x214
> +#define CDM_SDRAM      (1<<3)
> +
> +
> +// about 2000 cpu cycles for one sdram cycle here
> +// just increase, to be on the safe side?
> +#define TCK    5000

Please avoid c++ comments

> +
> +
> +#define DONT_DEBUG 1

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

> +restore_regs:
> +       lis     r4, registers@h
> +       ori     r4, r4, registers@l
> +#ifdef DONT_DEBUG

should be #if !defined(DEBUG)

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195