* Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> [080819 20:03]:
> On Fri, Jun 06, 2008 at 07:12:28PM -0700, Tony Lindgren wrote:
> > Some register offsets are different for 242x and 243x. This
> > will allow compiling sleep code for both chips into the same
> > kernel.
> >
> > Note that some PM patches are still missing. The PM patches will
> > be added later on once the base files are in sync with linux-omap
> > tree.
>
> Please use git diff -M, since it makes the changes across renames more
> obvious.
OK
> > +ENTRY(omap242x_idle_loop_suspend)
> > + stmfd sp!, {r0, lr} @ save registers on stack
> > + mov r0, #0x0 @ clear for mrc call
> > + mcr p15, 0, r0, c7, c0, 4 @ wait for interrupt
> > + ldmfd sp!, {r0, pc} @ restore regs and return
>
> What's been lost because of the lack of git diff -M here is the real
> change:
>
> -ENTRY(omap24xx_idle_loop_suspend)
> +ENTRY(omap242x_idle_loop_suspend)
> stmfd sp!, {r0, lr} @ save registers on stack
> - mov r0, #0 @ clear for mcr setup
> + mov r0, #0x0 @ clear for mrc call
> mcr p15, 0, r0, c7, c0, 4 @ wait for interrupt
> ldmfd sp!, {r0, pc} @ restore regs and return
>
> which makes the problem stand out. That change of the 'mov' line
> along with the comment is completely bogus. In fact, the change to
> the comment is clearly wrong. The same applies to sleep243x.S
Will remove.
> Realistically, the only real difference between the two files are
> these lines:
>
> omap2_ocs_sdrc_power:
> - .word OMAP242X_SDRC_REGADDR(SDRC_POWER)
> + .word OMAP243X_SDRC_REGADDR(SDRC_POWER)
> A_SDRC0:
> .word A_SDRC0_V
> omap2_ocs_sdrc_dlla_ctrl:
> - .word OMAP242X_SDRC_REGADDR(SDRC_DLLA_CTRL)
> + .word OMAP243X_SDRC_REGADDR(SDRC_DLLA_CTRL)
>
> so is doubling the size of this code really justified?
Yes duplication is a problem. We had code that was dynamically
rewriting the addresses but it was not very easy to follow and
hard to debug. This code is only compiled in twice if both 242x
and 243x are both selected though.
> Looking harder at this code:
>
> ENTRY(omap242x_cpu_suspend)
> stmfd sp!, {r0 - r12, lr} @ save registers on stack
> ...
> mov r5, #0x2000 @ set delay (DPLL relock + DLL relock)
> ...
> nop
> mcr p15, 0, r2, c7, c0, 4 @ wait for interrupt
> nop
> loop:
> subs r5, r5, #0x1 @ awake, wait just a bit
> bne loop
>
> ldmfd sp!, {r0 - r12, pc} @ restore regs and return
>
> it's clear that registers are preserved across the wait-for-interrupt
> instruction, so I'm not sure why saving all the registers is really
> necessary, but that's only a side point to my main point, which is...
>
> ... that you could pass the addresses of these registers into the
> function, either directly:
>
> void omap24xx_cpu_suspend(u32 dll_ctrl, u32 cpu_revision,
> void __iomem *sdrc_pwr,
> void __iomem *sdrc_dlla_ctrl);
>
> or via a structure, and thereby avoid this duplication.
The structure would have to be also in SRAM then. I guess in this case
there are only two addresses, so passing them via into the function
should be enough. It's unlikely that this code changes any further
to need more addresses.
Will repost.
Tony
--
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
