[U-Boot] [PATCH 000/172] socfpga: SPL and DDR init

[Pavel Machek]

On Tue 2015-07-28 15:30:06, Marek Vasut wrote:
> On Tuesday, July 28, 2015 at 03:13:09 PM, Pavel Machek wrote:
> > Hi!
> > 
> > > This series fixes the SPL support on SoCFPGA and cleans up the DDR
> > > init code such that it is becoming remotely mainlinable. After this
> > > series, the SPL is capable of booting from both SD/MMC and QSPI NOR.
> > > 
> > > There is still work to be done, but I'd like to start picking it up
> > > so it can land in 2015.10 . Reviews and comments are welcome.
> > 
> > Do you have series in git somewhere? I guess I'd like to review
> > different diffs than these...
> > 
> 
> Here is what you'd probably make most use of -- complete sockit support:
> http://git.denx.de/?p=u-boot/u-boot-
> socfpga.git;a=shortlog;h=refs/heads/wip/sockit

Thanks!

Random notes:

+static int check_cache_range(unsigned long start, unsigned long stop)
+{
+       int ok = 1;
+
+       if (start & (CONFIG_SYS_CACHELINE_SIZE - 1))
+               ok = 0;
+
+       if (stop & (CONFIG_SYS_CACHELINE_SIZE - 1))
+               ok = 0;
+
+       if (!ok)
+               debug("CACHE: Misaligned operation at range [%08lx,
%08lx]\n",
+                       start, stop);
+

if ((start | end) & (...))
    debug()

..and you get rid of a variable.

+int iocsr_get_config_table(const unsigned int chain_id,
+                          const unsigned long **table,
+                          unsigned int *table_len)
+{
+       switch (chain_id) {
+       case 0:
+               *table = iocsr_scan_chain0_table;
+               *table_len = CONFIG_HPS_IOCSR_SCANCHAIN0_LENGTH;
+               break;
+       case 1:
+               *table = iocsr_scan_chain1_table;
+               *table_len = CONFIG_HPS_IOCSR_SCANCHAIN1_LENGTH;
+               break;
+       case 2:
+               *table = iocsr_scan_chain2_table;
+               *table_len = CONFIG_HPS_IOCSR_SCANCHAIN2_LENGTH;
+               break;
+       case 3:
+               *table = iocsr_scan_chain3_table;
+               *table_len = CONFIG_HPS_IOCSR_SCANCHAIN3_LENGTH;
+               break;
+       default:
+               return -EINVAL;

I'd do *table = NULL, *table_len = 0 here, to catch any mistakes.

+static void sdram_set_rule(struct sdram_prot_rule *prule)
+{
+       uint32_t lo_addr_bits;
+       uint32_t hi_addr_bits;

Can we do u32 here, and in other places like this?

+       rw_mgr_mem_load_user(RW_MGR_MRS0_USER_MIRR, RW_MGR_MRS0_USER,
1);
+       /*
+        * Need to wait tMOD (12CK or 15ns) time before issuing other
+        * commands, but we will have plenty of NIOS cycles before
actual
+        * handoff so its okay.
+        */

I don't understand this comment.

+       return 0;
+
+       /* Calibration Stage 1 completed OK. */
+cal_done_ok:
+       /*
+        * Reset the delay chains back to zero if they have moved > 1
+        * (check for > 1 because loop will increase d even when pass
in
+        * first case).
+        */
+       if (d > 2)
+               scc_mgr_zero_group(rw_group, 1);
+
+       return 1;
+}

Other functions use 0/-EIO protocol here, so it would be good to be
consistent.

It looks much better now, thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html