| Message ID | 20250925160148.21624-1-ziyao@disroot.org |
|---|---|
| State | Accepted |
| Commit | e6646b35f410c4ffbdb0f309d4dad1e16c1e4714 |
| Delegated to: | Andes |
| Headers | show |
| Series | Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S" | expand |
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>
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
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>
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
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
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 /*
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(-)