Add SPL support for SoCFGPA

From: Dinh Nguyen <[email protected]>

This adds the code to configure the SDRAM controller that is found in the SoCFGPA Cyclone5 and Arria5 platforms.

Signed-off-by Dinh Nguyen <[email protected]>

---
 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

++++++++++++++++++++ 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

board/altera/socfpga/sdram/sdram_io.h new file +5 -0

+#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))   ? \

board/altera/socfpga/sdram/sdram_io.h new file +1 -0

+

(quoted diff) +0 -0

b/board/altera/socfpga/sdram/sequencer.c new file mode 100644 index 0000000..c0ba0e5

board/altera/socfpga/sdram/sequencer.c new file +19 -0

@@ -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.

            ** +

**************************************************************************
**** +
**************************************************************************
****/ +

board/altera/socfpga/sdram/sequencer.c new file +3 -0

+asm(".global __alt_stack_pointer");
+asm("__alt_stack_pointer = " STRINGIFY(STACK_POINTER));
+#endif

+#define IPRINT(fmt, args...) printf("SEQ.C: " fmt "\n" , ##

board/altera/socfpga/sdram/sequencer.c new file +3 -0

+		debug_data->di_report.flags = 0;
+	}
+}

(quoted diff) +0 -0

b/board/altera/socfpga/sdram/sequencer.h new file mode 100644 index 0000000..ce4c7bf

board/altera/socfpga/sdram/sequencer.h new file +3 -0

@@ -0,0 +1,504 @@
+/*
+ * Copyright Altera Corporation (C) 2012-2014. All rights reserved
+ *

board/altera/socfpga/sdram/sequencer.h new file +2 -0

+#if ENABLE_ASSERT
+#define ERR_IE_TEXT "Internal Error: Sub-system: %s, File: %s, Line:

%d\n%s%s"

board/altera/socfpga/sdram/sequencer.h new file +4 -0

+#else
+
+#define ALTERA_ASSERT(condition)
+#define ALTERA_INFO_ASSERT(condition, text)

board/altera/socfpga/sdram/sequencer.h new file +10 -0

+
+#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)) \

board/altera/socfpga/sdram/sequencer.h new file +4 -0

+	| (((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))

board/altera/socfpga/sdram/sequencer.c line 243

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c new file +6 -0

+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.

Hi!

Quoted 9 lines

board/altera/socfpga/sdram/sequencer.c line 243

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c new file +6 -0

+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.

What would you propose in case of timeout? Message on serial console and reboot?

Endless loop should trigger watchdog, anyway, no?

Quoted 1 line · Pavel Machek

Hi!

Hi!

Quoted 10 lines

Quoted 9 lines

board/altera/socfpga/sdram/sequencer.c line 243

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c new file +6 -0

+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.

Quoted 2 lines

What would you propose in case of timeout? Message on serial console and reboot?

Yes, reboot and try again would work I suspect.

Quoted 1 line

Endless loop should trigger watchdog, anyway, no?

If and only if the WDT is enabled, which is not necessarily the case.

Quoted 1 line · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Hi!

Quoted 11 lines

This adds the code to configure the SDRAM controller that is found in the SoCFGPA Cyclone5 and Arria5 platforms.

Signed-off-by Dinh Nguyen <[email protected]>

---
 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 ?

Quoted 5 lines

++++++++++++++++++++ board/altera/socfpga/sdram/sequencer.h |
504 ++

 board/altera/socfpga/sdram/sequencer_auto.h        |  216 +
 .../altera/socfpga/sdram/sequencer_auto_ac_init.c  |   88 +

board/altera/socfpga/sdram/sdram_config.h line 4

Quoted 5 lines

board/altera/socfpga/sdram/sdram_config.h new file +4 -0

@@ -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 :)

board/altera/socfpga/sdram/sdram_io.h line 10

Quoted 6 lines

board/altera/socfpga/sdram/sdram_io.h new file +6 -0

+ */
+
+#include <asm/io.h>
+
+#ifdef CONFIG_SPL_SERIAL_SUPPORT
+#define HPS_HW_SERIAL_SUPPORT

Are these redundant definitions really needed ?

board/altera/socfpga/sdram/sdram_io.h line 13

Quoted 4 lines

board/altera/socfpga/sdram/sdram_io.h new file +3 -0

+#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.

board/altera/socfpga/sdram/sdram_io.h line 21

Quoted 8 lines

board/altera/socfpga/sdram/sdram_io.h new file +8 -0

+#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.

Quoted 10 lines

board/altera/socfpga/sdram/sdram_io.h new file +9 -0

+#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))  ? \

board/altera/socfpga/sdram/sdram_io.h line 37

Quoted 6 lines

board/altera/socfpga/sdram/sdram_io.h new file +6 -0

+	(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.

board/altera/socfpga/sdram/sdram_io.h line 43

Quoted 7 lines

board/altera/socfpga/sdram/sdram_io.h new file +6 -0

+#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.

Quoted 45 lines

(quoted diff) +1 -0

+

(quoted diff) +0 -0

b/board/altera/socfpga/sdram/sequencer.c new file mode 100644 index 0000000..c0ba0e5

board/altera/socfpga/sdram/sequencer.c new file +19 -0

@@ -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

board/altera/socfpga/sdram/sequencer.c line 40

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c new file +6 -0

+/*
+ * 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.

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.c new file +3 -0

+asm(".global __alt_stack_pointer");
+asm("__alt_stack_pointer = " STRINGIFY(STACK_POINTER));
+#endif

[...]

board/altera/socfpga/sdram/sequencer.c line 133

Quoted 10 lines

board/altera/socfpga/sdram/sequencer.c new file +6 -0

+/* 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
+ */

board/altera/socfpga/sdram/sequencer.c new file +2 -0

+#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) .

Quoted 2 lines

+#define IPRINT(fmt, args...) printf("SEQ.C: " fmt "\n" , ##

args)

board/altera/socfpga/sdram/sequencer.c line 146

Quoted 12 lines

board/altera/socfpga/sdram/sequencer.c new file +12 -0

+#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 ;-)

[...]

board/altera/socfpga/sdram/sequencer.c line 243

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c new file +6 -0

+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.

Quoted 3 lines

board/altera/socfpga/sdram/sequencer.c new file +3 -0

+		debug_data->di_report.flags = 0;
+	}
+}

[...]

Quoted 15 lines

(quoted diff) +0 -0

b/board/altera/socfpga/sdram/sequencer.h new file mode 100644 index 0000000..ce4c7bf

board/altera/socfpga/sdram/sequencer.h new file +9 -0

@@ -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_
+

board/altera/socfpga/sdram/sequencer.h line 13

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.h new file +4 -0

+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.

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.h new file +2 -0

+#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 ?

board/altera/socfpga/sdram/sequencer.h line 19

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.h new file +2 -0

+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.

board/altera/socfpga/sdram/sequencer.h line 30

Quoted 11 lines

board/altera/socfpga/sdram/sequencer.h new file +10 -0

+#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 ;-)

Quoted 5 lines

board/altera/socfpga/sdram/sequencer.h new file +4 -0

+#else
+
+#define ALTERA_ASSERT(condition)
+#define ALTERA_INFO_ASSERT(condition, text)

[...]

Quoted 12 lines

board/altera/socfpga/sdram/sequencer.h new file +10 -0

+
+#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)) \

board/altera/socfpga/sdram/sequencer.h new file +1 -0

+	| (((spd_byte) & (1 << 0)) << (1-0)))

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.h new file +3 -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 ... :)

board/altera/socfpga/sdram/sequencer.h line 87

Quoted 11 lines

board/altera/socfpga/sdram/sequencer.h new file +10 -0

+#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 !

Quoted 18 lines · Marek Vasut

Quoted 1 line · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Hi!

Quoted 11 lines

This adds the code to configure the SDRAM controller that is found in the SoCFGPA Cyclone5 and Arria5 platforms.

Signed-off-by Dinh Nguyen <[email protected]>

---
  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

Quoted 2 lines

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?

From: Dinh Nguyen <[email protected]>

This adds the code to configure the SDRAM controller that is found in the SoCFGPA Cyclone5 and Arria5 platforms.

Signed-off-by Dinh Nguyen <[email protected]>

+/******************************************************************************
+ ******************************************************************************

+ ** 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. **
+ ******************************************************************************
+ ******************************************************************************/

+/*
+ * case:56390
+ * VFIFO_CONTROL_WIDTH_PER_DQS is the number of VFIFOs actually instantiated
+ * per DQS. This is always one except:
+ * AV QDRII where it is 2 for x18 and x18w2, and 4 for x36 and x36w2
+ * RLDRAMII x36 and x36w2 where it is 2.
+ * In 12.0sp1 we set this to 4 for all of the special cases above to
+ * keep it simple.
+ * In 12.0sp2 or 12.1 this should get moved to generation and unified with
+ * the same constant used in the phy mgr
+ */

+#if ENABLE_BRINGUP_DEBUGGING

Hi!

Quoted 1 line

+#if ENABLE_BRINGUP_DEBUGGING

Could we get rid of this for initial merge?

board/altera/socfpga/sdram/sequencer.c line 415

Quoted 5 lines

board/altera/socfpga/sdram/sequencer.c new file +4 -0

+static inline void reg_file_set_sub_stage(uint32_t set_sub_stage)
+{
+	/* Read the current group and stage */
+	uint32_t cur_stage_group = IORD_32DIRECT(REG_FILE_CUR_STAGE, 0);

Could normal u-boot memory access macros be used here?

Quoted 5 lines · Pavel Machek

Hi!

Quoted 1 line

+#if ENABLE_BRINGUP_DEBUGGING

Could we get rid of this for initial merge?

Yeah, it can be removed.

Quoted 1 line

Quoted 7 lines

board/altera/socfpga/sdram/sequencer.c line 415

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.c new file +4 -0

+static inline void reg_file_set_sub_stage(uint32_t set_sub_stage)
+{
+	/* Read the current group and stage */
+	uint32_t cur_stage_group = IORD_32DIRECT(REG_FILE_CUR_STAGE, 0);

Could normal u-boot memory access macros be used here?

I'll look..

Quoted 16 lines · Dinh Nguyen

Quoted 5 lines · Pavel Machek

Hi!

Quoted 1 line

+#if ENABLE_BRINGUP_DEBUGGING

Could we get rid of this for initial merge?

Yeah, it can be removed.

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c line 415

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.c new file +4 -0

+static inline void reg_file_set_sub_stage(uint32_t set_sub_stage)
+{
+	/* Read the current group and stage */
+	uint32_t cur_stage_group = IORD_32DIRECT(REG_FILE_CUR_STAGE, 0);

Could normal u-boot memory access macros be used here?

Quoted 1 line

I'll look..

Actually, I'm reworking this whole IORD_* that uses the AVL_TO_APB to a function. This was already commented by Marek.

Quoted 17 lines · Dinh Nguyen

Quoted 16 lines · Dinh Nguyen

Quoted 5 lines · Pavel Machek

Hi!

Quoted 1 line

+#if ENABLE_BRINGUP_DEBUGGING

Could we get rid of this for initial merge?

Yeah, it can be removed.

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c line 415

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.c new file +4 -0

+static inline void reg_file_set_sub_stage(uint32_t set_sub_stage)
+{
+	/* Read the current group and stage */
+	uint32_t cur_stage_group = IORD_32DIRECT(REG_FILE_CUR_STAGE, 0);

Could normal u-boot memory access macros be used here?

Quoted 4 lines

Quoted 1 line

I'll look..

Actually, I'm reworking this whole IORD_* that uses the AVL_TO_APB to a function. This was already commented by Marek.

If this can be switched to regular readl()/writel() stuff, then please do so, since that'd be much less confusing.

Quoted 20 lines · Marek Vasut

Quoted 19 lines · Dinh Nguyen

Quoted 17 lines · Dinh Nguyen

Quoted 5 lines · Pavel Machek

Hi!

Quoted 1 line

+#if ENABLE_BRINGUP_DEBUGGING

Could we get rid of this for initial merge?

Yeah, it can be removed.

Quoted 6 lines

board/altera/socfpga/sdram/sequencer.c line 415

Quoted 4 lines

board/altera/socfpga/sdram/sequencer.c new file +4 -0

+static inline void reg_file_set_sub_stage(uint32_t set_sub_stage)
+{
+	/* Read the current group and stage */
+	uint32_t cur_stage_group = IORD_32DIRECT(REG_FILE_CUR_STAGE, 0);

Could normal u-boot memory access macros be used here?

I'll look..

Quoted 5 lines

Quoted 2 lines

Actually, I'm reworking this whole IORD_* that uses the AVL_TO_APB to a function. This was already commented by Marek.

If this can be switched to regular readl()/writel() stuff, then please do so, since that'd be much less confusing.

Yes, that is what I've done.

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Tom Rini <[email protected]>

Cc Pavel Machek <[email protected]>

---
 include/configs/socfpga_common.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/include/configs/socfpga_common.h index 6b1f967..673377b 100644

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Tom Rini <[email protected]>

Cc Pavel Machek <[email protected]>

---
 include/configs/socfpga_common.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

include/configs/socfpga_common.h +0 -0

index 6b1f967..673377b 100644

Hi!

Quoted 25 lines

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Tom Rini <[email protected]>

Cc Pavel Machek <[email protected]>

---

 include/configs/socfpga_common.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/include/configs/socfpga_common.h index 6b1f967..673377b 100644

include/configs/socfpga_common.h +0 -1

@@ -281,9 +281,12 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 
 #define CONFIG_SYS_CONSOLE_IS_IN_ENV
 #define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE
 #define CONFIG_SYS_CONSOLE_ENV_OVERWRITE
 
-#define CONFIG_ENV_IS_NOWHERE
 
 #define CONFIG_ENV_SIZE			4096
 

include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 289

Quoted 4 lines

include/configs/socfpga_common.h +4 -0

+#define CONFIG_ENV_IS_IN_MMC
+#define CONFIG_SYS_MMC_ENV_DEV		0/* device 0 */
+#define CONFIG_ENV_OFFSET		512/* just after the MBR */
+

Would it make sense to make environment bigger (64K?) at this point, and enabling the redundant environment option, so that one bad block does not bring the whole device down?

Quoted 14 lines · Pavel Machek

Quoted 14 lines

(quoted diff) +0 -0

b/include/configs/socfpga_common.h index 6b1f967..673377b 100644

include/configs/socfpga_common.h +0 -1

@@ -281,9 +281,12 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 
  #define CONFIG_SYS_CONSOLE_IS_IN_ENV
  #define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE
  #define CONFIG_SYS_CONSOLE_ENV_OVERWRITE
 
-#define CONFIG_ENV_IS_NOWHERE
 
  #define CONFIG_ENV_SIZE			4096
 

Quoted 8 lines

include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 289

Quoted 4 lines

include/configs/socfpga_common.h +4 -0

+#define CONFIG_ENV_IS_IN_MMC
+#define CONFIG_SYS_MMC_ENV_DEV		0/* device 0 */
+#define CONFIG_ENV_OFFSET		512/* just after the MBR */
+

Would it make sense to make environment bigger (64K?) at this point, and enabling the redundant environment option, so that one bad block does not bring the whole device down?

Do you think you'll ever have more than a 4KiB of text data in the env ?

I can see a reason for some 16 KiB environment, but that's the ceiling.

Usually, the env only has to be a few KiB and it's size has to be multiple of the underlying device's erase block. But in case of SD card, there are no explicit erase blocks exported to the user, all is hidden by the controller in the SD card and gives an illusion of a device which has 512b blocks and the user doesn't care for bad block management. Since the controller in the card does the bad block management and wear leveling/relocation of blocks, it also means that in case the controller did fail to provide the block which contains env, the whole card would already be in a very bad state.

Quoted 5 lines

Quoted 3 lines

Would it make sense to make environment bigger (64K?) at this point, and enabling the redundant environment option, so that one bad block does not bring the whole device down?

Do you think you'll ever have more than a 4KiB of text data in the env ?

I have used bigger environment in some cases.

Quoted 1 line

I can see a reason for some 16 KiB environment, but that's the ceiling.

Then lets switch to at least 8KiB please.

Pavel, would be 8KiB enough for your project?

Quoted 9 lines

Usually, the env only has to be a few KiB and it's size has to be multiple of the underlying device's erase block. But in case of SD card, there are no explicit erase blocks exported to the user, all is hidden by the controller in the SD card and gives an illusion of a device which has 512b blocks and the user doesn't care for bad block management. Since the controller in the card does the bad block management and wear leveling/relocation of blocks, it also means that in case the controller did fail to provide the block which contains env, the whole card would already be in a very bad state.

Right. Nevertheless it makes sense to support the redundant envrionment (at least from my point of view), regardless of the device the evironment is stored in. Thinking for example about power-supply failure (power-cut) while writing into the one envrionment version.

Quoted 4 lines

Quoted 4 lines

-#define CONFIG_ENV_IS_NOWHERE

#define CONFIG_ENV_SIZE 4096

Quoted 8 lines

include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 289

Quoted 4 lines

include/configs/socfpga_common.h +4 -0

+#define CONFIG_ENV_IS_IN_MMC
+#define CONFIG_SYS_MMC_ENV_DEV		0/* device 0 */
+#define CONFIG_ENV_OFFSET		512/* just after the MBR */
+

Would it make sense to make environment bigger (64K?) at this point, and enabling the redundant environment option, so that one bad block does not bring the whole device down?

Do you think you'll ever have more than a 4KiB of text data in the env ? I can see a reason for some 16 KiB environment, but that's the ceiling.

Usually, the env only has to be a few KiB and it's size has to be multiple of the underlying device's erase block. But in case of SD card, there are no explicit erase blocks exported to the user, all is hidden by the controller in the SD card and gives an illusion of a device which has 512b blocks and

the user doesn't care for bad block management. Since the controller in the card does the bad block management and wear leveling/relocation of blocks, it also means that in case the controller did fail to provide the block which contains env, the whole card would already be in a very bad state.

Hi!

Quoted 16 lines

Quoted 4 lines

Quoted 4 lines

-#define CONFIG_ENV_IS_NOWHERE

#define CONFIG_ENV_SIZE 4096

Quoted 8 lines

include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 289

Quoted 4 lines

include/configs/socfpga_common.h +4 -0

+#define CONFIG_ENV_IS_IN_MMC
+#define CONFIG_SYS_MMC_ENV_DEV		0/* device 0 */
+#define CONFIG_ENV_OFFSET		512/* just after the MBR */
+

Would it make sense to make environment bigger (64K?) at this point, and enabling the redundant environment option, so that one bad block does not bring the whole device down?

Do you think you'll ever have more than a 4KiB of text data in the env ? I can see a reason for some 16 KiB environment, but that's the

Quoted 1 line

ceiling.

We have hit 4KiB size already, and I believe we are over 8KiB, even. wagabuibui loads a lot of small files from flash.

So if 16KiB is acceptable, lets do that.

Quoted 6 lines

Usually, the env only has to be a few KiB and it's size has to be multiple of the underlying device's erase block. But in case of SD card, there are no explicit erase blocks exported to the user, all is hidden by the controller in the SD card and gives an illusion of a device which has 512b
blocks and

Allocation unit is usually 2, 4, 6, 8, 12 or 16 MiB on recent flash cards. It is field in uSD card structure somewhere. We could choose 24MiB, to have GCD, but... see:

https://wiki.linaro.org/WorkingGroups/KernelArchived/Projects/FlashCardSurv
ey

Quoted 5 lines

the user doesn't care for bad block management. Since the controller in the card does the bad block management and wear leveling/relocation of blocks, it also means that in case the controller did fail to provide the block which contains env, the whole card would already be in a very bad state.

Bad block management means you don't get errors during write, but you can still get errors during read.

From: Dinh Nguyen <[email protected]>

Add SDRAM initialization for the SOCFPGA platform.

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/socfpga/Makefile       |    2 +-
 arch/arm/cpu/armv7/socfpga/sdram.c        | 1311
+++++++++++++++++++++++++++++ arch/arm/include/asm/arch-socfpga/sdram.h | 
434 ++++++++++
 include/configs/socfpga_cyclone5.h        |    1 +
 4 files changed, 1747 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/cpu/armv7/socfpga/sdram.c
 create mode 100644 arch/arm/include/asm/arch-socfpga/sdram.h

From: Dinh Nguyen <[email protected]>

Add a section of SRAM to the SPL linker file.

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

Update SPL linker file to use SRAM.

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

These functions will be needed for use by the SPL for enabling the console and sdram initialization.

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

These functions will be needed for use by the SPL for enabling the console and sdram initialization.

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/socfpga/spl.c | 4 ++++
 1 file changed, 4 insertions(+)

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/spl.c index bd9f338..b123336 100644

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

Quoted 3 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

You might want to check common/spl/spl.c , which implements most of the standard SPL init procedure. It even includes the call to timer_init() there in board_init_r() . I wonder if you really need to call it here one more time. Do you ?

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

Quoted 3 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

Acked-by Pavel Machek <[email protected]>

...Actually, you can merge this with 7/10, I guess. One 2000 lines patch + twenty 10 line patches is not "interesting" form of series

From: Dinh Nguyen <[email protected]>

Add a call to checkboard along with sdram intilialization and calibration.

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/socfpga/spl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/spl.c index 9ef2983..9481fd5 100644

arch/arm/cpu/armv7/socfpga/spl.c +1 -0

@@ -16,6 +16,7 @@
 #include <asm/arch/freeze_controller.h>
 #include <asm/arch/clock_manager.h>
 #include <asm/arch/scan_manager.h>
+#include <asm/arch/sdram.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 

From: Dinh Nguyen <[email protected]>

Add a call to checkboard along with sdram intilialization and calibration.

arch/arm/cpu/armv7/socfpga/spl.c +0 -0

 	/* enable console uart printing */
 	preloader_console_init();

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/socfpga/spl.c | 4 ++++
 1 file changed, 4 insertions(+)

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/spl.c index 9481fd5..16ca8b0 100644

arch/arm/cpu/armv7/socfpga/spl.c +1 -0

@@ -55,6 +55,7 @@ u32 spl_boot_device(void)
  */
 void spl_board_init(void)
 {
+	unsigned long sdram_size;
 #ifndef CONFIG_SOCFPGA_VIRTUAL_TARGET
 	cm_config_t cm_default_cfg = {
 		/* main group */

From: Dinh Nguyen <[email protected]>

For SoCFGPA, use the common ARMv7 lowlevel_init.

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/Makefile                | 2 +-
 arch/arm/cpu/armv7/socfpga/lowlevel_init.S | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

arch/arm/cpu/armv7/Makefile +0 -1

index 409e6f5..d0434b1 100644
@@ -12,7 +12,7 @@ obj-y	+= cache_v7.o
 obj-y	+= cpu.o
 obj-y	+= syslib.o
 
-ifneq

($(CONFIG_AM43XX)$(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CON FIG_TEGRA)$(CONFIG_MX6)$(CONFIG_TI81XX)$(CONFIG_AT91FAMILY)$(CONFIG_SUNXI), ) +ifneq ($(CONFIG_AM43XX)$(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CON FIG_TEGRA)$(CONFIG_MX6)$(CONFIG_TI81XX)$(CONFIG_AT91FAMILY)$(CONFIG_SUNXI)$ (CONFIG_SOCFPGA),) ifneq ($(CONFIG_SKIP_LOWLEVEL_INIT),y)
obj-y += lowlevel_init.o
endif

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/lowlevel_init.S index 2f2e9fc..f251f07 100644

From: Dinh Nguyen <[email protected]>

s_init will map SDRAM to 0x0.

Signed-off-by Dinh Nguyen <[email protected]>

arch/arm/cpu/armv7/socfpga/s_init.c new file +15 -0

+void s_init(void)
+{
+#ifdef CONFIG_SPL_BUILD
+	struct socfpga_system_manager *sysmgr_regs =
+		(struct socfpga_system_manager *)SOCFPGA_SYSMGR_ADDRESS;
+	unsigned long reg;
+	/*
+	 * First C code to run. Clear fake OCRAM ECC first as SBE
+	 * and DBE might triggered during power on
+	 */
+	reg = readl(&sysmgr_regs->eccgrp_ocram);
+	if (reg & SYSMGR_ECC_OCRAM_SERR)
+		writel(SYSMGR_ECC_OCRAM_SERR | SYSMGR_ECC_OCRAM_EN,
+			&sysmgr_regs->eccgrp_ocram);
+	if (reg & SYSMGR_ECC_OCRAM_DERR)

arch/arm/cpu/armv7/socfpga/s_init.c new file +1 -0

+}

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-socfpga/system_manager.h index 51d9815..10659f7 100644

arch/arm/include/asm/arch-socfpga/system_manager.h +0 -0

@@ -144,4 +144,14 @@ struct socfpga_system_manager {
 #define SYSMGR_EMACGRP_CTRL_PHYSEL1_LSB			2
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK			0x3

arch/arm/include/asm/arch-socfpga/system_manager.h +0 -0

 #endif /* _SYSTEM_MANAGER_H_ */

Quoted 5 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

s_init will map SDRAM to 0x0.

Signed-off-by Dinh Nguyen <[email protected]>

Hi!

[...]

Quoted 20 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +20 -0

+void s_init(void)
+{
+#ifdef CONFIG_SPL_BUILD
+	struct socfpga_system_manager *sysmgr_regs =
+		(struct socfpga_system_manager *)SOCFPGA_SYSMGR_ADDRESS;
+	unsigned long reg;
+	/*
+	 * First C code to run. Clear fake OCRAM ECC first as SBE
+	 * and DBE might triggered during power on
+	 */
+	reg = readl(&sysmgr_regs->eccgrp_ocram);
+	if (reg & SYSMGR_ECC_OCRAM_SERR)
+		writel(SYSMGR_ECC_OCRAM_SERR | SYSMGR_ECC_OCRAM_EN,
+			&sysmgr_regs->eccgrp_ocram);
+	if (reg & SYSMGR_ECC_OCRAM_DERR)
+		writel(SYSMGR_ECC_OCRAM_DERR  | SYSMGR_ECC_OCRAM_EN,
+			&sysmgr_regs->eccgrp_ocram);
+#else
+	/*
+	 * Private components security

arch/arm/cpu/armv7/socfpga/s_init.c line 43

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+	 * U-Boot : configure private timer, global timer and cpu
+	 * component access as non secure for kernel stage (as required
+	 * by kernel)
+	 */
+	setbits_le32(SOCFPGA_SCU_SNSAC, 0xfff);

We do this in socfpga/misc.c in arch_early_init_r() already. You should consolidate this code so we don't have it twice I think.

arch/arm/cpu/armv7/socfpga/s_init.c line 48

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+
+#endif	/* CONFIG_SPL_BUILD */
+
+	/* Configure the L2 controller to make SDRAM start at 0	*/
+	writel(0x1, (SOCFPGA_MPUL2_ADDRESS + SOCFPGA_MPUL2_ADRFLTR_START));

This as well.

Quoted 10 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +1 -0

+}

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-socfpga/system_manager.h index 51d9815..10659f7 100644

arch/arm/include/asm/arch-socfpga/system_manager.h +0 -0

@@ -144,4 +144,14 @@ struct socfpga_system_manager {
 #define SYSMGR_EMACGRP_CTRL_PHYSEL1_LSB			2
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK			0x3
 

arch/arm/include/asm/arch-socfpga/system_manager.h struct socfpga_system_manager { · line 155

Quoted 9 lines

arch/arm/include/asm/arch-socfpga/system_manager.h +9 -0

+/*
+ * SCU Non-secure Access Control
+ */
+#define SOCFPGA_SCU_SNSAC	(SOCFPGA_MPUSCU_ADDRESS + 0x54)
+/*
+ * L2 PL-310
+ * for configuring L2 address filtering start address in assembly.
+ */
+#define SOCFPGA_MPUL2_ADRFLTR_START			(0xC00)

I think this is not needed if you use the code from misc.c , since these registers are already defined there.

On 01/14/2015 05:54 PM, Marek Vasut wrote:

Quoted 10 lines

arch/arm/cpu/armv7/socfpga/s_init.c line 43

Quoted 7 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +7 -0

+	/*
+	 * Private components security
+	 * U-Boot : configure private timer, global timer and cpu
+	 * component access as non secure for kernel stage (as required
+	 * by kernel)
+	 */
+	setbits_le32(SOCFPGA_SCU_SNSAC, 0xfff);

We do this in socfpga/misc.c in arch_early_init_r() already. You should consolidate this code so we don't have it twice I think.

Yes, this can be removed.

Quoted 7 lines

arch/arm/cpu/armv7/socfpga/s_init.c line 48

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+
+#endif	/* CONFIG_SPL_BUILD */
+
+	/* Configure the L2 controller to make SDRAM start at 0	*/
+	writel(0x1, (SOCFPGA_MPUL2_ADDRESS + SOCFPGA_MPUL2_ADRFLTR_START));

This as well.

This call has to stay because we want to map the SDRAM to 0x0 in the SPL. The call in arch_early_init_r() is in u-boot, and thus is too late.

Quoted 4 lines

Quoted 4 lines

arch/arm/include/asm/arch-socfpga/system_manager.h +4 -0

+/*
+ * SCU Non-secure Access Control
+ */
+#define SOCFPGA_SCU_SNSAC	(SOCFPGA_MPUSCU_ADDRESS + 0x54)

Quoted 8 lines

arch/arm/include/asm/arch-socfpga/system_manager.h struct socfpga_system_manager { · line 155

Quoted 5 lines

arch/arm/include/asm/arch-socfpga/system_manager.h +5 -0

+/*
+ * L2 PL-310
+ * for configuring L2 address filtering start address in assembly.
+ */
+#define SOCFPGA_MPUL2_ADRFLTR_START			(0xC00)

I think this is not needed if you use the code from misc.c , since these registers are already defined there.

Yes, I can remove this and use the mappings already defined.

Quoted 1 line · Dinh Nguyen

On 01/14/2015 05:54 PM, Marek Vasut wrote:

[...]

Quoted 11 lines

Quoted 10 lines

arch/arm/cpu/armv7/socfpga/s_init.c line 43

Quoted 7 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +7 -0

+	/*
+	 * Private components security
+	 * U-Boot : configure private timer, global timer and cpu
+	 * component access as non secure for kernel stage (as required
+	 * by kernel)
+	 */
+	setbits_le32(SOCFPGA_SCU_SNSAC, 0xfff);

We do this in socfpga/misc.c in arch_early_init_r() already. You should consolidate this code so we don't have it twice I think.

Quoted 1 line

Yes, this can be removed.

Hi! Please use whichever place you think is better.

Quoted 8 lines

Quoted 7 lines

arch/arm/cpu/armv7/socfpga/s_init.c line 48

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+
+#endif	/* CONFIG_SPL_BUILD */
+
+	/* Configure the L2 controller to make SDRAM start at 0	*/
+	writel(0x1, (SOCFPGA_MPUL2_ADDRESS + SOCFPGA_MPUL2_ADRFLTR_START));

This as well.

Quoted 2 lines

This call has to stay because we want to map the SDRAM to 0x0 in the SPL. The call in arch_early_init_r() is in u-boot, and thus is too late.

Roger, but please remove the other invocation once the SPL is in place. OK?

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/s_init.c

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+
+#endif	/* CONFIG_SPL_BUILD */
+
+	/* Configure the L2 controller to make SDRAM start at 0	*/
+	writel(0x1, (SOCFPGA_MPUL2_ADDRESS + 

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

This as well.

This call has to stay because we want to map the SDRAM to 0x0 in the SPL. The call in arch_early_init_r() is in u-boot, and thus is too late.

Roger, but please remove the other invocation once the SPL is in place. OK?

Yes, I can remove it in the series?

Quoted 32 lines · Marek Vasut

Quoted 5 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

s_init will map SDRAM to 0x0.

Signed-off-by Dinh Nguyen <[email protected]>

Hi!

[...]

Quoted 20 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +20 -0

+void s_init(void)
+{
+#ifdef CONFIG_SPL_BUILD
+    struct socfpga_system_manager *sysmgr_regs =
+            (struct socfpga_system_manager *)SOCFPGA_SYSMGR_ADDRESS;
+    unsigned long reg;
+    /*
+     * First C code to run. Clear fake OCRAM ECC first as SBE
+     * and DBE might triggered during power on
+     */
+    reg = readl(&sysmgr_regs->eccgrp_ocram);
+    if (reg & SYSMGR_ECC_OCRAM_SERR)
+            writel(SYSMGR_ECC_OCRAM_SERR | SYSMGR_ECC_OCRAM_EN,
+                    &sysmgr_regs->eccgrp_ocram);
+    if (reg & SYSMGR_ECC_OCRAM_DERR)
+            writel(SYSMGR_ECC_OCRAM_DERR  | SYSMGR_ECC_OCRAM_EN,
+                    &sysmgr_regs->eccgrp_ocram);
+#else
+    /*
+     * Private components security

Quoted 8 lines

arch/arm/cpu/armv7/socfpga/s_init.c line 43

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+     * U-Boot : configure private timer, global timer and cpu
+     * component access as non secure for kernel stage (as required
+     * by kernel)
+     */
+    setbits_le32(SOCFPGA_SCU_SNSAC, 0xfff);

We do this in socfpga/misc.c in arch_early_init_r() already. You should consolidate this code so we don't have it twice I think.

Yes, this can be removed.

Quoted 9 lines

arch/arm/cpu/armv7/socfpga/s_init.c line 48

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/s_init.c new file +5 -0

+
+#endif      /* CONFIG_SPL_BUILD */
+
+    /* Configure the L2 controller to make SDRAM start at 0 */
+    writel(0x1, (SOCFPGA_MPUL2_ADDRESS + SOCFPGA_MPUL2_ADRFLTR_START));

This as well.

This call has to stay because we want to map the SDRAM to 0x0 in the SPL. The call in arch_early_init_r() is in u-boot, and thus is too late.

From: Dinh Nguyen <[email protected]>

Add code to handle status of the bsel.

From: Dinh Nguyen <[email protected]>

Add a function to relocate the stack from OCRAM to SDRAM.

Quoted 3 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Add a function to relocate the stack from OCRAM to SDRAM.

Hi,

is this functionality really needed ? There's like 128 KiB of OCRAM on SoCFPGA,

which should be plenty, right? If moving the stack to SDRAM is really needed, then you might want to use the common stack relocation code (see relocate_code() function). Also, you should thoroughly describe the reason in the commit message, in this case it's really important.

Hi Marek,

Quoted 6 lines

On Wednesday, January 14, 2015 at 05:40:55 PM,
dinguyen at opensource.altera.com

Quoted 2 lines · wrote:

From: Dinh Nguyen <[email protected]>

Quoted 6 lines

Quoted 1 line

Add a function to relocate the stack from OCRAM to SDRAM.

Hi,

is this functionality really needed ? There's like 128 KiB of OCRAM on SoCFPGA,

On cyclone5 and arria5, there is only 64KiB.

Quoted 4 lines

which should be plenty, right? If moving the stack to SDRAM is really needed, then you might want to use the common stack relocation code (see relocate_code() function). Also, you should thoroughly describe the reason in the commit message, in this case it's really important.

I'll update in v2.

Quoted 1 line · Dinh Nguyen

Hi Marek,

Hi Dinh,

Quoted 7 lines · Marek Vasut

Quoted 6 lines

On Wednesday, January 14, 2015 at 05:40:55 PM,
dinguyen at opensource.altera.com

Quoted 2 lines · wrote:

From: Dinh Nguyen <[email protected]>

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

Add a function to relocate the stack from OCRAM to SDRAM.

Hi,

is this functionality really needed ? There's like 128 KiB of OCRAM on SoCFPGA,

On cyclone5 and arria5, there is only 64KiB.

Ooops, sorry, my bad. You're right. But that is still boatload of memory, given that the SPL will use some hundreds of bytes for the stack under normal conditions.

Quoted 6 lines

Quoted 4 lines

which should be plenty, right? If moving the stack to SDRAM is really needed, then you might want to use the common stack relocation code (see relocate_code() function). Also, you should thoroughly describe the reason in the commit message, in this case it's really important.

I'll update in v2.

So uh, what's the reason for moving the stack into the SDRAM anyway please ?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

which should be plenty, right? If moving the stack to SDRAM is really needed, then you might want to use the common stack relocation code (see relocate_code() function). Also, you should thoroughly describe the reason in the commit message, in this case it's really important.

I'll update in v2.

So uh, what's the reason for moving the stack into the SDRAM anyway please ?

I think this is needed to be able to support SPL_FAT_LOAD.

From: Dinh Nguyen <[email protected]>

Allocate a stack in the OCRAM for the SPL to use.

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/socfpga/u-boot-spl.lds | 7 +++++++
 include/configs/socfpga_common.h          | 1 +
 2 files changed, 8 insertions(+)

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

---
 include/configs/socfpga_common.h | 5 +++++
 1 file changed, 5 insertions(+)

(quoted diff) +0 -0

b/include/configs/socfpga_common.h index 90ca031..6bcb013 100644

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

---
 include/configs/socfpga_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/include/configs/socfpga_common.h index 4b98378..29d02d6 100644

From: Dinh Nguyen <[email protected]>

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/cpu/armv7/socfpga/u-boot-spl.lds | 7 +++++++
 include/configs/socfpga_common.h          | 1 +
 2 files changed, 8 insertions(+)

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds index 739f3f1..f87e951 100644