Hi Ben,
Here are couple more big endian related fixes. We run into these with vexpress TC2 board, when CONFIG_MCPM and CONFIG_ARM_CCI configs are enabled. Big endian image fails to boot. With fixes BE TC2 boots and it sees all 5 cores (2xA15, 3xA7).
These were tested with vexpress TC2 on latest linux-linaro branch (include BE patch series) and it was tested on very recent rmk/fixes branch along with BE series on top of it.
Thanks, Victor
Victor Kamensky (2): ARM: mcpm: fix big endian issue in mcpm startup code ARM: cci: driver need big endian fixes in asm code
arch/arm/common/mcpm_head.S | 2 ++ drivers/bus/arm-cci.c | 6 ++++++ 2 files changed, 8 insertions(+)
In big endian mode mcpm_entry_point is first function that called on secondaries CPU. First it should switch CPU into big endian code.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org --- arch/arm/common/mcpm_head.S | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/common/mcpm_head.S b/arch/arm/common/mcpm_head.S index 39c96df..4f88f5e 100644 --- a/arch/arm/common/mcpm_head.S +++ b/arch/arm/common/mcpm_head.S @@ -15,6 +15,7 @@
#include <linux/linkage.h> #include <asm/mcpm.h> +#include <asm/assembler.h>
#include "vlock.h"
@@ -47,6 +48,7 @@
ENTRY(mcpm_entry_point)
+ ARM_BE8(setend be) THUMB( adr r12, BSYM(1f) ) THUMB( bx r12 ) THUMB( .thumb )
cci_enable_port_for_self written in asm and it works with h/w registers that are in little endian format. When run in big endian mode it needs byte swap before/after it writes/reads to/from such registers
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org --- drivers/bus/arm-cci.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 2009266..6db173e 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) /* Enable the CCI port */ " ldr r0, [r0, %[offsetof_port_phys]] \n" " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n" +#ifdef __ARMEB__ +" rev r3, r3 \n" +#endif /* __ARMEB__ */ " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
/* poll the status reg for completion */ @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) " ldr r0, [r1] \n" " ldr r0, [r0, r1] @ cci_ctrl_base \n" "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n" +#ifdef __ARMEB__ +" rev r1, r1 \n" +#endif /* __ARMEB__ */ " tst r1, #1 \n" " bne 4b \n"
On Mon, Oct 07, 2013 at 09:37:20PM -0700, Victor Kamensky wrote:
cci_enable_port_for_self written in asm and it works with h/w registers that are in little endian format. When run in big endian mode it needs byte swap before/after it writes/reads to/from such registers
Lorenzo should comment on this if he's not already seen it.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
drivers/bus/arm-cci.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 2009266..6db173e 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) /* Enable the CCI port */ " ldr r0, [r0, %[offsetof_port_phys]] \n" " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n" +#ifdef __ARMEB__ +" rev r3, r3 \n" +#endif /* __ARMEB__ */
Hmmm, this is a bit ugly, but we can't easily use ARM_BE8() here.
That would be more easily solved if this was a separate .S file...
Maybe it's not worth it.
Cheers ---Dave
" str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n" /* poll the status reg for completion */ @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) " ldr r0, [r1] \n" " ldr r0, [r0, r1] @ cci_ctrl_base \n" "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n" +#ifdef __ARMEB__ +" rev r1, r1 \n" +#endif /* __ARMEB__ */ " tst r1, #1 \n" " bne 4b \n" -- 1.8.1.4
linaro-kernel mailing list linaro-kernel@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-kernel
On Tue, 8 Oct 2013, Dave Martin wrote:
On Mon, Oct 07, 2013 at 09:37:20PM -0700, Victor Kamensky wrote:
cci_enable_port_for_self written in asm and it works with h/w registers that are in little endian format. When run in big endian mode it needs byte swap before/after it writes/reads to/from such registers
Lorenzo should comment on this if he's not already seen it.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
drivers/bus/arm-cci.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 2009266..6db173e 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) /* Enable the CCI port */ " ldr r0, [r0, %[offsetof_port_phys]] \n" " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n" +#ifdef __ARMEB__ +" rev r3, r3 \n" +#endif /* __ARMEB__ */
Hmmm, this is a bit ugly, but we can't easily use ARM_BE8() here.
In any case, it would be more efficient to swap the CCI_ENABLE_REQ constant at compile time rather than doing it at run time with an extra instruction.
Nicolas
On Tue, Oct 08, 2013 at 01:57:48PM -0400, Nicolas Pitre wrote:
On Tue, 8 Oct 2013, Dave Martin wrote:
On Mon, Oct 07, 2013 at 09:37:20PM -0700, Victor Kamensky wrote:
cci_enable_port_for_self written in asm and it works with h/w registers that are in little endian format. When run in big endian mode it needs byte swap before/after it writes/reads to/from such registers
Lorenzo should comment on this if he's not already seen it.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
drivers/bus/arm-cci.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 2009266..6db173e 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) /* Enable the CCI port */ " ldr r0, [r0, %[offsetof_port_phys]] \n" " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n" +#ifdef __ARMEB__ +" rev r3, r3 \n" +#endif /* __ARMEB__ */
Hmmm, this is a bit ugly, but we can't easily use ARM_BE8() here.
In any case, it would be more efficient to swap the CCI_ENABLE_REQ constant at compile time rather than doing it at run time with an extra instruction.
Indeed, but we can't stringify that.
A different way of wedging it into the assembler would be needed... We might need to use ldr= too, which doesn't pay nice with inline asm.
Cheers ---Dave
Hi Nicolas,
On 8 October 2013 10:57, Nicolas Pitre nicolas.pitre@linaro.org wrote:
On Tue, 8 Oct 2013, Dave Martin wrote:
On Mon, Oct 07, 2013 at 09:37:20PM -0700, Victor Kamensky wrote:
cci_enable_port_for_self written in asm and it works with h/w registers that are in little endian format. When run in big endian mode it needs byte swap before/after it writes/reads to/from such registers
Lorenzo should comment on this if he's not already seen it.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
drivers/bus/arm-cci.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 2009266..6db173e 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) /* Enable the CCI port */ " ldr r0, [r0, %[offsetof_port_phys]] \n" " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n" +#ifdef __ARMEB__ +" rev r3, r3 \n" +#endif /* __ARMEB__ */
Hmmm, this is a bit ugly, but we can't easily use ARM_BE8() here.
In any case, it would be more efficient to swap the CCI_ENABLE_REQ constant at compile time rather than doing it at run time with an extra instruction.
I could try to do this but it would require introducing another constant, because CCI_ENABLE_REQ is used in another place in writel_relaxed call which will do byteswap inside. Is such run-time optimization really worth it compared to code readability. Please let me know. I am OK either way. Personally I thought that run-time byteswap would be more readable.
Also I am concerned how can I compile time byteswap CCI_ENABLE_REQ in context of its being used with __stringify ... Wondering it should be stringfied into real number not long expression
Thanks, Victor
Nicolas
On Tue, 8 Oct 2013, Victor Kamensky wrote:
Hi Nicolas,
On 8 October 2013 10:57, Nicolas Pitre nicolas.pitre@linaro.org wrote:
On Tue, 8 Oct 2013, Dave Martin wrote:
On Mon, Oct 07, 2013 at 09:37:20PM -0700, Victor Kamensky wrote:
cci_enable_port_for_self written in asm and it works with h/w registers that are in little endian format. When run in big endian mode it needs byte swap before/after it writes/reads to/from such registers
Lorenzo should comment on this if he's not already seen it.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
drivers/bus/arm-cci.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 2009266..6db173e 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void) /* Enable the CCI port */ " ldr r0, [r0, %[offsetof_port_phys]] \n" " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n" +#ifdef __ARMEB__ +" rev r3, r3 \n" +#endif /* __ARMEB__ */
Hmmm, this is a bit ugly, but we can't easily use ARM_BE8() here.
In any case, it would be more efficient to swap the CCI_ENABLE_REQ constant at compile time rather than doing it at run time with an extra instruction.
I could try to do this but it would require introducing another constant, because CCI_ENABLE_REQ is used in another place in writel_relaxed call which will do byteswap inside. Is such run-time optimization really worth it compared to code readability. Please let me know. I am OK either way. Personally I thought that run-time byteswap would be more readable.
You're probably right.
Also I am concerned how can I compile time byteswap CCI_ENABLE_REQ in context of its being used with __stringify ... Wondering it should be stringfied into real number not long expression
The assembler can accept a subset of the simple C operands so it is possible that this could just work. Otherwise it is best to go with a runtime swap.
Nicolas
On 08/10/13 06:37, Victor Kamensky wrote:
Hi Ben,
Here are couple more big endian related fixes. We run into these with vexpress TC2 board, when CONFIG_MCPM and CONFIG_ARM_CCI configs are enabled. Big endian image fails to boot. With fixes BE TC2 boots and it sees all 5 cores (2xA15, 3xA7).
These were tested with vexpress TC2 on latest linux-linaro branch (include BE patch series) and it was tested on very recent rmk/fixes branch along with BE series on top of it.
Thanks, Victor
Victor Kamensky (2): ARM: mcpm: fix big endian issue in mcpm startup code ARM: cci: driver need big endian fixes in asm code
arch/arm/common/mcpm_head.S | 2 ++ drivers/bus/arm-cci.c | 6 ++++++ 2 files changed, 8 insertions(+)
Thanks, anyone any objections if these go into my 312-rc4 branch?
linaro-kernel@lists.linaro.org
-
Ben Dooks -
Dave Martin -
Nicolas Pitre -
Victor Kamensky