Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Doug Thompson]

--- On Wed, 4/29/09, Ingo Molnar <mingo@elte.hu> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Subject: Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3
> To: "Andrew Morton" <akpm@linux-foundation.org>
> Cc: torvalds@linux-foundation.org, borislav.petkov@amd.com, greg@kroah.com, tglx@linutronix.de, hpa@zytor.com, dougthompson@xmission.com, linux-kernel@vger.kernel.org
> Date: Wednesday, April 29, 2009, 1:53 PM
> 
> * Andrew Morton <akpm@linux-foundation.org>
> wrote:
> 
> > On Wed, 29 Apr 2009 21:23:26 +0200
> > Ingo Molnar <mingo@elte.hu>
> wrote:
> > 
> > > > > > +   
>     if (CSFound >= 0) {
> > > > > > +   
>         *node_id = NodeID;
> > > > > > +   
>         *channel_select =
> ChannelSelect;
> > > > > > +   
>     }
> > > > > > +    }
> > > > > > +
> > > > > > +    return
> CSFound;
> > > > > > +}
> > > > > 
> > > > > this function is probably too large,
> and also it uses some weird 
> > > > > hungarian notation coding style. Please
> dont do that! It's 
> > > > > completely unacceptable.
> > > > 
> > > > These identifers (or at least,
> DctSelBaseOffsetLong, which is the 
> > > > only one I googled for) come straight out of
> the AMD "BIOS and 
> > > > Kernel Developer's Guide".
> > > > 
> > > > Sucky though they are, there's value in
> making the kernel code 
> > > > match up with the documentation.
> > > 
> > > I'm generally resisting patches that hungarinize
> arch/x86/ (and heck 
> > > there's been many attempts ...) but there's some
> conflicting advice 
> > > here. I've Cc:-ed Linus, maybe he has an opinion
> about this.
> > > 
> > > My gut reaction would be 'hell no'. There's
> other, structural 
> > > problems with this code too, and doing some saner
> naming would 
> > > mostly be a sed job and would take minimal amount
> of time. The 
> > > naming can still be intuitive. The symbols from
> the documentation 
> > > can perhaps be mentioned in a couple of comments
> to establish a 
> > > mapping.
> > 
> > I think I disagree.  For those identifiers which
> map 1:1 with the 
> > manufacturer's document, the ugliness involved in
> exactly copying 
> > the manufacturer's chosen identifiers is outweighed by
> the benefit 
> > of exactly copying the manufacturer's chosen
> identifiers.
> > 
> > Of course, we don't have to use StinkyIdentifiers
> anywhere else.  
> > And the nice thing about that is that when one reads
> the code and 
> > comes across a StinkyIdentifier, one immeditely knows
> that it's an 
> > AMD-provided thing rather than a Linux-provided
> thing.
> > 
> > Zillions of StinkyIdentifiers get merged via this
> logic.
> 
> Andrew, for heaven's sake, please review the patchset - as
> i did.
> 
> The thing is, up to 12/21, the patches look like normal
> Linux 
> patches. (there's problems with them too, but on a
> different level)
> 
> Then do the StinkyIdentifiers show up, in full force:
> 
> +static int f10_match_to_this_node(struct amd64_pvt *pvt,
> int DramRange,
> +               
>            
>    u64 SystemAddr,
> +               
>            
>    int *node_id,
> +               
>            
>    int *channel_select)
> +{
> +       int CSFound = -1;
> +       int NodeID;
> +       int HiRangeSelected;
> +       u32 IntlvEn, IntlvSel;
> +       u32 DramEn;
> +       u32 Ilog;
> +    u32 HoleOffset, HoleEn;
> +       u32 InputAddr, Temp;
> +       u32 DctSelBaseAddr,
> DctSelIntLvAddr;
> +       u32 DctSelHi;
> +       u32 ChannelSelect;
> +       u64 DramBaseLong,
> DramLimitLong;
> +    u64 DctSelBaseOffsetLong,
> ChannelAddrLong;
> 
> Tell me, how is 'SystemAddr' or 'Temp' or 'Ilog' an AMD
> document 
> thing?
> 
> I have a much simpler explanation really: someone got
> really bored 
> at converting some code written For Another OS, somewhere
> in the 
> middle - and started plopping Other OS Code into a Linux
> driver ...
> 
> I dont mind the occasional _constant_ that tells us a hw
> API detail 
> in whatever externally dictated style - but this thing
> stinks 
> HeadToToe ... ;-)
> 
>     Ingo
> 

I think I didn't reply to ALL on this, so sorry for a possible repost.

Right from the BKDG from the AMD website is the following reference code.

The format is lost in the mailing, but it IS the AMD reference code. This code also lacks the fixes for 2 bugs I found and which AMD emailed me and which is in the driver patches.  I did do some refactoring and tried to break the monolithic monster into a main function with supporting functions, I think this adds to the understanding somewhat. 

So, I am looking for what IS the policy for utilizing mfger's reference code in a linux driver?

Keeping the StinkyIdentifiers does help when referencing the the reference code. Maybe with Boris' help, we can get AMD to update their document with a better linux version of the reference code:

doug t


31116 Rev 3.06 - March 26, 2008 AMD Family 10h Processor BKDG
Page 67

(int,int,int,int) TranslateSysAddrToCS((uint64)SystemAddr){
int SwapDone, BadDramCs;
int CSFound, NodeID, CS, F1Offset, F2Offset, F2MaskOffset, Ilog, device;
int HiRangeSelected, DramRange;
uint32 IntlvEn, IntlvSel;
uint32 DramBaseLow, DramLimitLow, DramEn;
uint32 HoleOffset, HoleEn;
uint32 CSBase, CSLimit, CSMask, CSEn;
uint32 InputAddr, Temp;
uint32 OnlineSpareCTL;
uint32 DctSelBaseAddr, DctSelIntLvAddr, DctGangEn, DctSelIntLvEn;
uint32 DctSelHiRngEn,DctSelHi;
uint64 DramBaseLong, DramLimitLong;
uint64 DctSelBaseOffsetLong, ChannelOffsetLong,ChannelAddrLong;
// device is a user supplied value for the PCI device ID of the processor
// from which CSRs are initially read from (current processor is fastest).
// CH0SPARE_RANK and CH1SPARE_RANK are user supplied values, determined
// by BIOS during DIMM sizing.
CSFound = 0;
for(DramRange = 0; DramRange < 8; DramRange++)
{
F1Offset = 0x40 + (DramRange << 3);
DramBaseLow = Get_PCI(bus0, device, func1, F1Offset);
DramEn = DramBaseLow & 0x00000003;

IntlvEn = (DramBaseLow & 0x00000700) >> 8;
DramBaseLow = DramBaseLow & 0xFFFF0000;
DramBaseLong = ((Get_PCI(bus0, device, func1, F1Offset + 0x100))<<32 +
DramBaseLow)<<8;
DramLimitLow = Get_PCI(bus0, device, func1, F1Offset + 4);
NodeID = DramLimitLow & 0x00000007;
IntlvSel = (DramLimitLow & 0x00000700) >> 8;
DramLimitLow = DramLimitLow | 0x0000FFFF;
DramLimitLong = ((Get_PCI(bus0, device, func1, F1Offset + 0x104))<<32 +
DramLimitLow)<<8 | 0xFF;
HoleEn = Get_PCI(bus0, dev24 + NodeID, func1, 0xF0);
HoleOffset = (HoleEn & 0x0000FF80);
HoleEn = (HoleEn &0x00000003);
if(DramEn && DramBaseLong<=SystemAddr && SystemAddr <= DramLimitLong)
{
if(IntlvEn == 0 || IntlvSel == ((SystemAddr >> 12) & IntlvEn))
{
if(IntlvEn == 1) Ilog = 1;
else if(IntlvEn == 3) Ilog = 2;
else if(IntlvEn == 7) Ilog = 3;
else Ilog = 0;
Temp = Get_PCI(bus0, device, func2, 0x110);
DctSelHiRngEn = Temp & 1;
DctSelHi = Temp>>1 & 1;
DctSelIntLvEn = Temp & 4;
DctGangEn = Temp & 0x10;
DctSelIntLvAddr = (Temp>>6) & 3;
DctSelBaseAddr = Temp & 0xFFFFF800;
DctSelBaseOffsetLong = Get_PCI(bus0, device, func2, 0x114)<<16;
//Determine if High range is selected
if(DctSelHiRngEn && DctGangEn==0 && (SystemAddr>>27) >=
(DctSelBaseAddr>>11)) HiRangeSelected = 1;
else HiRangeSelected=0;
//Determine Channel
if(DctGangEn) ChannelSelect = 0;
else if (HiRangeSelected) ChannelSelect = DctSelHi;
else if (DctSelIntLvEn && DctSelIntLvAddr == 0)
ChannelSelect = SystemAddr>>6 & 1;
else if (DctSelIntLvEn && DctSelIntLvAddr>>1 & 1)
{
Temp = fUnaryXOR(SystemAddr>>16&0x1F); //function returns odd parity
//1= number of set bits in argument is odd.
//0= number of set bits in argument is even.
if(DctSelIntLvAddr & 1) ChannelSelect = (SystemAddr>>9 & 1)^Temp;
else ChannelSelect = (SystemAddr>>6 & 1)^Temp;
}
else if (DctSelIntLvEn && IntlvEn&4)ChannelSelect = SystemAddr>>15&1;
else if (DctSelIntLvEn && IntlvEn&2)ChannelSelect = SystemAddr>>14&1;
else if (DctSelIntLvEn && IntlvEn&1)ChannelSelect = SystemAddr>>13&1;
else if (DctSelIntLvEn) ChannelSelect = SystemAddr>>12&1;
else if (DctSelHiRngEn && DctGangEn==0) ChannelSelect = ~DctSelHi&1;
else ChannelSelect = 0;
//Determine Base address Offset to use
if(HiRangeSelected)
{
if(!(DctSelBaseAddr & 0xFFFF0000) && (HoleEn & 1) &&
(SystemAddr >= 0x1_00000000))
ChannelOffsetLong = HoleOffset<<16;
31116 Rev 3.06 - March 26, 2008 AMD Family 10h Processor BKDG
68
else
ChannelOffsetLong= DctSelBaseOffsetLong;
}
else
{
if((HoleEn & 1) && (SystemAddr >= 0x1_00000000))
ChannelOffsetLong = HoleOffset<<16;
else
ChannelOffsetLong = DramBaseLong & 0xFFFF_F8000000;
}
//Remove hoisting offset and normalize to DRAM bus addresses
ChannelAddrLong = SystemAddr & 0x0000FFFF_FFFFFFC0 -
ChannelOffsetLong & 0x0000FFFF_FF800000;
//Remove Node ID (in case of processor interleaving)
Temp = ChannelAddrLong & 0xFC0;
ChannelAddrLong = (ChannelAddrLong >>Ilog & 0xFFFF_FFFFF000)|Temp;
//Remove Channel interleave and hash
if(DctSelIntLvEn && DctSelHiRngEn==0 && DctGangEn==0)
{
if(DctSelIntLvAddr & 1 != 1)
ChannelAddrLong = (ChannelAddrLong>>1) & 0xFFFFFFFF_FFFFFFC0;
else if(DctSelIntLvAddr == 1)
{
Temp = ChannelAddrLong & 0xFC0;
ChannelAddrLong = ((ChannelAddrLong & 0xFFFFFFFF_FFFFE000) >> 1)| Temp;
}
else
{
Temp = ChannelAddrLong & 0x1C0;
ChannelAddrLong = ((ChannelAddrLong & 0xFFFFFFFF_FFFFFC00) >> 1)| Temp;
}
InputAddr = ChannelAddrLong>>8;
for(CS = 0; CS < 8; CS++)
{
F2Offset = 0x40 + (CS << 2);
if ((CS % 2) == 0)
F2MaskOffset = 0x60 + (CS << 1);
else
F2MaskOffset = 0x60 + ((CS-1) << 1);
if(ChannelSelect)
{
F2Offset+=0x100;
F2MaskOffset+=0x100;
}
CSBase = Get_PCI(bus0, dev24 + NodeID, func2, F2Offset);
CSEn = CSBase & 0x00000001;
CSBase = CSBase & 0x1FF83FE0;
CSMask = Get_PCI(bus0, dev24 + NodeID, func2, F2MaskOffset);
CSMask = (CSMask | 0x0007C01F) & 0x1FFFFFFF;
if(CSEn && ((InputAddr & ~CSMask) == (CSBase & ~CSMask)))
{
CSFound = 1;
OnlineSpareCTL = Get_PCI(bus0, dev24 + NodeID, func3, 0xB0);
if(ChannelSelect)
{
SwapDone = (OnlineSpareCTL >> 3) & 0x00000001;
BadDramCs = (OnlineSpareCTL >> 8) & 0x00000007;
if(SwapDone && CS == BadDramCs) CS=CH1SPARE_RANK;
}
else
31116 Rev 3.06 - March 26, 2008 AMD Family 10h Processor BKDG
69
{
SwapDone = (OnlineSpareCTL >> 1) & 0x00000001;
BadDramCs = (OnlineSpareCTL >> 4) & 0x00000007;
if(SwapDone && CS == BadDramCs) CS=CH0SPARE_RANK;
}
break;
}
}
}
}
if(CSFound) break;
} // for each DramRange
return(CSFound,NodeID,ChannelSelect,CS);

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Doug Thompson]

--- On Mon, 5/4/09, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:

> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Subject: Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3
> To: "Borislav Petkov" <borislav.petkov@amd.com>
> Cc: akpm@linux-foundation.org, greg@kroah.com, mingo@elte.hu, tglx@linutronix.de, hpa@zytor.com, dougthompson@xmission.com, linux-kernel@vger.kernel.org
> Date: Monday, May 4, 2009, 5:36 PM
> Borislav Petkov escreveu:
> > From: Doug Thompson <dougthompson@xmission.com>
> > 
> > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> > ---
> >  drivers/edac/amd64_edac.c |  318
> +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 318 insertions(+), 0
> deletions(-)
> > 
> > diff --git a/drivers/edac/amd64_edac.c
> b/drivers/edac/amd64_edac.c
> > index fe2342c..84075c0 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -2726,4 +2726,322 @@ static int
> f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32
> ChannelSelect)
> >      return CSFound;
> >  }
> >  +/*
> > + * f10_match_to_this_node
> > + *
> > + * For a given 'DramRange' value, check if
> 'SystemAddr' fall within this value
> > + */
> > +static int f10_match_to_this_node(struct amd64_pvt
> *pvt, int DramRange,
> > +       
>         u64 SystemAddr,
> > +       
>         int *node_id,
> > +       
>         int *channel_select)
> > +{
> > +    int CSFound = -1;
> >   
> 
> As in the previous patch, please use a standard error code,
> instead of -1.
> 
> > +static int f10_translate_sysaddr_to_CS(struct
> amd64_pvt *pvt,
> > +       
>     u64 SysAddr,
> > +       
>     int *node,
> > +       
>     int *chanSel)
> > +{
> > +    int DramRange;
> > +    int CSFound = -1;
> >   
> Same here.
> 
> Cheers,
> Mauro
> 

Code was reference code from AMD. Probably safe to convert to use -EINVAL yes

doug t

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Mauro Carvalho Chehab]

Borislav Petkov escreveu:
> From: Doug Thompson <dougthompson@xmission.com>
>
> Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  drivers/edac/amd64_edac.c |  318 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 318 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index fe2342c..84075c0 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2726,4 +2726,322 @@ static int f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32 ChannelSelect)
>  	return CSFound;
>  }
>  
> +/*
> + * f10_match_to_this_node
> + *
> + * For a given 'DramRange' value, check if 'SystemAddr' fall within this value
> + */
> +static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
> +				u64 SystemAddr,
> +				int *node_id,
> +				int *channel_select)
> +{
> +	int CSFound = -1;
>   

As in the previous patch, please use a standard error code, instead of -1.

> +static int f10_translate_sysaddr_to_CS(struct amd64_pvt *pvt,
> +			u64 SysAddr,
> +			int *node,
> +			int *chanSel)
> +{
> +	int DramRange;
> +	int CSFound = -1;
>   
Same here.

Cheers,
Mauro

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Ingo Molnar]

* Borislav Petkov <borislav.petkov@amd.com> wrote:

> Hi,
> 
> On Wed, Apr 29, 2009 at 10:47:30PM +0200, Ingo Molnar wrote:
> 
> [..]
> 
> > What i point out below is precisely what you say is ineligible 
> > under:
> > 
> > > > Of course, we don't have to use StinkyIdentifiers anywhere else.  
> > 
> > I'd extend that rule to say that StinkyIdentifiers should only be 
> > used for hw API definitions/constants - macros, enums - not really 
> > local variable names. The moment they are allowed into local 
> > variables the stuff below happens.
> 
> to agree with Andrew, at a certain point in time I thought that 
> having the same register bit names as in the docs would be 
> preferential when you look at the docs and what the code does. But 
> Ingo's also quite right: we can't have "normal kernel coding 
> style" and StinkyIdentifiers
> :) in the same source file.
> 
> /me locking himself back in the patch creation basement.

I think you can still cleanly use those identifiers for hardware 
constants, register offsets and similar. But if it shows up in a 
variable (or function) name, it has spread too far IMHO :-)

And it's not like we dont have our own historic mistakes in that 
area, right in the heart of Linux - just type:

  git grep Page mm/*.c

and cringe.

IIRC i might even have added a new method or two to that array of 
CrappyPageAPIs, many years ago. (back in the days when i wrote lot 
of crappy code myself ;-) Oh, PageHighMem() it is.

	Ingo

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Borislav Petkov]

Hi,

On Wed, Apr 29, 2009 at 10:47:30PM +0200, Ingo Molnar wrote:

[..]

> What i point out below is precisely what you say is ineligible 
> under:
> 
> > > Of course, we don't have to use StinkyIdentifiers anywhere else.  
> 
> I'd extend that rule to say that StinkyIdentifiers should only be 
> used for hw API definitions/constants - macros, enums - not really 
> local variable names. The moment they are allowed into local 
> variables the stuff below happens.

to agree with Andrew, at a certain point in time I thought that having
the same register bit names as in the docs would be preferential when
you look at the docs and what the code does. But Ingo's also quite
right: we can't have "normal kernel coding style" and StinkyIdentifiers
:) in the same source file.

/me locking himself back in the patch creation basement.

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 29 Apr 2009 21:23:26 +0200
> > Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > > > > +		if (CSFound >= 0) {
> > > > > > +			*node_id = NodeID;
> > > > > > +			*channel_select = ChannelSelect;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	return CSFound;
> > > > > > +}
> > > > > 
> > > > > this function is probably too large, and also it uses some weird 
> > > > > hungarian notation coding style. Please dont do that! It's 
> > > > > completely unacceptable.
> > > > 
> > > > These identifers (or at least, DctSelBaseOffsetLong, which is the 
> > > > only one I googled for) come straight out of the AMD "BIOS and 
> > > > Kernel Developer's Guide".
> > > > 
> > > > Sucky though they are, there's value in making the kernel code 
> > > > match up with the documentation.
> > > 
> > > I'm generally resisting patches that hungarinize arch/x86/ (and heck 
> > > there's been many attempts ...) but there's some conflicting advice 
> > > here. I've Cc:-ed Linus, maybe he has an opinion about this.
> > > 
> > > My gut reaction would be 'hell no'. There's other, structural 
> > > problems with this code too, and doing some saner naming would 
> > > mostly be a sed job and would take minimal amount of time. The 
> > > naming can still be intuitive. The symbols from the documentation 
> > > can perhaps be mentioned in a couple of comments to establish a 
> > > mapping.
> > 
> > I think I disagree.  For those identifiers which map 1:1 with the 
> > manufacturer's document, the ugliness involved in exactly copying 
> > the manufacturer's chosen identifiers is outweighed by the benefit 
> > of exactly copying the manufacturer's chosen identifiers.
> > 
> > Of course, we don't have to use StinkyIdentifiers anywhere else.  
> > And the nice thing about that is that when one reads the code and 
> > comes across a StinkyIdentifier, one immeditely knows that it's an 
> > AMD-provided thing rather than a Linux-provided thing.
> > 
> > Zillions of StinkyIdentifiers get merged via this logic.
> 
> Andrew, for heaven's sake, please review the patchset - as i did.

Let me apologize for this rude reply ... it appears we do agree, i 
just didnt properly read your paragraphs above :-/

What i point out below is precisely what you say is ineligible 
under:

> > Of course, we don't have to use StinkyIdentifiers anywhere else.  

I'd extend that rule to say that StinkyIdentifiers should only be 
used for hw API definitions/constants - macros, enums - not really 
local variable names. The moment they are allowed into local 
variables the stuff below happens.

Thanks,

	Ingo

> 
> The thing is, up to 12/21, the patches look like normal Linux 
> patches. (there's problems with them too, but on a different level)
> 
> Then do the StinkyIdentifiers show up, in full force:
> 
> +static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
> +                               u64 SystemAddr,
> +                               int *node_id,
> +                               int *channel_select)
> +{
> +       int CSFound = -1;
> +       int NodeID;
> +       int HiRangeSelected;
> +       u32 IntlvEn, IntlvSel;
> +       u32 DramEn;
> +       u32 Ilog;
> +	u32 HoleOffset, HoleEn;
> +       u32 InputAddr, Temp;
> +       u32 DctSelBaseAddr, DctSelIntLvAddr;
> +       u32 DctSelHi;
> +       u32 ChannelSelect;
> +       u64 DramBaseLong, DramLimitLong;
> +	u64 DctSelBaseOffsetLong, ChannelAddrLong;
> 
> Tell me, how is 'SystemAddr' or 'Temp' or 'Ilog' an AMD document 
> thing?
> 
> I have a much simpler explanation really: someone got really bored 
> at converting some code written For Another OS, somewhere in the 
> middle - and started plopping Other OS Code into a Linux driver ...
> 
> I dont mind the occasional _constant_ that tells us a hw API detail 
> in whatever externally dictated style - but this thing stinks 
> HeadToToe ... ;-)
> 
> 	Ingo

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 29 Apr 2009 21:23:26 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > > > +		if (CSFound >= 0) {
> > > > > +			*node_id = NodeID;
> > > > > +			*channel_select = ChannelSelect;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return CSFound;
> > > > > +}
> > > > 
> > > > this function is probably too large, and also it uses some weird 
> > > > hungarian notation coding style. Please dont do that! It's 
> > > > completely unacceptable.
> > > 
> > > These identifers (or at least, DctSelBaseOffsetLong, which is the 
> > > only one I googled for) come straight out of the AMD "BIOS and 
> > > Kernel Developer's Guide".
> > > 
> > > Sucky though they are, there's value in making the kernel code 
> > > match up with the documentation.
> > 
> > I'm generally resisting patches that hungarinize arch/x86/ (and heck 
> > there's been many attempts ...) but there's some conflicting advice 
> > here. I've Cc:-ed Linus, maybe he has an opinion about this.
> > 
> > My gut reaction would be 'hell no'. There's other, structural 
> > problems with this code too, and doing some saner naming would 
> > mostly be a sed job and would take minimal amount of time. The 
> > naming can still be intuitive. The symbols from the documentation 
> > can perhaps be mentioned in a couple of comments to establish a 
> > mapping.
> 
> I think I disagree.  For those identifiers which map 1:1 with the 
> manufacturer's document, the ugliness involved in exactly copying 
> the manufacturer's chosen identifiers is outweighed by the benefit 
> of exactly copying the manufacturer's chosen identifiers.
> 
> Of course, we don't have to use StinkyIdentifiers anywhere else.  
> And the nice thing about that is that when one reads the code and 
> comes across a StinkyIdentifier, one immeditely knows that it's an 
> AMD-provided thing rather than a Linux-provided thing.
> 
> Zillions of StinkyIdentifiers get merged via this logic.

Andrew, for heaven's sake, please review the patchset - as i did.

The thing is, up to 12/21, the patches look like normal Linux 
patches. (there's problems with them too, but on a different level)

Then do the StinkyIdentifiers show up, in full force:

+static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
+                               u64 SystemAddr,
+                               int *node_id,
+                               int *channel_select)
+{
+       int CSFound = -1;
+       int NodeID;
+       int HiRangeSelected;
+       u32 IntlvEn, IntlvSel;
+       u32 DramEn;
+       u32 Ilog;
+	u32 HoleOffset, HoleEn;
+       u32 InputAddr, Temp;
+       u32 DctSelBaseAddr, DctSelIntLvAddr;
+       u32 DctSelHi;
+       u32 ChannelSelect;
+       u64 DramBaseLong, DramLimitLong;
+	u64 DctSelBaseOffsetLong, ChannelAddrLong;

Tell me, how is 'SystemAddr' or 'Temp' or 'Ilog' an AMD document 
thing?

I have a much simpler explanation really: someone got really bored 
at converting some code written For Another OS, somewhere in the 
middle - and started plopping Other OS Code into a Linux driver ...

I dont mind the occasional _constant_ that tells us a hw API detail 
in whatever externally dictated style - but this thing stinks 
HeadToToe ... ;-)

	Ingo

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Andrew Morton]

On Wed, 29 Apr 2009 21:23:26 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> > > > +		if (CSFound >= 0) {
> > > > +			*node_id = NodeID;
> > > > +			*channel_select = ChannelSelect;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return CSFound;
> > > > +}
> > > 
> > > this function is probably too large, and also it uses some weird 
> > > hungarian notation coding style. Please dont do that! It's 
> > > completely unacceptable.
> > 
> > These identifers (or at least, DctSelBaseOffsetLong, which is the 
> > only one I googled for) come straight out of the AMD "BIOS and 
> > Kernel Developer's Guide".
> > 
> > Sucky though they are, there's value in making the kernel code 
> > match up with the documentation.
> 
> I'm generally resisting patches that hungarinize arch/x86/ (and heck 
> there's been many attempts ...) but there's some conflicting advice 
> here. I've Cc:-ed Linus, maybe he has an opinion about this.
> 
> My gut reaction would be 'hell no'. There's other, structural 
> problems with this code too, and doing some saner naming would 
> mostly be a sed job and would take minimal amount of time. The 
> naming can still be intuitive. The symbols from the documentation 
> can perhaps be mentioned in a couple of comments to establish a 
> mapping.

I think I disagree.  For those identifiers which map 1:1 with the
manufacturer's document, the ugliness involved in exactly copying the
manufacturer's chosen identifiers is outweighed by the benefit of
exactly copying the manufacturer's chosen identifiers.

Of course, we don't have to use StinkyIdentifiers anywhere else.  And
the nice thing about that is that when one reads the code and comes
across a StinkyIdentifier, one immeditely knows that it's an
AMD-provided thing rather than a Linux-provided thing.

Zillions of StinkyIdentifiers get merged via this logic.

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 29 Apr 2009 20:22:55 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > +		InputAddr = ChannelAddrLong >> 8;
> > > +
> > > +		debugf1("   (ChannelAddrLong=0x%llx) >> 8 becomes "
> > > +			"InputAddr=0x%x\n", ChannelAddrLong, InputAddr);
> > > +
> > > +		/* Iterate over the DRAM DCTs looking for a
> > > +		 * match for InputAddr on the selected NodeID
> > > +		 */
> > > +		CSFound = f10_lookup_addr_in_dct(InputAddr,
> > > +						NodeID, ChannelSelect);
> > > +
> > > +		if (CSFound >= 0) {
> > > +			*node_id = NodeID;
> > > +			*channel_select = ChannelSelect;
> > > +		}
> > > +	}
> > > +
> > > +	return CSFound;
> > > +}
> > 
> > this function is probably too large, and also it uses some weird 
> > hungarian notation coding style. Please dont do that! It's 
> > completely unacceptable.
> 
> These identifers (or at least, DctSelBaseOffsetLong, which is the 
> only one I googled for) come straight out of the AMD "BIOS and 
> Kernel Developer's Guide".
> 
> Sucky though they are, there's value in making the kernel code 
> match up with the documentation.

I'm generally resisting patches that hungarinize arch/x86/ (and heck 
there's been many attempts ...) but there's some conflicting advice 
here. I've Cc:-ed Linus, maybe he has an opinion about this.

My gut reaction would be 'hell no'. There's other, structural 
problems with this code too, and doing some saner naming would 
mostly be a sed job and would take minimal amount of time. The 
naming can still be intuitive. The symbols from the documentation 
can perhaps be mentioned in a couple of comments to establish a 
mapping.

	Ingo

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Andrew Morton]

On Wed, 29 Apr 2009 20:22:55 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> > +		InputAddr = ChannelAddrLong >> 8;
> > +
> > +		debugf1("   (ChannelAddrLong=0x%llx) >> 8 becomes "
> > +			"InputAddr=0x%x\n", ChannelAddrLong, InputAddr);
> > +
> > +		/* Iterate over the DRAM DCTs looking for a
> > +		 * match for InputAddr on the selected NodeID
> > +		 */
> > +		CSFound = f10_lookup_addr_in_dct(InputAddr,
> > +						NodeID, ChannelSelect);
> > +
> > +		if (CSFound >= 0) {
> > +			*node_id = NodeID;
> > +			*channel_select = ChannelSelect;
> > +		}
> > +	}
> > +
> > +	return CSFound;
> > +}
> 
> this function is probably too large, and also it uses some weird 
> hungarian notation coding style. Please dont do that! It's 
> completely unacceptable.

These identifers (or at least, DctSelBaseOffsetLong, which is the only
one I googled for) come straight out of the AMD "BIOS and Kernel
Developer's Guide".

Sucky though they are, there's value in making the kernel code match up
with the documentation.

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Borislav Petkov <borislav.petkov@amd.com> wrote:
> 
> > From: Doug Thompson <dougthompson@xmission.com>
> > 
> > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> > ---
> >  drivers/edac/amd64_edac.c |  318 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 318 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> > index fe2342c..84075c0 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -2726,4 +2726,322 @@ static int f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32 ChannelSelect)
> >  	return CSFound;
> >  }
> >  
> > +/*
> > + * f10_match_to_this_node
> > + *
> > + * For a given 'DramRange' value, check if 'SystemAddr' fall within this value
> > + */
> > +static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
> > +				u64 SystemAddr,
> > +				int *node_id,
> > +				int *channel_select)
> > +{
> > +	int CSFound = -1;
> > +	int NodeID;
> > +	int HiRangeSelected;
> > +	u32 IntlvEn, IntlvSel;
> > +	u32 DramEn;
> > +	u32 Ilog;
> > +	u32 HoleOffset, HoleEn;
> > +	u32 InputAddr, Temp;
> > +	u32 DctSelBaseAddr, DctSelIntLvAddr;
> > +	u32 DctSelHi;
> > +	u32 ChannelSelect;
> > +	u64 DramBaseLong, DramLimitLong;
> > +	u64 DctSelBaseOffsetLong, ChannelAddrLong;
> > +
> > +	/* DRAM Base value for this DRAM instance */
> > +	DramBaseLong = pvt->dram_base[DramRange];
> > +	DramEn = pvt->dram_rw_en[DramRange];
> > +	IntlvEn = pvt->dram_IntlvEn[DramRange];
> > +
> > +	/* DRAM Limit value for this DRAM instance */
> > +	DramLimitLong = pvt->dram_limit[DramRange];
> > +	NodeID = pvt->dram_DstNode[DramRange];
> > +	IntlvSel = pvt->dram_IntlvSel[DramRange];
> > +
> > +	debugf1("%s(dram=%d) Base=0x%llx SystemAddr= 0x%llx Limit=0x%llx\n",
> > +		__func__, DramRange, DramBaseLong, SystemAddr, DramLimitLong);
> > +
> > +	/* This assumes that one node's DHAR is the same as
> > +	 * all the other node's DHARs
> > +	 */
> > +	HoleEn = pvt->dhar;
> > +	HoleOffset = (HoleEn & 0x0000FF80);
> > +	HoleEn = (HoleEn & 0x00000003);
> > +
> > +	debugf1("   HoleOffset=0x%x  HoleEn=0x%x IntlvSel=0x%x\n",
> > +			HoleOffset, HoleEn, IntlvSel);
> > +
> > +	if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {
> > +
> > +		Ilog = f10_map_IntlvEn_to_shift(IntlvEn);
> > +
> > +		Temp = pvt->dram_ctl_select_low;
> > +		DctSelBaseOffsetLong = pvt->dram_ctl_select_high << 16;
> > +
> > +		DctSelHi = (Temp >> 1) & 1;
> > +		DctSelIntLvAddr = dct_sel_interleave_addr(pvt);
> > +		DctSelBaseAddr = dct_sel_baseaddr(pvt);
> > +
> > +		if (dct_high_range_enabled(pvt) &&
> > +		   !dct_ganging_enabled(pvt) &&
> > +		   ((SystemAddr >> 27) >= (DctSelBaseAddr >> 11)))
> > +			HiRangeSelected = 1;
> > +		else
> > +			HiRangeSelected = 0;
> > +
> > +		ChannelSelect = f10_determine_channel(pvt, SystemAddr,
> > +						HiRangeSelected, IntlvEn);
> > +
> > +		ChannelAddrLong = f10_determine_base_addr_offset(
> > +						SystemAddr,
> > +						HiRangeSelected,
> > +						DctSelBaseAddr,
> > +						DctSelBaseOffsetLong,
> > +						HoleEn,
> > +						HoleOffset,
> > +						DramBaseLong);
> > +
> > +		/* Remove Node ID (in case of processor interleaving) */
> > +		Temp = ChannelAddrLong & 0xFC0;
> > +
> > +		ChannelAddrLong = ((ChannelAddrLong >> Ilog) &
> > +					0xFFFFFFFFF000ULL) | Temp;
> > +
> > +		/* Remove Channel interleave and hash */
> > +		if (dct_interleave_enabled(pvt) &&
> > +		   !dct_high_range_enabled(pvt) &&
> > +		   !dct_ganging_enabled(pvt)) {
> > +			if (DctSelIntLvAddr != 1)
> > +				ChannelAddrLong =
> > +					(ChannelAddrLong >> 1) &
> > +					0xFFFFFFFFFFFFFFC0ULL;
> > +			else {
> > +				Temp = ChannelAddrLong & 0xFC0;
> > +				ChannelAddrLong =
> > +					((ChannelAddrLong &
> > +					0xFFFFFFFFFFFFC000ULL)
> > +					>> 1) | Temp;
> > +			}
> > +		}
> > +
> > +		/* Form a normalize InputAddr (Move bits 36:8 down to 28:0
> > +		 * which will set it up to match the DCT Base register
> > +		 */
> > +		InputAddr = ChannelAddrLong >> 8;
> > +
> > +		debugf1("   (ChannelAddrLong=0x%llx) >> 8 becomes "
> > +			"InputAddr=0x%x\n", ChannelAddrLong, InputAddr);
> > +
> > +		/* Iterate over the DRAM DCTs looking for a
> > +		 * match for InputAddr on the selected NodeID
> > +		 */
> > +		CSFound = f10_lookup_addr_in_dct(InputAddr,
> > +						NodeID, ChannelSelect);
> > +
> > +		if (CSFound >= 0) {
> > +			*node_id = NodeID;
> > +			*channel_select = ChannelSelect;
> > +		}
> > +	}
> > +
> > +	return CSFound;
> > +}
> 
> this function is probably too large, and also it uses some weird 
> hungarian notation coding style. Please dont do that! It's 
> completely unacceptable.
> 
> this condition:
> 
> > +	if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {
> 
> could be inverted and an early "return cs_found" could be done - 
> saving an indentitation level for most of the above code.
> 
> etc. etc.
> 
> Please look at the function in a really large xterm, from far 
> away. If the shape does not look 'good', and the structure is not 
> an obvious pattern seen a hundred times elsewhere in the kernel, 
> there's something weird going on with the function. It should be 
> split up, cleaned up, simplified. Variable names could become 
> shorter, etc. etc.

... and this general observation about variable naming and general 
structure holds for the rest of the patches as well. This really 
needs to be sorted out before a more detailed review can be done.

	Ingo

Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Ingo Molnar]

* Borislav Petkov <borislav.petkov@amd.com> wrote:

> From: Doug Thompson <dougthompson@xmission.com>
> 
> Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  drivers/edac/amd64_edac.c |  318 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 318 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index fe2342c..84075c0 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2726,4 +2726,322 @@ static int f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32 ChannelSelect)
>  	return CSFound;
>  }
>  
> +/*
> + * f10_match_to_this_node
> + *
> + * For a given 'DramRange' value, check if 'SystemAddr' fall within this value
> + */
> +static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
> +				u64 SystemAddr,
> +				int *node_id,
> +				int *channel_select)
> +{
> +	int CSFound = -1;
> +	int NodeID;
> +	int HiRangeSelected;
> +	u32 IntlvEn, IntlvSel;
> +	u32 DramEn;
> +	u32 Ilog;
> +	u32 HoleOffset, HoleEn;
> +	u32 InputAddr, Temp;
> +	u32 DctSelBaseAddr, DctSelIntLvAddr;
> +	u32 DctSelHi;
> +	u32 ChannelSelect;
> +	u64 DramBaseLong, DramLimitLong;
> +	u64 DctSelBaseOffsetLong, ChannelAddrLong;
> +
> +	/* DRAM Base value for this DRAM instance */
> +	DramBaseLong = pvt->dram_base[DramRange];
> +	DramEn = pvt->dram_rw_en[DramRange];
> +	IntlvEn = pvt->dram_IntlvEn[DramRange];
> +
> +	/* DRAM Limit value for this DRAM instance */
> +	DramLimitLong = pvt->dram_limit[DramRange];
> +	NodeID = pvt->dram_DstNode[DramRange];
> +	IntlvSel = pvt->dram_IntlvSel[DramRange];
> +
> +	debugf1("%s(dram=%d) Base=0x%llx SystemAddr= 0x%llx Limit=0x%llx\n",
> +		__func__, DramRange, DramBaseLong, SystemAddr, DramLimitLong);
> +
> +	/* This assumes that one node's DHAR is the same as
> +	 * all the other node's DHARs
> +	 */
> +	HoleEn = pvt->dhar;
> +	HoleOffset = (HoleEn & 0x0000FF80);
> +	HoleEn = (HoleEn & 0x00000003);
> +
> +	debugf1("   HoleOffset=0x%x  HoleEn=0x%x IntlvSel=0x%x\n",
> +			HoleOffset, HoleEn, IntlvSel);
> +
> +	if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {
> +
> +		Ilog = f10_map_IntlvEn_to_shift(IntlvEn);
> +
> +		Temp = pvt->dram_ctl_select_low;
> +		DctSelBaseOffsetLong = pvt->dram_ctl_select_high << 16;
> +
> +		DctSelHi = (Temp >> 1) & 1;
> +		DctSelIntLvAddr = dct_sel_interleave_addr(pvt);
> +		DctSelBaseAddr = dct_sel_baseaddr(pvt);
> +
> +		if (dct_high_range_enabled(pvt) &&
> +		   !dct_ganging_enabled(pvt) &&
> +		   ((SystemAddr >> 27) >= (DctSelBaseAddr >> 11)))
> +			HiRangeSelected = 1;
> +		else
> +			HiRangeSelected = 0;
> +
> +		ChannelSelect = f10_determine_channel(pvt, SystemAddr,
> +						HiRangeSelected, IntlvEn);
> +
> +		ChannelAddrLong = f10_determine_base_addr_offset(
> +						SystemAddr,
> +						HiRangeSelected,
> +						DctSelBaseAddr,
> +						DctSelBaseOffsetLong,
> +						HoleEn,
> +						HoleOffset,
> +						DramBaseLong);
> +
> +		/* Remove Node ID (in case of processor interleaving) */
> +		Temp = ChannelAddrLong & 0xFC0;
> +
> +		ChannelAddrLong = ((ChannelAddrLong >> Ilog) &
> +					0xFFFFFFFFF000ULL) | Temp;
> +
> +		/* Remove Channel interleave and hash */
> +		if (dct_interleave_enabled(pvt) &&
> +		   !dct_high_range_enabled(pvt) &&
> +		   !dct_ganging_enabled(pvt)) {
> +			if (DctSelIntLvAddr != 1)
> +				ChannelAddrLong =
> +					(ChannelAddrLong >> 1) &
> +					0xFFFFFFFFFFFFFFC0ULL;
> +			else {
> +				Temp = ChannelAddrLong & 0xFC0;
> +				ChannelAddrLong =
> +					((ChannelAddrLong &
> +					0xFFFFFFFFFFFFC000ULL)
> +					>> 1) | Temp;
> +			}
> +		}
> +
> +		/* Form a normalize InputAddr (Move bits 36:8 down to 28:0
> +		 * which will set it up to match the DCT Base register
> +		 */
> +		InputAddr = ChannelAddrLong >> 8;
> +
> +		debugf1("   (ChannelAddrLong=0x%llx) >> 8 becomes "
> +			"InputAddr=0x%x\n", ChannelAddrLong, InputAddr);
> +
> +		/* Iterate over the DRAM DCTs looking for a
> +		 * match for InputAddr on the selected NodeID
> +		 */
> +		CSFound = f10_lookup_addr_in_dct(InputAddr,
> +						NodeID, ChannelSelect);
> +
> +		if (CSFound >= 0) {
> +			*node_id = NodeID;
> +			*channel_select = ChannelSelect;
> +		}
> +	}
> +
> +	return CSFound;
> +}

this function is probably too large, and also it uses some weird 
hungarian notation coding style. Please dont do that! It's 
completely unacceptable.

this condition:

> +	if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {

could be inverted and an early "return cs_found" could be done - 
saving an indentitation level for most of the above code.

etc. etc.

Please look at the function in a really large xterm, from far away. 
If the shape does not look 'good', and the structure is not an 
obvious pattern seen a hundred times elsewhere in the kernel, 
there's something weird going on with the function. It should be 
split up, cleaned up, simplified. Variable names could become 
shorter, etc. etc.

	Ingo

[PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Borislav Petkov]

From: Doug Thompson <dougthompson@xmission.com>

Signed-off-by: Doug Thompson <dougthompson@xmission.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 drivers/edac/amd64_edac.c |  318 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 318 insertions(+), 0 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index fe2342c..84075c0 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2726,4 +2726,322 @@ static int f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32 ChannelSelect)
 	return CSFound;
 }
 
+/*
+ * f10_match_to_this_node
+ *
+ * For a given 'DramRange' value, check if 'SystemAddr' fall within this value
+ */
+static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
+				u64 SystemAddr,
+				int *node_id,
+				int *channel_select)
+{
+	int CSFound = -1;
+	int NodeID;
+	int HiRangeSelected;
+	u32 IntlvEn, IntlvSel;
+	u32 DramEn;
+	u32 Ilog;
+	u32 HoleOffset, HoleEn;
+	u32 InputAddr, Temp;
+	u32 DctSelBaseAddr, DctSelIntLvAddr;
+	u32 DctSelHi;
+	u32 ChannelSelect;
+	u64 DramBaseLong, DramLimitLong;
+	u64 DctSelBaseOffsetLong, ChannelAddrLong;
+
+	/* DRAM Base value for this DRAM instance */
+	DramBaseLong = pvt->dram_base[DramRange];
+	DramEn = pvt->dram_rw_en[DramRange];
+	IntlvEn = pvt->dram_IntlvEn[DramRange];
+
+	/* DRAM Limit value for this DRAM instance */
+	DramLimitLong = pvt->dram_limit[DramRange];
+	NodeID = pvt->dram_DstNode[DramRange];
+	IntlvSel = pvt->dram_IntlvSel[DramRange];
+
+	debugf1("%s(dram=%d) Base=0x%llx SystemAddr= 0x%llx Limit=0x%llx\n",
+		__func__, DramRange, DramBaseLong, SystemAddr, DramLimitLong);
+
+	/* This assumes that one node's DHAR is the same as
+	 * all the other node's DHARs
+	 */
+	HoleEn = pvt->dhar;
+	HoleOffset = (HoleEn & 0x0000FF80);
+	HoleEn = (HoleEn & 0x00000003);
+
+	debugf1("   HoleOffset=0x%x  HoleEn=0x%x IntlvSel=0x%x\n",
+			HoleOffset, HoleEn, IntlvSel);
+
+	if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {
+
+		Ilog = f10_map_IntlvEn_to_shift(IntlvEn);
+
+		Temp = pvt->dram_ctl_select_low;
+		DctSelBaseOffsetLong = pvt->dram_ctl_select_high << 16;
+
+		DctSelHi = (Temp >> 1) & 1;
+		DctSelIntLvAddr = dct_sel_interleave_addr(pvt);
+		DctSelBaseAddr = dct_sel_baseaddr(pvt);
+
+		if (dct_high_range_enabled(pvt) &&
+		   !dct_ganging_enabled(pvt) &&
+		   ((SystemAddr >> 27) >= (DctSelBaseAddr >> 11)))
+			HiRangeSelected = 1;
+		else
+			HiRangeSelected = 0;
+
+		ChannelSelect = f10_determine_channel(pvt, SystemAddr,
+						HiRangeSelected, IntlvEn);
+
+		ChannelAddrLong = f10_determine_base_addr_offset(
+						SystemAddr,
+						HiRangeSelected,
+						DctSelBaseAddr,
+						DctSelBaseOffsetLong,
+						HoleEn,
+						HoleOffset,
+						DramBaseLong);
+
+		/* Remove Node ID (in case of processor interleaving) */
+		Temp = ChannelAddrLong & 0xFC0;
+
+		ChannelAddrLong = ((ChannelAddrLong >> Ilog) &
+					0xFFFFFFFFF000ULL) | Temp;
+
+		/* Remove Channel interleave and hash */
+		if (dct_interleave_enabled(pvt) &&
+		   !dct_high_range_enabled(pvt) &&
+		   !dct_ganging_enabled(pvt)) {
+			if (DctSelIntLvAddr != 1)
+				ChannelAddrLong =
+					(ChannelAddrLong >> 1) &
+					0xFFFFFFFFFFFFFFC0ULL;
+			else {
+				Temp = ChannelAddrLong & 0xFC0;
+				ChannelAddrLong =
+					((ChannelAddrLong &
+					0xFFFFFFFFFFFFC000ULL)
+					>> 1) | Temp;
+			}
+		}
+
+		/* Form a normalize InputAddr (Move bits 36:8 down to 28:0
+		 * which will set it up to match the DCT Base register
+		 */
+		InputAddr = ChannelAddrLong >> 8;
+
+		debugf1("   (ChannelAddrLong=0x%llx) >> 8 becomes "
+			"InputAddr=0x%x\n", ChannelAddrLong, InputAddr);
+
+		/* Iterate over the DRAM DCTs looking for a
+		 * match for InputAddr on the selected NodeID
+		 */
+		CSFound = f10_lookup_addr_in_dct(InputAddr,
+						NodeID, ChannelSelect);
+
+		if (CSFound >= 0) {
+			*node_id = NodeID;
+			*channel_select = ChannelSelect;
+		}
+	}
+
+	return CSFound;
+}
+
+static int f10_translate_sysaddr_to_CS(struct amd64_pvt *pvt,
+			u64 SysAddr,
+			int *node,
+			int *chanSel)
+{
+	int DramRange;
+	int CSFound = -1;
+	u64 DramBaseLong, DramLimitLong;
+
+	for (DramRange = 0; DramRange < DRAM_REG_COUNT; DramRange++) {
+
+		if (!pvt->dram_rw_en[DramRange])
+			continue;
+
+		DramBaseLong = pvt->dram_base[DramRange];
+		DramLimitLong = pvt->dram_limit[DramRange];
+
+		if ((DramBaseLong <= SysAddr) && (SysAddr <= DramLimitLong)) {
+
+			CSFound = f10_match_to_this_node(pvt,
+							DramRange, SysAddr,
+							node, chanSel);
+			if (CSFound >= 0)
+				break;
+		}
+	}
+	return CSFound;
+}
+
+/*
+ * f10_map_sysaddr_to_csrow
+ *
+ *	This the F10 reference code from AMD to
+ *	map a SystemAddress to NodeID, CSROW, Channel
+ *
+ *	See the Family 10h BKDG (with bug fixes in the code from AMD)
+ *
+ *	The SystemAddress is usually an error address received from the
+ *	hardware error detector.
+ */
+static void f10_map_sysaddr_to_csrow(struct mem_ctl_info *mci,
+					struct amd64_error_info_regs *info,
+					u64 SystemAddress)
+{
+	struct amd64_pvt *pvt = mci->pvt_info;
+	u32 page, offset;
+	unsigned short syndrome;
+	int chan = 0;
+	int node_id;
+	int csrow;
+
+	csrow = f10_translate_sysaddr_to_CS(pvt,
+					SystemAddress,
+					&node_id,
+					&chan);
+
+	if (csrow >= 0) {
+		error_address_to_page_and_offset(SystemAddress, &page, &offset);
+
+		syndrome = EXTRACT_HIGH_SYNDROME(info->nbsl) << 8;
+		syndrome |= EXTRACT_LOW_SYNDROME(info->nbsh);
+
+		/* Is CHIPKILL ON?
+		 * If so, then we can attempt to use the 'syndrome' to isolate
+		 * which channel the error was on
+		 */
+		if (pvt->nbcfg & K8_NBCFG_CHIPKILL)
+			chan = get_channel_from_ecc_syndrome(syndrome);
+
+		if (chan >= 0) {
+			edac_mc_handle_ce(mci, page, offset, syndrome,
+					csrow, chan, EDAC_MOD_STR);
+		} else {
+			/* Channel is not known,
+			 * report all channels on this CSROW as failed.
+			 */
+			for (chan = 0; chan < mci->csrows[csrow].nr_channels;
+								chan++) {
+					edac_mc_handle_ce(mci, page, offset,
+							syndrome,
+							csrow, chan,
+							EDAC_MOD_STR);
+			}
+		}
+
+	} else {
+		edac_mc_handle_ce_no_info(mci, EDAC_MOD_STR);
+	}
+}
+
+/* map_dbam_to_csrow_size
+ *
+ * Input (index) is the DBAM DIMM value (1 of 4) used as an index into a
+ * shift table (revf_quad_ddr2_shift) which starts at 128MB DIMM size.
+ *
+ * Index of 0 indicates an empty DIMM slot, as reported by Hardware on
+ * empty slots.
+ *
+ * Normalize to 128MB by subracting 27 bit shift
+ */
+static int map_dbam_to_csrow_size(int index)
+{
+	int mega_bytes = 0;
+
+	if (index > 0 && index <= DBAM_MAX_VALUE)
+		mega_bytes = ((128 << (revf_quad_ddr2_shift[index]-27)));
+
+	return mega_bytes;
+}
+
+/*
+ * f10_debug_display_dimm_sizes
+ *
+ *	debug routine to display the memory sizes of a DIMM
+ *	(ganged or not) and it CSROWs as well
+ */
+static void f10_debug_display_dimm_sizes(int ctrl,
+					struct amd64_pvt *pvt, int ganged)
+{
+	int dimm;
+	int size0;
+	int size1;
+	u32 dbam;
+	u32 *dcsb;
+
+	debugf1("  dbam%d: 0x%8.08x  CSROW is %s\n", ctrl,
+			ctrl ? pvt->dbam1 : pvt->dbam0,
+			ganged ? "GANGED - dbam1 not used" : "NON-GANGED");
+
+	dbam = ctrl ? pvt->dbam1 : pvt->dbam0;
+	dcsb = ctrl ? pvt->dcsb1 : pvt->dcsb0;
+
+	/* Dump memory sizes for DIMM and its CSROWs */
+	for (dimm = 0; dimm < 4; dimm++) {
+
+		size0 = 0;
+		if (dcsb[dimm*2] & K8_DCSB_CS_ENABLE)
+			size0 = map_dbam_to_csrow_size(DBAM_DIMM(dimm, dbam));
+
+		size1 = 0;
+		if (dcsb[dimm*2 + 1] & K8_DCSB_CS_ENABLE)
+			size1 = map_dbam_to_csrow_size(DBAM_DIMM(dimm, dbam));
+
+		debugf1("     CTRL-%d DIMM-%d=%5dMB   CSROW-%d=%5dMB "
+				"CSROW-%d=%5dMB\n",
+				ctrl,
+				dimm,
+				size0 + size1,
+				dimm * 2,
+				size0,
+				dimm * 2 + 1,
+				size1);
+	}
+}
+
+/*
+ * f10_probe_valid_hardware
+ *
+ * Very early hardware probe on pci_probe thread to determine if this module can
+ * support the hardware.
+ *
+ * Return:
+ *      0 for OK
+ *      1 for error
+ */
+static int f10_probe_valid_hardware(struct amd64_pvt *pvt)
+{
+	int rc = 0;
+
+	/*
+	 * If we are on a DDR3 machine, we don't know yet if
+	 * we support that properly at this time
+	 */
+	if ((pvt->dchr0 & F10_DCHR_Ddr3Mode) ||
+	    (pvt->dchr1 & F10_DCHR_Ddr3Mode)) {
+
+		amd64_printk(KERN_WARNING,
+			"%s() This machine is running with DDR3 memory. "
+			"This is not currently supported. "
+			"DCHR0=0x%x DCHR1=0x%x\n",
+			__func__, pvt->dchr0, pvt->dchr1);
+
+		amd64_printk(KERN_WARNING,
+			"   Contact '%s' module MAINTAINER to help add"
+			" support.\n",
+			EDAC_MOD_STR);
+
+		rc = 1;
+
+	} else {
+		debugf0("%s() DDR2 Memory Installed\n", __func__);
+	}
+
+	return rc;
+}
 
-- 
1.6.2.4