Kevin, The reworked version has been resent. Please use '[PATCH v3] OMAP3: clean up ASM idle code' which has the correct e-mail addresses for the sign-offs. Thanks, Jean On Tue, Dec 14, 2010 at 12:34 PM, Vishwanath Sripathy <vishwanath.bs@xxxxxx> wrote: > Jean, > >> -----Original Message----- >> From: Jean Pihet [mailto:jean.pihet@xxxxxxxxxxxxxx] >> Sent: Tuesday, December 14, 2010 2:53 PM >> To: Kevin Hilman; Vishwanath BS >> Cc: linux-omap@xxxxxxxxxxxxxxx; Jean Pihet >> Subject: Re: [PATCH] OMAP3: clean up ASM idle code >> >> Kevin, >> >> On Tue, Dec 14, 2010 at 5:12 AM, Kevin Hilman >> <khilman@xxxxxxxxxxxxxxxxxxx> wrote: >> > jean.pihet@xxxxxxxxxxxxxx writes: >> > >> >> From: Jean Pihet <j-pihet@xxxxxx> >> >> >> >> Clean up of the ASM code: >> >> - reorganized the code in logical sections: defines, API >> >> á áfunctions, internal functions and variables, >> >> - reworked and simplified the execution paths, for better >> >> á áreadability and to avoid duplication of code, >> >> - added comments on the entry and exit points, >> >> - reworked the existing comments for better readability, >> >> - reworked the code formating, alignment and white spaces, >> >> - added comments for the i443 erratum workarounds, >> >> - changed the hardcoded values in favor of existing macros >> >> á áfrom include files, >> >> - clean up of non used symbols. >> > >> > The 'lock_scratchpad_sem' code is also unused. áIIRC, you removed >> that >> > in an earlier version of the series. áAre you still planning to remove >> > that? maybe in a subsequent patch? >> >> I asked previously about it and the reply was that this code is >> needed: (quoting Vishwa's reply) "This is indeed used during DVFS when >> Core DPLL configuration is changed". Note: the DVFS code is not >> upstreamed yet. >> >> Vishwa, can you confirm? > lock_scratchpad_sem is needed when DVFS feature is supported. If you want > to add this implementation as part of DVFS feature, then also it's fine. > > Vishwa >> >> > That being said, pure whitespace changes and unused code removal >> should >> > probably be a separate patch. áIt's a great help to reviewers if >> > functional changes are separated from whitespace changes. áIt's a bit >> > tricky in this series as there's lots of code moving as well, so I'll >> > leave it up to your judgement on how/if to separate these out. >> >> There is no functional change at all. The code has been reorganized >> for better readability. >> I agree the patch is not easy to read but that is the way diff reports >> the changes. >> >> I do not see the point to provide separate patches for code move, >> white space clean-up, alignment fixes etc. >> >> >> Tested on OMAP3EVM and Beagleboard with full RET and OFF modes. >> > >> > In idle? áin suspend? áboth? áwas CPUidle enabled? >> > >> > FWIW, I tested on 3430-ES3.1/n900 with retention idle & suspend and >> off >> > idle & suspend with CPUidle enabled. >> Tested with cpuidle and suspend. I will update the description. >> >> > >> >> Heavily reworked from Vishwa's original patch. >> > >> > Here, it's more customary to ásay "Based on original patch from >> Vishwa" >> > and ensure the original author is CC'd (which you've done.) >> > >> > Other than that, this is a great cleanup, and makes this much more >> > readable. áThanks. >> > >> > Some other minor comments below. >> >> OK thanks for the review. I will post a new version asap. >> >> > >> >> Signed-off-by: Jean Pihet <j-pihet@xxxxxx> >> >> Cc: Vishwanath BS <vishwanath.bs@xxxxxx> >> >> --- >> >> Applies on top of Nishant's latest idle path errata fixes step 2, >> >> cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2 >> >> >> > >> > [...] >> > >> >> @@ -208,36 +172,153 @@ api_params: >> >> áENTRY(save_secure_ram_context_sz) >> >> á á á .word á . - save_secure_ram_context >> >> >> >> + >> >> +/* ====================== >> >> + * == Idle entry point == >> >> + * ====================== >> >> + */ >> > >> > Let's keep C multi-line coding style even for assembly. á Same goes > for >> > several other places below. >> Ok >> >> > >> >> á/* >> >> á * Forces OMAP into idle state >> >> á * >> >> - * omap34xx_suspend() - This bit of code just executes the WFI >> >> - * for normal idles. >> >> + * omap34xx_suspend() - This bit of code saves the CPU context if >> needed >> >> + * and executes the WFI instruction >> >> + * >> >> + * Note: This code get's copied to internal SRAM at boot. >> >> á * >> >> - * Note: This code get's copied to internal SRAM at boot. When the >> OMAP >> >> - * á áwakes up it continues execution at the point it went to sleep. >> >> + * Note: When the OMAP wakes up it continues at different >> execution points, >> >> + * ádepending on the low power mode (non-OFF vs OFF modes), >> >> + * ácf. 'Resume path for xxx mode' comments. >> >> á */ >> >> áENTRY(omap34xx_cpu_suspend) >> >> á á á stmfd á sp!, {r0-r12, lr} á á á á á á á @ save registers on > stack >> >> áloop: >> >> á á á /*b á á loop*/ á@Enable to debug by stepping through code >> > >> > While here, let's get rid of these infinite loop hacks for debugging > as >> > anyone debugging this code can add these back as needed. >> áOtherwise, >> > they clutter the code. á There are a few of theses throughout the >> > original code. >> Ok. Agree those are not used at all (even when doing heavy debugging). >> >> > [...] >> > >> >> @@ -250,9 +331,28 @@ loop: >> >> á á á nop >> >> á á á bl wait_sdrc_ok >> >> >> >> - á á ldmfd á sp!, {r0-r12, pc} á á á á á á á @ restore regs and > return >> >> +/* =================================== >> >> + * == Exit point from non-OFF modes == >> >> + * =================================== >> >> + */ >> >> + á á ldmfd á sp!, {r0-r12, pc} á á á @ restore regs and return >> >> + >> >> + >> >> +/* ============================== >> >> + * == Resume path for OFF mode == >> >> + * ============================== >> >> + */ >> > >> > I don't think this is quite correct. áI don't believe it starts >> > immediately here. áDoesn't it resume from DDR first, and then ájump >> > here. áA brief description of that process would help clarify that >> process. >> This is the restore point from OFF mode. The ROM code calls this >> directly, cf. the get_pointer* functions that are used to get the >> resume pointer. >> I will update the comment. >> >> >> +/* >> >> + * The restore_* functions are executed when back from WFI in >> OFF mode >> >> + * >> >> + * árestore_es3: applies to 34xx >= ES3.0 >> >> + * árestore_3630: applies to 36xx >> >> + * árestore: common code for 3xxx >> >> + */ >> >> árestore_es3: >> >> á á á /*b restore_es3*/ á á á á á á á @ Enable to debug restore code >> >> + >> >> á á á ldr á á r5, pm_prepwstst_core_p >> >> á á á ldr á á r4, [r5] >> >> á á á and á á r4, r4, #0x3 >> >> @@ -282,18 +382,20 @@ restore_3630: >> >> á á á ldr á á r1, control_mem_rta >> >> á á á mov á á r2, #OMAP36XX_RTA_DISABLE >> >> á á á str á á r2, [r1] >> >> - á á /* Fall thru for the remaining logic */ >> >> + >> >> + á á /* Fall thru to common code for the remaining logic */ >> >> + >> >> árestore: >> >> á á á /* b restore*/ á@ Enable to debug restore code >> >> - á á á á/* Check what was the reason for mpu reset and store the >> reason in r9*/ >> >> - á á á á/* 1 - Only L1 and logic lost */ >> >> - á á á á/* 2 - Only L2 lost - In this case, we wont be here */ >> >> - á á á á/* 3 - Both L1 and L2 lost */ >> >> + á á /* Check what was the reason for mpu reset and store the >> reason in r9*/ >> >> + á á /* 1 - Only L1 and logic lost */ >> >> + á á /* 2 - Only L2 lost - In this case, we wont be here */ >> >> + á á /* 3 - Both L1 and L2 lost */ >> >> á á á ldr á á r1, pm_pwstctrl_mpu >> >> á á á ldr á á r2, [r1] >> >> á á á and á á r2, r2, #0x3 >> >> á á á cmp á á r2, #0x0 á á á á@ Check if target power state was OFF > or >> RET >> >> - á á á ámoveq á r9, #0x3 á á á á@ MPU OFF => L1 and L2 lost >> >> + á á moveq á r9, #0x3 á á á á@ MPU OFF => L1 and L2 lost >> >> á á á movne á r9, #0x1 á á á á@ Only L1 and L2 lost => avoid L2 >> invalidation >> >> á á á bne á á logic_l1_restore >> >> >> >> @@ -309,23 +411,23 @@ skipl2dis: >> >> á á á and á á r1, #0x700 >> >> á á á cmp á á r1, #0x300 >> >> á á á beq á á l2_inv_gp >> >> - á á mov á á r0, #40 á á á á @ set service ID for PPA >> >> - á á mov á á r12, r0 á á á á @ copy secure Service ID in r12 >> >> - á á mov á á r1, #0 á á á á á@ set task id for ROM code in r1 >> >> - á á mov á á r2, #4 á á á á á@ set some flags in r2, r6 >> >> + á á mov á á r0, #40 á á á á á á á á @ set service ID for PPA >> >> + á á mov á á r12, r0 á á á á á á á á @ copy secure Service ID in r12 >> >> + á á mov á á r1, #0 á á á á á á á á á@ set task id for ROM code in > r1 >> >> + á á mov á á r2, #4 á á á á á á á á á@ set some flags in r2, r6 >> > >> > This is the type of change that should normally be in a separate > patch, >> > as it seems to be pure whitespace change. >> > >> > >> > Kevin >> > >> >> Jean > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
