On Wednesday, January 14, 2015 at 05:40:41 PM, dinguyen@opensource.altera.com wrote:
From: Dinh Nguyen dinguyen@opensource.altera.com
Hi!
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
arch/arm/cpu/armv7/socfpga/config.mk | 3 +- board/altera/socfpga/Makefile | 1 + board/altera/socfpga/sdram/Makefile | 12 + board/altera/socfpga/sdram/sdram_config.h | 100 + board/altera/socfpga/sdram/sdram_io.h | 44 + board/altera/socfpga/sdram/sequencer.c | 7993
A general remark: SDRAM stuff should most likely reside in drivers/ddr/ , don't you think so ?
++++++++++++++++++++ board/altera/socfpga/sdram/sequencer.h | 504 ++ board/altera/socfpga/sdram/sequencer_auto.h | 216 + .../altera/socfpga/sdram/sequencer_auto_ac_init.c | 88 + .../socfpga/sdram/sequencer_auto_inst_init.c | 273 + board/altera/socfpga/sdram/sequencer_defines.h | 154 + board/altera/socfpga/sdram/system.h | 15 + 12 files changed, 9402 insertions(+), 1 deletion(-) create mode 100644 board/altera/socfpga/sdram/Makefile create mode 100644 board/altera/socfpga/sdram/sdram_config.h create mode 100644 board/altera/socfpga/sdram/sdram_io.h create mode 100644 board/altera/socfpga/sdram/sequencer.c create mode 100644 board/altera/socfpga/sdram/sequencer.h create mode 100644 board/altera/socfpga/sdram/sequencer_auto.h create mode 100644 board/altera/socfpga/sdram/sequencer_auto_ac_init.c create mode 100644 board/altera/socfpga/sdram/sequencer_auto_inst_init.c create mode 100644 board/altera/socfpga/sdram/sequencer_defines.h create mode 100644 board/altera/socfpga/sdram/system.h
[...]
diff --git a/board/altera/socfpga/sdram/sdram_io.h b/board/altera/socfpga/sdram/sdram_io.h new file mode 100644 index 0000000..f24308d --- /dev/null +++ b/board/altera/socfpga/sdram/sdram_io.h @@ -0,0 +1,44 @@ +/*
- Copyright Altera Corporation (C) 2012-2014. All rights reserved
- SPDX-License-Identifier: BSD-3-Clause
Any chance we can get these files under GPL-2.0+ ? It's not a showstopper, but it'd be nice :)
- */
+#include <asm/io.h>
+#ifdef CONFIG_SPL_SERIAL_SUPPORT +#define HPS_HW_SERIAL_SUPPORT
Are these redundant definitions really needed ?
+#endif
+#define HPS_SDR_BASE SOCFPGA_SDR_ADDRESS
This here as well, just use SOCFPGA_SDR_ADDRESS in the code. Besides, it'd be nice to consistently use <space> after #define keyword.
+#define MGR_SELECT_MASK 0xf8000
+#define APB_BASE_SCC_MGR SDR_PHYGRP_SCCGRP_ADDRESS +#define APB_BASE_PHY_MGR SDR_PHYGRP_PHYMGRGRP_ADDRESS +#define APB_BASE_RW_MGR SDR_PHYGRP_RWMGRGRP_ADDRESS +#define APB_BASE_DATA_MGR SDR_PHYGRP_DATAMGRGRP_ADDRESS +#define APB_BASE_REG_FILE SDR_PHYGRP_REGFILEGRP_ADDRESS +#define APB_BASE_MMR SDR_CTRLGRP_ADDRESS
More redundancy here.
+#define __AVL_TO_APB(ADDR) \
- ((((ADDR) & MGR_SELECT_MASK) == (BASE_PHY_MGR)) ? \
- (APB_BASE_PHY_MGR) | (((ADDR) >> (14-6)) & (0x1<<6)) | \
- ((ADDR) & 0x3f) : \
- (((ADDR) & MGR_SELECT_MASK) == (BASE_RW_MGR)) ? \
- (APB_BASE_RW_MGR) | ((ADDR) & 0x1fff) : \
- (((ADDR) & MGR_SELECT_MASK) == (BASE_DATA_MGR)) ? \
- (APB_BASE_DATA_MGR) | ((ADDR) & 0x7ff) : \
- (((ADDR) & MGR_SELECT_MASK) == (BASE_SCC_MGR)) ? \
- (APB_BASE_SCC_MGR) | ((ADDR) & 0xfff) : \
- (((ADDR) & MGR_SELECT_MASK) == (BASE_REG_FILE)) ? \
- (APB_BASE_REG_FILE) | ((ADDR) & 0x7ff) : \
- (((ADDR) & MGR_SELECT_MASK) == (BASE_MMR)) ? \
- (APB_BASE_MMR) | ((ADDR) & 0xfff) : -1)
This stuff here should be a function, to get a proper typechecking on it.
+#define IOWR_32DIRECT(BASE, OFFSET, DATA) \
- writel(DATA, HPS_SDR_BASE + __AVL_TO_APB((uint32_t)((BASE) + \
- (OFFSET))))
+#define IORD_32DIRECT(BASE, OFFSET) \
- readl(HPS_SDR_BASE + __AVL_TO_APB((uint32_t)((BASE) + (OFFSET))))
OK, these macros look really questionable. You might want to make them into a function too, but before you do so, I'd rather suggest you rethink this entire _AVL_TO_APB usage and make this entire section more readable. The code is really confusing here.
diff --git a/board/altera/socfpga/sdram/sequencer.c b/board/altera/socfpga/sdram/sequencer.c new file mode 100644 index 0000000..c0ba0e5 --- /dev/null +++ b/board/altera/socfpga/sdram/sequencer.c @@ -0,0 +1,7993 @@ +/*
- Copyright Altera Corporation (C) 2012-2014. All rights reserved
- SPDX-License-Identifier: BSD-3-Clause
- */
+#include <common.h> +#include <asm/io.h> +#include <asm/arch/sdram.h> +#include "sdram_io.h" +#include "sequencer.h" +#include "sequencer_auto.h" +#include "sequencer_defines.h" +#include "system.h"
+static void scc_mgr_load_dqs_for_write_group(uint32_t write_group); +static void rw_mgr_mem_dll_lock_wait(void);
+/************************************************************************* ***** +
**** + ** 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. ** +
**** +
****/ + +#if ARRIAV +/*
- Temporary workaround to place the initial stack pointer at a safe
- offset from end
- */
+#define STRINGIFY(s) STRINGIFY_STR(s) +#define STRINGIFY_STR(s) #s
There's already a macro named __stringify(), no need to duplicate it here.
+asm(".global __alt_stack_pointer"); +asm("__alt_stack_pointer = " STRINGIFY(STACK_POINTER)); +#endif
[...]
+/* For HPS running on actual hardware */
+#define DLEVEL 0 +#ifdef HPS_HW_SERIAL_SUPPORT +/*
- space around comma is required for varargs macro to remove comma if
args + * is empty
- */
+#define DPRINT(level, fmt, args...) if (DLEVEL >= (level)) \
printf("SEQ.C: " fmt "\n" , ## args)
This could just use __FILE__ instead of hard-coding seq.c (which is not even the filename anymore) .
+#define IPRINT(fmt, args...) printf("SEQ.C: " fmt "\n" , ##
args)
+#else +#define DPRINT(level, fmt, args...) +#define IPRINT(fmt, args...) +#endif +#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)
+#define TRACE_FUNC(fmt, args...) \
- DPRINT(1, "%s[%d]: " fmt, __func__, __LINE__ , ## args)
I suspect you can use debug() or debug_cond() macro to avoid re-implementing the entire debug infrastructure here ;-) [...]
+void wait_di_buffer(void) +{
- if (debug_data->di_report.cur_samples == NUM_DI_SAMPLE) {
debug_data->di_report.flags |= DI_REPORT_FLAGS_READY;while (debug_data->di_report.cur_samples != 0);
Please get rid of such endless loops, since the platform might get stuck forever in them.
debug_data->di_report.flags = 0;- }
+}
[...]
diff --git a/board/altera/socfpga/sdram/sequencer.h b/board/altera/socfpga/sdram/sequencer.h new file mode 100644 index 0000000..ce4c7bf --- /dev/null +++ b/board/altera/socfpga/sdram/sequencer.h @@ -0,0 +1,504 @@ +/*
- Copyright Altera Corporation (C) 2012-2014. All rights reserved
- SPDX-License-Identifier: BSD-3-Clause
- */
+#ifndef _SEQUENCER_H_ +#define _SEQUENCER_H_
+extern int32_t inst_rom_init_size; +extern uint32_t inst_rom_init[]; +extern uint32_t ac_rom_init_size; +extern uint32_t ac_rom_init[];
Extern variables usually mean the code is not well contained. You might want to review whether making these variables visible globally is really needed or not.
+#if ENABLE_ASSERT +#define ERR_IE_TEXT "Internal Error: Sub-system: %s, File: %s, Line: %d\n%s%s"
This might just use debug_cond() in some way, no ?
+extern void err_report_internal_error(const char *description,
- const char *module, const char *file, int line);
In case of functions, the extern is really not needed. Just define the prototype without the extern keyword.
+#define ALTERA_INTERNAL_ERROR(string) \
- {err_report_internal_error(string, "SEQ", __FILE__, __LINE__); \
- exit(-1); }
+#define ALTERA_ASSERT(condition) \
- if (!(condition)) {\
ALTERA_INTERNAL_ERROR(#condition); }+#define ALTERA_INFO_ASSERT(condition, text) \
- if (!(condition)) {\
ALTERA_INTERNAL_ERROR(text); }
Oh wow, debug_cond() would help, really ;-)
+#else
+#define ALTERA_ASSERT(condition) +#define ALTERA_INFO_ASSERT(condition, text)
[...]
+#if LRDIMM +/* USER RTT_NOM: bits {4,3,2} of the SPD = bits {9,6,2} of the MR */ +#define LRDIMM_SPD_MR_RTT_NOM(spd_byte) \
- ((((spd_byte) & (1 << 4)) << (9-4)) \
- | (((spd_byte) & (1 << 3)) << (6-3)) \
- | (((spd_byte) & (1 << 2)) << (2-2)))
+/* USER RTT_DRV: bits {1,0} of the SPD = bits {5,1} of the MR */ +#define LRDIMM_SPD_MR_RTT_DRV(spd_byte) \ + ((((spd_byte) & (1 << 1)) << (5-1)) \
- | (((spd_byte) & (1 << 0)) << (1-0)))
+/* USER RTT_WR: bits {7,6} of the SPD = bits {10,9} of the MR */ +#define LRDIMM_SPD_MR_RTT_WR(spd_byte) \ + (((spd_byte) & (3 << 6)) << (9-6))
We already have multiple SPD parsers in U-Boot (git grep 'spd' should give you an idea , for example see common/ddr_spd.c ). Could that be used to replace your SPD code ? I might be wrong here, so please correct me if I am ... :)
+#endif /* LRDIMM */ +#endif /* DDR3 */
+#define RW_MGR_MEM_NUMBER_OF_RANKS 1 +#define NUM_SHADOW_REGS 1
+#define RW_MGR_LOAD_CNTR_0 (BASE_RW_MGR + 0x0800) +#define RW_MGR_LOAD_CNTR_1 (BASE_RW_MGR + 0x0804) +#define RW_MGR_LOAD_CNTR_2 (BASE_RW_MGR + 0x0808) +#define RW_MGR_LOAD_CNTR_3 (BASE_RW_MGR + 0x080C)
Can you please rework these register definitions to struct-based ones?
[...]
Thank you for all the work you put into this !
Best regards, Marek Vasut