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

[Kever Yang <kever.yang@rock-chips.com>]

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 <kever.yang@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@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?
>>
>> 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
>>>
>>>
>>>
>>
>
>