On Wednesday, April 29, 2015 at 04:58:15 PM, dinguyen@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.
[...]