| Message ID | 1522142971-20739-37-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | rockchip: clean up board file for rockchip SoCs | expand |
> 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. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > 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 > Acked-by: 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 >
Philipp, On 04/02/2018 05:28 AM, Philipp Tomsich wrote: > > > 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)... I understand review the patches piece by piece is much more comfortable, and this patch including "too much" things. And I never expect this patch set can be merge quickly, but we have to do this ASAP before more SoC coming. I have do a lot of test and re-work in my local branch and at last make it landed in rockchip vendor U-Boot, with testing in most of SoCs(not including rk3066/rk3188). Well, I do try to split it into pieces, but I found that actually not help very much except waste much more time: - The target is(very clear) to make rockchip soc board file in a good shape with common files, instead of copy-paste for each soc(more than 10 of them now) - then we need to identify what's common and what should go to soc and board; - remove using common rockchip timer and use arm generic timer instead for armv7 SoCs(rk3066 and rk3188 need still using rockchip timer) - most soc need to do uart init, boot order select, and some arch_cpu_init(). - don't break the boards already working, so I still leave some code which not so common in board file, but I would like to remove or move them into right place if I got a board to verify; @Simon, @Tom, This patch set is to remove some common files and add some other common files for all Rockchip SoCs, I have to make sure the whole patch set can running good for all SoCs, but it's really hard to make every patch to build and work perfect for all SoCs, is there any mandatory rules for this? I have to do a lot of temporary work for this like add temporary MACRO for those SoCs convert to use common code, and remove it after all the SoCs have convert to use common code, which have no any help for what we get at last, but it really cost a lot of time. > >> >> 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. I expect USB dwc2 maintainer can do this(convert to use DM for dwc2 gadget) very long time ago, but I never see anyone say 'yes' and do it. Most of Rockchip SoCs(except rk3399) using dwc2, so it's right to present in common board file, you can see many copies for it before this patch set. My patch set is to merge the common code in Rockchip platform, and not try to do any modify to dwc2 driver. > > Could we do something similar to > https://patchwork.ozlabs.org/patch/890968/ > here? This is dwc3 host mode, not for dwc2, there is a similar file for rockchip dwc3. > >> + >> +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. Sorry for missing copyright for your company, will add it back. > >> + * >> + * 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. I know your proposal is to use DM for board init, then could you make it more clear about how to handle this in your solution? We need to do: - same board init flow for all rockchip platform; - something different but common in soc level; - something different in board level; > >> + >> +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. I do consider about using one separate file for tiny sram init, but please understand this is not 'legacy', because it will always there and new soc will also use it. > >> + >> + 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. Why? Add one separate common file with only one function inside? Thanks, - Kever
On Sun, Apr 08, 2018 at 09:45:22AM +0800, Kever Yang wrote: > Philipp, > > > On 04/02/2018 05:28 AM, Philipp Tomsich wrote: > > > > > > 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)... > > I understand review the patches piece by piece is much more comfortable, > and this patch including "too much" things. And I never expect this > patch set > can be merge quickly, but we have to do this ASAP before more SoC coming. > I have do a lot of test and re-work in my local branch and at last make > it landed in > rockchip vendor U-Boot, with testing in most of SoCs(not including > rk3066/rk3188). > Well, I do try to split it into pieces, but I found that actually not > help very much > except waste much more time: > - The target is(very clear) to make rockchip soc board file in a good > shape with common files, > instead of copy-paste for each soc(more than 10 of them now) > - then we need to identify what's common and what should go to soc and > board; > - remove using common rockchip timer and use arm generic timer instead > for armv7 > SoCs(rk3066 and rk3188 need still using rockchip timer) > - most soc need to do uart init, boot order select, and some > arch_cpu_init(). > - don't break the boards already working, so I still leave some code > which not so common > in board file, but I would like to remove or move them into right > place if I got a board > to verify; > > @Simon, @Tom, > This patch set is to remove some common files and add some other common > files for > all Rockchip SoCs, I have to make sure the whole patch set can running > good for all SoCs, > but it's really hard to make every patch to build and work perfect for > all SoCs, is there > any mandatory rules for this? So you mean possibly breaking some existing platforms? I don't like the idea of doing that...
Kever, > On 9 Apr 2018, at 00:35, Tom Rini <trini@konsulko.com> wrote: > > On Sun, Apr 08, 2018 at 09:45:22AM +0800, Kever Yang wrote: >> Philipp, >> >> >> On 04/02/2018 05:28 AM, Philipp Tomsich wrote: >>> >>> >>> 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)... >> >> I understand review the patches piece by piece is much more comfortable, >> and this patch including "too much" things. And I never expect this >> patch set >> can be merge quickly, but we have to do this ASAP before more SoC coming. >> I have do a lot of test and re-work in my local branch and at last make >> it landed in >> rockchip vendor U-Boot, with testing in most of SoCs(not including >> rk3066/rk3188). >> Well, I do try to split it into pieces, but I found that actually not >> help very much >> except waste much more time: >> - The target is(very clear) to make rockchip soc board file in a good >> shape with common files, >> instead of copy-paste for each soc(more than 10 of them now) >> - then we need to identify what's common and what should go to soc and >> board; >> - remove using common rockchip timer and use arm generic timer instead >> for armv7 >> SoCs(rk3066 and rk3188 need still using rockchip timer) >> - most soc need to do uart init, boot order select, and some >> arch_cpu_init(). >> - don't break the boards already working, so I still leave some code >> which not so common >> in board file, but I would like to remove or move them into right >> place if I got a board >> to verify; Having a clean commit-history and the ability to bisect and revert individual patches is an important requirement for the overall project. I thought to have already provided you the needed guidance on how to get most fo this merged and highlighted where architectural discussions will be needed. So to get most of this merged quickly, you will need to break this up into series that are more manageable (this is likely to not be a full list): * the changes for split out the UART configs * the timer changes * adding a common board-file and switching boards over For the common board-file, you should add this first and then start moving SoCs over onto this new file. I believe (my memory may be wrong) to have commented so in some of the individual reviews. Finally, there’s a few architectural issues to discuss: 1. You are merging the “board”-files, but these are now merged across multiple SoCs and across multiple boards per SoC. This is already causing some fraying at the edges (e.g. the number of rk_* hooks and the weak functions added). We should very carefully consider how this will affect adding boards (which may have a specific market segment in mind and may—for lack of a better example—not want to have rockusb included) in the future. 2. The new weak-functions are causing a major headache: they are at odds with us trying to move to DM across the entire tree. I strongly believe that these weak functions will cause added debug issues in the short term and that they will eventually be replaced with more DM-like model in the long term. Thanks, Philipp. >> @Simon, @Tom, >> This patch set is to remove some common files and add some other common >> files for >> all Rockchip SoCs, I have to make sure the whole patch set can running >> good for all SoCs, >> but it's really hard to make every patch to build and work perfect for >> all SoCs, is there >> any mandatory rules for this? > > So you mean possibly breaking some existing platforms? I don't like the > idea of doing that... > > -- > Tom
Kever, > On 8 Apr 2018, at 03:45, Kever Yang <kever.yang@rock-chips.com> wrote: > > Philipp, > > > On 04/02/2018 05:28 AM, Philipp Tomsich wrote: >> >> >> 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)... > > I understand review the patches piece by piece is much more comfortable, In fact I do not like to do these reviews, as they are a tiresome chore… …but they need to be done, as some issues are best caught at this stage. Note, that a good commit message (i.e. one that summarises the status quo/presents the motivation for the specific change; then summarises what is changed how and to what effect; finally notes on anything that the reviewer/someone debugging in the future should know) helps very much in reviewing. However, a review can not catch all issues and once a patch-set gets to a certain level of complexity, it is likely to introduce unnecessary breakage that could have been avoided in a review (if there simply hadn’t been too many changes at once). Consequently, we need a clean history consisting of orthogonal changes so we can bisect and revert if necessary (and I’d prefer not to revert an entire series)… which again requires patches that are (a) in a healthy application-order and (b) do a well-defined number of things well. > and this patch including "too much" things. And I never expect this > patch set > can be merge quickly, but we have to do this ASAP before more SoC coming. The quickest way to get at least some of this merged quickly (e.g. the UART changes) is to have smaller series for these. > I have do a lot of test and re-work in my local branch and at last make > it landed in > rockchip vendor U-Boot, with testing in most of SoCs(not including > rk3066/rk3188). > Well, I do try to split it into pieces, but I found that actually not > help very much > except waste much more time: > - The target is(very clear) to make rockchip soc board file in a good > shape with common files, > instead of copy-paste for each soc(more than 10 of them now) > - then we need to identify what's common and what should go to soc and > board; > - remove using common rockchip timer and use arm generic timer instead > for armv7 > SoCs(rk3066 and rk3188 need still using rockchip timer) > - most soc need to do uart init, boot order select, and some > arch_cpu_init(). > - don't break the boards already working, so I still leave some code > which not so common > in board file, but I would like to remove or move them into right > place if I got a board > to verify; > > @Simon, @Tom, > This patch set is to remove some common files and add some other common > files for > all Rockchip SoCs, I have to make sure the whole patch set can running > good for all SoCs, > but it's really hard to make every patch to build and work perfect for > all SoCs, is there > any mandatory rules for this? > > I have to do a lot of temporary work for this like add temporary MACRO > for those SoCs > convert to use common code, and remove it after all the SoCs have > convert to use common > code, which have no any help for what we get at last, but it really cost > a lot of time. > >> >>> >>> 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. > > I expect USB dwc2 maintainer can do this(convert to use DM for dwc2 > gadget) very long time ago, > but I never see anyone say 'yes' and do it. > Most of Rockchip SoCs(except rk3399) using dwc2, so it's right to > present in common board > file, you can see many copies for it before this patch set. > My patch set is to merge the common code in Rockchip platform, and not > try to do any > modify to dwc2 driver. > >> >> Could we do something similar to >> https://patchwork.ozlabs.org/patch/890968/ <https://patchwork.ozlabs.org/patch/890968/> >> here? > > This is dwc3 host mode, not for dwc2, there is a similar file for > rockchip dwc3. >> >>> + >>> +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. > > Sorry for missing copyright for your company, will add it back. >> >>> + * >>> + * 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. > > I know your proposal is to use DM for board init, then could you make it > more > clear about how to handle this in your solution? > We need to do: > - same board init flow for all rockchip platform; > - something different but common in soc level; > - something different in board level; > >> >>> + >>> +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. > > I do consider about using one separate file for tiny sram init, but > please understand this is not 'legacy', > because it will always there and new soc will also use it. >> >>> + >>> + 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. > > Why? > Add one separate common file with only one function inside? > > Thanks, > - Kever
Hi Tom, On 04/09/2018 06:35 AM, Tom Rini wrote: >> I have do a lot of test and re-work in my local branch and at last make >> it landed in >> rockchip vendor U-Boot, with testing in most of SoCs(not including >> rk3066/rk3188). >> Well, I do try to split it into pieces, but I found that actually not >> help very much >> except waste much more time: >> - The target is(very clear) to make rockchip soc board file in a good >> shape with common files, >> instead of copy-paste for each soc(more than 10 of them now) >> - then we need to identify what's common and what should go to soc and >> board; >> - remove using common rockchip timer and use arm generic timer instead >> for armv7 >> SoCs(rk3066 and rk3188 need still using rockchip timer) >> - most soc need to do uart init, boot order select, and some >> arch_cpu_init(). >> - don't break the boards already working, so I still leave some code >> which not so common >> in board file, but I would like to remove or move them into right >> place if I got a board >> to verify; >> >> @Simon, @Tom, >> This patch set is to remove some common files and add some other common >> files for >> all Rockchip SoCs, I have to make sure the whole patch set can running >> good for all SoCs, >> but it's really hard to make every patch to build and work perfect for >> all SoCs, is there >> any mandatory rules for this? > So you mean possibly breaking some existing platforms? I don't like the > idea of doing that... No, I'm not intent to to breaking some existing platforms, this patch set including 36 patches, all the platform should work fine after apply all these patches, but if only some of them applied, there is compile error or running error because of feature missing. Thanks, - Kever
On Tue, Apr 10, 2018 at 02:54:09PM +0800, Kever Yang wrote: > Hi Tom, > > > On 04/09/2018 06:35 AM, Tom Rini wrote: > >> I have do a lot of test and re-work in my local branch and at last make > >> it landed in > >> rockchip vendor U-Boot, with testing in most of SoCs(not including > >> rk3066/rk3188). > >> Well, I do try to split it into pieces, but I found that actually not > >> help very much > >> except waste much more time: > >> - The target is(very clear) to make rockchip soc board file in a good > >> shape with common files, > >> instead of copy-paste for each soc(more than 10 of them now) > >> - then we need to identify what's common and what should go to soc and > >> board; > >> - remove using common rockchip timer and use arm generic timer instead > >> for armv7 > >> SoCs(rk3066 and rk3188 need still using rockchip timer) > >> - most soc need to do uart init, boot order select, and some > >> arch_cpu_init(). > >> - don't break the boards already working, so I still leave some code > >> which not so common > >> in board file, but I would like to remove or move them into right > >> place if I got a board > >> to verify; > >> > >> @Simon, @Tom, > >> This patch set is to remove some common files and add some other common > >> files for > >> all Rockchip SoCs, I have to make sure the whole patch set can running > >> good for all SoCs, > >> but it's really hard to make every patch to build and work perfect for > >> all SoCs, is there > >> any mandatory rules for this? > > So you mean possibly breaking some existing platforms? I don't like the > > idea of doing that... > > No, I'm not intent to to breaking some existing platforms, > this patch set including 36 patches, all the platform should work fine > after apply all these patches, but if only some of them applied, > there is compile error or running error because of feature missing. OK. Similar to the Linux kernel, it's not a good thing to break buildability of things during a patch series.
> On 10 Apr 2018, at 14:32, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Apr 10, 2018 at 02:54:09PM +0800, Kever Yang wrote: >> Hi Tom, >> >> >> On 04/09/2018 06:35 AM, Tom Rini wrote: >>>> I have do a lot of test and re-work in my local branch and at last make >>>> it landed in >>>> rockchip vendor U-Boot, with testing in most of SoCs(not including >>>> rk3066/rk3188). >>>> Well, I do try to split it into pieces, but I found that actually not >>>> help very much >>>> except waste much more time: >>>> - The target is(very clear) to make rockchip soc board file in a good >>>> shape with common files, >>>> instead of copy-paste for each soc(more than 10 of them now) >>>> - then we need to identify what's common and what should go to soc and >>>> board; >>>> - remove using common rockchip timer and use arm generic timer instead >>>> for armv7 >>>> SoCs(rk3066 and rk3188 need still using rockchip timer) >>>> - most soc need to do uart init, boot order select, and some >>>> arch_cpu_init(). >>>> - don't break the boards already working, so I still leave some code >>>> which not so common >>>> in board file, but I would like to remove or move them into right >>>> place if I got a board >>>> to verify; >>>> >>>> @Simon, @Tom, >>>> This patch set is to remove some common files and add some other common >>>> files for >>>> all Rockchip SoCs, I have to make sure the whole patch set can running >>>> good for all SoCs, >>>> but it's really hard to make every patch to build and work perfect for >>>> all SoCs, is there >>>> any mandatory rules for this? >>> So you mean possibly breaking some existing platforms? I don't like the >>> idea of doing that... >> >> No, I'm not intent to to breaking some existing platforms, >> this patch set including 36 patches, all the platform should work fine >> after apply all these patches, but if only some of them applied, >> there is compile error or running error because of feature missing. > > OK. Similar to the Linux kernel, it's not a good thing to break > buildability of things during a patch series. Independent of this: this is not a single series, but multiple series rolled into one. Once the commit messages are reworked to convey what’s changed in a more meaningful way, this will be even more apparent than it already is today. Thanks, Philipp.
Hi Philipp, On 04/08/2018 09:45 AM, Kever Yang wrote: >>> +__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. > I know your proposal is to use DM for board init, then could you make it > more > clear about how to handle this in your solution? > We need to do: > - same board init flow for all rockchip platform; > - something different but common in soc level; > - something different in board level; I didn't see your response for this, could you send out your patches? I admit that I'm not very clear about the limitation of '__weak' function, but I do see there are many '__weak' function in common/board_f/r.c, and my common board file is connect to the board_r.c. @Simon, @Tom, Could you kindly give some comment here? Thanks, - Kever
On Fri, Apr 13, 2018 at 03:51:07PM +0800, Kever Yang wrote: > Hi Philipp, > > > On 04/08/2018 09:45 AM, Kever Yang wrote: > >>> +__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. > > I know your proposal is to use DM for board init, then could you make it > > more > > clear about how to handle this in your solution? > > We need to do: > > - same board init flow for all rockchip platform; > > - something different but common in soc level; > > - something different in board level; > > I didn't see your response for this, could you send out your patches? > > I admit that I'm not very clear about the limitation of '__weak' function, > but I do see there are many '__weak' function in common/board_f/r.c, > and my common board file is connect to the board_r.c. > > @Simon, @Tom, > Could you kindly give some comment here? I am perhaps more of a fan of using weak functions than other people are. The problem with weak functions is that you must know when designing the code that it can safely only be overridden in one place per build. Otherwise the results are not predictable.
Kever, > On 13 Apr 2018, at 09:51, Kever Yang <kever.yang@rock-chips.com> wrote: > On 04/08/2018 09:45 AM, Kever Yang wrote: > >>>> +__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. >> I know your proposal is to use DM for board init, then could you make it >> more >> clear about how to handle this in your solution? >> We need to do: >> - same board init flow for all rockchip platform; >> - something different but common in soc level; >> - something different in board level; > > I didn't see your response for this, could you send out your patches? This isn’t at the stage of a patch-set yet… I had asked for comments to this, so we could design this in a way that benefits all platforms. > I admit that I'm not very clear about the limitation of '__weak' function, > but I do see there are many '__weak' function in common/board_f/r.c, > and my common board file is connect to the board_r.c. I like __weak as a way to provide a hook for something that is part of the common API (so it’s ok, if spl.c uses this). However, I don’t want individual platforms to suddenly expose new extension points. And with the two examples above (arch_cpu_init and rk_board_init_f), you basically highlight what’s wrong about using __weak at this level: 1 arch_cpu_init is an extension point to do low-level initialisation for a CPU (not a board). Implementation for it usually live below arch/arm/cpu and takes care of things like MMU maintenance. Now we suddenly provide this below arch/arm/mach-rockchip … and using a __weak function. This goes against everything that users will expect. So just move it to arch/arm/cpu (you’ll probably need to have 2 separate ones for armv7 and armv8) and nothing unexpected will ever happen. 2 If we rk_board_init_f here, we are again changing the extension API of U-Boot: board_init_f belongs to each board (i.e. any board can expect to override it w/o ill effect), but now you’d suddenly create a link error. Instead users need to override rk_board_init_f. This is a documentation nightmare (and the current solution would be to provide a common function that all board_init_f implementations could call as their head or tail…). My question—as to whether the DM could/should be extended to CPUs, SoCs, architectures and boards—was meant to discuss exactly these observed issues in how boards and SoCs today interact. I usually don’t mind to touch APIs and extend them (or the driver model), but a solution for how to handle board/SoC/CPU init sequences is nothing I want to start before getting an actual design discussion going and reaching something resembling a consensus of how this aspect of U-Boot should be structured in a year’s (or two years’) time. > > @Simon, @Tom, > Could you kindly give some comment here? > > Thanks, > - Kever
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 - 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); +} + +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 + * + * 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) +void board_return_to_bootrom(void) +{ + back_to_bootrom(BROM_BOOT_NEXTSTAGE); +} + +__weak const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = { +}; + +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 + 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; +} + +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 + + 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; +} + +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
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. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- 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