Message ID | 20150528163605.GI30984@atomide.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, May 28, 2015 at 09:36:05AM -0700, Tony Lindgren wrote: > * Dave Martin <Dave.Martin@arm.com> [150528 09:19]: > > On Thu, May 28, 2015 at 07:21:25AM -0700, Tony Lindgren wrote: > > > --- a/arch/arm/mach-omap2/sleep34xx.S > > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > > @@ -203,23 +203,8 @@ save_context_wfi: > > > */ > > > ldr r1, kernel_flush > > > blx r1 > > > - /* > > > - * The kernel doesn't interwork: v7_flush_dcache_all in particluar will > > > - * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled. > > > - * This sequence switches back to ARM. Note that .align may insert a > > > - * nop: bx pc needs to be word-aligned in order to work. > > > - */ > > > - THUMB( .thumb ) > > > - THUMB( .align ) > > > - THUMB( bx pc ) > > > - THUMB( nop ) > > > - .arm > > > - > > > > ^ This looks unrelated to the issue? This code is simply unnecessary > > after Russell introduced the "ret" macro in commit 6ebbf2ce43. That > > made v7_flush_dcache_all do a proper interworking return. > > > > It should probably be in a separate commit, or at least mentioned > > explicitly in the commit log. > > Thanks, I've updated the description with the commit info above. > > I'll keep the removal of the duplicate code in this patch, as it's > all related to the mode switching and we don't want to do it twice. > > > > b omap3_do_wfi > > > - > > > -/* > > > - * Local variables > > > - */ > > > > ^ Likewise this is just tidyup, not bug fixing. > > Will keep that too, the local variables comments are now just too > confusing while reading the code after adding the ENDPROC. And that > too is just removal. I have no objection to tidyups ;) So long as the commit message is clear about which parts are bugfix and which parts are tidyup, I'm fine with that. Cheers ---Dave
On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote: > * Dave Martin <Dave.Martin@arm.com> [150528 09:19]: > > On Thu, May 28, 2015 at 07:21:25AM -0700, Tony Lindgren wrote: > > > --- a/arch/arm/mach-omap2/sleep34xx.S > > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > > @@ -203,23 +203,8 @@ save_context_wfi: > > > */ > > > ldr r1, kernel_flush > > > blx r1 > > > - /* > > > - * The kernel doesn't interwork: v7_flush_dcache_all in particluar will > > > - * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled. > > > - * This sequence switches back to ARM. Note that .align may insert a > > > - * nop: bx pc needs to be word-aligned in order to work. > > > - */ > > > - THUMB( .thumb ) > > > - THUMB( .align ) > > > - THUMB( bx pc ) > > > - THUMB( nop ) > > > - .arm > > > - > > > > ^ This looks unrelated to the issue? This code is simply unnecessary > > after Russell introduced the "ret" macro in commit 6ebbf2ce43. That > > made v7_flush_dcache_all do a proper interworking return. > > > > It should probably be in a separate commit, or at least mentioned > > explicitly in the commit log. > > Thanks, I've updated the description with the commit info above. > > I'll keep the removal of the duplicate code in this patch, as it's > all related to the mode switching and we don't want to do it twice. I would have thought that the change is actually necessary after 6ebbf2ce43, because it now returns in ARM mode, which will cause the "bx pc; nop" thumb instruction sequence be misinterpreted as an ARM instruction. Arnd
* Arnd Bergmann <arnd@arndb.de> [150528 11:57]: > On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote: > > * Dave Martin <Dave.Martin@arm.com> [150528 09:19]: > > > On Thu, May 28, 2015 at 07:21:25AM -0700, Tony Lindgren wrote: > > > > --- a/arch/arm/mach-omap2/sleep34xx.S > > > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > > > @@ -203,23 +203,8 @@ save_context_wfi: > > > > */ > > > > ldr r1, kernel_flush > > > > blx r1 > > > > - /* > > > > - * The kernel doesn't interwork: v7_flush_dcache_all in particluar will > > > > - * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled. > > > > - * This sequence switches back to ARM. Note that .align may insert a > > > > - * nop: bx pc needs to be word-aligned in order to work. > > > > - */ > > > > - THUMB( .thumb ) > > > > - THUMB( .align ) > > > > - THUMB( bx pc ) > > > > - THUMB( nop ) > > > > - .arm > > > > - > > > > > > ^ This looks unrelated to the issue? This code is simply unnecessary > > > after Russell introduced the "ret" macro in commit 6ebbf2ce43. That > > > made v7_flush_dcache_all do a proper interworking return. > > > > > > It should probably be in a separate commit, or at least mentioned > > > explicitly in the commit log. > > > > Thanks, I've updated the description with the commit info above. > > > > I'll keep the removal of the duplicate code in this patch, as it's > > all related to the mode switching and we don't want to do it twice. > > I would have thought that the change is actually necessary after > 6ebbf2ce43, because it now returns in ARM mode, which will cause the > "bx pc; nop" thumb instruction sequence be misinterpreted as an > ARM instruction. Could be it's necessary.. But alone that change is not enough. Regards, Tony
On Thu, May 28, 2015 at 07:55:16PM +0100, Arnd Bergmann wrote: > On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote: > > * Dave Martin <Dave.Martin@arm.com> [150528 09:19]: > > > On Thu, May 28, 2015 at 07:21:25AM -0700, Tony Lindgren wrote: > > > > --- a/arch/arm/mach-omap2/sleep34xx.S > > > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > > > @@ -203,23 +203,8 @@ save_context_wfi: > > > > */ > > > > ldr r1, kernel_flush > > > > blx r1 > > > > - /* > > > > - * The kernel doesn't interwork: v7_flush_dcache_all in particluar will > > > > - * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled. > > > > - * This sequence switches back to ARM. Note that .align may insert a > > > > - * nop: bx pc needs to be word-aligned in order to work. > > > > - */ > > > > - THUMB( .thumb ) > > > > - THUMB( .align ) > > > > - THUMB( bx pc ) > > > > - THUMB( nop ) > > > > - .arm > > > > - > > > > > > ^ This looks unrelated to the issue? This code is simply unnecessary > > > after Russell introduced the "ret" macro in commit 6ebbf2ce43. That > > > made v7_flush_dcache_all do a proper interworking return. > > > > > > It should probably be in a separate commit, or at least mentioned > > > explicitly in the commit log. > > > > Thanks, I've updated the description with the commit info above. > > > > I'll keep the removal of the duplicate code in this patch, as it's > > all related to the mode switching and we don't want to do it twice. > > I would have thought that the change is actually necessary after > 6ebbf2ce43, because it now returns in ARM mode, which will cause the > "bx pc; nop" thumb instruction sequence be misinterpreted as an > ARM instruction. > > Arnd You're right. The THUMB2_KERNEL indeed won't work without deleting this code. Cheers ---Dave
* Dave P Martin <Dave.Martin@arm.com> [150529 04:08]: > On Thu, May 28, 2015 at 07:55:16PM +0100, Arnd Bergmann wrote: > > On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote: > > > * Dave Martin <Dave.Martin@arm.com> [150528 09:19]: > > > > On Thu, May 28, 2015 at 07:21:25AM -0700, Tony Lindgren wrote: > > > > > --- a/arch/arm/mach-omap2/sleep34xx.S > > > > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > > > > @@ -203,23 +203,8 @@ save_context_wfi: > > > > > */ > > > > > ldr r1, kernel_flush > > > > > blx r1 > > > > > - /* > > > > > - * The kernel doesn't interwork: v7_flush_dcache_all in particluar will > > > > > - * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled. > > > > > - * This sequence switches back to ARM. Note that .align may insert a > > > > > - * nop: bx pc needs to be word-aligned in order to work. > > > > > - */ > > > > > - THUMB( .thumb ) > > > > > - THUMB( .align ) > > > > > - THUMB( bx pc ) > > > > > - THUMB( nop ) > > > > > - .arm > > > > > - > > > > > > > > ^ This looks unrelated to the issue? This code is simply unnecessary > > > > after Russell introduced the "ret" macro in commit 6ebbf2ce43. That > > > > made v7_flush_dcache_all do a proper interworking return. > > > > > > > > It should probably be in a separate commit, or at least mentioned > > > > explicitly in the commit log. > > > > > > Thanks, I've updated the description with the commit info above. > > > > > > I'll keep the removal of the duplicate code in this patch, as it's > > > all related to the mode switching and we don't want to do it twice. > > > > I would have thought that the change is actually necessary after > > 6ebbf2ce43, because it now returns in ARM mode, which will cause the > > "bx pc; nop" thumb instruction sequence be misinterpreted as an > > ARM instruction. > > > > Arnd > > You're right. The THUMB2_KERNEL indeed won't work without deleting this > code. OK thanks for checking. Regards, Tony
--- a/arch/arm/mach-omap2/sleep34xx.S +++ b/arch/arm/mach-omap2/sleep34xx.S @@ -203,23 +203,8 @@ save_context_wfi: */ ldr r1, kernel_flush blx r1 - /* - * The kernel doesn't interwork: v7_flush_dcache_all in particluar will - * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled. - * This sequence switches back to ARM. Note that .align may insert a - * nop: bx pc needs to be word-aligned in order to work. - */ - THUMB( .thumb ) - THUMB( .align ) - THUMB( bx pc ) - THUMB( nop ) - .arm - b omap3_do_wfi - -/* - * Local variables - */ +ENDPROC(omap34xx_cpu_suspend) omap3_do_wfi_sram_addr: .word omap3_do_wfi_sram kernel_flush: @@ -364,10 +349,7 @@ exit_nonoff_modes: * =================================== */ ldmfd sp!, {r4 - r11, pc} @ restore regs and return - -/* - * Local variables - */ +ENDPROC(omap3_do_wfi) sdrc_power: .word SDRC_POWER_V cm_idlest1_core: