[U-Boot] [PATCH 1/6] arm64: rk3399: add ddr controller driver
Simon Glass
sjg at chromium.org
Thu Jan 26 15:23:27 CET 2017
Hi Kever,
On 18 January 2017 at 05:16, 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>
> ---
>
> arch/arm/dts/rk3399-sdram-lpddr3-4GB-1600.dtsi | 1536 +++++++++++++++++++++
> arch/arm/include/asm/arch-rockchip/sdram_rk3399.h | 120 ++
> arch/arm/mach-rockchip/rk3399/Makefile | 1 +
> arch/arm/mach-rockchip/rk3399/sdram_rk3399.c | 1243 +++++++++++++++++
> 4 files changed, 2900 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
>
Change log?
Re your comments about MASK/SHIFT and register definitions, I'm not
convinced. In my case I set up a Python script to create the registers
(with the MASK set incorrectly unfortunately) and did something like
this:
pdftotext -layout -f 130 -l 350 rk3288-fs.pdf /tmp/asc
tools/rkmux.py /tmp/asc GRF_GPIO4C_IOMUX
to generate the definitions. Does that help?
My concern is that if we don't write a good quality driver now, when
will it be updated/fixed? It seems better to do it now. Is it really a
lot of work?
> 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..5568be2
> --- /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
Can you use lower-case hex please?
> + 0x3
> + 0x2
> + 0x2
> + 0x0
...
> +#define SYS_REG_ENC_ROW_3_4(n, ch) ((n) << (30 + (ch)))
> +#define SYS_REG_DEC_ROW_3_4(n, ch) ((n >> (30 + ch)) & 0x1)
> +#define SYS_REG_ENC_CHINFO(ch) (1 << (28 + (ch)))
> +#define SYS_REG_ENC_DDRTYPE(n) ((n) << 13)
> +#define SYS_REG_ENC_NUM_CH(n) (((n) - 1) << 12)
> +#define SYS_REG_DEC_NUM_CH(n) (1 + ((n >> 12) & 0x1))
> +#define SYS_REG_ENC_RANK(n, ch) (((n) - 1) << (11 + ((ch) * 16)))
> +#define SYS_REG_DEC_RANK(n, ch) (1 + ((n >> (11 + 16 * ch)) & 0x1))
> +#define SYS_REG_ENC_COL(n, ch) (((n) - 9) << (9 + ((ch) * 16)))
> +#define SYS_REG_DEC_COL(n, ch) (9 + ((n >> (9 + 16 * ch)) & 0x3))
> +#define SYS_REG_ENC_BK(n, ch) (((n) == 3 ? 0 : 1) \
> + << (8 + ((ch) * 16)))
> +#define SYS_REG_DEC_BK(n, ch) (3 - ((n >> (8 + 16 * ch)) & 0x1))
> +#define SYS_REG_ENC_CS0_ROW(n, ch) (((n) - 13) << (6 + ((ch) * 16)))
> +#define SYS_REG_DEC_CS0_ROW(n, ch) (13 + ((n >> (6 + 16 * ch)) & 0x3))
> +#define SYS_REG_ENC_CS1_ROW(n, ch) (((n) - 13) << (4 + ((ch) * 16)))
> +#define SYS_REG_DEC_CS1_ROW(n, ch) (13 + ((n >> (4 + 16 * ch)) & 0x3))
> +#define SYS_REG_ENC_BW(n, ch) ((2 >> (n)) << (2 + ((ch) * 16)))
> +#define SYS_REG_DEC_BW(n, ch) (2 >> ((n >> (2 + 16 * ch)) & 0x3))
> +#define SYS_REG_ENC_DBW(n, ch) ((2 >> (n)) << (0 + ((ch) * 16)))
> +#define SYS_REG_DEC_DBW(n, ch) (2 >> ((n >> (0 + 16 * ch)) & 0x3))
These seem impenetrable to me :-( Is this the coreboot code standard?
> +
> +#define DDR_STRIDE(n, r) writel((0x1F << (10 + 16)) \
> + | (n << 10), r)
This is only used one, so can you just inline it?
> +
> +#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))
> +
> +#define PHY_DRV_ODT_Hi_Z (0x0)
Drop () on these simple constants.
> +#define PHY_DRV_ODT_240 (0x1)
> +#define PHY_DRV_ODT_120 (0x8)
> +#define PHY_DRV_ODT_80 (0x9)
> +#define PHY_DRV_ODT_60 (0xc)
> +#define PHY_DRV_ODT_48 (0xd)
> +#define PHY_DRV_ODT_40 (0xe)
> +#define PHY_DRV_ODT_34_3 (0xf)
> +
> +#ifdef CONFIG_SPL_BUILD
> +
> +struct rockchip_dmc_plat {
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> + struct dtd_rockchip_rk3399_dmc dtplat;
> +#endif
> + struct regmap *map;
> +};
> +
> +static void copy_to_reg(u32 *dest, const u32 *src, u32 n)
> +{
> + int i;
> +
> + for (i = 0; i < n / sizeof(u32); i++) {
> + writel(*src, dest);
> + src++;
> + dest++;
> + }
> +}
> +
> +static void phy_dll_bypass_set(struct rk3399_ddr_publ_regs *ddr_publ_regs,
> + u32 freq)
> +{
> + u32 *denali_phy = ddr_publ_regs->denali_phy;
> +
> + if (freq <= 125) {
Please add a comment as to why 125 is the magic number.
> + /* phy_sw_master_mode_X PHY_86/214/342/470 4bits offset_8 */
> + setbits_le32(&denali_phy[86], (0x3 << 2) << 8);
> + setbits_le32(&denali_phy[214], (0x3 << 2) << 8);
> + setbits_le32(&denali_phy[342], (0x3 << 2) << 8);
> + setbits_le32(&denali_phy[470], (0x3 << 2) << 8);
> +
> + /* phy_adrctl_sw_master_mode PHY_547/675/803 4bits offset_16 */
> + setbits_le32(&denali_phy[547], (0x3 << 2) << 16);
> + setbits_le32(&denali_phy[675], (0x3 << 2) << 16);
> + setbits_le32(&denali_phy[803], (0x3 << 2) << 16);
> + } else {
> + /* phy_sw_master_mode_X PHY_86/214/342/470 4bits offset_8 */
> + clrbits_le32(&denali_phy[86], (0x3 << 2) << 8);
> + clrbits_le32(&denali_phy[214], (0x3 << 2) << 8);
> + clrbits_le32(&denali_phy[342], (0x3 << 2) << 8);
> + clrbits_le32(&denali_phy[470], (0x3 << 2) << 8);
> +
> + /* phy_adrctl_sw_master_mode PHY_547/675/803 4bits offset_16 */
> + clrbits_le32(&denali_phy[547], (0x3 << 2) << 16);
> + clrbits_le32(&denali_phy[675], (0x3 << 2) << 16);
> + clrbits_le32(&denali_phy[803], (0x3 << 2) << 16);
> + }
> +}
> +
> +static void set_memory_map(const struct chan_info *chan, u32 channel,
> + const struct rk3399_sdram_params *sdram_params)
> +{
> + const struct rk3399_sdram_channel *sdram_ch =
> + &sdram_params->ch[channel];
> + u32 *denali_ctl = chan->pctl->denali_ctl;
> + u32 *denali_pi = chan->pi->denali_pi;
> + u32 cs_map;
> + u32 reduc;
> + u32 row;
> +
> + /* Get row number from ddrconfig setting */
> + if ((sdram_ch->ddrconfig < 2) || (sdram_ch->ddrconfig == 4))
nit: too many brackets
> + row = 16;
> + else if (sdram_ch->ddrconfig == 3)
> + row = 14;
> + else
> + row = 15;
> +
> + cs_map = (sdram_ch->rank > 1) ? 3 : 1;
> + reduc = (sdram_ch->bw == 2) ? 0 : 1;
> +
> + clrsetbits_le32(&denali_ctl[191], 0xF, (12 - sdram_ch->col));
> + clrsetbits_le32(&denali_ctl[190], (0x3 << 16) | (0x7 << 24),
> + ((3 - sdram_ch->bk) << 16) |
> + ((16 - row) << 24));
What are all these magic numbers? Shouldn't there be register names in
denali_ctl, i.e have it as a struct instead of 191, 190?
> + clrsetbits_le32(&denali_phy[6], 0xffffff, reg_value);
> + clrsetbits_le32(&denali_phy[134], 0xffffff, reg_value);
> + clrsetbits_le32(&denali_phy[262], 0xffffff, reg_value);
> + clrsetbits_le32(&denali_phy[390], 0xffffff, reg_value);
> +
> + /*
> + * phy_dqs_tsel_select_X 24bits DENALI_PHY_7/135/263/391 offset_0
> + * sets termination values for read/idle cycles and drive strength
> + * for write cycles for DQS
> + */
...
Regards,
Simon
More information about the U-Boot
mailing list