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

[Stephen Warren]

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?