From: "Grant Likely" <grant.likely@secretlab.ca>
To: "Domen Puncer" <domen.puncer@telargo.com>
Cc: linuxppc-embedded@ozlabs.org
Subject: Re: [PATCH 5/5] lite5200b suspend: low-power mode
Date: Thu, 15 Mar 2007 08:09:43 -0600 [thread overview]
Message-ID: <528646bc0703150709v49af8085p959bd36e0adf1e4a@mail.gmail.com> (raw)
In-Reply-To: <20070315104447.GG22215@moe.telargo.com>
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
next prev parent reply other threads:[~2007-03-15 15:11 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-03-15 10:39 [PATCH 0/5 v2] MPC5200 and Lite5200b low power modes Domen Puncer
2007-03-15 10:41 ` [PATCH 1/5] mpc52xx suspend: UART Domen Puncer
2007-03-15 10:41 ` [PATCH 2/5] mpc52xx suspend: FEC (ethernet) Domen Puncer
2007-03-15 13:35 ` Grant Likely
2007-03-15 10:42 ` [PATCH 3/5] mpc52xx suspend: USB Domen Puncer
2007-03-15 13:24 ` Grant Likely
2007-03-15 14:37 ` Wrong board info for ML403 Leonid
2007-03-16 8:15 ` Andrei Konovalov
2007-03-22 7:44 ` [PATCH 3/5 v2] mpc52xx suspend: USB Domen Puncer
2007-03-23 11:56 ` Sylvain Munaut
2007-03-23 16:00 ` Grant Likely
2007-03-15 10:43 ` [PATCH 4/5] mpc52xx suspend: deep-sleep Domen Puncer
2007-03-23 15:58 ` Grant Likely
2007-04-04 7:37 ` Domen Puncer
2007-04-16 5:40 ` Grant Likely
2007-04-17 7:05 ` Domen Puncer
2007-04-17 7:10 ` Grant Likely
2007-03-15 10:44 ` [PATCH] icecube/lite5200b: wakeup from low-power support Domen Puncer
2007-03-15 10:44 ` [U-Boot-Users] " Domen Puncer
2007-03-26 16:08 ` Grant Likely
2007-03-26 16:08 ` [U-Boot-Users] " Grant Likely
2007-04-03 8:46 ` Domen Puncer
2007-04-03 8:46 ` [U-Boot-Users] " Domen Puncer
2007-04-16 4:45 ` Grant Likely
2007-04-16 4:45 ` [U-Boot-Users] " Grant Likely
2007-04-16 6:25 ` Domen Puncer
2007-04-16 6:25 ` [U-Boot-Users] " Domen Puncer
2007-04-16 7:10 ` Wolfgang Denk
2007-04-16 12:04 ` Stefan Roese
2007-04-16 13:08 ` Domen Puncer
2007-04-16 13:36 ` Grant Likely
2007-04-20 12:13 ` Stefan Roese
2007-04-20 13:47 ` Wolfgang Denk
2007-04-17 11:29 ` Stefan Roese
2007-04-17 14:50 ` Wolfgang Denk
2007-04-18 5:55 ` [U-Boot-Users] [PATCH] icecube/lite5200b: document " Domen Puncer
2007-03-31 17:20 ` [PATCH] icecube/lite5200b: " Rafal Jaworowski
2007-03-31 17:20 ` [U-Boot-Users] " Rafal Jaworowski
2007-03-31 18:38 ` Domen Puncer
2007-03-31 18:38 ` Domen Puncer
2007-03-15 10:44 ` [PATCH 5/5] lite5200b suspend: low-power mode Domen Puncer
2007-03-15 14:09 ` Grant Likely [this message]
2007-03-15 16:36 ` Domen Puncer
2007-03-22 7:41 ` Domen Puncer
2007-03-26 13:23 ` Domen Puncer
2007-03-26 15:54 ` Grant Likely
2007-04-17 7:11 ` Domen Puncer
2007-04-17 7:25 ` Grant Likely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=528646bc0703150709v49af8085p959bd36e0adf1e4a@mail.gmail.com \
--to=grant.likely@secretlab.ca \
--cc=domen.puncer@telargo.com \
--cc=linuxppc-embedded@ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.