[coreboot] [patch][v2][2/4] Family 10

[Marc Jones]

Stefan Reinauer wrote:
> * Marc Jones <Marc.Jones at amd.com> [080416 19:19]:
>> Update the FAM10 microcode to current versions. In addition, AP microcode is 
>> now updated in early initialization to support errata settings that require it.
>>
>>
>> Signed-off-by: Marc Jones (marc.jones at amd.com)
>  
> Acked-by: Stefan Reinauer <stepan at coresystems.de>
> 

> See a few comments below. They're mostly future ideas.
> 
> Stefan
> 
>> +		printk_err("microcode: Not updated! Fix microcode_updates[] \n");
>>  		return 0;
> 
> Is this really an error condition? Or rather a warning? How serious is
> it?
> 

I think it should always be reported which is why I made it an _err. It 
will continue to boot but you could have any number of instabilities due 
to errata (publicly documented or not).


>> Index: coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h
>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h	2008-04-16 09:49:40.000000000 -0600
>> @@ -0,0 +1,164 @@
>> +/*
>> + ============================================================
>> + (c) Advanced Micro Devices, Inc., 2004-2008
>> +
>> + The  enclosed microcode  is intended  to be  used  with AMD
>> + Microprocessors.  You  may   copy,  view  and  install  the
> 
> I know this is an old license and we have been accepting this before.
> We are fine with linking this into our code, are we?
> 
> Would it make sense for v3 to pack this into a lar? and load it
> externally?
> 
> 

That would depend on how we incorporate AGESA in v3, but it is a good 
idea since this is a binary blob.


>> @@ -455,6 +450,7 @@
>>  	{ X86_VENDOR_AMD, 0x100f21 },
>>  	{ X86_VENDOR_AMD, 0x100f2A },
>>  	{ X86_VENDOR_AMD, 0x100f22 },
>> +	{ X86_VENDOR_AMD, 0x100f23 },
>>  	{ 0, 0 },
>>  };
> 
> I want to suggest here that the list of CPUs is changed to only use
> family and model but not stepping. AMD CPUs are not broken enough that
> we need different cpu init code per stepping. So it is enough to write
> 
> { X86_VENDOR_AMD, 0x100f20 },
> 
> and it will match 1, a, 2, 3, ....
> 
> I am not a fan of artificially restricting the number of machines that
> are correctly initialized just because a developer didnt have it in
> her/his hands yet.
> 


I have mixed feelings. I think it is important to have all the stepping 
errata in place. This is important for stability and performance. I 
agree that AMD aren't broken enough that things will catch on fire 
without the errata but I would prefer coreboot have the same (or better) 
quality as the IBVs.

Marc


-- 
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors