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

Pavel Machek
2015-07-28T13:58:34Z

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