Scott Duplichan wrote:
> I accidentally did the 'svn cp' step interactively instead of by
> patch. The attached patch completes the work of converting AMD
> Persimmon into ASRock E350M1.
Good stuff. Some simple comments, then I'll ack.
> A video option rom needs to be added to support the built-in uma
> graphics.
Does the default filename match what was extracted from factory EFI?
> +++ src/mainboard/asrock/e350m1/dsdt.asl (working copy)
> @@ -23,7 +23,7 @@
> "DSDT", /* Signature */
> 0x02, /* DSDT Revision, needs to be 2 for 64bit */
> "AMD ", /* OEMID */
> - "PERSIMMO", /* TABLE ID */
> + "E350M1 ", /* TABLE ID */
Also change AMD?
> +++ src/mainboard/asrock/e350m1/Kconfig (working copy)
..
> @@ -64,7 +64,7 @@
>
> config MAINBOARD_PART_NUMBER
> string
> - default "Persimmon"
> + default "e350m1"
I think this should be uppercase.
> -#define CONFIG_VGA_BIOS_ID "1002,9804"
> +#define CONFIG_VGA_BIOS_ID "1002,9802"
Why have this comment at all? Better just remove it.
> +++ src/mainboard/asrock/e350m1/mainboard.c (working copy)
..
> - printk(BIOS_INFO, "Mainboard Persimmon Enable. dev=0x%p\n", dev);
> + printk(BIOS_INFO, "Mainboard E350M1 Enable. dev=0x%p\n", dev);
Please change to: "Mainboard " CONFIG_MAINBOARD_PART_NUMBER " Enable." ..
> @@ -110,6 +110,6 @@
> return 0;
> }
> struct chip_operations mainboard_ops = {
> - CHIP_NAME("AMD PERSIMMON Mainboard")
> - .enable_dev = persimmon_enable,
> + CHIP_NAME("ASRock E350M1 Mainboard")
> + .enable_dev = e350m1_enable,
Same here.
CHIP_NAME(CONFIG_MAINBOARD_VENDOR " " CONFIG_MAINBOARD_PART_NUMBER " Mainboard")
> +++ src/mainboard/asrock/e350m1/romstage.c (working copy)
..
> @@ -52,7 +52,7 @@
> sb_poweron_init();
>
> post_code(0x31);
> - f81865f_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
> + w83627hf_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
Whitespace?
//Peter
--
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
