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.