From: "Mark A. Greer" <mgreer@animalcreek.com> To: Paul Walmsley <paul@pwsan.com> Cc: Ranjith Lohithakshan <ranjithl@ti.com>, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, khilman@ti.com Subject: Re: [PATCH 11/12] arm: omap3: am35x: Add do_wfi routine for EMIF4 submodules Date: Thu, 12 Apr 2012 17:12:27 -0700 [thread overview] Message-ID: <20120413001227.GP31197@animalcreek.com> (raw) In-Reply-To: <alpine.DEB.2.00.1204111623110.29473@utopia.booyaka.com> On Wed, Apr 11, 2012 at 04:36:25PM -0600, Paul Walmsley wrote: > Hi > > just a few comments based on a quick look > > On Wed, 11 Apr 2012, Mark A. Greer wrote: > > diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S > > index 1f62f23..22aac4c 100644 > > --- a/arch/arm/mach-omap2/sleep34xx.S > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > @@ -391,6 +401,52 @@ wait_dll_lock_counter: > > ENTRY(omap3_do_wfi_sz) > > .word . - omap3_do_wfi > > > > +/* > > + * Do WFI instruction (SDRC module has EMIF4 submodule) > > + * > > + * This code gets copied to internal SRAM and is accessible > > + * from both SDRAM and SRAM: > > + * - executed from SRAM for non-off modes (omap3_emif4_do_wfi_sram), > > + * - executed from SDRAM for OFF mode (omap3_emif4_do_wfi). > > + */ > > + .align 3 > > +ENTRY(omap3_emif4_do_wfi) > > + /* Put EMIF in self-refresh */ > > Hmm, I guess this comment isn't quite right; it just tells it to go to > self-refresh when it's "idle" ? (see below) > > > + ldr r4, pwr_mgmt_ctrl > > + ldr r5, [r4] > > + orr r5, r5, #0x200 > > Maybe use a macro here in place of the #0x200 to indicate what it's trying > to set... > > Do you happen to know what "idle" means in the context of SPRUGR0B Section > 9.2.3.4.5.1 "SDRAM Self-Refresh Mode" ? Is it referring to > target module-level idle, e.g. SIdleReq; or is it referring to > an internal definition of idle, e.g., something like "no reads or writes > from/to the SDRAM" ? Am wondering if this should just be set once during > EMIF initialization. I can't tell from tne manual. I assumed it meant the latter of the choices you listed. I just tested a kernel with it set up during EMIF init (value of 0x80000200) and removed the related bits from omap3_emif4_do_wfi() and it booted and returned from suspend-to-RAM just fine. So, it seems that it can be moved to EMIF init. [I swear I tested this some time ago and discovered that it was needed in omap3_emif4_do_wfi(). Oh well...] > > > + str r5, [r4] > > Might be good to do a readback after this to ensure that the store has > made it to the EMIF. > > Also, scanning SPRUGR0B, it looks like this setting is dependent partially > on the REG_PM_TIM field in the same register. Might be good to set > REG_PM_TIM during EMIF init to avoid any bootloader dependency. True but moot now since I'll move the code to EMIF init. > > + > > + dsb > > + dmb > > + > > + wfi > > + > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > Are these NOPs necessary? Is the number of NOPs dependent on CPU or bus > speed or some ARM parameter? After looking through the am35x errata, I don't think so. AFAICT, they are there to work around--from omap3535 errata (sprz278f.pdf)-- Advisory 3.1.1.75, "IVA2: CAM/SGX Dependencies (OMAP3530/25 only)". I don't see that or any other errata that requires a nop in the am35x errata so I'll get rid of them. [As I went through the errata again, I noticed talk about the device going into RET state. Sigh...] Mark --
WARNING: multiple messages have this Message-ID (diff)
From: mgreer@animalcreek.com (Mark A. Greer) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 11/12] arm: omap3: am35x: Add do_wfi routine for EMIF4 submodules Date: Thu, 12 Apr 2012 17:12:27 -0700 [thread overview] Message-ID: <20120413001227.GP31197@animalcreek.com> (raw) In-Reply-To: <alpine.DEB.2.00.1204111623110.29473@utopia.booyaka.com> On Wed, Apr 11, 2012 at 04:36:25PM -0600, Paul Walmsley wrote: > Hi > > just a few comments based on a quick look > > On Wed, 11 Apr 2012, Mark A. Greer wrote: > > diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S > > index 1f62f23..22aac4c 100644 > > --- a/arch/arm/mach-omap2/sleep34xx.S > > +++ b/arch/arm/mach-omap2/sleep34xx.S > > @@ -391,6 +401,52 @@ wait_dll_lock_counter: > > ENTRY(omap3_do_wfi_sz) > > .word . - omap3_do_wfi > > > > +/* > > + * Do WFI instruction (SDRC module has EMIF4 submodule) > > + * > > + * This code gets copied to internal SRAM and is accessible > > + * from both SDRAM and SRAM: > > + * - executed from SRAM for non-off modes (omap3_emif4_do_wfi_sram), > > + * - executed from SDRAM for OFF mode (omap3_emif4_do_wfi). > > + */ > > + .align 3 > > +ENTRY(omap3_emif4_do_wfi) > > + /* Put EMIF in self-refresh */ > > Hmm, I guess this comment isn't quite right; it just tells it to go to > self-refresh when it's "idle" ? (see below) > > > + ldr r4, pwr_mgmt_ctrl > > + ldr r5, [r4] > > + orr r5, r5, #0x200 > > Maybe use a macro here in place of the #0x200 to indicate what it's trying > to set... > > Do you happen to know what "idle" means in the context of SPRUGR0B Section > 9.2.3.4.5.1 "SDRAM Self-Refresh Mode" ? Is it referring to > target module-level idle, e.g. SIdleReq; or is it referring to > an internal definition of idle, e.g., something like "no reads or writes > from/to the SDRAM" ? Am wondering if this should just be set once during > EMIF initialization. I can't tell from tne manual. I assumed it meant the latter of the choices you listed. I just tested a kernel with it set up during EMIF init (value of 0x80000200) and removed the related bits from omap3_emif4_do_wfi() and it booted and returned from suspend-to-RAM just fine. So, it seems that it can be moved to EMIF init. [I swear I tested this some time ago and discovered that it was needed in omap3_emif4_do_wfi(). Oh well...] > > > + str r5, [r4] > > Might be good to do a readback after this to ensure that the store has > made it to the EMIF. > > Also, scanning SPRUGR0B, it looks like this setting is dependent partially > on the REG_PM_TIM field in the same register. Might be good to set > REG_PM_TIM during EMIF init to avoid any bootloader dependency. True but moot now since I'll move the code to EMIF init. > > + > > + dsb > > + dmb > > + > > + wfi > > + > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > Are these NOPs necessary? Is the number of NOPs dependent on CPU or bus > speed or some ARM parameter? After looking through the am35x errata, I don't think so. AFAICT, they are there to work around--from omap3535 errata (sprz278f.pdf)-- Advisory 3.1.1.75, "IVA2: CAM/SGX Dependencies (OMAP3530/25 only)". I don't see that or any other errata that requires a nop in the am35x errata so I'll get rid of them. [As I went through the errata again, I noticed talk about the device going into RET state. Sigh...] Mark --
next prev parent reply other threads:[~2012-04-13 0:12 UTC|newest] Thread overview: 126+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-04-11 19:05 arm: omap3: am35x: Powerdomain, EMIF4, etc. fixups Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 01/12] arm: omap3: Only access IVA if one exists Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 02/12] arm: omap3: Only sleep during cpu_idle if I/O wake-ups work Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:38 ` Paul Walmsley 2012-04-11 21:38 ` Paul Walmsley 2012-04-11 23:42 ` Jon Hunter 2012-04-11 23:42 ` Jon Hunter 2012-04-13 0:13 ` Mark A. Greer 2012-04-13 0:13 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 03/12] arm: omap3: Only sleep in cpuidle driver " Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:37 ` Paul Walmsley 2012-04-11 21:37 ` Paul Walmsley 2012-04-11 22:23 ` Mark A. Greer 2012-04-11 22:23 ` Mark A. Greer 2012-04-11 22:47 ` Paul Walmsley 2012-04-11 22:47 ` Paul Walmsley 2012-04-11 23:08 ` Mark A. Greer 2012-04-11 23:08 ` Mark A. Greer 2012-04-24 20:51 ` Mark A. Greer 2012-04-24 20:51 ` Mark A. Greer 2012-04-24 23:25 ` Mark A. Greer 2012-04-24 23:25 ` Mark A. Greer 2012-04-27 21:12 ` Kevin Hilman 2012-04-27 21:12 ` Kevin Hilman 2012-04-27 21:55 ` Mark A. Greer 2012-04-27 21:55 ` Mark A. Greer 2012-04-30 21:34 ` Mark A. Greer 2012-04-30 21:34 ` Mark A. Greer 2012-04-30 22:00 ` Kevin Hilman 2012-04-30 22:00 ` Kevin Hilman 2012-04-30 22:18 ` Mark A. Greer 2012-04-30 22:18 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 04/12] arm: omap3: am35x: Don't mark missing features as present Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 05/12] arm: omap3: am35x: Add PWROFF feature Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 22:46 ` Kevin Hilman 2012-04-11 22:46 ` Kevin Hilman 2012-04-11 23:11 ` Mark A. Greer 2012-04-11 23:11 ` Mark A. Greer 2012-04-24 4:36 ` Mark A. Greer 2012-04-24 4:36 ` Mark A. Greer 2012-04-27 21:07 ` Kevin Hilman 2012-04-27 21:07 ` Kevin Hilman 2012-04-30 22:08 ` Mark A. Greer 2012-04-30 22:08 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 06/12] arm: omap3: am35x: Add full PWRDM_POWER_INACTIVE support Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 20:56 ` Jean Pihet 2012-04-11 20:56 ` Jean Pihet 2012-04-11 21:08 ` Paul Walmsley 2012-04-11 21:08 ` Paul Walmsley 2012-04-11 21:14 ` Mark A. Greer 2012-04-11 21:14 ` Mark A. Greer 2012-04-11 21:15 ` Jean Pihet 2012-04-11 21:15 ` Jean Pihet 2012-04-11 21:12 ` Mark A. Greer 2012-04-11 21:12 ` Mark A. Greer 2012-04-11 22:17 ` Paul Walmsley 2012-04-11 22:17 ` Paul Walmsley 2012-04-11 19:05 ` [PATCH 07/12] arm: omap3: am35x: Set proper powerdomain states Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:53 ` Paul Walmsley 2012-04-11 21:53 ` Paul Walmsley 2012-04-11 22:40 ` Mark A. Greer 2012-04-11 22:40 ` Mark A. Greer 2012-04-12 0:24 ` Jon Hunter 2012-04-12 0:24 ` Jon Hunter 2012-04-12 2:19 ` Mark A. Greer 2012-04-12 2:19 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 08/12] arm: omap3: am35x: Fix clockdomain dependencies Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:44 ` Paul Walmsley 2012-04-11 21:44 ` Paul Walmsley 2012-04-11 21:55 ` Mark A. Greer 2012-04-11 21:55 ` Mark A. Greer 2012-04-11 22:04 ` Paul Walmsley 2012-04-11 22:04 ` Paul Walmsley 2012-04-11 22:49 ` Mark A. Greer 2012-04-11 22:49 ` Mark A. Greer 2012-04-11 23:49 ` Paul Walmsley 2012-04-11 23:49 ` Paul Walmsley 2012-04-12 2:23 ` Mark A. Greer 2012-04-12 2:23 ` Mark A. Greer 2012-04-12 2:29 ` Paul Walmsley 2012-04-12 2:29 ` Paul Walmsley 2012-04-12 23:00 ` Mark A. Greer 2012-04-12 23:00 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 09/12] arm: omap3: am35x: Add SDRC EMIF4 feature Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:29 ` Paul Walmsley 2012-04-11 21:29 ` Paul Walmsley 2012-04-11 22:50 ` Mark A. Greer 2012-04-11 22:50 ` Mark A. Greer 2012-04-11 22:56 ` Paul Walmsley 2012-04-11 22:56 ` Paul Walmsley 2012-04-11 23:23 ` Mark A. Greer 2012-04-11 23:23 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 10/12] arm: omap3: am35x: Add minimal EMIF4 support Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:31 ` Paul Walmsley 2012-04-11 21:31 ` Paul Walmsley 2012-04-11 23:22 ` Mark A. Greer 2012-04-11 23:22 ` Mark A. Greer 2012-04-11 19:05 ` [PATCH 11/12] arm: omap3: am35x: Add do_wfi routine for EMIF4 submodules Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 22:35 ` Kevin Hilman 2012-04-11 22:35 ` Kevin Hilman 2012-04-11 23:26 ` Mark A. Greer 2012-04-11 23:26 ` Mark A. Greer 2012-04-11 22:36 ` Paul Walmsley 2012-04-11 22:36 ` Paul Walmsley 2012-04-13 0:12 ` Mark A. Greer [this message] 2012-04-13 0:12 ` Mark A. Greer 2012-04-11 22:54 ` Paul Walmsley 2012-04-11 22:54 ` Paul Walmsley 2012-04-11 19:05 ` [PATCH 12/12] arm: omap3: am35x: Register davinci_mdio before davinci_emac Mark A. Greer 2012-04-11 19:05 ` Mark A. Greer 2012-04-11 21:24 ` Paul Walmsley 2012-04-11 21:24 ` Paul Walmsley 2012-04-11 22:00 ` Mark A. Greer 2012-04-11 22:00 ` Mark A. Greer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20120413001227.GP31197@animalcreek.com \ --to=mgreer@animalcreek.com \ --cc=khilman@ti.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=paul@pwsan.com \ --cc=ranjithl@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.