On 01/14/2015 05:34 PM, Marek Vasut wrote:
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 ?
Yes, I suppose. Move it to drivers/ddr/altera?
++++++++++++++++++++ board/altera/socfpga/sdram/sequencer.h | 504 ++ board/altera/socfpga/sdram/sequencer_auto.h | 216 + .../altera/socfpga/sdram/sequencer_auto_ac_init.c | 88 +
<snip>
@@ -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 ?
Probably not, but I can check again.
+#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.
Will fix in v2.
+#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.
Ok.
+#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.
Will look to fix in V2.
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.
Ok.
+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) .
Ok..
+#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 ;-)
Will fix up in V2.
[...]
+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.
Will fix up in V2.
+#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 ?
Probably...
+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.
Ok.
+#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 ;-)
Ok..
+#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 ... :)
Ok..
+#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?
Ok..
[...]
Thank you for all the work you put into this !
Really appreciate all the comments.
Dinh