Re: [PATCH 2/2] socfpga: support suspend to ram

[Pavel Machek <pavel@denx.de>]

Hi!

> Add code that requests that the sdr controller go into
> self-refresh mode.  This code is run from ocram.
> 
> This patch assumes that u-boot has already configured sdr:
>   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
>   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
>   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512

I'm not sure if we should make assumptions like that. u-boot is not
the only bootloader.

At the very least, it should go to comment in the code, not to changelog.

> +u32 socfpga_sdram_self_refresh(u32 sdr_base, u32 scu_base);
> +extern unsigned int socfpga_sdram_self_refresh_sz;

_sz -> size.

Is it ok to just copy code around?

> +/* Round up a pointer address to fix aligment for fncpy() */
> +static void *fncpy_align(void *ptr)
> +{
> +	u32 value = (u32)ptr;
> +
> +	if ((value & (FNCPY_ALIGN - 1)) != 0)
> +		value = ((value & ~(FNCPY_ALIGN - 1)) + FNCPY_ALIGN);
> +
> +	return (void *)value;
> +}

Don't we have a nice macro doing aligning?

I guess the if() is not neccessary.

> +static int socfpga_pm_suspend(unsigned long arg)
> +{
> +	u32 ret;
> +
> +	ret = socfpga_sdram_self_refresh_in_ocram((u32)sdr_ctl_base_addr,
> +						  (u32)socfpga_scu_base_addr);
> +
> +	pr_debug("%s self-refresh loops request=%d exit=%d\n", __func__,
> +		 ret & 0xffff, (ret >> 16) & 0xffff);
> +
> +	return 0;
> +}

return ret?


> +	.arch   armv7-a
> +	.text
> +	.align 3
> +
> +	/*
> +	 * socfpga_sdram_self_refresh
> +	 *
> +	 *  r0 : sdr_ctl_base_addr
> +	 *  r1 : socfpga_scu_base_addr
> +	 *  r2 : temp storage of register values
> +	 *  r3 : loop counter
> +	 *  r4 : temp storage of return value
> +	 *
> +	 *  return value: lower 16 bits: loop count going into self refresh
> +	 *                upper 16 bits: loop count exiting self refresh
> +	 */
> +ENTRY(socfpga_sdram_self_refresh)

r0, r1 are the parameters?

> @@ -77,6 +78,15 @@ void __init socfpga_sysmgr_init(void)
>  
>  	np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");
>  	rst_manager_base_addr = of_iomap(np, 0);
> +
> +	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
> +		return;
> +	}
> +
> +	sdr_ctl_base_addr = of_iomap(np, 0);
> +	WARN_ON(!sdr_ctl_base_addr);
>  }
>  
>  static void __init socfpga_init_irq(void)

Actually, "sdr-ctl" is quite hard to understand. I guess it means
"sdram-control"? Should we do something like altr,sdram-ctrl-1.0, so
that we have way forward if hardware changes in future?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html