[U-Boot] [RESENT PATCH 1/2] rockchip: rk3128: use ROCKCHIP_BOOT_MODE_REG to update reboot flag
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@rock-chips.com ---
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 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; }
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@rock-chips.com ---
arch/arm/mach-rockchip/rk322x-board.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/arm/mach-rockchip/rk322x-board.c b/arch/arm/mach-rockchip/rk322x-board.c index 7366d45ab6..61863d7424 100644 --- a/arch/arm/mach-rockchip/rk322x-board.c +++ b/arch/arm/mach-rockchip/rk322x-board.c @@ -142,12 +142,9 @@ int board_usb_cleanup(int index, enum usb_init_type init) #if CONFIG_IS_ENABLED(FASTBOOT) int fastboot_set_reboot_flag(void) { - struct rk322x_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; }
Kever,
On 28.11.2018, at 03:04, Kever Yang kever.yang@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@rock-chips.com
NAK, as there are still pending changes. 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).
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
Hi Philipp,
On 11/28/2018 05:07 PM, Philipp Tomsich wrote:
Kever,
On 28.11.2018, at 03:04, Kever Yang kever.yang@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@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? 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
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang kever.yang@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@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@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?
Regards, Simon
Simon,
On 29.11.2018, at 19:43, Simon Glass sjg@chromium.org wrote:
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang <kever.yang@rock-chips.com mailto:kever.yang@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@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@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.
Thanks, Philipp.
Hi Philipp,
On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 29.11.2018, at 19:43, Simon Glass sjg@chromium.org wrote:
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang kever.yang@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@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@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.
Regards, Simon
Simon,
On 06.12.2018, at 02:31, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 29.11.2018, at 19:43, Simon Glass sjg@chromium.org wrote:
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang kever.yang@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@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@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.
Thanks, Philipp.
Hi Philipp,
On Thu, 6 Dec 2018 at 04:02, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 06.12.2018, at 02:31, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 29.11.2018, at 19:43, Simon Glass sjg@chromium.org wrote:
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang kever.yang@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@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@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 - ..
Regards, Simon
On 06.12.2018, at 19:18, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On Thu, 6 Dec 2018 at 04:02, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 06.12.2018, at 02:31, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 29.11.2018, at 19:43, Simon Glass sjg@chromium.org wrote:
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang kever.yang@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@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@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.
Hi Philipp,
On Thu, 6 Dec 2018 at 11:23, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 06.12.2018, at 19:18, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On Thu, 6 Dec 2018 at 04:02, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 06.12.2018, at 02:31, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On Thu, 29 Nov 2018 at 13:57, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Simon,
On 29.11.2018, at 19:43, Simon Glass sjg@chromium.org wrote:
Hi Kever,
On Wed, 28 Nov 2018 at 18:10, Kever Yang kever.yang@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@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@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
Kever,
On 29.11.2018, at 02:10, Kever Yang kever.yang@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.yang@rock-chips.com mailto:kever.yang@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@rock-chips.com mailto:kever.yang@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
participants (3)
-
Kever Yang -
Philipp Tomsich -
Simon Glass