| Message ID | 1422266761-24487-1-git-send-email-wenyou.yang@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 1/26/2015 1:06 PM, Wenyou Yang wrote: > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> > --- > arch/arm/mach-at91/pm_suspend.S | 54 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 53 insertions(+), 1 deletion(-) > diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S > index 122a3f1..e796722 100644 > --- a/arch/arm/mach-at91/pm_suspend.S > +++ b/arch/arm/mach-at91/pm_suspend.S > @@ -53,6 +53,58 @@ mode .req r6 > beq 1b > .endm > > +/* > + * Put the processor to enter the WFI state > + */ > + .macro _do_wfi > + > +#if defined(CONFIG_CPU_V7) > + /* > + * Execute an ISB instruction to flush the pipeline to ensure > + * that all of operations have beem completed. Been. > + */ > + isb > + > + /* > + * Execute an ISB instruction to ensure that all of the ISB again, while you're executing DSB/DMB? > + * CP15 register changes have been committed. > + */ > + dsb > + dmb > + > + /* Disable the processor's clock */ > + mov tmp1, #AT91_PMC_PCK What's 'tmp1'? Is that a register name? > + str tmp1, [pmc, #AT91_PMC_SCDR] > + > + /* Execute a WFI instruction */ Self-obvious comment, I'd say... > + wfi @ Wait For Interrupt > + > + /* > + * CPU can specualatively prefetch the instructions Speculatively. [...] WBR, Sergei
Hi Sergei, Thank you for your review. > -----Original Message----- > From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] > Sent: Monday, January 26, 2015 9:05 PM > To: Yang, Wenyou; Ferre, Nicolas; linux@arm.linux.org.uk > Cc: sylvain.rochet@finsecur.com; Vilchez, Patrice; linux-kernel@vger.kernel.org; > alexandre.belloni@free-electrons.com; peda@axentia.se; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 > > Hello. > > On 1/26/2015 1:06 PM, Wenyou Yang wrote: > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> > > --- > > arch/arm/mach-at91/pm_suspend.S | 54 > ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 53 insertions(+), 1 deletion(-) > > > diff --git a/arch/arm/mach-at91/pm_suspend.S > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644 > > --- a/arch/arm/mach-at91/pm_suspend.S > > +++ b/arch/arm/mach-at91/pm_suspend.S > > @@ -53,6 +53,58 @@ mode .req r6 > > beq 1b > > .endm > > > > +/* > > + * Put the processor to enter the WFI state */ > > + .macro _do_wfi > > + > > +#if defined(CONFIG_CPU_V7) > > + /* > > + * Execute an ISB instruction to flush the pipeline to ensure > > + * that all of operations have beem completed. > > Been. > > > + */ > > + isb > > + > > + /* > > + * Execute an ISB instruction to ensure that all of the > > ISB again, while you're executing DSB/DMB? > Thank you for your pointing. > > + * CP15 register changes have been committed. > > + */ > > + dsb > > + dmb > > + > > + /* Disable the processor's clock */ > > + mov tmp1, #AT91_PMC_PCK > > What's 'tmp1'? Is that a register name? Yes, a register name defined at the head of file. > > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > + > > + /* Execute a WFI instruction */ > > Self-obvious comment, I'd say... > > > + wfi @ Wait For Interrupt > > + > > + /* > > + * CPU can specualatively prefetch the instructions > > Speculatively. Thanks. > > [...] > > WBR, Sergei Best Regards, Wenyou Yang
On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote: Commit log please. > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> > --- > arch/arm/mach-at91/pm_suspend.S | 54 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 53 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S > index 122a3f1..e796722 100644 > --- a/arch/arm/mach-at91/pm_suspend.S > +++ b/arch/arm/mach-at91/pm_suspend.S > @@ -53,6 +53,58 @@ mode .req r6 > beq 1b > .endm > > +/* > + * Put the processor to enter the WFI state > + */ > + .macro _do_wfi You will have to explain why you need this, really. > + > +#if defined(CONFIG_CPU_V7) > + /* > + * Execute an ISB instruction to flush the pipeline to ensure > + * that all of operations have beem completed. s/beem/been > + */ > + isb > + > + /* > + * Execute an ISB instruction to ensure that all of the > + * CP15 register changes have been committed. > + */ > + dsb This is a dsb not an isb. > + dmb You have to explain why you need every single one of these barriers, otherwise I am NAKing this patch. > + > + /* Disable the processor's clock */ > + mov tmp1, #AT91_PMC_PCK > + str tmp1, [pmc, #AT91_PMC_SCDR] > + > + /* Execute a WFI instruction */ > + wfi @ Wait For Interrupt This one looks ok :) > + > + /* > + * CPU can specualatively prefetch the instructions > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. So what ? I suspect your issue is related to wfi completion on pending IRQ. I would like to know the details that describe the issue you are trying to solve here please. > + */ > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > + nop > +#else > + mcr p15, 0, tmp1, c7, c0, 4 > +#endif Tell us what's the problem you have to solve, first, then we will see how to fix it. Thanks, Lorenzo > + > + .endm > + > .text > > /* > @@ -181,7 +233,7 @@ sdr_sr_done: > > skip_disable_main_clock: > /* Wait for interrupt */ > - mcr p15, 0, tmp1, c7, c0, 4 > + _do_wfi > > tst mode, #AT91_PM_SLOW_CLOCK > beq skip_enable_main_clock > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
Hi Lorenzo, Thank you for your review. > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > Sent: Wednesday, January 28, 2015 7:26 PM > To: Yang, Wenyou > Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com; > sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 > > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote: > > Commit log please. Added in the v2.0 > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> > > --- > > arch/arm/mach-at91/pm_suspend.S | 54 > ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 53 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-at91/pm_suspend.S > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644 > > --- a/arch/arm/mach-at91/pm_suspend.S > > +++ b/arch/arm/mach-at91/pm_suspend.S > > @@ -53,6 +53,58 @@ mode .req r6 > > beq 1b > > .endm > > > > +/* > > + * Put the processor to enter the WFI state */ > > + .macro _do_wfi > > You will have to explain why you need this, really. I don't understand your meaning. > > > + > > +#if defined(CONFIG_CPU_V7) > > + /* > > + * Execute an ISB instruction to flush the pipeline to ensure > > + * that all of operations have beem completed. > > s/beem/been Thanks. > > > + */ > > + isb > > + > > + /* > > + * Execute an ISB instruction to ensure that all of the > > + * CP15 register changes have been committed. > > + */ > > + dsb > > This is a dsb not an isb. Changed in the v2.0 > > > + dmb > > You have to explain why you need every single one of these barriers, otherwise I > am NAKing this patch. No need this one? > > > + > > + /* Disable the processor's clock */ > > + mov tmp1, #AT91_PMC_PCK > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > + > > + /* Execute a WFI instruction */ > > + wfi @ Wait For Interrupt > > This one looks ok :) > > > + > > + /* > > + * CPU can specualatively prefetch the instructions > > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. > > So what ? I suspect your issue is related to wfi completion on pending IRQ. I > would like to know the details that describe the issue you are trying to solve here > please. Honestly, I referred to others, I will dig more, and test it. > > > + */ > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > +#else > > + mcr p15, 0, tmp1, c7, c0, 4 > > +#endif > > Tell us what's the problem you have to solve, first, then we will see how to fix it. > > Thanks, > Lorenzo > > > + > > + .endm > > + > > .text > > > > /* > > @@ -181,7 +233,7 @@ sdr_sr_done: > > > > skip_disable_main_clock: > > /* Wait for interrupt */ > > - mcr p15, 0, tmp1, c7, c0, 4 > > + _do_wfi > > > > tst mode, #AT91_PM_SLOW_CLOCK > > beq skip_enable_main_clock > > -- > > 1.7.9.5 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe > > linux-kernel" in the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > Best Regards, Wenyou Yang
On Thu, Jan 29, 2015 at 02:36:01AM +0000, Yang, Wenyou wrote: > Hi Lorenzo, > > Thank you for your review. > > > -----Original Message----- > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > > Sent: Wednesday, January 28, 2015 7:26 PM > > To: Yang, Wenyou > > Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org; > > linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com; > > sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice > > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 > > > > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote: > > > > Commit log please. > Added in the v2.0 > > > > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> > > > --- > > > arch/arm/mach-at91/pm_suspend.S | 54 > > ++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 53 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/mach-at91/pm_suspend.S > > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644 > > > --- a/arch/arm/mach-at91/pm_suspend.S > > > +++ b/arch/arm/mach-at91/pm_suspend.S > > > @@ -53,6 +53,58 @@ mode .req r6 > > > beq 1b > > > .endm > > > > > > +/* > > > + * Put the processor to enter the WFI state */ > > > + .macro _do_wfi > > > > You will have to explain why you need this, really. > I don't understand your meaning. I want to understand why this assembly snippet (that can be rewritten in C BTW): /* Disable the processor's clock */ mov tmp1, #AT91_PMC_PCK str tmp1, [pmc, #AT91_PMC_SCDR] + cpu_do_idle() is not sufficient for you, or put it differently, why do you need this macro. > > > > > > + > > > +#if defined(CONFIG_CPU_V7) > > > + /* > > > + * Execute an ISB instruction to flush the pipeline to ensure > > > + * that all of operations have beem completed. > > > > s/beem/been > Thanks. > > > > > > + */ > > > + isb This isb should not be there, unless you know a reason why it should and you explain it to me. > > > + > > > + /* > > > + * Execute an ISB instruction to ensure that all of the > > > + * CP15 register changes have been committed. > > > + */ > > > + dsb > > > > This is a dsb not an isb. > Changed in the v2.0 Yes, but I still do not understand why you want to execute it before disabling the clocks (I really hope that by "disabling the clocks" you mean "set the power controller to a state when, on wfi execution, the clocks are gated"). > > > > > > + dmb > > > > You have to explain why you need every single one of these barriers, otherwise I > > am NAKing this patch. > No need this one? No, remove it. > > > > > > + > > > + /* Disable the processor's clock */ > > > + mov tmp1, #AT91_PMC_PCK > > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > > + > > > + /* Execute a WFI instruction */ > > > + wfi @ Wait For Interrupt > > > > This one looks ok :) > > > > > + > > > + /* > > > + * CPU can specualatively prefetch the instructions > > > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. > > > > So what ? I suspect your issue is related to wfi completion on pending IRQ. I > > would like to know the details that describe the issue you are trying to solve here > > please. > Honestly, I referred to others, I will dig more, and test it. You should not copy and paste code, because: 1) it might be broken 2) and/or unoptimized 3) and/or does not apply to your platform See my suggestion above, if it does not work for you, you will report the issue and we will take it from there. Thanks, Lorenzo > > > > > > + */ > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > + nop > > > +#else > > > + mcr p15, 0, tmp1, c7, c0, 4 > > > +#endif > > > > Tell us what's the problem you have to solve, first, then we will see how to fix it. > > > > Thanks, > > Lorenzo > > > > > + > > > + .endm > > > + > > > .text > > > > > > /* > > > @@ -181,7 +233,7 @@ sdr_sr_done: > > > > > > skip_disable_main_clock: > > > /* Wait for interrupt */ > > > - mcr p15, 0, tmp1, c7, c0, 4 > > > + _do_wfi > > > > > > tst mode, #AT91_PM_SLOW_CLOCK > > > beq skip_enable_main_clock > > > -- > > > 1.7.9.5 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe > > > linux-kernel" in the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > Best Regards, > Wenyou Yang >
Hi Lorenzo, Thanks a lot. > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > Sent: Thursday, January 29, 2015 8:22 PM > To: Yang, Wenyou > Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com; > sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 > > On Thu, Jan 29, 2015 at 02:36:01AM +0000, Yang, Wenyou wrote: > > Hi Lorenzo, > > > > Thank you for your review. > > > > > -----Original Message----- > > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > > > Sent: Wednesday, January 28, 2015 7:26 PM > > > To: Yang, Wenyou > > > Cc: Ferre, Nicolas; linux@arm.linux.org.uk; > > > linux-arm-kernel@lists.infradead.org; > > > linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com; > > > sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice > > > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support > > > for ARMv7 > > > > > > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote: > > > > > > Commit log please. > > Added in the v2.0 > > > > > > > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> > > > > --- > > > > arch/arm/mach-at91/pm_suspend.S | 54 > > > ++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 53 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm/mach-at91/pm_suspend.S > > > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644 > > > > --- a/arch/arm/mach-at91/pm_suspend.S > > > > +++ b/arch/arm/mach-at91/pm_suspend.S > > > > @@ -53,6 +53,58 @@ mode .req r6 > > > > beq 1b > > > > .endm > > > > > > > > +/* > > > > + * Put the processor to enter the WFI state */ > > > > + .macro _do_wfi > > > > > > You will have to explain why you need this, really. > > I don't understand your meaning. > > I want to understand why this assembly snippet (that can be rewritten in C BTW): > > /* Disable the processor's clock */ > mov tmp1, #AT91_PMC_PCK > str tmp1, [pmc, #AT91_PMC_SCDR] > > + > > cpu_do_idle() > > is not sufficient for you, or put it differently, why do you need this macro. This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state. So, it can't invoke other functions generally. > > > > > > > > > > + > > > > +#if defined(CONFIG_CPU_V7) > > > > + /* > > > > + * Execute an ISB instruction to flush the pipeline to ensure > > > > + * that all of operations have beem completed. > > > > > > s/beem/been > > Thanks. > > > > > > > > > + */ > > > > + isb > > This isb should not be there, unless you know a reason why it should and you > explain it to me. I encountered system lock during verifying the pm function. Anyway, I will tested again whether it works after removing it. > > > > > + > > > > + /* > > > > + * Execute an ISB instruction to ensure that all of the > > > > + * CP15 register changes have been committed. > > > > + */ > > > > + dsb > > > > > > This is a dsb not an isb. > > Changed in the v2.0 > > Yes, but I still do not understand why you want to execute it before disabling the > clocks (I really hope that by "disabling the clocks" you mean "set the power > controller to a state when, on wfi execution, the clocks are gated"). Are you meaning to execute dsb and wfi after disabling the clocks? > > > > > > > > > > + dmb > > > > > > You have to explain why you need every single one of these barriers, > > > otherwise I am NAKing this patch. > > No need this one? > > No, remove it. OK, thanks > > > > > > > > > > + > > > > + /* Disable the processor's clock */ > > > > + mov tmp1, #AT91_PMC_PCK > > > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > > > + > > > > + /* Execute a WFI instruction */ > > > > + wfi @ Wait For Interrupt > > > > > > This one looks ok :) > > > > > > > + > > > > + /* > > > > + * CPU can specualatively prefetch the instructions > > > > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. > > > > > > So what ? I suspect your issue is related to wfi completion on > > > pending IRQ. I would like to know the details that describe the > > > issue you are trying to solve here please. > > Honestly, I referred to others, I will dig more, and test it. > > You should not copy and paste code, because: > > 1) it might be broken > 2) and/or unoptimized > 3) and/or does not apply to your platform > > See my suggestion above, if it does not work for you, you will report the issue and > we will take it from there. OK, thanks. > > Thanks, > Lorenzo > > > > > > > > > > + */ > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > + nop > > > > +#else > > > > + mcr p15, 0, tmp1, c7, c0, 4 > > > > +#endif > > > > > > Tell us what's the problem you have to solve, first, then we will see how to fix it. > > > > > > Thanks, > > > Lorenzo > > > > > > > + > > > > + .endm > > > > + > > > > .text > > > > > > > > /* > > > > @@ -181,7 +233,7 @@ sdr_sr_done: > > > > > > > > skip_disable_main_clock: > > > > /* Wait for interrupt */ > > > > - mcr p15, 0, tmp1, c7, c0, 4 > > > > + _do_wfi > > > > > > > > tst mode, #AT91_PM_SLOW_CLOCK > > > > beq skip_enable_main_clock > > > > -- > > > > 1.7.9.5 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe > > > > linux-kernel" in the body of a message to > > > > majordomo@vger.kernel.org More majordomo info at > > > > http://vger.kernel.org/majordomo-info.html > > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > > > > Best Regards, > > Wenyou Yang > > Best Regards, Wenyou Yang
On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote: [...] > > > > > + * Put the processor to enter the WFI state */ > > > > > + .macro _do_wfi > > > > > > > > You will have to explain why you need this, really. > > > I don't understand your meaning. > > > > I want to understand why this assembly snippet (that can be rewritten in C BTW): > > > > /* Disable the processor's clock */ > > mov tmp1, #AT91_PMC_PCK > > str tmp1, [pmc, #AT91_PMC_SCDR] > > > > + > > > > cpu_do_idle() > > > > is not sufficient for you, or put it differently, why do you need this macro. > This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state. > So, it can't invoke other functions generally. Ok, thanks for explaining, I will have a look again to check where you use the macro to verify the code flow. > > > > > > > > > > > > > > > + > > > > > +#if defined(CONFIG_CPU_V7) > > > > > + /* > > > > > + * Execute an ISB instruction to flush the pipeline to ensure > > > > > + * that all of operations have beem completed. > > > > > > > > s/beem/been > > > Thanks. > > > > > > > > > > > > + */ > > > > > + isb > > > > This isb should not be there, unless you know a reason why it should and you > > explain it to me. > I encountered system lock during verifying the pm function. > Anyway, I will tested again whether it works after removing it. See above, I have to check when you switch to SRAM execution to see if an isb is appropriate there, it is not self-explanatory. Thank you, Lorenzo > > > > > > > > + > > > > > + /* > > > > > + * Execute an ISB instruction to ensure that all of the > > > > > + * CP15 register changes have been committed. > > > > > + */ > > > > > + dsb > > > > > > > > This is a dsb not an isb. > > > Changed in the v2.0 > > > > Yes, but I still do not understand why you want to execute it before disabling the > > clocks (I really hope that by "disabling the clocks" you mean "set the power > > controller to a state when, on wfi execution, the clocks are gated"). > Are you meaning to execute dsb and wfi after disabling the clocks? > > > > > > > > > > > > > > > + dmb > > > > > > > > You have to explain why you need every single one of these barriers, > > > > otherwise I am NAKing this patch. > > > No need this one? > > > > No, remove it. > OK, thanks > > > > > > > > > > > > > > > + > > > > > + /* Disable the processor's clock */ > > > > > + mov tmp1, #AT91_PMC_PCK > > > > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > > > > + > > > > > + /* Execute a WFI instruction */ > > > > > + wfi @ Wait For Interrupt > > > > > > > > This one looks ok :) > > > > > > > > > + > > > > > + /* > > > > > + * CPU can specualatively prefetch the instructions > > > > > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. > > > > > > > > So what ? I suspect your issue is related to wfi completion on > > > > pending IRQ. I would like to know the details that describe the > > > > issue you are trying to solve here please. > > > Honestly, I referred to others, I will dig more, and test it. > > > > You should not copy and paste code, because: > > > > 1) it might be broken > > 2) and/or unoptimized > > 3) and/or does not apply to your platform > > > > See my suggestion above, if it does not work for you, you will report the issue and > > we will take it from there. > OK, thanks. > > > > > Thanks, > > Lorenzo > > > > > > > > > > > > > > + */ > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > +#else > > > > > + mcr p15, 0, tmp1, c7, c0, 4 > > > > > +#endif > > > > > > > > Tell us what's the problem you have to solve, first, then we will see how to fix it. > > > > > > > > Thanks, > > > > Lorenzo > > > > > > > > > + > > > > > + .endm > > > > > + > > > > > .text > > > > > > > > > > /* > > > > > @@ -181,7 +233,7 @@ sdr_sr_done: > > > > > > > > > > skip_disable_main_clock: > > > > > /* Wait for interrupt */ > > > > > - mcr p15, 0, tmp1, c7, c0, 4 > > > > > + _do_wfi > > > > > > > > > > tst mode, #AT91_PM_SLOW_CLOCK > > > > > beq skip_enable_main_clock > > > > > -- > > > > > 1.7.9.5 > > > > > > > > > > -- > > > > > To unsubscribe from this list: send the line "unsubscribe > > > > > linux-kernel" in the body of a message to > > > > > majordomo@vger.kernel.org More majordomo info at > > > > > http://vger.kernel.org/majordomo-info.html > > > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > > > > > > > Best Regards, > > > Wenyou Yang > > > > > Best Regards, > Wenyou Yang >
On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote: [...] > > > > > + */ > > > > > + isb > > > > This isb should not be there, unless you know a reason why it should and you > > explain it to me. > I encountered system lock during verifying the pm function. > Anyway, I will tested again whether it works after removing it. Re-checked the code flow, I see no reason why this isb should be there. Adding barriers changes code execution, this might paper over some issues instead of solving them, so every single barrier should be commented and added with a specific purpose that I do not see here. Lorenzo
diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644 --- a/arch/arm/mach-at91/pm_suspend.S +++ b/arch/arm/mach-at91/pm_suspend.S @@ -53,6 +53,58 @@ mode .req r6 beq 1b .endm +/* + * Put the processor to enter the WFI state + */ + .macro _do_wfi + +#if defined(CONFIG_CPU_V7) + /* + * Execute an ISB instruction to flush the pipeline to ensure + * that all of operations have beem completed. + */ + isb + + /* + * Execute an ISB instruction to ensure that all of the + * CP15 register changes have been committed. + */ + dsb + dmb + + /* Disable the processor's clock */ + mov tmp1, #AT91_PMC_PCK + str tmp1, [pmc, #AT91_PMC_SCDR] + + /* Execute a WFI instruction */ + wfi @ Wait For Interrupt + + /* + * CPU can specualatively prefetch the instructions + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. + */ + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop + nop +#else + mcr p15, 0, tmp1, c7, c0, 4 +#endif + + .endm + .text /* @@ -181,7 +233,7 @@ sdr_sr_done: skip_disable_main_clock: /* Wait for interrupt */ - mcr p15, 0, tmp1, c7, c0, 4 + _do_wfi tst mode, #AT91_PM_SLOW_CLOCK beq skip_enable_main_clock
Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> --- arch/arm/mach-at91/pm_suspend.S | 54 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)