[U-Boot] [PATCH v3 5/6] arm: atmel: add ddr2 initialization function

[Bo Shen <voice.shen@atmel.com>]

Hi Andreas,

On 11/13/2013 09:03 PM, Andreas Bie?mann wrote:
> Hi Bo,
>
> On 11/06/2013 06:29 AM, Bo Shen wrote:
>> The MPDDRC supports different type of SDRAM
>> This patch add ddr2 initialization function
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>>
>> ---
>> Changes in v3:
>>    - Move to at91 common folder
>>
>> Changes in v2:
>>    - None
>>
>>   arch/arm/cpu/at91-common/Makefile             |   32 +++++++
>>   arch/arm/cpu/at91-common/mpddrc.c             |  123 +++++++++++++++++++++++++
>>   arch/arm/include/asm/arch-at91/atmel_mpddrc.h |  111 ++++++++++++++++++++++
>>   spl/Makefile                                  |    4 +
>>   4 files changed, 270 insertions(+)
>>   create mode 100644 arch/arm/cpu/at91-common/Makefile
>>   create mode 100644 arch/arm/cpu/at91-common/mpddrc.c
>>   create mode 100644 arch/arm/include/asm/arch-at91/atmel_mpddrc.h
>>
>> diff --git a/arch/arm/cpu/at91-common/Makefile b/arch/arm/cpu/at91-common/Makefile
>> new file mode 100644
>> index 0000000..6f1a9e6
>> --- /dev/null
>> +++ b/arch/arm/cpu/at91-common/Makefile
>> @@ -0,0 +1,32 @@
>> +#
>> +# (C) Copyright 2000-2008
>> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>> +#
>> +# (C) Copyright 2013 Atmel Corporation
>> +#		     Bo Shen <voice.shen@atmel.com>
>> +#
>> +# SPDX-License-Identifier:	GPL-2.0+
>> +#
>> +
>> +include $(TOPDIR)/config.mk
>> +
>> +LIB	= $(obj)libat91-common.o
>> +
>> +COBJS-$(CONFIG_SPL_BUILD) += mpddrc.o
>> +
>> +SRCS    := $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
>> +OBJS    := $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
>> +
>> +all:	$(obj).depend $(LIB)
>> +
>> +$(LIB):	$(OBJS)
>> +	$(call cmd_link_o_target, $(OBJS))
>> +
>> +#########################################################################
>> +
>> +# defines $(obj).depend target
>> +include $(SRCTREE)/rules.mk
>> +
>> +sinclude $(obj).depend
>> +
>> +#########################################################################
>
> please rewrite in KBuild style.

OK, I will use KBuild style in next version.

>> diff --git a/arch/arm/cpu/at91-common/mpddrc.c b/arch/arm/cpu/at91-common/mpddrc.c
>> new file mode 100644
>> index 0000000..1abde1e
>> --- /dev/null
>> +++ b/arch/arm/cpu/at91-common/mpddrc.c
>> @@ -0,0 +1,123 @@
>> +/*
>> + * Copyright (C) 2013 Atmel Corporation
>> + *		      Bo Shen <voice.shen@atmel.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/atmel_mpddrc.h>
>> +
>> +static void atmel_mpddr_op(int mode, u32 ram_address)
>
> static inline, could give the compiler a hint to optimize here.

I am not sure whether the write(0, ram_address) will be optimized. 
Because this must excute later than write mode to let it work.

>> +{
>> +	struct atmel_mpddr *mpddr = (struct atmel_mpddr *)ATMEL_BASE_MPDDRC;
>> +
>> +	writel(mode, &mpddr->mr);
>> +	writel(0, ram_address);
>> +}
>> +
>> +int ddr2_init(u32 ram_address, struct atmel_mpddr *mpddr_value)
>
> could you please constify mpddr_value for the very same reason?

OK.

>> +{
>> +	struct atmel_mpddr *mpddr = (struct atmel_mpddr *)ATMEL_BASE_MPDDRC;
>> +	u32 ba_off, cr;
>> +
>> +	/* Compute bank offset according to NC in configuration register */
>> +	ba_off = (mpddr_value->cr & ATMEL_MPDDRC_CR_NC_MASK) + 9;
>> +	if (!(mpddr_value->cr & ATMEL_MPDDRC_CR_DECOD_INTERLEAVED))
>> +		ba_off += ((mpddr->cr & ATMEL_MPDDRC_CR_NR_MASK) >> 2) + 11;
>> +
>> +	ba_off += (mpddr_value->mdr & ATMEL_MPDDRC_MDR_DBW_MASK) ? 1 : 2;
>> +
>> +	/* Program the memory device type into the memory device register */
>> +	writel(mpddr_value->mdr, &mpddr->mdr);
>> +
>> +	/* Program the configuration register */
>> +	writel(mpddr_value->cr, &mpddr->cr);
>> +
>> +	/* Program the timing register */
>> +	writel(mpddr_value->tp0r, &mpddr->tp0r);
>> +	writel(mpddr_value->tp1r, &mpddr->tp1r);
>> +	writel(mpddr_value->tp2r, &mpddr->tp2r);
>> +
>> +	/* Issue a NOP command */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_NOP_CMD, ram_address);
>> +
>> +	/* A 200 us is provided to precede any signal toggle */
>> +	udelay(200);
>> +
>> +	/* Issue a NOP command */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_NOP_CMD, ram_address);
>> +
>> +	/* Issue an all banks precharge command */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_PRCGALL_CMD, ram_address);
>> +
>> +	/* Issue an extended mode register set(EMRS2) to choose operation */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_EXT_LMR_CMD,
>> +		       ram_address + (0x2 << ba_off));
>> +
>> +	/* Issue an extended mode register set(EMRS3) to set EMSR to 0 */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_EXT_LMR_CMD,
>> +		       ram_address + (0x3 << ba_off));
>> +
>> +	/*
>> +	 * Issue an extended mode register set(EMRS1) to enable DLL and
>> +	 * program D.I.C (output driver impedance control)
>> +	 */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_EXT_LMR_CMD,
>> +		       ram_address + (0x1 << ba_off));
>> +
>> +	/* Enable DLL reset */
>> +	cr = readl(&mpddr->cr);
>> +	writel(cr | ATMEL_MPDDRC_CR_EN_RESET_DLL, &mpddr->cr);
>> +
>> +	/* A mode register set(MRS) cycle is issued to reset DLL */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_LMR_CMD, ram_address);
>> +
>> +	/* Issue an all banks precharge command */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_PRCGALL_CMD, ram_address);
>> +
>> +	/* Two auto-refresh (CBR) cycles are provided */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_RFSH_CMD, ram_address);
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_RFSH_CMD, ram_address);
>> +
>> +	/* Disable DLL reset */
>> +	cr = readl(&mpddr->cr);
>> +	writel(cr & (~ATMEL_MPDDRC_CR_EN_RESET_DLL), &mpddr->cr);
>> +
>> +	/* A mode register set (MRS) cycle is issued to disable DLL reset */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_LMR_CMD, ram_address);
>> +
>> +	/* Set OCD calibration in defautl state */
>
> typo default

thanks.

>> +	cr = readl(&mpddr->cr);
>> +	writel(cr | ATMEL_MPDDRC_CR_OCD_DEFAULT, &mpddr->cr);
>> +
>> +	/*
>> +	 * An extended mode register set (EMRS1) cycle is issued
>> +	 * to OCD default value
>> +	 */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_EXT_LMR_CMD,
>> +		       ram_address + (0x1 << ba_off));
>> +
>> +	 /* OCD calibration mode exit */
>> +	cr = readl(&mpddr->cr);
>> +	writel(cr & (~ATMEL_MPDDRC_CR_OCD_DEFAULT), &mpddr->cr);
>> +
>> +	/*
>> +	 * An extended mode register set (EMRS1) cycle is issued
>> +	 * to enable OCD exit
>> +	 */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_EXT_LMR_CMD,
>> +		       ram_address + (0x1 << ba_off));
>> +
>> +	/* A nornal mode command is provided */
>> +	atmel_mpddr_op(ATMEL_MPDDRC_MR_NORMAL_CMD, ram_address);
>> +
>> +	/* Perform a write access to any DDR2-SDRAM address */
>> +	writel(0, ram_address);
>> +
>> +	/* Write the refresh rate */
>> +	writel(mpddr_value->rtr, &mpddr->rtr);
>> +
>
> I haven't checked that sequence deeply, I trust in you that it is correct ;)
>
>> +	return 0;
>> +}
>> diff --git a/arch/arm/include/asm/arch-at91/atmel_mpddrc.h b/arch/arm/include/asm/arch-at91/atmel_mpddrc.h
>> new file mode 100644
>> index 0000000..abd8b68
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-at91/atmel_mpddrc.h
>> @@ -0,0 +1,111 @@
>> +/*
>> + * Copyright (C) 2013 Atmel Corporation
>> + *		      Bo Shen <voice.shen@atmel.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#ifndef __ATMEL_MPDDRC_H__
>> +#define __ATMEL_MPDDRC_H__
>> +
>> +struct atmel_mpddr {
>> +	u32 mr;
>> +	u32 rtr;
>> +	u32 cr;
>> +	u32 tp0r;
>
> Datasheet names them tprX

Actually, this name is Timing Parameter 0 Register,
Timing Parameter 1 Register.

>> +	u32 tp1r;
>> +	u32 tp2r;
>> +	u32 reserved[2];
>> +	u32 mdr;
>
> Datasheet names it just md.

All other registers with "r" suffix, so, add r to this register name too.

> I think it would be good to also add the other register positions or at
> least mention that these the only one needed currently (in a comment
> right here in the struct).

OK, I will add comments here.

>> +};
>> +
>> +int ddr2_init(unsigned int ram_address,
>> +	       struct atmel_mpddr *mpddr);
>> +
>> +/* bit field in mode register */
>> +#define ATMEL_MPDDRC_MR_NORMAL_CMD		0x0
>> +#define ATMEL_MPDDRC_MR_NOP_CMD			0x1
>> +#define ATMEL_MPDDRC_MR_PRCGALL_CMD		0x2
>> +#define ATMEL_MPDDRC_MR_LMR_CMD			0x3
>> +#define ATMEL_MPDDRC_MR_RFSH_CMD		0x4
>> +#define	ATMEL_MPDDRC_MR_EXT_LMR_CMD		0x5
>> +#define	ATMEL_MPDDRC_MR_DEEP_CMD		0x6
>> +#define	ATMEL_MPDDRC_MR_LPDDR2_CMD		0x7
>
> Could you please drop the tabs between 'define' and the definition.

OK

>> +
>> +/* bit field in configuration register */
>> +#define	ATMEL_MPDDRC_CR_NC_MASK			0x3
>> +#define	ATMEL_MPDDRC_CR_NC_COL_9		0x0
>> +#define	ATMEL_MPDDRC_CR_NC_COL_10		0x1
>> +#define	ATMEL_MPDDRC_CR_NC_COL_11		0x2
>> +#define	ATMEL_MPDDRC_CR_NC_COL_12		0x3
>> +#define	ATMEL_MPDDRC_CR_NR_MASK			(0x3 << 2)
>> +#define	ATMEL_MPDDRC_CR_NR_ROW_11		(0x0 << 2)
>> +#define	ATMEL_MPDDRC_CR_NR_ROW_12		(0x1 << 2)
>> +#define	ATMEL_MPDDRC_CR_NR_ROW_13		(0x2 << 2)
>> +#define	ATMEL_MPDDRC_CR_NR_ROW_14		(0x3 << 2)
>> +#define ATMEL_MPDDRC_CR_CAS			(0x7 << 4)
>> +#define	ATMEL_MPDDRC_CR_CAS_2			(0x2 << 4)
>> +#define	ATMEL_MPDDRC_CR_CAS_3			(0x3 << 4)
>> +#define	ATMEL_MPDDRC_CR_CAS_4			(0x4 << 4)
>> +#define	ATMEL_MPDDRC_CR_CAS_5			(0x5 << 4)
>> +#define	ATMEL_MPDDRC_CR_CAS_6			(0x6 << 4)
>> +#define ATMEL_MPDDRC_CR_EN_RESET_DLL		(0x1 << 7)
>> +#define ATMEL_MPDDRC_CR_DIC_DS			(0x1 << 8)
>> +#define ATMEL_MPDDRC_CR_DIS_DLL			(0x1 << 9)
>> +#define ATMEL_MPDDRC_CR_OCD_DEFAULT		(0x7 << 12)
>> +#define ATMEL_MPDDRC_CR_EN_ENRDM		(0x1 << 17)
>> +#define ATMEL_MPDDRC_CR_NB_8BANKS		(0x1 << 20)
>> +#define ATMEL_MPDDRC_CR_DIS_NDQS		(0x1 << 21)
>> +#define ATMEL_MPDDRC_CR_DECOD_INTERLEAVED	(0x1 << 22)
>> +#define ATMEL_MPDDRC_CR_UNAL_SUPPORTED		(0x1 << 23)
>
> Some of the defines have the strict scheme <offset><register name><field
> name> with <offset> being ATMEL_MPDDRC, <register name> CR here and the
> respective field names from datasheet. Some however have some more
> information like UNAL_SUPPORTED or DECOD_INTERLEAVED (those two are Ok I
> think, but we could discuss if we like to have the strict scheme or
> leave some space). But EN_ENRDM is definitely too much ;)
> Has anyone a opinion about the strict scheme?
>
> Regarding EN_ENRDM I think using ENRDM_ON would be better

Agree.

>
>> +
>> +/* bit field in timing parameter 0 register */
>> +#define ATMEL_MPDDRC_TP0R_TRAS_OFFSET		0
>> +#define ATMEL_MPDDRC_TP0R_TRAS_MASK		0xf
>> +#define ATMEL_MPDDRC_TP0R_TRCD_OFFSET		4
>> +#define ATMEL_MPDDRC_TP0R_TRCD_MASK		0xf
>> +#define ATMEL_MPDDRC_TP0R_TWR_OFFSET		8
>> +#define ATMEL_MPDDRC_TP0R_TWR_MASK		0xf
>> +#define ATMEL_MPDDRC_TP0R_TRC_OFFSET		12
>> +#define ATMEL_MPDDRC_TP0R_TRC_MASK		0xf
>> +#define ATMEL_MPDDRC_TP0R_TRP_OFFSET		16
>> +#define ATMEL_MPDDRC_TP0R_TRP_MASK		0xf
>> +#define ATMEL_MPDDRC_TP0R_TRRD_OFFSET		20
>> +#define ATMEL_MPDDRC_TP0R_TRRD_MASK		0xf
>> +#define ATMEL_MPDDRC_TP0R_TWTR_OFFSET		24
>> +#define ATMEL_MPDDRC_TP0R_TWTR_MASK		0x7
>> +#define ATMEL_MPDDRC_TP0R_RDC_WRRD_OFFSET	27
>> +#define ATMEL_MPDDRC_TP0R_RDC_WRRD_MASK		0x1
>> +#define ATMEL_MPDDRC_TP0R_TMRD_OFFSET		28
>> +#define ATMEL_MPDDRC_TP0R_TMRD_MASK		0xf
>> +
>> +/* bit field in timing parameter 1 register */
>> +#define ATMEL_MPDDRC_TP1R_TRFC_OFFSET		0
>> +#define ATMEL_MPDDRC_TP1R_TRFC_MASK		0x7f
>> +#define ATMEL_MPDDRC_TP1R_TXSNR_OFFSET		8
>> +#define ATMEL_MPDDRC_TP1R_TXSNR_MASK		0xff
>> +#define ATMEL_MPDDRC_TP1R_TXSRD_OFFSET		16
>> +#define ATMEL_MPDDRC_TP1R_TXSRD_MASK		0xff
>> +#define ATMEL_MPDDRC_TP1R_TXP_OFFSET		24
>> +#define ATMEL_MPDDRC_TP1R_TXP_MASK		0xf
>> +
>> +/* bit field in timing parameter 2 register */
>> +#define ATMEL_MPDDRC_TP2R_TXARD_OFFSET		0
>> +#define ATMEL_MPDDRC_TP2R_TXARD_MASK		0xf
>> +#define ATMEL_MPDDRC_TP2R_TXARDS_OFFSET		4
>> +#define ATMEL_MPDDRC_TP2R_TXARDS_MASK		0xf
>> +#define ATMEL_MPDDRC_TP2R_TRPA_OFFSET		8
>> +#define ATMEL_MPDDRC_TP2R_TRPA_MASK		0xf
>> +#define ATMEL_MPDDRC_TP2R_TRTP_OFFSET		12
>> +#define ATMEL_MPDDRC_TP2R_TRTP_MASK		0x7
>> +#define ATMEL_MPDDRC_TP2R_TFAW_OFFSET		16
>> +#define ATMEL_MPDDRC_TP2R_TFAW_MASK		0xf
>> +
>> +/* bit field in Memory Device Register */
>> +#define ATMEL_MPDDRC_MDR_LPDDR_SDRAM	0x3
>> +#define ATMEL_MPDDRC_MDR_DDR2_SDRAM	0x6
>> +#define ATMEL_MPDDRC_MDR_DBW_MASK	(0x1 << 4)
>> +#define ATMEL_MPDDRC_MDR_DBW_32BITS	(0x0 << 4)
>> +#define ATMEL_MPDDRC_MDR_DBW_16BITS	(0x1 << 4)
>> +
>> +#endif
>> diff --git a/spl/Makefile b/spl/Makefile
>> index b366ac2..6dd1e5d 100644
>> --- a/spl/Makefile
>> +++ b/spl/Makefile
>> @@ -123,6 +123,10 @@ ifeq ($(SOC),exynos)
>>   LIBS-y += $(CPUDIR)/s5p-common/libs5p-common.o
>>   endif
>>
>> +ifeq ($(SOC),at91)
>> +LIBS-y += arch/$(ARCH)/cpu/at91-common/libat91-common.o
>> +endif
>
> The libat91-common.o should be built in any case. The mpddrc.o should
> only be included for SPL build (that mentioned Heiko in another mail).

OK.

>> +
>>   # Add GCC lib
>>   ifeq ("$(USE_PRIVATE_LIBGCC)", "yes")
>>   PLATFORM_LIBGCC = $(SPLTREE)/arch/$(ARCH)/lib/libgcc.o
>>
>
> Best regards
>
> Andreas Bie?mann
>

Best Regards,
Bo Shen