Fam10 FIDVID in SVI : disabling microcode update

[Xavi Drudis Ferran]

Hello.

I finally splitted my changes into some 25 small patches (some 8-10 quite trivial refactorings, but I hesitate to use "trivial"  in signing off) .
 I compiled each for my board and tested it , and I hoped to find which of them fixed my fidvid issues. I have to add some commit comments
and I hope to send them tonight or tomorrow evening.

But after applying all of them it still didn't work. So I looked again and discovered in my previous tests I had commented the 
calls to update_microcode(...) . In the test of the smaller patches I had mistakenly commented only the call for the BSP 
but not for the APs. I tried with both calls commented and it works, then I thought, "ok, it must work with both uncommented, 
having diferent microcode versions in different cores is just weird". But with the calls uncommented it won't work (it hangs in 
ram initialization, like without most of the patches, or with microcode updated only in some cores, and then it depends, sometines
it works).

So in order for my board to pass fidvid and ram init I have to disable microcode updating. I haven't tried any new microcode in 
Agesa code submitted this last sunday, because, after all, it does not have source, so for all I care I'll be happier  
if it works without it.

I guess the reason could be either of: 
- coreboot is applying the wrong microcode version, and some logic selecting the microcode can be fixed
- the version of the microcode itself is faulty, and may be fixed in a later version (maybe already under /vendorcode/...)
- the way to update the microcode does not work in my CPU

But I don't feel like fixing any.

Yet I don't think I should send a patch just commenting those calls to update_microcode, should I ?

Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?

Should I just send the patches for fidvid and keep the update_microcode commented for myself only, 
even if this still does not fix the issues for my setup, so that someone else may investigate what's wrong 
with the microcode ?

Are my patches moot because fam10 fidvid is going to be replaced with something from Agesa ?

Finally, I've never used abuild, but taking into account I'm touching

src/cpu/amd/model_10xxx/fidvid.c
src/cpu/amd/model_10xxx/defaults.h
src/cpu/amd/model_10xxx/init_cpus.c
src/northbridge/amd/amdmct/wrappers/mcti_d.c
src/northbridge/amd/amdmct/amddefs.h
src/northbridge/amd/amdfam10/Kconfig
src/northbridge/amd/amdfam10/raminit_amdmct.c
src/northbridge/amd/amdht/AsPsDefs.h

should I try to abuild before sending my patches ?

thanks.

[PATCH] Fam10 FIDVID in SVI 01/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 01/25

[Georgi, Patrick]

Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
> see patch

Any opinion on these patches? Patch 1-8 seem to be refactorings only,
and splitting functions into smaller logical units looks good to me, but
I'd like to hear from someone deeper in the AMD code.

Patrick

[PATCH] Fam10 FIDVID in SVI 01/25

[Marc Jones]

On Thu, Feb 24, 2011 at 6:31 AM, Georgi, Patrick
 wrote:
> Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
> > see patch
>
> Any opinion on these patches? Patch 1-8 seem to be refactorings only,
> and splitting functions into smaller logical units looks good to me, but
> I'd like to hear from someone deeper in the AMD code.
>
> Patrick

Yes, These patches are improvements in the SVI code, and I think we
need to pull them in. I just haven't had a chance to test them or
review them yet. I think that all new Fam10 systems are SVI now. If
anyone else has time to try it, please do.

Marc

[PATCH] Fam10 FIDVID in SVI 01/25

[xdrudis]

On Thu, Feb 24, 2011 at 02:31:29PM +0100, Georgi, Patrick wrote:
> Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
> > see patch
>
> Any opinion on these patches? Patch 1-8 seem to be refactorings only,
> and splitting functions into smaller logical units looks good to me, but
> I'd like to hear from someone deeper in the AMD code.

Yes, if these 8 are not refactorings, then it's a bug.

I know it's a little work to review it all, but it does not have
to be one person. You can review just one patch, maybe.

Testing is maybe better to do with all of them, or all without 
negative reviews, or something. I've tested them one by one 
and it is a little a waste of time. And I haven't found a single
one that fixes it for me. Must be a combination, possibly not
all but not sure which ones. They're secuential although not
each and every one needs all previous ones.

By the way testing for both SVI and PVI is welcome (for AMD FAM 10).
I don't intend to break PVI, but I can't test it.

Some of the later ones may be a little paranoid or a matter of taste
but I tried to split them in small pieces so they can be rejected
or modified.

[PATCH] Fam10 FIDVID in SVI 01/25

[Marc Jones]

On Thu, Feb 24, 2011 at 2:27 PM, xdrudis  wrote:
> On Thu, Feb 24, 2011 at 02:31:29PM +0100, Georgi, Patrick wrote:
> > Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
> > > see patch
> >
> > Any opinion on these patches? Patch 1-8 seem to be refactorings only,
> > and splitting functions into smaller logical units looks good to me, but
> > I'd like to hear from someone deeper in the AMD code.
>
> Yes, if these 8 are not refactorings, then it's a bug.
>
> I know it's a little work to review it all, but it does not have
> to be one person. You can review just one patch, maybe.
>
> Testing is maybe better to do with all of them, or all without
> negative reviews, or something. I've tested them one by one
> and it is a little a waste of time. And I haven't found a single
> one that fixes it for me. Must be a combination, possibly not
> all but not sure which ones. They're secuential although not
> each and every one needs all previous ones.
>
> By the way testing for both SVI and PVI is welcome (for AMD FAM 10).
> I don't intend to break PVI, but I can't test it.
>
> Some of the later ones may be a little paranoid or a matter of taste
> but I tried to split them in small pieces so they can be rejected
> or modified.

Acked-by: Marc Jones

r6387

[PATCH] Fam10 FIDVID in SVI 02/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 02/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:36 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6388

[PATCH] Fam10 FIDVID in SVI 03/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 03/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:37 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6389

[PATCH] Fam10 FIDVID in SVI 04/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 04/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:37 PM, xdrudis  wrote:
> see patch

Acked-by:Marc Jones

r6390

[PATCH] Fam10 FIDVID in SVI 05/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 05/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:37 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6391

[PATCH] Fam10 FIDVID in SVI 06/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 06/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:38 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6392

[PATCH] Fam10 FIDVID in SVI 07/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 07/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:38 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6393

[PATCH] Fam10 FIDVID in SVI 08/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 08/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:39 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6394

[PATCH] Fam10 FIDVID in SVI 09/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 09/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:39 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6395

[PATCH] Fam10 FIDVID in SVI 10/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 10/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:39 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6396

[PATCH] Fam10 FIDVID in SVI 11/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 11/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:40 PM, xdrudis  wrote:
> see patch

Acked-by:Marc Jones

r6397

[PATCH] Fam10 FIDVID in SVI 12/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 12/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:40 PM, xdrudis  wrote:
> see patch

Acked-by:Marc Jones

r6398

[PATCH] Fam10 FIDVID in SVI 13/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 13/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:40 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6399

[PATCH] Fam10 FIDVID in SVI 14/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 14/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:42 PM, xdrudis  wrote:
> see patch

Minor tweak to remove codeDelay() function  that was no longer used..

Acked-by: Marc Jones 
r6401

[PATCH] Fam10 FIDVID in SVI 15/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 15/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:43 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6402

[PATCH] Fam10 FIDVID in SVI 16/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 16/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:43 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6403

[PATCH] Fam10 FIDVID in SVI 17/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 17/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:43 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6404

[PATCH] Fam10 FIDVID in SVI 18/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 18/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:44 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6405

[PATCH] Fam10 FIDVID in SVI 19/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 19/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:44 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6406

[PATCH] Fam10 FIDVID in SVI 20/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 20/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:44 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6407

[PATCH] Fam10 FIDVID in SVI 21/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 21/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:45 PM, xdrudis  wrote:
> see patch

I moved coreDelay() to mcti_d where it was being used.

Acked-by: Marc Jones

r6408

[PATCH] Fam10 FIDVID in SVI 22/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 22/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:45 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones

r6409

[PATCH] Fam10 FIDVID in SVI 23/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 23/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:45 PM, xdrudis  wrote:
> see patch

Acked-by: Marc Jones 
r 6410

[PATCH] Fam10 FIDVID in SVI 24/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 24/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:46 PM, xdrudis  wrote:
> see patch

yay documentation! :)

Acked-by: Marc Jones

r 6411

[PATCH] Fam10 FIDVID in SVI 25/25

[xdrudis]

see patch

[PATCH] Fam10 FIDVID in SVI 25/25

[Marc Jones]

On Wed, Feb 16, 2011 at 11:46 PM, xdrudis  wrote:
> see patch

Oops no patch attached.

Marc

[Ivaylo Valkov]

Xavi Drudis Ferran writes:
> Hello.

Hello.
> Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?

I have interest in that. I am working on ECS A740GM-M port and have
asked that question in my mail, but for different reasons. [1] My
patches are not in the repository yet. Still haven't split them. :(
Right now I use a workaround to prevent loading of microcode. I've made
a zero-filled microcode patch file that is kept in the board
directory. The Kconfig option for the microcode file points to that
file. The effect of this is same revision (0) in patch and CPU and the
microcode is not loaded. This worked on Sempron 140 and r6725. Haven't
tested it with recent changes and revisions.

[1] http://www.coreboot.org/pipermail/coreboot/2011-January/063067.html

[Ward Vandewege]

Hi Xavi,

On Wed, Feb 16, 2011 at 02:45:02PM +0100, Xavi Drudis Ferran wrote:
> Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?

Yes, please. I would test and ack that.

Thanks,
Ward.

[PATCH] disabling microcode update

[xdrudis]

On Fri, Feb 18, 2011 at 10:19:31AM -0500, Ward Vandewege wrote:
> Hi Xavi,
>
> On Wed, Feb 16, 2011 at 02:45:02PM +0100, Xavi Drudis Ferran wrote:
> > Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?
>
> Yes, please. I would test and ack that.
>
> Thanks,
> Ward.

Here it is. It's limited to fam10. I don't know about other families nor can test.

Sorry about having missed the request from Ivaylo, for a moment I
thought I was the only one interested.

It works in my board (but my board still does not boot, so tests in
functional boards are welcome).

Thanks.

[PATCH] disabling microcode update

[Stefan Reinauer]

On 2/19/11 12:45 PM, xdrudis wrote:
> On Fri, Feb 18, 2011 at 10:19:31AM -0500, Ward Vandewege wrote:
> > > Hi Xavi,
> > >
> > > On Wed, Feb 16, 2011 at 02:45:02PM +0100, Xavi Drudis Ferran wrote:
> > > > > Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?

What's the particular rationale behind this? When disabling microcode 
updates, why don't you also disable the other erratas?
>      Select this to apply (propietary?) patches to the cpu

I don't think the word proprietary (nor the question mark) applies very 
well here.
>      microcode provided by AMD to correct issues in the CPU after
>      production, and distributed with coreboot (not necessarily
>      the latest microcode version produced by AMD, but only
>      applied if newer than the version in your CPU).
>  Unselect to let FAM10 CPUs run with factory microcode.  If

Here's some formatting problem. It's also unclear what you mean by 
"factory microcode" imho.
>      you unselect this, no binary microcode patches will be
>      included in the image, so it will help you get an image
>      which you have the entire source code for and may simplify
>      license compliance (IANAL).

I don't see how this makes license compliance any easier. Also, please 
refrain from using terms like IANAL. If we make claims they should be 
either right to the best of our knowledge or we don't put them in the 
source code, anyways.

BTW, some corporations had legal teams analyze the microcode license and 
it was considered not problematic for inclusion in coreboot in the sense 
of the GPL.
> --- src/cpu/amd/model_10xxx/update_microcode.c	2011-02-19 21:56:44.000000000 +0100
> +++ src/cpu/amd/model_10xxx/update_microcode.c	2011-02-19 22:09:17.000000000 +0100
> @@ -29,6 +29,7 @@
>   #include<cpu/amd/microcode.h>
>   #endif
>
> +#if CONFIG_UPDATE_CPU_MICROCODE
>   static const u8 microcode_updates[] __attribute__ ((aligned(16))) = {

Please change the patch to not include update_microcode.c in the 
Makefile for the case that microcode selection is disabled.

Stefan