On 01/14/2015 05:34 PM, Marek Vasut wrote:
> 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 ?
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
