[RFC PATCH v5 0/2] arch: riscv: cpu: Add callback to init each core

[RFC PATCH v5 0/2] arch: riscv: cpu: Add callback to init each core

[Green Wan]

Add a callback interface, harts_early_init() to perform riscv CPU 
hart-dependent configuration for each hart. harts_early_init() is
placed after stack of harts are initialized and before main boot hart is
picked up. Several conditions should be aware of or avoided are listed:

  - cannot access gd
    At the moment, gd hasn't initialized yet.

  - all operations in harts_early_init() should only affect core itself
    For example, the operations for board level should be moved to mach_xxx()
    or board_init_xxx() functions instead.

  - Common resource need protection if multicore
    Since all harts might enter harts_early_init() in parallel, common
    resource need a mechanism to handle race condition.

  - A reference implementation is added in ./arch/riscv/cpu/cpu.c
    The implementation is declared with '__weak' and can be overridden.

Changelogs:
v5
  - rename riscv_hart_early_init to harts_early_init
  - rephrase dummy for harts_early_init() to callback but keep current
    comments not specific for customizing CSRs only.
  - Fix nit in [2/2] and add reviewed-by
v4
  - Revising the comments for riscv_hart_early_init() in [1/2]
  - Remove unnecessary braces and add reviewed-by in [2/2]

Green Wan (2):
  arch: riscv: cpu: Add callback to init each core
  board: sifive: unmatched: clear feature disable CSR

 arch/riscv/cpu/cpu.c         | 15 +++++++++++++++
 arch/riscv/cpu/start.S       | 14 ++++++++++++++
 board/sifive/unmatched/spl.c | 15 +++++++++++++++
 3 files changed, 44 insertions(+)

-- 
2.31.0

[RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core

[Green Wan]

Add a callback harts_early_init() to ./arch/riscv/cpu/start.S to
allow different riscv hart perform setup code for each hart as early
as possible. Since all the harts enter the callback, they must be able
to run the same setup.

Signed-off-by: Green Wan <green.wan@sifive.com>
---
 arch/riscv/cpu/cpu.c   | 15 +++++++++++++++
 arch/riscv/cpu/start.S | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index 85592f5bee..b2b49812c4 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -140,3 +140,18 @@ int arch_early_init_r(void)
 {
 	return riscv_cpu_probe();
 }
+
+/**
+ * harts_early_init() - A callback function called by
+ * ./arch/riscv/cpu/start.S to allow to disable/enable features of each
+ * core. For example, turn on or off the functional block of CPU harts.
+ *
+ * In a multi-core system, this function must not access shared resources.
+ *
+ * Any access to such resources would probably be better done with
+ * available_harts_lock held. However, I doubt that any such access will be
+ * necessary.
+ */
+__weak void harts_early_init(void)
+{
+}
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 8589509e01..a481102960 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -117,6 +117,18 @@ call_board_init_f_0:
 	mv	sp, a0
 #endif
 
+#if CONFIG_IS_ENABLED(RISCV_MMODE)
+	/*
+	 * Jump to harts_early_init() to perform init for each core. Not
+	 * expect to access gd since gd is not initialized. All operations in the
+	 * function should affect core itself only. In multi-core system, any access
+	 * to common resource or registers outside core should be avoided or need a
+	 * protection for multicore.
+	 */
+call_harts_early_init:
+	jal	harts_early_init
+#endif
+
 #ifndef CONFIG_XIP
 	/*
 	 * Pick hart to initialize global data and run U-Boot. The other harts
-- 
2.31.0

[RFC PATCH v5 2/2] board: sifive: unmatched: clear feature disable CSR

[Green Wan]

Clear feature disable CSR to turn on all features of hart. The detail
is specified at section, 'SiFive Feature Disable CSR', in user manual

https://sifive.cdn.prismic.io/sifive/aee0dd4c-d156-496e-a6c4-db0cf54bbe68_sifive_U74MC_rtl_full_20G1.03.00_manual.pdf

Signed-off-by: Green Wan <green.wan@sifive.com>
Reviewed-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 board/sifive/unmatched/spl.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/board/sifive/unmatched/spl.c b/board/sifive/unmatched/spl.c
index 5e1333b09a..ed48dac511 100644
--- a/board/sifive/unmatched/spl.c
+++ b/board/sifive/unmatched/spl.c
@@ -12,6 +12,7 @@
 #include <log.h>
 #include <linux/delay.h>
 #include <linux/io.h>
+#include <asm/csr.h>
 #include <asm/gpio.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/spl.h>
@@ -22,6 +23,20 @@
 #define MODE_SELECT_SD		0xb
 #define MODE_SELECT_MASK	GENMASK(3, 0)
 
+#define CSR_U74_FEATURE_DISABLE	0x7c1
+
+void harts_early_init(void)
+{
+	/*
+	 * Feature Disable CSR
+	 *
+	 * Clear feature disable CSR to '0' to turn on all features for
+	 * each core. This operation must be in M-mode.
+	 */
+	if (CONFIG_IS_ENABLED(RISCV_MMODE))
+		csr_write(CSR_U74_FEATURE_DISABLE, 0);
+}
+
 int spl_board_init_f(void)
 {
 	int ret;
-- 
2.31.0

[RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core

[Rick Chen]

> From: Green Wan [mailto:green.wan at sifive.com]
> Sent: Tuesday, April 13, 2021 5:32 PM
> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Sean Anderson; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; open list
> Subject: [RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core
>
> Add a callback harts_early_init() to ./arch/riscv/cpu/start.S to

Please don't describe the file path because in the following log it
can be found obviously.
or you can reword as:
Add a callback harts_early_init() to start.S

> allow different riscv hart perform setup code for each hart as early
> as possible. Since all the harts enter the callback, they must be able
> to run the same setup.
>
> Signed-off-by: Green Wan <green.wan@sifive.com>
> ---
>  arch/riscv/cpu/cpu.c   | 15 +++++++++++++++
>  arch/riscv/cpu/start.S | 12 ++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> index 85592f5bee..b2b49812c4 100644
> --- a/arch/riscv/cpu/cpu.c
> +++ b/arch/riscv/cpu/cpu.c
> @@ -140,3 +140,18 @@ int arch_early_init_r(void)
>  {
>         return riscv_cpu_probe();
>  }
> +
> +/**
> + * harts_early_init() - A callback function called by
> + * ./arch/riscv/cpu/start.S to allow to disable/enable features of each

Don't describe the called path.
_weak function is a common usage in U-Boot, if someone grep the U-Boot
and will know how to use this callback.

allow to disable/enable features ...
it shall be reword as:
configure feature settings of each hart (Because someone maybe not
just enable or disable features but need to adjust the default value
or somewhat)

> + * core. For example, turn on or off the functional block of CPU harts.

Same meaning as disable/enable features of each hart. It is
unnecessary to describe again.

> + *
> + * In a multi-core system, this function must not access shared resources.
> + *
> + * Any access to such resources would probably be better done with
> + * available_harts_lock held. However, I doubt that any such access will be

The point is atomic instruction but not the available_harts_lock, it
is only a variable allocated in memory.
You may describe that:
Memory access shall be careful here, it shall take care race conditions.

> + * necessary.
> + */
> +__weak void harts_early_init(void)
> +{
> +}
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 8589509e01..a481102960 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -117,6 +117,18 @@ call_board_init_f_0:
>         mv      sp, a0
>  #endif
>
> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> +       /*
> +        * Jump to harts_early_init() to perform init for each core. Not
> +        * expect to access gd since gd is not initialized. All operations in the
> +        * function should affect core itself only. In multi-core system, any access
> +        * to common resource or registers outside core should be avoided or need a
> +        * protection for multicore.
> +        */

Don't describe too much duplicate words for harts_early_init in
start.S and cpu.c.
Here you just describe this callback can configure not standard or
customized CSRs.
And describe race condition issue of memory access in SMP system for
harts_early_init() in cpu.c

Thanks,
Rick

> +call_harts_early_init:
> +       jal     harts_early_init
> +#endif
> +
>  #ifndef CONFIG_XIP
>         /*
>          * Pick hart to initialize global data and run U-Boot. The other harts
> --
> 2.31.0

[RFC PATCH v5 2/2] board: sifive: unmatched: clear feature disable CSR

[Rick Chen]

Hi Green,

> From: Green Wan [mailto:green.wan at sifive.com]
> Sent: Tuesday, April 13, 2021 5:32 PM
> Cc: Green Wan; Sean Anderson; Bin Meng; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; open list
> Subject: [RFC PATCH v5 2/2] board: sifive: unmatched: clear feature disable CSR
>
> Clear feature disable CSR to turn on all features of hart. The detail
> is specified at section, 'SiFive Feature Disable CSR', in user manual
>
> https://sifive.cdn.prismic.io/sifive/aee0dd4c-d156-496e-a6c4-db0cf54bbe68_sifive_U74MC_rtl_full_20G1.03.00_manual.pdf
>
> Signed-off-by: Green Wan <green.wan@sifive.com>
> Reviewed-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  board/sifive/unmatched/spl.c | 15 +++++++++++++++

Is this CSR depends on unmatched board ?
I just wonder why not add in /arch/riscv/cpu/ and create fu74, just
like /arch/riscv/cpu/fu540/spl.c ?

Thanks,
Rick

>  1 file changed, 15 insertions(+)
>
> diff --git a/board/sifive/unmatched/spl.c b/board/sifive/unmatched/spl.c
> index 5e1333b09a..ed48dac511 100644
> --- a/board/sifive/unmatched/spl.c
> +++ b/board/sifive/unmatched/spl.c
> @@ -12,6 +12,7 @@
>  #include <log.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> +#include <asm/csr.h>
>  #include <asm/gpio.h>
>  #include <asm/arch/gpio.h>
>  #include <asm/arch/spl.h>
> @@ -22,6 +23,20 @@
>  #define MODE_SELECT_SD         0xb
>  #define MODE_SELECT_MASK       GENMASK(3, 0)
>
> +#define CSR_U74_FEATURE_DISABLE        0x7c1
> +
> +void harts_early_init(void)
> +{
> +       /*
> +        * Feature Disable CSR
> +        *
> +        * Clear feature disable CSR to '0' to turn on all features for
> +        * each core. This operation must be in M-mode.
> +        */
> +       if (CONFIG_IS_ENABLED(RISCV_MMODE))
> +               csr_write(CSR_U74_FEATURE_DISABLE, 0);
> +}
> +
>  int spl_board_init_f(void)
>  {
>         int ret;
> --
> 2.31.0

[RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core

[Green Wan]

Thanks, Rick,

It's very helpful. I'll fix them.


On Wed, Apr 14, 2021 at 11:07 AM Rick Chen <rickchen36@gmail.com> wrote:
>
> > From: Green Wan [mailto:green.wan at sifive.com]
> > Sent: Tuesday, April 13, 2021 5:32 PM
> > Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Sean Anderson; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; open list
> > Subject: [RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core
> >
> > Add a callback harts_early_init() to ./arch/riscv/cpu/start.S to
>
> Please don't describe the file path because in the following log it
> can be found obviously.
> or you can reword as:
> Add a callback harts_early_init() to start.S
>
> > allow different riscv hart perform setup code for each hart as early
> > as possible. Since all the harts enter the callback, they must be able
> > to run the same setup.
> >
> > Signed-off-by: Green Wan <green.wan@sifive.com>
> > ---
> >  arch/riscv/cpu/cpu.c   | 15 +++++++++++++++
> >  arch/riscv/cpu/start.S | 12 ++++++++++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > index 85592f5bee..b2b49812c4 100644
> > --- a/arch/riscv/cpu/cpu.c
> > +++ b/arch/riscv/cpu/cpu.c
> > @@ -140,3 +140,18 @@ int arch_early_init_r(void)
> >  {
> >         return riscv_cpu_probe();
> >  }
> > +
> > +/**
> > + * harts_early_init() - A callback function called by
> > + * ./arch/riscv/cpu/start.S to allow to disable/enable features of each
>
> Don't describe the called path.
> _weak function is a common usage in U-Boot, if someone grep the U-Boot
> and will know how to use this callback.
>
> allow to disable/enable features ...
> it shall be reword as:
> configure feature settings of each hart (Because someone maybe not
> just enable or disable features but need to adjust the default value
> or somewhat)
>
> > + * core. For example, turn on or off the functional block of CPU harts.
>
> Same meaning as disable/enable features of each hart. It is
> unnecessary to describe again.
>
> > + *
> > + * In a multi-core system, this function must not access shared resources.
> > + *
> > + * Any access to such resources would probably be better done with
> > + * available_harts_lock held. However, I doubt that any such access will be
>
> The point is atomic instruction but not the available_harts_lock, it
> is only a variable allocated in memory.
> You may describe that:
> Memory access shall be careful here, it shall take care race conditions.
>
> > + * necessary.
> > + */
> > +__weak void harts_early_init(void)
> > +{
> > +}
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 8589509e01..a481102960 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -117,6 +117,18 @@ call_board_init_f_0:
> >         mv      sp, a0
> >  #endif
> >
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > +       /*
> > +        * Jump to harts_early_init() to perform init for each core. Not
> > +        * expect to access gd since gd is not initialized. All operations in the
> > +        * function should affect core itself only. In multi-core system, any access
> > +        * to common resource or registers outside core should be avoided or need a
> > +        * protection for multicore.
> > +        */
>
> Don't describe too much duplicate words for harts_early_init in
> start.S and cpu.c.
> Here you just describe this callback can configure not standard or
> customized CSRs.
> And describe race condition issue of memory access in SMP system for
> harts_early_init() in cpu.c
>
> Thanks,
> Rick
>
> > +call_harts_early_init:
> > +       jal     harts_early_init
> > +#endif
> > +
> >  #ifndef CONFIG_XIP
> >         /*
> >          * Pick hart to initialize global data and run U-Boot. The other harts
> > --
> > 2.31.0

[RFC PATCH v5 2/2] board: sifive: unmatched: clear feature disable CSR

[Green Wan]

On Wed, Apr 14, 2021 at 11:22 AM Rick Chen <rickchen36@gmail.com> wrote:
>
> Hi Green,
>
> > From: Green Wan [mailto:green.wan at sifive.com]
> > Sent: Tuesday, April 13, 2021 5:32 PM
> > Cc: Green Wan; Sean Anderson; Bin Meng; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; open list
> > Subject: [RFC PATCH v5 2/2] board: sifive: unmatched: clear feature disable CSR
> >
> > Clear feature disable CSR to turn on all features of hart. The detail
> > is specified at section, 'SiFive Feature Disable CSR', in user manual
> >
> > https://sifive.cdn.prismic.io/sifive/aee0dd4c-d156-496e-a6c4-db0cf54bbe68_sifive_U74MC_rtl_full_20G1.03.00_manual.pdf
> >
> > Signed-off-by: Green Wan <green.wan@sifive.com>
> > Reviewed-by: Sean Anderson <seanga2@gmail.com>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >  board/sifive/unmatched/spl.c | 15 +++++++++++++++
>
> Is this CSR depends on unmatched board ?
> I just wonder why not add in /arch/riscv/cpu/ and create fu74, just
> like /arch/riscv/cpu/fu540/spl.c ?

You're right. This CSR doesn't depend on the Unmatched. Moving to
arch/riscv/cpu/fu740/spl.c makes more sense. will do it.


>
> Thanks,
> Rick
>
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/board/sifive/unmatched/spl.c b/board/sifive/unmatched/spl.c
> > index 5e1333b09a..ed48dac511 100644
> > --- a/board/sifive/unmatched/spl.c
> > +++ b/board/sifive/unmatched/spl.c
> > @@ -12,6 +12,7 @@
> >  #include <log.h>
> >  #include <linux/delay.h>
> >  #include <linux/io.h>
> > +#include <asm/csr.h>
> >  #include <asm/gpio.h>
> >  #include <asm/arch/gpio.h>
> >  #include <asm/arch/spl.h>
> > @@ -22,6 +23,20 @@
> >  #define MODE_SELECT_SD         0xb
> >  #define MODE_SELECT_MASK       GENMASK(3, 0)
> >
> > +#define CSR_U74_FEATURE_DISABLE        0x7c1
> > +
> > +void harts_early_init(void)
> > +{
> > +       /*
> > +        * Feature Disable CSR
> > +        *
> > +        * Clear feature disable CSR to '0' to turn on all features for
> > +        * each core. This operation must be in M-mode.
> > +        */
> > +       if (CONFIG_IS_ENABLED(RISCV_MMODE))
> > +               csr_write(CSR_U74_FEATURE_DISABLE, 0);
> > +}
> > +
> >  int spl_board_init_f(void)
> >  {
> >         int ret;
> > --
> > 2.31.0