| Message ID | 1389686397-46555-3-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Scott Wood |
| Headers | show |
On Tue, 2014-01-14 at 15:59 +0800, Dongsheng Wang wrote: > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > Use fsl_cpu_state_save/fsl_cpu_state_restore to save/restore registers. > Use the functions to save/restore registers, so we don't need to > maintain the code. > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> Is there any functional change with this patchset (e.g. suspend supported on chips where it wasn't before), or is it just cleanup? A cover letter would be useful to describe the purpose of the overall patchset when it isn't obvious. > > diff --git a/arch/powerpc/kernel/swsusp_booke.S b/arch/powerpc/kernel/swsusp_booke.S > index 553c140..b5992db 100644 > --- a/arch/powerpc/kernel/swsusp_booke.S > +++ b/arch/powerpc/kernel/swsusp_booke.S > @@ -4,92 +4,28 @@ > * Copyright (c) 2009-2010 MontaVista Software, LLC. > */ > > -#include <linux/threads.h> > -#include <asm/processor.h> > #include <asm/page.h> > -#include <asm/cputable.h> > -#include <asm/thread_info.h> > #include <asm/ppc_asm.h> > #include <asm/asm-offsets.h> > #include <asm/mmu.h> > - > -/* > - * Structure for storing CPU registers on the save area. > - */ > -#define SL_SP 0 > -#define SL_PC 4 > -#define SL_MSR 8 > -#define SL_TCR 0xc > -#define SL_SPRG0 0x10 > -#define SL_SPRG1 0x14 > -#define SL_SPRG2 0x18 > -#define SL_SPRG3 0x1c > -#define SL_SPRG4 0x20 > -#define SL_SPRG5 0x24 > -#define SL_SPRG6 0x28 > -#define SL_SPRG7 0x2c > -#define SL_TBU 0x30 > -#define SL_TBL 0x34 > -#define SL_R2 0x38 > -#define SL_CR 0x3c > -#define SL_LR 0x40 > -#define SL_R12 0x44 /* r12 to r31 */ > -#define SL_SIZE (SL_R12 + 80) > - > - .section .data > - .align 5 > - > -_GLOBAL(swsusp_save_area) > - .space SL_SIZE > - > +#include <asm/fsl_sleep.h> > > .section .text > .align 5 > > _GLOBAL(swsusp_arch_suspend) > - lis r11,swsusp_save_area@h > - ori r11,r11,swsusp_save_area@l > - > - mflr r0 > - stw r0,SL_LR(r11) > - mfcr r0 > - stw r0,SL_CR(r11) > - stw r1,SL_SP(r11) > - stw r2,SL_R2(r11) > - stmw r12,SL_R12(r11) > - > - /* Save MSR & TCR */ > - mfmsr r4 > - stw r4,SL_MSR(r11) > - mfspr r4,SPRN_TCR > - stw r4,SL_TCR(r11) > - > - /* Get a stable timebase and save it */ > -1: mfspr r4,SPRN_TBRU > - stw r4,SL_TBU(r11) > - mfspr r5,SPRN_TBRL > - stw r5,SL_TBL(r11) > - mfspr r3,SPRN_TBRU > - cmpw r3,r4 > - bne 1b > + mflr r15 > + lis r3, core_registers_save_area@h > + ori r3, r3, core_registers_save_area@l > + > + /* Save base register */ > + li r4, 0 > + bl fsl_cpu_state_save > > - /* Save SPRGs */ > - mfspr r4,SPRN_SPRG0 > - stw r4,SL_SPRG0(r11) > - mfspr r4,SPRN_SPRG1 > - stw r4,SL_SPRG1(r11) > - mfspr r4,SPRN_SPRG2 > - stw r4,SL_SPRG2(r11) > - mfspr r4,SPRN_SPRG3 > - stw r4,SL_SPRG3(r11) > - mfspr r4,SPRN_SPRG4 > - stw r4,SL_SPRG4(r11) > - mfspr r4,SPRN_SPRG5 > - stw r4,SL_SPRG5(r11) > - mfspr r4,SPRN_SPRG6 > - stw r4,SL_SPRG6(r11) > - mfspr r4,SPRN_SPRG7 > - stw r4,SL_SPRG7(r11) > + /* Save LR */ > + lis r3, core_registers_save_area@h > + ori r3, r3, core_registers_save_area@l > + stw r15, SR_LR(r3) > > /* Call the low level suspend stuff (we should probably have made > * a stackframe... > @@ -97,11 +33,12 @@ _GLOBAL(swsusp_arch_suspend) > bl swsusp_save > > /* Restore LR from the save area */ > - lis r11,swsusp_save_area@h > - ori r11,r11,swsusp_save_area@l > - lwz r0,SL_LR(r11) > - mtlr r0 > + lis r3, core_registers_save_area@h > + ori r3, r3, core_registers_save_area@l > + lwz r15, SR_LR(r3) > + mtlr r15 > > + li r3, 0 > blr > > _GLOBAL(swsusp_arch_resume) > @@ -138,9 +75,6 @@ _GLOBAL(swsusp_arch_resume) > bl flush_dcache_L1 > bl flush_instruction_cache > > - lis r11,swsusp_save_area@h > - ori r11,r11,swsusp_save_area@l > - > /* > * Mappings from virtual addresses to physical addresses may be > * different than they were prior to restoring hibernation state. > @@ -149,53 +83,12 @@ _GLOBAL(swsusp_arch_resume) > */ > bl _tlbil_all > > - lwz r4,SL_SPRG0(r11) > - mtspr SPRN_SPRG0,r4 > - lwz r4,SL_SPRG1(r11) > - mtspr SPRN_SPRG1,r4 > - lwz r4,SL_SPRG2(r11) > - mtspr SPRN_SPRG2,r4 > - lwz r4,SL_SPRG3(r11) > - mtspr SPRN_SPRG3,r4 > - lwz r4,SL_SPRG4(r11) > - mtspr SPRN_SPRG4,r4 > - lwz r4,SL_SPRG5(r11) > - mtspr SPRN_SPRG5,r4 > - lwz r4,SL_SPRG6(r11) > - mtspr SPRN_SPRG6,r4 > - lwz r4,SL_SPRG7(r11) > - mtspr SPRN_SPRG7,r4 > - > - /* restore the MSR */ > - lwz r3,SL_MSR(r11) > - mtmsr r3 > - > - /* Restore TB */ > - li r3,0 > - mtspr SPRN_TBWL,r3 > - lwz r3,SL_TBU(r11) > - lwz r4,SL_TBL(r11) > - mtspr SPRN_TBWU,r3 > - mtspr SPRN_TBWL,r4 > - > - /* Restore TCR and clear any pending bits in TSR. */ > - lwz r4,SL_TCR(r11) > - mtspr SPRN_TCR,r4 > - lis r4, (TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS)@h > - mtspr SPRN_TSR,r4 > - > - /* Kick decrementer */ > - li r0,1 > - mtdec r0 > - > - /* Restore the callee-saved registers and return */ > - lwz r0,SL_CR(r11) > - mtcr r0 > - lwz r2,SL_R2(r11) > - lmw r12,SL_R12(r11) > - lwz r1,SL_SP(r11) > - lwz r0,SL_LR(r11) > - mtlr r0 > + lis r3, core_registers_save_area@h > + ori r3, r3, core_registers_save_area@l > + > + /* Restore base register */ > + li r4, 0 > + bl fsl_cpu_state_restore Why are you calling anything with "fsl" in the name from code that is supposed to be for all booke? -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, January 15, 2014 7:30 AM > To: Wang Dongsheng-B40534 > Cc: benh@kernel.crashing.org; Zhao Chenhui-B35336; anton@enomsg.org; linuxppc- > dev@lists.ozlabs.org > Subject: Re: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore > registers > > On Tue, 2014-01-14 at 15:59 +0800, Dongsheng Wang wrote: > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > Use fsl_cpu_state_save/fsl_cpu_state_restore to save/restore registers. > > Use the functions to save/restore registers, so we don't need to > > maintain the code. > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > Is there any functional change with this patchset (e.g. suspend > supported on chips where it wasn't before), or is it just cleanup? A > cover letter would be useful to describe the purpose of the overall > patchset when it isn't obvious. > Yes, just cleanup.. > > + > > + /* Restore base register */ > > + li r4, 0 > > + bl fsl_cpu_state_restore > > Why are you calling anything with "fsl" in the name from code that is > supposed to be for all booke? > E200, E300 not support. Support E500, E500v2, E500MC, E5500, E6500. Do you have any suggestions about this? Thanks, -Dongsheng
On Tue, 2014-01-14 at 20:57 -0600, Wang Dongsheng-B40534 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Wednesday, January 15, 2014 7:30 AM > > To: Wang Dongsheng-B40534 > > Cc: benh@kernel.crashing.org; Zhao Chenhui-B35336; anton@enomsg.org; linuxppc- > > dev@lists.ozlabs.org > > Subject: Re: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore > > registers > > > > On Tue, 2014-01-14 at 15:59 +0800, Dongsheng Wang wrote: > > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > > > Use fsl_cpu_state_save/fsl_cpu_state_restore to save/restore registers. > > > Use the functions to save/restore registers, so we don't need to > > > maintain the code. > > > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > Is there any functional change with this patchset (e.g. suspend > > supported on chips where it wasn't before), or is it just cleanup? A > > cover letter would be useful to describe the purpose of the overall > > patchset when it isn't obvious. > > > > Yes, just cleanup.. It seems to be introducing complexity rather than removing it. Is this cleanup needed to prepare for adding new functionality? Plus, I'm skeptical that this is functionally equivalent. It looks like the new code saves a lot more than the old code does. Why? > > > + > > > + /* Restore base register */ > > > + li r4, 0 > > > + bl fsl_cpu_state_restore > > > > Why are you calling anything with "fsl" in the name from code that is > > supposed to be for all booke? > > > E200, E300 not support. > Support E500, E500v2, E500MC, E5500, E6500. > > Do you have any suggestions about this? What about non-FSL booke such as 44x? Or if this file never supported 44x, rename it appropriately. -Scott
> > > > Use fsl_cpu_state_save/fsl_cpu_state_restore to save/restore registers. > > > > Use the functions to save/restore registers, so we don't need to > > > > maintain the code. > > > > > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > > > Is there any functional change with this patchset (e.g. suspend > > > supported on chips where it wasn't before), or is it just cleanup? A > > > cover letter would be useful to describe the purpose of the overall > > > patchset when it isn't obvious. > > > > > > > Yes, just cleanup.. > > It seems to be introducing complexity rather than removing it. Is this > cleanup needed to prepare for adding new functionality? > > Plus, I'm skeptical that this is functionally equivalent. It looks like > the new code saves a lot more than the old code does. Why? > Actually, I want to take a practical example to push the save/restore patches. And this is also reasonable for 32bit-hibernation, the code is more clean. :) I think I need to change the description of the patch. > > > > + > > > > + /* Restore base register */ > > > > + li r4, 0 > > > > + bl fsl_cpu_state_restore > > > > > > Why are you calling anything with "fsl" in the name from code that is > > > supposed to be for all booke? > > > > > E200, E300 not support. > > Support E500, E500v2, E500MC, E5500, E6500. > > > > Do you have any suggestions about this? > > What about non-FSL booke such as 44x? > > Or if this file never supported 44x, rename it appropriately. > Currently does not support. ok change the name first, if later support, and then again to modify the name of this function. How about 85xx_cpu_state_restore? Thanks, -Dongsheng
On Sun, 2014-01-19 at 23:57 -0600, Wang Dongsheng-B40534 wrote: > > > > > Use fsl_cpu_state_save/fsl_cpu_state_restore to save/restore registers. > > > > > Use the functions to save/restore registers, so we don't need to > > > > > maintain the code. > > > > > > > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > > > > > Is there any functional change with this patchset (e.g. suspend > > > > supported on chips where it wasn't before), or is it just cleanup? A > > > > cover letter would be useful to describe the purpose of the overall > > > > patchset when it isn't obvious. > > > > > > > > > > Yes, just cleanup.. > > > > It seems to be introducing complexity rather than removing it. Is this > > cleanup needed to prepare for adding new functionality? > > > > Plus, I'm skeptical that this is functionally equivalent. It looks like > > the new code saves a lot more than the old code does. Why? > > > > Actually, I want to take a practical example to push the save/restore patches. > And this is also reasonable for 32bit-hibernation, the code is more clean. :) > I think I need to change the description of the patch. > > > > > > + > > > > > + /* Restore base register */ > > > > > + li r4, 0 > > > > > + bl fsl_cpu_state_restore > > > > > > > > Why are you calling anything with "fsl" in the name from code that is > > > > supposed to be for all booke? > > > > > > > E200, E300 not support. > > > Support E500, E500v2, E500MC, E5500, E6500. > > > > > > Do you have any suggestions about this? > > > > What about non-FSL booke such as 44x? > > > > Or if this file never supported 44x, rename it appropriately. > > > Currently does not support. ok change the name first, if later support, and > then again to modify the name of this function. > > How about 85xx_cpu_state_restore? Symbols can't begin with numbers. booke_cpu_state_restore would be better (it would still provide a place for 44x to be added if somebody actually cared about doing so). I'm still not convinced that asm code is the place to do this, though. -Scott
> > Currently does not support. ok change the name first, if later support, and > > then again to modify the name of this function. > > > > How about 85xx_cpu_state_restore? > > Symbols can't begin with numbers. booke_cpu_state_restore would be > better (it would still provide a place for 44x to be added if somebody > actually cared about doing so). > :). Thanks. -Dongsheng
diff --git a/arch/powerpc/kernel/swsusp_booke.S b/arch/powerpc/kernel/swsusp_booke.S index 553c140..b5992db 100644 --- a/arch/powerpc/kernel/swsusp_booke.S +++ b/arch/powerpc/kernel/swsusp_booke.S @@ -4,92 +4,28 @@ * Copyright (c) 2009-2010 MontaVista Software, LLC. */ -#include <linux/threads.h> -#include <asm/processor.h> #include <asm/page.h> -#include <asm/cputable.h> -#include <asm/thread_info.h> #include <asm/ppc_asm.h> #include <asm/asm-offsets.h> #include <asm/mmu.h> - -/* - * Structure for storing CPU registers on the save area. - */ -#define SL_SP 0 -#define SL_PC 4 -#define SL_MSR 8 -#define SL_TCR 0xc -#define SL_SPRG0 0x10 -#define SL_SPRG1 0x14 -#define SL_SPRG2 0x18 -#define SL_SPRG3 0x1c -#define SL_SPRG4 0x20 -#define SL_SPRG5 0x24 -#define SL_SPRG6 0x28 -#define SL_SPRG7 0x2c -#define SL_TBU 0x30 -#define SL_TBL 0x34 -#define SL_R2 0x38 -#define SL_CR 0x3c -#define SL_LR 0x40 -#define SL_R12 0x44 /* r12 to r31 */ -#define SL_SIZE (SL_R12 + 80) - - .section .data - .align 5 - -_GLOBAL(swsusp_save_area) - .space SL_SIZE - +#include <asm/fsl_sleep.h> .section .text .align 5 _GLOBAL(swsusp_arch_suspend) - lis r11,swsusp_save_area@h - ori r11,r11,swsusp_save_area@l - - mflr r0 - stw r0,SL_LR(r11) - mfcr r0 - stw r0,SL_CR(r11) - stw r1,SL_SP(r11) - stw r2,SL_R2(r11) - stmw r12,SL_R12(r11) - - /* Save MSR & TCR */ - mfmsr r4 - stw r4,SL_MSR(r11) - mfspr r4,SPRN_TCR - stw r4,SL_TCR(r11) - - /* Get a stable timebase and save it */ -1: mfspr r4,SPRN_TBRU - stw r4,SL_TBU(r11) - mfspr r5,SPRN_TBRL - stw r5,SL_TBL(r11) - mfspr r3,SPRN_TBRU - cmpw r3,r4 - bne 1b + mflr r15 + lis r3, core_registers_save_area@h + ori r3, r3, core_registers_save_area@l + + /* Save base register */ + li r4, 0 + bl fsl_cpu_state_save - /* Save SPRGs */ - mfspr r4,SPRN_SPRG0 - stw r4,SL_SPRG0(r11) - mfspr r4,SPRN_SPRG1 - stw r4,SL_SPRG1(r11) - mfspr r4,SPRN_SPRG2 - stw r4,SL_SPRG2(r11) - mfspr r4,SPRN_SPRG3 - stw r4,SL_SPRG3(r11) - mfspr r4,SPRN_SPRG4 - stw r4,SL_SPRG4(r11) - mfspr r4,SPRN_SPRG5 - stw r4,SL_SPRG5(r11) - mfspr r4,SPRN_SPRG6 - stw r4,SL_SPRG6(r11) - mfspr r4,SPRN_SPRG7 - stw r4,SL_SPRG7(r11) + /* Save LR */ + lis r3, core_registers_save_area@h + ori r3, r3, core_registers_save_area@l + stw r15, SR_LR(r3) /* Call the low level suspend stuff (we should probably have made * a stackframe... @@ -97,11 +33,12 @@ _GLOBAL(swsusp_arch_suspend) bl swsusp_save /* Restore LR from the save area */ - lis r11,swsusp_save_area@h - ori r11,r11,swsusp_save_area@l - lwz r0,SL_LR(r11) - mtlr r0 + lis r3, core_registers_save_area@h + ori r3, r3, core_registers_save_area@l + lwz r15, SR_LR(r3) + mtlr r15 + li r3, 0 blr _GLOBAL(swsusp_arch_resume) @@ -138,9 +75,6 @@ _GLOBAL(swsusp_arch_resume) bl flush_dcache_L1 bl flush_instruction_cache - lis r11,swsusp_save_area@h - ori r11,r11,swsusp_save_area@l - /* * Mappings from virtual addresses to physical addresses may be * different than they were prior to restoring hibernation state. @@ -149,53 +83,12 @@ _GLOBAL(swsusp_arch_resume) */ bl _tlbil_all - lwz r4,SL_SPRG0(r11) - mtspr SPRN_SPRG0,r4 - lwz r4,SL_SPRG1(r11) - mtspr SPRN_SPRG1,r4 - lwz r4,SL_SPRG2(r11) - mtspr SPRN_SPRG2,r4 - lwz r4,SL_SPRG3(r11) - mtspr SPRN_SPRG3,r4 - lwz r4,SL_SPRG4(r11) - mtspr SPRN_SPRG4,r4 - lwz r4,SL_SPRG5(r11) - mtspr SPRN_SPRG5,r4 - lwz r4,SL_SPRG6(r11) - mtspr SPRN_SPRG6,r4 - lwz r4,SL_SPRG7(r11) - mtspr SPRN_SPRG7,r4 - - /* restore the MSR */ - lwz r3,SL_MSR(r11) - mtmsr r3 - - /* Restore TB */ - li r3,0 - mtspr SPRN_TBWL,r3 - lwz r3,SL_TBU(r11) - lwz r4,SL_TBL(r11) - mtspr SPRN_TBWU,r3 - mtspr SPRN_TBWL,r4 - - /* Restore TCR and clear any pending bits in TSR. */ - lwz r4,SL_TCR(r11) - mtspr SPRN_TCR,r4 - lis r4, (TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS)@h - mtspr SPRN_TSR,r4 - - /* Kick decrementer */ - li r0,1 - mtdec r0 - - /* Restore the callee-saved registers and return */ - lwz r0,SL_CR(r11) - mtcr r0 - lwz r2,SL_R2(r11) - lmw r12,SL_R12(r11) - lwz r1,SL_SP(r11) - lwz r0,SL_LR(r11) - mtlr r0 + lis r3, core_registers_save_area@h + ori r3, r3, core_registers_save_area@l + + /* Restore base register */ + li r4, 0 + bl fsl_cpu_state_restore li r3,0 blr