On Wednesday, January 14, 2015 at 05:40:41 PM, dingu...@opensource.altera.com wrote: > From: Dinh Nguyen <dingu...@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 <dingu...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot