Re: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore registers
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.w...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore registers
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore registers
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.w...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore registers
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: b...@kernel.crashing.org; Zhao Chenhui-B35336; an...@enomsg.org; linuxppc- d...@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.w...@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.w...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 3/3] powerpc/fsl: Use the new interface to save or restore registers
From: Wang Dongsheng dongsheng.w...@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.w...@freescale.com 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 - - mflrr0 - stw r0,SL_LR(r11) - mfcrr0 - stw r0,SL_CR(r11) - stw r1,SL_SP(r11) - stw r2,SL_R2(r11) - stmwr12,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 - cmpwr3,r4 - bne 1b + mflrr15 + 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) - mtlrr0 + lis r3, core_registers_save_area@h + ori r3, r3, core_registers_save_area@l + lwz r15, SR_LR(r3) + mtlrr15 + 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. */ -
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.w...@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.w...@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_SP0 -#define SL_PC4 -#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_R20x38 -#define SL_CR0x3c -#define SL_LR0x40 -#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 - - mflrr0 - stw r0,SL_LR(r11) - mfcrr0 - stw r0,SL_CR(r11) - stw r1,SL_SP(r11) - stw r2,SL_R2(r11) - stmwr12,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 - cmpwr3,r4 - bne 1b + mflrr15 + 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) - mtlrr0 + lis r3, core_registers_save_area@h + ori r3, r3, core_registers_save_area@l + lwz r15, SR_LR(r3) + mtlrr15 + 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
RE: [PATCH 3/3] powerpc/fsl: Use the new interface to save or restore registers
-Original Message- From: Wood Scott-B07421 Sent: Wednesday, January 15, 2014 7:30 AM To: Wang Dongsheng-B40534 Cc: b...@kernel.crashing.org; Zhao Chenhui-B35336; an...@enomsg.org; linuxppc- d...@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.w...@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.w...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
