[U-Boot] [PATCHv1 01/22] arm: socfpga: spl: Add main sdram code

Pavel Machek
2015-02-15T23:11:59Z

Hi!

> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>
> This adds the code to configure the SDRAM controller that is found in the
> SoCFGPA Cyclone5 and Arria5 platforms.
>
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>

> +/******************************************************************************
> + ******************************************************************************
> + ** NOTE: Special Rules for Globale Variables **
> + ** **
> + ** All global variables that are explicitly initialized (including **
> + ** explicitly initialized to zero), are only initialized once, during **
> + ** configuration time, and not again on reset. This means that they **
> + ** preserve their current contents across resets, which is needed for some **
> + ** special cases involving communication with external modules. In **
> + ** addition, this avoids paying the price to have the memory initialized, **
> + ** even for zeroed data, provided it is explicitly set to zero in the code, **
> + ** and doesn't rely on implicit initialization. **
> + ******************************************************************************
> + ******************************************************************************/

Comment coding style, please?

And they not globale variables.

But more importantly: is it good idea? What state is shared over
reset? What are the special cases this is needed for?

What happens, when kernel corrupts that state?

> +/*
> + * case:56390
> + * VFIFO_CONTROL_WIDTH_PER_DQS is the number of VFIFOs actually instantiated
> + * per DQS. This is always one except:
> + * AV QDRII where it is 2 for x18 and x18w2, and 4 for x36 and x36w2
> + * RLDRAMII x36 and x36w2 where it is 2.
> + * In 12.0sp1 we set this to 4 for all of the special cases above to
> + * keep it simple.
> + * In 12.0sp2 or 12.1 this should get moved to generation and unified with
> + * the same constant used in the phy mgr
> + */

Does case: refer to something public?

> +/*
> + * Given a rank, select the set of shadow registers that is responsible
> + * for the delays of such rank, so that subsequent SCC updates will
> + * go to those shadow registers.
> + */
> +void select_shadow_regs_for_update(uint32_t rank, uint32_t group,
> + uint32_t update_scan_chains)
> +{
> +#if USE_SHADOW_REGS
> + uint32_t rank_one_hot = (0xFF & (1 << rank));

Does this need to be configurable?

Best regards,

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