[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 at freeshell.de>
>>> Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
>>> Signed-off-by: Yao Zi <ziyao at 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 at freeshell.de>
>>
> 
> Best regards,
> Yao Zi

I emphasize that I appreciate your work, Yao.

Sincerely,

-E