> On 06.12.2018, at 19:18, Simon Glass <s...@chromium.org> wrote: > > Hi Philipp, > > On Thu, 6 Dec 2018 at 04:02, Philipp Tomsich > <philipp.toms...@theobroma-systems.com> wrote: >> >> Simon, >> >>> On 06.12.2018, at 02:31, Simon Glass <s...@chromium.org> wrote: >>> >>> Hi Philipp, >>> >>> On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich >>> <philipp.toms...@theobroma-systems.com> wrote: >>>> >>>> Simon, >>>> >>>> On 29.11.2018, at 19:43, Simon Glass <s...@chromium.org> wrote: >>>> >>>> Hi Kever, >>>> >>>> On Wed, 28 Nov 2018 at 18:10, Kever Yang <kever.y...@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.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> >>>> >>>> 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
