Hi Kever, On 13 February 2017 at 02:38, Kever Yang <kever.y...@rock-chips.com> wrote: > RK3399 support DDR3, LPDDR3, DDR4 sdram, this patch is porting from > coreboot, support 4GB lpddr3 in this version. > > Signed-off-by: Kever Yang <kever.y...@rock-chips.com> > --- > > Changes in v2: > - use lower-case hex for input dts data > - using rk3288 like style to encode/decode sys_reg > - gather some parameters as base params in rk3399_sdram_params > - add some missing comment > > Changes in v1: > - use dts for parameter > - get all controller base address from dts instead of hard code > - gather all controller into dram_info instead of separate global > variables. > - add return value for error case > > arch/arm/dts/rk3399-sdram-lpddr3-4GB-1600.dtsi | 1536 > +++++++++++++++++++++ > arch/arm/include/asm/arch-rockchip/sdram_rk3399.h | 124 ++ > arch/arm/mach-rockchip/rk3399/Makefile | 1 + > arch/arm/mach-rockchip/rk3399/sdram_rk3399.c | 1259 +++++++++++++++++ > 4 files changed, 2920 insertions(+) > create mode 100644 arch/arm/dts/rk3399-sdram-lpddr3-4GB-1600.dtsi > create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h > create mode 100644 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c
Looks good, one more thing to do, plus some nits below. > > diff --git a/arch/arm/dts/rk3399-sdram-lpddr3-4GB-1600.dtsi > b/arch/arm/dts/rk3399-sdram-lpddr3-4GB-1600.dtsi > new file mode 100644 > index 0000000..65dfc38 > --- /dev/null > +++ b/arch/arm/dts/rk3399-sdram-lpddr3-4GB-1600.dtsi > @@ -0,0 +1,1536 @@ > +/* > + * (C) Copyright 2016 Rockchip Electronics Co., Ltd > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +&dmc { > + rockchip,sdram-params = < > + 0x2 > + 0xa > + 0x3 [...] > + >; > +} Please can you add a binding file with the meaning of this, like doc/device-tree-bindings/clock/rockchip,rk3288-dmc.txt:. ; > diff --git a/arch/arm/include/asm/arch-rockchip/sdram_rk3399.h > b/arch/arm/include/asm/arch-rockchip/sdram_rk3399.h > new file mode 100644 > index 0000000..0b7cb1f > --- /dev/null > +++ b/arch/arm/include/asm/arch-rockchip/sdram_rk3399.h > @@ -0,0 +1,124 @@ > +/* > + * Copyright (C) 2015 Rockchip Electronics Co., Ltd Is 2015 correct? > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef _ASM_ARCH_SDRAM_RK3399_H > +#define _ASM_ARCH_SDRAM_RK3399_H > + > +enum { > + DDR3 = 0x3, > + LPDDR2 = 0x5, > + LPDDR3 = 0x6, > + LPDDR4 = 0x7, > + UNUSED = 0xFF > +}; > + > +struct rk3399_ddr_pctl_regs { > + u32 denali_ctl[332]; > +}; > + > +struct rk3399_ddr_publ_regs { > + u32 denali_phy[959]; > +}; > + > +struct rk3399_ddr_pi_regs { > + u32 denali_pi[200]; > +}; > + > +struct rk3399_msch_regs { > + u32 coreid; > + u32 revisionid; > + u32 ddrconf; > + u32 ddrsize; > + u32 ddrtiminga0; > + u32 ddrtimingb0; > + u32 ddrtimingc0; > + u32 devtodev0; > + u32 reserved0[(0x110 - 0x20) / 4]; > + u32 ddrmode; > + u32 reserved1[(0x1000 - 0x114) / 4]; > + u32 agingx0; > +}; > + > +struct rk3399_msch_timings { > + u32 ddrtiminga0; > + u32 ddrtimingb0; > + u32 ddrtimingc0; > + u32 devtodev0; > + u32 ddrmode; > + u32 agingx0; > +}; > + > +struct rk3399_ddr_cic_regs { > + u32 cic_ctrl0; > + u32 cic_ctrl1; > + u32 cic_idle_th; > + u32 cic_cg_wait_th; > + u32 cic_status0; > + u32 cic_status1; > + u32 cic_ctrl2; > + u32 cic_ctrl3; > + u32 cic_ctrl4; > +}; > + > +/* DENALI_CTL_00 */ > +#define START 1 > + > +/* DENALI_CTL_68 */ > +#define PWRUP_SREFRESH_EXIT (1 << 16) > + > +/* DENALI_CTL_274 */ > +#define MEM_RST_VALID 1 > + > +struct rk3399_sdram_channel { > + unsigned int rank; > + /* dram column number, 0 means this channel is invalid */ > + unsigned int col; > + /* dram bank number, 3:8bank, 2:4bank */ > + unsigned int bk; > + /* channel buswidth, 2:32bit, 1:16bit, 0:8bit */ > + unsigned int bw; > + /* die buswidth, 2:32bit, 1:16bit, 0:8bit */ > + unsigned int dbw; > + /* > + * row_3_4 = 1: 6Gb or 12Gb die > + * row_3_4 = 0: normal die, power of 2 > + */ > + unsigned int row_3_4; > + unsigned int cs0_row; > + unsigned int cs1_row; > + unsigned int ddrconfig; > + struct rk3399_msch_timings noc_timings; > +}; > + > +struct rk3399_base_params { > + unsigned int ddr_freq; > + unsigned int dramtype; > + unsigned int num_channels; > + unsigned int stride; > + unsigned int odt; > +}; > + > +struct rk3399_sdram_params { > + struct rk3399_sdram_channel ch[2]; > + struct rk3399_base_params base; > + /* align 8 byte */ > + struct rk3399_ddr_pctl_regs pctl_regs; > + /* align 8 byte */ > + struct rk3399_ddr_pi_regs pi_regs; > + /* align 8 byte */ > + struct rk3399_ddr_publ_regs phy_regs; > + /* used for align 8byte for next struct */ > + unsigned int align_8; > +}; > + > +#define PI_CA_TRAINING (1 << 0) > +#define PI_WRITE_LEVELING (1 << 1) > +#define PI_READ_GATE_TRAINING (1 << 2) > +#define PI_READ_LEVELING (1 << 3) > +#define PI_WDQ_LEVELING (1 << 4) > +#define PI_FULL_TRAINING (0xff) No brackets on this last one. Also can you line these values up with tabs? > + > +#endif > diff --git a/arch/arm/mach-rockchip/rk3399/Makefile > b/arch/arm/mach-rockchip/rk3399/Makefile > index 98ebeac..437d851 100644 > --- a/arch/arm/mach-rockchip/rk3399/Makefile > +++ b/arch/arm/mach-rockchip/rk3399/Makefile > @@ -7,3 +7,4 @@ > obj-y += clk_rk3399.o > obj-y += rk3399.o > obj-y += syscon_rk3399.o > +obj-y += sdram_rk3399.o Should move up one line to preserve order. > diff --git a/arch/arm/mach-rockchip/rk3399/sdram_rk3399.c > b/arch/arm/mach-rockchip/rk3399/sdram_rk3399.c > new file mode 100644 > index 0000000..3989f67 > --- /dev/null > +++ b/arch/arm/mach-rockchip/rk3399/sdram_rk3399.c > @@ -0,0 +1,1259 @@ > +/* > + * (C) Copyright 2016 Rockchip Inc. > + * > + * SPDX-License-Identifier: GPL-2.0 > + * > + * Adapted from coreboot. > + */ > +#include <common.h> > +#include <clk.h> > +#include <dm.h> > +#include <dt-structs.h> > +#include <ram.h> > +#include <regmap.h> > +#include <syscon.h> > +#include <asm/io.h> > +#include <asm/arch/clock.h> > +#include <asm/arch/sdram_rk3399.h> > +#include <asm/arch/cru_rk3399.h> > +#include <asm/arch/grf_rk3399.h> > +#include <asm/arch/hardware.h> > +#include <linux/err.h> > + > +DECLARE_GLOBAL_DATA_PTR; > +struct chan_info { > + struct rk3399_ddr_pctl_regs *pctl; > + struct rk3399_ddr_pi_regs *pi; > + struct rk3399_ddr_publ_regs *publ; > + struct rk3399_msch_regs *msch; > +}; > + > +struct dram_info { > +#ifdef CONFIG_SPL_BUILD > + struct chan_info chan[2]; > + struct clk ddr_clk; > + struct rk3399_cru *cru; > + struct rk3399_pmucru *pmucru; > + struct rk3399_pmusgrf_regs *pmusgrf; > + struct rk3399_ddr_cic_regs *cic; > +#endif > + struct ram_info info; > + struct rk3399_pmugrf_regs *pmugrf; > +}; > + > +/* > + * sys_reg bitfield struct > + * [31] row_3_4_ch1 > + * [30] row_3_4_ch0 > + * [29:28] chinfo > + * [27] rank_ch1 > + * [26:25] col_ch1 > + * [24] bk_ch1 > + * [23:22] cs0_row_ch1 > + * [21:20] cs1_row_ch1 > + * [19:18] bw_ch1 > + * [17:16] dbw_ch1; > + * [15:13] ddrtype > + * [12] channelnum > + * [11] rank_ch0 > + * [10:9] col_ch0 > + * [8] bk_ch0 > + * [7:6] cs0_row_ch0 > + * [5:4] cs1_row_ch0 > + * [3:2] bw_ch0 > + * [1:0] dbw_ch0 > +*/ > +#define SYS_REG_DDRTYPE_SHIFT 13 > +#define SYS_REG_DDRTYPE_MASK 7 > +#define SYS_REG_NUM_CH_SHIFT 12 > +#define SYS_REG_NUM_CH_MASK 1 > +#define SYS_REG_ROW_3_4_SHIFT(ch) (30 + (ch)) > +#define SYS_REG_ROW_3_4_MASK 1 > +#define SYS_REG_CHINFO_SHIFT(ch) (28 + (ch)) > +#define SYS_REG_RANK_SHIFT(ch) (11 + (ch) * 16) > +#define SYS_REG_RANK_MASK 1 > +#define SYS_REG_COL_SHIFT(ch) (9 + (ch) * 16) > +#define SYS_REG_COL_MASK 3 > +#define SYS_REG_BK_SHIFT(ch) (8 + (ch) * 16) > +#define SYS_REG_BK_MASK 1 > +#define SYS_REG_CS0_ROW_SHIFT(ch) (6 + (ch) * 16) > +#define SYS_REG_CS0_ROW_MASK 3 > +#define SYS_REG_CS1_ROW_SHIFT(ch) (4 + (ch) * 16) > +#define SYS_REG_CS1_ROW_MASK 3 > +#define SYS_REG_BW_SHIFT(ch) (2 + (ch) * 16) > +#define SYS_REG_BW_MASK 3 > +#define SYS_REG_DBW_SHIFT(ch) ((ch) * 16) > +#define SYS_REG_DBW_MASK 3 > + > + > + Drop two extra blank lines > +#define PRESET_SGRF_HOLD(n) ((0x1 << (6+16)) | ((n) << 6)) > +#define PRESET_GPIO0_HOLD(n) ((0x1 << (7+16)) | ((n) << 7)) > +#define PRESET_GPIO1_HOLD(n) ((0x1 << (8+16)) | ((n) << 8)) Spaces around the + in those three. [..] > +static int phy_io_config(const struct chan_info *chan, > + const struct rk3399_sdram_params *sdram_params) > +{ > + u32 *denali_phy = chan->publ->denali_phy; > + u32 vref_mode_dq, vref_value_dq, vref_mode_ac, vref_value_ac; > + u32 mode_sel; > + u32 reg_value; > + u32 drv_value, odt_value; > + u32 speed; > + > + /* vref setting */ > + if (sdram_params->base.dramtype == LPDDR4) { > + /* LPDDR4 */ > + vref_mode_dq = 0x6; > + vref_value_dq = 0x1f; > + vref_mode_ac = 0x6; > + vref_value_ac = 0x1f; > + } else if (sdram_params->base.dramtype == LPDDR3) { > + if (sdram_params->base.odt == 1) { > + vref_mode_dq = 0x5; /* LPDDR3 ODT */ > + drv_value = (readl(&denali_phy[6]) >> 12) & 0xf; > + odt_value = (readl(&denali_phy[6]) >> 4) & 0xf; > + if (drv_value == PHY_DRV_ODT_48) { > + switch (odt_value) { > + case PHY_DRV_ODT_240: > + vref_value_dq = 0x16; > + break; > + case PHY_DRV_ODT_120: > + vref_value_dq = 0x26; > + break; > + case PHY_DRV_ODT_60: > + vref_value_dq = 0x36; > + break; > + default: > + debug("Halting: Invalid ODT > value.\n"); You probably shouldn't have 'Halting' here. This function is reporting the error, but it its caller that halts. Up to you though. > + return -EINVAL; > + } > + } else if (drv_value == PHY_DRV_ODT_40) { > + switch (odt_value) { > + case PHY_DRV_ODT_240: > + vref_value_dq = 0x19; > + break; > + case PHY_DRV_ODT_120: > + vref_value_dq = 0x23; > + break; > + case PHY_DRV_ODT_60: > + vref_value_dq = 0x31; > + break; > + default: > + debug("Halting: Invalid ODT > value.\n"); > + return -EINVAL; > + } > + } else if (drv_value == PHY_DRV_ODT_34_3) { > + switch (odt_value) { > + case PHY_DRV_ODT_240: > + vref_value_dq = 0x17; > + break; > + case PHY_DRV_ODT_120: > + vref_value_dq = 0x20; > + break; > + case PHY_DRV_ODT_60: > + vref_value_dq = 0x2e; > + break; > + default: > + debug("Halting: Invalid ODT > value.\n"); > + return -EINVAL; > + } > + } else { > + debug("Halting: Invalid DRV value.\n"); > + return -EINVAL; > + } > + } else { > + vref_mode_dq = 0x2; /* LPDDR3 */ > + vref_value_dq = 0x1f; > + } > + vref_mode_ac = 0x2; > + vref_value_ac = 0x1f; > + } else if (sdram_params->base.dramtype == DDR3) { > + /* DDR3L */ > + vref_mode_dq = 0x1; > + vref_value_dq = 0x1f; > + vref_mode_ac = 0x1; > + vref_value_ac = 0x1f; > + } else { > + debug("Halting: Unknown DRAM type.\n"); > + return -ENODEV; -EINVAL for consistency? > + } > + > + reg_value = (vref_mode_dq << 9) | (0x1 << 8) | vref_value_dq; > + [..] > +static int switch_to_phy_index1(struct dram_info *dram, > + const struct rk3399_sdram_params > *sdram_params) > +{ > + u32 channel; > + u32 *denali_phy; > + u32 ch_count = sdram_params->base.num_channels; > + int i = 0; > + > + writel(RK_CLRSETBITS(0x03 << 4 | 1 << 2 | 1, > + 1 << 4 | 1 << 2 | 1), > + &dram->cic->cic_ctrl0); > + while (!(readl(&dram->cic->cic_status0) & (1 << 2))) { > + mdelay(10); > + i++; > + if (i > 10) { > + debug("index1 frequency change overtime, reset\n"); > + return -ETIME; > + } > + } > + > + i = 0; > + writel(RK_CLRSETBITS(1 << 1, 1 << 1), &dram->cic->cic_ctrl0); > + while (!(readl(&dram->cic->cic_status0) & (1 << 0))) { > + mdelay(10); > + if (i > 10) { > + debug("index1 frequency done overtime, reset\n"); > + return -ETIME; > + } > + } > + > + for (channel = 0; channel < ch_count; channel++) { > + denali_phy = dram->chan[channel].publ->denali_phy; > + clrsetbits_le32(&denali_phy[896], (0x3 << 8) | 1, 1 << 8); > + if (data_training(&dram->chan[channel], channel, > + sdram_params, PI_FULL_TRAINING)) { > + debug("index1 training failed, reset\n"); > + return -EIO; Can you return the value of data_training() in this case, or do you want to change it to -EIO? > + } > + } > + > + return 0; > +} > + > +static int sdram_init(struct dram_info *dram, > + const struct rk3399_sdram_params *sdram_params) > +{ > + unsigned char dramtype = sdram_params->base.dramtype; > + unsigned int ddr_freq = sdram_params->base.ddr_freq; > + int channel; > + > + printf("Starting SDRAM initialization...\n"); debug()? > + > + if ((dramtype == DDR3 && ddr_freq > 800) || > + (dramtype == LPDDR3 && ddr_freq > 933) || > + (dramtype == LPDDR4 && ddr_freq > 800)) { > + debug("SDRAM frequency is to high!"); > + return -E2BIG; > + } > + > + for (channel = 0; channel < 2; channel++) { > + const struct chan_info *chan = &dram->chan[channel]; > + struct rk3399_ddr_publ_regs *publ = chan->publ; > + > + phy_dll_bypass_set(publ, ddr_freq); > + > + if (channel >= sdram_params->base.num_channels) > + continue; > + > + if (pctl_cfg(chan, channel, sdram_params) != 0) { > + printf("pctl_cfg fail, reset\n"); > + return -EIO; > + } > + > + /* LPDDR2/LPDDR3 need to wait DAI complete, max 10us */ > + if (dramtype == LPDDR3) > + udelay(10); > + > + if (data_training(chan, channel, > + sdram_params, PI_FULL_TRAINING)) { > + printf("SDRAM initialization failed, reset\n"); > + return -EIO; > + } > + > + set_ddrconfig(chan, sdram_params, channel, > + sdram_params->ch[channel].ddrconfig); > + } > + dram_all_config(dram, sdram_params); > + switch_to_phy_index1(dram, sdram_params); > + > + printf("Finish SDRAM initialization...\n"); debug()? > + return 0; > +} > +#endif > + > +size_t sdram_size_mb(struct dram_info *dram) > +{ > + u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4; > + size_t chipsize_mb = 0; > + size_t size_mb = 0; > + u32 ch; > + > + u32 sys_reg = readl(&dram->pmugrf->os_reg2); > + u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT) > + & SYS_REG_NUM_CH_MASK); > + > + for (ch = 0; ch < ch_num; ch++) { > + rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) & > + SYS_REG_RANK_MASK); > + col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & > SYS_REG_COL_MASK); > + bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & > SYS_REG_BK_MASK); > + cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) & > + SYS_REG_CS0_ROW_MASK); > + cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) & > + SYS_REG_CS1_ROW_MASK); > + bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) & > + SYS_REG_BW_MASK)); > + row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) & > + SYS_REG_ROW_3_4_MASK; > + > + chipsize_mb = (1 << (cs0_row + col + bk + bw - 20)); > + > + if (rank > 1) > + chipsize_mb += chipsize_mb >> (cs0_row - cs1_row); > + if (row_3_4) > + chipsize_mb = chipsize_mb * 3 / 4; > + size_mb += chipsize_mb; > + } > + > + /* > + * we use the 0x00000000~0xf7ffffff space > + * since 0xf8000000~0xffffffff is soc register space > + * so we reserve it > + */ > + size_mb = min_t(size_t, size_mb, 0xf8000000/(1<<20)); > + > + return size_mb; > +} > + > +static int rk3399_dmc_probe(struct udevice *dev) > +{ > + struct dram_info *priv = dev_get_priv(dev); > +#ifdef CONFIG_SPL_BUILD Can you put all of this code within #ifdef in a separate function, something like rk3399_dmc_init()? Also you should be able to build without of-platdata - i.e. you still need the code which reads from device tree. See for example how rk3288 does it. I think you just need to add two versions of the code in this #ifdef. See the of-platdata docs: "Drivers should always support device tree as an option. The of-platdata feature is intended as a add-on to existing drivers." > + struct rockchip_dmc_plat *plat = dev_get_platdata(dev); > + struct dtd_rockchip_rk3399_dmc *dtplat = &plat->dtplat; > + struct rk3399_sdram_params *params = > + (void *)dtplat->rockchip_sdram_params; > + int ret; > + > + priv->cic = syscon_get_first_range(ROCKCHIP_SYSCON_CIC); > + priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); > + priv->pmusgrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUSGRF); > + priv->pmucru = rockchip_get_pmucru(); > + priv->cru = rockchip_get_cru(); > + ret = regmap_init_mem_platdata(dev, dtplat->reg, > + ARRAY_SIZE(dtplat->reg) / 4, > + &plat->map); > + if (ret) > + return ret; > + priv->chan[0].pctl = regmap_get_range(plat->map, 0); > + priv->chan[0].pi = regmap_get_range(plat->map, 1); > + priv->chan[0].publ = regmap_get_range(plat->map, 2); > + priv->chan[0].msch = regmap_get_range(plat->map, 3); > + priv->chan[1].pctl = regmap_get_range(plat->map, 4); > + priv->chan[1].pi = regmap_get_range(plat->map, 5); > + priv->chan[1].publ = regmap_get_range(plat->map, 6); > + priv->chan[1].msch = regmap_get_range(plat->map, 7); > + > + debug("con reg %p %p %p %p %p %p %p %p\n", > + priv->chan[0].pctl, priv->chan[0].pi, > + priv->chan[0].publ, priv->chan[0].msch, > + priv->chan[1].pctl, priv->chan[1].pi, > + priv->chan[1].publ, priv->chan[1].msch); > + debug("cru %p, cic %p, grf %p, sgrf %p, pmucru %p\n", priv->cru, > + priv->cic, priv->pmugrf, priv->pmusgrf, priv->pmucru); > + > + ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, > &priv->ddr_clk); > + if (ret) { > + printf("%s clk get failed %d\n", __func__, ret); > + return ret; > + } > + ret = clk_set_rate(&priv->ddr_clk, params->base.ddr_freq * MHz); > + if (ret < 0) { > + printf("%s clk set failed %d\n", __func__, ret); > + return ret; > + } > + ret = sdram_init(priv, params); > + if (ret < 0) { > + printf("%s DRAM init failed%d\n", __func__, ret); > + return ret; > + } > +#else > + priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); > + debug("%s: pmugrf=%p\n", __func__, priv->pmugrf); > +#endif > + return 0; > +} > + [..] Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot