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