On Thu, Apr 19, 2018 at 11:02 AM, Marek Vasut <ma...@denx.de> wrote: > On 04/19/2018 11:50 AM, Ley Foon Tan wrote: >> Add DDR support for Stratix SoC > > OT: How very different is the DDR controller on Stratix 10 and Arria 10? The IP blocks are different, so can't combine both of them. > >> Signed-off-by: Chin Liang See <chin.liang....@intel.com> >> Signed-off-by: Ley Foon Tan <ley.foon....@intel.com> >> --- >> arch/arm/mach-socfpga/include/mach/sdram_s10.h | 340 ++++++++++++++++++++ > > If this is used only be the driver, no point having it in mach/ > >> drivers/ddr/altera/Makefile | 1 + >> drivers/ddr/altera/sdram_s10.c | 392 >> ++++++++++++++++++++++++ >> 3 files changed, 733 insertions(+), 0 deletions(-) >> create mode 100644 arch/arm/mach-socfpga/include/mach/sdram_s10.h >> create mode 100644 drivers/ddr/altera/sdram_s10.c > [...] > >> + >> +union caltiming9_reg { >> + struct { >> + u32 cfg_4_act_to_act:8; >> + u32 reserved:24; >> + }; >> + u32 word; >> +}; > > I wonder if this struct stuff is really a good idea. This easier for driver to access each field in register. > >> +#define DDR_SCHED_DDRTIMING_ACTTOACT_OFFSET 0 >> +#define DDR_SCHED_DDRTIMING_RDTOMISS_OFFSET 6 >> +#define DDR_SCHED_DDRTIMING_WRTOMISS_OFFSET 12 >> +#define DDR_SCHED_DDRTIMING_BURSTLEN_OFFSET 18 >> +#define DDR_SCHED_DDRTIMING_RDTOWR_OFFSET 21 >> +#define DDR_SCHED_DDRTIMING_WRTORD_OFFSET 26 >> +#define DDR_SCHED_DDRTIMING_BWRATIO_OFFSET 31 >> +#define DDR_SCHED_DDRMOD_BWRATIOEXTENDED_OFFSET 1 >> +#define DDR_SCHED_ACTIVATE_RRD_OFFSET 0 >> +#define DDR_SCHED_ACTIVATE_FAW_OFFSET 4 >> +#define DDR_SCHED_ACTIVATE_FAWBANK_OFFSET 10 >> +#define DDR_SCHED_DEVTODEV_BUSRDTORD_OFFSET 0 >> +#define DDR_SCHED_DEVTODEV_BUSRDTOWR_OFFSET 2 >> +#define DDR_SCHED_DEVTODEV_BUSWRTORD_OFFSET 4 >> +#define DDR_HMC_DDRIOCTRL_IOSIZE_MSK 0x00000003 >> +#define DDR_HMC_DDRCALSTAT_CAL_MSK 0x00000001 >> +#define DDR_HMC_ECCCTL_AWB_CNT_RST_SET_MSK 0x00010000 >> +#define DDR_HMC_ECCCTL_CNT_RST_SET_MSK 0x00000100 >> +#define DDR_HMC_ECCCTL_ECC_EN_SET_MSK 0x00000001 >> +#define DDR_HMC_ECCCTL2_RMW_EN_SET_MSK 0x00000100 >> +#define DDR_HMC_ECCCTL2_AWB_EN_SET_MSK 0x00000001 >> +#define DDR_HMC_ECC_DIAGON_ECCDIAGON_EN_SET_MSK 0x00010000 >> +#define DDR_HMC_ECC_DIAGON_WRDIAGON_EN_SET_MSK 0x00000001 >> +#define DDR_HMC_ERRINTEN_SERRINTEN_EN_SET_MSK 0x00000001 >> +#define DDR_HMC_ERRINTEN_DERRINTEN_EN_SET_MSK 0x00000002 >> +#define DDR_HMC_INTSTAT_SERRPENA_SET_MSK 0x00000001 >> +#define DDR_HMC_INTSTAT_DERRPENA_SET_MSK 0x00000002 >> +#define DDR_HMC_INTSTAT_ADDRMTCFLG_SET_MSK 0x00010000 >> +#define DDR_HMC_INTMODE_INTMODE_SET_MSK 0x00000001 >> +#define DDR_HMC_RSTHANDSHAKE_MASK 0x000000ff >> +#define DDR_HMC_CORE2SEQ_INT_REQ 0xF >> +#define DDR_HMC_SEQ2CORE_INT_RESP_MASK 0x8 >> +#define DDR_HMC_HPSINTFCSEL_ENABLE_MASK 0x001f1f1f >> + >> +#define CCU_CPU0_MPRT_ADBASE_DDRREG_ADDR 0xf7004400 >> +#define CCU_CPU0_MPRT_ADBASE_MEMSPACE0_ADDR 0xf70045c0 >> +#define CCU_CPU0_MPRT_ADBASE_MEMSPACE1A_ADDR 0xf70045e0 >> +#define CCU_CPU0_MPRT_ADBASE_MEMSPACE1B_ADDR 0xf7004600 >> +#define CCU_CPU0_MPRT_ADBASE_MEMSPACE1C_ADDR 0xf7004620 >> +#define CCU_CPU0_MPRT_ADBASE_MEMSPACE1D_ADDR 0xf7004640 >> +#define CCU_CPU0_MPRT_ADBASE_MEMSPACE1E_ADDR 0xf7004660 > > Can all this come from DT, maybe except for the offsets ? Need to add a new node in DT if want get from DT. Or another way is add CCU base address to base_addr_s10.h and remove all these defines and use offset instead. Is it okay?
> >> +#define CCU_IOM_MPRT_ADBASE_MEMSPACE0_ADDR 0xf7018560 >> +#define CCU_IOM_MPRT_ADBASE_MEMSPACE1A_ADDR 0xf7018580 >> +#define CCU_IOM_MPRT_ADBASE_MEMSPACE1B_ADDR 0xf70185a0 >> +#define CCU_IOM_MPRT_ADBASE_MEMSPACE1C_ADDR 0xf70185c0 >> +#define CCU_IOM_MPRT_ADBASE_MEMSPACE1D_ADDR 0xf70185e0 >> +#define CCU_IOM_MPRT_ADBASE_MEMSPACE1E_ADDR 0xf7018600 >> + >> +#define CCU_ADBASE_DI_MASK 0x00000010 > > [...] > >> +#define DDR_CONFIG(A, B, C, R) ((A << 24) | (B << 16) | (C << 8) | R) > > You need parenthesis around A, B, C, R in the macro. Okay. > >> +/* The followring are the supported configurations */ >> +u32 ddr_config[] = { >> + /* DDR_CONFIG(Address order,Bank,Column,Row) */ >> + /* List for DDR3 or LPDDR3 (pinout order > chip, row, bank, column) */ >> + DDR_CONFIG(0, 3, 10, 12), >> + DDR_CONFIG(0, 3, 9, 13), >> + DDR_CONFIG(0, 3, 10, 13), >> + DDR_CONFIG(0, 3, 9, 14), >> + DDR_CONFIG(0, 3, 10, 14), >> + DDR_CONFIG(0, 3, 10, 15), >> + DDR_CONFIG(0, 3, 11, 14), >> + DDR_CONFIG(0, 3, 11, 15), >> + DDR_CONFIG(0, 3, 10, 16), >> + DDR_CONFIG(0, 3, 11, 16), >> + DDR_CONFIG(0, 3, 12, 15), /* 0xa */ >> + /* List for DDR4 only (pinout order > chip, bank, row, column) */ >> + DDR_CONFIG(1, 3, 10, 14), >> + DDR_CONFIG(1, 4, 10, 14), >> + DDR_CONFIG(1, 3, 10, 15), >> + DDR_CONFIG(1, 4, 10, 15), >> + DDR_CONFIG(1, 3, 10, 16), >> + DDR_CONFIG(1, 4, 10, 16), >> + DDR_CONFIG(1, 3, 10, 17), >> + DDR_CONFIG(1, 4, 10, 17), >> +}; >> + >> +#define DDR_CONFIG_ELEMENTS (sizeof(ddr_config) / sizeof(u32)) > > Is that ARRAY_SIZE() ? Yes, will change this. > >> +int match_ddr_conf(u32 ddr_conf) >> +{ >> + int i; >> + >> + for (i = 0; i < DDR_CONFIG_ELEMENTS; i++) { >> + if (ddr_conf == ddr_config[i]) >> + return i; >> + } >> + return 0; >> +} >> + >> +static int emif_clear(void) >> +{ >> + u32 s2c, i; >> + >> + writel(0, &socfpga_ecc_hmc_base->rsthandshakectrl); >> + s2c = readl(&socfpga_ecc_hmc_base->rsthandshakestat) & >> + DDR_HMC_RSTHANDSHAKE_MASK; >> + >> + for (i = 1000; (i > 0) && s2c; i--) { >> + WATCHDOG_RESET(); >> + mdelay(1); >> + s2c = readl(&socfpga_ecc_hmc_base->rsthandshakestat) & >> + DDR_HMC_RSTHANDSHAKE_MASK; > > wait_for_bit() ? Okay. > >> + } >> + return !s2c; >> +} >> + >> +static int emif_reset(void) >> +{ >> + u32 c2s, s2c, i; >> + >> + c2s = readl(&socfpga_ecc_hmc_base->rsthandshakectrl) & >> + DDR_HMC_RSTHANDSHAKE_MASK; >> + s2c = readl(&socfpga_ecc_hmc_base->rsthandshakestat) & >> + DDR_HMC_RSTHANDSHAKE_MASK; >> + >> + debug("DDR: c2s=%08x s2c=%08x nr0=%08x nr1=%08x nr2=%08x dst=%08x\n", >> + c2s, s2c, readl(&socfpga_io48_mmr_base->niosreserve0), >> + readl(&socfpga_io48_mmr_base->niosreserve1), >> + readl(&socfpga_io48_mmr_base->niosreserve2), >> + readl(&socfpga_io48_mmr_base->dramsts)); >> + >> + if (s2c && emif_clear()) { >> + printf("DDR: emif_clear() failed\n"); >> + return -1; >> + } >> + >> + puts("DDR: Triggerring emif reset\n"); >> + writel(DDR_HMC_CORE2SEQ_INT_REQ, >> + &socfpga_ecc_hmc_base->rsthandshakectrl); >> + >> + for (i = 1000; i > 0; i--) { >> + /* if seq2core[3] = 0, we are good */ >> + if (!(readl(&socfpga_ecc_hmc_base->rsthandshakestat) & >> + DDR_HMC_SEQ2CORE_INT_RESP_MASK)) >> + break; >> + WATCHDOG_RESET(); >> + mdelay(1); > > Another wait_for_bit() ? Okay. > >> + } >> + >> + if (!i) { >> + printf("DDR: failed to get ack from EMIF\n"); >> + return -2; >> + } >> + >> + if (emif_clear()) { >> + printf("DDR: emif_clear() failed\n"); >> + return -3; >> + } >> + >> + printf("DDR: %s triggered successly\n", __func__); > > debug() ? Okay. > >> + return 0; >> +} > Comb through this and fix the things mentioned above globally. > > -- > Best regards, > Marek Vasut Regards Ley Foon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot