[PATCH] ARM:VFPv3:enable {d16-d31} access

[PATCH] ARM:VFPv3:enable {d16-d31} access

[DebBarma, Tarun Kanti]

(Including ARM mailing list)

From: Tarun Kanti Debbarma <a0876346@a0876346-desktop.(none)>

This patch attempts to fix two related problems:

(1) vfp_get_double(), vfp_put_double() functions have VFPv3 specific implementation
guarded within CONFIG_VFPv3 macro. The intent is to access {d16-d31} additional registers
provided in VFPv3. However, it still wrongly refers to {d0-d15}. This has been corrected.

(2) While compiling the above changes, it produced compilation error because arch/arm/vfp/Makefile
had the -mfpu=vfp option which could not recognize VFPv3 registers. This has been corrected to
-mfpu=vfp3. With this option we are also able to compile pre-VFPv3 code.

Signed-off-by: Tarun Kanti Debbarma <tarun.kanti@ti.com>
---
 arch/arm/vfp/Makefile |    2 +-
 arch/arm/vfp/vfphw.S  |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
 mode change 100644 => 100755 arch/arm/vfp/vfphw.S

diff --git a/arch/arm/vfp/Makefile b/arch/arm/vfp/Makefile
index 39f6d8e..75855c0 100644
--- a/arch/arm/vfp/Makefile
+++ b/arch/arm/vfp/Makefile
@@ -7,7 +7,7 @@
 # EXTRA_CFLAGS := -DDEBUG
 # EXTRA_AFLAGS := -DDEBUG
 
-KBUILD_AFLAGS	:=$(KBUILD_AFLAGS:-msoft-float=-Wa,-mfpu=softvfp+vfp)
+KBUILD_AFLAGS	:=$(KBUILD_AFLAGS:-msoft-float=-Wa,-mfpu=vfp3 -mfloat-abi=softfp)
 LDFLAGS		+=--no-warn-mismatch
 
 obj-y			+= vfp.o
diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
old mode 100644
new mode 100755
index 66dc2d0..b034076
--- a/arch/arm/vfp/vfphw.S
+++ b/arch/arm/vfp/vfphw.S
@@ -254,8 +254,8 @@ ENTRY(vfp_get_double)
 	.endr
 #ifdef CONFIG_VFPv3
 	@ d16 - d31 registers
-	.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-1:	mrrc	p11, 3, r0, r1, c\dr	@ fmrrd	r0, r1, d\dr
+	.irp	dr,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
+1:	fmrrd	r0, r1, d\dr
 	mov	pc, lr
 	.org	1b + 8
 	.endr
@@ -276,8 +276,8 @@ ENTRY(vfp_put_double)
 	.endr
 #ifdef CONFIG_VFPv3
 	@ d16 - d31 registers
-	.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-1:	mcrr	p11, 3, r1, r2, c\dr	@ fmdrr	r1, r2, d\dr
+	.irp	dr,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
+1:	fmdrr	d\dr, r1, r2
 	mov	pc, lr
 	.org	1b + 8
 	.endr
-- 
1.6.0.4

[PATCH] ARM:VFPv3:enable {d16-d31} access

[Siarhei Siamashka]

On Tuesday 25 May 2010, DebBarma, Tarun Kanti wrote:
> (Including ARM mailing list)
>
> From: Tarun Kanti Debbarma <a0876346@a0876346-desktop.(none)>
>
> This patch attempts to fix two related problems:
>
> (1) vfp_get_double(), vfp_put_double() functions have VFPv3 specific
> implementation guarded within CONFIG_VFPv3 macro. The intent is to access
> {d16-d31} additional registers provided in VFPv3. However, it still wrongly
> refers to {d0-d15}. This has been corrected.

This does not seem to be the case, see below.

> (2) While compiling the above changes, it produced compilation error
> because arch/arm/vfp/Makefile had the -mfpu=vfp option which could not
> recognize VFPv3 registers. This has been corrected to -mfpu=vfp3. With this
> option we are also able to compile pre-VFPv3 code.
>
> Signed-off-by: Tarun Kanti Debbarma <tarun.kanti@ti.com>
> ---
>  arch/arm/vfp/Makefile |    2 +-
>  arch/arm/vfp/vfphw.S  |    8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>  mode change 100644 => 100755 arch/arm/vfp/vfphw.S
>
> diff --git a/arch/arm/vfp/Makefile b/arch/arm/vfp/Makefile
> index 39f6d8e..75855c0 100644
> --- a/arch/arm/vfp/Makefile
> +++ b/arch/arm/vfp/Makefile
> @@ -7,7 +7,7 @@
>  # EXTRA_CFLAGS := -DDEBUG
>  # EXTRA_AFLAGS := -DDEBUG
>
> -KBUILD_AFLAGS	:=$(KBUILD_AFLAGS:-msoft-float=-Wa,-mfpu=softvfp+vfp)
> +KBUILD_AFLAGS	:=$(KBUILD_AFLAGS:-msoft-float=-Wa,-mfpu=vfp3
> -mfloat-abi=softfp) LDFLAGS		+=--no-warn-mismatch
>
>  obj-y			+= vfp.o
> diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
> old mode 100644
> new mode 100755
> index 66dc2d0..b034076
> --- a/arch/arm/vfp/vfphw.S
> +++ b/arch/arm/vfp/vfphw.S
> @@ -254,8 +254,8 @@ ENTRY(vfp_get_double)
>  	.endr
>  #ifdef CONFIG_VFPv3
>  	@ d16 - d31 registers
> -	.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -1:	mrrc	p11, 3, r0, r1, c\dr	@ fmrrd	r0, r1, d\dr
> +	.irp	dr,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
> +1:	fmrrd	r0, r1, d\dr

The existing code does exactly the same, but avoids the need to
specify -mfpu=vfp3 option. It's quite easy to verify:

/**** test.S *******/
test:
    mrrc  p11, 3, r0, r1, c0
    fmrrd r0, r1, d16
/*******************/

gcc -c test.S
objdump -d test.o

Disassembly of section .text:

00000000 <test>:
   0:   ec510b30        vmov    r0, r1, d16
   4:   ec510b30        vmov    r0, r1, d16

The comment in the existing code is a bit misleading though.

-- 
Best regards,
Siarhei Siamashka

[PATCH] ARM:VFPv3:enable {d16-d31} access

[DebBarma, Tarun Kanti]

Hello Siarhei Siamashka,
Thanks for the inputs. Please see my comments.

-----Original Message-----
From: Siarhei Siamashka [mailto:siarhei.siamashka at gmail.com] 
Sent: Tuesday, May 25, 2010 5:57 PM
To: DebBarma, Tarun Kanti
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] ARM:VFPv3:enable {d16-d31} access

On Tuesday 25 May 2010, DebBarma, Tarun Kanti wrote:
> (Including ARM mailing list)
>
> From: Tarun Kanti Debbarma <a0876346@a0876346-desktop.(none)>
>
> This patch attempts to fix two related problems:
>
> (1) vfp_get_double(), vfp_put_double() functions have VFPv3 specific
> implementation guarded within CONFIG_VFPv3 macro. The intent is to access
> {d16-d31} additional registers provided in VFPv3. However, it still wrongly
> refers to {d0-d15}. This has been corrected.

This does not seem to be the case, see below.

Are you saying following line generates code for {d16-d31}?
.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15


> (2) While compiling the above changes, it produced compilation error
> because arch/arm/vfp/Makefile had the -mfpu=vfp option which could not
> recognize VFPv3 registers. This has been corrected to -mfpu=vfp3. With this
> option we are also able to compile pre-VFPv3 code.
>
> Signed-off-by: Tarun Kanti Debbarma <tarun.kanti@ti.com>
> ---
>  arch/arm/vfp/Makefile |    2 +-
>  arch/arm/vfp/vfphw.S  |    8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>  mode change 100644 => 100755 arch/arm/vfp/vfphw.S
>
> diff --git a/arch/arm/vfp/Makefile b/arch/arm/vfp/Makefile
> index 39f6d8e..75855c0 100644
> --- a/arch/arm/vfp/Makefile
> +++ b/arch/arm/vfp/Makefile
> @@ -7,7 +7,7 @@
>  # EXTRA_CFLAGS := -DDEBUG
>  # EXTRA_AFLAGS := -DDEBUG
>
> -KBUILD_AFLAGS	:=$(KBUILD_AFLAGS:-msoft-float=-Wa,-mfpu=softvfp+vfp)
> +KBUILD_AFLAGS	:=$(KBUILD_AFLAGS:-msoft-float=-Wa,-mfpu=vfp3
> -mfloat-abi=softfp) LDFLAGS		+=--no-warn-mismatch
>
>  obj-y			+= vfp.o
> diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
> old mode 100644
> new mode 100755
> index 66dc2d0..b034076
> --- a/arch/arm/vfp/vfphw.S
> +++ b/arch/arm/vfp/vfphw.S
> @@ -254,8 +254,8 @@ ENTRY(vfp_get_double)
>  	.endr
>  #ifdef CONFIG_VFPv3
>  	@ d16 - d31 registers
> -	.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -1:	mrrc	p11, 3, r0, r1, c\dr	@ fmrrd	r0, r1, d\dr
> +	.irp	dr,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
> +1:	fmrrd	r0, r1, d\dr

The existing code does exactly the same, but avoids the need to
specify -mfpu=vfp3 option. It's quite easy to verify:

/**** test.S *******/
test:
    mrrc  p11, 3, r0, r1, c0
    fmrrd r0, r1, d16
/*******************/

gcc -c test.S
objdump -d test.o

Disassembly of section .text:

00000000 <test>:
   0:   ec510b30        vmov    r0, r1, d16
   4:   ec510b30        vmov    r0, r1, d16

The comment in the existing code is a bit misleading though.

I get following compilation error when:
-fmpu=vfp3 option used with
mrrc	p11, 3, r0, r1, c\dr instruction



arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c16'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c17'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c18'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c19'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c20'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c21'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c22'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c23'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c24'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c25'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c26'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c27'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c28'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c29'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c30'
arch/arm/vfp/vfphw.S:234: Error: co-processor register expected -- `mrrc p11,3,r0,r1,c31'

Is this behavior expected? 
However I do not get the same when I use:
fmrrd	r0, r1, d\dr

Question: Is there any disadvantage of using: fmrrd	r0, r1, d\dr over the other one?

Thanks once again for your inputs.

-- 
Best regards,
Siarhei Siamashka

[PATCH] ARM:VFPv3:enable {d16-d31} access

[Russell King - ARM Linux]

On Tue, May 25, 2010 at 02:39:17PM +0530, DebBarma, Tarun Kanti wrote:
>  #ifdef CONFIG_VFPv3
>  	@ d16 - d31 registers
> -	.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -1:	mrrc	p11, 3, r0, r1, c\dr	@ fmrrd	r0, r1, d\dr
> +	.irp	dr,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
> +1:	fmrrd	r0, r1, d\dr

The existing code is correct.

For every fmrrd instruction, there is a corresponding mrrc version which
assembles to exactly the same opcode.

mrrc instructions take:

1. Co-processor number, range 0-15.
2. Opcode number N, range 0-15.
3. Destination register 1, range 0-15.
4. Destination register 2, range 0-15.
5. Co-processor register number R, range 0-15.

For fmrrd encodings, the first 16 registers are encoded using N=1 with
R=0 to 15.  The second 16 registers are encoded using N=3 with R=0 to 15.
Specifying a co-processor register number greater than 15 is illegal,
hence why the 'irp' specifies the numbers 0 to 15.

If we look at the instruction encodings, for MRRC:

 3 3 2 2 2 2 2 2 2 2 2 2 1 1 1 1 1 1 1 1 1 1
 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
|  cond |1 1 0 0 0 1 0 1|  Rn   |  Rm   |CP Num |   N   |   R   |

For FMRRD:

 3 3 2 2 2 2 2 2 2 2 2 2 1 1 1 1 1 1 1 1 1 1
 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
|  cond |1 1 0 0 0 1 0 1|  Rn   |  Rm   |1 0 1 1 0 0 M 1|   R   |

where "M" and "R" together define the register.

As I said above, the existing code is correct.  What problem are you
actually trying to solve here?

[PATCH] ARM:VFPv3:enable {d16-d31} access

[DebBarma, Tarun Kanti]

Russell,

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
Sent: Wednesday, May 26, 2010 12:28 AM
To: DebBarma, Tarun Kanti
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] ARM:VFPv3:enable {d16-d31} access

On Tue, May 25, 2010 at 02:39:17PM +0530, DebBarma, Tarun Kanti wrote:
>  #ifdef CONFIG_VFPv3
>  	@ d16 - d31 registers
> -	.irp	dr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -1:	mrrc	p11, 3, r0, r1, c\dr	@ fmrrd	r0, r1, d\dr
> +	.irp	dr,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
> +1:	fmrrd	r0, r1, d\dr

The existing code is correct.

For every fmrrd instruction, there is a corresponding mrrc version which
assembles to exactly the same opcode.

mrrc instructions take:

1. Co-processor number, range 0-15.
2. Opcode number N, range 0-15.
3. Destination register 1, range 0-15.
4. Destination register 2, range 0-15.
5. Co-processor register number R, range 0-15.

For fmrrd encodings, the first 16 registers are encoded using N=1 with
R=0 to 15.  The second 16 registers are encoded using N=3 with R=0 to 15.
Specifying a co-processor register number greater than 15 is illegal,
hence why the 'irp' specifies the numbers 0 to 15.

If we look at the instruction encodings, for MRRC:

 3 3 2 2 2 2 2 2 2 2 2 2 1 1 1 1 1 1 1 1 1 1
 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
|  cond |1 1 0 0 0 1 0 1|  Rn   |  Rm   |CP Num |   N   |   R   |

For FMRRD:

 3 3 2 2 2 2 2 2 2 2 2 2 1 1 1 1 1 1 1 1 1 1
 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
|  cond |1 1 0 0 0 1 0 1|  Rn   |  Rm   |1 0 1 1 0 0 M 1|   R   |

where "M" and "R" together define the register.

As I said above, the existing code is correct.  What problem are you
actually trying to solve here?

######################
I have a test case which exercise all VFP general purpose registers by writing a known value and reading it back using vfp_put_double() and vfp_get_double() APIs.

	long long d1=777, d2=0;
	int i=31;
	for (; i>=0; i--){
		vfp_put_double(d1, i);
		d2 = vfp_get_double(i);
		printk("D%d read=%lld\n",i, (long long)d2);
		d2 = 0.0;
	}

1) With the existing implementation I am not able to correctly write/read {d0-d15} but not the {d16-d31} set

2) With my changes I am able to write/read correctly.

Tarun

[PATCH] ARM:VFPv3:enable {d16-d31} access

[Russell King - ARM Linux]

On Wed, May 26, 2010 at 05:13:24PM +0530, DebBarma, Tarun Kanti wrote:
> 1) With the existing implementation I am not able to correctly
> write/read {d0-d15} but not the {d16-d31} set
> 
> 2) With my changes I am able to write/read correctly.

The reason this happens is simple.  In vfp_get_double(), we have:

1:      fmrrd   r0, r1, d\dr
1:      mrrc    p11, 3, r0, r1, c\dr    @ fmrrd r0, r1, d\dr

but in vfp_put_double(), we have:

1:      fmdrr   d\dr, r0, r1
1:      mcrr    p11, 3, r1, r2, c\dr    @ fmdrr r1, r2, d\dr

Note the different registers.  Change "r1, r2" to "r0, r1" and it
should work.

[PATCH] ARM:VFPv3:enable {d16-d31} access

[DebBarma, Tarun Kanti]

Russell,

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Thursday, May 27, 2010 1:40 AM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] ARM:VFPv3:enable {d16-d31} access
> 
> On Wed, May 26, 2010 at 05:13:24PM +0530, DebBarma, Tarun Kanti wrote:
> > 1) With the existing implementation I am not able to correctly
> > write/read {d0-d15} but not the {d16-d31} set
> >
> > 2) With my changes I am able to write/read correctly.
> 
> The reason this happens is simple.  In vfp_get_double(), we have:
> 
> 1:      fmrrd   r0, r1, d\dr
> 1:      mrrc    p11, 3, r0, r1, c\dr    @ fmrrd r0, r1, d\dr
> 
> but in vfp_put_double(), we have:
> 
> 1:      fmdrr   d\dr, r0, r1
> 1:      mcrr    p11, 3, r1, r2, c\dr    @ fmdrr r1, r2, d\dr
> 
> Note the different registers.  Change "r1, r2" to "r0, r1" and it
> should work.

Yes, that's right. I figured that out yesterday and confirmed the test results. Thanks!

Can I go ahead with next version of the patch?

BR
Tarun

[PATCH] ARM:VFPv3:enable {d16-d31} access

[Russell King - ARM Linux]

On Thu, May 27, 2010 at 10:07:40AM +0530, DebBarma, Tarun Kanti wrote:
> > Note the different registers.  Change "r1, r2" to "r0, r1" and it
> > should work.
> 
> Yes, that's right. I figured that out yesterday and confirmed the test
> results. Thanks!

I've now committed a fix to change those registers.  Are you ok with
being credited with a Reported-by tag?

[PATCH] ARM:VFPv3:enable {d16-d31} access

[DebBarma, Tarun Kanti]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Thursday, May 27, 2010 3:00 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] ARM:VFPv3:enable {d16-d31} access
> 
> On Thu, May 27, 2010 at 10:07:40AM +0530, DebBarma, Tarun Kanti wrote:
> > > Note the different registers.  Change "r1, r2" to "r0, r1" and it
> > > should work.
> >
> > Yes, that's right. I figured that out yesterday and confirmed the test
> > results. Thanks!
> 
> I've now committed a fix to change those registers.  Are you ok with
> being credited with a Reported-by tag?

That's fine!