Re: [coreboot] Adding CAR support to v2

[Carl-Daniel Hailfinger]

On 15.06.2009 17:45, ron minnich wrote:
> This is the beginning of CAR support for v2 for qemu. It is done in
> such a way that few changes are required and, more the the point,
> romcc-based qemu targets keep working and remain the default for now.

THIS IS SOOOO AWESOME!
> It builds on but substantially improves the v3 model, a most important
> point being that there are *no* included code files in .c or .S.

I like this change and should forward port it to v3. That would ease
copying of the CAR code for other architectures.
> The .S code is from v3, however.

Excellent.
> This is working and boots to linux in both car and non-car versions.
>
> Note that I use the name 'rom.c' instead of the awkward 'cache_as_ram_auto.c'.
>
> Some work remains. We need to have a standard rom main as we do in v3
> (we call it stage0/1 in v3). But this is a good start IMHO.

Yes. Merge now, rename later.
> comments from the patch:
> Note that the current non-CAR implementation is the default and
> continues to work (tested FILO boot to Linux
> on both CAR and non-CAR).
>
> Index: src/mainboard/emulation/qemu-x86/Config.lb
> Change this to be sensitive to USE_DCACHE_RAM. All settings etc. that
> depend on this variable are grouped
> in one if, and the other parts (romcc etc.) are in the else. This
> change is a model of how we should be able to do
> other motherboards.

I think some include refactoring is due for those files anyway, but I
can handle your changes manually. (My statement about this is to be
considered as "comment", not "review".)
> Index: src/mainboard/emulation/qemu-x86/Options.lb
> add needed options.
>
> Index: src/mainboard/emulation/qemu-x86/failover.c
> remove code inclusion from this not-yet-used file.
>
> Index: src/mainboard/emulation/qemu-x86/rom.c
> This is the entry point for the rom-based code. Called stage1.c in v3.

It would be great if rom.c could eventually become a copy of stage1.c in v3.
> Index: src/lib/Config.lb
> change initobject to a .o from a .c; this fixed a build problem.
>
> Index: src/pc80/serial.c
> make uart_init non-static.
>
> Index: src/pc80/Config.lb
> add initobject
>
> Index: src/arch/i386/init/entry.S
> Entry point. Unify a bunch of files that were fiddly lttle includes. From v3.
>
> Index: src/arch/i386/init/ldscript.ld
> new file. The goal is to hang all init changes for CAR here, to
> minimize other changes to any
> other ldscript. Besides, putting this in init makes sense; entry and
> car are manage init.
>
> Index: src/arch/i386/init/car.S
> generic i386 car code from v3.
>
> Index: src/arch/i386/init/ldscript_fallback_cbfs.lb
> Fix what looks like a bug: this was not including the init.text section.
>
> Index: targets/emulation/qemu-x86/Config.lb
> push up the console loglevel. qemu is for debugging so we might as well
> get all the debugging we can.
>
> Index: targets/emulation/qemu-x86/Config-car.lb
> For CAR bullds.
>
> Signed-off-by: Ronald G. minnich

Unless I misread something, you could kill everything except an empty
main() {} in src/mainboard/emulation/qemu-x86/failover.c. Feel free to
postpone that, though.
> --- src/mainboard/emulation/qemu-x86/rom.c	(revision 0)
> +++ src/mainboard/emulation/qemu-x86/rom.c	(revision 0)
> @@ -0,0 +1,30 @@
> +void main(void)
> +{
> int i;
> void uart_init(void);
> void (*v)();

What is v()? Please use a name which has a chance to be understood.
> outb(5, 0x80);
> uart_init();
> v = cbfs_load_stage("fallback/coreboot_ram");
> v();

Same here. Searching for one-byte identifiers is hard.
> +}

All of the issues I brought up can be fixed in a followup patch. I want
this in ASAP.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel