Re: [coreboot] [PATCH] Add CPP logic to VIA CAR init code.

[Warren Turkal]

I think I see what is happening from the following explanation.
http://sourceware.org/binutils/docs-2.20/as/Symbol-Value.html#Symbol-Value

Kinda odd that the the symbol's value is only set to 0 after the
operators are applied to the symbol. I think this is why the file both
assembles without issue and the separate orl instruction works where
the "$(REAL_XIP_ROM_BASE | MTRR_TYPE_WRBACK)" does not work. Does this
sound correct?

Thanks,
wt

On Sun, Oct 3, 2010 at 9:54 AM, Warren Turkal  wrote:
> I see that is the change that fixed it. However, I'm still not sure I
> understand the fix completely. When CONFIG_TINY_BOOTBLOCK is defined
> and AUTO_XIP_ROM_BASE is not defined, we get the following line (line
> number 159 or so) in build/mainboard/via/vt8454c/crt0.s:
>  movl $AUTO_XIP_ROM_BASE, %eax
>
> How does that assemble without an error?
>
> Is the AUTO_XIP_ROM_BASE set to some value (presumably 0) by the
> assembler for some reason that I don't see? For reference, here is the
> equivalent line from crt0.disasm:
> 100 0115 B8000000 >---->-------movl>---$REAL_XIP_ROM_BASE, %eax
>
> Thanks,
> wt
>
> On Sun, Oct 3, 2010 at 9:31 AM, Warren Turkal  wrote:
> > Is this what your orl change fixed?
> >
> > wt
> >
> > On Sun, Oct 3, 2010 at 6:34 AM, Stefan Reinauer
> >  wrote:
> > > On 10/3/10 11:24 AM, Warren Turkal wrote:
> > > > *ping* I really need an ack or nack on this.
> > >
> > > NACK.. Still the wrong way, it just blindly comments out an arbitrary
> > > piece of code.
> > >
> > > However, the code has been fixed already in the repo.
> > >
> > > Stefan
> > > > Thanks,
> > > > wt
> > > >
> > > > On Sat, Oct 2, 2010 at 1:59 AM, Warren Turkal  wrote:
> > > > > VIA/AMD experts,
> > > > >
> > > > > This patch get's the via/vt8454c back to building. However, I am not
> > > > > sure if the code that is being #ifdef'ed out will actually ever be used
> > > > > on a via platform. The code comes straight from the amd CAR
> > > > > implementation. A couple of questions are raised by this:
> > > > > Should we just delete the code from the via file instead of this
> > > > >
> > > > > patch?
> > > > > 2) Should the amd and via CAR code be integrated into one file? Maybe
> > > > >   just portions of the files if not the whole files?
> > > > >
> > > > > Also, another happy side effect of this change is that all the c7 boards
> > > > > seem to build with tiny bootblocks. Would everyone be ok with my making
> > > > > that change?
> > > > >
> > > > > Thanks,
> > > > > wt
> > > > > 8<----------------------------------------------------------------------
> > > > > The execute-in-place (XIP) config options need to be set in order to get
> > > > > XIP functionality, so it needs to be excluded when those settings are
> > > > > not set.
> > > > > Signed-off-by: Warren Turkal
> > > > >
> > > > > src/cpu/via/car/cache_as_ram.inc |    4 ++++
> > > > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/src/cpu/via/car/cache_as_ram.inc b/src/cpu/via/car/cache_as_ram.inc
> > > > > index be00fe3..d18ac3a 100644
> > > > > --- a/src/cpu/via/car/cache_as_ram.inc
> > > > > +++ b/src/cpu/via/car/cache_as_ram.inc
> > > > > @@ -85,6 +85,8 @@ clear_fixed_var_mtrr_out:
> > > > >        movl    $(~(CacheSize - 1) | 0x800), %eax
> > > > >        wrmsr
> > > > >
> > > > > +#if defined(CONFIG_XIP_ROM_SIZE) && defined(CONFIG_XIP_ROM_BASE)
> > > > >
> > > > > #if defined(CONFIG_TINY_BOOTBLOCK) && CONFIG_TINY_BOOTBLOCK
> > > > >  #define REAL_XIP_ROM_BASE AUTO_XIP_ROM_BASE
> > > > >  #else
> > > > > @@ -106,6 +108,8 @@ clear_fixed_var_mtrr_out:
> > > > >        movl    $(~(CONFIG_XIP_ROM_SIZE - 1) | 0x800), %eax
> > > > >        wrmsr
> > > > >
> > > > > +#endif /* CONFIG_XIP_ROM_SIZE && CONFIG_XIP_ROM_BASE */
> > > > >
> > > > > /* Set the default memory type and enable fixed and variable MTRRs. */
> > > > >        /* TODO: Or also enable fixed MTRRs? Bug in the code? */
> > > > >        movl    $MTRRdefType_MSR, %ecx
> > > > > --
> > > > > 1.7.1