Kever,
> On 29.11.2018, at 02:10, Kever Yang <kever.y...@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.y...@rock-chips.com
>>> <mailto:kever.y...@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.y...@rock-chips.com
>>> <mailto:kever.y...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
