[U-Boot] [PATCHv3 01/17] arm: socfpga: spl: Add main sdram code
Wolfgang Denk
wd at denx.de
Tue Mar 31 08:41:46 CEST 2015
Dear dinguyen at opensource.altera.com,
In message <1427752878-18426-2-git-send-email-dinguyen at opensource.altera.com> you wrote:
>
...
> +/* Register: sdr.ctrlgrp.ctrlcfg */
> +#define SDR_CTRLGRP_CTRLCFG_ADDRESS 0x5000
> +/* Register: sdr.ctrlgrp.dramtiming1 */
> +#define SDR_CTRLGRP_DRAMTIMING1_ADDRESS 0x5004
> +/* Register: sdr.ctrlgrp.dramtiming2 */
> +#define SDR_CTRLGRP_DRAMTIMING2_ADDRESS 0x5008
> +/* Register: sdr.ctrlgrp.dramtiming3 */
> +#define SDR_CTRLGRP_DRAMTIMING3_ADDRESS 0x500c
> +/* Register: sdr.ctrlgrp.dramtiming4 */
> +#define SDR_CTRLGRP_DRAMTIMING4_ADDRESS 0x5010
> +/* Register: sdr.ctrlgrp.lowpwrtiming */
> +#define SDR_CTRLGRP_LOWPWRTIMING_ADDRESS 0x5014
> +/* Register: sdr.ctrlgrp.dramodt */
> +#define SDR_CTRLGRP_DRAMODT_ADDRESS 0x5018
> +/* Register: sdr.ctrlgrp.dramaddrw */
> +#define SDR_CTRLGRP_DRAMADDRW_ADDRESS 0x502c
...
First, this whole block of registers should probably made a C struct.
Also, the comments are pretty much redundant - they do not add any new
information that is not already included in the #define, so they could
be omitted to make the code easier to read.
> +#define SDR_CTRLGRP_PHYCTRL_ADDRESS 0x5150
> +/* Register: sdr.ctrlgrp.phyctrl.phyctrl_0 */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_ADDRESS 0x5150
> +/* Register: sdr.ctrlgrp.phyctrl.phyctrl_1 */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_1_ADDRESS 0x5154
> +/* Register: sdr.ctrlgrp.phyctrl.phyctrl_2 */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_2_ADDRESS 0x5158
> +/* Register instance: sdr::ctrlgrp::phyctrl.phyctrl_0 */
> +/* Register template referenced: sdr::ctrlgrp::phyctrl::phyctrl_0 */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_OFFSET 0x150
> +/* Register instance: sdr::ctrlgrp::phyctrl.phyctrl_1 */
> +/* Register template referenced: sdr::ctrlgrp::phyctrl::phyctrl_1 */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_1_OFFSET 0x154
> +/* Register instance: sdr::ctrlgrp::phyctrl.phyctrl_2 */
> +/* Register template referenced: sdr::ctrlgrp::phyctrl::phyctrl_2 */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_2_OFFSET 0x158
Is it not redundant to define both the address and the offset? Decide
for one. And again, this should probably be rather a C struct.
> +/* Register template: sdr::ctrlgrp::ctrlcfg */
> +#define SDR_CTRLGRP_CTRLCFG_OUTPUTREG_LSB 26
> +#define SDR_CTRLGRP_CTRLCFG_OUTPUTREG_MASK 0x04000000
> +#define SDR_CTRLGRP_CTRLCFG_BURSTTERMEN_LSB 25
> +#define SDR_CTRLGRP_CTRLCFG_BURSTTERMEN_MASK 0x02000000
> +#define SDR_CTRLGRP_CTRLCFG_BURSTINTREN_LSB 24
> +#define SDR_CTRLGRP_CTRLCFG_BURSTINTREN_MASK 0x01000000
> +#define SDR_CTRLGRP_CTRLCFG_NODMPINS_LSB 23
> +#define SDR_CTRLGRP_CTRLCFG_NODMPINS_MASK 0x00800000
> +#define SDR_CTRLGRP_CTRLCFG_DQSTRKEN_LSB 22
> +#define SDR_CTRLGRP_CTRLCFG_DQSTRKEN_MASK 0x00400000
> +#define SDR_CTRLGRP_CTRLCFG_STARVELIMIT_LSB 16
> +#define SDR_CTRLGRP_CTRLCFG_STARVELIMIT_MASK 0x003f0000
> +#define SDR_CTRLGRP_CTRLCFG_REORDEREN_LSB 15
> +#define SDR_CTRLGRP_CTRLCFG_REORDEREN_MASK 0x00008000
> +#define SDR_CTRLGRP_CTRLCFG_GENDBE_LSB 14
> +#define SDR_CTRLGRP_CTRLCFG_GENDBE_MASK 0x00004000
> +#define SDR_CTRLGRP_CTRLCFG_GENSBE_LSB 13
> +#define SDR_CTRLGRP_CTRLCFG_GENSBE_MASK 0x00002000
> +#define SDR_CTRLGRP_CTRLCFG_CFG_ENABLE_ECC_CODE_OVERWRITES_LSB 12
> +#define SDR_CTRLGRP_CTRLCFG_CFG_ENABLE_ECC_CODE_OVERWRITES_MASK 0x00001000
> +#define SDR_CTRLGRP_CTRLCFG_ECCCORREN_LSB 11
> +#define SDR_CTRLGRP_CTRLCFG_ECCCORREN_MASK 0x00000800
> +#define SDR_CTRLGRP_CTRLCFG_ECCEN_LSB 10
> +#define SDR_CTRLGRP_CTRLCFG_ECCEN_MASK 0x00000400
> +#define SDR_CTRLGRP_CTRLCFG_ADDRORDER_LSB 8
> +#define SDR_CTRLGRP_CTRLCFG_ADDRORDER_MASK 0x00000300
> +#define SDR_CTRLGRP_CTRLCFG_MEMBL_LSB 3
> +#define SDR_CTRLGRP_CTRLCFG_MEMBL_MASK 0x000000f8
> +#define SDR_CTRLGRP_CTRLCFG_MEMTYPE_LSB 0
> +#define SDR_CTRLGRP_CTRLCFG_MEMTYPE_MASK 0x00000007
> +/* Register template: sdr::ctrlgrp::dramtiming1 */
> +#define SDR_CTRLGRP_DRAMTIMING1_TRFC_LSB 24
> +#define SDR_CTRLGRP_DRAMTIMING1_TRFC_MASK 0xff000000
> +#define SDR_CTRLGRP_DRAMTIMING1_TFAW_LSB 18
> +#define SDR_CTRLGRP_DRAMTIMING1_TFAW_MASK 0x00fc0000
> +#define SDR_CTRLGRP_DRAMTIMING1_TRRD_LSB 14
> +#define SDR_CTRLGRP_DRAMTIMING1_TRRD_MASK 0x0003c000
> +#define SDR_CTRLGRP_DRAMTIMING1_TCL_LSB 9
> +#define SDR_CTRLGRP_DRAMTIMING1_TCL_MASK 0x00003e00
> +#define SDR_CTRLGRP_DRAMTIMING1_TAL_LSB 4
> +#define SDR_CTRLGRP_DRAMTIMING1_TAL_MASK 0x000001f0
> +#define SDR_CTRLGRP_DRAMTIMING1_TCWL_LSB 0
> +#define SDR_CTRLGRP_DRAMTIMING1_TCWL_MASK 0x0000000f
> +/* Register template: sdr::ctrlgrp::dramtiming2 */
> +#define SDR_CTRLGRP_DRAMTIMING2_TWTR_LSB 25
> +#define SDR_CTRLGRP_DRAMTIMING2_TWTR_MASK 0x1e000000
> +#define SDR_CTRLGRP_DRAMTIMING2_TWR_LSB 21
> +#define SDR_CTRLGRP_DRAMTIMING2_TWR_MASK 0x01e00000
> +#define SDR_CTRLGRP_DRAMTIMING2_TRP_LSB 17
> +#define SDR_CTRLGRP_DRAMTIMING2_TRP_MASK 0x001e0000
> +#define SDR_CTRLGRP_DRAMTIMING2_TRCD_LSB 13
> +#define SDR_CTRLGRP_DRAMTIMING2_TRCD_MASK 0x0001e000
> +#define SDR_CTRLGRP_DRAMTIMING2_TREFI_LSB 0
> +#define SDR_CTRLGRP_DRAMTIMING2_TREFI_MASK 0x00001fff
> +/* Register template: sdr::ctrlgrp::dramtiming3 */
> +#define SDR_CTRLGRP_DRAMTIMING3_TCCD_LSB 19
> +#define SDR_CTRLGRP_DRAMTIMING3_TCCD_MASK 0x00780000
> +#define SDR_CTRLGRP_DRAMTIMING3_TMRD_LSB 15
> +#define SDR_CTRLGRP_DRAMTIMING3_TMRD_MASK 0x00078000
> +#define SDR_CTRLGRP_DRAMTIMING3_TRC_LSB 9
> +#define SDR_CTRLGRP_DRAMTIMING3_TRC_MASK 0x00007e00
> +#define SDR_CTRLGRP_DRAMTIMING3_TRAS_LSB 4
> +#define SDR_CTRLGRP_DRAMTIMING3_TRAS_MASK 0x000001f0
> +#define SDR_CTRLGRP_DRAMTIMING3_TRTP_LSB 0
> +#define SDR_CTRLGRP_DRAMTIMING3_TRTP_MASK 0x0000000f
> +/* Register template: sdr::ctrlgrp::dramtiming4 */
> +#define SDR_CTRLGRP_DRAMTIMING4_MINPWRSAVECYCLES_LSB 20
> +#define SDR_CTRLGRP_DRAMTIMING4_MINPWRSAVECYCLES_MASK 0x00f00000
> +#define SDR_CTRLGRP_DRAMTIMING4_PWRDOWNEXIT_LSB 10
> +#define SDR_CTRLGRP_DRAMTIMING4_PWRDOWNEXIT_MASK 0x000ffc00
> +#define SDR_CTRLGRP_DRAMTIMING4_SELFRFSHEXIT_LSB 0
> +#define SDR_CTRLGRP_DRAMTIMING4_SELFRFSHEXIT_MASK 0x000003ff
> +/* Register template: sdr::ctrlgrp::lowpwrtiming */
> +#define SDR_CTRLGRP_LOWPWRTIMING_CLKDISABLECYCLES_LSB 16
> +#define SDR_CTRLGRP_LOWPWRTIMING_CLKDISABLECYCLES_MASK 0x000f0000
> +#define SDR_CTRLGRP_LOWPWRTIMING_AUTOPDCYCLES_LSB 0
> +#define SDR_CTRLGRP_LOWPWRTIMING_AUTOPDCYCLES_MASK 0x0000ffff
> +/* Register template: sdr::ctrlgrp::dramaddrw */
> +#define SDR_CTRLGRP_DRAMADDRW_CSBITS_LSB 13
> +#define SDR_CTRLGRP_DRAMADDRW_CSBITS_MASK 0x0000e000
> +#define SDR_CTRLGRP_DRAMADDRW_BANKBITS_LSB 10
> +#define SDR_CTRLGRP_DRAMADDRW_BANKBITS_MASK 0x00001c00
> +#define SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB 5
> +#define SDR_CTRLGRP_DRAMADDRW_ROWBITS_MASK 0x000003e0
> +#define SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB 0
> +#define SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK 0x0000001f
...
Are all these #defines actually used in the code? Maybe we can remove
some that will never play a role here?
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMTYPE (2)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMBL (8)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ADDRORDER (0)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCEN (1)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCCORREN (1)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_REORDEREN (1)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_STARVELIMIT (10)
...
Please remove parens from all such simple definitions. Please fix
globally.
> +#define ADDRORDER2_INFO \
> + "INFO: Changing address order to 2 (row, chip, bank, column)\n"
> +#define ADDRORDER0_INFO \
> + "INFO: Changing address order to 0 (chip, row, bank, column)\n"
Please insert the strings where they are used, this makes the code
much easier to read.
> +typedef struct _sdram_prot_rule {
> + uint32_t rule; /* SDRAM protection rule number: 0-19 */
> + uint64_t sdram_start; /* SDRAM start address */
> + uint64_t sdram_end; /* SDRAM end address */
> + int valid; /* Rule valid or not? 1 - valid, 0 not*/
> +
> + uint32_t security;
> + uint32_t portmask;
> + uint32_t result;
> + uint32_t lo_prot_id;
> + uint32_t hi_prot_id;
> +} sdram_prot_rule, *psdram_prot_rule;
Would it make sense to move the "uint32_t rule" after the uint64_t
entries to avoid a gap in the struct?
> + /* Select the rule */
> + regoffs = SDR_CTRLGRP_PROTRULERDWR_ADDRESS;
> + writel(ruleno, (SOCFPGA_SDR_ADDRESS + regoffs));
NAK. We do not allow such syntax. Please use a C struct.
Please fix globally.
> +/* Function to update the field within variable */
> +static unsigned sdram_write_register_field(unsigned masked_value,
> + unsigned data, unsigned shift, unsigned mask)
> +{
> + masked_value &= ~(mask);
> + masked_value |= (data << shift) & mask;
> + return masked_value;
> +}
Is this really needed? I guess you could use standard I/O accessors
like clrsetbits-*() instead?
> +#if defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS) && \
> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) && \
> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS) && \
> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS) && \
> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS)
> +
> + int cs = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS;
> + int width = 8;
> + int rows = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS;
> + int banks = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS;
> + int cols = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS;
> + unsigned long long workaround_memsize = MEMSIZE_4G;
> +
> + writel(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS,
> + &sysmgr_regs->iswgrp_handoff[4]);
> +#endif
> +
> + /***** CTRLCFG *****/
> +#if defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMTYPE) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMBL) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ADDRORDER) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCEN) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCCORREN) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_REORDEREN) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_STARVELIMIT) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_DQSTRKEN) || \
> +defined(CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_NODMPINS)
> + debug("\nConfiguring CTRLCFG\n");
> + register_offset = SDR_CTRLGRP_CTRLCFG_ADDRESS;
> + /* Read original register value */
> + reg_value = readl(SOCFPGA_SDR_ADDRESS + register_offset);
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMTYPE
This is an unreadable #ifdef mess. Can we somehow clean up this code,
please?
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCEN
> + reg_value = sdram_write_register_field(reg_value,
> + CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCEN,
> + SDR_CTRLGRP_CTRLCFG_ECCEN_LSB,
> + SDR_CTRLGRP_CTRLCFG_ECCEN_MASK);
> +#endif
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCCORREN
> + reg_value = sdram_write_register_field(reg_value,
> + CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ECCCORREN,
> + SDR_CTRLGRP_CTRLCFG_ECCCORREN_LSB,
> + SDR_CTRLGRP_CTRLCFG_ECCCORREN_MASK);
> +#endif
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_REORDEREN
> + reg_value = sdram_write_register_field(reg_value,
> + CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_REORDEREN,
> + SDR_CTRLGRP_CTRLCFG_REORDEREN_LSB,
> + SDR_CTRLGRP_CTRLCFG_REORDEREN_MASK);
> +#endif
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_STARVELIMIT
> + reg_value = sdram_write_register_field(reg_value,
> + CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_STARVELIMIT,
> + SDR_CTRLGRP_CTRLCFG_STARVELIMIT_LSB,
> + SDR_CTRLGRP_CTRLCFG_STARVELIMIT_MASK);
> +#endif
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_DQSTRKEN
> + reg_value = sdram_write_register_field(reg_value,
> + CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_DQSTRKEN,
> + SDR_CTRLGRP_CTRLCFG_DQSTRKEN_LSB,
> + SDR_CTRLGRP_CTRLCFG_DQSTRKEN_MASK);
> +#endif
> +#ifdef CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_NODMPINS
> + reg_value = sdram_write_register_field(reg_value,
> + CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_NODMPINS,
> + SDR_CTRLGRP_CTRLCFG_NODMPINS_LSB,
> + SDR_CTRLGRP_CTRLCFG_NODMPINS_MASK);
> +#endif
You would get much simpler code if you did not define shift and mask,
but the direct hex value instead - this just became one big |.
> +/*
> + * space around comma is required for varargs macro to remove comma if args
> + * is empty
> + */
> +#define BFM_GBL_SET(field, value)
where do we have a varargs macro here?
> +#define DYNAMIC_CALIB_STEPS (dyn_calib_steps)
The following code uses a mix of DYNAMIC_CALIB_STEPS and
dyn_calib_steps. Please drop DYNAMIC_CALIB_STEPS and use
dyn_calib_steps consistently.
> +static int sdr_ops(u32 *base, u32 offset, u32 data, bool write)
> +{
Is this needed? It appears we could use standard I/O accessors
instead?
> +static inline void scc_mgr_set_dqs_bypass(uint32_t write_group, uint32_t bypass)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_DQS_BYPASS(write_group, bypass);
> +}
> +
> +inline void scc_mgr_set_dq_out1_delay(uint32_t write_group,
> + uint32_t dq_in_group, uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + sdr_ops((u32 *)SCC_MGR_IO_OUT1_DELAY, dq_in_group << 2, delay, SDR_WRITE);
> +}
> +
> +inline void scc_mgr_set_dq_out2_delay(uint32_t write_group,
> + uint32_t dq_in_group, uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_DQ_OUT2_DELAY(dq_in_group, delay);
> +}
> +
> +inline void scc_mgr_set_dq_in_delay(uint32_t write_group,
> + uint32_t dq_in_group, uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + sdr_ops((u32 *)SCC_MGR_IO_IN_DELAY, dq_in_group << 2, delay, SDR_WRITE);
> +}
> +
> +static inline void scc_mgr_set_dq_bypass(uint32_t write_group,
> + uint32_t dq_in_group, uint32_t bypass)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_DQ_BYPASS(dq_in_group, bypass);
> +}
> +
> +static inline void scc_mgr_set_rfifo_mode(uint32_t write_group,
> + uint32_t dq_in_group, uint32_t mode)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_RFIFO_MODE(dq_in_group, mode);
> +}
> +
> +static inline void scc_mgr_set_hhp_extras(void)
> +{
> + /*
> + * Load the fixed setting in the SCC manager
> + * bits: 0:0 = 1'b1 - dqs bypass
> + * bits: 1:1 = 1'b1 - dq bypass
> + * bits: 4:2 = 3'b001 - rfifo_mode
> + * bits: 6:5 = 2'b01 - rfifo clock_select
> + * bits: 7:7 = 1'b0 - separate gating from ungating setting
> + * bits: 8:8 = 1'b0 - separate OE from Output delay setting
> + */
> + uint32_t value = (0<<8) | (0<<7) | (1<<5) | (1<<2) | (1<<1) | (1<<0);
> + sdr_ops((u32 *)SCC_MGR_HHP_GLOBALS, SCC_MGR_HHP_EXTRAS_OFFSET,
> + value, SDR_WRITE);
> +}
> +
> +static inline void scc_mgr_set_hhp_dqse_map(void)
> +{
> + /* Load the fixed setting in the SCC manager */
> + sdr_ops((u32 *)SCC_MGR_HHP_GLOBALS, SCC_MGR_HHP_DQSE_MAP_OFFSET, 0,
> + SDR_WRITE);
> +}
> +
> +static inline void scc_mgr_set_dqs_out1_delay(uint32_t write_group,
> + uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + sdr_ops((u32 *)SCC_MGR_IO_OUT1_DELAY, RW_MGR_MEM_DQ_PER_WRITE_DQS << 2,
> + delay, SDR_WRITE);
> +}
> +
> +static inline void scc_mgr_set_dqs_out2_delay(uint32_t write_group,
> + uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_DQS_IO_OUT2_DELAY(delay);
> +}
> +
> +static inline void scc_mgr_set_dm_out1_delay(uint32_t write_group,
> + uint32_t dm, uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + sdr_ops((u32 *)SCC_MGR_IO_OUT1_DELAY,
> + (RW_MGR_MEM_DQ_PER_WRITE_DQS + 1 + dm) << 2, delay, SDR_WRITE);
> +}
> +
> +static inline void scc_mgr_set_dm_out2_delay(uint32_t write_group, uint32_t dm,
> + uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_DM_IO_OUT2_DELAY(dm, delay);
> +}
> +
> +static inline void scc_mgr_set_dm_in_delay(uint32_t write_group,
> + uint32_t dm, uint32_t delay)
> +{
> + /* Load the setting in the SCC manager */
> + sdr_ops((u32 *)SCC_MGR_IO_IN_DELAY,
> + (RW_MGR_MEM_DQ_PER_WRITE_DQS + 1 + dm) << 2, delay, SDR_WRITE);
> +}
> +
> +static inline void scc_mgr_set_dm_bypass(uint32_t write_group, uint32_t dm,
> + uint32_t bypass)
> +{
> + /* Load the setting in the SCC manager */
> + WRITE_SCC_DM_BYPASS(dm, bypass);
> +}
All these one-line functions should probably rather be inlined?
...
drivers/ddr/altera/sdram.c is 1,300+ lines; and
drivers/ddr/altera/sequencer.c is nearly 4,000 lines.
Have you considered splitting this off into smaller units?
Eventually this might help to reduce the #ifdef mess, too ?
> +#define SCC_MGR_DQS_ENA (BASE_SCC_MGR + 0x0E00)
> +#define SCC_MGR_DQS_IO_ENA (BASE_SCC_MGR + 0x0E04)
> +#define SCC_MGR_DQ_ENA (BASE_SCC_MGR + 0x0E08)
> +#define SCC_MGR_DM_ENA (BASE_SCC_MGR + 0x0E0C)
> +#define SCC_MGR_UPD (BASE_SCC_MGR + 0x0E20)
> +#define SCC_MGR_ACTIVE_RANK (BASE_SCC_MGR + 0x0E40)
Guess this should be a C struct?
> diff --git a/drivers/ddr/altera/sequencer_auto.h b/drivers/ddr/altera/sequencer_auto.h
> new file mode 100644
> index 0000000..de26b6b
> --- /dev/null
> +++ b/drivers/ddr/altera/sequencer_auto.h
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright Altera Corporation (C) 2012-2014. All rights reserved
Can we please get rid of the non-sense "All rights reserved" clause
here (and in the other fiels)? Thanks!
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +#define __RW_MGR_ac_mrs1 0x04
> +#define __RW_MGR_ac_mrs3 0x06
> +#define __RW_MGR_ac_write_bank_0_col_0_nodata_wl_1 0x1C
> +#define __RW_MGR_ac_act_1 0x11
Macro names must be all caps. Please fix globally.
Also, please be aware of the special meaning of a __ prefix for C
macros. I think your usage here might be incorrect.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If anything can go wrong, it will." - Edsel Murphy
More information about the U-Boot
mailing list