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

[Philipp Tomsich]

Kever,

> On 29.11.2018, at 02:10, Kever Yang <kever.yang at rock-chips.com> wrote:
> 
> 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 <mailto: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 <mailto: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/ <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;

As I had earlier explained that the ACK means that the patch has gone into
my review queue and that I started processing it.  I did change my process
since then, as this ACK had apparently been misleading to people … now the
first sign that I started processing the patch usually is a review comment.

I don’t feel that skipping the ACK is an improvement, but it seems to be less
misleading to users.

> - 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.

When I state that changes are requested, this is usually for a good reason
(e.g. maintainability).  So unless the comment actually serves to address
the technical concern, there is no reason to reply (as the only reply would
be a “I stand by my earlier comment.” which isn’t really helpful.

Thanks,
Philipp.

> - 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;

Allowing the first patches to go in was a mistake in retrospect.  Back then,
I did not consider that this model would suddenly be extended beyond the
initial few use cases.

> - 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