RE: [PATCH] OMAP3: clean up ASM idle code

[Vishwanath Sripathy <vishwanath.bs@ti.com>]

Jean,

> -----Original Message-----
> From: Jean Pihet [mailto:jean.pihet@newoldbits.com]
> Sent: Tuesday, December 14, 2010 2:53 PM
> To: Kevin Hilman; Vishwanath BS
> Cc: linux-omap@vger.kernel.org; Jean Pihet
> Subject: Re: [PATCH] OMAP3: clean up ASM idle code
>
> Kevin,
>
> On Tue, Dec 14, 2010 at 5:12 AM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
> > jean.pihet@newoldbits.com writes:
> >
> >> From: Jean Pihet <j-pihet@ti.com>
> >>
> >> 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@ti.com>
> >> Cc: Vishwanath BS <vishwanath.bs@ti.com>
> >> ---
> >> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html