socfpga: SPL and DDR init
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest0 of 172 patches 0 repliesNew
Patches in v1 No applies recorded yet
None of the 172 patches the cover letter announces are in this archive.
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [U-Boot] [PATCH 000/172] socfpga: SPL and DDR init
Quoted 38 lines · Pavel Machek
Quoted 10 lines · Marek Vasut
Quoted 9 lines · Pavel Machek
Hi!
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
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/sockitThanks!
Random notes:
(quoted diff) +12
-0+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",
(quoted diff) +2
-0+ start, stop); +
Quoted 4 lines
if ((start | end) & (...))
debug()..and you get rid of a variable.
This is what I was afraid of -- you not commenting on a specific patch, but at bulk on a series which doesn't even contain the patch :'-(
I'd like to get some feedback on that patch from Albert/Tom though.
Quoted 24 lines
(quoted diff) +23
-0+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;
Quoted 1 line
I'd do *table = NULL,
*table_len= 0 here, to catch any mistakes.
You can check the return value of the function, I explicitly don't want to modify the output args in case of a failure :)
Quoted 6 lines
(quoted diff) +4
-0+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?
Yes, linting the rest for proper types is on my list, there is much more than these two variables.
Quoted 10 lines
+ rw_mgr_mem_load_user(RW_MGR_MRS0_USER_MIRR,
RW_MGR_MRS0_USER,
1);(quoted diff) +3
-0+ /* + * Need to wait tMOD (12CK or 15ns) time before issuing other + * commands, but we will have plenty of NIOS cycles beforeactual
(quoted diff) +2
-0+ * handoff so its okay. + */I don't understand this comment.
There's plenty of code between issuing the above instruction and triggering the mem handoff, so we don't need to add explicit delay into this function. Also, 15nS is like 15 instructions on the SoCFPGA, so there is zero chance we will trigger the handoff too early.
Quoted 16 lines
(quoted diff) +7
-0+ 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 passin
(quoted diff) +7
-0+ * first case). + */ + if (d > 2) + scc_mgr_zero_group(rw_group, 1); + + return 1; +}
Quoted 2 lines
Other functions use 0/-EIO protocol here, so it would be good to be consistent.
This is also on my list and will do that after the series is in -- this and the type cleanup.
Quoted 1 line
It looks much better now, thanks,
It's far from good though and there will be another series after this one I'm afraid. It might be even heftier, but I want to open the door to proper SoCFPGA support this MW even if it means blowing them to shreds ...