[PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[Yao Zi]

This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.

It has been reported that the commit causes boot regression for SPL on
StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
spot one logic error for deciding whether Zaamo or Zalrsc is used, and
it's still unclear what exactly causes the regression, let's revert it
for now.

Reported-by: E Shattow <e@freeshell.de>
Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
Signed-off-by: Yao Zi <ziyao@disroot.org>
---

The original series causing the problem[1] contains 3 patches, and I
think it should be enough to revert the change of start.S only, since
the others touch no code, and should be relatively safe. I'll fix the
reverted change up and get it work on VisionFive 2 when I got my new
board. Sorry for the inconvenience.

[1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/

 arch/riscv/cpu/start.S | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 6324ff585d4..7bafdfd390a 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -151,15 +151,8 @@ call_harts_early_init:
 	 */
 	la	t0, hart_lottery
 	li	t1, 1
-#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
 	amoswap.w s2, t1, 0(t0)
 	bnez	s2, wait_for_gd_init
-#else
-	lr.w	s2, (t0)
-	bnez	s2, wait_for_gd_init
-	sc.w	s2, t1, (t0)
-	bnez	s2, wait_for_gd_init
-#endif
 #else
 	/*
 	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
@@ -184,12 +177,7 @@ call_harts_early_init:
 #if !CONFIG_IS_ENABLED(XIP)
 #ifdef CONFIG_AVAILABLE_HARTS
 	la	t0, available_harts_lock
-#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
 	amoswap.w.rl zero, zero, 0(t0)
-#else
-	fence	rw, w
-	sw	zero, 0(t0)
-#endif
 #endif
 
 wait_for_gd_init:
@@ -202,14 +190,7 @@ wait_for_gd_init:
 #ifdef CONFIG_AVAILABLE_HARTS
 	la	t0, available_harts_lock
 	li	t1, 1
-1:
-#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
-	amoswap.w.aq t1, t1, 0(t0)
-#else
-	lr.w.aq	t1, 0(t0)
-	bnez	t1, 1b
-	sc.w.rl t1, t1, 0(t0)
-#endif
+1:	amoswap.w.aq t1, t1, 0(t0)
 	bnez	t1, 1b
 
 	/* register available harts in the available_harts mask */
@@ -219,12 +200,7 @@ wait_for_gd_init:
 	or	t2, t2, t1
 	SREG	t2, GD_AVAILABLE_HARTS(gp)
 
-#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
 	amoswap.w.rl zero, zero, 0(t0)
-#else
-	fence	rw, w
-	sw	zero, 0(t0)
-#endif
 #endif
 
 	/*
-- 
2.50.1

Re: [PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[E Shattow]

On 9/25/25 09:01, Yao Zi wrote:
> This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.
> 
> It has been reported that the commit causes boot regression for SPL on
> StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
> spot one logic error for deciding whether Zaamo or Zalrsc is used, and
> it's still unclear what exactly causes the regression, let's revert it
> for now.
> 
> Reported-by: E Shattow <e@freeshell.de>
> Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
> 
> The original series causing the problem[1] contains 3 patches, and I
> think it should be enough to revert the change of start.S only, since
> the others touch no code, and should be relatively safe. I'll fix the
> reverted change up and get it work on VisionFive 2 when I got my new
> board. Sorry for the inconvenience.
> 
> [1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/
> 
>  arch/riscv/cpu/start.S | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 6324ff585d4..7bafdfd390a 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -151,15 +151,8 @@ call_harts_early_init:
>  	 */
>  	la	t0, hart_lottery
>  	li	t1, 1
> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>  	amoswap.w s2, t1, 0(t0)
>  	bnez	s2, wait_for_gd_init
> -#else
> -	lr.w	s2, (t0)
> -	bnez	s2, wait_for_gd_init
> -	sc.w	s2, t1, (t0)
> -	bnez	s2, wait_for_gd_init
> -#endif
>  #else
>  	/*
>  	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
> @@ -184,12 +177,7 @@ call_harts_early_init:
>  #if !CONFIG_IS_ENABLED(XIP)
>  #ifdef CONFIG_AVAILABLE_HARTS
>  	la	t0, available_harts_lock
> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>  	amoswap.w.rl zero, zero, 0(t0)
> -#else
> -	fence	rw, w
> -	sw	zero, 0(t0)
> -#endif
>  #endif
>  
>  wait_for_gd_init:
> @@ -202,14 +190,7 @@ wait_for_gd_init:
>  #ifdef CONFIG_AVAILABLE_HARTS
>  	la	t0, available_harts_lock
>  	li	t1, 1
> -1:
> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> -	amoswap.w.aq t1, t1, 0(t0)
> -#else
> -	lr.w.aq	t1, 0(t0)
> -	bnez	t1, 1b
> -	sc.w.rl t1, t1, 0(t0)
> -#endif
> +1:	amoswap.w.aq t1, t1, 0(t0)
>  	bnez	t1, 1b
>  
>  	/* register available harts in the available_harts mask */
> @@ -219,12 +200,7 @@ wait_for_gd_init:
>  	or	t2, t2, t1
>  	SREG	t2, GD_AVAILABLE_HARTS(gp)
>  
> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>  	amoswap.w.rl zero, zero, 0(t0)
> -#else
> -	fence	rw, w
> -	sw	zero, 0(t0)
> -#endif
>  #endif
>  
>  	/*

Argument in favor of reverting three patches in the series are as: I do
not understand the use of invisible config symbols in this way for a
transition of something so dependent on toolchain implementation. How
would I express this from menuconfig selections without directly editing
symbols in the config ?

I only have some JH7110 boards (and one EIC7700X there is not U-Boot for
this upstream yet) that I can offer to do some testing for, what all
hardware for should this affect?  It's every riscv board ?

Anyhow a revert of this one patch in the series is the minimum to
restore working SPL for starfive visionfive2 (as Star64, and Mars
CM/Lite). Thanks, Yao!

Acked-by: E Shattow <e@freeshell.de>

Re: [PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[Yao Zi]

On Thu, Sep 25, 2025 at 09:27:25AM -0700, E Shattow wrote:
> 
> 
> On 9/25/25 09:01, Yao Zi wrote:
> > This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.
> > 
> > It has been reported that the commit causes boot regression for SPL on
> > StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
> > spot one logic error for deciding whether Zaamo or Zalrsc is used, and
> > it's still unclear what exactly causes the regression, let's revert it
> > for now.
> > 
> > Reported-by: E Shattow <e@freeshell.de>
> > Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> > 
> > The original series causing the problem[1] contains 3 patches, and I
> > think it should be enough to revert the change of start.S only, since
> > the others touch no code, and should be relatively safe. I'll fix the
> > reverted change up and get it work on VisionFive 2 when I got my new
> > board. Sorry for the inconvenience.
> > 
> > [1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/
> > 
> >  arch/riscv/cpu/start.S | 26 +-------------------------
> >  1 file changed, 1 insertion(+), 25 deletions(-)
> > 
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 6324ff585d4..7bafdfd390a 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -151,15 +151,8 @@ call_harts_early_init:
> >  	 */
> >  	la	t0, hart_lottery
> >  	li	t1, 1
> > -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >  	amoswap.w s2, t1, 0(t0)
> >  	bnez	s2, wait_for_gd_init
> > -#else
> > -	lr.w	s2, (t0)
> > -	bnez	s2, wait_for_gd_init
> > -	sc.w	s2, t1, (t0)
> > -	bnez	s2, wait_for_gd_init
> > -#endif
> >  #else
> >  	/*
> >  	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
> > @@ -184,12 +177,7 @@ call_harts_early_init:
> >  #if !CONFIG_IS_ENABLED(XIP)
> >  #ifdef CONFIG_AVAILABLE_HARTS
> >  	la	t0, available_harts_lock
> > -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >  	amoswap.w.rl zero, zero, 0(t0)
> > -#else
> > -	fence	rw, w
> > -	sw	zero, 0(t0)
> > -#endif
> >  #endif
> >  
> >  wait_for_gd_init:
> > @@ -202,14 +190,7 @@ wait_for_gd_init:
> >  #ifdef CONFIG_AVAILABLE_HARTS
> >  	la	t0, available_harts_lock
> >  	li	t1, 1
> > -1:
> > -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> > -	amoswap.w.aq t1, t1, 0(t0)
> > -#else
> > -	lr.w.aq	t1, 0(t0)
> > -	bnez	t1, 1b
> > -	sc.w.rl t1, t1, 0(t0)
> > -#endif
> > +1:	amoswap.w.aq t1, t1, 0(t0)
> >  	bnez	t1, 1b
> >  
> >  	/* register available harts in the available_harts mask */
> > @@ -219,12 +200,7 @@ wait_for_gd_init:
> >  	or	t2, t2, t1
> >  	SREG	t2, GD_AVAILABLE_HARTS(gp)
> >  
> > -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >  	amoswap.w.rl zero, zero, 0(t0)
> > -#else
> > -	fence	rw, w
> > -	sw	zero, 0(t0)
> > -#endif
> >  #endif
> >  
> >  	/*
> 
> Argument in favor of reverting three patches in the series are as: I do
> not understand the use of invisible config symbols in this way for a
> transition of something so dependent on toolchain implementation. How
> would I express this from menuconfig selections without directly editing
> symbols in the config ?

I'm not sure what do you mean about "invisible", the two newly
introduced Kconfig symbols have their description, are visible in
menuconfig and thus could be changed by hand manually.

I introduced these symbols for describing whether Zaamo/Zalrsc is
availalbe on the targetted platform, and expecting one of them to be
explicitly disabled by board-level config if a platform doesn't support
Zalrsc or Zaamo, like how CONFIG_RISCV_ISA_A is tackled in
ibex-ast2700_defconfig.

> I only have some JH7110 boards (and one EIC7700X there is not U-Boot for
> this upstream yet) that I can offer to do some testing for, what all
> hardware for should this affect?  It's every riscv board ?

Technically speaking, yes, it introduces two Kconfig symbols that change
the logic of determining the ISA extension string for a build, so every
board is "affected".

However, among all the RISC-V board-level configs we've already
supported, only ibex-ast2700_defconfig explicitly disables
CONFIG_RISCV_ISA_A, which means all other ports are compiled with "a"
extension contained in ISA extension string before. For these ports, "a"
will still be added to the ISA extension string, since
CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC are default to "y".

For ibex-ast2700_defconfig, as long as we adjust the defconfig to
disable CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC, it will be
built with the same ISA extension string as before (without "a"), too,
which is done in PATCH 2.

I only revert PATCH 3 since it really causes problems: I'm willing to
rework it, and since it depends on PATCH 1, 2, they'll be probably added
back as-is latter when I send v3 if they have been reverted together. If
it's the case, I think reverting only PATCH 3 makes Git log clearer.

If it's found PATCH 1, 2 really break something, I'm willing to revert
them. Thanks for the comment.

> Anyhow a revert of this one patch in the series is the minimum to
> restore working SPL for starfive visionfive2 (as Star64, and Mars
> CM/Lite). Thanks, Yao!
> 
> Acked-by: E Shattow <e@freeshell.de>
> 

Best regards,
Yao Zi

Re: [PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[E Shattow]

Leo (and Tom?) please give Yao the attention they deserve for their
contributions, starfive_visionfive2 has been broken for several weeks by
this error and the revert needs to happen sooner rather than later. Thanks!

Hi Yao,

On 9/25/25 21:11, Yao Zi wrote:
> On Thu, Sep 25, 2025 at 09:27:25AM -0700, E Shattow wrote:
>>
>>
>> On 9/25/25 09:01, Yao Zi wrote:
>>> This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.
>>>
>>> It has been reported that the commit causes boot regression for SPL on
>>> StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
>>> spot one logic error for deciding whether Zaamo or Zalrsc is used, and
>>> it's still unclear what exactly causes the regression, let's revert it
>>> for now.
>>>
>>> Reported-by: E Shattow <e@freeshell.de>
>>> Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
>>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>>> ---
>>>
>>> The original series causing the problem[1] contains 3 patches, and I
>>> think it should be enough to revert the change of start.S only, since
>>> the others touch no code, and should be relatively safe. I'll fix the
>>> reverted change up and get it work on VisionFive 2 when I got my new
>>> board. Sorry for the inconvenience.
>>>
>>> [1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/
>>>
>>>  arch/riscv/cpu/start.S | 26 +-------------------------
>>>  1 file changed, 1 insertion(+), 25 deletions(-)
>>>
>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>>> index 6324ff585d4..7bafdfd390a 100644
>>> --- a/arch/riscv/cpu/start.S
>>> +++ b/arch/riscv/cpu/start.S
>>> @@ -151,15 +151,8 @@ call_harts_early_init:
>>>  	 */
>>>  	la	t0, hart_lottery
>>>  	li	t1, 1
>>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>>>  	amoswap.w s2, t1, 0(t0)
>>>  	bnez	s2, wait_for_gd_init
>>> -#else
>>> -	lr.w	s2, (t0)
>>> -	bnez	s2, wait_for_gd_init
>>> -	sc.w	s2, t1, (t0)
>>> -	bnez	s2, wait_for_gd_init
>>> -#endif
>>>  #else
>>>  	/*
>>>  	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
>>> @@ -184,12 +177,7 @@ call_harts_early_init:
>>>  #if !CONFIG_IS_ENABLED(XIP)
>>>  #ifdef CONFIG_AVAILABLE_HARTS
>>>  	la	t0, available_harts_lock
>>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>>>  	amoswap.w.rl zero, zero, 0(t0)
>>> -#else
>>> -	fence	rw, w
>>> -	sw	zero, 0(t0)
>>> -#endif
>>>  #endif
>>>  
>>>  wait_for_gd_init:
>>> @@ -202,14 +190,7 @@ wait_for_gd_init:
>>>  #ifdef CONFIG_AVAILABLE_HARTS
>>>  	la	t0, available_harts_lock
>>>  	li	t1, 1
>>> -1:
>>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>>> -	amoswap.w.aq t1, t1, 0(t0)
>>> -#else
>>> -	lr.w.aq	t1, 0(t0)
>>> -	bnez	t1, 1b
>>> -	sc.w.rl t1, t1, 0(t0)
>>> -#endif
>>> +1:	amoswap.w.aq t1, t1, 0(t0)
>>>  	bnez	t1, 1b
>>>  
>>>  	/* register available harts in the available_harts mask */
>>> @@ -219,12 +200,7 @@ wait_for_gd_init:
>>>  	or	t2, t2, t1
>>>  	SREG	t2, GD_AVAILABLE_HARTS(gp)
>>>  
>>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
>>>  	amoswap.w.rl zero, zero, 0(t0)
>>> -#else
>>> -	fence	rw, w
>>> -	sw	zero, 0(t0)
>>> -#endif
>>>  #endif
>>>  
>>>  	/*
>>
>> Argument in favor of reverting three patches in the series are as: I do
>> not understand the use of invisible config symbols in this way for a
>> transition of something so dependent on toolchain implementation. How
>> would I express this from menuconfig selections without directly editing
>> symbols in the config ?
> 
> I'm not sure what do you mean about "invisible", the two newly
> introduced Kconfig symbols have their description, are visible in
> menuconfig and thus could be changed by hand manually.

Invisible means you cannot select the option with menuconfig. Visible
symbols show up as user-selectable in the menuconfig interface.

> 
> I introduced these symbols for describing whether Zaamo/Zalrsc is
> availalbe on the targetted platform, and expecting one of them to be
> explicitly disabled by board-level config if a platform doesn't support
> Zalrsc or Zaamo, like how CONFIG_RISCV_ISA_A is tackled in
> ibex-ast2700_defconfig.
> 
>> I only have some JH7110 boards (and one EIC7700X there is not U-Boot for
>> this upstream yet) that I can offer to do some testing for, what all
>> hardware for should this affect?  It's every riscv board ?
> 
> Technically speaking, yes, it introduces two Kconfig symbols that change
> the logic of determining the ISA extension string for a build, so every
> board is "affected".

Yes, then this is a failure to test affected configurations, and further
a failure to communicate this fact in the series and in the commit
message. Now the `git bisect` story is utterly broken for visionfive2
for a month worth of commits since.

Because of some unfortunate luck the mail host I am using was offline
for maintenance when your series posted. I was not aware of this
breaking change sooner or else I would have resolved my own concerns now
by having tested and found the error then, and especially if there was
another revision or two of the series that made some mention asking to
test this hardware specifically.

> 
> However, among all the RISC-V board-level configs we've already
> supported, only ibex-ast2700_defconfig explicitly disables
> CONFIG_RISCV_ISA_A, which means all other ports are compiled with "a"
> extension contained in ISA extension string before. For these ports, "a"
> will still be added to the ISA extension string, since
> CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC are default to "y".
> 
> For ibex-ast2700_defconfig, as long as we adjust the defconfig to
> disable CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC, it will be
> built with the same ISA extension string as before (without "a"), too,
> which is done in PATCH 2.
> 
> I only revert PATCH 3 since it really causes problems: I'm willing to
> rework it, and since it depends on PATCH 1, 2, they'll be probably added
> back as-is latter when I send v3 if they have been reverted together. If
> it's the case, I think reverting only PATCH 3 makes Git log clearer.
> 
> If it's found PATCH 1, 2 really break something, I'm willing to revert
> them. Thanks for the comment.

It is not helpful to argue about code which you did not communicate
upfront of what was tested and what was not. I would trust your opinion
however it is equally important to verify your assertions, and that has
not been done properly.

> 
>> Anyhow a revert of this one patch in the series is the minimum to
>> restore working SPL for starfive visionfive2 (as Star64, and Mars
>> CM/Lite). Thanks, Yao!
>>
>> Acked-by: E Shattow <e@freeshell.de>
>>
> 
> Best regards,
> Yao Zi

I emphasize that I appreciate your work, Yao.

Sincerely,

-E

[SPAM Warning!]Re: [PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[Yao Zi]

On Sun, Oct 12, 2025 at 02:49:59PM -0700, E Shattow wrote:
> Leo (and Tom?) please give Yao the attention they deserve for their
> contributions, starfive_visionfive2 has been broken for several weeks by
> this error and the revert needs to happen sooner rather than later. Thanks!
> 
> Hi Yao,
> 
> On 9/25/25 21:11, Yao Zi wrote:
> > On Thu, Sep 25, 2025 at 09:27:25AM -0700, E Shattow wrote:
> >>
> >>
> >> On 9/25/25 09:01, Yao Zi wrote:
> >>> This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.
> >>>
> >>> It has been reported that the commit causes boot regression for SPL on
> >>> StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
> >>> spot one logic error for deciding whether Zaamo or Zalrsc is used, and
> >>> it's still unclear what exactly causes the regression, let's revert it
> >>> for now.
> >>>
> >>> Reported-by: E Shattow <e@freeshell.de>
> >>> Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
> >>> Signed-off-by: Yao Zi <ziyao@disroot.org>
> >>> ---
> >>>
> >>> The original series causing the problem[1] contains 3 patches, and I
> >>> think it should be enough to revert the change of start.S only, since
> >>> the others touch no code, and should be relatively safe. I'll fix the
> >>> reverted change up and get it work on VisionFive 2 when I got my new
> >>> board. Sorry for the inconvenience.
> >>>
> >>> [1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/
> >>>
> >>>  arch/riscv/cpu/start.S | 26 +-------------------------
> >>>  1 file changed, 1 insertion(+), 25 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> >>> index 6324ff585d4..7bafdfd390a 100644
> >>> --- a/arch/riscv/cpu/start.S
> >>> +++ b/arch/riscv/cpu/start.S
> >>> @@ -151,15 +151,8 @@ call_harts_early_init:
> >>>  	 */
> >>>  	la	t0, hart_lottery
> >>>  	li	t1, 1
> >>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >>>  	amoswap.w s2, t1, 0(t0)
> >>>  	bnez	s2, wait_for_gd_init
> >>> -#else
> >>> -	lr.w	s2, (t0)
> >>> -	bnez	s2, wait_for_gd_init
> >>> -	sc.w	s2, t1, (t0)
> >>> -	bnez	s2, wait_for_gd_init
> >>> -#endif
> >>>  #else
> >>>  	/*
> >>>  	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
> >>> @@ -184,12 +177,7 @@ call_harts_early_init:
> >>>  #if !CONFIG_IS_ENABLED(XIP)
> >>>  #ifdef CONFIG_AVAILABLE_HARTS
> >>>  	la	t0, available_harts_lock
> >>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >>>  	amoswap.w.rl zero, zero, 0(t0)
> >>> -#else
> >>> -	fence	rw, w
> >>> -	sw	zero, 0(t0)
> >>> -#endif
> >>>  #endif
> >>>  
> >>>  wait_for_gd_init:
> >>> @@ -202,14 +190,7 @@ wait_for_gd_init:
> >>>  #ifdef CONFIG_AVAILABLE_HARTS
> >>>  	la	t0, available_harts_lock
> >>>  	li	t1, 1
> >>> -1:
> >>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >>> -	amoswap.w.aq t1, t1, 0(t0)
> >>> -#else
> >>> -	lr.w.aq	t1, 0(t0)
> >>> -	bnez	t1, 1b
> >>> -	sc.w.rl t1, t1, 0(t0)
> >>> -#endif
> >>> +1:	amoswap.w.aq t1, t1, 0(t0)
> >>>  	bnez	t1, 1b
> >>>  
> >>>  	/* register available harts in the available_harts mask */
> >>> @@ -219,12 +200,7 @@ wait_for_gd_init:
> >>>  	or	t2, t2, t1
> >>>  	SREG	t2, GD_AVAILABLE_HARTS(gp)
> >>>  
> >>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >>>  	amoswap.w.rl zero, zero, 0(t0)
> >>> -#else
> >>> -	fence	rw, w
> >>> -	sw	zero, 0(t0)
> >>> -#endif
> >>>  #endif
> >>>  
> >>>  	/*
> >>
> >> Argument in favor of reverting three patches in the series are as: I do
> >> not understand the use of invisible config symbols in this way for a
> >> transition of something so dependent on toolchain implementation. How
> >> would I express this from menuconfig selections without directly editing
> >> symbols in the config ?
> > 
> > I'm not sure what do you mean about "invisible", the two newly
> > introduced Kconfig symbols have their description, are visible in
> > menuconfig and thus could be changed by hand manually.
> 
> Invisible means you cannot select the option with menuconfig. Visible
> symbols show up as user-selectable in the menuconfig interface.

But RISCV_ISA_ZAAMO and RISCV_ISA_ZALRSC are visible in menuconfig, you
could find them in "RISC-V architecture" -> "Standard extension for
Atomic Memory Operations" / "Standard extensions for LR/SC
instructions". I do provide description for them and don't realize why
you think they're invisible in the menu.

> > 
> > I introduced these symbols for describing whether Zaamo/Zalrsc is
> > availalbe on the targetted platform, and expecting one of them to be
> > explicitly disabled by board-level config if a platform doesn't support
> > Zalrsc or Zaamo, like how CONFIG_RISCV_ISA_A is tackled in
> > ibex-ast2700_defconfig.
> > 
> >> I only have some JH7110 boards (and one EIC7700X there is not U-Boot for
> >> this upstream yet) that I can offer to do some testing for, what all
> >> hardware for should this affect?  It's every riscv board ?
> > 
> > Technically speaking, yes, it introduces two Kconfig symbols that change
> > the logic of determining the ISA extension string for a build, so every
> > board is "affected".
> 
> Yes, then this is a failure to test affected configurations, and further
> a failure to communicate this fact in the series and in the commit
> message. Now the `git bisect` story is utterly broken for visionfive2
> for a month worth of commits since.
> 
> Because of some unfortunate luck the mail host I am using was offline
> for maintenance when your series posted. I was not aware of this
> breaking change sooner or else I would have resolved my own concerns now
> by having tested and found the error then, and especially if there was
> another revision or two of the series that made some mention asking to
> test this hardware specifically.

I should have been more explicit about affected platforms in the
original series, which may raise more attention on testing and review.
Will do it in v3, and I'm sorry for the mistake.

> > 
> > However, among all the RISC-V board-level configs we've already
> > supported, only ibex-ast2700_defconfig explicitly disables
> > CONFIG_RISCV_ISA_A, which means all other ports are compiled with "a"
> > extension contained in ISA extension string before. For these ports, "a"
> > will still be added to the ISA extension string, since
> > CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC are default to "y".
> > 
> > For ibex-ast2700_defconfig, as long as we adjust the defconfig to
> > disable CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC, it will be
> > built with the same ISA extension string as before (without "a"), too,
> > which is done in PATCH 2.
> > 
> > I only revert PATCH 3 since it really causes problems: I'm willing to
> > rework it, and since it depends on PATCH 1, 2, they'll be probably added
> > back as-is latter when I send v3 if they have been reverted together. If
> > it's the case, I think reverting only PATCH 3 makes Git log clearer.
> > 
> > If it's found PATCH 1, 2 really break something, I'm willing to revert
> > them. Thanks for the comment.
> 
> It is not helpful to argue about code which you did not communicate
> upfront of what was tested and what was not. I would trust your opinion
> however it is equally important to verify your assertions, and that has
> not been done properly.

For verification for the first two patches, as it only modifies the
logic to determine availability of RISCV_ISA_A Kconfig option, and the
option is only used to construct the -march compiler option string, I've
written a script to automatically compare the "-march" argument passed
to compilers with or without the first two patches applied[1].

I've run it locally for all mainline U-Boot ports, and found no
-march argument difference with the two patches applied. Do you think
it's a clue strong enough to consider the two patches safe and
unnecessary to be reverted?

Best regards,
Yao Zi

[1]: https://gist.github.com/ziyao233/13490f0a51bd556bfeb6017427c3b594

> > 
> >> Anyhow a revert of this one patch in the series is the minimum to
> >> restore working SPL for starfive visionfive2 (as Star64, and Mars
> >> CM/Lite). Thanks, Yao!
> >>
> >> Acked-by: E Shattow <e@freeshell.de>
> >>
> > 
> > Best regards,
> > Yao Zi
> 
> I emphasize that I appreciate your work, Yao.
> 
> Sincerely,
> 
> -E

Re: [PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"

[Leo Liang]

On Thu, Sep 25, 2025 at 04:01:48PM +0000, Yao Zi wrote:
> [EXTERNAL MAIL]
> 
> This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.
> 
> It has been reported that the commit causes boot regression for SPL on
> StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
> spot one logic error for deciding whether Zaamo or Zalrsc is used, and
> it's still unclear what exactly causes the regression, let's revert it
> for now.
> 
> Reported-by: E Shattow <e@freeshell.de>
> Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
> 
> The original series causing the problem[1] contains 3 patches, and I
> think it should be enough to revert the change of start.S only, since
> the others touch no code, and should be relatively safe. I'll fix the
> reverted change up and get it work on VisionFive 2 when I got my new
> board. Sorry for the inconvenience.
> 
> [1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/
> 
>  arch/riscv/cpu/start.S | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)

Acked-by: Leo Yu-Chi Liang <ycliang@andestech.com>