[U-Boot] [ARM] RFC: Add board support for Colorado Engineering TK1-SOM

[Peter Chubb <Peter.Chubb@data61.csiro.au>]

>>>>> "Stephen" == Stephen Warren <swarren@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