Patchwork Coreboot for AMD Fusion family 14h: ASRock E350M1

Peter wrote:

]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?

I do not know of a way to recover the original filename of the
extracted video BIOS. After booting the factory BIOS, I saved
physical memory range C0000-CFFFF to a file with a made up name.


> +++ 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?
Thanks for pointing this out, I changed it.
In the revised patch, I also changed it in mptable.c


> +++ 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.
Agree, I revised it.

> -#define CONFIG_VGA_BIOS_ID "1002,9804"
> +#define CONFIG_VGA_BIOS_ID "1002,9802"

]Why have this comment at all? Better just remove it.
Good point, I removed 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." ..
Good idea.


> @@ -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")
Thanks.


> +++ 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?
Oops, I fixed it.


//Peter

Scott Duplichan wrote:
> ]> 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?
> 
> I do not know of a way to recover the original filename of the
> extracted video BIOS. After booting the factory BIOS, I saved
> physical memory range C0000-CFFFF to a file with a made up name.

Ahh! And that worked? It doesn't always. The original filename would
be discovered by using bios_extract or another similar tool on the
factory firmware image. That also doesn't work always, but it's worth
a try.


> > +++ 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?
> Thanks for pointing this out, I changed it.
> In the revised patch, I also changed it in mptable.c

Nice catch!


> +++ src/mainboard/asrock/e350m1/mainboard.c	(working copy)
..
> @@ -110,6 +110,6 @@
>  	return 0;
>  }
>  struct chip_operations mainboard_ops = {
> -	CHIP_NAME("AMD PERSIMMON Mainboard")
> -	.enable_dev = persimmon_enable,
> +    CHIP_NAME(CONFIG_MAINBOARD_VENDOR " " CONFIG_MAINBOARD_PART_NUMBER " Mainboard")
> +	.enable_dev = e350m1_enable,

Some whitespace on the CHIP_NAME line seems off. After that and if
you find something out about the VGA BIOS filename it's

Acked-by: Peter Stuge <peter@stuge.se>

Peter wrote:

]Scott Duplichan wrote:
]> ]> 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?
]> 
]> I do not know of a way to recover the original filename of the
]> extracted video BIOS. After booting the factory BIOS, I saved
]> physical memory range C0000-CFFFF to a file with a made up name.

]Ahh! And that worked? It doesn't always. The original filename would
]be discovered by using bios_extract or another similar tool on the
]factory firmware image. That also doesn't work always, but it's worth
]a try.

I tried AMIBCP.exe. That is the only tool I have for AMI Aptio UEFI BIOS.
It displays lots of strings, but no filenames. Wonderful UEFI uses GUIDs
internally for option rom tracking.


]> > +++ 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?
]> Thanks for pointing this out, I changed it.
]> In the revised patch, I also changed it in mptable.c
]
]Nice catch!


]> +++ src/mainboard/asrock/e350m1/mainboard.c	(working copy)
]..
]> @@ -110,6 +110,6 @@
]>  	return 0;
]>  }
]>  struct chip_operations mainboard_ops = {
]> -	CHIP_NAME("AMD PERSIMMON Mainboard")
]> -	.enable_dev = persimmon_enable,
]> +    CHIP_NAME(CONFIG_MAINBOARD_VENDOR " " CONFIG_MAINBOARD_PART_NUMBER " Mainboard")
]> +	.enable_dev = e350m1_enable,
]
]Some whitespace on the CHIP_NAME line seems off. After that and if
]you find something out about the VGA BIOS filename it's

Oops, got it.

Acked-by: Peter Stuge <peter@stuge.se>

On 02/24/11 13:48, Scott Duplichan wrote:
> Peter wrote:
>
> ]Scott Duplichan wrote:
> ]>  ]>  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?
> ]>
> ]>  I do not know of a way to recover the original filename of the
> ]>  extracted video BIOS. After booting the factory BIOS, I saved
> ]>  physical memory range C0000-CFFFF to a file with a made up name.
>
> ]Ahh! And that worked? It doesn't always. The original filename would
> ]be discovered by using bios_extract or another similar tool on the
> ]factory firmware image. That also doesn't work always, but it's worth
> ]a try.
>
> I tried AMIBCP.exe. That is the only tool I have for AMI Aptio UEFI BIOS.
> It displays lots of strings, but no filenames. Wonderful UEFI uses GUIDs
> internally for option rom tracking.
>
>
this EFI bios is using the CSM module so it can use the traditional vBIOS.
are there any plans to support this new GOP model they (the EFI guys) 
are trying to push?
> ]>  >  +++ 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?
> ]>  Thanks for pointing this out, I changed it.
> ]>  In the revised patch, I also changed it in mptable.c
> ]
> ]Nice catch!
>
>
> ]>  +++ src/mainboard/asrock/e350m1/mainboard.c	(working copy)
> ]..
> ]>  @@ -110,6 +110,6 @@
> ]>   	return 0;
> ]>   }
> ]>   struct chip_operations mainboard_ops = {
> ]>  -	CHIP_NAME("AMD PERSIMMON Mainboard")
> ]>  -	.enable_dev = persimmon_enable,
> ]>  +    CHIP_NAME(CONFIG_MAINBOARD_VENDOR " " CONFIG_MAINBOARD_PART_NUMBER " Mainboard")
> ]>  +	.enable_dev = e350m1_enable,
> ]
> ]Some whitespace on the CHIP_NAME line seems off. After that and if
> ]you find something out about the VGA BIOS filename it's
>
> Oops, got it.
>
> Acked-by: Peter Stuge<peter@stuge.se>
>
>
>

Anish Patel wrote:

]this EFI bios is using the CSM module so it can use the traditional vBIOS.
]are there any plans to support this new GOP model they (the EFI guys) 
]are trying to push?

I like the idea of replacing a 16-bit binary module with 32-bit
source code. But I think the use of EFI GOP is still quite limited.
All of the UEFI BIOS I have seen from AMI, Insyde, and Phoenix still
use CSM to execute the old legacy 16-bit video option rom. The only
EFI BIOS I know of that executes video code natively is Apple. Native
video code execution probably contributes to Apple's fast boot time.
The problem for us is getting the needed source code from the video
manufacturer.

Thanks,
Scott

On 03/10/11 13:36, Scott Duplichan wrote:
> Anish Patel wrote:
>
> ]this EFI bios is using the CSM module so it can use the traditional vBIOS.
> ]are there any plans to support this new GOP model they (the EFI guys)
> ]are trying to push?
>
> I like the idea of replacing a 16-bit binary module with 32-bit
> source code. But I think the use of EFI GOP is still quite limited.
> All of the UEFI BIOS I have seen from AMI, Insyde, and Phoenix still
> use CSM to execute the old legacy 16-bit video option rom. The only
> EFI BIOS I know of that executes video code natively is Apple. Native
> video code execution probably contributes to Apple's fast boot time.
> The problem for us is getting the needed source code from the video
> manufacturer.
>
> Thanks,
> Scott
>
>
>
>
Well Intel lets you get to their GOP module with IMGD/EMGD for certain 
products.  They also have this EPOG thingy for their BLDK thing they 
just built.
which apparently is also going to be released for their other procs too.

Anish Patel wrote:
>> ]are there any plans to support this new GOP model they (the EFI
>> ]guys) are trying to push?
>>
>> I like the idea of replacing a 16-bit binary module with 32-bit
>> source code.
..
> Well Intel lets you get to their GOP module with IMGD/EMGD for
> certain products.  They also have this EPOG thingy for their BLDK
> thing they just built.

Great. Please send a patch that you have made sure is legal.


> which apparently is also going to be released for their other
> procs too.

Great. Please send patches for those too!


//Peter