| Message ID | 1403875377-940-3-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
Hi Thomas, > >> 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() ? I try to avoid too long name because then we have to do a lot of multiline code. However I agree that this name is better, I will use it > > 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. > OK >> 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. OK > >> +#define CRYPT0_ENG_ATTR 0x1 > > For consistency, I'd prefer to see this being passed as argument to > mvebu_boot_addr_wa(). The attribute is the same, so why bother with it? If later we have a SoC where this attribute can be different then I agree to add this argument. > >> +#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. I was lazy for the resume_addr_reg and u32 was easier to use, but indeed void __iomem * is better. I also agree for the other, I didn't notice it was unsigned int instead of int. > >> +{ >> + 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? yes > >> + >> + > > 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); Oh yes I forgot it was "iomaped" Thanks, Gregory
Gregory, On Thu, 03 Jul 2014 00:58:22 +0200, Gregory CLEMENT wrote: > >> +#define CRYPT0_ENG_ATTR 0x1 > > > > For consistency, I'd prefer to see this being passed as argument to > > mvebu_boot_addr_wa(). > > The attribute is the same, so why bother with it? If later we have a SoC > where this attribute can be different then I agree to add this argument. Simply for consistency. MBus windows are defined by their target and attribute values, so it makes sense to always pass both these values, even if for the existing cases the attribute is for now always 0x1. Thanks, Thomas
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 +#define CRYPT0_ENG_ATTR 0x1 + +#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) +{ + 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; +} + 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 */ + 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)
On some of the mvebu SoC and due to internal bootrom issue, CPU initial jump code should be placed in SRAM memory of the SoC. In order to achieve this, we have to unmap the BootROM and at some specific location where the BootROM was place, create a specific MBus window for the SRAM. This SRAM is initialized with a few instructions of code that allows to jump into the real secondary CPU boot address. This work around currently needed for booting SMP on Aramda 375 Z1 and will be needed for cpuidle support on Armada 370. Instead of duplicate the same code, this commit introduce a common function to handle it: mvebu_boot_addr_wa(). 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(+)