From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kever Yang Date: Mon, 13 Feb 2017 15:52:46 +0800 Subject: [U-Boot] [PATCH 1/6] arm64: rk3399: add ddr controller driver In-Reply-To: References: <1484741774-22289-1-git-send-email-kever.yang@rock-chips.com> <1484741774-22289-2-git-send-email-kever.yang@rock-chips.com> <589691B9.6010705@rock-chips.com> Message-ID: <58A165CE.7020801@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 02/08/2017 01:10 PM, Simon Glass wrote: > +Tom in case you have some thoughts > > Hi Kever, > > On 4 February 2017 at 18:45, Kever Yang wrote: >> Hi Simon, >> >> On 01/26/2017 10:23 PM, Simon Glass wrote: >>> Hi Kever, >>> >>> On 18 January 2017 at 05:16, Kever Yang wrote: >>>> RK3399 support DDR3, LPDDR3, DDR4 sdram, this patch is porting from >>>> coreboot, support 4GB lpddr3 in this version. >>>> >>>> Signed-off-by: Kever Yang >>>> --- >>>> >>>> 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? >> >> Sorry for missing change log, basically, I apply all the comments from you >> but >> MASK/SHIFT. >> >>> 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? >> >> Is not only header file need to update, but also almost all the register >> operation in C source code. > Yes that's right. > >>> >>> 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? >> >> I agree that we should write a good quality driver, but it does not mean >> the quality is not good if I don't include SHIFT in MASK, right? >> I would like to include the SHIFT in MASK if this is the first time for >> the driver commit to public as the requirement from maintainer, >> but this is porting from coreboot which has prove to be a good quality >> driver >> with a lot of test. >> Another point is that in order to make it easier to maintain the source >> code, >> we have already sync our internal source code to the copy on coreboot, it's >> really not convenient for the driver owner(not me, there are other guys >> focus >> on dram drivers) to maintain all these drivers in different platform. >> And here comes another question, what should we do for next SoC driver >> if we need to upstream the driver to coreboot and U-Boot, and maybe UEFI, >> make totally different format version for different platform? I think try to >> convince some of the maintainer should be the best choice and then we can >> focus on maintain the same one copy of source code, right? >> >> Back to U-Boot, I don't the format of MASK is so important, just like no >> more >> than 80-bytes in one line in coding style, we should consider to accept it >> if >> there are some reason. There are also many MASKs in U-Boot drivers without >> SHIFT in it, I wouldn't say which kind is better, but U-Boot can say prefer >> to >> use MASKs with SHIFT, but please don't refuse everything other than that. > Well we can leave the MASK to not include the SHIFT. That was my > mistake originally so perhaps I should clean it up. Thanks very much. > > But this sort of thing: > >> +#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)) > is really not nice IMO, particularly for something that is only used > once in the C code. Does coreboot actually require this style, or > could it use the more explicit SHIFT/MASK used like U-Boot? The sys_reg is for record dram size info, also appear in rk3288, this change is intent to make easier to read the C source code, I will update this blob of code back to the style like rk3288 dram driver. > > I do understand the problem of multiple platforms but in general each > project has its own code style and we try to stick to it. U-Boot > follows kernel style pretty closely, but it is try that SHIFT/MASK > things are much more common in U-Boot than Linux since it sets up > hardware. > > Re keeping the drivers in sync, yes it is a pain. But I hope that it > becomes easier to maintain. There is a pretty big user community > around U-Boot, and Rockchip is a popular chip. So I'm trying to keep > it as a good example of how to do things. > > So I'd really like to change this. > > Regards, > Simon > >> Thanks, >> - Kever >> >> >>>> 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? Will update. >>> >>>> + 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? Will update. >>> >>>> + >>>> +#define DDR_STRIDE(n, r) writel((0x1F << (10 + 16)) \ >>>> + | (n << 10), r) >>> This is only used one, so can you just inline it? Will remove this Macro and use C directly. >>> >>>> + >>>> +#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. Will update. >>> >>>> +#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. Will update. >>> >>>> + /* 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 Will update. >>> >>>> + 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? Will update the comment, there is no register name in IP spec. Thanks, - Kever >>> >>>> + 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 >>> >>> >>> >> > >