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
