| Message ID | 1522142971-20739-6-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | rockchip: clean up board file for rockchip SoCs | expand |
> STIMER is can only access in secure mode if the SoCs supports trust, > and it locate in alive power domain, as the source of ARM arch/generic > timer, we add a base addr for all SoCs so that we can init with a common > function. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/mach-rockchip/Kconfig | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
On Tue, 27 Mar 2018, Kever Yang wrote: > STIMER is can only access in secure mode if the SoCs supports trust, > and it locate in alive power domain, as the source of ARM arch/generic > timer, we add a base addr for all SoCs so that we can init with a common > function. The commit message does not really tell what the source changes are (although it seems to describe part of the motivation for this change). > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> See below for requested changes. > --- > > arch/arm/mach-rockchip/Kconfig | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig > index 007cb22..5dfe452 100644 > --- a/arch/arm/mach-rockchip/Kconfig > +++ b/arch/arm/mach-rockchip/Kconfig > @@ -190,6 +190,25 @@ config ROCKCHIP_BOOT_MODE_REG > The Soc will enter to different boot mode(defined in asm/arch/boot_mode.h) > according to the value from this register. > > +config ROCKCHIP_STIMER_BASE > + hex "Rockchip Secure timer base address" > + default 0xff220020 if ROCKCHIP_PX30 We don't have support for the PX30 in the U-Boot code base yet. Until a series to add support for the PX30 comes in, you should not have this here yet (and then add this specific case when the PX30 support is added). > + default 0x200440a0 if ROCKCHIP_RK3036 > + default 0x2000e000 if ROCKCHIP_RK3066 > + default 0x20018020 if ROCKCHIP_RK3126 > + default 0x200440a0 if ROCKCHIP_RK3128 > + default 0x2000e000 if ROCKCHIP_RK3188 > + default 0x110d0020 if ROCKCHIP_RK322X > + default 0xff810020 if ROCKCHIP_RK3288 > + default 0xff1d0020 if ROCKCHIP_RK3328 > + default 0xff830020 if ROCKCHIP_RK3368 > + default 0xff8680a0 if ROCKCHIP_RK3399 > + default 0x10350020 if ROCKCHIP_RV1108 > + default 0 > + help > + The secure timer inited in SPL/TPL in secure word, ARM generic timer > + works after this timer work. This should not be in Kconfig and rather in a header-file. With what you do here, a user (e.g. via 'make menuconfig') or a defconfig file (e.g. due to a typo) could accidentially change overwrite this. > + > config ROCKCHIP_SPL_RESERVE_IRAM > hex "Size of IRAM reserved in SPL" > default 0 >
On 04/02/2018 04:58 AM, Philipp Tomsich wrote: > > > On Tue, 27 Mar 2018, Kever Yang wrote: > >> STIMER is can only access in secure mode if the SoCs supports trust, >> and it locate in alive power domain, as the source of ARM arch/generic >> timer, we add a base addr for all SoCs so that we can init with a common >> function. > > The commit message does not really tell what the source changes are Then I have no idea what kind information need to add in this commit message, could you make it more clear? I can add message about there is a coming up patch to use this address. > (although it seems to describe part of the motivation for this change). > >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > > See below for requested changes. > >> --- >> >> arch/arm/mach-rockchip/Kconfig | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/arch/arm/mach-rockchip/Kconfig >> b/arch/arm/mach-rockchip/Kconfig >> index 007cb22..5dfe452 100644 >> --- a/arch/arm/mach-rockchip/Kconfig >> +++ b/arch/arm/mach-rockchip/Kconfig >> @@ -190,6 +190,25 @@ config ROCKCHIP_BOOT_MODE_REG >> The Soc will enter to different boot mode(defined in >> asm/arch/boot_mode.h) >> according to the value from this register. >> >> +config ROCKCHIP_STIMER_BASE >> + hex "Rockchip Secure timer base address" >> + default 0xff220020 if ROCKCHIP_PX30 > > We don't have support for the PX30 in the U-Boot code base yet. > Until a series to add support for the PX30 comes in, you should not > have this here yet (and then add this specific case when the PX30 > support is added). > >> + default 0x200440a0 if ROCKCHIP_RK3036 >> + default 0x2000e000 if ROCKCHIP_RK3066 >> + default 0x20018020 if ROCKCHIP_RK3126 >> + default 0x200440a0 if ROCKCHIP_RK3128 >> + default 0x2000e000 if ROCKCHIP_RK3188 >> + default 0x110d0020 if ROCKCHIP_RK322X >> + default 0xff810020 if ROCKCHIP_RK3288 >> + default 0xff1d0020 if ROCKCHIP_RK3328 >> + default 0xff830020 if ROCKCHIP_RK3368 >> + default 0xff8680a0 if ROCKCHIP_RK3399 >> + default 0x10350020 if ROCKCHIP_RV1108 >> + default 0 >> + help >> + The secure timer inited in SPL/TPL in secure word, ARM generic >> timer >> + works after this timer work. > > This should not be in Kconfig and rather in a header-file. I think it's better to make this information in one place instead of in different header files(more then 10 soc header files). > With what you do here, a user (e.g. via 'make menuconfig') or a > defconfig file (e.g. due to a typo) could accidentially change > overwrite this. This value connect with the SoC type, I don't understand what kind of typo will overwrite this. Thanks, - Kever > >> + >> config ROCKCHIP_SPL_RESERVE_IRAM >> hex "Size of IRAM reserved in SPL" >> default 0 >> >
Hi U-Boot Maintainers: I would like to close this topic and update all related source code, you can find the previous discussion at [0][1]. Philipp object this patch and his original reason is: > This should not be in Kconfig and rather in a header-file. > With what you do here, a user (e.g. via 'make menuconfig') or a defconfig > file (e.g. due to a typo) could accidentially change overwrite this. and Philipp also think: > asm/arch-rockchip would be preferable over include/config as a header There are many ways for feature configure on rockchip platform now: - board dts; - header file in include/config - Kconfig in $(BOARD)_defconfig - Kconfig default value in 'Kconfig' file Already too many place to config, we should make it simple rather than complex. I think people are migrating more and more configurations from header file to Kconfig, including: - option for enable/disable modules; - option for module parameter which is numerical value; - option for module parameter which is string; I think the target is that most of the configs goes to Kconfig and user can config options with menuconfig and no need to touch header file(Please tell me if this is not true). And for all those config options, I think at lease can be separate into two kind: - options per-board; - options per-soc (some of them may per-vendor); I think the idealized model would be per-soc options goes to 'Kconfig' file with default value combine with SOC, and per-board options at $(BOARD)_defconfig. ROCKCHIP_STIMER_REG(another case is BOOT_MODE_REG) is per-soc config option, just like SYS_TEXT_BASE, TPL_TEXT_BASE, TPL_MAX_SIZE, SYS_SOC and so on. Driver for different SOCs to use this reg are just the same, and this reg needs to be used very early in TPL/SPL which means no DM, no dts available. Here are the solutions from previous discussion: - dts (by Simon) extra dtb code needed, setting separate into individual board dts; not available and not suggestion to use in TPL/SPL; - move to header in 'asm/arch-rockchip' (by Philipp) one more place to config, make config options more complicate; may separate in more than 10+ files for different soc/board; - RMII setting like, (by Philipp) reference to 'drivers/net/gmac_rockchip.c', driver for different SoCs are not re-usable, driver needs to update for each new SoC support; not available with DM in early TPL/SPL; - Kconfig with default value in this patch Keep driver clean and no need to update with new SoC; One place rather than 10+ individual files, same usage like SYS_TEXT_BASE, reference to: 341c058654 sunxi: move CONFIG_SYS_TEXT_BASE out of defconfigs I think the default value in Kconfig is the best solution, the SPL/TPL should be small, fast and with enough functionality for use. Thanks, - Kever [0] https://patchwork.ozlabs.org/patch/1004148/ [1] http://patchwork.ozlabs.org/patch/891462/ Kever Yang <kever.yang@rock-chips.com> 于2018年3月27日周二 下午5:30写道： > STIMER is can only access in secure mode if the SoCs supports trust, > and it locate in alive power domain, as the source of ARM arch/generic > timer, we add a base addr for all SoCs so that we can init with a common > function. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/mach-rockchip/Kconfig | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm/mach-rockchip/Kconfig > b/arch/arm/mach-rockchip/Kconfig > index 007cb22..5dfe452 100644 > --- a/arch/arm/mach-rockchip/Kconfig > +++ b/arch/arm/mach-rockchip/Kconfig > @@ -190,6 +190,25 @@ config ROCKCHIP_BOOT_MODE_REG > The Soc will enter to different boot mode(defined in > asm/arch/boot_mode.h) > according to the value from this register. > > +config ROCKCHIP_STIMER_BASE > + hex "Rockchip Secure timer base address" > + default 0xff220020 if ROCKCHIP_PX30 > + default 0x200440a0 if ROCKCHIP_RK3036 > + default 0x2000e000 if ROCKCHIP_RK3066 > + default 0x20018020 if ROCKCHIP_RK3126 > + default 0x200440a0 if ROCKCHIP_RK3128 > + default 0x2000e000 if ROCKCHIP_RK3188 > + default 0x110d0020 if ROCKCHIP_RK322X > + default 0xff810020 if ROCKCHIP_RK3288 > + default 0xff1d0020 if ROCKCHIP_RK3328 > + default 0xff830020 if ROCKCHIP_RK3368 > + default 0xff8680a0 if ROCKCHIP_RK3399 > + default 0x10350020 if ROCKCHIP_RV1108 > + default 0 > + help > + The secure timer inited in SPL/TPL in secure word, ARM generic > timer > + works after this timer work. > + > config ROCKCHIP_SPL_RESERVE_IRAM > hex "Size of IRAM reserved in SPL" > default 0 > -- > 1.9.1 > >
Hi U-Boot Maintainers: The use-case is: - A register address is needed for a function init, eg. stimer init, boot mode detect; - It's a per-SoC config, the address is not the same for different SoCs; - The driver can be just the same other than the address itself is different; - The address will be used very early in SPL/TPL; There is no no effective communication other than the requirement from the maintainer in the whole passed *YEAR*, while the requirement does not make sense to this case. And no matter how I explain why I implement the patch in this format over and over, there is no response to my explanation but only the same 'change request'. I have no experience on mainline U-Boot maintain, but I do maintain a lot of other project, and I have to convince others and get agreement if I ask for implement in my way. It has pending for such a loooooooong time, and I would like to close this topic and update all related source code, you can find the previous discussion at [0][1]. Philipp object this patch and his original reason is: > This should not be in Kconfig and rather in a header-file. > With what you do here, a user (e.g. via 'make menuconfig') or a defconfig > file (e.g. due to a typo) could accidentially change overwrite this. and Philipp also think: > asm/arch-rockchip would be preferable over include/config as a header There are many ways for feature configure on rockchip platform now: - board dts; - header file in 'include/config' - Kconfig in $(BOARD)_defconfig - Kconfig default value in 'Kconfig' file Already too many place to config, we should make it simple rather than complex. I think people are migrating more and more configurations from header file to Kconfig, including: - option for enable/disable modules; - option for module parameter which is numerical value; - option for module parameter which is string; I think the target is that most of the configs goes to Kconfig and user can config options with menuconfig and no need to touch header file(Please tell me if this is not true). And for all those config options, I think at lease can be separate into two kind: - options per-board; - options per-soc (some of them may per-vendor); I think the idealized model would be per-soc options goes to 'Kconfig' file with default value combine with SOC, and per-board options at $(BOARD)_defconfig. ROCKCHIP_STIMER_REG(another case is BOOT_MODE_REG) is per-soc config option, just like SYS_TEXT_BASE, TPL_TEXT_BASE, TPL_MAX_SIZE, SYS_SOC and so on. Driver for different SOCs to use this reg are just the same, and this reg needs to be used very early in TPL/SPL which means no DM, no dts available. Here are the solutions from previous discussion: - dts (by Simon) extra dtb code needed, setting separate into individual board dts; not available and not suggestion to use in TPL/SPL; - move to header in 'asm/arch-rockchip' (by Philipp) one more place to config, make config options more complicate; may separate in more than 10+ files for different soc/board; - RMII setting like, (by Philipp) reference to 'drivers/net/gmac_rockchip.c', driver for different SoCs are not re-usable, driver needs to update for each new SoC support; not available with DM in early TPL/SPL; - Kconfig with default value in this patch Keep driver clean and no need to update with new SoC; One place rather than 10+ individual files, same usage like SYS_TEXT_BASE, reference to: 341c058654 sunxi: move CONFIG_SYS_TEXT_BASE out of defconfigs I think the default value in Kconfig is the best solution, the SPL/TPL should be small, fast and with enough functionality for use. Thanks, - Kever [0] https://patchwork.ozlabs.org/patch/1004148/ [1] http://patchwork.ozlabs.org/patch/891462/ Kever Yang <kever.yang@rock-chips.com <mailto:kever.yang@rock-chips.com>> 于2018年3月27日周二 下午5:30写道： STIMER is can only access in secure mode if the SoCs supports trust, and it locate in alive power domain, as the source of ARM arch/generic timer, we add a base addr for all SoCs so that we can init with a common function. Signed-off-by: Kever Yang <kever.yang@rock-chips.com <mailto:kever.yang@rock-chips.com>> --- arch/arm/mach-rockchip/Kconfig | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig index 007cb22..5dfe452 100644 --- a/arch/arm/mach-rockchip/Kconfig +++ b/arch/arm/mach-rockchip/Kconfig @@ -190,6 +190,25 @@ config ROCKCHIP_BOOT_MODE_REG The Soc will enter to different boot mode(defined in asm/arch/boot_mode.h) according to the value from this register. +config ROCKCHIP_STIMER_BASE + hex "Rockchip Secure timer base address" + default 0xff220020 if ROCKCHIP_PX30 + default 0x200440a0 if ROCKCHIP_RK3036 + default 0x2000e000 if ROCKCHIP_RK3066 + default 0x20018020 if ROCKCHIP_RK3126 + default 0x200440a0 if ROCKCHIP_RK3128 + default 0x2000e000 if ROCKCHIP_RK3188 + default 0x110d0020 if ROCKCHIP_RK322X + default 0xff810020 if ROCKCHIP_RK3288 + default 0xff1d0020 if ROCKCHIP_RK3328 + default 0xff830020 if ROCKCHIP_RK3368 + default 0xff8680a0 if ROCKCHIP_RK3399 + default 0x10350020 if ROCKCHIP_RV1108 + default 0 + help + The secure timer inited in SPL/TPL in secure word, ARM generic timer + works after this timer work. + config ROCKCHIP_SPL_RESERVE_IRAM hex "Size of IRAM reserved in SPL" default 0 -- 1.9.1
On 03/29/2019 09:02 AM, Kever Yang wrote: > > Here are the solutions from previous discussion: > - dts (by Simon) > extra dtb code needed, setting separate into individual board dts; > not available and not suggestion to use in TPL/SPL; > - move to header in 'asm/arch-rockchip' (by Philipp) > one more place to config, make config options more complicate; > may separate in more than 10+ files for different soc/board; > - RMII setting like, (by Philipp) > reference to 'drivers/net/gmac_rockchip.c', driver for different > SoCs are not re-usable, driver needs to update for each new SoC support; > not available with DM in early TPL/SPL; > - Kconfig with default value in this patch > Keep driver clean and no need to update with new SoC; > One place rather than 10+ individual files, same usage like > SYS_TEXT_BASE, reference to: > 341c058654 sunxi: move CONFIG_SYS_TEXT_BASE out of defconfigs If you are not happy with the format which all soc reg address defined in the same place, one more solution: How about using the format like TPL_TEXT_BASE followed by each SoC and system will report error if not defined: 140 if ROCKCHIP_RK3368 141 142 config TPL_TEXT_BASE 143 default 0xff8c1000 144 145 config TPL_MAX_SIZE 146 default 28672 147 148 config TPL_STACK 149 default 0xff8cffff 150 151 endif Thanks, - Kever
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig index 007cb22..5dfe452 100644 --- a/arch/arm/mach-rockchip/Kconfig +++ b/arch/arm/mach-rockchip/Kconfig @@ -190,6 +190,25 @@ config ROCKCHIP_BOOT_MODE_REG The Soc will enter to different boot mode(defined in asm/arch/boot_mode.h) according to the value from this register. +config ROCKCHIP_STIMER_BASE + hex "Rockchip Secure timer base address" + default 0xff220020 if ROCKCHIP_PX30 + default 0x200440a0 if ROCKCHIP_RK3036 + default 0x2000e000 if ROCKCHIP_RK3066 + default 0x20018020 if ROCKCHIP_RK3126 + default 0x200440a0 if ROCKCHIP_RK3128 + default 0x2000e000 if ROCKCHIP_RK3188 + default 0x110d0020 if ROCKCHIP_RK322X + default 0xff810020 if ROCKCHIP_RK3288 + default 0xff1d0020 if ROCKCHIP_RK3328 + default 0xff830020 if ROCKCHIP_RK3368 + default 0xff8680a0 if ROCKCHIP_RK3399 + default 0x10350020 if ROCKCHIP_RV1108 + default 0 + help + The secure timer inited in SPL/TPL in secure word, ARM generic timer + works after this timer work. + config ROCKCHIP_SPL_RESERVE_IRAM hex "Size of IRAM reserved in SPL" default 0
STIMER is can only access in secure mode if the SoCs supports trust, and it locate in alive power domain, as the source of ARM arch/generic timer, we add a base addr for all SoCs so that we can init with a common function. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- arch/arm/mach-rockchip/Kconfig | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)