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

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

[Mateusz Kulikowski]

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.

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.
+
 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.
+	 *
 	 */
-	mrs     x0, S3_1_c15_c2_1               /* cpuactlr_el1 */
+#ifdef CONFIG_ARMV8_SET_SMPEN
+	mrs     x0, S3_1_c15_c2_1               /* cpuectlr_el1 */
 	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 => "
-- 
2.7.4

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

[André Przywara]

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 => "
> 

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

[Tom Rini]

On Tue, Jul 12, 2016 at 11:10:28PM +0100, Andr? Przywara wrote:
> 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?

OK, I'm going to revert the original patch here (and would recommend
that distros that are picking up v2016.07 do the same).  Thanks all!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160714/03931d88/attachment.sig>