Patchwork Add CPP logic to VIA CAR init code.

login
register
about
Submitter Warren Turkal
Date 2010-10-02 08:59:32
Message ID <1286009972-6589-1-git-send-email-wt@penguintechs.org>
Download mbox | patch
Permalink /patch/2024/
State Rejected
Headers show

Comments

Warren Turkal - 2010-10-02 08:59:32
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:
1) 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 <wt@penguintechs.org>
---
 src/cpu/via/car/cache_as_ram.inc |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Warren Turkal - 2010-10-03 09:24:15
*ping* I really need an ack or nack on this.

Thanks,
wt

On Sat, Oct 2, 2010 at 1:59 AM, Warren Turkal <wt@penguintechs.org> 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:
> 1) 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 <wt@penguintechs.org>
> ---
>  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
>
>
Stefan Reinauer - 2010-10-03 13:34:45
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 <wt@penguintechs.org> 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:
>> 1) 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 <wt@penguintechs.org>
>> ---
>>  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
>>
>>
Warren Turkal - 2010-10-03 16:31:51
Is this what your orl change fixed?

wt

On Sun, Oct 3, 2010 at 6:34 AM, Stefan Reinauer
<stefan.reinauer@coresystems.de> 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 <wt@penguintechs.org> 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:
>>> 1) 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 <wt@penguintechs.org>
>>> ---
>>>  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
>>>
>>>
>
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
Warren Turkal - 2010-10-03 16:54:37
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 <wt@penguintechs.org> wrote:
> Is this what your orl change fixed?
>
> wt
>
> On Sun, Oct 3, 2010 at 6:34 AM, Stefan Reinauer
> <stefan.reinauer@coresystems.de> 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 <wt@penguintechs.org> 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:
>>>> 1) 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 <wt@penguintechs.org>
>>>> ---
>>>>  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
>>>>
>>>>
>>
>>
>> --
>> coreboot mailing list: coreboot@coreboot.org
>> http://www.coreboot.org/mailman/listinfo/coreboot
>>
>
Warren Turkal - 2010-10-03 17:17:42
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 <wt@penguintechs.org> 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 <wt@penguintechs.org> wrote:
>> Is this what your orl change fixed?
>>
>> wt
>>
>> On Sun, Oct 3, 2010 at 6:34 AM, Stefan Reinauer
>> <stefan.reinauer@coresystems.de> 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 <wt@penguintechs.org> 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:
>>>>> 1) 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 <wt@penguintechs.org>
>>>>> ---
>>>>>  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
>>>>>
>>>>>
>>>
>>>
>>> --
>>> coreboot mailing list: coreboot@coreboot.org
>>> http://www.coreboot.org/mailman/listinfo/coreboot
>>>
>>
>
Patrick Georgi - 2010-10-03 17:23:08
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 03.10.2010 18:54, schrieb Warren Turkal:
> 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
This becomes a symbol in the .o file that is (must be) resolved on link
time. That's why it must be provided verbatim (without "|anything") in
the code.

What we _could_ do is to define REAL_XIP_ROM_BASE_WITH_FLAGS_SET (or
sth. to that effect) and run with that. It would eliminate the "orl"
line. I hesitated to do so because I wasn't sure if all systems use the
same flags (I didn't look if that's the case now), and didn't want to
provide lots of REAL_XIP_ROM_BASE_* for all combinations.


Patrick
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyou/wACgkQfp0gE8eLOvS2KACbB1IklshTAYS5FrNbwm9iV+XW
WsgAn3apJhOupF7ShPuvIE26CnM5QZi7
=vp5n
-----END PGP SIGNATURE-----

Patch

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