From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C6873C48BE0 for ; Fri, 11 Jun 2021 16:09:50 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6988F611CD for ; Fri, 11 Jun 2021 16:09:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6988F611CD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=foundries.io Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4A56C801E8; Fri, 11 Jun 2021 18:09:47 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=foundries.io Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=foundries.io header.i=@foundries.io header.b="SJsgyTqn"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4FAFD803B9; Fri, 11 Jun 2021 18:09:45 +0200 (CEST) Received: from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com [IPv6:2a00:1450:4864:20::32d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BF47D801E8 for ; Fri, 11 Jun 2021 18:09:41 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=foundries.io Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=jorge@foundries.io Received: by mail-wm1-x32d.google.com with SMTP id s70-20020a1ca9490000b02901a589651424so6626769wme.0 for ; Fri, 11 Jun 2021 09:09:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foundries.io; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=U+w4BRLwN2G3s3dFGsNyzRJUJ1VRm98b/iv1Iy1XXwQ=; b=SJsgyTqnib2ZFrXOQgWC7v2p9Q3bpPz9IV1ygxk3C6k9F+m2a/jbcWGTxYmqE16Rvk JJkzj5ptHIU+c276lGoUdEGm69oyD8/UaYEzlGMEF7jQH/Gje0brsR72gCGAXHGudhxg JgpIsmsxYVEhHwAjmLjPqOpAkc6/YqFsDqw41dT5KZFzQaTgttIEnaLsfSNIxfLAwKln sh+YcLV+ZehoDJV1vw57PyLkDysL0alnDvyYs9+ms56SjASk3jEA+uHXYCBgK+3J788y fSTaDgT+MFz42tF4Gk3W0iXmdCUiOTt7vI0s1Z39mXp2fZ9KWpTs2k1Z4TF8GOFBIHj/ 1C7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=U+w4BRLwN2G3s3dFGsNyzRJUJ1VRm98b/iv1Iy1XXwQ=; b=BSY4Omb50SHOYxlOOIpZNcCyqZxlagumnhCIb+xY9wtPLY0emtWPuICyxy8659N8Jj m/yQdAJ2a8zyqAHayySAsqM/ktpPWy7NyKnkd3IdyesBAt52bTwhaBxkCoJGl/DnidGn wqvcn7hR1dy/YWrB2/gbtjwLv8jU6JylOdf8s+9aBqG/gVRSuZCigUuY77YxvFrtZmTJ HOrOpxbY4YBU7hBQwL0pKsRdQteDV3Du51ysGmNf7WJ5ahDkGBsbbIWJT9hJGvtdkLrP Oc2c4AbtUaZC9E2bnDKOXe60BGT9qYgL6CDpbBFyPdppoMVDKCC5PG9ChbpFV9ho5SHN SUEw== X-Gm-Message-State: AOAM531iqqD7BhTeGN7VZYguWElxZWO0xw7n2+jPuP6VV4sg/fBVeNL5 MyY1CduDP3wRF+ceAxzDDnZDww== X-Google-Smtp-Source: ABdhPJyp1HtODaTm0N3hGbAMCjnMWxtdaX0BJXLPPa5c5Yq0LWMtjEdMsV74s02fPLA5ApWb1C1iUA== X-Received: by 2002:a05:600c:2109:: with SMTP id u9mr21335960wml.7.1623427781237; Fri, 11 Jun 2021 09:09:41 -0700 (PDT) Received: from trex (138.red-79-146-80.dynamicip.rima-tde.net. [79.146.80.138]) by smtp.gmail.com with ESMTPSA id 4sm6999489wry.74.2021.06.11.09.09.40 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Fri, 11 Jun 2021 09:09:40 -0700 (PDT) From: "Jorge Ramirez-Ortiz, Foundries" X-Google-Original-From: "Jorge Ramirez-Ortiz, Foundries" Date: Fri, 11 Jun 2021 18:09:39 +0200 To: Michal Simek Cc: Jorge Ramirez-Ortiz , ricardo@foundries.io, mike@foundries.io, u-boot@lists.denx.de Subject: Re: [PATCH] zynqmp: spl: support DRAM ECC initialization Message-ID: <20210611160939.GA5195@trex> References: <20210611100936.12474-1-jorge@foundries.io> <998eab6b-e12e-ffd0-41f9-79ca5c06000c@monstr.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <998eab6b-e12e-ffd0-41f9-79ca5c06000c@monstr.eu> User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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 > > --- > > 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 > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > Already included by spl.h. > > > +#include > > +#include > > 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 > > #include > > > > +#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 >