[U-Boot] [PATCH] arm: start.S: Fix startup of dragonboard410c

["André Przywara" <andre.przywara@arm.com>]

On 12/07/16 21:15, Mateusz Kulikowski wrote:

Hi Mateusz,

> Commit d73718f3 breaks devices where:
> - There is EL2/EL3 firmware and
> - U-Boot starts in NS EL1 and
> - EL2/EL3 firmware didn't unlocked write-access to CPUECTLR_EL1.
> 
> This patch makes that change opt-out configuration option,
> and disables it for dragonboard410c.

thanks for bringing this up!
You are totally right, as this register isn't architecturally defined,
we should never set it without checking for a particular core.
Actually even that is not enough, as it's also access restricted - even
defaulting to RO - as you mentioned.

So I am very much for a board specific _opt-in_ solution, also as ARM
Trusted Firmware takes care of the bit already, for instance.

Mingkai Hu, can you say for which board/SoC/family the original patch
was needed for? Layerscape?

...

> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
> ---
> I make it opt-out so nobody will complain that I broke someones board.
> 
> If you prefer opt-in - just let me know, I'll flip the switch
> in Kconfig.
> 
> BTW. I wonder if this register should be written at all on devices
> that implement ARMv8 but are *not* ARM Cortex.
> 
> Mateusz
> 
>  arch/arm/cpu/armv8/Kconfig        | 13 +++++++++++++
>  arch/arm/cpu/armv8/start.S        |  7 +++++--
>  configs/dragonboard410c_defconfig |  1 +
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
> index 3d19bbf..33af0a2 100644
> --- a/arch/arm/cpu/armv8/Kconfig
> +++ b/arch/arm/cpu/armv8/Kconfig
> @@ -3,4 +3,17 @@ if ARM64
>  config ARMV8_MULTIENTRY
>          boolean "Enable multiple CPUs to enter into U-Boot"
>  
> +config ARMV8_SET_SMPEN
> +        boolean "Enable data coherency with other cores in cluster"
> +        default y if ARM64
> +        help
> +          Cortex A53/57/72 cores require CPUECTLR_EL1.SMPEN set even
> +          for single core systems. Unfortunately write access to this
> +          register may be controlled by EL3/EL2 firmware. To be more
> +          precise, by default (if there is EL2/EL3 firmware running)
> +          this register is RO for NS EL1.
> +          This switch can be used to avoid writing to CPUECTLR_EL1,
> +          it can be safely enabled when El2/EL3 initialized SMPEN bit
> +          or when CPU implementation doesn't include that register.

I don't think we need this option to be user visible. Just a simple
"boolean" should suffice, boards in need for the bit should select it in
their defconfig or in a board specific Kconfig file.
Either it's needed on board or not, a _user_ shouldn't be bothered with
that choice.
You can copy the help text into the commit message.

> +
>  endif
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index dfce469..777cad3 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -81,13 +81,16 @@ reset:
>  	msr	cpacr_el1, x0			/* Enable FP/SIMD */
>  0:
>  
> -	/* Enalbe SMPEN bit for coherency.
> +	/* Enable SMPEN bit for coherency.
>  	 * This register is not architectural but at the moment
>  	 * this bit should be set for A53/A57/A72.
> +	 *

Shouldn't this extra line be at the beginning of the comment instead?

>  	 */
> -	mrs     x0, S3_1_c15_c2_1               /* cpuactlr_el1 */
> +#ifdef CONFIG_ARMV8_SET_SMPEN
> +	mrs     x0, S3_1_c15_c2_1               /* cpuectlr_el1 */

Ah, thanks for fixing that typo, I was confused for a moment or two.

Thanks,
Andre

>  	orr     x0, x0, #0x40
>  	msr     S3_1_c15_c2_1, x0
> +#endif
>  
>  	/* Apply ARM core specific erratas */
>  	bl	apply_core_errata
> diff --git a/configs/dragonboard410c_defconfig b/configs/dragonboard410c_defconfig
> index ad2e8b8..d3cfa69 100644
> --- a/configs/dragonboard410c_defconfig
> +++ b/configs/dragonboard410c_defconfig
> @@ -1,5 +1,6 @@
>  CONFIG_ARM=y
>  CONFIG_ARCH_SNAPDRAGON=y
> +CONFIG_ARMV8_SET_SMPEN=n
>  CONFIG_DEFAULT_DEVICE_TREE="dragonboard410c"
>  CONFIG_HUSH_PARSER=y
>  CONFIG_SYS_PROMPT="dragonboard410c => "
>