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 <jo...@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 <jo...@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
>
