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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 05205C77B72 for ; Mon, 17 Apr 2023 19:45:47 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5CE1F85FFB; Mon, 17 Apr 2023 21:45:45 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org 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=linaro.org header.i=@linaro.org header.b="UZoYmx4G"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 3C4C986003; Mon, 17 Apr 2023 21:45:43 +0200 (CEST) Received: from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com [IPv6:2607:f8b0:4864:20::f2c]) (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 51D9985FE8 for ; Mon, 17 Apr 2023 21:45:40 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ralph.siemsen@linaro.org Received: by mail-qv1-xf2c.google.com with SMTP id h14so15228161qvr.7 for ; Mon, 17 Apr 2023 12:45:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1681760739; x=1684352739; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=y/CMPvrixZVXg83I1MKMh4dcfJjYZixBGhSidZUKcRM=; b=UZoYmx4GWArYIsewpo8pXBYYV1qXYYbGWB7iPAfP1Qtd7Sr4+CvOpjFAY+o0o8ZjaJ PPTCb/V5+A+HGluCfW92Cpae5pxRScpXiiAKKIYbYAUU7eZHq+31T15JuXnlDfWsS7CQ SSwWxQX+LCJrY0rrYEpFvakekxcD2I10FE5Ys2vpeX5wkTIqHqHitJq8aTF6qMm+Onfp prE/Xzvyype70QSorZPsBra7FVc1NHCUnur9IaSNLkN9Sxbbs0sI2oXV/vn+NHtij6fs CKRDujz/PIPFd1hegEE0W3Iqj2/tH0wH7qSJxPLEXqTyO1GRKQfzV7+PGwPXkk9mADx8 hrvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681760739; x=1684352739; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=y/CMPvrixZVXg83I1MKMh4dcfJjYZixBGhSidZUKcRM=; b=Bj9OXDDQW6otOlUO/p6ifUXl/nY1whRCn/IOtQGU0uOs1U3/v6bqBzw52oRuM2sDk+ L9cCjQKLmFy3JXR415h5HmsaWvwgH9iCQwRAK1rl0Xvlr8V60jAsx76BDvEOsiSjh+wE SOHtszB6mwiHvHLjiFCQ/HKogGvcs9fnEAYdFCb1c82lMAaigDEpcK/nMfQSUIoiR6Xc HpFcEExpumrLNDn8l/Zp0CrsYMUrJl9g3Nx5S3UTu8sopkcA1hZ74BKcQgOORBr0EwMG kCv06M7oiKo3q5tnekD00tRP7EpewieW5kky/NS2d8NTqRV/EojFiNuXTcbBUA7kntXs eiVw== X-Gm-Message-State: AAQBX9feLUyGOwdKrJhfNu+d9Jduj2zXJ8RtAkWX0VVpV86rm9LiXcBZ k33IAj5WHu7qWbEcO+agY17VmA== X-Google-Smtp-Source: AKy350bYSX4XeiI7Qt5wwMfeUaMVZ7RImB2ZubbW/ystmTA+XbmbvAtXoCFTLRmCg1XAod27uuLz2Q== X-Received: by 2002:ad4:596d:0:b0:5a4:548e:4ed6 with SMTP id eq13-20020ad4596d000000b005a4548e4ed6mr14218211qvb.40.1681760738662; Mon, 17 Apr 2023 12:45:38 -0700 (PDT) Received: from localhost (rfs.netwinder.org. [206.248.184.2]) by smtp.gmail.com with ESMTPSA id ee1-20020a0562140a4100b005ed90bf69efsm3258394qvb.114.2023.04.17.12.45.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Apr 2023 12:45:38 -0700 (PDT) Date: Mon, 17 Apr 2023 15:45:37 -0400 From: Ralph Siemsen To: Marek Vasut Cc: u-boot@lists.denx.de, Chris Packham Subject: Re: [PATCH v4 08/10] board: schneider: add RZN1 board support Message-ID: <20230417194537.GF642444@maple.netwinder.org> References: <20230308202653.1926303-1-ralph.siemsen@linaro.org> <20230308202653.1926303-9-ralph.siemsen@linaro.org> <3a8dc96b-c3e4-b0e2-24d4-c9b4c35dda92@mailbox.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <3a8dc96b-c3e4-b0e2-24d4-c9b4c35dda92@mailbox.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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.8 at phobos.denx.de X-Virus-Status: Clean On Mon, Apr 17, 2023 at 07:18:46PM +0200, Marek Vasut wrote: >On 3/8/23 21:26, Ralph Siemsen wrote: >>diff --git a/board/schneider/rzn1-snarc/ddr_timing.c >>b/board/schneider/rzn1-snarc/ddr_timing.c >>new file mode 100644 >>index 0000000000..8bc3fe7be4 >>--- /dev/null >>+++ b/board/schneider/rzn1-snarc/ddr_timing.c >>@@ -0,0 +1,140 @@ >>+// SPDX-License-Identifier: GPL-2.0 >>+ >>+#include >>+ >>+#include "jedec_ddr3_2g_x16_1333h_500_cl8.h" >>+ >>+u32 ddr_00_87_async[] = { > >Should this be 'static u32...' ? It should, but there is bit of kludge going on here. This table is used when starting the DDR controller, in drivers/ram/cadence/ddr_async.c There are several other boards in u-boot already which appear to use the same (or a similar) DDR controller from Cadence. Some of these also use a similar kludge as I have done. Others instead put the DDR parameters into the device tree. While using the devicetree is cleaner, it does increase the size of the DTB by quite a bit, which creates some additional challenges. Even more so when you need to support multiple DDR chips, each with their own configuration parameters. So I opted for the low-tech approach, at least in this initial version. >>+#define DENALI_CTL_00_DATA 0x00000600 > >You might want to run checkpatch --f --fix --fix-inplace on this to >fix formatting , esp. use one space after #define and tab after the >macro name. Note that diff -wdb will let you diff updates to this file >while ignoring space changes. > >>+#define DENALI_CTL_01_DATA 0x00000000 >>+#define DENALI_CTL_02_DATA 0x00000000 >>+#define DENALI_CTL_03_DATA 0x00000000 >>+#define DENALI_CTL_04_DATA 0x00000000 I had also considered eliminating the header file entirely, and just putting the values directly into the array in the .c file. It is not like the names of the #defines are particularly illuminating. However, this runs into friction each time a new DDR appears, along with the new set of parameters (autogenerated by some vendor tool). In fact I've had to add some new DDR devices quite recently (but that didn't make it into the current patches). I'm happy to reformat this to match upstream preference. But I would also be happy to discuss how we can better handle this type of "large binary blob" of configuration data, to make it simpler for everyone. >>+++ b/board/schneider/rzn1-snarc/rzn1.c >>@@ -0,0 +1,39 @@ >>+// SPDX-License-Identifier: GPL-2.0+ >>+ >>+#include >>+#include >>+#include >>+ >>+DECLARE_GLOBAL_DATA_PTR; >>+ >>+int board_init(void) >>+{ >>+ gd->bd->bi_boot_params = CFG_SYS_SDRAM_BASE + 0x100; >>+ >>+ return 0; >>+} >>+ >>+int dram_init(void) >>+{ >>+ struct udevice *dev; >>+ int err; >>+ >>+ /* This will end up calling cadence_ddr_probe() */ >>+ err = uclass_get_device(UCLASS_RAM, 0, &dev); >>+ if (err) { >>+ debug("DRAM init failed: %d\n", err); >>+ return err; >>+ } >>+ >>+ if (fdtdec_setup_mem_size_base() != 0) >>+ return -EINVAL; >>+ >>+ return 0; >>+} >>+ >>+int dram_init_banksize(void) >>+{ >>+ fdtdec_setup_memory_banksize(); >>+ >>+ return 0; >>+} > >I wonder how much of this should really be in arch/... since this is >common to all machines with RZN1 . Maybe move it there instead ? I can certainly do that. When I first put this together, I looked at many other dram_init() functions, both in arch/... and board/... Ralph