[U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform

[Philipp Tomsich <philipp.tomsich@theobroma-systems.com>]

On Tue, 27 Mar 2018, Kever Yang wrote:

> We use common board/spl/tpl file for all rockchip SoCs,
> - all the SoC spec setting should move into SoC file like rk3288.c;
> - tpl is option and only purpose to init DRAM, clock, uart(option);
> - spl do secure relate one time init, boot device select, boot into
>  U-Boot or trust or OS in falcon mode;
> - board do boot mode detect, enable regulator, usb init and so on.

There's too much going on in a single commit/single series.
This needs to be split up into multiple, independent steps (e.g. one for 
the timer changes, another one for the UART changes)...

>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>

See below for requested changes (beyond splitting this up).
Reviewing this in this state is a real chore, so I'll probably have more 
comments, once I see this presented in more manageable parcels.

> ---
>
> arch/arm/mach-rockchip/Makefile |  23 +----
> arch/arm/mach-rockchip/board.c  | 136 ++++++++++++++++++++++++++++
> arch/arm/mach-rockchip/spl.c    | 195 ++++++++++++++++++++++++++++++++++++++++
> arch/arm/mach-rockchip/tpl.c    | 111 +++++++++++++++++++++++
> 4 files changed, 445 insertions(+), 20 deletions(-)
> create mode 100644 arch/arm/mach-rockchip/board.c
> create mode 100644 arch/arm/mach-rockchip/spl.c
> create mode 100644 arch/arm/mach-rockchip/tpl.c
>
> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
> index e1b0519..3aba66c 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -11,15 +11,8 @@
> obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o
> obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o
>
> -obj-tpl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-tpl.o
> -obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o
> -
> -obj-spl-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board-spl.o
> -obj-spl-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board-spl.o
> -obj-spl-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board-spl.o
> -obj-spl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-spl.o
> -obj-spl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-spl.o spl-boot-order.o
> -obj-spl-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board-spl.o spl-boot-order.o
> +obj-tpl-y += tpl.o
> +obj-spl-y += spl.o spl-boot-order.o
>
> ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),)
>
> @@ -28,21 +21,11 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),)
> # we can have the preprocessor correctly recognise both 0x0 and 0
> # meaning "turn it off".
> obj-y += boot_mode.o
> -
> -obj-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board.o
> -obj-$(CONFIG_ROCKCHIP_RK3128) += rk3128-board.o
> -obj-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board.o
> -obj-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board.o
> -obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board.o
> -obj-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board.o
> +obj-y += board.o
> endif
>
> obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram_common.o
>
> -ifndef CONFIG_ARM64
> -obj-y += rk_timer.o
> -endif

This would need to have gone with the rk_timer.c removal.
Otherwise things don't build between the earlier patch and here.

> -
> obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036/
> obj-$(CONFIG_ROCKCHIP_RK3128) += rk3128/
> ifndef CONFIG_TPL_BUILD
> diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c
> new file mode 100644
> index 0000000..52c6f66
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/board.c
> @@ -0,0 +1,136 @@
> +/*
> + * (C) Copyright 2017 Rockchip Electronics Co., Ltd.
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <debug_uart.h>
> +#include <ram.h>
> +#include <syscon.h>
> +#include <asm/io.h>
> +#include <asm/gpio.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/periph.h>
> +#include <asm/arch/boot_mode.h>
> +#ifdef CONFIG_DM_REGULATOR
> +#include <power/regulator.h>
> +#endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#if defined(CONFIG_USB_FUNCTION_FASTBOOT)
> +int fb_set_reboot_flag(void)
> +{
> +	printf("Setting reboot to fastboot flag ...\n");
> +	/* Set boot mode to fastboot */
> +	writel(BOOT_FASTBOOT, CONFIG_ROCKCHIP_BOOT_MODE_REG);
> +
> +	return 0;
> +}
> +
> +#define FASTBOOT_KEY_GPIO 43 /* GPIO1_B3 */
> +static int fastboot_key_pressed(void)
> +{
> +	gpio_request(FASTBOOT_KEY_GPIO, "fastboot_key");
> +	gpio_direction_input(FASTBOOT_KEY_GPIO);
> +	return !gpio_get_value(FASTBOOT_KEY_GPIO);
> +}
> +#endif
> +
> +__weak int rk_board_init(void)
> +{
> +	return 0;
> +}
> +
> +__weak int rk_board_late_init(void)
> +{
> +	return 0;
> +}
> +
> +int board_late_init(void)
> +{
> +#if defined(CONFIG_USB_FUNCTION_FASTBOOT)
> +	if (fastboot_key_pressed()) {
> +		printf("fastboot key pressed!\n");
> +		fb_set_reboot_flag();
> +	}
> +#endif
> +
> +#if (CONFIG_ROCKCHIP_BOOT_MODE_REG > 0)
> +	setup_boot_mode();
> +#endif
> +
> +	return rk_board_late_init();
> +}
> +
> +int board_init(void)
> +{
> +	int ret;
> +
> +#if !defined(CONFIG_SUPPORT_SPL)
> +	board_debug_uart_init();
> +#endif
> +#ifdef CONFIG_DM_REGULATOR
> +	ret = regulators_enable_boot_on(false);
> +	if (ret)
> +		debug("%s: Cannot enable boot on regulator\n", __func__);
> +#endif
> +
> +	return rk_board_init();
> +}
> +
> +#if !defined(CONFIG_SYS_DCACHE_OFF) && !defined(CONFIG_ARM64)
> +void enable_caches(void)
> +{
> +	/* Enable D-cache. I-cache is already enabled in start.S */
> +	dcache_enable();
> +}
> +#endif
> +
> +#if defined(CONFIG_USB_GADGET) && defined(CONFIG_USB_GADGET_DWC2_OTG)
> +#include <usb.h>
> +#include <usb/dwc2_udc.h>
> +
> +static struct dwc2_plat_otg_data otg_data = {
> +	.rx_fifo_sz	= 512,
> +	.np_tx_fifo_sz	= 16,
> +	.tx_fifo_sz	= 128,
> +};
> +
> +int board_usb_init(int index, enum usb_init_type init)
> +{
> +	int node;
> +	const char *mode;
> +	bool matched = false;
> +	const void *blob = gd->fdt_blob;
> +
> +	/* find the usb_otg node */
> +	node = fdt_node_offset_by_compatible(blob, -1,
> +					     "snps,dwc2");
> +
> +	while (node > 0) {
> +		mode = fdt_getprop(blob, node, "dr_mode", NULL);
> +		if (mode && strcmp(mode, "otg") == 0) {
> +			matched = true;
> +			break;
> +		}
> +
> +		node = fdt_node_offset_by_compatible(blob, node,
> +						     "snps,dwc2");
> +	}
> +	if (!matched) {
> +		debug("Not found usb_otg device\n");
> +		return -ENODEV;
> +	}
> +	otg_data.regs_otg = fdtdec_get_addr(blob, node, "reg");
> +
> +	return dwc2_udc_probe(&otg_data);
> +}

This function is already caught in review in another thread (where both I 
and Simon had complained about the way the device-tree is traversed from 
here).

Now this change would suddenly adds this code to all our SOCs instead of 
cleaning this up... in other words, this is our last change to clean it up 
w/o people depending on it: please do so.

Could we do something similar to
 	https://patchwork.ozlabs.org/patch/890968/
here?

> +
> +int board_usb_cleanup(int index, enum usb_init_type init)
> +{
> +	return 0;
> +}
> +#endif
> diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c
> new file mode 100644
> index 0000000..3c10b63
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/spl.c
> @@ -0,0 +1,195 @@
> +/*
> + * (C) Copyright 2018 Rockchip Electronics Co., Ltd

Given that there's quite a bit of code that we contributed (e.g. the 
entire spl_was_booted_from logic), I'd expect you to also (additionally) 
retain the Theobroma Systems copyright.

> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <debug_uart.h>
> +#include <dm.h>
> +#include <ram.h>
> +#include <spl.h>
> +#include <asm/arch/bootrom.h>
> +#include <asm/arch-rockchip/sys_proto.h>
> +#include <asm/io.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define BROM_BOOTSOURCE_ID_ADDR (CONFIG_ROCKCHIP_IRAM_START_ADDR + 0x10)

This should be a const-variable down where it's needed.
Plus, you'll need to document somewhere that you expect this at offset 
0x10 from the start of the IRAM on every SOC.

> +void board_return_to_bootrom(void)
> +{
> +	back_to_bootrom(BROM_BOOT_NEXTSTAGE);
> +}
> +
> +__weak const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
> +};

Having this defined as __weak here is an invitation for future trouble.
Someone will eventually forget to define it.  Better to rely on a 
link-time error to stop people from abusing this.

> +
> +const char *board_spl_was_booted_from(void)
> +{
> +	u32  bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);
> +	const char *bootdevice_ofpath = NULL;
> +
> +	if (bootdevice_brom_id < ARRAY_SIZE(boot_devices))
> +		bootdevice_ofpath = boot_devices[bootdevice_brom_id];
> +
> +	if (bootdevice_ofpath)
> +		debug("%s: brom_bootdevice_id %x maps to '%s'\n",
> +		      __func__, bootdevice_brom_id, bootdevice_ofpath);
> +	else
> +		debug("%s: failed to resolve brom_bootdevice_id %x\n",
> +		      __func__, bootdevice_brom_id);
> +
> +	return bootdevice_ofpath;
> +}
> +
> +u32 spl_boot_device(void)
> +{
> +	u32 boot_device = BOOT_DEVICE_MMC1;
> +
> +#if defined(CONFIG_TARGET_CHROMEBOOK_JERRY) || \
> +		defined(CONFIG_TARGET_CHROMEBIT_MICKEY) || \
> +		defined(CONFIG_TARGET_CHROMEBOOK_MINNIE)
> +	return BOOT_DEVICE_SPI;
> +#endif

This is not how it should be: if this is a common file, then there should 
be a common way to do this instead of having target-specific #ifdefs in 
here.

> +	if (CONFIG_IS_ENABLED(ROCKCHIP_BACK_TO_BROM))
> +		return BOOT_DEVICE_BOOTROM;
> +
> +	return boot_device;
> +}
> +
> +u32 spl_boot_mode(const u32 boot_device)
> +{
> +	return MMCSD_MODE_RAW;
> +}
> +
> +__weak void rockchip_stimer_init(void)
> +{
> +#ifndef CONFIG_ARM64
> +	asm volatile("mcr p15, 0, %0, c14, c0, 0"
> +		     : : "r"(COUNTER_FREQUENCY));
> +#endif
> +	writel(0, CONFIG_ROCKCHIP_STIMER_BASE + 0x10);
> +	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE);
> +	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
> +	writel(1, CONFIG_ROCKCHIP_STIMER_BASE + 0x10);
> +}
> +
> +__weak int arch_cpu_init(void)
> +{
> +	return 0;
> +}
> +
> +__weak int rk_board_init_f(void)
> +{
> +	return 0;
> +}

This doesn't really help in modularising our board-support and I am not a 
fan of adding something like 'rk_board_init_f' in the first place.

Instead this should be implemented in a way that actually makes the code 
structure easier and more resilient for the future (having __weak 
functions at the architecture-level doesn't really help)... in fact the 
only other uses of __weak in the U-Boot source-base are within SPL, as 
there's no other way to provide hooks there.

> +
> +void board_init_f(ulong dummy)
> +{
> +#ifdef CONFIG_SPL_FRAMEWORK
> +	int ret;
> +#if !defined(CONFIG_SUPPORT_TPL)
> +	struct udevice *dev;
> +#endif
> +#endif
> +
> +#if !defined(CONFIG_SUPPORT_TPL)
> +	rockchip_stimer_init();
> +	arch_cpu_init();
> +#endif
> +#define EARLY_UART
> +#if defined(EARLY_UART) && defined(CONFIG_DEBUG_UART)
> +	/*
> +	 * Debug UART can be used from here if required:
> +	 *
> +	 * debug_uart_init();
> +	 * printch('a');
> +	 * printhex8(0x1234);
> +	 * printascii("string");
> +	 */
> +	debug_uart_init();
> +	printascii("U-Boot SPL board init");
> +#endif
> +
> +#ifdef CONFIG_SPL_FRAMEWORK
> +	ret = spl_early_init();
> +	if (ret) {
> +		printf("spl_early_init() failed: %d\n", ret);
> +		hang();
> +	}
> +#if !defined(CONFIG_SUPPORT_TPL)
> +	debug("\nspl:init dram\n");
> +	ret = uclass_get_device(UCLASS_RAM, 0, &dev);
> +	if (ret) {
> +		printf("DRAM init failed: %d\n", ret);
> +		return;
> +	}
> +#endif
> +	preloader_console_init();
> +#else
> +	/* Some SoCs like rk3036 does not use any frame work */
> +	sdram_init();
> +#endif

This doesn't improve things at all, compared to having multiple files.
In fact, it makes things worse, now that we have to have support for the 
(legacy) sdram_init and the other code.

> +
> +	rk_board_init_f();
> +#if CONFIG_IS_ENABLED(ROCKCHIP_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT)
> +	back_to_bootrom(BROM_BOOT_NEXTSTAGE);
> +#endif
> +}
> +
> +#ifdef CONFIG_SPL_LOAD_FIT
> +int board_fit_config_name_match(const char *name)
> +{
> +	/* Just empty function now - can't decide what to choose */
> +	debug("%s: %s\n", __func__, name);
> +
> +	return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_SPL_BOARD_INIT
> +__weak int rk_spl_board_init(void)
> +{
> +	return 0;
> +}
> +
> +static int setup_led(void)
> +{
> +#ifdef CONFIG_SPL_LED
> +	struct udevice *dev;
> +	char *led_name;
> +	int ret;
> +
> +	led_name = fdtdec_get_config_string(gd->fdt_blob, "u-boot,boot-led");
> +	if (!led_name)
> +		return 0;
> +	ret = led_get_by_label(led_name, &dev);
> +	if (ret) {
> +		debug("%s: get=%d\n", __func__, ret);
> +		return ret;
> +	}
> +	ret = led_set_on(dev, 1);
> +	if (ret)
> +		return ret;
> +#endif
> +
> +	return 0;
> +}

Please move this to a separate file, that gets compiled in for 
CONFIG_SPL_LED only.

> +
> +void spl_board_init(void)
> +{
> +	int ret;
> +
> +	ret = setup_led();
> +
> +	if (ret) {
> +		debug("LED ret=%d\n", ret);
> +		hang();
> +	}
> +
> +	rk_spl_board_init();
> +#if CONFIG_IS_ENABLED(ROCKCHIP_BACK_TO_BROM)
> +	back_to_bootrom(BROM_BOOT_NEXTSTAGE);
> +#endif
> +}
> +#endif
> diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
> new file mode 100644
> index 0000000..6f9fbaf
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/tpl.c
> @@ -0,0 +1,111 @@
> +/*
> + * (C) Copyright 2017 Rockchip Electronics Co., Ltd
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <debug_uart.h>
> +#include <dm.h>
> +#include <ns16550.h>
> +#include <ram.h>
> +#include <spl.h>
> +#include <version.h>
> +#include <asm/io.h>
> +#include <asm/arch/bootrom.h>
> +#include <asm/arch/uart.h>
> +
> +#ifndef CONFIG_SPL_LIBCOMMON_SUPPORT
> +void puts(const char *str)
> +{
> +	while (*str)
> +		putc(*str++);
> +}
> +
> +void putc(char c)
> +{
> +	if (c == '\n')
> +		NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), '\r');
> +
> +	NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), c);
> +}
> +#endif /* CONFIG_SPL_LIBCOMMON_SUPPORT */
> +
> +u32 spl_boot_device(void)
> +{
> +	return BOOT_DEVICE_BOOTROM;
> +}
> +
> +__weak void rockchip_stimer_init(void)
> +{
> +#ifndef CONFIG_ARM64
> +	asm volatile("mcr p15, 0, %0, c14, c0, 0"
> +		     : : "r"(COUNTER_FREQUENCY));
> +#endif
> +	writel(0, CONFIG_ROCKCHIP_STIMER_BASE + 0x10);
> +	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE);
> +	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
> +	writel(1, CONFIG_ROCKCHIP_STIMER_BASE + 0x10);
> +}
> +
> +__weak int arch_cpu_init(void)
> +{
> +	return 0;
> +}
> +
> +void board_init_f(ulong dummy)
> +{
> +	struct udevice *dev;
> +	int ret;
> +
> +	/*
> +	 * Init the timer at the very beginning so that we can get more
> +	 * accurate value from timer_get_boot_us()
> +	 */
> +	rockchip_stimer_init();
> +	arch_cpu_init();
> +#define EARLY_DEBUG
> +#ifdef EARLY_DEBUG
> +	/*
> +	 * Debug UART can be used from here if required:
> +	 *
> +	 * debug_uart_init();
> +	 * printch('a');
> +	 * printhex8(0x1234);
> +	 * printascii("string");
> +	 */
> +	debug_uart_init();
> +	printascii("\nU-Boot TPL " PLAIN_VERSION " (" U_BOOT_DATE " - "
> +		   U_BOOT_TIME ")\n");
> +#endif
> +	ret = spl_early_init();
> +	if (ret) {
> +		debug("spl_early_init() failed: %d\n", ret);
> +		hang();
> +	}
> +
> +	/* Init ARM arch timer */
> +	timer_init();
> +	ret = uclass_get_device(UCLASS_RAM, 0, &dev);
> +	if (ret) {
> +		printf("DRAM init failed: %d\n", ret);
> +		return;
> +	}
> +
> +#if defined(CONFIG_TPL_ROCKCHIP_BACK_TO_BROM) && !defined(CONFIG_TPL_BOARD_INIT)
> +	back_to_bootrom(BROM_BOOT_NEXTSTAGE);
> +#endif
> +}
> +
> +#ifndef CONFIG_SPL_FRAMEWORK
> +/* Place Holders */
> +void board_init_r(gd_t *id, ulong dest_addr)
> +{
> +	/*
> +	 * Function attribute is no-return
> +	 * This Function never executes
> +	 */
> +	while (1)
> +		;
> +}
> +#endif
>