Kever,
> On 8 Apr 2018, at 03:45, Kever Yang <kever.y...@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.y...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
