From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Tue, 23 Aug 2016 12:29:25 -0600 Subject: [U-Boot] [ARM] RFC: Add board support for Colorado Engineering TK1-SOM In-Reply-To: <84h9acwi7d.wl-Peter.Chubb@data61.csiro.au> References: <84h9acwi7d.wl-Peter.Chubb@data61.csiro.au> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 08/22/2016 04:29 PM, Peter Chubb wrote: > > This patch adds support for the TK1-SOM board, which is almost the Nit: That blank line at start of the commit description should be removed. > same as the Jetson TK1. Board info at > https://tk1som.com/products/tk1-som tk1-som sounds like a rather generic name. I'm sure there are many boards that could be called that. Can we add the company name to the U-Boot board name? They seem to abbreviate themselves to CEI on their website, so cei-tk1-som seems like a reasonable name. > Query: Is this the best way to support this board? Or is there an > easy way to merge the necessary changes into the Jetson-TK1 board? > The main differences are in the pinmux and device tree. And the > device tree is maybe 10 lines different. Having a separate top-level board identity seems reasonable if the HW is different. Judging by a diff between the files in this patch and the existing Jetson TK1 support, there are some differences, so I think this is all fine. > board/nvidia/tk1-som/Kconfig | 12 + This isn't an NVIDIA board. I think that should be board/cei/tk1-som/Kconfig. Similar comment for most of the other files. > diff --git a/arch/arm/mach-tegra/tegra124/Kconfig b/arch/arm/mach-tegra/tegra124/Kconfig > +config TARGET_TK1_SOM > + bool "Colorado/NVIDIA Tegra124 TK1-som board" I'd say "Colorado Engineering Inc. TK1-SOM" > diff --git a/board/nvidia/tk1-som/pinmux-config-tk1-som.h b/board/nvidia/tk1-som/pinmux-config-tk1-som.h > +/* > + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved. > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +/* > + * THIS FILE IS AUTO-GENERATED - DO NOT EDIT! > + * > + * To generate this file, use the tegra-pinmux-scripts tool available from > + * https://github.com/NVIDIA/tegra-pinmux-scripts > + * Run "board-to-uboot.py tk1-som". > + */ Did you generate this yourself, or extract it from some CEI U-Boot source tree? I ask because the file is obviously based on an old version of the pinmux spreadsheet and pinmux scripts (e.g. the unsupported OWR pin is configured, there's no tk1_som_mipipadctrlgrps variable, etc. If you generated this yourself, if you could update it based on the latest spreadsheet that'd be great. If you're just taking this from some CEI U-Boot source tree, then the commit description should mention this, and you likely need to preserve the original CEI authorship and Signed-off-by lines. > diff --git a/board/nvidia/tk1-som/tk1-som.c b/board/nvidia/tk1-som/tk1-som.c > +int tegra_pcie_board_init(void) > + err = as3722_gpio_configure(pmic, 1, AS3722_GPIO_OUTPUT_VDDH | > + AS3722_GPIO_INVERT); > + if (err < 0) { > + error("failed to configure GPIO#1 as output: %d\n", err); > + return err; > + } > + > + err = as3722_gpio_direction_output(pmic, 2, 1); > + if (err < 0) { > + error("failed to set GPIO#2 high: %d\n", err); > + return err; > + } Perhaps you could add a comment describing what that PMIC GPIO is used for, and hence why it's necessary to do this. Actually, if you look at the history of jetson-tk1.c, you'll find commit 7fb82986be62 "ARM: tegra: rm Jetson TK1 PMIC GPIO programming" which implies that such PMIC GPIO programming should not be performed. > + > + > + return 0; Nit: Two blank lines? > diff --git a/board/nvidia/venice2/as3722_init.h b/board/nvidia/venice2/as3722_init.h > -#if defined(CONFIG_TARGET_JETSON_TK1) || defined(CONFIG_TARGET_NYAN_BIG) > +#if defined(CONFIG_TARGET_JETSON_TK1) || defined(CONFIG_TARGET_NYAN_BIG) || defined(CONFIG_TARGET_TK1_SOM) > #define AS3722_SD0VOLTAGE_DATA (0x3C00 | AS3722_SD0VOLTAGE_REG) > #else > #define AS3722_SD0VOLTAGE_DATA (0x2800 | AS3722_SD0VOLTAGE_REG) > #endif Hmm. Considering we have 3 boards taking the "if" path and I think just one taking the "else" path, I think it would be better to do the following instead: #if defined(CONFIG_TARGET_VENICE2) // existing else value #else // existing if value #endif ... since Venice2 is an odd-ball board that likely few people have, and almost any new board we support will be derived from Jetson TK1 and hence also take the "if" path in the existing code. > diff --git a/configs/tk1-som_defconfig b/configs/tk1-som_defconfig > +CONFIG_USE_PRIVATE_LIBGCC=y > +CONFIG_CPU_V7_HAS_NONSEC=y > +CONFIG_CPU_V7_HAS_VIRT=y > +CONFIG_ARMV7_NONSEC=y > +CONFIG_ARMV7_VIRT=y > +CONFIG_SUPPORT_SPL=y > +CONFIG_SPL=y > +CONFIG_SPL_BUILD=y > +CONFIG_ARMV7_PSCI=y None of those are in jetson-tk1_defconfig. Was this addition deliberate?