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

[Nicolas Pitre]

On Mon, 14 Oct 2013, Ben Dooks wrote:

> On 08/10/13 23:34, Ben Dooks wrote:
> > From: Victor Kamensky<victor.kamensky at linaro.org>
> > 
> > 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
> > 
> > CC: Lorenzon Pieralisi<lorenzo.pieralisi at arm.com>
> > Signed-off-by: Victor Kamensky<victor.kamensky at linaro.org>
> > Signed-off-by: Ben Dooks<ben.dooks at codethink.co.uk>
> > ---
> >   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"
> 
> I was just thinking if this would be a better way to change
> the code:
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 6db173e..2ad2511 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -280,10 +280,7 @@ 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__ */
> +"      mov     r3, %[cci_enable_req]\n"
>  "      str     r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
> 
>         /* poll the status reg for completion */
> @@ -291,10 +288,7 @@ 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"
> +"      tst     r1, %[cci_control_status_bits] \n"
>  "      bne     4b \n"
> 
>  "      mov     r0, #0 \n"
> @@ -307,6 +301,8 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>  "7:    .word   cci_ctrl_phys - . \n"
>         : :
>         [sizeof_cpu_port] "i" (sizeof(cpu_port)),
> +       [cci_enable_req] "i" cpu_to_le32(CCI_ENABLE_REQ),
> +       [cci_control_status_bits] "i" cpu_to_le32(1),
> 
> so swap the data bits before passing them into the code
> which could also remove some of the use of the __stringify()
> calls in there too.
That certainly looks nicer to me.

> It seems to compile ok, but is not complete.

What do you mean?


Nicolas