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

[Heiko Stuebner <heiko@sntech.de>]

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