[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[Dmitry Eremin-Solenikov]

According to the manuals I have, XScale auxiliary register should be
reached with opc_2 = 1 instead of crn = 1. cpu_xscale_proc_init
correctly uses c1, c0, 1 arguments, but cpu_xscale_do_suspend and
cpu_xscale_do_resume use c1, c1, 0. Correct suspend/resume functions to
also use c1, c0, 1.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 arch/arm/mm/proc-xscale.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/proc-xscale.S b/arch/arm/mm/proc-xscale.S
index 23259f1..afa2b3c 100644
--- a/arch/arm/mm/proc-xscale.S
+++ b/arch/arm/mm/proc-xscale.S
@@ -535,7 +535,7 @@ ENTRY(cpu_xscale_do_suspend)
 	mrc	p15, 0, r5, c15, c1, 0	@ CP access reg
 	mrc	p15, 0, r6, c13, c0, 0	@ PID
 	mrc	p15, 0, r7, c3, c0, 0	@ domain ID
-	mrc	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
+	mrc	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
 	mrc	p15, 0, r9, c1, c0, 0	@ control reg
 	bic	r4, r4, #2		@ clear frequency change bit
 	stmia	r0, {r4 - r9}		@ store cp regs
@@ -552,7 +552,7 @@ ENTRY(cpu_xscale_do_resume)
 	mcr	p15, 0, r6, c13, c0, 0	@ PID
 	mcr	p15, 0, r7, c3, c0, 0	@ domain ID
 	mcr	p15, 0, r1, c2, c0, 0	@ translation table base addr
-	mcr	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
+	mcr	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
 	mov	r0, r9			@ control register
 	b	cpu_resume_mmu
 ENDPROC(cpu_xscale_do_resume)
-- 
2.1.1

[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[Robert Jarzmik]

Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:

> According to the manuals I have, XScale auxiliary register should be
> reached with opc_2 = 1 instead of crn = 1. cpu_xscale_proc_init
> correctly uses c1, c0, 1 arguments, but cpu_xscale_do_suspend and
> cpu_xscale_do_resume use c1, c1, 0. Correct suspend/resume functions to
> also use c1, c0, 1.

> -	mrc	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
> +	mrc	p15, 0, r8, c1, c0, 1	@ auxiliary control reg

My PXA320 manual says (PXA320 Developer's Guide, chapter 2.16.1, table 5) :
 - Auxiliary Register: Crn=1, Crm=0, Opcode1=0, Opcode2=1
   => that is what your patch changes

My PXA270 manual says (PXA270 Developer's Guide, chapter 2.2.5.3) :
 - Auxiliary Register: Crn=1, Crm=<don't care>, Opcode1=0, Opcode2=1
   => same story as for the PX320

My PXA255 manual doesn't say anything, but the Intel XScale Core Architecture
manual seems to be applicable to it, and gives you reason.

Moreover the CP15, Opcode=1, Crn=1, Crm=0, Opcode2=* doesn't point to any valid
register in XScale architecture AFAIK.

So your patch looks correct to me. I have a couple of questions :
 - did you see an incorrect behaviour without your patch or is this coming from
   code inspection ?
 - did you test this patch during a suspend/resume cycle, and if so on which
   variants (pxa25x and tosa maybe) ?

As you might haved guessed, I'd like all these questions to have an answer in
the commit message.

Cheers.

--
Robert

[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[Dmitry Eremin-Solenikov]

2014-11-09 18:23 GMT+03:00 Robert Jarzmik <robert.jarzmik@free.fr>:
> Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:
>
>> According to the manuals I have, XScale auxiliary register should be
>> reached with opc_2 = 1 instead of crn = 1. cpu_xscale_proc_init
>> correctly uses c1, c0, 1 arguments, but cpu_xscale_do_suspend and
>> cpu_xscale_do_resume use c1, c1, 0. Correct suspend/resume functions to
>> also use c1, c0, 1.
>
>> -     mrc     p15, 0, r8, c1, c1, 0   @ auxiliary control reg
>> +     mrc     p15, 0, r8, c1, c0, 1   @ auxiliary control reg
>
> My PXA320 manual says (PXA320 Developer's Guide, chapter 2.16.1, table 5) :
>  - Auxiliary Register: Crn=1, Crm=0, Opcode1=0, Opcode2=1
>    => that is what your patch changes
>
> My PXA270 manual says (PXA270 Developer's Guide, chapter 2.2.5.3) :
>  - Auxiliary Register: Crn=1, Crm=<don't care>, Opcode1=0, Opcode2=1
>    => same story as for the PX320
>
> My PXA255 manual doesn't say anything, but the Intel XScale Core Architecture
> manual seems to be applicable to it, and gives you reason.
>
> Moreover the CP15, Opcode=1, Crn=1, Crm=0, Opcode2=* doesn't point to any valid
> register in XScale architecture AFAIK.
>
> So your patch looks correct to me. I have a couple of questions :
>  - did you see an incorrect behaviour without your patch or is this coming from
>    code inspection ?

Noticed thanks to qemu that reported 'unsupported instruction' on suspend.

>  - did you test this patch during a suspend/resume cycle, and if so on which
>    variants (pxa25x and tosa maybe) ?

Tested on tosa (that is pxa255).

>
> As you might haved guessed, I'd like all these questions to have an answer in
> the commit message.

Do you need an updated message?

-- 
With best wishes
Dmitry

[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[Robert Jarzmik]

Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:

> Noticed thanks to qemu that reported 'unsupported instruction' on suspend.
>
>>  - did you test this patch during a suspend/resume cycle, and if so on which
>>    variants (pxa25x and tosa maybe) ?
>
> Tested on tosa (that is pxa255).
>
>>
>> As you might haved guessed, I'd like all these questions to have an answer in
>> the commit message.
>
> Do you need an updated message?
In a couple of days, if you don't have any other comments, yes please.

And I think the patch subject would rather be "ARM: xscale: ..." if I judge by
the other patches in that area.

And as I tested that also on pxa270 and found no regression, with my:
Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

-- 
Robert

[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:
> In a couple of days, if you don't have any other comments, yes please.

I think you can send it. Don't forget to put Russell in the To: and have his
ack, as this will probably go through his patch system.

Cheers.

-- 
Robert

[PATCH] arm: xscale: correct auxiliary register in suspend/resume

[Dmitry Eremin-Solenikov]

2014-11-15 14:52 GMT+03:00 Robert Jarzmik <robert.jarzmik@free.fr>:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>
>> Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:
>> In a couple of days, if you don't have any other comments, yes please.
>
> I think you can send it. Don't forget to put Russell in the To: and have his
> ack, as this will probably go through his patch system.

Done

-- 
With best wishes
Dmitry

[PATCH] ARM: xscale: correct auxiliary register in suspend/resume

[Dmitry Eremin-Solenikov]

According to the manuals I have, XScale auxiliary register should be
reached with opc_2 = 1 instead of crn = 1. cpu_xscale_proc_init
correctly uses c1, c0, 1 arguments, but cpu_xscale_do_suspend and
cpu_xscale_do_resume use c1, c1, 0. Correct suspend/resume functions to
also use c1, c0, 1.

The issue was primarily noticed thanks to qemu reporing "unsupported
instruction" on the pxa suspend path. Confirmed in PXA210/250 and PXA255
XScale Core manuals and in PXA270 and PXA320 Developers Guides.

Harware tested by me on tosa (pxa255). Robert confirmed on pxa270 board.

Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 arch/arm/mm/proc-xscale.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/proc-xscale.S b/arch/arm/mm/proc-xscale.S
index 23259f1..afa2b3c 100644
--- a/arch/arm/mm/proc-xscale.S
+++ b/arch/arm/mm/proc-xscale.S
@@ -535,7 +535,7 @@ ENTRY(cpu_xscale_do_suspend)
 	mrc	p15, 0, r5, c15, c1, 0	@ CP access reg
 	mrc	p15, 0, r6, c13, c0, 0	@ PID
 	mrc	p15, 0, r7, c3, c0, 0	@ domain ID
-	mrc	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
+	mrc	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
 	mrc	p15, 0, r9, c1, c0, 0	@ control reg
 	bic	r4, r4, #2		@ clear frequency change bit
 	stmia	r0, {r4 - r9}		@ store cp regs
@@ -552,7 +552,7 @@ ENTRY(cpu_xscale_do_resume)
 	mcr	p15, 0, r6, c13, c0, 0	@ PID
 	mcr	p15, 0, r7, c3, c0, 0	@ domain ID
 	mcr	p15, 0, r1, c2, c0, 0	@ translation table base addr
-	mcr	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
+	mcr	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
 	mov	r0, r9			@ control register
 	b	cpu_resume_mmu
 ENDPROC(cpu_xscale_do_resume)
-- 
2.1.1

[PATCH] ARM: xscale: correct auxiliary register in suspend/resume

[Robert Jarzmik]

Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:
...zip...
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Hi Russell,

Is this patch in [1] fine by you so that Dmitry can submit it to your patch system ?

Cheers.

--
Robert

[1] The patch

---8>---
According to the manuals I have, XScale auxiliary register should be
reached with opc_2 = 1 instead of crn = 1. cpu_xscale_proc_init
correctly uses c1, c0, 1 arguments, but cpu_xscale_do_suspend and
cpu_xscale_do_resume use c1, c1, 0. Correct suspend/resume functions to
also use c1, c0, 1.

The issue was primarily noticed thanks to qemu reporing "unsupported
instruction" on the pxa suspend path. Confirmed in PXA210/250 and PXA255
XScale Core manuals and in PXA270 and PXA320 Developers Guides.

Harware tested by me on tosa (pxa255). Robert confirmed on pxa270 board.

Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 arch/arm/mm/proc-xscale.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/proc-xscale.S b/arch/arm/mm/proc-xscale.S
index 23259f1..afa2b3c 100644
--- a/arch/arm/mm/proc-xscale.S
+++ b/arch/arm/mm/proc-xscale.S
@@ -535,7 +535,7 @@ ENTRY(cpu_xscale_do_suspend)
 	mrc	p15, 0, r5, c15, c1, 0	@ CP access reg
 	mrc	p15, 0, r6, c13, c0, 0	@ PID
 	mrc	p15, 0, r7, c3, c0, 0	@ domain ID
-	mrc	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
+	mrc	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
 	mrc	p15, 0, r9, c1, c0, 0	@ control reg
 	bic	r4, r4, #2		@ clear frequency change bit
 	stmia	r0, {r4 - r9}		@ store cp regs
@@ -552,7 +552,7 @@ ENTRY(cpu_xscale_do_resume)
 	mcr	p15, 0, r6, c13, c0, 0	@ PID
 	mcr	p15, 0, r7, c3, c0, 0	@ domain ID
 	mcr	p15, 0, r1, c2, c0, 0	@ translation table base addr
-	mcr	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
+	mcr	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
 	mov	r0, r9			@ control register
 	b	cpu_resume_mmu
 ENDPROC(cpu_xscale_do_resume)

-- 
Robert

[PATCH] ARM: xscale: correct auxiliary register in suspend/resume

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:
> ...zip...
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Hi Russell,
>
> Is this patch in [1] fine by you so that Dmitry can submit it to your patch system ?

OK Dmitry, let's say that Russell didn't object. Please submit it to his patch
system (with the required procedure, kernel version upon which this applies, etc
... all in [2]).

If Russell disagrees, he'll drop the patch and you'll know there is more work.

Thanks.

--
Robert

[1] The patch
>
> ---8>---
> According to the manuals I have, XScale auxiliary register should be
> reached with opc_2 = 1 instead of crn = 1. cpu_xscale_proc_init
> correctly uses c1, c0, 1 arguments, but cpu_xscale_do_suspend and
> cpu_xscale_do_resume use c1, c1, 0. Correct suspend/resume functions to
> also use c1, c0, 1.
>
> The issue was primarily noticed thanks to qemu reporing "unsupported
> instruction" on the pxa suspend path. Confirmed in PXA210/250 and PXA255
> XScale Core manuals and in PXA270 and PXA320 Developers Guides.
>
> Harware tested by me on tosa (pxa255). Robert confirmed on pxa270 board.
>
> Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  arch/arm/mm/proc-xscale.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/proc-xscale.S b/arch/arm/mm/proc-xscale.S
> index 23259f1..afa2b3c 100644
> --- a/arch/arm/mm/proc-xscale.S
> +++ b/arch/arm/mm/proc-xscale.S
> @@ -535,7 +535,7 @@ ENTRY(cpu_xscale_do_suspend)
>  	mrc	p15, 0, r5, c15, c1, 0	@ CP access reg
>  	mrc	p15, 0, r6, c13, c0, 0	@ PID
>  	mrc	p15, 0, r7, c3, c0, 0	@ domain ID
> -	mrc	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
> +	mrc	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
>  	mrc	p15, 0, r9, c1, c0, 0	@ control reg
>  	bic	r4, r4, #2		@ clear frequency change bit
>  	stmia	r0, {r4 - r9}		@ store cp regs
> @@ -552,7 +552,7 @@ ENTRY(cpu_xscale_do_resume)
>  	mcr	p15, 0, r6, c13, c0, 0	@ PID
>  	mcr	p15, 0, r7, c3, c0, 0	@ domain ID
>  	mcr	p15, 0, r1, c2, c0, 0	@ translation table base addr
> -	mcr	p15, 0, r8, c1, c1, 0	@ auxiliary control reg
> +	mcr	p15, 0, r8, c1, c0, 1	@ auxiliary control reg
>  	mov	r0, r9			@ control register
>  	b	cpu_resume_mmu
>  ENDPROC(cpu_xscale_do_resume)

[2] Russell's patch system rules
http://www.arm.linux.org.uk/developer/patches/info.php

[PATCH] ARM: xscale: correct auxiliary register in suspend/resume

[Dmitry Eremin-Solenikov]

2014-11-27 22:00 GMT+03:00 Robert Jarzmik <robert.jarzmik@free.fr>:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>
>> Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:
>> ...zip...
>> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>>
>> Hi Russell,
>>
>> Is this patch in [1] fine by you so that Dmitry can submit it to your patch system ?
>
> OK Dmitry, let's say that Russell didn't object. Please submit it to his patch
> system (with the required procedure, kernel version upon which this applies, etc
> ... all in [2]).

Already in applied:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8216/1

> If Russell disagrees, he'll drop the patch and you'll know there is more work.

-- 
With best wishes
Dmitry