| Message ID | 1415479134-19999-1-git-send-email-dbaryshkov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
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 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.
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
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)
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(-)