[U-Boot] [PATCH v2 1/9] arm64: rk3399: add ddr controller driver
Simon Glass
sjg at chromium.org
Thu Feb 16 20:43:42 UTC 2017
Hi Kever,
On 13 February 2017 at 02:38, Kever Yang <kever.yang at 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.yang at 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
More information about the U-Boot
mailing list