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
