[U-Boot] armv8: release slave cores from CPU_RELEASE_ADDR

[Oded Gabbay]

On Tue, Jan 17, 2017 at 11:07 AM, Masahiro Yamada
<yamada.masahiro at socionext.com> wrote:
> 2017-01-16 3:30 GMT+09:00 Tom Rini <trini at 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 at gmail.com>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Reviewed-by: Simon Glass <sjg at 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