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? > 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
