[PATCH] hxd8-resume.patch

[Ben Dooks]

On Wed, May 23, 2007 at 12:07:35PM +0200, andrzej zaborowski wrote:
> Hi,
> 
> On 23/05/07, Rtp Arnaud Patard <arnaud.patard at rtp-net.org> wrote:
> >Harald Welte <laforge at openmoko.org> writes:
> >
> >Hi,
> >
> >> On Tue, May 22, 2007 at 06:20:49PM +0200, Arnaud Patard wrote:
> >>
> >>> > ?? ????2007-05-22 ?? 13:50 +0200???Harald Welte ????????
> >>> >> Hi Matt!
> >>> >>
> >>> >> The patch seems fine to me (style-wise), however:
> >>> >>
> >>> >> On Sun, May 20, 2007 at 04:13:20PM +0800, matt_hsu wrote:
> >>> >> > Log:
> >>> >> >         HXD8: GPG[15:13] must be selected as input for resuming 
> >from u-boot.
> >>> >>
> >>> >> the most important bit is missing: WHY?
> >>> >>
> >>> > I checked the source, it's result from s3c2410_ts_connect() in
> >>> > s3c2410_ts.c. Even the GPIO configuration (GPGCON) is correct in 
> >uboot.
> >>> > TS module would modify these important bits.
> >>> >
> >>> > But doesn't these bits affect resume logic for GTA01?
> >>> > This caution is not mentioned in S3C2410 datasheet.
> >>> > That is, GPG[13:15] are not functional pins in NAND boot mode in 2410.
> >>> >
> >>> > So I think this patch should be applied on GTA02 if TS module is
> >>> > included.
> >>>
> >>> hmmm... Sorry, but I don't understand. When loaded, the ts driver 
> >configures
> >>> the gpio to "ts mode" and doesn't play anymore with them. Nothing to do
> >>> with suspend/resume.
> >>
> >> well, not exactly.  Without looking at the documentation again, IIRC the 
> >GPG[13:15]
> >> gpio's are used to determine the bootup memory configuration on the
> >> s3c2440.  The resume-from-RAM code in a NAND-only configuration requires
> >
> >2440 ? uh... I though we were talking about the 2410 :(
> >
> >> that the first 4k of the bootloader are loaded from NAND into internal
> >> SRAM (steppingstone) for the resume logic to work.
> >>
> >> The current s3c2410_ts driver however seems to assume that it is always
> >> only running on s3c2410, not s3c2440.  Thus, it reconfigures GPG12..15
> >> unconditionally.
> >
> >the ts driver is called "s3c2410_ts" for some reason :) For the 2440 but
> >the driver may need some tweaks. I'm not even sure that you need to play
> >with GPG12..15 (I don't have a 2440 datasheet at hand so it's iirc).
> >
> >>
> >> The correct solution to this problem thus is: Put the GPG12..15
> >> initialization in a s3c2410 specific section (runtime, not compile
> >> time).
> >
> >What about doint like in the udc driver: different platforms driver
> >structure and check against pdev->name ?
> >May be interesting also to look at the other SoC from Samsung if they
> >need similar tweak so that you can design a generic solution not
> >something limited to s3c2410/s3c2440.
> 
> How about something similar to include/asm-arm/arch-omap/cpu.h for OMAPs?

I have actually asked RMK about this at a previous occasion when
it came up, and local arch functions where not what he wanted. it
turns out at least one cpu support type has ended up implementing
it anyway.

I have also run into dbrownell making complaints we have >1 driver
structure per driver, which he classed as a needless waste of memory.

My current stance on this is that renaming the devices as necessary
makes it cleaer in /sys what is going on.
 
> mock-up:
> 
> --- a/arch/arm/plat-s3c24xx/cpu.c
> +++ b/arch/arm/plat-s3c24xx/cpu.c
> @@ -233,6 +233,11 @@ void __init s3c24xx_init_io(struct map_desc 
> *mach_desc, int
> size)
>        (cpu->map_io)(mach_desc, size);
> }
> 
> +unsigned long s3c24xx_idcode_get(void)
> +{
> +       return cpu->idcode & cpu->idmask;
> +}
> +
>
> /* s3c24xx_init_clocks
>  *
>  * Initialise the clock subsystem and associated information from the
> --- a/include/asm-arm/plat-s3c24xx/cpu.h
> +++ b/include/asm-arm/plat-s3c24xx/cpu.h
> @@ -52,3 +52,79 @@ extern struct sysdev_class s3c2412_sysclass;
> extern struct sysdev_class s3c2440_sysclass;
> extern struct sysdev_class s3c2442_sysclass;
> extern struct sysdev_class s3c2443_sysclass;
> +
> +/* cpu version reporting */
> +
> +extern unsigned long s3c24xx_idcode_get(void);
> +
> +#include <linux/autoconf.h>
> +
> +#undef S3C_VERSION
> +#undef S3C_MULTI
> +
> +#ifdef CONFIG_CPU_S3C2410
> +# ifdef S3C_VERSION
> +#  undef S3C_MULTI
> +#  define S3C_MULTI
> +# else
> +#  define S3C_VERSION
> +# endif
> +#endif

this should probably come from the Kconfig setup as defined
config saying that there is more than one cpu type defined.

> +
> +#ifdef CONFIG_CPU_S3C2412
> +# ifdef S3C_VERSION
> +#  undef S3C_MULTI
> +#  define S3C_MULTI
> +# else
> +#  define S3C_VERSION
> +# endif
> +#endif
> +
> +#ifdef CONFIG_CPU_S3C2440
> +# ifdef S3C_VERSION
> +#  undef S3C_MULTI
> +#  define S3C_MULTI
> +# else
> +#  define S3C_VERSION
> +# endif
> +#endif
> +
> +etc
> +
> +#ifdef S3C_MULTI
> +
> +# define S3C_DEFINE(ID)                \
> +static inline int s3c_cpu_ ## ID(void) \
> +{                              \
> +       return ((s3c24xx_idcode_get() >> 12) & 0xffff) == ID;   \

That should be >> 16, and then you could lose the & as well.
I suppose it would be better as an enum read from a global
variable as at least one of these devices has >1 ID code.

> +}
> +
> +S3C_DEFINE(2410)
> +S3C_DEFINE(2412)
> +S3C_DEFINE(2440)
> +
> +etc
> +
> +#else
> +# define s3c24xx_cpu_2410()    0
> +# define s3c24xx_cpu_2412()    0
> +# define s3c24xx_cpu_2440()    0
> +
> +# ifdef CONFIG_CPU_S3C2410
> +#  undef s3c24xx_cpu_2410
> +#  define s3c24xx_cpu_2410()   1
> +# endif
> +
> +# ifdef CONFIG_CPU_S3C2412
> +#  undef s3c24xx_cpu_2412
> +#  define s3c24xx_cpu_2412()   1
> +# endif
> +
> +# ifdef CONFIG_CPU_S3C2440
> +#  undef s3c24xx_cpu_2440
> +#  define s3c24xx_cpu_2440()   1
> +# endif
> +
> +etc
> +
> +#endif

personally not in favour of this at this point in time.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.