>>>>> "Stephen" == Stephen Warren <swar...@wwwdotorg.org> writes:
Stephen> On 08/22/2016 04:29 PM, Peter Chubb wrote: >> >> This patch adds support for the TK1-SOM board, which is almost the Stephen> Nit: That blank line at start of the commit description Stephen> should be removed. OK. >> same as the Jetson TK1. Board info at >> https://tk1som.com/products/tk1-som Stephen> tk1-som sounds like a rather generic name. I'm sure there are Stephen> many boards that could be called that. Can we add the company Stephen> name to the U-Boot board name? They seem to abbreviate Stephen> themselves to CEI on their website, so cei-tk1-som seems like Stephen> a reasonable name. OK, will do. >> 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. Stephen> Having a separate top-level board identity seems reasonable Stephen> if the HW is different. Judging by a diff between the files Stephen> in this patch and the existing Jetson TK1 support, there are Stephen> some differences, so I think this is all fine. >> board/nvidia/tk1-som/Kconfig | 12 + Stephen> This isn't an NVIDIA board. I think that should be Stephen> board/cei/tk1-som/Kconfig. Similar comment for most of the Stephen> other files. OK. >> 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" Stephen> I'd say "Colorado Engineering Inc. TK1-SOM" OK. >> 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". + */ Stephen> Did you generate this yourself, or extract it from some CEI Stephen> U-Boot source tree? I ask because the file is obviously based Stephen> on an old version of the pinmux spreadsheet and pinmux Stephen> scripts (e.g. the unsupported OWR pin is configured, there's Stephen> no tk1_som_mipipadctrlgrps variable, etc. If you generated Stephen> this yourself, if you could update it based on the latest Stephen> spreadsheet that'd be great. If you're just taking this from Stephen> some CEI U-Boot source tree, then the commit description Stephen> should mention this, and you likely need to preserve the Stephen> original CEI authorship and Signed-off-by lines. I generated this myself from a spreadsheet provided by Colorado Engineering and the current tegra-pinmux-scripts. Is there any easy way to see the differences in versions between the one this was based on (v09 of the upstream NVIDIA sheet) and the current v11 one to be able to transform the spreadsheet? >> diff --git a/board/nvidia/tk1-som/tk1-som.c >> b/board/nvidia/tk1-som/tk1-som.c Stephen> Perhaps you could add a comment describing what that PMIC Stephen> GPIO is used for, and hence why it's necessary to do this. Stephen> Actually, if you look at the history of jetson-tk1.c, you'll Stephen> find commit 7fb82986be62 "ARM: tegra: rm Jetson TK1 PMIC GPIO Stephen> programming" which implies that such PMIC GPIO programming Stephen> should not be performed. It was in the upstream Colorado-supplied U-Boot; I'll test without it. >> + + + return 0; Stephen> Nit: Two blank lines? OK. >> 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 Stephen> Hmm. Considering we have 3 boards taking the "if" path and I Stephen> think just one taking the "else" path, I think it would be Stephen> better to do the following instead: Stephen> #if defined(CONFIG_TARGET_VENICE2) // existing else value Stephen> #else // existing if value #endif OK. The alternative is to have a third symbol to test on, a feature-test macro rather than a board macro. >> +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 Stephen> None of those are in jetson-tk1_defconfig. Was this addition Stephen> deliberate? Yes, except the USE_PRIVATE_LIBGCC which I think is an error. The main use-case I have for the board is for virtualisation; enabling booting in nonsec mode is essential for this. And it's harmless, as the feature has to be enabled via an environment variable. Peter C -- Dr Peter Chubb Tel: +61 2 9490 5852 http://www.data61.csiro.au http://ts.data61.csiro.au Software Systems Research Group/NICTA/Data61 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot