From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavel Machek <pavel@denx.de>
Date: Mon, 16 Feb 2015 00:11:59 +0100
Subject: [U-Boot] [PATCHv1 01/22] arm: socfpga: spl: Add main sdram code
In-Reply-To: <1421253662-27222-2-git-send-email-dinguyen@opensource.altera.com>
References: <1421253662-27222-1-git-send-email-dinguyen@opensource.altera.com>
 <1421253662-27222-2-git-send-email-dinguyen@opensource.altera.com>
Message-ID: <20150215231159.GA8297@amd>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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