Re: [U-Boot] [PATCH 1/6] arm64: rk3399: add ddr controller driver

Hi Kever,

On 18 January 2017 at 05:16, Kever Yang kever.yang@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@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