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

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Gregory,

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

> On some of the mvebu SoC and due to internal bootrom issue, CPU

"due to an internal BootROM issue, the CPU"

> initial jump code should be placed in SRAM memory of the SoC. In order

"in SRAM memory" -> "in the SRAM memory".

> to achieve this, we have to unmap the BootROM and at some specific
> location where the BootROM was place, create a specific MBus window

place -> placed

> for the SRAM. This SRAM is initialized with a few instructions of code
> that allows to jump into the real secondary CPU boot address.

jump into -> jump to.

> This work around currently needed for booting SMP on Aramda 375 Z1 and

This work around *is* currently needed for SMP support on *Armada*.

> will be needed for cpuidle support on Armada 370. Instead of duplicate

duplicate -> duplicating.

> the same code, this commit introduce a common function to handle it:

introduce -> introduces.

> mvebu_boot_addr_wa().

I'm not sure the name of the function is appropriate, as we don't know
what it is doing. Maybe mvebu_setup_boot_addr_wa() ?

Also, maybe your commit log should indicate that the workaround
involves using the Crypto engine SRAM, which will help understanding
the reference to the crypto engine in the code.

> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/pmsu.c    | 31 +++++++++++++++++++++++++++++++
>  arch/arm/mach-mvebu/pmsu.h    |  1 +
>  arch/arm/mach-mvebu/pmsu_ll.S | 19 +++++++++++++++++++
>  3 files changed, 51 insertions(+)

I don't really have a better suggestion, but the workaround doesn't
seem to be related to the PMSU, so are pmsu.c and pmsu_ll.S really the
right files to store this code?

> 
> diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> index 5584d35b8e88..991560905ccc 100644
> --- a/arch/arm/mach-mvebu/pmsu.c
> +++ b/arch/arm/mach-mvebu/pmsu.c
> @@ -22,6 +22,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/io.h>
> +#include <linux/mbus.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/resource.h>
> @@ -63,6 +64,14 @@ static void __iomem *pmsu_mp_base;
>  #define L2C_NFABRIC_PM_CTL		    0x4
>  #define L2C_NFABRIC_PM_CTL_PWR_DOWN		BIT(20)
>  
> +#define ARMADA_370_CRYPT0_ENG_ID	0x9

Not needed in this file, the MBus window target ID is passed as
argument to the mvebu_boot_addr_wa() function.

> +#define CRYPT0_ENG_ATTR	0x1

For consistency, I'd prefer to see this being passed as argument to
mvebu_boot_addr_wa().

> +#define SRAM_PHYS_BASE	    0xFFFF0000
> +
> +#define BOOTROM_BASE    0xFFF00000
> +#define BOOTROM_SIZE    0x100000
> +
>  extern void ll_disable_coherency(void);
>  extern void ll_enable_coherency(void);
>  
> @@ -85,6 +94,28 @@ void mvebu_pmsu_set_cpu_boot_addr(int hw_cpu, void *boot_addr)
>  		PMSU_BOOT_ADDR_REDIRECT_OFFSET(hw_cpu));
>  }
>  
> +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)

unsigned int crypto_eng_target ('unsigned int' is the type used by
the mvebu-mbus API), and add unsigned int crypto_eng_attribute, to
match the mvebu-mbus API. Also, void * seems more appropriate than u32
for an address, maybe even void __iomem * since it's actually pointing
to a memory-mapped register.

> +{
> +	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);

Maybe a return value check?

> +
> +

One too many new line.

> +	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;

Maybe instead:

	writel(resume_addr_reg, sram_virt_base + code_len - 4);

And also:

	iounmap(sram_virt_base);

> +}
> +
>  static int __init armada_370_xp_pmsu_init(void)
>  {
>  	struct device_node *np;
> diff --git a/arch/arm/mach-mvebu/pmsu.h b/arch/arm/mach-mvebu/pmsu.h
> index 07a737c6b95d..2d97a70c2558 100644
> --- a/arch/arm/mach-mvebu/pmsu.h
> +++ b/arch/arm/mach-mvebu/pmsu.h
> @@ -12,5 +12,6 @@
>  #define __MACH_MVEBU_PMSU_H
>  
>  int armada_xp_boot_cpu(unsigned int cpu_id, void *phys_addr);
> +void mvebu_boot_addr_wa(int crypto_eng_id, unsigned long resume_addr_reg);
>  
>  #endif	/* __MACH_370_XP_PMSU_H */
> diff --git a/arch/arm/mach-mvebu/pmsu_ll.S b/arch/arm/mach-mvebu/pmsu_ll.S
> index fc3de68d8c54..3b702a16bd3d 100644
> --- a/arch/arm/mach-mvebu/pmsu_ll.S
> +++ b/arch/arm/mach-mvebu/pmsu_ll.S
> @@ -23,3 +23,22 @@ ARM_BE8(setend	be )			@ go BE8 if entered LE
>  	b	cpu_resume
>  ENDPROC(armada_370_xp_cpu_resume)
>  
> +.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 */

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

"filled with". "filled by" indicates *who* is filling the missing
information, "filled with" indicates with *what* it is being filled.

> + * address of the boot address register just after being copied in SRAM
> + */
> +1:
> +	.long   .
> +mvebu_boot_wa_end:
> +ENDPROC(mvebu_boot_wa_end)

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com