[U-Boot] [PATCHv1 01/22] arm: socfpga: spl: Add main sdram code

[Marek Vasut]

On Wednesday, January 14, 2015 at 05:40:41 PM, dinguyen at opensource.altera.com 
wrote:
> From: Dinh Nguyen <dinguyen at 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 at 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