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

[atull]

On Wed, 1 Oct 2014, Pavel Machek wrote:

> 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.

Yes, that's why I wanted to document it.

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

I agree.  I'm about to post the next version.  I will add this in
the code.

> > +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?
> 

You have to use fncpy to do it.

> > +/* 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?

Actually the next version is going to fix some of these other comments by 
using the ocram sram driver to allocate ocram space.  

> 
> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/