diff mbox series

[v2,1/1] platform: implement K210 system reset

Message ID 20210226011957.19830-1-xypron.glpk@gmx.de
State Superseded
Headers show
Series [v2,1/1] platform: implement K210 system reset | expand

Commit Message

Heinrich Schuchardt Feb. 26, 2021, 1:19 a.m. UTC 
Implement rebooting the K210 via the system reset extension.

All reset types are treated in the same way.
A request for shutdown results in a reboot.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
---
v2:
	use a constant for the reset bit mask
---
 platform/kendryte/k210/platform.c | 19 +++++++++++++++++++
 platform/kendryte/k210/platform.h |  9 +++++++++
 2 files changed, 28 insertions(+)

--
2.30.0

Comments

Jessica Clarke Feb. 26, 2021, 1:28 a.m. UTC | #1 
On 26 Feb 2021, at 01:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> Implement rebooting the K210 via the system reset extension.
> 
> All reset types are treated in the same way.
> A request for shutdown results in a reboot.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> v2:
> 	use a constant for the reset bit mask
> ---
> platform/kendryte/k210/platform.c | 19 +++++++++++++++++++
> platform/kendryte/k210/platform.h |  9 +++++++++
> 2 files changed, 28 insertions(+)
> 
> diff --git a/platform/kendryte/k210/platform.c b/platform/kendryte/k210/platform.c
> index 944b388..247cb86 100644
> --- a/platform/kendryte/k210/platform.c
> +++ b/platform/kendryte/k210/platform.c
> @@ -129,6 +129,22 @@ static int k210_timer_init(bool cold_boot)
> 	return clint_warm_timer_init();
> }
> 
> +static int k210_system_reset_check(u32 type, u32 reason)
> +{
> +	return 1;
> +}
> +
> +static void k210_system_reset(u32 type, u32 reason)
> +{
> +	u32 val;
> +
> +	val = k210_read_sysreg(K210_RESET);
> +	val |= K210_RESET_MASK;
> +	k210_write_sysreg(val, K210_RESET);
> +
> +	while(1);

Formatting requires a space.

> +}
> +

It strikes me that this is just a kind of syscons-reset device. Should
we not be implementing that slightly more generic driver and have the
k210 use a suitable device tree (or hard-code it if you really want)?

> const struct sbi_platform_operations platform_ops = {
> 	.final_init	= k210_final_init,
> 
> @@ -142,6 +158,9 @@ const struct sbi_platform_operations platform_ops = {
> 	.ipi_send  = clint_ipi_send,
> 	.ipi_clear = clint_ipi_clear,
> 
> +	.system_reset_check = k210_system_reset_check,
> +	.system_reset = k210_system_reset,

The ='s are generally kept aligned within blocks.

Jess

> +
> 	.timer_init	   = k210_timer_init,
> 	.timer_value	   = clint_timer_value,
> 	.timer_event_stop  = clint_timer_event_stop,
> diff --git a/platform/kendryte/k210/platform.h b/platform/kendryte/k210/platform.h
> index 5269bc4..e425faf 100644
> --- a/platform/kendryte/k210/platform.h
> +++ b/platform/kendryte/k210/platform.h
> @@ -27,10 +27,19 @@
> /* Registers */
> #define K210_PLL0		0x08
> #define K210_CLKSEL0		0x20
> +#define K210_RESET		0x30
> +
> +/* Register bit masks */
> +#define K210_RESET_MASK		0x01
> 
> static inline u32 k210_read_sysreg(u32 reg)
> {
> 	return readl((volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
> }
> 
> +static inline void k210_write_sysreg(u32 val, u32 reg)
> +{
> +	writel(val, (volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
> +}
> +
> #endif /* _K210_PLATFORM_H_ */
> --
> 2.30.0
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Damien Le Moal Feb. 26, 2021, 1:59 a.m. UTC | #2 
On 2021/02/26 10:28, Jessica Clarke wrote:
> On 26 Feb 2021, at 01:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Implement rebooting the K210 via the system reset extension.
>>
>> All reset types are treated in the same way.
>> A request for shutdown results in a reboot.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>> v2:
>> 	use a constant for the reset bit mask
>> ---
>> platform/kendryte/k210/platform.c | 19 +++++++++++++++++++
>> platform/kendryte/k210/platform.h |  9 +++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/platform/kendryte/k210/platform.c b/platform/kendryte/k210/platform.c
>> index 944b388..247cb86 100644
>> --- a/platform/kendryte/k210/platform.c
>> +++ b/platform/kendryte/k210/platform.c
>> @@ -129,6 +129,22 @@ static int k210_timer_init(bool cold_boot)
>> 	return clint_warm_timer_init();
>> }
>>
>> +static int k210_system_reset_check(u32 type, u32 reason)
>> +{
>> +	return 1;
>> +}
>> +
>> +static void k210_system_reset(u32 type, u32 reason)
>> +{
>> +	u32 val;
>> +
>> +	val = k210_read_sysreg(K210_RESET);
>> +	val |= K210_RESET_MASK;
>> +	k210_write_sysreg(val, K210_RESET);
>> +
>> +	while(1);
> 
> Formatting requires a space.
> 
>> +}
>> +
> 
> It strikes me that this is just a kind of syscons-reset device. Should
> we not be implementing that slightly more generic driver and have the
> k210 use a suitable device tree (or hard-code it if you really want)?

Device tree for this using generic syscon-reboot is queued on the kernel side
for 5.12. That can be reused, but there are many dependencies on other drivers
though (clock and sysctl). A syscon-reboot like generic driver in opensbi would
be good too, as long as there is more than one user. Not sure if other platform
in opensbi can benefit from it. This exercise can be left for another patch I think.

> 
>> const struct sbi_platform_operations platform_ops = {
>> 	.final_init	= k210_final_init,
>>
>> @@ -142,6 +158,9 @@ const struct sbi_platform_operations platform_ops = {
>> 	.ipi_send  = clint_ipi_send,
>> 	.ipi_clear = clint_ipi_clear,
>>
>> +	.system_reset_check = k210_system_reset_check,
>> +	.system_reset = k210_system_reset,
> 
> The ='s are generally kept aligned within blocks.
> 
> Jess
> 
>> +
>> 	.timer_init	   = k210_timer_init,
>> 	.timer_value	   = clint_timer_value,
>> 	.timer_event_stop  = clint_timer_event_stop,
>> diff --git a/platform/kendryte/k210/platform.h b/platform/kendryte/k210/platform.h
>> index 5269bc4..e425faf 100644
>> --- a/platform/kendryte/k210/platform.h
>> +++ b/platform/kendryte/k210/platform.h
>> @@ -27,10 +27,19 @@
>> /* Registers */
>> #define K210_PLL0		0x08
>> #define K210_CLKSEL0		0x20
>> +#define K210_RESET		0x30
>> +
>> +/* Register bit masks */
>> +#define K210_RESET_MASK		0x01
>>
>> static inline u32 k210_read_sysreg(u32 reg)
>> {
>> 	return readl((volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
>> }
>>
>> +static inline void k210_write_sysreg(u32 val, u32 reg)
>> +{
>> +	writel(val, (volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
>> +}
>> +
>> #endif /* _K210_PLATFORM_H_ */
>> --
>> 2.30.0
>>
>>
>> -- 
>> opensbi mailing list
>> opensbi@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
> 
>
Heinrich Schuchardt Feb. 26, 2021, 9:19 a.m. UTC | #3 
On 2/26/21 2:59 AM, Damien Le Moal wrote:
> On 2021/02/26 10:28, Jessica Clarke wrote:
>> On 26 Feb 2021, at 01:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> Implement rebooting the K210 via the system reset extension.
>>>
>>> All reset types are treated in the same way.
>>> A request for shutdown results in a reboot.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
>>> ---
>>> v2:
>>> 	use a constant for the reset bit mask
>>> ---
>>> platform/kendryte/k210/platform.c | 19 +++++++++++++++++++
>>> platform/kendryte/k210/platform.h |  9 +++++++++
>>> 2 files changed, 28 insertions(+)
>>>
>>> diff --git a/platform/kendryte/k210/platform.c b/platform/kendryte/k210/platform.c
>>> index 944b388..247cb86 100644
>>> --- a/platform/kendryte/k210/platform.c
>>> +++ b/platform/kendryte/k210/platform.c
>>> @@ -129,6 +129,22 @@ static int k210_timer_init(bool cold_boot)
>>> 	return clint_warm_timer_init();
>>> }
>>>
>>> +static int k210_system_reset_check(u32 type, u32 reason)
>>> +{
>>> +	return 1;
>>> +}
>>> +
>>> +static void k210_system_reset(u32 type, u32 reason)
>>> +{
>>> +	u32 val;
>>> +
>>> +	val = k210_read_sysreg(K210_RESET);
>>> +	val |= K210_RESET_MASK;
>>> +	k210_write_sysreg(val, K210_RESET);
>>> +
>>> +	while(1);
>>
>> Formatting requires a space.

Thank you for reviewing.

>>
>>> +}
>>> +
>>
>> It strikes me that this is just a kind of syscons-reset device. Should
>> we not be implementing that slightly more generic driver and have the
>> k210 use a suitable device tree (or hard-code it if you really want)?

Yes, this is a syscon reboot. The relevant part of the device tree is:

syscon@50440000 {
         compatible = "kendryte,k210-sysctl", "syscon", "simple-mfd";
         reg = <0x50440000 0x00000100>;
         reg-io-width = <0x00000004>;
         phandle = <0x0000000b>;
         reboot {
                 compatible = "syscon-reboot";
                 regmap = <0x0000000b>;
                 offset = <0x00000030>;
                 mask = <0x00000001>;
                 value = <0x00000001>;
         };
};

Implementation of a driver would require to write library code first for
handling regmaps and reg-io-widths.

>
> Device tree for this using generic syscon-reboot is queued on the kernel side
> for 5.12. That can be reused, but there are many dependencies on other drivers
> though (clock and sysctl). A syscon-reboot like generic driver in opensbi would
> be good too, as long as there is more than one user. Not sure if other platform
> in opensbi can benefit from it. This exercise can be left for another patch I think.

Currently I do not see that a device-tree based driver would simplify
the coding.

I will resend the patch with the requested formatting changes.

Best regards

Heinrich

>
>>
>>> const struct sbi_platform_operations platform_ops = {
>>> 	.final_init	= k210_final_init,
>>>
>>> @@ -142,6 +158,9 @@ const struct sbi_platform_operations platform_ops = {
>>> 	.ipi_send  = clint_ipi_send,
>>> 	.ipi_clear = clint_ipi_clear,
>>>
>>> +	.system_reset_check = k210_system_reset_check,
>>> +	.system_reset = k210_system_reset,
>>
>> The ='s are generally kept aligned within blocks.
>>
>> Jess
>>
>>> +
>>> 	.timer_init	   = k210_timer_init,
>>> 	.timer_value	   = clint_timer_value,
>>> 	.timer_event_stop  = clint_timer_event_stop,
>>> diff --git a/platform/kendryte/k210/platform.h b/platform/kendryte/k210/platform.h
>>> index 5269bc4..e425faf 100644
>>> --- a/platform/kendryte/k210/platform.h
>>> +++ b/platform/kendryte/k210/platform.h
>>> @@ -27,10 +27,19 @@
>>> /* Registers */
>>> #define K210_PLL0		0x08
>>> #define K210_CLKSEL0		0x20
>>> +#define K210_RESET		0x30
>>> +
>>> +/* Register bit masks */
>>> +#define K210_RESET_MASK		0x01
>>>
>>> static inline u32 k210_read_sysreg(u32 reg)
>>> {
>>> 	return readl((volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
>>> }
>>>
>>> +static inline void k210_write_sysreg(u32 val, u32 reg)
>>> +{
>>> +	writel(val, (volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
>>> +}
>>> +
>>> #endif /* _K210_PLATFORM_H_ */
>>> --
>>> 2.30.0
>>>
>>>
>>> --
>>> opensbi mailing list
>>> opensbi@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>
>>
>
>
diff mbox series

Patch

diff --git a/platform/kendryte/k210/platform.c b/platform/kendryte/k210/platform.c
index 944b388..247cb86 100644
--- a/platform/kendryte/k210/platform.c
+++ b/platform/kendryte/k210/platform.c
@@ -129,6 +129,22 @@  static int k210_timer_init(bool cold_boot)
 	return clint_warm_timer_init();
 }

+static int k210_system_reset_check(u32 type, u32 reason)
+{
+	return 1;
+}
+
+static void k210_system_reset(u32 type, u32 reason)
+{
+	u32 val;
+
+	val = k210_read_sysreg(K210_RESET);
+	val |= K210_RESET_MASK;
+	k210_write_sysreg(val, K210_RESET);
+
+	while(1);
+}
+
 const struct sbi_platform_operations platform_ops = {
 	.final_init	= k210_final_init,

@@ -142,6 +158,9 @@  const struct sbi_platform_operations platform_ops = {
 	.ipi_send  = clint_ipi_send,
 	.ipi_clear = clint_ipi_clear,

+	.system_reset_check = k210_system_reset_check,
+	.system_reset = k210_system_reset,
+
 	.timer_init	   = k210_timer_init,
 	.timer_value	   = clint_timer_value,
 	.timer_event_stop  = clint_timer_event_stop,
diff --git a/platform/kendryte/k210/platform.h b/platform/kendryte/k210/platform.h
index 5269bc4..e425faf 100644
--- a/platform/kendryte/k210/platform.h
+++ b/platform/kendryte/k210/platform.h
@@ -27,10 +27,19 @@ 
 /* Registers */
 #define K210_PLL0		0x08
 #define K210_CLKSEL0		0x20
+#define K210_RESET		0x30
+
+/* Register bit masks */
+#define K210_RESET_MASK		0x01

 static inline u32 k210_read_sysreg(u32 reg)
 {
 	return readl((volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
 }

+static inline void k210_write_sysreg(u32 val, u32 reg)
+{
+	writel(val, (volatile void *)(K210_SYSCTL_BASE_ADDR + reg));
+}
+
 #endif /* _K210_PLATFORM_H_ */