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
