[U-Boot] [PATCH 1/6] arm64: rk3399: add ddr controller driver
Simon Glass
sjg at chromium.org
Wed Feb 8 05:10:28 UTC 2017
+Tom in case you have some thoughts
Hi Kever,
On 4 February 2017 at 18:45, Kever Yang <kever.yang at rock-chips.com> wrote:
> Hi Simon,
>
> On 01/26/2017 10:23 PM, Simon Glass wrote:
>>
>> 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?
>
>
> 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.
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?
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?
>>
>>> + 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