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 <redacted>
---
 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?

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().

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

"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