[U-Boot] [RESENT PATCH 1/2] rockchip: rk3128: use ROCKCHIP_BOOT_MODE_REG to update reboot flag

[Kever Yang]

Hi Philipp,


On 11/28/2018 05:07 PM, Philipp Tomsich wrote:
> Kever,
>
>> On 28.11.2018, at 03:04, Kever Yang <kever.yang at rock-chips.com> wrote:
>>
>> Use ROCKCHIP_BOOT_MODE_REG instead of grf structure so that
>> we can re-use the source code later.
>>
>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> NAK, as there are still pending changes.
Yes, I got that, and I send out my comments on your comments with no
more response.
> See below for the reminder, in case this got lost.
>
>> ---
>>
>> arch/arm/mach-rockchip/Kconfig        | 1 +
>> arch/arm/mach-rockchip/rk3128-board.c | 5 +----
>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
>> index 145d96b1f0..94a03e2a38 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -179,6 +179,7 @@ config TPL_ROCKCHIP_BACK_TO_BROM
>> config ROCKCHIP_BOOT_MODE_REG
>> 	hex "Rockchip boot mode flag register address"
>> 	default 0x200081c8 if ROCKCHIP_RK3036
>> +	default 0x100a0038 if ROCKCHIP_RK3128
>> 	default 0x20004040 if ROCKCHIP_RK3188
>> 	default 0x110005c8 if ROCKCHIP_RK322X
>> 	default 0xff730094 if ROCKCHIP_RK3288
> As previously discussed: these should all go into header files, as they are not user-configurable.
> This affects multiple patch series (as I requested the same for the STIMER address).
I believe you mention about this: http://patchwork.ozlabs.org/patch/891462/
Now the model in u-boot rockchip channel is:
- People send out patches to mailing list;
- The patch get an ACK patch and review patch may request for change in
1 week~4 months;
- According to the maintainer's comment, people reply for why the patch
like this,
   and maybe the patch do not need to change just like what the
maintainer want.
- BUT, there will never be more reply/comments.
- Then, people have to resend the patches they think it may be
reasonable, and maintainer
   then complain people doesn't address his comment.

For this patch, I think:
- This is not an first patch for this operation, this just make rk3128
work like other SoCs, it's not a new feature;
- This kind of default value setting is all over the U-Boot project, I'm
not say it's correct,
  but it's a good solution and convenient for us to use the same object
with different value in different SoCs,
  It's much better to separate them into more then 10 header files or
lots of "#ifdef CONFIG_ROCKCHIIP_RK3128"
  in one header files.

  I hope I can get reply for this mail this time.

Hi Simon,
    Could you help to comment on this?
 
Thanks,
- Kever
>
> Please also refer to the discussion on the topic of "rockchip: add STIMER_BASE for Rockchip SoCs”
> where we discussed that asm/arch-rockchip would be preferable over include/config as a header
> file directory.
>
> Thanks,
> Philipp.
>
>> diff --git a/arch/arm/mach-rockchip/rk3128-board.c b/arch/arm/mach-rockchip/rk3128-board.c
>> index 7fd667a0b8..f64ccc51a0 100644
>> --- a/arch/arm/mach-rockchip/rk3128-board.c
>> +++ b/arch/arm/mach-rockchip/rk3128-board.c
>> @@ -114,12 +114,9 @@ int board_usb_cleanup(int index, enum usb_init_type init)
>> #if CONFIG_IS_ENABLED(FASTBOOT)
>> int fastboot_set_reboot_flag(void)
>> {
>> -	struct rk3128_grf *grf;
>> -
>> 	printf("Setting reboot to fastboot flag ...\n");
>> -	grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
>> 	/* Set boot mode to fastboot */
>> -	writel(BOOT_FASTBOOT, &grf->os_reg[0]);
>> +	writel(BOOT_FASTBOOT, CONFIG_ROCKCHIP_BOOT_MODE_REG);
>>
>> 	return 0;
>> }
>> -- 
>> 2.18.0
>>
>