Re: [PATCH 02/16] ARM: mvebu: Add a common function for the boot address work around

Thomas Petazzoni
2014-07-01T14:57:16Z

Dear Gregory CLEMENT,

On Fri, 27 Jun 2014 15:22:43 +0200, Gregory CLEMENT wrote:

> +extern unsigned char mvebu_boot_wa_start;
> +extern unsigned char mvebu_boot_wa_end;
> +
> +void mvebu_boot_addr_wa(int crypto_eng_id, u32 resume_addr_reg)
> +{
> + void __iomem *sram_virt_base;
> + u32 code_len = &mvebu_boot_wa_end - &mvebu_boot_wa_start;
> +
> + mvebu_mbus_del_window(BOOTROM_BASE, BOOTROM_SIZE);
> + mvebu_mbus_add_window_by_id(crypto_eng_id, CRYPT0_ENG_ATTR,
> + SRAM_PHYS_BASE, SZ_64K);
> + sram_virt_base = ioremap(SRAM_PHYS_BASE, SZ_64K);
> +
> +
> + memcpy(sram_virt_base, &mvebu_boot_wa_start, code_len);
> + /*
> + * The last word of the code copied in SRAM must contain the
> + * physical base address of the PMSU register
> + */
> + *(unsigned long *)(sram_virt_base + code_len - 4) = resume_addr_reg;

Contrary to what I said, use __raw_writel() and not writel() here, to
keep the native endianness of the system when writing the value:

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 9c56f8c..2b37e01 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -135,9 +135,11 @@ void mvebu_boot_addr_wa(int crypto_eng_id, u32 resume_addr_reg)
memcpy(sram_virt_base, &mvebu_boot_wa_start, code_len);
/*
* The last word of the code copied in SRAM must contain the
- * physical base address of the PMSU register
+ * physical base address of the PMSU register. We
+ * intentionally store this address in the native endianness
+ * of the system.
*/
- *(unsigned long *)(sram_virt_base + code_len - 4) = resume_addr_reg;
+ __raw_writel(resume_addr_reg, sram_virt_base + code_len - 4);
}

> +.global mvebu_boot_wa_start
> +.global mvebu_boot_wa_end
> +
> +/* The following code will be executed from SRAM */
> +ENTRY(mvebu_boot_wa_start)
> +mvebu_boot_wa_start:
> +/* use physical address of the boot address register register */
> + adr r0, 1f
> + ldr r0, [r0]
> + ldr r0, [r0]
> + mov pc, r0
> +/*
> + * the last word of this piece of code will be filled by the physical
> + * address of the boot address register just after being copied in SRAM
> + */
> +1:
> + .long .
> +mvebu_boot_wa_end:
> +ENDPROC(mvebu_boot_wa_end)

And this needs to be changed a bit to work properly in a big-endian
configuration:

diff --git a/arch/arm/mach-mvebu/pmsu_ll.S b/arch/arm/mach-mvebu/pmsu_ll.S
index 15b823d..2f8b021 100644
--- a/arch/arm/mach-mvebu/pmsu_ll.S
+++ b/arch/arm/mach-mvebu/pmsu_ll.S
@@ -43,11 +43,14 @@ ENDPROC(armada_38x_cpu_resume)
/* The following code will be executed from SRAM */
ENTRY(mvebu_boot_wa_start)
mvebu_boot_wa_start:
-/* use physical address of the boot address register register */
+ARM_BE8(setend be ) @ go BE8 if entered LE
adr r0, 1f
- ldr r0, [r0]
- ldr r0, [r0]
- mov pc, r0
+ ldr r0, [r0] @ load the address of the
+ @ resume register
+ ldr r0, [r0] @ load the value in the
+ @ resume register
+ARM_BE8(rev r0, r0) @ the value is stored LE
+ mov pc, r0 @ jump to this value

Note that the first ldr r0, [r0] does not need a rev r0, r0 because the
value is stored in the native endianness of the system thanks to the
__raw_writel() mentioned before. However, the second ldr r0, [r0] reads
the value in the Resume Address register, which is written in
little-endian by the writel() call in mvebu_pmsu_set_cpu_boot_addr().

Could you include this in your next iteration of the patches?

Thanks,

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering