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

[Philipp Tomsich]

> On 06.12.2018, at 19:18, Simon Glass <sjg at chromium.org> wrote:
> 
> Hi Philipp,
> 
> On Thu, 6 Dec 2018 at 04:02, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>> 
>> Simon,
>> 
>>> On 06.12.2018, at 02:31, Simon Glass <sjg at chromium.org> wrote:
>>> 
>>> Hi Philipp,
>>> 
>>> On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich
>>> <philipp.tomsich at theobroma-systems.com> wrote:
>>>> 
>>>> Simon,
>>>> 
>>>> On 29.11.2018, at 19:43, Simon Glass <sjg at chromium.org> wrote:
>>>> 
>>>> Hi Kever,
>>>> 
>>>> On Wed, 28 Nov 2018 at 18:10, Kever Yang <kever.yang at rock-chips.com> wrote:
>>>> 
>>>> 
>>>> 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?
>>>> 
>>>> 
>>>> What happens if the user changes the value?
>>>> 
>>>> Can this go in the device tree?
>>>> 
>>>> It seems like this should be in a driver, to me. We have a SYSCON
>>>> driver for GRF. Should we add an ioctl-type interface to it?
>>>> 
>>>> 
>>>> This affects a number of settings by now, including the addresses for the
>>>> debug UARTs, secure timer base addresses and the boot-mode register.
>>>> 
>>>> What we’d really need would be a “read/write named-register” operation
>>>> (which could either be an ioctl or a new read/write operation that takes a
>>>> selector that can then internally be mapped onto an actual address).
>>>> However, this would require a custom syscon for each chip (or at least a
>>>> per-chip driver-data), which also doesn’t sound like a desirable design.
>>> 
>>> I assume it would come from the device tree.
>>> 
>>> To me this seems like a reasonable design. Yes it would need per-chip
>>> DT settings, or perhaps driver data.
>>> 
>>> But I believe we alreayd have a syscon_xxxx.c for each chip.
>> 
>> Yes, there are syscon_*.c files per chip, but these only contain the compatible
>> tables and reuse the generic syscon implementation.
>> 
>> I’ll take a stab at creating a RFC series to extend the generic syscon with an
>> ‘read/write named register’ mechanism.
> 
> OK, but I think it might be better to use something like ioctl(), if
> the operation is generally the same. E.g. if you have operations like:
> 
> - set boot mode
> - enable JTAG
> - ..

Good point. I still have my old prototype that tried something similar for
RGMII settings — should be a good-enough starting point.

Philipp.