[PATCH 0/2] additional vexpress-tc2 big endian fixes

[PATCH 0/2] additional vexpress-tc2 big endian fixes

[Victor Kamensky]

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

-- 
1.8.1.4

[PATCH 1/2] ARM: mcpm: fix big endian issue in mcpm startup code

[Victor Kamensky]

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

[PATCH 2/2] ARM: cci: driver need big endian fixes in asm code

[Victor Kamensky]

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

[PATCH 0/2] additional vexpress-tc2 big endian fixes

[Ben Dooks]

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?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 2/2] ARM: cci: driver need big endian fixes in asm code

[Dave Martin]

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 at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[PATCH 2/2] ARM: cci: driver need big endian fixes in asm code

[Nicolas Pitre]

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

[PATCH 2/2] ARM: cci: driver need big endian fixes in asm code

[Dave Martin]

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

[PATCH 2/2] ARM: cci: driver need big endian fixes in asm code

[Victor Kamensky]

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

[PATCH 2/2] ARM: cci: driver need big endian fixes in asm code

[Nicolas Pitre]

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

[PATCH 1/2] ARM: mcpm: fix big endian issue in mcpm startup code

[Nicolas Pitre]

On Tue, 8 Oct 2013, Dave Martin wrote:

> On Mon, Oct 07, 2013 at 09:37:19PM -0700, Victor Kamensky wrote:
> > 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>
> 
> Providing Nico's also OK with it, I don't see a problem with this.
> 
> Minor cosmetic nit: please line up the ) after be with the others.
> Not the end of the world, though.
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>

No problem here.

Acked-by: Nicolas Pitre <nico@linaro.org>


> 
> Cheers
> ---Dave
> 
> > ---
> >  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			)
> > -- 
> > 1.8.1.4
> > 
> > 
> > _______________________________________________
> > linaro-kernel mailing list
> > linaro-kernel at lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/linaro-kernel
>