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

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

[Green Wan]

Add a callback interface, riscv_hart_early_init() to perform riscv CPU 
hart-dependent configuration for each hart. riscv_hart_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 riscv_hart_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 riscv_hart_early_init() in parallel, common
    resource need a mechanism to handle race condition.

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

Changelogs:
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 v4 1/2] arch: riscv: cpu: Add callback to init each core

[Green Wan]

Add a callback riscv_hart_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 | 14 ++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index 85592f5bee..1652e51137 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();
 }
+
+/**
+ * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
+{
+}
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 8589509e01..ab73008f23 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -117,6 +117,20 @@ call_board_init_f_0:
 	mv	sp, a0
 #endif
 
+#if CONFIG_IS_ENABLED(RISCV_MMODE)
+	/*
+	 * Jump to riscv_hart_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.
+	 *
+	 * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
+	 */
+call_riscv_hart_early_init:
+	jal	riscv_hart_early_init
+#endif
+
 #ifndef CONFIG_XIP
 	/*
 	 * Pick hart to initialize global data and run U-Boot. The other harts
-- 
2.31.0

[RFC PATCH v4 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>
---
 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..2af3069b55 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 riscv_hart_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 v4 1/2] arch: riscv: cpu: Add callback to init each core

[Sean Anderson]

On 3/30/21 1:26 AM, Green Wan wrote:
> Add a callback riscv_hart_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 | 14 ++++++++++++++
>   2 files changed, 29 insertions(+)
> 
> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> index 85592f5bee..1652e51137 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();
>   }
> +
> +/**
> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> +{
> +}
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 8589509e01..ab73008f23 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -117,6 +117,20 @@ call_board_init_f_0:
>   	mv	sp, a0
>   #endif
>   
> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> +	/*
> +	 * Jump to riscv_hart_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.
> +	 *
> +	 * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> +	 */
> +call_riscv_hart_early_init:
> +	jal	riscv_hart_early_init
> +#endif
> +

I wonder if we could move the calls to icache_enable and dcache_enable
into this function. Though this would have the consequence of enabling
caches on all harts for CPUs which previously only enabled them for the
boot hart. I think ax25 is the only CPU which currently does this. Bin,
would this be an issue?

--Sean

>   #ifndef CONFIG_XIP
>   	/*
>   	 * Pick hart to initialize global data and run U-Boot. The other harts
> 

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

[Bin Meng]

On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/30/21 1:26 AM, Green Wan wrote:
> > Add a callback riscv_hart_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 | 14 ++++++++++++++
> >   2 files changed, 29 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > index 85592f5bee..1652e51137 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();
> >   }
> > +
> > +/**
> > + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> > +{
> > +}
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 8589509e01..ab73008f23 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -117,6 +117,20 @@ call_board_init_f_0:
> >       mv      sp, a0
> >   #endif
> >
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > +     /*
> > +      * Jump to riscv_hart_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.
> > +      *
> > +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> > +      */
> > +call_riscv_hart_early_init:
> > +     jal     riscv_hart_early_init
> > +#endif
> > +
>
> I wonder if we could move the calls to icache_enable and dcache_enable
> into this function. Though this would have the consequence of enabling
> caches on all harts for CPUs which previously only enabled them for the
> boot hart. I think ax25 is the only CPU which currently does this. Bin,
> would this be an issue?

Good catch. I believe AX25 cache support is currently somehow broken?

I think Rick has to comment on this as he added this via commit:

commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
Author: Rick Chen <rick@andestech.com>
Date:   Wed Nov 7 09:34:06 2018 +0800

    riscv: cache: Implement i/dcache [status, enable, disable]

    AndeStar RISC-V(V5) provide mcache_ctl register which
    can configure I/D cache as enabled or disabled.

    This CSR will be encapsulated by CONFIG_RISCV_NDS.
    If you want to configure cache on AndeStar V5
    AE350 platform. YOu can enable [*] AndeStar V5 ISA support
    by make menuconfig.

    This approach also provide the expansion when the
    vender specific features are going to join in.

    Signed-off-by: Rick Chen <rick@andestech.com>
    Cc: Greentime Hu <greentime@andestech.com>

The original commit has i/d cache enabled on all harts. But it was
later moved to boot hart due to SMP support.

However on the cleanup side, it looks only the boot hart calls i/d
cache disable?

See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()

Regards,
Bin

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

[Bin Meng]

On Tue, Mar 30, 2021 at 1:27 PM Green Wan <green.wan@sifive.com> wrote:
>
> 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>
> ---
>  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..2af3069b55 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 riscv_hart_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.

nits: M-mode

> +        */
> +       if (CONFIG_IS_ENABLED(RISCV_MMODE))
> +               csr_write(CSR_U74_FEATURE_DISABLE, 0);
> +}

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

[Green Wan]

k, thanks, will fix it.

On Tue, Apr 6, 2021 at 5:17 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 1:27 PM Green Wan <green.wan@sifive.com> wrote:
> >
> > 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>
> > ---
> >  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..2af3069b55 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 riscv_hart_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.
>
> nits: M-mode
>
> > +        */
> > +       if (CONFIG_IS_ENABLED(RISCV_MMODE))
> > +               csr_write(CSR_U74_FEATURE_DISABLE, 0);
> > +}
>
> Otherwise,
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

[Rick Chen]

Hi Sean ,Bin

> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> Sent: Tuesday, April 06, 2021 5:16 PM
> To: Sean Anderson
> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>
> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
> >
> > On 3/30/21 1:26 AM, Green Wan wrote:
> > > Add a callback riscv_hart_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 | 14 ++++++++++++++
> > >   2 files changed, 29 insertions(+)
> > >
> > > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > > index 85592f5bee..1652e51137 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();
> > >   }
> > > +
> > > +/**
> > > + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> > > +{
> > > +}
> > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > > index 8589509e01..ab73008f23 100644
> > > --- a/arch/riscv/cpu/start.S
> > > +++ b/arch/riscv/cpu/start.S
> > > @@ -117,6 +117,20 @@ call_board_init_f_0:
> > >       mv      sp, a0
> > >   #endif
> > >
> > > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > > +     /*
> > > +      * Jump to riscv_hart_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.
> > > +      *
> > > +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> > > +      */
> > > +call_riscv_hart_early_init:
> > > +     jal     riscv_hart_early_init
> > > +#endif
> > > +
> >
> > I wonder if we could move the calls to icache_enable and dcache_enable
> > into this function. Though this would have the consequence of enabling
> > caches on all harts for CPUs which previously only enabled them for the
> > boot hart. I think ax25 is the only CPU which currently does this. Bin,
> > would this be an issue?

No, they are functions shall be called in different stage about lottery.
riscv_hart_early_init() is called before lottery for all harts.
It shall not move icache_enable() and dcache_enable() into
riscv_hart_early_init(), they are belong to the stage after lottery
only for main hart.

>
> Good catch. I believe AX25 cache support is currently somehow broken?

No, AX25 cache support is currently work well.

>
> I think Rick has to comment on this as he added this via commit:
>
> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
> Author: Rick Chen <rick@andestech.com>
> Date:   Wed Nov 7 09:34:06 2018 +0800
>
>     riscv: cache: Implement i/dcache [status, enable, disable]
>
>     AndeStar RISC-V(V5) provide mcache_ctl register which
>     can configure I/D cache as enabled or disabled.
>
>     This CSR will be encapsulated by CONFIG_RISCV_NDS.
>     If you want to configure cache on AndeStar V5
>     AE350 platform. YOu can enable [*] AndeStar V5 ISA support
>     by make menuconfig.
>
>     This approach also provide the expansion when the
>     vender specific features are going to join in.
>
>     Signed-off-by: Rick Chen <rick@andestech.com>
>     Cc: Greentime Hu <greentime@andestech.com>
>
> The original commit has i/d cache enabled on all harts. But it was
> later moved to boot hart due to SMP support.

Not all harts will enable i/d cache during startup currently, only
main hart will enable i/d cache here.
So only main hart will disable i/d cache in cleanup_before_linux().

Thanks,
Rick

>
> However on the cleanup side, it looks only the boot hart calls i/d
> cache disable?

>
> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
>
> Regards,
> Bin

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

[Sean Anderson]

On 4/9/21 4:16 AM, Rick Chen wrote:
> Hi Sean ,Bin
> 
>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
>> Sent: Tuesday, April 06, 2021 5:16 PM
>> To: Sean Anderson
>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>>
>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
>>>
>>> On 3/30/21 1:26 AM, Green Wan wrote:
>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
>>>>    2 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
>>>> index 85592f5bee..1652e51137 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();
>>>>    }
>>>> +
>>>> +/**
>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
>>>> +{
>>>> +}
>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>>>> index 8589509e01..ab73008f23 100644
>>>> --- a/arch/riscv/cpu/start.S
>>>> +++ b/arch/riscv/cpu/start.S
>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
>>>>        mv      sp, a0
>>>>    #endif
>>>>
>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
>>>> +     /*
>>>> +      * Jump to riscv_hart_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.
>>>> +      *
>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
>>>> +      */
>>>> +call_riscv_hart_early_init:
>>>> +     jal     riscv_hart_early_init
>>>> +#endif
>>>> +
>>>
>>> I wonder if we could move the calls to icache_enable and dcache_enable
>>> into this function. Though this would have the consequence of enabling
>>> caches on all harts for CPUs which previously only enabled them for the
>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
>>> would this be an issue?
> 
> No, they are functions shall be called in different stage about lottery.
> riscv_hart_early_init() is called before lottery for all harts.
> It shall not move icache_enable() and dcache_enable() into
> riscv_hart_early_init(), they are belong to the stage after lottery
> only for main hart.
> 
>>
>> Good catch. I believe AX25 cache support is currently somehow broken?
> 
> No, AX25 cache support is currently work well.
> 
>>
>> I think Rick has to comment on this as he added this via commit:
>>
>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
>> Author: Rick Chen <rick@andestech.com>
>> Date:   Wed Nov 7 09:34:06 2018 +0800
>>
>>      riscv: cache: Implement i/dcache [status, enable, disable]
>>
>>      AndeStar RISC-V(V5) provide mcache_ctl register which
>>      can configure I/D cache as enabled or disabled.
>>
>>      This CSR will be encapsulated by CONFIG_RISCV_NDS.
>>      If you want to configure cache on AndeStar V5
>>      AE350 platform. YOu can enable [*] AndeStar V5 ISA support
>>      by make menuconfig.
>>
>>      This approach also provide the expansion when the
>>      vender specific features are going to join in.
>>
>>      Signed-off-by: Rick Chen <rick@andestech.com>
>>      Cc: Greentime Hu <greentime@andestech.com>
>>
>> The original commit has i/d cache enabled on all harts. But it was
>> later moved to boot hart due to SMP support.
> 
> Not all harts will enable i/d cache during startup currently, only
> main hart will enable i/d cache here.
> So only main hart will disable i/d cache in cleanup_before_linux().

Ok, so we have ax25 where cache is disabled on all harts before Linux,
and fu740 where cache will be enabled on all harts. Is there any
guidance from Linux on what should be happening here?

--Sean

> Thanks,
> Rick
> 
>>
>> However on the cleanup side, it looks only the boot hart calls i/d
>> cache disable?
> 
>>
>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
>>
>> Regards,
>> Bin

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

[Green Wan]

Hi folks,

Correct me if I'm wrong, like Rick mentioned, i/dcache
enable/disable() is only called on the main hart. Right now the dummy
i/dcache enable/disable are empty and shared among all riscv CPU. The
ax25 is the only one that has its own implementation for now.

FU540/FU740 also leverages the dummy i/dcache enable/disable()
functions (only main hart calls them). L2 cache on FU540/FU740 is
enabled as SRAM purpose. And according to the HW design behavior, once
L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
L2$ driver will handle that according to the configuration of L2
registers.

[1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf

Thanks,

On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 4/9/21 4:16 AM, Rick Chen wrote:
> > Hi Sean ,Bin
> >
> >> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> >> Sent: Tuesday, April 06, 2021 5:16 PM
> >> To: Sean Anderson
> >> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
> >> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >>
> >> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
> >>>
> >>> On 3/30/21 1:26 AM, Green Wan wrote:
> >>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
> >>>>    2 files changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> >>>> index 85592f5bee..1652e51137 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();
> >>>>    }
> >>>> +
> >>>> +/**
> >>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> >>>> +{
> >>>> +}
> >>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> >>>> index 8589509e01..ab73008f23 100644
> >>>> --- a/arch/riscv/cpu/start.S
> >>>> +++ b/arch/riscv/cpu/start.S
> >>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
> >>>>        mv      sp, a0
> >>>>    #endif
> >>>>
> >>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> >>>> +     /*
> >>>> +      * Jump to riscv_hart_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.
> >>>> +      *
> >>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> >>>> +      */
> >>>> +call_riscv_hart_early_init:
> >>>> +     jal     riscv_hart_early_init
> >>>> +#endif
> >>>> +
> >>>
> >>> I wonder if we could move the calls to icache_enable and dcache_enable
> >>> into this function. Though this would have the consequence of enabling
> >>> caches on all harts for CPUs which previously only enabled them for the
> >>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
> >>> would this be an issue?
> >
> > No, they are functions shall be called in different stage about lottery.
> > riscv_hart_early_init() is called before lottery for all harts.
> > It shall not move icache_enable() and dcache_enable() into
> > riscv_hart_early_init(), they are belong to the stage after lottery
> > only for main hart.
> >
> >>
> >> Good catch. I believe AX25 cache support is currently somehow broken?
> >
> > No, AX25 cache support is currently work well.
> >
> >>
> >> I think Rick has to comment on this as he added this via commit:
> >>
> >> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
> >> Author: Rick Chen <rick@andestech.com>
> >> Date:   Wed Nov 7 09:34:06 2018 +0800
> >>
> >>      riscv: cache: Implement i/dcache [status, enable, disable]
> >>
> >>      AndeStar RISC-V(V5) provide mcache_ctl register which
> >>      can configure I/D cache as enabled or disabled.
> >>
> >>      This CSR will be encapsulated by CONFIG_RISCV_NDS.
> >>      If you want to configure cache on AndeStar V5
> >>      AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> >>      by make menuconfig.
> >>
> >>      This approach also provide the expansion when the
> >>      vender specific features are going to join in.
> >>
> >>      Signed-off-by: Rick Chen <rick@andestech.com>
> >>      Cc: Greentime Hu <greentime@andestech.com>
> >>
> >> The original commit has i/d cache enabled on all harts. But it was
> >> later moved to boot hart due to SMP support.
> >
> > Not all harts will enable i/d cache during startup currently, only
> > main hart will enable i/d cache here.
> > So only main hart will disable i/d cache in cleanup_before_linux().
>
> Ok, so we have ax25 where cache is disabled on all harts before Linux,
> and fu740 where cache will be enabled on all harts. Is there any
> guidance from Linux on what should be happening here?
>
> --Sean
>
> > Thanks,
> > Rick
> >
> >>
> >> However on the cleanup side, it looks only the boot hart calls i/d
> >> cache disable?
> >
> >>
> >> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
> >>
> >> Regards,
> >> Bin
>
>

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

[Sean Anderson]

On 4/9/21 12:05 PM, Green Wan wrote:
> Hi folks,
> 
> Correct me if I'm wrong, like Rick mentioned, i/dcache
> enable/disable() is only called on the main hart. Right now the dummy
> i/dcache enable/disable are empty and shared among all riscv CPU. The
> ax25 is the only one that has its own implementation for now.

Right, so why are caches are disabled on all harts before booting Linux
on ax25? Is there a requirement for this on ax25 which that other
platforms (which have always-on caches like k210, or which have
non-disableable caches like fuX40) do not have?

--Sean

> 
> FU540/FU740 also leverages the dummy i/dcache enable/disable()
> functions (only main hart calls them). L2 cache on FU540/FU740 is
> enabled as SRAM purpose. And according to the HW design behavior, once
> L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
> L2$ driver will handle that according to the configuration of L2
> registers.
> 
> [1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> 
> Thanks,
> 
> On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 4/9/21 4:16 AM, Rick Chen wrote:
>>> Hi Sean ,Bin
>>>
>>>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
>>>> Sent: Tuesday, April 06, 2021 5:16 PM
>>>> To: Sean Anderson
>>>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
>>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>>>>
>>>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
>>>>>
>>>>> On 3/30/21 1:26 AM, Green Wan wrote:
>>>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
>>>>>>     2 files changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
>>>>>> index 85592f5bee..1652e51137 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();
>>>>>>     }
>>>>>> +
>>>>>> +/**
>>>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
>>>>>> +{
>>>>>> +}
>>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>>>>>> index 8589509e01..ab73008f23 100644
>>>>>> --- a/arch/riscv/cpu/start.S
>>>>>> +++ b/arch/riscv/cpu/start.S
>>>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
>>>>>>         mv      sp, a0
>>>>>>     #endif
>>>>>>
>>>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
>>>>>> +     /*
>>>>>> +      * Jump to riscv_hart_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.
>>>>>> +      *
>>>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
>>>>>> +      */
>>>>>> +call_riscv_hart_early_init:
>>>>>> +     jal     riscv_hart_early_init
>>>>>> +#endif
>>>>>> +
>>>>>
>>>>> I wonder if we could move the calls to icache_enable and dcache_enable
>>>>> into this function. Though this would have the consequence of enabling
>>>>> caches on all harts for CPUs which previously only enabled them for the
>>>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
>>>>> would this be an issue?
>>>
>>> No, they are functions shall be called in different stage about lottery.
>>> riscv_hart_early_init() is called before lottery for all harts.
>>> It shall not move icache_enable() and dcache_enable() into
>>> riscv_hart_early_init(), they are belong to the stage after lottery
>>> only for main hart.
>>>
>>>>
>>>> Good catch. I believe AX25 cache support is currently somehow broken?
>>>
>>> No, AX25 cache support is currently work well.
>>>
>>>>
>>>> I think Rick has to comment on this as he added this via commit:
>>>>
>>>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
>>>> Author: Rick Chen <rick@andestech.com>
>>>> Date:   Wed Nov 7 09:34:06 2018 +0800
>>>>
>>>>       riscv: cache: Implement i/dcache [status, enable, disable]
>>>>
>>>>       AndeStar RISC-V(V5) provide mcache_ctl register which
>>>>       can configure I/D cache as enabled or disabled.
>>>>
>>>>       This CSR will be encapsulated by CONFIG_RISCV_NDS.
>>>>       If you want to configure cache on AndeStar V5
>>>>       AE350 platform. YOu can enable [*] AndeStar V5 ISA support
>>>>       by make menuconfig.
>>>>
>>>>       This approach also provide the expansion when the
>>>>       vender specific features are going to join in.
>>>>
>>>>       Signed-off-by: Rick Chen <rick@andestech.com>
>>>>       Cc: Greentime Hu <greentime@andestech.com>
>>>>
>>>> The original commit has i/d cache enabled on all harts. But it was
>>>> later moved to boot hart due to SMP support.
>>>
>>> Not all harts will enable i/d cache during startup currently, only
>>> main hart will enable i/d cache here.
>>> So only main hart will disable i/d cache in cleanup_before_linux().
>>
>> Ok, so we have ax25 where cache is disabled on all harts before Linux,
>> and fu740 where cache will be enabled on all harts. Is there any
>> guidance from Linux on what should be happening here?
>>
>> --Sean
>>
>>> Thanks,
>>> Rick
>>>
>>>>
>>>> However on the cleanup side, it looks only the boot hart calls i/d
>>>> cache disable?
>>>
>>>>
>>>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
>>>>
>>>> Regards,
>>>> Bin
>>
>>

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

[Green Wan]

Hi Bin and Sean,

While we keep the consistency of cache control discussion going, later
today I'd like to send the v5 patch which is not directly relevant to
cache control.

Regards,
Green

On Sun, Apr 11, 2021 at 11:43 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 4/9/21 12:05 PM, Green Wan wrote:
> > Hi folks,
> >
> > Correct me if I'm wrong, like Rick mentioned, i/dcache
> > enable/disable() is only called on the main hart. Right now the dummy
> > i/dcache enable/disable are empty and shared among all riscv CPU. The
> > ax25 is the only one that has its own implementation for now.
>
> Right, so why are caches are disabled on all harts before booting Linux
> on ax25? Is there a requirement for this on ax25 which that other
> platforms (which have always-on caches like k210, or which have
> non-disableable caches like fuX40) do not have?
>
> --Sean
>
> >
> > FU540/FU740 also leverages the dummy i/dcache enable/disable()
> > functions (only main hart calls them). L2 cache on FU540/FU740 is
> > enabled as SRAM purpose. And according to the HW design behavior, once
> > L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
> > L2$ driver will handle that according to the configuration of L2
> > registers.
> >
> > [1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> >
> > Thanks,
> >
> > On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 4/9/21 4:16 AM, Rick Chen wrote:
> >>> Hi Sean ,Bin
> >>>
> >>>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> >>>> Sent: Tuesday, April 06, 2021 5:16 PM
> >>>> To: Sean Anderson
> >>>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
> >>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >>>>
> >>>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>
> >>>>> On 3/30/21 1:26 AM, Green Wan wrote:
> >>>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
> >>>>>>     2 files changed, 29 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> >>>>>> index 85592f5bee..1652e51137 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();
> >>>>>>     }
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> >>>>>> +{
> >>>>>> +}
> >>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> >>>>>> index 8589509e01..ab73008f23 100644
> >>>>>> --- a/arch/riscv/cpu/start.S
> >>>>>> +++ b/arch/riscv/cpu/start.S
> >>>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
> >>>>>>         mv      sp, a0
> >>>>>>     #endif
> >>>>>>
> >>>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> >>>>>> +     /*
> >>>>>> +      * Jump to riscv_hart_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.
> >>>>>> +      *
> >>>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> >>>>>> +      */
> >>>>>> +call_riscv_hart_early_init:
> >>>>>> +     jal     riscv_hart_early_init
> >>>>>> +#endif
> >>>>>> +
> >>>>>
> >>>>> I wonder if we could move the calls to icache_enable and dcache_enable
> >>>>> into this function. Though this would have the consequence of enabling
> >>>>> caches on all harts for CPUs which previously only enabled them for the
> >>>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
> >>>>> would this be an issue?
> >>>
> >>> No, they are functions shall be called in different stage about lottery.
> >>> riscv_hart_early_init() is called before lottery for all harts.
> >>> It shall not move icache_enable() and dcache_enable() into
> >>> riscv_hart_early_init(), they are belong to the stage after lottery
> >>> only for main hart.
> >>>
> >>>>
> >>>> Good catch. I believe AX25 cache support is currently somehow broken?
> >>>
> >>> No, AX25 cache support is currently work well.
> >>>
> >>>>
> >>>> I think Rick has to comment on this as he added this via commit:
> >>>>
> >>>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
> >>>> Author: Rick Chen <rick@andestech.com>
> >>>> Date:   Wed Nov 7 09:34:06 2018 +0800
> >>>>
> >>>>       riscv: cache: Implement i/dcache [status, enable, disable]
> >>>>
> >>>>       AndeStar RISC-V(V5) provide mcache_ctl register which
> >>>>       can configure I/D cache as enabled or disabled.
> >>>>
> >>>>       This CSR will be encapsulated by CONFIG_RISCV_NDS.
> >>>>       If you want to configure cache on AndeStar V5
> >>>>       AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> >>>>       by make menuconfig.
> >>>>
> >>>>       This approach also provide the expansion when the
> >>>>       vender specific features are going to join in.
> >>>>
> >>>>       Signed-off-by: Rick Chen <rick@andestech.com>
> >>>>       Cc: Greentime Hu <greentime@andestech.com>
> >>>>
> >>>> The original commit has i/d cache enabled on all harts. But it was
> >>>> later moved to boot hart due to SMP support.
> >>>
> >>> Not all harts will enable i/d cache during startup currently, only
> >>> main hart will enable i/d cache here.
> >>> So only main hart will disable i/d cache in cleanup_before_linux().
> >>
> >> Ok, so we have ax25 where cache is disabled on all harts before Linux,
> >> and fu740 where cache will be enabled on all harts. Is there any
> >> guidance from Linux on what should be happening here?
> >>
> >> --Sean
> >>
> >>> Thanks,
> >>> Rick
> >>>
> >>>>
> >>>> However on the cleanup side, it looks only the boot hart calls i/d
> >>>> cache disable?
> >>>
> >>>>
> >>>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
> >>>>
> >>>> Regards,
> >>>> Bin
> >>
> >>
>
>

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

[Rick Chen]

Hi Green,

> From: Green Wan [mailto:green.wan at sifive.com]
> Sent: Monday, April 12, 2021 10:33 AM
> To: Sean Anderson
> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>
> Hi Bin and Sean,
>
> While we keep the consistency of cache control discussion going, later
> today I'd like to send the v5 patch which is not directly relevant to
> cache control.

I will prefer not to mix cache control issue into this patch.
Like I said, this callback is a init for all harts before lottery.

Thanks,
Rick

>
> Regards,
> Green
>
> On Sun, Apr 11, 2021 at 11:43 PM Sean Anderson <seanga2@gmail.com> wrote:
> >
> > On 4/9/21 12:05 PM, Green Wan wrote:
> > > Hi folks,
> > >
> > > Correct me if I'm wrong, like Rick mentioned, i/dcache
> > > enable/disable() is only called on the main hart. Right now the dummy
> > > i/dcache enable/disable are empty and shared among all riscv CPU. The
> > > ax25 is the only one that has its own implementation for now.
> >
> > Right, so why are caches are disabled on all harts before booting Linux
> > on ax25? Is there a requirement for this on ax25 which that other
> > platforms (which have always-on caches like k210, or which have
> > non-disableable caches like fuX40) do not have?
> >
> > --Sean
> >
> > >
> > > FU540/FU740 also leverages the dummy i/dcache enable/disable()
> > > functions (only main hart calls them). L2 cache on FU540/FU740 is
> > > enabled as SRAM purpose. And according to the HW design behavior, once
> > > L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
> > > L2$ driver will handle that according to the configuration of L2
> > > registers.
> > >
> > > [1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > >
> > > Thanks,
> > >
> > > On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
> > >>
> > >> On 4/9/21 4:16 AM, Rick Chen wrote:
> > >>> Hi Sean ,Bin
> > >>>
> > >>>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> > >>>> Sent: Tuesday, April 06, 2021 5:16 PM
> > >>>> To: Sean Anderson
> > >>>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
> > >>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> > >>>>
> > >>>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
> > >>>>>
> > >>>>> On 3/30/21 1:26 AM, Green Wan wrote:
> > >>>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
> > >>>>>>     2 files changed, 29 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > >>>>>> index 85592f5bee..1652e51137 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();
> > >>>>>>     }
> > >>>>>> +
> > >>>>>> +/**
> > >>>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> > >>>>>> +{
> > >>>>>> +}
> > >>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > >>>>>> index 8589509e01..ab73008f23 100644
> > >>>>>> --- a/arch/riscv/cpu/start.S
> > >>>>>> +++ b/arch/riscv/cpu/start.S
> > >>>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
> > >>>>>>         mv      sp, a0
> > >>>>>>     #endif
> > >>>>>>
> > >>>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > >>>>>> +     /*
> > >>>>>> +      * Jump to riscv_hart_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.
> > >>>>>> +      *
> > >>>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> > >>>>>> +      */
> > >>>>>> +call_riscv_hart_early_init:
> > >>>>>> +     jal     riscv_hart_early_init
> > >>>>>> +#endif
> > >>>>>> +
> > >>>>>
> > >>>>> I wonder if we could move the calls to icache_enable and dcache_enable
> > >>>>> into this function. Though this would have the consequence of enabling
> > >>>>> caches on all harts for CPUs which previously only enabled them for the
> > >>>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
> > >>>>> would this be an issue?
> > >>>
> > >>> No, they are functions shall be called in different stage about lottery.
> > >>> riscv_hart_early_init() is called before lottery for all harts.
> > >>> It shall not move icache_enable() and dcache_enable() into
> > >>> riscv_hart_early_init(), they are belong to the stage after lottery
> > >>> only for main hart.
> > >>>
> > >>>>
> > >>>> Good catch. I believe AX25 cache support is currently somehow broken?
> > >>>
> > >>> No, AX25 cache support is currently work well.
> > >>>
> > >>>>
> > >>>> I think Rick has to comment on this as he added this via commit:
> > >>>>
> > >>>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
> > >>>> Author: Rick Chen <rick@andestech.com>
> > >>>> Date:   Wed Nov 7 09:34:06 2018 +0800
> > >>>>
> > >>>>       riscv: cache: Implement i/dcache [status, enable, disable]
> > >>>>
> > >>>>       AndeStar RISC-V(V5) provide mcache_ctl register which
> > >>>>       can configure I/D cache as enabled or disabled.
> > >>>>
> > >>>>       This CSR will be encapsulated by CONFIG_RISCV_NDS.
> > >>>>       If you want to configure cache on AndeStar V5
> > >>>>       AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> > >>>>       by make menuconfig.
> > >>>>
> > >>>>       This approach also provide the expansion when the
> > >>>>       vender specific features are going to join in.
> > >>>>
> > >>>>       Signed-off-by: Rick Chen <rick@andestech.com>
> > >>>>       Cc: Greentime Hu <greentime@andestech.com>
> > >>>>
> > >>>> The original commit has i/d cache enabled on all harts. But it was
> > >>>> later moved to boot hart due to SMP support.
> > >>>
> > >>> Not all harts will enable i/d cache during startup currently, only
> > >>> main hart will enable i/d cache here.
> > >>> So only main hart will disable i/d cache in cleanup_before_linux().
> > >>
> > >> Ok, so we have ax25 where cache is disabled on all harts before Linux,
> > >> and fu740 where cache will be enabled on all harts. Is there any
> > >> guidance from Linux on what should be happening here?
> > >>
> > >> --Sean
> > >>
> > >>> Thanks,
> > >>> Rick
> > >>>
> > >>>>
> > >>>> However on the cleanup side, it looks only the boot hart calls i/d
> > >>>> cache disable?
> > >>>
> > >>>>
> > >>>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
> > >>>>
> > >>>> Regards,
> > >>>> Bin
> > >>
> > >>
> >

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

[Green Wan]

On Tue, Apr 13, 2021 at 10:39 AM Rick Chen <rickchen36@gmail.com> wrote:
>
> Hi Green,
>
> > From: Green Wan [mailto:green.wan at sifive.com]
> > Sent: Monday, April 12, 2021 10:33 AM
> > To: Sean Anderson
> > Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
> > Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >
> > Hi Bin and Sean,
> >
> > While we keep the consistency of cache control discussion going, later
> > today I'd like to send the v5 patch which is not directly relevant to
> > cache control.
>
> I will prefer not to mix cache control issue into this patch.
> Like I said, this callback is a init for all harts before lottery.
>
> Thanks,
> Rick

Agree with you.

- Green

>
> >
> > Regards,
> > Green
> >
> > On Sun, Apr 11, 2021 at 11:43 PM Sean Anderson <seanga2@gmail.com> wrote:
> > >
> > > On 4/9/21 12:05 PM, Green Wan wrote:
> > > > Hi folks,
> > > >
> > > > Correct me if I'm wrong, like Rick mentioned, i/dcache
> > > > enable/disable() is only called on the main hart. Right now the dummy
> > > > i/dcache enable/disable are empty and shared among all riscv CPU. The
> > > > ax25 is the only one that has its own implementation for now.
> > >
> > > Right, so why are caches are disabled on all harts before booting Linux
> > > on ax25? Is there a requirement for this on ax25 which that other
> > > platforms (which have always-on caches like k210, or which have
> > > non-disableable caches like fuX40) do not have?
> > >
> > > --Sean
> > >
> > > >
> > > > FU540/FU740 also leverages the dummy i/dcache enable/disable()
> > > > functions (only main hart calls them). L2 cache on FU540/FU740 is
> > > > enabled as SRAM purpose. And according to the HW design behavior, once
> > > > L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
> > > > L2$ driver will handle that according to the configuration of L2
> > > > registers.
> > > >
> > > > [1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > > >
> > > > Thanks,
> > > >
> > > > On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
> > > >>
> > > >> On 4/9/21 4:16 AM, Rick Chen wrote:
> > > >>> Hi Sean ,Bin
> > > >>>
> > > >>>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> > > >>>> Sent: Tuesday, April 06, 2021 5:16 PM
> > > >>>> To: Sean Anderson
> > > >>>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
> > > >>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> > > >>>>
> > > >>>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
> > > >>>>>
> > > >>>>> On 3/30/21 1:26 AM, Green Wan wrote:
> > > >>>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
> > > >>>>>>     2 files changed, 29 insertions(+)
> > > >>>>>>
> > > >>>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > > >>>>>> index 85592f5bee..1652e51137 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();
> > > >>>>>>     }
> > > >>>>>> +
> > > >>>>>> +/**
> > > >>>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> > > >>>>>> +{
> > > >>>>>> +}
> > > >>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > > >>>>>> index 8589509e01..ab73008f23 100644
> > > >>>>>> --- a/arch/riscv/cpu/start.S
> > > >>>>>> +++ b/arch/riscv/cpu/start.S
> > > >>>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
> > > >>>>>>         mv      sp, a0
> > > >>>>>>     #endif
> > > >>>>>>
> > > >>>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > > >>>>>> +     /*
> > > >>>>>> +      * Jump to riscv_hart_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.
> > > >>>>>> +      *
> > > >>>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> > > >>>>>> +      */
> > > >>>>>> +call_riscv_hart_early_init:
> > > >>>>>> +     jal     riscv_hart_early_init
> > > >>>>>> +#endif
> > > >>>>>> +
> > > >>>>>
> > > >>>>> I wonder if we could move the calls to icache_enable and dcache_enable
> > > >>>>> into this function. Though this would have the consequence of enabling
> > > >>>>> caches on all harts for CPUs which previously only enabled them for the
> > > >>>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
> > > >>>>> would this be an issue?
> > > >>>
> > > >>> No, they are functions shall be called in different stage about lottery.
> > > >>> riscv_hart_early_init() is called before lottery for all harts.
> > > >>> It shall not move icache_enable() and dcache_enable() into
> > > >>> riscv_hart_early_init(), they are belong to the stage after lottery
> > > >>> only for main hart.
> > > >>>
> > > >>>>
> > > >>>> Good catch. I believe AX25 cache support is currently somehow broken?
> > > >>>
> > > >>> No, AX25 cache support is currently work well.
> > > >>>
> > > >>>>
> > > >>>> I think Rick has to comment on this as he added this via commit:
> > > >>>>
> > > >>>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
> > > >>>> Author: Rick Chen <rick@andestech.com>
> > > >>>> Date:   Wed Nov 7 09:34:06 2018 +0800
> > > >>>>
> > > >>>>       riscv: cache: Implement i/dcache [status, enable, disable]
> > > >>>>
> > > >>>>       AndeStar RISC-V(V5) provide mcache_ctl register which
> > > >>>>       can configure I/D cache as enabled or disabled.
> > > >>>>
> > > >>>>       This CSR will be encapsulated by CONFIG_RISCV_NDS.
> > > >>>>       If you want to configure cache on AndeStar V5
> > > >>>>       AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> > > >>>>       by make menuconfig.
> > > >>>>
> > > >>>>       This approach also provide the expansion when the
> > > >>>>       vender specific features are going to join in.
> > > >>>>
> > > >>>>       Signed-off-by: Rick Chen <rick@andestech.com>
> > > >>>>       Cc: Greentime Hu <greentime@andestech.com>
> > > >>>>
> > > >>>> The original commit has i/d cache enabled on all harts. But it was
> > > >>>> later moved to boot hart due to SMP support.
> > > >>>
> > > >>> Not all harts will enable i/d cache during startup currently, only
> > > >>> main hart will enable i/d cache here.
> > > >>> So only main hart will disable i/d cache in cleanup_before_linux().
> > > >>
> > > >> Ok, so we have ax25 where cache is disabled on all harts before Linux,
> > > >> and fu740 where cache will be enabled on all harts. Is there any
> > > >> guidance from Linux on what should be happening here?
> > > >>
> > > >> --Sean
> > > >>
> > > >>> Thanks,
> > > >>> Rick
> > > >>>
> > > >>>>
> > > >>>> However on the cleanup side, it looks only the boot hart calls i/d
> > > >>>> cache disable?
> > > >>>
> > > >>>>
> > > >>>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
> > > >>>>
> > > >>>> Regards,
> > > >>>> Bin
> > > >>
> > > >>
> > >

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

[Sean Anderson]

On 4/12/21 10:39 PM, Rick Chen wrote:
> Hi Green,
> 
>> From: Green Wan [mailto:green.wan at sifive.com]
>> Sent: Monday, April 12, 2021 10:33 AM
>> To: Sean Anderson
>> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>>
>> Hi Bin and Sean,
>>
>> While we keep the consistency of cache control discussion going, later
>> today I'd like to send the v5 patch which is not directly relevant to
>> cache control.
> 
> I will prefer not to mix cache control issue into this patch.
> Like I said, this callback is a init for all harts before lottery.

Yes, but enabling caches is a very similar thing (this proposal even
uses it to turn on caches, among other things). At the moment we have
two calls to enable caches at almost the same time as what Green
proposes. These calls only translate into work done on one platform. I
think having one call (or perhaps two) for this purpose would help
reduce codepaths across different platforms going forward.

--Sean

> 
> Thanks,
> Rick
> 
>>
>> Regards,
>> Green
>>
>> On Sun, Apr 11, 2021 at 11:43 PM Sean Anderson <seanga2@gmail.com> wrote:
>>>
>>> On 4/9/21 12:05 PM, Green Wan wrote:
>>>> Hi folks,
>>>>
>>>> Correct me if I'm wrong, like Rick mentioned, i/dcache
>>>> enable/disable() is only called on the main hart. Right now the dummy
>>>> i/dcache enable/disable are empty and shared among all riscv CPU. The
>>>> ax25 is the only one that has its own implementation for now.
>>>
>>> Right, so why are caches are disabled on all harts before booting Linux
>>> on ax25? Is there a requirement for this on ax25 which that other
>>> platforms (which have always-on caches like k210, or which have
>>> non-disableable caches like fuX40) do not have?
>>>
>>> --Sean
>>>
>>>>
>>>> FU540/FU740 also leverages the dummy i/dcache enable/disable()
>>>> functions (only main hart calls them). L2 cache on FU540/FU740 is
>>>> enabled as SRAM purpose. And according to the HW design behavior, once
>>>> L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
>>>> L2$ driver will handle that according to the configuration of L2
>>>> registers.
>>>>
>>>> [1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf
>>>>
>>>> Thanks,
>>>>
>>>> On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
>>>>>
>>>>> On 4/9/21 4:16 AM, Rick Chen wrote:
>>>>>> Hi Sean ,Bin
>>>>>>
>>>>>>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
>>>>>>> Sent: Tuesday, April 06, 2021 5:16 PM
>>>>>>> To: Sean Anderson
>>>>>>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
>>>>>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>>>>>>>
>>>>>>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 3/30/21 1:26 AM, Green Wan wrote:
>>>>>>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
>>>>>>>>>      2 files changed, 29 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
>>>>>>>>> index 85592f5bee..1652e51137 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();
>>>>>>>>>      }
>>>>>>>>> +
>>>>>>>>> +/**
>>>>>>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
>>>>>>>>> +{
>>>>>>>>> +}
>>>>>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>>>>>>>>> index 8589509e01..ab73008f23 100644
>>>>>>>>> --- a/arch/riscv/cpu/start.S
>>>>>>>>> +++ b/arch/riscv/cpu/start.S
>>>>>>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
>>>>>>>>>          mv      sp, a0
>>>>>>>>>      #endif
>>>>>>>>>
>>>>>>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
>>>>>>>>> +     /*
>>>>>>>>> +      * Jump to riscv_hart_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.
>>>>>>>>> +      *
>>>>>>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
>>>>>>>>> +      */
>>>>>>>>> +call_riscv_hart_early_init:
>>>>>>>>> +     jal     riscv_hart_early_init
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>
>>>>>>>> I wonder if we could move the calls to icache_enable and dcache_enable
>>>>>>>> into this function. Though this would have the consequence of enabling
>>>>>>>> caches on all harts for CPUs which previously only enabled them for the
>>>>>>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
>>>>>>>> would this be an issue?
>>>>>>
>>>>>> No, they are functions shall be called in different stage about lottery.
>>>>>> riscv_hart_early_init() is called before lottery for all harts.
>>>>>> It shall not move icache_enable() and dcache_enable() into
>>>>>> riscv_hart_early_init(), they are belong to the stage after lottery
>>>>>> only for main hart.
>>>>>>
>>>>>>>
>>>>>>> Good catch. I believe AX25 cache support is currently somehow broken?
>>>>>>
>>>>>> No, AX25 cache support is currently work well.
>>>>>>
>>>>>>>
>>>>>>> I think Rick has to comment on this as he added this via commit:
>>>>>>>
>>>>>>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
>>>>>>> Author: Rick Chen <rick@andestech.com>
>>>>>>> Date:   Wed Nov 7 09:34:06 2018 +0800
>>>>>>>
>>>>>>>        riscv: cache: Implement i/dcache [status, enable, disable]
>>>>>>>
>>>>>>>        AndeStar RISC-V(V5) provide mcache_ctl register which
>>>>>>>        can configure I/D cache as enabled or disabled.
>>>>>>>
>>>>>>>        This CSR will be encapsulated by CONFIG_RISCV_NDS.
>>>>>>>        If you want to configure cache on AndeStar V5
>>>>>>>        AE350 platform. YOu can enable [*] AndeStar V5 ISA support
>>>>>>>        by make menuconfig.
>>>>>>>
>>>>>>>        This approach also provide the expansion when the
>>>>>>>        vender specific features are going to join in.
>>>>>>>
>>>>>>>        Signed-off-by: Rick Chen <rick@andestech.com>
>>>>>>>        Cc: Greentime Hu <greentime@andestech.com>
>>>>>>>
>>>>>>> The original commit has i/d cache enabled on all harts. But it was
>>>>>>> later moved to boot hart due to SMP support.
>>>>>>
>>>>>> Not all harts will enable i/d cache during startup currently, only
>>>>>> main hart will enable i/d cache here.
>>>>>> So only main hart will disable i/d cache in cleanup_before_linux().
>>>>>
>>>>> Ok, so we have ax25 where cache is disabled on all harts before Linux,
>>>>> and fu740 where cache will be enabled on all harts. Is there any
>>>>> guidance from Linux on what should be happening here?
>>>>>
>>>>> --Sean
>>>>>
>>>>>> Thanks,
>>>>>> Rick
>>>>>>
>>>>>>>
>>>>>>> However on the cleanup side, it looks only the boot hart calls i/d
>>>>>>> cache disable?
>>>>>>
>>>>>>>
>>>>>>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
>>>>>>>
>>>>>>> Regards,
>>>>>>> Bin
>>>>>
>>>>>
>>>

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

[Rick Chen]

Hi Green,

> From: Green Wan [mailto:green.wan at sifive.com]
> Sent: Tuesday, March 30, 2021 1:27 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; u-boot at lists.denx.de
> Subject: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>
> Add a callback riscv_hart_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 | 14 ++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> index 85592f5bee..1652e51137 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();
>  }
> +
> +/**
> + * riscv_hart_early_init() - A dummy function called by

Maybe you can rename as harts_early_init(), riscv seems to be extra prefix here.
And dummy sounds like a negative word, it will be better to describe
it as a callback for customize CSR features ...etc.

> + * ./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 riscv_hart_early_init(void)
> +{
> +}
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 8589509e01..ab73008f23 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -117,6 +117,20 @@ call_board_init_f_0:
>         mv      sp, a0
>  #endif
>
> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> +       /*
> +        * Jump to riscv_hart_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.

You can emphasize this init is a callback for customize CSR settings
for all harts simply.
Any memory access is prohibited here.

> +        *
> +        * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.

This description for path is not necessary here.

Thanks,
Rick

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

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

[Rick Chen]

Hi Sean

> On 4/12/21 10:39 PM, Rick Chen wrote:
> > Hi Green,
> >
> >> From: Green Wan [mailto:green.wan at sifive.com]
> >> Sent: Monday, April 12, 2021 10:33 AM
> >> To: Sean Anderson
> >> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
> >> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >>
> >> Hi Bin and Sean,
> >>
> >> While we keep the consistency of cache control discussion going, later
> >> today I'd like to send the v5 patch which is not directly relevant to
> >> cache control.
> >
> > I will prefer not to mix cache control issue into this patch.
> > Like I said, this callback is a init for all harts before lottery.
>
> Yes, but enabling caches is a very similar thing (this proposal even
> uses it to turn on caches, among other things). At the moment we have
> two calls to enable caches at almost the same time as what Green
> proposes. These calls only translate into work done on one platform. I
> think having one call (or perhaps two) for this purpose would help
> reduce codepaths across different platforms going forward.
>

Maybe we can add two callbacks (early_lottery_init and
late_lottery_init) before and after lottery individually for all
scenarios.

Thanks,
Rick

> --Sean
>
> >
> > Thanks,
> > Rick
> >
> >>
> >> Regards,
> >> Green
> >>
> >> On Sun, Apr 11, 2021 at 11:43 PM Sean Anderson <seanga2@gmail.com> wrote:
> >>>
> >>> On 4/9/21 12:05 PM, Green Wan wrote:
> >>>> Hi folks,
> >>>>
> >>>> Correct me if I'm wrong, like Rick mentioned, i/dcache
> >>>> enable/disable() is only called on the main hart. Right now the dummy
> >>>> i/dcache enable/disable are empty and shared among all riscv CPU. The
> >>>> ax25 is the only one that has its own implementation for now.
> >>>
> >>> Right, so why are caches are disabled on all harts before booting Linux
> >>> on ax25? Is there a requirement for this on ax25 which that other
> >>> platforms (which have always-on caches like k210, or which have
> >>> non-disableable caches like fuX40) do not have?
> >>>
> >>> --Sean
> >>>
> >>>>
> >>>> FU540/FU740 also leverages the dummy i/dcache enable/disable()
> >>>> functions (only main hart calls them). L2 cache on FU540/FU740 is
> >>>> enabled as SRAM purpose. And according to the HW design behavior, once
> >>>> L2 is enabled, it can't be disabled unless doing a reset.[1] The Linux
> >>>> L2$ driver will handle that according to the configuration of L2
> >>>> registers.
> >>>>
> >>>> [1] https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> >>>>
> >>>> Thanks,
> >>>>
> >>>> On Fri, Apr 9, 2021 at 9:18 PM Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>
> >>>>> On 4/9/21 4:16 AM, Rick Chen wrote:
> >>>>>> Hi Sean ,Bin
> >>>>>>
> >>>>>>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> >>>>>>> Sent: Tuesday, April 06, 2021 5:16 PM
> >>>>>>> To: Sean Anderson
> >>>>>>> Cc: Green Wan; Rick Jian-Zhi Chen(???); Paul Walmsley; Pragnesh Patel; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim; U-Boot Mailing List
> >>>>>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >>>>>>>
> >>>>>>> On Sat, Apr 3, 2021 at 6:53 AM Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 3/30/21 1:26 AM, Green Wan wrote:
> >>>>>>>>> Add a callback riscv_hart_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 | 14 ++++++++++++++
> >>>>>>>>>      2 files changed, 29 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> >>>>>>>>> index 85592f5bee..1652e51137 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();
> >>>>>>>>>      }
> >>>>>>>>> +
> >>>>>>>>> +/**
> >>>>>>>>> + * riscv_hart_early_init() - A dummy 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 riscv_hart_early_init(void)
> >>>>>>>>> +{
> >>>>>>>>> +}
> >>>>>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> >>>>>>>>> index 8589509e01..ab73008f23 100644
> >>>>>>>>> --- a/arch/riscv/cpu/start.S
> >>>>>>>>> +++ b/arch/riscv/cpu/start.S
> >>>>>>>>> @@ -117,6 +117,20 @@ call_board_init_f_0:
> >>>>>>>>>          mv      sp, a0
> >>>>>>>>>      #endif
> >>>>>>>>>
> >>>>>>>>> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> >>>>>>>>> +     /*
> >>>>>>>>> +      * Jump to riscv_hart_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.
> >>>>>>>>> +      *
> >>>>>>>>> +      * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
> >>>>>>>>> +      */
> >>>>>>>>> +call_riscv_hart_early_init:
> >>>>>>>>> +     jal     riscv_hart_early_init
> >>>>>>>>> +#endif
> >>>>>>>>> +
> >>>>>>>>
> >>>>>>>> I wonder if we could move the calls to icache_enable and dcache_enable
> >>>>>>>> into this function. Though this would have the consequence of enabling
> >>>>>>>> caches on all harts for CPUs which previously only enabled them for the
> >>>>>>>> boot hart. I think ax25 is the only CPU which currently does this. Bin,
> >>>>>>>> would this be an issue?
> >>>>>>
> >>>>>> No, they are functions shall be called in different stage about lottery.
> >>>>>> riscv_hart_early_init() is called before lottery for all harts.
> >>>>>> It shall not move icache_enable() and dcache_enable() into
> >>>>>> riscv_hart_early_init(), they are belong to the stage after lottery
> >>>>>> only for main hart.
> >>>>>>
> >>>>>>>
> >>>>>>> Good catch. I believe AX25 cache support is currently somehow broken?
> >>>>>>
> >>>>>> No, AX25 cache support is currently work well.
> >>>>>>
> >>>>>>>
> >>>>>>> I think Rick has to comment on this as he added this via commit:
> >>>>>>>
> >>>>>>> commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
> >>>>>>> Author: Rick Chen <rick@andestech.com>
> >>>>>>> Date:   Wed Nov 7 09:34:06 2018 +0800
> >>>>>>>
> >>>>>>>        riscv: cache: Implement i/dcache [status, enable, disable]
> >>>>>>>
> >>>>>>>        AndeStar RISC-V(V5) provide mcache_ctl register which
> >>>>>>>        can configure I/D cache as enabled or disabled.
> >>>>>>>
> >>>>>>>        This CSR will be encapsulated by CONFIG_RISCV_NDS.
> >>>>>>>        If you want to configure cache on AndeStar V5
> >>>>>>>        AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> >>>>>>>        by make menuconfig.
> >>>>>>>
> >>>>>>>        This approach also provide the expansion when the
> >>>>>>>        vender specific features are going to join in.
> >>>>>>>
> >>>>>>>        Signed-off-by: Rick Chen <rick@andestech.com>
> >>>>>>>        Cc: Greentime Hu <greentime@andestech.com>
> >>>>>>>
> >>>>>>> The original commit has i/d cache enabled on all harts. But it was
> >>>>>>> later moved to boot hart due to SMP support.
> >>>>>>
> >>>>>> Not all harts will enable i/d cache during startup currently, only
> >>>>>> main hart will enable i/d cache here.
> >>>>>> So only main hart will disable i/d cache in cleanup_before_linux().
> >>>>>
> >>>>> Ok, so we have ax25 where cache is disabled on all harts before Linux,
> >>>>> and fu740 where cache will be enabled on all harts. Is there any
> >>>>> guidance from Linux on what should be happening here?
> >>>>>
> >>>>> --Sean
> >>>>>
> >>>>>> Thanks,
> >>>>>> Rick
> >>>>>>
> >>>>>>>
> >>>>>>> However on the cleanup side, it looks only the boot hart calls i/d
> >>>>>>> cache disable?
> >>>>>>
> >>>>>>>
> >>>>>>> See arch/riscv/cpu/ax25/cpu.c:: cleanup_before_linux()
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Bin
> >>>>>
> >>>>>
> >>>
>
>

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

[Sean Anderson]

On 4/13/21 12:12 AM, Rick Chen wrote:
> Hi Sean
> 
>> On 4/12/21 10:39 PM, Rick Chen wrote:
>>> Hi Green,
>>>
>>>> From: Green Wan [mailto:green.wan at sifive.com]
>>>> Sent: Monday, April 12, 2021 10:33 AM
>>>> To: Sean Anderson
>>>> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
>>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>>>>
>>>> Hi Bin and Sean,
>>>>
>>>> While we keep the consistency of cache control discussion going, later
>>>> today I'd like to send the v5 patch which is not directly relevant to
>>>> cache control.
>>>
>>> I will prefer not to mix cache control issue into this patch.
>>> Like I said, this callback is a init for all harts before lottery.
>>
>> Yes, but enabling caches is a very similar thing (this proposal even
>> uses it to turn on caches, among other things). At the moment we have
>> two calls to enable caches at almost the same time as what Green
>> proposes. These calls only translate into work done on one platform. I
>> think having one call (or perhaps two) for this purpose would help
>> reduce codepaths across different platforms going forward.
>>
> 
> Maybe we can add two callbacks (early_lottery_init and
> late_lottery_init) before and after lottery individually for all
> scenarios.

Yes, that is a possibility. But do we actually need that flexibility?
This comes back around to my original question: why does ax25 disable
cache on all harts before jumping to Linux?

And of course, does this actually need to be done before the lottery?

--Sean

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

[Green Wan]

Hi Rick,

Sorry for the late reply. My working environment got problems. Just
recovered from it. See my reply below.

On Tue, Apr 13, 2021 at 11:33 AM Rick Chen <rickchen36@gmail.com> wrote:
>
> Hi Green,
>
> > From: Green Wan [mailto:green.wan at sifive.com]
> > Sent: Tuesday, March 30, 2021 1:27 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; u-boot at lists.denx.de
> > Subject: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >
> > Add a callback riscv_hart_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 | 14 ++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > index 85592f5bee..1652e51137 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();
> >  }
> > +
> > +/**
> > + * riscv_hart_early_init() - A dummy function called by
>
> Maybe you can rename as harts_early_init(), riscv seems to be extra prefix here.
okay, I'll remove the prefix.

> And dummy sounds like a negative word, it will be better to describe
> it as a callback for customize CSR features ...etc.
>
sure, I can remove 'dummy' and rephrase the comments.

But I wonder whether we all agree the callback is ONLY for customizing
CSRs. It's probably fine to leave the current comments for some
flexibility if any implementation needs to do init differently.

> > + * ./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 riscv_hart_early_init(void)
> > +{
> > +}
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 8589509e01..ab73008f23 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -117,6 +117,20 @@ call_board_init_f_0:
> >         mv      sp, a0
> >  #endif
> >
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > +       /*
> > +        * Jump to riscv_hart_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.
>
> You can emphasize this init is a callback for customize CSR settings
> for all harts simply.
> Any memory access is prohibited here.

Same here. To satisfy my current need, I'll say 'yes'. ;)
But I don't know whether everyone is with me and want to restrict the
callback only for customizing CRSs.

>
> > +        *
> > +        * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
>
> This description for path is not necessary here.
>

Will fix it.

> Thanks,
> Rick
>
> > +        */
> > +call_riscv_hart_early_init:
> > +       jal     riscv_hart_early_init
> > +#endif
> > +
> >  #ifndef CONFIG_XIP
> >         /*
> >          * Pick hart to initialize global data and run U-Boot. The other harts
> > --
> > 2.31.0

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

[Green Wan]

On Tue, Apr 13, 2021 at 12:17 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 4/13/21 12:12 AM, Rick Chen wrote:
> > Hi Sean
> >
> >> On 4/12/21 10:39 PM, Rick Chen wrote:
> >>> Hi Green,
> >>>
> >>>> From: Green Wan [mailto:green.wan at sifive.com]
> >>>> Sent: Monday, April 12, 2021 10:33 AM
> >>>> To: Sean Anderson
> >>>> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
> >>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >>>>
> >>>> Hi Bin and Sean,
> >>>>
> >>>> While we keep the consistency of cache control discussion going, later
> >>>> today I'd like to send the v5 patch which is not directly relevant to
> >>>> cache control.
> >>>
> >>> I will prefer not to mix cache control issue into this patch.
> >>> Like I said, this callback is a init for all harts before lottery.
> >>
> >> Yes, but enabling caches is a very similar thing (this proposal even
> >> uses it to turn on caches, among other things). At the moment we have
> >> two calls to enable caches at almost the same time as what Green
> >> proposes. These calls only translate into work done on one platform. I
> >> think having one call (or perhaps two) for this purpose would help
> >> reduce codepaths across different platforms going forward.
> >>
> >
> > Maybe we can add two callbacks (early_lottery_init and
> > late_lottery_init) before and after lottery individually for all
> > scenarios.
>
> Yes, that is a possibility. But do we actually need that flexibility?
> This comes back around to my original question: why does ax25 disable
> cache on all harts before jumping to Linux?

I sort of agree with Sean to merge some callbacks and keep most boot
flow clean. And I feel early/late_lottery_init not a
generic/meaningful name for a bootloader. But I doubt we should
restrict all harts that must be cache enabled/disabled. Won't it
conflict with some H/W behavior or decrease flexibilities?

>
> And of course, does this actually need to be done before the lottery?
Not sure the question is for me. Since some callbacks are only called
by main hart after picking up the lottery, any init must be done
within each hart itself, we have to do them before the lottery unless
HW provides a common way to set up all harts. Of course, we can use
IPI. But IPI is not a straightforward way.

-
Geen
>
> --Sean
>

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

[Rick Chen]

Hi Sean,

> On 4/13/21 12:12 AM, Rick Chen wrote:
> > Hi Sean
> >
> >> On 4/12/21 10:39 PM, Rick Chen wrote:
> >>> Hi Green,
> >>>
> >>>> From: Green Wan [mailto:green.wan at sifive.com]
> >>>> Sent: Monday, April 12, 2021 10:33 AM
> >>>> To: Sean Anderson
> >>>> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
> >>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >>>>
> >>>> Hi Bin and Sean,
> >>>>
> >>>> While we keep the consistency of cache control discussion going, later
> >>>> today I'd like to send the v5 patch which is not directly relevant to
> >>>> cache control.
> >>>
> >>> I will prefer not to mix cache control issue into this patch.
> >>> Like I said, this callback is a init for all harts before lottery.
> >>
> >> Yes, but enabling caches is a very similar thing (this proposal even
> >> uses it to turn on caches, among other things). At the moment we have
> >> two calls to enable caches at almost the same time as what Green
> >> proposes. These calls only translate into work done on one platform. I
> >> think having one call (or perhaps two) for this purpose would help
> >> reduce codepaths across different platforms going forward.
> >>
> >
> > Maybe we can add two callbacks (early_lottery_init and
> > late_lottery_init) before and after lottery individually for all
> > scenarios.
>
> Yes, that is a possibility. But do we actually need that flexibility?
> This comes back around to my original question: why does ax25 disable
> cache on all harts before jumping to Linux?

clarify your above expressions:
only disable main hart (not all harts) before jumping to Linux.

Why you think it is a question to disable cache before jumping to Linux.
You can find many same cache flow in cleanup_before_linux of /arch/arm
Are they all the questions ?

Thanks,
Rick

>
> And of course, does this actually need to be done before the lottery?
>
> --Sean
>

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

[Sean Anderson]

On 4/13/21 9:05 PM, Rick Chen wrote:
> Hi Sean,
> 
>> On 4/13/21 12:12 AM, Rick Chen wrote:
>>> Hi Sean
>>>
>>>> On 4/12/21 10:39 PM, Rick Chen wrote:
>>>>> Hi Green,
>>>>>
>>>>>> From: Green Wan [mailto:green.wan at sifive.com]
>>>>>> Sent: Monday, April 12, 2021 10:33 AM
>>>>>> To: Sean Anderson
>>>>>> Cc: Rick Chen; Rick Jian-Zhi Chen(???); Bin Meng; U-Boot Mailing List; Paul Walmsley; Pragnesh Patel; Simon Glass; Atish Patra; Leo Yu-Chi Liang(???); Brad Kim
>>>>>> Subject: Re: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
>>>>>>
>>>>>> Hi Bin and Sean,
>>>>>>
>>>>>> While we keep the consistency of cache control discussion going, later
>>>>>> today I'd like to send the v5 patch which is not directly relevant to
>>>>>> cache control.
>>>>>
>>>>> I will prefer not to mix cache control issue into this patch.
>>>>> Like I said, this callback is a init for all harts before lottery.
>>>>
>>>> Yes, but enabling caches is a very similar thing (this proposal even
>>>> uses it to turn on caches, among other things). At the moment we have
>>>> two calls to enable caches at almost the same time as what Green
>>>> proposes. These calls only translate into work done on one platform. I
>>>> think having one call (or perhaps two) for this purpose would help
>>>> reduce codepaths across different platforms going forward.
>>>>
>>>
>>> Maybe we can add two callbacks (early_lottery_init and
>>> late_lottery_init) before and after lottery individually for all
>>> scenarios.
>>
>> Yes, that is a possibility. But do we actually need that flexibility?
>> This comes back around to my original question: why does ax25 disable
>> cache on all harts before jumping to Linux?
> 
> clarify your above expressions:
> only disable main hart (not all harts) before jumping to Linux.
> 
> Why you think it is a question to disable cache before jumping to Linux.
> You can find many same cache flow in cleanup_before_linux of /arch/arm
> Are they all the questions ?

Well, for example K210 cannot disable cache (or rather, cache is a
property of memory address), so cache is always enabled. And this series
from Green suggests that the FU740 also cannot disable cache once
enabled, and he would like to enable it on all harts very early in boot.
So does it matter whether cache is enabled or disabled?

Alternatively, why is cache only enabled on the main hart on ax25? Would
it be OK to enable it on all harts (and then later disable it on all
harts)?

--Sean

> 
> Thanks,
> Rick
> 
>>
>> And of course, does this actually need to be done before the lottery?
>>
>> --Sean
>>