[U-Boot] [PATCH v1 3/9] sunxi: initial sun7i dram setup support

[Tom Rini]

On Fri, Mar 14, 2014 at 12:23:50PM -0500, Alex G. wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/14/2014 09:17 AM, Tom Rini wrote:
> > On Fri, Mar 14, 2014 at 10:33:45AM +0000, Ian Campbell wrote:
> > 
> > [snip]
> >> +static void mctl_ddr3_reset(void) +{ +	struct sunxi_dram_reg 
> >> *dram = +			(struct sunxi_dram_reg *)SUNXI_DRAMC_BASE; + +	{ + 
> >> clrbits_le32(&dram->mcr, DRAM_MCR_RESET); +		udelay(2); + 
> >> setbits_le32(&dram->mcr, DRAM_MCR_RESET); +	}
> > 
> > That seems like an odd construction, why the extra braces?
> > 
> This originally had a conditional depending on the SoC family. (Yeah,
> they need to reset the ram differently). It seems it wasn't removed
> properly.
> 
> > And as for the rest of the code, lots of magic numbers to #define 
> > what/why (why udelay(2) and 22?)
> > 
> Before going into more detail, remember this is ram initialization
> code. That's always going to be a pain :(.
> There's nothing magic here. It's just a fact of life. Every step is
> going to need a different delay. No need to bloat the headers by
> #defining each. It also makes raminit code more unreadable.
> 
> We got these numbers from allwinner code dumps. We used to have these
> as sdelay() numbers, which usually meant units of 2 clock cycles. So
> we had to convert them to udelay() to at least make the delays
> independent of CPU clock. The old sdelay() numbers made no sense either.

Yeah, I can accept a certain amount of black magic here.  We need to
make sure things are commented as much as it can be.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140314/adb8dfb7/attachment.pgp>