[PATCH] zynqmp: spl: support DRAM ECC initialization
Michal Simek
monstr at monstr.eu
Fri Jun 11 13:13:02 CEST 2021
Hi,
On 6/11/21 12:09 PM, Jorge Ramirez-Ortiz wrote:
> Use the ZDMA channel 0 to initialize the DRAM banks.
A little bit short. Can you please extend it?
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge at foundries.io>
> ---
> arch/arm/mach-zynqmp/Kconfig | 35 +++++++
> arch/arm/mach-zynqmp/Makefile | 1 +
> arch/arm/mach-zynqmp/ecc_spl_init.c | 139 ++++++++++++++++++++++++++++
> arch/arm/mach-zynqmp/spl.c | 8 ++
> 4 files changed, 183 insertions(+)
> create mode 100644 arch/arm/mach-zynqmp/ecc_spl_init.c
>
> diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig
> index f1301f6661..79197a351f 100644
> --- a/arch/arm/mach-zynqmp/Kconfig
> +++ b/arch/arm/mach-zynqmp/Kconfig
> @@ -92,6 +92,41 @@ config ZYNQMP_NO_DDR
> This option configures MMU with no DDR to avoid speculative
> access to DDR memory where DDR is not present.
>
> +config SPL_ZYNQMP_DRAM_ECC_INIT
> + bool "Initialize DRAM ECC"
> + depends on SPL
> + help
> + This option initializes all memory to 0xdeadbeef. Must be set if your
> + memory is of ECC type.
> +
> +config SPL_ZYNQMP_DRAM_BANK1
_BASE suffix
> + depends on SPL_ZYNQMP_DRAM_ECC_INIT
> + hex "DRAM Bank1 address"
> + default 0x00000000
> + help
> + Start address of DRAM ECC bank1
> +
> +config SPL_ZYNQMP_DRAM_BANK1_LEN
> + depends on SPL_ZYNQMP_DRAM_ECC_INIT
> + hex "DRAM Bank1 size"
> + default 0x80000000
> + help
> + Size in bytes of the DRAM ECC bank1
> +
> +config SPL_ZYNQMP_DRAM_BANK2
> + depends on SPL_ZYNQMP_DRAM_ECC_INIT
> + hex "DRAM Bank2 address"
> + default 0x0
Here default can be also setup which is unchanged.
IIRC it is 0x8_0000_0000;
> + help
> + Start address of DRAM ECC bank2
> +
> +config SPL_ZYNQMP_DRAM_BANK2_LEN
> + depends on SPL_ZYNQMP_DRAM_ECC_INIT
> + hex "DRAM Bank2 size"
> + default 0x0
> + help
> + Size in bytes of the DRAM ECC bank2. A null size takes no action.
> +
> config SYS_MALLOC_F_LEN
> default 0x600
>
> diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
> index 8a3b074724..eb6c5112b3 100644
> --- a/arch/arm/mach-zynqmp/Makefile
> +++ b/arch/arm/mach-zynqmp/Makefile
> @@ -7,4 +7,5 @@ obj-y += clk.o
> obj-y += cpu.o
> obj-$(CONFIG_MP) += mp.o
> obj-$(CONFIG_SPL_BUILD) += spl.o handoff.o
> +obj-$(CONFIG_SPL_ZYNQMP_DRAM_ECC_INIT) += ecc_spl_init.o
> obj-$(CONFIG_ZYNQMP_PSU_INIT_ENABLED) += psu_spl_init.o
> diff --git a/arch/arm/mach-zynqmp/ecc_spl_init.c b/arch/arm/mach-zynqmp/ecc_spl_init.c
> new file mode 100644
> index 0000000000..d8d93883c2
> --- /dev/null
> +++ b/arch/arm/mach-zynqmp/ecc_spl_init.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2015 - 2020 Xilinx, Inc.
> + *
> + * Jorge Ramirez-Ortiz <jorge at foundries.io>
> + */
> +
> +#include <common.h>
> +#include <cpu_func.h>
> +#include <image.h>
> +#include <init.h>
> +#include <spl.h>
> +
> +#include <asm/io.h>
> +#include <asm/spl.h>
Already included by spl.h.
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/sys_proto.h>
are you using something from this file?
> +
> +#define ADMA_CH0_BASEADDR 0xFFA80000U
This address should be in hardware.h
> +#define ZDMA_TRANSFER_MAX_LEN (0x3FFFFFFFU - 7U)
> +#define ZDMA_CH_STATUS ((ADMA_CH0_BASEADDR) + 0x0000011CU)
> +#define ZDMA_CH_STATUS_STATE_MASK 0x00000003U
> +#define ZDMA_CH_STATUS_STATE_DONE 0x00000000U
> +#define ZDMA_CH_STATUS_STATE_ERR 0x00000003U
> +#define ZDMA_CH_CTRL0 ((ADMA_CH0_BASEADDR) + 0x00000110U)
> +#define ZDMA_CH_CTRL0_POINT_TYPE_MASK (u32)0x00000040U
> +#define ZDMA_CH_CTRL0_POINT_TYPE_NORMAL (u32)0x00000000U
> +#define ZDMA_CH_CTRL0_MODE_MASK (u32)0x00000030U
> +#define ZDMA_CH_CTRL0_MODE_WR_ONLY (u32)0x00000010U
> +#define ZDMA_CH_CTRL0_TOTAL_BYTE_COUNT ((ADMA_CH0_BASEADDR) + 0x00000188U)
> +#define ZDMA_CH_WR_ONLY_WORD0 ((ADMA_CH0_BASEADDR) + 0x00000148U)
> +#define ZDMA_CH_WR_ONLY_WORD1 ((ADMA_CH0_BASEADDR) + 0x0000014CU)
> +#define ZDMA_CH_WR_ONLY_WORD2 ((ADMA_CH0_BASEADDR) + 0x00000150U)
> +#define ZDMA_CH_WR_ONLY_WORD3 ((ADMA_CH0_BASEADDR) + 0x00000154U)
> +#define ZDMA_CH_DST_DSCR_WORD0 ((ADMA_CH0_BASEADDR) + 0x00000138U)
> +#define ZDMA_CH_DST_DSCR_WORD0_LSB_MASK 0xFFFFFFFFU
> +#define ZDMA_CH_DST_DSCR_WORD1 ((ADMA_CH0_BASEADDR) + 0x0000013CU)
> +#define ZDMA_CH_DST_DSCR_WORD1_MSB_MASK 0x0001FFFFU
> +#define ZDMA_CH_SRC_DSCR_WORD2 ((ADMA_CH0_BASEADDR) + 0x00000130U)
> +#define ZDMA_CH_DST_DSCR_WORD2 ((ADMA_CH0_BASEADDR) + 0x00000140U)
> +#define ZDMA_CH_CTRL2 ((ADMA_CH0_BASEADDR) + 0x00000200U)
> +#define ZDMA_CH_CTRL2_EN_MASK 0x00000001U
> +#define ZDMA_CH_ISR ((ADMA_CH0_BASEADDR) + 0x00000100U)
> +#define ZDMA_CH_ISR_DMA_DONE_MASK 0x00000400U
> +#define ECC_INIT_VAL_WORD 0xDEADBEEFU
> +
> +static void ecc_dram_bank_init(u64 addr, u64 len)
> +{
> + u64 bytes = len;
> + u64 src = addr;
> + u32 size;
> + u32 reg;
> +
> + flush_dcache_all();
> + dcache_disable();
cpu at this stage should be after reset. Not sure without closer look if
caches are on but you need to do write before using this memory that's
why all Dcache lines shouldn't have any data for DDR that's why I think
you don't need to do any handling with cache at this stage.
And OCM access is fast where dcache shouldn't be even used for this region.
> +
> + while (bytes > 0) {
> + size = bytes > ZDMA_TRANSFER_MAX_LEN ?
> + ZDMA_TRANSFER_MAX_LEN : (u32)bytes;
> +
> + /* Wait until the DMA is in idle state */
> + do {
> + reg = readl(ZDMA_CH_STATUS);
> + reg &= ZDMA_CH_STATUS_STATE_MASK;
> + } while ((reg != ZDMA_CH_STATUS_STATE_DONE) &&
> + (reg != ZDMA_CH_STATUS_STATE_ERR));
Normally this should loop with timeout. Is time working at this stage
already?
> +
> + /* Enable Simple (Write Only) Mode */
> + reg = readl(ZDMA_CH_CTRL0);
> + reg &= (ZDMA_CH_CTRL0_POINT_TYPE_MASK |
> + ZDMA_CH_CTRL0_MODE_MASK);
> + reg |= (ZDMA_CH_CTRL0_POINT_TYPE_NORMAL |
> + ZDMA_CH_CTRL0_MODE_WR_ONLY);
> + writel(reg, ZDMA_CH_CTRL0);
> +
> + /* Fill in the data to be written */
> + writel(ECC_INIT_VAL_WORD, ZDMA_CH_WR_ONLY_WORD0);
> + writel(ECC_INIT_VAL_WORD, ZDMA_CH_WR_ONLY_WORD1);
> + writel(ECC_INIT_VAL_WORD, ZDMA_CH_WR_ONLY_WORD2);
> + writel(ECC_INIT_VAL_WORD, ZDMA_CH_WR_ONLY_WORD3);
> +
> + /* Write Destination Address */
> + writel((u32)(src & ZDMA_CH_DST_DSCR_WORD0_LSB_MASK),
> + ZDMA_CH_DST_DSCR_WORD0);
> + writel((u32)((src >> 32) & ZDMA_CH_DST_DSCR_WORD1_MSB_MASK),
> + ZDMA_CH_DST_DSCR_WORD1);
> +
> + /* Size to be Transferred. Recommended to set both src and dest sizes */
> + writel(size, ZDMA_CH_SRC_DSCR_WORD2);
> + writel(size, ZDMA_CH_DST_DSCR_WORD2);
> +
> + /* DMA Enable */
> + reg = readl(ZDMA_CH_CTRL2);
> + reg |= ZDMA_CH_CTRL2_EN_MASK;
> + writel(reg, ZDMA_CH_CTRL2);
> +
> + /* Check the status of the transfer by polling on DMA Done */
> + do {
> + reg = readl(ZDMA_CH_ISR);
> + reg &= ZDMA_CH_ISR_DMA_DONE_MASK;
> + } while (reg != ZDMA_CH_ISR_DMA_DONE_MASK);
Same as above. Timeout could be able to handle at this stage.
> +
> + /* Clear DMA status */
> + reg = readl(ZDMA_CH_ISR);
> + reg |= ZDMA_CH_ISR_DMA_DONE_MASK;
> + writel(ZDMA_CH_ISR_DMA_DONE_MASK, ZDMA_CH_ISR);
> +
> + /* Read the channel status for errors */
> + reg = readl(ZDMA_CH_STATUS);
> + if (reg == ZDMA_CH_STATUS_STATE_ERR)
> + break;
This sounds like a bug. It means any message should be shown if that
happens. And you shouldn't continue in execution.
Or I can imagine that you will reset IP and try to init it again.
> +
> + bytes -= size;
> + src += size;
> + }
> +
> + dcache_enable();
The same here.
> +
> + /* Restore reset values for the DMA registers used */
> + writel(ZDMA_CH_CTRL0, 0x00000080U);
> + writel(ZDMA_CH_WR_ONLY_WORD0, 0x00000000U);
> + writel(ZDMA_CH_WR_ONLY_WORD1, 0x00000000U);
> + writel(ZDMA_CH_WR_ONLY_WORD2, 0x00000000U);
> + writel(ZDMA_CH_WR_ONLY_WORD3, 0x00000000U);
> + writel(ZDMA_CH_DST_DSCR_WORD0, 0x00000000U);
> + writel(ZDMA_CH_DST_DSCR_WORD1, 0x00000000U);
> + writel(ZDMA_CH_SRC_DSCR_WORD2, 0x00000000U);
> + writel(ZDMA_CH_DST_DSCR_WORD2, 0x00000000U);
> + writel(ZDMA_CH_CTRL0_TOTAL_BYTE_COUNT, 0x00000000U);
Isn't it easier to reset this IP to get to that state?
Anyway I would move this to separate function and call it before you do
transfers. And next thing is if 1/2GB cases you not calling this code
for second region when len is 0 which doesn't look right.
> +}
> +
> +void zynqmp_ecc_init(void)
> +{
> + ecc_dram_bank_init(CONFIG_SPL_ZYNQMP_DRAM_BANK1,
> + CONFIG_SPL_ZYNQMP_DRAM_BANK1_LEN);
> +
> + ecc_dram_bank_init(CONFIG_SPL_ZYNQMP_DRAM_BANK2,
> + CONFIG_SPL_ZYNQMP_DRAM_BANK2_LEN);
> +}
> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
> index 88386b23e5..f85e68b9c2 100644
> --- a/arch/arm/mach-zynqmp/spl.c
> +++ b/arch/arm/mach-zynqmp/spl.c
> @@ -18,10 +18,18 @@
> #include <asm/arch/psu_init_gpl.h>
> #include <asm/arch/sys_proto.h>
>
> +#ifdef CONFIG_SPL_ZYNQMP_DRAM_ECC_INIT
> +extern void zynqmp_ecc_init(void);
> +#endif
Create MIT header for this function to avoid extern and include it in
this file.
> +
> void board_init_f(ulong dummy)
> {
> board_early_init_f();
> board_early_init_r();
> + board_early_init_r();
Looks weird that you call it twice.
> +#ifdef CONFIG_SPL_ZYNQMP_DRAM_ECC_INIT
> + zynqmp_ecc_init();
> +#endif
> }
>
> static void ps_mode_reset(ulong mode)
>
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
More information about the U-Boot
mailing list