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

[Simon Glass]

Hi Philipp,

On Thu, 6 Dec 2018 at 11:23, Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
>
>
>
> > 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.

And just to be explicit, I worry that individual register settings
might not map 100% to the function. E.g. you might need to change two
registers to enable JTAG (pinmux and setting). It seems odd to have an
interface that accesses registers which doesn't specify the address.

Regards,
Simon