From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Sat, 2 May 2015 02:41:08 +0200 Subject: [U-Boot] [U-boot][PATCHv2 2/3] driver/ddr/altera/: Add the sdram calibration portion In-Reply-To: <1430319496-4217-3-git-send-email-dinguyen@opensource.altera.com> References: <1430319496-4217-1-git-send-email-dinguyen@opensource.altera.com> <1430319496-4217-3-git-send-email-dinguyen@opensource.altera.com> Message-ID: <201505020241.09080.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wednesday, April 29, 2015 at 04:58:15 PM, dinguyen at opensource.altera.com wrote: > From: Dinh Nguyen > > This patch adds the DDR calibration portion of the Altera SDRAM driver. > > Signed-off-by: Dinh Nguyen [...] > +/************************************************************************* > ***** + > ************************************************************************** > **** + ** 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. [...]