| Message ID | 1482925115-19527-1-git-send-email-oded.gabbay@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
+Tom On 28 December 2016 at 04:38, Oded Gabbay <oded.gabbay@gmail.com> wrote: > When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to > wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them > by writing to that location. The address of spin_table_cpu_release_addr is > transferred to the kernel using the device tree that is updated by > spin_table_update_dt(). > > However, if we also use SPL, then the slave cores are stuck at > CPU_RELEASE_ADDR instead and as a result, never wake up. > > This patch releases the slave cores by writing spl_image->entry_point to > CPU_RELEASE_ADDR location before the end of the SPL code > (at jump_to_image_no_args()). > > That way, the slave cores will start to execute the u-boot and will get to > the spin-table code and wait on the correct address > (spin_table_cpu_release_addr). > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > --- > common/spl/spl.c | 8 ++++++++ > 1 file changed, 8 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org>
On Wed, Dec 28, 2016 at 01:38:35PM +0200, Oded Gabbay wrote: > When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to > wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them > by writing to that location. The address of spin_table_cpu_release_addr is > transferred to the kernel using the device tree that is updated by > spin_table_update_dt(). > > However, if we also use SPL, then the slave cores are stuck at > CPU_RELEASE_ADDR instead and as a result, never wake up. > > This patch releases the slave cores by writing spl_image->entry_point to > CPU_RELEASE_ADDR location before the end of the SPL code > (at jump_to_image_no_args()). > > That way, the slave cores will start to execute the u-boot and will get to > the spin-table code and wait on the correct address > (spin_table_cpu_release_addr). > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
2017-01-16 3:30 GMT+09:00 Tom Rini <trini@konsulko.com>: > On Wed, Dec 28, 2016 at 01:38:35PM +0200, Oded Gabbay wrote: > >> When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to >> wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them >> by writing to that location. The address of spin_table_cpu_release_addr is >> transferred to the kernel using the device tree that is updated by >> spin_table_update_dt(). >> >> However, if we also use SPL, then the slave cores are stuck at >> CPU_RELEASE_ADDR instead and as a result, never wake up. >> >> This patch releases the slave cores by writing spl_image->entry_point to >> CPU_RELEASE_ADDR location before the end of the SPL code >> (at jump_to_image_no_args()). >> >> That way, the slave cores will start to execute the u-boot and will get to >> the spin-table code and wait on the correct address >> (spin_table_cpu_release_addr). >> >> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >> Cc: Simon Glass <sjg@chromium.org> >> Reviewed-by: Simon Glass <sjg@chromium.org> > > Applied to u-boot/master, thanks! I am the author of the spin table support of U-Boot. I should have checked this patch, but unfortunately I had not noticed it until I saw the commit in the upstream code. I am planning to revert it with the following log: Revert "armv8: release slave cores from CPU_RELEASE_ADDR" This reverts commit 8c36e99f211104fd7dcbf0669a35a47ce5e154f5. There is misunderstanding in commit 8c36e99f2111 ("armv8: release slave cores from CPU_RELEASE_ADDR"). How to bring the slave cores into U-Boot proper is platform-specific. So, it should be cared in SoC/board files instead of common/spl/spl.c. As you see SPL is the acronym of Secondary Program Loader, there is generally something that runs before SPL (the First one is usually Boot ROM). How to wake up slave cores from the Boot ROM is really SoC specific. So, the intention for the spin table support is to bring the slave cores into U-Boot proper (this must be done after relocation. see below) in an SoC specific manner. It is still possible to let the slave cores start from SPL, but it is not a common usecase that should be supported in the common code. One more thing is missing in the commit; spl_image->entry_point points to the entry address of U-Boot *before* relocation. U-Boot relocates itself between board_init_f() and board_init_r(). This means the master CPU sees the different copy of the spin table code than the slave CPUs are really spinning. The spin_table_update_dt() protects the code *after* relocation. As a result, the slave CPUs spin in unprotected code, this leads to unstable behavior.
On Tue, Jan 17, 2017 at 11:07 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2017-01-16 3:30 GMT+09:00 Tom Rini <trini@konsulko.com>: >> On Wed, Dec 28, 2016 at 01:38:35PM +0200, Oded Gabbay wrote: >> >>> When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to >>> wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them >>> by writing to that location. The address of spin_table_cpu_release_addr is >>> transferred to the kernel using the device tree that is updated by >>> spin_table_update_dt(). >>> >>> However, if we also use SPL, then the slave cores are stuck at >>> CPU_RELEASE_ADDR instead and as a result, never wake up. >>> >>> This patch releases the slave cores by writing spl_image->entry_point to >>> CPU_RELEASE_ADDR location before the end of the SPL code >>> (at jump_to_image_no_args()). >>> >>> That way, the slave cores will start to execute the u-boot and will get to >>> the spin-table code and wait on the correct address >>> (spin_table_cpu_release_addr). >>> >>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>> Cc: Simon Glass <sjg@chromium.org> >>> Reviewed-by: Simon Glass <sjg@chromium.org> >> >> Applied to u-boot/master, thanks! > > > I am the author of the spin table support of U-Boot. > > I should have checked this patch, but unfortunately > I had not noticed it until I saw the commit in the upstream code. > > I am planning to revert it with the following log: > > > Revert "armv8: release slave cores from CPU_RELEASE_ADDR" > > This reverts commit 8c36e99f211104fd7dcbf0669a35a47ce5e154f5. > > There is misunderstanding in commit 8c36e99f2111 ("armv8: release > slave cores from CPU_RELEASE_ADDR"). How to bring the slave cores > into U-Boot proper is platform-specific. So, it should be cared > in SoC/board files instead of common/spl/spl.c. As you see SPL > is the acronym of Secondary Program Loader, there is generally > something that runs before SPL (the First one is usually Boot ROM). > > How to wake up slave cores from the Boot ROM is really SoC specific. > So, the intention for the spin table support is to bring the slave > cores into U-Boot proper (this must be done after relocation. see > below) in an SoC specific manner. It is still possible to let the > slave cores start from SPL, but it is not a common usecase that should > be supported in the common code. > Then that should be written with CAPITAL letters somewhere in the code/documentation, because what happens now is that in the case all the cores do wake up at the start of the SPL, the slave cores will be forever stuck. Is there a common place to wake up secondary cores / release them from CPU_RELEASE_ADDR ? arch_early_init_r() perhaps ? > One more thing is missing in the commit; spl_image->entry_point > points to the entry address of U-Boot *before* relocation. U-Boot > relocates itself between board_init_f() and board_init_r(). This > means the master CPU sees the different copy of the spin table code > than the slave CPUs are really spinning. The spin_table_update_dt() > protects the code *after* relocation. As a result, the slave CPUs > spin in unprotected code, this leads to unstable behavior. > Agreed. This is indeed wrong. Thanks, Oded > > > > -- > Best Regards > Masahiro Yamada
2017-01-17 18:20 GMT+09:00 Oded Gabbay <oded.gabbay@gmail.com>: > On Tue, Jan 17, 2017 at 11:07 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> 2017-01-16 3:30 GMT+09:00 Tom Rini <trini@konsulko.com>: >>> On Wed, Dec 28, 2016 at 01:38:35PM +0200, Oded Gabbay wrote: >>> >>>> When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to >>>> wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them >>>> by writing to that location. The address of spin_table_cpu_release_addr is >>>> transferred to the kernel using the device tree that is updated by >>>> spin_table_update_dt(). >>>> >>>> However, if we also use SPL, then the slave cores are stuck at >>>> CPU_RELEASE_ADDR instead and as a result, never wake up. >>>> >>>> This patch releases the slave cores by writing spl_image->entry_point to >>>> CPU_RELEASE_ADDR location before the end of the SPL code >>>> (at jump_to_image_no_args()). >>>> >>>> That way, the slave cores will start to execute the u-boot and will get to >>>> the spin-table code and wait on the correct address >>>> (spin_table_cpu_release_addr). >>>> >>>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>>> Cc: Simon Glass <sjg@chromium.org> >>>> Reviewed-by: Simon Glass <sjg@chromium.org> >>> >>> Applied to u-boot/master, thanks! >> >> >> I am the author of the spin table support of U-Boot. >> >> I should have checked this patch, but unfortunately >> I had not noticed it until I saw the commit in the upstream code. >> >> I am planning to revert it with the following log: >> >> >> Revert "armv8: release slave cores from CPU_RELEASE_ADDR" >> >> This reverts commit 8c36e99f211104fd7dcbf0669a35a47ce5e154f5. >> >> There is misunderstanding in commit 8c36e99f2111 ("armv8: release >> slave cores from CPU_RELEASE_ADDR"). How to bring the slave cores >> into U-Boot proper is platform-specific. So, it should be cared >> in SoC/board files instead of common/spl/spl.c. As you see SPL >> is the acronym of Secondary Program Loader, there is generally >> something that runs before SPL (the First one is usually Boot ROM). >> >> How to wake up slave cores from the Boot ROM is really SoC specific. >> So, the intention for the spin table support is to bring the slave >> cores into U-Boot proper (this must be done after relocation. see >> below) in an SoC specific manner. It is still possible to let the >> slave cores start from SPL, but it is not a common usecase that should >> be supported in the common code. >> > Then that should be written with CAPITAL letters somewhere in the > code/documentation, because what happens now is that in the case all > the cores do wake up at the start of the SPL, the slave cores will be > forever stuck. I mentioned "bring in the slaves in a board specific manner" in the current Kconfig help. I meant "U-Boot" as "U-Boot proper" (or aka U-Boot full image) instead of SPL. I reworded to clarify it even more. http://patchwork.ozlabs.org/patch/717038/ Please let me know if there is something is unclear, or you are not convinced with the explanation. > Is there a common place to wake up secondary cores / release them from > CPU_RELEASE_ADDR ? arch_early_init_r() perhaps ? Anywhere suitable for you if it is run after relocation. If you look at common/board_r.c, you will see various callbacks, board_early_init_r(), board_init(), board_late_init, etc. For your information, my own code for spin-table is arch/arm/mach-uniphier/smp_kick_cpus.c this is called from board_init().
On Thu, Jan 19, 2017 at 2:22 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2017-01-17 18:20 GMT+09:00 Oded Gabbay <oded.gabbay@gmail.com>: >> On Tue, Jan 17, 2017 at 11:07 AM, Masahiro Yamada >> <yamada.masahiro@socionext.com> wrote: >>> 2017-01-16 3:30 GMT+09:00 Tom Rini <trini@konsulko.com>: >>>> On Wed, Dec 28, 2016 at 01:38:35PM +0200, Oded Gabbay wrote: >>>> >>>>> When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to >>>>> wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them >>>>> by writing to that location. The address of spin_table_cpu_release_addr is >>>>> transferred to the kernel using the device tree that is updated by >>>>> spin_table_update_dt(). >>>>> >>>>> However, if we also use SPL, then the slave cores are stuck at >>>>> CPU_RELEASE_ADDR instead and as a result, never wake up. >>>>> >>>>> This patch releases the slave cores by writing spl_image->entry_point to >>>>> CPU_RELEASE_ADDR location before the end of the SPL code >>>>> (at jump_to_image_no_args()). >>>>> >>>>> That way, the slave cores will start to execute the u-boot and will get to >>>>> the spin-table code and wait on the correct address >>>>> (spin_table_cpu_release_addr). >>>>> >>>>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>>>> Cc: Simon Glass <sjg@chromium.org> >>>>> Reviewed-by: Simon Glass <sjg@chromium.org> >>>> >>>> Applied to u-boot/master, thanks! >>> >>> >>> I am the author of the spin table support of U-Boot. >>> >>> I should have checked this patch, but unfortunately >>> I had not noticed it until I saw the commit in the upstream code. >>> >>> I am planning to revert it with the following log: >>> >>> >>> Revert "armv8: release slave cores from CPU_RELEASE_ADDR" >>> >>> This reverts commit 8c36e99f211104fd7dcbf0669a35a47ce5e154f5. >>> >>> There is misunderstanding in commit 8c36e99f2111 ("armv8: release >>> slave cores from CPU_RELEASE_ADDR"). How to bring the slave cores >>> into U-Boot proper is platform-specific. So, it should be cared >>> in SoC/board files instead of common/spl/spl.c. As you see SPL >>> is the acronym of Secondary Program Loader, there is generally >>> something that runs before SPL (the First one is usually Boot ROM). >>> >>> How to wake up slave cores from the Boot ROM is really SoC specific. >>> So, the intention for the spin table support is to bring the slave >>> cores into U-Boot proper (this must be done after relocation. see >>> below) in an SoC specific manner. It is still possible to let the >>> slave cores start from SPL, but it is not a common usecase that should >>> be supported in the common code. >>> >> Then that should be written with CAPITAL letters somewhere in the >> code/documentation, because what happens now is that in the case all >> the cores do wake up at the start of the SPL, the slave cores will be >> forever stuck. > > I mentioned "bring in the slaves in a board specific manner" > in the current Kconfig help. > > I meant "U-Boot" as "U-Boot proper" (or aka U-Boot full image) > instead of SPL. I reworded to clarify it even more. > > http://patchwork.ozlabs.org/patch/717038/ > > Please let me know if there is something is unclear, > or you are not convinced with the explanation. > I think it is much more clearer now. I sent an r-b to the ML. > > >> Is there a common place to wake up secondary cores / release them from >> CPU_RELEASE_ADDR ? arch_early_init_r() perhaps ? > > Anywhere suitable for you > if it is run after relocation. > > If you look at common/board_r.c, you will see > various callbacks, board_early_init_r(), board_init(), board_late_init, etc. > > For your information, my own code for spin-table is > arch/arm/mach-uniphier/smp_kick_cpus.c > this is called from board_init(). > Thanks for the tip, I'll take a look. Oded > > > > > > -- > Best Regards > Masahiro Yamada
>> For your information, my own code for spin-table is >> arch/arm/mach-uniphier/smp_kick_cpus.c >> this is called from board_init(). >> > Thanks for the tip, I'll take a look. I meant arch/arm/mach-uniphier/arm64/smp_kick_cpus.c
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..63d7db9 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -167,6 +167,14 @@ __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) (image_entry_noargs_t)(unsigned long)spl_image->entry_point; debug("image entry point: 0x%X\n", spl_image->entry_point); +#if defined(CONFIG_ARMV8_SPIN_TABLE) && defined(CONFIG_ARMV8_MULTIENTRY) + /* + * Release all slave cores from CPU_RELEASE_ADDR so they could + * arrive to the spin-table code in start.S of the u-boot + */ + *(ulong *)CPU_RELEASE_ADDR = (ulong)spl_image->entry_point; +#endif + image_entry(); }
When using ARMv8 with ARMV8_SPIN_TABLE=y, we want the slave cores to wait on spin_table_cpu_release_addr, until the Linux kernel will "wake" them by writing to that location. The address of spin_table_cpu_release_addr is transferred to the kernel using the device tree that is updated by spin_table_update_dt(). However, if we also use SPL, then the slave cores are stuck at CPU_RELEASE_ADDR instead and as a result, never wake up. This patch releases the slave cores by writing spl_image->entry_point to CPU_RELEASE_ADDR location before the end of the SPL code (at jump_to_image_no_args()). That way, the slave cores will start to execute the u-boot and will get to the spin-table code and wait on the correct address (spin_table_cpu_release_addr). Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> Cc: Simon Glass <sjg@chromium.org> --- common/spl/spl.c | 8 ++++++++ 1 file changed, 8 insertions(+)