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

[Dinh Nguyen <dinguyen@opensource.altera.com>]

On 01/14/2015 05:34 PM, Marek Vasut wrote:
> On Wednesday, January 14, 2015 at 05:40:41 PM, dinguyen at 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