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

[Marek Vasut <marex@denx.de>]

On Tuesday, July 28, 2015 at 03:58:34 PM, Pavel Machek wrote:
> 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.

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.

> +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.

You can check the return value of the function, I explicitly don't want
to modify the output args in case of a failure :)

> +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.

> +       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.

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.

> +       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.

This is also on my list and will do that after the series is in -- this
and the type cleanup.

> 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 ...