On Mon, Apr 07, 2008 at 03:06:29PM -0600, Marc Jones wrote:
Bring Fam10 memory controller init up to date with the latest AMD BKDG recomendations.
Changes include the following: fix > 4GB dqs tests fix channel interleaving fix memory hoisting across nodes ecc memory scrub updates debug print changes minor cleanups
Signed-off-by: Marc Jones (marc.jones@amd.com)
NAK.
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mct_d.c 2008-04-07 11:28:02.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c 2008-04-07 14:45:52.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -277,12 +277,15 @@ mctHookAfterCPU(); /* Setup external northbridge(s) */
print_t("mctAutoInitMCT_D: DQSTiming_D\n");
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing*/
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing */
- print_t("mctAutoInitMCT_D: UMAMemTyping_D\n");
- UMAMemTyping_D(pMCTstat, pDCTstatA); /* Fix up for UMA sizing */
Please keep code changes and whitespace changes apart. In particular when changing things like these (that are not very well known) I think that is important.
print_t("mctAutoInitMCT_D: :OtherTiming\n"); mct_OtherTiming(pMCTstat, pDCTstatA);
- if (ReconfigureDIMMspare_D(pMCTstat, pDCTstatA)) { /* RESET# if 1st pass of DIMM spare enabled*/
- if (ReconfigureDIMMspare_D(pMCTstat, pDCTstatA)) { /* RESET# if 1st pass of DIMM spare enabled */
..it goes on..
goto restartinit;}
@@ -290,7 +293,7 @@ InterleaveChannels_D(pMCTstat, pDCTstatA);
print_t("mctAutoInitMCT_D: ECCInit_D\n");
- if (ECCInit_D(pMCTstat, pDCTstatA)) { /* Setup ECC control and ECC check-bits*/
- if (ECCInit_D(pMCTstat, pDCTstatA)) { /* Setup ECC control and ECC check-bits */
..and on.
//Peter
Peter Stuge wrote:
On Mon, Apr 07, 2008 at 03:06:29PM -0600, Marc Jones wrote:
Bring Fam10 memory controller init up to date with the latest AMD BKDG recomendations.
Changes include the following: fix > 4GB dqs tests fix channel interleaving fix memory hoisting across nodes ecc memory scrub updates debug print changes minor cleanups
Signed-off-by: Marc Jones (marc.jones@amd.com)
NAK.
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mct_d.c 2008-04-07 11:28:02.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c 2008-04-07 14:45:52.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -277,12 +277,15 @@ mctHookAfterCPU(); /* Setup external northbridge(s) */
print_t("mctAutoInitMCT_D: DQSTiming_D\n");
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing*/
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing */
- print_t("mctAutoInitMCT_D: UMAMemTyping_D\n");
- UMAMemTyping_D(pMCTstat, pDCTstatA); /* Fix up for UMA sizing */
Please keep code changes and whitespace changes apart. In particular when changing things like these (that are not very well known) I think that is important.
Serious? You are naking over a couple whitespace changes that get fixed as the code is fixed? Do you have concerns that the patch is confused by the changes or are you just naking on principal?
Marc
On Mon, Apr 07, 2008 at 03:33:20PM -0600, Marc Jones wrote:
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing*/
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing */
- print_t("mctAutoInitMCT_D: UMAMemTyping_D\n");
- UMAMemTyping_D(pMCTstat, pDCTstatA); /* Fix up for UMA sizing */
Please keep code changes and whitespace changes apart. In particular when changing things like these (that are not very well known) I think that is important.
Serious?
Yes, afraid so.
You are naking over a couple whitespace changes that get fixed as the code is fixed?
Right.
Do you have concerns that the patch is confused by the changes
Yes, I think it is.
or are you just naking on principal?
No, not really.
This is how I reason:
Ideally whitespace changes should not be needed, all commits should always be formatted perfectly.
In practice this isn't true, and whitespace changes are a good thing. I don't care all that much about whitespace formatting myself, but others do and I agree it does make the code as a whole look more tidy.
But, since the basic assumption is that commits change code and nothing else, I think whitespace changes should be isolated commits, since they clutter code diffs and make human diff processing more difficult.
I think this argument is even more important when code is changed that is not very well-known and well understood.
//Peter
Peter Stuge wrote:
On Mon, Apr 07, 2008 at 03:33:20PM -0600, Marc Jones wrote:
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing*/
- DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing */
- print_t("mctAutoInitMCT_D: UMAMemTyping_D\n");
- UMAMemTyping_D(pMCTstat, pDCTstatA); /* Fix up for UMA sizing */
Please keep code changes and whitespace changes apart. In particular when changing things like these (that are not very well known) I think that is important.
Serious?
Yes, afraid so.
You are naking over a couple whitespace changes that get fixed as the code is fixed?
Right.
Do you have concerns that the patch is confused by the changes
Yes, I think it is.
or are you just naking on principal?
No, not really.
This is how I reason:
Ideally whitespace changes should not be needed, all commits should always be formatted perfectly.
In practice this isn't true, and whitespace changes are a good thing. I don't care all that much about whitespace formatting myself, but others do and I agree it does make the code as a whole look more tidy.
But, since the basic assumption is that commits change code and nothing else, I think whitespace changes should be isolated commits, since they clutter code diffs and make human diff processing more difficult.
I think this argument is even more important when code is changed that is not very well-known and well understood.
//Peter
So, yes, you are naking on principal.
Zuh? Whitespace issues are generally so insignificant that if it doesn't get fixed when the code is fixed then the whitespace issues might never get revisited. They're also tedious to keep track of. And for a large patches the follow-up whitespace code review can fall out of sync with the actual code review and make resolving conflicts between the original patch and follow-up whitespace patch prone to error. And in case we have to cherrypick changes to rollback it doesn't make sense to have to worry about a separate patch for whitespace fixes.
Just my $0.02.
Clearly, this is one of those cases simple enough that everybody can have an opinion and an endless thread of non-sense is likely to be the result. I beg your pardon that I don't keep out of this ;-)
David Hendricks wrote:
Zuh? Whitespace issues are generally so insignificant that if it doesn't get fixed when the code is fixed then the whitespace issues might never get revisited. They're also tedious to keep track of. And for a large patches the follow-up whitespace code review can fall out of sync with the actual code review and make resolving conflicts between the original patch and follow-up whitespace patch prone to error. And in case we have to cherrypick changes to rollback it doesn't make sense to have to worry about a separate patch for whitespace fixes.
Just my $0.02.
Absolutely.
While in theory I agree with Peter that separating whitespace is a lot easier to read, I don't believe this is really a practical approach. When working on code, I fix whitespace on the fly, if I do at all. I never put in a separate round for just fixing whitespace because that's just a bogus way of spending precious time. So is seperating the whitespace fixes from the rest of the code after working on it.
So, guys, please. If Marc saves half an hour by not having to seperate the patches, and every reviewing coreboot developer has to spend 5 minutes more during the review process, I am all for it to just check this in as it is. We're smart people and can separate the wheat from the chaff without being all too confused. If it happens to be viable, yes, please try to make it like that, but NACKing a patch for that reason is unacceptable.
Therefore I say Acked-by: Stefan Reinauer stepan@coresystems.de for that patch as it is now, and I hope we can keep this low-profile.
All the best
Stefan
Hi,
(sorry, can't resist to join the thread :)
On Mon, Apr 07, 2008 at 05:10:34PM -0700, Stefan Reinauer wrote:
While in theory I agree with Peter that separating whitespace is a lot easier to read,
Yes.
I don't believe this is really a practical approach.
Yes and no. I personally don't mind one or two whitespace fixes in a patch which actually does functional changes, as long as there are no _massive_ whitespace changes mixed into the patch. Those should definately be separated in an extra patch.
That's not the case in Marc's patch, though.
chaff without being all too confused. If it happens to be viable, yes, please try to make it like that, but NACKing a patch for that reason is unacceptable.
While I'm probably the whitespace fanatic #1 around here I have to agree with this ;) Yes, whitespace fixes are important, and yes they should be in separate patches if there are many of them, but that's not a reason to NACK a patch, IMO...
Uwe.
On Mon, Apr 07, 2008 at 03:56:33PM -0600, Marc Jones wrote:
Peter Stuge wrote:
Do you have concerns that the patch is confused by the changes
Yes, I think it is.
or are you just naking on principal?
No, not really.
This is how I reason:
So, yes, you are naking on principal.
It was sort-of a trick question since the principle applies when I think whitespace changes cause confusion.
I NAKed because I thought the whitespace changes were too confusing in this patch, not because of the principle.
On Mon, Apr 07, 2008 at 05:10:34PM -0700, Stefan Reinauer wrote:
While in theory I agree with Peter that separating whitespace is a lot easier to read, I don't believe this is really a practical approach.
Sure it is, just don't change whitespace until the code changes are done.
When working on code, I fix whitespace on the fly, if I do at all.
My point is that it is much better for patch readability and long term repo quality if whitespace is not changed at all while making changes to code.
I never put in a separate round for just fixing whitespace because that's just a bogus way of spending precious time.
Me neither. I don't want to change whitespace at all. Sometimes I will actually play around with comments and whitespace during my thought process but I review and restore forgotten changes before sending a patch.
So is seperating the whitespace fixes from the rest of the code after working on it.
Yes, I completely agree. Improvements done after the fact indeed waste some time but also lead to higher quality source.
Personally I don't care at all about whitespace, but I do care that changes in whitespace is mixed with changes in code.
I am the first to admit that following any coding style will be at least slightly annoying unless it happens to be your own style. :)
If it happens to be viable, yes, please try to make it like that, but NACKing a patch for that reason is unacceptable.
Hm, I should NAK patches that I have reviewed and don't want committed, right?
Therefore I say Acked-by: Stefan Reinauer stepan@coresystems.de for that patch as it is now, and I hope we can keep this low-profile.
Patches have been committed even with NAKs in the past and I consider that a good thing. Maybe I have just misunderstood the point of NAK?
//Peter
On Tue, 8 Apr 2008, Peter Stuge wrote:
On Mon, Apr 07, 2008 at 03:56:33PM -0600, Marc Jones wrote:
Peter Stuge wrote:
Do you have concerns that the patch is confused by the changes
Yes, I think it is.
or are you just naking on principal?
No, not really.
This is how I reason:
So, yes, you are naking on principal.
It was sort-of a trick question since the principle applies when I think whitespace changes cause confusion.
I NAKed because I thought the whitespace changes were too confusing in this patch, not because of the principle.
On Mon, Apr 07, 2008 at 05:10:34PM -0700, Stefan Reinauer wrote:
While in theory I agree with Peter that separating whitespace is a lot easier to read, I don't believe this is really a practical approach.
Sure it is, just don't change whitespace until the code changes are done.
When working on code, I fix whitespace on the fly, if I do at all.
My point is that it is much better for patch readability and long term repo quality if whitespace is not changed at all while making changes to code.
I never put in a separate round for just fixing whitespace because that's just a bogus way of spending precious time.
Me neither. I don't want to change whitespace at all. Sometimes I will actually play around with comments and whitespace during my thought process but I review and restore forgotten changes before sending a patch.
So is seperating the whitespace fixes from the rest of the code after working on it.
Yes, I completely agree. Improvements done after the fact indeed waste some time but also lead to higher quality source.
Personally I don't care at all about whitespace, but I do care that changes in whitespace is mixed with changes in code.
I am the first to admit that following any coding style will be at least slightly annoying unless it happens to be your own style. :)
If it happens to be viable, yes, please try to make it like that, but NACKing a patch for that reason is unacceptable.
Hm, I should NAK patches that I have reviewed and don't want committed, right?
Therefore I say Acked-by: Stefan Reinauer stepan@coresystems.de for that patch as it is now, and I hope we can keep this low-profile.
Why not add the -b option when using diff? (same as --ignore-space-change ) See man diff for other useful options.
my .02 Russ
On 08.04.2008 03:53, Russell Whitaker wrote:
On Tue, 8 Apr 2008, Peter Stuge wrote:
On Mon, Apr 07, 2008 at 03:56:33PM -0600, Marc Jones wrote:
Peter Stuge wrote:
Do you have concerns that the patch is confused by the changes
Yes, I think it is.
or are you just naking on principal?
No, not really.
This is how I reason:
So, yes, you are naking on principal.
It was sort-of a trick question since the principle applies when I think whitespace changes cause confusion.
I NAKed because I thought the whitespace changes were too confusing in this patch, not because of the principle.
On Mon, Apr 07, 2008 at 05:10:34PM -0700, Stefan Reinauer wrote:
While in theory I agree with Peter that separating whitespace is a lot easier to read, I don't believe this is really a practical approach.
Sure it is, just don't change whitespace until the code changes are done.
When working on code, I fix whitespace on the fly, if I do at all.
My point is that it is much better for patch readability and long term repo quality if whitespace is not changed at all while making changes to code.
I never put in a separate round for just fixing whitespace because that's just a bogus way of spending precious time.
Me neither. I don't want to change whitespace at all. Sometimes I will actually play around with comments and whitespace during my thought process but I review and restore forgotten changes before sending a patch.
So is seperating the whitespace fixes from the rest of the code after working on it.
Yes, I completely agree. Improvements done after the fact indeed waste some time but also lead to higher quality source.
Personally I don't care at all about whitespace, but I do care that changes in whitespace is mixed with changes in code.
I am the first to admit that following any coding style will be at least slightly annoying unless it happens to be your own style. :)
If it happens to be viable, yes, please try to make it like that, but NACKing a patch for that reason is unacceptable.
Hm, I should NAK patches that I have reviewed and don't want committed, right?
Therefore I say Acked-by: Stefan Reinauer stepan@coresystems.de for that patch as it is now, and I hope we can keep this low-profile.
Why not add the -b option when using diff? (same as --ignore-space-change ) See man diff for other useful options.
The corresponding svn diff command looks like this: svn diff -x -b
Regards, Carl-Daniel
Removed confusing whitespace changes.
On Tue, Apr 08, 2008 at 09:27:15AM -0600, Marc Jones wrote:
Removed confusing whitespace changes.
Awesome. Made the patch 1/3 smaller too!
Two questions:
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c @@ -3358,10 +3395,8 @@ /* Tri-state unused ODTs when motherboard termination is available */
// FIXME: skip for Ax
- dev = pDCTstat->dev_dct;
- word = 0;
- for (cs = 0; cs < 8; cs += 2) {
- // FIXME: BUG53109 function can incorrectly program F2x[1,0]9C_x0C[11:8]_ODTTri
+/* for (cs = 0; cs < 8; cs += 2) { if (!(pDCTstat->CSPresent & (1 << cs))) { if (!(pDCTstat->CSPresent & (1 << (cs + 1)))) word |= (1 << (cs >> 1)); @@ -3372,6 +3407,7 @@ val = Get_NB32_index_wait(dev, index_reg, index); val |= (word << 8); Set_NB32_index_wait(dev, index_reg, index, val); +*/ }
Can you comment on this comment? Why not just remove the code?
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctecc_d.c @@ -101,7 +101,7 @@
OF_ScrubCTL = 0; /* Scrub CTL for Dcache, L2, and dram */ val = mctGet_NVbits(NV_DCBKScrub);
- mct_AdjustScrub_D(pDCTstatA, val);
- mct_AdjustScrub_D(pDCTstatA, (u16) &val);
Will this really work?
val is u32, mct_AdjustScrub_D() wants u16*, isn't this casting/truncating the pointer?
//Peter
Peter Stuge wrote:
On Tue, Apr 08, 2008 at 09:27:15AM -0600, Marc Jones wrote:
Removed confusing whitespace changes.
Awesome. Made the patch 1/3 smaller too!
Two questions:
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c @@ -3358,10 +3395,8 @@ /* Tri-state unused ODTs when motherboard termination is available */
// FIXME: skip for Ax
- dev = pDCTstat->dev_dct;
- word = 0;
- for (cs = 0; cs < 8; cs += 2) {
- // FIXME: BUG53109 function can incorrectly program F2x[1,0]9C_x0C[11:8]_ODTTri
+/* for (cs = 0; cs < 8; cs += 2) { if (!(pDCTstat->CSPresent & (1 << cs))) { if (!(pDCTstat->CSPresent & (1 << (cs + 1)))) word |= (1 << (cs >> 1)); @@ -3372,6 +3407,7 @@ val = Get_NB32_index_wait(dev, index_reg, index); val |= (word << 8); Set_NB32_index_wait(dev, index_reg, index, val); +*/ }
Can you comment on this comment? Why not just remove the code?
The code was close to what needed to be done but some final details needed to be worked out by some people that know better than me. Turns out they have made the changes so I have added the fix back in.
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctecc_d.c @@ -101,7 +101,7 @@
OF_ScrubCTL = 0; /* Scrub CTL for Dcache, L2, and dram */ val = mctGet_NVbits(NV_DCBKScrub);
- mct_AdjustScrub_D(pDCTstatA, val);
- mct_AdjustScrub_D(pDCTstatA, (u16) &val);
Will this really work?
val is u32, mct_AdjustScrub_D() wants u16*, isn't this casting/truncating the pointer?
This was a good catch. I decided to add a nvbits variable. Reusing val was not really needed. That is just the register constrained asm programmer in me.
New patch attached. Marc
On 09.04.2008 00:28, Marc Jones wrote:
Bring Fam10 memory controller init up to date with the latest AMD BKDG recomendations.
Changes include the following: fix > 4GB dqs tests fix channel interleaving fix memory hoisting across nodes (memory hole was always on node0) ecc memory scrub updates debug print changes
Signed-off-by: Marc Jones (marc.jones@amd.com)
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net with some comments, though.
04:49 < carldani> mct_d.c, line 605. Should this be devx and not dev? 04:51 < carldani> marcj: mct_d.c, function HTMemMapInit_D(), the first big for (Node = 0; Node < MAX_NODES_SUPPORTED; Node++) loop 04:54 < marcj> ok, had to read through. The memmap is setup on node0 and copied to the other nodes later. so dev, not devx. 04:55 < carldani> marcj: the DRAM Hole Address Register is set via devx in each node, but the Node number <-> DRAM Base mapping and the Node number <-> DstNode mapping is set in Node 0 04:55 < marcj> yes 04:55 < carldani> ah ok thanks 04:56 < marcj> the hole address is only set on the node with the hole 04:57 < marcj> the entire map is the same for all nodes so it gets setup all in the first node. Otherwise you would have part of it in each node and would be a pain to copy iy to every node. 04:57 < marcj> and that was the bug, the hole was always being set on the first node. 05:04 < carldani> marcj: could you add the last two lines you wrote to the changelog? that would help immensely. 05:05 < marcj> sure
Adding that explanation to the code as comments would be even greater.
Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mct_d.c 2008-04-07 16:14:23.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c 2008-04-08 14:38:27.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -279,6 +279,9 @@ print_t("mctAutoInitMCT_D: DQSTiming_D\n"); DQSTiming_D(pMCTstat, pDCTstatA); /* Get Receiver Enable and DQS signal timing*/
- print_t("mctAutoInitMCT_D: UMAMemTyping_D\n");
- UMAMemTyping_D(pMCTstat, pDCTstatA); /* Fix up for UMA sizing */
- print_t("mctAutoInitMCT_D: :OtherTiming\n"); mct_OtherTiming(pMCTstat, pDCTstatA);
@@ -348,10 +351,11 @@ /* FIXME: BOZO- DQS training every time*/ nv_DQSTrainCTL = 1;
I assume the assignment above will be replaced with a calculated value in later code revisions. You confirmed that on IRC.
- print_t("DQSTiming_D: mct_BeforeDQSTrain_D:\n");
- mct_BeforeDQSTrain_D(pMCTstat, pDCTstatA);;
- phyAssistedMemFnceTraining(pMCTstat, pDCTstatA);
- if (nv_DQSTrainCTL) {
With the current code, this is always true. (see above)
print_t("DQSTiming_D: mct_BeforeDQSTrain_D:\n");mct_BeforeDQSTrain_D(pMCTstat, pDCTstatA);;phyAssistedMemFnceTraining(pMCTstat, pDCTstatA);mctHookBeforeAnyTraining();
print_t("DQSTiming_D: TrainReceiverEn_D FirstPass:\n");
@@ -512,7 +516,7 @@ u32 val; u32 base; u32 limit;
- u32 dev;
u32 dev, devx; struct DCTStatStruc *pDCTstat;
_MemHoleRemap = mctGet_NVbits(NV_MemHole);
@@ -531,6 +535,8 @@
for (Node = 0; Node < MAX_NODES_SUPPORTED; Node++) {
pDCTstat = pDCTstatA + Node;devx = pDCTstat->dev_map;@@ -569,7 +575,7 @@ val <<= 8; /* shl 16, rol 24 */ val |= DramHoleBase << 24; val |= 1 << DramHoleValid;
Set_NB32(dev, 0xF0, val); /*Dram Hole Address Register*/
Set_NB32(devx, 0xF0, val); /*Dram Hole Address Register*/ pDCTstat->DCTSysLimit += HoleSize; base = pDCTstat->DCTSysBase; limit = pDCTstat->DCTSysLimit;@@ -598,21 +604,31 @@ pMCTstat->SysLimit = limit; } Set_NB32(dev, 0x40 + (Node << 3), base); /* [Node] + Dram Base 0 */
val = limit & 0xffff0000;val |= Node; /* set DstNode*/
if ((mctSetNodeBoundary_D()) && (limit > 0x00400000)) {
Hm. Could it be that we have to check (limit >= 0x00400000) instead?
limit++;limit &= 0xFFC00000;limit--;
I see the formula, but I don't understand its purpose.
}val = limit & 0xFFFF0000;val |= Node;Set_NB32(dev, 0x44 + (Node << 3), val); /* set DstNode */
limit = pDCTstat->DCTSysLimit; if (limit) {
NextBase = (limit & 0xffff0000) + 0x10000;
NextBase = (limit & 0xFFFF0000) + 0x10000;if ((mctSetNodeBoundary_D()) && (NextBase > 0x00400000)) {NextBase++;NextBase &= 0xFFC00000;NextBase--;
Same here.
}} }
/* Copy dram map from Node 0 to Node 1-7 */ for (Node = 1; Node < MAX_NODES_SUPPORTED; Node++) {
pDCTstat = pDCTstatA + Node;u32 devx = pDCTstat->dev_map;
pDCTstat = pDCTstatA + Node;devx = pDCTstat->dev_map;if (pDCTstat->NodePresent) { printk_debug(" Copy dram map from Node 0 to Node %02x \n", Node);
@@ -1016,7 +1032,7 @@ if ((val == 0) || (val == 0xFF)) { pDCTstat->ErrStatus |= 1<<SB_NoTrcTrfc; pDCTstat->ErrCode = SC_VarianceErr;
val = Get_DefTrc_k_D(pDCTstat->DIMMAutoSpeed);
val = Get_DefTrc_k_D(pDCTstat->Speed); } else { byte = mctRead_SPD(smbaddr, SPD_TRCRFC); if (byte & 0xF0) {@@ -1054,7 +1070,7 @@
/* Convert DRAM CycleTiming values and store into DCT structure */ DDR2_1066 = 0;
- byte = pDCTstat->DIMMAutoSpeed;
- byte = pDCTstat->Speed; if (byte == 5) DDR2_1066 = 1; Tk40 = Get_40Tk_D(byte);
@@ -1175,12 +1191,10 @@ dword = Trtp * 10; pDCTstat->DIMMTrtp = dword; val = pDCTstat->Speed;
- if (val <= 2) {
val = 2; /* Calculate by 7.75ns / Speed in ns to get clock # */- } else if (val == 4) { /* Note a speed of 3 will be a Trtp of 3 */
val = 3;- } else if (val == 5){
val = 2;
- if (val <= 2) { /* Calculate by 7.75ns / Speed in ns to get clock # */
val = 2; /* for DDR400/DDR533 */- } else { /* Note a speed of 3 will be a Trtp of 3 */
val = 3; /* for DDR667/DDR800/DDR1066 */
Are the added tabs before the comments intentional?
} pDCTstat->Trtp = val;
@@ -1810,7 +1824,7 @@ /* SetCKETriState */ SetODTTriState(pMCTstat, pDCTstat, dct);
- if ( pDCTstat->Status & 1<<SB_128bitmode) {
- if ( pDCTstat->Status & (1 << SB_128bitmode)) {
Space after (
SetCSTriState(pMCTstat, pDCTstat, 1); /* force dct1) */ SetODTTriState(pMCTstat, pDCTstat, 1); /* force dct1) */} @@ -2631,7 +2645,7 @@ if (!pDCTstat->GangedMode) { dev = pDCTstat->dev_dct; pDCTstat->NodeSysLimit += pDCTstat->DCTSysLimit;
/* if DCT0 and DCT1 exist both, set DctSelBaseAddr[47:27] */
/* if DCT0 and DCT1 exist both, set DctSelBaseAddr[47:27] to the top of DCT0 */
Maybe use "...both exist...", but I'm not a native speaker.
if (dct == 0) { if (pDCTstat->DIMMValidDCT[1] > 0) { dword = pDCTstat->DCTSysLimit + 1;@@ -2641,8 +2655,7 @@ pMCTstat->HoleBase = (DramHoleBase & 0xFFFFF800) << 8; val = pMCTstat->HoleBase; val >>= 16;
val &= ~(0xFF);val |= (((~val) & 0xFF) + 1);
val = (((~val) & 0xFF) + 1); val <<= 8; dword += val; }@@ -2653,6 +2666,7 @@ val |= 3; /* Set F2x110[DctSelHiRngEn], F2x110[DctSelHi] */ Set_NB32(dev, reg, val); print_tx("AfterStitch DCT0 and DCT1: DRAM Controller Select Low Register = ", val);
print_tx("AfterStitch DCT0 and DCT1: DRAM Controller Select High Register = ", dword); reg = 0x114; val = dword;@@ -2664,7 +2678,7 @@ if (pDCTstat->DIMMValidDCT[0] == 0) { dword = pDCTstat->NodeSysBase; dword >>= 8;
if (dword >= DramHoleBase) {
if ((dword >= DramHoleBase) && _MemHoleRemap) { pMCTstat->HoleBase = (DramHoleBase & 0xFFFFF800) << 8; val = pMCTstat->HoleBase; val >>= 8;@@ -2680,6 +2694,7 @@ val |= 3; /* Set F2x110[DctSelHiRngEn], F2x110[DctSelHi] */ Set_NB32(dev, reg, val); print_tx("AfterStitch DCT1 only: DRAM Controller Select Low Register = ", val);
print_tx("AfterStitch DCT1 only: DRAM Controller Select High Register = ", dword); }@@ -2872,16 +2887,25 @@ static void Get_Twrrd(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstat, u8 dct) {
- u8 byte, bytex;
u8 byte, bytex, val; u32 index_reg = 0x98 + 0x100 * dct; u32 dev = pDCTstat->dev_dct;
/* On any given byte lane, the largest WrDatGrossDlyByte delay of any DIMM minus the DqsRcvEnGrossDelay delay of any other DIMM is equal to the Critical Gross Delay Difference (CGDD) for Twrrd.*/
- pDCTstat->Twrrd = 0;
- /* WrDatGrossDlyByte only use one set register when DDR400~DDR667
DDR800 have two set register for DIMM0 and DIMM1 */- if (pDCTstat->Speed > 3) {
val = Get_WrDatGross_Diff(pDCTstat, dct, dev, index_reg);- } else {
val = Get_WrDatGross_MaxMin(pDCTstat, dct, dev, index_reg, 1); /* WrDatGrossDlyByte byte 0,1,2,3 for DIMM0 */pDCTstat->WrDatGrossH = (u8) val; /* low byte = max value */- }
- Get_DqsRcvEnGross_Diff(pDCTstat, dev, index_reg);
- Get_WrDatGross_Diff(pDCTstat, dct, dev, index_reg);
- bytex = pDCTstat->DqsRcvEnGrossL; byte = pDCTstat->WrDatGrossH; if (byte > bytex) {
@@ -2946,12 +2970,16 @@ u8 i; u32 val; u8 byte;
u8 ecc_reg = 0;
Smallest_0 = 0xFF; Smallest_1 = 0xFF; Largest_0 = 0; Largest_1 = 0;
if (index == 0x12)
ecc_reg = 1;for (i=0; i < 8; i+=2) { if ( pDCTstat->DIMMValid & (1 << i)) { val = Get_NB32_index_wait(dev, index_reg, index);
@@ -2960,11 +2988,13 @@ Smallest_0 = byte; if (byte > Largest_0) Largest_0 = byte;
byte = (val >> 16) & 0xFF;if (byte < Smallest_1)Smallest_1 = byte;if (byte > Largest_1)Largest_1 = byte;
if (!(ecc_reg)) {byte = (val >> 16) & 0xFF;if (byte < Smallest_1)Smallest_1 = byte;if (byte > Largest_1)Largest_1 = byte;}@@ -2973,8 +3003,9 @@ two DIMMs is less than half of a MEMCLK */ if ((Largest_0 - Smallest_0) > 31) return 1;
- if ((Largest_1 - Smallest_1) > 31)
return 1;
- if (!(ecc_reg))
if ((Largest_1 - Smallest_1) > 31)return 1;}
@@ -3072,10 +3103,14 @@ u8 byte; u32 val; u16 word;
u8 ecc_reg = 0;
Smallest = 7; Largest = 0;
if (index == 0x12)
ecc_reg = 1;for (i=0; i < 8; i+=2) { if ( pDCTstat->DIMMValid & (1 << i)) { val = Get_NB32_index_wait(dev, index_reg, index);
@@ -3085,11 +3120,13 @@ Smallest = byte; if (byte > Largest) Largest = byte;
byte = (val >> (16 + 5)) & 0xFF;if (byte < Smallest)Smallest = byte;if (byte > Largest)Largest = byte;
if (!(ecc_reg)) {byte = (val >> (16 + 5)) & 0xFF;if (byte < Smallest)Smallest = byte;if (byte > Largest)Largest = byte;}@@ -3353,25 +3390,32 @@ u32 index_reg = 0x98 + 0x100 * dct; u8 cs; u32 index;
- u16 word;
- /* Tri-state unused ODTs when motherboard termination is available */
u8 odt;
u8 max_dimms;
// FIXME: skip for Ax
- dev = pDCTstat->dev_dct;
- word = 0;
- /* Tri-state unused ODTs when motherboard termination is available */
- max_dimms = (u8) mctGet_NVbits(NV_MAX_DIMMS);
- odt = 0x0F; /* tristate all the pins then clear the used ones. */
- for (cs = 0; cs < 8; cs += 2) {
if (!(pDCTstat->CSPresent & (1 << cs))) {if (!(pDCTstat->CSPresent & (1 << (cs + 1))))word |= (1 << (cs >> 1));
if (pDCTstat->CSPresent & (1 << cs)) {odt &= ~(1 << (cs / 2));/* if quad-rank capable platform clear adtitional pins */if (max_dimms != MAX_CS_SUPPORTED) {if (pDCTstat->CSPresent & (1 << (cs + 1)))odt &= ~(4 << (cs / 2));}} }
index = 0x0C; val = Get_NB32_index_wait(dev, index_reg, index);
- val |= (word << 8);
- val |= (odt << 8); Set_NB32_index_wait(dev, index_reg, index, val);
}
@@ -3414,8 +3458,14 @@ }
/* Override/Exception */
- if ((pDCTstat->Speed == 2) && (pDCTstat->MAdimms[dct] == 4))
dword &= 0xF18FFF18;
if (!pDCTstat->GangedMode) {
i = 0; // use i for the dct setting requiredif (pDCTstat->MAdimms[0] < 4)i = 1;if (((pDCTstat->Speed == 2) || (pDCTstat->Speed == 3)) && (pDCTstat->MAdimms[i] == 4))dword &= 0xF18FFF18;index_reg = 0x98; /* force dct = 0 */}
Set_NB32_index_wait(dev, index_reg, 0x0a, dword);
} @@ -3733,12 +3783,26 @@ struct DCTStatStruc *pDCTstat, u8 dct) { u8 Receiver;
- u32 val; u32 dev = pDCTstat->dev_dct; u32 reg_off = 0x100 * dct; u32 addr;
u32 lo, hi;
u8 wrap32dis = 0; u8 valid = 0;
/* FIXME: Skip reset DLL for B3 */
addr = HWCR;
_RDMSR(addr, &lo, &hi);
if(lo & (1<<17)) { /* save the old value */
wrap32dis = 1;}
lo |= (1<<17); /* HWCR.wrap32dis */
lo &= ~(1<<15); /* SSEDIS */
/* Setting wrap32dis allows 64-bit memory references in 32bit mode */
_WRMSR(addr, lo, hi);
pDCTstat->Channel = dct; Receiver = mct_InitReceiver_D(pDCTstat, dct); /* there are four receiver pairs, loosely associated with chipselects.*/
@@ -3747,23 +3811,24 @@ addr = mct_GetRcvrSysAddr_D(pMCTstat, pDCTstat, dct, Receiver, &valid); if (valid) { mct_Read1LTestPattern_D(pMCTstat, pDCTstat, addr); /* cache fills */
Set_NB32(dev, 0x98 + reg_off, 0x0D00000C);val = Get_NB32(dev, 0x9C + reg_off);val |= 1 << 15;Set_NB32(dev, 0x9C + reg_off, val);Set_NB32(dev, 0x98 + reg_off, 0x4D0F0F0C);mct_Wait_10ns(60); /* wait >= 300ns */Set_NB32(dev, 0x98 + reg_off, 0x0D00000C);val = Get_NB32(dev, 0x9C + reg_off);val &= ~(1 << 15);Set_NB32(dev, 0x9C + reg_off, val);Set_NB32(dev, 0x98 + reg_off, 0x4D0F0F0C);mct_Wait_10ns(400); /* wait >= 2us */
/* Write 0000_8000h to register F2x[1, 0]9C_xD080F0C */
Should there really be a space after the comma?
Set_NB32_index_wait(dev, 0x98 + reg_off, 0x4D080F0C, 0x00008000);mct_Wait(80); /* wait >= 300ns *//* Write 0000_0000h to register F2x[1, 0]9C_xD080F0C */
Dito.
Set_NB32_index_wait(dev, 0x98 + reg_off, 0x4D080F0C, 0x00000000);mct_Wait(800); /* wait >= 2us */ break; }- if(!wrap32dis) {
addr = HWCR;_RDMSR(addr, &lo, &hi);lo &= ~(1<<17); /* restore HWCR.wrap32dis */_WRMSR(addr, lo, hi);- }
}
@@ -3784,7 +3849,7 @@ // FIXME Skip for Cx dev = pDCTstat->dev_nbmisc; val = Get_NB32(dev, 0x8C); // NB Configuration Hi
val |= 36-32; // DisDatMask
val |= 1 << (36-32); // DisDatMask} @@ -3818,8 +3883,9 @@ u32 reg_off = 0x100 * dct; u32 dev = pDCTstat->dev_dct;
- /* FIXME: Add B3 */ if (pDCTstat->LogicalCPUID & AMD_DR_B2) {
mct_Wait_10ns(5000); /* Wait 50 us*/
mct_Wait(10000); /* Wait 50 us*/
The comment seems to contradict the code.
val = Get_NB32(dev, 0x110); if ( val & (1 << DramEnabled)) { /* If 50 us expires while DramEnable =0 then do the following */Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.h
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mct_d.h 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.h 2008-04-07 21:42:39.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -699,6 +699,7 @@ u8 mct_Get_Start_RcvrEnDly_1Pass(u8 Pass); u8 mct_Average_RcvrEnDly_Pass(struct DCTStatStruc *pDCTstat, u8 RcvrEnDly, u8 RcvrEnDlyLimit, u8 Channel, u8 Receiver, u8 Pass); void CPUMemTyping_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstatA); +void UMAMemTyping_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstatA); u32 mctGetLogicalCPUID(u32 Node); u8 ECCInit_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstatA); void TrainReceiverEn_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstatA, u8 Pass); @@ -730,7 +731,7 @@ u8 mct_SaveRcvEnDly_D_1Pass(struct DCTStatStruc *pDCTstat, u8 pass); static void mct_AdjustScrub_D(struct DCTStatStruc *pDCTstat, u16 *scrub_request); static u8 mct_InitReceiver_D(struct DCTStatStruc *pDCTstat, u8 dct); -static void mct_Wait_10ns (u32 cycles); +static void mct_Wait(u32 cycles); u8 mct_RcvrRankEnabled_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstat, u8 Channel, u8 ChipSel); u32 mct_GetRcvrSysAddr_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstat, u8 channel, u8 receiver, u8 *valid); void mct_Read1LTestPattern_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstat, u32 addr); Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctchi_d.c =================================================================== --- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mctchi_d.c 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mctchi_d.c 2008-04-07 21:42:57.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -41,11 +41,10 @@ /* call back to wrapper not needed ManualChannelInterleave_D(); */ /* call back - DctSelIntLvAddr = mctGet_NVbits(NV_ChannelIntlv);*/ /* override interleave */ // FIXME: Check for Cx
- DctSelIntLvAddr = 5; /* use default: Enable channel interleave */
- enabled = 1; /* with Hash*: exclusive OR of address bits[20:16, 6]. */
- DctSelIntLvAddr = mctGet_NVbits(NV_ChannelIntlv); /* typ=5: Hash*: exclusive OR of address bits[20:16, 6]. */ beforeInterleaveChannels_D(pDCTstatA, &enabled);
- if (enabled) {
- if (DctSelIntLvAddr & 1) { DctSelIntLvAddr >>= 1; HoleSize = 0; if ((pMCTstat->GStatus & (1 << GSB_SoftHole)) ||
@@ -84,7 +83,7 @@ dct0_size += DramBase; dct0_size += dct1_size; if (dct0_size >= HoleBase) /* if DctSelBaseAddr > HoleBase */
dct0_size += HoleBase;
dct0_size += HoleSize; DctSelBase = dct0_size; if (dct1_size == 0)@@ -100,7 +99,7 @@ val |= DctSelHi; val |= (DctSelIntLvAddr << 6) & 0xFF; Set_NB32(pDCTstat->dev_dct, 0x110, val);
print_tx("InterleaveChannels: DRAM Controller Select Low Register = ", val);
print_tx("InterleaveChannels: F2x110 DRAM Controller Select Low Register = ", val); if (HoleValid) { tmp = DramBase;@@ -112,10 +111,10 @@ } tmp += HoleSize; val = Get_NB32(pDCTstat->dev_map, 0xF0); /* DramHoleOffset */
val &= 0x7F;val |= (tmp & 0xFF);
val &= 0xFFFF007F;val |= (tmp & ~0xFFFF007F); Set_NB32(pDCTstat->dev_map, 0xF0, val);-print_tx("InterleaveChannels:0xF0 = ", val);
print_tx("InterleaveChannels: F1xF0 DRAM Hole Address Register = ", val); } }Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctdqs_d.c
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mctdqs_d.c 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mctdqs_d.c 2008-04-07 21:43:15.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -1130,6 +1130,9 @@ val += (1 << (15-8)); /* add 32K */ }
- /* Add a node seed */
- val += (((1 * pDCTstat->Node_ID) << 20) >> 8); /* Add 1MB per node to avoid aliases */
- /* HW remap disabled? */ if (!(pDCTstat->Status & (1 << SB_HWHole))) { if (!(pDCTstat->Status & (1 << SB_SWNodeHole))) {
@@ -1167,6 +1170,13 @@ *valid = 1; } }
- print_debug_dqs("mct_GetMCTSysAddr_D: receiver ", receiver, 2);
- print_debug_dqs("mct_GetMCTSysAddr_D: Channel ", Channel, 2);
- print_debug_dqs("mct_GetMCTSysAddr_D: base_addr ", val, 2);
- print_debug_dqs("mct_GetMCTSysAddr_D: valid ", *valid, 2);
- print_debug_dqs("mct_GetMCTSysAddr_D: status ", pDCTstat->Status, 2);
- print_debug_dqs("mct_GetMCTSysAddr_D: HoleBase ", pDCTstat->DCTHoleBase, 2);
- print_debug_dqs("mct_GetMCTSysAddr_D: Cachetop ", pMCTstat->Sub4GCacheTop, 2);
exitGetAddr: return val; Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctecc_d.c =================================================================== --- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mctecc_d.c 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mctecc_d.c 2008-04-08 15:17:01.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -88,6 +88,7 @@ u32 dev; u32 reg; u32 val;
u16 nvbits;
mctHookBeforeECC();
@@ -100,14 +101,15 @@ OB_ChipKill = mctGet_NVbits(NV_ChipKill); /* ECC Chip-kill mode */
OF_ScrubCTL = 0; /* Scrub CTL for Dcache, L2, and dram */
- val = mctGet_NVbits(NV_DCBKScrub);
- mct_AdjustScrub_D(pDCTstatA, val);
- OF_ScrubCTL |= val << 16;
- val = mctGet_NVbits(NV_L2BKScrub);
- OF_ScrubCTL |= val << 8;
- nvbits = mctGet_NVbits(NV_DCBKScrub);
- mct_AdjustScrub_D(pDCTstatA, &nvbits);
- OF_ScrubCTL |= (u32) nvbits << 16;
- val = mctGet_NVbits(NV_DramBKScrub);
- OF_ScrubCTL |= val;
nvbits = mctGet_NVbits(NV_L2BKScrub);
OF_ScrubCTL |= (u32) nvbits << 8;
nvbits = mctGet_NVbits(NV_DramBKScrub);
OF_ScrubCTL |= nvbits;
AllECC = 1; MemClrECC = 0;
@@ -190,7 +192,20 @@ Set_NB32(dev, 0x5C, val); /* Dram Scrub Addr Low */ val = curBase>>24; Set_NB32(dev, 0x60, val); /* Dram Scrub Addr High */
Set_NB32(dev, 0x58, OF_ScrubCTL); /*Scrub Control */ /*set dram background scrubbing to setup value */
Set_NB32(dev, 0x58, OF_ScrubCTL); /*Scrub Control *//* Divisor should not be set deeper than* divide by 16 when Dcache scrubber or* L2 scrubber is enabled.*/if ((OF_ScrubCTL & (0x1F << 16)) || (OF_ScrubCTL & (0x1F << 8))) {val = Get_NB32(dev, 0x84);if ((val & 0xE0000000) > 0x80000000) { /* Get F3x84h[31:29]ClkDivisor for C1 */val &= 0x1FFFFFFF; /* If ClkDivisor is deeper than divide-by-16 */val |= 0x80000000; /* set it to divide-by-16 */Set_NB32(dev, 0x84, val);}} } /* this node has ECC enabled dram */ } /*Node has Dram */Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctmtr_d.c
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mctmtr_d.c 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mctmtr_d.c 2008-04-07 21:43:46.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -61,15 +61,7 @@ Bottom40bIO = val; }
- val = mctGet_NVbits(NV_BottomUMA);
- if(val == 0)
val++;- val <<= (24-8);
- if(val > Bottom32bIO)
val = Bottom32bIO;- Cache32bTOP = val;
Cache32bTOP = Bottom32bIO;
/*====================================================================== Set default values for CPU registers
@@ -118,6 +110,8 @@ if(Bottom40bIO) { hi = Bottom40bIO >> 24; lo = Bottom40bIO << 8;
if (mctSetNodeBoundary_D())lo &= 0xC0000000;@@ -210,4 +204,54 @@ *pMtrrAddr = addr; }
+void UMAMemTyping_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstatA) +{ +/* UMA memory size may need splitting the MTRR configuration into two
- Before training use NB_BottomIO or the physical memory size to set the MTRRs.
- After training, add UMAMemTyping function reconfigure the MTRRs based on
Suggested wording: "...function to reconfigure..."
- NV_BottomUMA (for UMA systems only).
- This two-step process allows all memory to be cached for training
+*/
u32 Bottom32bIO, Cache32bTOP;
u32 val;
u32 addr;
u32 lo, hi;
/*======================================================================
* Adjust temp top of memory down to accomodate UMA memory start*======================================================================*//* Bottom32bIO=sub 4GB top of memory, right justified 8 bits
* (defines dram versus IO space type)* Cache32bTOP=sub 4GB top of WB cacheable memory, right justified 8 bits */Bottom32bIO = pMCTstat->Sub4GCacheTop >> 8;
val = mctGet_NVbits(NV_BottomUMA);
if (val == 0)
val++;val <<= (24-8);
if (val < Bottom32bIO) {
Cache32bTOP = val;pMCTstat->Sub4GCacheTop = val;/*======================================================================
* Clear variable MTRR values*======================================================================*/addr = 0x200;lo = 0;hi = lo;while( addr < 0x20C) {_WRMSR(addr, lo, hi); /* prog. MTRR with current region Mask */addr++; /* next MTRR pair addr */}/*======================================================================* Set variable MTRR values*======================================================================*/print_tx("\t UMAMemTyping_D: Cache32bTOP:", Cache32bTOP);SetMTRRrangeWB_D(0, &Cache32bTOP, &addr);if(addr == -1) /* ran out of MTRRs?*/pMCTstat->GStatus |= 1<<GSB_MTRRshort;}
+} Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mctsrc.c =================================================================== --- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mctsrc.c 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/mct/mctsrc.c 2008-04-07 21:43:58.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -42,10 +42,7 @@ u8 Channel, u8 Receiver, u32 dev, u32 index_reg, u8 Addl_Index, u8 Pass); -static void CalcMaxLatency_D(struct DCTStatStruc *pDCTstat,
u8 DQSRcvrEnDly, u8 Channel);static void mct_SetMaxLatency_D(struct DCTStatStruc *pDCTstat, u8 Channel, u8 DQSRcvEnDly); -static void mct_SetDQSRcvEn_D(struct DCTStatStruc *pDCTstat, u32 val); static void fenceDynTraining_D(struct MCTStatStruc *pMCTstat, struct DCTStatStruc *pDCTstat, u8 dct); static void mct_DisableDQSRcvEn_D(struct DCTStatStruc *pDCTstat); @@ -766,9 +763,8 @@ u8 *test_buf; u8 i; u8 result;
- u8 *addr_lo_buf;
- u8 value;
SetUpperFSbase(addr); // needed?
if(Pass == FirstPass) { if(pattern==1) {
@@ -780,44 +776,27 @@ test_buf = (u8 *)TestPattern2_D; }
- addr_lo_buf = (u8 *) (addr << 8);
- result = DQS_FAIL;
SetUpperFSbase(addr);
addr <<= 8;
if((pDCTstat->Status & (1<<SB_128bitmode)) && channel ) {
addr_lo_buf += 8; /* second channel */
addr += 8; /* second channel */
-#if DQS_TRAIN_DEBUG > 4
- print_debug("\t\t\t\t\t\tQW0 : test_buf = ");
- print_debug_hex32((unsigned)test_buf);
- print_debug(": ");
- print_debug_dqs_pair("\t\t\t\t\t\t test_buf = ", (u32)test_buf, " | addr_lo = ", addr, 4); for (i=0; i<8; i++) {
print_debug_hex8(test_buf[i]); print_debug(" ");- }
- print_debug("\n");
- print_debug("\t\t\t\t\t\tQW0 : addr_lo_buf = ");
- print_debug_hex32((unsigned)addr_lo_buf);
- print_debug(": ");
- for (i=0; i<8; i++) {
print_debug_hex8(addr_lo_buf[i]); print_debug(" ");- }
- print_debug("\n");
-#endif
value = read32_fs(addr);print_debug_dqs_pair("\t\t\t\t\t\t\t\t ", test_buf[i], " | ", value, 4);
- /* prevent speculative execution of following instructions */
- _EXECFENCE;
- for (i=0; i<8; i++) {
if(addr_lo_buf[i] == test_buf[i]) {
if (value == test_buf[i]) { pDCTstat->DqsRcvEn_Pass |= (1<<i);} else { pDCTstat->DqsRcvEn_Pass &= ~(1<<i); } }
result = DQS_FAIL;
if (Pass == FirstPass) { /* if first pass, at least one byte lane pass
@@ -1062,7 +1041,7 @@ Set_NB32_index_wait(dev, index_reg, 0x08, val);
/* Wait 200 MEMCLKs. */
- mct_Wait_10ns (20000); /* wait 200us */
mct_Wait(50000); /* wait 200us */
/* Clear F2x[1,0]9C_x08[PhyFenceTrEn]=0. */ val = Get_NB32_index_wait(dev, index_reg, 0x08);
@@ -1101,21 +1080,20 @@ }
-static void mct_Wait_10ns (u32 cycles) +static void mct_Wait(u32 cycles) {
- u32 saved, i;
- u32 saved; u32 hi, lo, msr;
- /* cycles = number of 10ns cycles(or longer) to delay */
- /* FIXME: Need to calibrate to CPU/NCLK speed? */
- /* Wait # of 50ns cycles
This seems like a hack to me... */
- msr = 0x10; /* TSC */
- for (i = 0; i < cycles; i++) {
_RDMSR(msr, &lo, &hi);saved = lo;
- cycles <<= 3; /* x8 (number of 1.25ns ticks) */
do {_RDMSR(msr, &lo, &hi);} while (lo - saved < 8); /* 8 x 1.25 ns as NCLK is at 1.25ns */- }
- msr = 0x10; /* TSC */
- _RDMSR(msr, &lo, &hi);
- saved = lo;
- do {
_RDMSR(msr, &lo, &hi);- } while (lo - saved < cycles );
} Index: coreboot-v2/src/northbridge/amd/amdmct/wrappers/mcti_d.c =================================================================== --- coreboot-v2.orig/src/northbridge/amd/amdmct/wrappers/mcti_d.c 2008-04-07 15:56:37.000000000 -0600 +++ coreboot-v2/src/northbridge/amd/amdmct/wrappers/mcti_d.c 2008-04-07 16:14:36.000000000 -0600 @@ -1,7 +1,7 @@ /*
- This file is part of the coreboot project.
- Copyright (C) 2007 Advanced Micro Devices, Inc.
- Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
@@ -129,11 +129,11 @@ //val = 1; /* enable */ break; case NV_BottomIO:
val = 0xC0; /* address bits [31:24] */
val = 0xE0; /* address bits [31:24] */#if (UMA_SUPPORT == 0)
val = 0xC0; /* address bits [31:24] */
val = 0xE0; /* address bits [31:24] */#elif (UMA_SUPPORT == 1) val = 0xB0; /* address bits [31:24] */ #endif @@ -205,7 +205,7 @@ val = 1; /* Enabled */ //val = 0; /* Disabled */ case NV_ChannelIntlv:
val = 5; /* Disabled */ /* Not currently checked in mctchi_d.c */
val = 5; /* Not currently checked in mctchi_d.c */
1 - Enable- Bits[2:1] = 00b - Address bits 6
@@ -336,3 +336,8 @@ { return mctGetLogicalCPUID(node); }
+u8 mctSetNodeBoundary_D(void) +{
- return 0;
+}
Overall, I have to admit that I'm not familiar enough with the memory controller to be able to perform a perfect review. However, the patch seems to add comments where appropriate and the changes look solid.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 09.04.2008 00:28, Marc Jones wrote:
Bring Fam10 memory controller init up to date with the latest AMD BKDG recomendations.
Changes include the following: fix > 4GB dqs tests fix channel interleaving fix memory hoisting across nodes (memory hole was always on node0) ecc memory scrub updates debug print changes
Signed-off-by: Marc Jones (marc.jones@amd.com)
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net with some comments, though.
comments addressed. Committed revision 3232.
Thanks, Marc
-
Carl-Daniel Hailfinger -
David Hendricks -
Marc Jones -
Marc Jones
-
Peter Stuge -
Russell Whitaker -
Stefan Reinauer -
Uwe Hermann