Hi Cyril,
On Mon, Apr 26, 2010 at 20:08:59, Chemparathy, Cyril wrote:
> Hi Sekhar,
>
> Thanks for the review. Please see my comments below...
>
> /* Block 1 - UART phys and virt already configured locally */
>
> [...]
> >> + /* Use davinci_uart_phys/virt if already configured */
> >> +10: mrc p15, 0, \rx, c1, c0
> >> + tst \rx, #1 @ MMU enabled?
> >> + ldreq \rx, =__virt_to_phys(davinci_uart_phys)
> >> + ldrne \rx, =davinci_uart_virt
> >> + ldr \rx, [\rx]
> >> + cmp \rx, #0 @ is port configured?
> >> + bne 99f @ already configured
>
> /* Block 2 - local config missing, copy DAVINCI_UART_INFO */
>
> [...]
> >> + /* Copy uart phys address from decompressor uart info */
> >> mrc p15, 0, \rx, c1, c0
> >> tst \rx, #1 @ MMU enabled?
> >> + ldreq \tmp, =__virt_to_phys(davinci_uart_phys)
> >> + ldrne \tmp, =davinci_uart_phys
> >> + ldreq \rx, =DAVINCI_UART_INFO
> >> + ldrne \rx, =__phys_to_virt(DAVINCI_UART_INFO)
> >> + ldr \rx, [\rx, #0]
> >> + str \rx, [\tmp]
>
> /* Block 3 - local config missing, copy DAVINCI_UART_INFO + 4 */
>
> [...]
> >> + mrc p15, 0, \rx, c1, c0
> >> + tst \rx, #1 @ MMU enabled?
> >> + ldreq \tmp, =__virt_to_phys(davinci_uart_virt)
> >> + ldrne \tmp, =davinci_uart_virt
> >> + ldreq \rx, =DAVINCI_UART_INFO
> >> + ldrne \rx, =__phys_to_virt(DAVINCI_UART_INFO)
> >> + ldr \rx, [\rx, #4]
> >> + str \rx, [\tmp]
> >
> > You should be able to combine the code chunks into
> > one by using ldr/str{cond} to configure either the
> > virtual or physical base depending on whether MMU
> > is enabled on not. No need to configure both at the
> > same time.
>
> The first time that the addruart macro is used, it copies the phys and
> virt base addresses from the DAVINCI_UART_INFO area.
Yes, this is what I was objecting to. addruart _may_ be invoked while mmu
is off or on. Not necessarily in both cases. So, you can decide to copy
from DAVINCI_UART_INFO "Just in time".
Even if you decide to copy both the first time, there should be no need
to re-run mrc since the condition flag isn't disturbed by any of the
instructions that follow.
> Ouch. I wish we had a few more usable registers to play with here,
> because that would have helped consolidate blocks 2 and 3 (above) into a
> single piece of streamlined code.
>
> Since the register allocation in early boot is pretty tight, I couldn't
> take the liberty of clobbering anything beyond the macro's parameter
> list. That said, if there is a register that we are free to clobber,
> I'd gladly use it to condense the code down a bit. Any thoughts?
At the least I would think the duplicate mrc (in blocks 2 and 3) can go.
Thanks,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
