[U-Boot] [U-boot][PATCHv2 2/3] driver/ddr/altera/: Add the sdram calibration portion

[Marek Vasut <marex@denx.de>]

On Wednesday, April 29, 2015 at 04:58:15 PM, dinguyen at opensource.altera.com 
wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> This patch adds the DDR calibration portion of the Altera SDRAM driver.
> 
> 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.                
>             ** +
> **************************************************************************
> **** +
> **************************************************************************
> ****/ +
> +/*
> + * Temporary workaround to place the initial stack pointer at a safe
> + * offset from end
> + */
> +asm(".global __alt_stack_pointer");
> +asm("__alt_stack_pointer = " __stringify(STACK_POINTER));

This is really scary. The idea here is to have some kind of a storage
which is preserved over warm reboots, right ? Tom, Simon, Albert, don't
we have some kind of a generic abstration for this kind of stuff ?

> +/* Just to make the debugging code more uniform */
> +#ifndef RW_MGR_MEM_NUMBER_OF_CS_PER_DIMM
> +#define RW_MGR_MEM_NUMBER_OF_CS_PER_DIMM 0
> +#endif
> +
> +#define SDR_WRITE	1
> +#define SDR_READ	0
> +
> +#define DELTA_D		1
> +#define MGR_SELECT_MASK		0xf8000
> +
> +/*
> + * In order to reduce ROM size, most of the selectable calibration steps
> are + * decided at compile time based on the user's calibration mode
> selection, + * as captured by the STATIC_CALIB_STEPS selection below.
> + *
> + * However, to support simulation-time selection of fast simulation mode,
> where + * we skip everything except the bare minimum, we need a few of the
> steps to + * be dynamic.  In those cases, we either use the
> DYNAMIC_CALIB_STEPS for the + * check, which is based on the rtl-supplied
> value, or we dynamically compute + * the value to use based on the
> dynamically-chosen calibration mode + */
> +
> +#define BTFLD_FMT "%u"

Nit: Maybe you should just massage this BTFLD_FMT into the code and drop the 
macro.

> +/* For HPS running on actual hardware */
> +
> +#define DLEVEL 0
> +/*
> + * space around comma is required for varargs macro to remove comma if
> args + * is empty
> + */
> +#define BFM_GBL_SET(field, value)
> +#define BFM_GBL_GET(field)		((long unsigned int)0)
> +#define BFM_STAGE(stage)
> +#define BFM_INC_VFIFO
> +#define COV(label)

Are the macros above needed ?

> +#define STATIC_IN_RTL_SIM 0
> +
> +#define STATIC_SKIP_DELAY_LOOPS 0
> +
> +#define STATIC_CALIB_STEPS (STATIC_IN_RTL_SIM | CALIB_SKIP_FULL_TEST | \
> +	STATIC_SKIP_DELAY_LOOPS)
> +
> +/* calibration steps requested by the rtl */
> +uint16_t dyn_calib_steps;

[...]

> +static inline void scc_mgr_set_dm_in_delay(uint32_t write_group,
> +					   uint32_t dm, uint32_t delay)
> +{
> +	/* Load the setting in the SCC manager */
> +	sdr_ops((u32 *)SCC_MGR_IO_IN_DELAY,
> +		(RW_MGR_MEM_DQ_PER_WRITE_DQS + 1 + dm) << 2, delay, SDR_WRITE);

Won't it be better to convert this entire sdr_ops() to something like ...

addr = get_reg_address(...args...);
writel(...value..., addr);

? I think that'd make this code a bit more readable. What do you think ?

> +}
> +

[...]

> +/* find a good dqs enable to use */
> +static uint32_t rw_mgr_mem_calibrate_vfifo_find_dqs_en_phase(uint32_t grp)
> +{
> +	uint32_t i, d, v, p;
> +	uint32_t max_working_cnt;
> +	uint32_t fail_cnt;
> +	uint32_t bit_chk;
> +	uint32_t dtaps_per_ptap;
> +	uint32_t found_begin, found_end;
> +	uint32_t work_bgn, work_mid, work_end, tmp_delay;
> +	uint32_t test_status;
> +	uint32_t found_passing_read, found_failing_read, initial_failing_dtap;
> +
> +	debug("%s:%d %u\n", __func__, __LINE__, grp);
> +	BFM_STAGE("find_dqs_en_phase");
> +
> +	reg_file_set_sub_stage(CAL_SUBSTAGE_VFIFO_CENTER);
> +
> +	scc_mgr_set_dqs_en_delay_all_ranks(grp, 0);
> +	scc_mgr_set_dqs_en_phase_all_ranks(grp, 0);
> +
> +	fail_cnt = 0;
> +
> +	/* ************************************************************** */
> +	/* * Step 0 : Determine number of delay taps for each phase tap * */

You might want to split the steps in this function into separate functions,
since this functions is really extremely long. Would thats work please?

> +	dtaps_per_ptap = 0;
> +	tmp_delay = 0;
> +	while (tmp_delay < IO_DELAY_PER_OPA_TAP) {
> +		dtaps_per_ptap++;
> +		tmp_delay += IO_DELAY_PER_DQS_EN_DCHAIN_TAP;
> +	}
> +	dtaps_per_ptap--;
> +	tmp_delay = 0;

[...]

> diff --git a/drivers/ddr/altera/sequencer.h
> b/drivers/ddr/altera/sequencer.h new file mode 100644
> index 0000000..29f61a8
> --- /dev/null
> +++ b/drivers/ddr/altera/sequencer.h

[...]

> +/* MarkW: how should these base addresses be done for A-V? */
> +#define BASE_PTR_MGR			(0x00040000)
> +#define BASE_SCC_MGR			(0x00058000)
> +#define BASE_REG_FILE			(0x00070000)
> +#define BASE_TIMER			(0x00078000)
> +#define BASE_PHY_MGR			(0x00088000)
> +#define BASE_RW_MGR			(0x00090000)
> +#define BASE_DATA_MGR			(0x00098000)
> +#define BASE_MMR                        (0x000C0000)
> +#define BASE_TRK_MGR			(0x000D0000)

No need for those () around value .

> +#define SCC_MGR_GROUP_COUNTER			(BASE_SCC_MGR + 0x0000)
> +#define SCC_MGR_DQS_IN_DELAY			(BASE_SCC_MGR + 0x0100)
> +#define SCC_MGR_DQS_EN_PHASE			(BASE_SCC_MGR + 0x0200)
> +#define SCC_MGR_DQS_EN_DELAY			(BASE_SCC_MGR + 0x0300)
> +#define SCC_MGR_DQDQS_OUT_PHASE			(BASE_SCC_MGR + 0x0400)
> +#define SCC_MGR_OCT_OUT1_DELAY			(BASE_SCC_MGR + 0x0500)
> +#define SCC_MGR_IO_OUT1_DELAY			(BASE_SCC_MGR + 0x0700)
> +#define SCC_MGR_IO_IN_DELAY			(BASE_SCC_MGR + 0x0900)

Here it _IS_ needed though ;)

[...]

> diff --git a/drivers/ddr/altera/sequencer_auto_ac_init.c
> b/drivers/ddr/altera/sequencer_auto_ac_init.c new file mode 100644
> index 0000000..358a0ca
> --- /dev/null
> +++ b/drivers/ddr/altera/sequencer_auto_ac_init.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright Altera Corporation (C) 2012-2015
> + *
> + * SPDX-License-Identifier:    BSD-3-Clause
> + */
> +
> +const unsigned long ac_rom_init_size = 36;

You can use ARRAY_SIZE() on the array below if that's really needed.
No need to explicitly hardcode values :)

> +const unsigned long ac_rom_init[36] = {

The compiler will compute the size of the array automatically.

> +#ifdef CONFIG_SOCFPGA_ARRIA5
> +/* The if..else... is not required if generated by tools */
> +	0x20700000,
> +	0x20780000,
> +	0x10080831,
> +	0x10080930,
> +	0x10090004,
> +	0x100a0008,
> +	0x100b0000,

[...]

> diff --git a/drivers/ddr/altera/sequencer_auto_inst_init.c
> b/drivers/ddr/altera/sequencer_auto_inst_init.c new file mode 100644
> index 0000000..9983c40
> --- /dev/null
> +++ b/drivers/ddr/altera/sequencer_auto_inst_init.c
> @@ -0,0 +1,270 @@
> +/*
> + * Copyright Altera Corporation (C) 2012-2015
> + *
> + * SPDX-License-Identifier:    BSD-3-Clause
> + */
> +
> +#ifdef CONFIG_SOCFPGA_ARRIA5
> +/* The if..else... is not required if generated by tools */
> +const alt_u32 inst_rom_init_size = 126;
> +const alt_u32 inst_rom_init[126] = {

DTTO here.

[...]