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

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

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.
>
> 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.

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
>
>
>