* [PATCH] zynqmp: spl: support DRAM ECC initialization
@ 2021-06-11 10:09 Jorge Ramirez-Ortiz
2021-06-11 11:13 ` Michal Simek
0 siblings, 1 reply; 3+ messages in thread
From: Jorge Ramirez-Ortiz @ 2021-06-11 10:09 UTC (permalink / raw)
To: jorge, monstr; +Cc: ricardo, mike, u-boot
Use the ZDMA channel 0 to initialize the DRAM banks.
Signed-off-by: Jorge Ramirez-Ortiz <jorge@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
+ 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
+ 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@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>
+#include <asm/arch/hardware.h>
+#include <asm/arch/sys_proto.h>
+
+#define ADMA_CH0_BASEADDR 0xFFA80000U
+#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();
+
+ 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));
+
+ /* 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);
+
+ /* 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;
+
+ bytes -= size;
+ src += size;
+ }
+
+ dcache_enable();
+
+ /* 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);
+}
+
+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
+
void board_init_f(ulong dummy)
{
board_early_init_f();
board_early_init_r();
+ board_early_init_r();
+#ifdef CONFIG_SPL_ZYNQMP_DRAM_ECC_INIT
+ zynqmp_ecc_init();
+#endif
}
static void ps_mode_reset(ulong mode)
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] zynqmp: spl: support DRAM ECC initialization
2021-06-11 10:09 [PATCH] zynqmp: spl: support DRAM ECC initialization Jorge Ramirez-Ortiz
@ 2021-06-11 11:13 ` Michal Simek
2021-06-11 16:09 ` Jorge Ramirez-Ortiz, Foundries
0 siblings, 1 reply; 3+ messages in thread
From: Michal Simek @ 2021-06-11 11:13 UTC (permalink / raw)
To: Jorge Ramirez-Ortiz; +Cc: ricardo, mike, u-boot
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@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@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] zynqmp: spl: support DRAM ECC initialization
2021-06-11 11:13 ` Michal Simek
@ 2021-06-11 16:09 ` Jorge Ramirez-Ortiz, Foundries
0 siblings, 0 replies; 3+ messages in thread
From: Jorge Ramirez-Ortiz, Foundries @ 2021-06-11 16:09 UTC (permalink / raw)
To: Michal Simek; +Cc: Jorge Ramirez-Ortiz, ricardo, mike, u-boot
On 11/06/21, Michal Simek wrote:
> 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?
ok
>
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@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
ok
>
> > + 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;
yes, that is the address that I am using on my end.
should I also default the length of 2G or should I leave the length at
0? not sure what is the typical configuration on these boards.
>
>
> > + 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@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?
nope
>
> > +
> > +#define ADMA_CH0_BASEADDR 0xFFA80000U
>
> This address should be in hardware.h
ah, ok. I wasnt sure if we wanted it all contained in this single
file. will change.
>
> > +#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.
completly gree - I just took the FSBL implementation as it was.
will remove.
> 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?
I'll check but does it really matter? the system will not do much without DDR.
>
> > +
> > + /* 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.
ok, I'll put a message out.
I pretty much followed the template of what was done in
board_early_init_f(): in the case where psu_init() fails to
initialize: ie, no error message, no retries.
do you think we should reset the IP and retry? if so, how many times?
>
> > +
> > + 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?
probably - didnt give it much thought TBH. will try that.
> Anyway I would move this to separate function and call it before you do
> transfers.
ok
> 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.
ok
>
> > +
> > 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.
um, sorry, my bad!
thanks a lot for the detailed review
>
> > +#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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-11 16:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11 10:09 [PATCH] zynqmp: spl: support DRAM ECC initialization Jorge Ramirez-Ortiz
2021-06-11 11:13 ` Michal Simek
2021-06-11 16:09 ` Jorge Ramirez-Ortiz, Foundries
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.