[U-Boot,v3,13/16] rockchip: rk3188: Add sdram driver

The sdram controller blocks are very similar to the rk3288 in utilizing
memory scheduler, Designware uPCTL and Designware PUBL blocks, only
limited to one bank instead of two.

There are some minimal differences when setting up the ram, so it gets
a separate driver for the rk3188 but reuses the driver structs, as there
is no need to define the same again.

More optimization can happen when the modelling of the controller parts
in the dts actually follow the hardware layout hopefully at some point
in the future.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/include/asm/arch-rockchip/ddr_rk3188.h |  22 +
 arch/arm/mach-rockchip/rk3188/Makefile          |   1 +
 arch/arm/mach-rockchip/rk3188/sdram_rk3188.c    | 985 ++++++++++++++++++++++++
 3 files changed, 1008 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
 create mode 100644 arch/arm/mach-rockchip/rk3188/sdram_rk3188.c

Hi Heiko,

On 3 February 2017 at 08:09, Heiko Stuebner <heiko@sntech.de> wrote:
> The sdram controller blocks are very similar to the rk3288 in utilizing
> memory scheduler, Designware uPCTL and Designware PUBL blocks, only
> limited to one bank instead of two.
>
> There are some minimal differences when setting up the ram, so it gets
> a separate driver for the rk3188 but reuses the driver structs, as there
> is no need to define the same again.
>
> More optimization can happen when the modelling of the controller parts
> in the dts actually follow the hardware layout hopefully at some point
> in the future.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/arm/include/asm/arch-rockchip/ddr_rk3188.h |  22 +
>  arch/arm/mach-rockchip/rk3188/Makefile          |   1 +
>  arch/arm/mach-rockchip/rk3188/sdram_rk3188.c    | 985 ++++++++++++++++++++++++
>  3 files changed, 1008 insertions(+)
>  create mode 100644 arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
>  create mode 100644 arch/arm/mach-rockchip/rk3188/sdram_rk3188.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Nits below.

>
> diff --git a/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h b/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
> new file mode 100644
> index 0000000000..993c58d1a8
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
> @@ -0,0 +1,22 @@
> +/*
> + * (C) Copyright 2015 Google, Inc
> + *
> + * SPDX-License-Identifier:    GPL-2.0
> + */
> +
> +#ifndef _ASM_ARCH_DDR_RK3188_H
> +#define _ASM_ARCH_DDR_RK3188_H
> +
> +#include <asm/arch/ddr_rk3288.h>
> +
> +struct rk3188_msch {
> +       u32 coreid;
> +       u32 revisionid;
> +       u32 ddrconf;
> +       u32 ddrtiming;
> +       u32 ddrmode;
> +       u32 readlatency;

Can you comment this struct? What is it for?

> +};
> +check_member(rk3188_msch, readlatency, 0x0014);
> +
> +#endif
> diff --git a/arch/arm/mach-rockchip/rk3188/Makefile b/arch/arm/mach-rockchip/rk3188/Makefile
> index 7fa010405b..2dc9511de7 100644
> --- a/arch/arm/mach-rockchip/rk3188/Makefile
> +++ b/arch/arm/mach-rockchip/rk3188/Makefile
> @@ -6,5 +6,6 @@
>
>  ifndef CONFIG_TPL_BUILD
>  obj-y += clk_rk3188.o
> +obj-y += sdram_rk3188.o
>  obj-y += syscon_rk3188.o
>  endif
> diff --git a/arch/arm/mach-rockchip/rk3188/sdram_rk3188.c b/arch/arm/mach-rockchip/rk3188/sdram_rk3188.c
> new file mode 100644
> index 0000000000..9e41723c9f
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/rk3188/sdram_rk3188.c
> @@ -0,0 +1,985 @@
> +/*
> + * (C) Copyright 2015 Google, Inc
> + * Copyright 2014 Rockchip Inc.
> + *
> + * SPDX-License-Identifier:     GPL-2.0
> + *
> + * Adapted from coreboot.

Does coreboot support the rk3188?

> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <dt-structs.h>
> +#include <errno.h>
> +#include <ram.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/cru_rk3188.h>
> +#include <asm/arch/ddr_rk3188.h>
> +#include <asm/arch/grf_rk3188.h>
> +#include <asm/arch/pmu_rk3188.h>
> +#include <asm/arch/sdram.h>
> +#include <linux/err.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct chan_info {
> +       struct rk3288_ddr_pctl *pctl;
> +       struct rk3288_ddr_publ *publ;
> +       struct rk3188_msch *msch;
> +};
> +
> +struct dram_info {
> +       struct chan_info chan[1];
> +       struct ram_info info;
> +       struct clk ddr_clk;
> +       struct rk3188_cru *cru;
> +       struct rk3188_grf *grf;
> +       struct rk3188_sgrf *sgrf;
> +       struct rk3188_pmu *pmu;
> +};
> +
> +struct rk3188_sdram_params {
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +       struct dtd_rockchip_rk3188_dmc of_plat;
> +#endif
> +       struct rk3288_sdram_channel ch[2];
> +       struct rk3288_sdram_pctl_timing pctl_timing;
> +       struct rk3288_sdram_phy_timing phy_timing;
> +       struct rk3288_base_params base;
> +       int num_channels;
> +       struct regmap *map;
> +};
> +
> +#define TEST_PATTEN    0x5aa5f00f
> +#define DQS_GATE_TRAINING_ERROR_RANK0  (1 << 4)
> +#define DQS_GATE_TRAINING_ERROR_RANK1  (2 << 4)
> +
> +#ifdef CONFIG_SPL_BUILD
> +static void copy_to_reg(u32 *dest, const u32 *src, u32 n)

Seems like this should go in a common file as there are several users
- rk_copy_to_reg() ?

> +{
> +       int i;
> +
> +       for (i = 0; i < n / sizeof(u32); i++) {
> +               writel(*src, dest);
> +               src++;
> +               dest++;
> +       }
> +}
> +
> +static void ddr_reset(struct rk3188_cru *cru, u32 ch, u32 ctl, u32 phy)
> +{
> +       u32 phy_ctl_srstn_shift = 5 + 8 * ch;
> +       u32 ctl_psrstn_shift = 3 + 8 * ch;
> +       u32 ctl_srstn_shift = 2 + 8 * ch;
> +       u32 phy_psrstn_shift = 1 + 8 * ch;
> +       u32 phy_srstn_shift = 8 * ch;
> +
> +       rk_clrsetreg(&cru->cru_softrst_con[5],
> +                    1 << phy_ctl_srstn_shift | 1 << ctl_psrstn_shift |
> +                    1 << ctl_srstn_shift | 1 << phy_psrstn_shift |
> +                    1 << phy_srstn_shift,
> +                    phy << phy_ctl_srstn_shift | ctl << ctl_psrstn_shift |
> +                    ctl << ctl_srstn_shift | phy << phy_psrstn_shift |
> +                    phy << phy_srstn_shift);
> +}
> +
> +static void ddr_phy_ctl_reset(struct rk3188_cru *cru, u32 ch, u32 n)
> +{
> +       u32 phy_ctl_srstn_shift = 5 + 8 * ch;
> +
> +       rk_clrsetreg(&cru->cru_softrst_con[5],
> +                    1 << phy_ctl_srstn_shift, n << phy_ctl_srstn_shift);
> +}
> +
> +static void phy_pctrl_reset(struct rk3188_cru *cru,
> +                           struct rk3288_ddr_publ *publ,
> +                           u32 channel)
> +{
> +       int i;
> +
> +       ddr_reset(cru, channel, 1, 1);
> +       udelay(1);
> +       clrbits_le32(&publ->acdllcr, ACDLLCR_DLLSRST);
> +       for (i = 0; i < 4; i++)
> +               clrbits_le32(&publ->datx8[i].dxdllcr, DXDLLCR_DLLSRST);
> +
> +       udelay(10);
> +       setbits_le32(&publ->acdllcr, ACDLLCR_DLLSRST);
> +       for (i = 0; i < 4; i++)
> +               setbits_le32(&publ->datx8[i].dxdllcr, DXDLLCR_DLLSRST);
> +
> +       udelay(10);
> +       ddr_reset(cru, channel, 1, 0);
> +       udelay(10);
> +       ddr_reset(cru, channel, 0, 0);
> +       udelay(10);
> +}
> +
> +static void phy_dll_bypass_set(struct rk3288_ddr_publ *publ,
> +       u32 freq)
> +{
> +       int i;

blank line here

Do you have a comment for this logic, and why it is how it is?

> +       if (freq <= 250000000) {
> +               if (freq <= 150000000)
> +                       clrbits_le32(&publ->dllgcr, SBIAS_BYPASS);
> +               else
> +                       setbits_le32(&publ->dllgcr, SBIAS_BYPASS);
> +               setbits_le32(&publ->acdllcr, ACDLLCR_DLLDIS);
> +               for (i = 0; i < 4; i++)
> +                       setbits_le32(&publ->datx8[i].dxdllcr,
> +                                    DXDLLCR_DLLDIS);
> +
> +               setbits_le32(&publ->pir, PIR_DLLBYP);
> +       } else {
> +               clrbits_le32(&publ->dllgcr, SBIAS_BYPASS);
> +               clrbits_le32(&publ->acdllcr, ACDLLCR_DLLDIS);
> +               for (i = 0; i < 4; i++) {
> +                       clrbits_le32(&publ->datx8[i].dxdllcr,
> +                                    DXDLLCR_DLLDIS);
> +               }
> +
> +               clrbits_le32(&publ->pir, PIR_DLLBYP);
> +       }
> +}
> +
> +static void dfi_cfg(struct rk3288_ddr_pctl *pctl, u32 dramtype)
> +{
> +       writel(DFI_INIT_START, &pctl->dfistcfg0);
> +       writel(DFI_DRAM_CLK_SR_EN | DFI_DRAM_CLK_DPD_EN,
> +              &pctl->dfistcfg1);
> +       writel(DFI_PARITY_INTR_EN | DFI_PARITY_EN, &pctl->dfistcfg2);
> +       writel(7 << TLP_RESP_TIME_SHIFT | LP_SR_EN | LP_PD_EN,
> +              &pctl->dfilpcfg0);
> +
> +       writel(2 << TCTRL_DELAY_TIME_SHIFT, &pctl->dfitctrldelay);
> +       writel(1 << TPHY_WRDATA_TIME_SHIFT, &pctl->dfitphywrdata);
> +       writel(0xf << TPHY_RDLAT_TIME_SHIFT, &pctl->dfitphyrdlat);
> +       writel(2 << TDRAM_CLK_DIS_TIME_SHIFT, &pctl->dfitdramclkdis);
> +       writel(2 << TDRAM_CLK_EN_TIME_SHIFT, &pctl->dfitdramclken);
> +       writel(1, &pctl->dfitphyupdtype0);
> +
> +       /* cs0 and cs1 write odt enable */
> +       writel((RANK0_ODT_WRITE_SEL | RANK1_ODT_WRITE_SEL),
> +              &pctl->dfiodtcfg);
> +       /* odt write length */
> +       writel(7 << ODT_LEN_BL8_W_SHIFT, &pctl->dfiodtcfg1);
> +       /* phyupd and ctrlupd disabled */
> +       writel(0, &pctl->dfiupdcfg);
> +}
> +
> +static void ddr_set_enable(struct rk3188_grf *grf, uint channel, bool enable)
> +{
> +       uint val = 0;
> +
> +       if (enable)
> +               val = 1 << DDR_16BIT_EN_SHIFT;
> +
> +       rk_clrsetreg(&grf->ddrc_con0, 1 << DDR_16BIT_EN_SHIFT, val);
> +}
> +
> +static void ddr_set_ddr3_mode(struct rk3188_grf *grf, uint channel,
> +                             bool ddr3_mode)
> +{
> +       uint mask, val;
> +
> +       mask = 1 << MSCH4_MAINDDR3_SHIFT;
> +       val = ddr3_mode << MSCH4_MAINDDR3_SHIFT;
> +       rk_clrsetreg(&grf->soc_con2, mask, val);
> +}
> +
> +#define RANK_2_ROW15_EN 1

Can this go at the top of the file?

> +static void ddr_rank_2_row15en(struct rk3188_grf *grf, bool enable)
> +{
> +       uint mask, val;
> +
> +       mask = 1 << RANK_2_ROW15_EN;
> +       val = enable << RANK_2_ROW15_EN;
> +       rk_clrsetreg(&grf->soc_con2, mask, val);
> +}
> +
> +static void pctl_cfg(u32 channel, struct rk3288_ddr_pctl *pctl,
> +                    struct rk3188_sdram_params *sdram_params,
> +                    struct rk3188_grf *grf)
> +{
> +       copy_to_reg(&pctl->togcnt1u, &sdram_params->pctl_timing.togcnt1u,
> +                   sizeof(sdram_params->pctl_timing));
> +       switch (sdram_params->base.dramtype) {
> +       case DDR3:
> +               if (sdram_params->phy_timing.mr[1] & DDR3_DLL_DISABLE) {
> +                       writel(sdram_params->pctl_timing.tcl - 3,
> +                              &pctl->dfitrddataen);
> +               } else {
> +                       writel(sdram_params->pctl_timing.tcl - 2,
> +                              &pctl->dfitrddataen);
> +               }
> +               writel(sdram_params->pctl_timing.tcwl - 1,
> +                      &pctl->dfitphywrlat);
> +               writel(0 << MDDR_LPDDR2_CLK_STOP_IDLE_SHIFT | DDR3_EN |
> +                      DDR2_DDR3_BL_8 | (6 - 4) << TFAW_SHIFT | PD_EXIT_SLOW |
> +                      1 << PD_TYPE_SHIFT | 0 << PD_IDLE_SHIFT,
> +                      &pctl->mcfg);
> +               ddr_set_ddr3_mode(grf, channel, true);
> +               ddr_set_enable(grf, channel, true);
> +               break;
> +       }
> +
> +       setbits_le32(&pctl->scfg, 1);
> +}
> +
> +static void phy_cfg(const struct chan_info *chan, u32 channel,
> +                   struct rk3188_sdram_params *sdram_params)
> +{
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +       struct rk3188_msch *msch = chan->msch;
> +       uint ddr_freq_mhz = sdram_params->base.ddr_freq / 1000000;
> +       u32 dinit2;
> +       int i;
> +
> +       dinit2 = DIV_ROUND_UP(ddr_freq_mhz * 200000, 1000);
> +       /* DDR PHY Timing */
> +       copy_to_reg(&publ->dtpr[0], &sdram_params->phy_timing.dtpr0,
> +                   sizeof(sdram_params->phy_timing));
> +       writel(sdram_params->base.noc_timing, &msch->ddrtiming);
> +       writel(0x3f, &msch->readlatency);
> +       writel(DIV_ROUND_UP(ddr_freq_mhz * 5120, 1000) << PRT_DLLLOCK_SHIFT |
> +              DIV_ROUND_UP(ddr_freq_mhz * 50, 1000) << PRT_DLLSRST_SHIFT |
> +              8 << PRT_ITMSRST_SHIFT, &publ->ptr[0]);
> +       writel(DIV_ROUND_UP(ddr_freq_mhz * 500000, 1000) << PRT_DINIT0_SHIFT |
> +              DIV_ROUND_UP(ddr_freq_mhz * 400, 1000) << PRT_DINIT1_SHIFT,
> +              &publ->ptr[1]);
> +       writel(min(dinit2, 0x1ffffU) << PRT_DINIT2_SHIFT |
> +              DIV_ROUND_UP(ddr_freq_mhz * 1000, 1000) << PRT_DINIT3_SHIFT,
> +              &publ->ptr[2]);
> +
> +       switch (sdram_params->base.dramtype) {
> +       case DDR3:
> +               clrbits_le32(&publ->pgcr, 0x1f);
> +               clrsetbits_le32(&publ->dcr, DDRMD_MASK << DDRMD_SHIFT,
> +                               DDRMD_DDR3 << DDRMD_SHIFT);
> +               break;
> +       }
> +       if (sdram_params->base.odt) {
> +               /*dynamic RTT enable */
> +               for (i = 0; i < 4; i++)
> +                       setbits_le32(&publ->datx8[i].dxgcr, DQSRTT | DQRTT);
> +       } else {
> +               /*dynamic RTT disable */
> +               for (i = 0; i < 4; i++)
> +                       clrbits_le32(&publ->datx8[i].dxgcr, DQSRTT | DQRTT);
> +       }
> +}
> +
> +static void phy_init(struct rk3288_ddr_publ *publ)
> +{
> +       setbits_le32(&publ->pir, PIR_INIT | PIR_DLLSRST
> +               | PIR_DLLLOCK | PIR_ZCAL | PIR_ITMSRST | PIR_CLRSR);
> +       udelay(1);
> +       while ((readl(&publ->pgsr) &
> +               (PGSR_IDONE | PGSR_DLDONE | PGSR_ZCDONE)) !=
> +               (PGSR_IDONE | PGSR_DLDONE | PGSR_ZCDONE))
> +               ;
> +}
> +
> +static void send_command(struct rk3288_ddr_pctl *pctl, u32 rank,
> +                        u32 cmd, u32 arg)
> +{
> +       writel((START_CMD | (rank << 20) | arg | cmd), &pctl->mcmd);
> +       udelay(1);
> +       while (readl(&pctl->mcmd) & START_CMD)
> +               ;
> +}
> +
> +static inline void send_command_op(struct rk3288_ddr_pctl *pctl,
> +                                  u32 rank, u32 cmd, u32 ma, u32 op)
> +{
> +       send_command(pctl, rank, cmd, (ma & LPDDR2_MA_MASK) << LPDDR2_MA_SHIFT |
> +                    (op & LPDDR2_OP_MASK) << LPDDR2_OP_SHIFT);
> +}
> +
> +static void memory_init(struct rk3288_ddr_publ *publ,
> +                       u32 dramtype)
> +{
> +       setbits_le32(&publ->pir,
> +                    (PIR_INIT | PIR_DRAMINIT | PIR_LOCKBYP
> +                     | PIR_ZCALBYP | PIR_CLRSR | PIR_ICPC
> +                     | (dramtype == DDR3 ? PIR_DRAMRST : 0)));
> +       udelay(1);
> +       while ((readl(&publ->pgsr) & (PGSR_IDONE | PGSR_DLDONE))
> +               != (PGSR_IDONE | PGSR_DLDONE))
> +               ;
> +}
> +
> +static void move_to_config_state(struct rk3288_ddr_publ *publ,
> +                                struct rk3288_ddr_pctl *pctl)
> +{
> +       unsigned int state;
> +
> +       while (1) {
> +               state = readl(&pctl->stat) & PCTL_STAT_MSK;
> +
> +               switch (state) {
> +               case LOW_POWER:
> +                       writel(WAKEUP_STATE, &pctl->sctl);
> +                       while ((readl(&pctl->stat) & PCTL_STAT_MSK)
> +                               != ACCESS)
> +                               ;
> +                       /* wait DLL lock */
> +                       while ((readl(&publ->pgsr) & PGSR_DLDONE)
> +                               != PGSR_DLDONE)
> +                               ;
> +                       /* if at low power state,need wakeup first,

Check multi-line comment style:

/*
 * If at low-power state ...
 * ...
 */

> +                        * and then enter the config, so
> +                        * fallthrough
> +                        */
> +               case ACCESS:
> +                       /* fallthrough */
> +               case INIT_MEM:
> +                       writel(CFG_STATE, &pctl->sctl);
> +                       while ((readl(&pctl->stat) & PCTL_STAT_MSK) != CONFIG)
> +                               ;
> +                       break;
> +               case CONFIG:
> +                       return;
> +               default:
> +                       break;
> +               }
> +       }
> +}
> +
> +static void set_bandwidth_ratio(const struct chan_info *chan, u32 channel,
> +                               u32 n, struct rk3188_grf *grf)
> +{
> +       struct rk3288_ddr_pctl *pctl = chan->pctl;
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +       struct rk3188_msch *msch = chan->msch;
> +
> +       if (n == 1) {
> +               setbits_le32(&pctl->ppcfg, 1);
> +               ddr_set_enable(grf, channel, 1);
> +               setbits_le32(&msch->ddrtiming, 1 << 31);
> +               /* Data Byte disable*/
> +               clrbits_le32(&publ->datx8[2].dxgcr, 1);
> +               clrbits_le32(&publ->datx8[3].dxgcr, 1);
> +               /* disable DLL */
> +               setbits_le32(&publ->datx8[2].dxdllcr, DXDLLCR_DLLDIS);
> +               setbits_le32(&publ->datx8[3].dxdllcr, DXDLLCR_DLLDIS);
> +       } else {
> +               clrbits_le32(&pctl->ppcfg, 1);
> +               ddr_set_enable(grf, channel, 0);
> +               clrbits_le32(&msch->ddrtiming, 1 << 31);
> +               /* Data Byte enable*/
> +               setbits_le32(&publ->datx8[2].dxgcr, 1);
> +               setbits_le32(&publ->datx8[3].dxgcr, 1);
> +
> +               /* enable DLL */
> +               clrbits_le32(&publ->datx8[2].dxdllcr, DXDLLCR_DLLDIS);
> +               clrbits_le32(&publ->datx8[3].dxdllcr, DXDLLCR_DLLDIS);
> +               /* reset DLL */
> +               clrbits_le32(&publ->datx8[2].dxdllcr, DXDLLCR_DLLSRST);
> +               clrbits_le32(&publ->datx8[3].dxdllcr, DXDLLCR_DLLSRST);
> +               udelay(10);
> +               setbits_le32(&publ->datx8[2].dxdllcr, DXDLLCR_DLLSRST);
> +               setbits_le32(&publ->datx8[3].dxdllcr, DXDLLCR_DLLSRST);
> +       }
> +       setbits_le32(&pctl->dfistcfg0, 1 << 2);
> +}
> +
> +static int data_training(const struct chan_info *chan, u32 channel,
> +                        struct rk3188_sdram_params *sdram_params)

Function comment? What does it return? channel could just be int, right?

> +{
> +       unsigned int j;
> +       int ret = 0;
> +       u32 rank;
> +       int i;
> +       u32 step[2] = { PIR_QSTRN, PIR_RVTRN };
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +       struct rk3288_ddr_pctl *pctl = chan->pctl;
> +
> +       /* disable auto refresh */
> +       writel(0, &pctl->trefi);
> +
> +       if (sdram_params->base.dramtype != LPDDR3)
> +               setbits_le32(&publ->pgcr, 1 << PGCR_DQSCFG_SHIFT);
> +       rank = sdram_params->ch[channel].rank | 1;
> +       for (j = 0; j < ARRAY_SIZE(step); j++) {
> +               /*
> +                * trigger QSTRN and RVTRN
> +                * clear DTDONE status
> +                */
> +               setbits_le32(&publ->pir, PIR_CLRSR);
> +
> +               /* trigger DTT */
> +               setbits_le32(&publ->pir,
> +                            PIR_INIT | step[j] | PIR_LOCKBYP | PIR_ZCALBYP |
> +                            PIR_CLRSR);
> +               udelay(1);
> +               /* wait echo byte DTDONE */
> +               while ((readl(&publ->datx8[0].dxgsr[0]) & rank)
> +                       != rank)
> +                       ;
> +               while ((readl(&publ->datx8[1].dxgsr[0]) & rank)
> +                       != rank)
> +                       ;
> +               if (!(readl(&pctl->ppcfg) & 1)) {
> +                       while ((readl(&publ->datx8[2].dxgsr[0])
> +                               & rank) != rank)
> +                               ;
> +                       while ((readl(&publ->datx8[3].dxgsr[0])
> +                               & rank) != rank)
> +                               ;
> +               }
> +               if (readl(&publ->pgsr) &
> +                   (PGSR_DTERR | PGSR_RVERR | PGSR_RVEIRR)) {
> +                       ret = -1;
> +                       break;
> +               }
> +       }
> +       /* send some auto refresh to complement the lost while DTT */
> +       for (i = 0; i < (rank > 1 ? 8 : 4); i++)
> +               send_command(pctl, rank, REF_CMD, 0);
> +
> +       if (sdram_params->base.dramtype != LPDDR3)
> +               clrbits_le32(&publ->pgcr, 1 << PGCR_DQSCFG_SHIFT);
> +
> +       /* resume auto refresh */
> +       writel(sdram_params->pctl_timing.trefi, &pctl->trefi);
> +
> +       return ret;
> +}
> +
> +static void move_to_access_state(const struct chan_info *chan)
> +{
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +       struct rk3288_ddr_pctl *pctl = chan->pctl;
> +       unsigned int state;
> +
> +       while (1) {
> +               state = readl(&pctl->stat) & PCTL_STAT_MSK;
> +
> +               switch (state) {
> +               case LOW_POWER:
> +                       if (((readl(&pctl->stat) >> LP_TRIG_SHIFT) &
> +                                       LP_TRIG_MASK) == 1)
> +                               return;
> +
> +                       writel(WAKEUP_STATE, &pctl->sctl);
> +                       while ((readl(&pctl->stat) & PCTL_STAT_MSK) != ACCESS)
> +                               ;
> +                       /* wait DLL lock */
> +                       while ((readl(&publ->pgsr) & PGSR_DLDONE)
> +                               != PGSR_DLDONE)
> +                               ;
> +                       break;
> +               case INIT_MEM:
> +                       writel(CFG_STATE, &pctl->sctl);
> +                       while ((readl(&pctl->stat) & PCTL_STAT_MSK) != CONFIG)
> +                               ;
> +                       /* fallthrough */
> +               case CONFIG:
> +                       writel(GO_STATE, &pctl->sctl);
> +                       while ((readl(&pctl->stat) & PCTL_STAT_MSK) == CONFIG)
> +                               ;
> +                       break;
> +               case ACCESS:
> +                       return;
> +               default:
> +                       break;
> +               }
> +       }
> +}
> +
> +static void dram_cfg_rbc(const struct chan_info *chan, u32 chnum,
> +                        struct rk3188_sdram_params *sdram_params)
> +{
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +
> +       if (sdram_params->ch[chnum].bk == 3)
> +               clrsetbits_le32(&publ->dcr, PDQ_MASK << PDQ_SHIFT,
> +                               1 << PDQ_SHIFT);
> +       else
> +               clrbits_le32(&publ->dcr, PDQ_MASK << PDQ_SHIFT);
> +
> +       writel(sdram_params->base.ddrconfig, &chan->msch->ddrconf);
> +}
> +
> +static void dram_all_config(const struct dram_info *dram,
> +                           struct rk3188_sdram_params *sdram_params)
> +{
> +       unsigned int chan;
> +       u32 sys_reg = 0;
> +
> +       sys_reg |= sdram_params->base.dramtype << SYS_REG_DDRTYPE_SHIFT;
> +       sys_reg |= (sdram_params->num_channels - 1) << SYS_REG_NUM_CH_SHIFT;
> +       for (chan = 0; chan < sdram_params->num_channels; chan++) {
> +               const struct rk3288_sdram_channel *info =
> +                       &sdram_params->ch[chan];
> +
> +               sys_reg |= info->row_3_4 << SYS_REG_ROW_3_4_SHIFT(chan);
> +               sys_reg |= 1 << SYS_REG_CHINFO_SHIFT(chan);
> +               sys_reg |= (info->rank - 1) << SYS_REG_RANK_SHIFT(chan);
> +               sys_reg |= (info->col - 9) << SYS_REG_COL_SHIFT(chan);
> +               sys_reg |= info->bk == 3 ? 0 : 1 << SYS_REG_BK_SHIFT(chan);
> +               sys_reg |= (info->cs0_row - 13) << SYS_REG_CS0_ROW_SHIFT(chan);
> +               sys_reg |= (info->cs1_row - 13) << SYS_REG_CS1_ROW_SHIFT(chan);
> +               sys_reg |= (2 >> info->bw) << SYS_REG_BW_SHIFT(chan);
> +               sys_reg |= (2 >> info->dbw) << SYS_REG_DBW_SHIFT(chan);
> +
> +               dram_cfg_rbc(&dram->chan[chan], chan, sdram_params);
> +       }
> +       if (sdram_params->ch[0].rank == 2)
> +               ddr_rank_2_row15en(dram->grf, 0);
> +       else
> +               ddr_rank_2_row15en(dram->grf, 1);
> +       writel(sys_reg, &dram->pmu->sys_reg[2]);
> +}

blank line

> +const int ddrconf_table[] = {
> +       /*
> +        * [5:4] row(13+n)
> +        * [1:0] col(9+n), assume bw=2
> +        * row      col,bw */
> +       0,
> +       ((2 << 4) | 1),
> +       ((1 << 4) | 1),
> +       ((0 << 4) | 1),
> +       ((2 << 4) | 2),
> +       ((1 << 4) | 2),
> +       ((0 << 4) | 2),
> +       ((1 << 4) | 0),
> +       ((0 << 4) | 0),
> +       0,
> +       0,
> +       0,
> +       0,
> +       0,
> +       0,
> +       0,
> +};

Can this go at the top of the file? What are the <<4 for ? Can we have
a #define?

> +
> +static int sdram_rank_bw_detect(struct dram_info *dram, int channel,
> +               struct rk3188_sdram_params *sdram_params)
> +{
> +       int reg;
> +       int need_trainig = 0;
> +       const struct chan_info *chan = &dram->chan[channel];
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +
> +       ddr_rank_2_row15en(dram->grf, 0);
> +
> +       if (-1 == data_training(chan, channel, sdram_params)) {

Can you check for <0 rather than == -1? That is the normal error check.

> +               printf("first data training fail!\n");
> +               reg = readl(&publ->datx8[0].dxgsr[0]);
> +               /* Check the result for rank 0 */
> +               if ((channel == 0) && (reg & DQS_GATE_TRAINING_ERROR_RANK0)) {
> +                       printf("data training fail!\n");
> +                               return -EIO;

funny indent here

> +               }
> +
> +               /* Check the result for rank 1 */
> +               if (reg & DQS_GATE_TRAINING_ERROR_RANK1) {
> +                       sdram_params->ch[channel].rank = 1;
> +                       clrsetbits_le32(&publ->pgcr, 0xF << 18,
> +                                       sdram_params->ch[channel].rank << 18);
> +                       need_trainig = 1;
> +               }
> +               reg = readl(&publ->datx8[2].dxgsr[0]);
> +               if (reg & (1 << 4)) {
> +                       sdram_params->ch[channel].bw = 1;
> +                       set_bandwidth_ratio(chan, channel,
> +                                           sdram_params->ch[channel].bw,
> +                                           dram->grf);
> +                       need_trainig = 1;
> +               }
> +       }
> +       /* Assume the Die bit width are the same with the chip bit width */
> +       sdram_params->ch[channel].dbw = sdram_params->ch[channel].bw;
> +
> +       if (need_trainig &&
> +           (-1 == data_training(chan, channel, sdram_params))) {

same as above

> +               if (sdram_params->base.dramtype == LPDDR3) {
> +                       ddr_phy_ctl_reset(dram->cru, channel, 1);
> +                       udelay(10);
> +                       ddr_phy_ctl_reset(dram->cru, channel, 0);
> +                       udelay(10);
> +               }
> +               printf("2nd data training failed!");
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int sdram_col_row_detect(struct dram_info *dram, int channel,
> +               struct rk3188_sdram_params *sdram_params)

Function comment

> +{
> +       int row, col;
> +       unsigned int addr;
> +       const struct chan_info *chan = &dram->chan[channel];
> +       struct rk3288_ddr_pctl *pctl = chan->pctl;
> +       struct rk3288_ddr_publ *publ = chan->publ;
> +       int ret = 0;
> +
> +       /* Detect col */
> +       for (col = 11; col >= 9; col--) {
> +               writel(0, CONFIG_SYS_SDRAM_BASE);
> +               addr = CONFIG_SYS_SDRAM_BASE +
> +                       (1 << (col + sdram_params->ch[channel].bw - 1));
> +               writel(TEST_PATTEN, addr);
> +               if ((readl(addr) == TEST_PATTEN) &&
> +                   (readl(CONFIG_SYS_SDRAM_BASE) == 0))
> +                       break;
> +       }
> +       if (col == 8) {
> +               printf("Col detect error\n");
> +               ret = -EINVAL;
> +               goto out;
> +       } else {
> +               sdram_params->ch[channel].col = col;
> +       }
> +
> +       ddr_rank_2_row15en(dram->grf, 1);
> +       move_to_config_state(publ, pctl);
> +       writel(1, &chan->msch->ddrconf);
> +       move_to_access_state(chan);
> +       /* Detect row, max 15,min13 in rk3188*/
> +       for (row = 16; row >= 13; row--) {
> +               writel(0, CONFIG_SYS_SDRAM_BASE);
> +               addr = CONFIG_SYS_SDRAM_BASE + (1 << (row + 15 - 1));
> +               writel(TEST_PATTEN, addr);
> +               if ((readl(addr) == TEST_PATTEN) &&
> +                   (readl(CONFIG_SYS_SDRAM_BASE) == 0))
> +                       break;
> +       }
> +       if (row == 12) {
> +               printf("Row detect error\n");
> +               ret = -EINVAL;
> +       } else {
> +               sdram_params->ch[channel].cs1_row = row;
> +               sdram_params->ch[channel].row_3_4 = 0;
> +               printf("chn %d col %d, row %d\n", channel, col, row);
> +               sdram_params->ch[channel].cs0_row = row;
> +       }
> +
> +out:
> +       return ret;
> +}
> +
> +static int sdram_get_niu_config(struct rk3188_sdram_params *sdram_params)
> +{
> +       int i, tmp, size, ret = 0;
> +
> +       tmp = sdram_params->ch[0].col - 9;
> +       tmp -= (sdram_params->ch[0].bw == 2) ? 0 : 1;
> +       tmp |= ((sdram_params->ch[0].cs0_row - 13) << 4);
> +       size = sizeof(ddrconf_table)/sizeof(ddrconf_table[0]);
> +       for (i = 0; i < size; i++)
> +               if (tmp == ddrconf_table[i])
> +                       break;
> +       if (i >= size) {
> +               printf("niu config not found\n");
> +               ret = -EINVAL;
> +       } else {
> +               printf("niu config %d\n", i);
> +               sdram_params->base.ddrconfig = i;
> +       }
> +
> +       return ret;
> +}
> +
> +static int sdram_init(struct dram_info *dram,
> +                     struct rk3188_sdram_params *sdram_params)
> +{
> +       int channel;
> +       int zqcr;
> +       int ret;
> +
> +       printf("%s start 001\n", __func__);

debug()

> +       if ((sdram_params->base.dramtype == DDR3 &&
> +            sdram_params->base.ddr_freq > 800000000)) {
> +               printf("SDRAM frequency is too high!");
> +               return -E2BIG;
> +       }
> +
> +       ret = clk_set_rate(&dram->ddr_clk, sdram_params->base.ddr_freq);
> +       printf("ret=%d\n", ret);

debug()

> +       if (ret) {
> +               printf("Could not set DDR clock\n");
> +               return ret;
> +       }
> +
> +       for (channel = 0; channel < 1; channel++) {
> +               const struct chan_info *chan = &dram->chan[channel];
> +               struct rk3288_ddr_pctl *pctl = chan->pctl;
> +               struct rk3288_ddr_publ *publ = chan->publ;
> +
> +               phy_pctrl_reset(dram->cru, publ, channel);
> +               phy_dll_bypass_set(publ, sdram_params->base.ddr_freq);
> +
> +               dfi_cfg(pctl, sdram_params->base.dramtype);
> +
> +               pctl_cfg(channel, pctl, sdram_params, dram->grf);
> +
> +               phy_cfg(chan, channel, sdram_params);
> +
> +               phy_init(publ);
> +
> +               writel(POWER_UP_START, &pctl->powctl);
> +               while (!(readl(&pctl->powstat) & POWER_UP_DONE))
> +                       ;
> +
> +               memory_init(publ, sdram_params->base.dramtype);
> +               move_to_config_state(publ, pctl);
> +
> +               /* Using 32bit bus width for detect */
> +               sdram_params->ch[channel].bw = 2;
> +               set_bandwidth_ratio(chan, channel,
> +                                   sdram_params->ch[channel].bw, dram->grf);
> +               /*
> +                * set cs, using n=3 for detect
> +                * CS0, n=1
> +                * CS1, n=2
> +                * CS0 & CS1, n = 3
> +                */
> +               sdram_params->ch[channel].rank = 2,
> +               clrsetbits_le32(&publ->pgcr, 0xF << 18,
> +                               (sdram_params->ch[channel].rank | 1) << 18);
> +
> +               /* DS=40ohm,ODT=155ohm */
> +               zqcr = 1 << ZDEN_SHIFT | 2 << PU_ONDIE_SHIFT |
> +                       2 << PD_ONDIE_SHIFT | 0x19 << PU_OUTPUT_SHIFT |
> +                       0x19 << PD_OUTPUT_SHIFT;
> +               writel(zqcr, &publ->zq1cr[0]);
> +               writel(zqcr, &publ->zq0cr[0]);
> +
> +               /* Detect the rank and bit-width with data-training */
> +               writel(1, &chan->msch->ddrconf);
> +               sdram_rank_bw_detect(dram, channel, sdram_params);
> +
> +               if (sdram_params->base.dramtype == LPDDR3) {
> +                       u32 i;
> +                       writel(0, &pctl->mrrcfg0);
> +                       for (i = 0; i < 17; i++)
> +                               send_command_op(pctl, 1, MRR_CMD, i, 0);
> +               }
> +               writel(4, &chan->msch->ddrconf);
> +               move_to_access_state(chan);
> +               /* DDR3 and LPDDR3 are always 8 bank, no need detect */
> +               sdram_params->ch[channel].bk = 3;
> +               /* Detect Col and Row number*/
> +               ret = sdram_col_row_detect(dram, channel, sdram_params);
> +               if (ret)
> +                       goto error;
> +       }
> +       /* Find NIU DDR configuration */
> +       ret = sdram_get_niu_config(sdram_params);
> +       if (ret)
> +               goto error;
> +
> +       dram_all_config(dram, sdram_params);
> +       printf("%s done\n", __func__);
> +
> +       return 0;
> +error:
> +       printf("DRAM init failed!\n");
> +       hang();
> +}
> +#endif /* CONFIG_SPL_BUILD */
> +
> +size_t sdram_size_mb(struct rk3188_pmu *pmu)
> +{
> +       u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4;
> +       size_t chipsize_mb = 0;
> +       size_t size_mb = 0;
> +       u32 ch;
> +       u32 sys_reg = readl(&pmu->sys_reg[2]);
> +       u32 chans;
> +
> +       chans = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT) & SYS_REG_NUM_CH_MASK);
> +
> +       for (ch = 0; ch < chans; ch++) {
> +               rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> +                       SYS_REG_RANK_MASK);
> +               col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> +               bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> +               cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> +                               SYS_REG_CS0_ROW_MASK);
> +               cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> +                               SYS_REG_CS1_ROW_MASK);
> +               bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> +                       SYS_REG_BW_MASK));
> +               row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> +                       SYS_REG_ROW_3_4_MASK;
> +               chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> +
> +               if (rank > 1)
> +                       chipsize_mb += chipsize_mb >>
> +                               (cs0_row - cs1_row);
> +               if (row_3_4)
> +                       chipsize_mb = chipsize_mb * 3 / 4;
> +               size_mb += chipsize_mb;
> +       }
> +
> +       /* there can be no more than 2gb of memory */
> +       size_mb = min(size_mb, 0x80000000 >> 20);
> +
> +       return size_mb;
> +}
> +
> +#ifdef CONFIG_SPL_BUILD
> +static int setup_sdram(struct udevice *dev)
> +{
> +       struct dram_info *priv = dev_get_priv(dev);
> +       struct rk3188_sdram_params *params = dev_get_platdata(dev);
> +
> +       return sdram_init(priv, params);
> +}
> +
> +static int rk3188_dmc_ofdata_to_platdata(struct udevice *dev)
> +{
> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
> +       struct rk3188_sdram_params *params = dev_get_platdata(dev);
> +       const void *blob = gd->fdt_blob;
> +       int node = dev->of_offset;
> +       int ret;
> +
> +       /* rk3188 supports only one-channel */
> +       params->num_channels = 1;
> +       ret = fdtdec_get_int_array(blob, node, "rockchip,pctl-timing",
> +                                  (u32 *)&params->pctl_timing,
> +                                  sizeof(params->pctl_timing) / sizeof(u32));
> +       if (ret) {
> +               printf("%s: Cannot read rockchip,pctl-timing\n", __func__);
> +               return -EINVAL;
> +       }
> +       ret = fdtdec_get_int_array(blob, node, "rockchip,phy-timing",
> +                                  (u32 *)&params->phy_timing,
> +                                  sizeof(params->phy_timing) / sizeof(u32));
> +       if (ret) {
> +               printf("%s: Cannot read rockchip,phy-timing\n", __func__);
> +               return -EINVAL;
> +       }
> +       ret = fdtdec_get_int_array(blob, node, "rockchip,sdram-params",
> +                                  (u32 *)&params->base,
> +                                  sizeof(params->base) / sizeof(u32));
> +       if (ret) {
> +               printf("%s: Cannot read rockchip,sdram-params\n", __func__);
> +               return -EINVAL;
> +       }
> +       ret = regmap_init_mem(dev, &params->map);
> +       if (ret)
> +               return ret;
> +#endif
> +
> +       return 0;
> +}
> +#endif /* CONFIG_SPL_BUILD */
> +
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +static int conv_of_platdata(struct udevice *dev)
> +{
> +       struct rk3188_sdram_params *plat = dev_get_platdata(dev);
> +       struct dtd_rockchip_rk3188_dmc *of_plat = &plat->of_plat;
> +       int ret;
> +
> +       memcpy(&plat->pctl_timing, of_plat->rockchip_pctl_timing,
> +              sizeof(plat->pctl_timing));
> +       memcpy(&plat->phy_timing, of_plat->rockchip_phy_timing,
> +              sizeof(plat->phy_timing));
> +       memcpy(&plat->base, of_plat->rockchip_sdram_params, sizeof(plat->base));
> +       /* rk3188 supports dual-channel, set default channel num to 2 */
> +       plat->num_channels = 1;
> +       ret = regmap_init_mem_platdata(dev, of_plat->reg,
> +                                      ARRAY_SIZE(of_plat->reg) / 2,
> +                                      &plat->map);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +#endif
> +
> +static int rk3188_dmc_probe(struct udevice *dev)
> +{
> +#ifdef CONFIG_SPL_BUILD
> +       struct rk3188_sdram_params *plat = dev_get_platdata(dev);
> +#endif
> +       struct dram_info *priv = dev_get_priv(dev);
> +       struct regmap *map;
> +       int ret;
> +       struct udevice *dev_clk;
> +
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +       ret = conv_of_platdata(dev);
> +       if (ret)
> +               return ret;
> +#endif
> +       map = syscon_get_regmap_by_driver_data(ROCKCHIP_SYSCON_NOC);
> +       if (IS_ERR(map))
> +               return PTR_ERR(map);
> +       priv->chan[0].msch = regmap_get_range(map, 0);
> +
> +       priv->grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
> +       priv->pmu = syscon_get_first_range(ROCKCHIP_SYSCON_PMU);
> +
> +#ifdef CONFIG_SPL_BUILD
> +       priv->chan[0].pctl = regmap_get_range(plat->map, 0);
> +       priv->chan[0].publ = regmap_get_range(plat->map, 1);
> +#endif
> +
> +       ret = rockchip_get_clk(&dev_clk);
> +       if (ret)
> +               return ret;
> +       priv->ddr_clk.id = CLK_DDR;
> +       ret = clk_request(dev_clk, &priv->ddr_clk);
> +       if (ret)
> +               return ret;
> +
> +       priv->cru = rockchip_get_cru();
> +       if (IS_ERR(priv->cru))
> +               return PTR_ERR(priv->cru);
> +#ifdef CONFIG_SPL_BUILD
> +       ret = setup_sdram(dev);
> +       if (ret)
> +               return ret;
> +#endif
> +       priv->info.base = 0;
> +       priv->info.size = sdram_size_mb(priv->pmu) << 20;
> +
> +       return 0;
> +}
> +
> +static int rk3188_dmc_get_info(struct udevice *dev, struct ram_info *info)
> +{
> +       struct dram_info *priv = dev_get_priv(dev);
> +
> +       *info = priv->info;
> +
> +       return 0;
> +}
> +
> +static struct ram_ops rk3188_dmc_ops = {
> +       .get_info = rk3188_dmc_get_info,
> +};
> +
> +static const struct udevice_id rk3188_dmc_ids[] = {
> +       { .compatible = "rockchip,rk3188-dmc" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(dmc_rk3188) = {
> +       .name = "rockchip_rk3188_dmc",
> +       .id = UCLASS_RAM,
> +       .of_match = rk3188_dmc_ids,
> +       .ops = &rk3188_dmc_ops,
> +#ifdef CONFIG_SPL_BUILD
> +       .ofdata_to_platdata = rk3188_dmc_ofdata_to_platdata,
> +#endif
> +       .probe = rk3188_dmc_probe,
> +       .priv_auto_alloc_size = sizeof(struct dram_info),
> +#ifdef CONFIG_SPL_BUILD
> +       .platdata_auto_alloc_size = sizeof(struct rk3188_sdram_params),
> +#endif
> +};
> --
> 2.11.0
>

Regards,
Simon

Hi Simon,

Am Montag, 6. Februar 2017, 07:35:23 CET schrieb Simon Glass:
> On 3 February 2017 at 08:09, Heiko Stuebner <heiko@sntech.de> wrote:
> > The sdram controller blocks are very similar to the rk3288 in utilizing
> > memory scheduler, Designware uPCTL and Designware PUBL blocks, only
> > limited to one bank instead of two.
> > 
> > There are some minimal differences when setting up the ram, so it gets
> > a separate driver for the rk3188 but reuses the driver structs, as there
> > is no need to define the same again.
> > 
> > More optimization can happen when the modelling of the controller parts
> > in the dts actually follow the hardware layout hopefully at some point
> > in the future.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  arch/arm/include/asm/arch-rockchip/ddr_rk3188.h |  22 +
> >  arch/arm/mach-rockchip/rk3188/Makefile          |   1 +
> >  arch/arm/mach-rockchip/rk3188/sdram_rk3188.c    | 985
> >  ++++++++++++++++++++++++ 3 files changed, 1008 insertions(+)
> >  create mode 100644 arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
> >  create mode 100644 arch/arm/mach-rockchip/rk3188/sdram_rk3188.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Nits below.
> 
> > diff --git a/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
> > b/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h new file mode 100644
> > index 0000000000..993c58d1a8
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
> > @@ -0,0 +1,22 @@
> > +/*
> > + * (C) Copyright 2015 Google, Inc
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0
> > + */
> > +
> > +#ifndef _ASM_ARCH_DDR_RK3188_H
> > +#define _ASM_ARCH_DDR_RK3188_H
> > +
> > +#include <asm/arch/ddr_rk3288.h>
> > +
> > +struct rk3188_msch {
> > +       u32 coreid;
> > +       u32 revisionid;
> > +       u32 ddrconf;
> > +       u32 ddrtiming;
> > +       u32 ddrmode;
> > +       u32 readlatency;
> 
> Can you comment this struct? What is it for?

Added a comment that this the memory scheduler register map, but that is all I 
can say for it, as it is sparsely documented in the chip manuals.


> > +#ifdef CONFIG_SPL_BUILD
> > +static void copy_to_reg(u32 *dest, const u32 *src, u32 n)
> 
> Seems like this should go in a common file as there are several users
> - rk_copy_to_reg() ?

Right now we don't have a common file with shared code and as that function is 
specific to the SPL build I'm not sure if it wouldn't cause issues if we have a 
big bunch of common code that gets included in all SPLs.

Looking at my compilation result, it looks like the compiler inlined the 
function anyway, so maybe just define it as inline in the hardware.h ?


> 
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < n / sizeof(u32); i++) {
> > +               writel(*src, dest);
> > +               src++;
> > +               dest++;
> > +       }
> > +}

[...]

> > +static void phy_dll_bypass_set(struct rk3288_ddr_publ *publ,
> > +       u32 freq)
> > +{
> > +       int i;
> 
> blank line here
> 
> Do you have a comment for this logic, and why it is how it is?

Not really ... the memory controllers of the rk3066,rk3188 are very much 
similar to the rk3288, so this simply mirrors your own sdram code from rk3288 
;-) .

Speaking of duplicate code, you might already realize that this sdram driver 
is very much similar to the rk3288.
My current plan is to submit the rk3188 separately at first and after 
everything lands try to make the common code shared.

rk3066, rk3188 and rk3288 all use a dw-upctl + dw-publ + phy, with some 
minimal differences, while the rk3036 uses a dw-upctl with a different phy 
block, so it might get a bit tricky on where to share and how to name stuff.

Also another developer seems to be working on rk3066 uboot support already, so 
it might be interesting to see if this will also require additional 
adaptations.


> > +       if (freq <= 250000000) {
> > +               if (freq <= 150000000)
> > +                       clrbits_le32(&publ->dllgcr, SBIAS_BYPASS);
> > +               else
> > +                       setbits_le32(&publ->dllgcr, SBIAS_BYPASS);
> > +               setbits_le32(&publ->acdllcr, ACDLLCR_DLLDIS);
> > +               for (i = 0; i < 4; i++)
> > +                       setbits_le32(&publ->datx8[i].dxdllcr,
> > +                                    DXDLLCR_DLLDIS);
> > +
> > +               setbits_le32(&publ->pir, PIR_DLLBYP);
> > +       } else {
> > +               clrbits_le32(&publ->dllgcr, SBIAS_BYPASS);
> > +               clrbits_le32(&publ->acdllcr, ACDLLCR_DLLDIS);
> > +               for (i = 0; i < 4; i++) {
> > +                       clrbits_le32(&publ->datx8[i].dxdllcr,
> > +                                    DXDLLCR_DLLDIS);
> > +               }
> > +
> > +               clrbits_le32(&publ->pir, PIR_DLLBYP);
> > +       }
> > +}

[...]

> > +static void ddr_set_ddr3_mode(struct rk3188_grf *grf, uint channel,
> > +                             bool ddr3_mode)
> > +{
> > +       uint mask, val;
> > +
> > +       mask = 1 << MSCH4_MAINDDR3_SHIFT;
> > +       val = ddr3_mode << MSCH4_MAINDDR3_SHIFT;
> > +       rk_clrsetreg(&grf->soc_con2, mask, val);
> > +}
> > +
> > +#define RANK_2_ROW15_EN 1
> 
> Can this go at the top of the file?

actually already contained in the grf header, so can be simply used from there


> > +static void ddr_rank_2_row15en(struct rk3188_grf *grf, bool enable)
> > +{
> > +       uint mask, val;
> > +
> > +       mask = 1 << RANK_2_ROW15_EN;
> > +       val = enable << RANK_2_ROW15_EN;
> > +       rk_clrsetreg(&grf->soc_con2, mask, val);
> > +}




> > +static int data_training(const struct chan_info *chan, u32 channel,
> > +                        struct rk3188_sdram_params *sdram_params)
> 
> Function comment? What does it return? channel could just be int, right?

I'm not particular insighted on what this code actually does, except what the 
function name implies ;-) ... but I did swap the channel to int.


> 
> > +{
> > +       unsigned int j;
> > +       int ret = 0;
> > +       u32 rank;
> > +       int i;
> > +       u32 step[2] = { PIR_QSTRN, PIR_RVTRN };
> > +       struct rk3288_ddr_publ *publ = chan->publ;
> > +       struct rk3288_ddr_pctl *pctl = chan->pctl;
> > +
> > +       /* disable auto refresh */
> > +       writel(0, &pctl->trefi);
> > +
> > +       if (sdram_params->base.dramtype != LPDDR3)
> > +               setbits_le32(&publ->pgcr, 1 << PGCR_DQSCFG_SHIFT);
> > +       rank = sdram_params->ch[channel].rank | 1;
> > +       for (j = 0; j < ARRAY_SIZE(step); j++) {
> > +               /*
> > +                * trigger QSTRN and RVTRN
> > +                * clear DTDONE status
> > +                */
> > +               setbits_le32(&publ->pir, PIR_CLRSR);
> > +
> > +               /* trigger DTT */
> > +               setbits_le32(&publ->pir,
> > +                            PIR_INIT | step[j] | PIR_LOCKBYP |
> > PIR_ZCALBYP | +                            PIR_CLRSR);
> > +               udelay(1);
> > +               /* wait echo byte DTDONE */
> > +               while ((readl(&publ->datx8[0].dxgsr[0]) & rank)
> > +                       != rank)
> > +                       ;
> > +               while ((readl(&publ->datx8[1].dxgsr[0]) & rank)
> > +                       != rank)
> > +                       ;
> > +               if (!(readl(&pctl->ppcfg) & 1)) {
> > +                       while ((readl(&publ->datx8[2].dxgsr[0])
> > +                               & rank) != rank)
> > +                               ;
> > +                       while ((readl(&publ->datx8[3].dxgsr[0])
> > +                               & rank) != rank)
> > +                               ;
> > +               }
> > +               if (readl(&publ->pgsr) &
> > +                   (PGSR_DTERR | PGSR_RVERR | PGSR_RVEIRR)) {
> > +                       ret = -1;
> > +                       break;
> > +               }
> > +       }
> > +       /* send some auto refresh to complement the lost while DTT */
> > +       for (i = 0; i < (rank > 1 ? 8 : 4); i++)
> > +               send_command(pctl, rank, REF_CMD, 0);
> > +
> > +       if (sdram_params->base.dramtype != LPDDR3)
> > +               clrbits_le32(&publ->pgcr, 1 << PGCR_DQSCFG_SHIFT);
> > +
> > +       /* resume auto refresh */
> > +       writel(sdram_params->pctl_timing.trefi, &pctl->trefi);
> > +
> > +       return ret;
> > +}
> > +



> > +const int ddrconf_table[] = {
> > +       /*
> > +        * [5:4] row(13+n)
> > +        * [1:0] col(9+n), assume bw=2
> > +        * row      col,bw */
> > +       0,
> > +       ((2 << 4) | 1),
> > +       ((1 << 4) | 1),
> > +       ((0 << 4) | 1),
> > +       ((2 << 4) | 2),
> > +       ((1 << 4) | 2),
> > +       ((0 << 4) | 2),
> > +       ((1 << 4) | 0),
> > +       ((0 << 4) | 0),
> > +       0,
> > +       0,
> > +       0,
> > +       0,
> > +       0,
> > +       0,
> > +       0,
> > +};
> 
> Can this go at the top of the file? What are the <<4 for ? Can we have
> a #define?

Having that shift as define might get difficult. As you can see this is the value 
that gets written into the ddrconf register of the memory scheduler and the 
contents of this registers aren't documented in any TRM (rk3288, rk3188, 
rk3066).

So I'd like to refrain from introducting possible wrong defines right now :-) .


Heiko

Hi Heiko,

On 17 February 2017 at 13:39, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Simon,
>
> Am Montag, 6. Februar 2017, 07:35:23 CET schrieb Simon Glass:
>> On 3 February 2017 at 08:09, Heiko Stuebner <heiko@sntech.de> wrote:
>> > The sdram controller blocks are very similar to the rk3288 in utilizing
>> > memory scheduler, Designware uPCTL and Designware PUBL blocks, only
>> > limited to one bank instead of two.
>> >
>> > There are some minimal differences when setting up the ram, so it gets
>> > a separate driver for the rk3188 but reuses the driver structs, as there
>> > is no need to define the same again.
>> >
>> > More optimization can happen when the modelling of the controller parts
>> > in the dts actually follow the hardware layout hopefully at some point
>> > in the future.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  arch/arm/include/asm/arch-rockchip/ddr_rk3188.h |  22 +
>> >  arch/arm/mach-rockchip/rk3188/Makefile          |   1 +
>> >  arch/arm/mach-rockchip/rk3188/sdram_rk3188.c    | 985
>> >  ++++++++++++++++++++++++ 3 files changed, 1008 insertions(+)
>> >  create mode 100644 arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
>> >  create mode 100644 arch/arm/mach-rockchip/rk3188/sdram_rk3188.c
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Nits below.
>>
>> > diff --git a/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
>> > b/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h new file mode 100644
>> > index 0000000000..993c58d1a8
>> > --- /dev/null
>> > +++ b/arch/arm/include/asm/arch-rockchip/ddr_rk3188.h
>> > @@ -0,0 +1,22 @@
>> > +/*
>> > + * (C) Copyright 2015 Google, Inc
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0
>> > + */
>> > +
>> > +#ifndef _ASM_ARCH_DDR_RK3188_H
>> > +#define _ASM_ARCH_DDR_RK3188_H
>> > +
>> > +#include <asm/arch/ddr_rk3288.h>
>> > +
>> > +struct rk3188_msch {
>> > +       u32 coreid;
>> > +       u32 revisionid;
>> > +       u32 ddrconf;
>> > +       u32 ddrtiming;
>> > +       u32 ddrmode;
>> > +       u32 readlatency;
>>
>> Can you comment this struct? What is it for?
>
> Added a comment that this the memory scheduler register map, but that is all I
> can say for it, as it is sparsely documented in the chip manuals.
>
>
>> > +#ifdef CONFIG_SPL_BUILD
>> > +static void copy_to_reg(u32 *dest, const u32 *src, u32 n)
>>
>> Seems like this should go in a common file as there are several users
>> - rk_copy_to_reg() ?
>
> Right now we don't have a common file with shared code and as that function is
> specific to the SPL build I'm not sure if it wouldn't cause issues if we have a
> big bunch of common code that gets included in all SPLs.
>
> Looking at my compilation result, it looks like the compiler inlined the
> function anyway, so maybe just define it as inline in the hardware.h ?

I'd rather not do that - how about cleaning it up in a separate patch later.

>
>
>>
>> > +{
>> > +       int i;
>> > +
>> > +       for (i = 0; i < n / sizeof(u32); i++) {
>> > +               writel(*src, dest);
>> > +               src++;
>> > +               dest++;
>> > +       }
>> > +}
>
> [...]
>
>> > +static void phy_dll_bypass_set(struct rk3288_ddr_publ *publ,
>> > +       u32 freq)
>> > +{
>> > +       int i;
>>
>> blank line here
>>
>> Do you have a comment for this logic, and why it is how it is?
>
> Not really ... the memory controllers of the rk3066,rk3188 are very much
> similar to the rk3288, so this simply mirrors your own sdram code from rk3288
> ;-) .
>
> Speaking of duplicate code, you might already realize that this sdram driver
> is very much similar to the rk3288.
> My current plan is to submit the rk3188 separately at first and after
> everything lands try to make the common code shared.
>
> rk3066, rk3188 and rk3288 all use a dw-upctl + dw-publ + phy, with some
> minimal differences, while the rk3036 uses a dw-upctl with a different phy
> block, so it might get a bit tricky on where to share and how to name stuff.
>
> Also another developer seems to be working on rk3066 uboot support already, so
> it might be interesting to see if this will also require additional
> adaptations.

OK

>
>
>> > +       if (freq <= 250000000) {
>> > +               if (freq <= 150000000)
>> > +                       clrbits_le32(&publ->dllgcr, SBIAS_BYPASS);
>> > +               else
>> > +                       setbits_le32(&publ->dllgcr, SBIAS_BYPASS);
>> > +               setbits_le32(&publ->acdllcr, ACDLLCR_DLLDIS);
>> > +               for (i = 0; i < 4; i++)
>> > +                       setbits_le32(&publ->datx8[i].dxdllcr,
>> > +                                    DXDLLCR_DLLDIS);
>> > +
>> > +               setbits_le32(&publ->pir, PIR_DLLBYP);
>> > +       } else {
>> > +               clrbits_le32(&publ->dllgcr, SBIAS_BYPASS);
>> > +               clrbits_le32(&publ->acdllcr, ACDLLCR_DLLDIS);
>> > +               for (i = 0; i < 4; i++) {
>> > +                       clrbits_le32(&publ->datx8[i].dxdllcr,
>> > +                                    DXDLLCR_DLLDIS);
>> > +               }
>> > +
>> > +               clrbits_le32(&publ->pir, PIR_DLLBYP);
>> > +       }
>> > +}
>
> [...]
>
>> > +static void ddr_set_ddr3_mode(struct rk3188_grf *grf, uint channel,
>> > +                             bool ddr3_mode)
>> > +{
>> > +       uint mask, val;
>> > +
>> > +       mask = 1 << MSCH4_MAINDDR3_SHIFT;
>> > +       val = ddr3_mode << MSCH4_MAINDDR3_SHIFT;
>> > +       rk_clrsetreg(&grf->soc_con2, mask, val);
>> > +}
>> > +
>> > +#define RANK_2_ROW15_EN 1
>>
>> Can this go at the top of the file?
>
> actually already contained in the grf header, so can be simply used from there
>
>
>> > +static void ddr_rank_2_row15en(struct rk3188_grf *grf, bool enable)
>> > +{
>> > +       uint mask, val;
>> > +
>> > +       mask = 1 << RANK_2_ROW15_EN;
>> > +       val = enable << RANK_2_ROW15_EN;
>> > +       rk_clrsetreg(&grf->soc_con2, mask, val);
>> > +}
>
>
>
>
>> > +static int data_training(const struct chan_info *chan, u32 channel,
>> > +                        struct rk3188_sdram_params *sdram_params)
>>
>> Function comment? What does it return? channel could just be int, right?
>
> I'm not particular insighted on what this code actually does, except what the
> function name implies ;-) ... but I did swap the channel to int.
>
>
>>
>> > +{
>> > +       unsigned int j;
>> > +       int ret = 0;
>> > +       u32 rank;
>> > +       int i;
>> > +       u32 step[2] = { PIR_QSTRN, PIR_RVTRN };
>> > +       struct rk3288_ddr_publ *publ = chan->publ;
>> > +       struct rk3288_ddr_pctl *pctl = chan->pctl;
>> > +
>> > +       /* disable auto refresh */
>> > +       writel(0, &pctl->trefi);
>> > +
>> > +       if (sdram_params->base.dramtype != LPDDR3)
>> > +               setbits_le32(&publ->pgcr, 1 << PGCR_DQSCFG_SHIFT);
>> > +       rank = sdram_params->ch[channel].rank | 1;
>> > +       for (j = 0; j < ARRAY_SIZE(step); j++) {
>> > +               /*
>> > +                * trigger QSTRN and RVTRN
>> > +                * clear DTDONE status
>> > +                */
>> > +               setbits_le32(&publ->pir, PIR_CLRSR);
>> > +
>> > +               /* trigger DTT */
>> > +               setbits_le32(&publ->pir,
>> > +                            PIR_INIT | step[j] | PIR_LOCKBYP |
>> > PIR_ZCALBYP | +                            PIR_CLRSR);
>> > +               udelay(1);
>> > +               /* wait echo byte DTDONE */
>> > +               while ((readl(&publ->datx8[0].dxgsr[0]) & rank)
>> > +                       != rank)
>> > +                       ;
>> > +               while ((readl(&publ->datx8[1].dxgsr[0]) & rank)
>> > +                       != rank)
>> > +                       ;
>> > +               if (!(readl(&pctl->ppcfg) & 1)) {
>> > +                       while ((readl(&publ->datx8[2].dxgsr[0])
>> > +                               & rank) != rank)
>> > +                               ;
>> > +                       while ((readl(&publ->datx8[3].dxgsr[0])
>> > +                               & rank) != rank)
>> > +                               ;
>> > +               }
>> > +               if (readl(&publ->pgsr) &
>> > +                   (PGSR_DTERR | PGSR_RVERR | PGSR_RVEIRR)) {
>> > +                       ret = -1;
>> > +                       break;
>> > +               }
>> > +       }
>> > +       /* send some auto refresh to complement the lost while DTT */
>> > +       for (i = 0; i < (rank > 1 ? 8 : 4); i++)
>> > +               send_command(pctl, rank, REF_CMD, 0);
>> > +
>> > +       if (sdram_params->base.dramtype != LPDDR3)
>> > +               clrbits_le32(&publ->pgcr, 1 << PGCR_DQSCFG_SHIFT);
>> > +
>> > +       /* resume auto refresh */
>> > +       writel(sdram_params->pctl_timing.trefi, &pctl->trefi);
>> > +
>> > +       return ret;
>> > +}
>> > +
>
>
>
>> > +const int ddrconf_table[] = {
>> > +       /*
>> > +        * [5:4] row(13+n)
>> > +        * [1:0] col(9+n), assume bw=2
>> > +        * row      col,bw */
>> > +       0,
>> > +       ((2 << 4) | 1),
>> > +       ((1 << 4) | 1),
>> > +       ((0 << 4) | 1),
>> > +       ((2 << 4) | 2),
>> > +       ((1 << 4) | 2),
>> > +       ((0 << 4) | 2),
>> > +       ((1 << 4) | 0),
>> > +       ((0 << 4) | 0),
>> > +       0,
>> > +       0,
>> > +       0,
>> > +       0,
>> > +       0,
>> > +       0,
>> > +       0,
>> > +};
>>
>> Can this go at the top of the file? What are the <<4 for ? Can we have
>> a #define?
>
> Having that shift as define might get difficult. As you can see this is the value
> that gets written into the ddrconf register of the memory scheduler and the
> contents of this registers aren't documented in any TRM (rk3288, rk3188,
> rk3066).
>
> So I'd like to refrain from introducting possible wrong defines right now :-) .

OK. The memory init code is a pain point for many SoCs, let's just do
our best and hopefully docs and meaning can improve with time.

Regards,
Simon