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

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

[Kever Yang]

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;
 }
-- 
2.18.0

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

[Kever Yang]

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;
 }
-- 
2.18.0

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

[Philipp Tomsich]

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
> 

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

[Kever Yang]

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
>>
>

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

[Simon Glass]

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

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

[Philipp Tomsich]

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 at 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 at 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

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

[Philipp Tomsich]

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 at 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.

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

[Simon Glass]

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

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

[Philipp Tomsich]

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.

[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 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

[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@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.

[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@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

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

[Kever Yang]

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;
 }
-- 
2.18.0