Hi Heiko, On 17 February 2017 at 13:39, Heiko Stuebner <he...@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 <he...@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 <he...@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 <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot